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.

Sign up for GitHub

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

Always guard against undefined type in fieldAvailableOnType #231

Closed
wants to merge 2 commits into from
Closed

Always guard against undefined type in fieldAvailableOnType #231

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9576912
Always guard against undefined type in fieldAvailableOnType
vitorbal Jun 22, 2019
5412f50
Fix unrelated test that has been failing for a while
vitorbal Jun 22, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

### vNEXT

- Fix an issue that caused `graphql/required-fields` to throw on non-existent field references. [PR #231](https://github.com/apollographql/eslint-plugin-graphql/pull/231) by [Vitor Balocco](https://github.com/vitorbal)

### v3.0.3

- chore: Update dependency `graphql-tools` to `v4.0.4`. [PR #210](https://github.com/apollographql/eslint-plugin-graphql/pull/210)
Expand Down
57 changes: 15 additions & 42 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
"author": "Sashko Stubailo",
"main": "lib/index.js",
"scripts": {
"test": "tav --ci && mocha test/index.js",
"test": "tav",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"prepublish": "babel ./src --ignore test --out-dir ./lib",
"pretest": "node test/updateSchemaJson.js",
"tav": "tav",
Expand Down
6 changes: 5 additions & 1 deletion src/customGraphQLValidationRules.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,12 @@ function getFieldWasRequestedOnNode(node, field) {
}

function fieldAvailableOnType(type, field) {
if (!type) {
return false;
}

return (
(type && type._fields && type._fields[field]) ||
(type._fields && type._fields[field]) ||
(type.ofType && fieldAvailableOnType(type.ofType, field))
);
}
Expand Down
4 changes: 2 additions & 2 deletions test/validationRules/built-in.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ const validatorCases = {
"const x = gql`fragment FilmFragment on Floof { title } { allFilms { films { ...FilmFragment } } }`",
errors: [
{
message: 'Unknown type "Floof".',
message: /Unknown type "Floof"/,
type: "TaggedTemplateExpression"
}
]
Expand Down Expand Up @@ -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/,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

type: "TaggedTemplateExpression"
}
]
Expand Down
1 change: 1 addition & 0 deletions test/validationRules/required-fields.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 }`",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"const x = gql`fragment Id on Node { id ... on NodeA { fieldA } }`",
"const x = gql`query { nodes { id ... on NodeA { fieldA } } }`",
],
Expand Down