-
Notifications
You must be signed in to change notification settings - Fork 708
UN-4009 [MISC] Generate and commit the API deployment OpenAPI spec in-repo #2237
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
base: main
Are you sure you want to change the base?
Changes from all commits
c49234b
482ca1b
86761cc
05413c1
3ebfafb
3198b52
51c7cd4
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,29 @@ | ||
| """URLconf the published OpenAPI spec is generated against. | ||
|
|
||
| Each entry is an included sub-urlconf: generating against one directly yields | ||
| paths without the prefix it is mounted at, i.e. a spec describing URLs the | ||
| server does not serve. The mounts are selected out of the served urlconf | ||
| rather than restated, so moving one moves the generated paths with it. | ||
|
|
||
| Widening the spec to another endpoint means annotating its view with | ||
| ``@extend_schema`` and adding its urlconf here. | ||
| """ | ||
|
|
||
| from django.core.exceptions import ImproperlyConfigured | ||
|
|
||
| from backend import base_urls | ||
|
|
||
| SPEC_URLCONFS = ("api_v2.execution_urls",) | ||
|
|
||
| urlpatterns = [ | ||
| entry | ||
| for entry in base_urls.urlpatterns | ||
| if getattr(getattr(entry, "urlconf_name", None), "__name__", None) in SPEC_URLCONFS | ||
| ] | ||
|
|
||
| missing = set(SPEC_URLCONFS) - {entry.urlconf_name.__name__ for entry in urlpatterns} | ||
| if missing: | ||
| raise ImproperlyConfigured( | ||
| f"{', '.join(sorted(missing))} is not mounted in backend.base_urls; the " | ||
| "spec would be generated for routes the server does not serve." | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,104 @@ | ||
| """Regenerate the committed API deployment OpenAPI spec. | ||
|
|
||
| The spec is the contract the published clients and their generated SDKs are | ||
| built from, so it is committed and CI fails on drift: change a route, a | ||
| serializer or the schema annotation, and regenerate in the same PR. | ||
|
|
||
| uv run python manage.py generate_docstudio_spec # from backend/ | ||
| uv run python manage.py generate_docstudio_spec --check # no write, drift is an error | ||
|
|
||
| The generated paths carry ``API_DEPLOYMENT_PATH_PREFIX``, so regenerate in an | ||
| environment that does not override it — the committed artifact describes the | ||
| deployment as it is served publicly, not as one installation mounts it. | ||
| """ | ||
|
|
||
| import json | ||
| from pathlib import Path | ||
| from typing import Any | ||
|
|
||
| from django.core.management.base import BaseCommand, CommandError | ||
| from drf_spectacular.drainage import GENERATOR_STATS | ||
| from drf_spectacular.generators import SchemaGenerator | ||
|
|
||
| DEFAULT_OUT = Path(__file__).resolve().parents[4] / "specs" / "docstudio-oss.json" | ||
| URLCONF = "api_v2.deployment_spec_urls" | ||
| REGENERATE = "uv run python manage.py generate_docstudio_spec" | ||
| # Named in every failure message: the repos that regenerate from this file are | ||
| # the ones a spec change actually breaks, and nothing there watches this repo. | ||
| DOWNSTREAM = ( | ||
| "The published client (Zipstack/unstract-python-client) and the CLI " | ||
| "(Zipstack/unstract-cli) are generated from this file — raise the matching " | ||
| "PRs there for anything that changes an operation id, a tag or a schema." | ||
| ) | ||
|
|
||
|
|
||
| class SpecGenerationFailed(CommandError): | ||
| """Raised when the generator had to guess.""" | ||
|
|
||
|
|
||
| def render_spec() -> str: | ||
| """The committed artifact, byte for byte. | ||
|
|
||
| Shared with the drift test: two copies of this could disagree, and then | ||
| the gate rejects exactly the file the command it names produces. | ||
| """ | ||
| GENERATOR_STATS.reset() | ||
| schema = SchemaGenerator(urlconf=URLCONF).get_schema(request=None, public=True) | ||
| if GENERATOR_STATS: | ||
| # spectacular downgrades "unable to guess serializer" to a warning and | ||
|
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. [Low] [Lens 16] — This comment misstates spectacular's diagnostic severity and what it emits The comment says spectacular "downgrades 'unable to guess serializer' to a warning and writes a plausible, wrong operation" (restated at The guard is correct because it checks both caches. But a maintainer debugging a future failure looks in the wrong bucket and expects a fabricated schema that is never there. Evidence: Also here: |
||
| # writes a plausible, wrong operation. Nothing downstream can tell that | ||
| # apart from an annotation that is simply thin. | ||
|
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. [Medium] [Lens 3] — The generation gate rejects guessed shapes but never checks the spec is legal OpenAPI
Verified — injecting Fix: call |
||
| diagnostics = "\n".join( | ||
| f" {severity}: {message}" | ||
| for severity, cache in ( | ||
| ("error", GENERATOR_STATS._error_cache), | ||
| ("warning", GENERATOR_STATS._warn_cache), | ||
| ) | ||
| for message in cache | ||
| ) | ||
| raise SpecGenerationFailed( | ||
| f"The generator reported problems, so the spec would describe an " | ||
| f"API nobody implements:\n{diagnostics}" | ||
| ) | ||
| # Sorted keys are what make the committed artifact a usable drift signal. | ||
| return json.dumps(schema, indent=2, sort_keys=True) + "\n" | ||
|
|
||
|
|
||
| class Command(BaseCommand): | ||
| help = "Generate the API deployment OpenAPI spec." | ||
|
|
||
| def add_arguments(self, parser: Any) -> None: | ||
| parser.add_argument("--out", type=Path, default=DEFAULT_OUT) | ||
| parser.add_argument( | ||
| "--check", | ||
| action="store_true", | ||
| help="Fail if the file on disk differs, instead of writing it.", | ||
| ) | ||
|
|
||
| def handle(self, *args: Any, **options: Any) -> None: | ||
| rendered = render_spec() | ||
|
|
||
| out: Path = options["out"] | ||
| if options["check"]: | ||
| current = out.read_text() if out.exists() else "" | ||
| if current != rendered: | ||
| raise CommandError( | ||
| f"{out} is out of date. Run `{REGENERATE}` from `backend/` " | ||
| f"and commit the result.\n\n{DOWNSTREAM}" | ||
| ) | ||
| self.stdout.write(f"{out} is up to date") | ||
| return | ||
|
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. [Low] [Lens 13] — The
Fix: exercise (For the record, the drift gate itself is real — |
||
|
|
||
| out.parent.mkdir(parents=True, exist_ok=True) | ||
| out.write_text(rendered) | ||
| schema = json.loads(rendered) | ||
| operations = sum( | ||
| 1 | ||
| for methods in schema["paths"].values() | ||
| for method in methods | ||
| if method in {"get", "post", "put", "patch", "delete"} | ||
| ) | ||
| self.stdout.write( | ||
| f"{out}: {len(schema['paths'])} paths, {operations} operations, " | ||
| f"{len(schema.get('components', {}).get('schemas', {}))} schemas" | ||
| ) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,161 @@ | ||
| """OpenAPI annotations for the API deployment endpoints. | ||
|
|
||
| The serializers here shape the published spec only; none of them is used to | ||
| parse a request or build a response. They live outside ``serializers.py`` so | ||
| that nothing at request time imports one by accident. | ||
|
|
||
| Their docstrings are published as the client-facing model descriptions, so | ||
| they are written for the caller rather than the maintainer. | ||
| """ | ||
|
|
||
| from drf_spectacular.utils import ( | ||
| OpenApiParameter, | ||
| OpenApiResponse, | ||
| extend_schema, | ||
| extend_schema_serializer, | ||
| extend_schema_view, | ||
| ) | ||
| from rest_framework import serializers | ||
|
|
||
| from api_v2.serializers import ( | ||
| APIExecutionResponseSerializer, | ||
| ExecutionQuerySerializer, | ||
| ExecutionRequestSerializer, | ||
| ) | ||
|
|
||
|
|
||
| # Declares no field of its own, so a change to the real serializer moves the | ||
| # spec. It exists to carry a caller-facing description and a stable name. | ||
| @extend_schema_serializer(component_name="ExecuteRequest") | ||
| class ExecuteRequest(ExecutionRequestSerializer): | ||
| """The documents to run, and the options that shape the result. | ||
|
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. [Medium] [Lens 1, 3, 7] — The OSS spec advertises two request fields that always 400 in OSS
An OSS SDK user gets a 400 with an enterprise sales message from a parameter the SDK told them exists. Fix: Open question: is a cloud spec generated from this same command? That decides flat-exclude vs conditional. |
||
|
|
||
| Supply `files`, `presigned_urls`, or both. | ||
| """ | ||
|
|
||
|
|
||
| class FileResult(serializers.Serializer): | ||
| file = serializers.CharField() | ||
| file_execution_id = serializers.CharField(required=False) | ||
|
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. [Low] [Lens 7] —
UNVERIFIED — I did not find a producer that leaves it unset, so this is a question rather than an asserted defect: is there a path where a cached file result carries no |
||
| status = serializers.CharField(required=False) | ||
|
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. [Medium] [Lens 7] — Metrics live one level down at Separately, Verified — traced both API-result writers ( Fix: drop |
||
| result = serializers.JSONField(required=False) | ||
| metadata = serializers.JSONField(required=False) | ||
| metrics = serializers.JSONField(required=False) | ||
| error = serializers.CharField(required=False, allow_null=True) | ||
|
|
||
|
|
||
| class ExecutionMessage(APIExecutionResponseSerializer): | ||
| """The execution's identity and, once it has finished, its per-file | ||
| results. | ||
| """ | ||
|
|
||
|
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. [High] [Lens 7, 3] —
Verified — the real serializer replayed over the real dataclass: A pydantic/Go/Java client raises on the success response. The comment just above at Fix: restate both as |
||
| # Restated because the real declaration is an untyped JSONField, and | ||
| # because a pending execution sends `result: null`, which a generated | ||
| # deserialiser iterates and crashes on without allow_null. | ||
| result = FileResult(many=True, required=False, allow_null=True) | ||
|
|
||
|
|
||
| class ExecuteResponse(serializers.Serializer): | ||
| message = ExecutionMessage() | ||
|
|
||
|
|
||
| class StatusResponse(serializers.Serializer): | ||
| status = serializers.CharField() | ||
| message = FileResult(many=True, required=False, allow_null=True) | ||
|
|
||
|
|
||
| class ErrorResponse(serializers.Serializer): | ||
| status = serializers.CharField(required=False) | ||
| message = serializers.JSONField(required=False, allow_null=True) | ||
|
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. [High] [Lens 7, 3, 2] — Every error response in the spec has a shape the server never sends
Verified — the pinned handler run against this repo's own exceptions: Corroborated in-repo by Fix: the repo already ships |
||
|
|
||
|
|
||
| # Restates the route's own pattern so a client rejects a mistyped identifier | ||
| # without a round trip. | ||
| PATH_SEGMENT = {"type": "string", "pattern": r"^[\w-]+$"} | ||
|
|
||
| DEPLOYMENT_PATH_PARAMETERS = [ | ||
| OpenApiParameter( | ||
| "org_name", | ||
| PATH_SEGMENT, | ||
| OpenApiParameter.PATH, | ||
| description="Organization identifier.", | ||
| ), | ||
| OpenApiParameter( | ||
| "api_name", | ||
| PATH_SEGMENT, | ||
| OpenApiParameter.PATH, | ||
| description="API deployment name.", | ||
| ), | ||
| ] | ||
|
|
||
|
|
||
| DEPLOYMENT_AUTH = [{"deploymentKey": []}] | ||
|
|
||
| # A client generated without these treats an authentication or rate-limit | ||
| # response as an unknown status and has nothing to branch on. | ||
| DEPLOYMENT_ERRORS = { | ||
| 400: OpenApiResponse(ErrorResponse, description="The request failed validation."), | ||
| 401: OpenApiResponse(ErrorResponse, description="The API key is not valid."), | ||
| 403: OpenApiResponse(ErrorResponse, description="No API key was supplied."), | ||
| 404: OpenApiResponse(ErrorResponse, description="No such active deployment."), | ||
| 429: OpenApiResponse( | ||
| ErrorResponse, description="Too many concurrent executions; retry later." | ||
| ), | ||
| 500: ErrorResponse, | ||
| } | ||
|
|
||
| EXECUTE_DESCRIPTION = ( | ||
| "Execute an API deployment against one or more documents.\n\n" | ||
| "Supply the documents either as `files` (multipart upload) or as " | ||
| "`presigned_urls` (HTTPS S3 URLs), or both — a request carrying neither is " | ||
| f"rejected, and the two together may not exceed " | ||
| f"{ExecutionRequestSerializer.MAX_FILES_ALLOWED} documents.\n\n" | ||
| "With the default `timeout` of -1 the call returns as soon as the " | ||
| "execution is queued; read the outcome from the status endpoint." | ||
| ) | ||
|
|
||
| STATUS_DESCRIPTION = ( | ||
| "Read the result of a previously started execution.\n\n" | ||
| "This read is one-shot: the first call that observes a completed execution " | ||
| "acknowledges it and the stored result is discarded, so every later call " | ||
|
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. [Medium] [Lens 7, 16] — "Poll while the execution is pending" reads as a 200-returning loop. Generated SDKs raise on 4xx by default, so the documented polling loop throws on every iteration until completion. The shape is declared ( Fix: one sentence — a still-running execution answers 422 with the current |
||
| "for that execution answers 406. Poll while the execution is pending, and " | ||
| "keep the payload of the call that returns it — it cannot be fetched again." | ||
| ) | ||
|
|
||
|
|
||
| # Generated clients take their command names, module paths and request shapes | ||
| # from here, so this is part of the public API surface. | ||
| DEPLOYMENT_EXECUTION_SCHEMA = extend_schema_view( | ||
| post=extend_schema( | ||
| operation_id="execute", | ||
| tags=["deployment"], | ||
| auth=DEPLOYMENT_AUTH, | ||
| parameters=DEPLOYMENT_PATH_PARAMETERS, | ||
| request={"multipart/form-data": ExecuteRequest}, | ||
| responses={ | ||
| 200: ExecuteResponse, | ||
|
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. [Medium] [Lens 7] —
So an expired S3 signature surfaces as a 403 whose spec description reads "No API key was supplied.", and an S3 404 as "No such active deployment." Undeclared statuses reach a generated client as an unmodelled response. Fix: declare 413/502/504, and reword the 403/404 descriptions so they don't assert a cause the endpoint can't guarantee. Better still, normalise upstream statuses to a single 502 in |
||
| 409: OpenApiResponse( | ||
| ErrorResponse, description="The deployment has no active API key." | ||
|
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. [Low] [Lens 7] — Two declared statuses this endpoint cannot return 409 on Fix: drop 409 from |
||
| ), | ||
| 422: ExecuteResponse, | ||
| **DEPLOYMENT_ERRORS, | ||
| }, | ||
| description=EXECUTE_DESCRIPTION, | ||
| ), | ||
| get=extend_schema( | ||
| operation_id="status", | ||
| tags=["deployment"], | ||
| auth=DEPLOYMENT_AUTH, | ||
| parameters=DEPLOYMENT_PATH_PARAMETERS + [ExecutionQuerySerializer], | ||
| responses={ | ||
| 200: StatusResponse, | ||
| 406: OpenApiResponse( | ||
| ErrorResponse, | ||
| description="The result was already consumed by an earlier call.", | ||
| ), | ||
| 422: StatusResponse, | ||
| **DEPLOYMENT_ERRORS, | ||
| }, | ||
| description=STATUS_DESCRIPTION, | ||
| ), | ||
| ) | ||
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.
[Low] [Lens 13] — The drift gate's outcome depends on ambient
API_DEPLOYMENT_PATH_PREFIX, enforced only by this docstringGenerated paths carry
settings.API_DEPLOYMENT_PATH_PREFIX(backend/backend/base_urls.py:20), read from the environment at import with a default ofdeployment. A developer whose environment overrides it sees the drift test fail, regenerates as instructed, and commits a spec whose paths carry a private prefix — the test then passes on the wrong artifact and the downstream SDK repos are generated from it.Low because nothing in-repo sets the variable (no
.envsample, no compose file, and the rig'sbackend_test_envdoesn't pin it) and a prefix change is visible in thespecs/diff.Fix: assert the rendered path prefix equals the default, or override the setting for the duration of
render_spec().