-
Notifications
You must be signed in to change notification settings - Fork 51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Schema renaming: Interface implementation suppression #1002
Open
LWprogramming
wants to merge
22
commits into
main
Choose a base branch
from
interface_type_suppression
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 8 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 70ccf6d
Write interface & implementation suppression tests
LWprogramming 0c21b8c
Implement interface implementation suppression (with hack)
LWprogramming bd62568
Update interface implementation suppression comment
LWprogramming e06a5e6
Keep only interface implementation suppression tests to cut scope
LWprogramming b1c18cc
Replace hack with proper code
LWprogramming ae662c1
lint
LWprogramming 9a321bf
Clean up comments
LWprogramming cffdb7b
Address comments
LWprogramming 499d4b5
lint
LWprogramming 269ab07
typo
LWprogramming 70e02ce
Implement cascading suppression error for fields of unqueryable types
LWprogramming 16b120f
lint
LWprogramming 80284bd
clean up test for this particular PR
LWprogramming 154c93b
Update graphql_compiler/schema_transformation/rename_schema.py
LWprogramming 3e53924
Update graphql_compiler/schema_transformation/rename_schema.py
LWprogramming 79f99f4
Update graphql_compiler/schema_transformation/rename_schema.py
LWprogramming 658a2c5
remove trailing space
LWprogramming ee3048c
address comments
LWprogramming c985dea
Merge branch 'main' into interface_type_suppression
LWprogramming 2d997ee
lint
LWprogramming 22bd925
Address comments
LWprogramming File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
|
@@ -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. | ||
- 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. | ||
|
@@ -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, Generator, List, Mapping, Optional, Set, Tuple, Union, cast | ||
|
||
from graphql import ( | ||
DocumentNode, | ||
|
@@ -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 | ||
|
@@ -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), | ||
|
@@ -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 enum or interface suppressions because that hasn't been | ||
LWprogramming marked this conversation as resolved.
Show resolved
Hide resolved
|
||
implemented yet. | ||
|
||
The input AST will not be modified. | ||
|
||
|
@@ -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) | ||
|
@@ -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 = [ | ||
|
@@ -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." | ||
) | ||
|
@@ -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. | ||
|
@@ -405,8 +405,9 @@ def _rename_and_suppress_types_and_fields( | |
|
||
Returns: | ||
Tuple containing the modified version of the schema AST, the renamed type name to original | ||
LWprogramming marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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. | ||
type name map, the renamed field name to original field name map, and the set of interfaces | ||
to make unqueryable because one of their descendants in the inheritance hierarchy was | ||
LWprogramming marked this conversation as resolved.
Show resolved
Hide resolved
LWprogramming marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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 | ||
|
@@ -518,15 +519,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. | ||
|
||
|
@@ -538,18 +558,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 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( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Necessary to define this function outside the |
||
schema: DocumentNode, | ||
node: Union[ObjectTypeDefinitionNode, InterfaceTypeDefinitionNode], | ||
interface_name_to_definition_node_map: Dict[str, InterfaceTypeDefinitionNode], | ||
) -> Generator[str, None, None]: | ||
LWprogramming marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"""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.""" | ||
|
||
|
@@ -684,6 +725,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 names of interfaces to keep in the schema but make unqueryable as a result of one of | ||
# their descendants in the inheritance hierarchy being suppressed. If a type named "Foo" | ||
# implements at least one interface, then suppressed_types_implementing_interfaces will contain | ||
# "Foo". | ||
LWprogramming marked this conversation as resolved.
Show resolved
Hide resolved
|
||
suppressed_types_implementing_interfaces: Set[ | ||
Union[ObjectTypeDefinitionNode, InterfaceTypeDefinitionNode] | ||
LWprogramming marked this conversation as resolved.
Show resolved
Hide resolved
|
||
] | ||
|
||
def __init__( | ||
self, | ||
type_renamings: Mapping[str, Optional[str]], | ||
|
@@ -720,6 +769,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 | ||
|
@@ -750,6 +800,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): | ||
|
@@ -873,14 +928,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 of | ||
their descendants in the inheritance hierarchy was | ||
suppressed. | ||
|
||
Raises: | ||
- SchemaTransformError if every field in the query type was suppressed | ||
|
@@ -890,6 +953,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, | ||
|
@@ -932,6 +996,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 | ||
|
@@ -1101,10 +1167,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). | ||
|
@@ -1114,7 +1179,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, | ||
|
@@ -1141,19 +1205,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) |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
then it should be legal to rename the
name
field, just like ifBar
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 theNotImplementedError
checks not been so strict.There was a problem hiding this comment.
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 theOperations that are not yet supported but will be implemented:
section there should be something likeautomatic suppression of fields when the field is of a suppressed interface type
?There was a problem hiding this comment.
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 theGiraffe
type'sfriend
field is of typeCharacter
, andCharacter
is to be made unqueryable. However, there's no way to suppress any fields inGiraffe
right now becauseGiraffe
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.There was a problem hiding this comment.
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?