Fix null vs nullable, and add a OpenAPI version switch, starting with v3.1#343
Fix null vs nullable, and add a OpenAPI version switch, starting with v3.1#343philsturgeon wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds configurable OpenAPI document version support (3.0.x and 3.1.x): types/config option, version normalization and injection into generated docs, OpenAPI-version–aware schema normalization (including nullable handling), and expanded tests and README docs. ChangesConfigurable OpenAPI Version Support
Sequence DiagramsequenceDiagram
participant Types as src/types.ts
participant Index as src/index.ts
participant SchemaGen as src/openapi.ts
participant Tests as test/*
Types->>Index: export OpenAPIVersion and updated config types
Index->>SchemaGen: call toOpenAPISchema(..., openapiVersion)
SchemaGen->>SchemaGen: unwrapSchema(..., openapiVersion) selects draft & normalizes nullable/null
SchemaGen->>Index: return OpenAPIDocument with top-level openapi set
Index->>Tests: tests validate emitted OpenAPI versions and nullable handling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/openapi.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. test/openapi/null-to-openapi.test.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/index.ts (1)
102-102: 💤 Low valuePfft, casting something that's already that type? Silly~ ♡(╹◡╹)
effectiveOpenAPIVersionis already typedOpenAPIVersionat Line 90, soas OpenAPIVersionhere is dead weight. Drop it.♻️ tiny tidy-up
- openapi: effectiveOpenAPIVersion as OpenAPIVersion, + openapi: effectiveOpenAPIVersion,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/index.ts` at line 102, Remove the redundant type cast in the object passed from the main logic in src/index.ts: `effectiveOpenAPIVersion` is already an `OpenAPIVersion`, so drop the unnecessary `as OpenAPIVersion` in the `openapi` assignment and leave the existing typed value as-is.test/index.test.ts (1)
30-42: ⚡ Quick winTesting 3.0 with the easiest possible route? How conveniently cowardly of you~ ♡(˘ω˘)
This 3.0.x test only validates default routes, so it never exercises a nullable/
t.Null()schema. That's exactly the case that would expose the version mismatch flagged insrc/openapi.ts(Lines 540-545): a3.0.1doc carryingtype: 'null'should failSwaggerParser.validate. Adding a nullable-union case under3.0.xwould either confirm the gap or prove me wrong~🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/index.test.ts` around lines 30 - 42, The test for OpenAPI 3.0.x only covers default routes and never exercises a nullable or t.Null() schema, which may hide a version mismatch issue. Modify the test inside the 'it emits OpenAPI 3.0.x when configured' block to add a route that returns or includes a nullable union schema to ensure the emitted OpenAPI 3.0.1 document is validated for nullable support and that SwaggerParser.validate properly fails or passes as expected.src/types.ts (1)
61-65: ⚡ Quick winYeah yeah, lazy typing surelyyyy ♡ Just use the union the library gives you, dummy~
The intersection
Partial<OpenAPIV3.Document> & Partial<OpenAPIV3_1.Document>technically works here, but that's just dumb whenopenapi-typesalready exports a union type for multi-version support:OpenAPI.Documentis literallyOpenAPIV2.Document | OpenAPIV3.Document | OpenAPIV3_1.Document[1]. Intersecting version-specific types is risky (properties with incompatible types can collapse tonever), and the library docs recommend unions, not intersections, for handling multiple OpenAPI versions [2].Use
Omit<OpenAPI.Document, ...>instead and stop reinventing the wheel~ (´∇｀)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/types.ts` around lines 61 - 65, Replace the intersection-based type used for the documentation property with the library-provided union: change the type on documentation (currently Omit<Partial<OpenAPIV3.Document> & Partial<OpenAPIV3_1.Document>, 'x-express-openapi-additional-middleware' | 'x-express-openapi-validation-strict'>) to use Omit<OpenAPI.Document, 'x-express-openapi-additional-middleware' | 'x-express-openapi-validation-strict'> so you rely on the union exported by openapi-types (OpenAPI.Document) instead of intersecting version-specific types.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/openapi.ts`:
- Around line 540-545: The current code unconditionally emits JSON Schema using
schema['~standard'].jsonSchema(... { target: 'draft-2020-12' }) and then runs
enumToOpenApi, which produces 3.1-style null/type shapes that break when
openapiVersion is 3.0.x; update the logic where enumToOpenApi is invoked (and
any response-building code that special-cases type === 'null') to branch on the
effective openapiVersion value (threaded from src/index.ts), calling the schema
generator with a 3.0-compatible dialect/target (or draft-07) when openapiVersion
startsWith '3.0' and transforming emitted schemas so nullable types become
nullable: true (remove type: 'null' entries or anyOf containing null and set
nullable: true on the non-null schema) before passing to enumToOpenApi; ensure
the response/content creation code also treats nullable via nullable:true for
3.0.x.
---
Nitpick comments:
In `@src/index.ts`:
- Line 102: Remove the redundant type cast in the object passed from the main
logic in src/index.ts: `effectiveOpenAPIVersion` is already an `OpenAPIVersion`,
so drop the unnecessary `as OpenAPIVersion` in the `openapi` assignment and
leave the existing typed value as-is.
In `@src/types.ts`:
- Around line 61-65: Replace the intersection-based type used for the
documentation property with the library-provided union: change the type on
documentation (currently Omit<Partial<OpenAPIV3.Document> &
Partial<OpenAPIV3_1.Document>, 'x-express-openapi-additional-middleware' |
'x-express-openapi-validation-strict'>) to use Omit<OpenAPI.Document,
'x-express-openapi-additional-middleware' |
'x-express-openapi-validation-strict'> so you rely on the union exported by
openapi-types (OpenAPI.Document) instead of intersecting version-specific types.
In `@test/index.test.ts`:
- Around line 30-42: The test for OpenAPI 3.0.x only covers default routes and
never exercises a nullable or t.Null() schema, which may hide a version mismatch
issue. Modify the test inside the 'it emits OpenAPI 3.0.x when configured' block
to add a route that returns or includes a nullable union schema to ensure the
emitted OpenAPI 3.0.1 document is validated for nullable support and that
SwaggerParser.validate properly fails or passes as expected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2f6f3fea-95e6-4763-9054-e3c7a7cc320e
⛔ Files ignored due to path filters (1)
src/gen/index.tsis excluded by!**/gen/**
📒 Files selected for processing (5)
README.mdsrc/index.tssrc/openapi.tssrc/types.tstest/index.test.ts
philsturgeon
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/openapi.ts (1)
262-266: 💤 Low valueForgetting to pass
openapiVersionhere, sloppy~ ♡ ʕ •ᴥ•ʔ
mergeSchemaPropertyandunwrapResponseSchemacallunwrapSchema(...)without forwardingopenapiVersion, so the merge of standalone-validator schemas is normalized with the hardcoded default3.1.2. It mostly recovers becausetoOpenAPISchemare-unwraps the merged result later (andnormalizeNullableSchemaForOAS30can re-collapse atype: ['x','null']produced by the first pass), but relying on a double round-trip to undo the wrong version is fragile. ThreadopenapiVersionthrough these helpers for a single, consistent normalization.Also applies to: 294-320
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openapi.ts` around lines 262 - 266, mergeSchemaProperty and unwrapResponseSchema call unwrapSchema(...) without forwarding openapiVersion, causing standalone-validator schemas to be normalized using the default 3.1.2; update both mergeSchemaProperty and unwrapResponseSchema to accept and pass the openapiVersion parameter through to unwrapSchema (and any intermediate calls) so unwrapSchema(incomingSchema, vendors, openapiVersion) is used everywhere, and adjust any call sites and function signatures accordingly (also ensure to propagate openapiVersion into any code paths mentioned like toOpenAPISchema and normalizeNullableSchemaForOAS30 where a consistent version is required).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/openapi.ts`:
- Around line 640-714: normalizeNullableSchemaForOAS30 (and similarly
normalizeNullableSchemaForOAS31) currently recurses into every object key via
Object.entries and treats arbitrary keyword values (e.g., default) as schemas,
causing corruption like stripping type:'null'; change the recursion to only
descend into known schema-bearing keys (e.g., properties, patternProperties,
items, additionalProperties, unevaluatedProperties, allOf, anyOf, oneOf, not,
prefixItems, items, dependencies/schema-valued keywords, etc.) and into array
elements of those specific keys, leaving other keyword objects (like default,
examples, metadata) untouched; update both normalizeNullableSchemaForOAS30 and
normalizeNullableSchemaForOAS31 to check key names before calling
normalizeNullableSchemaForOAS30/31 recursively and only transform { type: 'null'
} when it is actually a schema node.
---
Nitpick comments:
In `@src/openapi.ts`:
- Around line 262-266: mergeSchemaProperty and unwrapResponseSchema call
unwrapSchema(...) without forwarding openapiVersion, causing
standalone-validator schemas to be normalized using the default 3.1.2; update
both mergeSchemaProperty and unwrapResponseSchema to accept and pass the
openapiVersion parameter through to unwrapSchema (and any intermediate calls) so
unwrapSchema(incomingSchema, vendors, openapiVersion) is used everywhere, and
adjust any call sites and function signatures accordingly (also ensure to
propagate openapiVersion into any code paths mentioned like toOpenAPISchema and
normalizeNullableSchemaForOAS30 where a consistent version is required).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e35e0dd6-de29-4812-b718-0663c9217769
📒 Files selected for processing (4)
src/index.tssrc/openapi.tstest/index.test.tstest/openapi/null-to-openapi.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/index.test.ts
1 participant
Hello friends. I've been tasked with updated Speakeasy's Elysia integration guide, and noticed you've got a new OpenAPI plugin to replace the Swagger plugin. Thank you!
You've made the first step, but there's a few issues in this otherwise fantastic approach you're taking to making OpenAPI a first-class part of the web framework.
type: nullinto the YAML and that's one of the major differences between OpenAPI Schema and JSON Schema.Now that OpenAPI v3.1 is fully JSON Schema compatible, I've been helping tools migrate from OpenAPI v3.0 to v3.1 for years. I wrote this guide to help see what other differences: https://www.openapis.org/blog/2021/02/16/migrating-from-openapi-3-0-to-3-1-0
I don't think you do anything much with exclusiveMinimum and example vs examples is fine as its just deprecated not removed. That means you could honestly claim OpenAPI v3.1 support with this switch, and adding
nullable: trueto OAS 3.0 is fixing a bug.This is a double win I recon.
Let me know as soon as OpenAPI v3.1 is supported so I can pop it up on OpenAPI.Tools. We don't add new "legacy" tools (those that dont support at least 3.1), but the Elysia OpenAPI support is so good I'm eager to raise awareness of your framework to the OpenAPI community.
Relates to #343, #299
Summary by CodeRabbit
New Features
Documentation
openapiVersionconfiguration option and supported version ranges