Skip to content

fix: preserve NewType in pydantic type resolution for scalar_map - #4483

Draft
binggao1230 wants to merge 8 commits into
strawberry-graphql:mainfrom
binggao1230:fix/pydantic-scalar-map-newtype
Draft

fix: preserve NewType in pydantic type resolution for scalar_map#4483
binggao1230 wants to merge 8 commits into
strawberry-graphql:mainfrom
binggao1230:fix/pydantic-scalar-map-newtype

Conversation

@binggao1230

@binggao1230 binggao1230 commented Jun 24, 2026

Copy link
Copy Markdown

Fixes #4482scalar_map entries keyed by typing.NewType were silently ignored for pydantic model fields during schema generation.

Problem

The pydantic compat layer (PydanticV1Compat.get_basic_type / PydanticV2Compat.get_basic_type in strawberry/experimental/pydantic/_compat.py) unconditionally unwrapped typing.NewType annotations 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 consults scalar_registry (which includes scalar_map entries), the original NewType identity has already been destroyed — the field type is str, not the NewType the user keyed in scalar_map.

Fix

Three coordinated changes:

  1. _compat.py: Remove the if is_new_type(type_): return new_type_supertype(type_) lines from both compat classes (4 lines)
  2. scalars.py: Add __supertype__ fallback to is_scalar() — if a NewType is not in scalar_registry, recursively check its supertype
  3. schema_converter.py: Add __supertype__ fallback to from_scalar() — unwrap a NewType to its supertype as a last resort

This ensures:

  • NewTypes in scalar_map are matched correctly (the fix)
  • NewTypes without a scalar_map entry still resolve via their supertype (backward compatible)
  • No behavioral change for existing code that does not use scalar_map with NewTypes

This 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:

  • Ensure scalar_map entries keyed by typing.NewType are honored for pydantic model fields during schema generation.
  • Prevent NewType-based scalar annotations from being silently treated as their underlying builtin types when resolving schema scalars.

Enhancements:

  • Add recursive supertype fallback in scalar and schema conversion logic so NewTypes without explicit scalar mappings still resolve via their supertypes.

@github-actions

github-actions Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Thanks for adding the RELEASE.md file!

Below is the changelog that will be used for the release.


This release fixes StrawberryConfig.scalar_map being ignored for Pydantic
fields annotated with NewType.

Strawberry now preserves the annotation until schema scalar resolution, so the
generated GraphQL type and runtime scalar conversion use the same configured
scalar. Unmapped NewType annotations continue to resolve through their
underlying type.

This release was contributed by @gaoflow in #4483

Additional contributors: @patrick91

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hey - I've found 1 issue, and left some high level feedback:

  • 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__.
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>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment on lines -214 to -215
if is_new_type(type_):
return new_type_supertype(type_)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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-apps

greptile-apps Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes a bug where scalar_map entries keyed by typing.NewType were silently ignored for pydantic model fields, because the pydantic compat layer unconditionally unwrapped NewTypes to their supertypes before the scalar registry was consulted. The fix stops that early unwrapping and instead adds __supertype__ fallbacks in both is_scalar and from_scalar.

  • _compat.py: Removes the NewType-to-supertype unwrap in get_basic_type for both V1 and V2 compat classes, preserving the NewType identity so scalar_map lookups succeed. This also inadvertently stops the unwrap for NewTypes whose supertype is a pydantic BaseModel subclass, which used to be resolved correctly.
  • scalars.py: is_scalar now recursively follows __supertype__, but from_scalar in schema_converter.py only unwraps one level — for chained NewTypes not in the registry, is_scalar returns True while from_scalar crashes with AttributeError.
  • No new tests are included to verify the fix or guard against regressions.

Confidence Score: 3/5

The 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

Filename Overview
strawberry/experimental/pydantic/_compat.py Removes unconditional NewType-to-supertype unwrapping from both compat classes, enabling scalar_map lookups by NewType identity, but silently breaks fields whose NewType supertype is a pydantic BaseModel subclass.
strawberry/scalars.py Adds a recursive supertype fallback to is_scalar; logic is sound, but is now ahead of the single-level unwrap in from_scalar, creating an inconsistency for chained NewTypes.
strawberry/schema/schema_converter.py Adds a one-level supertype fallback to from_scalar; inconsistent with the recursive is_scalar — will crash with AttributeError for any chained NewType whose intermediate type is also not in the registry.

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)
Loading
%%{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)
Loading

Comments Outside Diff (1)

  1. strawberry/experimental/pydantic/_compat.py, line 207-214 (link)

    P1 NewTypes wrapping pydantic model types no longer resolved

    Removing the is_new_type unwrap means get_basic_type now passes a NewType object through intact. Downstream, replace_pydantic_types calls is_model_class (a lenient_issubclass check against BaseModel). A NewType object in Python 3.10+ is a callable instance, not a class, so issubclass raises TypeError and lenient_issubclass returns False. The pydantic model hidden inside the NewType is never replaced with its strawberry counterpart, causing type resolution to fail for any field annotated with a NewType whose supertype is a pydantic BaseModel subclass — a pattern that worked before this PR.

