Skip to content
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.

Sign up for GitHub

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

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

Merged
merged 2 commits into from
Jul 24, 2017
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
36f6ca2
feat(GraphQL::Batch::SetupMultiplex) use multiplex instrumentation to…
rmosolgo Jun 6, 2017
bcf1d87
refactor(tests) use Schema.multiplex directly
rmosolgo Jul 14, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion lib/graphql/batch.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,11 @@ def self.batch
end

def self.use(schema_defn)
schema_defn.instrument(:query, GraphQL::Batch::Setup)
if GraphQL::VERSION >= "1.6.0"
schema_defn.instrument(:multiplex, GraphQL::Batch::SetupMultiplex)
else
schema_defn.instrument(:query, GraphQL::Batch::Setup)
end
schema_defn.lazy_resolve(::Promise, :sync)
end

Expand All @@ -34,3 +38,4 @@ def self.use(schema_defn)
require_relative "batch/executor"
require_relative "batch/promise"
require_relative "batch/setup"
require_relative "batch/setup_multiplex"
14 changes: 14 additions & 0 deletions lib/graphql/batch/setup_multiplex.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
module GraphQL::Batch
module SetupMultiplex
extend self

def before_multiplex(multiplex)
raise NestedError if GraphQL::Batch::Executor.current
GraphQL::Batch::Executor.current = GraphQL::Batch::Executor.new
end

def after_multiplex(multiplex)
GraphQL::Batch::Executor.current = nil
end
end
end
46 changes: 46 additions & 0 deletions test/multiplex_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
require_relative 'test_helper'

class GraphQL::MultiplexTest < Minitest::Test
attr_reader :queries

def setup
@queries = []
QueryNotifier.subscriber = ->(query) { @queries << query }
end

def teardown
QueryNotifier.subscriber = nil
end

def test_batched_find_by_id
query_string = <<-GRAPHQL
query FetchTwoProducts($id1: ID!, $id2: ID!){
first: product(id: $id1) { id, title }
second: product(id: $id2) { id, title }
}
GRAPHQL

results = Schema.multiplex([
{ query: query_string, variables: { "id1" => "1", "id2" => "2"} },
{ query: query_string, variables: { "id1" => "1", "id2" => "3"}},
])

expected_1 = {
"data" => {
"first" => { "id" => "1", "title" => "Shirt" },
"second" => { "id" => "2", "title" => "Pants" },
}
}

expected_2 = {
"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 ["Product/1,2,3"], queries
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Copy link
Contributor

@dylanahsmith dylanahsmith Jun 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

end