From b303e000848118f8930fec1cc2741da8c1a4940d Mon Sep 17 00:00:00 2001 From: tangkikodo Date: Tue, 6 Oct 2026 21:48:06 +0800 Subject: [PATCH] fix(graphql): align SDL nullability with introspection for nullable annotations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - compose: _render_struct_type_sdl decided the ! suffix via _is_optional alone and missed Literal[..., None], which its sibling _build_type_ref does handle — SDL and introspection disagreed within the same module. - entity: map_to_sdl no longer forces NON_NULL on output for nullable annotations (Optional[T] / T | None / Literal[..., None]); output SDL now matches map_to_introspection and the Python annotation (#320 option A; behavioral change — such fields lose the ! suffix). - extract annotation_is_nullable as the single predicate for NON_NULL decisions; dedup the Optional-unwrap step shared by literal_info and _literal_constraint (_optional_core); drop literal_allowed_values (no library callers since #318). Fixes #320 Co-Authored-By: Claude Code --- .../graphql/schema/type_mapper.py | 16 ++-- pydantic_resolve/graphql/type_mapping.py | 79 ++++++++++++------- pydantic_resolve/use_case/compose_schema.py | 18 +++-- tests/graphql/fixtures/literal_entities.py | 5 +- tests/graphql/test_input_types.py | 9 ++- tests/graphql/test_literal_type_mapping.py | 37 ++++++--- tests/use_case/test_compose_backports.py | 12 +++ 7 files changed, 113 insertions(+), 63 deletions(-) diff --git a/pydantic_resolve/graphql/schema/type_mapper.py b/pydantic_resolve/graphql/schema/type_mapper.py index fc31ee10..a47e2eb2 100644 --- a/pydantic_resolve/graphql/schema/type_mapper.py +++ b/pydantic_resolve/graphql/schema/type_mapper.py @@ -15,9 +15,9 @@ from pydantic_resolve.utils.class_util import safe_issubclass from pydantic_resolve.utils.types import get_core_types, _is_optional, _is_list from pydantic_resolve.graphql.type_mapping import ( + annotation_is_nullable, map_scalar_type, is_enum_type, - literal_is_nullable, ) if TYPE_CHECKING: @@ -248,22 +248,20 @@ def map_to_sdl(self, python_type: type, is_input: bool = False) -> str: Args: python_type: Python type is_input: Whether this is for an input type. - When True, Optional[T] fields produce "T" (no ! suffix). Returns: SDL type string (e.g., "String!", "[User!]!") """ - is_optional = _is_optional(python_type) - # Literal['open', None] allows null without being a Union — the - # None member must suppress the ! suffix just like Optional. - if literal_is_nullable(python_type): - is_optional = True + # Nullable annotations (Optional[T] / T | None, or a None member + # inside a Literal) skip the ! suffix on both the input and output + # sides — matching map_to_introspection and the Python annotation + # itself (#320: output SDL previously forced NON_NULL on them). + is_nullable = annotation_is_nullable(python_type) gql_type = self.map_to_graphql_type(python_type, is_input) sdl = gql_type.to_sdl() # Add NON_NULL wrapper if not already wrapped. - # For Optional[T] in input types, skip the ! suffix. - if is_optional and is_input: + if is_nullable: return sdl.rstrip('!') if not sdl.endswith('!'): diff --git a/pydantic_resolve/graphql/type_mapping.py b/pydantic_resolve/graphql/type_mapping.py index 778e67b8..d782aeb7 100644 --- a/pydantic_resolve/graphql/type_mapping.py +++ b/pydantic_resolve/graphql/type_mapping.py @@ -143,6 +143,23 @@ def map_python_to_graphql(python_type: type, include_required: bool = True) -> s return f"{scalar_name}{required_suffix}" +def _optional_core(annotation: Any) -> tuple[Any, bool] | None: + """Strip one ``Optional`` / ``X | None`` wrapper; return ``(core, had_none)``. + + Returns ``None`` for multi-branch unions (``Union[X, Y]`` without + ``None``), which are not a plain Optional. Single source for the + unwrap step shared by ``literal_info`` and ``_literal_constraint``. + """ + origin = get_origin(annotation) + if origin is Union or origin is _types.UnionType: + all_args = get_args(annotation) + args = [arg for arg in all_args if arg is not type(None)] + if len(args) != 1: + return None + return args[0], len(args) != len(all_args) + return annotation, False + + def literal_info(annotation: Any) -> tuple[type, bool] | None: """Validate a scalar ``Literal`` annotation; return ``(scalar_type, has_none)``. @@ -157,14 +174,13 @@ def literal_info(annotation: Any) -> tuple[type, bool] | None: (``schema/type_mapper.py`` / generators) both call this so the rules cannot drift. Ported from nexusx #151 via #313. """ - origin = get_origin(annotation) - if origin is Union or origin is _types.UnionType: - args = [arg for arg in get_args(annotation) if arg is not type(None)] - if len(args) != 1: - return None - core = args[0] - else: - core = annotation + stripped = _optional_core(annotation) + if stripped is None: + return None + # Optional-wrapper nullability is deliberately ignored here (callers + # consult _is_optional for that layer); has_none below covers only the + # None members inside the Literal itself. + core = stripped[0] if get_origin(core) is not Literal: return None @@ -208,36 +224,39 @@ def literal_is_nullable(annotation: Any) -> bool: return info is not None and info[1] +def annotation_is_nullable(annotation: Any) -> bool: + """True when ``annotation`` admits ``null`` through either layer. + + Covers ``Optional[T]`` / ``T | None`` wrappers *and* a ``None`` member + inside a ``Literal``. Single predicate for NON_NULL decisions so the + SDL renders and the introspection/type-ref renders cannot drift apart + again (#320: the compose SDL renderer asked only ``_is_optional`` and + missed the Literal layer its sibling ``_build_type_ref`` did handle). + """ + return _is_optional(annotation) or literal_is_nullable(annotation) + + def _literal_constraint(annotation: Any) -> tuple[tuple[Any, ...], bool] | None: """Extract literal values and nullability through Optional/list wrappers.""" - origin = get_origin(annotation) - if origin is Union or origin is _types.UnionType: - all_args = get_args(annotation) - args = [arg for arg in all_args if arg is not type(None)] - if len(args) == 1: - constraint = _literal_constraint(args[0]) - if constraint is not None: - values, has_none = constraint - return values, has_none or len(args) != len(all_args) + stripped = _optional_core(annotation) + if stripped is None: return None - if origin is list: - args = get_args(annotation) - if args: - return _literal_constraint(args[0]) + core, wrapper_none = stripped + if get_origin(core) is list: + args = get_args(core) + constraint = _literal_constraint(args[0]) if args else None + if constraint is not None: + return constraint[0], constraint[1] or wrapper_none return None - if origin is Literal: - all_values = get_args(annotation) + if get_origin(core) is Literal: + all_values = get_args(core) values = tuple(v for v in all_values if v is not None) - return (values, len(values) != len(all_values)) if values else None + if not values: + return None + return values, wrapper_none or len(values) != len(all_values) return None -def literal_allowed_values(annotation: Any) -> tuple[Any, ...] | None: - """Return non-None Literal values, unwrapping Optional/list annotations.""" - constraint = _literal_constraint(annotation) - return constraint[0] if constraint is not None else None - - def _format_literal_value(value: Any) -> str: if isinstance(value, bool): return "true" if value else "false" diff --git a/pydantic_resolve/use_case/compose_schema.py b/pydantic_resolve/use_case/compose_schema.py index 4e39f8f2..41bc6620 100644 --- a/pydantic_resolve/use_case/compose_schema.py +++ b/pydantic_resolve/use_case/compose_schema.py @@ -39,6 +39,7 @@ SCALAR_TYPES, ) from pydantic_resolve.graphql.type_mapping import ( + annotation_is_nullable, is_enum_type, literal_info, describe_literal_values, @@ -47,7 +48,6 @@ from pydantic_resolve.use_case.context import is_from_context_annotation from pydantic_resolve.utils.class_util import safe_issubclass from pydantic_resolve.utils.types import ( - _is_optional, _resolve_function_type_hints, get_core_types, get_return_annotation, @@ -288,7 +288,7 @@ def _render_field(field_info: FieldInfo, registry: dict[str, TypeInfo]) -> dict[ "type": _build_type_ref( field_info.python_type, registry, - force_non_null=not _is_optional(field_info.python_type), + force_non_null=not annotation_is_nullable(field_info.python_type), ), "isDeprecated": field_info.is_deprecated, "deprecationReason": field_info.deprecation_reason, @@ -296,8 +296,8 @@ def _render_field(field_info: FieldInfo, registry: dict[str, TypeInfo]) -> dict[ def _render_arg(arg_info: ArgumentInfo, registry: dict[str, TypeInfo]) -> dict[str, Any]: - # An arg is nullable when its annotation is Optional OR it has a default. - is_optional = _is_optional(arg_info.python_type) or arg_info.default_value is not None + # An arg is nullable when its annotation admits null OR it has a default. + is_optional = annotation_is_nullable(arg_info.python_type) or arg_info.default_value is not None return { "name": arg_info.name, "description": arg_info.description, @@ -318,7 +318,7 @@ def _render_input_field(field_info: FieldInfo, registry: dict[str, TypeInfo]) -> "type": _build_type_ref( field_info.python_type, registry, - force_non_null=not _is_optional(field_info.python_type), + force_non_null=not annotation_is_nullable(field_info.python_type), is_input=True, ), "defaultValue": field_info.default_value, @@ -460,11 +460,11 @@ def method_sdl( sdl_parts: list[str] = [] # Method signature as a comment header args_sdl = ", ".join( - f"{a.name}: {_type_ref_to_sdl(_build_type_ref(a.python_type, registry, force_non_null=(a.default_value is None and not _is_optional(a.python_type)), is_input=True))}" + f"{a.name}: {_type_ref_to_sdl(_build_type_ref(a.python_type, registry, force_non_null=(a.default_value is None and not annotation_is_nullable(a.python_type)), is_input=True))}" for a in field_info.args ) return_sdl = _type_ref_to_sdl( - _build_type_ref(field_info.python_type, registry, force_non_null=not _is_optional(field_info.python_type)) + _build_type_ref(field_info.python_type, registry, force_non_null=not annotation_is_nullable(field_info.python_type)) ) sdl_parts.append(f"# {service_name}.{method_name}({args_sdl}): {return_sdl}") for type_name, type_def in sorted(reachable.items()): @@ -527,7 +527,9 @@ def _render_struct_type_sdl(type_info: TypeInfo, *, keyword: str) -> str: field_lines.append(_indent(_block_string(field.description), 2)) gql = _TYPE_MAPPER.map_to_graphql_type(field.python_type) sdl = gql.to_sdl() - if not _is_optional(field.python_type) and not sdl.endswith("!"): + # #320: ask annotation_is_nullable (not bare _is_optional) so the + # SDL renderer agrees with _build_type_ref on Literal[..., None]. + if not annotation_is_nullable(field.python_type) and not sdl.endswith("!"): sdl = f"{sdl}!" field_lines.append(f" {field.name}: {sdl}") parts.append(f"{keyword} {type_info.name} {{\n" + "\n".join(field_lines) + "\n}") diff --git a/tests/graphql/fixtures/literal_entities.py b/tests/graphql/fixtures/literal_entities.py index 96dc11a5..9a7bdba4 100644 --- a/tests/graphql/fixtures/literal_entities.py +++ b/tests/graphql/fixtures/literal_entities.py @@ -5,7 +5,7 @@ bool, and a None member (nullable). Used by test_literal_type_mapping.py. """ -from typing import List, Literal +from typing import List, Literal, Optional from pydantic import BaseModel from pydantic_resolve import base_entity, query @@ -33,6 +33,9 @@ class TaskLiteEntity(BaseModel, BaseEntity): flag: Literal[True, False] = True # None member: the value may be null although this is not a Union. note: Literal["draft", None] = None + # Plain Optional output field: pins the #320 option-A change (nullable + # in SDL output, matching introspection, instead of the old String!). + memo: Optional[str] = None @query async def get_all(cls) -> List["TaskLiteEntity"]: diff --git a/tests/graphql/test_input_types.py b/tests/graphql/test_input_types.py index c0024453..09843ddb 100644 --- a/tests/graphql/test_input_types.py +++ b/tests/graphql/test_input_types.py @@ -142,9 +142,12 @@ def test_input_type_generated_in_schema(self, schema_builder): # Check CreateUserInput is generated as input type assert "input CreateUserInput {" in schema - assert "name: String!" in schema - assert "email: String!" in schema - assert "age: Int!" in schema + input_block = schema.split("input CreateUserInput")[1].split("}")[0] + assert "name: String!" in input_block + assert "email: String!" in input_block + # age is Optional[int] — nullable, no ! suffix (#320). + assert "age: Int" in input_block + assert "age: Int!" not in input_block def test_nested_input_type_generated(self, schema_builder): """Test that nested input types are generated""" diff --git a/tests/graphql/test_literal_type_mapping.py b/tests/graphql/test_literal_type_mapping.py index e8310371..d9ee73d5 100644 --- a/tests/graphql/test_literal_type_mapping.py +++ b/tests/graphql/test_literal_type_mapping.py @@ -16,8 +16,8 @@ from pydantic_resolve import config_global_resolver from pydantic_resolve.graphql import GraphQLHandler, SchemaBuilder from pydantic_resolve.graphql.type_mapping import ( + annotation_is_nullable, describe_literal_values, - literal_allowed_values, literal_is_nullable, map_python_to_graphql, map_scalar_type, @@ -90,11 +90,19 @@ def test_literal_is_nullable(self): assert literal_is_nullable(Optional[Literal["a"]]) is False assert literal_is_nullable(str) is False - def test_literal_allowed_values_unwraps_wrappers(self): - assert literal_allowed_values(Literal["open", "closed"]) == ("open", "closed") - assert literal_allowed_values(Optional[Literal["open", None]]) == ("open",) - assert literal_allowed_values(List[Literal[1, 2]]) == (1, 2) - assert literal_allowed_values(str) is None + def test_annotation_is_nullable_covers_both_layers(self): + # The single predicate NON_NULL decisions must ask (#320): Optional + # wrapper and Literal None member, either one alone suffices. + assert annotation_is_nullable(Optional[str]) is True + assert annotation_is_nullable(str | None) is True + assert annotation_is_nullable(Literal["a", None]) is True + assert annotation_is_nullable(Optional[Literal["a"]]) is True + assert annotation_is_nullable(Literal["a"]) is False + assert annotation_is_nullable(str) is False + assert annotation_is_nullable(int | str) is False + # Element nullability inside a list is the inner type's concern — + # the list annotation itself is not nullable. + assert annotation_is_nullable(List[Literal["a", None]]) is False def test_describe_literal_values(self): assert describe_literal_values(None, Literal["open", "closed"]) == ( @@ -150,12 +158,17 @@ def test_bool_literal_field_type(self): def test_str_literal_field_type(self): assert "status: String!" in self.sdl - def test_none_member_field_follows_optional_convention(self): - # Output fields are NON_NULL by convention on this path (even - # Optional[T] renders as T!); the None-member Literal matches the - # Optional treatment. Nullability itself is covered by the input - # side (below) and by map_python_to_graphql. - assert "note: String!" in self.sdl + def test_none_member_output_field_is_nullable(self): + # #320: output SDL matches introspection — nullable annotations + # (Literal[..., None] included) no longer force NON_NULL. + assert "note: String" in self.sdl + assert "note: String!" not in self.sdl + + def test_optional_output_field_is_nullable(self): + # #320 option A: plain Optional[T] output fields used to render as + # T! by convention; they now match introspection (nullable). + assert "memo: String" in self.sdl + assert "memo: String!" not in self.sdl def test_none_member_input_field_is_nullable(self): input_block = self.sdl.split("input TaskFilter")[1] diff --git a/tests/use_case/test_compose_backports.py b/tests/use_case/test_compose_backports.py index 6157a243..ccb195d5 100644 --- a/tests/use_case/test_compose_backports.py +++ b/tests/use_case/test_compose_backports.py @@ -23,6 +23,7 @@ from pydantic_resolve import query from pydantic_resolve.use_case.business import UseCaseService from pydantic_resolve.use_case.compose import ComposeError +from pydantic_resolve.use_case.compose_schema import method_sdl from pydantic_resolve.use_case.manager import UseCaseAppConfig, UseCaseManager @@ -171,6 +172,17 @@ def test_input_and_output_literal_descriptions(self): assert dto.fields["enabled"].description == "Allowed values: true, false" assert dto.fields["note"].description == "Allowed values: draft (or null)" + def test_method_sdl_literal_nullability_matches_introspection(self): + # #320: the SDL renderer must agree with _build_type_ref/introspection — + # a None member suppresses the ! suffix instead of forcing NON_NULL. + res = UseCaseManager([_app([LiteralService])]).get_app("t") + sdl = method_sdl(res.compose_schema, "LiteralService", "get_task") + block = sdl.split("type LiteralTaskDTO")[1] + assert "note: String" in block + assert "note: String!" not in block + assert "enabled: Boolean!" in block + assert "status: String!" in block + @pytest.mark.parametrize( ("status", "ok"), [("open", True), ("pending", False)] )