-
-
Notifications
You must be signed in to change notification settings - Fork 880
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
keep properties of invalid schemas #553
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR!
- The change of behaviour is only possible for a specific value of removeAdditional option, as you proposed in removeAdditional doesn't work well with anyOf, oneOf or allOf #129 (comment), it cannot be done for all option values. Not sure if "afterValidation" is clear enough, I will think about it, but the main point is that it should be something specific. Current values should keep the current behaviour.
- In addition to the test you added (where the option value should be changed), there should be two more tests
- for the option
removeAdditional: true
the property is removed even though the validation failed (the current behaviour) - for the new option value the property is removed when validation passed
- for the option
An open question is how would be handled the situation when additionalProperties is a schema? There three possibilities
- just preserve old behaviour
- we need two new option values (new behaviour for current options
false
and"fail"
) - create a completely new option, e.g.
removeAdditionalOnSuccess
: true
I need to sleep on it, but the last choice seems better.
For choices 2 or 3 there will be needed additional 3 tests (same as above) for cases when additionalProperties is a schema.
What do you think?
spec/options.spec.js
Outdated
object.should.have.property('baz'); | ||
object.should.not.have.property('fizz'); | ||
console.log(ajv.getSchema('//test/fooBar2').toString()); | ||
ajv.validate('//test/fooBar2', failingObject).should.equal(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@epoberezkin I think I found a bug in master. When additionalProperties
is a schema, properties are not validated (and this test fails at this location 😬 .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(it's only a problem if you supply an invalid additional property with an additional properties schema, and removeAdditional: 'failing'
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@epoberezkin I figured out the issue. $nextValid
was not being unset when the additional property was removed, which caused the function to exit with no errors.
0609054
to
755cd79
Compare
@epoberezkin I can go with |
@epoberezkin note that the diff is much easier to read if you ignore whitespace ( |
@sberan I will review, thank you |
@@ -121,7 +131,8 @@ var {{=$nextValid}} = true; | |||
if (errors) validate.errors.length = errors; | |||
else validate.errors = null; | |||
} | |||
delete {{=$data}}[{{=$key}}]; | |||
{{=$nextValid}} = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setting this field to true fixed the bug I mentioned.
@sberan Thank you! I don't think it is going to work as expected (that is to only remove additional properties when otherwise current schema succeeds). In your code additional properties are removed when all other properties succeed, but it doesn't check that other keywords succeed, e.g. Also the changes in tests are difficult to follow. I suggest not touching the existing tests and instead add other tests that validate that without this new option even if schema fails additional props are removed anyway and with this option they don't. Regarding the fix for 'failing' option - I still don't understand the issue. Maybe it's better to make it a separate PR or at least make a separate test case that demonstrates the issue. |
Closing, it is old. |
What issue does this pull request resolve?
#129 (comment)
What changes did you make?
Only remove additional properties when the schema is valid.