feat: apply ADR 0038 (URL structure standardization) across 6 standardized APIs - #39078
Abdul-Muqadim-Arbisoft wants to merge 12 commits into
Conversation
…ons (ADR 0038) Register the CourseKeyConverter / UsageKeyConverter path converters — added to edx-drf-extensions 10.8.0 (openedx/edx-drf-extensions#573) per ADR 0038's 'Code examples' section — once per service in lms/urls.py and cms/urls.py, as <course_key:...> / <usage_key:...>. ADR 0038 rule 9: conforming routes resolve opaque keys in the URLconf, views receive parsed keys, and malformed or deprecated (Org/Course/Run, i4x://) keys become routing-level 404s. Bumps edx-drf-extensions 10.7.0 -> 10.8.0, the release that adds the converters (plus the ADR 0029/0032/0036 building blocks this API series already consumes). Converter unit tests live in the library; the per-API URL tests in the following commits cover resolve/reverse integration through the real routes.
0f1ccfa to
4ef11af
Compare
Mount the conforming routes beside the legacy /api/contentstore/v1/xblock/ ones (OEP-21), serving the same XblockViewSet: the collection becomes plural (rule 2), the API name describes the domain rather than the implementing Django app (rule 3), the usage key is resolved by the shared usage_key converter, which turns malformed and deprecated i4x:// keys into routing-level 404s (rule 9), and URL names are snake_case, version-free, and unique (rule 11). The viewset's initial() coerces a parsed UsageKey back to the string form the action methods expect, so both mounts share one contract. The legacy routes stay live for their deprecation window and are marked deprecated: true in the OpenAPI schema via the new cms_mark_migrated_paths post-processing hook; cms_api_filter now also admits /api/authoring/ paths. Tests pin reverse() literals, same-view resolution for both mounts, the routing-level 404, and handler parity on the conforming routes. ADR 0038 (implementation note 4) asks that /api/authoring/v1/xblocks/ be reconciled with the 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.
…ernal BFF) Mount the conforming home/, home/courses/, and home/libraries/ routes at /api/authoring/v3/ beside the legacy /api/contentstore/v3/home/ ones (OEP-21), serving the same HomeViewSet, with snake_case version-free URL names (rule 11) and the domain-named api_name (rule 3). home is a BFF aggregate for the Studio home screen. Rule 4 disfavors screen names as resources, but the ADR's BFF provision applies: the surface keeps the /api/ prefix and one canonical conforming mount, and is marked x-internal in the OpenAPI schema — on both mounts — so clients can tell it apart from a stable resource contract. The legacy routes are additionally marked deprecated: true. Tests pin reverse() literals, same-view resolution for all three action pairs, and the ADR 0029 envelope on the conforming mount.
Mount the conforming courses/ collection at /api/authoring/v4/ beside the legacy /api/contentstore/v4/home/courses/ route (OEP-21), serving the same HomeCoursesViewSet: the screen-shaped home/courses/ address becomes the concrete plural collection of authorable courses (rule 4), filtered, sorted, and paginated in the query string, under the domain-named api_name (rule 3) with a snake_case version-free URL name (rule 11). The legacy route stays live for its deprecation window and is marked deprecated: true in the OpenAPI schema. Tests pin the reverse() literal, same-view resolution for both mounts, and 401/200 contract parity on the conforming mount.
Mount the conforming /api/authoring/v3/courses/{course_key}/details/ route beside the legacy /api/contentstore/v3/course_details/{course_id}/ one (OEP-21), serving the same CourseDetailsViewSet: the screen-shaped collection becomes a sub-resource of the plural courses/ collection, one level deep — the ADR's own target for these endpoints (rules 4 and 8) — with the course key resolved by the shared course_key converter, which turns malformed and deprecated Org/Course/Run keys into routing-level 404s (rule 9).
resolve_course_key() now also accepts an already-parsed CourseKey, so both mounts funnel through one code path and share one contract. The legacy route stays live for its deprecation window and is marked deprecated: true in the OpenAPI schema. Tests pin the reverse() literal, same-view resolution, the routing-level 404, and 401/403 parity on the conforming mount.
Mount the conforming /api/authoring/v3/courses/{course_key}/grading/ route beside the legacy /api/contentstore/v3/authoring_grading/{course_key}/ one (OEP-21), serving the same AuthoringGradingViewSet: the app-flavored authoring_grading collection becomes the grading sub-resource of the plural courses/ collection, one level deep (rules 3, 4 and 8) — the authoring_ prefix is dropped because the namespace already says it — with the course key resolved by the shared course_key converter (rule 9). Both mounts funnel through resolve_course_key(), which already accepts parsed keys, so they share one contract.
The legacy route stays live for its deprecation window and is marked deprecated: true in the OpenAPI schema. Tests pin the reverse() literal, same-view resolution, the routing-level 404, and 401/200 PATCH parity on the conforming mount.
…nforming URL names)
/api/enrollment/v2/ already conforms in API name and version position; this fixes the remaining rule 6 and rule 11 violations. Conforming routes are dual-mounted (OEP-21) beside the legacy slashless ones, serving the same views: GET /enrollments/ (the admin list's optional-slash pattern — the ADR's own rule 6 example — is split into an exact slashed route plus a slashless legacy route, so every address that resolved before still resolves), GET /enrollments/{username},{course_key}/ under the plural collection (rule 2), GET /courses/{course_key}/ (plural, slashed), and roles/ renamed from the versioned kebab-case enrollment-v2-roles to user_roles (rule 11; path unchanged). Conforming member routes resolve course keys with the shared course_key converter (rule 9); the views coerce a parsed CourseKey back to the string form their bodies expect, so both mounts share one contract.
The two legacy retrieve forms no longer share one URL name — Django resolved that only by argument signature, the fragility rule 11 calls out — and the slashless legacy addresses are marked deprecated: true in the OpenAPI schema via a post-processing hook scoped to /v2/ (deprecating v1 is its own DEPR decision). Deeper ADR 0038 targets — collapsing the singular enrollment/ collection into enrollments/, replacing the unenroll verb (rule 10) with DELETE on the member address, and addressing the requesting user as me (rule 9) — are contract changes and belong to a future v3 per ADR 0037.
Tests pin the reverse() literals, same-view resolution for every legacy/conforming pair, the optional-slash coverage split, the unique legacy names, the routing-level 404, and 401 parity on both admin-list addresses.
4ef11af to
1ad2f0e
Compare
…igration Review feedback: the new authoring_urls.py module docstrings restated ADR 0038's rules rather than describing the module, and several comments repeated what the commits already say. The three urls.py modules now carry a one-line docstring matching their siblings in the same package, the per-rule conformance lists and the "same view, same contract" notes are gone, and the test-class docstrings, Enrollment v2 docstring, and spectacular helpers are trimmed; comments that prevent a mistake are kept but shortened (conforming routes pass a parsed CourseKey/UsageKey where legacy routes pass the raw string; POSTPROCESSING_HOOKS replaces rather than extends drf-spectacular's default list). Prose only — with docstrings stripped, all 19 files parse to ASTs identical to the previous revision, ruff passes, and no view or serializer docstring is touched.
dc84ca6 to
1677cb5
Compare
Review feedback on openedx#39037: the code should follow the standards without naming the documents. The three authoring urls.py modules now read "Authoring API vN URLs.", the URL-structure test banners and the Enrollment v2 module docstring lose their rule references, and the remaining comments say what the code does instead — deprecation window rather than OEP-21 window, "whose course_key path converter hands views a parsed key" rather than a rule number. Only lines this branch adds are touched; the pre-existing ADR references elsewhere in these files are left alone, the one exception being the ADR 0028 line in the Enrollment v2 module docstring, which this branch was already rewriting. Prose only apart from one assert message in the new URL-structure test, which now reads "missing error-envelope field". ruff passes.
| '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', |
There was a problem hiding this comment.
What's the reasoning for the trimming here? Won't this result in incorrect top level URLs being output in the api spec?
There was a problem hiding this comment.
I just pushed the fix, by the way trimming came from the 2023 API-gateway work (#33694) while this PR made it worse by adding /api/authoring/ paths that the single prefix couldn't trim, so basically it was producing wrong URLs. The trim only worked if a client picked the one server that re-adds /api/contentstore, the LMS had copied it with no compensating server at all, and this PR's /api/authoring/ paths couldn't be trimmed by the single prefix anyway.
…refix SCHEMA_PATH_PREFIX_TRIM is a boolean that we had set to a string in three places; it only worked because the string was truthy. More importantly the trimming itself produced wrong URLs: the LMS spec advertised /v2/enrollments/ against a bare LMS_ROOT_URL server, and adding /api/authoring/ to the CMS spec left those paths untrimmed beside trimmed contentstore ones. Widening the prefix regex would collapse /api/contentstore/v3/home/ and /api/authoring/v3/home/ onto one key. Drop the trim, drop the CMS-contentstore server that existed only to compensate, keep SCHEMA_PATH_PREFIX for tag extraction, and update both post-processing hooks to full paths. Adds tests for the hooks, which had none.
Resolves conflicts with openedx#39037 (queryset scoping) and the edx-drf-extensions 10.9.0 bump on master. Keeps both new Enrollment v2 test classes; takes master's requirements and uv.lock, which supersede this branch's 10.8.0 bump so the branch no longer changes any requirements file.
| return filtered | ||
|
|
||
|
|
||
| def cms_mark_migrated_paths(result, generator, request, public): # pylint: disable=unused-argument |
There was a problem hiding this comment.
Both mounts strip to the same tokens under SCHEMA_PATH_PREFIX, so drf-spectacular derives one operationId per pair and breaks the tie with a numeral suffix. Generating the CMS schema warns three times, and the suffix lands on the legacy path:
/api/contentstore/v3/home/ v3_home_retrieve -> v3_home_retrieve_2
/api/contentstore/v3/home/courses/ v3_home_courses_retrieve -> v3_home_courses_retrieve_2
/api/contentstore/v3/home/libraries/ v3_home_libraries_retrieve -> v3_home_libraries_retrieve_2
These are the only operationId changes in the CMS schema, and operationId is what the SDK names its modules after. edly-io/openedx-platform-sdk has openedx_platform_sdk/api/openedx_platform_sdk/v3_home_retrieve.py pointing at /v3/home/ today, so regenerating after this lands moves that name onto the new address and gives the old one _2.
Set explicit operation ids on the dual mounts so which name goes where is a decision, not registration order.
There was a problem hiding this comment.
Agree, Fixed with an AutoSchema subclass per service: legacy addresses get _legacy, conforming ones keep the clean id, so a regenerated SDK follows the existing function name onto the non-deprecated address and nothing renames again when legacy goes.
| return filtered | ||
|
|
||
|
|
||
| def lms_mark_legacy_paths_deprecated(result, generator, request, public): # pylint: disable=unused-argument |
There was a problem hiding this comment.
Same operationId collision here, landing the other way round. /api/enrollment/v2/enrollments, the legacy address this hook marks deprecated, keeps v2_enrollments_list, and the conforming /api/enrollment/v2/enrollments/ becomes v2_enrollments_list_2. The SDK already ships v2_enrollments_list.py, so regenerating keeps its users on the deprecated address under the clean name.
Give the conforming route an explicit operation id.
There was a problem hiding this comment.
Same fix, same rule. /enrollments/ keeps v2_enrollments_list, the slashless legacy becomes v2_enrollments_list_legacy.
| CourseEnrollmentDetailView.as_view(), | ||
| name="enrollment-v2-course-detail", | ||
| ), | ||
| path("roles/", UserRolesView.as_view(), name="enrollment-v2-roles"), |
There was a problem hiding this comment.
reverse("v2:enrollment-v2-roles") and reverse("v2:enrollment-v2-retrieve", kwargs={"course_id": ...}) both work on master and raise NoReverseMatch here. Addresses are all preserved, but these two names are not. Nothing in-tree reverses them; out-of-tree plugins can.
These two are the whole of it. I diffed the name-to-pattern mapping for both services and nothing else changed: CMS adds 8 names and touches none, and enrollment-v2-admin-list moves from ^enrollments/?$ to ^enrollments$ but reverses to /api/enrollment/v2/enrollments on both, so it's fine.
Keep both as aliases for the deprecation window. Registering the same path a second time under the old name keeps reverse() working and leaves resolution on the first entry. If you'd rather drop them, say in the description that URL names change, since it currently reads as though nothing is removed.
There was a problem hiding this comment.
You're right, I dropped those. Added both back as aliases.
…iew actions Review feedback on the dual mounts. A legacy mount and its conforming mount serve the same view and tokenize to the same operationId, so drf-spectacular was breaking the tie with a numeral suffix in registration order: the CMS schema put _2 on the legacy path and the LMS schema put it on the conforming one, so regenerating the SDK would have renamed modules by accident. Each service now has an AutoSchema subclass wired in as DEFAULT_SCHEMA_CLASS that suffixes the legacy address with _legacy and leaves the conforming address with the clean id, so a regenerated SDK follows the same function name onto the non-deprecated address. The two Studio home viewsets carry their own schema instances, which bypass the default, so those are rebased onto the CMS class as well; tests pin the ids for each pair, assert the default wiring, and assert the per-view schemas chain the CMS class. The LMS legacy predicate is shared between the schema class and the deprecation hook. Two URL names had been dropped although every address was kept: enrollment-v2-roles and the course-only form of enrollment-v2-retrieve. Both are registered again on their paths after the new names, so reverse() keeps working for out-of-tree callers and resolution stays on the first entry. The route-sharing tests compared .func.cls, which is the same object for a ViewSet whatever actions a mount wires up; they now compare .func.actions as well, and the v1 test covers the list route too.
…gration The open URL-structure work (openedx#39078) reaches the same schema conclusion this branch did — paths published in full, the service-prefixed server entry dropped — but expresses it differently. Adopt its shapes so the two merge mechanically instead of conflicting: - cms/lib/spectacular.py becomes its module, with one added prefix for the v0 video upload addresses. Our separately named hook, its operation-key set and the shortened path form are gone; nothing trims any more, so the short form was dead. - SPECTACULAR_SETTINGS matches it byte for byte: SCHEMA_PATH_PREFIX_TRIM removed rather than set false, the prefix reduced to a tag-extraction regex covering both services, and SERVERS restored to the plain two-entry list. - The opaque-key converters register before autodiscovery, as they do there. - The authoring URLConf follows its layout. Its xblock routes are deliberately not taken; they belong to that change. Two consequences worth naming. Operation ids lose the service prefix, so the new endpoints become v1_courses_videos_* — they are new here, so nothing depends on the old spelling, and contentstore ids are untouched. And an unset public host is published as an empty server URL again, which is how the setting behaves on master; the guard against it belongs in its own change rather than hidden in this one.
| legacy = resolve(reverse(legacy_name)).func | ||
| conforming = resolve(reverse(conforming_name)).func | ||
| assert conforming.cls is legacy.cls, f"{conforming_name} must serve the same view as {legacy_name}" | ||
| assert conforming.actions == legacy.actions, f"{conforming_name} must map the same actions as {legacy_name}" |
There was a problem hiding this comment.
Sorry, my suggestion here was incomplete. DRF adds head to a mount's actions dict the first time it serves a GET (rest_framework/viewsets.py:106-107), and the error-shape tests above request the legacy URL first. This fails in cms-2, along with the v1 xblock and v3 course_details copies:
assert {'get': 'list'} == {'get': 'list', 'head': 'list'}
Drop head before comparing, in all five copies:
def _routed(func):
"""The mount's action map, minus the ``head`` entry DRF adds on its first request."""
return {method: action for method, action in func.actions.items() if method != "head"}With assert _routed(conforming) == _routed(legacy) the five files go from 3 failed / 102 passed to 105 passed.
| def get_operation_id(self): | ||
| """Suffix the legacy address so the pair never shares an operationId.""" | ||
| operation_id = super().get_operation_id() | ||
| if self.path.startswith(LEGACY_MIGRATED_PATH_PREFIXES): |
There was a problem hiding this comment.
This suffixes every legacy path, but only v3/home collided. The other legacy mounts already had operationIds distinct from their conforming twins (v1_xblock_retrieve vs v1_xblocks_retrieve), so against master this renames 9 that were never in conflict:
v1_xblock_{create,retrieve,update,partial_update,destroy} -> ..._legacy
v3_course_details_{retrieve,update} -> ..._legacy
v3_authoring_grading_partial_update -> ..._legacy
v4_home_courses_retrieve -> ..._legacy
The SDK has a module for each of those, so regenerating removes all nine instead of adding the new ones beside them. Limit the suffix to the pairs that collide:
COLLIDING_LEGACY_PATH_PREFIXES = ("/api/contentstore/v3/home/",)With that no operationId is removed relative to master and the collision warnings are still gone.
| def get_operation_id(self): | ||
| """Suffix the legacy address so the pair never shares an operationId.""" | ||
| operation_id = super().get_operation_id() | ||
| if _is_legacy_path(self.path): |
There was a problem hiding this comment.
Same here. Only /api/enrollment/v2/enrollments collided. v2_course_retrieve, v2_enrollment_retrieve and v2_enrollment_,_retrieve were already distinct and get renamed anyway.
| if _is_legacy_path(self.path): | |
| if self.path == "/api/enrollment/v2/enrollments": |
lms/lib/tests/test_spectacular.py:100 asserts v2_enrollment_retrieve_legacy and changes with it.
Summary
Applies ADR 0038 — Standardize REST API URL Structure (#39003) to the 6 APIs previously standardized under the FC-0118 effort, per the ADR's implementation note 4: "Migrate the APIs already standardized under FC-0118 first … each keeping its current version number."
Every migration follows the ADR's OEP-21 rule: the conforming path is mounted as an additional route to the same view, the legacy path stays live for its deprecation window and is marked
deprecated: truein the OpenAPI schema. No existing address changes behaviour and none is removed.Depends on
edx-drf-extensions==10.8.0, whose sharedcourse_key/usage_keypath converters (edx-drf-extensions#573) this PR consumes (ADR 0038 rule 9).<course_key:…>/<usage_key:…>per service/api/contentstore/v1/xblock/{usage_key}//api/authoring/v1/xblocks/{usage_key}//api/contentstore/v3/home/…/api/authoring/v3/home/…+x-internal/api/contentstore/v4/home/courses//api/authoring/v4/courses//api/contentstore/v3/course_details/{id}//api/authoring/v3/courses/{course_key}/details//api/contentstore/v3/authoring_grading/{id}//api/authoring/v3/courses/{course_key}/grading/snake_caseURL namesChanges
feat— shared opaque-key URL converters (commit 1)Registers the
CourseKeyConverter/UsageKeyConverterfrom edx-drf-extensions 10.8.0 once per service (lms/urls.py,cms/urls.py). Conforming routes resolve opaque keys in the URLconf, views receive parsed keys, and malformed or deprecated (Org/Course/Run,i4x://) keys become routing-level 404s. Bumpsedx-drf-extensions10.7.0 → 10.8.0.feat— the five CMS APIs (commits 2–6)New
authoring_urls.pymodules per version, mounted fromcms/urls.pyunder their fullapi/authoring/v{N}/prefix (rule 5), serving the same viewsets as the legacy mounts:api_name(authoring), not the implementing Django app (contentstore);course_details/authoring_gradingbecomecourses/{course_key}/details/and…/grading/(the ADR's own target), one level deep;home/courses/(v4) becomes the concrete pluralcourses/;homestays screen-named under the ADR's BFF provision: one canonical conforming mount, markedx-internalin the schema;snake_case, version-free, unique URL names.cms/lib/spectacular.pygains a post-processing hook marking the legacy addressesdeprecated: trueand the home BFFx-internal;cms_api_filternow also admits/api/authoring/paths.feat— Enrollment v2 (commit 7)/api/enrollment/v2/already conforms in name and version position; this fixes the remaining rule 6/11 violations, dual-mounted beside the legacy slashless routes:^enrollments/?$(optional slash — the ADR's rule 6 example)enrollments/(exact) + slashless legacy — every address that resolved before still resolvesenrollment/{username},{course_key}enrollments/{username},{course_key}/(plural collection, rule 2)course/{course_key}courses/{course_key}/enrollment-v2-rolesuser_roles(path unchanged)The two legacy retrieve forms no longer share one URL name (the argument-signature fragility rule 11 calls out). The LMS schema hook marks slashless
/v2/addresses deprecated — scoped to v2, since deprecating v1 is its own DEPR decision. Deeper targets (collapsing singularenrollment/,DELETEon the member address for unenroll,meaddressing) are contract changes that belong to a future v3 per ADR 0037.