Custom rule breaks NoUnusedVariablesRule

[alanchristensen]

I created a rule that visits a fragment's selectionSet when it encounters a FRAGMENT_SPREAD. The rule itself worked as desired, however doing so caused NoUnusedVariablesRule to fail if a variable was used in a fragment spread. It appears that my custom rule is changing the visitor in the default rules.

Here's a slimmed down version of my code that demonstrates the problem:

import { buildSchema, parse, validate, NoUnusedVariablesRule, Kind, ValidationContext, ASTVisitor } from 'graphql';

function CustomRule(context: ValidationContext): ASTVisitor {
  return {
    enter: (node) => {
      // If FRAGMENT_SPREAD, then return the selectionSet of the fragment definition
      // This causes visitor to visit the selectionSet instead of ending on the current node
      if (node.kind === Kind.FRAGMENT_SPREAD) {
        return context.getFragment(node.name.value)?.selectionSet;
      }

      return undefined;
    },
    leave: () => {},
  };
}

const schema = buildSchema(/* GraphQL */ `
  type Author {
    name: String
  }
  type Query {
    author: Author
  }
`);

const doc = parse(/* GraphQL */ `
  query AUTHOR($s: Boolean = false) {
    author {
      ...FRAG @skip(if: $s)
    }
  }

  fragment FRAG on Author {
    name
  }
`);

console.debug(validate(schema, doc, [NoUnusedVariablesRule, CustomRule]));

This causes a GraphQLError: Variable "$s" is never used in operation "AUTHOR". error. If you remove the CustomRule then no errors are reported.

[IvanGoncharov]

@alanchristensen Wow, this is unexpected behavior. I will take a look, at what is happening here.

[JoviDeCroock]

I wonder whether this actually is unexpected behavior we are mutating our operation inside of a validation-rule. The result of this mutation will be the following

query AUTHOR($s: Boolean = false) {
  author {
    name
  }
}

As we omitted the @skip, our variable $s indeed becomes unused, I am uncertain whether the intention here would be to i.e. copy over the directive to all selections of the root-selection of the fragment. That however would fix the issue.

We don't really take the variables into account during validation so we don't know whether this fragment-spread will in fact be skipped.