From 44e314b55545730729fa441cc4f73d1b3407c20d Mon Sep 17 00:00:00 2001 From: Leon Wu Date: Wed, 27 May 2020 10:19:51 -0400 Subject: [PATCH 01/29] Suppress type tests --- .../schema_transformation/utils.py | 7 + .../input_schema_strings.py | 28 ++++ .../test_merge_schemas.py | 5 + .../test_rename_schema.py | 158 +++++++++++++++++- 4 files changed, 197 insertions(+), 1 deletion(-) diff --git a/graphql_compiler/schema_transformation/utils.py b/graphql_compiler/schema_transformation/utils.py index 463a2267c..28ee5a5ed 100644 --- a/graphql_compiler/schema_transformation/utils.py +++ b/graphql_compiler/schema_transformation/utils.py @@ -57,6 +57,13 @@ class InvalidCrossSchemaEdgeError(SchemaTransformError): """ +class CascadingSuppressionError(SchemaTransformError): + """Raised if the schema suppressions would result in empty types that would need to be, in turn, suppressed (possibly leading to a cascade of suppressions). + + This may be raised if all the fields of a type are suppressed but the type itself is not suppressed, if all the members of a union are suppressed but the union itself is not suppressed, or if a type is suppressed but a field of that type is not suppressed. + """ + + _alphanumeric_and_underscore = frozenset(six.text_type(string.ascii_letters + string.digits + "_")) 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 39b71f5be..60ae534a3 100644 --- a/graphql_compiler/tests/schema_transformation_tests/input_schema_strings.py +++ b/graphql_compiler/tests/schema_transformation_tests/input_schema_strings.py @@ -202,8 +202,13 @@ class InputSchemaStrings(object): friend: Dog! } + type Cat { + id: String + } + type SchemaQuery { Dog: Dog! + Cat: Cat } """ ) @@ -371,3 +376,26 @@ class InputSchemaStrings(object): } """ ) + + multiple_fields_schema = dedent( + """\ + schema { + query: SchemaQuery + } + + type Human { + id: String + name: String + pet: Dog + } + + type Dog { + nickname: String + } + + type SchemaQuery { + Human: Human + Dog: Dog + } + """ + ) diff --git a/graphql_compiler/tests/schema_transformation_tests/test_merge_schemas.py b/graphql_compiler/tests/schema_transformation_tests/test_merge_schemas.py index 9bc752973..6f77d29a0 100644 --- a/graphql_compiler/tests/schema_transformation_tests/test_merge_schemas.py +++ b/graphql_compiler/tests/schema_transformation_tests/test_merge_schemas.py @@ -88,6 +88,7 @@ def test_multiple_merge(self): Character: Character Kid: Kid Dog: Dog! + Cat: Cat } directive @stitch(source_field: String!, sink_field: String!) on FIELD_DEFINITION @@ -117,6 +118,10 @@ def test_multiple_merge(self): id: String! friend: Dog! } + + type Cat { + id: String + } """ ) self.assertEqual(merged_schema_string, print_ast(merged_schema.schema_ast)) 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 5e57874a5..24b069253 100644 --- a/graphql_compiler/tests/schema_transformation_tests/test_rename_schema.py +++ b/graphql_compiler/tests/schema_transformation_tests/test_rename_schema.py @@ -8,7 +8,7 @@ from graphql.pyutils import snake_to_camel from ...schema_transformation.rename_schema import RenameSchemaTypesVisitor, rename_schema -from ...schema_transformation.utils import InvalidTypeNameError, SchemaNameConflictError +from ...schema_transformation.utils import InvalidTypeNameError, SchemaNameConflictError, CascadingSuppressionError from .input_schema_strings import InputSchemaStrings as ISS @@ -59,6 +59,53 @@ def test_original_unmodified(self): rename_schema(original_ast, {"Human": "NewHuman"}) self.assertEqual(original_ast, parse(ISS.basic_schema)) + def test_basic_suppress(self): + renamed_schema = rename_schema(parse(ISS.multiple_objects_schema), {"Human": None}) + renamed_schema_string = dedent( + """\ + schema { + query: SchemaQuery + } + + type Droid { + id: String + } + + type Dog { + nickname: String + } + + type SchemaQuery { + Droid: Droid + Dog: Dog + } + """ + ) + self.assertEqual(renamed_schema_string, print_ast(renamed_schema.schema_ast)) + self.assertEqual({}, renamed_schema.reverse_name_map) + + def test_multiple_type_suppress(self): + renamed_schema = rename_schema( + parse(ISS.multiple_objects_schema), {"Human": None, "Droid": None} + ) + renamed_schema_string = dedent( + """\ + schema { + query: SchemaQuery + } + + type Dog { + nickname: String + } + + type SchemaQuery { + Dog: Dog + } + """ + ) + self.assertEqual(renamed_schema_string, print_ast(renamed_schema.schema_ast)) + self.assertEqual({}, renamed_schema.reverse_name_map) + def test_swap_rename(self): renamed_schema = rename_schema( parse(ISS.multiple_objects_schema), {"Human": "Droid", "Droid": "Human"} @@ -279,6 +326,36 @@ def test_union_rename(self): renamed_schema.reverse_name_map, ) + def test_entire_union_suppress(self): + renamed_schema = rename_schema( + parse(ISS.union_schema), {"HumanOrDroid": None, "Droid": "NewDroid"} + ) + renamed_schema_string = dedent( + """\ + schema { + query: SchemaQuery + } + + type Human { + id: String + } + + type NewDroid { + id: String + } + + type SchemaQuery { + Human: Human + NewDroid: NewDroid + } + """ + ) + self.assertEqual(renamed_schema_string, print_ast(renamed_schema.schema_ast)) + self.assertEqual( + {"NewDroid": "Droid"}, + renamed_schema.reverse_name_map, + ) + def test_list_rename(self): renamed_schema = rename_schema( parse(ISS.list_schema), @@ -327,6 +404,48 @@ def test_list_rename(self): renamed_schema.reverse_name_map, ) + def test_list_suppress(self): + renamed_schema = rename_schema( + parse(ISS.list_schema), + { + "Droid": "NewDroid", + "Character": "NewCharacter", + "Height": None, + "Date": "NewDate", + "id": "NewId", + "String": "NewString", + }, + ) + renamed_schema_string = dedent( + """\ + schema { + query: SchemaQuery + } + + type NewDroid implements NewCharacter { + id: String + dates: [Date] + friends: [NewDroid] + enemies: [NewCharacter] + } + + type SchemaQuery { + NewDroid: [NewDroid] + } + + scalar Date + + interface NewCharacter { + id: String + } + """ + ) + self.assertEqual(renamed_schema_string, print_ast(renamed_schema.schema_ast)) + self.assertEqual( + {"NewCharacter": "Character", "NewDroid": "Droid",}, + renamed_schema.reverse_name_map, + ) + def test_non_null_rename(self): renamed_schema = rename_schema(parse(ISS.non_null_schema), {"Dog": "NewDog"}) renamed_schema_string = dedent( @@ -340,14 +459,39 @@ def test_non_null_rename(self): friend: NewDog! } + type Cat { + id: String + } + type SchemaQuery { NewDog: NewDog! + Cat: Cat } """ ) self.assertEqual(renamed_schema_string, print_ast(renamed_schema.schema_ast)) self.assertEqual({"NewDog": "Dog"}, renamed_schema.reverse_name_map) + def test_non_null_suppress(self): + renamed_schema = rename_schema(parse(ISS.non_null_schema), {"Dog": None}) + renamed_schema_string = dedent( + """\ + schema { + query: SchemaQuery + } + + type Cat { + id: String + } + + type SchemaQuery { + Cat: Cat + } + """ + ) + self.assertEqual(renamed_schema_string, print_ast(renamed_schema.schema_ast)) + self.assertEqual({}, renamed_schema.reverse_name_map) + def test_directive_rename(self): renamed_schema = rename_schema( parse(ISS.directive_schema), @@ -553,6 +697,18 @@ def test_illegal_rename_to_reserved_name_type(self): with self.assertRaises(InvalidTypeNameError): rename_schema(parse(ISS.basic_schema), {"Human": "__Type"}) + def test_suppress_every_type(self): + with self.assertRaises(CascadingSuppressionError): + rename_schema(parse(ISS.basic_schema), {"Human": None}) + + def test_suppress_all_union_members_cascading_error(self): + with self.assertRaises(CascadingSuppressionError): + rename_schema(parse(ISS.union_schema), {"Human": None, "Droid": None}) + + def test_suppress_still_needed_type_cascading_error(self): + with self.assertRaises(CascadingSuppressionError): + rename_schema(parse(ISS.multiple_fields_schema), {"Dog": None}) # a field in Human is of type Dog. + def test_rename_using_dict_like_prefixer_class(self): class PrefixNewDict(object): def get(self, key, default=None): From 2d6edb602b5676feec83b0144b27294f35524b9b Mon Sep 17 00:00:00 2001 From: Leon Wu Date: Thu, 28 May 2020 11:49:24 -0400 Subject: [PATCH 02/29] Fix CascadingSuppressionError tests --- .../test_rename_schema.py | 50 +++---------------- 1 file changed, 6 insertions(+), 44 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 24b069253..f7430c9d3 100644 --- a/graphql_compiler/tests/schema_transformation_tests/test_rename_schema.py +++ b/graphql_compiler/tests/schema_transformation_tests/test_rename_schema.py @@ -404,48 +404,6 @@ def test_list_rename(self): renamed_schema.reverse_name_map, ) - def test_list_suppress(self): - renamed_schema = rename_schema( - parse(ISS.list_schema), - { - "Droid": "NewDroid", - "Character": "NewCharacter", - "Height": None, - "Date": "NewDate", - "id": "NewId", - "String": "NewString", - }, - ) - renamed_schema_string = dedent( - """\ - schema { - query: SchemaQuery - } - - type NewDroid implements NewCharacter { - id: String - dates: [Date] - friends: [NewDroid] - enemies: [NewCharacter] - } - - type SchemaQuery { - NewDroid: [NewDroid] - } - - scalar Date - - interface NewCharacter { - id: String - } - """ - ) - self.assertEqual(renamed_schema_string, print_ast(renamed_schema.schema_ast)) - self.assertEqual( - {"NewCharacter": "Character", "NewDroid": "Droid",}, - renamed_schema.reverse_name_map, - ) - def test_non_null_rename(self): renamed_schema = rename_schema(parse(ISS.non_null_schema), {"Dog": "NewDog"}) renamed_schema_string = dedent( @@ -701,14 +659,18 @@ def test_suppress_every_type(self): with self.assertRaises(CascadingSuppressionError): rename_schema(parse(ISS.basic_schema), {"Human": None}) - def test_suppress_all_union_members_cascading_error(self): + def test_suppress_all_union_members(self): with self.assertRaises(CascadingSuppressionError): rename_schema(parse(ISS.union_schema), {"Human": None, "Droid": None}) - def test_suppress_still_needed_type_cascading_error(self): + def test_field_still_depends_on_suppressed_type(self): with self.assertRaises(CascadingSuppressionError): rename_schema(parse(ISS.multiple_fields_schema), {"Dog": None}) # a field in Human is of type Dog. + def test_field_in_list_still_depends_on_suppressed_type(self): + with self.assertRaises(CascadingSuppressionError): + rename_schema(parse(ISS.list_schema), {"Height": None}) + def test_rename_using_dict_like_prefixer_class(self): class PrefixNewDict(object): def get(self, key, default=None): From e03a281c98cc025bc750d610ea5c0d5349accde2 Mon Sep 17 00:00:00 2001 From: Leon Wu Date: Thu, 28 May 2020 11:58:54 -0400 Subject: [PATCH 03/29] Update union tests --- .../input_schema_strings.py | 28 +++++++++++++++++ .../test_rename_schema.py | 30 ++++++++++++++++++- 2 files changed, 57 insertions(+), 1 deletion(-) 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 60ae534a3..e319931f2 100644 --- a/graphql_compiler/tests/schema_transformation_tests/input_schema_strings.py +++ b/graphql_compiler/tests/schema_transformation_tests/input_schema_strings.py @@ -160,6 +160,34 @@ class InputSchemaStrings(object): """ ) + extended_union_schema = dedent( + """\ + schema { + query: SchemaQuery + } + + type Human { + id: String + } + + type Droid { + id: String + } + + type Dog { + nickname: String + } + + union HumanOrDroid = Human | Droid + + type SchemaQuery { + Human: Human + Droid: Droid + Dog: Dog + } + """ + ) + list_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 f7430c9d3..bfbaf2355 100644 --- a/graphql_compiler/tests/schema_transformation_tests/test_rename_schema.py +++ b/graphql_compiler/tests/schema_transformation_tests/test_rename_schema.py @@ -356,6 +356,33 @@ def test_entire_union_suppress(self): renamed_schema.reverse_name_map, ) + def test_union_member_suppress(self): + renamed_schema = rename_schema( + parse(ISS.union_schema), {"Droid": None} + ) + renamed_schema_string = dedent( + """\ + schema { + query: SchemaQuery + } + + type Human { + id: String + } + + union HumanOrDroid = Human + + type SchemaQuery { + Human: Human + } + """ + ) + self.assertEqual(renamed_schema_string, print_ast(renamed_schema.schema_ast)) + self.assertEqual( + {}, + renamed_schema.reverse_name_map, + ) + def test_list_rename(self): renamed_schema = rename_schema( parse(ISS.list_schema), @@ -661,7 +688,8 @@ def test_suppress_every_type(self): def test_suppress_all_union_members(self): with self.assertRaises(CascadingSuppressionError): - rename_schema(parse(ISS.union_schema), {"Human": None, "Droid": None}) + # Note that we can't use ISS.union_schema directly here because suppressing all the members of the union would mean suppressing every type in general, which means we couldn't be sure what was causing the CascadingSuppressionError. + rename_schema(parse(ISS.extended_union_schema), {"Human": None, "Droid": None}) def test_field_still_depends_on_suppressed_type(self): with self.assertRaises(CascadingSuppressionError): From a621c835cd2c9fba0c8353a10de5b547e74b9458 Mon Sep 17 00:00:00 2001 From: Leon Wu Date: Thu, 28 May 2020 12:24:40 -0400 Subject: [PATCH 04/29] Implement type suppression --- .../schema_transformation/rename_schema.py | 71 ++++++++++++++----- 1 file changed, 53 insertions(+), 18 deletions(-) diff --git a/graphql_compiler/schema_transformation/rename_schema.py b/graphql_compiler/schema_transformation/rename_schema.py index 6a2340bf7..5c1965224 100644 --- a/graphql_compiler/schema_transformation/rename_schema.py +++ b/graphql_compiler/schema_transformation/rename_schema.py @@ -1,8 +1,9 @@ # Copyright 2019-present Kensho Technologies, LLC. from collections import namedtuple +from typing import Optional, Dict -from graphql import build_ast_schema -from graphql.language.visitor import Visitor, visit +from graphql import build_ast_schema, DocumentNode +from graphql.language.visitor import Visitor, visit, REMOVE import six from .utils import ( @@ -11,9 +12,9 @@ check_type_name_is_valid, get_copy_of_node_with_new_name, get_query_type_name, - get_scalar_names, + get_scalar_names, CascadingSuppressionError, ) - +from ..ast_manipulation import get_ast_with_non_null_and_list_stripped RenamedSchemaDescriptor = namedtuple( "RenamedSchemaDescriptor", @@ -26,7 +27,7 @@ ) -def rename_schema(ast, renamings): +def rename_schema(ast: DocumentNode, renamings: Dict[str, Optional[str]]) -> RenamedSchemaDescriptor: """Create a RenamedSchemaDescriptor; types and query type fields are renamed using renamings. Any type, interface, enum, or fields of the root type/query type whose name @@ -38,9 +39,8 @@ def rename_schema(ast, renamings): ast: Document, representing a valid schema that does not contain extensions, input object definitions, mutations, or subscriptions, whose fields of the query type share the same name as the types they query. Not modified by this function - renamings: Dict[str, str], mapping original type/field names to renamed type/field names. - Type or query type field names that do not appear in the dict will be unchanged. - Any dict-like object that implements get(key, [default]) may also be used + renamings: Dict[str, Optional[str]], mapping original type/field names to renamed type/field names or None. + Type or query type field names that do not appear in the dict will be unchanged. If a type is mapped to None, it will be suppressed in the renamed schema and queries will not be able to access it. Any dict-like object that implements get(key, [default]) may also be used Returns: RenamedSchemaDescriptor, a namedtuple that contains the AST of the renamed schema, and the @@ -74,7 +74,8 @@ def rename_schema(ast, renamings): } # Rename query type fields - ast = _rename_query_type_fields(ast, renamings, query_type) + from graphql.language.printer import print_ast + ast = _rename_fields(ast, renamings, query_type) return RenamedSchemaDescriptor( schema_ast=ast, schema=build_ast_schema(ast), @@ -113,8 +114,8 @@ def _rename_types(ast, renamings, query_type, scalars): return renamed_ast, visitor.reverse_name_map -def _rename_query_type_fields(ast, renamings, query_type): - """Rename all fields of the query type. +def _rename_fields(ast, renamings, query_type): + """Rename fields, including query type fields. The input AST will not be modified. @@ -127,7 +128,7 @@ def _rename_query_type_fields(ast, renamings, query_type): Returns: DocumentNode, representing the modified version of the input schema AST """ - visitor = RenameQueryTypeFieldsVisitor(renamings, query_type) + visitor = RenameFieldsVisitor(renamings, query_type) renamed_ast = visit(ast, visitor) return renamed_ast @@ -232,6 +233,9 @@ def _rename_name_and_add_to_record(self, node): return node new_name_string = self.renamings.get(name_string, name_string) # Default use original + if new_name_string is None: + # suppress the type + return REMOVE check_type_name_is_valid(new_name_string) if ( @@ -268,16 +272,16 @@ def enter(self, node, key, parent, path, ancestors): renamed_node = self._rename_name_and_add_to_record(node) if renamed_node is node: # Name unchanged, continue traversal return None - else: # Name changed, return new node, `visit` will make shallow copies along path + else: # Name changed or suppressed, return new node, `visit` will make shallow copies along path return renamed_node else: # All Node types should've been taken care of, this line should never be reached raise AssertionError('Unreachable code reached. Missed type: "{}"'.format(node_type)) -class RenameQueryTypeFieldsVisitor(Visitor): +class RenameFieldsVisitor(Visitor): def __init__(self, renamings, query_type): - """Create a visitor for renaming fields of the query type in a schema AST. + """Create a visitor for renaming fields in a schema AST. Args: renamings: Dict[str, str], from original field name to renamed field name. Any @@ -285,7 +289,7 @@ def __init__(self, renamings, query_type): query_type: str, name of the query type (e.g. RootSchemaQuery) """ # Note that as field names and type names have been confirmed to match up, any renamed - # field already has a corresponding renamed type. If no errors, due to either invalid + # query type field already has a corresponding renamed type. If no errors, due to either invalid # names or name conflicts, were raised when renaming type, no errors will occur when # renaming query type fields. self.in_query_type = False @@ -299,16 +303,47 @@ def enter_object_type_definition(self, node, *args): def leave_object_type_definition(self, node, key, parent, path, ancestors): """If the node's name matches the query type, record that we left the query type.""" + if len(node.fields) == 0: + raise CascadingSuppressionError( + f"Type renamings {self.renamings} suppressed every type in the schema so it will be impossible to query for anything. To fix this, check why the `renamings` argument of `rename_schema` mapped every type to None." + ) if node.name.value == self.query_type: self.in_query_type = False def enter_field_definition(self, node, *args): - """If inside the query type, rename field and add the name pair to reverse_field_map.""" + """Update fields as needed. + + If inside the query type, rename field and add the name pair to reverse_field_map. + If not inside the query type, check that no field depends on a type that was suppressed. + + Raises: + CascadingSuppressionError if a field that was not suppressed depends on a type that was suppressed. + """ + field_name = node.name.value if self.in_query_type: - field_name = node.name.value new_field_name = self.renamings.get(field_name, field_name) # Default use original if new_field_name == field_name: return None + if new_field_name is None: + # suppress the type + return REMOVE else: # Make copy of node with the changed name, return the copy field_node_with_new_name = get_copy_of_node_with_new_name(node, new_field_name) return field_node_with_new_name + else: + # at a field of a type that is not the query type + type = get_ast_with_non_null_and_list_stripped(node.type) + if type == REMOVE: + # then this field depends on a type that was suppressed, which is illegal + type_name = args[-1][-1].name.value + raise CascadingSuppressionError( + f"Type renamings {self.renamings} attempted to suppress a type, but type {type_name}'s field {field_name} still depends on that type. Suppressing individual fields hasn't been implemented yet, but when it is, you can fix this error by suppressing the field as well." + ) + return None + + def enter_union_type_definition(self, node, *args): + union_name = node.name.value + if len(node.types) == 0: + raise CascadingSuppressionError( + f"Type renamings {self.renamings} suppressed all types belonging to the union {union_name}. To fix this, you can suppress the union as well by adding {union_name}: None to the `renamings` argument of `rename_schema`." + ) From f8c78df3bf5753e20f4ed4be2b8595a212e26ea5 Mon Sep 17 00:00:00 2001 From: Leon Wu Date: Thu, 28 May 2020 12:28:06 -0400 Subject: [PATCH 05/29] lint --- .../schema_transformation/rename_schema.py | 17 ++++++++++------ .../test_rename_schema.py | 20 ++++++++++--------- 2 files changed, 22 insertions(+), 15 deletions(-) diff --git a/graphql_compiler/schema_transformation/rename_schema.py b/graphql_compiler/schema_transformation/rename_schema.py index 5c1965224..e4db51368 100644 --- a/graphql_compiler/schema_transformation/rename_schema.py +++ b/graphql_compiler/schema_transformation/rename_schema.py @@ -1,20 +1,22 @@ # Copyright 2019-present Kensho Technologies, LLC. from collections import namedtuple -from typing import Optional, Dict +from typing import Dict, Optional -from graphql import build_ast_schema, DocumentNode -from graphql.language.visitor import Visitor, visit, REMOVE +from graphql import DocumentNode, build_ast_schema +from graphql.language.visitor import REMOVE, Visitor, visit import six +from ..ast_manipulation import get_ast_with_non_null_and_list_stripped from .utils import ( + CascadingSuppressionError, SchemaNameConflictError, check_ast_schema_is_valid, check_type_name_is_valid, get_copy_of_node_with_new_name, get_query_type_name, - get_scalar_names, CascadingSuppressionError, + get_scalar_names, ) -from ..ast_manipulation import get_ast_with_non_null_and_list_stripped + RenamedSchemaDescriptor = namedtuple( "RenamedSchemaDescriptor", @@ -27,7 +29,9 @@ ) -def rename_schema(ast: DocumentNode, renamings: Dict[str, Optional[str]]) -> RenamedSchemaDescriptor: +def rename_schema( + ast: DocumentNode, renamings: Dict[str, Optional[str]] +) -> RenamedSchemaDescriptor: """Create a RenamedSchemaDescriptor; types and query type fields are renamed using renamings. Any type, interface, enum, or fields of the root type/query type whose name @@ -75,6 +79,7 @@ def rename_schema(ast: DocumentNode, renamings: Dict[str, Optional[str]]) -> Ren # Rename query type fields from graphql.language.printer import print_ast + ast = _rename_fields(ast, renamings, query_type) return RenamedSchemaDescriptor( schema_ast=ast, 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 bfbaf2355..8cbb892bf 100644 --- a/graphql_compiler/tests/schema_transformation_tests/test_rename_schema.py +++ b/graphql_compiler/tests/schema_transformation_tests/test_rename_schema.py @@ -8,7 +8,11 @@ from graphql.pyutils import snake_to_camel from ...schema_transformation.rename_schema import RenameSchemaTypesVisitor, rename_schema -from ...schema_transformation.utils import InvalidTypeNameError, SchemaNameConflictError, CascadingSuppressionError +from ...schema_transformation.utils import ( + CascadingSuppressionError, + InvalidTypeNameError, + SchemaNameConflictError, +) from .input_schema_strings import InputSchemaStrings as ISS @@ -352,14 +356,11 @@ def test_entire_union_suppress(self): ) self.assertEqual(renamed_schema_string, print_ast(renamed_schema.schema_ast)) self.assertEqual( - {"NewDroid": "Droid"}, - renamed_schema.reverse_name_map, + {"NewDroid": "Droid"}, renamed_schema.reverse_name_map, ) def test_union_member_suppress(self): - renamed_schema = rename_schema( - parse(ISS.union_schema), {"Droid": None} - ) + renamed_schema = rename_schema(parse(ISS.union_schema), {"Droid": None}) renamed_schema_string = dedent( """\ schema { @@ -379,8 +380,7 @@ def test_union_member_suppress(self): ) self.assertEqual(renamed_schema_string, print_ast(renamed_schema.schema_ast)) self.assertEqual( - {}, - renamed_schema.reverse_name_map, + {}, renamed_schema.reverse_name_map, ) def test_list_rename(self): @@ -693,7 +693,9 @@ def test_suppress_all_union_members(self): def test_field_still_depends_on_suppressed_type(self): with self.assertRaises(CascadingSuppressionError): - rename_schema(parse(ISS.multiple_fields_schema), {"Dog": None}) # a field in Human is of type Dog. + rename_schema( + parse(ISS.multiple_fields_schema), {"Dog": None} + ) # a field in Human is of type Dog. def test_field_in_list_still_depends_on_suppressed_type(self): with self.assertRaises(CascadingSuppressionError): From 9fed25ef1a2c6330df96728d9b90999b43e30096 Mon Sep 17 00:00:00 2001 From: Leon Wu Date: Thu, 28 May 2020 14:30:40 -0400 Subject: [PATCH 06/29] fix lint issues --- .../schema_transformation/rename_schema.py | 51 ++++++++++++------- .../schema_transformation/utils.py | 6 ++- .../input_schema_strings.py | 4 +- .../test_rename_schema.py | 5 +- 4 files changed, 44 insertions(+), 22 deletions(-) diff --git a/graphql_compiler/schema_transformation/rename_schema.py b/graphql_compiler/schema_transformation/rename_schema.py index e4db51368..99db1516a 100644 --- a/graphql_compiler/schema_transformation/rename_schema.py +++ b/graphql_compiler/schema_transformation/rename_schema.py @@ -35,16 +35,20 @@ def rename_schema( """Create a RenamedSchemaDescriptor; types and query type fields are renamed using renamings. Any type, interface, enum, or fields of the root type/query type whose name - appears in renamings will be renamed to the corresponding value. Any such names that do not - appear in renamings will be unchanged. Scalars, directives, enum values, and fields not - belonging to the root/query type will never be renamed. + appears in renamings will be renamed to the corresponding value if the value is not None. If the + value is None, it will be suppressed in the renamed schema and queries will not be able to + access it. + Any such names that do not appear in renamings will be unchanged. + Scalars, directives, enum values, and fields not belonging to the root/query type will never be + renamed. Args: ast: Document, representing a valid schema that does not contain extensions, input object definitions, mutations, or subscriptions, whose fields of the query type share the same name as the types they query. Not modified by this function - renamings: Dict[str, Optional[str]], mapping original type/field names to renamed type/field names or None. - Type or query type field names that do not appear in the dict will be unchanged. If a type is mapped to None, it will be suppressed in the renamed schema and queries will not be able to access it. Any dict-like object that implements get(key, [default]) may also be used + renamings: Dict[str, Optional[str]], mapping original type/root type field names to renamed + type/root type field names or None. + Any dict-like object that implements get(key, [default]) may also be used Returns: RenamedSchemaDescriptor, a namedtuple that contains the AST of the renamed schema, and the @@ -77,9 +81,7 @@ def rename_schema( if renamed_name != original_name } - # Rename query type fields - from graphql.language.printer import print_ast - + # Rename fields, including query type fields ast = _rename_fields(ast, renamings, query_type) return RenamedSchemaDescriptor( schema_ast=ast, @@ -277,7 +279,9 @@ def enter(self, node, key, parent, path, ancestors): renamed_node = self._rename_name_and_add_to_record(node) if renamed_node is node: # Name unchanged, continue traversal return None - else: # Name changed or suppressed, return new node, `visit` will make shallow copies along path + else: + # Name changed or suppressed, return new node, `visit` will make shallow copies + # along path return renamed_node else: # All Node types should've been taken care of, this line should never be reached @@ -294,8 +298,8 @@ def __init__(self, renamings, query_type): query_type: str, name of the query type (e.g. RootSchemaQuery) """ # Note that as field names and type names have been confirmed to match up, any renamed - # query type field already has a corresponding renamed type. If no errors, due to either invalid - # names or name conflicts, were raised when renaming type, no errors will occur when + # query type field already has a corresponding renamed type. If no errors, due to either + # invalid names or name conflicts, were raised when renaming type, no errors will occur when # renaming query type fields. self.in_query_type = False self.renamings = renamings @@ -310,7 +314,9 @@ def leave_object_type_definition(self, node, key, parent, path, ancestors): """If the node's name matches the query type, record that we left the query type.""" if len(node.fields) == 0: raise CascadingSuppressionError( - f"Type renamings {self.renamings} suppressed every type in the schema so it will be impossible to query for anything. To fix this, check why the `renamings` argument of `rename_schema` mapped every type to None." + f"Type renamings {self.renamings} suppressed every type in the schema so it will be" + f" impossible to query for anything. To fix this, check why the `renamings` " + f"argument of `rename_schema` mapped every type to None." ) if node.name.value == self.query_type: self.in_query_type = False @@ -322,7 +328,8 @@ def enter_field_definition(self, node, *args): If not inside the query type, check that no field depends on a type that was suppressed. Raises: - CascadingSuppressionError if a field that was not suppressed depends on a type that was suppressed. + CascadingSuppressionError if a non-suppressed field still depends on a type that was + suppressed. """ field_name = node.name.value if self.in_query_type: @@ -337,18 +344,28 @@ def enter_field_definition(self, node, *args): return field_node_with_new_name else: # at a field of a type that is not the query type - type = get_ast_with_non_null_and_list_stripped(node.type) - if type == REMOVE: + node_type = get_ast_with_non_null_and_list_stripped(node.type) + if node_type == REMOVE: # then this field depends on a type that was suppressed, which is illegal type_name = args[-1][-1].name.value raise CascadingSuppressionError( - f"Type renamings {self.renamings} attempted to suppress a type, but type {type_name}'s field {field_name} still depends on that type. Suppressing individual fields hasn't been implemented yet, but when it is, you can fix this error by suppressing the field as well." + f"Type renamings {self.renamings} attempted to suppress a type, but type " + f"{type_name}'s field {field_name} still depends on that type. Suppressing " + f"individual fields hasn't been implemented yet, but when it is, you can fix " + f"this error by suppressing the field as well." ) return None def enter_union_type_definition(self, node, *args): + """Check that each union still has at least one member. + + Raises: + CascadingSuppressionError if all the members of this union were suppressed. + """ union_name = node.name.value if len(node.types) == 0: raise CascadingSuppressionError( - f"Type renamings {self.renamings} suppressed all types belonging to the union {union_name}. To fix this, you can suppress the union as well by adding {union_name}: None to the `renamings` argument of `rename_schema`." + f"Type renamings {self.renamings} suppressed all types belonging to the union " + f"{union_name}. To fix this, you can suppress the union as well by adding " + f"`{union_name}: None` to the `renamings` argument of `rename_schema`." ) diff --git a/graphql_compiler/schema_transformation/utils.py b/graphql_compiler/schema_transformation/utils.py index 28ee5a5ed..c04b4d91f 100644 --- a/graphql_compiler/schema_transformation/utils.py +++ b/graphql_compiler/schema_transformation/utils.py @@ -58,9 +58,11 @@ class InvalidCrossSchemaEdgeError(SchemaTransformError): class CascadingSuppressionError(SchemaTransformError): - """Raised if the schema suppressions would result in empty types that would need to be, in turn, suppressed (possibly leading to a cascade of suppressions). + """Raised if existing suppressions would require further suppressions. - This may be raised if all the fields of a type are suppressed but the type itself is not suppressed, if all the members of a union are suppressed but the union itself is not suppressed, or if a type is suppressed but a field of that type is not suppressed. + This may be raised during schema renaming if it suppresses all the fields of a type but not the + type itself, if it suppresses all the members of a union but not the union itself, or if it + suppresses a type but fails to suppress all fields that depend on that type. """ 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 e319931f2..50cf9a345 100644 --- a/graphql_compiler/tests/schema_transformation_tests/input_schema_strings.py +++ b/graphql_compiler/tests/schema_transformation_tests/input_schema_strings.py @@ -173,11 +173,11 @@ class InputSchemaStrings(object): type Droid { id: String } - + type Dog { nickname: String } - + union HumanOrDroid = Human | Droid type SchemaQuery { 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 8cbb892bf..7e0b2818f 100644 --- a/graphql_compiler/tests/schema_transformation_tests/test_rename_schema.py +++ b/graphql_compiler/tests/schema_transformation_tests/test_rename_schema.py @@ -688,7 +688,10 @@ def test_suppress_every_type(self): def test_suppress_all_union_members(self): with self.assertRaises(CascadingSuppressionError): - # Note that we can't use ISS.union_schema directly here because suppressing all the members of the union would mean suppressing every type in general, which means we couldn't be sure what was causing the CascadingSuppressionError. + # Can't use ISS.union_schema here because suppressing all the members of the union would + # mean suppressing every type in general, which means we couldn't be sure that + # suppressing every member of a union specifically was raising the + # CascadingSuppressionError. rename_schema(parse(ISS.extended_union_schema), {"Human": None, "Droid": None}) def test_field_still_depends_on_suppressed_type(self): From 7e46999fbc08bc5ba68af743bd2899bd429c5191 Mon Sep 17 00:00:00 2001 From: Leon Wu Date: Thu, 28 May 2020 14:49:08 -0400 Subject: [PATCH 07/29] Add to CascadingSuppressionError comment --- graphql_compiler/schema_transformation/utils.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/graphql_compiler/schema_transformation/utils.py b/graphql_compiler/schema_transformation/utils.py index c04b4d91f..07770d82b 100644 --- a/graphql_compiler/schema_transformation/utils.py +++ b/graphql_compiler/schema_transformation/utils.py @@ -63,6 +63,12 @@ class CascadingSuppressionError(SchemaTransformError): This may be raised during schema renaming if it suppresses all the fields of a type but not the type itself, if it suppresses all the members of a union but not the union itself, or if it suppresses a type but fails to suppress all fields that depend on that type. + + The error message will suggest fixing this illegal state by describing further suppressions, but + adding these suppressions may lead to other types, unions, fields, etc. needing suppressions of + their own. Most real-world schemas wouldn't have these cascading situations, and if they do, + they are unlikely to have many of them, so the error messages are not meant to describe the full + sequence of steps required to fix all suppression errors in one pass. """ From 9bd3c665b950c69de0167fffb713e2809bc94951 Mon Sep 17 00:00:00 2001 From: Leon Wu Date: Thu, 28 May 2020 14:58:27 -0400 Subject: [PATCH 08/29] Add info about iterated suppression --- .../schema_transformation/rename_schema.py | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/graphql_compiler/schema_transformation/rename_schema.py b/graphql_compiler/schema_transformation/rename_schema.py index 99db1516a..e37d86e57 100644 --- a/graphql_compiler/schema_transformation/rename_schema.py +++ b/graphql_compiler/schema_transformation/rename_schema.py @@ -316,7 +316,9 @@ def leave_object_type_definition(self, node, key, parent, path, ancestors): raise CascadingSuppressionError( f"Type renamings {self.renamings} suppressed every type in the schema so it will be" f" impossible to query for anything. To fix this, check why the `renamings` " - f"argument of `rename_schema` mapped every type to None." + f"argument of `rename_schema` mapped every type to None. Note that adding " + f"suppressions may lead to other types, fields, unions, etc. requiring suppression " + f"so you may need to iterate on this before getting a legal schema." ) if node.name.value == self.query_type: self.in_query_type = False @@ -352,7 +354,9 @@ def enter_field_definition(self, node, *args): f"Type renamings {self.renamings} attempted to suppress a type, but type " f"{type_name}'s field {field_name} still depends on that type. Suppressing " f"individual fields hasn't been implemented yet, but when it is, you can fix " - f"this error by suppressing the field as well." + f"this error by suppressing the field as well. Note that adding suppressions " + f"may lead to other types, fields, unions, etc. requiring suppression so you " + f"may need to iterate on this before getting a legal schema." ) return None @@ -367,5 +371,7 @@ def enter_union_type_definition(self, node, *args): raise CascadingSuppressionError( f"Type renamings {self.renamings} suppressed all types belonging to the union " f"{union_name}. To fix this, you can suppress the union as well by adding " - f"`{union_name}: None` to the `renamings` argument of `rename_schema`." + f"`{union_name}: None` to the `renamings` argument of `rename_schema`. Note that " + f"adding suppressions may lead to other types, fields, unions, etc. requiring " + f"suppression so you may need to iterate on this before getting a legal schema." ) From 43ef8f7d1a5767caded5cf04dfe436eb4e1298c4 Mon Sep 17 00:00:00 2001 From: Leon Wu Date: Thu, 28 May 2020 15:27:03 -0400 Subject: [PATCH 09/29] Refactor cascading suppression check out from query type field editing --- .../schema_transformation/rename_schema.py | 96 +++++++++++++------ 1 file changed, 65 insertions(+), 31 deletions(-) diff --git a/graphql_compiler/schema_transformation/rename_schema.py b/graphql_compiler/schema_transformation/rename_schema.py index e37d86e57..cf2000fe2 100644 --- a/graphql_compiler/schema_transformation/rename_schema.py +++ b/graphql_compiler/schema_transformation/rename_schema.py @@ -81,8 +81,11 @@ def rename_schema( if renamed_name != original_name } - # Rename fields, including query type fields - ast = _rename_fields(ast, renamings, query_type) + # Rename query type fields + ast = _rename_query_type_fields(ast, renamings, query_type) + + # check for fields or unions that depend on types that were suppressed + ast = _check_for_cascading_type_suppression(ast, renamings, query_type) return RenamedSchemaDescriptor( schema_ast=ast, schema=build_ast_schema(ast), @@ -121,8 +124,8 @@ def _rename_types(ast, renamings, query_type, scalars): return renamed_ast, visitor.reverse_name_map -def _rename_fields(ast, renamings, query_type): - """Rename fields, including query type fields. +def _rename_query_type_fields(ast, renamings, query_type): + """Rename all fields of the query type. The input AST will not be modified. @@ -135,7 +138,13 @@ def _rename_fields(ast, renamings, query_type): Returns: DocumentNode, representing the modified version of the input schema AST """ - visitor = RenameFieldsVisitor(renamings, query_type) + visitor = RenameQueryTypeFieldsVisitor(renamings, query_type) + renamed_ast = visit(ast, visitor) + return renamed_ast + + +def _check_for_cascading_type_suppression(ast, renamings, query_type): + visitor = CascadingSuppressionCheckVisitor(renamings, query_type) renamed_ast = visit(ast, visitor) return renamed_ast @@ -288,9 +297,9 @@ def enter(self, node, key, parent, path, ancestors): raise AssertionError('Unreachable code reached. Missed type: "{}"'.format(node_type)) -class RenameFieldsVisitor(Visitor): +class RenameQueryTypeFieldsVisitor(Visitor): def __init__(self, renamings, query_type): - """Create a visitor for renaming fields in a schema AST. + """Create a visitor for renaming fields of the query type in a schema AST. Args: renamings: Dict[str, str], from original field name to renamed field name. Any @@ -324,17 +333,9 @@ def leave_object_type_definition(self, node, key, parent, path, ancestors): self.in_query_type = False def enter_field_definition(self, node, *args): - """Update fields as needed. - - If inside the query type, rename field and add the name pair to reverse_field_map. - If not inside the query type, check that no field depends on a type that was suppressed. - - Raises: - CascadingSuppressionError if a non-suppressed field still depends on a type that was - suppressed. - """ - field_name = node.name.value + """If inside the query type, rename field and add the name pair to reverse_field_map.""" if self.in_query_type: + field_name = node.name.value new_field_name = self.renamings.get(field_name, field_name) # Default use original if new_field_name == field_name: return None @@ -344,21 +345,54 @@ def enter_field_definition(self, node, *args): else: # Make copy of node with the changed name, return the copy field_node_with_new_name = get_copy_of_node_with_new_name(node, new_field_name) return field_node_with_new_name - else: - # at a field of a type that is not the query type - node_type = get_ast_with_non_null_and_list_stripped(node.type) - if node_type == REMOVE: - # then this field depends on a type that was suppressed, which is illegal - type_name = args[-1][-1].name.value - raise CascadingSuppressionError( - f"Type renamings {self.renamings} attempted to suppress a type, but type " - f"{type_name}'s field {field_name} still depends on that type. Suppressing " - f"individual fields hasn't been implemented yet, but when it is, you can fix " - f"this error by suppressing the field as well. Note that adding suppressions " - f"may lead to other types, fields, unions, etc. requiring suppression so you " - f"may need to iterate on this before getting a legal schema." - ) + + +class CascadingSuppressionCheckVisitor(Visitor): + def __init__(self, renamings, query_type): + """Create a visitor to check that suppression does not cause an illegal state. + + Args: + renamings: Dict[str, str], from original field name to renamed field name. Any + name not in the dict will be unchanged + query_type: str, name of the query type (e.g. RootSchemaQuery) + """ + # Note that as field names and type names have been confirmed to match up, any renamed + # query type field already has a corresponding renamed type. If no errors, due to either + # invalid names or name conflicts, were raised when renaming type, no errors will occur when + # renaming query type fields. + self.in_query_type = False + self.renamings = renamings + self.query_type = query_type + + def enter_object_type_definition(self, node, *args): + """If the node's name matches the query type, record that we entered the query type.""" + if node.name.value == self.query_type: + self.in_query_type = True + + def leave_object_type_definition(self, node, key, parent, path, ancestors): + """If the node's name matches the query type, record that we left the query type.""" + if node.name.value == self.query_type: + self.in_query_type = False + + def enter_field_definition(self, node, *args): + """If not at query type, check that no field depends on a type that was suppressed.""" + if self.in_query_type: return None + # at a field of a type that is not the query type + field_name = node.name.value + node_type = get_ast_with_non_null_and_list_stripped(node.type) + if node_type == REMOVE: + # then this field depends on a type that was suppressed, which is illegal + type_name = args[-1][-1].name.value + raise CascadingSuppressionError( + f"Type renamings {self.renamings} attempted to suppress a type, but type " + f"{type_name}'s field {field_name} still depends on that type. Suppressing " + f"individual fields hasn't been implemented yet, but when it is, you can fix " + f"this error by suppressing the field as well. Note that adding suppressions " + f"may lead to other types, fields, unions, etc. requiring suppression so you " + f"may need to iterate on this before getting a legal schema." + ) + return None def enter_union_type_definition(self, node, *args): """Check that each union still has at least one member. From 69a9430443a1901655216391812d5d9fff202d46 Mon Sep 17 00:00:00 2001 From: Leon Wu Date: Thu, 28 May 2020 16:12:18 -0400 Subject: [PATCH 10/29] Add type hints --- .../schema_transformation/rename_schema.py | 67 ++++++++++++------- 1 file changed, 43 insertions(+), 24 deletions(-) diff --git a/graphql_compiler/schema_transformation/rename_schema.py b/graphql_compiler/schema_transformation/rename_schema.py index cf2000fe2..4ec0aeb41 100644 --- a/graphql_compiler/schema_transformation/rename_schema.py +++ b/graphql_compiler/schema_transformation/rename_schema.py @@ -1,8 +1,15 @@ # Copyright 2019-present Kensho Technologies, LLC. from collections import namedtuple -from typing import Dict, Optional - -from graphql import DocumentNode, build_ast_schema +from typing import Dict, Optional, Set, Tuple + +from graphql import ( + DocumentNode, + FieldDefinitionNode, + Node, + ObjectTypeDefinitionNode, + UnionTypeDefinitionNode, + build_ast_schema, +) from graphql.language.visitor import REMOVE, Visitor, visit import six @@ -93,7 +100,9 @@ def rename_schema( ) -def _rename_types(ast, renamings, query_type, scalars): +def _rename_types( + ast: DocumentNode, renamings: Dict[str, Optional[str]], query_type: str, scalars: Set[str] +) -> Tuple[DocumentNode, Dict[str, str]]: """Rename types, enums, interfaces using renamings. The query type will not be renamed. Scalar types, field names, enum values will not be renamed. @@ -124,7 +133,9 @@ def _rename_types(ast, renamings, query_type, scalars): return renamed_ast, visitor.reverse_name_map -def _rename_query_type_fields(ast, renamings, query_type): +def _rename_query_type_fields( + ast: DocumentNode, renamings: Dict[str, Optional[str]], query_type: str +) -> DocumentNode: """Rename all fields of the query type. The input AST will not be modified. @@ -143,7 +154,9 @@ def _rename_query_type_fields(ast, renamings, query_type): return renamed_ast -def _check_for_cascading_type_suppression(ast, renamings, query_type): +def _check_for_cascading_type_suppression( + ast: DocumentNode, renamings: Dict[str, Optional[str]], query_type: str +) -> DocumentNode: visitor = CascadingSuppressionCheckVisitor(renamings, query_type) renamed_ast = visit(ast, visitor) return renamed_ast @@ -203,19 +216,21 @@ class RenameSchemaTypesVisitor(Visitor): } ) - def __init__(self, renamings, query_type, scalar_types): + def __init__( + self, renamings: Dict[str, Optional[str]], query_type: str, scalar_types: Set[str] + ): """Create a visitor for renaming types in a schema AST. Args: - renamings: Dict[str, str], mapping from original type name to renamed type name. - Any name not in the dict will be unchanged + renamings: Dict[str, Optional[str]], mapping from original type name to renamed type + name. Any name not in the dict will be unchanged query_type: str, name of the query type (e.g. RootSchemaQuery), which will not be renamed scalar_types: Set[str], set of names of all scalars used in the schema, including all user defined scalars and any builtin scalars that were used """ self.renamings = renamings - self.reverse_name_map = {} # Dict[str, str], from renamed type name to original type name + self.reverse_name_map: Dict[str, str] = {} # from renamed type name to original type name # reverse_name_map contains all types, including those that were unchanged self.query_type = query_type self.scalar_types = frozenset(scalar_types) @@ -277,7 +292,7 @@ def _rename_name_and_add_to_record(self, node): node_with_new_name = get_copy_of_node_with_new_name(node, new_name_string) return node_with_new_name - def enter(self, node, key, parent, path, ancestors): + def enter(self, node: Node, key, parent, path, ancestors): """Upon entering a node, operate depending on node type.""" node_type = type(node).__name__ if node_type in self.noop_types: @@ -298,11 +313,11 @@ def enter(self, node, key, parent, path, ancestors): class RenameQueryTypeFieldsVisitor(Visitor): - def __init__(self, renamings, query_type): + def __init__(self, renamings: Dict[str, Optional[str]], query_type: str): """Create a visitor for renaming fields of the query type in a schema AST. Args: - renamings: Dict[str, str], from original field name to renamed field name. Any + renamings: Dict[str, Optional[str]], from original field name to renamed field name. Any name not in the dict will be unchanged query_type: str, name of the query type (e.g. RootSchemaQuery) """ @@ -314,14 +329,16 @@ def __init__(self, renamings, query_type): self.renamings = renamings self.query_type = query_type - def enter_object_type_definition(self, node, *args): + def enter_object_type_definition(self, node: ObjectTypeDefinitionNode, *args): """If the node's name matches the query type, record that we entered the query type.""" if node.name.value == self.query_type: self.in_query_type = True - def leave_object_type_definition(self, node, key, parent, path, ancestors): + def leave_object_type_definition( + self, node: ObjectTypeDefinitionNode, key, parent, path, ancestors + ): """If the node's name matches the query type, record that we left the query type.""" - if len(node.fields) == 0: + if not node.fields: raise CascadingSuppressionError( f"Type renamings {self.renamings} suppressed every type in the schema so it will be" f" impossible to query for anything. To fix this, check why the `renamings` " @@ -332,7 +349,7 @@ def leave_object_type_definition(self, node, key, parent, path, ancestors): if node.name.value == self.query_type: self.in_query_type = False - def enter_field_definition(self, node, *args): + def enter_field_definition(self, node: FieldDefinitionNode, *args): """If inside the query type, rename field and add the name pair to reverse_field_map.""" if self.in_query_type: field_name = node.name.value @@ -348,11 +365,11 @@ def enter_field_definition(self, node, *args): class CascadingSuppressionCheckVisitor(Visitor): - def __init__(self, renamings, query_type): + def __init__(self, renamings: Dict[str, Optional[str]], query_type: str): """Create a visitor to check that suppression does not cause an illegal state. Args: - renamings: Dict[str, str], from original field name to renamed field name. Any + renamings: Dict[str, Optional[str]], from original field name to renamed field name. Any name not in the dict will be unchanged query_type: str, name of the query type (e.g. RootSchemaQuery) """ @@ -364,17 +381,19 @@ def __init__(self, renamings, query_type): self.renamings = renamings self.query_type = query_type - def enter_object_type_definition(self, node, *args): + def enter_object_type_definition(self, node: ObjectTypeDefinitionNode, *args): """If the node's name matches the query type, record that we entered the query type.""" if node.name.value == self.query_type: self.in_query_type = True - def leave_object_type_definition(self, node, key, parent, path, ancestors): + def leave_object_type_definition( + self, node: ObjectTypeDefinitionNode, key, parent, path, ancestors + ): """If the node's name matches the query type, record that we left the query type.""" if node.name.value == self.query_type: self.in_query_type = False - def enter_field_definition(self, node, *args): + def enter_field_definition(self, node: FieldDefinitionNode, *args): """If not at query type, check that no field depends on a type that was suppressed.""" if self.in_query_type: return None @@ -394,14 +413,14 @@ def enter_field_definition(self, node, *args): ) return None - def enter_union_type_definition(self, node, *args): + def enter_union_type_definition(self, node: UnionTypeDefinitionNode, *args): """Check that each union still has at least one member. Raises: CascadingSuppressionError if all the members of this union were suppressed. """ union_name = node.name.value - if len(node.types) == 0: + if not node.types: raise CascadingSuppressionError( f"Type renamings {self.renamings} suppressed all types belonging to the union " f"{union_name}. To fix this, you can suppress the union as well by adding " From d2c6940f935fe56cdaad37b8b32ef9cacbb32210 Mon Sep 17 00:00:00 2001 From: Leon Wu Date: Thu, 28 May 2020 16:27:50 -0400 Subject: [PATCH 11/29] Fix incorrect comment --- graphql_compiler/schema_transformation/rename_schema.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/graphql_compiler/schema_transformation/rename_schema.py b/graphql_compiler/schema_transformation/rename_schema.py index 4ec0aeb41..5118f478d 100644 --- a/graphql_compiler/schema_transformation/rename_schema.py +++ b/graphql_compiler/schema_transformation/rename_schema.py @@ -250,8 +250,9 @@ def _rename_name_and_add_to_record(self, node): NameNode. Returns: - Node object, identical to the input node, except with possibly a new name. If the - name was not changed, the returned object is the exact same object as the input + Node object or REMOVE. If it returns REMOVE, then the type was suppressed. Otherwise, it + returns a Node object identical to the input node, except with possibly a new name. If + the name was not changed, the returned object is the exact same object as the input Raises: - InvalidTypeNameError if either the node's current name or renamed name is invalid From 360ce360edee43a6517d343a7a50e4b3a07d9577 Mon Sep 17 00:00:00 2001 From: Leon Wu Date: Mon, 1 Jun 2020 17:16:02 -0400 Subject: [PATCH 12/29] Address most comments Mostly type-hints, nits --- .../schema_transformation/rename_schema.py | 64 +++++++++---------- 1 file changed, 31 insertions(+), 33 deletions(-) diff --git a/graphql_compiler/schema_transformation/rename_schema.py b/graphql_compiler/schema_transformation/rename_schema.py index 5118f478d..28688d609 100644 --- a/graphql_compiler/schema_transformation/rename_schema.py +++ b/graphql_compiler/schema_transformation/rename_schema.py @@ -1,6 +1,6 @@ # Copyright 2019-present Kensho Technologies, LLC. from collections import namedtuple -from typing import Dict, Optional, Set, Tuple +from typing import Dict, Optional, Set, Tuple, Any, List, Union from graphql import ( DocumentNode, @@ -91,7 +91,7 @@ def rename_schema( # Rename query type fields ast = _rename_query_type_fields(ast, renamings, query_type) - # check for fields or unions that depend on types that were suppressed + # Check for fields or unions that depend on types that were suppressed ast = _check_for_cascading_type_suppression(ast, renamings, query_type) return RenamedSchemaDescriptor( schema_ast=ast, @@ -157,6 +157,7 @@ def _rename_query_type_fields( def _check_for_cascading_type_suppression( ast: DocumentNode, renamings: Dict[str, Optional[str]], query_type: str ) -> DocumentNode: + """Check for fields with suppressed types or unions whose members were all suppressed.""" visitor = CascadingSuppressionCheckVisitor(renamings, query_type) renamed_ast = visit(ast, visitor) return renamed_ast @@ -218,25 +219,25 @@ class RenameSchemaTypesVisitor(Visitor): def __init__( self, renamings: Dict[str, Optional[str]], query_type: str, scalar_types: Set[str] - ): + ) -> None: """Create a visitor for renaming types in a schema AST. Args: renamings: Dict[str, Optional[str]], mapping from original type name to renamed type - name. Any name not in the dict will be unchanged + name. Any name not in the dict will be unchanged query_type: str, name of the query type (e.g. RootSchemaQuery), which will not be renamed scalar_types: Set[str], set of names of all scalars used in the schema, including all user defined scalars and any builtin scalars that were used """ self.renamings = renamings - self.reverse_name_map: Dict[str, str] = {} # from renamed type name to original type name - # reverse_name_map contains all types, including those that were unchanged + self.reverse_name_map: Dict[str, str] = {} # From renamed type name to original type name + # reverse_name_map contains all non-suppressed types, including those that were unchanged self.query_type = query_type self.scalar_types = frozenset(scalar_types) self.builtin_types = frozenset({"String", "Int", "Float", "Boolean", "ID"}) - def _rename_name_and_add_to_record(self, node): + def _rename_name_and_add_to_record(self, node: Node) -> Optional[Node]: """Change the name of the input node if necessary, add the name pair to reverse_name_map. Don't rename if the type is the query type, a scalar type, or a builtin type. @@ -250,9 +251,7 @@ def _rename_name_and_add_to_record(self, node): NameNode. Returns: - Node object or REMOVE. If it returns REMOVE, then the type was suppressed. Otherwise, it - returns a Node object identical to the input node, except with possibly a new name. If - the name was not changed, the returned object is the exact same object as the input + Node object or REMOVE. REMOVE is a special return value defined by the GraphQL library. A visitor function returns REMOVE to delete the node it's currently at. This function returns REMOVE to suppress types. If the current node is not a type to be suppressed, it returns a Node object identical to the input node, except with possibly a new name. If the name was not changed, the returned object is the exact same object as the input Raises: - InvalidTypeNameError if either the node's current name or renamed name is invalid @@ -266,7 +265,7 @@ def _rename_name_and_add_to_record(self, node): new_name_string = self.renamings.get(name_string, name_string) # Default use original if new_name_string is None: - # suppress the type + # Suppress the type return REMOVE check_type_name_is_valid(new_name_string) @@ -293,7 +292,7 @@ def _rename_name_and_add_to_record(self, node): node_with_new_name = get_copy_of_node_with_new_name(node, new_name_string) return node_with_new_name - def enter(self, node: Node, key, parent, path, ancestors): + def enter(self, node: Node, key: Optional[Any], parent: Optional[Any], path: List[Any], ancestors: List[Any]) -> Optional[Node]: """Upon entering a node, operate depending on node type.""" node_type = type(node).__name__ if node_type in self.noop_types: @@ -314,7 +313,7 @@ def enter(self, node: Node, key, parent, path, ancestors): class RenameQueryTypeFieldsVisitor(Visitor): - def __init__(self, renamings: Dict[str, Optional[str]], query_type: str): + def __init__(self, renamings: Dict[str, Optional[str]], query_type: str) -> None: """Create a visitor for renaming fields of the query type in a schema AST. Args: @@ -330,27 +329,25 @@ def __init__(self, renamings: Dict[str, Optional[str]], query_type: str): self.renamings = renamings self.query_type = query_type - def enter_object_type_definition(self, node: ObjectTypeDefinitionNode, *args): + def enter_object_type_definition(self, node: ObjectTypeDefinitionNode, key: Optional[Any], parent: Optional[Any], path: List[Any], ancestors: List[Any]) -> None: """If the node's name matches the query type, record that we entered the query type.""" if node.name.value == self.query_type: self.in_query_type = True def leave_object_type_definition( - self, node: ObjectTypeDefinitionNode, key, parent, path, ancestors - ): + self, node: ObjectTypeDefinitionNode, key: Optional[Any], parent: Optional[Any], path: List[Any], ancestors: List[Any] + ) -> None: """If the node's name matches the query type, record that we left the query type.""" if not node.fields: raise CascadingSuppressionError( - f"Type renamings {self.renamings} suppressed every type in the schema so it will be" - f" impossible to query for anything. To fix this, check why the `renamings` " - f"argument of `rename_schema` mapped every type to None. Note that adding " - f"suppressions may lead to other types, fields, unions, etc. requiring suppression " - f"so you may need to iterate on this before getting a legal schema." + f"Type renamings {self.renamings} suppressed every type in the schema so it will " + f"be impossible to query for anything. To fix this, check why the `renamings` " + f"argument of `rename_schema` mapped every type to None." ) if node.name.value == self.query_type: self.in_query_type = False - def enter_field_definition(self, node: FieldDefinitionNode, *args): + def enter_field_definition(self, node: FieldDefinitionNode, key: Optional[Any], parent: Optional[Any], path: List[Any], ancestors: List[Any]) -> Optional[Node]: """If inside the query type, rename field and add the name pair to reverse_field_map.""" if self.in_query_type: field_name = node.name.value @@ -358,15 +355,15 @@ def enter_field_definition(self, node: FieldDefinitionNode, *args): if new_field_name == field_name: return None if new_field_name is None: - # suppress the type - return REMOVE + # Suppress the type + return REMOVE # TODO leave a note that the return is weird even though REMOVE is None because REMOVE is ellipsis and that might not necessarily be None in the future. Hence Union[ellipsis, Optional[Node]] else: # Make copy of node with the changed name, return the copy field_node_with_new_name = get_copy_of_node_with_new_name(node, new_field_name) return field_node_with_new_name class CascadingSuppressionCheckVisitor(Visitor): - def __init__(self, renamings: Dict[str, Optional[str]], query_type: str): + def __init__(self, renamings: Dict[str, Optional[str]], query_type: str) -> None: """Create a visitor to check that suppression does not cause an illegal state. Args: @@ -382,28 +379,29 @@ def __init__(self, renamings: Dict[str, Optional[str]], query_type: str): self.renamings = renamings self.query_type = query_type - def enter_object_type_definition(self, node: ObjectTypeDefinitionNode, *args): + def enter_object_type_definition(self, node: ObjectTypeDefinitionNode, key: Optional[Any], parent: Optional[Any], path: List[Any], ancestors: List[Any]) -> None: """If the node's name matches the query type, record that we entered the query type.""" if node.name.value == self.query_type: self.in_query_type = True def leave_object_type_definition( - self, node: ObjectTypeDefinitionNode, key, parent, path, ancestors - ): + self, node: ObjectTypeDefinitionNode, key: Optional[Any], parent: Optional[Any], path: List[Any], ancestors: List[Any] + ) -> None: """If the node's name matches the query type, record that we left the query type.""" if node.name.value == self.query_type: self.in_query_type = False - def enter_field_definition(self, node: FieldDefinitionNode, *args): + def enter_field_definition(self, node: FieldDefinitionNode, key: Optional[Any], parent: Optional[Any], path: List[Any], ancestors: List[Any]) -> None: """If not at query type, check that no field depends on a type that was suppressed.""" if self.in_query_type: return None - # at a field of a type that is not the query type + # At a field of a type that is not the query type field_name = node.name.value node_type = get_ast_with_non_null_and_list_stripped(node.type) if node_type == REMOVE: - # then this field depends on a type that was suppressed, which is illegal - type_name = args[-1][-1].name.value + # Then this field depends on a type that was suppressed, which is illegal + # Note that we refer to the last item in ancestors, which is not the same as parent. The last item in ancestors is actually the grandparent of node, specifically the ObjectTypeDefinitionNode that has this field. We use the grandparent node instead of the parent because the parent of a FieldDefinitionNode is simply a list of FieldDefinitionNodes, which doesn't contain the name of the type containing this node (which we need for the error message). + type_name = ancestors[-1].name.value raise CascadingSuppressionError( f"Type renamings {self.renamings} attempted to suppress a type, but type " f"{type_name}'s field {field_name} still depends on that type. Suppressing " @@ -414,7 +412,7 @@ def enter_field_definition(self, node: FieldDefinitionNode, *args): ) return None - def enter_union_type_definition(self, node: UnionTypeDefinitionNode, *args): + def enter_union_type_definition(self, node: UnionTypeDefinitionNode, key: Optional[Any], parent: Optional[Any], path: List[Any], ancestors: List[Any]) -> None: """Check that each union still has at least one member. Raises: From e813bb4a388c7656682d5a56e0f2992222a28bed Mon Sep 17 00:00:00 2001 From: Leon Wu Date: Mon, 1 Jun 2020 17:34:03 -0400 Subject: [PATCH 13/29] Partial lint Still a few linting errors that I'm not sure about, will look at tomorrow --- .../schema_transformation/rename_schema.py | 88 ++++++++++++++++--- 1 file changed, 75 insertions(+), 13 deletions(-) diff --git a/graphql_compiler/schema_transformation/rename_schema.py b/graphql_compiler/schema_transformation/rename_schema.py index 28688d609..cde9d7ede 100644 --- a/graphql_compiler/schema_transformation/rename_schema.py +++ b/graphql_compiler/schema_transformation/rename_schema.py @@ -1,6 +1,6 @@ # Copyright 2019-present Kensho Technologies, LLC. from collections import namedtuple -from typing import Dict, Optional, Set, Tuple, Any, List, Union +from typing import Any, Dict, List, Optional, Set, Tuple, Union from graphql import ( DocumentNode, @@ -237,7 +237,7 @@ def __init__( self.scalar_types = frozenset(scalar_types) self.builtin_types = frozenset({"String", "Int", "Float", "Boolean", "ID"}) - def _rename_name_and_add_to_record(self, node: Node) -> Optional[Node]: + def _rename_name_and_add_to_record(self, node: Node) -> Union[ellipsis, Optional[Node]]: """Change the name of the input node if necessary, add the name pair to reverse_name_map. Don't rename if the type is the query type, a scalar type, or a builtin type. @@ -251,7 +251,11 @@ def _rename_name_and_add_to_record(self, node: Node) -> Optional[Node]: NameNode. Returns: - Node object or REMOVE. REMOVE is a special return value defined by the GraphQL library. A visitor function returns REMOVE to delete the node it's currently at. This function returns REMOVE to suppress types. If the current node is not a type to be suppressed, it returns a Node object identical to the input node, except with possibly a new name. If the name was not changed, the returned object is the exact same object as the input + Node object or REMOVE. REMOVE is a special return value defined by the GraphQL library. + A visitor function returns REMOVE to delete the node it's currently at. This function + returns REMOVE to suppress types. If the current node is not to be suppressed, it + returns a Node object identical to the input node, except with possibly a new name. If + the name was not changed, the returned object is the exact same object as the input Raises: - InvalidTypeNameError if either the node's current name or renamed name is invalid @@ -292,7 +296,14 @@ def _rename_name_and_add_to_record(self, node: Node) -> Optional[Node]: node_with_new_name = get_copy_of_node_with_new_name(node, new_name_string) return node_with_new_name - def enter(self, node: Node, key: Optional[Any], parent: Optional[Any], path: List[Any], ancestors: List[Any]) -> Optional[Node]: + def enter( + self, + node: Node, + key: Optional[Any], + parent: Optional[Any], + path: List[Any], + ancestors: List[Any], + ) -> Union[ellipsis, Optional[Node]]: """Upon entering a node, operate depending on node type.""" node_type = type(node).__name__ if node_type in self.noop_types: @@ -329,13 +340,25 @@ def __init__(self, renamings: Dict[str, Optional[str]], query_type: str) -> None self.renamings = renamings self.query_type = query_type - def enter_object_type_definition(self, node: ObjectTypeDefinitionNode, key: Optional[Any], parent: Optional[Any], path: List[Any], ancestors: List[Any]) -> None: + def enter_object_type_definition( + self, + node: ObjectTypeDefinitionNode, + key: Optional[Any], + parent: Optional[Any], + path: List[Any], + ancestors: List[Any], + ) -> None: """If the node's name matches the query type, record that we entered the query type.""" if node.name.value == self.query_type: self.in_query_type = True def leave_object_type_definition( - self, node: ObjectTypeDefinitionNode, key: Optional[Any], parent: Optional[Any], path: List[Any], ancestors: List[Any] + self, + node: ObjectTypeDefinitionNode, + key: Optional[Any], + parent: Optional[Any], + path: List[Any], + ancestors: List[Any], ) -> None: """If the node's name matches the query type, record that we left the query type.""" if not node.fields: @@ -347,7 +370,14 @@ def leave_object_type_definition( if node.name.value == self.query_type: self.in_query_type = False - def enter_field_definition(self, node: FieldDefinitionNode, key: Optional[Any], parent: Optional[Any], path: List[Any], ancestors: List[Any]) -> Optional[Node]: + def enter_field_definition( + self, + node: FieldDefinitionNode, + key: Optional[Any], + parent: Optional[Any], + path: List[Any], + ancestors: List[Any], + ) -> Union[ellipsis, Optional[Node]]: """If inside the query type, rename field and add the name pair to reverse_field_map.""" if self.in_query_type: field_name = node.name.value @@ -356,10 +386,11 @@ def enter_field_definition(self, node: FieldDefinitionNode, key: Optional[Any], return None if new_field_name is None: # Suppress the type - return REMOVE # TODO leave a note that the return is weird even though REMOVE is None because REMOVE is ellipsis and that might not necessarily be None in the future. Hence Union[ellipsis, Optional[Node]] + return REMOVE else: # Make copy of node with the changed name, return the copy field_node_with_new_name = get_copy_of_node_with_new_name(node, new_field_name) return field_node_with_new_name + return None class CascadingSuppressionCheckVisitor(Visitor): @@ -379,19 +410,38 @@ def __init__(self, renamings: Dict[str, Optional[str]], query_type: str) -> None self.renamings = renamings self.query_type = query_type - def enter_object_type_definition(self, node: ObjectTypeDefinitionNode, key: Optional[Any], parent: Optional[Any], path: List[Any], ancestors: List[Any]) -> None: + def enter_object_type_definition( + self, + node: ObjectTypeDefinitionNode, + key: Optional[Any], + parent: Optional[Any], + path: List[Any], + ancestors: List[Any], + ) -> None: """If the node's name matches the query type, record that we entered the query type.""" if node.name.value == self.query_type: self.in_query_type = True def leave_object_type_definition( - self, node: ObjectTypeDefinitionNode, key: Optional[Any], parent: Optional[Any], path: List[Any], ancestors: List[Any] + self, + node: ObjectTypeDefinitionNode, + key: Optional[Any], + parent: Optional[Any], + path: List[Any], + ancestors: List[Any], ) -> None: """If the node's name matches the query type, record that we left the query type.""" if node.name.value == self.query_type: self.in_query_type = False - def enter_field_definition(self, node: FieldDefinitionNode, key: Optional[Any], parent: Optional[Any], path: List[Any], ancestors: List[Any]) -> None: + def enter_field_definition( + self, + node: FieldDefinitionNode, + key: Optional[Any], + parent: Optional[Any], + path: List[Any], + ancestors: List[Any], + ) -> None: """If not at query type, check that no field depends on a type that was suppressed.""" if self.in_query_type: return None @@ -400,7 +450,12 @@ def enter_field_definition(self, node: FieldDefinitionNode, key: Optional[Any], node_type = get_ast_with_non_null_and_list_stripped(node.type) if node_type == REMOVE: # Then this field depends on a type that was suppressed, which is illegal - # Note that we refer to the last item in ancestors, which is not the same as parent. The last item in ancestors is actually the grandparent of node, specifically the ObjectTypeDefinitionNode that has this field. We use the grandparent node instead of the parent because the parent of a FieldDefinitionNode is simply a list of FieldDefinitionNodes, which doesn't contain the name of the type containing this node (which we need for the error message). + # Note that we refer to the last item in ancestors, which is not the same as parent. The + # last item in ancestors is actually the grandparent of node, specifically the + # ObjectTypeDefinitionNode that has this field. We use the grandparent node instead of + # the parent because the parent of a FieldDefinitionNode is simply a list of + # FieldDefinitionNodes, which doesn't contain the name of the type containing this node + # (which we need for the error message). type_name = ancestors[-1].name.value raise CascadingSuppressionError( f"Type renamings {self.renamings} attempted to suppress a type, but type " @@ -412,7 +467,14 @@ def enter_field_definition(self, node: FieldDefinitionNode, key: Optional[Any], ) return None - def enter_union_type_definition(self, node: UnionTypeDefinitionNode, key: Optional[Any], parent: Optional[Any], path: List[Any], ancestors: List[Any]) -> None: + def enter_union_type_definition( + self, + node: UnionTypeDefinitionNode, + key: Optional[Any], + parent: Optional[Any], + path: List[Any], + ancestors: List[Any], + ) -> None: """Check that each union still has at least one member. Raises: From d3680eb01cffc81a8c3cea547518527fdbab7a36 Mon Sep 17 00:00:00 2001 From: Leon Wu Date: Tue, 2 Jun 2020 10:14:48 -0400 Subject: [PATCH 14/29] Replace Optional[Any] with Any I know these two aren't the same, technically (https://github.com/python/mypy/issues/3138) but the original code listed them as Any and not Optional[Any] so just type-hinting Any here is more accurate. --- .../schema_transformation/rename_schema.py | 32 +++++++++---------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/graphql_compiler/schema_transformation/rename_schema.py b/graphql_compiler/schema_transformation/rename_schema.py index cde9d7ede..e051be91b 100644 --- a/graphql_compiler/schema_transformation/rename_schema.py +++ b/graphql_compiler/schema_transformation/rename_schema.py @@ -299,8 +299,8 @@ def _rename_name_and_add_to_record(self, node: Node) -> Union[ellipsis, Optional def enter( self, node: Node, - key: Optional[Any], - parent: Optional[Any], + key: Any, + parent: Any, path: List[Any], ancestors: List[Any], ) -> Union[ellipsis, Optional[Node]]: @@ -343,8 +343,8 @@ def __init__(self, renamings: Dict[str, Optional[str]], query_type: str) -> None def enter_object_type_definition( self, node: ObjectTypeDefinitionNode, - key: Optional[Any], - parent: Optional[Any], + key: Any, + parent: Any, path: List[Any], ancestors: List[Any], ) -> None: @@ -355,8 +355,8 @@ def enter_object_type_definition( def leave_object_type_definition( self, node: ObjectTypeDefinitionNode, - key: Optional[Any], - parent: Optional[Any], + key: Any, + parent: Any, path: List[Any], ancestors: List[Any], ) -> None: @@ -373,8 +373,8 @@ def leave_object_type_definition( def enter_field_definition( self, node: FieldDefinitionNode, - key: Optional[Any], - parent: Optional[Any], + key: Any, + parent: Any, path: List[Any], ancestors: List[Any], ) -> Union[ellipsis, Optional[Node]]: @@ -413,8 +413,8 @@ def __init__(self, renamings: Dict[str, Optional[str]], query_type: str) -> None def enter_object_type_definition( self, node: ObjectTypeDefinitionNode, - key: Optional[Any], - parent: Optional[Any], + key: Any, + parent: Any, path: List[Any], ancestors: List[Any], ) -> None: @@ -425,8 +425,8 @@ def enter_object_type_definition( def leave_object_type_definition( self, node: ObjectTypeDefinitionNode, - key: Optional[Any], - parent: Optional[Any], + key: Any, + parent: Any, path: List[Any], ancestors: List[Any], ) -> None: @@ -437,8 +437,8 @@ def leave_object_type_definition( def enter_field_definition( self, node: FieldDefinitionNode, - key: Optional[Any], - parent: Optional[Any], + key: Any, + parent: Any, path: List[Any], ancestors: List[Any], ) -> None: @@ -470,8 +470,8 @@ def enter_field_definition( def enter_union_type_definition( self, node: UnionTypeDefinitionNode, - key: Optional[Any], - parent: Optional[Any], + key: Any, + parent: Any, path: List[Any], ancestors: List[Any], ) -> None: From 1217f9dd5ba2349d3679a335c03b1859338b611f Mon Sep 17 00:00:00 2001 From: Leon Wu Date: Tue, 2 Jun 2020 10:24:03 -0400 Subject: [PATCH 15/29] Clarify ancestors parameter usage --- .../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 e051be91b..237ac9f0f 100644 --- a/graphql_compiler/schema_transformation/rename_schema.py +++ b/graphql_compiler/schema_transformation/rename_schema.py @@ -450,13 +450,17 @@ def enter_field_definition( node_type = get_ast_with_non_null_and_list_stripped(node.type) if node_type == REMOVE: # Then this field depends on a type that was suppressed, which is illegal - # Note that we refer to the last item in ancestors, which is not the same as parent. The - # last item in ancestors is actually the grandparent of node, specifically the - # ObjectTypeDefinitionNode that has this field. We use the grandparent node instead of - # the parent because the parent of a FieldDefinitionNode is simply a list of - # FieldDefinitionNodes, which doesn't contain the name of the type containing this node - # (which we need for the error message). - type_name = ancestors[-1].name.value + if not ancestors: + raise AssertionError( + f"Expected ancestors to be non-empty list when entering field definition but" + f"ancestors was {ancestors} at node {node}" + ) + # We use the grandparent node (ObjectTypeDefinitionNode) instead of the parent because + # the parent of a FieldDefinitionNode is simply a list of FieldDefinitionNodes, which + # doesn't contain the name of the type containing this node (which we need for the error + # message). + node_grandparent = ancestors[-1] + type_name = node_grandparent.name.value raise CascadingSuppressionError( f"Type renamings {self.renamings} attempted to suppress a type, but type " f"{type_name}'s field {field_name} still depends on that type. Suppressing " From 841be44b9d5c7e70327224861b1fe35254a89fb2 Mon Sep 17 00:00:00 2001 From: Leon Wu Date: Tue, 2 Jun 2020 12:17:52 -0400 Subject: [PATCH 16/29] Change suppress every type's error type to SchemaTransformERror --- graphql_compiler/schema_transformation/rename_schema.py | 4 ++-- .../tests/schema_transformation_tests/test_rename_schema.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/graphql_compiler/schema_transformation/rename_schema.py b/graphql_compiler/schema_transformation/rename_schema.py index 237ac9f0f..703b742cc 100644 --- a/graphql_compiler/schema_transformation/rename_schema.py +++ b/graphql_compiler/schema_transformation/rename_schema.py @@ -21,7 +21,7 @@ check_type_name_is_valid, get_copy_of_node_with_new_name, get_query_type_name, - get_scalar_names, + get_scalar_names, SchemaTransformError, ) @@ -362,7 +362,7 @@ def leave_object_type_definition( ) -> None: """If the node's name matches the query type, record that we left the query type.""" if not node.fields: - raise CascadingSuppressionError( + raise SchemaTransformError( f"Type renamings {self.renamings} suppressed every type in the schema so it will " f"be impossible to query for anything. To fix this, check why the `renamings` " f"argument of `rename_schema` mapped every type to None." 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 7e0b2818f..da43f441c 100644 --- a/graphql_compiler/tests/schema_transformation_tests/test_rename_schema.py +++ b/graphql_compiler/tests/schema_transformation_tests/test_rename_schema.py @@ -11,7 +11,7 @@ from ...schema_transformation.utils import ( CascadingSuppressionError, InvalidTypeNameError, - SchemaNameConflictError, + SchemaNameConflictError, SchemaTransformError, ) from .input_schema_strings import InputSchemaStrings as ISS @@ -683,7 +683,7 @@ def test_illegal_rename_to_reserved_name_type(self): rename_schema(parse(ISS.basic_schema), {"Human": "__Type"}) def test_suppress_every_type(self): - with self.assertRaises(CascadingSuppressionError): + with self.assertRaises(SchemaTransformError): rename_schema(parse(ISS.basic_schema), {"Human": None}) def test_suppress_all_union_members(self): From 107b59790a62f8492f5b3dae83131714d46ed4ba Mon Sep 17 00:00:00 2001 From: Leon Wu Date: Tue, 2 Jun 2020 15:02:01 -0400 Subject: [PATCH 17/29] Lint --- .../schema_transformation/rename_schema.py | 16 ++++++---------- .../test_rename_schema.py | 3 ++- 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/graphql_compiler/schema_transformation/rename_schema.py b/graphql_compiler/schema_transformation/rename_schema.py index 703b742cc..231ac52f2 100644 --- a/graphql_compiler/schema_transformation/rename_schema.py +++ b/graphql_compiler/schema_transformation/rename_schema.py @@ -17,11 +17,12 @@ from .utils import ( CascadingSuppressionError, SchemaNameConflictError, + SchemaTransformError, check_ast_schema_is_valid, check_type_name_is_valid, get_copy_of_node_with_new_name, get_query_type_name, - get_scalar_names, SchemaTransformError, + get_scalar_names, ) @@ -237,7 +238,7 @@ def __init__( self.scalar_types = frozenset(scalar_types) self.builtin_types = frozenset({"String", "Int", "Float", "Boolean", "ID"}) - def _rename_name_and_add_to_record(self, node: Node) -> Union[ellipsis, Optional[Node]]: + def _rename_name_and_add_to_record(self, node: Node) -> Union[type(REMOVE), Optional[Node]]: """Change the name of the input node if necessary, add the name pair to reverse_name_map. Don't rename if the type is the query type, a scalar type, or a builtin type. @@ -297,13 +298,8 @@ def _rename_name_and_add_to_record(self, node: Node) -> Union[ellipsis, Optional return node_with_new_name def enter( - self, - node: Node, - key: Any, - parent: Any, - path: List[Any], - ancestors: List[Any], - ) -> Union[ellipsis, Optional[Node]]: + self, node: Node, key: Any, parent: Any, path: List[Any], ancestors: List[Any], + ) -> Union[type(REMOVE), Optional[Node]]: """Upon entering a node, operate depending on node type.""" node_type = type(node).__name__ if node_type in self.noop_types: @@ -377,7 +373,7 @@ def enter_field_definition( parent: Any, path: List[Any], ancestors: List[Any], - ) -> Union[ellipsis, Optional[Node]]: + ) -> Union[type(REMOVE), Optional[Node]]: """If inside the query type, rename field and add the name pair to reverse_field_map.""" if self.in_query_type: field_name = node.name.value 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 da43f441c..6190a0f5d 100644 --- a/graphql_compiler/tests/schema_transformation_tests/test_rename_schema.py +++ b/graphql_compiler/tests/schema_transformation_tests/test_rename_schema.py @@ -11,7 +11,8 @@ from ...schema_transformation.utils import ( CascadingSuppressionError, InvalidTypeNameError, - SchemaNameConflictError, SchemaTransformError, + SchemaNameConflictError, + SchemaTransformError, ) from .input_schema_strings import InputSchemaStrings as ISS From 64f089294277e6a24f24b7af35d03d0cbb34e0ec Mon Sep 17 00:00:00 2001 From: Leon Wu Date: Wed, 3 Jun 2020 11:36:02 -0400 Subject: [PATCH 18/29] Clean up comments --- .../schema_transformation/rename_schema.py | 43 +++++++++++++------ 1 file changed, 29 insertions(+), 14 deletions(-) diff --git a/graphql_compiler/schema_transformation/rename_schema.py b/graphql_compiler/schema_transformation/rename_schema.py index 231ac52f2..45967075b 100644 --- a/graphql_compiler/schema_transformation/rename_schema.py +++ b/graphql_compiler/schema_transformation/rename_schema.py @@ -104,7 +104,7 @@ def rename_schema( def _rename_types( ast: DocumentNode, renamings: Dict[str, Optional[str]], query_type: str, scalars: Set[str] ) -> Tuple[DocumentNode, Dict[str, str]]: - """Rename types, enums, interfaces using renamings. + """Rename types, enums, interfaces or suppress types using renamings. The query type will not be renamed. Scalar types, field names, enum values will not be renamed. @@ -120,8 +120,8 @@ def _rename_types( Returns: Tuple[Document, Dict[str, str]], containing the modified version of the AST, and - the renamed type name to original type name map. Map contains all types, including - those that were not renamed. + the renamed type name to original type name map. Map contains all non-suppressed types, + including those that were not renamed. Raises: - InvalidTypeNameError if the schema contains an invalid type name, or if the user attempts @@ -137,7 +137,7 @@ def _rename_types( def _rename_query_type_fields( ast: DocumentNode, renamings: Dict[str, Optional[str]], query_type: str ) -> DocumentNode: - """Rename all fields of the query type. + """Rename or suppress fields of the query type. The input AST will not be modified. @@ -149,6 +149,9 @@ def _rename_query_type_fields( Returns: DocumentNode, representing the modified version of the input schema AST + + Raises: + - InvalidTypeNameError if renamings suppressed every type """ visitor = RenameQueryTypeFieldsVisitor(renamings, query_type) renamed_ast = visit(ast, visitor) @@ -158,7 +161,22 @@ def _rename_query_type_fields( def _check_for_cascading_type_suppression( ast: DocumentNode, renamings: Dict[str, Optional[str]], query_type: str ) -> DocumentNode: - """Check for fields with suppressed types or unions whose members were all suppressed.""" + """Check for fields with suppressed types or unions whose members were all suppressed. + + The input AST will not be modified. + + Args: + ast: DocumentNode, the schema that we're returning a modified version of + renamings: Dict[str, str], mapping original field name to renamed name. If a name + does not appear in the dict, it will be unchanged + query_type: string, name of the query type, e.g. 'RootSchemaQuery' + + Returns: + DocumentNode, representing the modified version of the input schema AST + + Raises: + - CascadingSuppressionError if a type suppression would require further suppressions + """ visitor = CascadingSuppressionCheckVisitor(renamings, query_type) renamed_ast = visit(ast, visitor) return renamed_ast @@ -225,7 +243,8 @@ def __init__( Args: renamings: Dict[str, Optional[str]], mapping from original type name to renamed type - name. Any name not in the dict will be unchanged + name or None (for type suppression). Any name not in the dict will be + unchanged query_type: str, name of the query type (e.g. RootSchemaQuery), which will not be renamed scalar_types: Set[str], set of names of all scalars used in the schema, including @@ -324,8 +343,8 @@ def __init__(self, renamings: Dict[str, Optional[str]], query_type: str) -> None """Create a visitor for renaming fields of the query type in a schema AST. Args: - renamings: Dict[str, Optional[str]], from original field name to renamed field name. Any - name not in the dict will be unchanged + renamings: Dict[str, Optional[str]], from original field name to renamed field name or + None (for type suppression). Any name not in the dict will be unchanged query_type: str, name of the query type (e.g. RootSchemaQuery) """ # Note that as field names and type names have been confirmed to match up, any renamed @@ -394,14 +413,10 @@ def __init__(self, renamings: Dict[str, Optional[str]], query_type: str) -> None """Create a visitor to check that suppression does not cause an illegal state. Args: - renamings: Dict[str, Optional[str]], from original field name to renamed field name. Any - name not in the dict will be unchanged + renamings: Dict[str, Optional[str]], from original field name to renamed field name or + None (for type suppression). Any name not in the dict will be unchanged query_type: str, name of the query type (e.g. RootSchemaQuery) """ - # Note that as field names and type names have been confirmed to match up, any renamed - # query type field already has a corresponding renamed type. If no errors, due to either - # invalid names or name conflicts, were raised when renaming type, no errors will occur when - # renaming query type fields. self.in_query_type = False self.renamings = renamings self.query_type = query_type From 1efe78b0034812a4086a27daa00c52ff11e71d9a Mon Sep 17 00:00:00 2001 From: Leon Wu Date: Wed, 3 Jun 2020 11:59:03 -0400 Subject: [PATCH 19/29] Add type check for CascadingSuppressionError message --- graphql_compiler/schema_transformation/rename_schema.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/graphql_compiler/schema_transformation/rename_schema.py b/graphql_compiler/schema_transformation/rename_schema.py index 45967075b..9ca5d0f63 100644 --- a/graphql_compiler/schema_transformation/rename_schema.py +++ b/graphql_compiler/schema_transformation/rename_schema.py @@ -471,6 +471,12 @@ def enter_field_definition( # doesn't contain the name of the type containing this node (which we need for the error # message). node_grandparent = ancestors[-1] + if not isinstance(node_grandparent, ObjectTypeDefinitionNode): + raise TypeError( + f"Expected field node {node}'s grandparent node to be of type " + f"ObjectTypeDefinitionNode but grandparent node was of type " + f"{type(node_grandparent).__name__} instead." + ) type_name = node_grandparent.name.value raise CascadingSuppressionError( f"Type renamings {self.renamings} attempted to suppress a type, but type " From d940d08c20d92bfeaee4b6643185768b68ab000f Mon Sep 17 00:00:00 2001 From: Leon Wu Date: Wed, 3 Jun 2020 12:05:37 -0400 Subject: [PATCH 20/29] Docstring fix --- graphql_compiler/schema_transformation/rename_schema.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/graphql_compiler/schema_transformation/rename_schema.py b/graphql_compiler/schema_transformation/rename_schema.py index 9ca5d0f63..cfcc341ed 100644 --- a/graphql_compiler/schema_transformation/rename_schema.py +++ b/graphql_compiler/schema_transformation/rename_schema.py @@ -496,11 +496,7 @@ def enter_union_type_definition( path: List[Any], ancestors: List[Any], ) -> None: - """Check that each union still has at least one member. - - Raises: - CascadingSuppressionError if all the members of this union were suppressed. - """ + """Check that each union still has at least one member.""" union_name = node.name.value if not node.types: raise CascadingSuppressionError( From ebde2337b75907404f9ddfb102182acb3a090eab Mon Sep 17 00:00:00 2001 From: Leon Wu Date: Wed, 3 Jun 2020 14:04:22 -0400 Subject: [PATCH 21/29] Comment formatting --- .../schema_transformation/rename_schema.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/graphql_compiler/schema_transformation/rename_schema.py b/graphql_compiler/schema_transformation/rename_schema.py index cfcc341ed..fdf6995b9 100644 --- a/graphql_compiler/schema_transformation/rename_schema.py +++ b/graphql_compiler/schema_transformation/rename_schema.py @@ -46,7 +46,9 @@ def rename_schema( appears in renamings will be renamed to the corresponding value if the value is not None. If the value is None, it will be suppressed in the renamed schema and queries will not be able to access it. + Any such names that do not appear in renamings will be unchanged. + Scalars, directives, enum values, and fields not belonging to the root/query type will never be renamed. @@ -112,7 +114,7 @@ def _rename_types( Args: ast: Document, the schema that we're returning a modified version of - renamings: Dict[str, str], mapping original type/interface/enum name to renamed name. If + renamings: Dict[str, Optional[str]], mapping original type/interface/enum name to renamed name. If a name does not appear in the dict, it will be unchanged query_type: str, name of the query type, e.g. 'RootSchemaQuery' scalars: Set[str], the set of all scalars used in the schema, including user defined @@ -143,9 +145,9 @@ def _rename_query_type_fields( Args: ast: DocumentNode, the schema that we're returning a modified version of - renamings: Dict[str, str], mapping original field name to renamed name. If a name + renamings: Dict[str, Optional[str]], mapping original field name to renamed name. If a name does not appear in the dict, it will be unchanged - query_type: string, name of the query type, e.g. 'RootSchemaQuery' + query_type: str, name of the query type, e.g. 'RootSchemaQuery' Returns: DocumentNode, representing the modified version of the input schema AST @@ -167,9 +169,9 @@ def _check_for_cascading_type_suppression( Args: ast: DocumentNode, the schema that we're returning a modified version of - renamings: Dict[str, str], mapping original field name to renamed name. If a name + renamings: Dict[str, Optional[str]], mapping original field name to renamed name. If a name does not appear in the dict, it will be unchanged - query_type: string, name of the query type, e.g. 'RootSchemaQuery' + query_type: str, name of the query type, e.g. 'RootSchemaQuery' Returns: DocumentNode, representing the modified version of the input schema AST From a5cb84961ea1520778b36c0fd10257865942781e Mon Sep 17 00:00:00 2001 From: Leon Wu Date: Mon, 8 Jun 2020 15:00:05 -0400 Subject: [PATCH 22/29] Create VisitorReturnType --- .../schema_transformation/rename_schema.py | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/graphql_compiler/schema_transformation/rename_schema.py b/graphql_compiler/schema_transformation/rename_schema.py index fdf6995b9..7409afb65 100644 --- a/graphql_compiler/schema_transformation/rename_schema.py +++ b/graphql_compiler/schema_transformation/rename_schema.py @@ -36,6 +36,15 @@ ), ) +# AST visitor functions can return a number of different things, such as returning a Node (to update +# that node) or returning REMOVE (to remove the node). In the current GraphQL-core version +# (>=3,<3.1), REMOVE is set to the singleton object Ellipsis. However, returning Ellipsis prevents +# us from type-hinting functions with anything more specific than Any. For more information, see: +# https://github.com/kensho-technologies/graphql-compiler/pull/834#discussion_r434622400 +# and the issue opened with GraphQL-core here: +# https://github.com/graphql-python/graphql-core/issues/96 +VisitorReturnType = Union[Node, Any] + def rename_schema( ast: DocumentNode, renamings: Dict[str, Optional[str]] @@ -259,7 +268,7 @@ def __init__( self.scalar_types = frozenset(scalar_types) self.builtin_types = frozenset({"String", "Int", "Float", "Boolean", "ID"}) - def _rename_name_and_add_to_record(self, node: Node) -> Union[type(REMOVE), Optional[Node]]: + def _rename_name_and_add_to_record(self, node: Node) -> VisitorReturnType: """Change the name of the input node if necessary, add the name pair to reverse_name_map. Don't rename if the type is the query type, a scalar type, or a builtin type. @@ -320,7 +329,7 @@ def _rename_name_and_add_to_record(self, node: Node) -> Union[type(REMOVE), Opti def enter( self, node: Node, key: Any, parent: Any, path: List[Any], ancestors: List[Any], - ) -> Union[type(REMOVE), Optional[Node]]: + ) -> VisitorReturnType: """Upon entering a node, operate depending on node type.""" node_type = type(node).__name__ if node_type in self.noop_types: @@ -394,7 +403,7 @@ def enter_field_definition( parent: Any, path: List[Any], ancestors: List[Any], - ) -> Union[type(REMOVE), Optional[Node]]: + ) -> VisitorReturnType: """If inside the query type, rename field and add the name pair to reverse_field_map.""" if self.in_query_type: field_name = node.name.value From 0ffa6a713abc8e2b74a24dd409daf8800788b611 Mon Sep 17 00:00:00 2001 From: Leon Wu Date: Mon, 8 Jun 2020 15:35:27 -0400 Subject: [PATCH 23/29] lint --- graphql_compiler/schema_transformation/rename_schema.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/graphql_compiler/schema_transformation/rename_schema.py b/graphql_compiler/schema_transformation/rename_schema.py index 7409afb65..b4f22efb0 100644 --- a/graphql_compiler/schema_transformation/rename_schema.py +++ b/graphql_compiler/schema_transformation/rename_schema.py @@ -123,8 +123,8 @@ def _rename_types( Args: ast: Document, the schema that we're returning a modified version of - renamings: Dict[str, Optional[str]], mapping original type/interface/enum name to renamed name. If - a name does not appear in the dict, it will be unchanged + renamings: Dict[str, Optional[str]], mapping original type/interface/enum name to renamed + name. If a name does not appear in the dict, it will be unchanged query_type: str, name of the query type, e.g. 'RootSchemaQuery' scalars: Set[str], the set of all scalars used in the schema, including user defined scalars and and used builtin scalars, excluding unused builtins From d8c9e8c05a8490ca58b6299956a403b1ca7774ba Mon Sep 17 00:00:00 2001 From: Leon Wu Date: Mon, 8 Jun 2020 16:10:21 -0400 Subject: [PATCH 24/29] Fix _rename_name_and_add_to_record type-hints --- .../schema_transformation/rename_schema.py | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/graphql_compiler/schema_transformation/rename_schema.py b/graphql_compiler/schema_transformation/rename_schema.py index b4f22efb0..d3cdf7b70 100644 --- a/graphql_compiler/schema_transformation/rename_schema.py +++ b/graphql_compiler/schema_transformation/rename_schema.py @@ -1,10 +1,13 @@ # Copyright 2019-present Kensho Technologies, LLC. from collections import namedtuple -from typing import Any, Dict, List, Optional, Set, Tuple, Union +from typing import Any, Dict, List, Optional, Set, Tuple, Union, cast from graphql import ( DocumentNode, + EnumTypeDefinitionNode, FieldDefinitionNode, + InterfaceTypeDefinitionNode, + NamedTypeNode, Node, ObjectTypeDefinitionNode, UnionTypeDefinitionNode, @@ -246,6 +249,13 @@ class RenameSchemaTypesVisitor(Visitor): "UnionTypeDefinitionNode", } ) + type_hint_rename_types = Union[ + EnumTypeDefinitionNode, + InterfaceTypeDefinitionNode, + NamedTypeNode, + ObjectTypeDefinitionNode, + UnionTypeDefinitionNode, + ] def __init__( self, renamings: Dict[str, Optional[str]], query_type: str, scalar_types: Set[str] @@ -268,7 +278,7 @@ def __init__( self.scalar_types = frozenset(scalar_types) self.builtin_types = frozenset({"String", "Int", "Float", "Boolean", "ID"}) - def _rename_name_and_add_to_record(self, node: Node) -> VisitorReturnType: + def _rename_name_and_add_to_record(self, node: type_hint_rename_types) -> VisitorReturnType: """Change the name of the input node if necessary, add the name pair to reverse_name_map. Don't rename if the type is the query type, a scalar type, or a builtin type. @@ -337,7 +347,9 @@ def enter( return None elif node_type in self.rename_types: # Rename node, put name pair into record - renamed_node = self._rename_name_and_add_to_record(node) + renamed_node = self._rename_name_and_add_to_record( + cast(RenameSchemaTypesVisitor.type_hint_rename_types, node) + ) if renamed_node is node: # Name unchanged, continue traversal return None else: From ab550076e04b47fdf79401000892a3c43222623b Mon Sep 17 00:00:00 2001 From: Leon Wu Date: Thu, 11 Jun 2020 17:09:08 -0400 Subject: [PATCH 25/29] Sketch out more ergonomic CascadingSuppressionError message Described [here](https://github.com/kensho-technologies/graphql-compiler/pull/834/files#r432563284) and [here](https://github.com/kensho-technologies/graphql-compiler/pull/834/files#r432562122). Since I'm going to split this PR into several parts instead of trying to merge all the code here, I didn't polish this commit super carefully-- just a draft of a way to do this and making sure I don't lose this code. --- .../schema_transformation/rename_schema.py | 129 ++++++++++++------ 1 file changed, 85 insertions(+), 44 deletions(-) diff --git a/graphql_compiler/schema_transformation/rename_schema.py b/graphql_compiler/schema_transformation/rename_schema.py index d3cdf7b70..16a1fb3fa 100644 --- a/graphql_compiler/schema_transformation/rename_schema.py +++ b/graphql_compiler/schema_transformation/rename_schema.py @@ -95,6 +95,9 @@ def rename_schema( query_type = get_query_type_name(schema) scalars = get_scalar_names(schema) + # Check for fields or unions that depend on types that were suppressed + ast = _check_for_cascading_type_suppression(ast, renamings, query_type) + # Rename types, interfaces, enums ast, reverse_name_map = _rename_types(ast, renamings, query_type, scalars) reverse_name_map_changed_names_only = { @@ -105,9 +108,6 @@ def rename_schema( # Rename query type fields ast = _rename_query_type_fields(ast, renamings, query_type) - - # Check for fields or unions that depend on types that were suppressed - ast = _check_for_cascading_type_suppression(ast, renamings, query_type) return RenamedSchemaDescriptor( schema_ast=ast, schema=build_ast_schema(ast), @@ -193,6 +193,26 @@ def _check_for_cascading_type_suppression( """ visitor = CascadingSuppressionCheckVisitor(renamings, query_type) renamed_ast = visit(ast, visitor) + if visitor.fields_to_suppress or visitor.union_types_to_suppress: + error_message_components = [f"Type renamings {renamings} suppressed types that require " + f"further suppressions to produce a valid renamed schema.\n\n"] + if visitor.fields_to_suppress: + error_message_components.append(f"There exists at least one field that depend on types that would be suppressed:\n") + for object_type in visitor.fields_to_suppress: + error_message_components.append(f"Object type {object_type} contains ") + error_message_components += [f"field {field} of suppressed type {visitor.fields_to_suppress[object_type][field]}, " for field in visitor.fields_to_suppress[object_type]] + error_message_components.append("\n") + error_message_components.append("\nField suppression hasn't been implemented yet, but when it is, you can fix this problem by suppressing the fields described here.") + if visitor.union_types_to_suppress: + error_message_components.append(f"There exists at least one union that would have all of its member types suppressed:\n") + for union_type in visitor.union_types_to_suppress: + error_message_components.append(f"Union type {union_type} has no non-suppressed members: ") + error_message_components += [get_ast_with_non_null_and_list_stripped(union_member).name.value for union_member in union_type.types] + error_message_components.append("\n") + error_message_components.append(f"\nTo fix this, you can suppress the union as well by adding `union_type: None` to the `renamings` argument of `rename_schema`, for each value of union_type described here. Note that " + f"adding suppressions may lead to other types, fields, unions, etc. requiring " + f"suppression so you may need to iterate on this before getting a legal schema.") + raise CascadingSuppressionError("".join(error_message_components)) return renamed_ast @@ -440,9 +460,13 @@ def __init__(self, renamings: Dict[str, Optional[str]], query_type: str) -> None None (for type suppression). Any name not in the dict will be unchanged query_type: str, name of the query type (e.g. RootSchemaQuery) """ - self.in_query_type = False self.renamings = renamings self.query_type = query_type + self.current_type = None + # Maps a type T to a dict which maps a field F belonging to T to the field's type T' + self.fields_to_suppress: Dict[str, Dict[str, str]] = {} + # Record any unions to suppress because all their types were suppressed + self.union_types_to_suppress: List[UnionTypeDefinitionNode] = [] def enter_object_type_definition( self, @@ -453,8 +477,7 @@ def enter_object_type_definition( ancestors: List[Any], ) -> None: """If the node's name matches the query type, record that we entered the query type.""" - if node.name.value == self.query_type: - self.in_query_type = True + self.current_type = node.name.value def leave_object_type_definition( self, @@ -465,8 +488,7 @@ def leave_object_type_definition( ancestors: List[Any], ) -> None: """If the node's name matches the query type, record that we left the query type.""" - if node.name.value == self.query_type: - self.in_query_type = False + self.current_type = None def enter_field_definition( self, @@ -477,38 +499,54 @@ def enter_field_definition( ancestors: List[Any], ) -> None: """If not at query type, check that no field depends on a type that was suppressed.""" - if self.in_query_type: + if self.current_type == self.query_type: return None # At a field of a type that is not the query type field_name = node.name.value - node_type = get_ast_with_non_null_and_list_stripped(node.type) - if node_type == REMOVE: - # Then this field depends on a type that was suppressed, which is illegal - if not ancestors: - raise AssertionError( - f"Expected ancestors to be non-empty list when entering field definition but" - f"ancestors was {ancestors} at node {node}" - ) - # We use the grandparent node (ObjectTypeDefinitionNode) instead of the parent because - # the parent of a FieldDefinitionNode is simply a list of FieldDefinitionNodes, which - # doesn't contain the name of the type containing this node (which we need for the error - # message). - node_grandparent = ancestors[-1] - if not isinstance(node_grandparent, ObjectTypeDefinitionNode): - raise TypeError( - f"Expected field node {node}'s grandparent node to be of type " - f"ObjectTypeDefinitionNode but grandparent node was of type " - f"{type(node_grandparent).__name__} instead." - ) - type_name = node_grandparent.name.value - raise CascadingSuppressionError( - f"Type renamings {self.renamings} attempted to suppress a type, but type " - f"{type_name}'s field {field_name} still depends on that type. Suppressing " - f"individual fields hasn't been implemented yet, but when it is, you can fix " - f"this error by suppressing the field as well. Note that adding suppressions " - f"may lead to other types, fields, unions, etc. requiring suppression so you " - f"may need to iterate on this before getting a legal schema." + field_type = get_ast_with_non_null_and_list_stripped(node.type).name.value + if self.renamings.get(field_type, field_type): + return None + # Reaching this point means this field is of a type to be suppressed. + if self.current_type is None: + raise AssertionError( + f"Entered a field not in any ObjectTypeDefinition scope because " + f"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. + return None + 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 + # if node_type == REMOVE: + # # Then this field depends on a type that was suppressed, which is illegal + # if not ancestors: + # raise AssertionError( + # f"Expected ancestors to be non-empty list when entering field definition but" + # f"ancestors was {ancestors} at node {node}" + # ) + # # We use the grandparent node (ObjectTypeDefinitionNode) instead of the parent because + # # the parent of a FieldDefinitionNode is simply a list of FieldDefinitionNodes, which + # # doesn't contain the name of the type containing this node (which we need for the error + # # message). + # node_grandparent = ancestors[-1] + # if not isinstance(node_grandparent, ObjectTypeDefinitionNode): + # raise TypeError( + # f"Expected field node {node}'s grandparent node to be of type " + # f"ObjectTypeDefinitionNode but grandparent node was of type " + # f"{type(node_grandparent).__name__} instead." + # ) + # type_name = node_grandparent.name.value + # raise CascadingSuppressionError( + # f"Type renamings {self.renamings} attempted to suppress a type, but type " + # f"{type_name}'s field {field_name} still depends on that type. Suppressing " + # f"individual fields hasn't been implemented yet, but when it is, you can fix " + # f"this error by suppressing the field as well. Note that adding suppressions " + # f"may lead to other types, fields, unions, etc. requiring suppression so you " + # f"may need to iterate on this before getting a legal schema." + # ) return None def enter_union_type_definition( @@ -521,11 +559,14 @@ def enter_union_type_definition( ) -> None: """Check that each union still has at least one member.""" union_name = node.name.value - if not node.types: - raise CascadingSuppressionError( - f"Type renamings {self.renamings} suppressed all types belonging to the union " - f"{union_name}. To fix this, you can suppress the union as well by adding " - f"`{union_name}: None` to the `renamings` argument of `rename_schema`. Note that " - f"adding suppressions may lead to other types, fields, unions, etc. requiring " - f"suppression so you may need to iterate on this before getting a legal schema." - ) + # Check if al the union members are suppressed. + for union_member in node.types: + union_member_type = get_ast_with_non_null_and_list_stripped(union_member).name.value + if self.renamings.get(union_member_type, union_member_type): + # Then at least one member of the union is not suppressed, so there is no cascading + # suppression error concern. + return None + if self.renamings.get(union_name): + # If the union is also suppressed, then nothing needs to happen here + return None + self.union_types_to_suppress.append(node) From acf564ca4717d98045fff42ca6df3d06dea80a14 Mon Sep 17 00:00:00 2001 From: Leon Wu Date: Thu, 18 Jun 2020 12:20:52 -0400 Subject: [PATCH 26/29] lint --- .../schema_transformation/rename_schema.py | 40 ++++++++++++++----- 1 file changed, 29 insertions(+), 11 deletions(-) diff --git a/graphql_compiler/schema_transformation/rename_schema.py b/graphql_compiler/schema_transformation/rename_schema.py index 16a1fb3fa..dbf0d7714 100644 --- a/graphql_compiler/schema_transformation/rename_schema.py +++ b/graphql_compiler/schema_transformation/rename_schema.py @@ -194,24 +194,42 @@ def _check_for_cascading_type_suppression( visitor = CascadingSuppressionCheckVisitor(renamings, query_type) renamed_ast = visit(ast, visitor) if visitor.fields_to_suppress or visitor.union_types_to_suppress: - error_message_components = [f"Type renamings {renamings} suppressed types that require " - f"further suppressions to produce a valid renamed schema.\n\n"] + error_message_components = [ + f"Type renamings {renamings} suppressed types that require " + f"further suppressions to produce a valid renamed schema.\n\n" + ] if visitor.fields_to_suppress: - error_message_components.append(f"There exists at least one field that depend on types that would be suppressed:\n") + error_message_components.append( + f"There exists at least one field that depend on types that would be suppressed:\n" + ) for object_type in visitor.fields_to_suppress: error_message_components.append(f"Object type {object_type} contains ") - error_message_components += [f"field {field} of suppressed type {visitor.fields_to_suppress[object_type][field]}, " for field in visitor.fields_to_suppress[object_type]] + error_message_components += [ + f"field {field} of suppressed type {visitor.fields_to_suppress[object_type][field]}, " + for field in visitor.fields_to_suppress[object_type] + ] error_message_components.append("\n") - error_message_components.append("\nField suppression hasn't been implemented yet, but when it is, you can fix this problem by suppressing the fields described here.") + error_message_components.append( + "\nField suppression hasn't been implemented yet, but when it is, you can fix this problem by suppressing the fields described here." + ) if visitor.union_types_to_suppress: - error_message_components.append(f"There exists at least one union that would have all of its member types suppressed:\n") + error_message_components.append( + f"There exists at least one union that would have all of its member types suppressed:\n" + ) for union_type in visitor.union_types_to_suppress: - error_message_components.append(f"Union type {union_type} has no non-suppressed members: ") - error_message_components += [get_ast_with_non_null_and_list_stripped(union_member).name.value for union_member in union_type.types] + error_message_components.append( + f"Union type {union_type} has no non-suppressed members: " + ) + error_message_components += [ + get_ast_with_non_null_and_list_stripped(union_member).name.value + for union_member in union_type.types + ] error_message_components.append("\n") - error_message_components.append(f"\nTo fix this, you can suppress the union as well by adding `union_type: None` to the `renamings` argument of `rename_schema`, for each value of union_type described here. Note that " - f"adding suppressions may lead to other types, fields, unions, etc. requiring " - f"suppression so you may need to iterate on this before getting a legal schema.") + error_message_components.append( + f"\nTo fix this, you can suppress the union as well by adding `union_type: None` to the `renamings` argument of `rename_schema`, for each value of union_type described here. Note that " + f"adding suppressions may lead to other types, fields, unions, etc. requiring " + f"suppression so you may need to iterate on this before getting a legal schema." + ) raise CascadingSuppressionError("".join(error_message_components)) return renamed_ast From 09169db65ee2cc3282ac4134e379f476ff64ee10 Mon Sep 17 00:00:00 2001 From: Leon Wu Date: Thu, 18 Jun 2020 12:21:01 -0400 Subject: [PATCH 27/29] Add interface tests --- .../input_schema_strings.py | 5 + .../test_rename_schema.py | 128 +++++++++++++++++- 2 files changed, 132 insertions(+), 1 deletion(-) 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 50cf9a345..e2f09d640 100644 --- a/graphql_compiler/tests/schema_transformation_tests/input_schema_strings.py +++ b/graphql_compiler/tests/schema_transformation_tests/input_schema_strings.py @@ -293,11 +293,16 @@ class InputSchemaStrings(object): height: Height } + type Dog { + nickname: String + } + directive @stitch(source_field: String!, sink_field: String!) on FIELD_DEFINITION type SchemaQuery { 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 6190a0f5d..b77828ab4 100644 --- a/graphql_compiler/tests/schema_transformation_tests/test_rename_schema.py +++ b/graphql_compiler/tests/schema_transformation_tests/test_rename_schema.py @@ -59,11 +59,16 @@ def test_basic_rename(self): self.assertEqual(renamed_schema_string, print_ast(renamed_schema.schema_ast)) self.assertEqual({"NewHuman": "Human"}, renamed_schema.reverse_name_map) - def test_original_unmodified(self): + def test_original_unmodified_rename(self): original_ast = parse(ISS.basic_schema) rename_schema(original_ast, {"Human": "NewHuman"}) self.assertEqual(original_ast, parse(ISS.basic_schema)) + def test_original_unmodified_suppress(self): + original_ast = parse(ISS.multiple_objects_schema) + rename_schema(original_ast, {"Human": None}) + self.assertEqual(original_ast, parse(ISS.multiple_objects_schema)) + def test_basic_suppress(self): renamed_schema = rename_schema(parse(ISS.multiple_objects_schema), {"Human": None}) renamed_schema_string = dedent( @@ -235,6 +240,121 @@ def test_interface_rename(self): {"NewKid": "Kid", "NewCharacter": "Character"}, renamed_schema.reverse_name_map ) + def test_suppress_interface_implementation(self): + 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 Character { + id: String + } + + type Human implements Character { + id: String + name: String + birthday: Date + } + + type Dog { + nickname: String + } + + directive @stitch(source_field: String!, sink_field: String!) on FIELD_DEFINITION + + type SchemaQuery { + Human: Human + Dog: Dog + } + """ + ) + self.assertEqual(renamed_schema_string, print_ast(renamed_schema.schema_ast)) + self.assertEqual({}, renamed_schema.reverse_name_map) + + def test_suppress_interface_and_all_implementations(self): + renamed_schema = rename_schema( + parse(ISS.various_types_schema), {"Giraffe": None, "Character": None, "Human": None} + ) + renamed_schema_string = dedent( + """\ + schema { + query: SchemaQuery + } + + scalar Date + + enum Height { + TALL + SHORT + } + + type Dog { + nickname: String + } + + directive @stitch(source_field: String!, sink_field: String!) on FIELD_DEFINITION + + type SchemaQuery { + Dog: Dog + } + """ + ) + self.assertEqual(renamed_schema_string, print_ast(renamed_schema.schema_ast)) + self.assertEqual({}, renamed_schema.reverse_name_map) + + def test_suppress_interface_but_not_implementations(self): + renamed_schema = rename_schema(parse(ISS.various_types_schema), {"Character": None}) + renamed_schema_string = dedent( + """\ + schema { + query: SchemaQuery + } + + scalar Date + + enum Height { + TALL + SHORT + } + + type Human { + id: String + name: String + 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 { + Human: Human + Giraffe: Giraffe + Dog: Dog + } + """ + ) + self.assertEqual(renamed_schema_string, print_ast(renamed_schema.schema_ast)) + self.assertEqual( + {}, renamed_schema.reverse_name_map, + ) + def test_multiple_interfaces_rename(self): renamed_schema = rename_schema( parse(ISS.multiple_interfaces_schema), @@ -739,11 +859,16 @@ def get(self, key, default=None): height: NewHeight } + type NewDog { + nickname: String + } + directive @stitch(source_field: String!, sink_field: String!) on FIELD_DEFINITION type SchemaQuery { NewHuman: NewHuman NewGiraffe: NewGiraffe + NewDog: NewDog } """ ) @@ -754,6 +879,7 @@ def get(self, key, default=None): "NewGiraffe": "Giraffe", "NewHeight": "Height", "NewHuman": "Human", + "NewDog": "Dog", }, renamed_schema.reverse_name_map, ) From 21bc14f54241f6f60b6d73da6aa1f32339857006 Mon Sep 17 00:00:00 2001 From: Leon Wu Date: Thu, 18 Jun 2020 15:10:41 -0400 Subject: [PATCH 28/29] Temporarily remove interface implementation suppression tests --- .../test_rename_schema.py | 137 +++++++++++------- 1 file changed, 86 insertions(+), 51 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 b77828ab4..5e743002d 100644 --- a/graphql_compiler/tests/schema_transformation_tests/test_rename_schema.py +++ b/graphql_compiler/tests/schema_transformation_tests/test_rename_schema.py @@ -240,8 +240,83 @@ def test_interface_rename(self): {"NewKid": "Kid", "NewCharacter": "Character"}, renamed_schema.reverse_name_map ) - def test_suppress_interface_implementation(self): - renamed_schema = rename_schema(parse(ISS.various_types_schema), {"Giraffe": None}) + # def test_suppress_interface_implementation(self): + # 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 Character { + # id: String + # } + # + # type Human implements Character { + # id: String + # name: String + # birthday: Date + # } + # + # type Dog { + # nickname: String + # } + # + # directive @stitch(source_field: String!, sink_field: String!) on FIELD_DEFINITION + # + # type SchemaQuery { + # Human: Human + # Dog: Dog + # } + # """ + # ) + # self.assertEqual(renamed_schema_string, print_ast(renamed_schema.schema_ast)) + # self.assertEqual({}, renamed_schema.reverse_name_map) + + # def test_suppress_all_implementations_but_not_interface(self): + # 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 Character { + # id: String + # } + # + # type Dog { + # nickname: String + # } + # + # directive @stitch(source_field: String!, sink_field: String!) on FIELD_DEFINITION + # + # type SchemaQuery { + # Dog: Dog + # } + # """ + # ) + # self.assertEqual(renamed_schema_string, print_ast(renamed_schema.schema_ast)) + # self.assertEqual({}, renamed_schema.reverse_name_map) + + def test_suppress_interface_but_not_implementations(self): + renamed_schema = rename_schema(parse(ISS.various_types_schema), {"Character": None}) renamed_schema_string = dedent( """\ schema { @@ -255,14 +330,15 @@ def test_suppress_interface_implementation(self): SHORT } - interface Character { + type Human { id: String + name: String + birthday: Date } - type Human implements Character { + type Giraffe { id: String - name: String - birthday: Date + height: Height } type Dog { @@ -273,12 +349,15 @@ def test_suppress_interface_implementation(self): type SchemaQuery { Human: Human + Giraffe: Giraffe Dog: Dog } """ ) self.assertEqual(renamed_schema_string, print_ast(renamed_schema.schema_ast)) - self.assertEqual({}, renamed_schema.reverse_name_map) + self.assertEqual( + {}, renamed_schema.reverse_name_map, + ) def test_suppress_interface_and_all_implementations(self): renamed_schema = rename_schema( @@ -311,50 +390,6 @@ def test_suppress_interface_and_all_implementations(self): self.assertEqual(renamed_schema_string, print_ast(renamed_schema.schema_ast)) self.assertEqual({}, renamed_schema.reverse_name_map) - def test_suppress_interface_but_not_implementations(self): - renamed_schema = rename_schema(parse(ISS.various_types_schema), {"Character": None}) - renamed_schema_string = dedent( - """\ - schema { - query: SchemaQuery - } - - scalar Date - - enum Height { - TALL - SHORT - } - - type Human { - id: String - name: String - 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 { - Human: Human - Giraffe: Giraffe - Dog: Dog - } - """ - ) - self.assertEqual(renamed_schema_string, print_ast(renamed_schema.schema_ast)) - self.assertEqual( - {}, renamed_schema.reverse_name_map, - ) - def test_multiple_interfaces_rename(self): renamed_schema = rename_schema( parse(ISS.multiple_interfaces_schema), From 3ec80b4dc7a9956489fc99fa2404fabdc1be75bf Mon Sep 17 00:00:00 2001 From: Leon Wu Date: Thu, 18 Jun 2020 15:51:16 -0400 Subject: [PATCH 29/29] Remove unnecessary ast return for cascading error check Turns out that checking for CascadingSuppressionError before any types get modified doesn't require modifying the AST at all. --- .../schema_transformation/rename_schema.py | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/graphql_compiler/schema_transformation/rename_schema.py b/graphql_compiler/schema_transformation/rename_schema.py index dbf0d7714..3760c6964 100644 --- a/graphql_compiler/schema_transformation/rename_schema.py +++ b/graphql_compiler/schema_transformation/rename_schema.py @@ -96,7 +96,7 @@ def rename_schema( scalars = get_scalar_names(schema) # Check for fields or unions that depend on types that were suppressed - ast = _check_for_cascading_type_suppression(ast, renamings, query_type) + _check_for_cascading_type_suppression(ast, renamings, query_type) # Rename types, interfaces, enums ast, reverse_name_map = _rename_types(ast, renamings, query_type, scalars) @@ -174,7 +174,7 @@ def _rename_query_type_fields( def _check_for_cascading_type_suppression( ast: DocumentNode, renamings: Dict[str, Optional[str]], query_type: str -) -> DocumentNode: +) -> None: """Check for fields with suppressed types or unions whose members were all suppressed. The input AST will not be modified. @@ -185,14 +185,11 @@ def _check_for_cascading_type_suppression( does not appear in the dict, it will be unchanged query_type: str, name of the query type, e.g. 'RootSchemaQuery' - Returns: - DocumentNode, representing the modified version of the input schema AST - Raises: - CascadingSuppressionError if a type suppression would require further suppressions """ visitor = CascadingSuppressionCheckVisitor(renamings, query_type) - renamed_ast = visit(ast, visitor) + visit(ast, visitor) if visitor.fields_to_suppress or visitor.union_types_to_suppress: error_message_components = [ f"Type renamings {renamings} suppressed types that require " @@ -231,7 +228,6 @@ def _check_for_cascading_type_suppression( f"suppression so you may need to iterate on this before getting a legal schema." ) raise CascadingSuppressionError("".join(error_message_components)) - return renamed_ast class RenameSchemaTypesVisitor(Visitor):