diff --git a/lib/graphql/static_validation/rules/fields_will_merge.rb b/lib/graphql/static_validation/rules/fields_will_merge.rb index af9e9df3c9..a2d72f57ca 100644 --- a/lib/graphql/static_validation/rules/fields_will_merge.rb +++ b/lib/graphql/static_validation/rules/fields_will_merge.rb @@ -1,49 +1,355 @@ -# frozen_string_literal: true + # frozen_string_literal: true module GraphQL module StaticValidation module FieldsWillMerge - # Special handling for fields without arguments + # 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, :parents) + FragmentSpread = Struct.new(:name, :parents) def initialize(*) super + @visited_fragments = {} + @compared_fragments = {} + end + + 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 + + private + + def conflicts_within_selection_set(node, parent_type) + return if parent_type.nil? + + 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.parents, + [parent_type] + ) + + # (B) Then find conflicts between these fields and those represented by + # each spread fragment name found. + find_conflicts_between_fields_and_fragment( + fragment_spread, + fields, + 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_spreads[i + 1..-1].each do |fragment_spread2| + are_mutually_exclusive = mutually_exclusive?( + fragment_spread.parents, + fragment_spread2.parents + ) + + find_conflicts_between_fragments( + fragment_spread, + fragment_spread2, + mutually_exclusive: are_mutually_exclusive, + ) + end + end + end + + 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( + 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] - context.each_irep_node do |node| - if node.ast_nodes.size > 1 - defn_names = Set.new(node.ast_nodes.map(&:name)) + return if fragment1.nil? || fragment2.nil? - # 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) + 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, 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.parents, fragment_type2] + ) + + # (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_spreads2.each do |fragment_spread| + find_conflicts_between_fragments( + 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_spreads1.each do |fragment_spread| + find_conflicts_between_fragments( + fragment_spread2, + fragment_spread, + mutually_exclusive: mutually_exclusive, + ) + end + end + + 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 + + fragment = context.fragments[fragment_name] + return if fragment.nil? + + fragment_type = context.schema.types[fragment.type.name] + return if fragment_type.nil? + + 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. + 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_spreads.each do |fragment_spread| + find_conflicts_between_fields_and_fragment( + fragment_spread, + fields, + mutually_exclusive: mutually_exclusive, + ) + end + end + + 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 + find_conflict(key, fields[i], fields[j]) end + end + end + end - # 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 - else - NO_ARGS + def find_conflict(response_key, field1, field2, mutually_exclusive: false) + node1 = field1.node + node2 = field2.node + + are_mutually_exclusive = mutually_exclusive || + mutually_exclusive?(field1.parents, field2.parents) + + if !are_mutually_exclusive + if node1.name != 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 + + 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 + + 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:) + return if field1.definition.nil? || field2.definition.nil? + + 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: parents1 + ) + + fields2, fragment_spreads2 = fields_and_fragments_from_selection( + field2.node, + parents: parents2 + ) + + # (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_spreads2.each do |fragment_spread| + find_conflicts_between_fields_and_fragment( + fragment_spread, + fields, + 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| + find_conflicts_between_fields_and_fragment( + fragment_spread, + fields2, + 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_spreads.each do |frag1| + fragment_spreads2.each do |frag2| + find_conflicts_between_fragments( + frag1, + frag2, + mutually_exclusive: mutually_exclusive + ) + end + end + end + + def find_conflicts_between(response_keys, response_keys2, mutually_exclusive:) + response_keys.each do |key, fields| + fields2 = response_keys2[key] + if fields2 + fields.each do |field| + fields2.each do |field2| + find_conflict( + key, + field, + field2, + mutually_exclusive: mutually_exclusive, + ) end end - args.uniq! + end + end + end - 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) + def fields_and_fragments_from_selection(node, parents:) + 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: []) + selections.each do |node| + case node + when GraphQL::Language::Nodes::Field + 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] : 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, parents) + end + end + + [fields, fragment_spreads] + 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 + GraphQL::Language.serialize(arg_value) + end + memo end + else + NO_ARGS + 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 + + # 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/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..b15331dab1 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! @@ -348,12 +353,13 @@ 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 + 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) {%| { @@ -448,6 +455,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) {%| { @@ -473,6 +527,56 @@ 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 + 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 end describe "return types must be unambiguous" do @@ -599,7 +703,7 @@ } } fragment X on SomeBox { - scalar: deepBox { unreleatedField } + scalar: deepBox { unrelatedField } } fragment Y on SomeBox { scalar: unrelatedField