Skip to content

[FC-0118] docs: add OEP-69 REST API Conventions - #805

Open
Abdul-Muqadim-Arbisoft wants to merge 1 commit into
openedx:masterfrom
edly-io:abdul-muqadim/oep-69-rest-api-conventions
Open

[FC-0118] docs: add OEP-69 REST API Conventions#805
Abdul-Muqadim-Arbisoft wants to merge 1 commit into
openedx:masterfrom
edly-io:abdul-muqadim/oep-69-rest-api-conventions

Conversation

@Abdul-Muqadim-Arbisoft

Copy link
Copy Markdown
Contributor

Consolidate the FC-0118 REST API standardization ADRs (openedx-platform docs/decisions/0025-0037, plus the 0017 authentication pointer ADR) into a single Best Practice OEP, as requested during community review of the ADRs.

The OEP captures 13 conventions covering DRF serializers, ViewSets, endpoint consolidation, JWT authentication, permission classes, idempotent GETs, pagination, filtering/sorting, response shaping, error responses, API documentation, versioning, and the canonical MFE configuration endpoint.

Refs: openedx/openedx-platform#38137
Discussion: https://discuss.openedx.org/t/request-for-community-review-on-adrs-for-standardizing-the-open-edx-platform-api-endpoints/18717

Consolidate the FC-0118 REST API standardization ADRs
(openedx-platform docs/decisions/0025-0037, plus the 0017
authentication pointer ADR) into a single Best Practice OEP, as
requested during community review of the ADRs.

The OEP captures 13 conventions covering DRF serializers, ViewSets,
endpoint consolidation, JWT authentication, permission classes,
idempotent GETs, pagination, filtering/sorting, response shaping,
error responses, API documentation, versioning, and the canonical
MFE configuration endpoint.

Refs: openedx/openedx-platform#38137
@Abdul-Muqadim-Arbisoft
Abdul-Muqadim-Arbisoft force-pushed the abdul-muqadim/oep-69-rest-api-conventions branch from 42c5d54 to ce038a9 Compare July 5, 2026 20:12

@bradenmacdonald bradenmacdonald left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is great! I left some comments inline.

One major omission (?) though seems to be URLs.

Our REST API URLs today follow no consistent patterns whatsoever:

/transcripts/upload
/xblock/:id/handler/:handlerName/
/xblock/container/:id/
/organizations/
/api/toggles/
/event/
/api/user/v1/accounts
/api/v1/course_runs/
/course_team/:course_id/
/course_rerun/:course_id/
/videos/:course_id/:video_id/
/api/learning_sequences/...

I think this OEP should clearly define URL expectations like:

  • Whether we should use singular nouns (/user/:x) or plural nouns (/users/:x)
  • That REST API URLs should always start with /api/
  • That new CMS and LMS URLs should not conflict with each other so they can potentially be merged without needing major changes to API clients other than domain name.
  • Whether APIs are organized hierarchically using logical components /api/course/:course_key/teams/:team_id/ or the URLs should strictly map to the underlying organization of the codebase /api/teams/v1/course/:course_id/:team_id or some other organizational scheme ?
  • etc.

Comment on lines +145 to +147
fields the caller cannot set. When they differ, define them separately and point
``serializer_class`` at the **output** serializer (used by ``drf-spectacular``
for schema generation).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But don't we also need the input serializer to be included in the schema too? From the example below, it looks like the API tooling would only be aware of the output schema, so half of the endpoint's spec is missing.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, serializer_class only drives the response schema, so as written the request body is missing/misinferred. This is actually already handled correctly in the merged pilots, e.g. the Course Detail v3 view uses @extend_schema(request=OpenApiRequest(request=CourseDetailsSerializer), responses={200: OpenApiResponse(...)}). The OEP example just under-specified it. I'll fix the example to declare both explicitly and add a sentence: when request and response differ, both MUST be registered via @extend_schema(request=..., responses=...); serializer_class is only the fallback.

Comment on lines +197 to +213
*Source: ADR 0031, Merge Similar Endpoints.*

Groups of endpoints that operate on the same resource and differ only by the
operation applied **MUST** be consolidated into a single parameterised view (or shared
service layer) rather than proliferating narrow, action-scoped URLs.

* Expose a single URL per resource group, distinguishing the operation with an
``action``/``mode`` field (or HTTP verbs where REST conventions apply cleanly).
* Shared validation, audit logging, response shaping, and permission enforcement
**MUST** move into a common service layer or mixin. Consolidation removes
duplicated boilerplate; it **MUST NOT** flatten the authorization model: the view
performs a coarse access check and each operation handler enforces its own
specific permission.
* The unified endpoint **MUST** document its enumerated ``action``/``mode`` values
and their request/response shapes in OpenAPI.
* Legacy URLs **MUST** be preserved as deprecated aliases (emitting a
``Deprecation`` header) for a defined transition window.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole section is a bit vague and confusing to me. At first I thought it was saying that if you have two different HTTP verbs for a resource, like POST /users/5/profile/ and GET /users/5/profile/, that you have to implement them with a single function. ("endpoints that operate on the same resource and differ only by the operation applied... MUST be consolidated into a single parameterised view"). That would be messy!

At the very least, this needs some examples. But from looking at the ADR, I see it only lists one known example: "Relevance in edx-platform: Certificate views".

If this is just about cleaning up that one poorly-organized endpoint, I don't think it rises to the level of needing to be included as a section of this OEP. If you do think it's important enough to leave in, it needs to be clarified and given examples.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree this is bit unclear, and you've identified the real issue: it's the thinnest section. It's specifically about RPC-style action-named endpoints (the certificate enable_/start_/regenerate cluster → one certificate_tasks resource with a mode), not about how HTTP verbs on a resource are implemented (that's the ViewSet convention). Also worth noting: unlike the other conventions, this one has no merged implementation yet, certificates remain the only example. Given that, I'll (a) reframe it narrowly as "avoid proliferating action-scoped URLs," (b) add the concrete before/after, (c) explicitly clarify it does not mean collapsing verbs into one handler, and (d) soften MUST→SHOULD. your call.

Comment on lines +565 to +574
The reference implementation is the ongoing FC-0118 work in ``openedx-platform``,
tracked under the umbrella issue `#38137
<https://github.com/openedx/openedx-platform/issues/38137>`_ and recorded as ADRs
``docs/decisions/0025`` through ``0037`` (plus the pointer ADR
``openedx/core/djangoapps/oauth_dispatch/docs/decisions/0017``). Priority
migration targets identified by the ADRs include the Enrollment API ViewSet
consolidation, the ``drf-spectacular`` rollout, the ``BearerAuthentication``
deprecation via the ``view_auth_classes`` decorator, and the canonical
front-end configuration endpoint. This OEP may move to "Final" once the conventions
have representative, merged implementations across the highest-impact endpoints.

@bradenmacdonald bradenmacdonald Jul 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the reference implementation be in a single library, and outside of openedx-platform?

e.g. openedx_rest_lib could provide DefaultPagination, permission_classes, django-filter, drf-spectacular, Error-response standardization, CI tooling, and any of the other requirements specified in this OEP. So then following this OEP would be mostly about strictly using openedx_rest_lib.

I would personally make openedx_rest_lib re-export all of the DRF API as well, so that consumers can enjoy a much cleaner import story:

Today 🤢

from drf_spectacular.utils import (
    OpenApiParameter,
    OpenApiRequest,
    OpenApiResponse,
    extend_schema,
)
from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication
from edx_rest_framework_extensions.paginators import DefaultPagination
from rest_framework import permissions, status, viewsets
from rest_framework.decorators import action
from rest_framework.exceptions import NotFound, ValidationError
from rest_framework.generics import ListAPIView
from rest_framework.response import Response
from rest_framework.views import APIView
from openedx.core.lib.api.mixins import StandardizedErrorMixin
from openedx.core.lib.api.permissions import ApiKeyHeaderPermissionIsAuthenticated

Tomorrow? 🌈