Reviews (1): Last reviewed commit: "fix: preserve NewType in pydantic type r..." | Re-trigger Greptile

Comment thread strawberry/schema/schema_converter.py Outdated
Comment on lines +853 to +857
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__

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Suggested change
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__

@binggao1230
binggao1230 force-pushed the fix/pydantic-scalar-map-newtype branch from bad5292 to 0d15125 Compare June 24, 2026 06:16
binggao1230 added a commit to binggao1230/strawberry that referenced this pull request Jun 24, 2026
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.
@binggao1230

Copy link
Copy Markdown
Author

Thanks for the review @greptile-apps! Both issues are valid — I've pushed a fix:

  1. NewType wrapping pydantic BaseModelget_basic_type now conditionally unwraps NewType only when the supertype is a pydantic model (using is_model_class(supertype)). Scalar NewTypes (str/int/etc) preserve their identity for scalar_map interception.

  2. Chained NewType from_scalar inconsistencyfrom_scalar now walks the full __supertype__ chain looking for a registry match, matching the recursive behavior of is_scalar(). Falls back to single-level unwrap if nothing in the chain is registered.

@greptile-apps

greptile-apps Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

The fixes look solid. A few observations:

1. get_basic_type — chained NewType wrapping a pydantic model

The current guard only checks one level: NewType → pydantic model. But NewType(NewType(pydantic.BaseModel)) (a NewType wrapping another NewType that wraps a model) won't be caught. It's probably an unusual case, but it's worth being consistent with the recursive approach used in from_scalar. Consider looping/recursing here too — or at least leaving a comment acknowledging the limitation.

2. from_scalarwhile/else semantics

The while/else construct is correct Python (the else branch runs when the loop exits normally, i.e. no break), but it's subtle. The else clause here is a single-level unwrap fallback: if nothing in the chain matched the registry, use scalar.__supertype__ (the immediate supertype, not the terminal one). That means for a three-level chain A → B → C where none are registered, you'd resolve to B, not C. This could produce a surprising miss at the if scalar in self.scalar_registry check below. A small clarifying comment (or changing the fallback to resolved — the terminal type — might be safer):

                else:
                    # No match in chain — fall back to the immediate supertype
                    # and let the caller handle a miss.
                    scalar = scalar.__supertype__

(Or swap to resolved if you want to fall all the way down to the concrete type.)

3. is_scalar — infinite loop on a badly formed __supertype__ cycle

This is a very low-probability edge case, but is_scalar is now mutually recursive with no cycle guard. A __supertype__ pointing back to itself would infinite-loop. Worth a brief note in the docstring or a visited guard if you want to be defensive.

Overall the approach is correct and the backward-compat story holds up. The while/else fallback behaviour in point 2 is the only thing I'd want to double-check before merging.

Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.

@binggao1230

Copy link
Copy Markdown
Author

Addressed the chained NewType review points in 9ff8baef.

Changes:

  • get_basic_type now walks the NewType chain when looking for wrapped pydantic models, so NewType(NewType(BaseModel)) still resolves through replace_pydantic_types correctly while preserving scalar NewType identities.
  • from_scalar now continues the NewType fallback to the resolved terminal scalar instead of falling back to only the immediate supertype. It also stops when it reaches a custom scalar definition, so chained aliases of strawberry.scalar(NewType(...)) keep the custom scalar rather than degrading to the builtin supertype.
  • Added regression coverage for both cases: a pydantic nested model wrapped through two NewTypes, and a chained NewType alias to a custom scalar covering schema output plus serialize/parse behavior.

Verification:

  • uv run pytest tests/experimental/pydantic/schema/test_basic.py::test_basic_type_with_newtype_chain_nested_model tests/schema/test_scalars.py::test_newtype_chain_to_custom_scalar -q
  • uv run pytest tests/experimental/pydantic/schema/test_basic.py tests/schema/test_scalars.py -q
  • uv run ruff format --check strawberry/experimental/pydantic/_compat.py strawberry/schema/schema_converter.py tests/experimental/pydantic/schema/test_basic.py tests/schema/test_scalars.py
  • uv run ruff check strawberry/experimental/pydantic/_compat.py strawberry/schema/schema_converter.py tests/experimental/pydantic/schema/test_basic.py tests/schema/test_scalars.py
  • uv run mypy strawberry/experimental/pydantic/_compat.py strawberry/schema/schema_converter.py
  • git diff --check

binggao1230 and others added 6 commits July 23, 2026 23:40
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.
@patrick91
patrick91 force-pushed the fix/pydantic-scalar-map-newtype branch from 513dbd0 to d3ff297 Compare July 23, 2026 22:41
@patrick91 patrick91 self-assigned this Jul 23, 2026
@patrick91
patrick91 marked this pull request as draft July 23, 2026 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

scalar_map not being considered for pydantic types in schema generation

2 participants