[FC-0118] docs: migrate restful & legacy django api endpoints to standard drf viewsets - #38191
Conversation
| Additionally, some endpoints currently return both JSON and HTML responses depending on | ||
| the request context. Serving both response formats from a single endpoint violates the | ||
| principle of separation of concerns, creates confusion for external consumers and | ||
| automated systems, and increases the complexity of both the view logic and client-side | ||
| handling. |
There was a problem hiding this comment.
I'm not sure if I agree with this, Content Negotiation is a pretty common thing and I think it makes more sense to have one endpoint that can provide content in multiple formats as necessary than two have different endpoints based on content types. I don't think we do this well right now, but I don't think it's because the underlying idea is bad but that we have implemented it in a rough way.
Separating the two endpoints also means we'd have to extract any business logic farther away from either endpoint so that we don't duplicate that for two endpoints that are meant to answer the same question in two formats.
There was a problem hiding this comment.
You have a valid point! I think we can remove this statement in favor of DRF Content Negotiation.
But do you think we can do something about legacy html pages(Link) that still don't have a react page in MFE experience or there is no such html page to worry about? I know most of them have a dedicated react page in MFE experience which we can toggle with a waffle flag.
And this statement was actually added in favor of our decision statement Additionally, all API endpoints will be standardized to return only JSON responses, with all HTML rendering logic relocated to dedicated Micro Frontend (MFE) pages. which will also require changes. If we don't want to go this way like looking for any page on frontend that still relies on HTML legacy experience, then its better to remove this statement.
There was a problem hiding this comment.
And this statement was actually added in favor of our decision statement Additionally, all API endpoints will be standardized to return only JSON responses, with all HTML rendering logic relocated to dedicated Micro Frontend (MFE) pages. which will also require changes. If we don't want to go this way like looking for any page on frontend that still relies on HTML legacy experience, then its better to remove this statement.
@feanil what do you think, should we remove this?
There was a problem hiding this comment.
@Faraz32123 I don't quite understand what you mean here? Remove the whole Additionally... paragraph or some other smaller change?
There was a problem hiding this comment.
@feanil I meant to say, should we focus on converting mako templates to MFE pages so that we have minimal no.of APIs that render HTML pages. But I think we can skip this part in favor of Content Negotiation which is quite common, let me completely remove this part.
There was a problem hiding this comment.
Yeah, I think 99% of our APIs should return only JSON, and we may have one or two legacy endpoints that offer different formats like HTML, e.g. for rendering an XBlock, but in general it's not a pattern that we really need to support or standardize much as far as I know.
The related big problem we have, which I've mentioned in the error handling PR, is that some endpoints sometimes return HTML when a JSON response is expected, making debugging and error reporting much more difficult. Here is a real-world example from today, where the fairly technical user can't even see the error message because it's buried in HTML unexpectedly: https://discuss.openedx.org/t/unable-to-add-components-to-content-library/18782
There was a problem hiding this comment.
I think both are different, HTML responses depending on the request context if its expecting return type to be HTML, then this should be allowed.
But in case of errors or where we don't need HTML in some legacy code where its not updated yet, it should be avoided & return proper JSON.
@bradenmacdonald Do you have any recommendation that we can add to this ADR?
There was a problem hiding this comment.
We're already discussing it on the other "error handling" ADR PR. But basically, if the client is expecting a JSON response, the server should never send HTML, regardless of what kind of error happens.
I agree this is a different question than "HTML responses depending on the request context", but I just wanted to mention it here.
|
|
||
| * All related API actions (list, retrieve, create, update, delete) **MUST** be | ||
| consolidated into a single DRF ViewSet per resource. | ||
| * Endpoints currently returning both JSON and HTML **MUST** be refactored to return only |
There was a problem hiding this comment.
Flagging to update based on discussion in the above comment.
MaferMazu
left a comment
MaferMazu
left a comment
There was a problem hiding this comment.
Hello @Faraz32123, thanks for this ADR.
I added a few comments that I think are worth considering ✨
| @@ -0,0 +1,229 @@ | |||
| Standardize RESTful Endpoint Structure Using DRF ViewSets and also port legacy views to DRF endpoints | |||
There was a problem hiding this comment.
Standardizing the RESTful Endpoint Structure Using DRF ViewSets and porting legacy views to DRF endpoints seem to me to be different decisions. Because standardization has its own context, consequences, and port the legacy to DRF, its own. Depending on what we need, I suggest separating this into two decisions.
There was a problem hiding this comment.
Yes, we can separate both decisions.
There was a problem hiding this comment.
@MaferMazu, I have created a separate ADR(#38351) for porting legacy views to DRF and added you as reviewer there as well.
There was a problem hiding this comment.
@Faraz32123, thanks for splitting the ADRs based on my previous comment. I think I should clarify my point, as the current content still shares a similar goal (the decision section is too similar), which might lead to confusion during the review.
My suggestion to separate them was based on the fact that Standardization and Migration solve different problems: If the goal is only to migrate, we don't need two ADRs. One solid ADR focused on porting legacy views to DRF (like the enrollment example) is sufficient. We can remove 'Standardize' from the title and focus on the rollout and backward compatibility.
If we also have the goal of standardization, then the ADR for standardization should be a Definition of Architecture rather than an action plan. I imagine something like this Open edX REST API Conventions wiki doc, more of a definition of what we consider standard REST API in OpenedX, defining how an Open edX ViewSet must be built to be considered 'standard' (e.g., mandatory inheritance from edx-drf-extensions, explicit get_queryset optimizations, etc.).
My initial comment stemmed from the feeling that we were covering too much with a single ADR by trying to standardize and explain the migration decision. However, currently, both ADRs focus on action, which could be redundant. Depending on the scope, one or two ADRs are sufficient; if two, they should definitely address different decisions and objectives.
What do you think?
There was a problem hiding this comment.
@MaferMazu yes, both ADRs focus on actions, Standardize RESTful Endpoint Structure Using DRF ViewSets was focusing on refactoring all fragmented Open edX REST API endpoints to use DRF ViewSets. And legacy views to DRF decision could also be covered under this same ADR. That's why I had added this commit(Link) to this ADR. I think titles could have stayed same in this commit that triggered the confusion & we were good to go.
I think If you agree, let's keep both in this same PR and I can remove the last commit with titles changed a bit.
If we also have the goal of standardization, then the ADR for standardization should be a Definition of Architecture rather than an action plan. I imagine something like this Open edX REST API Conventions wiki doc, more of a definition of what we consider standard REST API in OpenedX, defining how an Open edX ViewSet must be built to be considered 'standard' (e.g., mandatory inheritance from edx-drf-extensions, explicit get_queryset optimizations, etc.).
Yes the last goal is kind of same as @bradenmacdonald has also mentioned here in forum post(Link) to have a finalized OEP once all ADRs are accepted.
There was a problem hiding this comment.
If it is so, I agree with you to maintain that in one PR ✨
For the title, I suggest something clearer that aligns with the goal. - I ask Gemini, and there are some suggestions: Migrating Legacy Endpoints to Standard DRF ViewSets, Consolidating Legacy Django Views into DRF ViewSets, Adoption of DRF ViewSets for Legacy REST APIs, Refactoring Legacy API Views to Standardized DRF ViewSets, Transitioning Legacy Django Views to Unified DRF ViewSets.
You don't need to use one of those, but I think it is important to calibrate the expectation and the scope of this ADR. - Again, my first thought was you wanted to have this big oep ready + also document the migration decision. 🙈
| * 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). |
There was a problem hiding this comment.
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.
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.
I think its a good suggestions for APIs optimization perspective, I'll add it.
|
|
||
| * Refactoring existing fragmented views into ViewSets requires non-trivial upfront | ||
| development effort. | ||
| * Existing URL patterns may change during migration, requiring updates to client-side |
There was a problem hiding this comment.
We can mitigate this by developing a clear, structured migration plan. We should be explicit about the strategy we will have.
For example:
- Release W: develop a v2 API following the DRF and add a deprecation tag
- Release X, when v2 is stable: add a more aggressive warning while still supporting v1
- Finally, in the next release, deprecate v1.
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.
There was a problem hiding this comment.
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.
In this commit, Instead of creating a new ADR, add decision for moving legacy Django views to DRF to similar existing ADR. Its related issue is: #38168
06dd383 to
e74ab70
Compare
e74ab70 to
18a1817
Compare
|
I think this is ready to merge. @Abdul-Muqadim-Arbisoft you had a review that requested changes that I believe were made but wanted to confirm with you before we merge this. |
Faraz32123
changed the title
* docs: add ADR for standardizing serializer usage (#38139) * docs: explicitly mention API versioning incase of backwards incompatible change (#38188) Co-authored-by: Muhammad Faraz Maqsood <faraz.maqsood@A006-01130.local> * docs: add ADR for standardizing permissions usage (#38187) Currently, authorization logic is implemented inconsistently across views, serializers, and custom access checks. This ADR will define a consistent approach using DRF permission classes, object-level permissions, and queryset scoping where appropriate. Co-authored-by: Taimoor Ahmed <taimoor.ahmed@A006-01711.local> * docs: minor change in ADR language * fix: add s back * docs: migrate restful & legacy django api endpoints to standard drf viewsets (#38191) * docs: Add ADR for ensuring GET requests are idempotent Add edx-platform/docs/decisions/0030-ensure-get-requests-are-idempotent.rst as an accepted ADR. Define policy that GET endpoints must be strictly read-only, with side effects moved to explicit write endpoints or async event pipelines. Include edx-platform relevance, anti-pattern vs preferred code examples, and rollout guidance for testing and migration. * docs: add ADR for standardizing API documentation and schema coverage - Propose adoption of drf-spectacular across Open edX services - Require @extend_schema decorators for all API endpoints - Document request/response schemas, status codes, and error conditions * docs: remove incorrect ADR number * docs: address api-doc-tools deprecation in ADR per review feedback - Add context explaining what api-doc-tools is and its relationship to drf-yasg - Document deprecation and archival of api-doc-tools as a consequence - Add migration guide mapping api-doc-tools decorators and URL helpers to their drf-spectacular equivalents - Add rejected alternative for updating api-doc-tools internals - Add rollout step for final archival cutover Closes review comment by @feanil * docs: expand ADR-0027 with api-doc-tools deprecation and drf-yasg incompatibility analysis Address review feedback on FC-0118 ADR 0027: - Add context paragraph explaining what api-doc-tools is (drf-yasg shim, decorators it provides, schema view, OpenAPI 2.0 output) - Document deprecation of api-doc-tools and drf-yasg as a consequence, including transition-window behavior - Add detailed 8-point incompatibility analysis explaining why drf-yasg cannot be replaced with drf-spectacular inside api-doc-tools (recorded in the ADR itself for future reference) - Add migration plan for existing api-doc-tools consumers with concrete decorator/import/setting mapping - Update Rollout Plan to track api-doc-tools removal - Add references to drf-spectacular migration guide, drf-yasg upstream status, and api-doc-tools repository * chore: fix edx-mantained to edX-platform * docs: add ADR for standardizing pagination across APIs (#38300) * docs: add ADR for api versioning strategy (#38304) * docs: add ADR for standardizing filtering/sorting parameters (#38303) * docs: add ADR-0029 standardized error responses decision (#38246) * docs: add ADR for merging similar endpoints (#38262) * docs: ADR for normalizing nested json apis (#38305) * docs: add separate example for input & output serializers * docs: ADR for documenting and consolidating internal MFE APIs (#38309) * docs: ADR for documenting and consolidating internal MFE APIs Define a plan to document all undocumented internal LMS APIs consumed by MFEs into stable, OpenAPI-described contracts. Introduces a consolidated config endpoint pattern with optional course/user context, authentication boundaries, and a rollout plan following OEP-21 DEPR process. * docs: add ADR for canonical MFE configuration endpoint Record that /api/frontend_site_config/v1/ is the canonical endpoint for MFE/front-end runtime configuration (frontend-base SiteConfig, OEP-65) and that /api/mfe_config/v1 is legacy, on the DEPR path tracked in #37255 and added, and that user-context data (roles, permissions) belongs on resource-oriented endpoints rather than on a configuration payload. Documentation/schema coverage is deferred to the API Documentation & Schema Coverage ADR (#38189). Partially supersedes ADR 0001 (MFE Config API). Part of FC-0118 Open edX REST API standardization (#38137). Refs #38280 * docs: add ADR for standardizing authentication patterns (#38301) * docs: add ADR for standardizing authentication patterns * docs: resolve confusion & update the ADR based on OEP-0042 * docs: support multiple valid auth schemes & deprecate BearerAuthentication * docs: change wording for decisions a bit. * docs: add real examples in accordance with our updated decisions * docs: sync ADR with edx-drf-extensions issue 284 openedx/edx-drf-extensions#284 * docs: make doc more explicit & address comments * docs: move Bearer auth depr plan out of ADR Move BearerAuthentication depr plan out of this doc So that it resides in single place i.e. to its deprecation ticket. * docs: add a pointer file in oauth_dispatch for this ADR * docs: make decision more clear * docs: make authentication_classes usage more clearer * docs: adress the comment related to session authentication * chore: correct file number w.r.t order of the ADRs --------- Co-authored-by: Muhammad Faraz Maqsood <faraz.maqsood@A006-01130.local> Co-authored-by: Taimoor Ahmed <68893403+taimoor-ahmed-1@users.noreply.github.com> Co-authored-by: Taimoor Ahmed <taimoor.ahmed@A006-01711.local> Co-authored-by: Robert Raposa <rraposa@edx.org> Co-authored-by: Abdul Muqadim <abdul.muqadim@A006-01811.local> Co-authored-by: Abdul Muqadim <abdul.muqadim@192.168.1.7> Co-authored-by: Abdul-Muqadim-Arbisoft <139064778+Abdul-Muqadim-Arbisoft@users.noreply.github.com>
5 participants
This is an initial PR for an ADR to standardize restful api endpoints usage.
Currently, many Open edX platform API endpoints are implemented as separate, individual class-based or function-based views for each HTTP action. This ADR focuses on using Django REST Framework (DRF) ViewSets to group related operations into a unified, well-structured view classes.
Related issues: #38165, #38168