From 626f11f6080a124dbd0db07a2280f74c67c611bb Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 20 Apr 2023 11:34:51 -0700 Subject: [PATCH] test: Import linter: optionally enforce usage of a package's public API (#31903) * test: warn about dependencies from cms->openedx->lms and vice versa * test: warn about importing from package's internal implementation code * chore: Update some imports to use public APIs only * chore: Update 'bookmarks' app to have stricter public API * fix: we are sharing 'adapters' from olx_rest_api to content_staging --- lms/djangoapps/courseware/block_render.py | 2 +- openedx/core/djangoapps/bookmarks/api.py | 213 +---------------- openedx/core/djangoapps/bookmarks/api_impl.py | 215 ++++++++++++++++++ openedx/core/djangoapps/bookmarks/services.py | 2 +- .../djangoapps/bookmarks/tests/test_api.py | 18 +- .../core/djangoapps/content_libraries/api.py | 13 +- .../content_libraries/library_bundle.py | 4 +- .../content_libraries/library_context.py | 2 +- .../content_libraries/tests/base.py | 44 +--- .../content_staging/block_serializer.py | 2 +- openedx/core/djangoapps/olx_rest_api/api.py | 22 ++ openedx/core/djangoapps/xblock/api.py | 16 +- .../djangolib/tests/test_blockstore_cache.py | 2 +- openedx/core/lib/blockstore_api/tests/base.py | 42 ++++ .../tests/test_blockstore_api.py | 2 +- openedx/testing/importlinter/__init__.py | 0 .../importlinter/isolated_apps_contract.py | 59 +++++ setup.cfg | 95 ++++++++ 18 files changed, 490 insertions(+), 263 deletions(-) create mode 100644 openedx/core/djangoapps/bookmarks/api_impl.py create mode 100644 openedx/core/djangoapps/olx_rest_api/api.py create mode 100644 openedx/core/lib/blockstore_api/tests/base.py create mode 100644 openedx/testing/importlinter/__init__.py create mode 100644 openedx/testing/importlinter/isolated_apps_contract.py diff --git a/lms/djangoapps/courseware/block_render.py b/lms/djangoapps/courseware/block_render.py index e9cf15678e..9d80d8365a 100644 --- a/lms/djangoapps/courseware/block_render.py +++ b/lms/djangoapps/courseware/block_render.py @@ -69,7 +69,7 @@ from lms.djangoapps.grades.api import GradesUtilService from lms.djangoapps.lms_xblock.field_data import LmsFieldData from lms.djangoapps.lms_xblock.runtime import LmsModuleSystem, UserTagsService from lms.djangoapps.verify_student.services import XBlockVerificationService -from openedx.core.djangoapps.bookmarks.services import BookmarksService +from openedx.core.djangoapps.bookmarks.api import BookmarksService from openedx.core.djangoapps.crawlers.models import CrawlersConfig from openedx.core.djangoapps.credit.services import CreditService from openedx.core.djangoapps.util.user_utils import SystemUser diff --git a/openedx/core/djangoapps/bookmarks/api.py b/openedx/core/djangoapps/bookmarks/api.py index 3558625fb4..7a65b0711f 100644 --- a/openedx/core/djangoapps/bookmarks/api.py +++ b/openedx/core/djangoapps/bookmarks/api.py @@ -1,204 +1,15 @@ """ -Bookmarks Python API. +Bookmarks Python public API. """ -from django.conf import settings -from eventtracking import tracker +# pylint: disable=unused-import -from xmodule.modulestore.django import modulestore -from xmodule.modulestore.exceptions import ItemNotFoundError - -from . import DEFAULT_FIELDS, OPTIONAL_FIELDS -from .models import Bookmark -from .serializers import BookmarkSerializer - - -class BookmarksLimitReachedError(Exception): - """ - if try to create new bookmark when max limit of bookmarks already reached - """ - pass # lint-amnesty, pylint: disable=unnecessary-pass - - -def get_bookmark(user, usage_key, fields=None): - """ - Return data for a bookmark. - - Arguments: - user (User): The user of the bookmark. - usage_key (UsageKey): The usage_key of the bookmark. - fields (list): List of field names the data should contain (optional). - - Returns: - Dict. - - Raises: - ObjectDoesNotExist: If a bookmark with the parameters does not exist. - """ - bookmarks_queryset = Bookmark.objects - - if len(set(fields or []) & set(OPTIONAL_FIELDS)) > 0: - bookmarks_queryset = bookmarks_queryset.select_related('user', 'xblock_cache') - else: - bookmarks_queryset = bookmarks_queryset.select_related('user') - - bookmark = bookmarks_queryset.get(user=user, usage_key=usage_key) - return BookmarkSerializer(bookmark, context={'fields': fields}).data - - -def get_bookmarks(user, course_key=None, fields=None, serialized=True): - """ - Return data for bookmarks of a user. - - Arguments: - user (User): The user of the bookmarks. - course_key (CourseKey): The course_key of the bookmarks (optional). - fields (list): List of field names the data should contain (optional). - N/A if serialized is False. - serialized (bool): Whether to return a queryset or a serialized list of dicts. - Default is True. - - Returns: - List of dicts if serialized is True else queryset. - """ - if user.is_authenticated: - bookmarks_queryset = Bookmark.objects.filter(user=user) - - if course_key: - bookmarks_queryset = bookmarks_queryset.filter(course_key=course_key) - - if len(set(fields or []) & set(OPTIONAL_FIELDS)) > 0: - bookmarks_queryset = bookmarks_queryset.select_related('user', 'xblock_cache') - else: - bookmarks_queryset = bookmarks_queryset.select_related('user') - - bookmarks_queryset = bookmarks_queryset.order_by('-id') - else: - bookmarks_queryset = Bookmark.objects.none() - - if serialized: - return BookmarkSerializer(bookmarks_queryset, context={'fields': fields}, many=True).data - - return bookmarks_queryset - - -def can_create_more(data): - """ - Determine if a new Bookmark can be created for the course - based on limit defined in django.conf.settings.MAX_BOOKMARKS_PER_COURSE - - Arguments: - data (dict): The data to create the object with. - Returns: - Boolean - """ - data = dict(data) - - user = data['user'] - course_key = data['usage_key'].course_key - - # User can create up to max_bookmarks_per_course bookmarks - if Bookmark.objects.filter(user=user, course_key=course_key).count() >= settings.MAX_BOOKMARKS_PER_COURSE: - return False - - return True - - -def create_bookmark(user, usage_key): - """ - Create a bookmark. - - Arguments: - user (User): The user of the bookmark. - usage_key (UsageKey): The usage_key of the bookmark. - - Returns: - Dict. - - Raises: - ItemNotFoundError: If no block exists for the usage_key. - BookmarksLimitReachedError: if try to create new bookmark when max limit of bookmarks already reached - """ - - usage_key = usage_key.replace(course_key=modulestore().fill_in_run(usage_key.course_key)) - data = { - 'user': user, - 'usage_key': usage_key - } - - if usage_key.course_key.run is None: - raise ItemNotFoundError - - if not can_create_more(data): - raise BookmarksLimitReachedError - - bookmark, created = Bookmark.create(data) - if created: - _track_event('edx.bookmark.added', bookmark) - return BookmarkSerializer(bookmark, context={'fields': DEFAULT_FIELDS + OPTIONAL_FIELDS}).data - - -def delete_bookmark(user, usage_key): - """ - Delete a bookmark. - - Arguments: - user (User): The user of the bookmark. - usage_key (UsageKey): The usage_key of the bookmark. - - Returns: - Dict. - - Raises: - ObjectDoesNotExist: If a bookmark with the parameters does not exist. - """ - bookmark = Bookmark.objects.get(user=user, usage_key=usage_key) - bookmark.delete() - _track_event('edx.bookmark.removed', bookmark) - - -def delete_bookmarks(usage_key): - """ - Delete all bookmarks for usage_key. - - Arguments: - usage_key (UsageKey): The usage_key of the bookmarks. - """ - units_keys = [] - - if usage_key.block_type == 'vertical': - units_keys.append(usage_key) - else: - # NOTE: Get all children for deleted block - descriptor = modulestore().get_item(usage_key) - for child in descriptor.get_children(): - if usage_key.block_type == 'chapter': - units_keys += [unit.location for unit in child.get_children()] - else: - units_keys.append(child.location) - - bookmarks = Bookmark.objects.filter(usage_key__in=units_keys) - - # Emit removed bookmard event - for bookmark in bookmarks: - _track_event('edx.bookmark.removed', bookmark) - - bookmarks.delete() - - -def _track_event(event_name, bookmark): - """ - Emit events for a bookmark. - - Arguments: - event_name: name of event to track - bookmark: Bookmark object - """ - tracker.emit( - event_name, - { - 'course_id': str(bookmark.course_key), - 'bookmark_id': bookmark.resource_id, - 'component_type': bookmark.usage_key.block_type, - 'component_usage_id': str(bookmark.usage_key), - } - ) +from .api_impl import ( + BookmarksLimitReachedError, + get_bookmark, + get_bookmarks, + can_create_more, + create_bookmark, + delete_bookmark, + delete_bookmarks, +) +from .services import BookmarksService diff --git a/openedx/core/djangoapps/bookmarks/api_impl.py b/openedx/core/djangoapps/bookmarks/api_impl.py new file mode 100644 index 0000000000..01559f64e5 --- /dev/null +++ b/openedx/core/djangoapps/bookmarks/api_impl.py @@ -0,0 +1,215 @@ +""" +Bookmarks Python API. +""" +# Why does this file exist instead of just using api.py? +# (1) to expose BookmarkService (from .services import BookmarksService) in +# api.py; its implementation in services.py requires code from api.py. It +# caused a circular import error unless we put the API implementation into +# a separate file like this. +# (2) The code below imports a number of symbols that shouldn't be part of the +# public API (DEFAULT_FIELDS, OPTIONAL_FIELDS, Bookmark, BookmarkSerializer, +# modulestore, ItemNotFoundError, settings, tracker) and it's much easier +# to make them private this way (importing them into api_impl.py without +# changes) than it is to import them with an underscore prefix and change +# the code to use the prefixed imports. +from django.conf import settings +from eventtracking import tracker + +from xmodule.modulestore.django import modulestore +from xmodule.modulestore.exceptions import ItemNotFoundError + +from . import DEFAULT_FIELDS, OPTIONAL_FIELDS +from .models import Bookmark +from .serializers import BookmarkSerializer + + +class BookmarksLimitReachedError(Exception): + """ + if try to create new bookmark when max limit of bookmarks already reached + """ + pass # lint-amnesty, pylint: disable=unnecessary-pass + + +def get_bookmark(user, usage_key, fields=None): + """ + Return data for a bookmark. + + Arguments: + user (User): The user of the bookmark. + usage_key (UsageKey): The usage_key of the bookmark. + fields (list): List of field names the data should contain (optional). + + Returns: + Dict. + + Raises: + ObjectDoesNotExist: If a bookmark with the parameters does not exist. + """ + bookmarks_queryset = Bookmark.objects + + if len(set(fields or []) & set(OPTIONAL_FIELDS)) > 0: + bookmarks_queryset = bookmarks_queryset.select_related('user', 'xblock_cache') + else: + bookmarks_queryset = bookmarks_queryset.select_related('user') + + bookmark = bookmarks_queryset.get(user=user, usage_key=usage_key) + return BookmarkSerializer(bookmark, context={'fields': fields}).data + + +def get_bookmarks(user, course_key=None, fields=None, serialized=True): + """ + Return data for bookmarks of a user. + + Arguments: + user (User): The user of the bookmarks. + course_key (CourseKey): The course_key of the bookmarks (optional). + fields (list): List of field names the data should contain (optional). + N/A if serialized is False. + serialized (bool): Whether to return a queryset or a serialized list of dicts. + Default is True. + + Returns: + List of dicts if serialized is True else queryset. + """ + if user.is_authenticated: + bookmarks_queryset = Bookmark.objects.filter(user=user) + + if course_key: + bookmarks_queryset = bookmarks_queryset.filter(course_key=course_key) + + if len(set(fields or []) & set(OPTIONAL_FIELDS)) > 0: + bookmarks_queryset = bookmarks_queryset.select_related('user', 'xblock_cache') + else: + bookmarks_queryset = bookmarks_queryset.select_related('user') + + bookmarks_queryset = bookmarks_queryset.order_by('-id') + else: + bookmarks_queryset = Bookmark.objects.none() + + if serialized: + return BookmarkSerializer(bookmarks_queryset, context={'fields': fields}, many=True).data + + return bookmarks_queryset + + +def can_create_more(data): + """ + Determine if a new Bookmark can be created for the course + based on limit defined in django.conf.settings.MAX_BOOKMARKS_PER_COURSE + + Arguments: + data (dict): The data to create the object with. + Returns: + Boolean + """ + data = dict(data) + + user = data['user'] + course_key = data['usage_key'].course_key + + # User can create up to max_bookmarks_per_course bookmarks + if Bookmark.objects.filter(user=user, course_key=course_key).count() >= settings.MAX_BOOKMARKS_PER_COURSE: + return False + + return True + + +def create_bookmark(user, usage_key): + """ + Create a bookmark. + + Arguments: + user (User): The user of the bookmark. + usage_key (UsageKey): The usage_key of the bookmark. + + Returns: + Dict. + + Raises: + ItemNotFoundError: If no block exists for the usage_key. + BookmarksLimitReachedError: if try to create new bookmark when max limit of bookmarks already reached + """ + + usage_key = usage_key.replace(course_key=modulestore().fill_in_run(usage_key.course_key)) + data = { + 'user': user, + 'usage_key': usage_key + } + + if usage_key.course_key.run is None: + raise ItemNotFoundError + + if not can_create_more(data): + raise BookmarksLimitReachedError + + bookmark, created = Bookmark.create(data) + if created: + _track_event('edx.bookmark.added', bookmark) + return BookmarkSerializer(bookmark, context={'fields': DEFAULT_FIELDS + OPTIONAL_FIELDS}).data + + +def delete_bookmark(user, usage_key): + """ + Delete a bookmark. + + Arguments: + user (User): The user of the bookmark. + usage_key (UsageKey): The usage_key of the bookmark. + + Returns: + Dict. + + Raises: + ObjectDoesNotExist: If a bookmark with the parameters does not exist. + """ + bookmark = Bookmark.objects.get(user=user, usage_key=usage_key) + bookmark.delete() + _track_event('edx.bookmark.removed', bookmark) + + +def delete_bookmarks(usage_key): + """ + Delete all bookmarks for usage_key. + + Arguments: + usage_key (UsageKey): The usage_key of the bookmarks. + """ + units_keys = [] + + if usage_key.block_type == 'vertical': + units_keys.append(usage_key) + else: + # NOTE: Get all children for deleted block + descriptor = modulestore().get_item(usage_key) + for child in descriptor.get_children(): + if usage_key.block_type == 'chapter': + units_keys += [unit.location for unit in child.get_children()] + else: + units_keys.append(child.location) + + bookmarks = Bookmark.objects.filter(usage_key__in=units_keys) + + # Emit removed bookmard event + for bookmark in bookmarks: + _track_event('edx.bookmark.removed', bookmark) + + bookmarks.delete() + + +def _track_event(event_name, bookmark): + """ + Emit events for a bookmark. + + Arguments: + event_name: name of event to track + bookmark: Bookmark object + """ + tracker.emit( + event_name, + { + 'course_id': str(bookmark.course_key), + 'bookmark_id': bookmark.resource_id, + 'component_type': bookmark.usage_key.block_type, + 'component_usage_id': str(bookmark.usage_key), + } + ) diff --git a/openedx/core/djangoapps/bookmarks/services.py b/openedx/core/djangoapps/bookmarks/services.py index 0d69da675b..2e2e25b01b 100644 --- a/openedx/core/djangoapps/bookmarks/services.py +++ b/openedx/core/djangoapps/bookmarks/services.py @@ -11,7 +11,7 @@ from edx_django_utils.cache import DEFAULT_REQUEST_CACHE from xmodule.modulestore.django import modulestore from xmodule.modulestore.exceptions import ItemNotFoundError -from . import DEFAULT_FIELDS, api +from . import DEFAULT_FIELDS, api_impl as api log = logging.getLogger(__name__) diff --git a/openedx/core/djangoapps/bookmarks/tests/test_api.py b/openedx/core/djangoapps/bookmarks/tests/test_api.py index 351886716f..0f5a3405bc 100644 --- a/openedx/core/djangoapps/bookmarks/tests/test_api.py +++ b/openedx/core/djangoapps/bookmarks/tests/test_api.py @@ -111,7 +111,7 @@ class BookmarksAPITests(BookmarkApiEventTestMixin, BookmarksTestsBase): assert len(bookmarks) == count assert bookmarks.model is Bookmark - @patch('openedx.core.djangoapps.bookmarks.api.tracker.emit') + @patch('openedx.core.djangoapps.bookmarks.api_impl.tracker.emit') def test_create_bookmark(self, mock_tracker): """ Verifies that create_bookmark create & returns data as expected. @@ -132,7 +132,7 @@ class BookmarksAPITests(BookmarkApiEventTestMixin, BookmarksTestsBase): assert len(api.get_bookmarks(user=self.user, course_key=self.course.id)) == 5 - @patch('openedx.core.djangoapps.bookmarks.api.tracker.emit') + @patch('openedx.core.djangoapps.bookmarks.api_impl.tracker.emit') def test_create_bookmark_do_not_create_duplicates(self, mock_tracker): """ Verifies that create_bookmark do not create duplicate bookmarks. @@ -163,7 +163,7 @@ class BookmarksAPITests(BookmarkApiEventTestMixin, BookmarksTestsBase): self.assert_no_events_were_emitted(mock_tracker) - @patch('openedx.core.djangoapps.bookmarks.api.tracker.emit') + @patch('openedx.core.djangoapps.bookmarks.api_impl.tracker.emit') def test_create_bookmark_raises_error(self, mock_tracker): """ Verifies that create_bookmark raises error as expected. @@ -174,7 +174,7 @@ class BookmarksAPITests(BookmarkApiEventTestMixin, BookmarksTestsBase): self.assert_no_events_were_emitted(mock_tracker) - @patch('openedx.core.djangoapps.bookmarks.api.tracker.emit') + @patch('openedx.core.djangoapps.bookmarks.api_impl.tracker.emit') @patch('django.conf.settings.MAX_BOOKMARKS_PER_COURSE', 5) def bookmark_more_than_limit_raise_error(self, mock_tracker): """ @@ -200,7 +200,7 @@ class BookmarksAPITests(BookmarkApiEventTestMixin, BookmarksTestsBase): api.create_bookmark(user=self.other_user, usage_key=blocks[-1].location) assert len(api.get_bookmarks(user=self.other_user, course_key=blocks[(- 1)].location.course_key)) == 1 - @patch('openedx.core.djangoapps.bookmarks.api.tracker.emit') + @patch('openedx.core.djangoapps.bookmarks.api_impl.tracker.emit') def test_delete_bookmark(self, mock_tracker): """ Verifies that delete_bookmark removes bookmark as expected. @@ -224,7 +224,7 @@ class BookmarksAPITests(BookmarkApiEventTestMixin, BookmarksTestsBase): assert str(self.sequential_1.location) != bookmarks_data[0]['usage_id'] assert str(self.sequential_1.location) != bookmarks_data[1]['usage_id'] - @patch('openedx.core.djangoapps.bookmarks.api.tracker.emit') + @patch('openedx.core.djangoapps.bookmarks.api_impl.tracker.emit') def test_delete_bookmarks_with_vertical_block_type(self, mock_tracker): assert len(api.get_bookmarks(user=self.user)) == 5 @@ -241,8 +241,8 @@ class BookmarksAPITests(BookmarkApiEventTestMixin, BookmarksTestsBase): assert len(api.get_bookmarks(self.user)) == 4 - @patch('openedx.core.djangoapps.bookmarks.api.modulestore') - @patch('openedx.core.djangoapps.bookmarks.api.tracker.emit') + @patch('openedx.core.djangoapps.bookmarks.api_impl.modulestore') + @patch('openedx.core.djangoapps.bookmarks.api_impl.tracker.emit') def test_delete_bookmarks_with_chapter_block_type(self, mock_tracker, mocked_modulestore): mocked_modulestore().get_item().get_children = Mock( return_value=[Mock(get_children=Mock(return_value=[Mock( @@ -263,7 +263,7 @@ class BookmarksAPITests(BookmarkApiEventTestMixin, BookmarksTestsBase): ) assert len(api.get_bookmarks(self.user)) == 4 - @patch('openedx.core.djangoapps.bookmarks.api.tracker.emit') + @patch('openedx.core.djangoapps.bookmarks.api_impl.tracker.emit') def test_delete_bookmark_raises_error(self, mock_tracker): """ Verifies that delete_bookmark raises error as expected. diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index ecc3abd8d0..ecb18d6817 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -92,10 +92,13 @@ from openedx.core.djangoapps.content_libraries.signals import ( LIBRARY_BLOCK_UPDATED, LIBRARY_BLOCK_DELETED, ) -from openedx.core.djangoapps.olx_rest_api.block_serializer import XBlockSerializer -from openedx.core.djangoapps.xblock.api import get_block_display_name, load_block -from openedx.core.djangoapps.xblock.learning_context.manager import get_learning_context_impl -from openedx.core.djangoapps.xblock.runtime.olx_parsing import XBlockInclude +from openedx.core.djangoapps.olx_rest_api.api import serialize_modulestore_block_for_blockstore +from openedx.core.djangoapps.xblock.api import ( + get_block_display_name, + get_learning_context_impl, + load_block, + XBlockInclude, +) from openedx.core.lib.blockstore_api import ( get_bundle, get_bundles, @@ -1258,7 +1261,7 @@ class EdxModulestoreImportClient(BaseEdxImportClient): Get block OLX by serializing it from modulestore directly. """ block = self.modulestore.get_item(block_key) - data = XBlockSerializer(block) + data = serialize_modulestore_block_for_blockstore(block) return {'olx': data.olx_str, 'static_files': {s.name: s for s in data.static_files}} diff --git a/openedx/core/djangoapps/content_libraries/library_bundle.py b/openedx/core/djangoapps/content_libraries/library_bundle.py index ed03b18012..3665365f79 100644 --- a/openedx/core/djangoapps/content_libraries/library_bundle.py +++ b/openedx/core/djangoapps/content_libraries/library_bundle.py @@ -10,11 +10,11 @@ from xblock.core import XBlock from xblock.plugin import PluginMissingError from openedx.core.djangoapps.content_libraries.models import ContentLibrary -from openedx.core.djangoapps.xblock.runtime.blockstore_runtime import xml_for_definition -from openedx.core.djangoapps.xblock.runtime.olx_parsing import ( +from openedx.core.djangoapps.xblock.api import ( BundleFormatException, definition_for_include, parse_xblock_include, + xml_for_definition, ) from openedx.core.djangolib.blockstore_cache import ( BundleCache, diff --git a/openedx/core/djangoapps/content_libraries/library_context.py b/openedx/core/djangoapps/content_libraries/library_context.py index d25d9a3f3a..8526193f9a 100644 --- a/openedx/core/djangoapps/content_libraries/library_context.py +++ b/openedx/core/djangoapps/content_libraries/library_context.py @@ -13,7 +13,7 @@ from openedx.core.djangoapps.content_libraries.library_bundle import ( usage_for_child_include, ) from openedx.core.djangoapps.content_libraries.models import ContentLibrary -from openedx.core.djangoapps.xblock.learning_context import LearningContext +from openedx.core.djangoapps.xblock.api import LearningContext log = logging.getLogger(__name__) diff --git a/openedx/core/djangoapps/content_libraries/tests/base.py b/openedx/core/djangoapps/content_libraries/tests/base.py index 34aee552ee..f567960f39 100644 --- a/openedx/core/djangoapps/content_libraries/tests/base.py +++ b/openedx/core/djangoapps/content_libraries/tests/base.py @@ -4,12 +4,10 @@ Tests for Blockstore-based Content Libraries from contextlib import contextmanager from io import BytesIO from urllib.parse import urlencode -from unittest import mock, skipUnless -from urllib.parse import urlparse +from unittest import mock from django.conf import settings from django.test import LiveServerTestCase -from django.test.client import RequestFactory from django.test.utils import override_settings from organizations.models import Organization from rest_framework.test import APITestCase, APIClient @@ -20,6 +18,11 @@ from openedx.core.djangoapps.content_libraries.libraries_index import MAX_SIZE from openedx.core.djangoapps.content_libraries.constants import COMPLEX, ALL_RIGHTS_RESERVED from openedx.core.djangolib.testing.utils import skip_unless_cms from openedx.core.lib import blockstore_api +from openedx.core.lib.blockstore_api.tests.base import ( + BlockstoreAppTestMixin, + requires_blockstore, + requires_blockstore_app, +) # Define the URLs here - don't use reverse() because we want to detect # backwards-incompatible changes like changed URLs. @@ -49,41 +52,6 @@ URL_BLOCK_METADATA_URL = '/api/xblock/v2/xblocks/{block_key}/' URL_BLOCK_XBLOCK_HANDLER = '/api/xblock/v2/xblocks/{block_key}/handler/{user_id}-{secure_token}/{handler_name}/' -# Decorators for tests that require the blockstore service/app -requires_blockstore = skipUnless(settings.RUN_BLOCKSTORE_TESTS, "Requires a running Blockstore server") - -requires_blockstore_app = skipUnless(settings.BLOCKSTORE_USE_BLOCKSTORE_APP_API, "Requires blockstore app") - - -class BlockstoreAppTestMixin: - """ - Sets up the environment for tests to be run using the installed Blockstore app. - """ - def setUp(self): - """ - Ensure there's an active request, so that bundle file URLs can be made absolute. - """ - super().setUp() - - # Patch the blockstore get_current_request to use our live_server_url - mock.patch('blockstore.apps.api.methods.get_current_request', - mock.Mock(return_value=self._get_current_request())).start() - self.addCleanup(mock.patch.stopall) - - def _get_current_request(self): - """ - Returns a request object using the live_server_url, if available. - """ - request_args = {} - if hasattr(self, 'live_server_url'): - live_server_url = urlparse(self.live_server_url) - name, port = live_server_url.netloc.split(':') - request_args['SERVER_NAME'] = name - request_args['SERVER_PORT'] = port or '80' - request_args['wsgi.url_scheme'] = live_server_url.scheme - return RequestFactory().request(**request_args) - - def elasticsearch_test(func): """ Decorator for tests which connect to elasticsearch when needed diff --git a/openedx/core/djangoapps/content_staging/block_serializer.py b/openedx/core/djangoapps/content_staging/block_serializer.py index f3d9d84538..e1d6d353df 100644 --- a/openedx/core/djangoapps/content_staging/block_serializer.py +++ b/openedx/core/djangoapps/content_staging/block_serializer.py @@ -8,7 +8,7 @@ from collections import namedtuple from lxml import etree -from openedx.core.djangoapps.olx_rest_api import adapters +from openedx.core.djangoapps.olx_rest_api.api import adapters log = logging.getLogger(__name__) diff --git a/openedx/core/djangoapps/olx_rest_api/api.py b/openedx/core/djangoapps/olx_rest_api/api.py new file mode 100644 index 0000000000..752e1e9cca --- /dev/null +++ b/openedx/core/djangoapps/olx_rest_api/api.py @@ -0,0 +1,22 @@ +""" +Public Python API for the OLX REST API app +""" +from .block_serializer import XBlockSerializer as _XBlockSerializer +# pylint: disable=unused-import +# 'adapters' are _temporarily_ part of the public API to keep the code DRY until +# we can consolidate the two different block_serializer implementations in +# content_staging and olx_rest_api. +from . import adapters + + +def serialize_modulestore_block_for_blockstore(block): + """ + Given a modulestore XBlock (e.g. loaded using + modulestore.get_item(block_key) + ), produce: + (1) A new definition ID for use in Blockstore + (2) an XML string defining the XBlock and referencing the IDs of its + children (but not containing the actual XML of its children) + (3) a list of any static files required by the XBlock and their URL + """ + return _XBlockSerializer(block) diff --git a/openedx/core/djangoapps/xblock/api.py b/openedx/core/djangoapps/xblock/api.py index b3ac882850..7c24308f6b 100644 --- a/openedx/core/djangoapps/xblock/api.py +++ b/openedx/core/djangoapps/xblock/api.py @@ -7,6 +7,7 @@ the older runtime. Note that these views are only for interacting with existing blocks. Other Studio APIs cover use cases like adding/deleting/editing blocks. """ +# pylint: disable=unused-import import logging import threading @@ -22,10 +23,21 @@ from xblock.exceptions import NoSuchViewError from openedx.core.djangoapps.xblock.apps import get_xblock_app_config from openedx.core.djangoapps.xblock.learning_context.manager import get_learning_context_impl from openedx.core.djangoapps.xblock.runtime.blockstore_runtime import BlockstoreXBlockRuntime, xml_for_definition -from openedx.core.djangoapps.xblock.runtime.runtime import XBlockRuntimeSystem +from openedx.core.djangoapps.xblock.runtime.runtime import XBlockRuntimeSystem as _XBlockRuntimeSystem from openedx.core.djangolib.blockstore_cache import BundleCache from .utils import get_secure_token_for_xblock_handler, get_xblock_id_for_anonymous_user +# Made available as part of this package's public API: +from openedx.core.djangoapps.xblock.learning_context import LearningContext +from openedx.core.djangoapps.xblock.runtime.olx_parsing import ( + BundleFormatException, + definition_for_include, + parse_xblock_include, + XBlockInclude, +) + +# Implementation: + log = logging.getLogger(__name__) @@ -54,7 +66,7 @@ def get_runtime_system(): runtime_class=BlockstoreXBlockRuntime, ) params.update(get_xblock_app_config().get_runtime_system_params()) - setattr(get_runtime_system, cache_name, XBlockRuntimeSystem(**params)) + setattr(get_runtime_system, cache_name, _XBlockRuntimeSystem(**params)) return getattr(get_runtime_system, cache_name) diff --git a/openedx/core/djangolib/tests/test_blockstore_cache.py b/openedx/core/djangolib/tests/test_blockstore_cache.py index 84d071ab8f..5c55e07f44 100644 --- a/openedx/core/djangolib/tests/test_blockstore_cache.py +++ b/openedx/core/djangolib/tests/test_blockstore_cache.py @@ -5,7 +5,7 @@ from unittest.mock import patch from django.test import TestCase from openedx.core.djangolib.blockstore_cache import BundleCache -from openedx.core.djangoapps.content_libraries.tests.base import ( +from openedx.core.lib.blockstore_api.tests.base import ( BlockstoreAppTestMixin, requires_blockstore, requires_blockstore_app, diff --git a/openedx/core/lib/blockstore_api/tests/base.py b/openedx/core/lib/blockstore_api/tests/base.py new file mode 100644 index 0000000000..0888e8c81d --- /dev/null +++ b/openedx/core/lib/blockstore_api/tests/base.py @@ -0,0 +1,42 @@ +""" +Common code for tests that work with Blockstore +""" +from unittest import mock, skipUnless +from urllib.parse import urlparse + +from django.conf import settings +from django.test.client import RequestFactory + +# Decorators for tests that require the blockstore service/app +requires_blockstore = skipUnless(settings.RUN_BLOCKSTORE_TESTS, "Requires a running Blockstore server") + +requires_blockstore_app = skipUnless(settings.BLOCKSTORE_USE_BLOCKSTORE_APP_API, "Requires blockstore app") + + +class BlockstoreAppTestMixin: + """ + Sets up the environment for tests to be run using the installed Blockstore app. + """ + def setUp(self): + """ + Ensure there's an active request, so that bundle file URLs can be made absolute. + """ + super().setUp() + + # Patch the blockstore get_current_request to use our live_server_url + mock.patch('blockstore.apps.api.methods.get_current_request', + mock.Mock(return_value=self._get_current_request())).start() + self.addCleanup(mock.patch.stopall) + + def _get_current_request(self): + """ + Returns a request object using the live_server_url, if available. + """ + request_args = {} + if hasattr(self, 'live_server_url'): + live_server_url = urlparse(self.live_server_url) + name, port = live_server_url.netloc.split(':') + request_args['SERVER_NAME'] = name + request_args['SERVER_PORT'] = port or '80' + request_args['wsgi.url_scheme'] = live_server_url.scheme + return RequestFactory().request(**request_args) diff --git a/openedx/core/lib/blockstore_api/tests/test_blockstore_api.py b/openedx/core/lib/blockstore_api/tests/test_blockstore_api.py index a1239d5a00..9c15e16668 100644 --- a/openedx/core/lib/blockstore_api/tests/test_blockstore_api.py +++ b/openedx/core/lib/blockstore_api/tests/test_blockstore_api.py @@ -8,7 +8,7 @@ import pytest from django.test import TestCase from openedx.core.lib import blockstore_api as api -from openedx.core.djangoapps.content_libraries.tests.base import ( +from openedx.core.lib.blockstore_api.tests.base import ( BlockstoreAppTestMixin, requires_blockstore, requires_blockstore_app, diff --git a/openedx/testing/importlinter/__init__.py b/openedx/testing/importlinter/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/openedx/testing/importlinter/isolated_apps_contract.py b/openedx/testing/importlinter/isolated_apps_contract.py new file mode 100644 index 0000000000..ba77a18077 --- /dev/null +++ b/openedx/testing/importlinter/isolated_apps_contract.py @@ -0,0 +1,59 @@ +""" +An importlinter contract that can flag imports of private APIs +""" + +from importlinter import Contract, ContractCheck, fields, output + + +class IsolatedAppsContract(Contract): + """ + Contract that defines most of an 'app' (python package) as private, and + ensures that python code outside of the package doesn't import anything + other than the public API defined in the package's `api.py` file. + """ + isolated_apps = fields.ListField(subfield=fields.StringField()) + # List of allowed modules (like ["api", "urls"] to allow "import x.api") + allowed_modules = fields.ListField(subfield=fields.StringField()) + + def check(self, graph, verbose): + forbidden_imports_found = [] + + for package in self.isolated_apps: + output.verbose_print( + verbose, + f"Getting import details for anything that imports {package}..." + ) + modules = graph.find_descendants(package) + for module in modules: + # We have a list of modules like "api.py" that *are* allowed to be imported from anywhere: + for allowed_module in self.allowed_modules: + if module.endswith(f".{allowed_module}"): + break + else: + # See who is importing this: + importers = graph.find_modules_that_directly_import(module) + for importer in importers: + if importer.startswith(package): + continue # Ignore imports from within the same package + # Add this import to our list of contract violations: + import_details = graph.get_import_details(importer=importer, imported=module) + for import_detail in import_details: + forbidden_imports_found.append({**import_detail, "package": package}) + + return ContractCheck( + kept=not bool(forbidden_imports_found), + metadata={ + 'forbidden_imports_found': forbidden_imports_found, + } + ) + + def render_broken_contract(self, check): + for details in check.metadata['forbidden_imports_found']: + package = details['package'] + importer = details['importer'] + line_number = details['line_number'] + line_contents = details['line_contents'] + output.print_error(f'{importer}:{line_number}: imported from non-public API of {package}:') + output.indent_cursor() + output.print_error(line_contents) + output.new_line() diff --git a/setup.cfg b/setup.cfg index 7b1be0be92..7fcb03ff15 100644 --- a/setup.cfg +++ b/setup.cfg @@ -52,7 +52,12 @@ skip= root_packages = lms cms + openedx include_external_packages = True +contract_types = + # Our custom contract which checks that we're only importing from 'api.py' + # for participating packages. + isolated_apps: openedx.testing.importlinter.isolated_apps_contract.IsolatedAppsContract [importlinter:contract:1] name = lms and cms are independent @@ -61,10 +66,16 @@ modules = lms cms ignore_imports = + ############################################################################ # lms side imports that we are ignoring for now lms.djangoapps.course_home_api.outline.tests.test_view -> cms.djangoapps.contentstore.outlines lms.djangoapps.courseware.plugins -> cms.djangoapps.contentstore.utils lms.djangoapps.course_home_api.tests.utils -> cms.djangoapps.contentstore.outlines + # lms.djangoapps.instructor.tests.test_api & lms.djangoapps.instructor.tests.test_tools + # -> openedx.core.djangoapps.course_date_signals.handlers + # -> cms.djangoapps.contentstore.config.waffle + openedx.core.djangoapps.course_date_signals.handlers -> cms.djangoapps.contentstore.config.waffle + ############################################################################ # cms side imports that we are ignoring for now cms.djangoapps.contentstore.views.tests.test_block -> lms.djangoapps.lms_xblock.mixin cms.envs.common -> lms.envs.common @@ -73,3 +84,87 @@ ignore_imports = cms.djangoapps.contentstore.views.preview -> lms.djangoapps.lms_xblock.field_data cms.envs.common -> lms.djangoapps.lms_xblock.mixin cms.envs.test -> lms.envs.test + # cms.djangoapps.contentstore.views.tests.test_group_configurations + # -> openedx.features.content_type_gating.helpers + # -> lms.djangoapps.courseware.masquerade + openedx.features.content_type_gating.helpers -> lms.djangoapps.courseware.masquerade + # cms.djangoapps.contentstore.utils + # -> openedx.core.djangoapps.django_comment_common.models + # -> openedx.core.djangoapps.course_groups.cohorts + # -> lms.djangoapps.courseware.courses + openedx.core.djangoapps.course_groups.cohorts -> lms.djangoapps.courseware.courses + # cms.djangoapps.models.settings.course_metadata + # -> openedx.features.course_experience + # -> openedx.features.course_experience.url_helpers + # -> lms.djangoapps.courseware.toggles + openedx.features.course_experience.url_helpers -> lms.djangoapps.courseware.toggles + # cms.djangoapps.contentstore.[various] + # -> openedx.features.content_type_gating.partitions + # -> lms.djangoapps.commerce.utils + openedx.features.content_type_gating.partitions -> lms.djangoapps.commerce.utils + # cms.djangoapps.contentstore.course_group_config + # -> openedx.core.djangoapps.course_groups.partition_scheme + # -> lms.djangoapps.courseware.masquerade + openedx.core.djangoapps.course_groups.partition_scheme -> lms.djangoapps.courseware.masquerade + # cms.djangoapps.contentstore.[various] & cms.djangoapps.coursegraph.[various] + # -> openedx.core.djangoapps.content.course_overviews.models + # -> lms.djangoapps.ccx.utils + # & lms.djangoapps.certificates.api + # & lms.djangoapps.discussion.django_comment_client + openedx.core.djangoapps.content.course_overviews.models -> lms.djangoapps.*.* + # cms.djangoapps.export_course_metadata.tasks + # -> openedx.core.djangoapps.schedules.content_highlights + # -> lms.djangoapps.courseware.block_render & lms.djangoapps.courseware.model_data + openedx.core.djangoapps.schedules.content_highlights -> lms.djangoapps.courseware.* + # cms.djangoapps.contentstore.[various] + # -> openedx.core.lib.gating.api + # -> lms.djangoapps.course_blocks.api & lms.djangoapps.courseware.access & lms.djangoapps.grades.api + openedx.core.lib.gating.api -> lms.djangoapps.*.* + # cms.djangoapps.contentstore.[various] + # -> openedx.features.content_type_gating.partitions + # -> openedx.features.discounts.utils + # -> lms.djangoapps.courseware.utils & lms.djangoapps.experiments.models + openedx.features.discounts.utils -> lms.djangoapps.courseware.utils + openedx.features.discounts.utils -> lms.djangoapps.experiments.models + # cms.djangoapps.contentstore.signals.handlers + # -> openedx.core.djangoapps.discussions.tasks + # -> openedx.core.djangoapps.discussions.utils + # -> lms.djangoapps.courseware.access + openedx.core.djangoapps.discussions.utils -> lms.djangoapps.courseware.access + # cms.djangoapps.contentstore.[various] + # -> openedx.features.content_type_gating.partitions + # -> openedx.features.discounts.utils + # -> openedx.features.discounts.applicability + # -> lms.djangoapps.courseware.toggles + # & lms.djangoapps.courseware.utils + # & lms.djangoapps.experiments.models + # & lms.djangoapps.experiments.stable_bucketing + openedx.features.discounts.applicability -> lms.djangoapps.courseware.* + openedx.features.discounts.applicability -> lms.djangoapps.experiments.* + # cms.djangoapps.contentstore.[various] + # -> openedx.core.djangoapps.content.learning_sequences.api + # -> openedx.core.djangoapps.content.learning_sequences.api.outlines + # -> openedx.core.djangoapps.content.learning_sequences.api.permissions + # -> lms.djangoapps.courseware.access + openedx.core.djangoapps.content.learning_sequences.api.permissions -> lms.djangoapps.courseware.access + # cms.djangoapps.contentstore.[various] + # -> openedx.features.content_type_gating.partitions + # -> openedx.features.discounts.utils + # -> openedx.features.discounts.applicability + # -> openedx.features.enterprise_support.utils + openedx.features.enterprise_support.utils -> lms.djangoapps.branding.api + + +[importlinter:contract:2] +name = Do not depend on non-public API of isolated apps. +type = isolated_apps +isolated_apps = + openedx.core.djangoapps.agreements + openedx.core.djangoapps.bookmarks + openedx.core.djangoapps.content_libraries + openedx.core.djangoapps.olx_rest_api + openedx.core.djangoapps.xblock +allowed_modules = + # Only imports from api.py are allowed elsewhere in the code + # See https://open-edx-proposals.readthedocs.io/en/latest/best-practices/oep-0049-django-app-patterns.html#api-py + api