Breaking: Ajv to v8.5.0, added ajv-draft-04 (fixes #13888)

[epoberezkin]

Prerequisites checklist

• I have read the contributing guidelines.

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[x] Other, please explain: Update Ajv version to v7

What changes did you make? (Give an overview)

• Update Ajv to version v7.0.0-rc.0
• Update schemas:
  • remove ignored additionalItems keywords (it is possible though that the intention was to have only one item allowed in these schemas, in which case schemas have to be changed from items: {...} to items: [{...}], additionalItems: false
  • add missing type keywords (in the second commit).
• Remove strictDefaults option (strict mode is on by default now) and other deprecated option
• Use JSON Schema draft-07 (although it was not explicitly specified that schemas are draft-04)
• Fix failing tests (all updates are because of changes in how dataPath is shown)

Is there anything you'd like reviewers to focus on?

• Review updated schemas whether they are correct.
• Please note that Ajv v7 no longer supports JSON Schema draft-04, but all schemas I have seen are draft-07 compatible, so no changes were necessary. As I wrote in Upgrade to Ajv version 8 #13888, it may have effect on plugins maybe? It is probably still worth trying to release as a minor version upgrade (once Ajv v7 becomes the main version in 1-2 weeks) and monitor if there are some issues while resolving them.

cc @nzakas

[eslint-deprecated]

Hi @epoberezkin!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

• The commit message tag must be one of the following:

  The Tag is one of the following:

  • Fix - for a bug fix.
  • Update - either for a backwards-compatible enhancement or for a rule change that adds reported problems.
  • New - implements a new feature.
  • Breaking - for a backwards-incompatible enhancement or feature.
  • Docs - changes to documentation only.
  • Build - changes to build process only.
  • Upgrade - for a dependency upgrade.
  • Chore - for anything that isn't user-facing (for example, refactoring, adding tests, etc.).

  You can use the labels of the issue you are working on to determine the best tag.

• There should be a space following the initial tag and colon, for example 'New: Message'.

Read more about contributing to ESLint here

[eslint-deprecated]

Hi @epoberezkin!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

• The commit message tag must be one of the following:

  The Tag is one of the following:

  • Fix - for a bug fix.
  • Update - either for a backwards-compatible enhancement or for a rule change that adds reported problems.
  • New - implements a new feature.
  • Breaking - for a backwards-incompatible enhancement or feature.
  • Docs - changes to documentation only.
  • Build - changes to build process only.
  • Upgrade - for a dependency upgrade.
  • Chore - for anything that isn't user-facing (for example, refactoring, adding tests, etc.).

  You can use the labels of the issue you are working on to determine the best tag.

• There should be a space following the initial tag and colon, for example 'New: Message'.

Read more about contributing to ESLint here

[eslint-deprecated]

Hi @epoberezkin!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

• The commit message tag must be one of the following:

  The Tag is one of the following:

  • Fix - for a bug fix.
  • Update - either for a backwards-compatible enhancement or for a rule change that adds reported problems.
  • New - implements a new feature.
  • Breaking - for a backwards-incompatible enhancement or feature.
  • Docs - changes to documentation only.
  • Build - changes to build process only.
  • Upgrade - for a dependency upgrade.
  • Chore - for anything that isn't user-facing (for example, refactoring, adding tests, etc.).

  You can use the labels of the issue you are working on to determine the best tag.

• There should be a space following the initial tag and colon, for example 'New: Message'.

Read more about contributing to ESLint here

[eslint-deprecated]

Hi @epoberezkin!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

• The issue reference must be formatted as follows:

  If the pull request addresses an issue, then the issue number should be mentioned at the end. If the commit doesn't completely fix the issue, then use (refs #1234) instead of (fixes #1234).

  Here are some good commit message summary examples:

  Build: Update Travis to only test Node 0.10 (refs #734)
  Fix: Semi rule incorrectly flagging extra semicolon (fixes #840)
  Upgrade: Esprima to 1.2, switch to using comment attachment (fixes #730)
  

Read more about contributing to ESLint here

[eslint-deprecated]

Hi @epoberezkin!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

• The issue reference must be formatted as follows:

  If the pull request addresses an issue, then the issue number should be mentioned at the end. If the commit doesn't completely fix the issue, then use (refs #1234) instead of (fixes #1234).

  Here are some good commit message summary examples:

  Build: Update Travis to only test Node 0.10 (refs #734)
  Fix: Semi rule incorrectly flagging extra semicolon (fixes #840)
  Upgrade: Esprima to 1.2, switch to using comment attachment (fixes #730)
  

Read more about contributing to ESLint here

[eslint-deprecated]

Hi @epoberezkin!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

• The issue reference must be formatted as follows:

  If the pull request addresses an issue, then the issue number should be mentioned at the end. If the commit doesn't completely fix the issue, then use (refs #1234) instead of (fixes #1234).

  Here are some good commit message summary examples:

  Build: Update Travis to only test Node 0.10 (refs #734)
  Fix: Semi rule incorrectly flagging extra semicolon (fixes #840)
  Upgrade: Esprima to 1.2, switch to using comment attachment (fixes #730)
  

Read more about contributing to ESLint here

[mdjermanovic]

@epoberezkin thanks for the PR! I marked this PR as a draft because of the rc version, and as "breaking" until we figure out whether or not it's a breaking change.

[mdjermanovic]

• add missing type keywords (in the second commit)

How was this working at all, does Ajv in non-strict mode assume it's an "object"/"array" type if there is "properties"/"items"?

• strict mode is on by default now

Should we then turn it off explicitly, to retain the previous behavior (and thus maybe avoid breaking plugins)?

• Please note that Ajv v7 no longer supports JSON Schema draft-04, but all schemas I have seen are draft-07 compatible, so no changes were necessary. As I wrote in Upgrade to Ajv version 8 #13888, it may have effect on plugins maybe?

Is there an overview of the differences between versions, so we could maybe try to estimate the impact on plugins? In particular, what was valid by draft-04, but would be invalid or ignored or maybe have different semantics with draft-07.

[epoberezkin]

add missing type keywords (in the second commit)

How was this working at all, does Ajv in non-strict mode assume it's an "object"/"array" type if there is "properties"/"items"?

No, it allows ANY data type, "property"/"items" only applies any restriction IF it is "object"/"array" - this is how JSON Schema specifies it. So if the data were an object/array it would validate it as expected, otherwise it would just allow it (incorrectly). It is a common mistake, that's why I made strict mode default.

strict mode is on by default now

Should we then turn it off explicitly, to retain the previous behavior (and thus maybe avoid breaking plugins)?

We could, but:

• there may be other schema mistakes that would remain unnoticed
• the main thing is that disabling strict mode won't guarantee that this is not a breaking change, because if plugins rely on eslint code validating their config schemas (am I correct - is this how it is?), they may use some draft-04 specific things that won't be supported any more:
  • "id" (rather than $id), although it is probably not very likely - most schemas don't seem to rely on schema "id"
  • old-style "exclusiveMaximum"/"exclusiveMinimum" keywords

I believe we should identify top-X (say, 5) most used plugins and update them too (if I am correct that they depend on eslint validation code), and release it as a minor version change - some small share of users will be affected, they'll have to lock the version, we can then fix any reported issues.

To make any such change easier in the future it might be good to split validation code to the separate package, something like schema-utils in webpack, in which case eslint and plugins would depend on different versions of validation utils, and can be upgraded independently.

Or we could take a cautious route and release it as a major version, but then users will be carrying 2 ajv versions for longer (and they would still not be able to migrate eslint until plugins migrate). I think npm have finally fixed that old issue when two different sub-dependency versions were breaking installation, so it's not such a big deal too.

Your call.

[mdjermanovic]

No, it allows ANY data type, "property"/"items" only applies any restriction IF it is "object"/"array" - this is how JSON Schema specifies it. So if the data were an object/array it would validate it as expected, otherwise it would just allow it (incorrectly). It is a common mistake, that's why I made strict mode default.

Thanks! I think I understand now. "type" in schema validates type of the instance. "properties" validate properties of the instance if the instance is an object (regardless of "type" in the schema), otherwise do nothing.

• if plugins rely on eslint code validating their config schemas (am I correct - is this how it is?)

Plugin rules usually define schema property and thus rely on eslint validating user's configuraton for the rule. It's an optional property - rule doesn't have to define schema and it will be still getting user's configuration (without any validations by eslint). A plugin could implement its own validation, though I don't think there are any plugins doing this.

The schema itself isn't validated "in production". Only RuleTester (a built-in tool for the development of rules) validates rule schemas.

[epoberezkin]

Plugin rules usually define schema property and thus rely on eslint validating user's configuraton for the rule. It's an optional property - rule doesn't have to define schema and it will be still getting user's configuration (without any validations by eslint). A plugin could implement its own validation, though I don't think there are any plugins doing this.

That’s what I thought.

So it means that the plugins that have “id” (it’s now $id) or “exclusiveMaximum/Minimum” (it was boolean in draft-04, number now) would throw exception. I think both should be quite rare.

The possible solution could be to add some code that would migrate plug-in schemas on the fly by replacing these two things (or we could just plug in json-schema-migrate) - what do you think?

Feels wrong though, probably better just upgrade top X plugins.

"type" in schema validates type of the instance. "properties" validate properties of the instance if the instance is an object (regardless of "type" in the schema), otherwise do nothing.

Exactly. So if you want to do minor version upgrade, we could avoid adding “type”, so that users validation don’t fail even if they have incorrect config.

[mdjermanovic]

strict mode is on by default now

Should we then turn it off explicitly, to retain the previous behavior (and thus maybe avoid breaking plugins)?

We could, but:

• there may be other schema mistakes that would remain unnoticed

There are two scenarios where schemas can be validated:

• runtime (end users) - indirectly via compile().
• development (of ESLint core and plugins) - RuleTester runs validateSchema() and compile() with bit stricter options.

For end users, we already have validateSchema: false (which disables the most severe errors?). I believe the intention was always to not validate schemas in runtime. Enabling additional validations now will break end users that have valid configurations, so they'll have to wait for plugins to fix rule schemas and release new versions, although those plugins already work perfectly well for them.

For example, linting of eslint/eslint repo itself as an end-user would be broken until eslint-plugin-eslint-plugin and eslint-plugin-node fix schemas in some rules (I tried npm run lint on this branch, but also applied your changes in eslint/eslintrc; we are extracting some functionality to that package, so we're currently using Ajv from two places).

I think we should turn off all validations of schemas in runtime, even for a major ESLint version, because it looks like it could cause a big breakage without clear benefits.

As for the development, it certainly makes sense to consider enabling additional validations in RuleTester in a major version. This also needs some discussion, because fixing schemas can also break end users of plugins, though only those with possibly invalid configurations.

(I'm aware that the incompatibility of schema versions is probably a bigger problem, just wanted to figure out how to handle validation changes first)

[mdjermanovic]

Also, it seems that compile() is more strict in Ajv 7.

For example, an invalid type value used to be ignored in Ajv 6, but it throws in Ajv 7:

const Ajv = require("ajv").default;

const ajv = new Ajv({ 
  validateSchema: false,
  strict: false,
  strictTypes: false,
});

const schema = {
  type: "arrayy", // typo
};

ajv.compile(schema); // Error: type must be JSONType or JSONType[]: arrayy

[epoberezkin]

Yes, in general the approach was to be stricter by default (with many restrictions flipping from opt-in to opt-out), and treating possible typos as errors - it's been the source of many user side bugs reported as Ajv bugs. The logic is that JSON Schema is a language with constrained vocabulary and for the absolute majority of users the expected behaviour is to fail on unknown words (rather than ignore them as the spec suggests) - same as programming languages behave.

This blog post covers the scope of changes and the motivation: https://www.poberezkin.com/posts/2020-11-14-ajv-time-to-migrate-to-version-7.html

[nzakas]

This seem complicated enough that an RFC would seem to be in order to work out the details. Thoughts?

[mdjermanovic]

I think npm have finally fixed that old issue when two different sub-dependency versions were breaking installation, so it's not such a big deal too.

There was an issue with ajv-keywords not getting the correct version of ajv as its peer dependency: #10022 (comment).

I'm not sure if this has been fixed in npm < 7, but there's probably nothing we can do about it. Hopefully, it won't be happening since table doesn't have ajv-keywords as a dependency as of v5.

[epoberezkin]

I’ve made the PR for table to upgrade to Ajv v7 - it doesn’t have to be a major version change there.

[epoberezkin]

Happy New year all!

Ajv v7 is the main version since mid-December; no complaints about version conflicts so far. There's been a few minor bugs reported and fixed, mostly around new standalone code generation, - all good otherwise.

Table was just released yesterday with ajv v7 in the same major version - thanks @gajus.

Not sure how it is best to move it forward - how would you decide between the options here: #13911 (comment) ?

There will be another major version release in the next 3-4 months, but it should be "mostly" backwards compatible.

[mdjermanovic]

This is also a breaking change for end users. The new version of Ajv#compile() throws errors on some mistakes in the provided schema (e.g. #13911 (comment)) that used to be ignored by Ajv 6.

We could add try-catch around ajv.compile() to mitigate this.

[mdjermanovic]

lib/shared/ajv.js is currently used by RuleTester only. Once we figure out what changes should be made, the same changes should be implemented in @eslint/eslintrc (that's where Ajv is used to validate user configurations).

[mdjermanovic]

I have concerns about "ajv": "^8.5.0" being declared as a peer dependency of ajv-draft-04, because ajv-draft-04 requires Ajv.

We can declare "ajv": "^8.5.0" as a dependency of ESLint, but will that guarantee that require("ajv") from ajv-draft-04 will be getting the right version of Ajv when ESLint is installed as a dependency/dev-dependency of a project?

For example, I made a test project with the following dependencies (it's using ESLint from this branch):

"dependencies": {
    "ajv": "^6.10.0",
    "eslint": "github:epoberezkin/eslint#epoberezkin/ajv-v7"
  }

Running ESLint failes, with this error:

Error: Cannot find module 'ajv/dist/core'
Require stack:
- /home/runner/work/use-ajv-draft-04/use-ajv-draft-04/node_modules/ajv-draft-04/dist/index.js
- /home/runner/work/use-ajv-draft-04/use-ajv-draft-04/node_modules/eslint/lib/shared/ajv.js
- /home/runner/work/use-ajv-draft-04/use-ajv-draft-04/node_modules/eslint/lib/shared/config-validator.js
- /home/runner/work/use-ajv-draft-04/use-ajv-draft-04/node_modules/eslint/lib/rule-tester/rule-tester.js
- /home/runner/work/use-ajv-draft-04/use-ajv-draft-04/node_modules/eslint/lib/rule-tester/index.js
- /home/runner/work/use-ajv-draft-04/use-ajv-draft-04/node_modules/eslint/lib/api.js
- /home/runner/work/use-ajv-draft-04/use-ajv-draft-04/index.js

because npm install creates the following structure:

node_modules
├── [email protected]    
├── ajv-draft-04
└── eslint
    └── node_modules
        └── [email protected]

so require("ajv/dist/core") from ajv-draft-04 gets resolved to Ajv v6.

Since ajv-draft-04 directly require-s ajv, shouldn't ajv be declared as dependency of ajv-draft-04? (instead of peer dependency)

[aladdin-add]

just moved it back to v8.0.0 project, as it seems a breaking change. :)

[epoberezkin]

Since ajv-draft-04 directly require-s ajv, shouldn't ajv be declared as dependency of ajv-draft-04? (instead of peer dependency)

I will see how to change it… Possibly in this case it should be a direct dependency indeed.

[mdjermanovic]

This is also a breaking change for end users. The new version of Ajv#compile() throws errors on some mistakes in the provided schema (e.g. #13911 (comment)) that used to be ignored by Ajv 6.

We could add try-catch around ajv.compile() to mitigate this.

Actually, try/catch may not be a good idea. It would be okay to just skip the validation, but it will also skip setting default values, so the rule might silently change behavior.

[epoberezkin]

The new version of Ajv#compile() throws errors on some mistakes in the provided schema (e.g. #13911 (comment)) that used to be ignored by Ajv 6.

@mdjermanovic The example in the comment with type: "arrayy" actually throws exception in v6 as well, unless you disable meta-schema validation (which is enabled in eslent).

The only difference I can think of is the change from strictDefaults to strictKeywords.

If it makes it simpler (e.g. could be released as a non-breaking change), I could add this option (strictDefaults) back? Let me know.

Looking at ajv-draft-04 anyway.

[mdjermanovic]

@mdjermanovic The example in the comment with type: "arrayy" actually throws exception in v6 as well, unless you disable meta-schema validation (which is enabled in eslent).

It's enabled in RuleTester, which is a tool for developers to test the rules. Rules with type: "arrayy" don't throw exceptions when they are actually used to lint code. We use one setting for user config validations, and another (more strict) in RuleTester (+ ajv.validateSchema() in RuleTester).

That said, we can assume that most plugins use RuleTester to test the rules, so the possibility that rules with type: "arrayy" were actually published should be very small.

[mdjermanovic]

just moved it back to v8.0.0 project, as it seems a breaking change. :)

We're adding only issues to the project, to avoid duplicates. I removed this PR from the project now (the related issue #13888 is already there).

[mdjermanovic]

Suggested change

	strict: "log",
	strictTuples: false,
	strict: false,

If we set strict: false, then we don't need to explicitly set strictTuples: false?

[epoberezkin]

Correct

[mdjermanovic]

Suggested change

	const field = error.instancePath[0] === "." ? error.instancePath.slice(1) : error.instancePath;
	const field = error.instancePath[0] === "/" ? error.instancePath.slice(1) : error.instancePath;

I guess the error.dataPath[0] === "." condition doesn't make sense with the new path format, and that we would want to remove root /?

[mdjermanovic]

Since ajv-draft-04 directly require-s ajv, shouldn't ajv be declared as dependency of ajv-draft-04? (instead of peer dependency)

I will see how to change it… Possibly in this case it should be a direct dependency indeed.

@epoberezkin any news on this? I think that's the only major issue. Aside from that, this looks good assuming we'll set strict: false.

[mdjermanovic]

I noticed that npm test is much slower on this branch, and located the cause in RuleTester.

ajv.compile takes ~ 3 minutes on this branch, vs only ~ 2 seconds on master.

I thought Ajv 8 might be slower due to the new validations, and made some benchmarks but they showed that Ajv 8 is actually much faster. edit: bad benchmark, didn't account for caching. It seems slower, but not that much.

The problem is probably here:

eslint/lib/rule-tester/rule-tester.js

Line 557 in 278813a

const schema = getRuleOptionsSchema(rule);

We're validating the rule's schema for each test case (that seems unnecessary, but may take some time to analyze and fix). If the rule uses ESLint's shorthand array schema, then getRuleOptionsSchema creates a new instance for each test case.

Ajv 6 used to cache compiled schemas by content, but Ajv 8 caches by reference?

If that's the case, we can move const schema = getRuleOptionsSchema(rule); to the upper scope. I tried that, and it reduces npm test time by almost half.

[mdjermanovic]

@epoberezkin I'm closing this PR as described in #13888 (comment), Thanks for the PR, and thanks for all the work on Ajv!

[epoberezkin]

@mdjermanovic sorry, missed these comments...

Ajv 6 used to cache compiled schemas by content, but Ajv 8 caches by reference?

That is correct. There is a reasonable argument for that, but it turns out to be a problem for quite a few users - e.g. ajv-validator/ajv#1413

It indeed highlights incorrect cases when the schema is compiled multiple times, so I don't think it should be reversed...

In today's TSC meeting, we have decided to stay on Ajv 6.

Are there notes? It would be great to formulate the changes needed that would make transition to the next major version easier - v6 is stable indeed, but there will be other dependencies going out of date, so at some point the migration would have to happen...

[nzakas]

See https://github.com/eslint/tsc-meetings for notes and transcripts of all TSC meetings.

[epoberezkin]

@nzakas - thank you - I will update the problem with peer dependency, and let's review for eslint v9. By then the main reason for migration could be security warnings in v6 dev dependencies (that of course can be addressed by additional v6 release:).

Users do migrate to v8 - I estimate that about 20% of users migrated (based on the download count of ajv-formats that's only needed for v8, accounting for the fact that some know dependencies do not need ajv-formats).