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

OneOfValidator is filtering out the required errors if all the oneOf schemas are having the issues. #678

Closed
rahultokase opened this issue Mar 21, 2023 · 3 comments · Fixed by #720

Comments

@rahultokase
Copy link

We are using the following schema. In the following schema we have

  1. OuterObject with innerObject as a property.
  2. InnerObject is marked as oneOf String or Object with required properties.

{ "$schema": "http://json-schema.org/draft-07/schema#", "$id": "https://example.com/issue-470.json", "title": "OneOf validation message", "description": "Test description", "type": "object", "properties": { "outerObject": { "type": "object", "properties": { "innerObject": { "oneOf": [ { "type": "string" }, { "type": "object", "properties": { "value": { "type": "string" }, "unit": { "type": "string" } }, "additionalProperties": false, "required": [ "value", "unit" ] } ] } } } }, "additionalProperties": false }

the above schema was validated with below json data.
{ "outerObject": { "innerObject": {} } }

The errors given by oneOfValidator are
$.letterSpacing.wide: object found, string expected

The expected one was
$.outerObject.innerObject: object found, string expected
$.outerObject.innerObject.value: is missing but it is required
$.outerObject.innerObject.unit: is missing but it is required

After Inspecting the oneOfValidator code we figured out that

  1. we are filtering the required error deliberately in the oneOfValidator. Any Known reason for the same?
@AndreasALoew
Copy link
Contributor

I think your problem rather is that the existing implementation would not expect the
$.outerObject.innerObject: object found, string expected
message - as innerObject is specified to be either of type string or of type (nested) object. So the fact that there is an object found IMHO should not cause a validation message about a string being expected.

I think that the original code by @ksychla would work fine in case there were only the two "is required" errors, because in this case, that's exactly the special case which makes lines 204-206 of the original commit remove any "not required" errors from the validation messages...

                if (!childNotRequiredErrors.isEmpty()) {`
                    childErrors = childNotRequiredErrors;
                }

So it might be that the question you'd need to ask rather is: Why do we see the "object found, string expected" message here although it is NOT required or even expected by the schema at all that "innerObject" would be a string - it can as well be an object (as the "oneOf" specifies).

Hope I've succeeded to describe it well enough to be generally understandable (sorry, I'm no native speaker).

@stevehu
Copy link
Contributor

stevehu commented Mar 22, 2023

@rahultokase @AndreasALoew I think the following should be the returned error given the condition failFast is true or false.

If failFast == true
$.outerObject.innerObject.value: is missing but it is required

If failFast == false
$.outerObject.innerObject.value: is missing but it is required
$.outerObject.innerObject.unit: is missing but it is required

The first error "$.outerObject.innerObject: object found, string expected" should not show up as you do provide an object although it is empty.

What do you think?

@AndreasALoew
Copy link
Contributor

@stevehu , as I already tried to state yesterday, I also think that the "object found, string expected" is the root cause of the issue here, not the logic as implemented by @ksychla .

We are always using the validator with failFast == false here, so I cannot really comment which of the two "is missing but required" errors should be the one to be thrown with failFast == true, but other than that, I fully agree with your analysis.

@fdutton fdutton linked a pull request Apr 20, 2023 that will close this issue
stevehu pushed a commit that referenced this issue Apr 20, 2023
Corrects #678 #516 #197

Co-authored-by: Faron Dutton <faron.dutton@insightglobal.com>
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 a pull request may close this issue.

3 participants