diff --git a/rainfall/blueprint/file_blueprint_test.py b/rainfall/blueprint/file_blueprint_test.py index 897da9b..c589acc 100644 --- a/rainfall/blueprint/file_blueprint_test.py +++ b/rainfall/blueprint/file_blueprint_test.py @@ -26,7 +26,7 @@ def test_delete_file(self, app, releases_user): rv = client.delete(f'/api/v1/file/{str(file_id)}') - assert rv.status == '204 NO CONTENT' + assert rv.status == '204 NO CONTENT', rv.json with app.app_context(): updated_user = db.session.get(User, BASIC_USER_ID) diff --git a/rainfall/blueprint/release.py b/rainfall/blueprint/release.py index 5c79ef3..e8f2a09 100644 --- a/rainfall/blueprint/release.py +++ b/rainfall/blueprint/release.py @@ -4,6 +4,7 @@ import flask +from rainfall import object_storage from rainfall.db import db from rainfall.decorators import with_current_user, with_validated_release from rainfall.models.release import Release @@ -51,15 +52,6 @@ def create_release(user): error=f'A release with the name "{release.name}" already exists'), 400 site.releases.append(release) - cur_release_path = release_path(flask.current_app.config['DATA_DIR'], release) - try: - os.makedirs(cur_release_path) - except FileExistsError: - db.session.rollback() - return flask.jsonify( - status=500, - error='Could not create that release (filesystem error)'), 500 - db.session.add(site) db.session.commit() return '', 204 @@ -132,8 +124,7 @@ def delete_release(release, user): try: path = release_path(flask.current_app.config['DATA_DIR'], release) - client = object_storage.connect(flask.current_app) - object_storage.rmtree(client, 'rainfall-data', path) + object_storage.rmtree(path) except Exception as e: db.session.rollback() return flask.jsonify( diff --git a/rainfall/blueprint/release_test.py b/rainfall/blueprint/release_test.py index be4aebf..b283ec1 100644 --- a/rainfall/blueprint/release_test.py +++ b/rainfall/blueprint/release_test.py @@ -3,6 +3,7 @@ from uuid_extensions import uuid7 +from rainfall import object_storage from rainfall.conftest import BASIC_USER_ID from rainfall.db import db from rainfall.models.artwork import Artwork @@ -25,7 +26,7 @@ def test_create(self, app, site_id): 'name': 'Release Create Test', 'site_id': site_id }}) - assert rv.status == '204 NO CONTENT' + assert rv.status == '204 NO CONTENT', rv.json with app.app_context(): user = db.session.get(User, BASIC_USER_ID) @@ -301,23 +302,26 @@ def test_update_release_name_duplicate(self, mock_release_rename, app, def test_delete_release(self, app, releases_user): with app.app_context(), app.test_client() as client: db.session.add(releases_user) - release = releases_user.sites[0].releases[0] + release = releases_user.sites[0].releases[1] release_id = release.id files = release.files + object_client = object_storage.connect(app) with client.session_transaction() as sess: sess['user_id'] = BASIC_USER_ID rv = client.delete(f'/api/v1/release/{release_id}') - assert rv.status == '204 NO CONTENT' + + assert rv.status == '204 NO CONTENT', rv.json assert release not in db.session for file in files: assert file not in db.session - assert not os.path.exists(file.path) assert release not in releases_user.sites[0].releases + assert not any( + object_client.list_objects(app.config['MINIO_BUCKET'], + prefix=release_path( + app.config['DATA_DIR'], release))) assert db.session.get(Release, release_id) is None - assert release_path(app.config['DATA_DIR'], - release) not in os.listdir(app.config['DATA_DIR']) def test_delete_release_not_exist(self, app, basic_user): with app.app_context(), app.test_client() as client: @@ -327,14 +331,15 @@ def test_delete_release_not_exist(self, app, basic_user): rv = client.delete(f'/api/v1/release/{uuid7()}') assert rv.status == '404 NOT FOUND' - @patch('rainfall.blueprint.release.shutil.rmtree') + @patch('rainfall.blueprint.release.object_storage.rmtree') def test_delete_release_filesystem_error(self, mock_rmtree, app, releases_user): with app.app_context(), app.test_client() as client: db.session.add(releases_user) release = releases_user.sites[0].releases[0] release_id = release.id - mock_rmtree.side_effect = Exception('Filesystem error') + mock_rmtree.side_effect = object_storage.ObjectStorageException( + 'Test error') with client.session_transaction() as sess: sess['user_id'] = BASIC_USER_ID @@ -347,8 +352,3 @@ def test_delete_release_filesystem_error(self, mock_rmtree, app, assert 'error' in rv.json assert db.session.get(Release, release_id) is not None assert release in releases_user.sites[0].releases - - release_dir_name = os.path.basename( - release_path(app.config['DATA_DIR'], release)) - site_data_path = site_path(app.config['DATA_DIR'], release.site) - assert release_dir_name in os.listdir(site_data_path) diff --git a/rainfall/blueprint/site.py b/rainfall/blueprint/site.py index a2f51aa..6c53f25 100644 --- a/rainfall/blueprint/site.py +++ b/rainfall/blueprint/site.py @@ -4,11 +4,11 @@ import flask +from rainfall import object_storage from rainfall.db import db -from rainfall.decorators import with_current_user, with_current_site +from rainfall.decorators import with_current_site, with_current_user from rainfall.models.site import Site -from rainfall import object_storage -from rainfall.site import site_path, rename_site_dir +from rainfall.site import rename_site_dir, site_path site = flask.Blueprint('site', __name__) @@ -67,10 +67,8 @@ def delete_site(site, user): try: path = site_path(flask.current_app.config['DATA_DIR'], site) - client = object_storage.connect(flask.current_app) - object_storage.rmtree(client, 'rainfall-data', path) + object_storage.rmtree(path) except Exception as e: - print(e) db.session.rollback() return flask.jsonify(status=500, error='Could not delete site (filesystem error)'), 500 diff --git a/rainfall/blueprint/site_test.py b/rainfall/blueprint/site_test.py index cfbeccc..9d02a1b 100644 --- a/rainfall/blueprint/site_test.py +++ b/rainfall/blueprint/site_test.py @@ -5,10 +5,11 @@ import pytest from uuid_extensions import uuid7 +from rainfall import object_storage from rainfall.conftest import BASIC_USER_ID from rainfall.db import db -from rainfall.models.user import User from rainfall.models.site import Site +from rainfall.models.user import User from rainfall.site import site_path @@ -178,8 +179,7 @@ def test_delete_site(self, app, sites_user): sess['user_id'] = BASIC_USER_ID rv = client.delete(f'/api/v1/site/{site_id}') - print(rv.json) - assert rv.status == '204 NO CONTENT' + assert rv.status == '204 NO CONTENT', rv.json site = db.session.get(Site, site_id) assert site is None @@ -202,12 +202,13 @@ def test_delete_site_no_site(self, app, sites_user): rv = client.delete(f'/api/v1/site/{uuid7()}') assert rv.status == '404 NOT FOUND' - @patch('rainfall.blueprint.site.shutil.rmtree') + @patch('rainfall.blueprint.site.object_storage.rmtree') def test_delete_site_filesystem_error(self, mock_rmtree, app, sites_user): with app.app_context(), app.test_client() as client: db.session.add(sites_user) site_id = sites_user.sites[0].id + object_client = object_storage.connect(app) with client.session_transaction() as sess: sess['user_id'] = BASIC_USER_ID @@ -218,6 +219,3 @@ def test_delete_site_filesystem_error(self, mock_rmtree, app, sites_user): site = db.session.get(Site, site_id) assert site is not None - site_basename = os.path.basename(site_path(app.config['DATA_DIR'], site)) - assert site_basename in os.listdir( - os.path.join(app.config['DATA_DIR'], str(site.user.id))) diff --git a/rainfall/blueprint/upload.py b/rainfall/blueprint/upload.py index 528d16c..44f7e3b 100644 --- a/rainfall/blueprint/upload.py +++ b/rainfall/blueprint/upload.py @@ -3,13 +3,14 @@ import flask +from rainfall import object_storage from rainfall.db import db from rainfall.decorators import with_current_user, with_validated_release from rainfall.models.artwork import Artwork from rainfall.models.file import File from rainfall.models.release import Release -from rainfall import object_storage -from rainfall.site import delete_file as delete_db_file, release_path, secure_filename +from rainfall.site import delete_file as delete_db_file +from rainfall.site import release_path, secure_filename upload = flask.Blueprint('upload', __name__) @@ -34,23 +35,23 @@ def allowed_file(filename): def write_files(release, claz, *files): - for song in files: - name = secure_filename(song.filename) + for file in files: + name = secure_filename(file.filename) if len(name) > 1024: return flask.jsonify(status=400, error=f'File name {name} is too long'), 400 - file = claz(filename=name) - # Yield so that the calling function can properly save file to db. - yield file + obj = claz(filename=name) + # Yield so that the calling function can properly save metadata to db. + yield obj - # Write the file to the filesystem. + # Write the file to object storage. cur_release_path = release_path(flask.current_app.config['DATA_DIR'], release) object_path = os.path.join(cur_release_path, file.filename) - client = object_storage.connect(flask.current_app) - client.put_object('rainfall-data', object_path, song.stream, - song.content_length, song.content_type) + file_size = file.seek(0, 2) + file.seek(0) # Reset the file pointer to the beginning. + object_storage.put_object(object_path, file, file_size, file.content_type) @upload.route('upload/release//song', methods=['POST']) diff --git a/rainfall/blueprint/upload_test.py b/rainfall/blueprint/upload_test.py index 316e007..3f7ea8b 100644 --- a/rainfall/blueprint/upload_test.py +++ b/rainfall/blueprint/upload_test.py @@ -1,18 +1,28 @@ -from unittest.mock import patch import io import os +import time +from unittest.mock import patch import flask -from rainfall.db import db +from rainfall import object_storage from rainfall.conftest import BASIC_USER_ID +from rainfall.db import db from rainfall.models.artwork import Artwork from rainfall.site import release_path, secure_filename -def assert_file_contents(file_path, contents): - with open(file_path, 'r') as f: - assert f.read() == contents +def assert_file_contents(app, path, contents): + object_client = object_storage.connect(app) + resp = None + try: + resp = object_client.get_object(app.config['MINIO_BUCKET'], path) + if resp: + resp_data = resp.read() + finally: + if resp is not None: + resp.close() + assert resp_data == contents class UploadTest: @@ -32,12 +42,9 @@ def test_upload_single_file(self, app, releases_user): assert rv.status == '204 NO CONTENT', rv.text - song_path = os.path.join(app.config['DATA_DIR'], str(releases_user.id), - secure_filename(site.name), - secure_filename(release.name), + song_path = os.path.join(release_path(app.config['DATA_DIR'], release), secure_filename('song1.wav')) - assert_file_contents(song_path, 'not-actually-a-song') - + assert_file_contents(app, song_path, b'not-actually-a-song') assert len(release.files) == 1 assert release.files[0].filename == 'song1.wav' @@ -60,13 +67,13 @@ def test_upload_multiple_files(self, app, releases_user): assert rv.status == '204 NO CONTENT', rv.text - release_path = os.path.join(app.config['DATA_DIR'], str(releases_user.id), - secure_filename(site.name), - secure_filename(release.name)) - song_1_path = os.path.join(release_path, secure_filename('song1.wav')) - song_2_path = os.path.join(release_path, secure_filename('song2.wav')) - assert_file_contents(song_1_path, 'not-actually-a-song') - assert_file_contents(song_2_path, 'not-actually-a-song-2') + song_release_path = release_path(app.config['DATA_DIR'], release) + song_1_path = os.path.join(song_release_path, + secure_filename('song1.wav')) + song_2_path = os.path.join(song_release_path, + secure_filename('song2.wav')) + assert_file_contents(app, song_1_path, b'not-actually-a-song') + assert_file_contents(app, song_2_path, b'not-actually-a-song-2') assert len(release.files) == 2 assert release.files[0].filename == 'song1.wav' @@ -138,7 +145,7 @@ def test_upload_release_art(self, app, releases_user): release_id = str(release.id) rv = client.post(f'/api/v1/upload/release/{release_id}/art', data={ - 'artwork': (io.BytesIO(b'not-actually-a-song'), + 'artwork': (io.BytesIO(b'not-actually-artwork'), 'some_artwork.jpg') }) @@ -149,7 +156,7 @@ def test_upload_release_art(self, app, releases_user): file_path = os.path.join( release_path(flask.current_app.config['DATA_DIR'], release), 'some_artwork.jpg') - assert os.path.exists(file_path) + assert_file_contents(app, file_path, b'not-actually-artwork') def test_upload_release_art_no_file(self, app, releases_user): with app.app_context(), app.test_client() as client: @@ -208,5 +215,4 @@ def test_upload_release_art_delete_existing(self, app, releases_user, file_path = os.path.join( release_path(flask.current_app.config['DATA_DIR'], release), 'artwork.jpg') - print(file_path) assert not os.path.exists(file_path) diff --git a/rainfall/conftest.py b/rainfall/conftest.py index 8db8381..5d3ca5a 100644 --- a/rainfall/conftest.py +++ b/rainfall/conftest.py @@ -1,15 +1,15 @@ import os import shutil -from unittest.mock import MagicMock, patch import uuid +from unittest.mock import MagicMock, patch import flask import pytest from uuid_extensions import uuid7 -from rainfall.test_constants import TEST_FILE_PATH +from rainfall import object_storage from rainfall.db import db -from rainfall.main import create_app +from rainfall.main import create_app, init_object_storage from rainfall.models.artwork import Artwork from rainfall.models.file import File from rainfall.models.integration import Integration @@ -17,6 +17,7 @@ from rainfall.models.site import Site from rainfall.models.user import User from rainfall.site import release_path, site_path +from rainfall.test_constants import TEST_FILE_PATH, TEST_MINIO_BUCKET BASIC_USER_ID = uuid.UUID('06543f11-12b6-71ea-8000-e026c63c22e2') @@ -34,10 +35,13 @@ def app(): app.mock_oauth = mock_oauth app.mock_remote_app = mock_remote_app + app.config['MINIO_BUCKET'] = TEST_MINIO_BUCKET app.config['SQLALCHEMY_DATABASE_URI'] = os.environ[ 'SQLALCHEMY_TEST_DATABASE_URI'] app.config['TESTING'] = True + db.init_app(app) + init_object_storage(app) with app.app_context(): db.create_all() @@ -46,7 +50,7 @@ def app(): with app.app_context(): db.drop_all() - shutil.rmtree(TEST_FILE_PATH) + object_storage.rmtree(None) @pytest.fixture @@ -93,11 +97,9 @@ def sites_user(app, welcomed_user): site_1 = Site(name='Cool Site 1') welcomed_user.sites.append(site_1) - os.makedirs(site_path(app.config['DATA_DIR'], site_1)) site_2 = Site(name='Another Cool Site') welcomed_user.sites.append(site_2) - os.makedirs(site_path(app.config['DATA_DIR'], site_2)) db.session.add(welcomed_user) db.session.commit() @@ -112,7 +114,6 @@ def releases_user(app, sites_user): release_1 = Release(name='Site 0 Release 1') sites_user.sites[0].releases.append(release_1) - os.makedirs(release_path(app.config['DATA_DIR'], release_1)) release_2 = Release(name='Site 0 Release 2', files=[ @@ -120,11 +121,9 @@ def releases_user(app, sites_user): File(filename='s0_r1_file_1.aiff') ]) sites_user.sites[0].releases.append(release_2) - os.makedirs(release_path(app.config['DATA_DIR'], release_2)) release_3 = Release(name='Site 1 Release 1') sites_user.sites[1].releases.append(release_3) - os.makedirs(release_path(app.config['DATA_DIR'], release_3)) db.session.add(sites_user) db.session.commit() diff --git a/rainfall/login.py b/rainfall/login.py index 3aaf15b..5da8d62 100644 --- a/rainfall/login.py +++ b/rainfall/login.py @@ -6,8 +6,8 @@ from sqlalchemy import select from rainfall.db import db -from rainfall.models.user import User from rainfall.models.mastodon_credential import MastodonCredential +from rainfall.models.user import User log = logging.getLogger(__name__) diff --git a/rainfall/main.py b/rainfall/main.py index 784acef..855be6e 100644 --- a/rainfall/main.py +++ b/rainfall/main.py @@ -1,10 +1,11 @@ import logging import os -from authlib.integrations.flask_client import OAuth import flask +from authlib.integrations.flask_client import OAuth from flask_seasurf import SeaSurf +from rainfall import object_storage from rainfall.blueprint.file import file as file_blueprint from rainfall.blueprint.oauth import OauthBlueprintFactory from rainfall.blueprint.release import release as release_blueprint @@ -13,9 +14,9 @@ from rainfall.blueprint.user import UserBlueprintFactory from rainfall.db import db from rainfall.decorators import with_current_site, with_current_user -from rainfall import object_storage -from rainfall.site import generate_site, generate_zip, public_dir, site_exists, zip_file_path -from rainfall.test_constants import TEST_FILE_PATH +from rainfall.site import (generate_site, generate_zip, public_dir, site_exists, + zip_file_path) +from rainfall.test_constants import TEST_FILE_PATH, TEST_MINIO_BUCKET log = logging.getLogger(__name__) logging.basicConfig(level=logging.WARNING) @@ -35,6 +36,7 @@ def create_app(): app.config['MINIO_ENDPOINT'] = os.environ['MINIO_ENDPOINT'] app.config['MINIO_ACCESS_KEY'] = os.environ['MINIO_ACCESS_KEY'] app.config['MINIO_SECRET_KEY'] = os.environ['MINIO_SECRET_KEY'] + app.config['MINIO_BUCKET'] = os.environ['MINIO_BUCKET'] # Authlib automatically extracts these app.config['NETLIFY_CLIENT_ID'] = os.environ['NETLIFY_CLIENT_ID'] @@ -44,27 +46,13 @@ def create_app(): app.config['RAINFALL_ENV'] = os.environ.get('RAINFALL_ENV', 'development') if app.config['RAINFALL_ENV'] != 'test': db.init_app(app) + init_object_storage(app) else: app.config['TESTING'] = True - app.config['DATA_DIR'] = os.path.join(TEST_FILE_PATH, - app.config['DATA_DIR']) - app.config['PREVIEW_DATA'] = os.path.join(TEST_FILE_PATH, - app.config['DATA_DIR']) + csrf = SeaSurf(app) oauth = OAuth(app) - object_storage.create_bucket_if_not_exists(app.config['MINIO_ENDPOINT'], - app.config['MINIO_ACCESS_KEY'], - app.config['MINIO_SECRET_KEY'], - 'rainfall-preview') - object_storage.create_bucket_if_not_exists(app.config['MINIO_ENDPOINT'], - app.config['MINIO_ACCESS_KEY'], - app.config['MINIO_SECRET_KEY'], - 'rainfall-data') - - os.makedirs(app.config['DATA_DIR'], exist_ok=True) - os.makedirs(app.config['PREVIEW_DIR'], exist_ok=True) - app.register_blueprint(UserBlueprintFactory(csrf).get_blueprint(), url_prefix='/api/v1') app.register_blueprint(OauthBlueprintFactory(oauth).get_blueprint(), @@ -151,3 +139,8 @@ def assets(filename=None): filename) return app + + +def init_object_storage(app): + object_storage.create_bucket_if_not_exists(app) + object_storage.create_bucket_if_not_exists(app) diff --git a/rainfall/object_storage.py b/rainfall/object_storage.py index 05827bb..79ae8a4 100644 --- a/rainfall/object_storage.py +++ b/rainfall/object_storage.py @@ -1,30 +1,71 @@ +import logging +from functools import wraps + +import flask from minio import Minio from minio.deleteobjects import DeleteObject +log = logging.getLogger(__name__) + + +class ObjectStorageException(Exception): + pass + -def connect(app): +class ObjectDeleteException(ObjectStorageException): + pass + + +def connect(app=None): + if app is None: + app = flask.current_app return Minio(app.config['MINIO_ENDPOINT'], access_key=app.config['MINIO_ACCESS_KEY'], secret_key=app.config['MINIO_SECRET_KEY'], secure=False) -def create_bucket_if_not_exists(endpoint_url, access_key, secret_key, name): - client = Minio(endpoint_url, - access_key=access_key, - secret_key=secret_key, - secure=False) - if not client.bucket_exists(name): - client.make_bucket(name) +def inject_client_and_bucket(fn): + + @wraps(fn) + def wrapper(*args, **kwargs): + client = connect() + bucket = flask.current_app.config['MINIO_BUCKET'] + return fn(client, bucket, *args, **kwargs) + return wrapper + +def create_bucket_if_not_exists(app): + client = connect(app) + bucket = app.config['MINIO_BUCKET'] + if not client.bucket_exists(bucket): + client.make_bucket(bucket) + + +@inject_client_and_bucket +def put_object(client, bucket, path, file, content_length, content_type): + ''' + This is a thin wrapper, but is here in case we want to do something + different in the future. + ''' + client.put_object(bucket, path, file, content_length, content_type) + + +@inject_client_and_bucket def rmtree(client, bucket, path): delete_object_list = map( lambda x: DeleteObject(x.object_name), client.list_objects(bucket, path, recursive=True), ) - # DO NOT SUBMIT: Error handling errors = client.remove_objects(bucket, delete_object_list) - for error in errors: - print(error) + num_errors = 0 + for e in errors: + log.error('Error deleting %s: %s', e.object_name, e.error_message) + num_errors += 1 + + if num_errors > 0: + raise ObjectDeleteException( + f'Could not delete all objects from {path}, {num_errors} errors (see log)' + ) diff --git a/rainfall/test_constants.py b/rainfall/test_constants.py index f085911..2c4456a 100644 --- a/rainfall/test_constants.py +++ b/rainfall/test_constants.py @@ -1 +1,2 @@ TEST_FILE_PATH = 'test-data' +TEST_MINIO_BUCKET = 'rainfall-test'