Schema renaming: Interface implementation suppression

graphql_compiler/schema_transformation/rename_schema.py

@@ -240,16 +240,40 @@ def rename_schema(
     schema = build_ast_schema(schema_ast)
     query_type = get_query_type_name(schema)
 
+    # If we suppress a type, any interfaces it implements must be made unqueryable as described in
+    # the module-level comment.
+    # Unfortunately, this must be done outside a schema visitor object because the interfaces field
+    # of ObjectTypeDefinitionNodes and InterfaceTypeDefinitionNodes is a list of NameNodes, which
+    # just indicates the name of the interface rather than give the node itself. This makes it
+    # impossible to find all ancestors when only given such a type definition node. Instead, it's
+    # necessary to use the definitions field of schema_ast to traverse the inheritance hierarchy and
+    # find which interfaces need to be made unqueryable.
+    # See discussion here: https://github.com/graphql-python/graphql-core/issues/124
+    interfaces_to_make_unqueryable = set()
+    interface_and_object_type_name_to_definition_node_map = {
+        node.name.value: node
+        for node in schema_ast.definitions
+        if isinstance(node, (ObjectTypeDefinitionNode, InterfaceTypeDefinitionNode))
+    }
+    for type_name in type_renamings:
+        type_not_suppressed = type_renamings[type_name] is not None
+        if type_not_suppressed:
+            continue
+        if type_name in interface_and_object_type_name_to_definition_node_map:
+            type_node = interface_and_object_type_name_to_definition_node_map[type_name]
+            interfaces_to_make_unqueryable.update(
+                set(
+                    _recursively_get_ancestor_interface_names(
+                        schema_ast, type_node, interface_and_object_type_name_to_definition_node_map
+                    )
+                )
+            )
+
     _ensure_no_unsupported_suppressions(schema_ast, type_renamings)
 
     # Rename types, interfaces, enums, unions and suppress types, unions
-    (
-        schema_ast,
-        reverse_name_map,
-        reverse_field_name_map,
-        interfaces_to_make_unqueryable,
-    ) = _rename_and_suppress_types_and_fields(
-        schema_ast, type_renamings, field_renamings, query_type
+    (schema_ast, reverse_name_map, reverse_field_name_map,) = _rename_and_suppress_types_and_fields(
+        schema_ast, type_renamings, field_renamings, query_type, interfaces_to_make_unqueryable
     )
 
     schema_ast = _rename_and_suppress_query_type_fields(
@@ -263,93 +287,6 @@ def rename_schema(
     )
 
 
-def _validate_renamings(
-    schema_ast: DocumentNode,
-    type_renamings: Mapping[str, Optional[str]],
-    field_renamings: Mapping[str, Mapping[str, Set[str]]],
-    query_type: str,
-) -> None:
-    """Validate the type_renamings argument before attempting to rename the schema.
-
-    Check for fields with suppressed types or unions whose members were all suppressed. Also,
-    confirm type_renamings contains no suppressions for enums or interfaces because they haven't
-    been implemented yet.
-
-    The input AST will not be modified.
-
-    Args:
-        schema_ast: represents a valid schema that does not contain extensions, input object
-                    definitions, mutations, or subscriptions, whose fields of the query type share
-                    the same name as the types they query. Not modified by this function
-        type_renamings: maps original type name to renamed name or None (for type suppression). A
-                        type named "Foo" will be unchanged iff type_renamings does not map "Foo" to
-                        anything, i.e. "Foo" not in type_renamings
-        field_renamings: maps type names to the field renamings for that type. The renamings map
-                         field names belonging to the type to a set of field names for the
-                         renamed schema
-        query_type: name of the query type, e.g. 'RootSchemaQuery'
-
-    Raises:
-        - CascadingSuppressionError if a type/field suppression would require further suppressions
-        - NotImplementedError if type_renamings attempts to suppress an enum or an interface
-    """
-    _ensure_no_cascading_type_suppressions(schema_ast, type_renamings, field_renamings, query_type)
-    _ensure_no_unsupported_suppressions(schema_ast, type_renamings)
-
-
-def _ensure_no_cascading_type_suppressions(
-    schema_ast: DocumentNode,
-    type_renamings: Mapping[str, Optional[str]],
-    field_renamings: Mapping[str, Mapping[str, Set[str]]],
-    query_type: str,
-) -> None:
-    """Check for situations that would require further suppressions to produce a valid schema."""
-    visitor = CascadingSuppressionCheckVisitor(type_renamings, field_renamings, query_type)
-    visit(schema_ast, visitor)
-    if visitor.fields_to_suppress or visitor.union_types_to_suppress or visitor.types_to_suppress:
-        error_message_components = [
-            "Renamings would require further suppressions to produce a valid renamed schema."
-        ]
-        if visitor.fields_to_suppress:
-            for object_type in visitor.fields_to_suppress:
-                error_message_components.append(f"Object type {object_type} contains: ")
-                error_message_components.extend(
-                    (
-                        f"field {field} of suppressed type "
-                        f"{visitor.fields_to_suppress[object_type][field]}, "
-                        for field in visitor.fields_to_suppress[object_type]
-                    )
-                )
-            error_message_components.append(
-                "A schema containing a field that is of a nonexistent type is invalid. To fix "
-                "this, suppress the previously-mentioned fields using the field_renamings argument "
-                "of rename_schema."
-            )
-        if visitor.union_types_to_suppress:
-            for union_type in visitor.union_types_to_suppress:
-                error_message_components.append(
-                    f"Union type {union_type} has no non-suppressed members: "
-                )
-                error_message_components.extend(
-                    (union_member.name.value for union_member in union_type.types)
-                )
-            error_message_components.append(
-                "A schema containing a union with no members is invalid. To fix this, suppress the "
-                "previously-mentioned unions using the type_renamings argument of rename_schema."
-            )
-        if visitor.types_to_suppress:
-            error_message_components.append(
-                f"The following types have no non-suppressed fields, which is invalid: "
-                f"{sorted(visitor.types_to_suppress)}. To fix this, suppress the "
-                f"previously-mentioned types using the type_renamings argument of rename_schema."
-            )
-        error_message_components.append(
-            "Note that adding suppressions may lead to other types, fields, etc. requiring "
-            "suppression so you may need to iterate on this before getting a legal schema."
-        )
-        raise CascadingSuppressionError("\n".join(error_message_components))
-
-
 def _ensure_no_unsupported_suppressions(
     schema_ast: DocumentNode, type_renamings: Mapping[str, Optional[str]]
 ) -> None:
@@ -386,7 +323,8 @@ def _rename_and_suppress_types_and_fields(
     type_renamings: Mapping[str, Optional[str]],
     field_renamings: Mapping[str, Mapping[str, Set[str]]],
     query_type: str,
-) -> Tuple[DocumentNode, Dict[str, str], Dict[str, Dict[str, str]], Set[str]]:
+    interfaces_to_make_unqueryable: Set[str],
+) -> Tuple[DocumentNode, Dict[str, str], Dict[str, Dict[str, str]]]:
     """Rename and suppress types, enums, interfaces, fields using renamings.
 
     The query type will not be renamed.
@@ -402,22 +340,25 @@ def _rename_and_suppress_types_and_fields(
                          field names belonging to the type to a set of field names for the
                          renamed schema
         query_type: name of the query type, e.g. 'RootSchemaQuery'
+        interfaces_to_make_unqueryable: interfaces to remove from the query type because one or more
+                                        of their descendants in the inheritance hierarchy was
+                                        suppressed.
 
     Returns:
         Tuple containing
             - modified version of the schema AST
             - renamed type name to original type name map
             - renamed field name to original field name map
-            - set of interfaces to make unqueryable because one or more of their descendants in the
-              inheritance hierarchy was suppressed
-        The maps contain entries for all non-suppressed types/ fields that were changed.
+        The maps contain entries for all non-suppressed types/fields that were changed.
 
     Raises:
         - InvalidNameError if the user attempts to rename a type or field to an invalid name
         - SchemaRenameNameConflictError if the rename causes name conflicts
         - NoOpRenamingError if renamings contains no-op renamings
     """
-    visitor = RenameSchemaTypesVisitor(type_renamings, field_renamings, query_type)
+    visitor = RenameSchemaTypesVisitor(
+        type_renamings, field_renamings, query_type, interfaces_to_make_unqueryable
+    )
     renamed_schema_ast = visit(schema_ast, visitor)
     if (
         visitor.object_types_to_suppress
@@ -555,26 +496,10 @@ def _rename_and_suppress_types_and_fields(
                 type_name
             ] = current_type_reverse_field_name_map_changed_names_only
 
-    interfaces_to_make_unqueryable = set()
-    interface_name_to_definition_node_map = {
-        node.name.value: node
-        for node in schema_ast.definitions
-        if isinstance(node, InterfaceTypeDefinitionNode)
-    }
-    for node in visitor.suppressed_types_implementing_interfaces:
-        interfaces_to_make_unqueryable.update(
-            set(
-                _recursively_get_ancestor_interface_names(
-                    schema_ast, node, interface_name_to_definition_node_map
-                )
-            )
-        )
-
     return (
         renamed_schema_ast,
         reverse_name_map_changed_names_only,
         reverse_field_name_map_changed_names_only,
-        interfaces_to_make_unqueryable,
     )
 
 
@@ -614,16 +539,18 @@ def _rename_and_suppress_query_type_fields(
 def _recursively_get_ancestor_interface_names(
     schema: DocumentNode,
     node: Union[ObjectTypeDefinitionNode, InterfaceTypeDefinitionNode],
-    interface_name_to_definition_node_map: Dict[str, InterfaceTypeDefinitionNode],
+    interface_and_object_type_name_to_definition_node_map: Dict[
+        str, Union[ObjectTypeDefinitionNode, InterfaceTypeDefinitionNode]
+    ],
 ) -> Iterable[str]:
     """Get all ancestor interface type names for the given node."""
     for interface_name_node in node.interfaces:
         yield interface_name_node.name.value
-        interface_definition_node = interface_name_to_definition_node_map[
+        interface_definition_node = interface_and_object_type_name_to_definition_node_map[
             interface_name_node.name.value
         ]
         yield from _recursively_get_ancestor_interface_names(
-            schema, interface_definition_node, interface_name_to_definition_node_map
+            schema, interface_definition_node, interface_and_object_type_name_to_definition_node_map
         )
 
 
@@ -769,11 +696,27 @@ class RenameSchemaTypesVisitor(Visitor):
         Union[ObjectTypeDefinitionNode, InterfaceTypeDefinitionNode]
     ]
 
+    # Collects cascading suppression errors involving types. If every field in a type gets
+    # suppressed but the type itself is not explicitly supppressed, object_types_to_suppress will
+    # contain that type's name.
+    object_types_to_suppress: Set[str]
+
+    # Collects cascading suppression errors involving unions. If every type in a union gets
+    # suppressed but the union itself is not explicitly supppressed, union_types_to_suppress will
+    # contain that union's name.
+    union_types_to_suppress: Set[str]
+
+    # Collects cascading suppression errors involving fields. If a field's type (named "V") gets
+    # suppressed but the field (named "F") or the type containing that field (named "T") is not
+    # explicitly supppressed, fields_to_suppress will map "T" to a dict which maps "F" to "V".
+    fields_to_suppress: Dict[str, Dict[str, str]]
+
     def __init__(
         self,
         type_renamings: Mapping[str, Optional[str]],
         field_renamings: Mapping[str, Mapping[str, Set[str]]],
         query_type: str,
+        interfaces_to_make_unqueryable: Set[str],
     ) -> None:
         """Create a visitor for renaming types in a schema AST.
 
@@ -785,6 +728,9 @@ def __init__(
                              field names belonging to the type to a set of field names for the
                              renamed schema
             query_type: name of the query type (e.g. RootSchemaQuery), which will not be renamed
+            interfaces_to_make_unqueryable: interfaces to remove from the query type because one or
+                                            more of their descendants in the inheritance hierarchy
+                                            was suppressed.
         """
         self.type_renamings = type_renamings
         self.reverse_name_map = {}
@@ -806,6 +752,10 @@ def __init__(
         self.field_name_conflicts = {}
         self.types_involving_interfaces_with_field_renamings = set()
         self.suppressed_types_implementing_interfaces = set()
+        self.object_types_to_suppress = set()
+        self.union_types_to_suppress = set()
+        self.fields_to_suppress = {}
+        self.interfaces_to_make_unqueryable = interfaces_to_make_unqueryable
 
     def _rename_or_suppress_or_ignore_name_and_add_to_record(
         self, node: RenameTypesT
@@ -889,10 +839,14 @@ def _rename_fields(self, node: ObjectTypeDefinitionNode) -> ObjectTypeDefinition
         This method only gets called for type nodes that are not to be suppressed, since if the type
         is to be suppressed then there's no need to check its fields for CascadingSuppressionError.
 
-        If a field F is of type T where node is not of type T, and T is suppressed, then F must also
-        be suppressed. If this is not the case, it will lead to a CascadingSuppressionError and this
-        method will collect the information necessary to raise a CascadingSuppressionError in the
-        visitor object.
+        If a field F is of type T where node is not of type T, and T is unqueryable, then F must
+        also be suppressed. There are two ways for a type to become unqueryable:
+            - the type itself was suppressed
+            - the type is an interface and another type implementing the interface was suppressed
+
+        If a field needs to be suppressed but isn't, it will lead to a CascadingSuppressionError.
+        This method will collect the information necessary to raise a CascadingSuppressionError in
+        the visitor object.
 
         Args:
             node: type node with fields to be renamed
@@ -921,11 +875,16 @@ def _rename_fields(self, node: ObjectTypeDefinitionNode) -> ObjectTypeDefinition
             field_type_suppressed = (
                 self.type_renamings.get(field_type_name, field_type_name) is None
             )
+            field_type_is_unqueryable_interface = (
+                field_type_name in self.interfaces_to_make_unqueryable
+            )
             field_node_suppressed = (
                 type_name in self.field_renamings
                 and self.field_renamings[type_name].get(field_name, {field_name}) == set()
             )
-            if field_type_suppressed and not field_node_suppressed:
+            if (
+                field_type_suppressed or field_type_is_unqueryable_interface
+            ) and not field_node_suppressed:
                 # If the type of the field is suppressed but the field itself is not, it's invalid.
                 current_type_fields_to_suppress[field_name] = field_type_name
         if current_type_fields_to_suppress != {}:

graphql_compiler/tests/schema_transformation_tests/input_schema_strings.py

@@ -337,6 +337,45 @@ class InputSchemaStrings(object):
     """
     )
 
+    interface_typed_field = dedent(
+        """\
+        schema {
+          query: SchemaQuery
+        }
+
+        interface AbstractCharacter {
+          id: String
+        }
+
+        interface Character {
+          id: String
+        }
+
+        type Human implements Character {
+          id: String
+          friend: Character
+        }
+
+        type Giraffe implements Character {
+          id: String
+          friend: Character
+        }
+
+        type Companion {
+          description: String
+          abstract_companion: AbstractCharacter
+        }
+
+        type SchemaQuery {
+          AbstractCharacter: AbstractCharacter
+          Character: Character
+          Human: Human
+          Giraffe: Giraffe
+          Companion: Companion
+        }
+    """
+    )
+
     same_field_schema = dedent(
         """\
         schema {

graphql_compiler/tests/schema_transformation_tests/test_rename_schema.py

@@ -663,6 +663,14 @@ def test_suppress_interface_and_implementations(self) -> None:
                 {},
             )
 
+    def test_suppress_interface_implementation_when_interface_typed_field_exists(self) -> None:
+        with self.assertRaises(CascadingSuppressionError):
+            # This rename is invalid because suppressing the Human type means that its interface,
+            # Character, must be made unqueryable-- but the renaming doesn't suppress all fields in
+            # the schema that are of a type that is an ancestor of Human (namely, Character and
+            # AbstractCharacter)
+            rename_schema(parse(ISS.interface_typed_field), {"Human": None}, {})
+
     def test_multiple_interfaces_rename(self) -> None:
         renamed_schema = rename_schema(
             parse(ISS.various_types_schema),