from openedx_rest_lib import (
    OpenApiParameter,
    OpenApiRequest,
    OpenApiResponse,
    extend_schema,
    StandardViewSet,  # Has DefaultPagination, JwtAuthentication, StandardizedErrorMixin, etc.
    ListAPIView,  # Has DefaultPagination, JwtAuthentication, StandardizedErrorMixin, etc.
    APIView,  # Has DefaultPagination, JwtAuthentication, StandardizedErrorMixin, etc.
    rest_exceptions,  # DRF NotFound, ValidationError, etc. (standardized)
    permissions,
    Response,
)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea. The pieces already exist but are split: DefaultPagination/JwtAuthentication are in edx-drf-extensions (a library, reusable), but StandardizedErrorMixin + the ADR-0029 handler live in openedx.core.lib.api inside the platform, and each pilot app defines its own permission classes (e.g. HasCourseAuthorAccess in the xblock PR). So plugins and other IDAs can't reuse the standardized bits today, exactly the problem a shared library solves. edx-drf-extensions looks like the natural home since it already ships pagination + JWT.

One caveat from the current implementation: the ADR-0029 handler currently imports ignored_error_exception_handler from openedx.core.lib.request_utils, so it's coupled to platform internals, extracting it means breaking that coupling first, not a straight move. On the re-export idea: my only caution is keep-in-sync cost, so I'd lean toward a curated set + a StandardViewSet/StandardAPIView base (pre-wiring pagination, JWT, and the error mixin) rather than a 1:1 re-export of all of DRF.

Since this is the same shape as the URL point, a design decision best captured as its own ADR + implementation rather than folded into this OEP, and FC-0118 is almost at close: @feanil , could we get your consensus on whether to add another ADR (extract a reusable REST library into edx-drf-extensions) under FC-0118 and start migrating the pilot endpoints onto it now, or track it as a documented follow-up? We'll then update OEP-69's Reference Implementation section to point at the library as the primary deliverable.

@Abdul-Muqadim-Arbisoft

Abdul-Muqadim-Arbisoft commented Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

Thanks, this is great! I left some comments inline.

One major omission (?) though seems to be URLs.

Our REST API URLs today follow no consistent patterns whatsoever:

/transcripts/upload
/xblock/:id/handler/:handlerName/
/xblock/container/:id/
/organizations/
/api/toggles/
/event/
/api/user/v1/accounts
/api/v1/course_runs/
/course_team/:course_id/
/course_rerun/:course_id/
/videos/:course_id/:video_id/
/api/learning_sequences/...

I think this OEP should clearly define URL expectations like:

  • Whether we should use singular nouns (/user/:x) or plural nouns (/users/:x)
  • That REST API URLs should always start with /api/
  • That new CMS and LMS URLs should not conflict with each other so they can potentially be merged without needing major changes to API clients other than domain name.
  • Whether APIs are organized hierarchically using logical components /api/course/:course_key/teams/:team_id/ or the URLs should strictly map to the underlying organization of the codebase /api/teams/v1/course/:course_id/:team_id or some other organizational scheme ?
  • etc.

This is a great point. The OEP already fixes URL versioning (Convention 12 mandates /api//vN/), but you're right that it says nothing about URL naming and organization and none of the source ADRs (0025–0037) did either. So this is worth pinning down. It would cover the items you listed:

  • REST URLs always under /api/
  • singular vs plural nouns (/user/:x vs /users/:x)
  • CMS and LMS not defining conflicting paths for the same logical resource, so they can be merged behind one domain without breaking clients
  • hierarchical/logical organization (/api/courses/:course_key/teams/:team_id/) vs. URLs mapping to the codebase layout (/api/teams/v1/course/:course_id/:team_id), or another scheme
    Given how the rest of this work was done (ADR first, then consolidated here), I'd suggest capturing this as a new ADR and validating it against the FC-0118 endpoints as we standardize them, then summarizing the outcome back into OEP-69 so it's still defined here. The one catch is scope: FC-0118 is almost at close.

@feanil could we get your consensus on whether it makes sense to add one more ADR (URL conventions) under FC-0118 and implement it against the selected endpoints now, given we're near close, or capture it as a documented follow-up to pick up separately?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants