Skip to content
This repository was archived by the owner on Feb 6, 2025. It is now read-only.

Schema renaming: Interface implementation suppression #1002

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open

Schema renaming: Interface implementation suppression #1002

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
425dc79
Move interfaces test schema into various types schema
LWprogramming Mar 16, 2021
70ccf6d
Write interface & implementation suppression tests
LWprogramming Mar 16, 2021
0c21b8c
Implement interface implementation suppression (with hack)
LWprogramming Mar 17, 2021
bd62568
Update interface implementation suppression comment
LWprogramming Mar 18, 2021
e06a5e6
Keep only interface implementation suppression tests to cut scope
LWprogramming Mar 18, 2021
b1c18cc
Replace hack with proper code
LWprogramming Mar 19, 2021
ae662c1
lint
LWprogramming Mar 19, 2021
9a321bf
Clean up comments
LWprogramming Mar 19, 2021
cffdb7b
Address comments
LWprogramming Mar 24, 2021
499d4b5
lint
LWprogramming Mar 24, 2021
269ab07
typo
LWprogramming Mar 24, 2021
70e02ce
Implement cascading suppression error for fields of unqueryable types
LWprogramming Mar 30, 2021
16b120f
lint
LWprogramming Mar 30, 2021
80284bd
clean up test for this particular PR
LWprogramming Mar 30, 2021
154c93b
Update graphql_compiler/schema_transformation/rename_schema.py
LWprogramming Apr 5, 2021
3e53924
Update graphql_compiler/schema_transformation/rename_schema.py
LWprogramming Apr 5, 2021
79f99f4
Update graphql_compiler/schema_transformation/rename_schema.py
LWprogramming Apr 5, 2021
658a2c5
remove trailing space
LWprogramming Apr 6, 2021
ee3048c
address comments
LWprogramming Apr 6, 2021
c985dea
Merge branch 'main' into interface_type_suppression
LWprogramming Apr 12, 2021
2d997ee
lint
LWprogramming Apr 12, 2021
22bd925
Address comments
LWprogramming May 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
153 changes: 102 additions & 51 deletions graphql_compiler/schema_transformation/rename_schema.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,14 +75,13 @@

Operations that are already supported:
- 1-1 renaming of object types, unions, enums, and interfaces.
- Suppressing types that don't implement an interface.
- Suppressing unions.
- Suppressing object types and unions.
- 1-1 and 1-many renamings for fields belonging to object types.
- Suppressions for fields belonging to object types.
- Renamings and suppressions for scalar types.

Operations that are not yet supported but will be implemented:
- Suppressions for enums, interfaces, and object types that implement interfaces.
- Suppressions for enums and interfaces.
- Renamings and suppressions for fields that belong to either interface types or object types that
implement interfaces.
- Renamings and suppressions for enum values.
Expand All @@ -92,6 +91,13 @@
- If you suppress a type Foo, no other type Bar may keep fields of type Foo (those fields must be
suppressed). However, if type Foo has a field of that type Foo, it is legal to suppress type Foo
without explicitly suppressing that particular field.
- If you suppress a type Foo implementing an interface Bar, then Bar will remain in the schema but
not in the root query type, making it unqueryable. This is to prevent situations where a scope in
a query is of type Bar without also being of some more specific type, which may yield vertices of
type Foo even though it was suppressed. For this reason, all interfaces that Bar implements will
also be made unqueryable in the same way, and this removal from the root query type will happen
recursively until all interfaces that Foo implements (either directly or indirectly) are
unqueryable.
Comment on lines +96 to +102
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this comment could be updated to mention that field interface suppression is not yet implemented i.e. what this test is addressing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, so this is a bit tricky-- the plan for field renamings in PR 1005 is more like allowing something that should already exist, specifically if we have a schema like

# Exists interface named Foo with a single field named id
type Bar implements Foo {
  name: String
  id: String
}

then it should be legal to rename the name field, just like if Bar implemented no interfaces. In other words, it seems like it's less of a large functionality change that's coming soon and more like allowing something that should be allowed without adding more special cases to the field renaming API, had the NotImplementedError checks not been so strict.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the case in the test case I linked above going to be implemented eventually or will it always be a CascadingSuppressionError? If it is going to be implemented eventually, maybe in the Operations that are not yet supported but will be implemented: section there should be something like automatic suppression of fields when the field is of a suppressed interface type?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current implementation will always be a CascadingSuppressionError-- the problem in that test case is that the Giraffe type's friend field is of type Character, and Character is to be made unqueryable. However, there's no way to suppress any fields in Giraffe right now because Giraffe implements an interface, so until the changes in #1005 relax this restriction, there's no way to implement the part of the test that shows how we'd want to do such a thing.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I'm not proposing you implement in this PR, but I think it should be documented somewhere that you can't suppress an object that implements an interface if that interfaces is a field type on another object. Is that already documented somewhere that I missed?

- If you suppress all the fields of a type Foo, then the type Foo must also be suppressed in
type_renamings.
- You may not suppress all types in the schema's root type.
Expand All @@ -118,7 +124,7 @@
"""
from collections import namedtuple
from copy import copy
from typing import Any, Dict, List, Mapping, Optional, Set, Tuple, Union, cast
from typing import Any, Dict, Iterable, List, Mapping, Optional, Set, Tuple, Union, cast

from graphql import (
DocumentNode,
Expand Down Expand Up @@ -215,8 +221,7 @@ def rename_schema(
also catch all of its subclasses. This will change after the error classes are modified so
that errors can be fixed programmatically, at which point it will make sense for the user
to attempt to treat different errors differently
- NotImplementedError if type_renamings attempts to suppress an enum, an interface, or a
type implementing an interface
- NotImplementedError if type_renamings attempts to suppress an enum or an interface
- InvalidNameError if the schema contains an invalid type name, or if the user attempts
to rename a type to an invalid name. A name is considered invalid if it does not consist
of alphanumeric characters and underscores, if it starts with a numeric character, or
Expand All @@ -238,11 +243,18 @@ def rename_schema(
_validate_renamings(schema_ast, type_renamings, field_renamings, query_type)

# Rename types, interfaces, enums, unions and suppress types, unions
schema_ast, reverse_name_map, reverse_field_name_map = _rename_and_suppress_types_and_fields(
(
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 = _rename_and_suppress_query_type_fields(schema_ast, type_renamings, query_type)
schema_ast = _rename_and_suppress_query_type_fields(
schema_ast, type_renamings, query_type, interfaces_to_make_unqueryable
)
return RenamedSchemaDescriptor(
schema_ast=schema_ast,
schema=build_ast_schema(schema_ast),
Expand All @@ -260,8 +272,8 @@ def _validate_renamings(
"""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 enums, interfaces, or interface implementation suppressions
because that hasn't been implemented yet.
confirm type_renamings contains no suppressions for enums or interfaces because they haven't
been implemented yet.

The input AST will not be modified.

Expand All @@ -279,8 +291,7 @@ def _validate_renamings(

Raises:
- CascadingSuppressionError if a type/field suppression would require further suppressions
- NotImplementedError if type_renamings attempts to suppress an enum, an interface, or a
type implementing an interface
- 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)
Expand Down Expand Up @@ -342,14 +353,10 @@ def _ensure_no_cascading_type_suppressions(
def _ensure_no_unsupported_suppressions(
schema_ast: DocumentNode, type_renamings: Mapping[str, Optional[str]]
) -> None:
"""Confirm type_renamings has no enums, interfaces, or interface implementation suppressions."""
"""Confirm type_renamings has no enum or interface suppressions."""
visitor = SuppressionNotImplementedVisitor(type_renamings)
visit(schema_ast, visitor)
if (
not visitor.unsupported_enum_suppressions
and not visitor.unsupported_interface_suppressions
and not visitor.unsupported_interface_implementation_suppressions
):
if not visitor.unsupported_enum_suppressions and not visitor.unsupported_interface_suppressions:
return
# Otherwise, attempted to suppress something we shouldn't suppress.
error_message_components = [
Expand All @@ -368,13 +375,6 @@ def _ensure_no_unsupported_suppressions(
f"{visitor.unsupported_interface_suppressions}, attempting to suppress them. However, "
f"type renaming has not implemented interface suppression yet."
)
if visitor.unsupported_interface_implementation_suppressions:
error_message_components.append(
f"Type renamings mapped these object types to None: "
f"{visitor.unsupported_interface_implementation_suppressions}, attempting to suppress "
f"them. Normally, this would be fine. However, these types each implement at least one "
f"interface and type renaming has not implemented this particular suppression yet."
)
error_message_components.append(
"To avoid these suppressions, remove the mappings from the type_renamings argument."
)
Expand All @@ -386,7 +386,7 @@ 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]]]:
) -> Tuple[DocumentNode, Dict[str, str], Dict[str, Dict[str, str]], Set[str]]:
"""Rename and suppress types, enums, interfaces, fields using renamings.

The query type will not be renamed.
Expand All @@ -404,9 +404,13 @@ def _rename_and_suppress_types_and_fields(
query_type: name of the query type, e.g. 'RootSchemaQuery'

Returns:
Tuple containing the modified version of the schema AST, the renamed type name to original
type name map, and the renamed field name to original field name map. The maps contain
entries for all non-suppressed types/ fields that were changed.
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.

Raises:
- InvalidNameError if the user attempts to rename a type or field to an invalid name
Expand Down Expand Up @@ -518,15 +522,34 @@ 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,
)


def _rename_and_suppress_query_type_fields(
schema_ast: DocumentNode, type_renamings: Mapping[str, Optional[str]], query_type: str
schema_ast: DocumentNode,
type_renamings: Mapping[str, Optional[str]],
query_type: str,
interfaces_to_make_unqueryable: Set[str],
) -> DocumentNode:
"""Rename or suppress all fields of the query type.

Expand All @@ -538,18 +561,39 @@ def _rename_and_suppress_query_type_fields(
type named "Foo" will be unchanged iff type_renamings does not map "Foo" to
anything, i.e. "Foo" not in type_renamings
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:
modified version of the input schema AST

Raises:
- SchemaTransformError if type_renamings suppressed every type
"""
visitor = RenameQueryTypeFieldsVisitor(type_renamings, query_type)
visitor = RenameQueryTypeFieldsVisitor(
type_renamings, query_type, interfaces_to_make_unqueryable
)
renamed_schema_ast = visit(schema_ast, visitor)
return renamed_schema_ast


def _recursively_get_ancestor_interface_names(
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Necessary to define this function outside the RenameSchemaTypesVisitor object because we need access to the schema's definitions field in order to get the interface definition nodes.

schema: DocumentNode,
node: Union[ObjectTypeDefinitionNode, InterfaceTypeDefinitionNode],
interface_name_to_definition_node_map: Dict[str, 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_name_node.name.value
]
yield from _recursively_get_ancestor_interface_names(
schema, interface_definition_node, interface_name_to_definition_node_map
)


class RenameSchemaTypesVisitor(Visitor):
"""Traverse a Document AST, editing the names of nodes."""

Expand Down Expand Up @@ -684,6 +728,14 @@ class RenameSchemaTypesVisitor(Visitor):
# type named "Foo", types_involving_interfaces_with_field_renamings will contain "Foo".
types_involving_interfaces_with_field_renamings: Set[str]

# Collects types that get suppressed and implement interfaces, so that all ancestor interfaces
# can be kept in the schema but made unqueryable. If a type named "Foo" implements at least one
# interface and type_renamings would suppress "Foo", then
# suppressed_types_implementing_interfaces will contain "Foo".
suppressed_types_implementing_interfaces: Set[
Union[ObjectTypeDefinitionNode, InterfaceTypeDefinitionNode]
]

def __init__(
self,
type_renamings: Mapping[str, Optional[str]],
Expand Down Expand Up @@ -720,6 +772,7 @@ def __init__(
self.invalid_field_names = {}
self.field_name_conflicts = {}
self.types_involving_interfaces_with_field_renamings = set()
self.suppressed_types_implementing_interfaces = set()

def _rename_or_suppress_or_ignore_name_and_add_to_record(
self, node: RenameTypesT
Expand Down Expand Up @@ -750,6 +803,11 @@ def _rename_or_suppress_or_ignore_name_and_add_to_record(

if desired_type_name is None:
# Suppress the type
if (
isinstance(node, (ObjectTypeDefinitionNode, InterfaceTypeDefinitionNode))
and node.interfaces
):
self.suppressed_types_implementing_interfaces.add(node)
self.suppressed_type_names.add(type_name)
return REMOVE
if not is_valid_nonreserved_name(desired_type_name):
Expand Down Expand Up @@ -873,14 +931,22 @@ def enter(


class RenameQueryTypeFieldsVisitor(Visitor):
def __init__(self, type_renamings: Mapping[str, Optional[str]], query_type: str) -> None:
def __init__(
self,
type_renamings: Mapping[str, Optional[str]],
query_type: str,
interfaces_to_make_unqueryable: Set[str],
) -> None:
"""Create a visitor for renaming or suppressing fields of the query type in a schema AST.

Args:
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
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.

Raises:
- SchemaTransformError if every field in the query type was suppressed
Expand All @@ -890,6 +956,7 @@ def __init__(self, type_renamings: Mapping[str, Optional[str]], query_type: str)
self.in_query_type = False
self.type_renamings = type_renamings
self.query_type = query_type
self.interfaces_to_make_unqueryable = interfaces_to_make_unqueryable

def enter_object_type_definition(
self,
Expand Down Expand Up @@ -932,6 +999,8 @@ def enter_field_definition(
"""If inside query type, rename or remove field as specified by type_renamings."""
if self.in_query_type:
field_name = node.name.value
if field_name in self.interfaces_to_make_unqueryable:
return REMOVE
new_field_name = self.type_renamings.get(field_name, field_name) # Default use original
if new_field_name == field_name:
return IDLE
Expand Down Expand Up @@ -1101,10 +1170,9 @@ class SuppressionNotImplementedVisitor(Visitor):

unsupported_enum_suppressions: Set[str]
unsupported_interface_suppressions: Set[str]
unsupported_interface_implementation_suppressions: Set[str]

def __init__(self, type_renamings: Mapping[str, Optional[str]]) -> None:
"""Confirm type_renamings doesn't try to suppress enum/interface/interface implementation.
"""Confirm type_renamings doesn't try to suppress enum/interface types.

Args:
type_renamings: maps original type name to renamed name or None (for type suppression).
Expand All @@ -1114,7 +1182,6 @@ def __init__(self, type_renamings: Mapping[str, Optional[str]]) -> None:
self.type_renamings = type_renamings
self.unsupported_enum_suppressions = set()
self.unsupported_interface_suppressions = set()
self.unsupported_interface_implementation_suppressions = set()

def enter_enum_type_definition(
self,
Expand All @@ -1141,19 +1208,3 @@ def enter_interface_type_definition(
interface_name = node.name.value
if self.type_renamings.get(interface_name, interface_name) is None:
self.unsupported_interface_suppressions.add(interface_name)

def enter_object_type_definition(
self,
node: ObjectTypeDefinitionNode,
key: Any,
parent: Any,
path: List[Any],
ancestors: List[Any],
) -> None:
"""If type_renamings suppresses interface implementations, record it for error message."""
if not node.interfaces:
return
object_name = node.name.value
if self.type_renamings.get(object_name, object_name) is None:
# Suppressing interface implementations isn't supported yet.
self.unsupported_interface_implementation_suppressions.add(object_name)
Loading