Proposal: Commit the drf-spectacular generated OpenAPI spec in git

[ChristopherChudzicki]

Description

tldr: I propose that Open5e-api commit its drf-spectacular generated OpenAPI spec in repo to improve visibility in PRs and enable tracking in git. A simple CI check + git attribute can ensure it does not get out of sync.

Current Spec Tooling: DRF-Spectacular

Open5e uses drf-spectacular to generate its OpenAPI spec. The OpenAPI spec is served at

• https://api.open5e.com/schema/swagger-ui/ with swagger ui
• https://api.open5e.com/schema/ as raw json

Generally, drf-spectacular generates the spec:

1. By introspecting models/serializers/viewsets/filtersets (~90% of spec)
2. With custom overrides provided by @extend_schema_field and other decorators. (extra ~10% manual work)

Where drf-spectacular warns, or manual work has an error/inaccuracy, the spec can be wrong.

Proposal

I propose that Open5e commit the YAML version of its OpenAPI spec. Provided that the spec stays in-sync with the code, making the spec visible in Git history is very useful:

1. Changes over time, particularly in PRs, are visible to reviewers
   • YAML version of spec is fairly easy to read.
   • This is, IMO, a big win. It's not always obvious how a serializer/model change will affect the spec. (E.g., this simple looking edit changed weapon.properties from a properly typed Array<WeaponPropertyAssignmentSerializer> to a string.¹)
   • And it makes reviewing a spec-fix PR very easy. Contributor adds decorator or makes trivial refactor to fix spec -> you see the change.
   • Very easy to see how a drf-spectacular upgrade will affect your spec.
2. Becomes easy to add a CI check that detects breaking changes, e.g., by using a tool like https://github.com/oasdiff/oasdiff to compare the spec on staging vs PR branch and detect if fields were removed, or changed type, etc.
   • In practice, I'm not sure I would recommend doing this, at least not as a CI check that can fail. It would treat a lot of spec bugfixes as breaking changes. (Like the fix above, or many places where the open5e code has a comment "this property should be nullable in spec")
3. You can generate frontend API clients against different branches, if you use generated clients for developing your frontend.

Keeping the spec in-sync

Committing the spec safely requires a way to ensure it stays up-to-date.

Concern 1: Author forgot to regenerate the spec.

Simple solution:

1. Github Action: Generate the spec in a github action. Does the committed spec match? If yes, then it's completely up-to-date at the moment CI checks run.
   • Note: Github's pull_request event provides a merge ref containing the PR branch already merged into the base branch; actions/checkout uses that ref by default, so spec is truly up-to-date at this moment.
   • Freshness action can show the diff plus message nudging you to regenerate.

Concern 2: Stale CI Check, 2 branches modify spec: However, there is still the possibility that two branches A and B both change the spec and both have up to date specs at the moment CI runs, and A is merged into staging first. Now B's spec is out-of-date.

Another simple solution:

2. Git attribute: Mark the spec as openapi-schema.yml text eol=lf merge=binary in git attributes:
   • merge=binary forces a conflict if pr-branch and main have both modified the spec. It prevents textual merges, so the file must be regenerated afresh.
   • eol=lf ensures cross-platform stability of the generated spec, which CI relies on (so that devs generating on Windows match CI)

This eliminates the "Stale CI Check, 2 branches modify spec" concern.

Concern 3: Spec not changed, but affected by base branch: Again, the CI check guarantees the branch is completely up-to-date at the time CI runs. There is one remaining concern that since the check ran, base (staging) has (A) not changed the spec itself, but (B) changed in some way that affects spec generation.

This can be prevented by enabling merge queue or "Require branch up to date before merging" in github. Both of these checks have their own pros/cons.

However: In practice, this seems quite rare. We've used the CI check + git attribute for ~ 2 years in https://github.com/mitodl/mit-learn ² and never run into this.

Additionally, any spec drift caused by Concern 3 is self-healing. At worst, the next CI check that runs against staging, on any PR, would detect the drift.

Recommendation: Commit the spec, do CI check + git attribute. Skip merge queue / require-up-to-date-before-merging unless you want it for other reasons.

Demonstration

I thought this feature would be easiest to understand / see with some PRs demonstrating it, so:

• feat: commit openapi-schema.yml + add freshness gate #951 This PR into open5e-api commits the spec and adds the CI check + git attribute, as described above.
• Then, demos of the spec diff + ci check working:
  • DEMO 1 (fix: type weapon.properties correctly in OpenAPI spec ChristopherChudzicki/open5e-api#4): PR against ☝️ fixing the weapon.property issue
  • DEMO 2 (demo: dangling Race -> Species ref fix (intentionally stale spec, then regen) ChristopherChudzicki/open5e-api#1): a real Race → Species bugfix where the contributor (me):
    1. "forgot" to regenerate the spec, watched CI fail with the regen banner
    2. then pushed the regenerated spec, CI now passes

Footnotes

1. It needs an @extend_schema_field(WeaponPropertyAssignmentSerializer(many=True)) decorator. ↩

2. The above proposal is based on what we do at my work, mit-learn is one of our repos. You can see our spec check at https://github.com/mitodl/mit-learn/blob/main/.github/workflows/openapi-diff.yml and our git attributes at https://github.com/mitodl/mit-learn/blob/main/.gitattributes#L9. ↩

[augustjohnson]

I am confident you are on to something here, and I follow the basic logic, but one premise I think I'm missing is basically this:

1. If the code itself is the source for the logic on the openapi spec, and the spec is deterministically generated each time (as part of the comparison), what purpose does the spec file itself uniquely serve?

[ChristopherChudzicki]

This got longwinded, sorry 😄.

what purpose does the spec file itself uniquely serve?

tldr: I think the main purpose / value is visibility, especially in PR reviews. The spec is your API's contract. Make it visible. Example: This branch demo: prefixed_aggregate_serializer helper for skill_bonuses and other aggregated fields (after a bit more polish) would be worth merging, IMO. But it has a big effect on your spec, and making that visible to reviewers is useful. If a PR affects the spec, you want to see it.

Also: I want to acknowledge this isn't clear cut. As you say, the spec is generated, so it does not need to be committed. I do think committing the spec is useful, but I completely understand if you all decide to reject that path.

---

I think there are basically two ways specs end up being generated:

1. A single explicit source of truth.: I haven't used FastAPI much, but my understanding is that FastAPI generates OpenAPI specs based on Pydantic schemas. Schemas used for validation AND spec generation. The source of truth for the spec is very clear cut: Pydantic schema. Period. End of story.
2. Introspection: DRF-Spectacular does this. It introspects viewsets, serializers, models, filtersets, and uses hooks to generate the spec. The path from code change -> spec change is not always clear.

I would not commit the spec in Case 1, where pydantic models are a clear source of truth for the spec.

But in Case 2, I do see value:

Pros:

• Visibility—Reviewing PRs: If you're reviewing a PR, you can easily see how the code change affects the spec. As an example of this, demo: prefixed_aggregate_serializer helper for skill_bonuses and other aggregated fields ChristopherChudzicki/open5e-api#6 ... This is a code change that heavily affects the spec. Seeing the effect on the spec is useful.
  • related: DRF-Spectacular's introspection produces more correct specs than extend_schema (since based on code), but is less explicit. You may be more comfortable relying on the introspection when you commit the spec.
• Visibility—Issue Creation: If someone finds a problem with the spec, it's real easy to create an issue referencing the exact line in the spec.
• Creating Clients on Branches: You could create a Typescript/Python/Whatever API client (e.g., with https://github.com/openapitools/openapi-generator) for different commits (think, staging env + release env) without running python.
  • I don't think this is relevant for this project right now... More of a btw.

The cons I see are:
Cons:

• Staleness: Spec could get stale. Eliminated by CI check + merge=binary git attribute.
  • with caveat that it's only perfectly eliminated if you enforce "branch must be up to date before merging", but that causing issues seems extremely unlikely to me in practice, for reasons described in original post.
• Artificially large PR Diff: A small code change could produce a huge spec change.
  • IMO, this is really a pro. You'd want to know, wouldn't you?
• Friction: Contributors need to regenerate it via uv run python manage.py spectacular --file openapi-schema.yml else CI will fail. CI errors help remind you to do that, but contributors still need to actually do it.
  • maybe worth a bash script to make this smoother.

Since staleness is handled by CI + git attribute, I think contributor friction is the only main downside.

I can't speak to how common this "commit the generated spec" procedure is in practice. AI tells me Kubernetes does something similar, and I know we do it at my work, but I will say, I suspect it is not what most drf-spectacular projects do.