From c0a80c843cf98d3ac1ef5f993491a207565e7f44 Mon Sep 17 00:00:00 2001 From: Oliver Chang Date: Wed, 10 Apr 2024 13:09:12 -0700 Subject: [PATCH 1/8] add more db files to gitignore --- .gitignore | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.gitignore b/.gitignore index b55d872576..c47336ad02 100644 --- a/.gitignore +++ b/.gitignore @@ -7,6 +7,8 @@ Gemfile.lock gemfiles/*.lock # Test database *.db +*.db-shm +*.db-wal .bundle/ vendor/ .idea/ From 9752c4cec345c23deabbb3b3341c9f94ae5a3597 Mon Sep 17 00:00:00 2001 From: Oliver Chang Date: Wed, 10 Apr 2024 16:22:12 -0700 Subject: [PATCH 2/8] override load_nodes --- .../active_record_relation_connection.rb | 20 +++++++++++++++++++ .../active_record_relation_connection_spec.rb | 5 ++--- 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/lib/graphql/pagination/active_record_relation_connection.rb b/lib/graphql/pagination/active_record_relation_connection.rb index 4c7d027d45..d2bc60a1e8 100644 --- a/lib/graphql/pagination/active_record_relation_connection.rb +++ b/lib/graphql/pagination/active_record_relation_connection.rb @@ -72,6 +72,26 @@ def set_offset(nodes, offset) def already_loaded?(relation) relation.is_a?(Array) || relation.loaded? end + + # Override to fetch one extra record to infer has_next_page, + # avoiding a potential expensive select in the future. + def load_nodes + @nodes ||= begin + original_node_limit = relation_limit(limited_nodes) + if original_node_limit + overshot_nodes = set_limit(limited_nodes, original_node_limit + 1).to_a + if overshot_nodes.size > original_node_limit + @has_next_page = true + overshot_nodes[0...-1] + else + @has_next_page = false + overshot_nodes + end + else + limited_nodes.to_a + end + end + end end end end diff --git a/spec/graphql/pagination/active_record_relation_connection_spec.rb b/spec/graphql/pagination/active_record_relation_connection_spec.rb index f8573c3814..3ab3f569ab 100644 --- a/spec/graphql/pagination/active_record_relation_connection_spec.rb +++ b/spec/graphql/pagination/active_record_relation_connection_spec.rb @@ -172,10 +172,9 @@ def total_count } }") end - # This currently runs one query to load the nodes, then another one to count _just beyond_ the nodes. - # A better implementation would load `first + 1` nodes and use that to set `has_next_page`. assert_equal ["Item", "Item", "Item"], results["data"]["items"]["nodes"].map { |i| i["__typename"] } - assert_equal 2, log.split("\n").size, "It runs two queries -- TODO this could be better" + assert_equal 1, log.split("\n").size, "It runs only one query" + assert_equal 0, log.scan("COUNT(").size, "It runs no count query" end describe "already-loaded ActiveRecord relations" do From a89a350df1fb05ef8d9bd166aba6e52e161210a2 Mon Sep 17 00:00:00 2001 From: Oliver Chang Date: Wed, 10 Apr 2024 16:47:24 -0700 Subject: [PATCH 3/8] add another comment --- lib/graphql/pagination/active_record_relation_connection.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/graphql/pagination/active_record_relation_connection.rb b/lib/graphql/pagination/active_record_relation_connection.rb index d2bc60a1e8..6cc155fe40 100644 --- a/lib/graphql/pagination/active_record_relation_connection.rb +++ b/lib/graphql/pagination/active_record_relation_connection.rb @@ -84,6 +84,7 @@ def load_nodes @has_next_page = true overshot_nodes[0...-1] else + # we didn't overshoot and there are no next pages @has_next_page = false overshot_nodes end From c33be2f015350ef90ea989439567a8cd257d8eb3 Mon Sep 17 00:00:00 2001 From: Robert Mosolgo Date: Mon, 15 Apr 2024 15:52:16 -0400 Subject: [PATCH 4/8] Move nodes optimization to relation connection, add test for different ordering of selections --- .../active_record_relation_connection.rb | 21 ------------------- lib/graphql/pagination/relation_connection.rb | 17 ++++++++++++++- .../active_record_relation_connection_spec.rb | 21 +++++++++++++++++++ 3 files changed, 37 insertions(+), 22 deletions(-) diff --git a/lib/graphql/pagination/active_record_relation_connection.rb b/lib/graphql/pagination/active_record_relation_connection.rb index 6cc155fe40..4c7d027d45 100644 --- a/lib/graphql/pagination/active_record_relation_connection.rb +++ b/lib/graphql/pagination/active_record_relation_connection.rb @@ -72,27 +72,6 @@ def set_offset(nodes, offset) def already_loaded?(relation) relation.is_a?(Array) || relation.loaded? end - - # Override to fetch one extra record to infer has_next_page, - # avoiding a potential expensive select in the future. - def load_nodes - @nodes ||= begin - original_node_limit = relation_limit(limited_nodes) - if original_node_limit - overshot_nodes = set_limit(limited_nodes, original_node_limit + 1).to_a - if overshot_nodes.size > original_node_limit - @has_next_page = true - overshot_nodes[0...-1] - else - # we didn't overshoot and there are no next pages - @has_next_page = false - overshot_nodes - end - else - limited_nodes.to_a - end - end - end end end end diff --git a/lib/graphql/pagination/relation_connection.rb b/lib/graphql/pagination/relation_connection.rb index 0e6b091474..fe4becbb01 100644 --- a/lib/graphql/pagination/relation_connection.rb +++ b/lib/graphql/pagination/relation_connection.rb @@ -221,7 +221,22 @@ def limited_nodes # returns an array of nodes def load_nodes # Return an array so we can consistently use `.index(node)` on it - @nodes ||= limited_nodes.to_a + @nodes ||= begin + original_node_limit = relation_limit(limited_nodes) + if original_node_limit + overshot_nodes = set_limit(limited_nodes, original_node_limit + 1).to_a + if overshot_nodes.size > original_node_limit + @has_next_page = true + overshot_nodes[0...-1] + else + # we didn't overshoot and there are no next pages + @has_next_page = false + overshot_nodes + end + else + limited_nodes.to_a + end + end end end end diff --git a/spec/graphql/pagination/active_record_relation_connection_spec.rb b/spec/graphql/pagination/active_record_relation_connection_spec.rb index 3ab3f569ab..885f269bf9 100644 --- a/spec/graphql/pagination/active_record_relation_connection_spec.rb +++ b/spec/graphql/pagination/active_record_relation_connection_spec.rb @@ -159,7 +159,10 @@ def total_count assert_equal ["Item"] * 10, results["data"]["items"]["nodes"].map { |i| i["__typename"] } assert_equal 1, log.split("\n").size, "It runs only one query when less than total count is requested" assert_equal 0, log.scan("COUNT(").size, "It runs no count query" + end + it "runs one query for both `nodes` and `hasNextPage` if nodes happen to be selected first" do + results = nil log = with_active_record_log do results = schema.execute("{ items(first: 3) { @@ -177,6 +180,24 @@ def total_count assert_equal 0, log.scan("COUNT(").size, "It runs no count query" end + it "runs two queries for `nodes` and `hasNextPage` if hasNextPage is selected first" + log = with_active_record_log do + results = schema.execute("{ + items(first: 3) { + pageInfo { + hasNextPage + } + nodes { + __typename + } + } + }") + end + assert_equal ["Item", "Item", "Item"], results["data"]["items"]["nodes"].map { |i| i["__typename"] } + assert_equal 2, log.split("\n").size, "It runs two queries" + assert_equal 1, log.scan("COUNT(").size, "It runs one count query" + end + describe "already-loaded ActiveRecord relations" do ALREADY_LOADED_QUERY_STRING = " query($first: Int, $after: String, $last: Int, $before: String) { From 644f347d8cd1f5c64e327cd5be6048a524130106 Mon Sep 17 00:00:00 2001 From: Robert Mosolgo Date: Mon, 15 Apr 2024 15:59:30 -0400 Subject: [PATCH 5/8] fix syntax error --- .../pagination/active_record_relation_connection_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/graphql/pagination/active_record_relation_connection_spec.rb b/spec/graphql/pagination/active_record_relation_connection_spec.rb index 885f269bf9..b527701450 100644 --- a/spec/graphql/pagination/active_record_relation_connection_spec.rb +++ b/spec/graphql/pagination/active_record_relation_connection_spec.rb @@ -180,7 +180,7 @@ def total_count assert_equal 0, log.scan("COUNT(").size, "It runs no count query" end - it "runs two queries for `nodes` and `hasNextPage` if hasNextPage is selected first" + it "runs two queries for `nodes` and `hasNextPage` if hasNextPage is selected first" do log = with_active_record_log do results = schema.execute("{ items(first: 3) { From 8920d949c08a79338f2c5632b492cf18e617f1ec Mon Sep 17 00:00:00 2001 From: Oliver Chang Date: Thu, 25 Apr 2024 19:02:22 -0700 Subject: [PATCH 6/8] hoist results in new test into scope --- .../graphql/pagination/active_record_relation_connection_spec.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/graphql/pagination/active_record_relation_connection_spec.rb b/spec/graphql/pagination/active_record_relation_connection_spec.rb index b527701450..57b4e9e0e0 100644 --- a/spec/graphql/pagination/active_record_relation_connection_spec.rb +++ b/spec/graphql/pagination/active_record_relation_connection_spec.rb @@ -181,6 +181,7 @@ def total_count end it "runs two queries for `nodes` and `hasNextPage` if hasNextPage is selected first" do + results = nil log = with_active_record_log do results = schema.execute("{ items(first: 3) { From 74b16aae83ca0714fd0d60009e4bd83603385f64 Mon Sep 17 00:00:00 2001 From: Oliver Chang Date: Thu, 25 Apr 2024 19:08:59 -0700 Subject: [PATCH 7/8] just skip it --- spec/graphql/authorization_spec.rb | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/spec/graphql/authorization_spec.rb b/spec/graphql/authorization_spec.rb index fee0731e14..e19ebe76ff 100644 --- a/spec/graphql/authorization_spec.rb +++ b/spec/graphql/authorization_spec.rb @@ -714,13 +714,13 @@ def auth_execute(*args, **kwargs) assert_nil edge.fetch("node") assert_equal "RelayObjectEdge", edge["__typename"] - unauthorized_object_paths = [ - ["unauthorizedConnection", "edges", 0, "node"], - ["unauthorizedConnection", "nodes", 0], - ["unauthorizedEdge", "node"] - ] - - assert_equal unauthorized_object_paths, unauthorized_res["errors"].map { |e| e["path"] } + # unauthorized_object_paths = [ + # ["unauthorizedConnection", "edges", 0, "node"], + # ["unauthorizedConnection", "nodes", 0], + # ["unauthorizedEdge", "node"] + # ] + # + # assert_equal unauthorized_object_paths, unauthorized_res["errors"].map { |e| e["path"] } authorized_res = auth_execute(query) conn = authorized_res["data"].fetch("unauthorizedConnection") From c0cfce4f9c21c758b8042e106a8fce6420c8e325 Mon Sep 17 00:00:00 2001 From: Oliver Chang Date: Thu, 25 Apr 2024 19:09:56 -0700 Subject: [PATCH 8/8] Revert "just skip it" This reverts commit 74b16aae83ca0714fd0d60009e4bd83603385f64. --- spec/graphql/authorization_spec.rb | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/spec/graphql/authorization_spec.rb b/spec/graphql/authorization_spec.rb index e19ebe76ff..fee0731e14 100644 --- a/spec/graphql/authorization_spec.rb +++ b/spec/graphql/authorization_spec.rb @@ -714,13 +714,13 @@ def auth_execute(*args, **kwargs) assert_nil edge.fetch("node") assert_equal "RelayObjectEdge", edge["__typename"] - # unauthorized_object_paths = [ - # ["unauthorizedConnection", "edges", 0, "node"], - # ["unauthorizedConnection", "nodes", 0], - # ["unauthorizedEdge", "node"] - # ] - # - # assert_equal unauthorized_object_paths, unauthorized_res["errors"].map { |e| e["path"] } + unauthorized_object_paths = [ + ["unauthorizedConnection", "edges", 0, "node"], + ["unauthorizedConnection", "nodes", 0], + ["unauthorizedEdge", "node"] + ] + + assert_equal unauthorized_object_paths, unauthorized_res["errors"].map { |e| e["path"] } authorized_res = auth_execute(query) conn = authorized_res["data"].fetch("unauthorizedConnection")