diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 5dd3452c..cca9ddf1 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -27,3 +27,16 @@ _____ This code has been copied over so it is easier to review the Python API implementation in development. * Make code into an installable package. + +[1.2.0] - 2021-01-25 +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Added +_____ + +* Adds API unit tests to improve test coverage. + +Changed +_______ + +* Updates the Python API to use the models directly. diff --git a/Makefile b/Makefile index 3d4a3f38..8bc9691a 100644 --- a/Makefile +++ b/Makefile @@ -85,10 +85,10 @@ static: ## Collect static files ${VENV_BIN}/python manage.py collectstatic --noinput test: clean ## Run tests and generate coverage report - ${VENV_BIN}/coverage run ./manage.py test blockstore --settings=blockstore.settings.test + ${VENV_BIN}/coverage run ${VENV_BIN}/pytest blockstore --ds=blockstore.settings.test ${VENV_BIN}/coverage html ${VENV_BIN}/coverage xml - ${VENV_BIN}/diff-cover coverage.xml --html-report diff-cover.html --compare-branch origin/master + ${VENV_BIN}/diff-cover coverage.xml --html-report diff-cover.html --compare-branch=origin/master easyserver: dev.up dev.provision # Start and provision a Blockstore container and run the server until CTRL-C, then stop it # Now run blockstore until the user hits CTRL-C: diff --git a/blockstore/__init__.py b/blockstore/__init__.py index d15b6c77..f48ad496 100644 --- a/blockstore/__init__.py +++ b/blockstore/__init__.py @@ -2,4 +2,4 @@ Blockstore is a system for storing educational content. """ -__version__ = '1.1.0' +__version__ = '1.2.0' diff --git a/blockstore/apps/api/__init__.py b/blockstore/apps/api/__init__.py index 483e97c2..11a1481a 100644 --- a/blockstore/apps/api/__init__.py +++ b/blockstore/apps/api/__init__.py @@ -5,15 +5,16 @@ openedx.core.djangolib.blockstore_cache) together with these API methods for improved performance. """ -from .models import ( - Collection, - Bundle, - Draft, - BundleFile, - DraftFile, - LinkReference, - LinkDetails, - DraftLinkDetails, +from .data import ( + CollectionData, + BundleData, + BundleVersionData, + DraftData, + BundleFileData, + DraftFileData, + Dependency, + BundleLinkData, + DraftLinkData, ) from .methods import ( # Collections: @@ -51,7 +52,9 @@ BlockstoreException, CollectionNotFound, BundleNotFound, + BundleVersionNotFound, DraftNotFound, + DraftHasNoChangesToCommit, BundleFileNotFound, BundleStorageError, ) diff --git a/blockstore/apps/api/models.py b/blockstore/apps/api/data.py similarity index 59% rename from blockstore/apps/api/models.py rename to blockstore/apps/api/data.py index 8f2127ca..f5cbe9c9 100644 --- a/blockstore/apps/api/models.py +++ b/blockstore/apps/api/data.py @@ -7,28 +7,24 @@ import attr - -def _convert_to_uuid(value): - if not isinstance(value, UUID): - return UUID(value) - return value +from blockstore.apps.bundles.links import convert_to_uuid, Dependency @attr.s(frozen=True) -class Collection: +class CollectionData: """ Metadata about a blockstore collection """ - uuid = attr.ib(type=UUID, converter=_convert_to_uuid) + uuid = attr.ib(type=UUID, converter=convert_to_uuid) title = attr.ib(type=str) @attr.s(frozen=True) -class Bundle: +class BundleData: """ Metadata about a blockstore bundle """ - uuid = attr.ib(type=UUID, converter=_convert_to_uuid) + uuid = attr.ib(type=UUID, converter=convert_to_uuid) title = attr.ib(type=str) description = attr.ib(type=str) slug = attr.ib(type=str) @@ -38,20 +34,34 @@ class Bundle: @attr.s(frozen=True) -class Draft: +class DraftData: """ Metadata about a blockstore draft """ - uuid = attr.ib(type=UUID, converter=_convert_to_uuid) - bundle_uuid = attr.ib(type=UUID, converter=_convert_to_uuid) + uuid = attr.ib(type=UUID, converter=convert_to_uuid) + bundle_uuid = attr.ib(type=UUID, converter=convert_to_uuid) name = attr.ib(type=str) + created_at = attr.ib(type=datetime, validator=attr.validators.instance_of(datetime)) updated_at = attr.ib(type=datetime, validator=attr.validators.instance_of(datetime)) files = attr.ib(type=dict) links = attr.ib(type=dict) @attr.s(frozen=True) -class BundleFile: +class BundleVersionData: + """ + Metadata about a blockstore bundle version. + """ + bundle_uuid = attr.ib(type=UUID, converter=convert_to_uuid) + version = attr.ib(type=int, validator=attr.validators.instance_of(int)) + change_description = attr.ib(type=str) + created_at = attr.ib(type=datetime, validator=attr.validators.instance_of(datetime)) + files = attr.ib(type=dict) + links = attr.ib(type=dict) + + +@attr.s(frozen=True) +class BundleFileData: """ Metadata about a file in a blockstore bundle or draft. """ @@ -62,7 +72,7 @@ class BundleFile: @attr.s(frozen=True) -class DraftFile(BundleFile): +class DraftFileData(BundleFileData): """ Metadata about a file in a blockstore draft. """ @@ -70,27 +80,17 @@ class DraftFile(BundleFile): @attr.s(frozen=True) -class LinkReference: - """ - A pointer to a specific BundleVersion - """ - bundle_uuid = attr.ib(type=UUID, converter=_convert_to_uuid) - version = attr.ib(type=int) - snapshot_digest = attr.ib(type=str) - - -@attr.s(frozen=True) -class LinkDetails: +class BundleLinkData: """ Details about a specific link in a BundleVersion or Draft """ name = attr.ib(type=str) - direct = attr.ib(type=LinkReference) - indirect = attr.ib(type=list) # List of LinkReference objects + direct = attr.ib(type=Dependency) + indirect = attr.ib(type=list) # List of Dependency objects @attr.s(frozen=True) -class DraftLinkDetails(LinkDetails): +class DraftLinkData(BundleLinkData): """ Details about a specific link in a Draft """ diff --git a/blockstore/apps/api/exceptions.py b/blockstore/apps/api/exceptions.py index b58251d3..cc167687 100644 --- a/blockstore/apps/api/exceptions.py +++ b/blockstore/apps/api/exceptions.py @@ -19,10 +19,18 @@ class BundleNotFound(NotFound): pass +class BundleVersionNotFound(NotFound): + pass + + class DraftNotFound(NotFound): pass +class DraftHasNoChangesToCommit(BlockstoreException): + pass + + class BundleFileNotFound(NotFound): pass diff --git a/blockstore/apps/api/methods.py b/blockstore/apps/api/methods.py index a49d2d6d..616aa1da 100644 --- a/blockstore/apps/api/methods.py +++ b/blockstore/apps/api/methods.py @@ -1,180 +1,100 @@ """ API Client methods for working with Blockstore bundles and drafts """ -# The code in this file has been copied over from edx-platform/openedx/core/lib/blockstore_api. -# The following line should be removed when the implementation is being updated as part of -# https://github.com/edx/blockstore/pull/97 -# pylint: skip-file import base64 -from urllib.parse import urlencode -from uuid import UUID - -import dateutil.parser -from django.conf import settings -from django.core.exceptions import ImproperlyConfigured -import requests - -from .models import ( - Bundle, - Collection, - Draft, - BundleFile, - DraftFile, - LinkDetails, - LinkReference, - DraftLinkDetails, +import re +from crum import get_current_request + +from django.db.models import Q +from rest_framework import serializers + +from blockstore.apps.bundles import models +from blockstore.apps.bundles.links import LinkCycleError +from blockstore.apps.bundles.store import DraftRepo, SnapshotRepo +from blockstore.apps.rest_api.v1.serializers.drafts import ( + DraftFileUpdateSerializer, +) + +from .data import ( + BundleData, + BundleVersionData, + CollectionData, + DraftData, + BundleFileData, + DraftFileData, + BundleLinkData, + DraftLinkData, ) from .exceptions import ( - NotFound, CollectionNotFound, BundleNotFound, + BundleVersionNotFound, DraftNotFound, + DraftHasNoChangesToCommit, BundleFileNotFound, ) -def api_url(*path_parts): - if not settings.BLOCKSTORE_API_URL or not settings.BLOCKSTORE_API_URL.endswith('/api/v1/'): - raise ImproperlyConfigured('BLOCKSTORE_API_URL must be set and should end with /api/v1/') - return settings.BLOCKSTORE_API_URL + '/'.join(path_parts) - - -def api_request(method, url, **kwargs): - """ - Helper method for making a request to the Blockstore REST API - """ - if not settings.BLOCKSTORE_API_AUTH_TOKEN: - raise ImproperlyConfigured("Cannot use Blockstore unless BLOCKSTORE_API_AUTH_TOKEN is set.") - kwargs.setdefault('headers', {})['Authorization'] = f"Token {settings.BLOCKSTORE_API_AUTH_TOKEN}" - response = requests.request(method, url, **kwargs) - if response.status_code == 404: - raise NotFound - response.raise_for_status() - if response.status_code == 204: - return None # No content - return response.json() - - -def _collection_from_response(data): - """ - Given data about a Collection returned by any blockstore REST API, convert it to - a Collection instance. - """ - return Collection(uuid=UUID(data['uuid']), title=data['title']) - - -def _bundle_from_response(data): - """ - Given data about a Bundle returned by any blockstore REST API, convert it to - a Bundle instance. - """ - return Bundle( - uuid=UUID(data['uuid']), - title=data['title'], - description=data['description'], - slug=data['slug'], - # drafts: Convert from a dict of URLs to a dict of UUIDs: - drafts={draft_name: UUID(url.split('/')[-1]) for (draft_name, url) in data['drafts'].items()}, - # versions field: take the last one and convert it from URL to an int - # i.e.: [..., 'https://blockstore/api/v1/bundle_versions/bundle_uuid,15'] -> 15 - latest_version=int(data['versions'][-1].split(',')[-1]) if data['versions'] else 0, - ) - - -def _draft_from_response(data): - """ - Given data about a Draft returned by any blockstore REST API, convert it to - a Draft instance. - """ - return Draft( - uuid=UUID(data['uuid']), - bundle_uuid=UUID(data['bundle_uuid']), - name=data['name'], - updated_at=dateutil.parser.parse(data['staged_draft']['updated_at']), - files={ - path: DraftFile(path=path, **file) - for path, file in data['staged_draft']['files'].items() - }, - links={ - name: DraftLinkDetails( - name=name, - direct=LinkReference(**link["direct"]), - indirect=[LinkReference(**ind) for ind in link["indirect"]], - modified=link["modified"], - ) - for name, link in data['staged_draft']['links'].items() - } - ) - - def get_collection(collection_uuid): """ - Retrieve metadata about the specified collection + Retrieve metadata about the specified collection. - Raises CollectionNotFound if the collection does not exist + Raises CollectionNotFound if collection with UUID does not exist. """ - assert isinstance(collection_uuid, UUID) - try: - data = api_request('get', api_url('collections', str(collection_uuid))) - except NotFound: - raise CollectionNotFound(f"Collection {collection_uuid} does not exist.") - return _collection_from_response(data) + collection_model = _get_collection_model(collection_uuid) + return _collection_data_from_model(collection_model) def create_collection(title): """ Create a new collection. """ - result = api_request('post', api_url('collections'), json={"title": title}) - return _collection_from_response(result) + collection_model = models.Collection(title=title) + collection_model.save() + return _collection_data_from_model(collection_model) def update_collection(collection_uuid, title): """ - Update a collection's title + Update a collection's title. """ - assert isinstance(collection_uuid, UUID) - data = {"title": title} - result = api_request('patch', api_url('collections', str(collection_uuid)), json=data) - return _collection_from_response(result) + collection_model = _get_collection_model(collection_uuid) + collection_model.title = title + collection_model.save() + return _collection_data_from_model(collection_model) def delete_collection(collection_uuid): """ - Delete a collection + Delete a collection. """ - assert isinstance(collection_uuid, UUID) - api_request('delete', api_url('collections', str(collection_uuid))) + collection_model = _get_collection_model(collection_uuid) + collection_model.delete() def get_bundles(uuids=None, text_search=None): """ - Get the details of all bundles + Get the details of all bundles. """ - query_params = {} + bundles_queryset = _bundle_queryset() if uuids: - query_params['uuid'] = ','.join(map(str, uuids)) + bundles_queryset = bundles_queryset.filter(uuid__in=uuids) if text_search: - query_params['text_search'] = text_search - version_url = api_url('bundles') + '?' + urlencode(query_params) - response = api_request('get', version_url) - # build bundle from response, convert map object to list and return - return [_bundle_from_response(item) for item in response] + bundles_queryset = bundles_queryset.filter( + Q(title__icontains=text_search) | Q(description__icontains=text_search) | Q(slug__icontains=text_search) + ) + return [_bundle_data_from_model(bundle_model) for bundle_model in bundles_queryset] def get_bundle(bundle_uuid): """ - Retrieve metadata about the specified bundle + Retrieve metadata about the specified bundle. - Raises BundleNotFound if the bundle does not exist + Raises BundleNotFound if bundle with UUID does not exist. """ - assert isinstance(bundle_uuid, UUID) - try: - data = api_request('get', api_url('bundles', str(bundle_uuid))) - except NotFound: - raise BundleNotFound(f"Bundle {bundle_uuid} does not exist.") - return _bundle_from_response(data) + bundle_model = _get_bundle_model(bundle_uuid) + return _bundle_data_from_model(bundle_model) def create_bundle(collection_uuid, slug, title="New Bundle", description=""): @@ -183,40 +103,42 @@ def create_bundle(collection_uuid, slug, title="New Bundle", description=""): Note that description is currently required. """ - result = api_request('post', api_url('bundles'), json={ - "collection_uuid": str(collection_uuid), - "slug": slug, - "title": title, - "description": description, - }) - return _bundle_from_response(result) + collection_model = _get_collection_model(collection_uuid) + bundle_model = models.Bundle( + title=title, + collection=collection_model, + slug=slug, + description=description, + ) + bundle_model.save() + return _bundle_data_from_model(bundle_model) def update_bundle(bundle_uuid, **fields): """ Update a bundle's title, description, slug, or collection. """ - assert isinstance(bundle_uuid, UUID) - data = {} - # Most validation will be done by Blockstore, so we don't worry too much about data validation + bundle_model = _get_bundle_model(bundle_uuid) for str_field in ("title", "description", "slug"): if str_field in fields: - data[str_field] = fields.pop(str_field) + setattr(bundle_model, str_field, fields.pop(str_field)) if "collection_uuid" in fields: - data["collection_uuid"] = str(fields.pop("collection_uuid")) + collection_uuid = fields.pop("collection_uuid") + collection_model = _get_collection_model(collection_uuid) + bundle_model.collection = collection_model if fields: - raise ValueError(f"Unexpected extra fields passed " # pylint: disable=dict-keys-not-iterating - f"to update_bundle: {fields.keys()}") - result = api_request('patch', api_url('bundles', str(bundle_uuid)), json=data) - return _bundle_from_response(result) + raise ValueError("Unexpected extra fields passed to update_bundle: {}".format(fields.keys())) + + bundle_model.save() + return _bundle_data_from_model(bundle_model) def delete_bundle(bundle_uuid): """ - Delete a bundle + Delete a bundle. """ - assert isinstance(bundle_uuid, UUID) - api_request('delete', api_url('bundles', str(bundle_uuid))) + bundle_model = _get_bundle_model(bundle_uuid) + bundle_model.delete() def get_draft(draft_uuid): @@ -224,29 +146,24 @@ def get_draft(draft_uuid): Retrieve metadata about the specified draft. If you don't know the draft's UUID, look it up using get_bundle() """ - assert isinstance(draft_uuid, UUID) - try: - data = api_request('get', api_url('drafts', str(draft_uuid))) - except NotFound: - raise DraftNotFound(f"Draft does not exist: {draft_uuid}") # lint-amnesty, pylint: disable=raise-missing-from - return _draft_from_response(data) + draft_model = _get_draft_model(draft_uuid) + return _draft_data_from_model(draft_model) def get_or_create_bundle_draft(bundle_uuid, draft_name): """ - Retrieve metadata about the specified draft. + Retrieve metadata about the specified draft, creating a new one if it does not exist yet. """ - bundle = get_bundle(bundle_uuid) try: - return get_draft(bundle.drafts[draft_name]) # pylint: disable=unsubscriptable-object - except KeyError: - # The draft doesn't exist yet, so create it: - response = api_request('post', api_url('drafts'), json={ - "bundle_uuid": str(bundle_uuid), - "name": draft_name, - }) - # The result of creating a draft doesn't include all the fields we want, so retrieve it now: - return get_draft(UUID(response["uuid"])) + draft_model = _draft_queryset().get(bundle__uuid=bundle_uuid, name=draft_name) + except models.Draft.DoesNotExist: + bundle_model = _get_bundle_model(bundle_uuid) + draft_model = models.Draft( + bundle=bundle_model, + name=draft_name, + ) + draft_model.save() + return _draft_data_from_model(draft_model) def commit_draft(draft_uuid): @@ -256,7 +173,16 @@ def commit_draft(draft_uuid): Does not return any value. """ - api_request('post', api_url('drafts', str(draft_uuid), 'commit')) + draft_repo = DraftRepo(SnapshotRepo()) + staged_draft = draft_repo.get(draft_uuid) + + if not staged_draft.files_to_overwrite and not staged_draft.links_to_overwrite: + raise DraftHasNoChangesToCommit("Draft {} does not have any changes to commit.".format(draft_uuid)) + + new_snapshot, _updated_draft = draft_repo.commit(staged_draft) + models.BundleVersion.create_new_version( + new_snapshot.bundle_uuid, new_snapshot.hash_digest + ) def delete_draft(draft_uuid): @@ -265,17 +191,20 @@ def delete_draft(draft_uuid): Does not return any value. """ - api_request('delete', api_url('drafts', str(draft_uuid))) + draft_model = _get_draft_model(draft_uuid) + draft_repo = DraftRepo(SnapshotRepo()) + draft_repo.delete(draft_uuid) + draft_model.delete() -def get_bundle_version(bundle_uuid, version_number): +def get_bundle_version(bundle_uuid, version_number=None): """ Get the details of the specified bundle version """ - if version_number == 0: + bundle_version_model = _get_bundle_version_model(bundle_uuid, version_number) + if bundle_version_model is None: return None - version_url = api_url('bundle_versions', str(bundle_uuid) + ',' + str(version_number)) - return api_request('get', version_url) + return _bundle_version_data_from_model(bundle_version_model) def get_bundle_version_files(bundle_uuid, version_number): @@ -283,9 +212,10 @@ def get_bundle_version_files(bundle_uuid, version_number): Get a list of the files in the specified bundle version """ if version_number == 0: + # There are no files in the initial version of a bundle return [] - version_info = get_bundle_version(bundle_uuid, version_number) - return [BundleFile(path=path, **file_metadata) for path, file_metadata in version_info["snapshot"]["files"].items()] + bundle_version = get_bundle_version(bundle_uuid, version_number) + return list(bundle_version.files.values() if bundle_version else []) def get_bundle_version_links(bundle_uuid, version_number): @@ -293,34 +223,29 @@ def get_bundle_version_links(bundle_uuid, version_number): Get a dictionary of the links in the specified bundle version """ if version_number == 0: + # There are no links in the initial version of a bundle return {} - version_info = get_bundle_version(bundle_uuid, version_number) - return { - name: LinkDetails( - name=name, - direct=LinkReference(**link["direct"]), - indirect=[LinkReference(**ind) for ind in link["indirect"]], - ) - for name, link in version_info['snapshot']['links'].items() - } + bundle_version = get_bundle_version(bundle_uuid, version_number) + return bundle_version.links if bundle_version else {} def get_bundle_files_dict(bundle_uuid, use_draft=None): """ - Get a dict of all the files in the specified bundle. + Get a dict of all the files in the specified bundle or draft. Returns a dict where the keys are the paths (strings) and the values are - BundleFile or DraftFile tuples. - """ - bundle = get_bundle(bundle_uuid) - if use_draft and use_draft in bundle.drafts: # pylint: disable=unsupported-membership-test - draft_uuid = bundle.drafts[use_draft] # pylint: disable=unsubscriptable-object - return get_draft(draft_uuid).files - elif not bundle.latest_version: - # This bundle has no versions so definitely does not contain any files - return {} - else: - return {file_meta.path: file_meta for file_meta in get_bundle_version_files(bundle_uuid, bundle.latest_version)} + BundleFileData or DraftFileData tuples. + """ + if use_draft: + try: + draft_model = _draft_queryset().get(bundle__uuid=bundle_uuid, name=use_draft) + except models.Draft.DoesNotExist: + pass + else: + return _draft_data_from_model(draft_model).files + + bundle_version = get_bundle_version(bundle_uuid) + return bundle_version.files if bundle_version else {} def get_bundle_files(bundle_uuid, use_draft=None): @@ -337,29 +262,29 @@ def get_bundle_links(bundle_uuid, use_draft=None): Returns a dict where the keys are the link names (strings) and the values are LinkDetails or DraftLinkDetails tuples. """ - bundle = get_bundle(bundle_uuid) - if use_draft and use_draft in bundle.drafts: # pylint: disable=unsupported-membership-test - draft_uuid = bundle.drafts[use_draft] # pylint: disable=unsubscriptable-object - return get_draft(draft_uuid).links - elif not bundle.latest_version: - # This bundle has no versions so definitely does not contain any links - return {} - else: - return get_bundle_version_links(bundle_uuid, bundle.latest_version) + if use_draft: + try: + draft_model = _draft_queryset().get(bundle__uuid=bundle_uuid, name=use_draft) + except models.Draft.DoesNotExist: + pass + else: + return _draft_data_from_model(draft_model).links + + bundle_version = get_bundle_version(bundle_uuid) + return get_bundle_version(bundle_uuid).links if bundle_version else {} def get_bundle_file_metadata(bundle_uuid, path, use_draft=None): """ Get the metadata of the specified file. """ - assert isinstance(bundle_uuid, UUID) files_dict = get_bundle_files_dict(bundle_uuid, use_draft=use_draft) try: return files_dict[path] - except KeyError: - raise BundleFileNotFound( # lint-amnesty, pylint: disable=raise-missing-from - f"Bundle {bundle_uuid} (draft: {use_draft}) does not contain a file {path}" - ) + except KeyError as exc: + raise BundleFileNotFound( + "Bundle {} (draft: {}) does not contain a file {}".format(bundle_uuid, use_draft, path) + ) from exc def get_bundle_file_data(bundle_uuid, path, use_draft=None): @@ -369,9 +294,24 @@ def get_bundle_file_data(bundle_uuid, path, use_draft=None): Do not use this for large files! """ - metadata = get_bundle_file_metadata(bundle_uuid, path, use_draft) - with requests.get(metadata.url, stream=True) as r: - return r.content + + if use_draft: + try: + draft_model = _draft_queryset().get(bundle__uuid=bundle_uuid, name=use_draft) + except models.Draft.DoesNotExist: + pass + else: + draft_repo = DraftRepo(SnapshotRepo()) + staged_draft = draft_model.staged_draft + with draft_repo.open(staged_draft, path) as file: + return file.read() + + bundle_version_model = _get_bundle_version_model(bundle_uuid, 0) + + snapshot_repo = SnapshotRepo() + snapshot = bundle_version_model.snapshot() + with snapshot_repo.open(snapshot, path) as file: + return file.read() def write_draft_file(draft_uuid, path, contents): @@ -384,11 +324,21 @@ def write_draft_file(draft_uuid, path, contents): Does not return anything. """ - api_request('patch', api_url('drafts', str(draft_uuid)), json={ + data = { 'files': { - path: encode_str_for_draft(contents) if contents is not None else None, + path: _encode_str_for_draft(contents) if contents is not None else None, }, - }) + } + serializer = DraftFileUpdateSerializer(data=data) + serializer.is_valid(raise_exception=True) + files_to_write = serializer.validated_data['files'] + dependencies_to_write = serializer.validated_data['links'] + + draft_repo = DraftRepo(SnapshotRepo()) + try: + draft_repo.update(draft_uuid, files_to_write, dependencies_to_write) + except LinkCycleError as exc: + raise serializers.ValidationError("Link cycle detected: Cannot create draft.") from exc def set_draft_link(draft_uuid, link_name, bundle_uuid, version): @@ -402,28 +352,29 @@ def set_draft_link(draft_uuid, link_name, bundle_uuid, version): Does not return anything. """ - api_request('patch', api_url('drafts', str(draft_uuid)), json={ + data = { 'links': { link_name: {"bundle_uuid": str(bundle_uuid), "version": version} if bundle_uuid is not None else None, }, - }) + } + serializer = DraftFileUpdateSerializer(data=data) + serializer.is_valid(raise_exception=True) + files_to_write = serializer.validated_data['files'] + dependencies_to_write = serializer.validated_data['links'] + draft_repo = DraftRepo(SnapshotRepo()) + try: + draft_repo.update(draft_uuid, files_to_write, dependencies_to_write) + except LinkCycleError as exc: + raise serializers.ValidationError("Link cycle detected: Cannot create draft.") from exc -def encode_str_for_draft(input_str): - """ - Given a string, return UTF-8 representation that is then base64 encoded. - """ - if isinstance(input_str, str): - binary = input_str.encode('utf8') - else: - binary = input_str - return base64.b64encode(binary) + +REGEX_BROWSER_URL = re.compile(r'http://edx.devstack.(studio|lms):') def force_browser_url(blockstore_file_url): """ - Ensure that the given URL Blockstore is a URL accessible from the end user's - browser. + Ensure that the given devstack URL is a URL accessible from the end user's browser. """ # Hack: on some devstacks, we must necessarily use different URLs for # accessing Blockstore file data from within and outside of docker @@ -434,4 +385,197 @@ def force_browser_url(blockstore_file_url): # read by edxapp. # In production, the same S3 URLs get used for internal and external access # so this hack is not necessary. - return blockstore_file_url.replace('http://edx.devstack.blockstore:', 'http://localhost:') + return re.sub(REGEX_BROWSER_URL, 'http://localhost:', blockstore_file_url) + + +def _encode_str_for_draft(input_str): + """ + Given a string, return UTF-8 representation that is then base64 encoded. + """ + if isinstance(input_str, str): + binary = input_str.encode('utf8') + else: + binary = input_str + return base64.b64encode(binary) + + +def _get_collection_model(collection_uuid): + """ + Get collection model from UUID. + + Raises CollectionNotFound if the collection does not exist. + """ + try: + collection_model = models.Collection.objects.get(uuid=collection_uuid) + except models.Collection.DoesNotExist as exc: + raise CollectionNotFound("Collection {} does not exist.".format(collection_uuid)) from exc + return collection_model + + +def _collection_data_from_model(collection_model): + """ + Create and return CollectionData from collection model. + """ + return CollectionData(uuid=collection_model.uuid, title=collection_model.title) + + +def _bundle_queryset(): + """ + Returns the bundle model queryset. + + Prefetch the data needed to create BundleData objects. + """ + return models.Bundle.objects.prefetch_related('drafts', 'versions') + + +def _get_bundle_model(bundle_uuid): + """ + Get Bundle model from UUID. + + Raises BundleNotFound if bundle with UUID does not exist. + """ + try: + bundle_model = _bundle_queryset().get(uuid=bundle_uuid) + except models.Bundle.DoesNotExist as exc: + raise BundleNotFound("Bundle {} does not exist.".format(bundle_uuid)) from exc + return bundle_model + + +def _bundle_data_from_model(bundle_model): + """ + Create and return BundleData from bundle model. + """ + latest_version = bundle_model.versions.order_by('-version_num').first() + latest_version_num = latest_version.version_num if latest_version else 0 + + return BundleData( + uuid=bundle_model.uuid, + title=bundle_model.title, + description=bundle_model.description, + slug=bundle_model.slug, + drafts={draft.name: draft.uuid for draft in bundle_model.drafts.all()}, + latest_version=latest_version_num, + ) + + +def _draft_queryset(): + """ + Returns the draft model queryset. + + Prefetch the data needed to create DraftData objects. + """ + return models.Draft.objects.select_related('bundle') + + +def _get_draft_model(draft_uuid): + """ + Get Draft model from UUID. + + Raises DraftNotFound if draft with UUID does not exist. + """ + try: + draft_model = _draft_queryset().get(uuid=draft_uuid) + except models.Draft.DoesNotExist as exc: + raise DraftNotFound("Draft {} does not exist.".format(draft_uuid)) from exc + return draft_model + + +def _build_absolute_uri(url): + """ + Build an absolute URI from the given url, using the CRUM middleware's stored request. + """ + request = get_current_request() + return request.build_absolute_uri(url) + + +def _draft_data_from_model(draft_model): + """ + Create and return DraftData from draft model. + """ + draft_repo = DraftRepo(SnapshotRepo()) + staged_draft = draft_model.staged_draft + + return DraftData( + uuid=draft_model.uuid, + bundle_uuid=draft_model.bundle.uuid, + name=draft_model.name, + created_at=draft_model.staged_draft.created_at, + updated_at=draft_model.staged_draft.updated_at, + files={ + path: DraftFileData( + path=path, + size=file_info.size, + url=_build_absolute_uri(draft_repo.url(staged_draft, path)), + hash_digest=file_info.hash_digest, + modified=path in draft_model.staged_draft.files_to_overwrite, + ) + for path, file_info in staged_draft.files.items() + }, + links={ + link.name: DraftLinkData( + name=link.name, + direct=link.direct_dependency, + indirect=link.indirect_dependencies, + modified=link.name in staged_draft.links_to_overwrite.modified_set, + ) + for link in staged_draft.composed_links() + } + ) + + +def _bundle_version_queryset(): + """ + Returns the bundle version model queryset. + + Prefetch the data needed to create BundleVersionData objects. + """ + return models.BundleVersion.objects.select_related('bundle') + + +def _get_bundle_version_model(bundle_uuid, version_number=None): + """ + Get BundleVersion from bundle UUID and version number. + + If version_number is None, returns the latest bundle version of the bundle. + """ + filter_kwargs = { + 'bundle__uuid': bundle_uuid + } + if version_number: + filter_kwargs['version_num'] = version_number + + bundle_version_model = _bundle_version_queryset().filter(**filter_kwargs).order_by('-version_num').first() + if version_number and bundle_version_model is None: + raise BundleVersionNotFound("Bundle Version {},{} does not exist.".format(bundle_uuid, version_number)) + return bundle_version_model + + +def _bundle_version_data_from_model(bundle_version_model): + """ + Create and return BundleVersionData from bundle version model. + """ + snapshot = bundle_version_model.snapshot() + snapshot_repo = SnapshotRepo() + + return BundleVersionData( + bundle_uuid=bundle_version_model.bundle.uuid, + version=bundle_version_model.version_num, + change_description=bundle_version_model.change_description, + created_at=snapshot.created_at, + files={ + path: BundleFileData( + path=path, + url=_build_absolute_uri(snapshot_repo.url(snapshot, path)), + size=file_info.size, + hash_digest=file_info.hash_digest.hex(), + ) for path, file_info in snapshot.files.items() + }, + links={ + link.name: BundleLinkData( + name=link.name, + direct=link.direct_dependency, + indirect=link.indirect_dependencies, + ) + for link in snapshot.links + }, + ) diff --git a/blockstore/apps/api/tests/test_blockstore_api.py b/blockstore/apps/api/tests/test_blockstore_api.py index e512b796..b6d98ace 100644 --- a/blockstore/apps/api/tests/test_blockstore_api.py +++ b/blockstore/apps/api/tests/test_blockstore_api.py @@ -1,16 +1,11 @@ """ -Tests for xblock_utils.py +Tests for the api. """ -# The code in this file has been copied over from edx-platform/openedx/core/lib/blockstore_api. -# The following line should be removed when the implementation is being updated as part of -# https://github.com/edx/blockstore/pull/97 -# pylint: skip-file - import unittest from uuid import UUID - +import crum +from django.test.client import RequestFactory import pytest -from django.conf import settings from blockstore.apps import api @@ -18,15 +13,17 @@ BAD_UUID = UUID('12345678-0000-0000-0000-000000000000') -@unittest.skip("Skip until the Python API has been implemented.") +@pytest.mark.django_db class BlockstoreApiClientTest(unittest.TestCase): """ Test for the Blockstore API Client. - - The goal of these tests is not to test that Blockstore works correctly, but - that the API client can interact with it and all the API client methods - work. """ + def setUp(self): + super().setUp() + + # Mock the current request, so that file URLs can be absolute. + request = RequestFactory().get('/') + crum.set_current_request(request) # Collections @@ -63,9 +60,20 @@ def test_nonexistent_bundle(self): with pytest.raises(api.BundleNotFound): api.get_bundle(BAD_UUID) + def test_nonexistent_bundle_version(self): + """ + Request a bundle version that doesn't exist. + """ + # If you don't pass a version number, then it just returns None. + assert api.get_bundle_version(BAD_UUID) is None + # But if you do want a specific version, it 404s + with pytest.raises(api.BundleVersionNotFound): + api.get_bundle_version(BAD_UUID, '1') + def test_bundle_crud(self): """ Create, Fetch, Update, and Delete a Bundle """ coll = api.create_collection("Test Collection") + coll2 = api.create_collection("Test Collection 2") args = { "title": "Water 💧 Bundle", "slug": "h2o", @@ -81,7 +89,7 @@ def test_bundle_crud(self): assert bundle == bundle2 # Update: new_description = "Water Nation Bending Lessons" - bundle3 = api.update_bundle(bundle.uuid, description=new_description) + bundle3 = api.update_bundle(bundle.uuid, description=new_description, collection_uuid=coll2.uuid) assert bundle3.description == new_description bundle4 = api.get_bundle(bundle.uuid) assert bundle4.description == new_description @@ -90,6 +98,47 @@ def test_bundle_crud(self): with pytest.raises(api.BundleNotFound): api.get_bundle(bundle.uuid) + def test_get_bundles(self): + """ Fetch multiple bundles, and filter by text """ + coll = api.create_collection("Test Collection") + water_bundle = api.create_bundle( + coll.uuid, + title="Water 💧 Bundle", + slug="h2o", + description="Sploosh", + ) + air_bundle = api.create_bundle( + coll.uuid, + title="Air 🌀 Bundle", + slug="no2", + description="Whoosh", + ) + fire_bundle = api.create_bundle( + coll.uuid, + title="Fire 🔥 Bundle", + slug="burn", + description="Crackle", + ) + + # Fetch multiple, and limit using text match + bundles = api.get_bundles([water_bundle.uuid, air_bundle.uuid, fire_bundle.uuid], text_search='oosh') + assert len(bundles) == 2 + assert air_bundle in bundles + assert water_bundle in bundles + assert fire_bundle not in bundles + + def test_update_bundle_invalid_fields(self): + """ Updating bundle with unexpected fields -> ValueError """ + coll = api.create_collection("Test Collection") + bundle = api.create_bundle( + coll.uuid, + title="Water 💧 Bundle", + slug="h2o", + description="Sploosh", + ) + with pytest.raises(ValueError): + api.update_bundle(bundle.uuid, invalid_field='Some invalid field value') + # Drafts, files, and reading/writing file contents: def test_nonexistent_draft(self): @@ -105,12 +154,12 @@ def test_drafts_and_files(self): coll = api.create_collection("Test Collection") bundle = api.create_bundle(coll.uuid, title="Earth 🗿 Bundle", slug="earth", description="another test bundle") # Create a draft - draft = api.get_or_create_draft(bundle.uuid, draft_name="test-draft") + draft = api.get_or_create_bundle_draft(bundle.uuid, draft_name="test-draft") assert draft.bundle_uuid == bundle.uuid assert draft.name == 'test-draft' assert draft.updated_at.year >= 2019 # And retrieve it again: - draft2 = api.get_or_create_draft(bundle.uuid, draft_name="test-draft") + draft2 = api.get_or_create_bundle_draft(bundle.uuid, draft_name="test-draft") assert draft == draft2 # Also test retrieving using get_draft draft3 = api.get_draft(draft.uuid) @@ -129,6 +178,14 @@ def test_drafts_and_files(self): assert published_contents == b'initial version' draft_contents2 = api.get_bundle_file_data(bundle.uuid, "test.txt", use_draft=draft.name) assert draft_contents2 == b'modified version' + + file_info2 = api.get_bundle_file_metadata(bundle.uuid, "test.txt", use_draft=draft.name) + assert file_info2.url.startswith('http://testserver') + assert file_info2.path == 'test.txt' + assert file_info2.size == len(b'modified version') + assert file_info2.hash_digest == b'oM\xac\xfcD\xd2F\x11\xd2\xa7;\xff\x88\x8eS\x12\xc6\xe3\xfb\\' + assert api.get_bundle_files_dict(bundle.uuid, use_draft=draft.name) == {'test.txt': file_info2} + # Now delete the draft: api.delete_draft(draft.uuid) draft_contents3 = api.get_bundle_file_data(bundle.uuid, "test.txt", use_draft=draft.name) @@ -137,12 +194,63 @@ def test_drafts_and_files(self): # Finaly, test the get_bundle_file* methods: file_info1 = api.get_bundle_file_metadata(bundle.uuid, "test.txt") + assert file_info1.url.startswith('http://testserver') assert file_info1.path == 'test.txt' assert file_info1.size == len(b'initial version') assert file_info1.hash_digest == 'a45a5c6716276a66c4005534a51453ab16ea63c4' assert list(api.get_bundle_files(bundle.uuid)) == [file_info1] assert api.get_bundle_files_dict(bundle.uuid) == {'test.txt': file_info1} + assert api.get_bundle_files_dict(bundle.uuid, use_draft=draft.name) == {'test.txt': file_info1} + + with pytest.raises(api.BundleFileNotFound): + api.get_bundle_file_metadata(bundle.uuid, "nonexistent.txt") + + def test_bundle_version(self): + """ + Test creating, reading, and writing bundle versions and files + """ + coll = api.create_collection("Test Collection") + bundle = api.create_bundle( + coll.uuid, + title="Water 💧 Bundle", + slug="h2o", + description="Sploosh", + ) + + no_files = api.get_bundle_version_files(bundle.uuid, 0) + assert not no_files + + # Create and commit a draft + draft = api.get_or_create_bundle_draft(bundle.uuid, draft_name="test-draft") + api.commit_draft(draft.uuid) + + # Fetch latest bundle version + latest_bundle_version = api.get_bundle_version(bundle.uuid) + bundle_version = api.get_bundle_version(bundle.uuid, 1) + assert bundle_version == latest_bundle_version + assert bundle_version.bundle_uuid == bundle.uuid + assert bundle_version.version == 1 + assert bundle_version.files == {} + assert bundle_version.links == {} + + # Unavailable version throws error + with pytest.raises(api.BundleVersionNotFound): + api.get_bundle_version(bundle.uuid, 2) + + # Add file to a new draft + draft2 = api.get_or_create_bundle_draft(bundle.uuid, draft_name="test-draft-files") + api.write_draft_file(draft2.uuid, "test.txt", b"initial version") + api.commit_draft(draft2.uuid) + + # Fetch this bundle version's files + files = api.get_bundle_version_files(bundle.uuid, 2) + assert len(files) == 1 + for bundle_file in files: + assert bundle_file.url.startswith('http://testserver') + assert bundle_file.path == 'test.txt' + assert bundle_file.size == len(b'initial version') + assert bundle_file.hash_digest == 'a45a5c6716276a66c4005534a51453ab16ea63c4' # Links @@ -153,11 +261,12 @@ def test_links(self): coll = api.create_collection("Test Collection") # Create two library bundles and a course bundle: lib1_bundle = api.create_bundle(coll.uuid, title="Library 1", slug="lib1") - lib1_draft = api.get_or_create_draft(lib1_bundle.uuid, draft_name="test-draft") + lib1_draft = api.get_or_create_bundle_draft(lib1_bundle.uuid, draft_name="test-draft") lib2_bundle = api.create_bundle(coll.uuid, title="Library 1", slug="lib2") - lib2_draft = api.get_or_create_draft(lib2_bundle.uuid, draft_name="other-draft") + lib2_draft = api.get_or_create_bundle_draft(lib2_bundle.uuid, draft_name="other-draft") course_bundle = api.create_bundle(coll.uuid, title="Library 1", slug="course") - course_draft = api.get_or_create_draft(course_bundle.uuid, draft_name="test-draft") + course_draft = api.get_or_create_bundle_draft(course_bundle.uuid, draft_name="test-draft") + assert not api.get_bundle_version_links(course_bundle.uuid, 0) # To create links, we need valid BundleVersions, which requires having committed at least one change: api.write_draft_file(lib1_draft.uuid, "lib1-data.txt", "hello world") @@ -193,6 +302,23 @@ def test_links(self): assert course_links[link2_name].indirect[0].bundle_uuid == lib1_bundle.uuid assert course_links[link2_name].indirect[0].version == 1 - # Finally, test deleting a link from course's draft: + bundle_version_links = api.get_bundle_version_links(course_bundle.uuid, None) + assert bundle_version_links == course_links + + # Test deleting a link from course's draft: api.set_draft_link(course_draft.uuid, link2_name, None, None) assert not api.get_bundle_links(course_bundle.uuid, use_draft=course_draft.name) + + # Finally, delete the draft from the course and ensure the links are unchanged + api.delete_draft(course_draft.uuid) + course_links2 = api.get_bundle_links(course_bundle.uuid, use_draft=course_draft.name) + assert course_links == course_links2 + + def test_force_browser_url(self): + """ + Test the browser URL hack for devstacks. + """ + assert api.force_browser_url('http://edx.devstack.studio:18010/media/snapshot/definition.xml') ==\ + 'http://localhost:18010/media/snapshot/definition.xml' + assert api.force_browser_url('http://edx.devstack.lms:18000/media/snapshot/definition.xml') ==\ + 'http://localhost:18000/media/snapshot/definition.xml' diff --git a/blockstore/apps/bundles/links.py b/blockstore/apps/bundles/links.py index c70b12cf..4d5e3a1e 100644 --- a/blockstore/apps/bundles/links.py +++ b/blockstore/apps/bundles/links.py @@ -11,12 +11,21 @@ import attr +def convert_to_uuid(value): + """ + Returns a UUID from the given (string or UUID) value. + """ + if isinstance(value, UUID): + return value + return UUID(value) + + @attr.s(frozen=True) class Dependency: """ A Dependency is a pointer to exactly one Bundle + Version + Snapshot. """ - bundle_uuid = attr.ib(type=UUID) + bundle_uuid = attr.ib(type=UUID, converter=convert_to_uuid) version = attr.ib(type=int) snapshot_digest = attr.ib(type=bytes) diff --git a/blockstore/apps/bundles/migrations/0003_update_character_set.py b/blockstore/apps/bundles/migrations/0003_update_character_set.py index afc59fb1..1e3e4555 100644 --- a/blockstore/apps/bundles/migrations/0003_update_character_set.py +++ b/blockstore/apps/bundles/migrations/0003_update_character_set.py @@ -3,6 +3,9 @@ from django.db import migrations, models +from blockstore.apps.bundles.models import DB_COLLATION + + class Migration(migrations.Migration): dependencies = [ @@ -13,31 +16,31 @@ class Migration(migrations.Migration): migrations.AlterField( model_name='bundle', name='description', - field=models.TextField(blank=True, db_collation='utf8mb4_general_ci', max_length=10000), + field=models.TextField(blank=True, db_collation=DB_COLLATION, max_length=10000), ), migrations.AlterField( model_name='bundle', name='slug', - field=models.SlugField(allow_unicode=True, db_collation='utf8mb4_general_ci'), + field=models.SlugField(allow_unicode=True, db_collation=DB_COLLATION), ), migrations.AlterField( model_name='bundle', name='title', - field=models.CharField(db_collation='utf8mb4_general_ci', db_index=True, max_length=180), + field=models.CharField(db_collation=DB_COLLATION, db_index=True, max_length=180), ), migrations.AlterField( model_name='bundleversion', name='change_description', - field=models.TextField(blank=True, db_collation='utf8mb4_general_ci', max_length=1000), + field=models.TextField(blank=True, db_collation=DB_COLLATION, max_length=1000), ), migrations.AlterField( model_name='collection', name='title', - field=models.CharField(db_collation='utf8mb4_general_ci', db_index=True, max_length=180), + field=models.CharField(db_collation=DB_COLLATION, db_index=True, max_length=180), ), migrations.AlterField( model_name='draft', name='name', - field=models.CharField(db_collation='utf8mb4_general_ci', max_length=180), + field=models.CharField(db_collation=DB_COLLATION, max_length=180), ), ] diff --git a/blockstore/apps/bundles/models.py b/blockstore/apps/bundles/models.py index 8685f4c6..faf875bb 100644 --- a/blockstore/apps/bundles/models.py +++ b/blockstore/apps/bundles/models.py @@ -54,6 +54,7 @@ """ import uuid +from django.conf import settings from django.db import models from .store import DraftRepo, SnapshotRepo, bytes_from_hex_str @@ -61,6 +62,14 @@ MAX_CHAR_FIELD_LENGTH = 180 +""" +Sqlite is used for testing on edx-platform, but doesn't contain the utf8mb4_general_ci collation sequence. + +So we detect this case here, and use a different db_collation if we're running on Sqlite. +""" +DB_COLLATION = 'binary' if 'sqlite' in settings.DATABASES['default']['ENGINE'] else 'utf8mb4_general_ci' + + class Collection(models.Model): """ Administrative grouping for Bundles: policy, permissions, licensing, etc. @@ -74,7 +83,7 @@ class Collection(models.Model): """ id = models.BigAutoField(primary_key=True) uuid = models.UUIDField(default=uuid.uuid4, unique=True, editable=False) - title = models.CharField(max_length=MAX_CHAR_FIELD_LENGTH, db_index=True, db_collation='utf8mb4_general_ci') + title = models.CharField(max_length=MAX_CHAR_FIELD_LENGTH, db_index=True, db_collation=DB_COLLATION) def __str__(self): return f"{self.uuid} - {self.title}" @@ -89,15 +98,15 @@ class Bundle(models.Model): """ id = models.BigAutoField(primary_key=True) uuid = models.UUIDField(default=uuid.uuid4, unique=True, editable=False) - title = models.CharField(max_length=MAX_CHAR_FIELD_LENGTH, db_index=True, db_collation='utf8mb4_general_ci') + title = models.CharField(max_length=MAX_CHAR_FIELD_LENGTH, db_index=True, db_collation=DB_COLLATION) collection = models.ForeignKey( Collection, related_name="bundles", related_query_name="bundle", editable=False, on_delete=models.CASCADE, ) - slug = models.SlugField(allow_unicode=True, db_collation='utf8mb4_general_ci') # For pretty URLs - description = models.TextField(max_length=10000, blank=True, db_collation='utf8mb4_general_ci') + slug = models.SlugField(allow_unicode=True, db_collation=DB_COLLATION) # For pretty URLs + description = models.TextField(max_length=10000, blank=True, db_collation=DB_COLLATION) def __str__(self): return f"Bundle {self.uuid} - {self.slug}" @@ -132,7 +141,7 @@ class BundleVersion(models.Model): # This is a CharField only because Django ORM doens't support indexed binary # fields in MySQL snapshot_digest = models.CharField(max_length=40, db_index=True, editable=False) - change_description = models.TextField(max_length=1000, blank=True, db_collation='utf8mb4_general_ci') + change_description = models.TextField(max_length=1000, blank=True, db_collation=DB_COLLATION) class Meta: unique_together = ( @@ -197,7 +206,7 @@ class Draft(models.Model): Bundle, related_name="drafts", related_query_name="draft", editable=False, on_delete=models.CASCADE, ) - name = models.CharField(max_length=MAX_CHAR_FIELD_LENGTH, db_collation='utf8mb4_general_ci') + name = models.CharField(max_length=MAX_CHAR_FIELD_LENGTH, db_collation=DB_COLLATION) def save(self, *args, **kwargs): if not self.pk: diff --git a/blockstore/apps/bundles/storage.py b/blockstore/apps/bundles/storage.py index 86f1adb3..9b0a453b 100644 --- a/blockstore/apps/bundles/storage.py +++ b/blockstore/apps/bundles/storage.py @@ -90,11 +90,19 @@ def __init__(self): raise self.BackendNotAvailable from attr_error if not (key and secret): raise self.BackendNotAvailable - self.s3_backend = S3Boto3Storage( - # All other S3 settings will be pulled in automatically from Django settings - # (such as AWS_QUERYSTRING_EXPIRE and AWS_STORAGE_BUCKET_NAME). - access_key=key, secret_key=secret + + # Merge the special key and secret with extra storage args when configuring + # the S3 URLs backend. + # All other S3 settings will be pulled in automatically from Django settings + # (such as AWS_QUERYSTRING_EXPIRE and AWS_STORAGE_BUCKET_NAME). + s3_backend_args = dict( + **settings.BUNDLE_ASSET_STORAGE_SETTINGS['STORAGE_KWARGS'] + ) + s3_backend_args.update( + access_key=key, + secret_key=secret, ) + self.s3_backend = S3Boto3Storage(**s3_backend_args) def url(self, name): """ @@ -116,55 +124,65 @@ def __init__(self): """ Initialize an instance of AssetStorage. + Use the BUNDLE_ASSET_STORAGE_SETTINGS['STORAGE_CLASS'] if provided; otherwise + fall back to the default storage class. + If `LongLivedSignedUrlStorage` is active, then instantiate an instance of - it for generating URLs; otherwise, fall back to the default storage class. + it for generating URLs; otherwise, use the asset storage class defined above. """ + storage_class = settings.BUNDLE_ASSET_STORAGE_SETTINGS.get('STORAGE_CLASS') + storage_kwargs = settings.BUNDLE_ASSET_STORAGE_SETTINGS.get('STORAGE_KWARGS', {}) + if storage_class: + self.asset_backend = get_storage_class(storage_class)(**storage_kwargs) + else: + self.asset_backend = default_storage + try: self.url_backend = LongLivedSignedUrlStorage() except LongLivedSignedUrlStorage.BackendNotAvailable: - self.url_backend = default_storage + self.url_backend = self.asset_backend def url(self, name): return self.url_backend.url(name) def delete(self, name): - return default_storage.delete(name) + return self.asset_backend.delete(name) def exists(self, name): - return default_storage.exists(name) + return self.asset_backend.exists(name) def listdir(self, path): - return default_storage.listdir(path) + return self.asset_backend.listdir(path) def path(self, name): - return default_storage.path(name) + return self.asset_backend.path(name) def size(self, name): - return default_storage.size(name) + return self.asset_backend.size(name) def get_accessed_time(self, name): - return default_storage.get_accessed_time(name) + return self.asset_backend.get_accessed_time(name) def get_created_time(self, name): - return default_storage.get_created_time(name) + return self.asset_backend.get_created_time(name) def get_modified_time(self, name): - return default_storage.get_modified_time(name) + return self.asset_backend.get_modified_time(name) def get_valid_name(self, name): - return default_storage.get_valid_name(name) + return self.asset_backend.get_valid_name(name) def get_alternative_name(self, file_root, file_ext): - return default_storage.get_alternative_name(file_root, file_ext) + return self.asset_backend.get_alternative_name(file_root, file_ext) def get_available_name(self, name, max_length=None): - return default_storage.get_available_name(name, max_length=None) + return self.asset_backend.get_available_name(name, max_length=None) def _open(self, name, mode='rb'): - return default_storage._open(name, mode=mode) # pylint: disable=protected-access + return self.asset_backend._open(name, mode=mode) # pylint: disable=protected-access def _save(self, name, content): - return default_storage._save(name, content) # pylint: disable=protected-access + return self.asset_backend._save(name, content) # pylint: disable=protected-access default_asset_storage = AssetStorage() diff --git a/blockstore/apps/bundles/tests/test_storage.py b/blockstore/apps/bundles/tests/test_storage.py index ec8b2acb..5c49756e 100644 --- a/blockstore/apps/bundles/tests/test_storage.py +++ b/blockstore/apps/bundles/tests/test_storage.py @@ -1,7 +1,7 @@ """ Tests for storage classes in storage.py """ -from unittest.mock import patch +from unittest.mock import patch, MagicMock from django.test import override_settings import pytest @@ -14,21 +14,53 @@ class _MockS3backend: A fake replacment for S3Boto3Backend in these tests. """ def __init__(self, **settings): - self.accesss_key = settings["access_key"] + self.access_key = settings["access_key"] self.secret_key = settings["secret_key"] + self.bucket_name = settings["bucket_name"] + self.location = settings["location"] def url(self, name): - return f"https://example.com/s3/{name}" + return f"https://{self.bucket_name}/{self.location}{name}" + + +def get_storage_class(class_name): + """ + Returns the MockS3Backend if S3Boto3Storage requested, + or a mock class spec'd on default_storage for any other class name. + """ + if class_name == 'storages.backends.s3boto3.S3Boto3Storage': + return _MockS3backend + return MagicMock(spec=storage_module.default_storage).__class__ _patch_default_storage = patch.object( storage_module, 'default_storage', autospec=True ) _patch_get_storage_class = patch.object( - storage_module, 'get_storage_class', autospec=True, return_value=_MockS3backend + storage_module, 'get_storage_class', autospec=True, side_effect=get_storage_class +) +_patch_s3_long_lived_credentials = override_settings( + BUNDLE_ASSET_URL_STORAGE_KEY="a-key", BUNDLE_ASSET_URL_STORAGE_SECRET="a-secret", + BUNDLE_ASSET_STORAGE_SETTINGS={ + 'STORAGE_CLASS': 'storages.backends.some.other.backend', + 'STORAGE_KWARGS': { + 'bucket_name': 'example-bucket', + 'location': 's3/', + 'access_key': 'another_key', + 'secret_key': 'another_secret', + }, + }, ) _patch_s3_credentials = override_settings( - BUNDLE_ASSET_URL_STORAGE_KEY="a-key", BUNDLE_ASSET_URL_STORAGE_SECRET="a-secret" + BUNDLE_ASSET_STORAGE_SETTINGS={ + 'STORAGE_CLASS': 'storages.backends.some.other.backend', + 'STORAGE_KWARGS': { + 'bucket_name': 'example-bucket', + 'location': 's3/', + 'access_key': 'another_key', + 'secret_key': 'another_secret', + }, + }, ) @@ -48,7 +80,7 @@ def test_asset_storage_long_lived_urls_disabled(mock_default_storage): mock_default_storage.get_accessed_time.assert_called_once_with('xyz') -@_patch_s3_credentials +@_patch_s3_long_lived_credentials @_patch_get_storage_class @_patch_default_storage def test_asset_storage_long_lived_urls_enabled(mock_default_storage, *_args): @@ -58,14 +90,41 @@ def test_asset_storage_long_lived_urls_enabled(mock_default_storage, *_args): """ backend = storage_module.AssetStorage() assert isinstance(backend.url_backend, storage_module.LongLivedSignedUrlStorage) - assert backend.url_backend.s3_backend.accesss_key == "a-key" + assert backend.url_backend.s3_backend.access_key == "a-key" assert backend.url_backend.s3_backend.secret_key == "a-secret" - assert backend.url('abc') == "https://example.com/s3/abc" + assert backend.url_backend.s3_backend.bucket_name == "example-bucket" + assert backend.url_backend.s3_backend.location == "s3/" + assert backend.asset_backend.access_key == "another_key" + assert backend.asset_backend.secret_key == "another_secret" + assert backend.asset_backend.bucket_name == "example-bucket" + assert backend.asset_backend.location == "s3/" + assert backend.url('abc') == "https://example-bucket/s3/abc" backend.listdir('123') backend.get_accessed_time('xyz') assert not mock_default_storage.url.called - mock_default_storage.listdir.assert_called_once_with('123') - mock_default_storage.get_accessed_time.assert_called_once_with('xyz') + backend.asset_backend.listdir.assert_called_once_with('123') + backend.asset_backend.get_accessed_time.assert_called_once_with('xyz') + + +@_patch_s3_credentials +@_patch_get_storage_class +@_patch_default_storage +def test_asset_storage_basic_s3(mock_default_storage, *_args): + """ + Test that `AssetStorage` is configured as expected when there's no long-lived URL signing credentials configured. + """ + backend = storage_module.AssetStorage() + assert backend.url_backend is backend.asset_backend + assert backend.asset_backend.access_key == "another_key" + assert backend.asset_backend.secret_key == "another_secret" + assert backend.asset_backend.bucket_name == "example-bucket" + assert backend.asset_backend.location == "s3/" + backend.url('abc') + backend.listdir('123') + backend.get_accessed_time('xyz') + assert not mock_default_storage.url.called + backend.asset_backend.listdir.assert_called_once_with('123') + backend.asset_backend.get_accessed_time.assert_called_once_with('xyz') @_patch_default_storage diff --git a/blockstore/settings/base.py b/blockstore/settings/base.py index 13da9875..ec8856fb 100644 --- a/blockstore/settings/base.py +++ b/blockstore/settings/base.py @@ -302,11 +302,14 @@ def root(*x): } } +################################################################################ +# BUNDLES CONFIGURATION + # .. setting_name: BUNDLE_ASSET_URL_STORAGE_KEY # .. setting_default: None # .. setting_description: When this is set, `BUNDLE_ASSET_URL_STORAGE_SECRET` is # set, and `boto3` is installed, this is used as an AWS IAM access key for -# generating signed, read-only URLs for assets stored in S3. +# generating signed, read-only URLs for blockstore assets stored in S3. # Otherwise, URLs are generated based on the default storage configuration. # See `blockstore.apps.bundles.storage.LongLivedSignedUrlStorage` for details. BUNDLE_ASSET_URL_STORAGE_KEY = None @@ -315,7 +318,14 @@ def root(*x): # .. setting_default: None # .. setting_description: When this is set, `BUNDLE_ASSET_URL_STORAGE_KEY` is # set, and `boto3` is installed, this is used as an AWS IAM secret key for -# generating signed, read-only URLs for assets stored in S3. +# generating signed, read-only URLs for blockstore assets stored in S3. # Otherwise, URLs are generated based on the default storage configuration. # See `blockstore.apps.bundles.storage.LongLivedSignedUrlStorage` for details. BUNDLE_ASSET_URL_STORAGE_SECRET = None + +# .. setting_name: BUNDLE_ASSET_STORAGE_SETTINGS +# .. setting_default: empty dict, uses django DEFAULT_STORAGE_CLASS and settings. +# .. setting_description: Provide `STORAGE_CLASS` and (optional) `STORAGE_KWARGS` +# to configure the storage settings for bundle asset files. +# See `blockstore.apps.bundles.storage.AssetStorage` for details. +BUNDLE_ASSET_STORAGE_SETTINGS = {}