Fix error type when field with resolver= lacks type annotations#4360
Conversation
When using strawberry.field(resolver=fn) where both the field annotation and the resolver return type are missing, raise MissingFieldAnnotationError instead of MissingReturnAnnotationError. This gives users more accurate guidance to annotate the field directly. The @strawberry.field decorator case still correctly raises MissingReturnAnnotationError by detecting whether the resolver function exists separately in the class or was applied as a decorator. Closes strawberry-graphql#447
Reviewer's GuideAdjusts Strawberry’s field annotation validation to raise a more appropriate error for untyped fields using explicit File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Thanks for adding the Below is the changelog that will be used for the release. Raise This release was contributed by @youngjaekwon in #4360 Additional contributors: @pre-commit-ci[bot] |
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The
resolver_defined_separatelycheck only inspectscls.__dict__, so resolvers inherited from base classes won’t be detected as “defined separately”; if that matters for your use case, consider walking the MRO or documenting/adjusting this behavior. - The resolver-vs-field detection logic inside
_check_field_annotationsis getting fairly dense; consider extracting it into a small helper function to make the intent clearer and keep_check_field_annotationsfocused on control flow.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `resolver_defined_separately` check only inspects `cls.__dict__`, so resolvers inherited from base classes won’t be detected as “defined separately”; if that matters for your use case, consider walking the MRO or documenting/adjusting this behavior.
- The resolver-vs-field detection logic inside `_check_field_annotations` is getting fairly dense; consider extracting it into a small helper function to make the intent clearer and keep `_check_field_annotations` focused on control flow.
## Individual Comments
### Comment 1
<location path="strawberry/types/object_type.py" line_range="86-91" />
<code_context>
+ # latter, MissingFieldAnnotationError is more helpful
+ # since the user should annotate the field directly.
+ resolver_func = field_.base_resolver.wrapped_func
+ resolver_defined_separately = any(
+ v is resolver_func
+ for k, v in cls.__dict__.items()
+ if k != field_name
+ and callable(v)
+ and not isinstance(v, StrawberryField)
+ )
+ if (
</code_context>
<issue_to_address>
**issue:** Resolver detection may miss classmethod/staticmethod-based resolvers and other descriptor wrappers.
Because this logic compares `v is resolver_func` over `cls.__dict__`, it will fail for `@classmethod` / `@staticmethod` or other descriptors used as `strawberry.field(resolver=MyClass.my_resolver)`. In those cases, `cls.__dict__` contains the descriptor (e.g. `classmethod`), not the underlying function, so `resolver_defined_separately` incorrectly remains `False`. To handle these cases, unwrap known descriptors (e.g. via `getattr(v, "__func__", v)`) before comparing to `resolver_func`.
</issue_to_address>
### Comment 2
<location path="tests/fields/test_resolvers.py" line_range="138" />
<code_context>
+ ),
)
-def test_raises_error_when_return_annotation_missing_resolver():
+def test_raises_field_error_when_using_untyped_explicit_resolver():
@strawberry.type
class Query2:
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test to cover the documented "known limitation" case where the resolver has the same name as the field.
The PR description mentions the specific case `goodbye = strawberry.field(resolver=goodbye)`, where the resolver in `cls.__dict__` is overwritten and we fall back to `MissingReturnAnnotationError`. Please add a dedicated test for this scenario to guard against regressions: define a type where `goodbye` is first declared as an unannotated function, then overwritten via `goodbye = strawberry.field(resolver=goodbye)`, and assert that `MissingReturnAnnotationError` (with the expected message) is raised.
</issue_to_address>
### Comment 3
<location path="tests/fields/test_resolvers.py" line_range="131-138" />
<code_context>
@pytest.mark.raises_strawberry_exception(
- MissingReturnAnnotationError,
- match='Return annotation missing for field "goodbye", did you forget to add it?',
+ MissingFieldAnnotationError,
+ match=(
+ 'Unable to determine the type of field "goodbye". Either annotate it '
+ "directly, or provide a typed resolver using @strawberry.field."
+ ),
)
-def test_raises_error_when_return_annotation_missing_resolver():
+def test_raises_field_error_when_using_untyped_explicit_resolver():
@strawberry.type
class Query2:
</code_context>
<issue_to_address>
**suggestion (testing):** Add/keep a dedicated test ensuring the decorator-style resolver without return annotation still raises `MissingReturnAnnotationError`.
This change now tests the `strawberry.field(resolver=fn)` path and expects `MissingFieldAnnotationError`. To also cover the decorator path introduced in `_check_field_annotations`, please add a test for the `@strawberry.field` decorator with an untyped resolver and no field annotation, asserting that `MissingReturnAnnotationError` is raised. If that test already exists, consider co-locating or briefly documenting it to clarify the distinction between the two behaviors.
Suggested implementation:
```python
@pytest.mark.raises_strawberry_exception(
MissingFieldAnnotationError,
match=(
'Unable to determine the type of field "goodbye". Either annotate it '
"directly, or provide a typed resolver using @strawberry.field."
),
)
def test_raises_field_error_when_using_untyped_explicit_resolver():
@strawberry.type
class Query2:
def adios(self):
@pytest.mark.raises_strawberry_exception(
MissingReturnAnnotationError,
match='Return annotation missing for field "hola", did you forget to add it?',
)
def test_raises_error_when_using_untyped_decorator_resolver():
@strawberry.type
class Query:
@strawberry.field
def hola(self):
return "hola"
strawberry.Schema(query=Query)
```
If this file does not already import `MissingReturnAnnotationError`, add it to the existing imports at the top of `tests/fields/test_resolvers.py`, alongside the other Strawberry exceptions.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| ), | ||
| ) | ||
| def test_raises_error_when_return_annotation_missing_resolver(): | ||
| def test_raises_field_error_when_using_untyped_explicit_resolver(): |
There was a problem hiding this comment.
suggestion (testing): Add a test to cover the documented "known limitation" case where the resolver has the same name as the field.
The PR description mentions the specific case goodbye = strawberry.field(resolver=goodbye), where the resolver in cls.__dict__ is overwritten and we fall back to MissingReturnAnnotationError. Please add a dedicated test for this scenario to guard against regressions: define a type where goodbye is first declared as an unannotated function, then overwritten via goodbye = strawberry.field(resolver=goodbye), and assert that MissingReturnAnnotationError (with the expected message) is raised.
| @pytest.mark.raises_strawberry_exception( | ||
| MissingReturnAnnotationError, | ||
| match='Return annotation missing for field "goodbye", did you forget to add it?', | ||
| MissingFieldAnnotationError, | ||
| match=( | ||
| 'Unable to determine the type of field "goodbye". Either annotate it ' | ||
| "directly, or provide a typed resolver using @strawberry.field." | ||
| ), | ||
| ) | ||
| def test_raises_error_when_return_annotation_missing_resolver(): | ||
| def test_raises_field_error_when_using_untyped_explicit_resolver(): |
There was a problem hiding this comment.
suggestion (testing): Add/keep a dedicated test ensuring the decorator-style resolver without return annotation still raises MissingReturnAnnotationError.
This change now tests the strawberry.field(resolver=fn) path and expects MissingFieldAnnotationError. To also cover the decorator path introduced in _check_field_annotations, please add a test for the @strawberry.field decorator with an untyped resolver and no field annotation, asserting that MissingReturnAnnotationError is raised. If that test already exists, consider co-locating or briefly documenting it to clarify the distinction between the two behaviors.
Suggested implementation:
@pytest.mark.raises_strawberry_exception(
MissingFieldAnnotationError,
match=(
'Unable to determine the type of field "goodbye". Either annotate it '
"directly, or provide a typed resolver using @strawberry.field."
),
)
def test_raises_field_error_when_using_untyped_explicit_resolver():
@strawberry.type
class Query2:
def adios(self):
@pytest.mark.raises_strawberry_exception(
MissingReturnAnnotationError,
match='Return annotation missing for field "hola", did you forget to add it?',
)
def test_raises_error_when_using_untyped_decorator_resolver():
@strawberry.type
class Query:
@strawberry.field
def hola(self):
return "hola"
strawberry.Schema(query=Query)If this file does not already import MissingReturnAnnotationError, add it to the existing imports at the top of tests/fields/test_resolvers.py, alongside the other Strawberry exceptions.
Greptile SummaryThis PR fixes a misleading error type when The fix uses two heuristics in Confidence Score: 5/5Safe to merge; both findings are P2 style/coverage suggestions that do not block correct behavior. The core logic is correct for all described use cases, the known limitation is documented, and the existing test change validates the primary scenario. The two P2 comments (missing test coverage for edge cases, and callable() robustness) are improvements but do not affect correctness on supported use cases. tests/fields/test_resolvers.py could benefit from additional test cases for the second heuristic branch and the known-limitation scenario. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Field has no type annotation\nand has a resolver] --> B{resolver.type_annotation\nis None?}
B -- No --> C[Copy resolver return type\nto cls_annotations]
B -- Yes --> D{resolver_func present\nin cls.__dict__\nunder a different key?}
D -- Yes --> E[raise MissingFieldAnnotationError\nAnnotate the field directly]
D -- No --> F{field_name !=\nresolver.name?}
F -- Yes --> E
F -- No --> G[raise MissingReturnAnnotationError\nAnnotate the resolver]
G -.->|Known limitation| H["goodbye = strawberry.field(resolver=goodbye)\nindistinguishable from @strawberry.field"]
|
| resolver_defined_separately = any( | ||
| v is resolver_func | ||
| for k, v in cls.__dict__.items() | ||
| if k != field_name | ||
| and callable(v) | ||
| and not isinstance(v, StrawberryField) | ||
| ) |
There was a problem hiding this comment.
callable() filter may exclude staticmethod/classmethod wrappers (Python < 3.10)
staticmethod and classmethod objects are only callable starting from Python 3.10. On Python 3.9, if a user passes a staticmethod resolver under a different field name, callable(v) returns False for that entry, so resolver_defined_separately will be False. The fallback field_name != field_.base_resolver.name condition would still rescue the case if the names differ, but it adds fragility. Dropping the callable() guard entirely (the identity check v is resolver_func is already specific enough) would be safer.
| resolver_defined_separately = any( | |
| v is resolver_func | |
| for k, v in cls.__dict__.items() | |
| if k != field_name | |
| and callable(v) | |
| and not isinstance(v, StrawberryField) | |
| ) | |
| resolver_defined_separately = any( | |
| v is resolver_func | |
| for k, v in cls.__dict__.items() | |
| if k != field_name | |
| and not isinstance(v, StrawberryField) | |
| ) |
|
@sourcery-ai review |
|
This PR was published as 0.315.1. Thank you for contributing! |
Description
When using
strawberry.field(resolver=fn)where both the field type annotation and the resolver return type are missing, the error raised isMissingReturnAnnotationError. This is misleading —MissingFieldAnnotationErroris more appropriate since it guides the user to annotate the field directly (e.g.goodbye: int = strawberry.field(resolver=adios)).The fix distinguishes
@strawberry.fielddecorator usage from explicitstrawberry.field(resolver=fn)assignment by comparing the resolver's__qualname__againstf"{cls.__qualname__}.{field_name}". Only the decorator pattern produces a resolver whose qualified name matches the enclosing class and field; every other shape (explicit in-class resolvers, external functions, methods borrowed from another class, module-level functions — including those sharing the field name) correctly raisesMissingFieldAnnotationError.Known limitation
When the resolver is defined inside the class body with the same name as the field and overwrites it (e.g.
goodbye = strawberry.field(resolver=goodbye)), its__qualname__is identical to what the decorator pattern would produce, making the two shapes indistinguishable. In this case,MissingReturnAnnotationErroris raised instead ofMissingFieldAnnotationError. This is still valid guidance — the user can fix it by adding a return type to the resolver.Types of Changes
Issues Fixed or Closed by This PR
Checklist
Summary by Sourcery
Adjust error handling for untyped Strawberry GraphQL fields using explicit resolvers and expand test coverage accordingly.
Bug Fixes:
Documentation:
Tests: