diff --git a/cms/djangoapps/contentstore/rest_api/v1/authoring_urls.py b/cms/djangoapps/contentstore/rest_api/v1/authoring_urls.py new file mode 100644 index 000000000000..ab66ea1854f3 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v1/authoring_urls.py @@ -0,0 +1,52 @@ +""" +Conforming (ADR 0038) URLs for the authoring API, v1. + +Mounted at ``api/authoring/v1/`` from ``cms/urls.py``, beside the legacy +``/api/contentstore/v1/xblock/`` routes, which stay live for their OEP-21 +deprecation window and are marked ``deprecated: true`` in the OpenAPI schema +(see ``cms/lib/spectacular.py``). + +ADR 0038 conformance relative to the legacy mount: + +* rule 2 — the collection is plural (``xblocks/``), the API name singular; +* rule 3 — the API name describes the domain (``authoring``), not the + implementing Django app (``contentstore``); +* rule 9 — the identifier is resolved by the shared ``usage_key`` path + converter (``edx_rest_framework_extensions.url_converters``), which rejects + deprecated ``i4x://`` keys with a 404; +* rule 11 — URL names are ``snake_case``, version-free, and unique. + +Note: ADR 0038 (implementation note 4) asks that ``/api/authoring/v1/xblocks/`` +be reconciled with the existing Learning Core ``/api/xblock/v2/xblocks/`` +rather than leaving two names for what looks like one API. That reconciliation +is an API-owner decision tracked with the DEPR work, not part of this +mechanical migration. +""" + +from django.urls import path + +from cms.djangoapps.contentstore.rest_api.v1.views import XblockViewSet + +app_name = "authoring_v1" + +urlpatterns = [ + # No ``list`` action exists on the viewset, so the collection URL accepts + # POST only — the same surface the legacy router-generated route exposes. + path( + "xblocks/", + XblockViewSet.as_view({"post": "create"}), + name="xblock_list", + ), + path( + "xblocks//", + XblockViewSet.as_view( + { + "get": "retrieve", + "put": "update", + "patch": "partial_update", + "delete": "destroy", + } + ), + name="xblock_detail", + ), +] diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_xblock_viewset.py b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_xblock_viewset.py index 638b0ce2eb35..0edee2c4b992 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_xblock_viewset.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_xblock_viewset.py @@ -10,7 +10,7 @@ from unittest.mock import patch from django.http import JsonResponse -from django.urls import reverse +from django.urls import resolve, reverse from rest_framework import status from rest_framework.test import APITestCase @@ -216,3 +216,70 @@ def test_minimal_view_is_noop_for_non_json_payload(self, mock_retrieve): response = self.client.get(_detail_url(), {"view": "minimal", "fields": "graderType"}) assert response.status_code == status.HTTP_200_OK assert response.json() == "notgraded" + + +# --------------------------------------------------------------------------- +# ADR 0038 — URL-structure tests +# --------------------------------------------------------------------------- + + +def _authoring_list_url(): + return reverse("authoring_v1:xblock_list") + + +def _authoring_detail_url(): + return reverse( + "authoring_v1:xblock_detail", + kwargs={"usage_key_string": TEST_LOCATOR}, + ) + + +class XblockViewSetUrlStructureTest(ModuleStoreTestCase, APITestCase): + """ + ADR 0038 — the conforming /api/authoring/v1/xblocks/ routes are mounted + beside the legacy /api/contentstore/v1/xblock/ routes and serve the same + view. + """ + + def setUp(self): + super().setUp() + self.staff = GlobalStaffFactory(password='password') + self.client.force_authenticate(user=self.staff) + + def test_conforming_urls_reverse_to_expected_paths(self): + assert _authoring_list_url() == "/api/authoring/v1/xblocks/" + assert _authoring_detail_url() == f"/api/authoring/v1/xblocks/{TEST_LOCATOR}/" + + def test_conforming_and_legacy_routes_share_view(self): + legacy_cls = resolve(_detail_url()).func.cls + conforming_cls = resolve(_authoring_detail_url()).func.cls + assert conforming_cls is legacy_cls + + def test_invalid_usage_key_is_404_on_conforming_route(self): + # The shared usage_key converter rejects unparseable keys with a + # routing-level 404. + response = self.client.get("/api/authoring/v1/xblocks/not-a-usage-key/") + assert response.status_code == status.HTTP_404_NOT_FOUND + + @patch(f"{_VIEW_MODULE}.retrieve_xblock_response", return_value=_MOCK_RESPONSE) + def test_get_on_conforming_route_calls_retrieve(self, mock_fn): + # Also exercises the UsageKey→str coercion in XblockViewSet.initial(). + response = self.client.get(_authoring_detail_url()) + assert response.status_code == status.HTTP_200_OK + mock_fn.assert_called_once() + assert mock_fn.call_args[0][0].method == "GET" + + @patch(f"{_VIEW_MODULE}.create_xblock_response", return_value=_MOCK_RESPONSE) + def test_post_on_conforming_route_calls_create(self, mock_fn): + data = {"parent_locator": PARENT_LOCATOR, "category": "html"} + response = self.client.post(_authoring_list_url(), data=data, format="json") + assert response.status_code == status.HTTP_200_OK + mock_fn.assert_called_once() + assert mock_fn.call_args[0][0].method == "POST" + + @patch(f"{_VIEW_MODULE}.delete_xblock_response", return_value=_MOCK_RESPONSE) + def test_delete_on_conforming_route_calls_destroy(self, mock_fn): + response = self.client.delete(_authoring_detail_url()) + assert response.status_code == status.HTTP_200_OK + mock_fn.assert_called_once() + assert mock_fn.call_args[0][0].method == "DELETE" diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/xblock.py b/cms/djangoapps/contentstore/rest_api/v1/views/xblock.py index 2d8d94ceb1de..dac3953c330c 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/xblock.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/xblock.py @@ -198,7 +198,15 @@ def initial(self, request, *args, **kwargs): bytes) rather than request.data to avoid consuming the WSGI stream before @expect_json_in_class_view runs. """ - usage_key_string = kwargs.get("usage_key_string") + # ADR 0038: the conforming /api/authoring/v1/xblocks// + # route passes a parsed UsageKey, while the legacy + # /api/contentstore/v1/xblock/ route passes the raw string. Coerce to + # the string form the action methods expect; ``self.kwargs`` is the + # same dict ``dispatch()`` unpacks into the handler, so the handler + # receives the coerced value as well. + if isinstance(self.kwargs.get("usage_key_string"), UsageKey): + self.kwargs["usage_key_string"] = str(self.kwargs["usage_key_string"]) + usage_key_string = self.kwargs.get("usage_key_string") if usage_key_string: try: self.course_key = UsageKey.from_string(usage_key_string).course_key diff --git a/cms/djangoapps/contentstore/rest_api/v3/authoring_urls.py b/cms/djangoapps/contentstore/rest_api/v3/authoring_urls.py new file mode 100644 index 000000000000..c9393179a3d5 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v3/authoring_urls.py @@ -0,0 +1,67 @@ +""" +Conforming (ADR 0038) URLs for the authoring API, v3. + +Mounted at ``api/authoring/v3/`` from ``cms/urls.py``, beside the legacy +``/api/contentstore/v3/`` routes, which stay live for their OEP-21 +deprecation window and are marked ``deprecated: true`` in the OpenAPI schema +(see ``cms/lib/spectacular.py``). + +ADR 0038 conformance relative to the legacy mount: + +* rule 3 — the API name describes the domain (``authoring``), not the + implementing Django app (``contentstore``); +* rule 4 / 8 — the screen-shaped ``course_details`` and ``authoring_grading`` + collections become sub-resources of the plural ``courses/`` collection, + one level deep, per the ADR's own target for these endpoints + (``/api/authoring/…/courses/{course_key}/details/`` "and siblings"); +* rule 9 — course keys are resolved by the shared ``course_key`` path + converter (``edx_rest_framework_extensions.url_converters``), which rejects + deprecated ``Org/Course/Run`` keys with a 404; +* rule 11 — URL names are ``snake_case``, version-free, and unique. + +``home/`` is a BFF aggregate for the Studio home screen. Rule 4 disfavors +screen names, but ADR 0038's BFF provision applies: the surface keeps its +``/api/`` prefix and a single canonical conforming mount, and is marked +``x-internal`` in the OpenAPI schema (``cms/lib/spectacular.py``) so clients +can tell it apart from a stable resource contract. +""" + +from django.urls import path + +from cms.djangoapps.contentstore.rest_api.v3.views import AuthoringGradingViewSet, CourseDetailsViewSet, HomeViewSet + +app_name = "authoring_v3" + +urlpatterns = [ + # Studio home BFF (x-internal — see module docstring). + path( + "home/", + HomeViewSet.as_view({"get": "list"}), + name="home", + ), + path( + "home/courses/", + HomeViewSet.as_view({"get": "courses"}), + name="home_courses", + ), + path( + "home/libraries/", + HomeViewSet.as_view({"get": "libraries"}), + name="home_libraries", + ), + # Course details — /api/contentstore/v3/course_details/{course_id}/ + # renamed per the ADR's target shape; same view, same contract. + path( + "courses//details/", + CourseDetailsViewSet.as_view({"get": "retrieve", "put": "update"}), + name="course_details", + ), + # Course grading — /api/contentstore/v3/authoring_grading/{course_key}/ + # renamed per the ADR's target shape; same view, same contract. The + # ``authoring_`` prefix is dropped because the namespace already says it. + path( + "courses//grading/", + AuthoringGradingViewSet.as_view({"patch": "partial_update"}), + name="course_grading", + ), +] diff --git a/cms/djangoapps/contentstore/rest_api/v3/tests/test_home.py b/cms/djangoapps/contentstore/rest_api/v3/tests/test_home.py index f2b4a4744d65..c882dc29278b 100644 --- a/cms/djangoapps/contentstore/rest_api/v3/tests/test_home.py +++ b/cms/djangoapps/contentstore/rest_api/v3/tests/test_home.py @@ -10,7 +10,7 @@ ``EXCEPTION_HANDLER`` setting is unchanged, so v0/v1/v2 endpoints continue to return the legacy error shape. """ -from django.urls import reverse +from django.urls import resolve, reverse from rest_framework import status from rest_framework.test import APIClient, APITestCase @@ -84,3 +84,37 @@ def test_v1_endpoint_unaffected_by_v3_envelope(self): # v1 still uses the project-default handler → ADR 0029 fields absent. assert "type" not in response.data assert "instance" not in response.data + + +# =========================================================================== +# ADR 0038 — URL-structure tests +# =========================================================================== +class TestHomeViewSetUrlStructure(APITestCase): + """ + ADR 0038 — the conforming /api/authoring/v3/home/ routes are mounted + beside the legacy /api/contentstore/v3/home/ routes and serve the same + view. + """ + + def test_conforming_urls_reverse_to_expected_paths(self): + assert reverse("authoring_v3:home") == "/api/authoring/v3/home/" + assert reverse("authoring_v3:home_courses") == "/api/authoring/v3/home/courses/" + assert reverse("authoring_v3:home_libraries") == "/api/authoring/v3/home/libraries/" + + def test_conforming_and_legacy_routes_share_view(self): + pairs = ( + ("cms.djangoapps.contentstore:v3:home-list", "authoring_v3:home"), + ("cms.djangoapps.contentstore:v3:home-courses", "authoring_v3:home_courses"), + ("cms.djangoapps.contentstore:v3:home-libraries", "authoring_v3:home_libraries"), + ) + for legacy_name, conforming_name in pairs: + legacy_cls = resolve(reverse(legacy_name)).func.cls + conforming_cls = resolve(reverse(conforming_name)).func.cls + assert conforming_cls is legacy_cls, f"{conforming_name} must serve the same view as {legacy_name}" + + def test_unauthenticated_conforming_route_returns_standardized_401(self): + """The conforming mount carries the same contract — ADR 0029 envelope included.""" + response = APIClient().get(reverse("authoring_v3:home")) + assert response.status_code == status.HTTP_401_UNAUTHORIZED + for field in _REQUIRED_ERROR_FIELDS: + assert field in response.data, f"ADR 0029: missing field '{field}'" diff --git a/cms/djangoapps/contentstore/rest_api/v3/utils.py b/cms/djangoapps/contentstore/rest_api/v3/utils.py index 79524acb8c53..4fed4e44f7d8 100644 --- a/cms/djangoapps/contentstore/rest_api/v3/utils.py +++ b/cms/djangoapps/contentstore/rest_api/v3/utils.py @@ -27,10 +27,15 @@ from openedx.core.djangoapps.content.course_overviews.models import CourseOverview -def resolve_course_key(course_key: str) -> CourseKey: +def resolve_course_key(course_key: str | CourseKey) -> CourseKey: """ - Parse ``course_key`` (string) into a :class:`CourseKey` and verify the - course exists. + Parse ``course_key`` into a :class:`CourseKey` and verify the course + exists. + + Accepts either the raw string (the legacy ``/api/contentstore/v3/`` + routes) or an already-parsed :class:`CourseKey` (the conforming + ``/api/authoring/v3/`` routes, whose ``course_key`` path converter — + ADR 0038 rule 9 — hands views a parsed key). Raises: rest_framework.exceptions.NotFound: if the string is unparseable @@ -44,7 +49,7 @@ def resolve_course_key(course_key: str) -> CourseKey: positional argument. """ try: - parsed = CourseKey.from_string(course_key) + parsed = course_key if isinstance(course_key, CourseKey) else CourseKey.from_string(course_key) except InvalidKeyError as exc: raise NotFound("The provided course key cannot be parsed.") from exc if not CourseOverview.course_exists(parsed): diff --git a/cms/djangoapps/contentstore/rest_api/v3/views/tests/test_authoring_grading.py b/cms/djangoapps/contentstore/rest_api/v3/views/tests/test_authoring_grading.py index 33f9efd69ff4..932a5b3b25d5 100644 --- a/cms/djangoapps/contentstore/rest_api/v3/views/tests/test_authoring_grading.py +++ b/cms/djangoapps/contentstore/rest_api/v3/views/tests/test_authoring_grading.py @@ -18,7 +18,7 @@ from unittest.mock import patch from django.test import TestCase -from django.urls import reverse +from django.urls import resolve, reverse from rest_framework import status from rest_framework.test import APIClient, APITestCase @@ -291,3 +291,66 @@ def test_v0_endpoint_unaffected_by_v3_envelope(self): assert response.status_code == status.HTTP_401_UNAUTHORIZED assert "type" not in response.data assert "instance" not in response.data + + +# =========================================================================== +# ADR 0038 — URL-structure tests +# =========================================================================== +class TestAuthoringGradingViewSetUrlStructure(APITestCase): + """ + ADR 0038 — the conforming /api/authoring/v3/courses/{course_key}/grading/ + route is mounted beside the legacy + /api/contentstore/v3/authoring_grading/{course_key}/ route and serves + the same view. + """ + + def setUp(self): + super().setUp() + self.client = APIClient() + self.conforming_url = reverse( + "authoring_v3:course_grading", + kwargs={"course_key": COURSE_ID}, + ) + self.legacy_url = reverse( + "cms.djangoapps.contentstore:v3:authoring_grading-detail", + kwargs={"course_key": COURSE_ID}, + ) + + def test_conforming_url_reverses_to_expected_path(self): + assert self.conforming_url == f"/api/authoring/v3/courses/{COURSE_ID}/grading/" + + def test_conforming_and_legacy_routes_share_view(self): + legacy_cls = resolve(self.legacy_url).func.cls + conforming_cls = resolve(self.conforming_url).func.cls + assert conforming_cls is legacy_cls + + def test_invalid_course_key_is_404_on_conforming_route(self): + # The shared course_key converter rejects unparseable keys with a + # routing-level 404. + response = self.client.patch( + "/api/authoring/v3/courses/not-a-course-key/grading/", + data={}, format="json", + ) + assert response.status_code == status.HTTP_404_NOT_FOUND + + def test_unauthenticated_patch_returns_401(self): + response = self.client.patch(self.conforming_url, data={}, format="json") + assert response.status_code == status.HTTP_401_UNAUTHORIZED + + @patch(MOCK_CREDIT_TASK) + @patch(MOCK_UPDATE_FROM_JSON, return_value=_MOCK_GRADING_MODEL) + @patch(MOCK_HAS_PERMISSION, return_value=True) + @patch(MOCK_COURSE_EXISTS, return_value=True) + def test_patch_on_conforming_route_updates_grading( + self, mock_exists, mock_perm, mock_update, mock_credit, # noqa: ARG002 + ): + """Same contract on the conforming mount as on the legacy one.""" + user = UserFactory.create() + self.client.force_authenticate(user=user) + response = self.client.patch( + self.conforming_url, + data={"graders": _GRADERS_PAYLOAD}, + format="json", + ) + assert response.status_code == status.HTTP_200_OK + mock_update.assert_called_once() diff --git a/cms/djangoapps/contentstore/rest_api/v3/views/tests/test_course_details.py b/cms/djangoapps/contentstore/rest_api/v3/views/tests/test_course_details.py index e25df0188d18..6b838e00a167 100644 --- a/cms/djangoapps/contentstore/rest_api/v3/views/tests/test_course_details.py +++ b/cms/djangoapps/contentstore/rest_api/v3/views/tests/test_course_details.py @@ -19,7 +19,7 @@ """ from unittest.mock import MagicMock, patch -from django.urls import reverse +from django.urls import resolve, reverse from rest_framework import status from rest_framework.test import APIClient, APITestCase @@ -378,3 +378,56 @@ def test_fields_csv_restricts_top_level_keys( assert response.status_code == status.HTTP_200_OK assert set(response.data.keys()) == {"course_id", "title"} + + +# =========================================================================== +# ADR 0038 — URL-structure tests +# =========================================================================== +class TestCourseDetailsViewSetUrlStructure(APITestCase): + """ + ADR 0038 — the conforming /api/authoring/v3/courses/{course_key}/details/ + route is mounted beside the legacy + /api/contentstore/v3/course_details/{course_id}/ route and serves the + same view. + """ + + def _conforming_url(self): + return reverse( + "authoring_v3:course_details", + kwargs={"course_id": TEST_COURSE_ID}, + ) + + def _legacy_url(self): + return reverse( + "cms.djangoapps.contentstore:v3:course_details-detail", + kwargs={"course_id": TEST_COURSE_ID}, + ) + + def test_conforming_url_reverses_to_expected_path(self): + assert self._conforming_url() == ( + f"/api/authoring/v3/courses/{TEST_COURSE_ID}/details/" + ) + + def test_conforming_and_legacy_routes_share_view(self): + legacy_cls = resolve(self._legacy_url()).func.cls + conforming_cls = resolve(self._conforming_url()).func.cls + assert conforming_cls is legacy_cls + + def test_invalid_course_key_is_404_on_conforming_route(self): + # The shared course_key converter rejects unparseable keys with a + # routing-level 404. + response = self.client.get("/api/authoring/v3/courses/not-a-course-key/details/") + assert response.status_code == status.HTTP_404_NOT_FOUND + + def test_unauthenticated_get_returns_401(self): + response = self.client.get(self._conforming_url()) + assert response.status_code == status.HTTP_401_UNAUTHORIZED + + @patch(MOCK_COURSE_EXISTS, return_value=True) + @patch(MOCK_HAS_PERMISSION, return_value=False) + def test_non_author_get_returns_403(self, mock_perm, mock_exists): # noqa: ARG002 + """The conforming mount enforces the same authorization as the legacy one.""" + user = UserFactory.create() + self.client.force_authenticate(user=user) + response = self.client.get(self._conforming_url()) + assert response.status_code == status.HTTP_403_FORBIDDEN diff --git a/cms/djangoapps/contentstore/rest_api/v4/authoring_urls.py b/cms/djangoapps/contentstore/rest_api/v4/authoring_urls.py new file mode 100644 index 000000000000..f97257f982e0 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v4/authoring_urls.py @@ -0,0 +1,31 @@ +""" +Conforming (ADR 0038) URLs for the authoring API, v4. + +Mounted at ``api/authoring/v4/`` from ``cms/urls.py``, beside the legacy +``/api/contentstore/v4/home/courses/`` route, which stays live for its OEP-21 +deprecation window and is marked ``deprecated: true`` in the OpenAPI schema +(see ``cms/lib/spectacular.py``). + +ADR 0038 conformance relative to the legacy mount: + +* rule 3 — the API name describes the domain (``authoring``), not the + implementing Django app (``contentstore``); +* rule 4 — the screen-shaped ``home/courses/`` address becomes the concrete + plural collection ``courses/`` (the authorable courses, filtered, sorted, + and paginated in the query string); +* rule 11 — the URL name is ``snake_case``, version-free, and unique. +""" + +from django.urls import path + +from cms.djangoapps.contentstore.rest_api.v4.views import home + +app_name = "authoring_v4" + +urlpatterns = [ + path( + "courses/", + home.HomeCoursesViewSet.as_view({"get": "list"}), + name="course_list", + ), +] diff --git a/cms/djangoapps/contentstore/rest_api/v4/views/tests/test_home.py b/cms/djangoapps/contentstore/rest_api/v4/views/tests/test_home.py index 4b6b89c92d6c..b9bc646ed8b4 100644 --- a/cms/djangoapps/contentstore/rest_api/v4/views/tests/test_home.py +++ b/cms/djangoapps/contentstore/rest_api/v4/views/tests/test_home.py @@ -8,7 +8,7 @@ import ddt from django.conf import settings -from django.urls import reverse +from django.urls import resolve, reverse from rest_framework import status from rest_framework.test import APIClient, APITestCase @@ -277,3 +277,41 @@ def test_no_ordering_param_no_deprecation_header(self): response = self.client.get(self.list_url) self.assertNotIn("Deprecation", response) # noqa: PT009 + + +# =========================================================================== +# ADR 0038 — URL-structure tests +# =========================================================================== +class TestHomeCoursesViewSetUrlStructure(APITestCase): + """ + ADR 0038 — the conforming /api/authoring/v4/courses/ route is mounted + beside the legacy /api/contentstore/v4/home/courses/ route and serves + the same view. + """ + + def test_conforming_url_reverses_to_expected_path(self): + assert reverse("authoring_v4:course_list") == "/api/authoring/v4/courses/" + + def test_conforming_and_legacy_routes_share_view(self): + legacy_cls = resolve( + reverse("cms.djangoapps.contentstore:v4:home-courses-list") + ).func.cls + conforming_cls = resolve(reverse("authoring_v4:course_list")).func.cls + assert conforming_cls is legacy_cls + + def test_unauthenticated_returns_401(self): + response = APIClient().get(reverse("authoring_v4:course_list")) + self.assertEqual(response.status_code, status.HTTP_401_UNAUTHORIZED) # noqa: PT009 + + def test_authenticated_staff_gets_200(self): + """Same contract on the conforming mount as on the legacy one.""" + from django.contrib.auth import get_user_model + + User = get_user_model() + user = User.objects.create_user( + username="teststaff-authoring", password="pass", is_staff=True + ) + self.client.force_authenticate(user=user) + with patch(_MOCK_GET_COURSE_CONTEXT_V2, return_value=([], [])): + response = self.client.get(reverse("authoring_v4:course_list")) + self.assertEqual(response.status_code, status.HTTP_200_OK) # noqa: PT009 diff --git a/cms/envs/devstack.py b/cms/envs/devstack.py index 0576a35272af..0562c3099a1c 100644 --- a/cms/envs/devstack.py +++ b/cms/envs/devstack.py @@ -356,6 +356,14 @@ def should_show_debug_toolbar(request): # pylint: disable=missing-function-docs 'SERVE_INCLUDE_SCHEMA': False, # restrict spectacular to CMS API endpoints (cms/lib/spectacular.py): 'PREPROCESSING_HOOKS': ['cms.lib.spectacular.cms_api_filter'], + # ADR 0038 / OEP-21: mark legacy addresses of migrated APIs deprecated + # and BFF surfaces x-internal (cms/lib/spectacular.py). The enum hook is + # drf-spectacular's default, restated because setting this key overrides + # the default list. + 'POSTPROCESSING_HOOKS': [ + 'drf_spectacular.hooks.postprocess_schema_enums', + 'cms.lib.spectacular.cms_mark_migrated_paths', + ], # remove the default schema path prefix to replace it with server-specific base paths: 'SCHEMA_PATH_PREFIX': '/api/contentstore', 'SCHEMA_PATH_PREFIX_TRIM': '/api/contentstore', diff --git a/cms/envs/production.py b/cms/envs/production.py index 604d2753bccd..350aa52037b6 100644 --- a/cms/envs/production.py +++ b/cms/envs/production.py @@ -416,6 +416,14 @@ def get_env_setting(setting): 'SERVE_INCLUDE_SCHEMA': False, # restrict spectacular to CMS API endpoints (cms/lib/spectacular.py): 'PREPROCESSING_HOOKS': ['cms.lib.spectacular.cms_api_filter'], + # ADR 0038 / OEP-21: mark legacy addresses of migrated APIs deprecated + # and BFF surfaces x-internal (cms/lib/spectacular.py). The enum hook is + # drf-spectacular's default, restated because setting this key overrides + # the default list. + 'POSTPROCESSING_HOOKS': [ + 'drf_spectacular.hooks.postprocess_schema_enums', + 'cms.lib.spectacular.cms_mark_migrated_paths', + ], # remove the default schema path prefix to replace it with server-specific base paths: 'SCHEMA_PATH_PREFIX': '/api/contentstore', 'SCHEMA_PATH_PREFIX_TRIM': '/api/contentstore', diff --git a/cms/lib/spectacular.py b/cms/lib/spectacular.py index 90bce5668fec..27e8a03c033c 100644 --- a/cms/lib/spectacular.py +++ b/cms/lib/spectacular.py @@ -2,14 +2,35 @@ import re +# Legacy schema paths of the APIs migrated to their ADR 0038-conforming +# /api/authoring/ addresses. The legacy routes stay live for their OEP-21 +# deprecation window and are marked ``deprecated: true`` in the schema so +# generated clients steer to the conforming address. Paths are as they appear +# in the schema, i.e. after SCHEMA_PATH_PREFIX_TRIM strips /api/contentstore. +LEGACY_MIGRATED_PATH_PREFIXES = ( + "/v1/xblock/", # → /api/authoring/v1/xblocks/ + "/v3/home/", # → /api/authoring/v3/home/ + "/v3/course_details/", # → /api/authoring/v3/courses/{course_key}/details/ + "/v3/authoring_grading/", # → /api/authoring/v3/courses/{course_key}/grading/ + "/v4/home/courses/", # → /api/authoring/v4/courses/ +) + +# BFF surfaces (ADR 0038): kept under /api/ with one canonical conforming +# mount, but marked ``x-internal`` so clients can tell them apart from a +# stable resource contract. Applies to both the legacy and conforming mounts. +INTERNAL_BFF_PATH_PREFIXES = ( + "/v3/home/", + "/api/authoring/v3/home/", +) + def cms_api_filter(endpoints): """ - Pre-processing hook: keep only contentstore versioned endpoints and select - course-level endpoints. + Pre-processing hook: keep only contentstore + authoring versioned + endpoints and select course-level endpoints. """ filtered = [] - CMS_PATH_PATTERN = re.compile(r"^/api/contentstore/v\d+/") + CMS_PATH_PATTERN = re.compile(r"^/api/(contentstore|authoring)/v\d+/") for path, path_regex, method, callback in endpoints: if ( @@ -22,3 +43,23 @@ def cms_api_filter(endpoints): filtered.append((path, path_regex, method, callback)) return filtered + + +def cms_mark_migrated_paths(result, generator, request, public): # pylint: disable=unused-argument + """ + Post-processing hook (ADR 0038 / OEP-21): mark the legacy addresses of + migrated APIs ``deprecated: true`` and BFF surfaces ``x-internal``. + """ + for path, path_item in result.get("paths", {}).items(): + legacy = path.startswith(LEGACY_MIGRATED_PATH_PREFIXES) + internal = path.startswith(INTERNAL_BFF_PATH_PREFIXES) + if not (legacy or internal): + continue + for operation in path_item.values(): + if not isinstance(operation, dict): + continue + if legacy: + operation["deprecated"] = True + if internal: + operation["x-internal"] = True + return result diff --git a/cms/urls.py b/cms/urls.py index c0f96f489bb8..38c2a71c0812 100644 --- a/cms/urls.py +++ b/cms/urls.py @@ -13,6 +13,7 @@ from django.views.generic import RedirectView from drf_spectacular.views import SpectacularAPIView, SpectacularSwaggerView from edx_api_doc_tools import make_docs_urls +from edx_rest_framework_extensions.url_converters import register_url_converters import openedx.core.djangoapps.common_views.xblock import openedx.core.djangoapps.debug.views @@ -26,6 +27,10 @@ from openedx.core.djangoapps.password_policy import compliance as password_policy_compliance from openedx.core.djangoapps.password_policy.forms import PasswordPolicyAwareAdminAuthForm +# Shared opaque-key path converters (ADR 0038): registered once per service, +# before any URL pattern that uses / . +register_url_converters() + django_autodiscover() admin.site.site_header = _('Studio Administration') admin.site.site_title = admin.site.site_header @@ -356,6 +361,16 @@ path('api/contentstore/', include('cms.djangoapps.contentstore.rest_api.urls')) ] +# Authoring REST APIs — the ADR 0038-conforming addresses of the APIs +# standardized under FC-0118, dual-mounted (OEP-21) beside their legacy +# /api/contentstore/ routes during the deprecation window. Per ADR 0038 +# rule 5, each mount declares its own full api/{api_name}/v{N}/ prefix. +urlpatterns += [ + path('api/authoring/v1/', include('cms.djangoapps.contentstore.rest_api.v1.authoring_urls')), + path('api/authoring/v3/', include('cms.djangoapps.contentstore.rest_api.v3.authoring_urls')), + path('api/authoring/v4/', include('cms.djangoapps.contentstore.rest_api.v4.authoring_urls')), +] + # Content tagging urlpatterns += [ path('api/content_tagging/', include(('openedx.core.djangoapps.content_tagging.urls', 'content_tagging'))), diff --git a/lms/envs/common.py b/lms/envs/common.py index f7a6f15558cb..96b9ee411228 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -2161,6 +2161,13 @@ 'VERSION': '0.1.0', 'SERVE_INCLUDE_SCHEMA': False, 'PREPROCESSING_HOOKS': ['lms.lib.spectacular.lms_api_filter'], + # ADR 0038 / OEP-21: mark legacy slashless enrollment addresses + # deprecated (lms/lib/spectacular.py). The enum hook is drf-spectacular's + # default, restated because setting this key overrides the default list. + 'POSTPROCESSING_HOOKS': [ + 'drf_spectacular.hooks.postprocess_schema_enums', + 'lms.lib.spectacular.lms_mark_legacy_paths_deprecated', + ], 'SCHEMA_PATH_PREFIX': '/api/enrollment', 'SCHEMA_PATH_PREFIX_TRIM': '/api/enrollment', # SERVERS is environment-specific (LMS_ROOT_URL differs per env) and is diff --git a/lms/lib/spectacular.py b/lms/lib/spectacular.py index 433b05f6db11..a1a053b4190c 100644 --- a/lms/lib/spectacular.py +++ b/lms/lib/spectacular.py @@ -15,3 +15,24 @@ def lms_api_filter(endpoints): filtered.append((path, path_regex, method, callback)) return filtered + + +def lms_mark_legacy_paths_deprecated(result, generator, request, public): # pylint: disable=unused-argument + """ + Post-processing hook (ADR 0038 / OEP-21): mark the legacy slashless + Enrollment v2 addresses ``deprecated: true``. + + ADR 0038 rule 6 requires the trailing slash on every conforming route, so + within the migrated v2 surface a path without one is, by construction, a + legacy address whose slashed (or renamed) replacement is mounted beside + it. Scoped to ``/v2/`` so the marking tracks this migration — deprecating + v1 is its own DEPR decision. Paths appear here after + SCHEMA_PATH_PREFIX_TRIM strips /api/enrollment. + """ + for path, path_item in result.get("paths", {}).items(): + if not path.startswith("/v2/") or path.endswith("/"): + continue + for operation in path_item.values(): + if isinstance(operation, dict): + operation["deprecated"] = True + return result diff --git a/lms/urls.py b/lms/urls.py index 0765504d4080..173f1d409baa 100644 --- a/lms/urls.py +++ b/lms/urls.py @@ -13,6 +13,7 @@ from drf_spectacular.views import SpectacularAPIView from edx_api_doc_tools import make_docs_urls from edx_django_utils.plugins import get_plugin_url_patterns +from edx_rest_framework_extensions.url_converters import register_url_converters from submissions import urls as submissions_urls from common.djangoapps.student import views as student_views @@ -53,6 +54,10 @@ from openedx.core.djangoapps.user_authn.views.login import redirect_to_lms_login from openedx.features.enterprise_support.api import enterprise_enabled +# Shared opaque-key path converters (ADR 0038): registered once per service, +# before any URL pattern that uses / . +register_url_converters() + RESET_COURSE_DEADLINES_NAME = 'reset_course_deadlines' RENDER_XBLOCK_NAME = 'render_xblock' RENDER_VIDEO_XBLOCK_NAME = 'render_public_video_xblock' diff --git a/openedx/core/djangoapps/enrollments/v2/tests/test_views.py b/openedx/core/djangoapps/enrollments/v2/tests/test_views.py index 414aa8075926..675553fc8c66 100644 --- a/openedx/core/djangoapps/enrollments/v2/tests/test_views.py +++ b/openedx/core/djangoapps/enrollments/v2/tests/test_views.py @@ -13,7 +13,7 @@ from unittest.mock import patch from django.test import override_settings -from django.urls import reverse +from django.urls import resolve, reverse from rest_framework import status from rest_framework.test import APITestCase @@ -213,7 +213,9 @@ class TestUserRolesViewAliases(APITestCase): def setUp(self): super().setUp() self.user = UserFactory.create(password="test") - self.url = reverse("v2:enrollment-v2-roles") + # Renamed from the versioned kebab-case ``enrollment-v2-roles`` + # (ADR 0038; the path is unchanged). + self.url = reverse("v2:user_roles") @patch("openedx.core.djangoapps.enrollments.v2.views.api.get_user_roles", return_value=[]) def test_new_course_key_param_no_header(self, mock_get): # noqa: ARG002 @@ -287,3 +289,85 @@ def test_minimal_view_collapses_course_details_to_course_id(self, mock_list, moc assert {r["course_id"] for r in response.data["results"]} == { "course-v1:org+a+r", "course-v1:org+b+r", } + + +# --------------------------------------------------------------------------- +# ADR 0038 — URL-structure tests +# --------------------------------------------------------------------------- + +@skip_unless_lms +class TestEnrollmentUrlStructure(APITestCase): + """ + ADR 0038 — conforming trailing-slash routes with snake_case URL names, + mounted beside the legacy slashless routes, which keep their names and + serve the same views. + """ + + USERNAME = "someone" + COURSE_ID = "course-v1:org+course+run" + + def test_conforming_urls_reverse_to_expected_paths(self): + assert reverse("v2:enrollment_admin_list") == "/api/enrollment/v2/enrollments/" + assert reverse( + "v2:enrollment_detail", + kwargs={"username": self.USERNAME, "course_id": self.COURSE_ID}, + ) == f"/api/enrollment/v2/enrollments/{self.USERNAME},{self.COURSE_ID}/" + assert reverse( + "v2:course_enrollment_detail", kwargs={"course_id": self.COURSE_ID}, + ) == f"/api/enrollment/v2/courses/{self.COURSE_ID}/" + assert reverse("v2:user_roles") == "/api/enrollment/v2/roles/" + + def test_conforming_and_legacy_routes_share_views(self): + pairs = ( + # (conforming path, legacy path) + ("/api/enrollment/v2/enrollments/", "/api/enrollment/v2/enrollments"), + ( + f"/api/enrollment/v2/enrollments/{self.USERNAME},{self.COURSE_ID}/", + f"/api/enrollment/v2/enrollment/{self.USERNAME},{self.COURSE_ID}", + ), + ( + f"/api/enrollment/v2/courses/{self.COURSE_ID}/", + f"/api/enrollment/v2/course/{self.COURSE_ID}", + ), + ) + for conforming, legacy in pairs: + assert resolve(conforming).func.cls is resolve(legacy).func.cls, ( + f"{conforming} must serve the same view as {legacy}" + ) + + def test_legacy_admin_list_optional_slash_coverage_is_preserved(self): + """ + The legacy ``^enrollments/?$`` optional-slash pattern is split into a + conforming slashed route plus a slashless legacy route: both + addresses still resolve, one route each. + """ + slashless = resolve("/api/enrollment/v2/enrollments") + slashed = resolve("/api/enrollment/v2/enrollments/") + assert slashless.func.cls is slashed.func.cls + assert slashless.url_name == "enrollment-v2-admin-list" + assert slashed.url_name == "enrollment_admin_list" + + def test_legacy_retrieve_routes_have_unique_names(self): + """ + The two legacy retrieve forms no longer share one URL name (Django + disambiguated them only by argument signature). + """ + composite = resolve( + f"/api/enrollment/v2/enrollment/{self.USERNAME},{self.COURSE_ID}" + ) + course_only = resolve(f"/api/enrollment/v2/enrollment/{self.COURSE_ID}") + assert composite.func.cls is course_only.func.cls + assert composite.url_name == "enrollment-v2-retrieve" + assert course_only.url_name == "enrollment-v2-retrieve-own" + + def test_invalid_course_key_is_404_on_conforming_route(self): + # The shared course_key converter rejects unparseable keys with a + # routing-level 404. + response = self.client.get("/api/enrollment/v2/courses/not-a-course-key/") + assert response.status_code == status.HTTP_404_NOT_FOUND + + def test_admin_list_contract_is_identical_on_both_addresses(self): + """Unauthenticated callers get the same 401 on legacy and conforming.""" + legacy = self.client.get("/api/enrollment/v2/enrollments") + conforming = self.client.get("/api/enrollment/v2/enrollments/") + assert legacy.status_code == conforming.status_code == status.HTTP_401_UNAUTHORIZED diff --git a/openedx/core/djangoapps/enrollments/v2/urls.py b/openedx/core/djangoapps/enrollments/v2/urls.py index cda839fd4319..174369c153c8 100644 --- a/openedx/core/djangoapps/enrollments/v2/urls.py +++ b/openedx/core/djangoapps/enrollments/v2/urls.py @@ -10,6 +10,20 @@ they remain as standalone ``APIView`` classes routed via ``path()`` / ``re_path()``. +ADR 0038 — the API name and version position already conform. The conforming +routes below fix the remaining rule 6 violations (a required trailing slash; +no optional-slash patterns) and rule 11 violations (``snake_case``, +version-free, unique URL names), and are dual-mounted (OEP-21) beside the +legacy slashless routes, which keep their original names and are marked +``deprecated: true`` in the OpenAPI schema (``lms/lib/spectacular.py``). +Conforming member routes live under the plural ``enrollments/`` and +``courses/`` collections (rule 2), with course keys resolved by the shared +``course_key`` converter (rule 9), which rejects deprecated ``Org/Course/Run`` +keys. Deeper ADR 0038 targets — collapsing the singular ``enrollment/`` +collection into ``enrollments/``, replacing ``unenroll`` (a verb, rule 10) +with ``DELETE`` on the member address, and addressing the requesting user as +``me`` — are contract changes and belong to a future v3 per ADR 0037. + URL surface ----------- @@ -21,12 +35,17 @@ POST /enrollment/enrollment_allowed/ DELETE /enrollment/enrollment_allowed/ -Explicit paths: +Conforming explicit paths (ADR 0038): + GET /enrollments/ (name: enrollment_admin_list) + GET /enrollments/{username},{course_key}/ (name: enrollment_detail) + GET /courses/{course_key}/ (name: course_enrollment_detail) + GET /roles/ (name: user_roles) + +Legacy paths (deprecated, kept for their OEP-21 window): GET /enrollment/{username},{course_key} (name: enrollment-v2-retrieve) - GET /enrollment/{course_key} (name: enrollment-v2-retrieve) - GET /enrollments/ (name: enrollment-v2-admin-list) + GET /enrollment/{course_key} (name: enrollment-v2-retrieve-own) + GET /enrollments (name: enrollment-v2-admin-list) GET /course/{course_key} (name: enrollment-v2-course-detail) - GET /roles/ (name: enrollment-v2-roles) """ from django.conf import settings @@ -46,7 +65,36 @@ router = DefaultRouter() router.register(r"enrollment", EnrollmentViewSet, basename="enrollment") -urlpatterns = router.urls + [ +urlpatterns = [ + *router.urls, + # -- Conforming routes (ADR 0038: required trailing slash, plural + # -- collections, snake_case version-free names, shared key converter). + path( + "enrollments/", + EnrollmentsAdminListView.as_view(), + name="enrollment_admin_list", + ), + path( + "enrollments/,/", + EnrollmentRetrieveView.as_view(), + name="enrollment_detail", + ), + path( + "courses//", + CourseEnrollmentDetailView.as_view(), + name="course_enrollment_detail", + ), + path("roles/", UserRolesView.as_view(), name="user_roles"), + # -- Legacy routes (OEP-21 deprecation window; ADR 0038 rule 6 + # -- violations frozen as-is, marked deprecated in the OpenAPI schema). + # -- The admin list's optional-slash pattern is narrowed to slashless + # -- only: the slashed address is now served by the conforming route + # -- above, so every address that resolved before still resolves. + re_path( + r"^enrollments$", + EnrollmentsAdminListView.as_view(), + name="enrollment-v2-admin-list", + ), re_path( r"^enrollment/{username},{course_key}$".format( # noqa: UP032 username=settings.USERNAME_PATTERN, course_key=settings.COURSE_ID_PATTERN, @@ -57,17 +105,15 @@ re_path( rf"^enrollment/{settings.COURSE_ID_PATTERN}$", EnrollmentRetrieveView.as_view(), - name="enrollment-v2-retrieve", - ), - re_path( - r"^enrollments/?$", - EnrollmentsAdminListView.as_view(), - name="enrollment-v2-admin-list", + # Previously this route shared the name ``enrollment-v2-retrieve`` + # with the composite-key form above, resolving only because Django + # disambiguates by argument signature (the fragility ADR 0038 rule 11 + # calls out). Nothing reverses it, so it gets its own name. + name="enrollment-v2-retrieve-own", ), re_path( rf"^course/{settings.COURSE_ID_PATTERN}$", CourseEnrollmentDetailView.as_view(), name="enrollment-v2-course-detail", ), - path("roles/", UserRolesView.as_view(), name="enrollment-v2-roles"), ] diff --git a/openedx/core/djangoapps/enrollments/v2/views.py b/openedx/core/djangoapps/enrollments/v2/views.py index 5979b0aa92f7..aa55fbfd6ef1 100644 --- a/openedx/core/djangoapps/enrollments/v2/views.py +++ b/openedx/core/djangoapps/enrollments/v2/views.py @@ -468,6 +468,13 @@ def get(self, request, course_id=None, username=None): ``has_api_key`` or staff privileges raises ``NotFound`` (so the caller cannot probe for the existence of other users' enrollments). """ + # ADR 0038: the conforming /enrollments/{username},{course_key}/ + # route passes a parsed CourseKey (shared ``course_key`` converter); + # the legacy slashless routes pass the raw string. Coerce to the + # string form the body below expects. + if course_id is not None and not isinstance(course_id, str): + course_id = str(course_id) + if username is None: username = request.user.username @@ -610,6 +617,11 @@ def get(self, request, course_id=None): course schedule and supported enrollment modes; pass ``?include_expired=1`` to include expired enrollment modes. """ + # ADR 0038: the conforming /courses/{course_key}/ route passes a + # parsed CourseKey (shared ``course_key`` converter); the legacy + # slashless /course/{course_key} route passes the raw string. + if course_id is not None and not isinstance(course_id, str): + course_id = str(course_id) try: course_key = CourseKey.from_string(course_id) except InvalidKeyError as exc: diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 74dcae4d3d8c..8fd1a45ca988 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -467,7 +467,7 @@ edx-django-utils==8.0.2 # ora2 # super-csv # xblocks-contrib -edx-drf-extensions==10.7.0 +edx-drf-extensions==10.8.0 # via # edx-completion # edx-enterprise diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 37e0d79208db..352848bce529 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -520,7 +520,7 @@ edx-django-utils==8.0.2 # ora2 # super-csv # xblocks-contrib -edx-drf-extensions==10.7.0 +edx-drf-extensions==10.8.0 # via # edx-completion # edx-enterprise diff --git a/uv.lock b/uv.lock index 310e9c56fb20..7bdde241397c 100644 --- a/uv.lock +++ b/uv.lock @@ -2023,7 +2023,7 @@ wheels = [ [[package]] name = "edx-drf-extensions" -version = "10.7.0" +version = "10.8.0" source = { registry = "https://pypi.org/simple" } dependencies = [ { name = "django", version = "4.2.30", source = { registry = "https://pypi.org/simple" }, marker = "extra == 'group-16-openedx-platform-django42'" }, @@ -2037,9 +2037,9 @@ dependencies = [ { name = "requests" }, { name = "semantic-version" }, ] -sdist = { url = "https://files.pythonhosted.org/packages/50/06/b32d6d48415d9278c188a70da786796715ce2c4e73178f114fd498108052/edx_drf_extensions-10.7.0.tar.gz", hash = "sha256:784710bf9dc77e4234d201295963c20fd15b4e27595f1c1587b180a79e0914d4", size = 80429, upload-time = "2026-08-18T15:32:48.976Z" } +sdist = { url = "https://files.pythonhosted.org/packages/03/ce/7b348f25bb9a975171166904abe740a026ce7c6dca10e3827c927aab36cb/edx_drf_extensions-10.8.0.tar.gz", hash = "sha256:f7a6d1d0a4cfdec7c95635b0f3eb427cc473f28428bfabe3c4e3db0095feb6da", size = 99711, upload-time = "2026-09-02T20:47:28.28Z" } wheels = [ - { url = "https://files.pythonhosted.org/packages/e8/e6/03aa9fc1de473702887657c0e165798c99dee32c77a6eca5a4b2f4d8809d/edx_drf_extensions-10.7.0-py2.py3-none-any.whl", hash = "sha256:c1931816a88ac60908051e28ecb6fd18ba97cc3d6f18d61360d8eb22d3203886", size = 79474, upload-time = "2026-08-18T15:32:47.815Z" }, + { url = "https://files.pythonhosted.org/packages/ff/cd/7db09d26bb1c01ecef32d0762207930c900caf689d9bdc635c31406d4488/edx_drf_extensions-10.8.0-py2.py3-none-any.whl", hash = "sha256:853892aaba931315e82a3d3cf3cab57c5fe105f8d1760d70a45455b6e995e33b", size = 102718, upload-time = "2026-09-02T20:47:26.99Z" }, ] [[package]]