Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Schema rename suppress types #834

Closed
wants to merge 30 commits into from
Closed
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
44e314b
Suppress type tests
LWprogramming May 27, 2020
2d6edb6
Fix CascadingSuppressionError tests
LWprogramming May 28, 2020
e03a281
Update union tests
LWprogramming May 28, 2020
a621c83
Implement type suppression
LWprogramming May 28, 2020
f8c78df
lint
LWprogramming May 28, 2020
9fed25e
fix lint issues
LWprogramming May 28, 2020
7e46999
Add to CascadingSuppressionError comment
LWprogramming May 28, 2020
9bd3c66
Add info about iterated suppression
LWprogramming May 28, 2020
43ef8f7
Refactor cascading suppression check out from query type field editing
LWprogramming May 28, 2020
69a9430
Add type hints
LWprogramming May 28, 2020
d2c6940
Fix incorrect comment
LWprogramming May 28, 2020
2e83ea4
Merge branch 'master' into schema_rename_suppress_types
LWprogramming May 29, 2020
360ce36
Address most comments
LWprogramming Jun 1, 2020
e813bb4
Partial lint
LWprogramming Jun 1, 2020
d3680eb
Replace Optional[Any] with Any
LWprogramming Jun 2, 2020
1217f9d
Clarify ancestors parameter usage
LWprogramming Jun 2, 2020
841be44
Change suppress every type's error type to SchemaTransformERror
LWprogramming Jun 2, 2020
107b597
Lint
LWprogramming Jun 2, 2020
64f0892
Clean up comments
LWprogramming Jun 3, 2020
1efe78b
Add type check for CascadingSuppressionError message
LWprogramming Jun 3, 2020
d940d08
Docstring fix
LWprogramming Jun 3, 2020
ebde233
Comment formatting
LWprogramming Jun 3, 2020
a5cb849
Create VisitorReturnType
LWprogramming Jun 8, 2020
0ffa6a7
lint
LWprogramming Jun 8, 2020
d8c9e8c
Fix _rename_name_and_add_to_record type-hints
LWprogramming Jun 8, 2020
ab55007
Sketch out more ergonomic CascadingSuppressionError message
LWprogramming Jun 11, 2020
acf564c
lint
LWprogramming Jun 18, 2020
09169db
Add interface tests
LWprogramming Jun 18, 2020
21bc14f
Temporarily remove interface implementation suppression tests
LWprogramming Jun 18, 2020
3ec80b4
Remove unnecessary ast return for cascading error check
LWprogramming Jun 18, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
233 changes: 205 additions & 28 deletions graphql_compiler/schema_transformation/rename_schema.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,21 @@
# Copyright 2019-present Kensho Technologies, LLC.
from collections import namedtuple

from graphql import build_ast_schema
from graphql.language.visitor import Visitor, visit
from typing import Any, Dict, List, Optional, Set, Tuple, Union

from graphql import (
DocumentNode,
FieldDefinitionNode,
Node,
ObjectTypeDefinitionNode,
UnionTypeDefinitionNode,
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,
Expand All @@ -26,20 +36,25 @@
)


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
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, 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.
renamings: Dict[str, Optional[str]], mapping original type/root type field names to renamed
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider removing the types from the Args section. Now that we have typehints, adding the type here is redundant and could accidentally get out of date.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good-- this PR is already pretty big so after this gets merged I'll be removing these in a separate PR

type/root type field names or None.
Any dict-like object that implements get(key, [default]) may also be used

Returns:
Expand Down Expand Up @@ -75,14 +90,19 @@ def rename_schema(ast, renamings):

# 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),
reverse_name_map=reverse_name_map_changed_names_only,
)


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.
Expand Down Expand Up @@ -113,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.
Expand All @@ -132,6 +154,15 @@ def _rename_query_type_fields(ast, renamings, query_type):
return renamed_ast


def _check_for_cascading_type_suppression(
ast: DocumentNode, renamings: Dict[str, Optional[str]], query_type: str
) -> DocumentNode:
LWprogramming marked this conversation as resolved.
Show resolved Hide resolved
"""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


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

Expand Down Expand Up @@ -186,25 +217,27 @@ 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]
) -> None:
"""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
# 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) -> Union[ellipsis, Optional[Node]]:
LWprogramming marked this conversation as resolved.
Show resolved Hide resolved
"""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.
Expand All @@ -218,8 +251,11 @@ 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. 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
Expand All @@ -232,6 +268,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
LWprogramming marked this conversation as resolved.
Show resolved Hide resolved
check_type_name_is_valid(new_name_string)

if (
Expand All @@ -257,7 +296,14 @@ 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: Optional[Any],
LWprogramming marked this conversation as resolved.
Show resolved Hide resolved
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:
Expand All @@ -268,47 +314,178 @@ 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):
def __init__(self, renamings, query_type):
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, 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)
"""
# 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
# 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
self.query_type = query_type

def enter_object_type_definition(self, node, *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, key, parent, path, ancestors):
def leave_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 left the query type."""
if not node.fields:
raise CascadingSuppressionError(
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, *args):
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
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
return None


class CascadingSuppressionCheckVisitor(Visitor):
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
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: 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],
) -> 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:
"""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
# 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
LWprogramming marked this conversation as resolved.
Show resolved Hide resolved
raise CascadingSuppressionError(
f"Type renamings {self.renamings} attempted to suppress a type, but type "
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like it would raise an error separately for each field it runs into, one at a time. That means fixing everything wrong with a schema requires many iterations where the code complains about one wrong thing at a time, instead of giving you everything that is wrong all at once. So this is working code, but not particularly ergonomic from an end-user point of view. See my comment below for an alternative suggestion that would make raising an error about all the issues at once easier.

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."
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we can add a separate helper (separate PR, though perhaps build it and merge it first?) that given a list of types we'd like to suppress, produces the list of fields etc. that must be suppressed as well for the type suppression to be legal. That enables a few nice things:

  • The workflow becomes more pleasant: "iterate until you stop getting errors" becomes "use this helper, review its output, and apply it if it looks good".
  • The invariant we check for here simply becomes "the helper finds no fields that need suppression", enabling code reuse.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a really good point-- now that you mention it, we can do something similar with unions as well.

i.e. Instead of raising an error here, we could add the information about the field to a data structure belonging to this visitor object. Then, after visit returns, we could perform a single pass through this data structure and raise errors as needed for any fields/ unions, showing everything in a single pass before rename calls _rename_types at all.

I think this actually shouldn't be all that much work since most of it is already done with this visitor class so I can probably get it in this PR.

return 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:
CascadingSuppressionError if all the members of this union were suppressed.
"""
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."
)
15 changes: 15 additions & 0 deletions graphql_compiler/schema_transformation/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,21 @@ class InvalidCrossSchemaEdgeError(SchemaTransformError):
"""


class CascadingSuppressionError(SchemaTransformError):
"""Raised if existing suppressions would require further suppressions.

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.
"""


_alphanumeric_and_underscore = frozenset(six.text_type(string.ascii_letters + string.digits + "_"))


Expand Down
Loading