GraphQL validation: "variable not defined" ignored

[aasseman]

Do you want to request a feature or report a bug?
Bug

What is the current behavior?
Queries with undefined variables are accepted by the graph-node. This is invalid as per the GraphQL spec: https://spec.graphql.org/June2018/#sec-All-Variable-Uses-Defined

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem.
Example with The Graph subgraph. Same behavior on all subgraphs.
Query:

query {
  allocations(first: $first) {
    id
  }
}

Variables:

{
  "first":  10
}

===> OK

What is the expected behavior?
Query:

query {
  allocations(first: $first) {
    id
  }
}

Variables:

{
  "first":  10
}

===> ERROR
But with

query ($first: Int) {
  allocations(first: $first) {
    id
  }
}

===> OK

[dotansimha]

Hi @aasseman !

We recently introduced GraphQL validations based on the complete spec.
As far as I know, it's not rolled out to the hosted service yet, and if you are running your own graph-node, you can enable it by setting ENABLE_GRAPHQL_VALIDATIONS=1 in your environment variables.

[aasseman]

Got it, thanks!
Closing then.

[aasseman]

Actually there seems to be a problem.

When variables are not defined, they are silently ignored altogether (ie. the value of the variable is not even used for the query execution).
Query:

query {
  allocations(first: $first) {
    id
  }
}

Variables:

{
  "first":  1
}

Yields the same result as:

query {
  allocations() {
    id
  }
}

But:

query ($first: Int) {
  allocations(first: $first) {
    id
  }
}

Correctly returns the first element.

This is a serious problem because we are currently observing a large number of queries using undefined variables, and for all of those the consumer is receiving wrong data.

[dotansimha]

Actually there seems to be a problem.

When variables are not defined, they are silently ignored altogether (ie. the value of the variable is not even used for the query execution). Query:

Are you running your own graph-node with ENABLE_GRAPHQL_VALIDATIONS=1 set?

[aasseman]

We are running an indexer in the distributed network without ENABLE_GRAPHQL_VALIDATIONS set. It looks like all indexers are running that way.

I also poked at the Discord, and no one ever mentioned that env variable before we did. Maybe validation should be on by default in some future version of graph-node? Or some other way to make it a requirement in the distributed network?

For now we don't know if being the only indexer with validation turned will be a problem for the protocol.

[aasseman]

Since it's more about setting a standard on the distributed network, maybe @azf20 could help give some insight?

[dotansimha]

Yeah, this is a fairly new feature, introduced a few months ago, and it's still disabled until we'll roll out to the hosted service.
In the meantime, it's disabled by default. You can activate it in order to have validation that matches the complete GraphQL spec.

I understand the concern regarding activating it, especially if others are not using it as well. @lutter what do you think?

[lutter]

We plan on making ENABLE_GRAPHQL_VALIDATIONS the default at some point; IIRC, the current plan is to do that after API versioning has been introduced, since it's a breaking change. We actually spent a lot of time trying to understand the impact on doing proper validations in the hosted service and the network, and concluded that it's just too disruptive to do in the current setup since it requires that lots of people update their apps.

As for the current behavior: that is less than ideal and sorta implementation defined. Some variables, when ommitted, will just use some built-in default (I believe that's the case for first: $not_a_variable which will silently default to 100) The behavior should be deterministic though, just with a lot of asterisks.