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

Fix for issue-366 #372

Merged
merged 2 commits into from
Feb 4, 2021
Merged

Fix for issue-366 #372

merged 2 commits into from
Feb 4, 2021

Conversation

Krishna-capone
Copy link
Contributor

The PR fixes the following:

For FailFast:

Now the OneOf Validator waits till all the subschemas are validated before throwing the exception instead of throwing the exception if the first subschema is invalid. I followed your suggestion for #209 (Looking to see if the parent schema is OneOf when deciding to throw the exception in BaseJsonValidator) and implemented it for OneOf.

If one of the subschema is valid and the other is not it does not throw exception which the current oneOf Validator does.

When all the subschemas are invalid it throws the following exception (this is an example for the test case provided):

com.networknt.schema.JsonSchemaException: $: should be valid to one and only one of the schemas but both schemas {{"type":"integer"}{"minimum":2}} are valid.

This has the detail about the subschemas. This is needed as there may be multiple oneOf conditions which are invalid in a schema and it would be difficult for the user to know which oneOf condition failed.

when neither of the subschemas are valid it throws the following exception:
com.networknt.schema.JsonSchemaException: [$: number found, integer expected, $: must have a minimum value of 2]

This has the detail about the subschemas. This detail is needed as there may be multiple oneOf conditions which are invalid in a schema and it would be difficult for the user to know which oneOf condition failed.

For Failslow:

when both of the subschemas are valid the following msg is sent:
$: should be valid to one and only one of the schemas but both schemas {{"type":"integer"}{"minimum":2}} are valid

when neither of the subschemas are valid the following messages are sent:
$: number found, integer expected
$: must have a minimum value of 2

@codecov-io
Copy link

codecov-io commented Jan 29, 2021

Codecov Report

Merging #372 (093d38e) into master (a5f9f7f) will decrease coverage by 0.23%.
The diff coverage is 60.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #372      +/-   ##
============================================
- Coverage     69.74%   69.51%   -0.24%     
- Complexity      697      700       +3     
============================================
  Files            81       81              
  Lines          2760     2778      +18     
  Branches        556      560       +4     
============================================
+ Hits           1925     1931       +6     
- Misses          621      630       +9     
- Partials        214      217       +3     
Impacted Files Coverage Δ Complexity Δ
...n/java/com/networknt/schema/BaseJsonValidator.java 66.07% <50.00%> (-1.21%) 21.00 <2.00> (+1.00) ⬇️
...main/java/com/networknt/schema/OneOfValidator.java 59.82% <62.06%> (-4.18%) 11.00 <2.00> (+2.00) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a5f9f7f...093d38e. Read the comment docs.

@stevehu
Copy link
Contributor

stevehu commented Jan 30, 2021

@prubdeploy Could you please review the changes to ensure that your use case won't be impacted. It looks good to me in my initial review as all existing test cases are passed. Thanks.

@guillomep
Copy link

I have the exact same case, where I want to retrieve "children" errors when the OneOf fails.

@stevehu
Copy link
Contributor

stevehu commented Feb 2, 2021

@guillomep It is nice to know this feature is welcomed. Could you please help to review the changes? Just want to make sure it covers your use case. Thanks.

@guillomep
Copy link

Beside my reviews I think it is okay.

@prubdeploy
Copy link
Contributor

@prubdeploy Could you please review the changes to ensure that your use case won't be impacted. It looks good to me in my initial review as all existing test cases are passed. Thanks.
@stevehu The changes look good to me and doesn't impact my case and infact cover more scenarios as OneOf is considered.

@@ -1,70 +1,4 @@
[
{
Copy link
Contributor

Choose a reason for hiding this comment

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

I tried to copy the original file to your branch and it works without any issue. I am wondering why some of the test cases are removed only in the draft4.

@stevehu stevehu merged commit 3758d52 into master Feb 4, 2021
@stevehu stevehu deleted the fixfor-366 branch February 4, 2021 13:25
@stevehu
Copy link
Contributor

stevehu commented Feb 4, 2021

Thanks a lot for your help. I have released a new version.

https://github.com/networknt/json-schema-validator/releases/tag/1.0.48

@guillomep
Copy link

Thank you !
Do you know when it will be available on Maven Central ?

@stevehu
Copy link
Contributor

stevehu commented Feb 4, 2021

Sonatype is down today. It will take some time for it to recovery.

https://status.maven.org/?utm_source=embed

@stevehu
Copy link
Contributor

stevehu commented Feb 5, 2021

Sonatype is up and running. The release should be available from the maven central. Enjoy!!!

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.

6 participants