Fix pydantic unions that annotate the None member - #4557
Conversation
A pydantic field like Union[str, SkipJsonSchema[None]] raised InvalidUnionTypeError at schema construction. replace_types_recursively kept the Annotated wrapper on the None member, so the union never collapsed to Optional[str] and strawberry tried to build a GraphQL union out of str. Unwrap union members that are None behind an Annotated. Metadata on other members stays untouched, so strawberry.lazy references and strawberry.union names keep resolving.
|
Thanks for adding the Below is the changelog that will be used for the release. This release fixes Pydantic fields that annotate the Write This release was contributed by @davidpavlovschi in #4557 |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
_unwrap_annotated_none, consider guardingget_args(type_)[0]with a length check to avoid a possibleIndexErroron malformedAnnotatedtypes, and perhaps clarify in the docstring that you returntype(None)rather than the singletonNone. - The
_unwrap_annotated_nonehelper only handlesAnnotated[None, ...]at the top level; if nested constructs likeAnnotated[Union[str, None], ...]or multiple layers ofAnnotatedare expected, you may want to make the unwrapping logic recursive or more general.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_unwrap_annotated_none`, consider guarding `get_args(type_)[0]` with a length check to avoid a possible `IndexError` on malformed `Annotated` types, and perhaps clarify in the docstring that you return `type(None)` rather than the singleton `None`.
- The `_unwrap_annotated_none` helper only handles `Annotated[None, ...]` at the top level; if nested constructs like `Annotated[Union[str, None], ...]` or multiple layers of `Annotated` are expected, you may want to make the unwrapping logic recursive or more general.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Greptile SummaryThis PR fixes Pydantic schema conversion for unions whose
Confidence Score: 5/5The PR appears safe to merge, with the union normalization narrowly scoped and covered at both type-conversion and schema-execution levels. The changed conversion preserves non-None annotation metadata, correctly handles both supported union representations, and introduces no accepted functional, security, or quality issues.
|
| Filename | Overview |
|---|---|
| strawberry/experimental/pydantic/fields.py | Extends recursive union reconstruction to normalize annotated None members without altering non-None metadata. |
| tests/experimental/pydantic/test_basic.py | Adds focused coverage for plain annotated None, Pydantic SkipJsonSchema, multi-member optionals, and named-union metadata. |
| tests/experimental/pydantic/schema/test_basic.py | Adds an end-to-end Pydantic v2 test confirming nullable SDL generation and successful query execution. |
| RELEASE.md | Documents the corrected optional-field behavior and its motivating schema-construction failure. |
Reviews (1): Last reviewed commit: "Fix pydantic unions that annotate the No..." | Re-trigger Greptile
Repro
A pydantic model that annotates the
Nonemember of a union kills schema construction:I hit this on main at 34e9707, pydantic 2.13.4, Python 3.13.
Root cause
replace_types_recursivelyinstrawberry/experimental/pydantic/fields.pyrebuilds the union member by member. Each member keeps itsAnnotatedwrapper, soUnion[str, Annotated[None, SkipJsonSchema()]]comes out the other side unchanged. Python does not collapse that toOptional[str], because the union holds no bareNoneType.StrawberryAnnotation._is_optionallooks for that bareNoneType. It does not find one, so strawberry classifies the field as a real GraphQL union and handsstrtofrom_union. Scalars are not valid union members, so you getInvalidUnionTypeError.Nothing here is specific to
SkipJsonSchema. AnyAnnotated[None, ...]union member breaks the same way.Fix
I unwrap union members that are
Nonebehind anAnnotated, right where the union gets rebuilt:The union branch now also catches
typing.Union, not only PEP 604UnionType. That matters becauseint | Annotated[None, "m"]evaluates totyping.Unionat runtime, nottypes.UnionType, so the oldisinstance(replaced_type, UnionType)branch never saw the broken case. Thetyping.Unionpath used to fall through tocopy_with, which isUnion[args]under the hood, so the rebuild itself behaves the same.What it does not change
Only the
Nonemember loses its metadata. Metadata onNonesays nothing about the GraphQL type. Metadata on any other member can, and it survives untouched:strawberry.lazyputs aStrawberryLazyReferenceinAnnotatedmetadata, read byStrawberryAnnotation._get_type_with_args.strawberry.unionputs aStrawberryUnioninAnnotatedmetadata, which names the union in the schema.I added a test for the second one: a
strawberry.union("AnimalUnion")inside an optional field still prints asAnimalUnion.Optional detection past this point is unchanged.
create_optionalstill filtersNoneTypeandUNSETand rebuilds the child type the same way.Test evidence
New tests, all failing before the fix and passing after:
tests/experimental/pydantic/test_basic.py::test_annotated_none_in_unioncovers a plainAnnotated[None, "metadata"]member, both a two member and a three member union.tests/experimental/pydantic/test_basic.py::test_skip_json_schema_none_in_unioncoversSkipJsonSchema[None], scalar and list.tests/experimental/pydantic/test_basic.py::test_annotated_union_member_keeps_metadatapins the metadata that must survive.tests/experimental/pydantic/schema/test_basic.py::test_basic_type_with_skip_json_schema_none_unionbuilds the schema from the issue and asserts the printed SDL plus a query result.Counts from my runs:
tests/experimental/pydanticon pydantic 2.13.4, before: 4 failed, 154 passed, 11 skipped, 1 xfailed. After: 158 passed, 11 skipped, 1 xfailed.tests/experimental/pydanticon pydantic 1.10.26, before: 2 failed, 167 passed, 10 skipped, 1 xfailed. After: 169 passed, 10 skipped, 1 xfailed. The twoSkipJsonSchematests skip on v1.tests/typecheckers/test_pydantic.py::test_pydantic_type, which raisesFileNotFoundError: [Errno 2] No such file or directory: 'ty'on my machine. It fails identically with my change reverted.uv run mypy strawberry/experimental/pydantic/fields.py: no issues.uv run ruff checkandruff format --checkon the changed files: clean.Fixes #3992
I used AI assistance on this change and reviewed everything.
Summary by Sourcery
Normalize Annotated None union members in Pydantic integration so Strawberry correctly treats these fields as optional while preserving metadata on other union members, and document the change with accompanying tests and release notes.
Bug Fixes:
Enhancements:
Documentation:
Tests: