From 425dc798d32d05510825663f338e011d64031582 Mon Sep 17 00:00:00 2001 From: Leon Wu Date: Tue, 16 Mar 2021 17:04:55 -0400 Subject: [PATCH 01/21] Move interfaces test schema into various types schema --- .../input_schema_strings.py | 42 ++++++---------- .../test_rename_schema.py | 50 +++++++++++++++---- 2 files changed, 53 insertions(+), 39 deletions(-) diff --git a/graphql_compiler/tests/schema_transformation_tests/input_schema_strings.py b/graphql_compiler/tests/schema_transformation_tests/input_schema_strings.py index c8abcfacb..28758bdfa 100644 --- a/graphql_compiler/tests/schema_transformation_tests/input_schema_strings.py +++ b/graphql_compiler/tests/schema_transformation_tests/input_schema_strings.py @@ -133,33 +133,6 @@ class InputSchemaStrings(object): """ ) - multiple_interfaces_schema = dedent( - """\ - schema { - query: SchemaQuery - } - - interface Character { - id: String - } - - interface Creature { - age: Int - } - - type Human implements Character & Creature { - id: String - age: Int - } - - type SchemaQuery { - Character: Character - Creature: Creature - Human: Human - } - """ - ) - scalar_schema = dedent( """\ schema { @@ -322,13 +295,23 @@ class InputSchemaStrings(object): SHORT } + interface AbstractCreature { + name: String + } + + interface Creature implements AbstractCreature { + name: String + age: Int + } + interface Character { id: String } - type Human implements Character { + type Human implements Character & Creature { id: String name: String + age: Int birthday: Date } @@ -344,6 +327,9 @@ class InputSchemaStrings(object): directive @stitch(source_field: String!, sink_field: String!) on FIELD_DEFINITION type SchemaQuery { + AbstractCreature: AbstractCreature + Creature: Creature + Character: Character Human: Human Giraffe: Giraffe Dog: Dog diff --git a/graphql_compiler/tests/schema_transformation_tests/test_rename_schema.py b/graphql_compiler/tests/schema_transformation_tests/test_rename_schema.py index 3dce44b27..7d0c4b14b 100644 --- a/graphql_compiler/tests/schema_transformation_tests/test_rename_schema.py +++ b/graphql_compiler/tests/schema_transformation_tests/test_rename_schema.py @@ -466,24 +466,24 @@ def test_field_renaming_illegal_noop_rename_fields_of_suppressed_type(self) -> N def test_field_renaming_in_interfaces(self) -> None: with self.assertRaises(NotImplementedError): rename_schema( - parse(ISS.multiple_interfaces_schema), {}, {"Character": {"id": {"new_id"}}} + parse(ISS.various_types_schema), {}, {"Character": {"id": {"new_id"}}} ) with self.assertRaises(NotImplementedError): rename_schema( - parse(ISS.multiple_interfaces_schema), {}, {"Character": {"id": {"id", "new_id"}}} + parse(ISS.various_types_schema), {}, {"Character": {"id": {"id", "new_id"}}} ) with self.assertRaises(NotImplementedError): - rename_schema(parse(ISS.multiple_interfaces_schema), {}, {"Character": {"id": set()}}) + rename_schema(parse(ISS.various_types_schema), {}, {"Character": {"id": set()}}) with self.assertRaises(NotImplementedError): # Cannot rename Human's fields because Human implements an interface and field_renamings # for object types that implement interfaces isn't supported yet. - rename_schema(parse(ISS.multiple_interfaces_schema), {}, {"Human": {"id": {"new_id"}}}) + rename_schema(parse(ISS.various_types_schema), {}, {"Human": {"id": {"new_id"}}}) with self.assertRaises(NotImplementedError): rename_schema( - parse(ISS.multiple_interfaces_schema), {}, {"Human": {"id": {"id", "new_id"}}} + parse(ISS.various_types_schema), {}, {"Human": {"id": {"id", "new_id"}}} ) with self.assertRaises(NotImplementedError): - rename_schema(parse(ISS.multiple_interfaces_schema), {}, {"Human": {"id": set()}}) + rename_schema(parse(ISS.various_types_schema), {}, {"Human": {"id": set()}}) def test_enum_rename(self) -> None: renamed_schema = rename_schema( @@ -575,7 +575,7 @@ def test_suppress_interface_and_all_implementations(self) -> None: def test_multiple_interfaces_rename(self) -> None: renamed_schema = rename_schema( - parse(ISS.multiple_interfaces_schema), + parse(ISS.various_types_schema), {"Human": "NewHuman", "Character": "NewCharacter", "Creature": "NewCreature"}, {}, ) @@ -585,23 +585,51 @@ def test_multiple_interfaces_rename(self) -> None: query: SchemaQuery } - interface NewCharacter { - id: String + scalar Date + + enum Height { + TALL + SHORT + } + + interface AbstractCreature { + name: String } - interface NewCreature { + interface NewCreature implements AbstractCreature { + name: String age: Int } + interface NewCharacter { + id: String + } + type NewHuman implements NewCharacter & NewCreature { id: String + name: String age: Int + birthday: Date + } + + type Giraffe implements NewCharacter { + id: String + height: Height } + type Dog { + nickname: String + } + + directive @stitch(source_field: String!, sink_field: String!) on FIELD_DEFINITION + type SchemaQuery { - NewCharacter: NewCharacter + AbstractCreature: AbstractCreature NewCreature: NewCreature + NewCharacter: NewCharacter NewHuman: NewHuman + Giraffe: Giraffe + Dog: Dog } """ ) From 70ccf6d3214cc2592956efaaba542c60b5b23e7f Mon Sep 17 00:00:00 2001 From: Leon Wu Date: Tue, 16 Mar 2021 17:36:40 -0400 Subject: [PATCH 02/21] Write interface & implementation suppression tests --- .../test_rename_schema.py | 258 +++++++++++++++++- 1 file changed, 248 insertions(+), 10 deletions(-) diff --git a/graphql_compiler/tests/schema_transformation_tests/test_rename_schema.py b/graphql_compiler/tests/schema_transformation_tests/test_rename_schema.py index 7d0c4b14b..a0d7f9dfe 100644 --- a/graphql_compiler/tests/schema_transformation_tests/test_rename_schema.py +++ b/graphql_compiler/tests/schema_transformation_tests/test_rename_schema.py @@ -554,24 +554,262 @@ def test_interface_rename(self) -> None: self.assertEqual({}, renamed_schema.reverse_field_name_map) def test_suppress_interface_implementation(self) -> None: - with self.assertRaises(NotImplementedError): - rename_schema(parse(ISS.various_types_schema), {"Giraffe": None}, {}) + renamed_schema = rename_schema(parse(ISS.various_types_schema), {"Giraffe": None}, {}) + renamed_schema_string = dedent( + """\ + schema { + query: SchemaQuery + } + + scalar Date + + enum Height { + TALL + SHORT + } + + interface AbstractCreature { + name: String + } + + interface Creature implements AbstractCreature { + name: String + age: Int + } + + interface Character { + id: String + } + + type Human implements Character & Creature { + id: String + name: String + age: Int + birthday: Date + } + + type Dog { + nickname: String + } + + directive @stitch(source_field: String!, sink_field: String!) on FIELD_DEFINITION + + type SchemaQuery { + AbstractCreature: AbstractCreature + Creature: Creature + Human: Human + Dog: Dog + } + """ + ) + compare_schema_texts_order_independently( + self, renamed_schema_string, print_ast(renamed_schema.schema_ast) + ) + self.assertEqual({}, renamed_schema.reverse_name_map) + self.assertEqual({}, renamed_schema.reverse_field_name_map) def test_suppress_all_implementations_but_not_interface(self) -> None: - with self.assertRaises(NotImplementedError): - rename_schema(parse(ISS.various_types_schema), {"Giraffe": None, "Human": None}, {}) + renamed_schema = rename_schema(parse(ISS.various_types_schema), {"Giraffe": None, "Human": None}, {}) + renamed_schema_string = dedent( + """\ + schema { + query: SchemaQuery + } + + scalar Date + + enum Height { + TALL + SHORT + } + + interface AbstractCreature { + name: String + } + + interface Creature implements AbstractCreature { + name: String + age: Int + } + + interface Character { + id: String + } + + type Dog { + nickname: String + } + + directive @stitch(source_field: String!, sink_field: String!) on FIELD_DEFINITION + + type SchemaQuery { + Dog: Dog + } + """ + ) + compare_schema_texts_order_independently( + self, renamed_schema_string, print_ast(renamed_schema.schema_ast) + ) + self.assertEqual({}, renamed_schema.reverse_name_map) + self.assertEqual({}, renamed_schema.reverse_field_name_map) def test_suppress_interface_but_not_implementations(self) -> None: - with self.assertRaises(NotImplementedError): - rename_schema(parse(ISS.various_types_schema), {"Character": None}, {}) + renamed_schema = rename_schema(parse(ISS.various_types_schema), {"Character": None}, {}) + renamed_schema_string = dedent( + """\ + schema { + query: SchemaQuery + } - def test_suppress_interface_and_all_implementations(self) -> None: - with self.assertRaises(NotImplementedError): - rename_schema( + scalar Date + + enum Height { + TALL + SHORT + } + + interface AbstractCreature { + name: String + } + + interface Creature implements AbstractCreature { + name: String + age: Int + } + + type Human implements Creature { + id: String + name: String + age: Int + birthday: Date + } + + type Giraffe { + id: String + height: Height + } + + type Dog { + nickname: String + } + + directive @stitch(source_field: String!, sink_field: String!) on FIELD_DEFINITION + + type SchemaQuery { + AbstractCreature: AbstractCreature + Creature: Creature + Human: Human + Giraffe: Giraffe + Dog: Dog + } + """ + ) + compare_schema_texts_order_independently( + self, renamed_schema_string, print_ast(renamed_schema.schema_ast) + ) + self.assertEqual({}, renamed_schema.reverse_name_map) + self.assertEqual({}, renamed_schema.reverse_field_name_map) + + def test_suppress_interface_implementing_other_interface(self) -> None: + # A synthesis of previous tests. Suppressing Creature means no types can implement it and + # it's no longer queryable (as shown by its removal from RootSchemaQuery), while the + # interface that Creature implements (AbstractCreature) also becomes non-queryable. + renamed_schema = rename_schema(parse(ISS.various_types_schema), {"Creature": None}, {}) + renamed_schema_string = dedent( + """\ + schema { + query: SchemaQuery + } + + scalar Date + + enum Height { + TALL + SHORT + } + + interface AbstractCreature { + name: String + } + + interface Character { + id: String + } + + type Human implements Character { + id: String + name: String + age: Int + birthday: Date + } + + type Giraffe implements Character { + id: String + height: Height + } + + type Dog { + nickname: String + } + + directive @stitch(source_field: String!, sink_field: String!) on FIELD_DEFINITION + + type SchemaQuery { + Character: Character + Human: Human + Giraffe: Giraffe + Dog: Dog + } + """ + ) + compare_schema_texts_order_independently( + self, renamed_schema_string, print_ast(renamed_schema.schema_ast) + ) + self.assertEqual({}, renamed_schema.reverse_name_map) + self.assertEqual({}, renamed_schema.reverse_field_name_map) + + def test_suppress_interface_and_implementations(self) -> None: + # Note how the Creature interface remains in the schema but, since types implementing it + # were suppressed, it's impossible to query specifically for Creature. + renamed_schema = rename_schema( parse(ISS.various_types_schema), - {"Giraffe": None, "Character": None, "Human": None}, + {"Giraffe": None, "Character": None, "Human": None, "AbstractCreature": None}, {}, ) + renamed_schema_string = dedent( + """\ + schema { + query: SchemaQuery + } + + scalar Date + + enum Height { + TALL + SHORT + } + + interface Creature { + name: String + age: Int + } + + type Dog { + nickname: String + } + + directive @stitch(source_field: String!, sink_field: String!) on FIELD_DEFINITION + + type SchemaQuery { + Dog: Dog + } + """ + ) + compare_schema_texts_order_independently( + self, renamed_schema_string, print_ast(renamed_schema.schema_ast) + ) + self.assertEqual({}, renamed_schema.reverse_name_map) + self.assertEqual({}, renamed_schema.reverse_field_name_map) def test_multiple_interfaces_rename(self) -> None: renamed_schema = rename_schema( From 0c21b8c9d0dd5a9cafbb1c310ce3f08033e9e38e Mon Sep 17 00:00:00 2001 From: Leon Wu Date: Wed, 17 Mar 2021 18:44:55 -0400 Subject: [PATCH 03/21] Implement interface implementation suppression (with hack) --- .../schema_transformation/rename_schema.py | 92 +++++++++++-------- 1 file changed, 52 insertions(+), 40 deletions(-) diff --git a/graphql_compiler/schema_transformation/rename_schema.py b/graphql_compiler/schema_transformation/rename_schema.py index 838746eda..8b8ffd189 100644 --- a/graphql_compiler/schema_transformation/rename_schema.py +++ b/graphql_compiler/schema_transformation/rename_schema.py @@ -118,7 +118,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, List, Mapping, Optional, Set, Tuple, Union, cast, Generator from graphql import ( DocumentNode, @@ -128,7 +128,7 @@ Node, ObjectTypeDefinitionNode, UnionTypeDefinitionNode, - build_ast_schema, + build_ast_schema, GraphQLSchema, ) from graphql.language.visitor import IDLE, REMOVE, Visitor, VisitorAction, visit from graphql.pyutils import FrozenList @@ -238,11 +238,11 @@ 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 +260,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 + implemented yet. The input AST will not be modified. @@ -342,13 +342,12 @@ 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 ): return # Otherwise, attempted to suppress something we shouldn't suppress. @@ -368,13 +367,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 +378,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 +397,9 @@ def _rename_and_suppress_types_and_fields( 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. + 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 + 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 +511,20 @@ def _rename_and_suppress_types_and_fields( type_name ] = current_type_reverse_field_name_map_changed_names_only + interfaces_to_make_unqueryable = set() + for node in visitor.suppressed_types_implementing_interfaces: + interfaces_to_make_unqueryable.update(set(_recursively_get_ancestor_interface_names(build_ast_schema(schema_ast), node))) + 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,6 +536,9 @@ 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 @@ -545,11 +546,24 @@ def _rename_and_suppress_query_type_fields( 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(schema: GraphQLSchema, node: Union[ObjectTypeDefinitionNode, InterfaceTypeDefinitionNode]) -> Generator[str, None, None]: + """Get all ancestor interface type names for the given node.""" + for node_implemented_interface in node.interfaces: + yield node_implemented_interface.name.value + # HACK(Leon): The reason for the code below and the reason why we can't simply recursively yield using just interface itself as the new node is that GraphQL-core currently stores interfaces as a list of NamedNodes rather than a list of InterfaceTypeDefinitionNodes, and this is actually the only way currently to find the actual InterfaceTypeDefinitionNode corresponding to interface. + for _, interface_implementation in schema._implementations_map.items(): + for interface in interface_implementation.interfaces: + if interface.name == node_implemented_interface.name.value: + # If the name matches, then interface.ast_node is the node in the schema corresponding to the one described by the NameNode node_implemented_interface. + yield from _recursively_get_ancestor_interface_names(schema, interface.ast_node) + # Note that interface_implementation also has a field called objects corresponding to the object types in the schema that implement the interface whose name is the key in schema._implementations_map. However, the NameNode node_implemented_interface necessarily describes an interface type, so we don't need to check the objects that implement interface_implementation is implemented to see if node_implemented_interface corresponds to an object type node. + + class RenameSchemaTypesVisitor(Visitor): """Traverse a Document AST, editing the names of nodes.""" @@ -684,6 +698,12 @@ 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". + suppressed_types_implementing_interfaces = Set[Union[ObjectTypeDefinitionNode, InterfaceTypeDefinitionNode]] + def __init__( self, type_renamings: Mapping[str, Optional[str]], @@ -720,6 +740,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 +771,8 @@ 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,7 +896,8 @@ 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: @@ -881,6 +905,9 @@ def __init__(self, type_renamings: Mapping[str, Optional[str]], query_type: str) 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 +917,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 +960,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 +1131,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 +1143,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 +1169,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) From bd62568e1f8a5637a78baf89d4264057e10a6730 Mon Sep 17 00:00:00 2001 From: Leon Wu Date: Thu, 18 Mar 2021 16:13:24 -0400 Subject: [PATCH 04/21] Update interface implementation suppression comment --- .../schema_transformation/rename_schema.py | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/graphql_compiler/schema_transformation/rename_schema.py b/graphql_compiler/schema_transformation/rename_schema.py index 8b8ffd189..47c7f6b41 100644 --- a/graphql_compiler/schema_transformation/rename_schema.py +++ b/graphql_compiler/schema_transformation/rename_schema.py @@ -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 + be removed from 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 result in + accessing 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. @@ -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 @@ -279,8 +284,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) From e06a5e69557d187d956394b09d5c7b9efdf0e698 Mon Sep 17 00:00:00 2001 From: Leon Wu Date: Thu, 18 Mar 2021 16:29:02 -0400 Subject: [PATCH 05/21] Keep only interface implementation suppression tests to cut scope --- .../test_rename_schema.py | 156 +----------------- 1 file changed, 5 insertions(+), 151 deletions(-) diff --git a/graphql_compiler/tests/schema_transformation_tests/test_rename_schema.py b/graphql_compiler/tests/schema_transformation_tests/test_rename_schema.py index a0d7f9dfe..68d54ea7f 100644 --- a/graphql_compiler/tests/schema_transformation_tests/test_rename_schema.py +++ b/graphql_compiler/tests/schema_transformation_tests/test_rename_schema.py @@ -654,162 +654,16 @@ def test_suppress_all_implementations_but_not_interface(self) -> None: self.assertEqual({}, renamed_schema.reverse_field_name_map) def test_suppress_interface_but_not_implementations(self) -> None: - renamed_schema = rename_schema(parse(ISS.various_types_schema), {"Character": None}, {}) - renamed_schema_string = dedent( - """\ - schema { - query: SchemaQuery - } - - scalar Date - - enum Height { - TALL - SHORT - } - - interface AbstractCreature { - name: String - } - - interface Creature implements AbstractCreature { - name: String - age: Int - } - - type Human implements Creature { - id: String - name: String - age: Int - birthday: Date - } - - type Giraffe { - id: String - height: Height - } - - type Dog { - nickname: String - } - - directive @stitch(source_field: String!, sink_field: String!) on FIELD_DEFINITION - - type SchemaQuery { - AbstractCreature: AbstractCreature - Creature: Creature - Human: Human - Giraffe: Giraffe - Dog: Dog - } - """ - ) - compare_schema_texts_order_independently( - self, renamed_schema_string, print_ast(renamed_schema.schema_ast) - ) - self.assertEqual({}, renamed_schema.reverse_name_map) - self.assertEqual({}, renamed_schema.reverse_field_name_map) - - def test_suppress_interface_implementing_other_interface(self) -> None: - # A synthesis of previous tests. Suppressing Creature means no types can implement it and - # it's no longer queryable (as shown by its removal from RootSchemaQuery), while the - # interface that Creature implements (AbstractCreature) also becomes non-queryable. - renamed_schema = rename_schema(parse(ISS.various_types_schema), {"Creature": None}, {}) - renamed_schema_string = dedent( - """\ - schema { - query: SchemaQuery - } - - scalar Date - - enum Height { - TALL - SHORT - } - - interface AbstractCreature { - name: String - } - - interface Character { - id: String - } - - type Human implements Character { - id: String - name: String - age: Int - birthday: Date - } - - type Giraffe implements Character { - id: String - height: Height - } - - type Dog { - nickname: String - } - - directive @stitch(source_field: String!, sink_field: String!) on FIELD_DEFINITION - - type SchemaQuery { - Character: Character - Human: Human - Giraffe: Giraffe - Dog: Dog - } - """ - ) - compare_schema_texts_order_independently( - self, renamed_schema_string, print_ast(renamed_schema.schema_ast) - ) - self.assertEqual({}, renamed_schema.reverse_name_map) - self.assertEqual({}, renamed_schema.reverse_field_name_map) + with self.assertRaises(NotImplementedError): + rename_schema(parse(ISS.various_types_schema), {"Character": None}, {}) def test_suppress_interface_and_implementations(self) -> None: - # Note how the Creature interface remains in the schema but, since types implementing it - # were suppressed, it's impossible to query specifically for Creature. - renamed_schema = rename_schema( + with self.assertRaises(NotImplementedError): + rename_schema( parse(ISS.various_types_schema), - {"Giraffe": None, "Character": None, "Human": None, "AbstractCreature": None}, + {"Giraffe": None, "Character": None, "Human": None}, {}, ) - renamed_schema_string = dedent( - """\ - schema { - query: SchemaQuery - } - - scalar Date - - enum Height { - TALL - SHORT - } - - interface Creature { - name: String - age: Int - } - - type Dog { - nickname: String - } - - directive @stitch(source_field: String!, sink_field: String!) on FIELD_DEFINITION - - type SchemaQuery { - Dog: Dog - } - """ - ) - compare_schema_texts_order_independently( - self, renamed_schema_string, print_ast(renamed_schema.schema_ast) - ) - self.assertEqual({}, renamed_schema.reverse_name_map) - self.assertEqual({}, renamed_schema.reverse_field_name_map) def test_multiple_interfaces_rename(self) -> None: renamed_schema = rename_schema( From b1c18cc5f577c86fdc302869165734c6ae0b48f4 Mon Sep 17 00:00:00 2001 From: Leon Wu Date: Fri, 19 Mar 2021 11:07:04 -0400 Subject: [PATCH 06/21] Replace hack with proper code --- .../schema_transformation/rename_schema.py | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/graphql_compiler/schema_transformation/rename_schema.py b/graphql_compiler/schema_transformation/rename_schema.py index 47c7f6b41..a60fce25d 100644 --- a/graphql_compiler/schema_transformation/rename_schema.py +++ b/graphql_compiler/schema_transformation/rename_schema.py @@ -516,8 +516,13 @@ def _rename_and_suppress_types_and_fields( ] = 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(build_ast_schema(schema_ast), node))) + interfaces_to_make_unqueryable.update(set(_recursively_get_ancestor_interface_names(schema_ast, node, interface_name_to_definition_node_map))) return ( renamed_schema_ast, @@ -555,17 +560,12 @@ def _rename_and_suppress_query_type_fields( return renamed_schema_ast -def _recursively_get_ancestor_interface_names(schema: GraphQLSchema, node: Union[ObjectTypeDefinitionNode, InterfaceTypeDefinitionNode]) -> Generator[str, None, None]: +def _recursively_get_ancestor_interface_names(schema: DocumentNode, node: Union[ObjectTypeDefinitionNode, InterfaceTypeDefinitionNode], interface_name_to_definition_node_map: Dict[str, InterfaceTypeDefinitionNode]) -> Generator[str, None, None]: """Get all ancestor interface type names for the given node.""" - for node_implemented_interface in node.interfaces: - yield node_implemented_interface.name.value - # HACK(Leon): The reason for the code below and the reason why we can't simply recursively yield using just interface itself as the new node is that GraphQL-core currently stores interfaces as a list of NamedNodes rather than a list of InterfaceTypeDefinitionNodes, and this is actually the only way currently to find the actual InterfaceTypeDefinitionNode corresponding to interface. - for _, interface_implementation in schema._implementations_map.items(): - for interface in interface_implementation.interfaces: - if interface.name == node_implemented_interface.name.value: - # If the name matches, then interface.ast_node is the node in the schema corresponding to the one described by the NameNode node_implemented_interface. - yield from _recursively_get_ancestor_interface_names(schema, interface.ast_node) - # Note that interface_implementation also has a field called objects corresponding to the object types in the schema that implement the interface whose name is the key in schema._implementations_map. However, the NameNode node_implemented_interface necessarily describes an interface type, so we don't need to check the objects that implement interface_implementation is implemented to see if node_implemented_interface corresponds to an object type 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): From ae662c13a9bde417bb6e5eb055c0b8ad50861bfe Mon Sep 17 00:00:00 2001 From: Leon Wu Date: Fri, 19 Mar 2021 11:13:03 -0400 Subject: [PATCH 07/21] lint --- .../schema_transformation/rename_schema.py | 70 ++++++++++++++----- .../test_rename_schema.py | 12 ++-- 2 files changed, 56 insertions(+), 26 deletions(-) diff --git a/graphql_compiler/schema_transformation/rename_schema.py b/graphql_compiler/schema_transformation/rename_schema.py index a60fce25d..9535dea63 100644 --- a/graphql_compiler/schema_transformation/rename_schema.py +++ b/graphql_compiler/schema_transformation/rename_schema.py @@ -124,7 +124,7 @@ """ from collections import namedtuple from copy import copy -from typing import Any, Dict, List, Mapping, Optional, Set, Tuple, Union, cast, Generator +from typing import Any, Dict, Generator, List, Mapping, Optional, Set, Tuple, Union, cast from graphql import ( DocumentNode, @@ -134,7 +134,7 @@ Node, ObjectTypeDefinitionNode, UnionTypeDefinitionNode, - build_ast_schema, GraphQLSchema, + build_ast_schema, ) from graphql.language.visitor import IDLE, REMOVE, Visitor, VisitorAction, visit from graphql.pyutils import FrozenList @@ -243,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, interfaces_to_make_unqueryable = _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, interfaces_to_make_unqueryable) + 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), @@ -349,10 +356,7 @@ def _ensure_no_unsupported_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 - ): + 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 = [ @@ -522,18 +526,27 @@ def _rename_and_suppress_types_and_fields( 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))) + 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 + interfaces_to_make_unqueryable, ) def _rename_and_suppress_query_type_fields( - schema_ast: DocumentNode, type_renamings: Mapping[str, Optional[str]], query_type: str, interfaces_to_make_unqueryable: Set[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. @@ -555,17 +568,27 @@ def _rename_and_suppress_query_type_fields( Raises: - SchemaTransformError if type_renamings suppressed every type """ - visitor = RenameQueryTypeFieldsVisitor(type_renamings, query_type, interfaces_to_make_unqueryable) + 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(schema: DocumentNode, node: Union[ObjectTypeDefinitionNode, InterfaceTypeDefinitionNode], interface_name_to_definition_node_map: Dict[str, InterfaceTypeDefinitionNode]) -> Generator[str, None, None]: +def _recursively_get_ancestor_interface_names( + schema: DocumentNode, + node: Union[ObjectTypeDefinitionNode, InterfaceTypeDefinitionNode], + interface_name_to_definition_node_map: Dict[str, InterfaceTypeDefinitionNode], +) -> Generator[str, None, None]: """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) + 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): @@ -706,7 +729,9 @@ class RenameSchemaTypesVisitor(Visitor): # 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". - suppressed_types_implementing_interfaces = Set[Union[ObjectTypeDefinitionNode, InterfaceTypeDefinitionNode]] + suppressed_types_implementing_interfaces: Set[ + Union[ObjectTypeDefinitionNode, InterfaceTypeDefinitionNode] + ] def __init__( self, @@ -775,7 +800,10 @@ 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: + if ( + isinstance(node, (ObjectTypeDefinitionNode, InterfaceTypeDefinitionNode)) + and node.interfaces + ): self.suppressed_types_implementing_interfaces.add(node) self.suppressed_type_names.add(type_name) return REMOVE @@ -900,8 +928,12 @@ def enter( class RenameQueryTypeFieldsVisitor(Visitor): - def __init__(self, type_renamings: Mapping[str, Optional[str]], query_type: str, interfaces_to_make_unqueryable: Set[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: diff --git a/graphql_compiler/tests/schema_transformation_tests/test_rename_schema.py b/graphql_compiler/tests/schema_transformation_tests/test_rename_schema.py index 68d54ea7f..ee4058bc3 100644 --- a/graphql_compiler/tests/schema_transformation_tests/test_rename_schema.py +++ b/graphql_compiler/tests/schema_transformation_tests/test_rename_schema.py @@ -465,9 +465,7 @@ def test_field_renaming_illegal_noop_rename_fields_of_suppressed_type(self) -> N def test_field_renaming_in_interfaces(self) -> None: with self.assertRaises(NotImplementedError): - rename_schema( - parse(ISS.various_types_schema), {}, {"Character": {"id": {"new_id"}}} - ) + rename_schema(parse(ISS.various_types_schema), {}, {"Character": {"id": {"new_id"}}}) with self.assertRaises(NotImplementedError): rename_schema( parse(ISS.various_types_schema), {}, {"Character": {"id": {"id", "new_id"}}} @@ -479,9 +477,7 @@ def test_field_renaming_in_interfaces(self) -> None: # for object types that implement interfaces isn't supported yet. rename_schema(parse(ISS.various_types_schema), {}, {"Human": {"id": {"new_id"}}}) with self.assertRaises(NotImplementedError): - rename_schema( - parse(ISS.various_types_schema), {}, {"Human": {"id": {"id", "new_id"}}} - ) + rename_schema(parse(ISS.various_types_schema), {}, {"Human": {"id": {"id", "new_id"}}}) with self.assertRaises(NotImplementedError): rename_schema(parse(ISS.various_types_schema), {}, {"Human": {"id": set()}}) @@ -609,7 +605,9 @@ def test_suppress_interface_implementation(self) -> None: self.assertEqual({}, renamed_schema.reverse_field_name_map) def test_suppress_all_implementations_but_not_interface(self) -> None: - renamed_schema = rename_schema(parse(ISS.various_types_schema), {"Giraffe": None, "Human": None}, {}) + renamed_schema = rename_schema( + parse(ISS.various_types_schema), {"Giraffe": None, "Human": None}, {} + ) renamed_schema_string = dedent( """\ schema { From 9a321bf42ca8a0ad1a12d15842d73df2f50fb03b Mon Sep 17 00:00:00 2001 From: Leon Wu Date: Fri, 19 Mar 2021 11:19:23 -0400 Subject: [PATCH 08/21] Clean up comments --- .../schema_transformation/rename_schema.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/graphql_compiler/schema_transformation/rename_schema.py b/graphql_compiler/schema_transformation/rename_schema.py index 9535dea63..29b2d0489 100644 --- a/graphql_compiler/schema_transformation/rename_schema.py +++ b/graphql_compiler/schema_transformation/rename_schema.py @@ -92,12 +92,12 @@ 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 - be removed from 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 result in - accessing 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. + 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. From cffdb7bc1f4469b65ddbc6d5afd51d4b7c6b585b Mon Sep 17 00:00:00 2001 From: Leon Wu Date: Wed, 24 Mar 2021 16:09:49 -0400 Subject: [PATCH 09/21] Address comments --- .../schema_transformation/rename_schema.py | 37 ++++++++++--------- 1 file changed, 20 insertions(+), 17 deletions(-) diff --git a/graphql_compiler/schema_transformation/rename_schema.py b/graphql_compiler/schema_transformation/rename_schema.py index 29b2d0489..6caaf48c5 100644 --- a/graphql_compiler/schema_transformation/rename_schema.py +++ b/graphql_compiler/schema_transformation/rename_schema.py @@ -124,7 +124,7 @@ """ from collections import namedtuple from copy import copy -from typing import Any, Dict, Generator, List, Mapping, Optional, Set, Tuple, Union, cast +from typing import Any, Dict, List, Mapping, Optional, Set, Tuple, Union, cast, Iterable from graphql import ( DocumentNode, @@ -272,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 enum or interface 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. @@ -404,10 +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, 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 - suppressed. 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 @@ -558,8 +561,8 @@ 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 + 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: @@ -579,7 +582,7 @@ def _recursively_get_ancestor_interface_names( schema: DocumentNode, node: Union[ObjectTypeDefinitionNode, InterfaceTypeDefinitionNode], interface_name_to_definition_node_map: Dict[str, InterfaceTypeDefinitionNode], -) -> Generator[str, None, None]: +) -> Iterable[str, None, None]: """Get all ancestor interface type names for the given node.""" for interface_name_node in node.interfaces: yield interface_name_node.name.value @@ -725,10 +728,10 @@ 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". + # 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] ] @@ -941,9 +944,9 @@ def __init__( 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. + 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 From 499d4b52ae2374d56e965e00b710d8db4ad3ca31 Mon Sep 17 00:00:00 2001 From: Leon Wu Date: Wed, 24 Mar 2021 16:09:59 -0400 Subject: [PATCH 10/21] lint --- graphql_compiler/schema_transformation/rename_schema.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/graphql_compiler/schema_transformation/rename_schema.py b/graphql_compiler/schema_transformation/rename_schema.py index 6caaf48c5..436c69243 100644 --- a/graphql_compiler/schema_transformation/rename_schema.py +++ b/graphql_compiler/schema_transformation/rename_schema.py @@ -124,7 +124,7 @@ """ from collections import namedtuple from copy import copy -from typing import Any, Dict, List, Mapping, Optional, Set, Tuple, Union, cast, Iterable +from typing import Any, Dict, Iterable, List, Mapping, Optional, Set, Tuple, Union, cast from graphql import ( DocumentNode, From 269ab079a22e8252298536c7cbad92c681a312b7 Mon Sep 17 00:00:00 2001 From: Leon Wu Date: Wed, 24 Mar 2021 16:10:46 -0400 Subject: [PATCH 11/21] typo --- graphql_compiler/schema_transformation/rename_schema.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/graphql_compiler/schema_transformation/rename_schema.py b/graphql_compiler/schema_transformation/rename_schema.py index 436c69243..033ce1573 100644 --- a/graphql_compiler/schema_transformation/rename_schema.py +++ b/graphql_compiler/schema_transformation/rename_schema.py @@ -582,7 +582,7 @@ def _recursively_get_ancestor_interface_names( schema: DocumentNode, node: Union[ObjectTypeDefinitionNode, InterfaceTypeDefinitionNode], interface_name_to_definition_node_map: Dict[str, InterfaceTypeDefinitionNode], -) -> Iterable[str, None, None]: +) -> 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 From 70e02cea4b8b67feb3253034f47690825176322a Mon Sep 17 00:00:00 2001 From: Leon Wu Date: Tue, 30 Mar 2021 17:36:29 -0400 Subject: [PATCH 12/21] Implement cascading suppression error for fields of unqueryable types --- .../schema_transformation/rename_schema.py | 112 +++++++++--------- .../input_schema_strings.py | 39 ++++++ .../test_rename_schema.py | 61 ++++++++++ 3 files changed, 159 insertions(+), 53 deletions(-) diff --git a/graphql_compiler/schema_transformation/rename_schema.py b/graphql_compiler/schema_transformation/rename_schema.py index 033ce1573..a71a37b01 100644 --- a/graphql_compiler/schema_transformation/rename_schema.py +++ b/graphql_compiler/schema_transformation/rename_schema.py @@ -240,14 +240,32 @@ def rename_schema( schema = build_ast_schema(schema_ast) query_type = get_query_type_name(schema) - _validate_renamings(schema_ast, type_renamings, field_renamings, query_type) + 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: + if type_renamings[type_name] is not None: + 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 + ) + ) + ) + + _validate_renamings(schema_ast, type_renamings, field_renamings, query_type, interfaces_to_make_unqueryable) # 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 ) @@ -268,6 +286,7 @@ def _validate_renamings( type_renamings: Mapping[str, Optional[str]], field_renamings: Mapping[str, Mapping[str, Set[str]]], query_type: str, + interfaces_to_make_unqueryable: Set[str], ) -> None: """Validate the type_renamings argument before attempting to rename the schema. @@ -288,12 +307,13 @@ def _validate_renamings( 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 that no query should be able to access because a type implementing the interface was suppressed 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_cascading_type_suppressions(schema_ast, type_renamings, field_renamings, query_type, interfaces_to_make_unqueryable) _ensure_no_unsupported_suppressions(schema_ast, type_renamings) @@ -302,9 +322,10 @@ def _ensure_no_cascading_type_suppressions( type_renamings: Mapping[str, Optional[str]], field_renamings: Mapping[str, Mapping[str, Set[str]]], query_type: str, + interfaces_to_make_unqueryable: Set[str], ) -> None: """Check for situations that would require further suppressions to produce a valid schema.""" - visitor = CascadingSuppressionCheckVisitor(type_renamings, field_renamings, query_type) + visitor = CascadingSuppressionCheckVisitor(type_renamings, field_renamings, query_type, interfaces_to_make_unqueryable) visit(schema_ast, visitor) if visitor.fields_to_suppress or visitor.union_types_to_suppress or visitor.types_to_suppress: error_message_components = [ @@ -386,7 +407,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]], 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. @@ -408,8 +429,6 @@ def _rename_and_suppress_types_and_fields( - 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: @@ -522,26 +541,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, ) @@ -581,16 +584,16 @@ 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 ) @@ -1038,6 +1041,7 @@ def __init__( 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 to check that suppression does not cause an illegal state. @@ -1049,11 +1053,13 @@ 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) + interfaces_to_make_unqueryable: interfaces that no query should be able to access because a type implementing the interface was suppressed """ self.type_renamings = type_renamings self.field_renamings = field_renamings self.query_type = query_type - self.current_type: Optional[str] = None + self.interfaces_to_make_unqueryable = interfaces_to_make_unqueryable + self.current_type: Optional[ObjectTypeDefinitionNode] = None self.fields_to_suppress = {} self.union_types_to_suppress = [] self.types_to_suppress = set() @@ -1067,12 +1073,12 @@ def enter_object_type_definition( ancestors: List[Any], ) -> None: """Record the current type that the visitor is traversing.""" - self.current_type = node.name.value - if self.current_type not in self.field_renamings: + self.current_type = node + if self.current_type.name.value not in self.field_renamings: # No field renamings for current type, so it's impossible for all its fields to have # been suppressed. return - current_type_field_renamings = self.field_renamings[self.current_type] + current_type_field_renamings = self.field_renamings[self.current_type.name.value] for field in node.fields: field_name = field.name.value if ( @@ -1083,7 +1089,7 @@ def enter_object_type_definition( # suppressed, either because field renamings didn't contain an entry for field_name # or if it didn't suppress the field return - self.types_to_suppress.add(self.current_type) + self.types_to_suppress.add(self.current_type.name.value) def leave_object_type_definition( self, @@ -1104,31 +1110,31 @@ def enter_field_definition( path: List[Any], ancestors: List[Any], ) -> None: - """Check that no type Bar contains a field of type Foo, where Foo is suppressed.""" - if self.current_type == self.query_type: - return IDLE - # At a field of a type that is not the query type - field_name = node.name.value - field_type = get_ast_with_non_null_and_list_stripped(node.type).name.value - if self.type_renamings.get(field_type, field_type): - return IDLE - # Reaching this point means this field is of a type to be suppressed. + """Check that no type Bar contains a field of type Foo, where Foo is unqueryable.""" if self.current_type is None: - raise AssertionError( - "Entered a field not in any ObjectTypeDefinition scope because " - "self.current_type is None" - ) - if self.current_type == field_type: - # Then node corresponds to a field belonging to type T that is also of type T. - # Therefore, we don't need to explicitly suppress the field as well and this should not - # raise errors. + # Do nothing if at fields in interfaces because interfaces cannot be suppressed for now. return IDLE - if self.field_renamings.get(self.current_type, {}).get(field_name, {field_name}) == set(): - # Field was also suppressed so this should not raise errors. + current_type_name = self.current_type.name.value + if current_type_name == self.query_type: return IDLE - if self.current_type not in self.fields_to_suppress: - self.fields_to_suppress[self.current_type] = {} - self.fields_to_suppress[self.current_type][field_name] = field_type + # At a field of a type that is not the query type + # A field must be suppressed if its type depends on an unqueryable type. There are two ways for a type to become unqueryable: either the type itself was suppressed, or the type is an interface and another type implementing the interface was suppressed. The field can either be suppressed by suppressing the current type altogether or suppressing just that individual field. + field_name = node.name.value + field_type_name = get_ast_with_non_null_and_list_stripped(node.type).name.value + 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 + current_type_suppressed = self.type_renamings.get(current_type_name, current_type_name) is None + field_suppressed = self.field_renamings.get(current_type_name, {}).get(field_name, {field_name}) == set() + + # Check if the field's type is unqueryable so the field itself needs to be suppressed + if field_type_suppressed or field_type_is_unqueryable_interface: + # Check if the field does actually get suppressed + if current_type_suppressed or field_suppressed: + return IDLE + # Record information in case field is to be suppressed but isn't + if current_type_name not in self.fields_to_suppress: + self.fields_to_suppress[current_type_name] = {} + self.fields_to_suppress[current_type_name][field_name] = field_type_name return IDLE def enter_union_type_definition( diff --git a/graphql_compiler/tests/schema_transformation_tests/input_schema_strings.py b/graphql_compiler/tests/schema_transformation_tests/input_schema_strings.py index 28758bdfa..15f97c1df 100644 --- a/graphql_compiler/tests/schema_transformation_tests/input_schema_strings.py +++ b/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 { diff --git a/graphql_compiler/tests/schema_transformation_tests/test_rename_schema.py b/graphql_compiler/tests/schema_transformation_tests/test_rename_schema.py index ee4058bc3..86c4d3c53 100644 --- a/graphql_compiler/tests/schema_transformation_tests/test_rename_schema.py +++ b/graphql_compiler/tests/schema_transformation_tests/test_rename_schema.py @@ -663,6 +663,67 @@ 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}, + {} + ) + with self.assertRaises(CascadingSuppressionError): + # This renaming is also invalid because it doesn't suppress the Companion type's field + # abstract_companion which is of type AbstractCharacter, but AbstractCharacter is made + # unqueryable in the process of suppressing the Human type + rename_schema( + parse(ISS.interface_typed_field), + {"Human": None}, + {"Giraffe": {"friend": set()}} + ) + # A valid renaming requires suppressing the interface-typed fields as well, to make the + # interfaces unqueryable + renamed_schema = rename_schema( + parse(ISS.interface_typed_field), + {"Human": None}, + {"Giraffe": {"friend": set()}, "Companion": {"abstract_companion": set()}} + ) + renamed_schema_string = dedent( + """ + schema { + query: SchemaQuery + } + + interface AbstractCharacter { + id: String + } + + interface Character { + id: String + } + + type Giraffe implements Character { + id: String + } + + type Companion { + description: String + } + + type SchemaQuery { + Giraffe: Giraffe + Companion: Companion + } + """ + ) + compare_schema_texts_order_independently( + self, renamed_schema_string, print_ast(renamed_schema.schema_ast) + ) + self.assertEqual({}, renamed_schema.reverse_name_map) + self.assertEqual({}, renamed_schema.reverse_field_name_map) + def test_multiple_interfaces_rename(self) -> None: renamed_schema = rename_schema( parse(ISS.various_types_schema), From 16b120fd09d561573c373d5167e28c6d5d56932a Mon Sep 17 00:00:00 2001 From: Leon Wu Date: Tue, 30 Mar 2021 17:47:51 -0400 Subject: [PATCH 13/21] lint --- .../schema_transformation/rename_schema.py | 42 ++++++++++++------- .../test_rename_schema.py | 12 ++---- 2 files changed, 31 insertions(+), 23 deletions(-) diff --git a/graphql_compiler/schema_transformation/rename_schema.py b/graphql_compiler/schema_transformation/rename_schema.py index a71a37b01..2c6e65416 100644 --- a/graphql_compiler/schema_transformation/rename_schema.py +++ b/graphql_compiler/schema_transformation/rename_schema.py @@ -259,14 +259,12 @@ def rename_schema( ) ) - _validate_renamings(schema_ast, type_renamings, field_renamings, query_type, interfaces_to_make_unqueryable) + _validate_renamings( + schema_ast, type_renamings, field_renamings, query_type, interfaces_to_make_unqueryable + ) # 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,) = _rename_and_suppress_types_and_fields( schema_ast, type_renamings, field_renamings, query_type ) @@ -307,13 +305,16 @@ def _validate_renamings( 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 that no query should be able to access because a type implementing the interface was suppressed + interfaces_to_make_unqueryable: interfaces that no query should be able to access because a + type implementing the interface was suppressed 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, interfaces_to_make_unqueryable) + _ensure_no_cascading_type_suppressions( + schema_ast, type_renamings, field_renamings, query_type, interfaces_to_make_unqueryable + ) _ensure_no_unsupported_suppressions(schema_ast, type_renamings) @@ -325,7 +326,9 @@ def _ensure_no_cascading_type_suppressions( interfaces_to_make_unqueryable: Set[str], ) -> None: """Check for situations that would require further suppressions to produce a valid schema.""" - visitor = CascadingSuppressionCheckVisitor(type_renamings, field_renamings, query_type, interfaces_to_make_unqueryable) + visitor = CascadingSuppressionCheckVisitor( + type_renamings, field_renamings, query_type, interfaces_to_make_unqueryable + ) visit(schema_ast, visitor) if visitor.fields_to_suppress or visitor.union_types_to_suppress or visitor.types_to_suppress: error_message_components = [ @@ -584,7 +587,9 @@ def _rename_and_suppress_query_type_fields( def _recursively_get_ancestor_interface_names( schema: DocumentNode, node: Union[ObjectTypeDefinitionNode, InterfaceTypeDefinitionNode], - interface_and_object_type_name_to_definition_node_map: Dict[str, Union[ObjectTypeDefinitionNode, 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: @@ -1053,7 +1058,8 @@ 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) - interfaces_to_make_unqueryable: interfaces that no query should be able to access because a type implementing the interface was suppressed + interfaces_to_make_unqueryable: interfaces that no query should be able to access + because a type implementing the interface was suppressed """ self.type_renamings = type_renamings self.field_renamings = field_renamings @@ -1118,13 +1124,21 @@ def enter_field_definition( if current_type_name == self.query_type: return IDLE # At a field of a type that is not the query type - # A field must be suppressed if its type depends on an unqueryable type. There are two ways for a type to become unqueryable: either the type itself was suppressed, or the type is an interface and another type implementing the interface was suppressed. The field can either be suppressed by suppressing the current type altogether or suppressing just that individual field. + # A field must be suppressed if its type depends on an unqueryable type. There are two ways + # for a type to become unqueryable: either the type itself was suppressed, or the type is an + # interface and another type implementing the interface was suppressed. The field can either + # be suppressed by suppressing the current type altogether or suppressing just that + # individual field. field_name = node.name.value field_type_name = get_ast_with_non_null_and_list_stripped(node.type).name.value 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 - current_type_suppressed = self.type_renamings.get(current_type_name, current_type_name) is None - field_suppressed = self.field_renamings.get(current_type_name, {}).get(field_name, {field_name}) == set() + current_type_suppressed = ( + self.type_renamings.get(current_type_name, current_type_name) is None + ) + field_suppressed = ( + self.field_renamings.get(current_type_name, {}).get(field_name, {field_name}) == set() + ) # Check if the field's type is unqueryable so the field itself needs to be suppressed if field_type_suppressed or field_type_is_unqueryable_interface: diff --git a/graphql_compiler/tests/schema_transformation_tests/test_rename_schema.py b/graphql_compiler/tests/schema_transformation_tests/test_rename_schema.py index 86c4d3c53..0a0f57c3f 100644 --- a/graphql_compiler/tests/schema_transformation_tests/test_rename_schema.py +++ b/graphql_compiler/tests/schema_transformation_tests/test_rename_schema.py @@ -669,26 +669,20 @@ def test_suppress_interface_implementation_when_interface_typed_field_exists(sel # 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}, - {} - ) + rename_schema(parse(ISS.interface_typed_field), {"Human": None}, {}) with self.assertRaises(CascadingSuppressionError): # This renaming is also invalid because it doesn't suppress the Companion type's field # abstract_companion which is of type AbstractCharacter, but AbstractCharacter is made # unqueryable in the process of suppressing the Human type rename_schema( - parse(ISS.interface_typed_field), - {"Human": None}, - {"Giraffe": {"friend": set()}} + parse(ISS.interface_typed_field), {"Human": None}, {"Giraffe": {"friend": set()}} ) # A valid renaming requires suppressing the interface-typed fields as well, to make the # interfaces unqueryable renamed_schema = rename_schema( parse(ISS.interface_typed_field), {"Human": None}, - {"Giraffe": {"friend": set()}, "Companion": {"abstract_companion": set()}} + {"Giraffe": {"friend": set()}, "Companion": {"abstract_companion": set()}}, ) renamed_schema_string = dedent( """ From 80284bdc37322f1131a549c4697f5876963f3295 Mon Sep 17 00:00:00 2001 From: Leon Wu Date: Tue, 30 Mar 2021 17:49:15 -0400 Subject: [PATCH 14/21] clean up test for this particular PR --- .../test_rename_schema.py | 47 ------------------- 1 file changed, 47 deletions(-) diff --git a/graphql_compiler/tests/schema_transformation_tests/test_rename_schema.py b/graphql_compiler/tests/schema_transformation_tests/test_rename_schema.py index 0a0f57c3f..720a3df62 100644 --- a/graphql_compiler/tests/schema_transformation_tests/test_rename_schema.py +++ b/graphql_compiler/tests/schema_transformation_tests/test_rename_schema.py @@ -670,53 +670,6 @@ def test_suppress_interface_implementation_when_interface_typed_field_exists(sel # 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}, {}) - with self.assertRaises(CascadingSuppressionError): - # This renaming is also invalid because it doesn't suppress the Companion type's field - # abstract_companion which is of type AbstractCharacter, but AbstractCharacter is made - # unqueryable in the process of suppressing the Human type - rename_schema( - parse(ISS.interface_typed_field), {"Human": None}, {"Giraffe": {"friend": set()}} - ) - # A valid renaming requires suppressing the interface-typed fields as well, to make the - # interfaces unqueryable - renamed_schema = rename_schema( - parse(ISS.interface_typed_field), - {"Human": None}, - {"Giraffe": {"friend": set()}, "Companion": {"abstract_companion": set()}}, - ) - renamed_schema_string = dedent( - """ - schema { - query: SchemaQuery - } - - interface AbstractCharacter { - id: String - } - - interface Character { - id: String - } - - type Giraffe implements Character { - id: String - } - - type Companion { - description: String - } - - type SchemaQuery { - Giraffe: Giraffe - Companion: Companion - } - """ - ) - compare_schema_texts_order_independently( - self, renamed_schema_string, print_ast(renamed_schema.schema_ast) - ) - self.assertEqual({}, renamed_schema.reverse_name_map) - self.assertEqual({}, renamed_schema.reverse_field_name_map) def test_multiple_interfaces_rename(self) -> None: renamed_schema = rename_schema( From 154c93b59c6c0f8575cba9b2e1adf71bf6287700 Mon Sep 17 00:00:00 2001 From: LWprogramming Date: Mon, 5 Apr 2021 12:11:54 -0400 Subject: [PATCH 15/21] Update graphql_compiler/schema_transformation/rename_schema.py Co-authored-by: chewselene <52711428+chewselene@users.noreply.github.com> --- graphql_compiler/schema_transformation/rename_schema.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/graphql_compiler/schema_transformation/rename_schema.py b/graphql_compiler/schema_transformation/rename_schema.py index 2c6e65416..8e2732fe8 100644 --- a/graphql_compiler/schema_transformation/rename_schema.py +++ b/graphql_compiler/schema_transformation/rename_schema.py @@ -432,7 +432,7 @@ def _rename_and_suppress_types_and_fields( - modified version of the schema AST - renamed type name to original type name map - renamed field name to original field name map - 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 From 3e539249bca5655ed4988a4be23e4c5955ae5382 Mon Sep 17 00:00:00 2001 From: LWprogramming Date: Mon, 5 Apr 2021 12:12:28 -0400 Subject: [PATCH 16/21] Update graphql_compiler/schema_transformation/rename_schema.py Co-authored-by: chewselene <52711428+chewselene@users.noreply.github.com> --- graphql_compiler/schema_transformation/rename_schema.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/graphql_compiler/schema_transformation/rename_schema.py b/graphql_compiler/schema_transformation/rename_schema.py index 8e2732fe8..6fe2b5d42 100644 --- a/graphql_compiler/schema_transformation/rename_schema.py +++ b/graphql_compiler/schema_transformation/rename_schema.py @@ -1123,7 +1123,7 @@ def enter_field_definition( current_type_name = self.current_type.name.value if current_type_name == self.query_type: return IDLE - # At a field of a type that is not the query type + # At a field of a type that is not the query type. # A field must be suppressed if its type depends on an unqueryable type. There are two ways # for a type to become unqueryable: either the type itself was suppressed, or the type is an # interface and another type implementing the interface was suppressed. The field can either From 79f99f49ec74c9bbfcd80924b2a2f3e7149f45f0 Mon Sep 17 00:00:00 2001 From: LWprogramming Date: Mon, 5 Apr 2021 12:13:59 -0400 Subject: [PATCH 17/21] Update graphql_compiler/schema_transformation/rename_schema.py Co-authored-by: chewselene <52711428+chewselene@users.noreply.github.com> --- graphql_compiler/schema_transformation/rename_schema.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/graphql_compiler/schema_transformation/rename_schema.py b/graphql_compiler/schema_transformation/rename_schema.py index 6fe2b5d42..eb7c11469 100644 --- a/graphql_compiler/schema_transformation/rename_schema.py +++ b/graphql_compiler/schema_transformation/rename_schema.py @@ -1125,8 +1125,10 @@ def enter_field_definition( return IDLE # At a field of a type that is not the query type. # A field must be suppressed if its type depends on an unqueryable type. There are two ways - # for a type to become unqueryable: either the type itself was suppressed, or the type is an - # interface and another type implementing the interface was suppressed. The field can either + # for a type to become unqueryable: + # - the type itself was suppressed + # - the type is an interface and another type implementing the interface was suppressed + # The field can either # be suppressed by suppressing the current type altogether or suppressing just that # individual field. field_name = node.name.value From 658a2c5e1bdbd33e3f178c1e57af0e183f92a288 Mon Sep 17 00:00:00 2001 From: Leon Wu Date: Tue, 6 Apr 2021 11:04:04 -0400 Subject: [PATCH 18/21] remove trailing space --- graphql_compiler/schema_transformation/rename_schema.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/graphql_compiler/schema_transformation/rename_schema.py b/graphql_compiler/schema_transformation/rename_schema.py index eb7c11469..6deec04cf 100644 --- a/graphql_compiler/schema_transformation/rename_schema.py +++ b/graphql_compiler/schema_transformation/rename_schema.py @@ -1125,7 +1125,7 @@ def enter_field_definition( return IDLE # At a field of a type that is not the query type. # A field must be suppressed if its type depends on an unqueryable type. There are two ways - # for a type to become unqueryable: + # for a type to become unqueryable: # - the type itself was suppressed # - the type is an interface and another type implementing the interface was suppressed # The field can either From ee3048cc0aed4d56926cebfb0259473901276b87 Mon Sep 17 00:00:00 2001 From: Leon Wu Date: Tue, 6 Apr 2021 11:18:33 -0400 Subject: [PATCH 19/21] address comments --- .../schema_transformation/rename_schema.py | 22 ++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/graphql_compiler/schema_transformation/rename_schema.py b/graphql_compiler/schema_transformation/rename_schema.py index 6deec04cf..bedfa94ad 100644 --- a/graphql_compiler/schema_transformation/rename_schema.py +++ b/graphql_compiler/schema_transformation/rename_schema.py @@ -240,6 +240,15 @@ 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 @@ -247,7 +256,8 @@ def rename_schema( if isinstance(node, (ObjectTypeDefinitionNode, InterfaceTypeDefinitionNode)) } for type_name in type_renamings: - if type_renamings[type_name] is not None: + 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] @@ -305,8 +315,9 @@ def _validate_renamings( 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 that no query should be able to access because a - type implementing the interface was suppressed + 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: - CascadingSuppressionError if a type/field suppression would require further suppressions @@ -1058,8 +1069,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) - interfaces_to_make_unqueryable: interfaces that no query should be able to access - because a type implementing the interface was suppressed + 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.field_renamings = field_renamings From 2d997eed11703123918de0bb01eac61ee9dad94d Mon Sep 17 00:00:00 2001 From: Leon Wu Date: Mon, 12 Apr 2021 13:57:26 -0400 Subject: [PATCH 20/21] lint --- .../schema_transformation/rename_schema.py | 23 ++++++++++++------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/graphql_compiler/schema_transformation/rename_schema.py b/graphql_compiler/schema_transformation/rename_schema.py index 497cc051b..5cc34dcb2 100644 --- a/graphql_compiler/schema_transformation/rename_schema.py +++ b/graphql_compiler/schema_transformation/rename_schema.py @@ -286,6 +286,7 @@ def rename_schema( reverse_field_name_map=reverse_field_name_map, ) + def _ensure_no_unsupported_suppressions( schema_ast: DocumentNode, type_renamings: Mapping[str, Optional[str]] ) -> None: @@ -322,7 +323,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, - interfaces_to_make_unqueryable: 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. @@ -355,7 +356,9 @@ def _rename_and_suppress_types_and_fields( - SchemaRenameNameConflictError if the rename causes name conflicts - NoOpRenamingError if renamings contains no-op renamings """ - visitor = RenameSchemaTypesVisitor(type_renamings, field_renamings, query_type, interfaces_to_make_unqueryable) + 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 @@ -713,7 +716,7 @@ def __init__( type_renamings: Mapping[str, Optional[str]], field_renamings: Mapping[str, Mapping[str, Set[str]]], query_type: str, - interfaces_to_make_unqueryable: Set[str] + interfaces_to_make_unqueryable: Set[str], ) -> None: """Create a visitor for renaming types in a schema AST. @@ -725,9 +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. + 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 = {} @@ -872,12 +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_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 or field_type_is_unqueryable_interface) 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 != {}: From 22bd925d89f77648279ab466c0dba78185b67e1e Mon Sep 17 00:00:00 2001 From: Leon Wu Date: Thu, 20 May 2021 10:52:01 -0400 Subject: [PATCH 21/21] Address comments --- .../schema_transformation/rename_schema.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/graphql_compiler/schema_transformation/rename_schema.py b/graphql_compiler/schema_transformation/rename_schema.py index 5cc34dcb2..6747e8092 100644 --- a/graphql_compiler/schema_transformation/rename_schema.py +++ b/graphql_compiler/schema_transformation/rename_schema.py @@ -91,6 +91,8 @@ - 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. + - The same goes for an interface type Foo made unqueryable in the process of suppressing a type + that implements Foo: no fields of type Foo may remain in the schema after applying renamings. - 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 @@ -256,10 +258,14 @@ def rename_schema( 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_suppressed = type_renamings[type_name] is None + if type_suppressed and type_name in interface_and_object_type_name_to_definition_node_map: + # This second condition is required in case there is no interface or object type named + # type_name, in which case we would then have an illegal renaming. However, the error + # handling/ error message comes later in the execution of rename_schema(), as we want to + # traverse the schema using a RenameSchemaTypesVisitor and find unused renamings in the + # process of attempting to produce a valid renamed schema, rather than having a separate + # validation step here. type_node = interface_and_object_type_name_to_definition_node_map[type_name] interfaces_to_make_unqueryable.update( set(