Raise an error for nested Strawberry field metadata (#4595)

Author: patrick91

RELEASE.md

@@ -0,0 +1,39 @@
+---
+release type: patch
+social_messages:
+  x: >-
+    {project_name} {version} is out! This release reports misplaced nested
+    strawberry.field() metadata instead of silently ignoring it.
+    https://strawberry.rocks/release/{version}
+  linkedin: >-
+    {project_name} {version} is out. This release reports misplaced nested
+    strawberry.field() metadata with a clear error instead of silently ignoring it.
+---
+
+This release fixes silently ignored `strawberry.field()` metadata in nested type
+annotations.
+
+Strawberry now raises a clear error when field metadata is placed below the
+class-field annotation, such as on a list item, and explains that it must be moved
+to the outermost `Annotated` metadata for the field.
+
+For example, Strawberry now reports this misplaced metadata:
+
+```python
+from typing import Annotated
+
+import strawberry
+
+
+@strawberry.type
+class Query:
+    names: list[Annotated[str, strawberry.field(description="A name")]]
+```
+
+Move `strawberry.field()` to the field's outermost `Annotated` metadata:
+
+```python
+@strawberry.type
+class Query:
+    names: Annotated[list[str], strawberry.field(description="The names")]
+```

docs/errors/invalid-strawberry-field-annotation.md

@@ -0,0 +1,40 @@
+---
+title: Invalid Strawberry Field Annotation Error
+---
+
+# Invalid Strawberry Field Annotation Error
+
+## Description
+
+This error is raised when `strawberry.field()` is nested inside another type in
+a class-field annotation. For example, this attempts to configure a list item,
+which is not a GraphQL field:
+
+```python
+from typing import Annotated
+
+import strawberry
+
+
+@strawberry.type
+class Query:
+    names: list[Annotated[str, strawberry.field(description="A name")]]
+```
+
+Strawberry only uses `strawberry.field()` metadata from the outermost
+`Annotated` type because that node represents the class field.
+
+## How to fix this error
+
+Move `strawberry.field()` to the outermost `Annotated` metadata for the field:
+
+```python
+from typing import Annotated
+
+import strawberry
+
+
+@strawberry.type
+class Query:
+    names: Annotated[list[str], strawberry.field(description="The names")]
+```

docs/types/object-types.md

@@ -158,6 +158,18 @@ Use only one `strawberry.field()` for each field. You can alternatively use the
 equivalent assignment syntax, such as
 `name: str = strawberry.field(description="The displayed name")`.
 
+`strawberry.field()` must be metadata on the field's outermost `Annotated` type.
+Placing it inside a wrapper configures no GraphQL field, so Strawberry raises an
+error instead of silently ignoring it:
+
+```python
+# Incorrect: strawberry.field() describes the list item, not `names`.
+names: list[Annotated[str, strawberry.field(description="A name")]]
+
+# Correct: strawberry.field() describes `names`.
+names: Annotated[list[str], strawberry.field(description="The names")]
+```
+
 ## API
 
 `@strawberry.type(name: str = None, description: str = None)`

strawberry/annotation.py

@@ -54,9 +54,16 @@
     typing.AsyncIterator,
 )
 
+_NOT_EVALUATED = object()
+
 
 class StrawberryAnnotation:
