Skip to content

fix(no-required-schema-properties-undefined): resolve false positives for bare-required schemas in allOf-composed types - #2836

Open
jeremyfiel wants to merge 1 commit into
Redocly:mainfrom
jeremyfiel:refactor/no-required-schema-properties-undefined-rule
Open

fix(no-required-schema-properties-undefined): resolve false positives for bare-required schemas in allOf-composed types#2836
jeremyfiel wants to merge 1 commit into
Redocly:mainfrom
jeremyfiel:refactor/no-required-schema-properties-undefined-rule

Conversation

@jeremyfiel

@jeremyfiel jeremyfiel commented May 22, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes false positives in no-required-schema-properties-undefined for bare required constraints used in parent-property composition contexts.

Problem

The no-required-schema-properties-undefined rule could report false positives for valid JSON Schema patterns where a bare required list is used as a presence constraint on a property sub-schema, while the property’s actual type is defined through a parent composition context.

This affected two cases:

  1. Bare-required property sub-schema override
properties:
  personName:
    required: [givenName, familyName]
allOf:
- $ref: '#/components/schemas/PersonBase'
  1. Constraint-fragment branches
properties:
  communication:
    oneOf:
    - required: [landlines]
    - required: [mobiles]
allOf:
- $ref: '#/components/schemas/PersonBase'

Both patterns are valid under JSON Schema, where required and properties are independent keywords.

Solution

The rule now checks the parent-property context before reporting a missing required property. When the usual local-property and composition-root checks do not find the required key, it walks up the ancestor chain to find the containing property schema and then evaluates sibling composition branches (allOf/anyOf/oneOf) for that property.

This refactor keeps the logic focused on the parent-property context rather than treating it as a general composition-root lookup. In particular, the new parent-context check is distinct from findCompositionRoot / isCompositionChild: the latter answers whether the current schema belongs to a composition chain, while the new logic answers whether the enclosing property owns the schema and whether a sibling compositional branch defines the required property.

This preserves the existing behavior for misspellings: if a required key is not actually defined in the resolved property type, the rule still reports it as undefined.

Testing

Added regression coverage for:

  • bare required constraints on property sub-schemas resolved through a parent allOf sibling
  • oneOf-style constraint fragments
  • anyOf-style constraint fragments
  • existing misspelling behavior and other composition-related cases
  • a new test ensuring the rule still reports undefined properties when the required key is not present in the parent-context sibling composition

Documentation

Updated the V2 rule documentation to describe the supported parent-context behavior for oneOf/anyOf constraint fragments.

relates to #2320 #2060 #2061

Check yourself

  • This PR follows the contributing guide
  • All new/updated code is covered by tests
  • Core code changed? - Tested with other Redocly products (internal contributions only)
  • New package installed? - Tested in different environments (browser/node)
  • Documentation update has been considered

Security

  • The security impact of the change has been considered
  • Code follows company security practices and guidelines

Note

Low Risk
Scoped to one lint rule’s property-resolution logic with broad test and doc updates; no auth, runtime API, or data-path changes.

Overview
Fixes false positives in no-required-schema-properties-undefined when schemas use valid JSON Schema patterns: property sub-schemas with only required (no local properties), or oneOf/anyOf branches that are pure required fragments, while the property’s shape comes from a parent allOf sibling (often via $ref).

The rule’s Schema visitor now falls back to hasPropertyInParentContext: it walks ancestor schemas, finds which named property holds the current sub-schema, and checks allOf/anyOf/oneOf siblings for definitions of keys listed in required. Misspelled required keys still fail that check and are reported.

Lint messages from the main schema path use is undefined instead of is not defined. Docs add PersonBase/Person examples for these patterns; a changeset marks @redocly/openapi-core and @redocly/cli as patch.

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

@changeset-bot

changeset-bot Bot commented May 22, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: d34a79f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
@redocly/openapi-core Patch
@redocly/cli Patch
@redocly/client-generator Patch
@redocly/respect-core Patch

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

@jeremyfiel
jeremyfiel force-pushed the refactor/no-required-schema-properties-undefined-rule branch from 8856aa1 to 929f198 Compare May 22, 2026 18:42
@jeremyfiel jeremyfiel changed the title refactor(rules): two use cases not handled by current implementation fix(no-required-schema-properties-undefined): resolve false positives for bare-required schemas in allOf-composed types May 22, 2026
@jeremyfiel
jeremyfiel marked this pull request as ready for review May 22, 2026 18:48
@jeremyfiel
jeremyfiel requested review from a team as code owners May 22, 2026 18:48
@jeremyfiel

