Skip to content

Commit 9cbb88f

Browse files
azulusclaude
andauthored
ref: Lint PUBLIC endpoints for typed return annotations (#117356)
Adds a check to the apidocs response-annotation linter: any HTTP method declared `ApiPublishStatus.PUBLIC` in `publish_status` must annotate its return with `Response[T]`, a union of `Response[T_i]` arms (optionally mixed with non-`Response` Django response types), or a concrete non-DRF type. Bare `-> Response` and missing annotations are errors. PUBLIC endpoints are documented in OpenAPI and consumed by SDK generators; an untyped body lets the runtime drift from the declared schema. The diagnostic includes a copy-able fix template and a pointer to `sentry.apidocs.response_types` for standard error shapes. --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
1 parent 70f51f4 commit 9cbb88f

2 files changed

Lines changed: 537 additions & 10 deletions

File tree

src/sentry/apidocs/_check_response_annotation_matches_schema.py

Lines changed: 225 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,8 @@
1-
"""Lint: assert decorator's declared `T`s match the method's `Response[T]` annotation.
1+
"""Lint: enforce typed response annotations on PUBLIC endpoints.
2+
3+
This module runs two checks against every HTTP method in an endpoint class.
4+
5+
## Check 1 — decorator `T` ⊆ annotation `T`s (drift)
26
37
For each method decorated with
48
@extend_schema(responses={N: inline_sentry_response_serializer("Name", T_decl), ...})
@@ -8,16 +12,16 @@
812
-> Response[T_success] | Response[T_error_400] | ...
913
1014
this linter asserts that the *set* of `T`s declared by
11-
`inline_sentry_response_serializer(...)` entries in the decorator equals the set of
12-
`T`s in the annotation's union arms. Names are compared verbatim — no
15+
`inline_sentry_response_serializer(...)` entries in the decorator is a subset of
16+
the `T`s in the annotation's union arms. Names are compared verbatim — no
1317
normalization, no convention-based pairing. mypy enforces body-vs-annotation;
1418
this linter enforces decorator-vs-annotation.
1519
1620
The status-code-to-`T` linkage is intentionally not enforced — mypy can't model
1721
it, and broad `except APIException` catches would lose the linkage anyway.
1822
1923
Skipped silently (no diagnostic):
20-
- Plain `-> Response` annotations (the unmigrated state).
24+
- Plain `-> Response` annotations (handled by Check 2 for PUBLIC).
2125
- Methods with no `@extend_schema` decorator.
2226
- Decorator entries that aren't `inline_sentry_response_serializer(...)` —
2327
direct serializer-class references (`MonitorSerializer`),
@@ -27,10 +31,30 @@
2731
`inline_sentry_response_serializer(...)` or wait for the generic
2832
`Serializer[T]` refactor.
2933
34+
## Check 2 — PUBLIC endpoints must have typed return annotations
35+
36+
For each HTTP method on an endpoint class whose `publish_status` declares the
37+
method as `ApiPublishStatus.PUBLIC`, the return annotation must be one of:
38+
- `Response[T]` for a concrete TypedDict / type `T`
39+
- a `|`-union of `Response[T_i]` arms (and optionally non-`Response` arms
40+
for endpoints that legitimately return Django responses, e.g.
41+
`Response[Foo] | StreamingHttpResponse`)
42+
- a non-`Response` Django response type if the endpoint never returns a
43+
DRF `Response` (e.g. `HttpResponseBase`, `StreamingHttpResponse`,
44+
`FileResponse`)
45+
46+
The following are diagnostics:
47+
- **missing annotation** — PUBLIC methods must explicitly declare what
48+
they return so mypy can verify the body and drf-spectacular can
49+
document it.
50+
- **bare `Response`** — `-> Response` without `[T]` opts the method out of
51+
type checking entirely. PUBLIC endpoints are documented in OpenAPI; an
52+
untyped body lets the runtime drift from the declared schema.
53+
3054
Invoke as:
3155
python -m sentry.apidocs._check_response_annotation_matches_schema [paths...]
3256
33-
Exits non-zero on any mismatch.
57+
Exits non-zero on any diagnostic.
3458
"""
3559

3660
from __future__ import annotations
@@ -55,6 +79,22 @@
5579
)
5680

5781

82+
_HTTP_METHODS = frozenset({"get", "post", "put", "patch", "delete", "head", "options"})
83+
84+
_PUBLIC_HOW_TO = (
85+
"Fix by replacing the annotation with one of:\n"
86+
" -> Response[YourResponseTypedDict] # single 200 shape\n"
87+
" -> Response[YourResponseTypedDict] | Response[DetailResponse]\n"
88+
" # success + error shape\n"
89+
" -> Response[None] # explicit empty body\n"
90+
" -> HttpResponseBase # streaming/redirect/non-DRF\n"
91+
"If the body shape isn't documented anywhere yet, define a local\n"
92+
"TypedDict in the endpoint file (`class _FooResponse(TypedDict): ...`)\n"
93+
"and reference it. Standard error shapes (`{'detail': str}`,\n"
94+
"`{field: errors}`) live in `sentry.apidocs.response_types`."
95+
)
96+
97+
5898
@dataclass(frozen=True)
5999
class Mismatch:
60100
path: Path
@@ -75,6 +115,31 @@ def __str__(self) -> str:
75115
)
76116

77117

118+
@dataclass(frozen=True)
119+
class PublicUntyped:
120+
"""A PUBLIC HTTP method that lacks a usable return annotation."""
121+
122+
path: Path
123+
line: int
124+
cls: str
125+
method: str
126+
reason: str # "missing" or "bare-Response"
127+
128+
def __str__(self) -> str:
129+
if self.reason == "missing":
130+
problem = "is PUBLIC but has no return annotation"
131+
else:
132+
problem = "is PUBLIC but annotated with bare `Response` (no `[T]`)"
133+
return (
134+
f"{self.path}:{self.line} {self.cls}.{self.method}: {problem}.\n"
135+
"Why: PUBLIC endpoints are documented in the OpenAPI spec and\n"
136+
"consumed by SDK generators and external clients. An untyped\n"
137+
"return lets the runtime body drift from the declared schema\n"
138+
"without anyone noticing (mypy can't verify what isn't typed).\n"
139+
f"{_PUBLIC_HOW_TO}"
140+
)
141+
142+
78143
def _name_of(node: ast.expr) -> str:
79144
"""Render `Foo`, `mod.Foo`, `Foo[T]` as a stable string for equality."""
80145
if isinstance(node, ast.Name):
@@ -188,6 +253,142 @@ def _iter_methods(tree: ast.Module) -> Iterator[tuple[str, ast.FunctionDef | ast
188253
yield node.name, item
189254

190255

256+
def _iter_classes(tree: ast.Module) -> Iterator[ast.ClassDef]:
257+
for node in tree.body:
258+
if isinstance(node, ast.ClassDef):
259+
yield node
260+
261+
262+
def _publish_status(cls: ast.ClassDef) -> dict[str, str]:
263+
"""Return `{HTTP_METHOD: ApiPublishStatus_attr_name}` parsed from
264+
`publish_status = {"GET": ApiPublishStatus.PUBLIC, ...}` on the class
265+
body. Also handles the annotated form
266+
`publish_status: dict[str, ApiPublishStatus] = {...}`. Returns `{}`
267+
when the class has no such assignment or the value doesn't statically
268+
match the expected literal-dict shape."""
269+
for item in cls.body:
270+
value: ast.expr | None = None
271+
if (
272+
isinstance(item, ast.Assign)
273+
and len(item.targets) == 1
274+
and isinstance(item.targets[0], ast.Name)
275+
and item.targets[0].id == "publish_status"
276+
):
277+
value = item.value
278+
elif (
279+
isinstance(item, ast.AnnAssign)
280+
and isinstance(item.target, ast.Name)
281+
and item.target.id == "publish_status"
282+
and item.value is not None
283+
):
284+
value = item.value
285+
if not isinstance(value, ast.Dict):
286+
continue
287+
out: dict[str, str] = {}
288+
for k, v in zip(value.keys, value.values):
289+
if (
290+
isinstance(k, ast.Constant)
291+
and isinstance(k.value, str)
292+
and isinstance(v, ast.Attribute)
293+
):
294+
out[k.value] = v.attr
295+
return out
296+
return {}
297+
298+
299+
_UNION_NAMES = frozenset({"Union", "Optional"})
300+
301+
302+
def _is_union_subscript(node: ast.expr) -> ast.expr | None:
303+
"""If `node` is `Union[a, b, ...]`, `Optional[a]`, or the `typing.`-prefixed
304+
forms, return the slice expression (a `Tuple` of arms for `Union`, or a
305+
single arm for `Optional`/`Union[X]`). Otherwise return None.
306+
307+
`Optional[X]` is `Union[X, None]` semantically — the `None` arm has no
308+
way to encode a bare `Response`, so unwrapping just the `X` is enough."""
309+
if not isinstance(node, ast.Subscript):
310+
return None
311+
val = node.value
312+
if isinstance(val, ast.Name) and val.id in _UNION_NAMES:
313+
return node.slice
314+
if isinstance(val, ast.Attribute) and val.attr in _UNION_NAMES:
315+
return node.slice
316+
return None
317+
318+
319+
def _annotation_has_bare_response(returns: ast.expr) -> bool:
320+
"""Walk a return annotation's union arms; return True iff any arm is the
321+
bare `Response` name (no `[T]` subscript). Handles both `X | Y` and
322+
`Union[X, Y]` union forms."""
323+
pending: list[ast.expr] = [returns]
324+
while pending:
325+
node = pending.pop()
326+
if isinstance(node, ast.BinOp) and isinstance(node.op, ast.BitOr):
327+
pending.append(node.left)
328+
pending.append(node.right)
329+
continue
330+
union_slice = _is_union_subscript(node)
331+
if union_slice is not None:
332+
if isinstance(union_slice, ast.Tuple):
333+
pending.extend(union_slice.elts)
334+
else:
335+
pending.append(union_slice)
336+
continue
337+
if isinstance(node, ast.Name) and node.id == "Response":
338+
return True
339+
if isinstance(node, ast.Attribute) and node.attr == "Response":
340+
# `rest_framework.response.Response` — bare attribute form
341+
return True
342+
return False
343+
344+
345+
def check_file_public_typed(path: Path) -> list[PublicUntyped]:
346+
"""Diagnose PUBLIC methods that lack a usable return annotation."""
347+
try:
348+
source = path.read_text()
349+
except OSError:
350+
return []
351+
try:
352+
tree = ast.parse(source)
353+
except SyntaxError:
354+
return []
355+
356+
out: list[PublicUntyped] = []
357+
for cls in _iter_classes(tree):
358+
ps = _publish_status(cls)
359+
if not ps:
360+
continue
361+
for item in cls.body:
362+
if not isinstance(item, (ast.FunctionDef, ast.AsyncFunctionDef)):
363+
continue
364+
if item.name not in _HTTP_METHODS:
365+
continue
366+
if ps.get(item.name.upper()) != "PUBLIC":
367+
continue
368+
if item.returns is None:
369+
out.append(
370+
PublicUntyped(
371+
path=path,
372+
line=item.lineno,
373+
cls=cls.name,
374+
method=item.name,
375+
reason="missing",
376+
)
377+
)
378+
continue
379+
if _annotation_has_bare_response(item.returns):
380+
out.append(
381+
PublicUntyped(
382+
path=path,
383+
line=item.lineno,
384+
cls=cls.name,
385+
method=item.name,
386+
reason="bare-Response",
387+
)
388+
)
389+
return out
390+
391+
191392
def check_file(path: Path) -> list[Mismatch]:
192393
try:
193394
source = path.read_text()
@@ -248,18 +449,32 @@ def iter_files(roots: Iterable[str]) -> Iterator[Path]:
248449
def main(argv: list[str]) -> int:
249450
roots = argv[1:] if len(argv) > 1 else list(DEFAULT_PATHS)
250451
all_mismatches: list[Mismatch] = []
452+
all_untyped: list[PublicUntyped] = []
251453
for path in iter_files(roots):
252454
all_mismatches.extend(check_file(path))
455+
all_untyped.extend(check_file_public_typed(path))
456+
253457
for m in all_mismatches:
254458
sys.stdout.write(f"{m}\n")
459+
for u in all_untyped:
460+
sys.stdout.write(f"{u}\n")
461+
255462
if all_mismatches:
256463
sys.stderr.write(
257-
f"\n{len(all_mismatches)} mismatch(es) — every `T` declared by "
258-
"`inline_sentry_response_serializer(...)` in `@extend_schema` must "
259-
"appear in the `Response[T]` (or union) annotation. The annotation "
260-
"MAY declare additional arms (e.g. local error TypedDicts) that the "
261-
"decorator does not expose.\n",
464+
f"\n{len(all_mismatches)} decorator/annotation mismatch(es) — every "
465+
"`T` declared by `inline_sentry_response_serializer(...)` in "
466+
"`@extend_schema` must appear in the `Response[T]` (or union) "
467+
"annotation. The annotation MAY declare additional arms (e.g. "
468+
"local error TypedDicts) that the decorator does not expose.\n",
469+
)
470+
if all_untyped:
471+
sys.stderr.write(
472+
f"\n{len(all_untyped)} PUBLIC method(s) without a typed return "
473+
"annotation. PUBLIC endpoints are documented in OpenAPI and must "
474+
"declare the runtime response shape statically so mypy + the "
475+
"OpenAPI generator can keep the spec honest.\n"
262476
)
477+
if all_mismatches or all_untyped:
263478
return 1
264479
return 0
265480

0 commit comments

Comments
 (0)