Skip to content
This repository has been archived by the owner on Feb 15, 2021. It is now read-only.

Bug in IfThenElse #163

Closed
jinhong- opened this issue May 14, 2018 · 9 comments
Closed

Bug in IfThenElse #163

jinhong- opened this issue May 14, 2018 · 9 comments
Assignees
Labels
bug Something's wrong.

Comments

@jinhong-
Copy link

Hi, I found an issue when using IfThenElse. Looks like it's unable to validate dependent properties. I also noticed the Validator Selector does not select the validator governing the required validation because the schema does not have PropertyNames.

Check out my unit tests below:
master...jinhong-:master

@jinhong-
Copy link
Author

jinhong- commented May 14, 2018

I think removing the PropertyNames != null seems to do the trick

@jinhong-
Copy link
Author

By the way i noticed the sub validation error messages from if then else gets swallowed. Would it make sense to return the underlying sub validation error messages instead?

On a separate note, it would be good if there is a way to include the full property path since in theory you can have 2 of the same properties but in different paths eg. A.B, B.B C.B will all print as B is required but i am not sure which B is the correct B

@gregsdennis
Copy link
Owner

Regarding the error messaging, the exception has a Path property that allows you to see the full path to the offending element.

I'll check into the validation issue. Could you post your schema and test JSON? I pass the official schema test suite. They may be interested in adding your case.

@jinhong-
Copy link
Author

I took it from here:
https://ajv.js.org/keywords.html#ifthenelse

But if you notice i tweaked the schema in my test cases because i think there are still variations in how it is being implemented. Look at this particular case where the "minimum" of the "power" property is 9000.

In their example, an empty object is considered valid. But I personally believe it should not be valid because it'll fall into the "then" validation since the "if" validation is valid. And even if it isn't, it should at least fall into the "else" validation but in the example, they are treating it as valid.

@jinhong-
Copy link
Author

What i noticed is that the validation error comes out as:
Message: Validation of if succeeded, but validation of then failed
PropertyName: then
What I meant to say is instead of showing this, I think it would make sense to show "Required property not found" since that is the validation that was applied

@gregsdennis
Copy link
Owner

gregsdennis commented May 15, 2018

In their example, an empty object is considered valid. But I personally believe it should not be valid because it'll fall into the "then" validation since the "if" validation is valid. And even if it isn't, it should at least fall into the "else" validation but in the example, they are treating it as valid.

@handrews (the primary author of JSON Schema, currently) and I both agree with your assessment of this example. {} should be an invalid instance. I'll make a test to cover this case.

I can also add your test cases.


On the topic of messaging, I'll try to incorporate the error messages from the failed subschema.

@gregsdennis
Copy link
Owner

I have identified a bug in that required is ignored if there are no properties defined. This is incorrect behavior. I've fixed that, but I'm also working on the error messaging before I push out a new version.

I also still need to pull in your tests. I'm having some issues with running my tests right now. I think it's just a local issue; the server build seems to run fine, but I don't want to rely on it.

@gregsdennis gregsdennis self-assigned this May 20, 2018
@gregsdennis gregsdennis added the bug Something's wrong. label May 20, 2018
@gregsdennis
Copy link
Owner

gregsdennis commented Jun 26, 2018

@jinhong- you may be interested in #112 and json-schema-org/json-schema-spec#530. I think this will open up the mechanism to customizing the error messages.

I'm closing this issue in favor of that one since the if-then-else bug has been fixed.

@gregsdennis
Copy link
Owner

@jinhong- you may be interested in the available prerelease of v10. The schema implementation has been completely reworked, including the output format. I think you'll find that the new format is more explicit as to what the actual failure is.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something's wrong.
Projects
None yet
Development

No branches or pull requests

2 participants