Skip to content

fix: keep a false property subschema when a conditional branch adds a constraint - #276

Merged
thiamsantos merged 1 commit into
mainfrom
fix-property-false-overwritten-by-conditional-branch
Sep 25, 2026
Merged

thiamsantos merged 1 commit into
mainfrom
fix-property-false-overwritten-by-conditional-branch

Conversation

@thiamsantos

@thiamsantos thiamsantos commented Sep 23, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Fixes a bug where a false property subschema was dropped when another allOf branch constrained the same property. The property then rendered as a normal field, and validation accepted a value the schema forbids.

Changes Made

mergeSchemaBranch merges each applied conditional branch into the base schema. When one branch set a property to false (forbidden) and a sibling branch added a constraint like { maximum: 12 }, the merge overwrote the false with the constraint object. A false subschema is unsatisfiable, so allOf semantics keep the property forbidden regardless of sibling constraints.

  • Keep the false when merging property subschemas instead of overwriting it.
  • Scope the check to property maps (properties / patternProperties) so a branch can still relax a boolean keyword such as additionalProperties: false.
  • Fixes both paths: the field is now hidden, and a value for the forbidden property is now rejected.

Repro (before this change, createHeadlessForm with contract_duration_type: 'indefinite'):

allOf: [
  { if: { properties: { contract_duration_type: { const: 'fixed_term' } }, required: ['contract_duration_type'] },
    then: { required: ['months'] },
    else: { properties: { months: false } } },
  { if: true, then: { properties: { months: { maximum: 12 } } } },
]
// Before: `months` visible, and handleValidation({ months: 5 }) passed.
// After:  `months` hidden, and handleValidation({ months: 5 }) reports an error.

Note

Medium Risk
Changes core conditional schema merge logic used when building forms; scoped to property maps but affects visibility and validation for complex allOf schemas.

Overview
Fixes conditional allOf merging so a false property subschema (forbidden field) is not replaced when another branch adds constraints on the same key (e.g. { maximum: 12 }).

mergeSchemaBranch now tracks when it is merging inside properties / patternProperties and skips overwriting an existing false subschema in that context, matching JSON Schema allOf semantics. The guard is not applied to boolean keywords like additionalProperties, so branches can still relax those.

End-to-end behavior: forbidden fields stay hidden and validation rejects values that should not be allowed when a sibling branch only constrains the same property. New unit and visibility tests cover merge edge cases and the contract-duration / months scenario.

Reviewed by Cursor Bugbot for commit b710af2. Bugbot is set up for automated code reviews on this repo. Configure here.

… a constraint

When two allOf branches touch the same property — one setting it to `false`
(forbidden) and another adding a constraint like `{ maximum }` — mergeSchemaBranch
overwrote the `false` with the constraint object. The property then rendered as a
normal visible field and validation accepted a value the schema forbids.

A `false` property subschema is unsatisfiable, so allOf semantics keep the property
forbidden regardless of sibling constraints. Keep the `false` when merging property
subschemas. The check is scoped to property maps so a branch can still relax a
boolean keyword such as `additionalProperties: false`.

Fixes both the field-build path (field is now hidden) and the validation path (a
value for the forbidden property is now rejected), covered by unit tests on
mergeSchemaBranch and an integration test through createHeadlessForm.
@thiamsantos
thiamsantos force-pushed the fix-property-false-overwritten-by-conditional-branch branch from 4ad1756 to b710af2 Compare September 23, 2026 17:30
@thiamsantos
thiamsantos marked this pull request as ready for review September 23, 2026 17:32
@ollyd
ollyd self-requested a review September 25, 2026 02:59
@thiamsantos
thiamsantos merged commit e2e9201 into main Sep 25, 2026
6 checks passed
@thiamsantos
thiamsantos deleted the fix-property-false-overwritten-by-conditional-branch branch September 25, 2026 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants