Always guard against undefined type in fieldAvailableOnType #231

vitorbal · 2019-06-22T02:01:14Z

Previously, only the first half of the check inside fieldAvailableOnType guarded against type being undefined. This meant that the function would throw an error.

To fix this, and o prevent it from accidentally happening again if we ever add another check to this logic, I moved the check for !type up into it's own guard, with an early return.

This change makes the required-fields rule more resilient (which uses this fieldAvailableOnType function internally).

Without this change, I see errors when trying to run eslint-plugin-graphql against my project:

Cannot read property 'ofType' of undefined
TypeError: Cannot read property 'ofType' of undefined
    at fieldAvailableOnType (/Code/vitorbal/node_modules/eslint-plugin-graphql/lib/customGraphQLValidationRules.js:33:62)
    at /Code/vitorbal/node_modules/eslint-plugin-graphql/lib/customGraphQLValidationRules.js:49:13
    at Array.forEach (<anonymous>)
    at Object.FragmentDefinition (/Code/vitorbal/node_modules/eslint-plugin-graphql/lib/customGraphQLValidationRules.js:42:22)
    at Object.enter (/Code/vitorbal/node_modules/graphql/language/visitor.js:332:29)
    at Object.enter (/Code/vitorbal/node_modules/graphql/language/visitor.js:383:25)
    at visit (/Code/vitorbal/node_modules/graphql/language/visitor.js:250:26)
    at validate (/Code/vitorbal/node_modules/graphql/validation/validate.js:63:22)
    at handleTemplateTag (/Code/vitorbal/node_modules/eslint-plugin-graphql/lib/createRule.js:133:57)
    at TaggedTemplateExpression (/Code/vitorbal/node_modules/eslint-plugin-graphql/lib/createRule.js:226:20)

TODO:

Make sure all of the significant new logic is covered by tests
Rebase your changes on master so that they can be merged easily
Make sure all tests pass
Update CHANGELOG.md with your change
If this was a change that affects the external API, update the README

Previously, only the first half of the check inside `fieldAvailableOnType` guarded against `type` being `undefined`. To prevent this from accidentally happening again if we ever have to add another check to this logic, I moved the check for `!type` up into it's own guard, with an early return.

vitorbal

Annotations 👇

vitorbal · 2019-06-22T03:02:31Z

package.json

@@ -5,7 +5,7 @@
  "author": "Sashko Stubailo",
  "main": "lib/index.js",
  "scripts": {
-    "test": "tav --ci && mocha test/index.js",
+    "test": "tav",


Tests have been failing in CI for a while, because in CI tests rung against all versions of graphql that satisfy the semver ranges defined in .tav.yml:

eslint-plugin-graphql/.tav.yml

Line 3 in ed2e619

versions: ^0.12.0 || ^0.13.0 || ^14.0.0

Amongst those was v14.3.1, which shows a slightly different error message for the ProvidedRequiredArguments validator. You can see the error here: https://travis-ci.org/apollographql/eslint-plugin-graphql/jobs/548949602

If you look at recent PRs, all of them are failing with the same error. What made this confusing to me was that tav was only being run on CI, and not locally. So locally tests were passing. For that reason, I thought it would be best if tav also runs locally now –– hence why the --ci flag is removed here. If that's not okay, I can revert this part of the PR.

Anyways, the fix for the actual error can be seen below in test/validationRules/built-in.js

vitorbal · 2019-06-22T03:03:24Z

test/validationRules/built-in.js

@@ -162,7 +162,7 @@ const validatorCases = {
    errors: [
      {
        message:
-          'Field "sum" argument "b" of type "Int!" is required but not provided.',
+          /Field "sum" argument "b" of type "Int!" is required/,


Make these regexes so they pass in all versions of graphql that we test (the error messages have slight variations depending on the version).

vitorbal · 2019-06-22T03:04:04Z

test/validationRules/required-fields.js

@@ -11,6 +11,7 @@ const requiredFieldsTestCases = {
    "const x = gql`query { greetings { id, hello, foo } }`",
    "const x = gql`query { greetings { id ... on Greetings { hello } } }`",
    "const x = gql`fragment Name on Greetings { id hello }`",
+    "const x = gql`fragment Foo on FooBar { id, hello, foo }`",


This test case would throw an error if not for the fix in src/customGraphQLValidationRules.js above

vitorbal · 2019-06-22T03:10:11Z

@jnwng @abernix 👋 sorry for the ping, but it looks like you're the active maintainers of this repo. Any chance you could take a look at this when you have a spare moment? Thanks in advance 🙂

vitorbal · 2019-07-03T08:52:47Z

@jnwng @abernix friendly ping, any chance you could take a look at this one? Should be an easy bug fix. Thanks in advance!

vitorbal · 2019-09-18T12:02:21Z

Hey @jnwng it's been almost 3 months, any chance you'd have time to take a look at this? It's blocking me from using eslint-plugin-graphql at work.
Thanks in advance!

jnwng · 2019-09-18T12:34:39Z

lgtm. please rebase onto master as we’ve resolved the tests running in CI and can get this released

…

On Wed, Sep 18 2019 at 8:02 AM, < ***@***.*** > wrote: Hey @jnwng ( https://github.com/jnwng ) it's been almost 3 months, any chance you'd have time to take a look at this? It's blocking me from using eslint-plugin-graphql at work. Thanks in advance! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub ( #231?email_source=notifications&email_token=AAIRCRVKXXU6AODT3CZLG6LQKIKE5A5CNFSM4H2VW5MKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD672HWA#issuecomment-532653016 ) , or mute the thread ( https://github.com/notifications/unsubscribe-auth/AAIRCRWFXMGEK4T34FMESIDQKIKE5ANCNFSM4H2VW5MA ).

vitorbal · 2019-09-18T22:55:10Z

@jnwng thanks so much for the reply! 🙏 I know y'all must be pretty busy, so I appreciate it!

I had an issue with the original branch this PR was based on (says "unknown repository" now 🤔), so I recreated the PR here.

I'll close this one and tag you over there!

vitorbal added 2 commits June 22, 2019 04:27

Fix unrelated test that has been failing for a while

5412f50

vitorbal commented Jun 22, 2019

View reviewed changes

vitorbal mentioned this pull request Aug 7, 2019

Always guard against undefined type in fieldAvailableOnType zapier/eslint-plugin-graphql#1

Merged

5 tasks

vitorbal closed this Sep 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Always guard against undefined type in fieldAvailableOnType #231

Always guard against undefined type in fieldAvailableOnType #231

vitorbal commented Jun 22, 2019 •

edited

Loading

vitorbal left a comment

vitorbal Jun 22, 2019

vitorbal Jun 22, 2019

vitorbal Jun 22, 2019

vitorbal commented Jun 22, 2019

vitorbal commented Jul 3, 2019

vitorbal commented Sep 18, 2019

jnwng commented Sep 18, 2019 via email

vitorbal commented Sep 18, 2019

Always guard against undefined type in fieldAvailableOnType #231

Always guard against undefined type in fieldAvailableOnType #231

Conversation

vitorbal commented Jun 22, 2019 • edited Loading

vitorbal left a comment

Choose a reason for hiding this comment

vitorbal Jun 22, 2019

Choose a reason for hiding this comment

vitorbal Jun 22, 2019

Choose a reason for hiding this comment

vitorbal Jun 22, 2019

Choose a reason for hiding this comment

vitorbal commented Jun 22, 2019

vitorbal commented Jul 3, 2019

vitorbal commented Sep 18, 2019

jnwng commented Sep 18, 2019 via email

vitorbal commented Sep 18, 2019

vitorbal commented Jun 22, 2019 •

edited

Loading