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

Schema JSON validation fails with ESLint 4.2.0 #8908

Closed
jseminck opened this Issue Jul 9, 2017 · 24 comments

Comments

Projects
None yet
@jseminck

jseminck commented Jul 9, 2017

Tell us about your environment

  • ESLint Version: 4.2.0
  • Node Version: 6.9.2
  • npm Version: 3.10.2

What parser (default, Babel-ESLint, etc.) are you using?
default

Please show your full configuration:
N/A

What did you do? Please include the actual source code causing the issue.

It seems that ESLint has upgraded its JSON validator for 4.2.0: #8852

Note: It's only a guess that the above change has caused the problems, but it's not fully confirmed.

This is causing issues for a rule in eslint-plugin-react:. It seems that the following schema validation is causing problems: https://github.com/yannickcr/eslint-plugin-react/blob/master/lib/rules/jsx-curly-spacing.js#L34

The produced error:

can't resolve reference #/definitions/basicConfig from id #
can't resolve reference #/definitions/basicConfigOrBoolean from id #
Warning: [object Object]:
        Configuration for rule "react/jsx-curly-spacing" is invalid:
refVal[2] is not a function Use --force to continue.
 at validateRuleOptions (node_modules\eslint\lib\config\config-validator.js:112:15)
      at Object.keys.forEach.id (node_modules\eslint\lib\config\config-validator.js:152:9)
      at Array.forEach (native)
      at validateRules (node_modules\eslint\lib\config\config-validator.js:151:30)
      at Object.validate (node_modules\eslint\lib\config\config-validator.js:206:5)
      at runRuleForItem (node_modules\eslint\lib\testers\rule-tester.js:331:23)
      at testValidTemplate (node_modules\eslint\lib\testers\rule-tester.js:413:28)
      at Context.RuleTester.it (node_modules\eslint\lib\testers\rule-tester.js:546:25)

The issue can be reproduced by cloning the eslint-plugin-react repo and running npm run test.

What did you expect to happen?

No errors.

What actually happened? Please include the actual, raw output from ESLint.

See above.

@eslintbot eslintbot added the triage label Jul 9, 2017

@gyandeeps

This comment has been minimized.

Member

gyandeeps commented Jul 9, 2017

@gajus @epoberezkin would love u guys feedback on this. thanks

@epoberezkin

This comment has been minimized.

Contributor

epoberezkin commented Jul 9, 2017

It seems related to #8864

Eslint currently does not correctly support references when schema is defined as array of schemas (see docs change in the referenced PR)

I will double check how it was working before, but I believe it was not working correctly anyway. The schema in the rule should be updated to be a schema object (to be able to use references).

@epoberezkin

This comment has been minimized.

Contributor

epoberezkin commented Jul 9, 2017

@jseminck are you passing some options to the rule or is it just enabled?

@jseminck

This comment has been minimized.

jseminck commented Jul 9, 2017

I am running the eslint-plugin-react test suite which covers many scenarios.

