fix: improve generated OpenAPI spec quality for SDK generation#1215
Draft
grdsdev wants to merge 14 commits into
Draft
fix: improve generated OpenAPI spec quality for SDK generation#1215grdsdev wants to merge 14 commits into
grdsdev wants to merge 14 commits into
Conversation
Every generated operation was missing operationId, which breaks SDK method naming in every OpenAPI code generator. Route configs already carry a stable, unique operation name (ROUTE_OPERATIONS) used for observability - derive operationId from it via a swagger transform instead of duplicating names per route.
Fastify's catch-all route segment is `*`, and @fastify/swagger mirrored it verbatim into the spec as a path param literally named `*` - not a legal identifier for any SDK generator. Rewrite it to `wildcard` for docs only; the raw route/schema Fastify validates against is untouched.
/bucket, /health, and /s3/{Bucket} were each documented twice (with
and without a trailing slash) because Fastify's exposeHeadRoutes and
some routes' explicit dual registration both genuinely respond on
either form. Fold the trailing-slash variant's methods into the
slash-less path via transformObject so SDKs only see one operation
per real endpoint.
Also hardens the operationId dedup from the previous commit: it only
tried one method-based suffix, which wasn't enough once two GET routes
(and their auto-derived HEAD counterparts) legitimately share the same
config.operation - loop with a numeric suffix until unique instead.
bucketSchema and objectSchema were already shared TS constants with their own $id, but every route embedded them by value instead of by $ref, so the spec had no reusable models at all outside the two globally addSchema'd ones (authSchema/errorSchema) - every response body was duplicated inline per operation. Register them via app.addSchema and switch the 5 call sites that embedded them by value to $ref instead (including objectSchema's own nested `buckets` field). Also make @fastify/swagger name components after their $id (bucketSchema, objectSchema) instead of the default opaque def-0/def-1 numbering.
Every operation only ever documented a generic 4XX (main API) or nothing at all (admin API) for errors, so a generated SDK gets exactly one exception type regardless of what actually happened. The JWT and admin API-key plugins already have a single onRoute hook that applies the security scheme to every route they guard - extend that same hook to also document the specific, code-verified status each one throws (403 for invalid JWT/role in jwt.ts, 401 for a bad admin apikey), since that's true for every route behind it without needing to touch each route file individually. Guards against dropping @fastify/swagger's default "200: Default Response" placeholder on routes that don't declare their own response schema, which setting schema.response unconditionally would otherwise suppress.
The S3-compatible surface dispatches ~18 real commands (PutObject, ListObjects, CreateMultipartUpload, ...) by matching query string/headers inside the internal s3/router.ts Router - Fastify (and therefore @fastify/swagger) only ever sees the outer catch-all route, with no request/response schema, since OpenAPI has no way to express "N operations, same path and method, picked by a query parameter". Documenting that catch-all as-is produced one method per path/verb with an untyped body and an untyped response standing in for the entire S3 API - worse for SDK generation than not documenting it. Hide it instead and point the s3 tag description at the real per-command schemas in s3/commands/*.
50 of 62 operations had a summary but no description, so generated SDK doc comments were empty for most methods. Add one-line descriptions across 27 route files, focused on what isn't already obvious from the summary: auth flow differences (public/authenticated/ signed-URL variants), pagination style (cursor vs limit/offset), side effects (CDN purge scope, x-upsert), and TUS protocol semantics per step.
Coverage Report for CI Build 29018018528Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage increased (+0.002%) to 78.997%Details
Uncovered Changes
Coverage Regressions102 previously-covered lines in 10 files lost coverage.
Coverage Stats
💛 - Coveralls |
…e: true
The document declares openapi: 3.0.3 but 4 properties used the
array-form nullable type (type: ["integer", "null"]), which is only
valid under OpenAPI 3.1/JSON-Schema-2020-12 - 3.0.x requires a plain
type plus nullable: true, which this codebase already uses elsewhere
(e.g. createBucket's file_size_limit). swift-openapi-generator hard-
fails parsing on the array form.
Fixes bucketSchema.file_size_limit/allowed_mime_types and the
GET /object/sign/{bucketName} response's error/signedURL fields.
…e: true
Same problem as the previous commit's type-array fix, different
spelling: anyOf: [<schema>, {type: 'null'}] is the other
JSON-Schema-2020-12/OpenAPI-3.1 way of expressing nullable, and it's
equally invalid under this document's declared openapi: 3.0.3.
swift-openapi-generator failed with "Cannot initialize JSONType from
invalid String value null" on objectSchema.id.
Fixes objectSchema's id/updated_at/created_at/last_accessed_at/
metadata/user_metadata. copyObject.ts spreads objectSchema.properties
by reference into its own response schema, so this single fix also
resolves the identical anyOf-null occurrences at the /object/copy
response - confirmed via a full structural scan of the generated
document for both the type-array and anyOf-null shapes (zero
remaining either way).
uploadSchema (upload.ts) and multipartUploadSchema/uploadPartSchema
(multipart.ts) have the same anyOf-null pattern but aren't referenced
by any HTTP route, so they never reach the generated spec - left
alone as out of scope for this fix.
The body already declared minProperties: 1 and a normal properties object (public/file_size_limit/allowed_mime_types), but also a top-level anyOf requiring at least one of those three - which swift-openapi-generator treats as anyOf-driven, dropping the co-located properties entirely and generating a body type with every field permanently nil. Verified before removing: minProperties: 1 alone is NOT actually equivalent, since additionalProperties isn't restricted here - a request body containing only unrecognized fields would satisfy minProperties without providing public/file_size_limit/ allowed_mime_types (confirmed empirically against a standalone ajv compile). The real backstop is storage.ts's updateBucket, which independently throws ERRORS.NoContentProvided() (400) when public/fileSizeLimit/allowedMimeTypes are all undefined regardless of what the request body contained - so dropping the schema-level anyOf doesn't weaken validation, it just moves the "at least one field" guarantee to where it was already independently enforced.
setErrorHandler already sends the same {statusCode, error, message}
shape for any uncaught error on any route, in both the main and admin
apps - but that default was only actually documented where a route
happened to go through createDefaultSchema (main app) or the jwt/
apikey onRoute hooks. 21 main-app operations (raw file/image streaming
routes with no response schema at all) and all 40 admin operations
(errorSchema was never even registered there) had no 4xx documented.
Add the default directly to the one place that already defines this
error shape for the whole server: setErrorHandler's onRoute hook now
fills in a generic 4xx -> errorSchema for every route in both apps,
without overriding a status code a route already documents more
specifically (jwt.ts's 403, apikey.ts's 401, or a route's own explicit
4xx/4XX). Also registers errorSchema as a component on the admin app,
which never called addSchema for it.
This comment has been minimized.
This comment has been minimized.
Three distinct bugs from the previous commit's setErrorHandler change,
found by actually running the suite instead of trusting the earlier
docs-only verification:
1. Unit tests building a bare Fastify() and calling setErrorHandler
directly (error-handler.test.ts, header-validator.test.ts) crashed
at boot with "Cannot resolve ref errorSchema#" - the onRoute hook
needs errorSchema registered on that instance, which only app.ts/
admin-app.ts did. setErrorHandler now registers it itself
(idempotently, since both apps also register it directly for
their own schemas).
2. errorSchema was missing `code`, which every real error response
actually sends (confirmed in error-handler.ts and storage-error.ts's
render()). Forcing a response schema onto routes that previously had
none made fast-json-stringify start enforcing it, silently dropping
`code` from real payloads - caught by error-handler.test.ts's
existing assertions on the response body.
3. The big one: iceberg/index.ts's setErrorHandler call passes a custom
`formatter` that reshapes every error into the REST catalog spec's
nested {error: {message, type, code}}, completely different from
errorSchema's flat shape. The global default (and registerJwtAuth's
403, added earlier in the same subtree) still forced the flat
schema onto iceberg routes, and fast-json-stringify serializing a
real object against a schema declaring that field as a string
doesn't just drop data, it throws - manifesting as a 500 on every
iceberg error response, including plain 404s (confirmed by
reproducing acceptance/specs/iceberg.test.ts's exact failure
locally: GET .../namespaces/missing returned 500 instead of 404).
setErrorHandler now takes an optional errorResponseSchema alongside
formatter, and when present, its onRoute hook strips any status-
specific entry an ancestor already added (all assuming the flat
shape) and documents errorResponseSchema instead - overriding
rather than only filling a gap, since this hook runs after the
ones it needs to correct. iceberg/index.ts now passes the schema
matching its actual formatter output.
Verified: full unit suite (110 files, 1130 tests) passes, and the
specific iceberg 500 reproduces on master's version of this commit
and is fixed after it (tested locally via inject with ICEBERG_ENABLED
=true, both the JWT-rejection 403 path and confirmed via a full docs
export that the documented response now matches the formatter's real
output).
Multitenant acceptance CI still failed after the last fix: GET
/migrations/failed?cursor=not-a-number returned 500 instead of 400.
Root cause is the same class of bug as the iceberg one, wider in
scope: this codebase has many handlers that reply with an ad-hoc,
partial error body directly (reply.status(400).send({message: '...'}),
bypassing setErrorHandler's formatter entirely - migrations.ts,
tenants.ts, queue.ts, jwks.ts, objects.ts all do this). Forcing
errorSchema's required fields onto those routes' response
*serialization* makes fast-json-stringify throw on the ones missing a
field ("statusCode" is required!), not silently drop it as assumed.
There's no way to safely enforce this at the real-route level without
auditing and fixing every ad-hoc reply site, which is out of scope and
risks more of the same whack-a-mole. Since the actual goal is
documentation, not enforcement, move the generic default into
openapi-transform.ts's doc-only transform instead - it edits a copy
used only for the generated spec and can never affect request
handling, so it can't cause this class of bug by construction.
registerJwtAuth's 403 and the iceberg-formatter override in
error-handler.ts stay as real onRoute hooks, since both are safe: every
route they cover either always goes through the central formatter
(iceberg) or always renders the full flat shape for a real 403
(AccessDenied is only ever thrown by the central auth code, never
replied ad-hoc).
Test/Postgres, OrioleDB, and Multigres all failed the same way: dozens
of integration tests (app.test.ts, bucket.test.ts, object.test.ts)
asserting exact error response bodies started receiving an unexpected
`code` field they don't expect, e.g. object.test.ts:446 expecting
{statusCode, error, message} and receiving {..., code: "AccessDenied"}
too.
routes-helper.ts's createDefaultSchema has forced every route's generic
4xx response through `$ref: errorSchema#` since long before this PR -
completely unrelated to anything added here. errorSchema never declared
`code`, so fast-json-stringify has been silently stripping it from
every one of those routes' real 4xx/413/403/etc responses for as long
as that's existed, even though error-handler.ts's actual formatter
always includes it. That stripping is the actual, long-shipped,
test-covered contract.
Adding `code` to errorSchema (to fix the unit-test crash two commits
ago) incidentally un-stripped it everywhere createDefaultSchema's
generic 4xx applies - nothing to do with the crash itself, which is
already fixed by the previous commit removing the generic onRoute
hook entirely. Revert just the schema change; the doc now goes back to
accurately describing what actually gets serialized.
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
2 participants
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.
Summary
Evaluated the OpenAPI spec generated from this service's route schemas (
npm run docs:export) against what SDK generators actually need, and fixed the issues found. Each fix is its own commit so the change and its rationale are easy to review independently.operationIdon every operation (0ebdf6e) - was missing on all 80 operations, which is what generators use to name SDK methods. Derived from the existingROUTE_OPERATIONSconstants via a shared@fastify/swaggertransform instead of touching every route file.*towildcard(0832ef9) - Fastify's catch-all route segment leaked straight into the spec as a parameter literally named*, which isn't a legal identifier in any target language. Doc-only rename; the live Fastify route/validation schema is untouched.bucketSchema/objectSchemaas named components (0070095) - these were already shared TS constants but got inlined by value into every route's response, socomponents.schemasonly ever had the two globallyaddSchema'd entries (def-0/def-1). Switched the 5 call sites to$refand named components after their$idinstead of the default opaquedef-N.bc35496) - every operation only ever documented a generic4XX(or nothing, on the admin API). The JWT and admin API-key plugins already apply their security scheme via a single sharedonRoutehook, so extended that same hook to also document the specific status each one throws.49c2746) -/bucket,/health, and/s3/{Bucket}were each documented twice because they genuinely respond on both forms (Fastify'sexposeHeadRoutesand some explicit dual registrations). Folded into one path per real endpoint viatransformObject.8464e06) - the S3-compatible surface dispatches ~18 real commands via query-string/header matching inside an internal router, which OpenAPI can't express as distinct operations. The generic catch-all Fastify saw had no request/response schema at all - worse for SDK generation than not documenting it. Hidden, with thes3tag description pointing at the real per-command schemas ins3/commands/*.e9606c3) - 50 of 62 operations had a summary but no description. Added one-liners focused on what wasn't already obvious from the summary (auth flow variant, pagination style, side effects, TUS protocol semantics).Skipped: a
serversblock. This service doesn't know its own deployment prefix (Kong adds/storage/v1in front in production; self-hosted setups vary), so a guessed base URL would be more misleading than useful.Impact (main
api.json)operationIddescriptiondef-0,def-1authSchema,errorSchema,bucketSchema,objectSchema*/s3/**paths403Admin
api-admin.json: paths 19→18,401responses 0→40.Follow-up: which fixes are docs-only vs. real route changes
Two different mechanisms are used above, worth flagging for review:
bc35496) are added via anonRoutehook that mutates the actualrouteOptions.schemabefore Fastify compiles the route - confirmed infastify/lib/route.js,onRoutefires on the live options object pre-registration. ThebucketSchema/objectSchemarefs and the added descriptions are plain edits to each route file.@fastify/swaggertransform/transformObject, never touch the live server):operationIdderivation, the wildcard*→wildcardrename,def-N→$idref naming, and the trailing-slash dedupe.Looked into whether any of the docs-only ones should move to being direct route changes instead:
operationIdROUTE_OPERATIONS) and can't help Fastify's auto-synthesized HEAD routes anyway - there's no route file for those to edit; the dedup logic has to run after Fastify creates them. Not worth it.*→wildcard:wildcard(.*)) as an alternative to*. A real fix would mean changing the actual route pattern and every handler that readsrequest.params['*']. Bigger diff, touches routing semantics, needs real regression testing - not a docs change anymore.def-N→$idref namingfastify({ignoreTrailingSlash: true})would collapse them for real, but S3's dual registrations are explicit and would likely collide with that flag. Real behavior change, needs testing.s3/index.tsalready buildsschema: { tags: ['s3'] }inline at the one place these routes are registered. Addinghide: truethere directly would be simpler than the tag-sniffing check currently in the shared transform (isS3ProtocolCatchAll), and would let that check be deleted entirely.Not applied yet - flagging for a follow-up commit if wanted.
Follow-up: 3 more bugs found downstream by swift-openapi-generator
A consumer repo adopting
swift-openapi-generatoragainst this spec hit three more real bugs,independent of the fixes above. Fixed here too, each as its own commit:
"type": ["integer", "null"](valid only under OpenAPI 3.1/JSON-Schema-2020-12), whichswift-openapi-generator hard-fails parsing on - OpenAPI 3.0.x requires a plain type plus
nullable: true, which this codebase already uses elsewhere (e.g.createBucket'sfile_size_limit). FixedbucketSchema.file_size_limit/allowed_mime_typesand theGET /object/sign/{bucketName}response'serror/signedURLfields.anyOf-with-null spelling.anyOf: [<schema>, {type: 'null'}]is the other3.1-only way of expressing nullable, equally invalid under 3.0.3. swift-openapi-generator failed
with
Cannot initialize JSONType from invalid String value nullonobjectSchema.id. FixedobjectSchema'sid/updated_at/created_at/last_accessed_at/metadata/user_metadata-copyObject.tsspreadsobjectSchema.propertiesby reference into its own response, so thissingle fix also resolved the identical occurrences at the
/object/copyresponse. A fullstructural scan of the generated document confirmed zero remaining type-array or
anyOf-with-null shapes anywhere. (uploadSchema/multipartUploadSchema/uploadPartSchemahave the same pattern but aren't referenced by any HTTP route, so they never reach the generated
spec - left alone, flagged separately as dead code.)
anyOfonbucketUpdate's request body dropped itspropertiesentirely in swift-openapi-generator. The body declared
minProperties: 1and normalproperties, but also a top-levelanyOfrequiring at least one ofpublic/file_size_limit/allowed_mime_types- which swift-openapi-generator treats asanyOf-driven, generating a body type with every field permanently
nil. Before removing it,verified
minProperties: 1is not actually equivalent on its own: sinceadditionalPropertiesisn't restricted, a body containing only unrecognized fields wouldsatisfy
minPropertieswithout providing any of the three real fields (confirmed against astandalone
ajvcompile). The real backstop turned out to be one layer down:storage.ts'supdateBucketalready independently throwsERRORS.NoContentProvided()(400) whenpublic/fileSizeLimit/allowedMimeTypesare allundefined, regardless of what the requestbody contained - so dropping the schema-level
anyOfdoesn't weaken validation, it just movesthe guarantee to where it was already enforced.
Follow-up: errorSchema as the default for any 4xx
setErrorHandleralready sends the same{statusCode, error, message}shape for any uncaughterror, on any route, in both apps - but that was only actually documented where a route happened
to go through
createDefaultSchema(main app) or the jwt/apikeyonRoutehooks added earlier inthis PR. 21 main-app operations (raw file/image streaming routes with no response schema at all)
and all 40 admin operations (
errorSchemawas never even registered there) had no4xxdocumented at all.
Added the default directly where the error shape is actually defined:
setErrorHandler'sonRoutehook now fills in a generic4xx->errorSchemafor every route in both apps, withoutoverriding a status code a route already documents more specifically (403, 401, or an explicit
4xx/4XX a route sets itself). Also registered
errorSchemaas a component on the admin app, whichnever called
addSchemafor it. Main API: 62/62 operations now have4xxdocumented (was 41/62).Admin API: 40/40 (was 0/40).
Test plan
npx tsc -noEmit -p .clean after every commitnpm run docs:exportregenerated both specs successfully after every commit (this is also how theFST_ERR_SCH_SERIALIZATION_BUILDcrash from the schema-registration commit and the dropped-default-200 regression from the auth-response commit were caught before landing)biome check/prettier --checkclean on all touched filesnpm run test:integration) not run in this session - needs Docker infra not available in the sandbox