fix(v4): allOf + additionalProperties: false rejects valid documents (#5390)#5702
fix(v4): allOf + additionalProperties: false rejects valid documents (#5390)#5702ebroder wants to merge 1 commit intocolinhacks:mainfrom
Conversation
…olinhacks#5390) In JSON Schema, additionalProperties only considers properties in the same schema object, not sibling allOf entries. This made intersections of strict objects unsatisfiable. For draft-2020-12 (when all entries are strict): replace per-entry additionalProperties: false with unevaluatedProperties: false on the wrapper. For older drafts (or mixed strict/loose): add {} placeholders for sibling property names. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approval: This is a well-crafted fix for a real JSON Schema semantics issue. The two-pronged approach (using unevaluatedProperties for draft-2020-12, placeholder properties for older drafts) is correct and handles the edge cases properly. Tests are comprehensive. Good to merge.
|
@pullfrog fresh review |
There was a problem hiding this comment.
Solid fix for a real JSON Schema interop bug. The two-strategy approach is well-suited to the problem: unevaluatedProperties: false on draft-2020-12 (which natively understands cross-subschema evaluation), and {} placeholder properties on older drafts. The helper functions are cleanly structured, the clone-on-write pattern avoids unnecessary allocations, and the isStrict semantics for allOf vs oneOf/anyOf are correct. Test coverage is thorough across both draft targets and multiple composition shapes (plain intersection, discriminated union, mixed strict/loose).
Two minor observations below — neither blocks merge.
Task list (5/5 completed)
- Read the full diff to understand the changes
- Review test changes in to-json-schema.test.ts
- Review implementation changes in json-schema-processors.ts
- Verify correctness of JSON Schema semantics
- Submit review
Claude Opus ｜ 𝕏
| // Returns the original object unchanged if nothing needed to be added. | ||
| function addSiblingPropertyPlaceholders(entry: JSONSchema.JSONSchema, allNames: Set<string>): JSONSchema.JSONSchema { | ||
| let cloned = false; | ||
| if (entry.additionalProperties === false && entry.properties) { |
There was a problem hiding this comment.
This guard skips entries with additionalProperties: false but no properties key. Such a schema would reject all properties including siblings'. In practice zod's objectProcessor always emits properties alongside additionalProperties: false, so this is safe — but a defensive entry.properties ??= {} before the missing-name check (or dropping the && entry.properties guard and initializing properties to {} when missing) would make the function correct for any JSON schema input, not just zod-generated ones.
| json: JSONSchema.JSONSchema, | ||
| target: string | ||
| ): void { | ||
| if (target === "draft-2020-12" && allOf.every(isStrict)) { |
There was a problem hiding this comment.
The target parameter is typed as string but checked via exact equality against "draft-2020-12". The other valid targets ("draft-04", "draft-07", "openapi-3.0") all correctly fall through to the placeholder path, and the ({} & string) union member in the target type makes this forward-compatible. Just noting that a new target supporting unevaluatedProperties (e.g., a future "draft-2024-xx") would need to be added here explicitly.
2 participants
In JSON Schema, additionalProperties only considers properties in the same schema object, not sibling allOf entries. This made intersections of strict objects unsatisfiable.
For draft-2020-12 (when all entries are strict): replace per-entry additionalProperties: false with unevaluatedProperties: false on the wrapper. For older drafts (or mixed strict/loose): add {} placeholders for sibling property names.