I did a bit of digging and saw that this doesn't crash:

  valid: [{
    code: [
      '<App foo={42} {...bar} baz={ { 4: 2 } }>',
      '{foo} { { bar: baz } }',
      '</App>'
    ].join('\n'),
    options: [{
      when: 'never',
      spacing: {objectLiterals: 'always'},
      attributes: true
    }]

But this does;

  valid: [{
    code: [
      '<App foo={42} {...bar} baz={ { 4: 2 } }>',
      '{foo} { { bar: baz } }',
      '</App>'
    ].join('\n'),
    options: [{
      when: 'never',
      spacing: {objectLiterals: 'always'},
      attributes: true
      children: {when: 'never'} // <-- This was added
    }]

Edit: But the following warning is always displayed. It's only displayed once even for all of the test cases, so I assume it's being logged when the rule is evaluated:

can't resolve reference #/definitions/basicConfig from id #
can't resolve reference #/definitions/basicConfigOrBoolean from id #
@mysticatea

This comment has been minimized.

Member

mysticatea commented Jul 9, 2017

As far as I know, we cannot use $ref if the schema is an array.
The invalid $refs have been ignored before, but it's errored since #8852.
I think it was a breaking change but released with a minor version for some reason.

@epoberezkin

This comment has been minimized.

Contributor

epoberezkin commented Jul 9, 2017

@jseminck here is the example showing that with is-my-json-valid it was never working correctly: https://runkit.com/esp/596268082d308c00122b7d86. Both options should fail validation ("whatever" is not an allowed value) yet they pass. It seems that when is-my-json-valid cannot resolve $ref it quietly passes validation.

This is the same example with Ajv: https://runkit.com/esp/59626adc6f924f00124f5fed
Ajv is configured to log warnings in case of missing references and pass validation, which it does in the first case but fails to do in the second case. I am going to make a fix so it correctly ignores missing $ref in the schema as was agreed: #8852 (comment)

But in any case, the schema in eslint-plugin-react should be either changed to use the full JSON Schema format or to be refactored to not use $ref, otherwise it will not work correctly (as the examples show).

@epoberezkin

This comment has been minimized.

Contributor

epoberezkin commented Jul 9, 2017

I think it was a breaking change but released with a minor version for some reason.

@mysticatea Ajv is supposed to ignore missing $ref, will release a fix.

@jseminck

This comment has been minimized.

jseminck commented Jul 9, 2017

I'll make a PR to eslint-plugin-react as well to use full json schema object instead of an array. Thanks for the quick response and good explanations. TIL alot 😄

The only thing I still fail to understand completely is why the $ref doesn't work for the schema when it is an array. As I understood from the PR comments, the schema array is being transformed into:

{
  type: 'array',
  items: [
    obj
  ],
}

But why don't the definitions inside obj not work with this schema?

@epoberezkin

This comment has been minimized.

Contributor

epoberezkin commented Jul 10, 2017

The only thing I still fail to understand completely is why the $ref doesn't work for the schema when it is an array. As I understood from the PR comments, the schema array is being transformed into:

@jseminck That is correct, as the result of this transformation all the JSON pointers to the same sub-schemas change. So instead of #/definitions/basicConfig you would have to use #/items/0/definitions/basicConfig. When you refactor to object, definitions will be on the top level.

@jseminck

This comment has been minimized.

jseminck commented Jul 10, 2017

Aha. That makes sense! Thanks for explanation. 👍

@epoberezkin

This comment has been minimized.

Contributor

epoberezkin commented Jul 10, 2017

Fixed in Ajv 5.2.2: https://runkit.com/esp/59626adc6f924f00124f5fed

No changes in eslint are required.

@ljharb

This comment has been minimized.

Contributor

ljharb commented Jul 10, 2017

@epoberezkin so with v5.2.2 of ajv installed, should I still be seeing "can't resolve reference" warnings? Specifically, I'm trying npm test in eslint-plugin-react's master branch, with npm install ajv@5.2.2 vs npm install ajv@5.2.1 (the top-level ajv is the only one in the graph)

@epoberezkin

This comment has been minimized.

Contributor

epoberezkin commented Jul 10, 2017

Yes, it's a warning that the reference in the schema is invalid. It passes validation though, same as it did before.

@epoberezkin

This comment has been minimized.

Contributor

epoberezkin commented Jul 10, 2017

The schema in eslint-plugin-react still needs to be fixed.

@not-an-aardvark

This comment has been minimized.

Member

not-an-aardvark commented Jul 10, 2017

To clarify, does this warning appear for all users of eslint-plugin-react, or just when running the tests? If eslint-plugin-react was unusable with the latest version of eslint for a few days, I would have expected this issue to have have more 👍s/duplicates.

@jseminck

This comment has been minimized.

jseminck commented Jul 11, 2017

It should have been unusable if the configuration was set in such a way that it crashes during schema validation (triggering the bug that has now been fixed in ajv).

I'm guessing most users did not have such a "complicated configuration"? Although the issue on eslint-plugin-react has a few references to other Github repo issues.

Also the bug would only occur when upgrading eslint. So anyone with locked dependency versions and a "broken configuration" wouldn't have had the issue yet, right?

I created PR for eslint-plugin-react to change the schema to full JSON object, just like you guys did with the comma-dangle rule for eslint.

@jseminck

This comment has been minimized.

jseminck commented Jul 12, 2017

I think this can be closed now, right?

@gyandeeps

This comment has been minimized.

Member

gyandeeps commented Jul 12, 2017

Cool thanks guys for your help. Special thanks to @epoberezkin for his help.

@jeffrey-l-turner

This comment has been minimized.

jeffrey-l-turner commented Jul 24, 2017

@jseminck Was a PR created for eslint-plugin-react to address this issue? I can't seem to find it.

pabigot added a commit to pabigot/eslint-plugin-pabigot that referenced this issue Jul 24, 2017

@ljharb

This comment has been minimized.

Contributor

ljharb commented Jul 24, 2017

@s-ilya s-ilya referenced this issue Aug 1, 2017

Merged

Upgrade deps #3

@sslotsky

This comment has been minimized.

sslotsky commented Aug 3, 2017

So should I no longer be seeing this issue given the package versions listed below?

$ eslint ./app/** ./spec/**/* --ext=js,jsx --fix
can't resolve reference #/definitions/basicConfig from id #
can't resolve reference #/definitions/basicConfigOrBoolean from id #
can't resolve reference #/definitions/basicConfigOrBoolean from id #
Done in 1.98s.
sam@sam-Alienware-13-R2:~/react-redux-boiler$ npm ls | grep eslint-plugin-react
├── eslint-plugin-react@7.1.0
npm ERR! extraneous: ajv@5.2.2 /home/sam/react-redux-boiler/node_modules/table/node_modules/ajv-keywords/node_modules/ajv
sam@sam-Alienware-13-R2:~/react-redux-boiler$ npm ls | grep eslint
├── babel-eslint@7.2.3
├─┬ eslint@4.3.0
│ ├─┬ eslint-scope@3.7.1
├─┬ eslint-config-airbnb@15.1.0
│ └─┬ eslint-config-airbnb-base@11.3.1
│   └── eslint-restricted-globals@0.1.1
├─┬ eslint-import-resolver-babel-module@3.0.0
├─┬ eslint-plugin-import@2.7.0
│ ├── eslint-import-resolver-node@0.3.1
│ ├─┬ eslint-module-utils@2.1.1
├─┬ eslint-plugin-jsx-a11y@5.1.1
├── eslint-plugin-react@7.1.0
npm ERR! extraneous: ajv@5.2.2 /home/sam/react-redux-boiler/node_modules/table/node_modules/ajv-keywords/node_modules/ajv
@epoberezkin

This comment has been minimized.

Contributor

epoberezkin commented Aug 3, 2017

You should be seeing the message, until the issue is resolve in the plugin(s). The fix prevented schema validation failing, but the warning should be still logged as those unresolved references are ignored.

@lukeapage

This comment has been minimized.

Contributor

lukeapage commented Aug 3, 2017

yannickcr/eslint-plugin-react#1292 Is merged but we are awaiting a release

@AmyAmy

This comment has been minimized.

AmyAmy commented Aug 4, 2017

FYI, it seems that eslint-plugin-sort-class-members is affected too.

bryanrsmith/eslint-plugin-sort-class-members#41

@eslint eslint bot locked and limited conversation to collaborators Feb 6, 2018

@eslint eslint bot added the archived due to age label Feb 6, 2018

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