[FC-0118] docs: add OEP-69 REST API Conventions#805
[FC-0118] docs: add OEP-69 REST API Conventions#805Abdul-Muqadim-Arbisoft wants to merge 1 commit into
Conversation
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
42c5d54 to
ce038a9
Compare
bradenmacdonald
left a comment
There was a problem hiding this comment.
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_idor some other organizational scheme ? - etc.
| 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). |
There was a problem hiding this comment.
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.
| *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. |
There was a problem hiding this comment.
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.
| 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. |
There was a problem hiding this comment.
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 ApiKeyHeaderPermissionIsAuthenticatedTomorrow? 🌈
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,
)
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