feat(openapi): resolve registered models as $ref pointers#338
Open
marioparaschiv wants to merge 2 commits into
Open
feat(openapi): resolve registered models as $ref pointers#338marioparaschiv wants to merge 2 commits into
marioparaschiv wants to merge 2 commits into
Conversation
Schemas registered via .model() are now matched against inline schemas in route definitions and replaced with $ref pointers to components/schemas. This reduces spec size and improves readability. Direct model usage (e.g. response: User) is matched by object identity on the raw schema before JSON Schema conversion. Nested usage (e.g. response: z.array(User)) is matched by deep equality on the converted JSON Schema output. Works with all Standard Schema validators (Zod, TypeBox, Valibot, etc).
Contributor
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (24)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughYouuuu thought you could just casually submit an OpenAPI schema generation improvement~? How adorable. ᐕ)ノ This PR replaces duplicate inline schemas with component ChangesModel Reference Resolution Feature
Sequence DiagramsequenceDiagram
participant Gen as OpenAPI Generator
participant Map as buildModelRefMap
participant Match as schemaMatches
participant Emit as Response Emitter
participant Resolve as resolveNestedModelRefs
Gen->>Map: Create schema→name map from definitions
Map-->>Gen: modelRefMap ready
Gen->>Gen: Generate paths & response schemas
Gen->>Match: Check if response matches registered model
Match-->>Gen: match found or not
alt Match Found
Gen->>Emit: Emit $ref response
Emit-->>Gen: application/json with $ref
else No Match
Gen->>Emit: Emit inline schema response
Emit-->>Gen: response with unwrapped content
end
Gen->>Resolve: Convert remaining inline schemas
Resolve-->>Gen: specification with nested $refs resolved
Gen-->>Gen: Return final OpenAPI document
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 1089-1100: The fast-path that checks modelRefMap.get(schema) and
unconditionally sets operation.responses[status].content['application/json'] is
wrong; instead, when a modelName is found reuse the existing media-type
selection logic (the same media-type keys and structure produced by your
unwrapSchema/type switch) but replace only the schema node with { $ref:
`#/components/schemas/${modelName}` }. Update the branch in the handler that
populates operation.responses (the block referencing modelRefMap,
operation.responses and status) so it copies or delegates to the media-type
selection used by unwrapSchema/type and swaps in the $ref rather than forcing
'application/json'; apply the same change to the similar block around lines
1136-1146.
- Around line 687-704: The deepEqual function allows mismatches like
deepEqual({}, []) because it only checks Array.isArray on the left side; update
deepEqual (referenced by walk() and tryReplace(), which may pass arrays like
tags or required) to explicitly handle arrays on both sides: if either value is
an array, immediately return true only when both are arrays and then compare
lengths and elements (i.e., use if (Array.isArray(a) || Array.isArray(b)) {
return Array.isArray(a) && Array.isArray(b) && a.length === b.length &&
a.every((v,i) => deepEqual(v, (b as unknown[])[i])); }), keeping the existing
null/type/object guards otherwise so objects and arrays cannot be confused.
🪄 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: 1cccd0d6-2558-4cea-a636-bb3777694206
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
package.jsonsrc/openapi.tstest/model-refs.test.ts
Comment on lines
+687
to
+704
| const deepEqual = (a: unknown, b: unknown): boolean => { | ||
| if (a === b) return true | ||
| if (a === null || b === null) return false | ||
| if (typeof a !== typeof b || typeof a !== 'object') return false | ||
|
|
||
| if (Array.isArray(a)) | ||
| return Array.isArray(b) | ||
| && a.length === b.length | ||
| && a.every((v, i) => deepEqual(v, b[i])) | ||
|
|
||
| const aObj = a as Record<string, unknown> | ||
| const bObj = b as Record<string, unknown> | ||
| const aKeys = Object.keys(aObj) | ||
| const bKeys = Object.keys(bObj) | ||
|
|
||
| return aKeys.length === bKeys.length | ||
| && aKeys.every((k) => deepEqual(aObj[k], bObj[k])) | ||
| } |
Contributor
There was a problem hiding this comment.
Keep arrays out of the matcher, baka~
walk() can hand tryReplace() array-valued nodes like tags or required, and deepEqual only checks Array.isArray on the left-hand side. That makes cases like deepEqual({}, []) slip through, so a permissive model that normalizes to {} can rewrite unrelated arrays into $refs and break the spec. Tiny guard, big save~ (￣︶￣*)
♻️ Quick fix
const deepEqual = (a: unknown, b: unknown): boolean => {
if (a === b) return true
if (a === null || b === null) return false
+ if (Array.isArray(a) !== Array.isArray(b)) return false
if (typeof a !== typeof b || typeof a !== 'object') return false
if (Array.isArray(a))
return Array.isArray(b)
&& a.length === b.length
&& a.every((v, i) => deepEqual(v, b[i]))
@@
const tryReplace = (
node: unknown,
parent: Record<string, unknown> | unknown[],
key: string | number
): boolean => {
- if (!node || typeof node !== 'object') return false
+ if (!node || typeof node !== 'object' || Array.isArray(node))
+ return falseAlso applies to: 728-744
🤖 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 687 - 704, The deepEqual function allows
mismatches like deepEqual({}, []) because it only checks Array.isArray on the
left side; update deepEqual (referenced by walk() and tryReplace(), which may
pass arrays like tags or required) to explicitly handle arrays on both sides: if
either value is an array, immediately return true only when both are arrays and
then compare lengths and elements (i.e., use if (Array.isArray(a) ||
Array.isArray(b)) { return Array.isArray(a) && Array.isArray(b) && a.length ===
b.length && a.every((v,i) => deepEqual(v, (b as unknown[])[i])); }), keeping the
existing null/type/object guards otherwise so objects and arrays cannot be
confused.
Comment on lines
+1089
to
+1100
| const modelName = modelRefMap.get(schema) | ||
|
|
||
| if (modelName) { | ||
| operation.responses[status] = { | ||
| description: `Response for status ${status}`, | ||
| content: { | ||
| 'application/json': { | ||
| schema: { $ref: `#/components/schemas/${modelName}` } | ||
| } | ||
| } | ||
| } | ||
| continue |
Contributor
There was a problem hiding this comment.
Don't force every direct model ref through application/json, silly~
These fast paths bypass the existing unwrapSchema/type switch and always emit JSON. If a registered model is t.String(), z.string(), t.Null(), etc., using it directly as response now changes the contract from text/plain or no-content into application/json just because the identity match hit first. Reuse the same media-type selection and only swap the schema node to a $ref, got it? ♡
🩹 Minimal direction
- if (modelName) {
- operation.responses[status] = {
- description: `Response for status ${status}`,
- content: {
- 'application/json': {
- schema: { $ref: `#/components/schemas/${modelName}` }
- }
- }
- }
+ if (modelName) {
+ const response = unwrapSchema(schema as any, vendors, 'output')
+ const { type, description } = unwrapReference(response, definitions)
+ const ref = { $ref: `#/components/schemas/${modelName}` }
+
+ operation.responses[status] = {
+ description: description ?? `Response for status ${status}`,
+ content:
+ type === 'void' ||
+ type === 'null' ||
+ type === 'undefined'
+ ? ({ type, description } as any)
+ : type === 'string' ||
+ type === 'number' ||
+ type === 'integer' ||
+ type === 'boolean'
+ ? { 'text/plain': { schema: ref } }
+ : { 'application/json': { schema: ref } }
+ }
continue
}Apply the same shape in the single-response branch too.
Also applies to: 1136-1146
🤖 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 1089 - 1100, The fast-path that checks
modelRefMap.get(schema) and unconditionally sets
operation.responses[status].content['application/json'] is wrong; instead, when
a modelName is found reuse the existing media-type selection logic (the same
media-type keys and structure produced by your unwrapSchema/type switch) but
replace only the schema node with { $ref: `#/components/schemas/${modelName}` }.
Update the branch in the handler that populates operation.responses (the block
referencing modelRefMap, operation.responses and status) so it copies or
delegates to the media-type selection used by unwrapSchema/type and swaps in the
$ref rather than forcing 'application/json'; apply the same change to the
similar block around lines 1136-1146.
marioparaschiv
force-pushed
the
fix/preserve-model-refs
branch
from
b53a8b9 to
f37137b
Compare
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.
Schemas registered via .model() are now matched against inline schemas
in route definitions and replaced with $ref pointers to
components/schemas. This reduces spec size and improves readability.
Direct model usage (e.g. response: User) is matched by object identity
on the raw schema before JSON Schema conversion. Nested usage (e.g.
response: z.array(User)) is matched by deep equality on the converted
JSON Schema output.
Works with all Standard Schema validators (Zod, TypeBox, Valibot, etc).
Summary by CodeRabbit
New Features
Tests
Chores