-
Notifications
You must be signed in to change notification settings - Fork 4.3k
[FC-0118] docs: migrate restful & legacy django api endpoints to standard drf viewsets #38191
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
b1b4783
c2ba440
5cccb54
b076b72
18a1817
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,200 @@ | ||
| Migrating RESTful & Legacy Django API Endpoints to Standard DRF ViewSets | ||
| ======================================================================== | ||
|
|
||
| :Status: Proposed | ||
| :Date: 2026-03-19 | ||
| :Deciders: API Working Group | ||
| :Technical Story: Open edX REST API Standards - RESTful & Legacy Django API endpoint structure standardization using DRF ViewSets | ||
|
|
||
| Context | ||
| ------- | ||
|
|
||
| Many Open edX platform API endpoints are currently implemented as separate, individual | ||
| class-based or function-based (legacy) views for each HTTP action. Instead of using Django REST | ||
| Framework (DRF) ViewSets to group related operations into a single, cohesive class, each | ||
| action (list, retrieve, create, update, delete) is handled by its own standalone view. | ||
| This fragmented approach leads to significant code duplication, inconsistent behavior | ||
| across related endpoints, and an API layer that is difficult to extend or maintain. | ||
|
|
||
| Decision | ||
| -------- | ||
|
|
||
| We will refactor all fragmented Open edX REST API endpoints to use DRF ViewSets, | ||
| consolidating related actions into unified, well-structured view classes. | ||
|
|
||
| Implementation requirements: | ||
|
|
||
| * All related API actions (list, retrieve, create, update, delete) **MUST** be | ||
| consolidated into a single DRF ViewSet per resource. | ||
| * ViewSets **MUST** be registered using DRF Routers to ensure consistent, predictable | ||
| URL patterns. | ||
| * All ViewSets **MUST** use explicit serializers for both request validation and response | ||
| formatting (per ADR 0025). | ||
| * All ViewSets **MUST** optimize their ``get_queryset`` method using ``select_related`` | ||
| and ``prefetch_related`` to match the fields required by their serializers, preventing | ||
| N+1 query regressions. | ||
| * Multi-method handler functions (e.g., a single method handling DELETE, POST, and PUT) | ||
| **MUST** be refactored into properly documented, action-specific methods within a | ||
| ViewSet. | ||
| * Legacy views should be migrated to ViewSets. And **APIView** should be used for | ||
| non-resource endpoints where ViewSets are not a good fit. | ||
| * Backward compatibility **MUST** be maintained during migration, using | ||
| versioned endpoints or deprecation notices. If a backwards incompatible change is | ||
| required, that change MUST be handled by creating a new version of the API and | ||
| transitioning to that API using the deprecation process. | ||
|
|
||
|
Faraz32123 marked this conversation as resolved.
|
||
| Relevance in edx-platform | ||
| ------------------------- | ||
|
|
||
| Current patterns that should be migrated: | ||
|
|
||
| * **Enrollment API** (``/api/enrollment/v1/``) - currently split across three | ||
| independent ``APIView`` classes, each handling a distinct part of the enrollment | ||
| resource: | ||
|
|
||
| * ``EnrollmentListView(APIView, ApiKeyPermissionMixIn)`` - handles | ||
| ``GET /api/enrollment/v1/enrollment`` (list all enrollments for the current user) | ||
| and ``POST /api/enrollment/v1/enrollment`` (enroll a user in a course, with support | ||
| for mode, enrollment attributes, and enterprise consent). | ||
| * ``UnenrollmentView(APIView)`` - handles | ||
| ``POST /api/enrollment/v1/unenrollment``, a privileged service-only endpoint that | ||
| unenrolls a single user from all courses as part of the user retirement pipeline. | ||
| * ``EnrollmentAllowedView(APIView)`` - handles retrieval and creation of | ||
| ``CourseEnrollmentAllowed`` records for a given user email and course ID; restricted | ||
| to admin users via ``permissions.IsAdminUser``. | ||
| * These three views operate on the same enrollment resource and should be unified into | ||
| a single ``EnrollmentViewSet``. | ||
|
|
||
| * **Assets Handling Endpoints** - currently exhibit a distinct issue: | ||
|
|
||
| * A single handler function mixes DELETE, POST, and PUT operations without clear | ||
| separation of concerns. | ||
| * **Resolution:** Refactor into a properly documented ``AssetsViewSet`` with distinct | ||
| action methods. | ||
|
|
||
| * **Legacy Django Views** - Many endpoints still use plain Django views instead of DRF: | ||
| * Hard-coded JSON responses using ``HttpResponse(json.dumps(...))`` instead of serializers | ||
| * Manual method dispatch instead of DRF mixins and ViewSets | ||
| * Missing DRF features like automatic authentication, permission classes, and schema generation | ||
| * Inconsistent error handling and response formats | ||
|
|
||
| Illustrative Example | ||
| -------------------- | ||
|
|
||
| The following shows the structural pattern being replaced and the target pattern. | ||
| Full implementation details will be addressed during the migration of each endpoint. | ||
|
|
||
| **Before - Enrollment API (current fragmented pattern):** | ||
|
|
||
| .. code-block:: python | ||
|
|
||
| # Three separate APIView classes for one logical resource | ||
| class EnrollmentListView(APIView, ApiKeyPermissionMixIn): ... | ||
| class UnenrollmentView(APIView): ... # privileged, retirement pipeline only | ||
| class EnrollmentAllowedView(APIView): ... # admin-only, CourseEnrollmentAllowed records | ||
|
|
||
| **After - Consolidated into a single ViewSet:** | ||
|
|
||
| .. code-block:: python | ||
|
|
||
| class EnrollmentViewSet(viewsets.ViewSet): | ||
| # viewsets.ViewSet used intentionally — enrollment logic routes through | ||
| # the api module, not direct ORM, so ModelViewSet is not appropriate. | ||
|
|
||
| def list(self, request): ... | ||
| def create(self, request): ... | ||
|
|
||
| @action(detail=False, methods=["post"], url_path="unenrollment", | ||
| permission_classes=[ApiKeyHeaderPermission]) | ||
| def unenroll(self, request): ... # preserves privileged-only restriction | ||
|
|
||
| @action(detail=False, methods=["get", "post"], url_path="allowed", | ||
| permission_classes=[permissions.IsAdminUser], | ||
| throttle_classes=[EnrollmentUserThrottle]) | ||
| def allowed(self, request): ... # reuses existing CourseEnrollmentAllowedSerializer | ||
|
|
||
| router = DefaultRouter() | ||
| router.register(r"enrollment", EnrollmentViewSet, basename="enrollment") | ||
|
|
||
| **Before - Assets handler (current pattern):** | ||
|
|
||
| .. code-block:: python | ||
|
|
||
| # Single function-based view with GET, POST, PUT, and DELETE all dispatched | ||
| # inside handle_assets(). | ||
| @login_required | ||
| @ensure_csrf_cookie | ||
| def assets_handler(request, course_key_string=None, asset_key_string=None): | ||
| return handle_assets(request, course_key_string, asset_key_string) | ||
|
|
||
| **After - Dedicated ViewSet:** | ||
|
|
||
| .. code-block:: python | ||
|
|
||
| class AssetsViewSet(viewsets.ViewSet): | ||
| # Reuses existing asset_storage_handlers service functions. | ||
| def list(self, request, course_key_string): ... # GET - paginated asset list | ||
| def create(self, request, course_key_string): ... # POST - upload asset | ||
| def update(self, request, course_key_string, pk): ... # PUT - update lock state | ||
| def destroy(self, request, course_key_string, pk): ...# DELETE - remove asset | ||
|
|
||
| Consequences | ||
| ------------ | ||
|
|
||
| Positive | ||
| ~~~~~~~~ | ||
|
|
||
| * Consistent, discoverable API structure that is easier for developers and third-party | ||
| integrators to understand and consume. | ||
| * Significant reduction in code duplication by consolidating related operations into a | ||
| single ViewSet class. | ||
| * Improved maintainability - changes to a resource's API logic are localized to one | ||
| ViewSet rather than scattered across multiple view files. | ||
| * DRF Routers automatically generate standard URL patterns, reducing manual URL | ||
| configuration and human error. | ||
| * Improved compatibility with AI systems, automated testing frameworks, and third-party | ||
| integrations that expect predictable, standardized responses. | ||
| * Enables automatic API schema generation and documentation via ``drf-spectacular``. | ||
|
|
||
| Negative / Trade-offs | ||
| ~~~~~~~~~~~~~~~~~~~~~ | ||
|
|
||
| * Refactoring existing fragmented views into ViewSets requires non-trivial upfront | ||
| development effort. | ||
| * Existing URL patterns may change during migration, requiring updates to client-side | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can mitigate this by developing a clear, structured migration plan. We should be explicit about the strategy we will have.
It doesn't need to be exactly like that, but it should allow the community to adapt their API endpoints without a crazy rush. In particular, the enrollment API is used across different OpenedX services and is also widely used for community plugins, so we should be conservative about it. I think it is worth adding more details on the migration in the rollout plan.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can handle migration plan in this ADR: https://github.com/openedx/openedx-platform/pull/38304/changes and add a little bit of info here. |
||
| code, documentation, and any hardcoded references. | ||
| * Teams unfamiliar with DRF ViewSets and Routers will require onboarding before | ||
| contributing to migrated endpoints. | ||
|
|
||
| Alternatives Considered | ||
| ----------------------- | ||
|
|
||
| * **Keep fragmented APIView classes:** Rejected. Maintains code duplication, | ||
| inconsistency, and high maintenance burden across related operations. | ||
| * **Use DRF GenericAPIView with mixins:** Partially considered but rejected as the | ||
| primary approach. While mixins reduce some duplication, they do not provide the same | ||
| level of structural consolidation or URL standardization as ViewSets with routers. | ||
|
|
||
| Rollout Plan | ||
| ------------ | ||
|
|
||
| 1. Audit existing API endpoints to identify all fragmented view patterns and legacy Django | ||
| views. | ||
| 2. Prioritize high-impact resources for migration: Enrollment API, Assets endpoints, and | ||
| any other endpoints identified in the audit. | ||
| 3. Refactor identified endpoints into DRF ViewSets, registered via DRF Routers. Ensure | ||
| all ViewSets use explicit serializers per ADR 0025. | ||
| 4. Include a comparison of SQL query counts to ensure no performance degradation. | ||
| 5. Update and expand test coverage to validate correct behavior of all refactored | ||
| ViewSet actions. | ||
| 6. Publish deprecation notices for any legacy URL patterns that will be replaced, | ||
| providing clear migration guidance to internal and external API consumers. | ||
| 7. Update API documentation to reflect the new ViewSet-based structure and URL patterns. | ||
|
|
||
| References | ||
| ---------- | ||
|
|
||
| * Django REST Framework documentation - ViewSets: | ||
| https://www.django-rest-framework.org/api-guide/viewsets/ | ||
| * Django REST Framework documentation - Routers: | ||
| https://www.django-rest-framework.org/api-guide/routers/ | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add something like: "All ViewSets MUST optimize their get_queryset method using select_related and prefetch_related to match the fields required by their serializers, preventing N+1 query regressions."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related to this, we should add to the rollout plan that it will be important to include a comparison of SQL query counts to ensure no performance degradation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think its a good suggestions for APIs optimization perspective, I'll add it.