-    __slots__ = "__resolve_cache__", "namespace", "raw_annotation"
+    __slots__ = (
+        "__evaluated_cache__",
+        "__resolve_cache__",
+        "namespace",
+        "raw_annotation",
+    )
 
     def __init__(
         self,
@@ -67,6 +74,7 @@ def __init__(
         self.raw_annotation = annotation
         self.namespace = namespace
 
+        self.__evaluated_cache__: object = _NOT_EVALUATED
         self.__resolve_cache__: StrawberryType | type | None = None
 
     def __eq__(self, other: object) -> bool:
@@ -93,7 +101,7 @@ def from_annotation(
     def annotation(self) -> object | str:
         """Return evaluated type on success or fallback to raw (string) annotation."""
         try:
-            return self.evaluate()
+            return self._evaluated_annotation
         except NameError:
             # Evaluation failures can happen when importing types within a TYPE_CHECKING
             # block or if the type is declared later on in a module.
@@ -103,6 +111,7 @@ def annotation(self) -> object | str:
     def annotation(self, value: object | str) -> None:
         self.raw_annotation = value
 
+        self.__evaluated_cache__ = _NOT_EVALUATED
         self.__resolve_cache__ = None
 
     def evaluate(self) -> type:
@@ -114,6 +123,13 @@ def evaluate(self) -> type:
 
         return eval_type(annotation, self.namespace, None)
 
+    @property
+    def _evaluated_annotation(self) -> object:
+        if self.__evaluated_cache__ is _NOT_EVALUATED:
+            self.__evaluated_cache__ = self.evaluate()
+
+        return self.__evaluated_cache__
+
     def _get_type_with_args(
         self, evaled_type: type[Any]
     ) -> tuple[type[Any], list[Any]]:
@@ -161,7 +177,7 @@ def resolve(
         return resolved
 
     def _resolve(self) -> StrawberryType | type:
-        evaled_type = cast("Any", self.evaluate())
+        evaled_type = cast("Any", self._evaluated_annotation)
         return self._resolve_evaled_type(evaled_type)
 
     def _resolve_evaled_type(self, evaled_type: Any) -> StrawberryType | type:
@@ -208,6 +224,7 @@ def set_namespace_from_field(self, field: StrawberryField) -> None:
         module = sys.modules[field.origin.__module__]
         self.namespace = module.__dict__
 
+        self.__evaluated_cache__ = _NOT_EVALUATED
         self.__resolve_cache__ = None  # Invalidate cache to allow re-evaluation
 
     def create_concrete_type(self, evaled_type: type) -> type:

strawberry/exceptions/__init__.py

@@ -10,6 +10,7 @@
 from .exception import StrawberryException, UnableToFindExceptionSource
 from .handler import setup_exception_handler
 from .invalid_argument_type import InvalidArgumentTypeError
+from .invalid_strawberry_field_annotation import InvalidStrawberryFieldAnnotationError
 from .invalid_superclass_interface import InvalidSuperclassInterfaceError
 from .invalid_union_type import InvalidTypeForUnionMergeError, InvalidUnionTypeError
 from .missing_arguments_annotations import MissingArgumentsAnnotationsError
@@ -176,6 +177,7 @@ def __init__(self, payload: dict[str, object] | None = None) -> None:
     "InvalidArgumentTypeError",
     "InvalidCustomContext",
     "InvalidDefaultFactoryError",
+    "InvalidStrawberryFieldAnnotationError",
     "InvalidSuperclassInterfaceError",
     "InvalidTypeForUnionMergeError",
     "InvalidUnionTypeError",

strawberry/exceptions/invalid_strawberry_field_annotation.py

@@ -0,0 +1,38 @@
+from __future__ import annotations
+
+from functools import cached_property
+from typing import TYPE_CHECKING
+
+from .exception import StrawberryException
+from .utils.source_finder import SourceFinder
+
+if TYPE_CHECKING:
+    from .exception_source import ExceptionSource
+
+
+class InvalidStrawberryFieldAnnotationError(StrawberryException):
+    def __init__(self, field_name: str, cls: type) -> None:
+        self.cls = cls
+        self.field_name = field_name
+
+        self.message = (
+            f"`strawberry.field()` for field `{field_name}` on type `{cls.__name__}` "
+            "must be placed at the top level of the field annotation"
+        )
+        self.rich_message = (
+            f"`strawberry.field()` for field `[underline]{field_name}[/]` on type "
+            f"`[underline]{cls.__name__}[/]` cannot be nested inside another type"
+        )
+        self.annotation_message = "strawberry.field() is nested inside another type"
+        self.suggestion = (
+            "To fix this error, move `strawberry.field()` to the outermost "
+            "`Annotated` metadata for the field."
+        )
+
+        super().__init__(self.message)
+
+    @cached_property
+    def exception_source(self) -> ExceptionSource | None:
+        source_finder = SourceFinder()
+
+        return source_finder.find_class_attribute_from_object(self.cls, self.field_name)

strawberry/types/field.py

@@ -8,16 +8,23 @@
 from functools import cached_property
 from typing import (
     TYPE_CHECKING,
+    Annotated,
     Any,
     NoReturn,
     TypeAlias,
     TypeVar,
     Union,
+    get_args,
+    get_origin,
     overload,
 )
 
 from strawberry.annotation import StrawberryAnnotation
-from strawberry.exceptions import InvalidArgumentTypeError, InvalidDefaultFactoryError
+from strawberry.exceptions import (
+    InvalidArgumentTypeError,
+    InvalidDefaultFactoryError,
+    InvalidStrawberryFieldAnnotationError,
+)
 from strawberry.types.base import (
     StrawberryType,
     WithStrawberryObjectDefinition,
@@ -124,6 +131,9 @@ def __init__(  # noqa: PLR0917
             self.python_name = python_name
 
         self.type_annotation = type_annotation
+        # Cache the exact annotation object after successful validation and
+        # resolution so replacing `type_annotation` automatically revalidates it.
+        self._validated_type_annotation: StrawberryAnnotation | None = None
 
         self.description: str | None = description
         self.origin = origin
@@ -345,7 +355,14 @@ def resolve_type(
         with contextlib.suppress(NameError):
             # Prioritise the field type over the resolver return type
             if self.type_annotation is not None:
-                resolved = self.type_annotation.resolve(type_definition=type_definition)
+                type_annotation = self.type_annotation
+                if type_annotation is not self._validated_type_annotation:
+                    self._validate_type_annotation(
+                        type_annotation._evaluated_annotation
+                    )
+
+                resolved = type_annotation.resolve(type_definition=type_definition)
+                self._validated_type_annotation = type_annotation
             elif self.base_resolver is not None and self.base_resolver.type is not None:
                 # Handle unannotated functions (such as lambdas)
                 # Generics will raise MissingTypesForGenericError later
@@ -355,6 +372,17 @@ def resolve_type(
 
         return resolved
 
+    def _validate_type_annotation(self, annotation: object) -> None:
+        if (
+            self.python_name is not None
+            and isinstance(self.origin, type)
+            and _contains_strawberry_field(annotation)
+        ):
+            raise InvalidStrawberryFieldAnnotationError(
+                field_name=self.python_name,
+                cls=self.origin,
+            )
+
     def copy_with(
         self, type_var_map: Mapping[str, StrawberryType | builtins.type]
     ) -> Self:
@@ -395,6 +423,29 @@ def is_async(self) -> bool:
         return self._has_async_base_resolver
 
 
+def _contains_strawberry_field(
+    annotation: object,
+    *,
+    at_field_annotation_root: bool = True,
+) -> bool:
+    if get_origin(annotation) is Annotated:
+        annotation, *metadata = get_args(annotation)
+        if not at_field_annotation_root and any(
+            isinstance(item, StrawberryField) for item in metadata
+        ):
+            return True
+
+        return _contains_strawberry_field(
+            annotation,
+            at_field_annotation_root=at_field_annotation_root,
+        )
+
+    return any(
+        _contains_strawberry_field(arg, at_field_annotation_root=False)
+        for arg in get_args(annotation)
+    )
+
+
 # NOTE: we are separating the sync and async resolvers because using both
 # in the same function will cause mypy to raise an error. Not sure if it is a bug
 

strawberry/types/fields/resolver.py

@@ -120,13 +120,13 @@ def find(
             annotation = resolver.strawberry_annotations[parameter]
             if isinstance(annotation, StrawberryAnnotation):
                 try:
-                    evaled_annotation: Any = annotation.evaluate()
+                    evaled_annotation: Any = annotation._evaluated_annotation
                 except NameError:
                     # If we fail to evaluate, check if the raw annotation string
                     # matches this reserved type. This handles cases where types
                     # are imported under TYPE_CHECKING or use forward references
                     # like "strawberry.Info".
-                    raw = annotation.annotation
+                    raw = annotation.raw_annotation
                     if isinstance(raw, str):
                         evaled_annotation = raw
                     else:

strawberry/types/object_type.py

@@ -24,6 +24,7 @@
 
 from strawberry.annotation import StrawberryAnnotation
 from strawberry.exceptions import (
+    InvalidStrawberryFieldAnnotationError,
     InvalidSuperclassInterfaceError,
     MissingFieldAnnotationError,
     MissingReturnAnnotationError,
@@ -36,7 +37,7 @@
 from strawberry.utils.str_converters import to_camel_case
 
 from .base import StrawberryObjectDefinition
-from .field import StrawberryField, field
+from .field import StrawberryField, _contains_strawberry_field, field
 from .type_resolver import _get_fields
 
 T = TypeVar("T", bound=builtins.type)
@@ -70,10 +71,12 @@ def _process_annotated_fields(cls: T) -> dict[str, StrawberryAnnotation]:
         else:
             annotation = raw_annotation
 
-        if get_origin(annotation) is not Annotated:
-            continue
+        if get_origin(annotation) is Annotated:
+            first, *rest = get_args(annotation)
+        else:
+            first = annotation
+            rest = []
 
-        first, *rest = get_args(annotation)
         strawberry_fields = [arg for arg in rest if isinstance(arg, StrawberryField)]
 
         if len(strawberry_fields) > 1 or (
@@ -82,6 +85,12 @@ def _process_annotated_fields(cls: T) -> dict[str, StrawberryAnnotation]:
         ):
             raise MultipleStrawberryFieldsError(field_name=field_name, cls=cls)
 
+        if _contains_strawberry_field(annotation):
+            raise InvalidStrawberryFieldAnnotationError(
+                field_name=field_name,
+                cls=cls,
+            )
+
         if not strawberry_fields:
             continue