feat(openapi): expand support for unions/intersection of objects when generating OpenAPI specs#1178
feat(openapi): expand support for unions/intersection of objects when generating OpenAPI specs#1178
Conversation
…arameters when generating OpenAPI specs Fixes #1162
dosubot
Bot
added
the
size:XL
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary of ChangesHello @unnoq, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the OpenAPI generator's ability to handle advanced schema compositions, specifically unions and intersections of object schemas. By introducing a new simplification utility, the generator can now produce more accurate and user-friendly OpenAPI specifications for complex input types, particularly for parameters in Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a new exported schema-simplification utility that flattens/merges composed object JSON Schemas (anyOf/oneOf/allOf, resolves $ref when doc provided) and integrates it into the OpenAPIGenerator; generator preprocessing now uses simplified schemas for params, queries, headers, bodies and responses. Tests added (one duplicate test present). Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Caller
participant Gen as OpenAPIGenerator
participant Simplify as Simplifier
participant Resolver as RefResolver
Client->>Gen: generate(contract)
Gen->>Gen: determine inputStructure & method
alt Preprocess needed (detailed OR compact+dynamic OR GET)
Gen->>Simplify: simplifyComposedObjectJsonSchemasAndRefs(schema, doc?)
alt doc provided
Simplify->>Resolver: resolve $ref(s)
Resolver-->>Simplify: resolved schema(s)
end
Simplify-->>Gen: simplified schema (flattened/merged)
end
Gen->>Gen: build path params, query params, headers, body, responses using simplified schemas
alt GET-specific nuance
Gen->>Gen: validate/build query params (may use original/unresolved schema in some checks)
end
Gen-->>Client: OpenAPI document
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Points needing attention:
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new function simplifyComposedObjectJsonSchemasAndRefs that flattens composed JSON schemas (using anyOf, oneOf, allOf) to make them compatible with OpenAPI parameters extraction in scenarios with path parameters or GET requests. The function merges object schemas from unions and intersections into a single flattened object schema.
Key changes:
- New
simplifyComposedObjectJsonSchemasAndRefsfunction that recursively flattens composed object schemas - Integration into the OpenAPI generator to simplify schemas before parameter extraction
- Removal of
minStructureDepthForRefconfiguration option - Comprehensive test coverage for the new simplification logic
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/openapi/src/schema-utils.test.ts | Reformatted imports for better readability |
| packages/openapi/src/openapi-utils.ts | Added new simplifyComposedObjectJsonSchemasAndRefs function and related imports |
| packages/openapi/src/openapi-utils.test.ts | Added comprehensive tests for the new schema simplification function |
| packages/openapi/src/openapi-generator.ts | Integrated schema simplification and removed minStructureDepthForRef option |
| packages/openapi/src/openapi-generator.test.ts | Added integration test for discriminated unions with path parameters |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
More templates
@orpc/ai-sdk
@orpc/arktype
@orpc/client
@orpc/contract
@orpc/experimental-durable-iterator
@orpc/hey-api
@orpc/interop
@orpc/json-schema
@orpc/nest
@orpc/openapi
@orpc/openapi-client
@orpc/otel
@orpc/experimental-publisher
@orpc/react
@orpc/react-query
@orpc/experimental-react-swr
@orpc/server
@orpc/shared
@orpc/solid-query
@orpc/standard-server
@orpc/standard-server-aws-lambda
@orpc/standard-server-fastify
@orpc/standard-server-fetch
@orpc/standard-server-node
@orpc/standard-server-peer
@orpc/svelte-query
@orpc/tanstack-query
@orpc/trpc
@orpc/valibot
@orpc/vue-colada
@orpc/vue-query
@orpc/zod
commit: |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature to expand support for union and intersection object types in parameters for OpenAPI spec generation. The implementation is mostly solid, introducing a new utility function simplifyComposedObjectJsonSchemasAndRefs to flatten complex schemas, along with comprehensive tests. I've identified a critical bug in how required properties are determined when combining allOf and anyOf schemas, and a potential performance improvement in the same function. My feedback includes a fix for the bug, a new test case to prevent regressions, and a suggestion to refactor for better performance.
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/openapi/src/openapi-generator.test.ts(1 hunks)packages/openapi/src/openapi-generator.ts(3 hunks)packages/openapi/src/openapi-utils.test.ts(2 hunks)packages/openapi/src/openapi-utils.ts(2 hunks)packages/openapi/src/schema-utils.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
packages/openapi/src/openapi-utils.test.ts (2)
packages/openapi/src/openapi-utils.ts (1)
simplifyComposedObjectJsonSchemasAndRefs(186-316)packages/openapi/src/openapi-generator.ts (3)
doc(190-276)doc(278-412)doc(414-539)
packages/openapi/src/openapi-utils.ts (3)
packages/shared/src/array.ts (1)
toArray(1-3)packages/openapi/src/schema-utils.ts (1)
isObjectSchema(15-17)packages/shared/src/json.ts (1)
stringifyJSON(9-12)
packages/openapi/src/openapi-generator.ts (2)
packages/openapi/src/openapi-utils.ts (2)
simplifyComposedObjectJsonSchemasAndRefs(186-316)toOpenAPIParameters(103-138)packages/openapi/src/schema-utils.ts (1)
isObjectSchema(15-17)
packages/openapi/src/openapi-generator.test.ts (2)
packages/openapi/src/openapi-generator.ts (1)
OpenAPIGenerator(91-602)packages/zod/src/zod4/converter.ts (3)
ZodToJsonSchemaConverter(88-631)schema(116-565)schema(567-589)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint
- GitHub Check: publish-commit
- GitHub Check: test
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/openapi/src/openapi-utils.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/openapi/src/openapi-utils.ts (4)
packages/openapi/src/schema.ts (2)
JSONSchema(5-5)ObjectSchema(10-10)packages/shared/src/array.ts (1)
toArray(1-3)packages/openapi/src/schema-utils.ts (1)
isObjectSchema(15-17)packages/shared/src/json.ts (1)
stringifyJSON(9-12)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/openapi/src/openapi-utils.ts (1)
238-255: Critical: Handle required keys that lack property definitions in allOf branches.When an
allOfbranch declares a key in itsrequiredarray but doesn't define it inproperties(the property schema may live in a different allOf branch), the current loop skips that key entirely. This causes the simplified schema to drop required flags that are still enforced by the intersection, breaking path parameter detection and other required field validations. JSON Schema explicitly allowsrequiredwithoutproperties.Based on learnings
Update this block to first process each branch's
requiredarray before iteratingproperties:const mergedInteractionPropertyMap: Map<string, { required: boolean, schemas: JSONSchema[] }> = new Map() for (const u of objectIntersectionSchemas) { + if (Array.isArray(u.required)) { + for (const key of u.required) { + let entry = mergedInteractionPropertyMap.get(key) + if (!entry) { + entry = { required: true, schemas: [] } + mergedInteractionPropertyMap.set(key, entry) + } else { + entry.required = true + } + } + } + if (u.properties) { for (const [key, value] of Object.entries(u.properties)) { let entry = mergedInteractionPropertyMap.get(key) if (!entry) { entry = { required: false, schemas: [] } mergedInteractionPropertyMap.set(key, entry) + } else { + entry.schemas.push(value) } - - entry.schemas.push(value) + + if (u.required?.includes(key)) { + entry.required = true + } } } } - for (const [key, entry] of mergedInteractionPropertyMap) { - if (objectIntersectionSchemas.some(s => s.required?.includes(key))) { - entry.required = true - } - }
🧹 Nitpick comments (2)
packages/openapi/src/openapi-utils.ts (2)
181-193: Verify the warning about looseness is accurate.The warning states the result is "looser than the original schema," but the actual behavior depends on the composition. For intersections (allOf), the simplified schema might be looser since it may not enforce all constraints from each branch. However, for unions (anyOf/oneOf), marking a property as required only when all union branches require it is actually stricter than the original (where the property could be required in just one branch and still satisfy the schema).
Consider clarifying the warning to explain that:
- Intersection simplification may lose constraint enforcement
- Union simplification may over-constrain by requiring properties that only need to be present in one branch
209-225: Consider refactoring to improve performance.The nested loop with
objectUnionSchemas.every(...)at line 222 results in O(schemas × properties × schemas) complexity. For schemas with many properties and union members, this could become inefficient.Refactor to two separate loops as suggested in the past review: first populate the map with all properties, then determine required status in a second pass. This reduces complexity to O(schemas × properties + unique_properties × schemas).
Apply this refactoring:
const mergedUnionPropertyMap: Map<string, { required: boolean, schemas: JSONSchema[] }> = new Map() for (const u of objectUnionSchemas) { if (u.properties) { for (const [key, value] of Object.entries(u.properties)) { let entry = mergedUnionPropertyMap.get(key) if (!entry) { entry = { required: false, schemas: [] } mergedUnionPropertyMap.set(key, entry) } entry.schemas.push(value) } } } + for (const [key, entry] of mergedUnionPropertyMap) { if (objectUnionSchemas.every(s => s.required?.includes(key))) { entry.required = true } } - for (const [key, entry] of mergedUnionPropertyMap) { - if (objectUnionSchemas.every(s => s.required?.includes(key))) { - entry.required = true - } - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/openapi/src/openapi-generator.ts(8 hunks)packages/openapi/src/openapi-utils.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/openapi/src/openapi-utils.ts (5)
packages/openapi/src/schema.ts (2)
JSONSchema(5-5)ObjectSchema(10-10)packages/openapi/src/openapi-generator.ts (3)
doc(190-276)doc(278-412)doc(414-541)packages/shared/src/array.ts (1)
toArray(1-3)packages/openapi/src/schema-utils.ts (1)
isObjectSchema(15-17)packages/shared/src/json.ts (1)
stringifyJSON(9-12)
packages/openapi/src/openapi-generator.ts (2)
packages/openapi/src/openapi-utils.ts (4)
simplifyComposedObjectJsonSchemasAndRefs(186-317)toOpenAPIParameters(103-138)resolveOpenAPIJsonSchemaRef(171-179)toOpenAPIContent(25-51)packages/openapi/src/schema-utils.ts (2)
isObjectSchema(15-17)applySchemaOptionality(135-146)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: publish-commit
- GitHub Check: test
- GitHub Check: lint
🔇 Additional comments (5)
packages/openapi/src/openapi-utils.ts (1)
311-313: LGTM: Required flag logic is correct.The use of
||correctly ensures a property is marked as required if either the union side or the intersection side declares it required. This properly handles cases whereallOfenforces a requirement thatanyOf/oneOfdoes not.packages/openapi/src/openapi-generator.ts (4)
314-316: LGTM: Schema preprocessing logic is correct.The conditional preprocessing appropriately applies schema simplification when:
- Input structure is
detailed(needs to extract params/query/headers/body)- Input structure is
compactwith dynamic path parameters (needs to extract and validate params)- Input structure is
compactwith GET method (needs to extract query parameters)This ensures composed schemas (anyOf/oneOf/allOf) are flattened before parameter extraction, which directly addresses the issue described in #1162.
370-372: LGTM: Params schema simplification is correctly applied.The simplification of the params schema before validation ensures that composed schemas (e.g.,
FooSchema.and(PlanetIdInputSchema)) are properly flattened so dynamic path parameters can be detected in theirrequiredarray.
386-403: LGTM: Consistent simplification across parameter sources.The loop correctly applies simplification to params, query, and headers schemas before validating they are object schemas and converting them to OpenAPI parameters. This ensures all parameter sources benefit from the composition-flattening logic.
473-540: LGTM: Success response handling correctly uses simplified schemas.The detailed output structure handling properly:
- Simplifies each union item before processing (line 473)
- Uses the simplified schema for all property accesses and checks
- Applies nested simplification to the headers schema (line 516)
- Correctly references
simplifiedItemfor required checks (lines 529, 537)This ensures composed response schemas are properly handled when extracting status, headers, and body.
dinwwwh
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/openapi/src/openapi-utils.test.ts(2 hunks)packages/openapi/src/openapi-utils.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/openapi/src/openapi-utils.test.ts (2)
packages/openapi/src/openapi-utils.ts (1)
simplifyComposedObjectJsonSchemasAndRefs(186-328)packages/openapi/src/openapi-generator.ts (3)
doc(190-276)doc(278-412)doc(414-541)
packages/openapi/src/openapi-utils.ts (4)
packages/openapi/src/schema.ts (2)
JSONSchema(5-5)ObjectSchema(10-10)packages/shared/src/array.ts (1)
toArray(1-3)packages/openapi/src/schema-utils.ts (1)
isObjectSchema(15-17)packages/shared/src/json.ts (1)
stringifyJSON(9-12)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint
- GitHub Check: publish-commit
- GitHub Check: test
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/openapi/src/openapi-utils.ts (1)
208-222: Consider optimizing the required-field computation.The
objectUnionSchemas.every(...)call on line 214 executes inside a nested loop for each property, resulting in O(schemas × properties × schemas) time complexity. For schemas with many union branches and properties, this could impact performance.You could refactor to compute the required status for all properties in a separate pass:
const mergedUnionPropertyMap: Map<string, { required: boolean, schemas: JSONSchema[] }> = new Map() for (const u of objectUnionSchemas) { if (u.properties) { for (const [key, value] of Object.entries(u.properties)) { let entry = mergedUnionPropertyMap.get(key) if (!entry) { - const required = objectUnionSchemas.every(s => s.required?.includes(key)) - - entry = { required, schemas: [] } + entry = { required: false, schemas: [] } mergedUnionPropertyMap.set(key, entry) } entry.schemas.push(value) } } } + + for (const [key, entry] of mergedUnionPropertyMap.entries()) { + entry.required = objectUnionSchemas.every(s => s.required?.includes(key)) + }This changes the complexity to O(schemas × properties + unique_properties × schemas), which is more efficient for complex schemas.
Based on past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/openapi/src/openapi-utils.test.ts(2 hunks)packages/openapi/src/openapi-utils.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/openapi/src/openapi-utils.test.ts (2)
packages/openapi/src/openapi-utils.ts (1)
simplifyComposedObjectJsonSchemasAndRefs(186-316)packages/openapi/src/openapi-generator.ts (3)
doc(190-276)doc(278-412)doc(414-541)
packages/openapi/src/openapi-utils.ts (4)
packages/openapi/src/openapi-generator.ts (3)
doc(190-276)doc(278-412)doc(414-541)packages/shared/src/array.ts (1)
toArray(1-3)packages/openapi/src/schema-utils.ts (1)
isObjectSchema(15-17)packages/shared/src/json.ts (1)
stringifyJSON(9-12)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: publish-commit
- GitHub Check: lint
- GitHub Check: test
🔇 Additional comments (5)
packages/openapi/src/openapi-utils.ts (5)
186-193: LGTM!The entry point correctly resolves references when a document is provided and efficiently short-circuits when the schema doesn't need simplification.
224-237: LGTM!Correctly processes
allOfschemas recursively and appropriately treats the base schema as part of the intersection when it's an object alongside composition keywords.
256-263: LGTM!Clean initialization of the result schema with appropriate early return when no properties are found.
265-276: LGTM!Straightforward deduplication using JSON stringification for schema equality checks.
278-313: LGTM!The property schema merging logic correctly combines union and intersection entries, using
anyOffor union alternatives andallOffor intersection constraints. The required-field logic on line 310 correctly uses||to mark a property as required if either composition type requires it.
Fixes #1162
Summary by CodeRabbit
New Features
Tests
Chores