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

graphql/required-fields rule ignores presence of required field in spread fragment #248

Open
fnune opened this issue Sep 19, 2019 · 7 comments · May be fixed by #300
Open

graphql/required-fields rule ignores presence of required field in spread fragment #248

fnune opened this issue Sep 19, 2019 · 7 comments · May be fixed by #300

Comments

@fnune
Copy link

fnune commented Sep 19, 2019

My configuration:

    'graphql/required-fields': [
      'error',
      {
        env: 'apollo',
        schemaJson: require('./schema.json'),
        requiredFields: ['id'],
      },
      {
        env: 'literal',
        schemaJson: require('./schema.json'),
        requiredFields: ['id'],
      },
    ],

My query:

query Main {
  nestedField {
    ...NestedField
  }
}

fragment NestedField on SomeType {
  id
  someField
}

Expected result

No linter error.

Actual result

/path/MyQuery.graphql
  2:3  error  'id' field required on 'nestedField'  graphql/required-fields

Apparently the required-fields rule doesn't account for fields included in a fragment.

Update

It seems like field selection merging is part of the GraphQL specification: https://graphql.github.io/graphql-spec/draft/#sec-Field-Selection-Merging

So this is not really a bug per se but more a feature request to be more exhaustive in checking for the inclusion of required fiels in fragments.

@jameswritescode
Copy link

Similar issue, using graphql-tag/loader with with a literal importing a fragment like this results in a required-fields error if requiredFields: ['id']:

#import "./MessageFragment.graphql"

mutation createMessage($input: MessageInput!) {
  createMessage(input: $input) {
    ...MessageFragment
  }
} 
fragment MessageFragment on Message {
  id
  snippedOtherFields
}

@moimael
Copy link

moimael commented Oct 28, 2019

Seems like it is the intended behaviour if I understand this comment properly:

// Every field that can have the field directly on it, should. It's not

@fnune
Copy link
Author

fnune commented Oct 28, 2019

I don't completely understand the comment, though. What does it mean for a fragment to cover all of the possible type options?

@moimael
Copy link

moimael commented Oct 28, 2019

I guess when you have something like this in your fragment:

... on FirstType {
  id
}

But the parent supports:

... on FirstType {
  id
}

... on SecondType {
  id
}

or

parent {
  id
  ... on FirstType {
	...
  }

  ... on SecondType {
    ...
  }
}

At least this is how I understand it.

@fnune
Copy link
Author

fnune commented Oct 28, 2019

I suppose a decision was taken not to implement checking fragments exhaustively to see if they include a required field.

@moimael
Copy link

moimael commented Oct 28, 2019

Yeah I guess so, not sure how hard it would be to implement that though

@fnune
Copy link
Author

fnune commented Oct 28, 2019

It seems like field selection merging is part of the GraphQL specification: https://graphql.github.io/graphql-spec/draft/#sec-Field-Selection-Merging

So this is not really a bug per se but more a feature request to be more exhaustive in checking for the inclusion of required fiels in fragments.

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

Successfully merging a pull request may close this issue.

3 participants