networknt / json-schema-validator

A fast Java JSON schema validator that supports draft V4, V6, V7, V2019-09 and V2020-12
Apache License 2.0
800 stars 320 forks source link

Remove incorrect logic for oneOf, anyOf and properties #1053

Closed justin-tay closed 3 weeks ago

justin-tay commented 1 month ago

This removes the incorrect logic introduced as part of the fixes for

This incorrect logic applies to applicators such as oneOf such that it will attempt to special handle cases where there is no validation failures but there are optional properties that are missing.

There is actually no special logic differences between JSON Schema and OpenAPI with regards to the handling of applicators such as oneOf. This misunderstanding likely arose due to the incorrect example for oneOf on one of the swagger websites which has not been fixed. The OpenAPI spec authors has clarified that the example is wrong and is not compliant with the OpenAPI spec.

codecov-commenter commented 1 month ago

Codecov Report

Attention: Patch coverage is 77.77778% with 10 lines in your changes missing coverage. Please review.

Project coverage is 78.81%. Comparing base (48ca3c2) to head (25705ed). Report is 21 commits behind head on master.

Files Patch % Lines
...etworknt/schema/AdditionalPropertiesValidator.java 80.00% 2 Missing and 2 partials :warning:
...in/java/com/networknt/schema/DependentSchemas.java 60.00% 1 Missing and 1 partial :warning:
...c/main/java/com/networknt/schema/NotValidator.java 50.00% 1 Missing and 1 partial :warning:
...main/java/com/networknt/schema/OneOfValidator.java 33.33% 1 Missing and 1 partial :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1053 +/- ## ============================================ - Coverage 78.90% 78.81% -0.10% - Complexity 1965 2017 +52 ============================================ Files 172 190 +18 Lines 6352 6513 +161 Branches 1255 1244 -11 ============================================ + Hits 5012 5133 +121 - Misses 867 910 +43 + Partials 473 470 -3 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

s-ven commented 4 days ago

Hum, should this change have been part of a MINOR revision change (1.4.0 to 1.4.1) ?

It is quite often that trying to upgrade to a more recent MINOR revision of networknt will breaks our JSON schemas

stevehu commented 4 days ago

I didn't realize that the changes broke the user's code. What is the best way to have the next release? Should we remove the 1.4.1, 1.4.2 and 1.4.3 and release 1.5.0?

justin-tay commented 4 days ago

I think it's too late to remove 1.4.1, 1.4.2, 1.4.3 as now that will break the libraries/users that are already using those versions.

I actually consider this change to be a bug fix. It should be noted that none of the existing tests nor even the additional tests actually hit the affected removed codes. Even the invalid swagger example actually behaved like the standard applicator, ie. the swagger example was already no longer applicable and didn't consider nodes that failed with required to be acceptable, likely this was fixed by other changes as these codes aren't specific to openapi and would have affected the standard json schema evaluations. This possibly introduced very subtle hard to find bugs in standard evaluations.

s-ven commented 4 days ago

agree, too late to remove, that's not what I'm asking, I just wanted you guys to know about this (it happened also when I migrated from 1.3.0 to 1.3.x, I had to refactor the schema builder code) thanks for prompt answer, and also for building this library !

stevehu commented 2 days ago

@s-ven I have merged all the latest PRs and released the 1.5.0 tag. Thanks a lot for the suggestions. It is hard to determine if any change can break users' code as different users will use the library differently. It is important for our users to report these breaking changes to us. Thanks a lot for your help.