From 2231a8e8b2cc8475f83193b347d664b8c414ad4a Mon Sep 17 00:00:00 2001 From: Robert Mosolgo Date: Thu, 16 Feb 2023 09:15:31 -0500 Subject: [PATCH 1/2] Assert matching response shapes in fields_will_merge --- .../rules/fields_will_merge.rb | 64 ++++++++++++++----- .../rules/fields_will_merge_error.rb | 8 ++- .../rules/fields_will_merge_spec.rb | 49 +++++++++++++- 3 files changed, 100 insertions(+), 21 deletions(-) diff --git a/lib/graphql/static_validation/rules/fields_will_merge.rb b/lib/graphql/static_validation/rules/fields_will_merge.rb index c5b4f822a4..35cd457e42 100644 --- a/lib/graphql/static_validation/rules/fields_will_merge.rb +++ b/lib/graphql/static_validation/rules/fields_will_merge.rb @@ -33,26 +33,19 @@ def on_field(node, _parent) private - def field_conflicts - @field_conflicts ||= Hash.new do |errors, field| - errors[field] = GraphQL::StaticValidation::FieldsWillMergeError.new(kind: :field, field_name: field) - end - end - - def arg_conflicts - @arg_conflicts ||= Hash.new do |errors, field| - errors[field] = GraphQL::StaticValidation::FieldsWillMergeError.new(kind: :argument, field_name: field) + def conflicts + @conflicts ||= Hash.new do |h, error_type| + h[error_type] = Hash.new do |h2, field_name| + h2[field_name] = GraphQL::StaticValidation::FieldsWillMergeError.new(kind: error_type, field_name: field_name) + end end end def setting_errors - @field_conflicts = nil - @arg_conflicts = nil - + @conflicts = nil yield # don't initialize these if they weren't initialized in the block: - @field_conflicts && @field_conflicts.each_value { |error| add_error(error) } - @arg_conflicts && @arg_conflicts.each_value { |error| add_error(error) } + @conflicts && @conflicts.each_value { |error_type| error_type.each_value { |error| add_error(error) } } end def conflicts_within_selection_set(node, parent_type) @@ -221,7 +214,7 @@ def find_conflict(response_key, field1, field2, mutually_exclusive: false) if !are_mutually_exclusive if node1.name != node2.name - conflict = field_conflicts[response_key] + conflict = conflicts[:field][response_key] conflict.add_conflict(node1, node1.name) conflict.add_conflict(node2, node2.name) @@ -230,7 +223,7 @@ def find_conflict(response_key, field1, field2, mutually_exclusive: false) end if !same_arguments?(node1, node2) - conflict = arg_conflicts[response_key] + conflict = conflicts[:argument][response_key] conflict.add_conflict(node1, GraphQL::Language.serialize(serialize_field_args(node1))) conflict.add_conflict(node2, GraphQL::Language.serialize(serialize_field_args(node2))) @@ -239,6 +232,18 @@ def find_conflict(response_key, field1, field2, mutually_exclusive: false) end end + if !conflicts[:field].key?(response_key) && + (t1 = field1.definition&.type) && + (t2 = field2.definition&.type) && + return_types_conflict?(t1, t2) + + conflict = conflicts[:return_type][response_key] + conflict.add_conflict(node1, "`#{t1.to_type_signature}`") + conflict.add_conflict(node2, "`#{t2.to_type_signature}`") + + @conflict_count += 1 + end + find_conflicts_between_sub_selection_sets( field1, field2, @@ -246,6 +251,31 @@ def find_conflict(response_key, field1, field2, mutually_exclusive: false) ) end + def return_types_conflict?(type1, type2) + if type1.list? + if type2.list? + return_types_conflict?(type1.of_type, type2.of_type) + else + true + end + elsif type2.list? + elsif type1.non_null? + if type2.non_null? + return_types_conflict?(type1.of_type, type2.of_type) + else + true + end + elsif type2.non_null? + true + elsif !type1.kind.fields? && !type2.kind.fields? + type1 != type2 + else + # One or more of these are composite types, + # their selections will be validated later on. + false + end + end + def find_conflicts_between_sub_selection_sets(field1, field2, mutually_exclusive:) return if field1.definition.nil? || field2.definition.nil? || @@ -344,7 +374,7 @@ def find_fields_and_fragments(selections, owner_type:, parents:, fields:, fragme fields << Field.new(node, definition, owner_type, parents) when GraphQL::Language::Nodes::InlineFragment fragment_type = node.type ? context.warden.get_type(node.type.name) : owner_type - find_fields_and_fragments(node.selections, parents: [*parents, fragment_type], owner_type: owner_type, fields: fields, fragment_spreads: fragment_spreads) if fragment_type + find_fields_and_fragments(node.selections, parents: [*parents, fragment_type], owner_type: fragment_type, fields: fields, fragment_spreads: fragment_spreads) if fragment_type when GraphQL::Language::Nodes::FragmentSpread fragment_spreads << FragmentSpread.new(node.name, parents) end diff --git a/lib/graphql/static_validation/rules/fields_will_merge_error.rb b/lib/graphql/static_validation/rules/fields_will_merge_error.rb index 742f4e5294..1fcc6c964b 100644 --- a/lib/graphql/static_validation/rules/fields_will_merge_error.rb +++ b/lib/graphql/static_validation/rules/fields_will_merge_error.rb @@ -26,7 +26,13 @@ def conflicts end def add_conflict(node, conflict_str) - return if nodes.include?(node) + # Can't use `.include?` here because AST nodes implement `#==` + # based on string value, not including location. But sometimes, + # identical nodes conflict because of their differing return types. + if nodes.any? { |n| n == node && n.line == node.line && n.col == node.col } + # already have an error for this node + return + end @nodes << node @conflicts << conflict_str diff --git a/spec/graphql/static_validation/rules/fields_will_merge_spec.rb b/spec/graphql/static_validation/rules/fields_will_merge_spec.rb index e1d40e39cd..e602cca5d8 100644 --- a/spec/graphql/static_validation/rules/fields_will_merge_spec.rb +++ b/spec/graphql/static_validation/rules/fields_will_merge_spec.rb @@ -49,7 +49,7 @@ interface Pet { name(surname: Boolean = false): String! - nickname: String + nickname: String! toys: [Toy!]! } @@ -63,7 +63,7 @@ type Dog implements Pet & Mammal & Canine { name(surname: Boolean = false): String! - nickname: String + nickname: String! doesKnowCommand(dogCommand: PetCommand): Boolean! barkVolume: Int! toys: [Toy!]! @@ -71,7 +71,7 @@ type Cat implements Pet & Mammal & Feline { name(surname: Boolean = false): String! - nickname: String + nickname: String! doesKnowCommand(catCommand: PetCommand): Boolean! meowVolume: Int! toys: [Toy!]! @@ -998,4 +998,47 @@ refute_match %r/\$arg12/, error_messages.first end end + + describe "Conflicting leaf typed fields" do + it "adds an error" do + schema = GraphQL::Schema.from_definition(<<-GRAPHQL) + interface Thing { + name: String + } + + type Dog implements Thing { + spots: Boolean + } + + type Jaguar implements Thing { + spots: Int + } + + type Query { + thing: Thing + } + GRAPHQL + + query_str = <<-GRAPHQL + { + thing { + ... on Dog { spots } + ... on Jaguar { spots } + } + } + GRAPHQL + + res = schema.validate(query_str) + expected_error = { + "message"=>"Field 'spots' has a return_type conflict: `Boolean` or `Int`?", + "locations"=>[{"line"=>3, "column"=>24}, {"line"=>4, "column"=>27}], + "path"=>[], + "extensions"=> + {"code"=>"fieldConflict", + "fieldName"=>"spots", + "conflicts"=>"`Boolean` or `Int`"} + } + assert_equal [expected_error], res.map(&:to_h) + end + end end From 6b6648cee85295e3a3af301ec2d3de7dcd4d9cc9 Mon Sep 17 00:00:00 2001 From: Robert Mosolgo Date: Thu, 16 Feb 2023 09:32:17 -0500 Subject: [PATCH 2/2] remove test that's contrary to the graphql spec --- spec/graphql/execution/interpreter_spec.rb | 27 ---------------------- 1 file changed, 27 deletions(-) diff --git a/spec/graphql/execution/interpreter_spec.rb b/spec/graphql/execution/interpreter_spec.rb index 6f5097ae46..c01d6aba47 100644 --- a/spec/graphql/execution/interpreter_spec.rb +++ b/spec/graphql/execution/interpreter_spec.rb @@ -41,11 +41,6 @@ def lazy_sym Box.new(value: object.sym) end - field :null_union_field_test, Integer, null: false - def null_union_field_test - 1 - end - field :always_cached_value, Integer, null: false def always_cached_value raise "should never be called" @@ -61,11 +56,6 @@ def expansion Query::EXPANSIONS.find { |e| e.sym == @object.expansion_sym } end - field :null_union_field_test, Integer - def null_union_field_test - nil - end - field :parent_class_name, String, null: false, extras: [:parent] def parent_class_name(parent:) @@ -484,23 +474,6 @@ def self.trace(event, data) assert_equal Hash, res["data"].class assert_equal Array, res["data"]["findMany"].class end - - it "works with union lists that have members of different kinds, with different nullabilities" do - res = InterpreterTest::Schema.execute <<-GRAPHQL - { - findMany(ids: ["RAV", "Dark Confidant"]) { - ... on Expansion { - nullUnionFieldTest - } - ... on Card { - nullUnionFieldTest - } - } - } - GRAPHQL - - assert_equal [1, nil], res["data"]["findMany"].map { |f| f["nullUnionFieldTest"] } - end end describe "duplicated fields" do