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

Allow easy detection of ignored default keywords in schemas #957

Closed
not-an-aardvark opened this issue Feb 22, 2019 · 9 comments
Closed

Allow easy detection of ignored default keywords in schemas #957

not-an-aardvark opened this issue Feb 22, 2019 · 9 comments

Comments

@not-an-aardvark
Copy link
Contributor

What version of Ajv you are you using?

6.9.1

What problem do you want to solve?

When using ajv with useDefaults: true, the default keyword is ignored in some cases, as documented in the readme. If someone is aware of the existence of the default keyword but isn't familiar with the details of where it can be used, they might inadvertently use it in a place where it's ignored, resulting in a buggy schema. (This recently caused some bugs in ESLint rules -- also see eslint/eslint#11427.)

We'd like to detect this problem so that schema authors can fix the issue. As far as I can tell, there isn't a simple way for an external tool to figure out whether an ignored default keyword exists in a user-provided schema, without reimplementing some of the logic from ajv.

What do you think is the correct solution to problem?

It should be possible to opt into a mode where invalid default keywords result in a compilation or validation error, rather than being ignored. It seems like this could in principle be added to the meta-schema, although it could make the meta-schema substantially larger. Another possibility could be to add an option to the Ajv constructor that causes this case to throw an error.

Will you be able to implement it?

I'm willing to give it a shot if there is agreement that this is worthwhile.

@epoberezkin
Copy link
Member

epoberezkin commented Feb 22, 2019

@not-an-aardvark I understand the problem. The docs list the scenarios where defaults are ignored, but Ajv simply uses them only in properties and items:

With option useDefaults Ajv will assign values from default keyword in the schemas of properties and items (when it is the array of schemas) to the missing properties and items.

So it doesn't really know when defaults are ignored... What would be possible though (I think:) is to make a meta-schema that would fail if a schema has defaults that are ignored, similar to this one: https://github.com/epoberezkin/ajv/blob/master/lib/refs/json-schema-secure.json

Then it would be one line of code to validate against it. I don't think it should be an option though - we can just add to the docs this line and you can also call it for user-provided schemas.

@epoberezkin
Copy link
Member

epoberezkin commented Feb 23, 2019

actually meta-schema is not possible, logging or throwing is possible though - see commit f59c995?w=true

@epoberezkin
Copy link
Member

epoberezkin commented Feb 23, 2019

It has to be an option, and it has to be excluded for meta-schema compilation, as several other options are, because standard meta-schema has default: true in its root that is ignored - it would log all the time without this option.

  • exclude option for meta-schemas (together with other excluded options)
  • option values: true (throw exception), "log" (to log a warning), false
  • tests
  • docs - in options and in "assigning defaults" section
  • types

@not-an-aardvark
Copy link
Contributor Author

Thanks for the feedback. One question -- is there a reason we couldn't just remove the ignored default: true property from the meta-schema, since it ostensibly doesn't have any effect? That would avoid the need to handle the option differently for meta-schema compilation.

@epoberezkin
Copy link
Member

The meta schema is part of JSON schema specification - I would rather we don’t have a different version of it here.
Also ignoring some options in meta-schema is something that already happens for three options - there is an array of such options to add to.

@epoberezkin
Copy link
Member

@not-an-aardvark
Copy link
Contributor Author

Ok thanks, I'll start working on this.

@epoberezkin
Copy link
Member

Cool, thank you, you can continue off my branch with this commit, and then I merge

@epoberezkin
Copy link
Member

@not-an-aardvark it's in 6.10.0. I also added strictKeywords - it is useful to catch typos in keywords that would lead to them being quietly ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants