Fast fail issue with One Of Validator

[Krishna-capone]

We were having issues with the One Of Validator and i ran the "One Of" test cases provided in your test suite to confirm the issues my team was having:

{
  "description": "oneOf",
  "schema": {
    "oneOf": [
      {
        "type": "integer"
      },
      {
        "minimum": 2
      }
    ]
  },
  "tests": [
    {
      "description": "neither oneOf valid",
      "data": 1.5,
      "valid": false
    }
  ]
}

The "One of" validator just gives this error message:

$: should be valid to one and only one of the schemas
but not all the reasons.
I have changed the code so that the following are returned:

$: number found, integer expected
$: must have a minimum value of 2

This is similar to the anyOf errors. Just wanted the confirmation for the errors returned . If everyone agrees on the errors i can do a PR.

[stevehu]

@prubdeploy @jiachen1120 Is this issue related to the one we fixed recently?

#354

[prubdeploy]

Yes and the errors he posted are right.

[stevehu]

@Krishna-capone The oneOf validator is pretty complicated when combining with other validators and we used to implement the way you mentioned. You can see how many PRs linked to this class in the Git history. Days ago, we made the decision to make this validator fail-fast based on the following reasons in #354.

1. The library is mainly used in the light-4j framework for OpenAPI specification and severless schema validations. Both frameworks are designed as fail-fast.
2. The official test suites only cover a minimum number of scenarios and our fail-fast implementation meets the requirement.
3. Most developers under-estimated the effort to get it fully implemented to cover all scenarios.

I don't know your use case in detail. If you want to return all the errors, you can dig into the historical implementations and use a flag fail-fast = false to switch to your implementation. The library does have a config file to customize the behaviour in order to meet the requirement for broader users. Let me know what you think. Thanks.

[Krishna-capone]

Thank You for your reply @stevehu. I tested the OneOf Validator with the code that was present before the fix for #354 was done with our team's schema and also with the test suite schema in your repo and they all seem to work fine. Also, i could not find the test data for which the pre #354 code was failing for the author of #354. The test case that was submitted as part of #354 fix was

     {
        "description": "neither oneOf given (complex)",
        "data": {},
        "valid": false
      }	

The pre #354 code, ( the original code ), would have failed for this test case because there is an issue in MininumValidator which is not accounting for empty data {}. I tested a fix for handling the empty data in MinumumValidator locally. I feel any error the author of #354 fix would have encountered would have been due to some other bug/issue but not because of any issue in the "OneOf Validator" .

Anyway as per your suggestion , I have added my fix to the latest code using the failfast flag. So, when the failfast flag is true, then it would behave as the #354 fix and if it is false then it would return the additional errors. Thank you for your help in this. Do i need any invite from you to create a PR? Thank you.

[stevehu]

@Krishna-capone I am glad that the existing code works for your use case. Fix #354 is not to resolve the OneOf issue but some combination of subschemas. Also, it is the right behaviour to fail fast in microservices.

Since we have two different implementations now, we need to split the test cases into common, fail fast and fail slow. This can ensure that any future changes won't cross-fire to the other implementation. I have sent you an invite and it would be better to create a branch for your PR in this repo. Let me know if you need any help from the team. Thanks.

[Krishna-capone]

@stevehu Currently if the Failfast flag is set, it throws an exception on the first error encountered which in the following case

{
  "description": "oneOf",
  "schema": {
    "oneOf": [
      {
        "type": "integer"
      },
      {
        "minimum": 2
      }
    ]
  },
  "tests": [
    {
      "description": "neither oneOf valid",
      "data": 1.5,
      "valid": false
    }
  ]
}

would be

com.networknt.schema.JsonSchemaException: $: object found, integer expected

This is because of the following code in the BaseJsonValidator:

protected ValidationMessage buildValidationMessage(String at, String... arguments) {
      final ValidationMessage message = ValidationMessage.of(getValidatorType().getValue(), errorMessageType, at, arguments);
      if (failFast) {
          throw new JsonSchemaException(message);
      }
      return message;
  }

From one of your above comments in this chain , the #354 is supposed to be a failfast implementation

@Krishna-capone The oneOf validator is pretty complicated when combining with other validators and we used to implement the way you mentioned. You can see how many PRs linked to this class in the Git history. Days ago, we made the decision to make this validator fail-fast based on the following reasons in #354.

but it does not throw exception with the message "should be valid to one and only one of the schemas" as #354 implementation expects. So, i feel #354 was really not implemented as how other Validator's (TypeValidator, MinimumValidators etc.,) failfast behaves.

so to be consistent with other Validator's failfast behavior and what #354 implementation wanted, I can do the following:

1. when the failfast flag is set to true, i can capture the first exception thrown by other Validators (TypeValidators, or MininumValidators etc.,) in OneOfValidators and throw the exception with a exception message OneOf validator is not throwing valid error if any of the child nodes has invalid schemas. #354 suggests.

2. and when the failfast flag is set to false, i can return all the errors as ValidationMessages just like the pre OneOf validator is not throwing valid error if any of the child nodes has invalid schemas. #354 implementation does.

I hope i have explained thing properly. Please let me know. Thank you for all the suggestions on this.

[stevehu]

The fail-fast implementation was contributed by one of the users and it is not 100% completed. For details, please visit #209

As the current behaviour is fail-fast as default, we just need to make sure the changes won't impact the default behaviour. We might need to create separate test tests for with fail-fast and fail-slow if necessary.

[Krishna-capone]

I fixed the following. But i could not push in my changes to the branch fixfor-366 which i had created.. When i push i get the error error: RPC failed; HTTP 403 curl 22 The requested URL returned error: 403. @stevehu do i need to do anything special or do i need any permission?

The fix:

For FailFast:

1. 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 failFast fails too early in case of AnyOf validator #209 (Looking to see if the parent schema is OneOf when deciding to throw the exception in BaseJsonValidator) and implemented it for OneOf.

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

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

4. 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:

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

2. when neither of the subschemas are valid the following messages are sent:

$: number found, integer expected
$: must have a minimum value of 2

I have provided separate test cases for failfast and failsafe scenarios.

[stevehu]

I am guessing you are using HTTPS instead of SSH. GitHub requires two-factor authentication and you have to use SSH.

[Krishna-capone]

@stevehu Thanks for all your suggestions and help. I have created a PR (#372) for the fix. Please review the changes. I have added separate test cases for failfast and failslow scenarios as you had suggested.

[fdutton]

This was resolved in #372