fix(docs): normalize nullable JSON Schema idioms for OpenAPI 3.0 docs - #1276
fix(docs): normalize nullable JSON Schema idioms for OpenAPI 3.0 docs#1276grdsdev wants to merge 1 commit into
Conversation
Route schemas use `type: [X, 'null']` and `anyOf: [X, { type: 'null' }]`,
valid JSON Schema that AJV validates natively, but not representable in
OpenAPI 3.0's Schema Object (which only allows a single `type` plus a
separate `nullable: true`). The generated docs declare `openapi: 3.0.3`
while emitting these 3.1-only idioms verbatim, breaking strict 3.0
consumers (e.g. OpenAPIKit-based codegen tools).
Add a docs-only swagger transform, composed with the existing
finite-keyword stripping, that rewrites both idioms into their OpenAPI
3.0 equivalent. Runtime request/response validation schemas are
untouched.
Coverage Report for CI Build 30390837140Coverage decreased (-0.08%) to 80.327%Details
Uncovered Changes
Coverage Regressions20 previously-covered lines in 3 files lost coverage.
Coverage Stats
💛 - Coveralls |
|
Closing in favor of a different approach — see #1276-replacement (bumping declared openapi version to 3.1.0 instead of transforming schemas at the docs layer). Will link the new PR here. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Looks good — approving with a minor nit noted inline (the $ref+anyOf-null branch should wrap in allOf for strict OpenAPI 3.0 compliance, but no current schema exercises that path).
Extended reasoning...
Overview
This PR adds src/http/openapi-nullable.ts, a docs-only swagger transform composed with the existing finiteSwaggerTransform, that rewrites two JSON Schema nullable idioms (type: [X, 'null'] and anyOf: [<schema>, { type: 'null' }]) into OpenAPI 3.0's type + nullable: true shape before the doc is emitted. It's wired into both src/app.ts and src/admin-app.ts in place of the previous bare finiteSwaggerTransform, and ships a thorough unit test file covering the main cases (type arrays, anyOf with null first/second, multi-branch anyOf left untouched, nested recursion, non-mutation of the source).
Security risks
None. This only affects the generated OpenAPI documentation (/documentation); runtime request/response validation schemas (AJV) are completely untouched, and the transform is a pure clone-and-rewrite function with no I/O, no user input, and no auth/permission logic involved.
Level of scrutiny
Low-to-moderate is appropriate: it's a self-contained, additive, docs-only change with clear intent and no impact on request handling or data validation. The bug hunting pass found one nit (the $ref-sibling case should use allOf wrapping per strict OpenAPI 3.0 semantics), but grep confirms no current route schema produces anyOf: [{ $ref }, { type: 'null' }], so it has zero effect on any currently generated doc — it only pre-emptively locks in wrong behavior for a case that doesn't exist yet.
Other factors
Test coverage is solid for a first pass and includes a non-mutation check, which matches the PR's stated 'same clone-before-mutate approach as stripFiniteKeyword' design. The wiring changes in app.ts/admin-app.ts are mechanical import swaps. Given the nit is explicitly scoped as low-severity and inert today, I don't think it should block this PR.
|
|
||
| const object = value as Record<string, unknown> | ||
|
|
||
| const anyOf = object.anyOf | ||
| if (Array.isArray(anyOf) && anyOf.length === 2) { | ||
| const nullIndex = anyOf.findIndex(isNullOnlySchema) | ||
| const other = nullIndex === -1 ? undefined : anyOf[1 - nullIndex] | ||
| if (other && typeof other === 'object') { | ||
| const merged: Record<string, unknown> = { | ||
| ...(normalizeNullableSchema(other) as Record<string, unknown>), | ||
| nullable: true, | ||
| } | ||
| for (const [key, nestedValue] of Object.entries(object)) { | ||
| if (key !== 'anyOf' && !(key in merged)) { | ||
| merged[key] = normalizeNullableSchema(nestedValue) | ||
| } | ||
| } | ||
| return merged as T | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🟡 In OpenAPI 3.0, sibling keywords next to $ref (including nullable) are ignored by compliant consumers — the valid idiom is { allOf: [{ $ref }], nullable: true }. The anyOf branch of normalizeNullableSchema instead emits { $ref, nullable: true }, so strict OpenAPI 3.0 tooling (e.g. Swift's OpenAPIKit30, the PR's stated target) will drop the nullable sibling and treat the property as non-nullable — defeating the fix for exactly the $ref-based nullable union case this PR is meant to solve. No current route schema hits this path (confirmed via grep), but the openapi-nullable.test.ts 'metadata' case explicitly asserts this incorrect output, locking it in for any future $ref+null property.
Extended reasoning...
The bug: In normalizeNullableSchema's anyOf branch (src/http/openapi-nullable.ts lines 33-53), when a property is anyOf: [{ $ref: 'someSchema#' }, { type: 'null' }], the code spreads the non-null branch and adds nullable: true as a sibling, producing { $ref: 'someSchema#', nullable: true }. That is exactly what the code does for the inline-schema case too (e.g. anyOf: [{ type: 'string' }, { type: 'null' }] → { type: 'string', nullable: true }), and that idiom is correct for inline schemas. But it is not correct when the merged-in branch is a $ref.
Why it's wrong: OpenAPI 3.0's Schema Object follows JSON Reference semantics for $ref. Per the OpenAPI 3.0.x spec, any sibling keywords next to $ref (including nullable) are ignored by compliant consumers — OpenAPI 3.0 (unlike 3.1, which adopted JSON Schema 2020-12's semantics) does not support Schema Object siblings alongside $ref. The documented, correct way to express "nullable reference" in OpenAPI 3.0 is to wrap the reference: { allOf: [{ $ref: 'someSchema#' }], nullable: true }. Emitting { $ref, nullable: true } as a flat sibling is invalid for the target spec version and will be silently ignored (or in some strict parsers, rejected) precisely by the kind of tooling this PR calls out as its motivation — Swift's OpenAPIKit30-based codegen used by supabase-swift.
Step-by-step proof:
- Suppose a route schema property is defined as
{ anyOf: [{ $ref: '#/components/schemas/Metadata' }, { type: 'null' }] }(a realistic pattern for an optional/nullable referenced object). normalizeNullableSchemafinds the null-only branch at index 1, takesother = anyOf[0] = { $ref: '#/components/schemas/Metadata' }, and merges it withnullable: true, producing{ $ref: '#/components/schemas/Metadata', nullable: true }.- This is exactly what
openapi-nullable.test.ts's 'metadata' case asserts as the expected output today. - When this document is consumed by an OpenAPI 3.0-Schema-Object-compliant parser (e.g. OpenAPIKit30), the parser sees
$refand, per JSON Reference semantics, ignores all sibling keywords —nullable: trueis discarded. - The generated Swift/codegen type for that property ends up non-optional/non-nullable, which is the exact bug this PR set out to fix (strict 3.0 consumers failing to represent nullable values correctly).
Why existing code doesn't prevent it: The function only special-cases the "other branch is a $ref" scenario by merging $ref in as one more key alongside nullable, without ever wrapping it in allOf. There's no logic distinguishing "other is an inline schema" (safe to merge nullable as a sibling) from "other is a $ref" (must be wrapped).
Impact today: Confirmed via grep that no current route schema in src/storage/schemas or src/http/routes actually produces anyOf: [{ $ref }, { type: 'null' }] — all current inline anyOf-with-null usages resolve to plain inline schemas, and the one existing $ref-in-anyOf case (vector/query-vectors.ts) is a two-$ref union with no null branch, so it's untouched by this code path. So no currently emitted doc loses nullability because of this. However, it is an explicitly tested and asserted capability of the new transform (the 'metadata' test case), so the incorrect behavior is locked in for the first future schema that legitimately needs a nullable $ref.
Fix: When the non-null branch contains a $ref, wrap it in allOf instead of merging nullable as a direct sibling: { allOf: [normalizeNullableSchema(other)], nullable: true }. This preserves correctness for both the inline-schema case (unaffected) and the $ref case (spec-compliant).
2 participants
Summary
"openapi": "3.0.3", but route schemas are authored as plain JSON Schema (type: [X, 'null'], or{ anyOf: [<schema>, { type: 'null' }] }for$ref-based unions). Both idioms are valid JSON Schema — AJV validates them natively — but OpenAPI 3.0's Schema Object can't represent either: it only allows a singletypestring plus a separatenullable: trueboolean.OpenAPIKit30-based codegen used by supabase-swift), which fail to decode the document entirely.src/http/openapi-nullable.ts, a docs-only swaggertransform(composed with the existingfiniteSwaggerTransform) that rewrites both idioms into their OpenAPI 3.0 equivalent before the doc is emitted. Runtime request/response validation schemas are completely untouched — same clone-before-mutate approach as the existingstripFiniteKeyword.nullable: true, butnullableisn't a real JSON Schema keyword — AJV would ignore it and reject legitimatenullvalues at runtime. The transform-only approach has no runtime risk and touches no schema files.Test plan
npx vitest run src/http/openapi-nullable.test.ts src/http/finite.test.ts— new + existing tests passnpx tsc -noEmit— cleannpx @biomejs/biome checkon changed files — clean