Run `require-id-when-available` on FragmentDefinition

[axynos]

We recently ran into an issue where a missing id field in a selection caused a cryptic error to be thrown in production that caused an incident.

After that incident, I looked into graphql-eslint, specifically the require-id-when-available rule. Turning it on did expose a couple areas where we missed adding id-s, but it does not report any problem on the fragment that caused our incident.

After peeking into the source, I see that it does, in fact, not run on FragmentDefinition.

Would it be possible to also check if ids are included for fields in a fragment definition?

[dimaMachina]

Do you still have error report in your operation? It was disabled after https://github.com/B2o5T/graphql-eslint/issues/731#issue-1039583843

[axynos]

I don't have the specific error as it was nearly 100kb of JSON, but essentially Apollo client failed to write a query result to the store due to a missing id field on a fragment several layers of nesting deep.

I remember it resulting in something like "invariant violation 5" being returned.

Running checkSelections using a selector like FragmentDefinition SelectionSet[parent.kind!=/(^FragmentDefinition|InlineFragment)$/] would have caught the issue for us. (And after testing it, caught several other missing id fields in fragments).

[dimaMachina]

I wanted to say that does you had graphql-eslint error about missing id in your operation? Do you use fragment spread ...MyFrgment in your graphql sdl? Or do you use useFragment hook or something similar?

[axynos]

We use fragment spread, here's a more visual explanation of what happens for us and what we'd like to prevent with this rule.

It's roughly something like this:

// queries/getPage.graphql
# import "../fragments/Section.graphql"
query getPage(some input vars) {
  getPages (input) {
    items {
      id // <-- if removed, would get caught by the rule currently
      sections(input) {
        items {
          ...Section
        }
      }
    }
  }
}

// fragments/Section.graphql
# import "./fragments/Article.graphql"
fragment Section on Section {
  id // <-- this needs to exist
  name
  articles {
    items {
      ...Article
    }
  }
}

// fragments/Article.graphql
fragment Article on Article {
  id // <-- this needs to exist
  title
  primaryCategory {
    id // <-- if this does not exist, it causes apollo writing the query result to its store to fail and spits the cryptic error in the console
    name
  }
}

[dimaMachina]

seems it is caused because nested selections in fragments are not verified, you can contribute and add failing tests in graphq-eslint https://github.com/B2o5T/graphql-eslint/blob/master/packages/plugin/tests/require-id-when-available.spec.ts with wrong behaviour and I'll improve it

[axynos]

I will do that, thanks!

[kushalpandya]

I'm running into this problem in a slightly different way where I want to disable this specific rule for field where id isn't available, but the field is used in queries and mutations via fragments nested at multiple levels, sadly, adding # eslint-disable-next-line @graphql-eslint/require-id-when-available on the field itself still triggers rule violation on parent fragment but when I add the ignore rule on parent too, it reports Unused eslint-disable directive (no problems were reported from '@graphql-eslint/require-id-when-available').

Is there a way to disable the rule on field globally (by perhaps setting a config in eslintrc)?

[adamesque]

We've run into this as well but as a direct consequence of #731. In our environment, we have repos that define fragments for reusable UI modules. We use schema linting to ensure the validity of these fragments, so they aren't included in any operation within the repo.

For our use-case (which I think is valid), it would be nice if the operation requirement was a config option.

[Stephen2]

I'd like to say that even the original request included the text:

An exception to this is when deeper objects are queried in fragments. Queries don't know about the fragment internals, so in this case, it's fragment's responsiblity to query the id.

Which we just encountered causing an issue, because our fragment with deep object queries forgot an id.

Because of this reason alone, I think it's imperative to make this setting a config option, to let people choose between two modes.

I can try to put up a PR for this, to keep momentum up for this change.