Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove incorrect logic for oneOf, anyOf and properties #1053

Merged
merged 4 commits into from
Jun 11, 2024

Conversation

justin-tay
Copy link
Contributor

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
Copy link

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 ⚠️
...in/java/com/networknt/schema/DependentSchemas.java 60.00% 1 Missing and 1 partial ⚠️
...c/main/java/com/networknt/schema/NotValidator.java 50.00% 1 Missing and 1 partial ⚠️
...main/java/com/networknt/schema/OneOfValidator.java 33.33% 1 Missing and 1 partial ⚠️

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

Additional details and impacted files
@@             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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@stevehu stevehu merged commit 3957163 into networknt:master Jun 11, 2024
4 checks passed
@justin-tay justin-tay deleted the fixlogic branch June 12, 2024 01:59
@s-ven
Copy link

s-ven commented Jul 4, 2024

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
Copy link
Contributor

stevehu commented Jul 5, 2024

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
Copy link
Contributor Author

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
Copy link

s-ven commented Jul 5, 2024

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
Copy link
Contributor

stevehu commented Jul 6, 2024

@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.

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.

4 participants