Copy link
Copy Markdown
Contributor Author

cc @baywet @Drowze would appreciate your feedback if you are using these constructs.

@jeremyfiel
jeremyfiel force-pushed the refactor/no-required-schema-properties-undefined-rule branch from 929f198 to 2cc6d1f Compare May 26, 2026 03:45

@tatomyr tatomyr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finally got my hands on this. It looks mostly okay for me. However, I'd like to ask you to explore the possibility to solve this without introducing a new concept leveraging the existing logic (if possible).

- Name
```

The rule accepts bare `required` constraints on property sub-schemas when the property's type is defined in a parent `allOf` sibling. This is a valid JSON Schema pattern for adding presence constraints on top of a referenced base type:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not modify V1 docs.

Comment on lines +123 to +124
!hasProperty(compositionRoot, requiredProperty, new Set()) &&
!hasPropertyInParentContext(requiredProperty, compositionRoot ?? currentSchema)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line actually seems like it should do the same as the previous one, but from a different prospective. Maybe it worth modifying findCompositionRoot / isCompositionChild instead?
Also, the existing code has protection against circular references. Does your code have it? (I haven't found that in your tests.)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think line 85 is actually a different concern from findCompositionRoot / isCompositionChild.

  • isCompositionChild only checks whether the current schema is an item in a parent allOf / anyOf / oneOf.
  • findCompositionRoot then walks that composition chain upward to the outer composed ancestor.

The logic at L85 is not just “what composition chain this schema belongs to”; it is “which ancestor property owns this schema, and does a sibling compositional branch define the required property?”

So I’d keep findCompositionRoot for composition-root lookup, and keep the parent-property / sibling-branch logic separate.

I did update the code a bit to reuse some existing utils like getOwn.

@tatomyr
tatomyr marked this pull request as draft August 6, 2026 16:50
@jeremyfiel
jeremyfiel force-pushed the refactor/no-required-schema-properties-undefined-rule branch from 2cc6d1f to c56e1ea Compare August 7, 2026 00:21
@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Performance Benchmark (Lower is Faster)

CLI Version Bundle Lint Check Config
cli-latest ▓▓ 1.06x ± 0.01 ▓▓ 1.04x ± 0.01 ▓▓▓▓▓▓▓ 1.20x ± 0.01
cli-next ▓ 1.00x (Fastest) ▓ 1.00x (Fastest) ▓ 1.00x (Fastest)

@jeremyfiel
jeremyfiel force-pushed the refactor/no-required-schema-properties-undefined-rule branch 2 times, most recently from f04e878 to 405e289 Compare August 7, 2026 00:29
@jeremyfiel
jeremyfiel force-pushed the refactor/no-required-schema-properties-undefined-rule branch from 405e289 to d34a79f Compare August 7, 2026 00:30
@jeremyfiel
jeremyfiel marked this pull request as ready for review August 7, 2026 00:32

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit d34a79f. Configure here.

propertyDef !== undefined &&
schemaHasProperty(propertyDef, propertyName, ctx, new Set(), location)
);
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sibling property lookup skips composition

Medium Severity

checkSiblings only reads a sibling’s direct properties map and returns false when that map is missing. Property keys defined through nested allOf/anyOf/oneOf on the resolved sibling are ignored, so valid bare-required overrides against composed base schemas still false-positive. This diverges from schemaHasProperty, which already walks composition.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit d34a79f. Configure here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this could be important for some very advanced schema users but, also a pretty advanced edge case. I'll play with this a bit and see if I can get it working with nested composition but, as it stands, I think I'm ok with the current implementation, if you are.

) {
ctx.report({
message: `Required property '${requiredProperty}' is not defined.`,
message: `Required property '${requiredProperty}' is undefined.`,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent required-property error text

Low Severity

The Schema enter path now reports is undefined, but reportUndefinedRequired used by the ref leave path still reports is not defined. Same rule, same condition, different user-facing text, and the docs only document the new wording.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit d34a79f. Configure here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'll leave this one to you if you want to update the grammar.

@jeremyfiel
jeremyfiel requested a review from tatomyr August 7, 2026 00:38
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.

3 participants