Skip to content

Add unevaluatedProperties instance location coverage#905

Open
RishiByte wants to merge 1 commit into
json-schema-org:mainfrom
RishiByte:evaluated-properties-instance-location-tests
Open

Add unevaluatedProperties instance location coverage#905
RishiByte wants to merge 1 commit into
json-schema-org:mainfrom
RishiByte:evaluated-properties-instance-location-tests

Conversation

@RishiByte
Copy link
Copy Markdown
Contributor

@RishiByte RishiByte commented May 24, 2026

Resolves #889

Adds additional coverage for evaluated property collection respecting instance location.

This extends the existing unevaluatedProperties test coverage for draft 2019-09 and draft 2020-12 with related cases for:

  • patternProperties
  • additionalProperties

Each case verifies that a property evaluated inside a nested object does not cause a same-named property at another instance location to be treated as evaluated.

Validation:

  • Edited JSON files parse successfully
  • git diff --check passes

Note: Full bin/jsonschema_suite check currently fails locally on unrelated older-draft sanity checks involving default as an unknown keyword.

@RishiByte RishiByte requested a review from a team as a code owner May 24, 2026 16:42
@jdesrosiers
Copy link
Copy Markdown
Member

I'm not convinced this is necessary. The current test will make sure an implementation handles this case in general and I don't see any reason why it would be any different for patternProperties or additionalProperties. If you can show that there's an implementation that fails these tests, then I would support adding them. Hint: you can use the bowtie CLI to run a test against all the implementations on bowtie.

@RishiByte
Copy link
Copy Markdown
Contributor Author

I'm not convinced this is necessary. The current test will make sure an implementation handles this case in general and I don't see any reason why it would be any different for patternProperties or additionalProperties. If you can show that there's an implementation that fails these tests, then I would support adding them. Hint: you can use the bowtie CLI to run a test against all the implementations on bowtie.

Thanks for the feedback. I'll run these cases through Bowtie against the supported implementations and report back if they expose any implementation failures.

@RishiByte
Copy link
Copy Markdown
Contributor Author

@jdesrosiers Thanks for the suggestion. I ran the new cases through Bowtie and found that they expose an existing implementation issue.

Using Bowtie 2026.5.15, opis-json-schema (PHP) version 2.6.0 fails both added instance-location cases for both drafts:

  • draft 2019-09: patternProperties and additionalProperties
  • draft 2020-12: patternProperties and additionalProperties

For each failing case, the implementation incorrectly accepts:

{
  "foo": { "bar": "foo" },
  "bar": "bar"
}

@jdesrosiers
Copy link
Copy Markdown
Member

Ok, but that implementation also fails the existing similar test. My assertion is that the existing test should be enough to expose the issue. If opis-json-schema were to fix that failing test and the two being added here were not still failing, that would be an indication that the new tests are useful.

@jdesrosiers
Copy link
Copy Markdown
Member

Thanks @sinclairzx81 for the reference. Someone implementing JSON Schema saying that the test is useful to them is good enough for me.

Copy link
Copy Markdown
Member

@jdesrosiers jdesrosiers left a comment

Choose a reason for hiding this comment

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

There are tests here that don't have to do with the stated purpose of the PR. (Didn't we recently merge these? Do you need to rebase your branch?)

Please add the new tests to v1 as well.

@RishiByte RishiByte force-pushed the evaluated-properties-instance-location-tests branch from 82ed340 to 716f309 Compare May 28, 2026 03:22
@RishiByte
Copy link
Copy Markdown
Contributor Author

There are tests here that don't have to do with the stated purpose of the PR. (Didn't we recently merge these? Do you need to rebase your branch?)

Please add the new tests to v1 as well.

@jdesrosiers Thanks, fixed.

I rebased the branch onto the latest upstream main so the unrelated tests are no longer included in this PR, and I added the same instance-location coverage to v1/unevaluatedProperties.json as well.

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.

Coverage: Evaluated properties collection needs to consider instance location

2 participants