fix: preserve NewType in pydantic type resolution for scalar_map - #4483
fix: preserve NewType in pydantic type resolution for scalar_map#4483binggao1230 wants to merge 8 commits into
Conversation
|
Thanks for adding the Below is the changelog that will be used for the release. This release fixes Strawberry now preserves the annotation until schema scalar resolution, so the This release was contributed by @gaoflow in #4483 Additional contributors: @patrick91 |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
schema_converter.from_scalar, the__supertype__unwrapping happens before checkingself.scalar_registry, which will bypass any scalar explicitly registered for theNewTypeitself; consider only unwrapping to__supertype__after a direct lookup inscalar_registryfails to preserve explicit NewType registrations. - The recursive
__supertype__fallback inis_scalarwill traverse any chain of types with a__supertype__attribute; it may be safer to guard this with a NewType-specific check (or a max depth) to avoid surprising behavior if non-NewType annotations define__supertype__.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `schema_converter.from_scalar`, the `__supertype__` unwrapping happens before checking `self.scalar_registry`, which will bypass any scalar explicitly registered for the `NewType` itself; consider only unwrapping to `__supertype__` after a direct lookup in `scalar_registry` fails to preserve explicit NewType registrations.
- The recursive `__supertype__` fallback in `is_scalar` will traverse any chain of types with a `__supertype__` attribute; it may be safer to guard this with a NewType-specific check (or a max depth) to avoid surprising behavior if non-NewType annotations define `__supertype__`.
## Individual Comments
### Comment 1
<location path="strawberry/experimental/pydantic/_compat.py" line_range="214-215" />
<code_context>
if type_ is None:
raise UnsupportedTypeError
- if is_new_type(type_):
- return new_type_supertype(type_)
-
return type_
</code_context>
<issue_to_address>
**question (bug_risk):** Dropping NewType unwrapping from `get_basic_type` may change behavior for non-scalar NewTypes.
Previously this function unwrapped NewTypes via `new_type_supertype`, so Pydantic saw the underlying builtin/standard type. Now it returns the NewType itself. Scalar cases may be covered by the new scalar logic, but non-scalar NewTypes (e.g. around complex/custom types) will behave differently and could break models relying on the underlying type. Please verify whether any non-scalar Pydantic paths depended on the old unwrapping before finalizing this change.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if is_new_type(type_): | ||
| return new_type_supertype(type_) |
There was a problem hiding this comment.
question (bug_risk): Dropping NewType unwrapping from get_basic_type may change behavior for non-scalar NewTypes.
Previously this function unwrapped NewTypes via new_type_supertype, so Pydantic saw the underlying builtin/standard type. Now it returns the NewType itself. Scalar cases may be covered by the new scalar logic, but non-scalar NewTypes (e.g. around complex/custom types) will behave differently and could break models relying on the underlying type. Please verify whether any non-scalar Pydantic paths depended on the old unwrapping before finalizing this change.
Greptile SummaryThis PR fixes a bug where
Confidence Score: 3/5The fix correctly handles the intended scalar_map + NewType case, but introduces two regressions: fields typed as a NewType wrapping a pydantic model no longer resolve, and chained NewTypes can crash from_scalar with an AttributeError. The core scalar_map lookup fix is sound, but the removal of the NewType unwrap in _compat.py is broader than needed — it also silences the model-type replacement path. Separately, is_scalar recurses through the full supertype chain while from_scalar only peels one level, so those two functions disagree for chained NewTypes. Both issues affect real code paths without test coverage to catch them. schema_converter.py (from_scalar single-level unwrap) and _compat.py (NewType wrapping pydantic model regression) Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant F as fields.py
participant C as _compat.py (get_basic_type)
participant SC as schema_converter.py (from_scalar)
participant SR as scalar_registry
Note over F,SR: Before this PR — NewType always unwrapped early
F->>C: get_basic_type(MyNewType)
C-->>F: str (supertype)
F->>SC: from_scalar(str)
SC->>SR: lookup str
SR-->>SC: GraphQL String
SC-->>F: String scalar (scalar_map entry for MyNewType ignored)
Note over F,SR: After this PR — NewType preserved, scalar_map consulted
F->>C: get_basic_type(MyNewType)
C-->>F: MyNewType (preserved)
F->>SC: from_scalar(MyNewType)
SC->>SR: lookup MyNewType
SR-->>SC: custom scalar (from scalar_map)
SC-->>F: Custom scalar
Note over F,SR: Edge case — chained NewType, from_scalar only unwraps 1 level
F->>C: get_basic_type(B)
C-->>F: B (preserved)
F->>SC: from_scalar(B)
SC->>SR: lookup B — not found
SC->>SC: unwrap one level to A
SC->>SR: lookup A — not found
SC-->>F: AttributeError (A._scalar_definition missing)
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant F as fields.py
participant C as _compat.py (get_basic_type)
participant SC as schema_converter.py (from_scalar)
participant SR as scalar_registry
Note over F,SR: Before this PR — NewType always unwrapped early
F->>C: get_basic_type(MyNewType)
C-->>F: str (supertype)
F->>SC: from_scalar(str)
SC->>SR: lookup str
SR-->>SC: GraphQL String
SC-->>F: String scalar (scalar_map entry for MyNewType ignored)
Note over F,SR: After this PR — NewType preserved, scalar_map consulted
F->>C: get_basic_type(MyNewType)
C-->>F: MyNewType (preserved)
F->>SC: from_scalar(MyNewType)
SC->>SR: lookup MyNewType
SR-->>SC: custom scalar (from scalar_map)
SC-->>F: Custom scalar
Note over F,SR: Edge case — chained NewType, from_scalar only unwraps 1 level
F->>C: get_basic_type(B)
C-->>F: B (preserved)
F->>SC: from_scalar(B)
SC->>SR: lookup B — not found
SC->>SC: unwrap one level to A
SC->>SR: lookup A — not found
SC-->>F: AttributeError (A._scalar_definition missing)
|
| elif hasattr(scalar, "__supertype__"): | ||
| # NewType fallback: unwrap to the underlying supertype so | ||
| # the scalar can be resolved via the default registry or | ||
| # scalar_map entries keyed by Python builtins. | ||
| scalar = scalar.__supertype__ |
There was a problem hiding this comment.
from_scalar unwraps only one level, inconsistent with recursive is_scalar
is_scalar in scalars.py follows the full __supertype__ chain recursively, so it returns True for a chained NewType like B = NewType("B", A) where A = NewType("A", str) and neither is in the registry. But from_scalar unwraps only one level — replacing B with A — and when A is still not in self.scalar_registry the fallback scalar._scalar_definition raises AttributeError. The two functions must agree: either recurse in from_scalar or loop until the resolved type is in the registry before falling through to _scalar_definition.
| elif hasattr(scalar, "__supertype__"): | |
| # NewType fallback: unwrap to the underlying supertype so | |
| # the scalar can be resolved via the default registry or | |
| # scalar_map entries keyed by Python builtins. | |
| scalar = scalar.__supertype__ | |
| elif hasattr(scalar, "__supertype__"): | |
| # NewType fallback: unwrap through the full __supertype__ chain | |
| # so the scalar can be resolved via the default registry or | |
| # scalar_map entries keyed by Python builtins. | |
| while ( | |
| hasattr(scalar, "__supertype__") | |
| and scalar not in self.scalar_registry | |
| ): | |
| scalar = scalar.__supertype__ |
bad5292 to
0d15125
Compare
The initial fix for strawberry-graphql#4483 removed NewType unwrapping in get_basic_type to allow scalar_map entries keyed by NewType to intercept. However, this unconditionally skipped the unwrap, breaking fields annotated with a NewType whose supertype is a pydantic BaseModel — is_model_class() returns False for NewType instances, so replace_pydantic_types() never detects the underlying model. Restore conditional unwrapping: only unwrap NewType when the supertype is a pydantic model (needed for type replacement); preserve NewType identity for scalar supertypes (needed for scalar_map interception). Additionally, make from_scalar() walk the full __supertype__ chain when looking up scalar registrations, matching the recursive behavior of is_scalar(). The previous single-level unwrap caused AttributeError for chained NewTypes (B = NewType('B', A), A = NewType('A', int)) when neither was in the registry.
|
Thanks for the review @greptile-apps! Both issues are valid — I've pushed a fix:
|
|
The fixes look solid. A few observations: 1. The current guard only checks one level: 2. The (Or swap to 3. This is a very low-probability edge case, but Overall the approach is correct and the backward-compat story holds up. The Tip: You can customize Greptile's behavior for this repo with |
|
Addressed the chained NewType review points in Changes:
Verification:
|
scalar_map entries keyed by NewType were ignored for pydantic model fields because the pydantic compat layer unconditionally unwrapped NewType annotations to their supertype during field processing. Remove the early NewType unwrapping from get_basic_type() in both PydanticV1Compat and PydanticV2Compat. Add __supertype__ fallback logic to is_scalar() and from_scalar() so that: - NewTypes in scalar_map are matched correctly (the fix) - NewTypes not in scalar_map still resolve via their supertype (backward compatible) Closes strawberry-graphql#4482
The initial fix for strawberry-graphql#4483 removed NewType unwrapping in get_basic_type to allow scalar_map entries keyed by NewType to intercept. However, this unconditionally skipped the unwrap, breaking fields annotated with a NewType whose supertype is a pydantic BaseModel — is_model_class() returns False for NewType instances, so replace_pydantic_types() never detects the underlying model. Restore conditional unwrapping: only unwrap NewType when the supertype is a pydantic model (needed for type replacement); preserve NewType identity for scalar supertypes (needed for scalar_map interception). Additionally, make from_scalar() walk the full __supertype__ chain when looking up scalar registrations, matching the recursive behavior of is_scalar(). The previous single-level unwrap caused AttributeError for chained NewTypes (B = NewType('B', A), A = NewType('A', int)) when neither was in the registry.
Add end-to-end Pydantic scalar_map coverage and retain enum resolution for unmapped aliases. Guard fallback traversal to genuine NewTypes and refresh the release note.
513dbd0 to
d3ff297
Compare
Fixes #4482 —
scalar_mapentries keyed bytyping.NewTypewere silently ignored for pydantic model fields during schema generation.Problem
The pydantic compat layer (
PydanticV1Compat.get_basic_type/PydanticV2Compat.get_basic_typeinstrawberry/experimental/pydantic/_compat.py) unconditionally unwrappedtyping.NewTypeannotations to their underlying supertype (e.g.ID = NewType("ID", str)→str) during field type resolution. This happens at decorator time, so by the time the schema converter consultsscalar_registry(which includesscalar_mapentries), the original NewType identity has already been destroyed — the field type isstr, not the NewType the user keyed inscalar_map.Fix
Three coordinated changes:
if is_new_type(type_): return new_type_supertype(type_)lines from both compat classes (4 lines)__supertype__fallback tois_scalar()— if a NewType is not inscalar_registry, recursively check its supertype__supertype__fallback tofrom_scalar()— unwrap a NewType to its supertype as a last resortThis ensures:
scalar_mapare matched correctly (the fix)scalar_mapentry still resolve via their supertype (backward compatible)scalar_mapwith NewTypesThis pull request was prepared with the assistance of AI, under my direction and review.
Summary by Sourcery
Preserve typing.NewType identities during scalar resolution so pydantic-backed fields correctly respect scalar mappings while maintaining fallback behavior via supertypes.
Bug Fixes:
Enhancements: