Skip to content

Empty runtime variables with @include directive #50

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

Closed
zcei opened this issue May 4, 2021 · 2 comments
Closed

Empty runtime variables with @include directive #50

zcei opened this issue May 4, 2021 · 2 comments

Comments

@zcei
Copy link
Contributor

zcei commented May 4, 2021

Hej folks,

when using the validation rule in combination with a query that provides a variable with a default value, the @include (and potentially also the @skip) directive is causing troubles.

GraphQLError: Argument "if" of required type "Boolean!" was provided the variable "$shouldSkip" which was not provided a runtime value.

I was able to reproduce this with a test case, and I'm wondering what the expected behavior is here.

it('should respect @include(if: false) via default variable value', () => {
  const ast = parse(`
    query Foo ($shouldSkip: Boolean = false) {
      variableScalar(count: 10) @include(if: $shouldSkip)
    }
  `);

  const complexity = getComplexity({
    estimators: [
      simpleEstimator({defaultComplexity: 1})
    ],
    schema,
    query: ast
  });
  expect(complexity).to.equal(0);
});

I see that you're only using graphql-js's getDirectiveValues, which under the hood has a logic that throws the error here.

But to me it sounds like it should first check for the query document default variables.

I feel like we have two options here:

  1. obtain default variables from operation definition here and add these to this.options.variables (or a derivate object of it).
  2. try to convince graphql-js maintainers that there's a flaw in their flow of obtaining runtime values.

As the graphql-js library does not seem to have issues with @include directives, I'm leaning towards (1), wdyt?

@ivome
Copy link
Collaborator

ivome commented May 4, 2021

Thanks for bringing this up! Looks like the default query variables were not considered at all for the complexity and this showed up in the skip / include directives now. I now added the same logic to coerce the variables that is used in the graphql-js library for the execution phase. Your test is now passing. I also added one for field input arguments + default values.

@ivome ivome closed this as completed in 0d340b2 May 4, 2021
@zcei
Copy link
Contributor Author

zcei commented May 4, 2021

Oh ma gee, that's awesome! I haven't even seen getVariableValues and was a bit 😞 that I would need to manually build it. Thanks so much ❤️

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

No branches or pull requests

2 participants