feat(GraphQL::Batch::SetupMultiplex) use multiplex instrumentation to share a batching context

[rmosolgo]

This patch adds support for a shared batching context when multiple queries are run concurrently. Is it something you're interested in for this gem?

Single queries are still supported because graphql 1.6+ treats Schema#execute as a special case of Schema#multiplex: a multiplex with one query. So it's still covered by instrument(:multiplex,...).

[dylanahsmith]

Thanks for trying to add support for this. I'm just a little bit concerned about the order of operations.

[dylanahsmith]

You can just use Schema.multiplex directly. The reason test/graphql_test.rb has a schema_execute method is that execution_strategy_test.rb overrides it to do the same tests.

[dylanahsmith]

I'm OK with merging this PR after this nitpick is addressed.

[rmosolgo]

🤦‍♂️ Sorry I forgot about this feedback all this time! I just pushed a fix for it.

[dylanahsmith]

I tried making the following changes to the test

diff --git a/test/multiplex_test.rb b/test/multiplex_test.rb
index c120614..6748aab 100644
--- a/test/multiplex_test.rb
+++ b/test/multiplex_test.rb
@@ -24,9 +24,15 @@ class GraphQL::MultiplexTest < Minitest::Test
         second: product(id: $id2) { id, title }
       }
     GRAPHQL
+    mutation_string = <<-GRAPHQL
+      mutation {
+        counter_loader
+      }
+    GRAPHQL
 
     results = schema_multiplex([
       { query: query_string, variables: { "id1" => "1", "id2" => "2"} },
+      { query: mutation_string, context: { counter: 0 } },
       { query: query_string, variables: { "id1" => "1", "id2" => "3"}},
     ])
 
@@ -39,13 +45,20 @@ class GraphQL::MultiplexTest < Minitest::Test
 
     expected_2 = {
       "data" => {
+        "counter_loader" => 0,
+      }
+    }
+
+    expected_3 = {
+      "data" => {
         "first" => { "id" => "1", "title" => "Shirt" },
         "second" => { "id" => "3", "title" => "Sweater" },
       }
     }
 
     assert_equal expected_1, results.first
-    assert_equal expected_2, results.last
+    assert_equal expected_2, results[1]
+    assert_equal expected_3, results.last
     assert_equal ["Product/1,2,3"], queries
   end
 end

and it still passed, even though there is a mutation in between the queries.

What should be the expectations around the ordering of the operations? The graphql-ruby multiplex documentation says it will "run queries concurrently", but also says this feature is for Apollo Client’s query batching which "is a transport-level mechanism" where I would expect the order of operations would be preserved.

[rmosolgo]

That's a great point, and it's something I overlooked. It makes no special treatment for mutations, but it could. (Apparently the JS server doesn't make special treatment either: apollographql/apollo-server#176 (comment))

Maybe we could do a flow like this:

• Shift the next operation from the set
  • If it's a mutation:
    • Run any already-gathered queries concurrently
    • Run it in isolation
  • If it's a query, add it to the next multiplex
• When the set is empty:
  • Run any already-gathered queries concurrently

I think that would maintain the expected ordering for mutations: queries before mutations would not reflect their changes, but queries after mutations would reflect mutations.

However, now that I write it, I don't know if the incoming queries are ordered. Maybe they come as an unordered set.

If that's the case, maybe a better flow is mutate-then-read:

• For each operation in the set, if it's a mutation, run its root fields
• Then, fullfill all other fields concurrently.

That way, all reads in the multiplex reflect the changes of all mutations in the multiplex.

Hmm, what if the mutations are order-dependent? :head_explodes:

[dylanahsmith]

Interesting, so it looks like they intentionally have the queries running in parallel according to the apollo server implementation. Given that, and the fact that graphql-ruby already follows that approach and documents it more clearly, I guess there is no need to be concerned about it here.

I suppose a client or a server could always choose to re-order the operations and multiplex/group them based on their desired semantics.

Hmm, what if the mutations are order-dependent? :head_explodes:

In that case I would put all the mutations in the same mutation query as separate field selections, although that is easier when using dynamic queries.

[etripier]

This is awesome! It fixes all the issues I was having integrating graphql-batch with react-apollo. Just chiming in to show interest in getting this merged. Thanks @rmosolgo!