#354-fixed OneOf validation error if number of valid schemas is not equal to 1

[prubdeploy]

OneOf validator is not throwing valid error if any of the child nodes has invalid schemas.
Proposed Fix: If there is no valid schema in child nodes, then clear existing error list which got generated while validating child nodes and push OneOf validation to the error list as per "OneOf" attribute requirement.

Since, "OneOf" validator, in every scenario, validates and generates the error list while validating the child nodes. The error list has to be cleared, in order, to push OneOf validation to the error list as per "OneOf" attribute requirement.

Once the "OneOf" scenario is not detected after counting the valid schemas i.e. 'if (numberOfValidSchema != 1)', then OneOf validation error is pushed to the error list as per "OneOf" attribute requirement.

[codecov-io]

Codecov Report

Merging #355 (646f80c) into master (13a5f0a) will decrease coverage by 0.08%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master     #355      +/-   ##
============================================
- Coverage     69.94%   69.86%   -0.09%     
+ Complexity      697      693       -4     
============================================
  Files            80       80              
  Lines          2745     2734      -11     
  Branches        569      565       -4     
============================================
- Hits           1920     1910      -10     
  Misses          610      610              
+ Partials        215      214       -1     

Impacted Files	Coverage Δ	Complexity Δ
...main/java/com/networknt/schema/OneOfValidator.java	68.96% <100.00%> (-1.45%)	8.00 <0.00> (-3.00)
...n/java/com/networknt/schema/ValidationMessage.java	61.11% <0.00%> (-1.39%)	12.00% <0.00%> (-1.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 13a5f0a...646f80c. Read the comment docs.

[stevehu]

@prubdeploy I have reviewed the code but not fully understand the fix. This validator has been updated by so many developers and it is never fixed completely. I am wondering if you could add more test cases to cover the fix you have put in. I am afraid some other use cases might be broken due to this fix. @jiachen1120 could you please review the PR and provide your comments? Thanks.

[jiachen1120]

@stevehu Hi Steve, I think Prudhvi's change simplifies the logic of oneOf. Since the validation handler within light-rest-4j can only return the first error in the error list, when the subset validation in oneof fails, I think we should only return oneof which fails, not which specific subset fails. Therefore, we don't need to record the specific failures returned by the subset of oneof, we can judge the returned results only by the number of validschema.

If validschema! = 1, I think we can all return oneof verification failure, but only when validaschema == 1, the verification is successful.

Previous logic cause this issue when handle validschema == 0 , since it may return the subset failure instead of oneof failure.

Look forward to your opinion. Thanks!

[stevehu]

@jiachen1120 The light-4j is designed as fail-fast; however, the json-schema-validator is not. There are a lot of users who are using it for different use cases. We cannot assume they want fail-fast from the validator. I am not saying the implementation might have some scenarios missed but only suggest we add more test cases to cover enough scenarios to make all of us comfortable. The other reason to have more test cases is to ensure nobody in the future makes the change to break the validator. What do you think?

[jiachen1120]

@stevehu I see, thanks! Too have more test case is totally make sense. Regarding to the fail fast, I think this fix won't lead to fail fast or not, oneOf validation path still the same, only different is the eventually generated error message. And it is true we need to have more test case to make sure it works like what we assumed.

@prubdeploy Could you please adding more test cases to cover more senarios? For example, 1. no one valid, and expected error message. 2. one valid, expected success 3. more than one valid, and expected error message. Intead of countting the error's num, probably also check the error message itself would be more accurate to cover this change, also, feel free to adding more corner cases. what do you think?

[prubdeploy]

@stevehu @jiachen1120 I think all the OneOf scenarios are covered under "src/test/resources/draft6/oneOf.json". I even added a new scenario when "neither oneOf given (complex)". All these test cases were passed when "src/test/java/com/networknt/schema/V6JsonSchemaTest.java" is run. As per my understanding, "oneOf.json" covers all the scenarios. If anymore needs to be added, please guide me through.

[stevehu]

Thanks a lot for the confirmation.

[prubdeploy]

Hi @stevehu,
The light-4j v1.6.30 is using json-schema-validator v1.0.29. But the latest changes we made are in json-schema-validator v1.0.46. The client needs these changes to be reflected as soon as possible. Could you please update the latest version of json-schema-validator v1.0.46 to light-4j and cut a new emergency release light-4j v1.6.31. Thank you.
@miklish @jiachen1120

Thanks,
Prudhvi

[stevehu]

There are numeric changes between 1.0.29 and 1.0.46 and I want you guys to get a project to test out before moving to 1.0.46. For any project, it is very easy to explicitly upgrade the json-schema-validator to 1.0.46 in the pom.xml to overwrite the default dependency in the light-rest-4j. Once we have confirmation from the test project, we would be more comfortable to upgrade to 1.0.46 for everybody. If we upgrade the light-rest-4j dependency, then all future builds for every project will be upgraded immediately. What do you think?