diff --git a/src/sentry/api/endpoints/organization_releases.py b/src/sentry/api/endpoints/organization_releases.py index a9299165c05d..8e12b6fb81e2 100644 --- a/src/sentry/api/endpoints/organization_releases.py +++ b/src/sentry/api/endpoints/organization_releases.py @@ -230,6 +230,14 @@ def _filter_releases_by_query( return queryset +class _ProjectsErrorResponse(TypedDict): + projects: list[str] + + +class _RefsErrorResponse(TypedDict): + refs: list[str] + + class ReleaseSerializerWithProjects(ReleaseWithVersionSerializer): projects = ListField( child=CharField(), @@ -804,7 +812,12 @@ def qs_load_func( ) def post( self, request: Request, organization: Organization - ) -> Response[ReleaseSerializerResponse] | Response[ValidationErrorResponse]: + ) -> ( + Response[ReleaseSerializerResponse] + | Response[_ProjectsErrorResponse] + | Response[_RefsErrorResponse] + | Response[ValidationErrorResponse] + ): """ Create a new release for the given organization. Releases are used by Sentry to improve error reporting by correlating first-seen events with the release that may diff --git a/src/sentry/apidocs/response_types.py b/src/sentry/apidocs/response_types.py index 48736e24f6f6..f428f192bca3 100644 --- a/src/sentry/apidocs/response_types.py +++ b/src/sentry/apidocs/response_types.py @@ -37,6 +37,16 @@ class DetailResponse(TypedDict): detail: str +class NonFieldErrorsResponse(TypedDict): + """DRF's error-body shape for errors not tied to one field: + `{"non_field_errors": ["..."]}`. + + Use for inline `Response({"non_field_errors": [...]}, status=4xx)` returns. + """ + + non_field_errors: list[str] + + ValidationErrorResponse: TypeAlias = dict[str, Any] """DRF's validation-error body shape: `{field_name: , ...}`. @@ -56,6 +66,10 @@ class DetailResponse(TypedDict): def post(...) -> Response[FooResponse] | Response[ValidationErrorResponse]: and produce the body via `as_validation_errors(serializer)` below. + +When the union also has TypedDict arms, the Sentry mypy plugin rejects a dict +literal body that matches none of them, even though this arm would accept it. +Declare a TypedDict for the shape of an inline literal instead. """ diff --git a/src/sentry/core/endpoints/organization_member_details.py b/src/sentry/core/endpoints/organization_member_details.py index 519bb7144515..df3f38dd1329 100644 --- a/src/sentry/core/endpoints/organization_member_details.py +++ b/src/sentry/core/endpoints/organization_member_details.py @@ -1,6 +1,6 @@ from __future__ import annotations -from typing import Literal +from typing import Literal, TypedDict from django.db import router, transaction from django.db.models import Q @@ -122,6 +122,10 @@ class UpdateOrgMemberRolesSerializer(OrganizationMemberRequestSerializer): ) +class _RoleErrorResponse(TypedDict): + role: str + + @extend_schema(tags=["Organizations"]) @cell_silo_endpoint class OrganizationMemberDetailsEndpoint(OrganizationMemberEndpoint): @@ -207,6 +211,7 @@ def put( Response[OrganizationMemberResponse] | Response[None] | Response[DetailResponse] + | Response[_RoleErrorResponse] | Response[ValidationErrorResponse] ): """ diff --git a/src/sentry/core/endpoints/organization_member_index.py b/src/sentry/core/endpoints/organization_member_index.py index e657dfda1cb0..7307de3da354 100644 --- a/src/sentry/core/endpoints/organization_member_index.py +++ b/src/sentry/core/endpoints/organization_member_index.py @@ -1,4 +1,4 @@ -from typing import Any +from typing import Any, TypedDict from django.conf import settings from django.db import router, transaction @@ -173,6 +173,14 @@ def validate_teamRoles(self, teamRoles: list[dict[str, Any]]) -> list[tuple[Team return valid_team_roles +class _OrganizationErrorResponse(TypedDict): + organization: str + + +class _EmailErrorResponse(TypedDict): + email: str + + @cell_silo_endpoint @extend_schema(tags=["Organizations"]) class OrganizationMemberIndexEndpoint(OrganizationEndpoint): @@ -349,6 +357,8 @@ def post( ) -> ( Response[OrganizationMemberResponse] | Response[DetailResponse] + | Response[_OrganizationErrorResponse] + | Response[_EmailErrorResponse] | Response[ValidationErrorResponse] ): """ diff --git a/src/sentry/core/endpoints/organization_teams.py b/src/sentry/core/endpoints/organization_teams.py index 20d4581a711d..664eb657c80c 100644 --- a/src/sentry/core/endpoints/organization_teams.py +++ b/src/sentry/core/endpoints/organization_teams.py @@ -1,3 +1,5 @@ +from typing import TypedDict + from django.db import IntegrityError, router, transaction from django.db.models import Q from drf_spectacular.utils import OpenApiResponse, extend_schema @@ -39,6 +41,11 @@ CONFLICTING_SLUG_ERROR = "A team with this slug already exists." +class _ConflictingSlugResponse(TypedDict): + non_field_errors: list[str] + detail: str + + # OrganizationPermission + team:write class OrganizationTeamsPermission(OrganizationPermission): scope_map = { @@ -198,7 +205,11 @@ def should_add_creator_to_team(self, request: Request) -> bool: ) def post( self, request: Request, organization: Organization, **kwargs: object - ) -> Response[TeamSerializerResponse] | Response[ValidationErrorResponse]: + ) -> ( + Response[TeamSerializerResponse] + | Response[_ConflictingSlugResponse] + | Response[ValidationErrorResponse] + ): """ Create a new team bound to an organization. Requires at least one of the `name` or `slug` body params to be set. diff --git a/src/sentry/core/endpoints/project_team_details.py b/src/sentry/core/endpoints/project_team_details.py index 06ef15c59c8e..daf6ad65eef2 100644 --- a/src/sentry/core/endpoints/project_team_details.py +++ b/src/sentry/core/endpoints/project_team_details.py @@ -1,4 +1,4 @@ -from typing import cast +from typing import TypedDict, cast from drf_spectacular.utils import extend_schema from rest_framework.request import Request @@ -18,11 +18,14 @@ from sentry.apidocs.constants import RESPONSE_FORBIDDEN, RESPONSE_NOT_FOUND from sentry.apidocs.examples.project_examples import ProjectExamples from sentry.apidocs.parameters import GlobalParams -from sentry.apidocs.response_types import ValidationErrorResponse from sentry.models.project import Project from sentry.models.team import Team +class _DetailListResponse(TypedDict): + detail: list[str] + + class ProjectTeamsPermission(ProjectPermission): scope_map = { "POST": ["project:write", "project:admin"], @@ -123,7 +126,7 @@ def post( ) def delete( self, request: Request, project: Project, team: Team - ) -> Response[ProjectWithTeamResponseDict] | Response[ValidationErrorResponse]: + ) -> Response[ProjectWithTeamResponseDict] | Response[_DetailListResponse]: """ Revoke a team's access to a project. diff --git a/src/sentry/issues/endpoints/group_integration_details.py b/src/sentry/issues/endpoints/group_integration_details.py index 636f8e03ac61..dfeb278dc966 100644 --- a/src/sentry/issues/endpoints/group_integration_details.py +++ b/src/sentry/issues/endpoints/group_integration_details.py @@ -28,7 +28,11 @@ from sentry.apidocs.descriptions import ApiDescription from sentry.apidocs.examples.integration_examples import IntegrationExamples from sentry.apidocs.parameters import GlobalParams, IssueParams -from sentry.apidocs.response_types import DetailResponse, ValidationErrorResponse +from sentry.apidocs.response_types import ( + DetailResponse, + NonFieldErrorsResponse, + ValidationErrorResponse, +) from sentry.apidocs.utils import inline_sentry_response_serializer from sentry.auth.principal import acting_user_id from sentry.constants import CELL_API_DEPRECATION_DATE @@ -122,6 +126,10 @@ class ExternalIssueLinkResponse(TypedDict): ] +class _ExternalIssueErrorResponse(TypedDict): + externalIssue: list[str] + + class IntegrationIssueConfigSerializer(Serializer[IntegrationIssueConfigResponse]): def __init__(self, action: str, config: list[IntegrationIssueConfigField]) -> None: self.action = action @@ -291,6 +299,7 @@ def post( ) -> ( Response[ExternalIssueLinkResponse] | Response[DetailResponse] + | Response[NonFieldErrorsResponse] | Response[ValidationErrorResponse] ): """ @@ -463,6 +472,8 @@ def put( ) -> ( Response[ExternalIssueLinkResponse] | Response[DetailResponse] + | Response[NonFieldErrorsResponse] + | Response[_ExternalIssueErrorResponse] | Response[ValidationErrorResponse] ): """ diff --git a/src/sentry/releases/endpoints/organization_release_details.py b/src/sentry/releases/endpoints/organization_release_details.py index af156757a4bb..33dac0abb918 100644 --- a/src/sentry/releases/endpoints/organization_release_details.py +++ b/src/sentry/releases/endpoints/organization_release_details.py @@ -84,6 +84,10 @@ class _ReleaseDateQueryAndOrder(TypedDict): order_by: list[str] +class _RefsErrorResponse(TypedDict): + refs: list[str] + + def add_status_filter_to_queryset( queryset: QuerySet[Release], status_filter: str ) -> QuerySet[Release]: @@ -521,7 +525,11 @@ def get( ) def put( self, request: Request, organization: Organization, version: str - ) -> Response[ReleaseSerializerResponse] | Response[ValidationErrorResponse]: + ) -> ( + Response[ReleaseSerializerResponse] + | Response[_RefsErrorResponse] + | Response[ValidationErrorResponse] + ): """ Update a release. This can change some metadata associated with diff --git a/src/sentry/sentry_apps/api/endpoints/sentry_app_details.py b/src/sentry/sentry_apps/api/endpoints/sentry_app_details.py index 82efbd340d02..8b2923a93acb 100644 --- a/src/sentry/sentry_apps/api/endpoints/sentry_app_details.py +++ b/src/sentry/sentry_apps/api/endpoints/sentry_app_details.py @@ -23,6 +23,7 @@ from sentry.apidocs.parameters import SentryAppParams from sentry.apidocs.response_types import ( DetailResponse, + NonFieldErrorsResponse, ValidationErrorResponse, as_validation_errors, ) @@ -125,6 +126,7 @@ def put( ) -> ( Response[SentryAppSerializerResponse] | Response[DetailResponse] + | Response[NonFieldErrorsResponse] | Response[ValidationErrorResponse] ): """ diff --git a/tests/tools/mypy_helpers/test_plugin.py b/tests/tools/mypy_helpers/test_plugin.py index 94d91a603aff..6d0caeb00ab9 100644 --- a/tests/tools/mypy_helpers/test_plugin.py +++ b/tests/tools/mypy_helpers/test_plugin.py @@ -787,6 +787,123 @@ def view() -> Response[FooResponse] | Response[StatsPeriodErrorResponse]: assert ret == 0, out +_LOOSE_ARM_SRC = """\ +from typing import Any, Final, Literal, NotRequired, TypedDict +from rest_framework.response import Response + +RUN_IN_FLIGHT: Final = "run_in_flight" + +class ConflictResponse(TypedDict): + detail: str + code: NotRequired[Literal["run_in_flight", "repos_not_linked"]] + +class DetailResponse(TypedDict): + detail: str + +def view() -> ( + Response[ConflictResponse] | Response[DetailResponse] | Response[dict[str, Any]] +): + return Response({BODY}, status=409) +""" + + +def test_response_union_loose_arm_rejects_wrong_literal_value() -> None: + """A `dict[str, Any]` arm (e.g. `ValidationErrorResponse`) accepts any + dict literal, so a body with a `code` outside its declared `Literal` used + to type-check through it. It must fail against the TypedDict arms.""" + src = _LOOSE_ARM_SRC.replace("{BODY}", '{"detail": "x", "code": "bogus"}') + ret, out = call_mypy(src) + assert ret, out + assert "[response-body]" in out + assert '"ConflictResponse", "DetailResponse"' in out + + +def test_response_union_loose_arm_rejects_typo_key() -> None: + src = _LOOSE_ARM_SRC.replace("{BODY}", '{"detial": "x"}') + ret, out = call_mypy(src) + assert ret, out + assert "[response-body]" in out + + +def test_response_union_loose_arm_accepts_matching_literals() -> None: + for body in ( + '{"detail": "x", "code": "run_in_flight"}', + '{"detail": "x", "code": RUN_IN_FLIGHT}', + '{"detail": "x"}', + ): + ret, out = call_mypy(_LOOSE_ARM_SRC.replace("{BODY}", body)) + assert ret == 0, (body, out) + + +def test_response_union_loose_arm_accepts_annotated_variable() -> None: + """Bodies that are deliberately loose opt out by not being a literal.""" + src = """\ +from typing import Any, TypedDict +from rest_framework.response import Response + +class DetailResponse(TypedDict): + detail: str + +def view() -> Response[DetailResponse] | Response[dict[str, Any]]: + errors: dict[str, Any] = {"field": ["This field is required."]} + return Response(errors, status=400) +""" + ret, out = call_mypy(src) + assert ret == 0, out + + +def test_response_union_nested_dict_literal_matches_nested_typeddict() -> None: + """Nested dict literals are inferred against the nested TypedDict field, + so a correct nested body matches its arm instead of being reported.""" + src = """\ +from typing import Any, TypedDict +from rest_framework.response import Response + +class CodedErrorDetail(TypedDict): + message: str + code: str + +class CodedErrorResponse(TypedDict): + detail: CodedErrorDetail + +class DetailResponse(TypedDict): + detail: str + +def view(flag: bool) -> ( + Response[CodedErrorResponse] | Response[DetailResponse] | Response[dict[str, Any]] +): + if flag: + return Response({"detail": {"message": "m", "code": "c"}}, status=400) + return Response({"detail": {"message": "m", "cod": "c"}}, status=400) +""" + ret, out = call_mypy(src) + assert ret, out + assert out.count("[response-body]") == 1, out + assert ":19:" in out, out + + +def test_response_union_no_loose_arm_reports_once() -> None: + """Without a loose arm mypy already reports the mismatch, so the plugin + adds no second error.""" + src = """\ +from typing import TypedDict +from rest_framework.response import Response + +class FooResponse(TypedDict): + x: int + +class DetailResponse(TypedDict): + detail: str + +def view() -> Response[FooResponse] | Response[DetailResponse]: + return Response({"wrong": "shape"}, status=400) +""" + ret, out = call_mypy(src) + assert ret, out + assert "Incompatible return value type" in out + assert "[response-body]" not in out + + # -- Serializer auto-derive ---------------------------------------------------- diff --git a/tools/mypy_helpers/plugin.py b/tools/mypy_helpers/plugin.py index 3ec24f0a1e7f..03c462fa17d7 100644 --- a/tools/mypy_helpers/plugin.py +++ b/tools/mypy_helpers/plugin.py @@ -5,10 +5,10 @@ from typing import Any from mypy.build import PRI_MYPY -from mypy.errorcodes import ATTR_DEFINED +from mypy.errorcodes import ATTR_DEFINED, ErrorCode from mypy.lookup import lookup_fully_qualified from mypy.messages import format_type -from mypy.nodes import ARG_POS, DictExpr, ListExpr, MypyFile, StrExpr, TypeInfo +from mypy.nodes import ARG_POS, DictExpr, Expression, ListExpr, MypyFile, StrExpr, TypeInfo from mypy.plugin import ( AttributeContext, ClassDefContext, @@ -328,26 +328,30 @@ def _dict_literal_matches_typeddict( mypy's `check_typeddict_call_with_dict` because that method emits errors directly on the call site — the plugin needs to probe silently across union arms. + + Each value is inferred against its declared field type, so nested dict + literals can satisfy nested TypedDict fields. Errors and inferred types + from that probe are discarded, the way mypy probes overloads. """ - literal_keys: set[str] = set() - literal_items: dict[str, Type] = {} + items: list[tuple[str, Expression]] = [] for k_expr, v_expr in body.items: if not isinstance(k_expr, StrExpr): return False - literal_keys.add(k_expr.value) - try: - literal_items[k_expr.value] = expr_checker.accept(v_expr) - except Exception: - return False + items.append((k_expr.value, v_expr)) + literal_keys = {key for key, _ in items} if not td.required_keys.issubset(literal_keys): return False if literal_keys - set(td.items.keys()): return False - for key, value_type in literal_items.items(): - declared = td.items.get(key) - if declared is None: + chk = expr_checker.chk + for key, v_expr in items: + declared = td.items[key] + try: + with chk.msg.filter_errors() as watcher, chk.local_type_map: + value_type = expr_checker.accept(v_expr, type_context=declared) + except Exception: return False - if not _is_subtype(value_type, declared): + if watcher.has_new_errors() or not _is_subtype(value_type, declared): return False return True @@ -378,6 +382,50 @@ def _empty_list_matches_arm(arm_T: Type) -> bool: return False +_RESPONSE_BODY = ErrorCode( + "response-body", + "Check that Response() dict-literal bodies match a declared TypedDict arm", + "General", +) + + +def _check_dict_literal_not_caught_by_loose_arm( + ctx: FunctionContext, + response_arms: list[Instance], + typeddict_arms: list[TypedDictType], +) -> None: + """Error when a dict-literal body matches none of the TypedDict arms but + still type-checks through a loose arm such as `Response[dict[str, Any]]`. + + Without this, a loose arm (e.g. `ValidationErrorResponse`) accepts every + dict literal, so a body with a typo'd key or a wrong `Literal` value + silently passes even though the endpoint declares the intended shape. When + no loose arm accepts the literal, mypy already reports an incompatible + return, so stay quiet to avoid a duplicate error. + """ + inferred = get_proper_type(ctx.default_return_type) + loose_arm = next( + ( + inst + for inst in response_arms + if not isinstance(get_proper_type(inst.args[0]), TypedDictType) + and _is_subtype(inferred, inst) + ), + None, + ) + if loose_arm is None: + return + names = ", ".join(sorted({format_type(td, ctx.api.options) for td in typeddict_arms})) + ctx.api.fail( + f"Dict literal body matches none of the declared TypedDict arms ({names}) " + f"and only type-checks through {format_type(loose_arm, ctx.api.options)}. " + "Fix the body, declare a TypedDict arm for its shape, or assign it to a " + "variable annotated with the loose type.", + ctx.context, + code=_RESPONSE_BODY, + ) + + def _narrow_response_literal_in_union(ctx: FunctionContext) -> Type: """Narrow `Response(, ...)` to a matching arm of the enclosing function's union return type. @@ -395,8 +443,10 @@ def _narrow_response_literal_in_union(ctx: FunctionContext) -> Type: - empty `ListExpr` `[]` → matches `list[T]` arms Returns `Response[that_T]` when exactly one arm accepts the literal. Zero - or multiple matches → returns default (mypy errors). Non-literal bodies - are untouched. Name-agnostic: no hardcoded TypedDict names. + or multiple matches → returns default (mypy errors). A non-empty dict + literal that matches no TypedDict arm is also reported when a loose arm + would accept it. Non-literal bodies are untouched. Name-agnostic: no + hardcoded TypedDict names. """ if not ctx.args or not ctx.args[0]: return ctx.default_return_type @@ -426,10 +476,12 @@ def _narrow_response_literal_in_union(ctx: FunctionContext) -> Type: expr_checker = ctx.api.expr_checker # type: ignore[attr-defined] matching: list[Instance] = [] + typeddict_arms: list[TypedDictType] = [] for inst in response_arms: T_arg = get_proper_type(inst.args[0]) if is_nonempty_dict and isinstance(T_arg, TypedDictType): assert isinstance(body_expr, DictExpr) + typeddict_arms.append(T_arg) if _dict_literal_matches_typeddict(body_expr, T_arg, expr_checker): matching.append(inst) elif is_empty_dict and _empty_dict_matches_arm(T_arg): @@ -437,6 +489,8 @@ def _narrow_response_literal_in_union(ctx: FunctionContext) -> Type: elif is_empty_list and _empty_list_matches_arm(T_arg): matching.append(inst) + if not matching and typeddict_arms: + _check_dict_literal_not_caught_by_loose_arm(ctx, response_arms, typeddict_arms) if len(matching) != 1: # Zero matches (real drift) or multiple (ambiguous) — fall back to # default and let mypy emit its standard error.