Better request normalization (#95)

docs/client.md

@@ -61,24 +61,24 @@ 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 {
   product(id: "1") { name }
 }
 ```
 
-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!) {

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

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
 

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

lib/graphql/stitching/version.rb

@@ -2,6 +2,6 @@
 
 module GraphQL
   module Stitching
-    VERSION = "1.0.5"
+    VERSION = "1.0.6"
   end
 end

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!

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)