typesafe-api-docs - #1368
Draft
RichDom2185 wants to merge 20 commits into
Draft
Conversation
What I built and verified this session
Per your decisions (full remediation, full open_api_spex migration, flag the dead routes), I established the foundation and proved the full
pattern end-to-end — everything green:
- Phase 0 safety net (open_api_spex 3.22.3): CadetWeb.ApiSpec builds the OpenAPI 3.0 doc from the router; PutApiSpec in the :api pipeline; a
mix openapi.spec generator; and test/cadet_web/api_spec_test.exs with three gates — spec-builds/serializes, a dead-route guard (permanently
catches the 2 dead routes + any future typo), and a completeness burn-down.
- Two controllers fully migrated as the template — IncentivesController (student) and AdminAchievementsController (admin, reusing the
Achievement schema): schemas as CadetWeb.Schemas.* modules, operation specs with full error paths (401/403/404) via a shared ErrorResponses
helper, and response bodies validated against the schemas in tests.
- The generated spec passes openapi-generator validate with zero issues (vs. the old spec's 44+5 invalidities). 243+ existing tests still
pass; format and credo clean.
"Code generation for safety" — now concretely realized
The spec is now code, so three guarantees hold that didn't before:
1. Valid by construction — referencing a non-existent schema is a compile error, and generation resolves every ref.
2. Runtime request validation (Phase 3, CastAndValidate) is wired-ready.
3. Response conformance in tests — and it already caught a real bug: Achievement.release/deadline come back null, so the schema needed
nullable: true.
Two useful discoveries refined the plan: un-migrated controllers are gracefully skipped, so migration is safely incremental; and paths come
from the router automatically, structurally eliminating the :course_id/{course_id} drift class of bug. The new spec generates to
priv/static/openapi.json during migration (the old compiler still owns swagger.json), reconciled at cutover.
What remains
This is inherently multi-PR. The repeatable recipe and remaining work are recorded in the plan file. Left to do: 26 more controllers
(heaviest are assessments and the admin grading/courses/user controllers, which introduce shared schemas like
CourseConfiguration/SourceVariant), then Phase 3 (roll out CastAndValidate + an error renderer to preserve the plain-text 400 contract),
Phase 4 (broaden response assertions), and Phase 5 (cutover: remove phoenix_swagger + its compiler, reconcile openapi.json↔swagger.json, add
the CI --check gate).
Want me to keep migrating controllers in this session (next logical batch: the small self-contained ones — notifications, sourcecast,
stories — then the shared-schema user/courses pair), or pause here so you can review the foundation and the two-controller template first?
Phase 2 progress: 16 of 28 controllers migrated
Everything is green — full web suite (470 tests), mix format, mix credo, and openapi-generator validate (zero issues). The generated spec is
now 37 paths / 42 operations / 50 schemas.
Migrated this session (14 new, on top of the original 2):
- Student/public: Notifications, Stories, Leaderboard, Team, Sourcecast, Chat, RagChat (closed a documentation gap), Courses, User, Answer,
Versions
- Admin: AdminGoals, AdminStories, AdminSourcecast
Contract bugs fixed along the way (the old docs lied; the new spec matches reality, verified by assert_schema in tests):
- CoursesController.index really returns {config: {…camelCase…}} — old schema had unwrapped snake_case
- UserController.update_latest_viewed body is {courseId}, not course_id; deeply-nested nullable user responses now modeled with
allOf+nullable
- ChatController.chat is POST /chats/message (old doc said PUT /chat) with a 3-field body, not a bare list
- Team/AdminGoals/AdminAchievements bodies are {goals:…}/{achievement:…} wrappers, not bare arrays
- Nullable release/deadline on achievements
The gate caught a real bug: I initially omitted the scope's course_id path param from the Answer/Versions operations — openapi-generator
validate flagged 6 errors. Fixed, and I strengthened api_spec_test.exs with a path-parameter lint so mix test now catches this class
in-suite (not just the external validator).
Remaining (12 controllers)
Devices, Auth, AICodeAnalysis, AdminTeams (+ its flagged dead route), AdminUser, AdminAssets, AdminCourses, and the heavy
Assessments/Grading family (Assessments, AdminAssessments, AdminGrading) which share the richest schemas in the codebase
(Assessment/Question/Submission/grading). Default/JWKS stay intentionally exempt. After these come Phase 3 (runtime CastAndValidate), Phase
4 (broaden response assertions), and Phase 5 (cutover: remove phoenix_swagger, reconcile the filename, CI gate).
Want me to keep going straight through the remaining 12 (I'll continue the same batch-and-verify rhythm, saving the Assessments/Grading
giants for last), or pause here so you can review the 16 migrated controllers and the strengthened gate first?
Phase 2 complete — all 26 controllers migrated
The whole open_api_spex migration is done and green:
┌───────────────────────────┬───────────────────────────────────────────────────────────────────────────────────────────────────────────┐
│ Metric │ Result │
├───────────────────────────┼───────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ Controllers migrated │ 26 / 26 documentable (Default/JWKS intentionally exempt) │
├───────────────────────────┼───────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ Generated spec │ OpenAPI 3.0.0 — 90 paths, 103 operations, 86 schemas │
├───────────────────────────┼───────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ openapi-generator │ No validation issues │
│ validate │ │
├───────────────────────────┼───────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ Old-spec defects │ bare-string schemas 0 (was 44); multi-body ops 0 (was 5); :param paths 0 (was 3) │
│ remaining │ │
├───────────────────────────┼───────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ Tests │ full web suite 461 passing; mix format clean; mix credo at parity-or-better than master (0 new blocking │
│ │ issues) │
└───────────────────────────┴───────────────────────────────────────────────────────────────────────────────────────────────────────────┘
The safety-net gate (test/cadet_web/api_spec_test.exs, 4 tests) now enforces, on every mix test:
1. the spec builds and serialises,
2. blanket completeness — every documentable route has an operation (the @migrated_controllers burn-down list is retired),
3. a path-parameter lint — every {param} is declared (this caught a real missing-course_id bug that only the external validator had
flagged),
4. a dead-route guard — every routed action exists (the 2 known dead routes are allowlisted and FIXME'd in-code).
Remediation delivered (verified by assert_schema response validation on ~10 controllers): fixed courseId, the {config:{…}} response
wrapping, the {goals:}/{achievement:}/{team:} body wrappers, update_role/delete_user path-vs-body confusion, chat's real POST
/chats/message, nullable fields, and the missing scope course_id; documented the 3 previously-undocumented controllers (incl. RagChat) and
the 3 router TODOs; added error paths throughout; and flagged the 2 dead routes.
What remains (separate phases, per the plan)
- Phase 3 — turn on runtime CastAndValidate (+ an error renderer to preserve the plain-text/400 contract); watch the camelCase-vs-snake_case
caveat on the courses/config bodies.
- Phase 4 — broaden assert_schema beyond the controllers already asserting responses.
- Phase 5 — cutover: remove phoenix_swagger (dep + :phoenix_swagger compiler + config/* entries + Router.swagger_info/0), reconcile
openapi.json ↔ swagger.json, swap /swagger to OpenApiSpex.Plug.SwaggerUI, and add the CI --check freshness gate. Since no controller-level
phoenix_swagger remains, this is dependency/config cleanup, not per-controller work.
Want me to continue into Phase 3 (runtime request validation), or stop here so you can review the completed spec migration first?
Phase 3 complete — runtime request validation is live
OpenApiSpex.Plug.CastAndValidate now runs on all 26 controllers, so malformed requests are rejected at the edge against the spec. Everything
is green: full web suite (461 tests), spec still validates externally, format clean, credo at parity.
The key obstacle and fix. CastAndValidate validates the request content-type against the operation's requestBody. Production sends
application/json, but Phoenix's test helper posts map bodies as multipart/mixed — so validation rejected nearly every POST/PUT test. I fixed
this once, centrally in test/support/conn_case.ex by overriding post/put/patch to send map bodies as JSON (with passthrough for
%Plug.Upload{} multipart and non-encodable bodies). This made the tests production-representative and drove failures from 52 → 0.
Validation philosophy. CastAndValidate acts as a structural edge gate — it enforces content-type, path/query param types (e.g.
/courses/abc/... is now rejected), and typed body fields — while required-field and business rules stay in the actions (which keep their own
error messages). A custom OpenApiErrorRenderer preserves the app's plain-text 400 contract. Where strict schemas would have broken existing
behavior, I relaxed request-body required/polymorphic fields (answer, gameStates, assessmentConfigs, etc.); a couple of
genuinely-invalid-input tests (bad viewable type, null username) were updated to expect the new edge-400.
Sensible exclusions (via plug ... when action not in [...]): the three multipart uploads, the wildcard AdminAssets.delete (*filename), and
the secret-based Devices routes (they bypass the :api pipeline, so PutApiSpec never runs).
Bonus: fixed a real doc bug — AuthController.create's params were documented as query but are sent in the body (now a LoginRequest body).
Remaining
- Phase 4 — broaden assert_schema/assert_operation_response in tests (already on ~10 controllers).
- Phase 5 — cutover: remove phoenix_swagger (dep + compiler + config + Router.swagger_info/0), reconcile openapi.json ↔ swagger.json, swap
/swagger to OpenApiSpex.Plug.SwaggerUI, add the CI --check gate.
Want me to continue with Phase 4, or go straight to the Phase 5 cutover (which fully retires phoenix_swagger)?
⏺ Phase 4 complete — response validation is now broad, and it earned its keep
I added assert_operation_response(conn) to happy-path JSON tests across 13 controllers. This primitive is the real drift detector: it
auto-resolves the operation from the conn (via CastAndValidate) and validates the actual response body against the operation's declared
response schema — no api_spec plumbing or schema-title lookup needed.
59 assertions added, and validating real responses immediately exposed 6 response-schema doc bugs where the OpenAPI doc lied about what the
API returns:
┌───────────────────┬───────────────────┬───────────────────────────────────────────────────────────────────────────────────────────────┐
│ Schema │ Declared │ Reality → Fix │
├───────────────────┼───────────────────┼───────────────────────────────────────────────────────────────────────────────────────────────┤
│ Question.grader │ non-null object │ null when ungraded → nullable allOf │
├───────────────────┼───────────────────┼───────────────────────────────────────────────────────────────────────────────────────────────┤
│ Library.globals │ array │ a map → type: object │
├───────────────────┼───────────────────┼───────────────────────────────────────────────────────────────────────────────────────────────┤
│ Library.variant │ string │ can be null → nullable │
├───────────────────┼───────────────────┼───────────────────────────────────────────────────────────────────────────────────────────────┤
│ Question.solution │ integer │ string (programming) or int (mcq) → nullable oneOf │
├───────────────────┼───────────────────┼───────────────────────────────────────────────────────────────────────────────────────────────┤
│ Sourcecast.audio │ filename string │ Waffle attachment object → type: object │
├───────────────────┼───────────────────┼───────────────────────────────────────────────────────────────────────────────────────────────┤
│ AdminGrading ×4 │ bare │ replaced with faithful models (reusing Library/Testcase/GraderInfo), which surfaced further │
│ │ %Schema{object} │ real drift: polymorphic answer/team, many nullable refs, preserved snake_case keys │
└───────────────────┴───────────────────┴───────────────────────────────────────────────────────────────────────────────────────────────┘
The admin_grading responses had been bare object placeholders that validated nothing — now the four richest grading endpoints are properly
typed.
Verification (all green):
- Full web suite: 461 tests, 0 failures
- mix openapi.spec regenerates → openapi-generator validate: No validation issues detected (the new oneOf/allOf/nullable + grading schemas
serialize to valid OpenAPI 3.0)
- mix format clean; mix credo --strict clean on all changed lib files
Remaining: Phase 5 (the cutover)
This is the last phase — dependency/config cleanup, no per-controller work:
- Remove phoenix_swagger (dep, the :phoenix_swagger compiler, config/config.exs + config/dev.exs entries, Router.swagger_info/0)
- Reconcile priv/static/openapi.json ↔ the old swagger.json filename and swap /swagger to OpenApiSpex.Plug.SwaggerUI
- Add the CI freshness gate (mix openapi.spec.json --check)
Note this flips the served document from Swagger 2.0 → OpenAPI 3.0 at the same URL, which the frontend consumes — so it's the one phase with
a coordination/consumer impact. Want me to proceed with Phase 5?
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
This was referenced Jul 28, 2026
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is part 3 of 3 in a stack made with GitButler: