Move validation of names into GraphQL object constructors

Replicates graphql/graphql-js@5774173

Author: Cito

.coveragerc

@@ -3,9 +3,10 @@ branch = True
 source = src
 omit =
     */conftest.py
+    */test_*_fuzz.py
     */cached_property.py
     */is_iterable.py
-    */test_*_fuzz.py
+    */assert_valid_name.py
 
 [report]
 exclude_lines =

src/graphql/__init__.py

@@ -132,6 +132,9 @@
     # Validate GraphQL schema.
     validate_schema,
     assert_valid_schema,
+    #  Uphold the spec rules about naming
+    assert_name,
+    assert_enum_value_name,
     # Types
     GraphQLType,
     GraphQLInputType,
@@ -508,6 +511,8 @@
     "get_named_type",
     "validate_schema",
     "assert_valid_schema",
+    "assert_name",
+    "assert_enum_value_name",
     "GraphQLType",
     "GraphQLInputType",
     "GraphQLOutputType",

src/graphql/type/__init__.py

@@ -14,6 +14,9 @@
     GraphQLSchema,
 )
 
+# Uphold the spec rules about naming.
+from .assert_name import assert_name, assert_enum_value_name
+
 from .definition import (
     # Predicates
     is_type,
@@ -149,6 +152,8 @@
 __all__ = [
     "is_schema",
     "assert_schema",
+    "assert_name",
+    "assert_enum_value_name",
     "GraphQLSchema",
     "is_type",
     "is_scalar_type",

src/graphql/type/assert_name.py

@@ -0,0 +1,33 @@
+from ..error import GraphQLError
+from ..language.character_classes import is_name_start, is_name_continue
+
+__all__ = ["assert_name", "assert_enum_value_name"]
+
+
+def assert_name(name: str) -> str:
+    """Uphold the spec rules about naming.
+
+    .. deprecated:: 3.2
+    Please use ``get_enter_leave_for_kind`` instead. Will be removed in v3.3.
+    """
+    if name is None:
+        raise TypeError("Must provide name.")
+    if not isinstance(name, str):
+        raise TypeError("Expected name to be a string.")
+    if not name:
+        raise GraphQLError("Expected name to be a non-empty string.")
+    if not all(is_name_continue(char) for char in name[1:]):
+        raise GraphQLError(
+            f"Names must only contain [_a-zA-Z0-9] but {name!r} does not."
+        )
+    if not is_name_start(name[0]):
+        raise GraphQLError(f"Names must start with [_a-zA-Z] but {name!r} does not.")
+    return name
+
+
+def assert_enum_value_name(name: str) -> str:
+    """Uphold the spec rules about naming enum values."""
+    assert_name(name)
+    if name in {"true", "false", "null"}:
+        raise GraphQLError(f"Enum values cannot be named: {name}.")
+    return name

src/graphql/type/definition.py

@@ -56,6 +56,7 @@
     Undefined,
 )
 from ..utilities.value_from_ast_untyped import value_from_ast_untyped
+from .assert_name import assert_name, assert_enum_value_name
 
 if TYPE_CHECKING:
     from .schema import GraphQLSchema  # noqa: F401
@@ -208,10 +209,7 @@ def __init__(
         ast_node: Optional[TypeDefinitionNode] = None,
         extension_ast_nodes: Optional[Collection[TypeExtensionNode]] = None,
     ) -> None:
-        if not name:
-            raise TypeError("Must provide name.")
-        if not isinstance(name, str):
-            raise TypeError("The name must be a string.")
+        assert_name(name)
         if description is not None and not is_description(description):
             raise TypeError("The description must be a string.")
         if extensions is None:
@@ -465,7 +463,7 @@ def __init__(
             )
         else:
             args = {
-                name: value
+                assert_name(name): value
                 if isinstance(value, GraphQLArgument)
                 else GraphQLArgument(cast(GraphQLInputType, value))
                 for name, value in args.items()
@@ -737,7 +735,8 @@ def fields(self) -> GraphQLFieldMap:
         try:
             fields = resolve_thunk(self._fields)
         except Exception as error:
-            raise TypeError(f"{self.name} fields cannot be resolved. {error}")
+            cls = GraphQLError if isinstance(error, GraphQLError) else TypeError
+            raise cls(f"{self.name} fields cannot be resolved. {error}") from error
         if not isinstance(fields, Mapping) or not all(
             isinstance(key, str) for key in fields
         ):
@@ -753,7 +752,7 @@ def fields(self) -> GraphQLFieldMap:
                 f"{self.name} fields must be GraphQLField or output type objects."
             )
         return {
-            name: value
+            assert_name(name): value
             if isinstance(value, GraphQLField)
             else GraphQLField(value)  # type: ignore
             for name, value in fields.items()
@@ -767,7 +766,8 @@ def interfaces(self) -> List["GraphQLInterfaceType"]:
                 self._interfaces  # type: ignore
             )
         except Exception as error:
-            raise TypeError(f"{self.name} interfaces cannot be resolved. {error}")
+            cls = GraphQLError if isinstance(error, GraphQLError) else TypeError
+            raise cls(f"{self.name} interfaces cannot be resolved. {error}") from error
         if interfaces is None:
             interfaces = []
         elif not is_collection(interfaces) or not all(
@@ -862,7 +862,8 @@ def fields(self) -> GraphQLFieldMap:
         try:
             fields = resolve_thunk(self._fields)
         except Exception as error:
-            raise TypeError(f"{self.name} fields cannot be resolved. {error}")
+            cls = GraphQLError if isinstance(error, GraphQLError) else TypeError
+            raise cls(f"{self.name} fields cannot be resolved. {error}") from error
         if not isinstance(fields, Mapping) or not all(
             isinstance(key, str) for key in fields
         ):
@@ -878,7 +879,7 @@ def fields(self) -> GraphQLFieldMap:
                 f"{self.name} fields must be GraphQLField or output type objects."
             )
         return {
-            name: value
+            assert_name(name): value
             if isinstance(value, GraphQLField)
             else GraphQLField(value)  # type: ignore
             for name, value in fields.items()
@@ -892,7 +893,8 @@ def interfaces(self) -> List["GraphQLInterfaceType"]:
                 self._interfaces  # type: ignore
             )
         except Exception as error:
-            raise TypeError(f"{self.name} interfaces cannot be resolved. {error}")
+            cls = GraphQLError if isinstance(error, GraphQLError) else TypeError
+            raise cls(f"{self.name} interfaces cannot be resolved. {error}") from error
         if interfaces is None:
             interfaces = []
         elif not is_collection(interfaces) or not all(
@@ -985,7 +987,8 @@ def types(self) -> List[GraphQLObjectType]:
         try:
             types: Collection[GraphQLObjectType] = resolve_thunk(self._types)
         except Exception as error:
-            raise TypeError(f"{self.name} types cannot be resolved. {error}")
+            cls = GraphQLError if isinstance(error, GraphQLError) else TypeError
+            raise cls(f"{self.name} types cannot be resolved. {error}") from error
         if types is None:
             types = []
         elif not is_collection(types) or not all(
@@ -1081,7 +1084,7 @@ def __init__(
             values = cast(Dict, values)
             values = {key: value.value for key, value in values.items()}
         values = {
-            key: value
+            assert_enum_value_name(key): value
             if isinstance(value, GraphQLEnumValue)
             else GraphQLEnumValue(value)
             for key, value in values.items()
@@ -1338,7 +1341,8 @@ def fields(self) -> GraphQLInputFieldMap:
         try:
             fields = resolve_thunk(self._fields)
         except Exception as error:
-            raise TypeError(f"{self.name} fields cannot be resolved. {error}")
+            cls = GraphQLError if isinstance(error, GraphQLError) else TypeError
+            raise cls(f"{self.name} fields cannot be resolved. {error}") from error
         if not isinstance(fields, Mapping) or not all(
             isinstance(key, str) for key in fields
         ):
@@ -1355,7 +1359,7 @@ def fields(self) -> GraphQLInputFieldMap:
                 " GraphQLInputField or input type objects."
             )
         return {
-            name: value
+            assert_name(name): value
             if isinstance(value, GraphQLInputField)
             else GraphQLInputField(value)  # type: ignore
             for name, value in fields.items()

src/graphql/type/directives.py

@@ -2,6 +2,7 @@
 
 from ..language import ast, DirectiveLocation
 from ..pyutils import inspect, is_description, FrozenList
+from .assert_name import assert_name
 from .definition import GraphQLArgument, GraphQLInputType, GraphQLNonNull, is_input_type
 from .scalars import GraphQLBoolean, GraphQLString
 
@@ -45,10 +46,7 @@ def __init__(
         extensions: Optional[Dict[str, Any]] = None,
         ast_node: Optional[ast.DirectiveDefinitionNode] = None,
     ) -> None:
-        if not name:
-            raise TypeError("Directive must be named.")
-        elif not isinstance(name, str):
-            raise TypeError("The directive name must be a string.")
+        assert_name(name)
         try:
             locations = [
                 value
@@ -76,7 +74,7 @@ def __init__(
             )
         else:
             args = {
-                name: value
+                assert_name(name): value
                 if isinstance(value, GraphQLArgument)
                 else GraphQLArgument(cast(GraphQLInputType, value))
                 for name, value in args.items()

src/graphql/type/validate.py

@@ -11,7 +11,7 @@
     cast,
 )
 
-from ..error import GraphQLError, located_error
+from ..error import GraphQLError
 from ..pyutils import inspect
 from ..language import (
     DirectiveNode,
@@ -41,7 +41,6 @@
     is_required_argument,
     is_required_input_field,
 )
-from ..utilities.assert_valid_name import is_valid_name_error
 from ..utilities.type_comparators import is_equal_type, is_type_sub_type_of
 from .directives import is_directive, GraphQLDeprecatedDirective
 from .introspection import is_introspection_type
@@ -109,10 +108,7 @@ def report_error(
         if nodes and not isinstance(nodes, Node):
             nodes = [node for node in nodes if node]
         nodes = cast(Optional[Collection[Node]], nodes)
-        self.add_error(GraphQLError(message, nodes))
-
-    def add_error(self, error: GraphQLError) -> None:
-        self.errors.append(error)
+        self.errors.append(GraphQLError(message, nodes))
 
     def validate_root_types(self) -> None:
         schema = self.schema
@@ -191,9 +187,12 @@ def validate_name(self, node: Any, name: Optional[str] = None) -> None:
         except AttributeError:  # pragma: no cover
             pass
         else:
-            error = is_valid_name_error(name)
-            if error:
-                self.add_error(located_error(error, ast_node))
+            if name.startswith("__"):
+                self.report_error(
+                    f"Name {name!r} must not begin with '__',"
+                    " which is reserved by GraphQL introspection.",
+                    ast_node,
+                )
 
     def validate_types(self) -> None:
         validate_input_object_circular_refs = InputObjectCircularRefsValidator(self)
@@ -457,12 +456,6 @@ def validate_enum_values(self, enum_type: GraphQLEnumType) -> None:
         for value_name, enum_value in enum_values.items():
             # Ensure valid name.
             self.validate_name(enum_value, value_name)
-            if value_name in ("true", "false", "null"):
-                self.report_error(
-                    f"Enum type {enum_type.name} cannot include value:"
-                    f" {value_name}.",
-                    enum_value.ast_node,
-                )
 
     def validate_input_fields(self, input_obj: GraphQLInputObjectType) -> None:
         fields = input_obj.fields

src/graphql/utilities/assert_valid_name.py

@@ -1,43 +1,38 @@
-import re
 from typing import Optional
 
-from ..language.character_classes import is_name_start, is_name_continue
+from ..type.assert_name import assert_name
 from ..error import GraphQLError
 
 __all__ = ["assert_valid_name", "is_valid_name_error"]
 
 
-re_name = re.compile("^[_a-zA-Z][_a-zA-Z0-9]*$")
-
-
 def assert_valid_name(name: str) -> str:
-    """Uphold the spec rules about naming."""
+    """Uphold the spec rules about naming.
+
+    .. deprecated:: 3.2
+       Please use ``assert_name`` instead. Will be removed in v3.3.
+    """
     error = is_valid_name_error(name)
     if error:
         raise error
     return name
 
 
 def is_valid_name_error(name: str) -> Optional[GraphQLError]:
-    """Return an Error if a name is invalid."""
+    """Return an Error if a name is invalid.
+
+    .. deprecated:: 3.2
+       Please use ``assert_name`` instead. Will be removed in v3.3.
+    """
     if not isinstance(name, str):
         raise TypeError("Expected name to be a string.")
-
     if name.startswith("__"):
         return GraphQLError(
             f"Name {name!r} must not begin with '__',"
             " which is reserved by GraphQL introspection."
         )
-
-    if not name:
-        return GraphQLError("Expected name to be a non-empty string.")
-
-    if not all(is_name_continue(char) for char in name[1:]):
-        return GraphQLError(
-            f"Names must only contain [_a-zA-Z0-9] but {name!r} does not."
-        )
-
-    if not is_name_start(name[0]):
-        return GraphQLError(f"Names must start with [_a-zA-Z] but {name!r} does not.")
-
+    try:
+        assert_name(name)
+    except GraphQLError as error:
+        return error
     return None

tests/pyutils/test_description.py

@@ -107,15 +107,15 @@ def graphql_named_type():
             named_type = GraphQLNamedType(name="Foo", description="not lazy")
             assert named_type.name == "Foo"
             assert named_type.description == "not lazy"
-            with raises(TypeError, match="The name must be a string\\."):
+            with raises(TypeError, match="Expected name to be a string\\."):
                 GraphQLNamedType(name=lazy_string)
             with raises(TypeError, match="The description must be a string\\."):
                 GraphQLNamedType(name="Foo", description=lazy_string)
             with registered(LazyString):
                 named_type = GraphQLNamedType(name="Foo", description=lazy_string)
                 assert named_type.description is lazy_string
                 assert str(named_type.description).endswith("lazy?")
-                with raises(TypeError, match="The name must be a string\\."):
+                with raises(TypeError, match="Expected name to be a string\\."):
                     GraphQLNamedType(name=lazy_string)
 
         def graphql_field():
@@ -185,15 +185,15 @@ def graphql_directive():
             directive = GraphQLDirective("Foo", [], description="not lazy")
             assert directive.name == "Foo"
             assert directive.description == "not lazy"
-            with raises(TypeError, match="The directive name must be a string\\."):
+            with raises(TypeError, match="Expected name to be a string\\."):
                 GraphQLDirective(lazy_string, [])
             with raises(TypeError, match="Foo description must be a string\\."):
                 GraphQLDirective("Foo", [], description=lazy_string)
             with registered(LazyString):
                 directive = GraphQLDirective("Foo", [], description=lazy_string)
                 assert directive.description is lazy_string
                 assert str(directive.description).endswith("lazy?")
-                with raises(TypeError, match="The directive name must be a string\\."):
+                with raises(TypeError, match="Expected name to be a string\\."):
                     GraphQLDirective(lazy_string, [])
 
     def introspection():