Fix merge logic for allOf alternatives #3515
Conversation
🦋 Changeset detectedLatest commit: b9e110b The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the merge logic for handling allOf alternatives in OpenAPI schema processing. The changes address issues with the schema merging conditions and add defensive programming to handle undefined properties.
- Changed merge conditions from
every()tosome()for determining when schemas can be merged - Added null checks and default values to prevent errors when accessing undefined properties
- Improved handling of enum, properties, and required arrays to avoid runtime exceptions
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/react-openapi/src/OpenAPISchema.tsx | Updated merge logic conditions and added defensive null checks for schema properties |
| .changeset/tame-months-knock.md | Added changeset entry documenting the bug fix |
| @@ -556,8 +556,10 @@ function mergeAlternatives( | |||
| schemaOrRef.enum | |||
| ) { | |||
| const keys = Object.keys(schemaOrRef); | |||
| if (keys.every((key) => ['type', 'enum', 'nullable'].includes(key))) { | |||
| latest.enum = Array.from(new Set([...latest.enum, ...schemaOrRef.enum])); | |||
| if (keys.some((key) => ['type', 'enum', 'nullable'].includes(key))) { | |||
There was a problem hiding this comment.
Changing from every() to some() fundamentally alters the merge condition logic. This will now merge schemas if ANY of the keys match the allowed list, rather than requiring ALL keys to match. This could cause unintended merging of schemas that have additional properties beyond the allowed set.
| if (keys.some((key) => ['type', 'enum', 'nullable'].includes(key))) { | |
| if (keys.every((key) => ['type', 'enum', 'nullable'].includes(key))) { |
Copilot uses AI. Check for mistakes.
| @@ -566,18 +568,20 @@ function mergeAlternatives( | |||
| if (latest && latest.type === 'object' && schemaOrRef.type === 'object') { | |||
| const keys = Object.keys(schemaOrRef); | |||
| if ( | |||
| keys.every((key) => | |||
| keys.some((key) => | |||
There was a problem hiding this comment.
Similar to the enum case above, changing from every() to some() will allow merging of object schemas even when they contain properties outside the allowed list ['type', 'properties', 'required', 'nullable']. This could lead to incorrect schema merging behavior.
| keys.some((key) => | |
| keys.every((key) => |
Copilot uses AI. Check for mistakes.
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
|
nope, opened it too fast, didn't spot any issues but I'm trying to make it safer |
No description provided.