Flaky `persisted_queries/schema_patch_spec.rb` with new `graphql-ruby`

[viralpraxis]

trafficstars

An MRE:

bundle exec rspec './spec/graphql/persisted_queries/compiled_queries/multiplex_patch_spec.rb[1:2:1:1]' './spec/graphql/persisted_queries/schema_patch_spec.rb[1:1:2:1,1:1:3:2]' --seed 1194
Run options: include {ids: {"./spec/graphql/persisted_queries/compiled_queries/multiplex_patch_spec.rb" => ["1:2:1:1"], "./spec/graphql/persisted_queries/schema_patch_spec.rb" => ["1:1:2:1", "1:1:3:2"]}}

Randomized with seed 1194

GraphQL::PersistedQueries::CompiledQueries::MultiplexPatch
  #multiplex
    when all queries are not passed
      returns errors

GraphQL::PersistedQueries::SchemaPatch
  #execute
    when cache is populated
`Schema.tracer(#<TestTracer:0x000075e6a7fd4ae0 @events={}>)` is deprecated; use module-based `trace_with` instead. See: https://graphql-ruby.org/queries/tracing.html
  /home/viralpraxis/Documents/github/graphql-ruby-persisted_queries/lib/graphql/persisted_queries/schema_patch.rb:92:in 'GraphQL::PersistedQueries::SchemaPatch#tracer'
      emits a query save event (FAILED - 1)
    when cache is warm
`Schema.tracer(#<TestTracer:0x000075e6a8123900 @events={}>)` is deprecated; use module-based `trace_with` instead. See: https://graphql-ruby.org/queries/tracing.html
  /home/viralpraxis/Documents/github/graphql-ruby-persisted_queries/lib/graphql/persisted_queries/schema_patch.rb:92:in 'GraphQL::PersistedQueries::SchemaPatch#tracer'
      emits a cache hit (FAILED - 2)

Failures:

  1) GraphQL::PersistedQueries::SchemaPatch#execute when cache is populated emits a query save event
     Failure/Error: expect(events).to eq([{ metadata: { adapter: :memory }, result: query }])
     
       expected: [{metadata: {adapter: :memory}, result: "query { someData }"}]
            got: [{metadata: {adapter: :memory}, result: "query { someData }"}, {metadata: {adapter: :memory}, result:...06ET0[\x00@\v[\x06U:$GraphQL::Language::Nodes::Field[\ri\x06i\x0E0I\"\rsomeData\x06;\aT@\v@\v@\v0"}]
     
       (compared using ==)
     
       Diff:
       @@ -1 +1,4 @@
       -[{metadata: {adapter: :memory}, result: "query { someData }"}]
       +[{metadata: {adapter: :memory}, result: "query { someData }"},
       + {metadata: {adapter: :memory},
       +  result:
       +   "\x04\bU:'GraphQL::Language::Nodes::Document[\ti\x06i\x060[\x06U:2GraphQL::Language::Nodes::OperationDefinition[\ri\x06i\x060I\"\nquery\x06:\x06ET0[\x00@\v[\x06U:$GraphQL::Language::Nodes::Field[\ri\x06i\x0E0I\"\rsomeData\x06;\aT@\v@\v@\v0"}]
       
     # ./spec/graphql/persisted_queries/schema_patch_spec.rb:66:in 'block (4 levels) in <top (required)>'

  2) GraphQL::PersistedQueries::SchemaPatch#execute when cache is warm emits a cache hit
     Failure/Error: expect(events).to eq([{ metadata: { adapter: :memory }, result: nil }])
     
       expected: [{metadata: {adapter: :memory}, result: nil}]
            got: [{metadata: {adapter: :memory}, result: nil}, {metadata: {adapter: :memory}, result: nil}]
     
       (compared using ==)
     
       Diff:
       @@ -1 +1,2 @@
       -[{metadata: {adapter: :memory}, result: nil}]
       +[{metadata: {adapter: :memory}, result: nil},
       + {metadata: {adapter: :memory}, result: nil}]
       
     # ./spec/graphql/persisted_queries/schema_patch_spec.rb:82:in 'block (4 levels) in <top (required)>'

Finished in 0.07232 seconds (files took 0.07618 seconds to load)
3 examples, 2 failures

Failed examples:

rspec ./spec/graphql/persisted_queries/schema_patch_spec.rb:63 # GraphQL::PersistedQueries::SchemaPatch#execute when cache is populated emits a query save event
rspec ./spec/graphql/persisted_queries/schema_patch_spec.rb:78 # GraphQL::PersistedQueries::SchemaPatch#execute when cache is warm emits a cache hit

Randomized with seed 119

Looks like this is due to removal of Multiplex.begin_query which happened 2 years ago

[viralpraxis]

I figured it out: the problem is that GraphQL::PersistedQueries::CompiledQueries::QueryPatch patch is applied globally:

https://github.com/DmitryTsepelev/graphql-ruby-persisted_queries/blob/39e58e4707981e0d293eb04fc1d69aee4a3538cf/lib/graphql/persisted_queries.rb#L51

and there's no check in QueryPatch#prepare_ast if compiled queries are used:

https://github.com/DmitryTsepelev/graphql-ruby-persisted_queries/blob/39e58e4707981e0d293eb04fc1d69aee4a3538cf/lib/graphql/persisted_queries/compiled_queries/query_patch.rb#L16-L19

Here's a patch which fixes the tests:

diff --git i/lib/graphql/persisted_queries/compiled_queries/query_patch.rb w/lib/graphql/persisted_queries/compiled_queries/query_patch.rb
index 4618fd8..84a028e 100644
--- i/lib/graphql/persisted_queries/compiled_queries/query_patch.rb
+++ w/lib/graphql/persisted_queries/compiled_queries/query_patch.rb
@@ -14,7 +14,7 @@ module GraphQL
         end
 
         def prepare_ast
-          return super if @context[:extensions].nil? || @document
+          return super if !schema.compiled_queries? || @context[:extensions].nil? || @document
 
           try_load_document!
 
diff --git i/lib/graphql/persisted_queries/schema_patch.rb w/lib/graphql/persisted_queries/schema_patch.rb
index 8b0652b..6c2c7db 100644
--- i/lib/graphql/persisted_queries/schema_patch.rb
+++ w/lib/graphql/persisted_queries/schema_patch.rb
@@ -14,6 +14,10 @@ module GraphQL
         def patch(schema, compiled_queries)
           schema.singleton_class.prepend(SchemaPatch)
 
+          schema.singleton_class.define_method(:compiled_queries?) do
+            compiled_queries
+          end
+
           if compiled_queries
             configure_compiled_queries(schema)
           else

but I have no idea if it actually fixes the underlying issue since I'm not yet familiar enough with the gem's codebase

FYI @DmitryTsepelev

[DmitryTsepelev]

Could you please make a PR? I guess we can merge it and see where it goes

[viralpraxis]

Here we go: https://github.com/DmitryTsepelev/graphql-ruby-persisted_queries/pull/85

[DmitryTsepelev]

I think I got it. I was able to reproduce the issue locally via bundle exec rspec, but CI runs specs in a different way: there's a task called ci_specs (bundle exec rake ci_specs), which isolates compiled queries into separate tasks. I guess we can keep things as is—usually people either use CQ or not use them, not change that back and forth :)

[viralpraxis]

usually people either use CQ or not use them, not change that back and forth :

I was thinking about a case when there are two different schemas (with enabled and disabled CQ) 🤔