From 10bf73a5762745348c76cbf0c277ea3d843ebacc Mon Sep 17 00:00:00 2001 From: Greg MacWilliam Date: Mon, 27 Nov 2023 09:44:02 -0500 Subject: [PATCH 1/2] cleanup request normalization. --- docs/client.md | 8 +- lib/graphql/stitching/client.rb | 4 +- lib/graphql/stitching/request.rb | 32 ++++---- lib/graphql/stitching/skip_include.rb | 6 +- test/graphql/stitching/request_test.rb | 86 +++++++++++++++------ test/graphql/stitching/skip_include_test.rb | 5 +- 6 files changed, 96 insertions(+), 45 deletions(-) diff --git a/docs/client.md b/docs/client.md index d8292d89..2e1df2e6 100644 --- a/docs/client.md +++ b/docs/client.md @@ -61,16 +61,16 @@ Arguments for the `execute` method include: The client provides cache hooks to enable caching query plans across requests. Without caching, every request made to the client will be planned individually. With caching, a query may be planned once, cached, and then executed from cache for subsequent requests. Cache keys are a normalized digest of each query string. ```ruby -client.on_cache_read do |key, _context| +client.on_cache_read do |key, _context, _request| $redis.get(key) # << 3P code end -client.on_cache_write do |key, payload, _context| +client.on_cache_write do |key, payload, _context, _request| $redis.set(key, payload) # << 3P code end ``` -Note that inlined input data works against caching, so you should _avoid_ this: +Note that inlined input data works against caching, so you should _avoid_ this when possible: ```graphql query { @@ -78,7 +78,7 @@ query { } ``` -Instead, always leverage variables in queries so that the document body remains consistent across requests: +Instead, leverage query variables so that the document body remains consistent across requests: ```graphql query($id: ID!) { diff --git a/lib/graphql/stitching/client.rb b/lib/graphql/stitching/client.rb index d0e52c31..2349ff4f 100644 --- a/lib/graphql/stitching/client.rb +++ b/lib/graphql/stitching/client.rb @@ -75,14 +75,14 @@ def on_error(&block) def fetch_plan(request) if @on_cache_read - cached_plan = @on_cache_read.call(request.digest, request.context) + cached_plan = @on_cache_read.call(request.digest, request.context, request) return GraphQL::Stitching::Plan.from_json(JSON.parse(cached_plan)) if cached_plan end plan = yield if @on_cache_write - @on_cache_write.call(request.digest, JSON.generate(plan.as_json), request.context) + @on_cache_write.call(request.digest, JSON.generate(plan.as_json), request.context, request) end plan diff --git a/lib/graphql/stitching/request.rb b/lib/graphql/stitching/request.rb index 54648184..514c38b2 100644 --- a/lib/graphql/stitching/request.rb +++ b/lib/graphql/stitching/request.rb @@ -4,14 +4,13 @@ module GraphQL module Stitching class Request SUPPORTED_OPERATIONS = ["query", "mutation"].freeze + SKIP_INCLUDE_DIRECTIVE = /@(?:skip|include)/ attr_reader :document, :variables, :operation_name, :context def initialize(document, operation_name: nil, variables: nil, context: nil) - @may_contain_runtime_directives = true - @document = if document.is_a?(String) - @may_contain_runtime_directives = document.include?("@") + @string = document GraphQL.parse(document) else document @@ -23,13 +22,21 @@ def initialize(document, operation_name: nil, variables: nil, context: nil) end def string - @string ||= @document.to_query_string + @string || normalized_string + end + + def normalized_string + @normalized_string ||= @document.to_query_string end def digest @digest ||= Digest::SHA2.hexdigest(string) end + def normalized_digest + @normalized_digest ||= Digest::SHA2.hexdigest(normalized_string) + end + def operation @operation ||= begin operation_defs = @document.definitions.select do |d| @@ -69,18 +76,15 @@ def fragment_definitions def prepare! operation.variables.each do |v| - @variables[v.name] = v.default_value if @variables[v.name].nil? + @variables[v.name] = v.default_value if @variables[v.name].nil? && !v.default_value.nil? end - if @may_contain_runtime_directives - @document, modified = SkipInclude.render(@document, @variables) - - if modified - @string = nil - @digest = nil - @operation = nil - @variable_definitions = nil - @fragment_definitions = nil + if @string.nil? || @string.match?(SKIP_INCLUDE_DIRECTIVE) + SkipInclude.render(@document, @variables) do |modified_ast| + @document = modified_ast + @string = @normalized_string = nil + @digest = @normalized_digest = nil + @operation = @operation_directives = @variable_definitions = nil end end diff --git a/lib/graphql/stitching/skip_include.rb b/lib/graphql/stitching/skip_include.rb index 85e2d5f8..7fe60444 100644 --- a/lib/graphql/stitching/skip_include.rb +++ b/lib/graphql/stitching/skip_include.rb @@ -15,7 +15,11 @@ def render(document, variables) definition end - return document.merge(definitions: definitions), changed + return document unless changed + + document = document.merge(definitions: definitions) + yield(document) if block_given? + document end private diff --git a/test/graphql/stitching/request_test.rb b/test/graphql/stitching/request_test.rb index add26098..7c5dd189 100644 --- a/test/graphql/stitching/request_test.rb +++ b/test/graphql/stitching/request_test.rb @@ -20,11 +20,11 @@ def test_selects_single_operation_by_default end def test_selects_from_multiple_operations_by_operation_name - query = " + query = %| query First { widget { id } } query Second { sprocket { id } } - mutation Third { makeSprocket(id: \"1\") { id } } - " + mutation Third { makeSprocket(id: "1") { id } } + | request1 = GraphQL::Stitching::Request.new(query, operation_name: "First") request2 = GraphQL::Stitching::Request.new(query, operation_name: "Second") request3 = GraphQL::Stitching::Request.new(query, operation_name: "Third") @@ -73,11 +73,11 @@ def test_access_operation_directives end def test_accesses_document_variable_definitions - query = " + query = %| query($ids: [ID!]!, $ns: String!, $lang: String) { widget(ids: $ids, ns: $ns) { id name(lang: $lang) } } - " + | request = GraphQL::Stitching::Request.new(query) variables = request.variable_definitions.each_with_object({}) do |(name, type), memo| memo[name] = GraphQL::Language::Printer.new.print(type) @@ -93,35 +93,60 @@ def test_accesses_document_variable_definitions end def test_accesses_document_fragment_definitions - query = " + query = %| query { things { ...WidgetAttrs ...SprocketAttrs } } fragment WidgetAttrs on Widget { widget } fragment SprocketAttrs on Sprocket { sprocket } - " + | request = GraphQL::Stitching::Request.new(query) assert_equal "widget", request.fragment_definitions["WidgetAttrs"].selections.first.name assert_equal "sprocket", request.fragment_definitions["SprocketAttrs"].selections.first.name end - def test_generates_a_digest_from_string_and_ast_input - sample_ast = GraphQL.parse("query { things { name } }") - sample_query = GraphQL::Language::Printer.new.print(sample_ast) - expected_digest = "88908d0790f7b20afe4a7508a8bba6343c62f98abb9c5abff17345c64d90c0d0" + def test_provides_string_and_normalized_string + string = %| + query { + things { name } + } + | - request1 = GraphQL::Stitching::Request.new(sample_ast) - assert_equal expected_digest, request1.digest + document = GraphQL.parse(string) + request = GraphQL::Stitching::Request.new(string) + assert_equal string, request.string + assert_equal GraphQL.parse(string).to_query_string, request.normalized_string + end - request2 = GraphQL::Stitching::Request.new(sample_query) - assert_equal expected_digest, request2.digest + def test_provides_string_and_normalized_string_for_parsed_ast_input + document = GraphQL.parse("query { things { name } }") + request = GraphQL::Stitching::Request.new(document) + expected = document.to_query_string + + assert_equal expected, request.string + assert_equal expected, request.normalized_string + end + + def test_provides_digest_and_normalized_digest + string = %| + query { + things { name } + } + | + + request = GraphQL::Stitching::Request.new(string) + expected = "ad4b4eb706f67020084a7927ed5bd73b7196e393e0af3535d25ae2d22df33232" + expected_normalized = "88908d0790f7b20afe4a7508a8bba6343c62f98abb9c5abff17345c64d90c0d0" + + assert_equal expected, request.digest + assert_equal expected_normalized, request.normalized_digest end def test_prepare_variables_collects_variable_defaults - query = <<~GRAPHQL + query = %| query($a: String! = "defaultA", $b: String! = "defaultB") { base(a: $a, b: $b) { id } } - GRAPHQL + | request = GraphQL::Stitching::Request.new(GraphQL.parse(query), variables: { "a" => "yes" }) request.prepare! @@ -131,11 +156,11 @@ def test_prepare_variables_collects_variable_defaults end def test_prepare_variables_preserves_boolean_values - query = <<~GRAPHQL + query = %| query($a: Boolean, $b: Boolean, $c: Boolean = true) { base(a: $a, b: $b, c: $c) { id } } - GRAPHQL + | variables = { "a" => true, "b" => false, "c" => false } request = GraphQL::Stitching::Request.new(GraphQL.parse(query), variables: variables) @@ -145,15 +170,30 @@ def test_prepare_variables_preserves_boolean_values assert_equal expected, request.variables end + def test_prepare_variables_does_not_add_null_keys + query = %| + query($a: Boolean, $b: Boolean = false) { + base(a: $a, b: $b) { id } + } + | + + variables = {} + request = GraphQL::Stitching::Request.new(GraphQL.parse(query), variables: variables) + request.prepare! + + expected = { "b" => false } + assert_equal expected, request.variables + end + def test_applies_skip_and_include_directives_via_boolean_literals - query = <<~GRAPHQL + query = %| query { skipKeep @skip(if: false) { id } skipOmit @skip(if: true) { id } includeKeep @include(if: true) { id } includeOmit @include(if: false) { id } } - GRAPHQL + | request = GraphQL::Stitching::Request.new(GraphQL.parse(query)) request.prepare! @@ -162,14 +202,14 @@ def test_applies_skip_and_include_directives_via_boolean_literals end def test_applies_skip_and_include_directives_via_variables - query = <<~GRAPHQL + query = %| query($yes: Boolean!, $no: Boolean!) { skipKeep @skip(if: $no) { id } skipOmit @skip(if: $yes) { id } includeKeep @include(if: $yes) { id } includeOmit @include(if: $no) { id } } - GRAPHQL + | request = GraphQL::Stitching::Request.new(GraphQL.parse(query), variables: { "yes" => true, "no" => false }) request.prepare! diff --git a/test/graphql/stitching/skip_include_test.rb b/test/graphql/stitching/skip_include_test.rb index 0157f8cf..95dcff2f 100644 --- a/test/graphql/stitching/skip_include_test.rb +++ b/test/graphql/stitching/skip_include_test.rb @@ -105,7 +105,10 @@ def test_lacking_conditionals_produces_no_changes def render_skip_include(source, variables = {}) @source = source - @result, @changed = GraphQL::Stitching::SkipInclude.render(GraphQL.parse(@source), variables) + @changed = false + @result = GraphQL::Stitching::SkipInclude.render(GraphQL.parse(@source), variables) do + @changed = true + end end def assert_result(result) From 98de2c5c14c1dc6ee6f376bb69d704ddd7de7f49 Mon Sep 17 00:00:00 2001 From: Greg MacWilliam Date: Tue, 28 Nov 2023 23:04:38 -0500 Subject: [PATCH 2/2] version bump. --- lib/graphql/stitching/version.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/graphql/stitching/version.rb b/lib/graphql/stitching/version.rb index 325ddfd9..ee0c709a 100644 --- a/lib/graphql/stitching/version.rb +++ b/lib/graphql/stitching/version.rb @@ -2,6 +2,6 @@ module GraphQL module Stitching - VERSION = "1.0.5" + VERSION = "1.0.6" end end