Feat: replace drf yasg with drf spectacular - #39108
Faraz32123 wants to merge 7 commits into
Conversation
Faraz32123
changed the title
Converts @swagger_auto_schema to @extend_schema in the five modules that import drf_yasg directly, following the drf-spectacular migration guide: https://drf-spectacular.readthedocs.io/en/latest/drf_yasg.html Structured openapi.Schema objects become inline_serializer so they get named components in the generated schema; untyped ones become OpenApiTypes.OBJECT. openapi.Parameter becomes OpenApiParameter. The other four drf_yasg users go through edx-api-doc-tools and will be migrated with it.
Converts @apidocs.schema to @extend_schema across openedx/core, replacing the parameter helpers with OpenApiParameter and string response descriptions with OpenApiResponse. bookmarks/serializers.py inlines is_schema_request, which has no drf-spectacular equivalent, and extends it to recognise drf-spectacular's swagger_fake_view alongside drf-yasg's format=openapi.
Converts @apidocs.schema and @Schema to @extend_schema across the lms app, excluding instructor. Parameter helpers become OpenApiParameter and string response descriptions become OpenApiResponse. discussion/rest_api/views.py also drops its remaining direct drf_yasg import, which was interleaved with the apidocs decorators.
Converts the 26 @apidocs.schema decorators in the instructor v1 and v2 APIs to @extend_schema. The course_id, problem, and exam_id path parameters were repeated verbatim across 29 decorators; those are now module-level constants.
Converts the remaining @apidocs.schema decorators across contentstore and modulestore_migrator to @extend_schema. The three class-level @apidocs.schema_for decorators become @extend_schema_view, splitting each docstring into summary and description as schema_for did. Files that already imported drf-spectacular for the FC-0118 work have their import lines merged rather than duplicated.
Six serializers subclass BaseSerializer, which has no `fields` attribute, so drf-spectacular raises AttributeError when generating a schema that covers them. Each extension declares the type its serializer produces. Registered from CommonInitializationConfig.ready() so they load in both services regardless of which schema is being generated.
6e12f7f to
c232307
Compare
Replaces make_docs_urls with SpectacularAPIView, SpectacularSwaggerView and SpectacularRedocView, preserving the swagger.json, swagger.yaml, api-docs/ and swagger/ routes and their URL names. The UI views reverse their schema URL without arguments, so api-docs/schema/ is registered alongside the format-suffixed routes. /api-docs serves the full API surface via custom_settings, leaving SPECTACULAR_SETTINGS to the narrower Authoring and Enrollment schemas the SDK consumes. Also removes drf_yasg from INSTALLED_APPS, drops SWAGGER_SETTINGS, and converts the docs security definitions to OpenAPI 3 form. `make swagger` now runs `manage.py lms spectacular`, since generate_swagger came from drf_yasg; docs_settings applies the same unfiltered configuration as /api-docs so the generated file still covers the whole surface. edx-api-doc-tools and drf-yasg remain installed as transitive dependencies of openedx-authz and django-user-tasks respectively.
c232307 to
b9f6467
Compare
| urlpatterns += [ | ||
| re_path( | ||
| r'^swagger\.(?P<format>json|yaml)$', | ||
| SpectacularAPIView.as_view(custom_settings=get_api_docs_settings()), |
There was a problem hiding this comment.
These endpoints lose their server-side cache.
openedx/envs/common.py has, with the comment "How long to cache OpenAPI schemas and UI, in seconds":
OPENAPI_CACHE_TIMEOUT = 60 * 60Nothing in lms/envs/production.py or cms/envs/production.py overrides it — only devstack and test set it to 0 — so production runs with a 1-hour cache today. edx-api-doc-tools fed that value into SchemaView.as_cached_view, which wraps the view in vary_on_headers("Cookie", "Authorization") + cache_page(timeout) (drf_yasg/views.py:134-164).
SpectacularAPIView has no caching of any kind, so after this change every request to /swagger.json, /swagger.yaml, and the new /api-docs/schema/ regenerates the entire schema. These are public and unauthenticated (SERVE_PERMISSIONS defaults to AllowAny, which matches the old permission_classes=(AllowAny,)), and the schema being generated is also roughly twice as large as before (see my note on openedx/core/apidocs.py).
It also leaves OPENAPI_CACHE_TIMEOUT as a dead setting that still claims to do something.
Could we keep using the existing setting — wrapping the two SpectacularAPIView routes in cache_page(settings.OPENAPI_CACHE_TIMEOUT)? Same applies to cms/urls.py.
| name='apidocs-schema', | ||
| ), | ||
| path( | ||
| 'api-docs/', |
There was a problem hiding this comment.
Heads up: this quietly drops the only test coverage of schema generation.
lms/tests.py has:
def test_api_docs(self):
"""Tests that requests to the `/api-docs/` endpoint do not raise an exception."""
response = self.client.get('/api-docs/')
assert response.status_code == 200Under drf-yasg that genuinely exercised generation end-to-end: SchemaView.get() calls generator.get_schema(...) unconditionally, including when the accepted renderer is the Swagger UI one (drf_yasg/views.py:122-132).
SpectacularSwaggerView only renders a template shell and never touches the generator, so the test will keep passing while covering nothing — and it's precisely the coverage that would have caught the BaseSerializer AttributeError that commit 6 exists to fix.
Could you point that test at /api-docs/schema/ instead, and add the CMS equivalent (which has no such test today)? That's cheap and it guards the whole migration.
| ), | ||
| path( | ||
| 'api-docs/', | ||
| SpectacularSwaggerView.as_view(url_name='apidocs-schema'), |
There was a problem hiding this comment.
Could we use drf-spectacular-sidecar here rather than the CDN defaults?
drf-spectacular 0.30.0 ships these defaults (drf_spectacular/settings.py:83-85):
'SWAGGER_UI_DIST': 'https://cdn.jsdelivr.net/npm/swagger-ui-dist@latest',
'SWAGGER_UI_FAVICON_HREF': 'https://cdn.jsdelivr.net/npm/swagger-ui-dist@latest/favicon-32x32.png',
'REDOC_DIST': 'https://cdn.jsdelivr.net/npm/redoc@latest',drf-yasg served these assets from its own bundled staticfiles, so this swaps self-hosted JS for unpinned third-party JS executing on the LMS and CMS origin. Every Open edX deployment would silently pick up whatever swagger-ui and redoc publish so it would introduce a potential supply chain vulnerability.
drf-spectacular-sidecar is the upstream-supported answer (self-hosted, version-pinned, and what the drf-spectacular README recommends): add the dependency, put drf_spectacular_sidecar in INSTALLED_APPS, and set SWAGGER_UI_DIST/SWAGGER_UI_FAVICON_HREF/REDOC_DIST to 'SIDECAR'. That also resolves the air-gapped caveat in your description.
| 'VERSION': 'v1', | ||
| # Document every endpoint, without the per-service filtering and prefix | ||
| # trimming that SPECTACULAR_SETTINGS applies. | ||
| 'PREPROCESSING_HOOKS': [], |
There was a problem hiding this comment.
Note only — no change needed in this PR.
The comment above frames /api-docs as "the full, untrimmed API surface" in contrast to the narrower per-service schemas, but the implementation being replaced wasn't unfiltered either. make_docs_urls(api_info) with no api_url_patterns (how both lms/urls.py and cms/urls.py called it) resolves to ApiSchemaGenerator, which overrides get_endpoints to keep only paths starting with /api/ and pins determine_path_prefix to /api/ (edx_api_doc_tools/conf_utils.py:157-177).
Your own numbers show the delta: 633 LMS paths now, versus 293 in the committed docs/lms-openapi.yaml, which the old /api/-only generator produced (all /api-relative under basePath: /api).
So two things change: the public docs now describe every DRF endpoint in the service rather than just the versioned /api/* surface, and make swagger — which feeds docs/references/lms_apis.rst — will turn that committed file from a 293-path Swagger 2.0 document into a ~633-path OpenAPI 3 one with untrimmed paths.
We're replacing docs/lms-openapi.yaml, with the new schema files. Flagging it mainly so the widening is a recorded decision rather than a side effect. Might be worth a sentence in the PR description.
| """ | ||
| Build the ``/api-docs`` schema settings, adding contact details if available. | ||
|
|
||
| ``API_ACCESS_MANAGER_EMAIL`` is an LMS-only setting, so it is included only |
There was a problem hiding this comment.
Small correction: API_ACCESS_MANAGER_EMAIL isn't LMS-only. It's defined in openedx/envs/common.py (~line 2721), which both lms/envs/common.py and cms/envs/common.py star-import — the old code here read it unconditionally at import time from a module cms/urls.py imported, which wouldn't have worked otherwise.
The getattr(..., None) guard is harmless, but the docstring should probably just say the contact is included when the setting is present, without the LMS-only claim.
| ``AttributeError`` when it tries to walk them. Each extension below declares | ||
| the type its serializer actually produces. | ||
|
|
||
| The extensions self-register on import; ``lms.lib.spectacular`` and |
There was a problem hiding this comment.
This paragraph doesn't match the implementation — neither lms/lib/spectacular.py nor cms/lib/spectacular.py imports this module. The only importer is openedx/core/djangoapps/common_initialization/apps.py in ready(), which is the right place.
Looks like a leftover from an earlier approach; worth repointing the docstring at the AppConfig so the next reader can find the registration site.
|
|
||
| Schema generators set a swagger_fake_view attribute on the view; that is | ||
| the drf-spectacular-compatible signal. ``format=openapi`` is drf-yasg's | ||
| convention, kept while it still serves ``/api-docs``. |
There was a problem hiding this comment.
The swagger_fake_view check is correct — drf-spectacular sets it on the view (generators.py:141) before build_mock_request builds the DRF Request, so parser_context['view'] resolves to that view.
The docstring is stale though: this PR is what stops drf-yasg serving /api-docs, so "kept while it still serves /api-docs" no longer holds and the format=openapi branch is dead as far as the platform is concerned. Either drop the fallback or reword to say it's kept for out-of-tree callers.
| @@ -2149,10 +2148,6 @@ | |||
|
|
|||
| ######################### Django Rest Framework ######################## | |||
There was a problem hiding this comment.
Nit: removing SWAGGER_SETTINGS leaves this banner with nothing under it. Worth deleting the header too, since the drf-spectacular block below has its own.
2 participants
This PR can be merged after this schema generation PR: #39025
Replace drf-yasg with drf-spectacular
Follow-up to the FC-0118 API standardization work. Migrates all platform API
documentation off
drf-yasgandedx-api-doc-toolsontodrf-spectacular,including the
/api-docssite itself.Slack Discussion thread, the goal was to drop both dependencies as part
of the drf-spectacular conversion, replacing the
edx-api-doc-tools/api-docsendpoint with the drf-spectacular equivalent.
What changed
~400 decorator call sites across 43 files, following the
drf-yasg migration guide:
@swagger_auto_schema/@apidocs.schema→@extend_schema@apidocs.schema_for→@extend_schema_viewapidocs.string_parameter/query_parameter/path_parameterandopenapi.Parameter→OpenApiParameter401: "Not authenticated.") →OpenApiResponseopenapi.Schemaobjects →inline_serializerwhere they had structure,OpenApiTypeswhere they didn't/api-docsnow served bySpectacularAPIView/SpectacularSwaggerView/SpectacularRedocViewCommits
Reviewable one at a time, each independently handled:
drf_yasgusers (5 files)openedx/core(10 files)lms, excluding instructor (4 files)instructor(2 files, ~120 sites)cms(22 files)BaseSerializersubclasses/api-docsswap and dependency removalTwo things worth knowing
The dependencies don't actually disappear.
edx-api-doc-toolsis stillrequired by
openedx-authz, anddrf-yasgbydjango-user-tasks. Both remainin
base.txtas transitive dependencies. What this PR removes is the platform'sown dependency on them —
pyproject.toml,INSTALLED_APPS, and every import.Fully dropping them needs those upstream packages to migrate first.
Commit 6 exists because
/api-docsis unfiltered. Six serializers subclassBaseSerializer, which has nofieldsattribute, so drf-spectacular raisedAttributeErrorwhen generating a schema covering the whole surface. This neversurfaced before because the existing drf-spectacular schemas
(
/authoring-api/,/lms-api/) are filtered down to a handful of endpoints.Each serializer now has an
OpenApiSerializerExtensiondeclaring the type itactually produces.
Verification
Tested against a running Tutor instance:
/api-docs/api-docs/authoring-api/schema//lms-api/schema/The last two matter most:
/api-docsusescustom_settingsrather than theglobal
SPECTACULAR_SETTINGS, so the narrow SDK-facing schemas are untouched —same paths, same prefix trimming. The SDK's generated client is unaffected.
Swagger UI and ReDoc confirmed rendering on both services.
swagger.json,swagger.yaml,api-docs/andswagger/keep their existing paths and URLnames;
api-docs/schema/is new, because the Swagger and ReDoc views reversetheir schema URL without arguments.
Notes for reviewers
instructor/views/api_v2.pyhad 91string responses and 46 parameters, so the repetitive conversions were
scripted. Spot-checking won't catch a systematic miss — I verified by
extracting every response and parameter from both versions and diffing the
sets. Same method used on commits 1–3 and 5 to confirm nothing was dropped.
docs/docs_settings.pysecurity definitions moved from Swagger 2.0(
type: basic) to OpenAPI 3 (type: http, scheme: basic). This is the onechange I couldn't exercise locally — it only takes effect in the Sphinx docs
build.
them. Rendering is fine on a normal deployment; air-gapped installs would need
drf-spectacular-sidecar.serializer_class,unresolvable authenticators, two
CourseEnrollmentcomponents with clashingnames). They don't block generation and are out of scope here.