From 891bc6a5b348b66b02be4c13b233acce18df1168 Mon Sep 17 00:00:00 2001 From: Marc-Andre Giroux Date: Thu, 30 Aug 2018 11:04:00 -0400 Subject: [PATCH 01/22] get the simple case working --- Untitled | 12 ++ .../rules/fields_will_merge.rb | 117 +++++++++++++----- .../static_validation/validation_context.rb | 10 +- lib/graphql/static_validation/validator.rb | 1 - .../rules/fields_will_merge_spec.rb | 7 ++ 5 files changed, 106 insertions(+), 41 deletions(-) create mode 100644 Untitled diff --git a/Untitled b/Untitled new file mode 100644 index 0000000000..2b7f46ed2f --- /dev/null +++ b/Untitled @@ -0,0 +1,12 @@ +class Analyzer < Visitor + def initialize + end +end + +class MyAnalyzer < Analyzer + def initial_state + end + + def on_field(node, parent, defn) + end +def diff --git a/lib/graphql/static_validation/rules/fields_will_merge.rb b/lib/graphql/static_validation/rules/fields_will_merge.rb index af9e9df3c9..ab27864c3b 100644 --- a/lib/graphql/static_validation/rules/fields_will_merge.rb +++ b/lib/graphql/static_validation/rules/fields_will_merge.rb @@ -2,48 +2,103 @@ module GraphQL module StaticValidation module FieldsWillMerge - # Special handling for fields without arguments NO_ARGS = {}.freeze - def initialize(*) + def on_operation_definition(node, _parent) + conflicts_within_selection_set(node, type_definition) super + end + + def on_field(node, _parent) + conflicts_within_selection_set(node, type_definition) + super + end - context.each_irep_node do |node| - if node.ast_nodes.size > 1 - defn_names = Set.new(node.ast_nodes.map(&:name)) + private + + def conflicts_within_selection_set(node, type_definition) + fields = find_fields(node.selections, parent_type: type_definition) + fragment_names = find_fragment_names(node.selections) + + # (A) Find find all conflicts "within" the fields of this selection set. + collect_conflicts_within(fields) + end - # Check for more than one GraphQL::Field backing this node: - if defn_names.size > 1 - defn_names = defn_names.sort.join(" or ") - msg = "Field '#{node.name}' has a field conflict: #{defn_names}?" - context.errors << GraphQL::StaticValidation::Message.new(msg, nodes: node.ast_nodes.to_a) + def collect_conflicts_within(response_keys) + response_keys.each do |key, fields| + next if fields.size < 2 + # find conflicts within nodes + for i in 0..fields.size-1 + for j in i+1..fields.size-1 + find_conflict(key, fields[i], fields[j]) end + end + end + end + + def find_conflict(response_key, field1, field2, mutually_exclusive: false) + parent_type_1 = field1[:parent_type] + parent_type_2 = field2[:parent_type] + + node1 = field1[:node] + node2 = field2[:node] + + are_mutually_exclusive = mutually_exclusive || + (parent_type_1 != parent_type_2 && + parent_type_1.kind.object? && + parent_type_2.kind.object?) + + if !are_mutually_exclusive + if node1.name != node2.name + msg = "Field '#{response_key}' has a field conflict: #{node1.name} or #{node2.name}?" + context.errors << GraphQL::StaticValidation::Message.new(msg, nodes: [node1, node2]) + end + + args = possible_arguments(node1, node2) + if args.size > 1 + msg = "Field '#{response_key}' has an argument conflict: #{args.map{ |arg| GraphQL::Language.serialize(arg) }.join(" or ")}?" + context.errors << GraphQL::StaticValidation::Message.new(msg, nodes: [node1, node2]) + end + end + end + + def find_fields(selections, parent_type:) + fields = selections.map do |node| + case node + when GraphQL::Language::Nodes::Field + { node: node, parent_type: parent_type } + when GraphQL::Language::Nodes::InlineFragment + find_fields(node.selections, parent_type: node.type || parent_type) + end + end.flatten - # Check for incompatible / non-identical arguments on this node: - args = node.ast_nodes.map do |n| - if n.arguments.any? - n.arguments.reduce({}) do |memo, a| - arg_value = a.value - memo[a.name] = case arg_value - when GraphQL::Language::Nodes::AbstractNode - arg_value.to_query_string - else - GraphQL::Language.serialize(arg_value) - end - memo - end + fields.group_by { |f| f[:node].alias || f[:node].name } + end + + def find_fragment_names(selections) + selections + .select { |s| s.is_a?(GraphQL::Language::Nodes::FragmentSpread) } + .map(&:name) + end + + def possible_arguments(field1, field2) + # Check for incompatible / non-identical arguments on this node: + [field1, field2].map do |n| + if n.arguments.any? + n.arguments.reduce({}) do |memo, a| + arg_value = a.value + memo[a.name] = case arg_value + when GraphQL::Language::Nodes::AbstractNode + arg_value.to_query_string else - NO_ARGS + GraphQL::Language.serialize(arg_value) end + memo end - args.uniq! - - if args.length > 1 - msg = "Field '#{node.name}' has an argument conflict: #{args.map{ |arg| GraphQL::Language.serialize(arg) }.join(" or ")}?" - context.errors << GraphQL::StaticValidation::Message.new(msg, nodes: node.ast_nodes.to_a) - end + else + NO_ARGS end - end + end.uniq end end end diff --git a/lib/graphql/static_validation/validation_context.rb b/lib/graphql/static_validation/validation_context.rb index 855b5b8c91..20df3cd89b 100644 --- a/lib/graphql/static_validation/validation_context.rb +++ b/lib/graphql/static_validation/validation_context.rb @@ -14,9 +14,8 @@ module StaticValidation class ValidationContext extend Forwardable - attr_reader :query, :errors, :visitor, - :on_dependency_resolve_handlers, :each_irep_node_handlers + :on_dependency_resolve_handlers def_delegators :@query, :schema, :document, :fragments, :operations, :warden @@ -24,9 +23,6 @@ def initialize(query, visitor_class) @query = query @literal_validator = LiteralValidator.new(context: query.context) @errors = [] - # TODO it will take some finegalling but I think all this state could - # be moved to `Visitor` - @each_irep_node_handlers = [] @on_dependency_resolve_handlers = [] @visitor = visitor_class.new(document, self) end @@ -39,10 +35,6 @@ def on_dependency_resolve(&handler) @on_dependency_resolve_handlers << handler end - def each_irep_node(&handler) - @each_irep_node_handlers << handler - end - def valid_literal?(ast_value, type) @literal_validator.validate(ast_value, type) end diff --git a/lib/graphql/static_validation/validator.rb b/lib/graphql/static_validation/validator.rb index 2b3eaf1094..1d60121935 100644 --- a/lib/graphql/static_validation/validator.rb +++ b/lib/graphql/static_validation/validator.rb @@ -39,7 +39,6 @@ def validate(query, validate: true) context.visitor.visit # Post-validation: allow validators to register handlers on rewritten query nodes rewrite_result = context.visitor.rewrite_document - GraphQL::InternalRepresentation::Visit.visit_each_node(rewrite_result.operation_definitions, context.each_irep_node_handlers) { errors: context.errors, 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 c625c6ba90..91493d2fc4 100644 --- a/spec/graphql/static_validation/rules/fields_will_merge_spec.rb +++ b/spec/graphql/static_validation/rules/fields_will_merge_spec.rb @@ -246,6 +246,7 @@ } |} + focus it "fails rule" do assert_equal ["Field 'fido' has a field conflict: name or nickname?"], error_messages end @@ -276,6 +277,7 @@ } |} + focus it "fails rule" do assert_equal [%q(Field 'doesKnowCommand' has an argument conflict: {} or {dogCommand:"HEEL"}?)], error_messages end @@ -291,6 +293,7 @@ } |} + focus it "fails rule" do assert_equal [%q(Field 'doesKnowCommand' has an argument conflict: {dogCommand:"SIT"} or {}?)], error_messages end @@ -306,6 +309,7 @@ } |} + focus it "fails rule" do assert_equal [%q(Field 'doesKnowCommand' has an argument conflict: {dogCommand:"SIT"} or {dogCommand:"HEEL"}?)], error_messages end @@ -321,6 +325,7 @@ } |} + focus it "fails rule" do assert_equal [%q(Field 'image' has an argument conflict: {maxWidth:"10"} or {maxWidth:"20"}?)], error_messages end @@ -354,6 +359,7 @@ end end + describe "deep conflict" do let(:query_string) {%| { @@ -429,6 +435,7 @@ end end + describe "same aliases allowed on non-overlapping fields" do let(:query_string) {%| { From c2910578572478723520b8aeb83f23eacf1f2b74 Mon Sep 17 00:00:00 2001 From: Marc-Andre Giroux Date: Thu, 30 Aug 2018 11:38:36 -0400 Subject: [PATCH 02/22] make subselections work --- .../rules/fields_will_merge.rb | 41 ++++++++++++++++++- .../rules/fields_will_merge_spec.rb | 1 + 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/lib/graphql/static_validation/rules/fields_will_merge.rb b/lib/graphql/static_validation/rules/fields_will_merge.rb index ab27864c3b..c1458e123f 100644 --- a/lib/graphql/static_validation/rules/fields_will_merge.rb +++ b/lib/graphql/static_validation/rules/fields_will_merge.rb @@ -60,13 +60,52 @@ def find_conflict(response_key, field1, field2, mutually_exclusive: false) context.errors << GraphQL::StaticValidation::Message.new(msg, nodes: [node1, node2]) end end + + find_conflicts_between_sub_selection_sets( + field1, + field2, + mutually_exclusive: are_mutually_exclusive + ) + end + + def find_conflicts_between_sub_selection_sets(field1, field2, mutually_exclusive:) + selections = field1[:node].selections + selections2 = field2[:node].selections + fields = find_fields(selections, parent_type: field1[:defn].type.unwrap ) + fields2 = find_fields(selections2, parent_type: field2[:defn].type.unwrap) + fragment_names = find_fragment_names(selections) + fragment_names_2 = find_fragment_names(selections2) + + collect_conflicts_between(fields, fields2, mutually_exclusive: mutually_exclusive) + end + + def collect_conflicts_between(response_keys, response_keys_2, mutually_exclusive:) + response_keys.each do |key, fields| + fields2 = response_keys_2[key] + if fields2 + fields.each do |field| + fields2.each do |field2| + find_conflict( + key, + field, + field2, + mutually_exclusive: mutually_exclusive + ) + end + end + end + end end def find_fields(selections, parent_type:) fields = selections.map do |node| case node when GraphQL::Language::Nodes::Field - { node: node, parent_type: parent_type } + { + node: node, + parent_type: parent_type, + defn: context.schema.get_field(parent_type, node.name) + } when GraphQL::Language::Nodes::InlineFragment find_fields(node.selections, parent_type: node.type || parent_type) end 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 91493d2fc4..6f154df95c 100644 --- a/spec/graphql/static_validation/rules/fields_will_merge_spec.rb +++ b/spec/graphql/static_validation/rules/fields_will_merge_spec.rb @@ -373,6 +373,7 @@ } |} + focus it "fails rule" do expected_errors = [ { From f563502613e7b687bdbae035ad2c8617a7f0572e Mon Sep 17 00:00:00 2001 From: Marc-Andre Giroux Date: Thu, 30 Aug 2018 14:39:06 -0400 Subject: [PATCH 03/22] one test to fix --- .../rules/fields_will_merge.rb | 172 +++++++++++++++++- .../rules/fields_will_merge_spec.rb | 5 +- 2 files changed, 164 insertions(+), 13 deletions(-) diff --git a/lib/graphql/static_validation/rules/fields_will_merge.rb b/lib/graphql/static_validation/rules/fields_will_merge.rb index c1458e123f..c22792d5bc 100644 --- a/lib/graphql/static_validation/rules/fields_will_merge.rb +++ b/lib/graphql/static_validation/rules/fields_will_merge.rb @@ -4,6 +4,12 @@ module StaticValidation module FieldsWillMerge NO_ARGS = {}.freeze + def initialize(*) + super + @visited_fragments = {} + @compared_fragments = {} + end + def on_operation_definition(node, _parent) conflicts_within_selection_set(node, type_definition) super @@ -17,14 +23,117 @@ def on_field(node, _parent) private def conflicts_within_selection_set(node, type_definition) - fields = find_fields(node.selections, parent_type: type_definition) + fields = response_keys_in_selection(node.selections, parent_type: type_definition) fragment_names = find_fragment_names(node.selections) # (A) Find find all conflicts "within" the fields of this selection set. - collect_conflicts_within(fields) + find_conflicts_within(fields) + + fragment_names.each_with_index do |fragment_name, i| + # (B) Then find conflicts between these fields and those represented by + # each spread fragment name found. + find_conflicts_between_fields_and_fragment( + fragment_name, + fields, + mutually_exclusive: false + ) + + # (C) Then compare this fragment with all other fragments found in this + # selection set to collect conflicts between fragments spread together. + # This compares each item in the list of fragment names to every other + # item in that same list (except for itself). + fragment_names[i+1..-1].each do |fragment_name2| + find_conflicts_between_fragments( + fragment_name, + fragment_name2, + mutually_exclusive: false + ) + end + end + end + + def find_conflicts_between_fragments(fragment_name1, fragment_name2, mutually_exclusive:) + return if fragment_name1 == fragment_name2 + + cache_key = compared_fragments_key(fragment_name1, fragment_name2) + return if @compared_fragments.key?(cache_key) + @compared_fragments[cache_key] = true + + fragment1 = context.fragments[fragment_name1] + fragment2 = context.fragments[fragment_name2] + + return if fragment1.nil? || fragment2.nil? + + fragment_type1 = context.schema.types[fragment1.type.name] + fragment_type2 = context.schema.types[fragment2.type.name] + + fragment_fields1 = response_keys_in_selection(fragment1.selections, parent_type: fragment_type1) + fragment_names1 = find_fragment_names(fragment1.selections) + + fragment_fields2 = response_keys_in_selection(fragment2.selections, parent_type: fragment_type2) + fragment_names2 = find_fragment_names(fragment2.selections) + + # (F) First, find all conflicts between these two collections of fields + # (not including any nested fragments). + find_conflicts_between( + fragment_fields1, + fragment_fields2, + mutually_exclusive: mutually_exclusive + ) + + # (G) Then collect conflicts between the first fragment and any nested + # fragments spread in the second fragment. + fragment_names2.each do |fragment_name| + find_conflicts_between_fragments( + fragment_name1, + fragment_name, + mutually_exclusive: mutually_exclusive + ) + end + + # (G) Then collect conflicts between the first fragment and any nested + # fragments spread in the second fragment. + fragment_names1.each do |fragment_name| + find_conflicts_between_fragments( + fragment_name1, + fragment_name, + mutually_exclusive: mutually_exclusive + ) + end + end + + def find_conflicts_between_fields_and_fragment(fragment_name, fields, mutually_exclusive:) + return if @visited_fragments.key?(fragment_name) + @visited_fragments[fragment_name] = true + + fragment = context.fragments[fragment_name] + return if fragment.nil? + + fragment_type = context.schema.types[fragment.type.name] + + fragment_fields = response_keys_in_selection(fragment.selections, parent_type: fragment_type) + fragment_fragment_names = find_fragment_names(fragment.selections) + + # (D) First find any conflicts between the provided collection of fields + # and the collection of fields represented by the given fragment. + find_conflicts_between( + fields, + fragment_fields, + mutually_exclusive: mutually_exclusive + ) + + # (E) Then collect any conflicts between the provided collection of fields + # and any fragment names found in the given fragment. + fragment_fragment_names.each do |fragment_name| + find_conflicts_between_fields_and_fragment( + fragment_name, + fields, + mutually_exclusive: mutually_exclusive + ) + end end - def collect_conflicts_within(response_keys) + def find_conflicts_within(response_keys) response_keys.each do |key, fields| next if fields.size < 2 # find conflicts within nodes @@ -50,7 +159,8 @@ def find_conflict(response_key, field1, field2, mutually_exclusive: false) if !are_mutually_exclusive if node1.name != node2.name - msg = "Field '#{response_key}' has a field conflict: #{node1.name} or #{node2.name}?" + errored_nodes = [node1.name, node2.name].sort.join(" or ") + msg = "Field '#{response_key}' has a field conflict: #{errored_nodes}?" context.errors << GraphQL::StaticValidation::Message.new(msg, nodes: [node1, node2]) end @@ -71,15 +181,49 @@ def find_conflict(response_key, field1, field2, mutually_exclusive: false) def find_conflicts_between_sub_selection_sets(field1, field2, mutually_exclusive:) selections = field1[:node].selections selections2 = field2[:node].selections - fields = find_fields(selections, parent_type: field1[:defn].type.unwrap ) - fields2 = find_fields(selections2, parent_type: field2[:defn].type.unwrap) + fields = response_keys_in_selection(selections, parent_type: field1[:defn].type.unwrap) + fields2 = response_keys_in_selection(selections2, parent_type: field2[:defn].type.unwrap) fragment_names = find_fragment_names(selections) fragment_names_2 = find_fragment_names(selections2) - collect_conflicts_between(fields, fields2, mutually_exclusive: mutually_exclusive) + # (H) First, collect all conflicts between these two collections of field. + find_conflicts_between(fields, fields2, mutually_exclusive: mutually_exclusive) + + # (I) Then collect conflicts between the first collection of fields and + # those referenced by each fragment name associated with the second. + fragment_names_2.each do |fragment_name| + find_conflicts_between_fields_and_fragment( + fields, + fragment_name, + mutually_exclusive: mutually_exclusive + ) + end + + # (I) Then collect conflicts between the second collection of fields and + # those referenced by each fragment name associated with the first. + fragment_names.each do |fragment_name| + find_conflicts_between_fields_and_fragment( + fields2, + fragment_name, + mutually_exclusive: mutually_exclusive + ) + end + + # (J) Also collect conflicts between any fragment names by the first and + # fragment names by the second. This compares each item in the first set of + # names to each item in the second set of names. + fragment_names.each do |frag1| + fragment_names_2.each do |frag2| + find_conflicts_between_fragments( + frag1, + frag2, + mutually_exclusive: mutually_exclusive + ) + end + end end - def collect_conflicts_between(response_keys, response_keys_2, mutually_exclusive:) + def find_conflicts_between(response_keys, response_keys_2, mutually_exclusive:) response_keys.each do |key, fields| fields2 = response_keys_2[key] if fields2 @@ -107,10 +251,14 @@ def find_fields(selections, parent_type:) defn: context.schema.get_field(parent_type, node.name) } when GraphQL::Language::Nodes::InlineFragment - find_fields(node.selections, parent_type: node.type || parent_type) + fragment_type = context.schema.types[node.type.name] + find_fields(node.selections, parent_type: fragment_type || parent_type) end - end.flatten + end.compact.flatten + end + def response_keys_in_selection(selections, parent_type:) + fields = find_fields(selections, parent_type: parent_type) fields.group_by { |f| f[:node].alias || f[:node].name } end @@ -120,6 +268,10 @@ def find_fragment_names(selections) .map(&:name) end + def compared_fragments_key(frag1, frag2) + [frag1, frag2].sort.join('_') + end + def possible_arguments(field1, field2) # Check for incompatible / non-identical arguments on this node: [field1, field2].map do |n| 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 6f154df95c..0fb3afdac9 100644 --- a/spec/graphql/static_validation/rules/fields_will_merge_spec.rb +++ b/spec/graphql/static_validation/rules/fields_will_merge_spec.rb @@ -353,8 +353,8 @@ it "fails rule" do assert_equal [ - "Field 'name' has a field conflict: name or nickname?", "Field 'x' has a field conflict: name or nickname?", + "Field 'name' has a field conflict: name or nickname?" ], error_messages end end @@ -373,7 +373,6 @@ } |} - focus it "fails rule" do expected_errors = [ { @@ -607,7 +606,7 @@ } } fragment X on SomeBox { - scalar: deepBox { unreleatedField } + scalar: deepBox { unrelatedField } } fragment Y on SomeBox { scalar: unrelatedField From 13ba1371072b8309e16c41e767c0e6f084259cab Mon Sep 17 00:00:00 2001 From: Marc-Andre Giroux Date: Thu, 30 Aug 2018 15:03:03 -0400 Subject: [PATCH 04/22] all green --- .../rules/fields_will_merge.rb | 28 +++++++++++-------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/lib/graphql/static_validation/rules/fields_will_merge.rb b/lib/graphql/static_validation/rules/fields_will_merge.rb index c22792d5bc..506638ca97 100644 --- a/lib/graphql/static_validation/rules/fields_will_merge.rb +++ b/lib/graphql/static_validation/rules/fields_will_merge.rb @@ -7,7 +7,6 @@ module FieldsWillMerge def initialize(*) super @visited_fragments = {} - @compared_fragments = {} end def on_operation_definition(node, _parent) @@ -23,6 +22,8 @@ def on_field(node, _parent) private def conflicts_within_selection_set(node, type_definition) + return if type_definition.nil? + fields = response_keys_in_selection(node.selections, parent_type: type_definition) fragment_names = find_fragment_names(node.selections) @@ -55,10 +56,6 @@ def conflicts_within_selection_set(node, type_definition) def find_conflicts_between_fragments(fragment_name1, fragment_name2, mutually_exclusive:) return if fragment_name1 == fragment_name2 - cache_key = compared_fragments_key(fragment_name1, fragment_name2) - return if @compared_fragments.key?(cache_key) - @compared_fragments[cache_key] = true - fragment1 = context.fragments[fragment_name1] fragment2 = context.fragments[fragment_name2] @@ -67,6 +64,8 @@ def find_conflicts_between_fragments(fragment_name1, fragment_name2, mutually_ex fragment_type1 = context.schema.types[fragment1.type.name] fragment_type2 = context.schema.types[fragment2.type.name] + return if fragment_type1.nil? || fragment_type2.nil? + fragment_fields1 = response_keys_in_selection(fragment1.selections, parent_type: fragment_type1) fragment_names1 = find_fragment_names(fragment1.selections) @@ -110,6 +109,7 @@ def find_conflicts_between_fields_and_fragment(fragment_name, fields, mutually_e return if fragment.nil? fragment_type = context.schema.types[fragment.type.name] + return if fragment_type.nil? fragment_fields = response_keys_in_selection(fragment.selections, parent_type: fragment_type) fragment_fragment_names = find_fragment_names(fragment.selections) @@ -181,6 +181,9 @@ def find_conflict(response_key, field1, field2, mutually_exclusive: false) def find_conflicts_between_sub_selection_sets(field1, field2, mutually_exclusive:) selections = field1[:node].selections selections2 = field2[:node].selections + + return if field1[:defn].nil? || field2[:defn].nil? + fields = response_keys_in_selection(selections, parent_type: field1[:defn].type.unwrap) fields2 = response_keys_in_selection(selections2, parent_type: field2[:defn].type.unwrap) fragment_names = find_fragment_names(selections) @@ -251,8 +254,15 @@ def find_fields(selections, parent_type:) defn: context.schema.get_field(parent_type, node.name) } when GraphQL::Language::Nodes::InlineFragment - fragment_type = context.schema.types[node.type.name] - find_fields(node.selections, parent_type: fragment_type || parent_type) + fragment_type = node.type ? context.schema.types[node.type.name] : parent_type + + if fragment_type + find_fields(node.selections, parent_type: fragment_type) + else + # A bad fragment name was provided, let it go and it will be + # caught by another rule + nil + end end end.compact.flatten end @@ -268,10 +278,6 @@ def find_fragment_names(selections) .map(&:name) end - def compared_fragments_key(frag1, frag2) - [frag1, frag2].sort.join('_') - end - def possible_arguments(field1, field2) # Check for incompatible / non-identical arguments on this node: [field1, field2].map do |n| From 6238d032481501607a5181cd2a669533ba67b09f Mon Sep 17 00:00:00 2001 From: Marc-Andre Giroux Date: Thu, 30 Aug 2018 15:07:30 -0400 Subject: [PATCH 05/22] linter --- lib/graphql/static_validation/rules/fields_will_merge.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/graphql/static_validation/rules/fields_will_merge.rb b/lib/graphql/static_validation/rules/fields_will_merge.rb index 506638ca97..3dc6d672c2 100644 --- a/lib/graphql/static_validation/rules/fields_will_merge.rb +++ b/lib/graphql/static_validation/rules/fields_will_merge.rb @@ -245,7 +245,7 @@ def find_conflicts_between(response_keys, response_keys_2, mutually_exclusive:) end def find_fields(selections, parent_type:) - fields = selections.map do |node| + selections.map do |node| case node when GraphQL::Language::Nodes::Field { From 8c12dcf544cb69d299e6aff093918ea0948d0fb4 Mon Sep 17 00:00:00 2001 From: Marc-Andre Giroux Date: Thu, 30 Aug 2018 15:09:01 -0400 Subject: [PATCH 06/22] run rufo --- .../rules/fields_will_merge.rb | 44 +++++++++---------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/lib/graphql/static_validation/rules/fields_will_merge.rb b/lib/graphql/static_validation/rules/fields_will_merge.rb index 3dc6d672c2..a0d58268fc 100644 --- a/lib/graphql/static_validation/rules/fields_will_merge.rb +++ b/lib/graphql/static_validation/rules/fields_will_merge.rb @@ -36,18 +36,18 @@ def conflicts_within_selection_set(node, type_definition) find_conflicts_between_fields_and_fragment( fragment_name, fields, - mutually_exclusive: false + mutually_exclusive: false, ) # (C) Then compare this fragment with all other fragments found in this # selection set to collect conflicts between fragments spread together. # This compares each item in the list of fragment names to every other # item in that same list (except for itself). - fragment_names[i+1..-1].each do |fragment_name2| + fragment_names[i + 1..-1].each do |fragment_name2| find_conflicts_between_fragments( fragment_name, fragment_name2, - mutually_exclusive: false + mutually_exclusive: false, ) end end @@ -77,7 +77,7 @@ def find_conflicts_between_fragments(fragment_name1, fragment_name2, mutually_ex find_conflicts_between( fragment_fields1, fragment_fields2, - mutually_exclusive: mutually_exclusive + mutually_exclusive: mutually_exclusive, ) # (G) Then collect conflicts between the first fragment and any nested @@ -86,7 +86,7 @@ def find_conflicts_between_fragments(fragment_name1, fragment_name2, mutually_ex find_conflicts_between_fragments( fragment_name1, fragment_name, - mutually_exclusive: mutually_exclusive + mutually_exclusive: mutually_exclusive, ) end @@ -96,7 +96,7 @@ def find_conflicts_between_fragments(fragment_name1, fragment_name2, mutually_ex find_conflicts_between_fragments( fragment_name1, fragment_name, - mutually_exclusive: mutually_exclusive + mutually_exclusive: mutually_exclusive, ) end end @@ -119,7 +119,7 @@ def find_conflicts_between_fields_and_fragment(fragment_name, fields, mutually_e find_conflicts_between( fields, fragment_fields, - mutually_exclusive: mutually_exclusive + mutually_exclusive: mutually_exclusive, ) # (E) Then collect any conflicts between the provided collection of fields @@ -128,7 +128,7 @@ def find_conflicts_between_fields_and_fragment(fragment_name, fields, mutually_e find_conflicts_between_fields_and_fragment( fragment_name, fields, - mutually_exclusive: mutually_exclusive + mutually_exclusive: mutually_exclusive, ) end end @@ -137,8 +137,8 @@ def find_conflicts_within(response_keys) response_keys.each do |key, fields| next if fields.size < 2 # find conflicts within nodes - for i in 0..fields.size-1 - for j in i+1..fields.size-1 + for i in 0..fields.size - 1 + for j in i + 1..fields.size - 1 find_conflict(key, fields[i], fields[j]) end end @@ -166,7 +166,7 @@ def find_conflict(response_key, field1, field2, mutually_exclusive: false) args = possible_arguments(node1, node2) if args.size > 1 - msg = "Field '#{response_key}' has an argument conflict: #{args.map{ |arg| GraphQL::Language.serialize(arg) }.join(" or ")}?" + msg = "Field '#{response_key}' has an argument conflict: #{args.map { |arg| GraphQL::Language.serialize(arg) }.join(" or ")}?" context.errors << GraphQL::StaticValidation::Message.new(msg, nodes: [node1, node2]) end end @@ -174,7 +174,7 @@ def find_conflict(response_key, field1, field2, mutually_exclusive: false) find_conflicts_between_sub_selection_sets( field1, field2, - mutually_exclusive: are_mutually_exclusive + mutually_exclusive: are_mutually_exclusive, ) end @@ -198,7 +198,7 @@ def find_conflicts_between_sub_selection_sets(field1, field2, mutually_exclusive find_conflicts_between_fields_and_fragment( fields, fragment_name, - mutually_exclusive: mutually_exclusive + mutually_exclusive: mutually_exclusive, ) end @@ -208,7 +208,7 @@ def find_conflicts_between_sub_selection_sets(field1, field2, mutually_exclusive find_conflicts_between_fields_and_fragment( fields2, fragment_name, - mutually_exclusive: mutually_exclusive + mutually_exclusive: mutually_exclusive, ) end @@ -220,7 +220,7 @@ def find_conflicts_between_sub_selection_sets(field1, field2, mutually_exclusive find_conflicts_between_fragments( frag1, frag2, - mutually_exclusive: mutually_exclusive + mutually_exclusive: mutually_exclusive, ) end end @@ -236,7 +236,7 @@ def find_conflicts_between(response_keys, response_keys_2, mutually_exclusive:) key, field, field2, - mutually_exclusive: mutually_exclusive + mutually_exclusive: mutually_exclusive, ) end end @@ -251,7 +251,7 @@ def find_fields(selections, parent_type:) { node: node, parent_type: parent_type, - defn: context.schema.get_field(parent_type, node.name) + defn: context.schema.get_field(parent_type, node.name), } when GraphQL::Language::Nodes::InlineFragment fragment_type = node.type ? context.schema.types[node.type.name] : parent_type @@ -285,11 +285,11 @@ def possible_arguments(field1, field2) n.arguments.reduce({}) do |memo, a| arg_value = a.value memo[a.name] = case arg_value - when GraphQL::Language::Nodes::AbstractNode - arg_value.to_query_string - else - GraphQL::Language.serialize(arg_value) - end + when GraphQL::Language::Nodes::AbstractNode + arg_value.to_query_string + else + GraphQL::Language.serialize(arg_value) + end memo end else From 3060ced8accb10753ab1616593455d10c0b1163e Mon Sep 17 00:00:00 2001 From: Marc-Andre Giroux Date: Thu, 30 Aug 2018 15:20:33 -0400 Subject: [PATCH 07/22] align things --- .../static_validation/rules/fields_will_merge.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/graphql/static_validation/rules/fields_will_merge.rb b/lib/graphql/static_validation/rules/fields_will_merge.rb index a0d58268fc..2a7580f1b8 100644 --- a/lib/graphql/static_validation/rules/fields_will_merge.rb +++ b/lib/graphql/static_validation/rules/fields_will_merge.rb @@ -285,11 +285,11 @@ def possible_arguments(field1, field2) n.arguments.reduce({}) do |memo, a| arg_value = a.value memo[a.name] = case arg_value - when GraphQL::Language::Nodes::AbstractNode - arg_value.to_query_string - else - GraphQL::Language.serialize(arg_value) - end + when GraphQL::Language::Nodes::AbstractNode + arg_value.to_query_string + else + GraphQL::Language.serialize(arg_value) + end memo end else From 851ec2fa7dd8f3b28bc95d9c6ab57a3ef6100a88 Mon Sep 17 00:00:00 2001 From: Marc-Andre Giroux Date: Thu, 30 Aug 2018 16:23:50 -0400 Subject: [PATCH 08/22] fix loop problem --- lib/graphql/static_validation/rules/fields_will_merge.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/graphql/static_validation/rules/fields_will_merge.rb b/lib/graphql/static_validation/rules/fields_will_merge.rb index 2a7580f1b8..a06c204c95 100644 --- a/lib/graphql/static_validation/rules/fields_will_merge.rb +++ b/lib/graphql/static_validation/rules/fields_will_merge.rb @@ -94,7 +94,7 @@ def find_conflicts_between_fragments(fragment_name1, fragment_name2, mutually_ex # fragments spread in the second fragment. fragment_names1.each do |fragment_name| find_conflicts_between_fragments( - fragment_name1, + fragment_name2, fragment_name, mutually_exclusive: mutually_exclusive, ) From bef746bb4ddcf0aa8304a51141f402c6c68e0370 Mon Sep 17 00:00:00 2001 From: Marc-Andre Giroux Date: Fri, 31 Aug 2018 10:36:21 -0400 Subject: [PATCH 09/22] Use struct to carry field info --- .../rules/fields_will_merge.rb | 43 ++++++++----------- 1 file changed, 18 insertions(+), 25 deletions(-) diff --git a/lib/graphql/static_validation/rules/fields_will_merge.rb b/lib/graphql/static_validation/rules/fields_will_merge.rb index a06c204c95..51e23d1f0a 100644 --- a/lib/graphql/static_validation/rules/fields_will_merge.rb +++ b/lib/graphql/static_validation/rules/fields_will_merge.rb @@ -3,6 +3,7 @@ module GraphQL module StaticValidation module FieldsWillMerge NO_ARGS = {}.freeze + Field = Struct.new(:node, :definition, :parent_type) def initialize(*) super @@ -146,11 +147,11 @@ def find_conflicts_within(response_keys) end def find_conflict(response_key, field1, field2, mutually_exclusive: false) - parent_type_1 = field1[:parent_type] - parent_type_2 = field2[:parent_type] + parent_type_1 = field1.parent_type + parent_type_2 = field2.parent_type - node1 = field1[:node] - node2 = field2[:node] + node1 = field1.node + node2 = field2.node are_mutually_exclusive = mutually_exclusive || (parent_type_1 != parent_type_2 && @@ -179,13 +180,13 @@ def find_conflict(response_key, field1, field2, mutually_exclusive: false) end def find_conflicts_between_sub_selection_sets(field1, field2, mutually_exclusive:) - selections = field1[:node].selections - selections2 = field2[:node].selections + selections = field1.node.selections + selections2 = field2.node.selections - return if field1[:defn].nil? || field2[:defn].nil? + return if field1.definition.nil? || field2.definition.nil? - fields = response_keys_in_selection(selections, parent_type: field1[:defn].type.unwrap) - fields2 = response_keys_in_selection(selections2, parent_type: field2[:defn].type.unwrap) + fields = response_keys_in_selection(selections, parent_type: field1.definition.type.unwrap) + fields2 = response_keys_in_selection(selections2, parent_type: field2.definition.type.unwrap) fragment_names = find_fragment_names(selections) fragment_names_2 = find_fragment_names(selections2) @@ -245,31 +246,23 @@ def find_conflicts_between(response_keys, response_keys_2, mutually_exclusive:) end def find_fields(selections, parent_type:) - selections.map do |node| + fields = selections.map do |node| case node when GraphQL::Language::Nodes::Field - { - node: node, - parent_type: parent_type, - defn: context.schema.get_field(parent_type, node.name), - } + definition = context.schema.get_field(parent_type, node.name) + Field.new(node, definition, parent_type) when GraphQL::Language::Nodes::InlineFragment fragment_type = node.type ? context.schema.types[node.type.name] : parent_type - - if fragment_type - find_fields(node.selections, parent_type: fragment_type) - else - # A bad fragment name was provided, let it go and it will be - # caught by another rule - nil - end + find_fields(node.selections, parent_type: fragment_type) if fragment_type end - end.compact.flatten + end + + fields.compact.flatten end def response_keys_in_selection(selections, parent_type:) fields = find_fields(selections, parent_type: parent_type) - fields.group_by { |f| f[:node].alias || f[:node].name } + fields.group_by { |f| f.node.alias || f.node.name } end def find_fragment_names(selections) From a0587bb0bb63f8937bcacc9abfcf8ecd3daa02ec Mon Sep 17 00:00:00 2001 From: Marc-Andre Giroux Date: Fri, 31 Aug 2018 10:40:16 -0400 Subject: [PATCH 10/22] add comment --- .../rules/fields_will_merge.rb | 31 +++++++++++-------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/lib/graphql/static_validation/rules/fields_will_merge.rb b/lib/graphql/static_validation/rules/fields_will_merge.rb index 51e23d1f0a..d000fc6f94 100644 --- a/lib/graphql/static_validation/rules/fields_will_merge.rb +++ b/lib/graphql/static_validation/rules/fields_will_merge.rb @@ -2,6 +2,11 @@ module GraphQL module StaticValidation module FieldsWillMerge + # Validates that a selection set is valid if all fields (including spreading any + # fragments) either correspond to distinct response names or can be merged + # without ambiguity. + # + # Original Algorithm: https://github.com/graphql/graphql-js/blob/master/src/validation/rules/OverlappingFieldsCanBeMerged.js NO_ARGS = {}.freeze Field = Struct.new(:node, :definition, :parent_type) @@ -22,10 +27,10 @@ def on_field(node, _parent) private - def conflicts_within_selection_set(node, type_definition) - return if type_definition.nil? + def conflicts_within_selection_set(node, parent_type) + return if parent_type.nil? - fields = response_keys_in_selection(node.selections, parent_type: type_definition) + fields = response_keys_in_selection(node.selections, parent_type: parent_type) fragment_names = find_fragment_names(node.selections) # (A) Find find all conflicts "within" the fields of this selection set. @@ -147,16 +152,16 @@ def find_conflicts_within(response_keys) end def find_conflict(response_key, field1, field2, mutually_exclusive: false) - parent_type_1 = field1.parent_type - parent_type_2 = field2.parent_type + parent_type1 = field1.parent_type + parent_type2 = field2.parent_type node1 = field1.node node2 = field2.node are_mutually_exclusive = mutually_exclusive || - (parent_type_1 != parent_type_2 && - parent_type_1.kind.object? && - parent_type_2.kind.object?) + (parent_type1 != parent_type2 && + parent_type1.kind.object? && + parent_type2.kind.object?) if !are_mutually_exclusive if node1.name != node2.name @@ -188,14 +193,14 @@ def find_conflicts_between_sub_selection_sets(field1, field2, mutually_exclusive fields = response_keys_in_selection(selections, parent_type: field1.definition.type.unwrap) fields2 = response_keys_in_selection(selections2, parent_type: field2.definition.type.unwrap) fragment_names = find_fragment_names(selections) - fragment_names_2 = find_fragment_names(selections2) + fragment_names2 = find_fragment_names(selections2) # (H) First, collect all conflicts between these two collections of field. find_conflicts_between(fields, fields2, mutually_exclusive: mutually_exclusive) # (I) Then collect conflicts between the first collection of fields and # those referenced by each fragment name associated with the second. - fragment_names_2.each do |fragment_name| + fragment_names2.each do |fragment_name| find_conflicts_between_fields_and_fragment( fields, fragment_name, @@ -217,7 +222,7 @@ def find_conflicts_between_sub_selection_sets(field1, field2, mutually_exclusive # fragment names by the second. This compares each item in the first set of # names to each item in the second set of names. fragment_names.each do |frag1| - fragment_names_2.each do |frag2| + fragment_names2.each do |frag2| find_conflicts_between_fragments( frag1, frag2, @@ -227,9 +232,9 @@ def find_conflicts_between_sub_selection_sets(field1, field2, mutually_exclusive end end - def find_conflicts_between(response_keys, response_keys_2, mutually_exclusive:) + def find_conflicts_between(response_keys, response_keys2, mutually_exclusive:) response_keys.each do |key, fields| - fields2 = response_keys_2[key] + fields2 = response_keys2[key] if fields2 fields.each do |field| fields2.each do |field2| From d56d134e5685455d7067473ff2cc0a5cff34cb7b Mon Sep 17 00:00:00 2001 From: Marc-Andre Giroux Date: Fri, 31 Aug 2018 10:51:26 -0400 Subject: [PATCH 11/22] compared fragment cache --- .../rules/fields_will_merge.rb | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/lib/graphql/static_validation/rules/fields_will_merge.rb b/lib/graphql/static_validation/rules/fields_will_merge.rb index d000fc6f94..b3c7a6b261 100644 --- a/lib/graphql/static_validation/rules/fields_will_merge.rb +++ b/lib/graphql/static_validation/rules/fields_will_merge.rb @@ -13,6 +13,7 @@ module FieldsWillMerge def initialize(*) super @visited_fragments = {} + @compared_fragments = {} end def on_operation_definition(node, _parent) @@ -62,6 +63,17 @@ def conflicts_within_selection_set(node, parent_type) def find_conflicts_between_fragments(fragment_name1, fragment_name2, mutually_exclusive:) return if fragment_name1 == fragment_name2 + cache_key = compared_fragments_key( + fragment_name1, + fragment_name2, + mutually_exclusive, + ) + if @compared_fragments.key?(cache_key) + return + else + @compared_fragments[cache_key] = true + end + fragment1 = context.fragments[fragment_name1] fragment2 = context.fragments[fragment_name2] @@ -295,6 +307,14 @@ def possible_arguments(field1, field2) end end.uniq end + + def compared_fragments_key(frag1, frag2, exclusive) + # Cache key to not compare two fragments more than once. + # The key includes both fragment names sorted (this way we + # avoid computing "A vs B" and "B vs A"). It also includes + # "exclusive" since the result may change depending on the parent_type + "#{[frag1, frag2].sort.join('-')}-#{exclusive}" + end end end end From af57fb8d6ef8aff7957708adcdad1b5a65b2f06b Mon Sep 17 00:00:00 2001 From: Marc-Andre Giroux Date: Fri, 31 Aug 2018 10:56:03 -0400 Subject: [PATCH 12/22] add fragment name and selection cache --- .../rules/fields_will_merge.rb | 47 ++++++++++--------- 1 file changed, 25 insertions(+), 22 deletions(-) diff --git a/lib/graphql/static_validation/rules/fields_will_merge.rb b/lib/graphql/static_validation/rules/fields_will_merge.rb index b3c7a6b261..f60423a61f 100644 --- a/lib/graphql/static_validation/rules/fields_will_merge.rb +++ b/lib/graphql/static_validation/rules/fields_will_merge.rb @@ -14,6 +14,8 @@ def initialize(*) super @visited_fragments = {} @compared_fragments = {} + @selections_for_node = {} + @fragment_names_for_node = {} end def on_operation_definition(node, _parent) @@ -31,8 +33,8 @@ def on_field(node, _parent) def conflicts_within_selection_set(node, parent_type) return if parent_type.nil? - fields = response_keys_in_selection(node.selections, parent_type: parent_type) - fragment_names = find_fragment_names(node.selections) + fields = response_keys_in_selection(node, parent_type: parent_type) + fragment_names = find_fragment_names(node) # (A) Find find all conflicts "within" the fields of this selection set. find_conflicts_within(fields) @@ -84,11 +86,11 @@ def find_conflicts_between_fragments(fragment_name1, fragment_name2, mutually_ex return if fragment_type1.nil? || fragment_type2.nil? - fragment_fields1 = response_keys_in_selection(fragment1.selections, parent_type: fragment_type1) - fragment_names1 = find_fragment_names(fragment1.selections) + fragment_fields1 = response_keys_in_selection(fragment1, parent_type: fragment_type1) + fragment_names1 = find_fragment_names(fragment1) - fragment_fields2 = response_keys_in_selection(fragment2.selections, parent_type: fragment_type2) - fragment_names2 = find_fragment_names(fragment2.selections) + fragment_fields2 = response_keys_in_selection(fragment2, parent_type: fragment_type2) + fragment_names2 = find_fragment_names(fragment2) # (F) First, find all conflicts between these two collections of fields # (not including any nested fragments). @@ -129,8 +131,8 @@ def find_conflicts_between_fields_and_fragment(fragment_name, fields, mutually_e fragment_type = context.schema.types[fragment.type.name] return if fragment_type.nil? - fragment_fields = response_keys_in_selection(fragment.selections, parent_type: fragment_type) - fragment_fragment_names = find_fragment_names(fragment.selections) + fragment_fields = response_keys_in_selection(fragment, parent_type: fragment_type) + fragment_fragment_names = find_fragment_names(fragment) # (D) First find any conflicts between the provided collection of fields # and the collection of fields represented by the given fragment. @@ -197,15 +199,12 @@ def find_conflict(response_key, field1, field2, mutually_exclusive: false) end def find_conflicts_between_sub_selection_sets(field1, field2, mutually_exclusive:) - selections = field1.node.selections - selections2 = field2.node.selections - return if field1.definition.nil? || field2.definition.nil? - fields = response_keys_in_selection(selections, parent_type: field1.definition.type.unwrap) - fields2 = response_keys_in_selection(selections2, parent_type: field2.definition.type.unwrap) - fragment_names = find_fragment_names(selections) - fragment_names2 = find_fragment_names(selections2) + fields = response_keys_in_selection(field1.node, parent_type: field1.definition.type.unwrap) + fields2 = response_keys_in_selection(field2.node, parent_type: field2.definition.type.unwrap) + fragment_names = find_fragment_names(field1.node) + fragment_names2 = find_fragment_names(field2.node) # (H) First, collect all conflicts between these two collections of field. find_conflicts_between(fields, fields2, mutually_exclusive: mutually_exclusive) @@ -277,15 +276,19 @@ def find_fields(selections, parent_type:) fields.compact.flatten end - def response_keys_in_selection(selections, parent_type:) - fields = find_fields(selections, parent_type: parent_type) - fields.group_by { |f| f.node.alias || f.node.name } + def response_keys_in_selection(node, parent_type:) + @selections_for_node[node] ||= begin + fields = find_fields(node.selections, parent_type: parent_type) + fields.group_by { |f| f.node.alias || f.node.name } + end end - def find_fragment_names(selections) - selections - .select { |s| s.is_a?(GraphQL::Language::Nodes::FragmentSpread) } - .map(&:name) + def find_fragment_names(node) + @fragment_names_for_node[node] ||= begin + node.selections + .select { |s| s.is_a?(GraphQL::Language::Nodes::FragmentSpread) } + .map(&:name) + end end def possible_arguments(field1, field2) From 95b945c61593701ce445cde36776c00d5568c1bf Mon Sep 17 00:00:00 2001 From: Marc-Andre Giroux Date: Fri, 31 Aug 2018 12:11:18 -0400 Subject: [PATCH 13/22] more optimizations, fix fragment name issue --- .../rules/fields_will_merge.rb | 58 ++++++++----------- .../rules/fields_will_merge_spec.rb | 1 + 2 files changed, 24 insertions(+), 35 deletions(-) diff --git a/lib/graphql/static_validation/rules/fields_will_merge.rb b/lib/graphql/static_validation/rules/fields_will_merge.rb index f60423a61f..74bb78c810 100644 --- a/lib/graphql/static_validation/rules/fields_will_merge.rb +++ b/lib/graphql/static_validation/rules/fields_will_merge.rb @@ -14,8 +14,7 @@ def initialize(*) super @visited_fragments = {} @compared_fragments = {} - @selections_for_node = {} - @fragment_names_for_node = {} + @fields_and_fragments_from_node = {} end def on_operation_definition(node, _parent) @@ -33,8 +32,7 @@ def on_field(node, _parent) def conflicts_within_selection_set(node, parent_type) return if parent_type.nil? - fields = response_keys_in_selection(node, parent_type: parent_type) - fragment_names = find_fragment_names(node) + fields, fragment_names = fields_and_fragments_from_selection(node, parent_type: parent_type) # (A) Find find all conflicts "within" the fields of this selection set. find_conflicts_within(fields) @@ -86,11 +84,8 @@ def find_conflicts_between_fragments(fragment_name1, fragment_name2, mutually_ex return if fragment_type1.nil? || fragment_type2.nil? - fragment_fields1 = response_keys_in_selection(fragment1, parent_type: fragment_type1) - fragment_names1 = find_fragment_names(fragment1) - - fragment_fields2 = response_keys_in_selection(fragment2, parent_type: fragment_type2) - fragment_names2 = find_fragment_names(fragment2) + fragment_fields1, fragment_names1 = fields_and_fragments_from_selection(fragment1, parent_type: fragment_type1) + fragment_fields2, fragment_names2 = fields_and_fragments_from_selection(fragment2, parent_type: fragment_type2) # (F) First, find all conflicts between these two collections of fields # (not including any nested fragments). @@ -131,8 +126,7 @@ def find_conflicts_between_fields_and_fragment(fragment_name, fields, mutually_e fragment_type = context.schema.types[fragment.type.name] return if fragment_type.nil? - fragment_fields = response_keys_in_selection(fragment, parent_type: fragment_type) - fragment_fragment_names = find_fragment_names(fragment) + fragment_fields, fragment_fragment_names = fields_and_fragments_from_selection(fragment, parent_type: fragment_type) # (D) First find any conflicts between the provided collection of fields # and the collection of fields represented by the given fragment. @@ -166,6 +160,7 @@ def find_conflicts_within(response_keys) end def find_conflict(response_key, field1, field2, mutually_exclusive: false) + binding.pry parent_type1 = field1.parent_type parent_type2 = field2.parent_type @@ -201,10 +196,8 @@ def find_conflict(response_key, field1, field2, mutually_exclusive: false) def find_conflicts_between_sub_selection_sets(field1, field2, mutually_exclusive:) return if field1.definition.nil? || field2.definition.nil? - fields = response_keys_in_selection(field1.node, parent_type: field1.definition.type.unwrap) - fields2 = response_keys_in_selection(field2.node, parent_type: field2.definition.type.unwrap) - fragment_names = find_fragment_names(field1.node) - fragment_names2 = find_fragment_names(field2.node) + fields, fragment_names = fields_and_fragments_from_selection(field1.node, parent_type: field1.definition.type.unwrap) + fields2, fragment_names2 = fields_and_fragments_from_selection(field2.node, parent_type: field2.definition.type.unwrap) # (H) First, collect all conflicts between these two collections of field. find_conflicts_between(fields, fields2, mutually_exclusive: mutually_exclusive) @@ -261,34 +254,29 @@ def find_conflicts_between(response_keys, response_keys2, mutually_exclusive:) end end - def find_fields(selections, parent_type:) - fields = selections.map do |node| + def fields_and_fragments_from_selection(node, parent_type:) + @fields_and_fragments_from_node[node] ||= begin + fields, fragment_names = find_fields_and_fragments(node.selections, parent_type: parent_type) + response_keys = fields.group_by { |f| f.node.alias || f.node.name } + [response_keys, fragment_names] + end + end + + def find_fields_and_fragments(selections, parent_type:, fields: [], fragment_names: []) + selections.each do |node| case node when GraphQL::Language::Nodes::Field definition = context.schema.get_field(parent_type, node.name) - Field.new(node, definition, parent_type) + fields << Field.new(node, definition, parent_type) when GraphQL::Language::Nodes::InlineFragment fragment_type = node.type ? context.schema.types[node.type.name] : parent_type - find_fields(node.selections, parent_type: fragment_type) if fragment_type + find_fields_and_fragments(node.selections, parent_type: fragment_type, fields: fields, fragment_names: fragment_names) if fragment_type + when GraphQL::Language::Nodes::FragmentSpread + fragment_names << node.name end end - fields.compact.flatten - end - - def response_keys_in_selection(node, parent_type:) - @selections_for_node[node] ||= begin - fields = find_fields(node.selections, parent_type: parent_type) - fields.group_by { |f| f.node.alias || f.node.name } - end - end - - def find_fragment_names(node) - @fragment_names_for_node[node] ||= begin - node.selections - .select { |s| s.is_a?(GraphQL::Language::Nodes::FragmentSpread) } - .map(&:name) - end + [fields, fragment_names] end def possible_arguments(field1, field2) 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 0fb3afdac9..06cf8503d9 100644 --- a/spec/graphql/static_validation/rules/fields_will_merge_spec.rb +++ b/spec/graphql/static_validation/rules/fields_will_merge_spec.rb @@ -477,6 +477,7 @@ } |} + focus it "passes rule" do assert_equal [], errors end From 00792cd95cdd8dff5a7a7cb6fcff9641cb66b1de Mon Sep 17 00:00:00 2001 From: Marc-Andre Giroux Date: Fri, 31 Aug 2018 15:56:08 -0400 Subject: [PATCH 14/22] fix test --- .../rules/fields_will_merge.rb | 83 +++++++++++-------- 1 file changed, 47 insertions(+), 36 deletions(-) diff --git a/lib/graphql/static_validation/rules/fields_will_merge.rb b/lib/graphql/static_validation/rules/fields_will_merge.rb index 74bb78c810..62ecd766fb 100644 --- a/lib/graphql/static_validation/rules/fields_will_merge.rb +++ b/lib/graphql/static_validation/rules/fields_will_merge.rb @@ -9,6 +9,7 @@ module FieldsWillMerge # Original Algorithm: https://github.com/graphql/graphql-js/blob/master/src/validation/rules/OverlappingFieldsCanBeMerged.js NO_ARGS = {}.freeze Field = Struct.new(:node, :definition, :parent_type) + FragmentSpread = Struct.new(:name, :parent_type) def initialize(*) super @@ -32,35 +33,45 @@ def on_field(node, _parent) def conflicts_within_selection_set(node, parent_type) return if parent_type.nil? - fields, fragment_names = fields_and_fragments_from_selection(node, parent_type: parent_type) + fields, fragment_spreads = fields_and_fragments_from_selection(node, parent_type: parent_type) # (A) Find find all conflicts "within" the fields of this selection set. find_conflicts_within(fields) - fragment_names.each_with_index do |fragment_name, i| + fragment_spreads.each_with_index do |fragment_spread, i| + are_mutually_exclusive = fragment_spread.parent_type != parent_type && + parent_type.kind.object? && + fragment_spread.parent_type.kind.object? + # (B) Then find conflicts between these fields and those represented by # each spread fragment name found. find_conflicts_between_fields_and_fragment( - fragment_name, + fragment_spread, fields, - mutually_exclusive: false, + mutually_exclusive: are_mutually_exclusive, ) # (C) Then compare this fragment with all other fragments found in this # selection set to collect conflicts between fragments spread together. # This compares each item in the list of fragment names to every other # item in that same list (except for itself). - fragment_names[i + 1..-1].each do |fragment_name2| + fragment_spreads[i + 1..-1].each do |fragment_spread2| + are_mutually_exclusive = fragment_spread.parent_type != fragment_spread2.parent_type && + fragment_spread.parent_type.kind.object? && + fragment_spread2.parent_type.kind.object? + find_conflicts_between_fragments( - fragment_name, - fragment_name2, - mutually_exclusive: false, + fragment_spread, + fragment_spread2, + mutually_exclusive: are_mutually_exclusive, ) end end end - def find_conflicts_between_fragments(fragment_name1, fragment_name2, mutually_exclusive:) + def find_conflicts_between_fragments(fragment_spread1, fragment_spread2, mutually_exclusive:) + fragment_name1 = fragment_spread1.name + fragment_name2 = fragment_spread2.name return if fragment_name1 == fragment_name2 cache_key = compared_fragments_key( @@ -84,8 +95,8 @@ def find_conflicts_between_fragments(fragment_name1, fragment_name2, mutually_ex return if fragment_type1.nil? || fragment_type2.nil? - fragment_fields1, fragment_names1 = fields_and_fragments_from_selection(fragment1, parent_type: fragment_type1) - fragment_fields2, fragment_names2 = fields_and_fragments_from_selection(fragment2, parent_type: fragment_type2) + fragment_fields1, fragment_spreads1 = fields_and_fragments_from_selection(fragment1, parent_type: fragment_type1) + fragment_fields2, fragment_spreads2 = fields_and_fragments_from_selection(fragment2, parent_type: fragment_type2) # (F) First, find all conflicts between these two collections of fields # (not including any nested fragments). @@ -97,26 +108,27 @@ def find_conflicts_between_fragments(fragment_name1, fragment_name2, mutually_ex # (G) Then collect conflicts between the first fragment and any nested # fragments spread in the second fragment. - fragment_names2.each do |fragment_name| + fragment_spreads2.each do |fragment_spread| find_conflicts_between_fragments( - fragment_name1, - fragment_name, + fragment_spread1, + fragment_spread, mutually_exclusive: mutually_exclusive, ) end # (G) Then collect conflicts between the first fragment and any nested # fragments spread in the second fragment. - fragment_names1.each do |fragment_name| + fragment_spreads1.each do |fragment_spread| find_conflicts_between_fragments( - fragment_name2, - fragment_name, + fragment_spread2, + fragment_spread, mutually_exclusive: mutually_exclusive, ) end end - def find_conflicts_between_fields_and_fragment(fragment_name, fields, mutually_exclusive:) + def find_conflicts_between_fields_and_fragment(fragment_spread, fields, mutually_exclusive:) + fragment_name = fragment_spread.name return if @visited_fragments.key?(fragment_name) @visited_fragments[fragment_name] = true @@ -126,7 +138,7 @@ def find_conflicts_between_fields_and_fragment(fragment_name, fields, mutually_e fragment_type = context.schema.types[fragment.type.name] return if fragment_type.nil? - fragment_fields, fragment_fragment_names = fields_and_fragments_from_selection(fragment, parent_type: fragment_type) + fragment_fields, fragment_spreads = fields_and_fragments_from_selection(fragment, parent_type: fragment_type) # (D) First find any conflicts between the provided collection of fields # and the collection of fields represented by the given fragment. @@ -138,9 +150,9 @@ def find_conflicts_between_fields_and_fragment(fragment_name, fields, mutually_e # (E) Then collect any conflicts between the provided collection of fields # and any fragment names found in the given fragment. - fragment_fragment_names.each do |fragment_name| + fragment_spreads.each do |fragment_spread| find_conflicts_between_fields_and_fragment( - fragment_name, + fragment_spreads, fields, mutually_exclusive: mutually_exclusive, ) @@ -160,7 +172,6 @@ def find_conflicts_within(response_keys) end def find_conflict(response_key, field1, field2, mutually_exclusive: false) - binding.pry parent_type1 = field1.parent_type parent_type2 = field2.parent_type @@ -196,28 +207,28 @@ def find_conflict(response_key, field1, field2, mutually_exclusive: false) def find_conflicts_between_sub_selection_sets(field1, field2, mutually_exclusive:) return if field1.definition.nil? || field2.definition.nil? - fields, fragment_names = fields_and_fragments_from_selection(field1.node, parent_type: field1.definition.type.unwrap) - fields2, fragment_names2 = fields_and_fragments_from_selection(field2.node, parent_type: field2.definition.type.unwrap) + fields, fragment_spreads = fields_and_fragments_from_selection(field1.node, parent_type: field1.definition.type.unwrap) + fields2, fragment_spreads2 = fields_and_fragments_from_selection(field2.node, parent_type: field2.definition.type.unwrap) # (H) First, collect all conflicts between these two collections of field. find_conflicts_between(fields, fields2, mutually_exclusive: mutually_exclusive) # (I) Then collect conflicts between the first collection of fields and # those referenced by each fragment name associated with the second. - fragment_names2.each do |fragment_name| + fragment_spreads2.each do |fragment_spread| find_conflicts_between_fields_and_fragment( fields, - fragment_name, + fragment_spread, mutually_exclusive: mutually_exclusive, ) end # (I) Then collect conflicts between the second collection of fields and # those referenced by each fragment name associated with the first. - fragment_names.each do |fragment_name| + fragment_spreads.each do |fragment_spread| find_conflicts_between_fields_and_fragment( fields2, - fragment_name, + fragment_spread, mutually_exclusive: mutually_exclusive, ) end @@ -225,8 +236,8 @@ def find_conflicts_between_sub_selection_sets(field1, field2, mutually_exclusive # (J) Also collect conflicts between any fragment names by the first and # fragment names by the second. This compares each item in the first set of # names to each item in the second set of names. - fragment_names.each do |frag1| - fragment_names2.each do |frag2| + fragment_spreads.each do |frag1| + fragment_spread2.each do |frag2| find_conflicts_between_fragments( frag1, frag2, @@ -256,13 +267,13 @@ def find_conflicts_between(response_keys, response_keys2, mutually_exclusive:) def fields_and_fragments_from_selection(node, parent_type:) @fields_and_fragments_from_node[node] ||= begin - fields, fragment_names = find_fields_and_fragments(node.selections, parent_type: parent_type) + fields, fragment_spreads = find_fields_and_fragments(node.selections, parent_type: parent_type) response_keys = fields.group_by { |f| f.node.alias || f.node.name } - [response_keys, fragment_names] + [response_keys, fragment_spreads] end end - def find_fields_and_fragments(selections, parent_type:, fields: [], fragment_names: []) + def find_fields_and_fragments(selections, parent_type:, fields: [], fragment_spreads: []) selections.each do |node| case node when GraphQL::Language::Nodes::Field @@ -270,13 +281,13 @@ def find_fields_and_fragments(selections, parent_type:, fields: [], fragment_nam fields << Field.new(node, definition, parent_type) when GraphQL::Language::Nodes::InlineFragment fragment_type = node.type ? context.schema.types[node.type.name] : parent_type - find_fields_and_fragments(node.selections, parent_type: fragment_type, fields: fields, fragment_names: fragment_names) if fragment_type + find_fields_and_fragments(node.selections, parent_type: fragment_type, fields: fields, fragment_spreads: fragment_spreads) if fragment_type when GraphQL::Language::Nodes::FragmentSpread - fragment_names << node.name + fragment_spreads << FragmentSpread.new(node.name, parent_type) end end - [fields, fragment_names] + [fields, fragment_spreads] end def possible_arguments(field1, field2) From 48922ba35cda870f339b388ddc2ab67be4ca17c7 Mon Sep 17 00:00:00 2001 From: Marc-Andre Giroux Date: Fri, 31 Aug 2018 16:09:30 -0400 Subject: [PATCH 15/22] more edge cases --- .../rules/fields_will_merge.rb | 58 +++++++++++++------ .../rules/fields_will_merge_spec.rb | 30 +++++++++- 2 files changed, 69 insertions(+), 19 deletions(-) diff --git a/lib/graphql/static_validation/rules/fields_will_merge.rb b/lib/graphql/static_validation/rules/fields_will_merge.rb index 62ecd766fb..ceda1403f0 100644 --- a/lib/graphql/static_validation/rules/fields_will_merge.rb +++ b/lib/graphql/static_validation/rules/fields_will_merge.rb @@ -39,9 +39,10 @@ def conflicts_within_selection_set(node, parent_type) find_conflicts_within(fields) fragment_spreads.each_with_index do |fragment_spread, i| - are_mutually_exclusive = fragment_spread.parent_type != parent_type && - parent_type.kind.object? && - fragment_spread.parent_type.kind.object? + are_mutually_exclusive = mutually_exclusive?( + fragment_spread.parent_type, + parent_type + ) # (B) Then find conflicts between these fields and those represented by # each spread fragment name found. @@ -56,9 +57,10 @@ def conflicts_within_selection_set(node, parent_type) # This compares each item in the list of fragment names to every other # item in that same list (except for itself). fragment_spreads[i + 1..-1].each do |fragment_spread2| - are_mutually_exclusive = fragment_spread.parent_type != fragment_spread2.parent_type && - fragment_spread.parent_type.kind.object? && - fragment_spread2.parent_type.kind.object? + are_mutually_exclusive = mutually_exclusive?( + fragment_spread.parent_type, + fragment_spread2.parent_type + ) find_conflicts_between_fragments( fragment_spread, @@ -152,7 +154,7 @@ def find_conflicts_between_fields_and_fragment(fragment_spread, fields, mutually # and any fragment names found in the given fragment. fragment_spreads.each do |fragment_spread| find_conflicts_between_fields_and_fragment( - fragment_spreads, + fragment_spread, fields, mutually_exclusive: mutually_exclusive, ) @@ -179,9 +181,7 @@ def find_conflict(response_key, field1, field2, mutually_exclusive: false) node2 = field2.node are_mutually_exclusive = mutually_exclusive || - (parent_type1 != parent_type2 && - parent_type1.kind.object? && - parent_type2.kind.object?) + mutually_exclusive?(parent_type1, parent_type2) if !are_mutually_exclusive if node1.name != node2.name @@ -207,8 +207,11 @@ def find_conflict(response_key, field1, field2, mutually_exclusive: false) def find_conflicts_between_sub_selection_sets(field1, field2, mutually_exclusive:) return if field1.definition.nil? || field2.definition.nil? - fields, fragment_spreads = fields_and_fragments_from_selection(field1.node, parent_type: field1.definition.type.unwrap) - fields2, fragment_spreads2 = fields_and_fragments_from_selection(field2.node, parent_type: field2.definition.type.unwrap) + parent_type1 = field1.definition.type.unwrap + parent_type2 = field2.definition.type.unwrap + + fields, fragment_spreads = fields_and_fragments_from_selection(field1.node, parent_type: parent_type1) + fields2, fragment_spreads2 = fields_and_fragments_from_selection(field2.node, parent_type: parent_type2) # (H) First, collect all conflicts between these two collections of field. find_conflicts_between(fields, fields2, mutually_exclusive: mutually_exclusive) @@ -216,20 +219,30 @@ def find_conflicts_between_sub_selection_sets(field1, field2, mutually_exclusive # (I) Then collect conflicts between the first collection of fields and # those referenced by each fragment name associated with the second. fragment_spreads2.each do |fragment_spread| + fragment_is_mutually_exclusive = mutually_exclusive?( + parent_type1, + fragment_spread.parent_type + ) + find_conflicts_between_fields_and_fragment( - fields, fragment_spread, - mutually_exclusive: mutually_exclusive, + fields, + mutually_exclusive: mutually_exclusive || fragment_is_mutually_exclusive, ) end # (I) Then collect conflicts between the second collection of fields and # those referenced by each fragment name associated with the first. fragment_spreads.each do |fragment_spread| + fragment_is_mutually_exclusive = mutually_exclusive?( + parent_type2, + fragment_spread.parent_type + ) + find_conflicts_between_fields_and_fragment( - fields2, fragment_spread, - mutually_exclusive: mutually_exclusive, + fields2, + mutually_exclusive: mutually_exclusive || fragment_is_mutually_exclusive, ) end @@ -237,11 +250,16 @@ def find_conflicts_between_sub_selection_sets(field1, field2, mutually_exclusive # fragment names by the second. This compares each item in the first set of # names to each item in the second set of names. fragment_spreads.each do |frag1| - fragment_spread2.each do |frag2| + fragment_spreads2.each do |frag2| + fragments_are_mutually_exclusive = mutually_exclusive?( + frag1.parent_type, + frag2.parent_type + ) + find_conflicts_between_fragments( frag1, frag2, - mutually_exclusive: mutually_exclusive, + mutually_exclusive: mutually_exclusive || fragments_are_mutually_exclusive, ) end end @@ -317,6 +335,10 @@ def compared_fragments_key(frag1, frag2, exclusive) # "exclusive" since the result may change depending on the parent_type "#{[frag1, frag2].sort.join('-')}-#{exclusive}" end + + def mutually_exclusive?(type1, type2) + type1 != type2 && type1.kind.object? && type2.kind.object? + end end end end 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 06cf8503d9..851894556c 100644 --- a/spec/graphql/static_validation/rules/fields_will_merge_spec.rb +++ b/spec/graphql/static_validation/rules/fields_will_merge_spec.rb @@ -477,7 +477,35 @@ } |} - focus + it "passes rule" do + assert_equal [], errors + end + end + + describe "allows different args where no conflict is possible deep" do + let(:query_string) {%| + { + pet { + ... on Dog { + ...X + } + } + pet { + ... on Cat { + ...Y + } + } + } + + fragment X on Pet { + name(surname: true) + } + + fragment Y on Pet { + name + } + |} + it "passes rule" do assert_equal [], errors end From 980f90e4fedbc5703f1f51e5290a75375a771cac Mon Sep 17 00:00:00 2001 From: Marc-Andre Giroux Date: Wed, 5 Sep 2018 14:02:30 -0400 Subject: [PATCH 16/22] failing test for yet another case --- .../rules/fields_will_merge_spec.rb | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) 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 851894556c..de1aea5ea6 100644 --- a/spec/graphql/static_validation/rules/fields_will_merge_spec.rb +++ b/spec/graphql/static_validation/rules/fields_will_merge_spec.rb @@ -480,6 +480,27 @@ it "passes rule" do assert_equal [], errors end + + describe "allows different args where no conflict is possible" do + let(:query_string) {%| + { + pet { + ... on Dog { + ... on Pet { + name + } + } + ... on Cat { + name(surname: true) + } + } + } + |} + + it "passes rule" do + assert_equal [], errors + end + end end describe "allows different args where no conflict is possible deep" do From 1cdd8d1f15fa1cb5a306e3fcfcc47e1d8e250d85 Mon Sep 17 00:00:00 2001 From: Marc-Andre Giroux Date: Wed, 5 Sep 2018 15:39:29 -0400 Subject: [PATCH 17/22] track all parents --- .../rules/fields_will_merge.rb | 72 +++++++++++-------- .../rules/fields_will_merge_spec.rb | 1 + 2 files changed, 43 insertions(+), 30 deletions(-) diff --git a/lib/graphql/static_validation/rules/fields_will_merge.rb b/lib/graphql/static_validation/rules/fields_will_merge.rb index ceda1403f0..2ac40e6551 100644 --- a/lib/graphql/static_validation/rules/fields_will_merge.rb +++ b/lib/graphql/static_validation/rules/fields_will_merge.rb @@ -1,4 +1,4 @@ -# frozen_string_literal: true + # frozen_string_literal: true module GraphQL module StaticValidation module FieldsWillMerge @@ -8,8 +8,8 @@ module FieldsWillMerge # # Original Algorithm: https://github.com/graphql/graphql-js/blob/master/src/validation/rules/OverlappingFieldsCanBeMerged.js NO_ARGS = {}.freeze - Field = Struct.new(:node, :definition, :parent_type) - FragmentSpread = Struct.new(:name, :parent_type) + Field = Struct.new(:node, :definition, :parents) + FragmentSpread = Struct.new(:name, :parents) def initialize(*) super @@ -33,14 +33,14 @@ def on_field(node, _parent) def conflicts_within_selection_set(node, parent_type) return if parent_type.nil? - fields, fragment_spreads = fields_and_fragments_from_selection(node, parent_type: parent_type) + fields, fragment_spreads = fields_and_fragments_from_selection(node, parents: [parent_type]) # (A) Find find all conflicts "within" the fields of this selection set. find_conflicts_within(fields) fragment_spreads.each_with_index do |fragment_spread, i| are_mutually_exclusive = mutually_exclusive?( - fragment_spread.parent_type, + fragment_spread.parents.last, parent_type ) @@ -58,8 +58,8 @@ def conflicts_within_selection_set(node, parent_type) # item in that same list (except for itself). fragment_spreads[i + 1..-1].each do |fragment_spread2| are_mutually_exclusive = mutually_exclusive?( - fragment_spread.parent_type, - fragment_spread2.parent_type + fragment_spread.parents.last, + fragment_spread2.parents.last ) find_conflicts_between_fragments( @@ -97,8 +97,14 @@ def find_conflicts_between_fragments(fragment_spread1, fragment_spread2, mutuall return if fragment_type1.nil? || fragment_type2.nil? - fragment_fields1, fragment_spreads1 = fields_and_fragments_from_selection(fragment1, parent_type: fragment_type1) - fragment_fields2, fragment_spreads2 = fields_and_fragments_from_selection(fragment2, parent_type: fragment_type2) + fragment_fields1, fragment_spreads1 = fields_and_fragments_from_selection( + fragment1, + parents: [*fragment_spread1.parents, fragment_type1] + ) + fragment_fields2, fragment_spreads2 = fields_and_fragments_from_selection( + fragment2, + parents: [*fragment_spread2, fragment_type2] + ) # (F) First, find all conflicts between these two collections of fields # (not including any nested fragments). @@ -140,7 +146,7 @@ def find_conflicts_between_fields_and_fragment(fragment_spread, fields, mutually fragment_type = context.schema.types[fragment.type.name] return if fragment_type.nil? - fragment_fields, fragment_spreads = fields_and_fragments_from_selection(fragment, parent_type: fragment_type) + fragment_fields, fragment_spreads = fields_and_fragments_from_selection(fragment, parents: [*fragment_spread.parents, fragment_type]) # (D) First find any conflicts between the provided collection of fields # and the collection of fields represented by the given fragment. @@ -174,8 +180,8 @@ def find_conflicts_within(response_keys) end def find_conflict(response_key, field1, field2, mutually_exclusive: false) - parent_type1 = field1.parent_type - parent_type2 = field2.parent_type + parent_type1 = field1.parents.last + parent_type2 = field2.parents.last node1 = field1.node node2 = field2.node @@ -207,11 +213,17 @@ def find_conflict(response_key, field1, field2, mutually_exclusive: false) def find_conflicts_between_sub_selection_sets(field1, field2, mutually_exclusive:) return if field1.definition.nil? || field2.definition.nil? - parent_type1 = field1.definition.type.unwrap - parent_type2 = field2.definition.type.unwrap + return_type1 = field1.definition.type.unwrap + return_type2 = field2.definition.type.unwrap - fields, fragment_spreads = fields_and_fragments_from_selection(field1.node, parent_type: parent_type1) - fields2, fragment_spreads2 = fields_and_fragments_from_selection(field2.node, parent_type: parent_type2) + fields, fragment_spreads = fields_and_fragments_from_selection( + field1.node, + parents: [*field1.parents, return_type1]) + + fields2, fragment_spreads2 = fields_and_fragments_from_selection( + field2.node, + parents: [*field2.parents, return_type2] + ) # (H) First, collect all conflicts between these two collections of field. find_conflicts_between(fields, fields2, mutually_exclusive: mutually_exclusive) @@ -220,8 +232,8 @@ def find_conflicts_between_sub_selection_sets(field1, field2, mutually_exclusive # those referenced by each fragment name associated with the second. fragment_spreads2.each do |fragment_spread| fragment_is_mutually_exclusive = mutually_exclusive?( - parent_type1, - fragment_spread.parent_type + return_type1, + fragment_spread.parents.last ) find_conflicts_between_fields_and_fragment( @@ -235,8 +247,8 @@ def find_conflicts_between_sub_selection_sets(field1, field2, mutually_exclusive # those referenced by each fragment name associated with the first. fragment_spreads.each do |fragment_spread| fragment_is_mutually_exclusive = mutually_exclusive?( - parent_type2, - fragment_spread.parent_type + return_type2, + fragment_spread.parents.last ) find_conflicts_between_fields_and_fragment( @@ -252,8 +264,8 @@ def find_conflicts_between_sub_selection_sets(field1, field2, mutually_exclusive fragment_spreads.each do |frag1| fragment_spreads2.each do |frag2| fragments_are_mutually_exclusive = mutually_exclusive?( - frag1.parent_type, - frag2.parent_type + frag1.parents.last, + frag2.parents.last ) find_conflicts_between_fragments( @@ -283,25 +295,25 @@ def find_conflicts_between(response_keys, response_keys2, mutually_exclusive:) end end - def fields_and_fragments_from_selection(node, parent_type:) + def fields_and_fragments_from_selection(node, parents:) @fields_and_fragments_from_node[node] ||= begin - fields, fragment_spreads = find_fields_and_fragments(node.selections, parent_type: parent_type) + fields, fragment_spreads = find_fields_and_fragments(node.selections, parents: parents) response_keys = fields.group_by { |f| f.node.alias || f.node.name } [response_keys, fragment_spreads] end end - def find_fields_and_fragments(selections, parent_type:, fields: [], fragment_spreads: []) + def find_fields_and_fragments(selections, parents:, fields: [], fragment_spreads: []) selections.each do |node| case node when GraphQL::Language::Nodes::Field - definition = context.schema.get_field(parent_type, node.name) - fields << Field.new(node, definition, parent_type) + definition = context.schema.get_field(parents.last, node.name) + fields << Field.new(node, definition, parents) when GraphQL::Language::Nodes::InlineFragment - fragment_type = node.type ? context.schema.types[node.type.name] : parent_type - find_fields_and_fragments(node.selections, parent_type: fragment_type, fields: fields, fragment_spreads: fragment_spreads) if fragment_type + fragment_type = node.type ? context.schema.types[node.type.name] : parents.last + find_fields_and_fragments(node.selections, parents: [*parents, fragment_type], fields: fields, fragment_spreads: fragment_spreads) if fragment_type when GraphQL::Language::Nodes::FragmentSpread - fragment_spreads << FragmentSpread.new(node.name, parent_type) + fragment_spreads << FragmentSpread.new(node.name, parents) end end 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 de1aea5ea6..b8e2465f7a 100644 --- a/spec/graphql/static_validation/rules/fields_will_merge_spec.rb +++ b/spec/graphql/static_validation/rules/fields_will_merge_spec.rb @@ -527,6 +527,7 @@ } |} + focus it "passes rule" do assert_equal [], errors end From 94b621e6917615ae1949e7999b3cd97fdc2a33de Mon Sep 17 00:00:00 2001 From: Marc-Andre Giroux Date: Wed, 5 Sep 2018 16:48:40 -0400 Subject: [PATCH 18/22] exclusivity logic, removed caching for now --- .../rules/fields_will_merge.rb | 70 ++++++++++--------- .../rules/fields_will_merge_spec.rb | 1 - 2 files changed, 37 insertions(+), 34 deletions(-) diff --git a/lib/graphql/static_validation/rules/fields_will_merge.rb b/lib/graphql/static_validation/rules/fields_will_merge.rb index 2ac40e6551..61c6531e1f 100644 --- a/lib/graphql/static_validation/rules/fields_will_merge.rb +++ b/lib/graphql/static_validation/rules/fields_will_merge.rb @@ -40,8 +40,8 @@ def conflicts_within_selection_set(node, parent_type) fragment_spreads.each_with_index do |fragment_spread, i| are_mutually_exclusive = mutually_exclusive?( - fragment_spread.parents.last, - parent_type + fragment_spread.parents, + [parent_type] ) # (B) Then find conflicts between these fields and those represented by @@ -58,8 +58,8 @@ def conflicts_within_selection_set(node, parent_type) # item in that same list (except for itself). fragment_spreads[i + 1..-1].each do |fragment_spread2| are_mutually_exclusive = mutually_exclusive?( - fragment_spread.parents.last, - fragment_spread2.parents.last + fragment_spread.parents, + fragment_spread2.parents ) find_conflicts_between_fragments( @@ -103,7 +103,7 @@ def find_conflicts_between_fragments(fragment_spread1, fragment_spread2, mutuall ) fragment_fields2, fragment_spreads2 = fields_and_fragments_from_selection( fragment2, - parents: [*fragment_spread2, fragment_type2] + parents: [*fragment_spread2.parents, fragment_type2] ) # (F) First, find all conflicts between these two collections of fields @@ -187,7 +187,7 @@ def find_conflict(response_key, field1, field2, mutually_exclusive: false) node2 = field2.node are_mutually_exclusive = mutually_exclusive || - mutually_exclusive?(parent_type1, parent_type2) + mutually_exclusive?(field1.parents, field2.parents) if !are_mutually_exclusive if node1.name != node2.name @@ -215,14 +215,17 @@ def find_conflicts_between_sub_selection_sets(field1, field2, mutually_exclusive return_type1 = field1.definition.type.unwrap return_type2 = field2.definition.type.unwrap + parents1 = [*field1.parents, return_type1] + parents2 = [*field2.parents, return_type2] fields, fragment_spreads = fields_and_fragments_from_selection( field1.node, - parents: [*field1.parents, return_type1]) + parents: parents1 + ) fields2, fragment_spreads2 = fields_and_fragments_from_selection( field2.node, - parents: [*field2.parents, return_type2] + parents: parents2 ) # (H) First, collect all conflicts between these two collections of field. @@ -231,30 +234,20 @@ def find_conflicts_between_sub_selection_sets(field1, field2, mutually_exclusive # (I) Then collect conflicts between the first collection of fields and # those referenced by each fragment name associated with the second. fragment_spreads2.each do |fragment_spread| - fragment_is_mutually_exclusive = mutually_exclusive?( - return_type1, - fragment_spread.parents.last - ) - find_conflicts_between_fields_and_fragment( fragment_spread, fields, - mutually_exclusive: mutually_exclusive || fragment_is_mutually_exclusive, + mutually_exclusive: mutually_exclusive, ) end # (I) Then collect conflicts between the second collection of fields and # those referenced by each fragment name associated with the first. fragment_spreads.each do |fragment_spread| - fragment_is_mutually_exclusive = mutually_exclusive?( - return_type2, - fragment_spread.parents.last - ) - find_conflicts_between_fields_and_fragment( fragment_spread, fields2, - mutually_exclusive: mutually_exclusive || fragment_is_mutually_exclusive, + mutually_exclusive: mutually_exclusive, ) end @@ -263,15 +256,10 @@ def find_conflicts_between_sub_selection_sets(field1, field2, mutually_exclusive # names to each item in the second set of names. fragment_spreads.each do |frag1| fragment_spreads2.each do |frag2| - fragments_are_mutually_exclusive = mutually_exclusive?( - frag1.parents.last, - frag2.parents.last - ) - find_conflicts_between_fragments( frag1, frag2, - mutually_exclusive: mutually_exclusive || fragments_are_mutually_exclusive, + mutually_exclusive: mutually_exclusive ) end end @@ -296,11 +284,9 @@ def find_conflicts_between(response_keys, response_keys2, mutually_exclusive:) end def fields_and_fragments_from_selection(node, parents:) - @fields_and_fragments_from_node[node] ||= begin - fields, fragment_spreads = find_fields_and_fragments(node.selections, parents: parents) - response_keys = fields.group_by { |f| f.node.alias || f.node.name } - [response_keys, fragment_spreads] - end + fields, fragment_spreads = find_fields_and_fragments(node.selections, parents: parents) + response_keys = fields.group_by { |f| f.node.alias || f.node.name } + [response_keys, fragment_spreads] end def find_fields_and_fragments(selections, parents:, fields: [], fragment_spreads: []) @@ -348,8 +334,26 @@ def compared_fragments_key(frag1, frag2, exclusive) "#{[frag1, frag2].sort.join('-')}-#{exclusive}" end - def mutually_exclusive?(type1, type2) - type1 != type2 && type1.kind.object? && type2.kind.object? + # Given two list of parents, find out if they are mutually exclusive + def mutually_exclusive?(parents1, parents2) + i = 0 + j = 0 + + while i <= parents1.size - 1 && j <= parents2.size - 1 do + type1 = parents1[i] + type2 = parents2[j] + + # If the types we're comparing are both different object types, + # they have to be mutually exclusive. + if type1 != type2 && type1.kind.object? && type2.kind.object? + return true + end + + i = i + 1 if i < parents1.size - 1 + j = j + 1 if j < parents2.size - 1 + end + + false end end end 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 b8e2465f7a..de1aea5ea6 100644 --- a/spec/graphql/static_validation/rules/fields_will_merge_spec.rb +++ b/spec/graphql/static_validation/rules/fields_will_merge_spec.rb @@ -527,7 +527,6 @@ } |} - focus it "passes rule" do assert_equal [], errors end From 81a9de2a7d3f2aaad60e0fb245d92b49fabb7735 Mon Sep 17 00:00:00 2001 From: Marc-Andre Giroux Date: Wed, 5 Sep 2018 16:52:50 -0400 Subject: [PATCH 19/22] lets not have infinite loops --- lib/graphql/static_validation/rules/fields_will_merge.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/graphql/static_validation/rules/fields_will_merge.rb b/lib/graphql/static_validation/rules/fields_will_merge.rb index 61c6531e1f..0211b487c4 100644 --- a/lib/graphql/static_validation/rules/fields_will_merge.rb +++ b/lib/graphql/static_validation/rules/fields_will_merge.rb @@ -349,8 +349,8 @@ def mutually_exclusive?(parents1, parents2) return true end - i = i + 1 if i < parents1.size - 1 - j = j + 1 if j < parents2.size - 1 + i = i + 1 if i <= parents1.size - 1 + j = j + 1 if j <= parents2.size - 1 end false From 48bb6bac235b9173c5a2f9b5e594c600a059c3f6 Mon Sep 17 00:00:00 2001 From: Marc-Andre Giroux Date: Wed, 5 Sep 2018 17:15:06 -0400 Subject: [PATCH 20/22] unused vars --- .../rules/fields_will_merge.rb | 3 - .../rules/fields_will_merge_spec.rb | 56 ++++++++++++++++++- 2 files changed, 54 insertions(+), 5 deletions(-) diff --git a/lib/graphql/static_validation/rules/fields_will_merge.rb b/lib/graphql/static_validation/rules/fields_will_merge.rb index 0211b487c4..089cf00f2c 100644 --- a/lib/graphql/static_validation/rules/fields_will_merge.rb +++ b/lib/graphql/static_validation/rules/fields_will_merge.rb @@ -180,9 +180,6 @@ def find_conflicts_within(response_keys) end def find_conflict(response_key, field1, field2, mutually_exclusive: false) - parent_type1 = field1.parents.last - parent_type2 = field2.parents.last - node1 = field1.node node2 = field2.node 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 de1aea5ea6..e295af0a6b 100644 --- a/spec/graphql/static_validation/rules/fields_will_merge_spec.rb +++ b/spec/graphql/static_validation/rules/fields_will_merge_spec.rb @@ -39,13 +39,18 @@ species: PetSpecies! } + interface Mammal { + name(surname: Boolean = false): String! + nickname: String + } + interface Pet { name(surname: Boolean = false): String! nickname: String toys: [Toy!]! } - type Dog implements Pet { + type Dog implements Pet & Mammal { name(surname: Boolean = false): String! nickname: String doesKnowCommand(dogCommand: PetCommand): Boolean! @@ -53,7 +58,7 @@ toys: [Toy!]! } - type Cat implements Pet { + type Cat implements Pet & Mammal { name(surname: Boolean = false): String! nickname: String doesKnowCommand(catCommand: PetCommand): Boolean! @@ -455,6 +460,53 @@ end end + describe "same aliases not allowed on different interfaces" do + let(:query_string) {%| + { + pet { + ... on Pet { + name + } + ... on Mammal { + name: nickname + } + } + } + |} + + it "fails rule" do + assert_equal [ + "Field 'name' has a field conflict: name or nickname?", + ], error_messages + end + end + + describe "same aliases allowed on different parent interfaces and different concrete types" do + let(:query_string) {%| + { + pet { + ... on Pet { + ...X + } + ... on Mammal { + ...Y + } + } + } + + fragment X on Dog { + name + } + fragment Y on Cat { + name: nickname + } + |} + + it "passes rule" do + assert_equal [], errors + end + end + describe "allows different args where no conflict is possible" do let(:query_string) {%| { From a28ec6c883774bc6dd14415dcc98fff3187f6a09 Mon Sep 17 00:00:00 2001 From: Robert Mosolgo Date: Thu, 13 Sep 2018 16:33:57 -0400 Subject: [PATCH 21/22] Remove Untitled --- Untitled | 12 ------------ 1 file changed, 12 deletions(-) delete mode 100644 Untitled diff --git a/Untitled b/Untitled deleted file mode 100644 index 2b7f46ed2f..0000000000 --- a/Untitled +++ /dev/null @@ -1,12 +0,0 @@ -class Analyzer < Visitor - def initialize - end -end - -class MyAnalyzer < Analyzer - def initial_state - end - - def on_field(node, parent, defn) - end -def From 345ecd42f4ba23b5c1453488f65a64b1511d0852 Mon Sep 17 00:00:00 2001 From: Robert Mosolgo Date: Thu, 13 Sep 2018 17:01:43 -0400 Subject: [PATCH 22/22] Remove unused ivar; remove focus --- lib/graphql/static_validation/rules/fields_will_merge.rb | 1 - .../static_validation/rules/fields_will_merge_spec.rb | 5 ----- 2 files changed, 6 deletions(-) diff --git a/lib/graphql/static_validation/rules/fields_will_merge.rb b/lib/graphql/static_validation/rules/fields_will_merge.rb index 089cf00f2c..a2d72f57ca 100644 --- a/lib/graphql/static_validation/rules/fields_will_merge.rb +++ b/lib/graphql/static_validation/rules/fields_will_merge.rb @@ -15,7 +15,6 @@ def initialize(*) super @visited_fragments = {} @compared_fragments = {} - @fields_and_fragments_from_node = {} end def on_operation_definition(node, _parent) 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 e295af0a6b..b15331dab1 100644 --- a/spec/graphql/static_validation/rules/fields_will_merge_spec.rb +++ b/spec/graphql/static_validation/rules/fields_will_merge_spec.rb @@ -251,7 +251,6 @@ } |} - focus it "fails rule" do assert_equal ["Field 'fido' has a field conflict: name or nickname?"], error_messages end @@ -282,7 +281,6 @@ } |} - focus it "fails rule" do assert_equal [%q(Field 'doesKnowCommand' has an argument conflict: {} or {dogCommand:"HEEL"}?)], error_messages end @@ -298,7 +296,6 @@ } |} - focus it "fails rule" do assert_equal [%q(Field 'doesKnowCommand' has an argument conflict: {dogCommand:"SIT"} or {}?)], error_messages end @@ -314,7 +311,6 @@ } |} - focus it "fails rule" do assert_equal [%q(Field 'doesKnowCommand' has an argument conflict: {dogCommand:"SIT"} or {dogCommand:"HEEL"}?)], error_messages end @@ -330,7 +326,6 @@ } |} - focus it "fails rule" do assert_equal [%q(Field 'image' has an argument conflict: {maxWidth:"10"} or {maxWidth:"20"}?)], error_messages end