GraphQL 2.2.10 + GraphQL Pro operation store nil reference error about graphql-ruby HOT 11 CLOSED

I thought I would be able to find a place where GraphQL::Language::Nodes::VariableDefinition objects were being created with directives: nil instead of directives: [], which would lead to the error described above.

But, I checked the two places where these nodes are created, but both of them use an empty Array. Here's the Ruby parser:

And the C Parser:

Another thought I had was, if you were using the C parser, maybe it was an old version that didn't handle the new VariableDefinition directives properly. But I tested it locally, and using the old version (1.0.7) failed with a different error (There was already a bug report for that: #4851).

I also remembered that OperationStore does some language-level work itself, to normalize definitions before saving them. So I reviewed that, but realized it was unrelated: it creates strings, not ASTs, and besides, it's only used for sync operations, not for running queries.

Is there anywhere in your app that works with AST nodes? Maybe a GraphQL::Language::Visitor or a call to Nodes::VariableDefinition.new?

Another interesting thing in the backtrace is that the call to query.query_string is going to GraphQL::Language::Printer. This means that the query was executed from an already-parsed document: .... Is that right? Do you mind sharing the context on that, for example, where is the query string parsed, and how is it passed to YourSchema.execute(...)?

from graphql-ruby.

I'm still working on a minimal reproduction case, but reverting has resolved my issue.

We had upgraded graphql pro and graphql at the same time, from 1.26.2 to 1.26.3 and 2.2.8 to 2.2.10, respectively. After reverting back to 1.2.6.2 and 2.2.8 the errors are no longer occurring.

from graphql-ruby.

Hey, thanks for reporting this and sharing the backtrace -- that's very helpful. In 2.2.10, I added support for directives on variable definitions -- but I must have missed a spot updating the printer! I'll work up a patch soon and follow up back here 👍

from graphql-ruby.

Thanks! I'll stop digging on a repro case for now, but please let me know if more details would help.

from graphql-ruby.

You're a wizard!

Yes, in our integration environment we let some code automation bypass the persisted queries. This flow caches the result of GraphQL.parse(...) in redis, and that's supplied via the document param of execute.
I'm looking through logs, and all the failure cases I've looked at actually did hit this flow, so this is very likely the issue.

I'm assuming the cached version from 2.2.8 is missing critical information required in 2.2.10?

from graphql-ruby.

critical information

Well, 2.2.10 added a directives: [] argument to VariableDefinition#initialize, but if nothing is provided, then the default value of [] is used:

GraphQL::Language::Nodes::VariableDefinition.new
# => #<GraphQL::Language::Nodes::VariableDefinition:0x000000012813f500
#  @col=nil,
#  @default_value=nil,
#  @definition_pos=nil,
#  @directives=[],
#  @filename=nil,
#  @line=nil,
#  @name=nil,
#  @pos=nil,
#  @source_string=nil,
#  @type=nil>

That value would work here (since it's an array, not nil). But somehow in the backtrace above, @directives is nil instead of []. Is it possible that the cache is injecting directives: nil instead? Or, if it's using Marshal.load, then it would create VariableDefinitions without a @directives ivar at all, which would be the same as if it were defined as nil.

from graphql-ruby.

Hey, just checking in on this -- were you able to find anything else about where this error was coming from?

from graphql-ruby.

Please let me know if you're still running into this error, otherwise I'll assume you found a solution on your side 👍

from graphql-ruby.

Hey, just checking in on this -- were you able to find anything else about where this error was coming from?

Hi, confirming this issue is specific to our usage of the library, not the library itself. We do:

Rails.cache.fetch(key) { GraphQL.parse(query) }

which under the hoods does a Marshal.dump of the GraphQL::Language::Nodes::Document returned by the GraphQL.parse call.

When we upgrade the gem version to 2.2.10 and reboot our app, the next request to our backend will do a Marshal.load of the previously parsed query.

Marshal.load will not populate the @directives instance variable because it's not using the initialize method of GraphQL::Language::Nodes::VariableDefinition to create the object.

Thanks for the help!

from graphql-ruby.

Interesting -- thanks for sharing what you found! I guess a fix would be to add a "-v2" to the key so that Rails would rebuild the cache. Sorry for the trouble with this!

from graphql-ruby.

No worries, this actually shed the light on some unnecessary home made caching that wasn't needed anymore, so we ended up removing dead code :)

from graphql-ruby.