Publish a per-tenant OpenAPI spec, off #36 and onto main

.github/workflows/ci.yml

@@ -31,4 +31,9 @@ jobs:
       - run: uv run ruff check .
       - run: uv run ruff format --check .
       - run: uv run mypy src
+      # tests/test_openapi_spec.py already asserts the spec is current. This
+      # step is here for the other half: nothing else runs the generator
+      # itself, and a build-time script that only ever runs by hand is one
+      # that breaks unnoticed and is discovered when someone needs it.
+      - run: uv run bin/generate-openapi-spec --check
       - run: uv run pytest --cov=ol_analytics_api --cov-report=term-missing

.github/workflows/openapi-diff.yml

@@ -0,0 +1,139 @@
+name: OpenAPI Diff
+
+# The committed spec is meant to be what a future Concourse client pipeline
+# generates a published TypeScript package from (see README.md), so a diff
+# here is a preview of a change to somebody else's build. This surfaces that
+# change as a comment and fails the PR on a breaking one, rather than leaving
+# it to whoever reads 1500 lines of YAML.
+
+on:
+  pull_request:
+    paths:
+      - "openapi/specs/**"
+
+permissions: {}
+
+jobs:
+  openapi-diff:
+    runs-on: ubuntu-24.04
+    permissions:
+      contents: read
+      pull-requests: write
+    steps:
+      - name: Checkout HEAD
+        uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
+        with:
+          # The exact commit under review, not the branch name: a push while
+          # this runs would otherwise diff a commit nobody reviewed.
+          ref: ${{ github.event.pull_request.head.sha }}
+          path: head
+          persist-credentials: false
+      - name: Checkout BASE
+        uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
+        with:
+          ref: ${{ github.event.pull_request.base.sha }}
+          path: base
+          persist-credentials: false
+      - name: Generate oasdiff changelog
+        run: | # Write the comment body to a file rather than a step output.
+          # A large changelog interpolated into a JS action's `body:` input becomes a
+          # huge INPUT_BODY env var, which can blow past the OS argv+envp size limit
+          # and crash the action with "Argument list too long". Writing straight to a
+          # file and using `body-path` avoids that entirely.
+          #
+          # The spec list is the union of base and head filenames, not just base's:
+          # a base-only loop silently drops both a spec added in this PR (never in
+          # base, so never iterated) and a spec removed in this PR (caught by the
+          # -f guard below, so skipped instead of reported as a removal).
+          specs=$(
+            {
+              [ -d base/openapi/specs ] && (cd base/openapi/specs && ls -1 ./*.yaml)
+              [ -d head/openapi/specs ] && (cd head/openapi/specs && ls -1 ./*.yaml)
+            } 2>/dev/null | xargs -n1 basename | sort -u
+          )
+          {
+            echo "## OpenAPI Changes"
+            echo ""
+            echo "<details>"
+            echo "<summary>Show/hide changes</summary>"
+            echo ""
+            echo '```'
+            for name in $specs; do
+              base_spec="base/openapi/specs/$name"
+              head_spec="head/openapi/specs/$name"
+              if [ -f "$base_spec" ] && [ -f "$head_spec" ]; then
+                echo "## Changes for $name:"
+                docker run --rm \
+                  --workdir "$GITHUB_WORKSPACE" \
+                  --volume "$GITHUB_WORKSPACE:$GITHUB_WORKSPACE:ro" \
+                  tufin/oasdiff@sha256:6065c16a4c9ce12504752f444d4981091e58c2a35436fac90b649be47d833db3 \
+                  changelog "$base_spec" "$head_spec"
+                echo ""
+              elif [ -f "$head_spec" ]; then
+                echo "## $name: added"
+                echo ""
+              elif [ -f "$base_spec" ]; then
+                echo "## $name: removed"
+                echo ""
+              fi
+            done
+            echo '```'
+            echo ""
+            echo "Unexpected changes? Ensure your branch is up-to-date with \`main\` (consider rebasing)."
+            echo "</details>"
+          } > comment_body.md
+      # A fork's GITHUB_TOKEN is read-only whatever this job asks for, so both
+      # comment steps would fail and take the breaking-change check below down
+      # with them. The check is the gate; the comment is a convenience. Skip
+      # the convenience rather than lose the gate.
+      - name: Find existing comment
+        id: find_comment
+        if: github.event.pull_request.head.repo.full_name == github.repository
+        uses: peter-evans/find-comment@b30e6a3c0ed37e7c023ccd3f1db5c6c0b0c23aad # v4
+        with:
+          token: ${{ secrets.GITHUB_TOKEN }}
+          repository: ${{ github.repository }}
+          issue-number: ${{ github.event.pull_request.number }}
+          body-includes: "## OpenAPI Changes"
+      - name: Post changes as comment
+        uses: peter-evans/create-or-update-comment@e8674b075228eee787fea43ef493e45ece1004c9 # v5
+        # Even with no changes, update the old comment if one was found.
+        if: github.event.pull_request.head.repo.full_name == github.repository
+        with:
+          token: ${{ secrets.GITHUB_TOKEN }}
+          edit-mode: "replace"
+          repository: ${{ github.repository }}
+          issue-number: ${{ github.event.pull_request.number }}
+          comment-id: ${{ steps.find_comment.outputs.comment-id }}
+          body-path: comment_body.md
+      - name: Check for breaking changes
+        run: |
+          # Breaking here means breaking a client someone else already
+          # generated and shipped, so this fails the PR rather than warning.
+          # A spec removed outright is the most breaking change there is —
+          # deleting the whole published API for a tenant — so it's checked
+          # explicitly rather than relying on the -f guard to skip it.
+          specs=$(
+            {
+              [ -d base/openapi/specs ] && (cd base/openapi/specs && ls -1 ./*.yaml)
+              [ -d head/openapi/specs ] && (cd head/openapi/specs && ls -1 ./*.yaml)
+            } 2>/dev/null | xargs -n1 basename | sort -u
+          )
+          for name in $specs; do
+            base_spec="base/openapi/specs/$name"
+            head_spec="head/openapi/specs/$name"
+            if [ -f "$base_spec" ] && [ -f "$head_spec" ]; then
+              echo "Checking $name for breaking changes..."
+              docker run --rm \
+                --workdir "$GITHUB_WORKSPACE" \
+                --volume "$GITHUB_WORKSPACE:$GITHUB_WORKSPACE:ro" \
+                tufin/oasdiff@sha256:6065c16a4c9ce12504752f444d4981091e58c2a35436fac90b649be47d833db3 \
+                breaking \
+                --fail-on ERR \
+                --format githubactions \
+                "$base_spec" "$head_spec"
+            elif [ -f "$base_spec" ]; then
+              echo "::error::$name was removed — deleting a published spec is a breaking change."
+              exit 1
+            fi
+          done

README.md

@@ -98,20 +98,29 @@ state. Wire it up with one entry in `main.py`'s `TENANTS` list:
 
 ```python
 TENANTS: list[Tenant] = [
-    Tenant("/api/v1/analytics", b2b_dashboard.create_app, b2b_dashboard.lifespan),
-    Tenant("/api/v1/<new-tenant>", new_tenant.create_app),
+    Tenant(
+        b2b_dashboard.TENANT_NAME,
+        "/api/v1/analytics",
+        b2b_dashboard.create_app,
+        b2b_dashboard.lifespan,
+    ),
+    Tenant(new_tenant.TENANT_NAME, "/api/v1/<new-tenant>", new_tenant.create_app),
 ]
 ```
 
 A `Tenant` takes a `create_app` *factory* (not a pre-built instance) so the
 root app constructs every sub-app after OpenTelemetry is configured — a
 tenant is instrumented regardless of import order. If the tenant owns
 resources that need startup/shutdown (e.g. an httpx client), it exposes them
-as an ordinary `lifespan` context manager and passes it as the third
+as an ordinary `lifespan` context manager and passes it as the fourth
 argument: a mounted sub-app's own `lifespan=` is never invoked by the ASGI
 server (only the root app's is), so the root lifespan enters each tenant's
 explicitly.
 
+The leading `name` is the tenant's own `TENANT_NAME`, which already names its
+readiness sub-path. It also names the tenant's published OpenAPI document
+(`openapi/specs/<name>.yaml`), so it ends up in a consumer-visible filename.
+
 Each tenant gets independent OpenAPI docs at `<mount-path>/docs`.
 
 ### Auth
@@ -208,3 +217,49 @@ uv run pytest
 uv run ruff check .
 uv run mypy src
 ```
+
+## The published API contract
+
+Each tenant's OpenAPI document is committed under `openapi/specs/<tenant>.yaml`
+and regenerated with:
+
+```bash
+uv run bin/generate-openapi-spec
+```
+
+Run it whenever a response model, route or query parameter changes. CI fails
+otherwise — both as a test (`tests/test_openapi_spec.py`) and as a
+`--check` run of the generator itself.
+
+The spec is committed rather than served-and-forgotten because it is meant to
+become a cross-repo interface. The intended pipeline mirrors the one already
+running for `mitxonline` and `mit-learn`: a Concourse pipeline in
+`ol-infrastructure` (`ol_concourse/pipelines/libraries/api_clients_pipeline.py`)
+watching these files on a release branch, running `openapi-generator` over
+them, and publishing a TypeScript client the same way
+`@mitodl/mitxonline-api-axios` and `@mitodl/mit-learn-api-axios` are today.
+None of that is wired up yet — this repo has no entry in `PIPELINE_CONFIGS`
+and no `release` branch, and MIT Learn's dashboard still uses its hand-written
+client. Until it is, committing the spec still buys the same thing locally: a
+column that appears here without appearing in the diff is a column a
+consumer would find out about at runtime once the pipeline exists.
+
+Three details are worth knowing before editing a route:
+
+- **`operation_id` is named explicitly on every route.** It becomes the
+  generated client's method name, so FastAPI's path-derived default would both
+  produce an unreadable name and rename the method whenever the path moves.
+- **Published paths carry the tenant's mount prefix.** A mounted sub-app
+  describes its routes relative to its own root; `openapi.py` re-prefixes them
+  so a generated client configured with the service host requests the URLs the
+  service actually serves.
+- **A repeatable query parameter is a plain `list[X]`, never `list[X] | None`.**
+  The optional form renders as `anyOf: [array, null]`, which openapi-generator
+  cannot reduce; it emits a client that spreads the value with `Object.entries`
+  and sends `?0=a&1=b` instead of repeating the parameter name. Use
+  `Query(default_factory=list)` and treat the empty list as "no filter".
+
+Note that `docs/openapi/b2b-learner-records-v1.yaml` is a different artifact:
+a hand-written draft published so partners could review the record shape
+before it was built. `openapi/specs/b2b_learner_records.yaml` is generated
+from the running code and is the one a client is built from.

bin/generate-openapi-spec

@@ -0,0 +1,68 @@
+#!/usr/bin/env python3
+"""Write each mounted tenant's OpenAPI document to openapi/specs/<tenant>.yaml.
+
+Run as `uv run bin/generate-openapi-spec`.
+
+The output is committed, and that is the point: a materialized view gaining or
+renaming a column changes a response model, which changes this file, which
+shows up in review as an interface diff instead of silently drifting away from
+the clients generated off it. `tests/test_openapi_spec.py` fails when the
+committed file no longer matches what the code produces.
+
+The intended consumer is a Concourse pipeline in ol-infrastructure
+(`ol_concourse/pipelines/libraries/api_clients_pipeline.py`), mirroring the one
+mitxonline and mit-learn already use: watch `openapi/specs/*.yaml` on a
+release branch and regenerate the published TypeScript client from it. That
+pipeline isn't wired up for this repo yet (no `PIPELINE_CONFIGS` entry, no
+`release` branch) — this file exists so the spec is ready to publish once it
+is.
+"""
+
+from __future__ import annotations
+
+import sys
+from pathlib import Path
+
+import cyclopts
+
+from ol_analytics_api.openapi import render, tenant_specs
+
+DEFAULT_DIRECTORY = Path("openapi/specs")
+
+app = cyclopts.App(name="generate-openapi-spec", help=__doc__)
+
+
+@app.default
+def generate(*, directory: Path = DEFAULT_DIRECTORY, check: bool = False) -> None:
+    """Write (or, with --check, verify) the per-tenant OpenAPI documents.
+
+    Parameters
+    ----------
+    directory
+        Where the <tenant>.yaml files are written.
+    check
+        Compare against what is already on disk and exit non-zero on any
+        difference, without writing anything.
+    """
+    stale = []
+    for tenant_name, spec in tenant_specs().items():
+        path = directory / f"{tenant_name}.yaml"
+        rendered = render(spec)
+        if check:
+            if not path.exists() or path.read_text() != rendered:
+                stale.append(path)
+            continue
+        path.parent.mkdir(parents=True, exist_ok=True)
+        path.write_text(rendered)
+        sys.stdout.write(f"wrote {path}\n")
+    if stale:
+        names = ", ".join(str(path) for path in stale)
+        sys.stderr.write(
+            f"OpenAPI spec is out of date: {names}. "
+            "Regenerate with `uv run bin/generate-openapi-spec`.\n"
+        )
+        raise SystemExit(1)
+
+
+if __name__ == "__main__":
+    app()