Fix performance degradation on handling conflict fields (#212)

Contributed by Kohei Morita

Replicates graphql/graphql-js@f94b511

Author: mrtc0

src/graphql/validation/rules/overlapping_fields_can_be_merged.py

@@ -10,9 +10,8 @@
     FragmentDefinitionNode,
     FragmentSpreadNode,
     InlineFragmentNode,
-    ObjectFieldNode,
-    ObjectValueNode,
     SelectionSetNode,
+    ValueNode,
     print_ast,
 )
 from ...type import (
@@ -558,7 +557,7 @@ def find_conflict(
             )
 
         # Two field calls must have the same arguments.
-        if stringify_arguments(node1) != stringify_arguments(node2):
+        if not same_arguments(node1, node2):
             return (response_name, "they have differing arguments"), [node1], [node2]
 
     directives1 = node1.directives
@@ -598,14 +597,34 @@ def find_conflict(
     return None  # no conflict
 
 
-def stringify_arguments(field_node: Union[FieldNode, DirectiveNode]) -> str:
-    input_object_with_args = ObjectValueNode(
-        fields=tuple(
-            ObjectFieldNode(name=arg_node.name, value=arg_node.value)
-            for arg_node in field_node.arguments
-        )
-    )
-    return print_ast(sort_value_node(input_object_with_args))
+def same_arguments(
+    node1: Union[FieldNode, DirectiveNode], node2: Union[FieldNode, DirectiveNode]
+) -> bool:
+    args1 = node1.arguments
+    args2 = node2.arguments
+
+    if args1 is None or len(args1) == 0:
+        return args2 is None or len(args2) == 0
+
+    if args2 is None or len(args2) == 0:
+        return False
+
+    if len(args1) != len(args2):
+        return False
+
+    values2 = {arg.name.value: arg.value for arg in args2}
+
+    for arg1 in args1:
+        value1 = arg1.value
+        value2 = values2.get(arg1.name.value)
+        if value2 is None or stringify_value(value1) != stringify_value(value2):
+            return False
+
+    return True
+
+
+def stringify_value(value: ValueNode) -> str:
+    return print_ast(sort_value_node(value))
 
 
 def get_stream_directive(
@@ -627,7 +646,7 @@ def same_streams(
         return True
     if stream1 and stream2:
         # check if both fields have equivalent streams
-        return stringify_arguments(stream1) == stringify_arguments(stream2)
+        return same_arguments(stream1, stream2)
     # fields have a mix of stream and no stream
     return False
 

tests/benchmarks/test_repeated_fields_benchmark.py

@@ -0,0 +1,27 @@
+from graphql import (
+    GraphQLField,
+    GraphQLObjectType,
+    GraphQLSchema,
+    GraphQLString,
+    graphql_sync,
+)
+
+
+schema = GraphQLSchema(
+    query=GraphQLObjectType(
+        name="Query",
+        fields={
+            "hello": GraphQLField(
+                GraphQLString,
+                resolve=lambda obj, info: "world",
+            )
+        },
+    )
+)
+source = "query {{ {fields} }}".format(fields="hello " * 250)
+
+
+def test_many_repeated_fields(benchmark):
+    print(source)
+    result = benchmark(lambda: graphql_sync(schema, source))
+    assert not result.errors

tests/validation/test_overlapping_fields_can_be_merged.py

@@ -166,6 +166,23 @@ def different_stream_directive_second_missing_args():
             ],
         )
 
+    def different_stream_directive_extra_argument():
+        assert_errors(
+            """
+            fragment conflictingArgs on Dog {
+              name @stream(label: "streamLabel", initialCount: 1)
+              name @stream(label: "streamLabel", initialCount: 1, extraArg: true)
+            }""",
+            [
+                {
+                    "message": "Fields 'name' conflict because they have differing"
+                    " stream directives. Use different aliases on the fields"
+                    " to fetch both if this was intentional.",
+                    "locations": [(3, 15), (4, 15)],
+                }
+            ],
+        )
+
     def mix_of_stream_and_no_stream():
         assert_errors(
             """