From 0924f9809ee8b2b543d3776aefbc587a7d62b247 Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Mon, 2 Nov 2026 15:58:36 -0500 Subject: [PATCH 1/7] feat(apidocs): Build an internal OpenAPI spec of every endpoint method The published spec describes only published methods, so it can't serve as a contract for the private and experimental endpoints the frontend calls. sentry.apidocs.internal builds one that does, reading undeclared responses from Response[T] annotations and omitting nothing. Its generator switches this on for its own build through hook parameters and a type-hint policy whose defaults are the published build's, so the published spec is unchanged. Extracted from the internal-spec half of #129315. --- .gitignore | 1 + Makefile | 6 + src/sentry/apidocs/extensions.py | 8 +- src/sentry/apidocs/hooks.py | 52 ++- src/sentry/apidocs/internal/__init__.py | 46 +++ src/sentry/apidocs/internal/generator.py | 223 +++++++++++ .../apidocs/internal/response_annotations.py | 184 +++++++++ src/sentry/apidocs/internal/schema.py | 103 +++++ src/sentry/apidocs/spectacular_ports.py | 86 ++++- src/sentry/apidocs/utils.py | 3 + tests/sentry/apidocs/internal/__init__.py | 0 .../sentry/apidocs/internal/test_generator.py | 352 ++++++++++++++++++ .../internal/test_response_annotations.py | 160 ++++++++ tests/sentry/apidocs/test_extensions.py | 159 +++++++- 14 files changed, 1365 insertions(+), 18 deletions(-) create mode 100644 src/sentry/apidocs/internal/__init__.py create mode 100644 src/sentry/apidocs/internal/generator.py create mode 100644 src/sentry/apidocs/internal/response_annotations.py create mode 100644 src/sentry/apidocs/internal/schema.py create mode 100644 tests/sentry/apidocs/internal/__init__.py create mode 100644 tests/sentry/apidocs/internal/test_generator.py create mode 100644 tests/sentry/apidocs/internal/test_response_annotations.py diff --git a/.gitignore b/.gitignore index c5e3a832aea6..5ebc7853a713 100644 --- a/.gitignore +++ b/.gitignore @@ -41,6 +41,7 @@ node_modules /tests/apidocs/openapi-deprecated.json /tests/sentry/utils/appleconnect/*.json /tests/apidocs/openapi-spectacular.json +/tests/apidocs/openapi-internal.json /fixtures/integrations/*/data/** /wheelhouse /test_cli/ diff --git a/Makefile b/Makefile index b3562a041f1f..789ba9f148f1 100644 --- a/Makefile +++ b/Makefile @@ -44,6 +44,12 @@ build-deprecated-docs: @echo "--> Building deprecated openapi spec from json files" pnpm run build-deprecated-docs +# Every endpoint method, published or not, for internal consumers such as frontend type +# generation; never published. See src/sentry/apidocs/internal/__init__.py. +build-internal-api-docs: build-deprecated-docs + @echo "--> Building internal drf-spectacular openapi spec (every endpoint method)" + @OPENAPIGENERATE=1 sentry django spectacular --generator-class sentry.apidocs.internal.InternalGenerator --file tests/apidocs/openapi-internal.json --format openapi-json --validate + build-api-docs: build-deprecated-docs build-spectacular-docs @echo "--> Dereference the json schema for ease of use" pnpm run deref-api-docs diff --git a/src/sentry/apidocs/extensions.py b/src/sentry/apidocs/extensions.py index fba6c421bb3c..aa4117dcb85f 100644 --- a/src/sentry/apidocs/extensions.py +++ b/src/sentry/apidocs/extensions.py @@ -12,7 +12,7 @@ from drf_spectacular.utils import Direction from sentry.api.serializers.rest_framework.base import snake_to_camel_case -from sentry.apidocs.spectacular_ports import resolve_type_hint +from sentry.apidocs.spectacular_ports import resolve_type_hint, resolve_type_hint_lenient from sentry.conf.server import GRANULAR_SCOPES @@ -38,7 +38,9 @@ class TokenAuthExtension(_TypedExtension, OpenApiAuthenticationExtension): def get_security_requirement(self, auto_schema: AutoSchema) -> dict[str, list[Any]]: scopes = set() for permission in auto_schema.view.get_permissions(): - for s in permission.scope_map.get(auto_schema.method, []): + # Permissions such as `SentryIsAuthenticated` require no scope and have no + # map. Only unpublished endpoints use them. + for s in getattr(permission, "scope_map", {}).get(auto_schema.method, []): scopes.add(s) scope_list = sorted(scopes - GRANULAR_SCOPES) @@ -88,7 +90,7 @@ def get_name(self, auto_schema: AutoSchema, direction: Direction) -> str | None: return name def map_serializer(self, auto_schema: AutoSchema, direction: Direction) -> Any: - return resolve_type_hint(self.target.typeSchema) + return resolve_type_hint_lenient(self.target.typeSchema, self.target.__name__) class SentryAppIssueActionSerializerExtension(_TypedExtension, OpenApiSerializerExtension): diff --git a/src/sentry/apidocs/hooks.py b/src/sentry/apidocs/hooks.py index 6bca8fecaf87..f80916f2d9c1 100644 --- a/src/sentry/apidocs/hooks.py +++ b/src/sentry/apidocs/hooks.py @@ -194,7 +194,25 @@ def _declared_silo(view_class: type) -> DeclaredSilo: EXPERIMENTAL_NOTICE = "**Experimental:** This API is under active development and may change." -def custom_preprocessing_hook(endpoints: Any) -> Any: # TODO: organize method, rename +def _is_published(status: ApiPublishStatus) -> bool: + return status.is_published + + +def _public_spelling(path: str) -> str: + return path + + +def custom_preprocessing_hook( # TODO: organize method, rename + endpoints: Any, + *, + admit: Callable[[ApiPublishStatus], bool] = _is_published, + public_path: Callable[[str], str] = _public_spelling, +) -> Any: + """Check every endpoint, collect its metadata, and keep the methods `admit` lets in. + + `public_path` gives the spelling of a path that the exclusions and the ownership + allowlist are written against. A build that spells paths otherwise, such as the + internal one (`sentry.apidocs.internal`), supplies both.""" _ENDPOINT_SERVERS.clear() _EXPERIMENTAL_OPERATIONS.clear() _OPERATION_SILOS.clear() @@ -219,7 +237,7 @@ def custom_preprocessing_hook(endpoints: Any) -> Any: # TODO: organize method, # Fail if endpoint is unowned if owner_team == ApiOwner.UNOWNED: - if path not in API_OWNERSHIP_ALLOWLIST_DONT_MODIFY: + if public_path(path) not in API_OWNERSHIP_ALLOWLIST_DONT_MODIFY: raise SentryApiBuildError( f"Endpoint {callback.view_class} is missing the attribute owner: ApiOwner. \n" + "If you can't find your team in ApiOwners feel free to add the associated github group. ", @@ -244,7 +262,7 @@ def custom_preprocessing_hook(endpoints: Any) -> Any: # TODO: organize method, f"Endpoint {callback.view_class} lists safe methods in idempotent_methods: {sorted(idempotent_methods & _SAFE_METHODS)}. Safe methods are always idempotent; only list non-safe methods such as POST.", ) - if any(path.startswith(p) for p in EXCLUSION_PATH_PREFIXES): + if any(public_path(path).startswith(p) for p in EXCLUSION_PATH_PREFIXES): pass else: @@ -254,12 +272,16 @@ def custom_preprocessing_hook(endpoints: Any) -> Any: # TODO: organize method, _IDEMPOTENT_OPERATIONS.add(operation_key) # endpoints that are documented via tooling status = callback.view_class.publish_status.get(method) - if status is not None and status.is_published: - if declared_idempotent is None and method not in _SAFE_METHODS: + if status is not None and admit(status): + if ( + status.is_published + and declared_idempotent is None + and method not in _SAFE_METHODS + ): raise SentryApiBuildError( f"Endpoint {callback.view_class} publishes {method} but does not declare idempotent_methods. List the methods a client may resend after a timeout or 5xx without changing the outcome, or set `idempotent_methods = frozenset()` if there are none. See src/sentry/apidocs/README.md.", ) - # only pass declared public methods of the endpoint + # only pass admitted (by default, published) methods of the endpoint # to the rest of the OpenAPI build pipeline filtered.append((path, path_regex, method, callback)) if status is ApiPublishStatus.PUBLIC_EXPERIMENTAL: @@ -339,9 +361,22 @@ def _validate_request_body( ) +def _in_reference(operation: Mapping[str, Any]) -> bool: + return True + + def custom_postprocessing_hook( - result: dict[str, Any], generator: Any, **kwargs: Any + result: dict[str, Any], + generator: Any, + *, + in_reference: Callable[[Mapping[str, Any]], bool] = _in_reference, + **kwargs: Any, ) -> dict[str, Any]: + """Stamp operation metadata, then check what the published reference needs. + + Operations `in_reference` rejects skip those checks. The published build has none; + the internal one (`sentry.apidocs.internal`) rejects its unpublished operations, + which it describes only for their shapes.""" # Stamp before alias canonicalization so metadata stays with its operation. for path, operations in result["paths"].items(): for method, operation in operations.items(): @@ -397,6 +432,9 @@ def custom_postprocessing_hook( endpoint_name = f"'{method_info['operationId']}'" + if not in_reference(method_info): + continue + summary = method_info.get("summary") if summary is not None: if summary in summaries_seen: diff --git a/src/sentry/apidocs/internal/__init__.py b/src/sentry/apidocs/internal/__init__.py new file mode 100644 index 000000000000..f704585c1e02 --- /dev/null +++ b/src/sentry/apidocs/internal/__init__.py @@ -0,0 +1,46 @@ +"""The internal OpenAPI build: every endpoint method Sentry serves, published or not. + +The published build (`make build-spectacular-docs`) describes only published methods, +for the public API reference. This one describes all of them, for internal consumers +such as frontend type generation, and is never published. `make build-internal-api-docs` +runs it, by handing drf-spectacular's `spectacular` command +`--generator-class sentry.apidocs.internal.InternalGenerator`. + +It is the published build, with the same hooks, checks, and settings, except that: + +- Nothing is omitted. Fields kept out of the public reference with + `omit_from_public_schema` (and TypedDict `exclude_fields` that declare a reason) are + described, since the backend sends them. With no omissions, it builds once, without + the published build's second build checking them. +- Every method is admitted, whatever its publish status, and stamped with it as + `x-sentry-publish-status`. The published reference's own checks (tags, + descriptions, unique summaries) skip unpublished operations. +- A literal alternation in a route, such as `(?:issues|groups)`, is spelled as its + first branch rather than `{var}`, so routes that differ only there stay apart. Every + path such a route answers on is listed under `x-sentry-path-aliases`. +- A response no `@extend_schema` declares is read from the handler's `Response[T]` + annotation, or marked `x-sentry-undocumented`. +- TypedDicts are closed objects, the null member of an optional union admits only null, + and a type hint that cannot be resolved becomes an `x-sentry-unresolved` placeholder + instead of failing the build. + +`InternalGenerator` switches all of this on for the duration of its own build, through +parameters of the shared hooks and `spectacular_ports.TypeHintPolicy`, whose defaults are +the published build's. The published build never sees any of it. +""" + +from sentry.apidocs.internal.generator import ( + PATH_ALIASES_EXTENSION, + PUBLISH_STATUS_EXTENSION, + InternalGenerator, +) +from sentry.apidocs.internal.response_annotations import UNDOCUMENTED_EXTENSION +from sentry.apidocs.spectacular_ports import UNRESOLVED_EXTENSION + +__all__ = ( + "InternalGenerator", + "PATH_ALIASES_EXTENSION", + "PUBLISH_STATUS_EXTENSION", + "UNDOCUMENTED_EXTENSION", + "UNRESOLVED_EXTENSION", +) diff --git a/src/sentry/apidocs/internal/generator.py b/src/sentry/apidocs/internal/generator.py new file mode 100644 index 000000000000..b1fb2a88176a --- /dev/null +++ b/src/sentry/apidocs/internal/generator.py @@ -0,0 +1,223 @@ +from __future__ import annotations + +import contextlib +import itertools +import re +from collections.abc import Callable, Iterator, Mapping, Sequence +from contextvars import ContextVar +from dataclasses import dataclass, field +from typing import Any, cast + +from django.core.exceptions import ImproperlyConfigured +from drf_spectacular.generators import SchemaGenerator +from drf_spectacular.openapi import AutoSchema +from drf_spectacular.settings import spectacular_settings + +from sentry.api.api_publish_status import ApiPublishStatus + +# Registers the extensions that resolve inline response serializers, including the ones +# this build infers. `sentry.runner.settings` does too, but only under OPENAPIGENERATE. +from sentry.apidocs import extensions # noqa: F401 +from sentry.apidocs.hooks import ( + CustomEndpointEnumerator, + custom_postprocessing_hook, + custom_preprocessing_hook, +) +from sentry.apidocs.internal.schema import internal_schema_class +from sentry.apidocs.omission_guard import omissions_disabled +from sentry.apidocs.spectacular_ports import TypeHintPolicy, type_hint_policy + +PUBLISH_STATUS_EXTENSION = "x-sentry-publish-status" +PATH_ALIASES_EXTENSION = "x-sentry-path-aliases" + +INTERNAL_TYPE_HINTS = TypeHintPolicy( + closed_typed_dicts=True, + null_only_union_member=True, + unresolvable_placeholder=True, +) + +_TITLE = "Sentry Internal API" +_DESCRIPTION = ( + "Every endpoint method Sentry serves, published or not, for internal consumers such " + "as frontend type generation. Never published: operations whose " + f"`{PUBLISH_STATUS_EXTENSION}` is not public carry no compatibility guarantee." +) + +# Literal alternations such as `(?:issues|groups)` or `(?:user-feedback|user-reports)`. +_ALTERNATION = re.compile(r"\(\?:([a-z-]+(?:\|[a-z-]+)+)\)") + + +@dataclass +class _InternalBuild: + # Each path spelled from a literal alternation, keyed by the spelling kept, listing + # every path the route answers on (the kept one first). + path_aliases: dict[str, list[str]] = field(default_factory=dict) + # The same paths as the published build spells them, with `{var}` for an alternation. + public_spellings: dict[str, str] = field(default_factory=dict) + # The publish status of each admitted operation, keyed by (path, lowercased method). + publish_status: dict[tuple[str, str], ApiPublishStatus] = field(default_factory=dict) + + +_current: ContextVar[_InternalBuild] = ContextVar("internal_build") + + +def _build() -> _InternalBuild: + try: + return _current.get() + except LookupError: + raise ImproperlyConfigured( + "The internal OpenAPI build's hooks run only under InternalGenerator." + ) from None + + +class InternalEndpointEnumerator(CustomEndpointEnumerator): + """Spells each literal alternation in a route as its first branch, not `{var}`. + + Rendering them all as `{var}` collapses distinct routes onto one path, such as + issue notes and issue user reports, and drf-spectacular keeps only one of them. + The first branch is the one the published build already uses for issues. + """ + + def get_api_endpoints(self, patterns: Any = None, prefix: str = "") -> Any: + build = _build() + build.path_aliases.clear() + build.public_spellings.clear() + return cast("Callable[..., Any]", super().get_api_endpoints)(patterns, prefix) + + def get_path_from_regex(self, path_regex: str) -> str: + public = super().get_path_from_regex(path_regex) + choices = [match.group(1).split("|") for match in _ALTERNATION.finditer(path_regex)] + if not choices: + return public + variants = [self._spelled(path_regex, branches) for branches in itertools.product(*choices)] + build = _build() + build.path_aliases[variants[0]] = variants + build.public_spellings[variants[0]] = public + return variants[0] + + def _spelled(self, path_regex: str, branches: Sequence[str]) -> str: + chosen = iter(branches) + return super().get_path_from_regex(_ALTERNATION.sub(lambda _: next(chosen), path_regex)) + + +class InternalGenerator(SchemaGenerator): + """Builds the internal spec. See `sentry.apidocs.internal`. + + Not a `CustomGenerator`, whose second build proves the published spec's omissions + removed only what they declare. This build omits nothing, so there is nothing to + prove. + """ + + endpoint_inspector_cls = InternalEndpointEnumerator + + def __init__(self, *args: object, **kwargs: object) -> None: + cast("Callable[..., None]", super().__init__)(*args, **kwargs) + + def get_schema(self, request: Any = None, public: bool = False) -> dict[str, Any]: + # Omissions keep fields out of the public reference and SDKs. This spec describes + # what the backend sends, and with closed TypedDicts, an omitted field would be + # one the spec says cannot exist. + get_schema = cast("Callable[..., dict[str, Any]]", super().get_schema) + with _internal_build(), omissions_disabled(): + return get_schema(request, public) + + def create_view(self, callback: Any, method: str, request: Any = None) -> Any: + view = cast("Callable[..., Any]", super().create_view)(callback, method, request) + schema = getattr(view, "schema", None) + if isinstance(schema, AutoSchema): + schema_class = internal_schema_class(type(schema)) + if schema_class is not type(schema): + view.schema = schema_class() + return view + + +def internal_preprocessing_hook(endpoints: Any) -> Any: + """The published build's preprocessing, admitting every method whatever its status.""" + build = _build() + admitted = custom_preprocessing_hook( + endpoints, + admit=lambda status: True, + public_path=lambda path: build.public_spellings.get(path, path), + ) + build.publish_status = { + (path, method.lower()): callback.view_class.publish_status[method] + for path, _path_regex, method, callback in admitted + } + return admitted + + +def internal_postprocessing_hook(result: Any, generator: Any, **kwargs: Any) -> Any: + """Stamp each operation's publish status and aliases, then postprocess as published.""" + build = _build() + # Before the published build's postprocessing, which can rewrite path keys. + for path, operations in result["paths"].items(): + aliases = build.path_aliases.get(path) + for method, operation in operations.items(): + status = build.publish_status.get((path, method)) + if status is not None: + operation[PUBLISH_STATUS_EXTENSION] = status.value + if aliases is not None: + operation[PATH_ALIASES_EXTENSION] = list(aliases) + return custom_postprocessing_hook(result, generator, in_reference=_in_reference, **kwargs) + + +def _in_reference(operation: Mapping[str, Any]) -> bool: + """Whether the published reference would carry the operation. + + `APPEND_PATHS` operations bypass enumeration and carry no status; they are public.""" + status = operation.get(PUBLISH_STATUS_EXTENSION) + return status is None or ApiPublishStatus(status).is_published + + +@contextlib.contextmanager +def _internal_build() -> Iterator[None]: + token = _current.set(_InternalBuild()) + try: + with ( + _spectacular_settings( + TITLE=_TITLE, + DESCRIPTION=_DESCRIPTION, + PREPROCESSING_HOOKS=_swapped( + spectacular_settings.PREPROCESSING_HOOKS, + custom_preprocessing_hook, + internal_preprocessing_hook, + ), + POSTPROCESSING_HOOKS=_swapped( + spectacular_settings.POSTPROCESSING_HOOKS, + custom_postprocessing_hook, + internal_postprocessing_hook, + ), + ), + type_hint_policy(INTERNAL_TYPE_HINTS), + ): + yield + finally: + _current.reset(token) + + +def _swapped( + hooks: Sequence[Callable[..., Any]], + published: Callable[..., Any], + internal: Callable[..., Any], +) -> list[Callable[..., Any]]: + """`hooks`, with the published build's hook replaced by the internal one wrapping it. + + Settings without Sentry's hooks (tests) still get the internal one, first.""" + swapped = [internal if hook is published else hook for hook in hooks] + return swapped if internal in swapped else [internal, *swapped] + + +@contextlib.contextmanager +def _spectacular_settings(**patches: Any) -> Iterator[None]: + """Patch drf-spectacular's settings, then restore exactly what was there. + + Unlike `drf_spectacular.settings.patched_settings`, this nests inside it, which the + `spectacular` command uses for `--custom-settings`.""" + original = {name: getattr(spectacular_settings, name) for name in patches} + for name, value in patches.items(): + setattr(spectacular_settings, name, value) + try: + yield + finally: + for name, value in original.items(): + setattr(spectacular_settings, name, value) diff --git a/src/sentry/apidocs/internal/response_annotations.py b/src/sentry/apidocs/internal/response_annotations.py new file mode 100644 index 000000000000..3299c6c5a7bf --- /dev/null +++ b/src/sentry/apidocs/internal/response_annotations.py @@ -0,0 +1,184 @@ +"""Response shapes read from endpoint handlers' ``Response[T]`` return annotations. + +The internal build describes every endpoint method, including the ones that never +declared ``@extend_schema(responses=...)``. Many of those still state their response +shape in the handler signature, ``def get(...) -> Response[FooResponse]``, so the +internal build reads it from there. + +``Response[T]`` is erased at runtime (DRF's ``Response.__class_getitem__`` returns the +class itself), so the annotation is read from the handler's source and its type +argument evaluated in the handler's module. +""" + +from __future__ import annotations + +import ast +import collections.abc +import inspect +import types +import typing +from collections import defaultdict +from collections.abc import Callable, Sequence +from dataclasses import dataclass +from typing import Any, Union, is_typeddict + +from drf_spectacular.drainage import warn + +from sentry.apidocs.utils import _RawSchema, inline_sentry_response_serializer + +# Marks a response whose shape the backend does not declare anywhere. Generated +# frontend types render it as `unknown`. +UNDOCUMENTED_EXTENSION = "x-sentry-undocumented" + +_RESPONSE_NAMES = frozenset({"Response"}) + + +@dataclass(frozen=True) +class AnnotatedResponse: + """The success shapes a handler's return annotation declares.""" + + # The type arguments of every success `Response[T]` member, in declaration order. + # Empty when the only success member is `Response[None]`. + types: tuple[Any, ...] + + +def _is_error_arm(node: ast.expr) -> bool: + """Whether a `Response[...]` type argument names an error body. + + Error arms are told apart by name: `ValidationErrorResponse` is an alias of + `dict[str, Any]`, so once evaluated it is indistinguishable from a loose + success shape. + """ + name = ast.unparse(node) + return name == "DetailResponse" or name.endswith("ErrorResponse") + + +def _union_members(node: ast.expr) -> list[ast.expr]: + if isinstance(node, ast.BinOp) and isinstance(node.op, ast.BitOr): + return _union_members(node.left) + _union_members(node.right) + return [node] + + +def _response_argument(node: ast.expr) -> ast.expr | None: + """The `T` in `Response[T]`, or None for anything else (bare `Response`, ...).""" + if not isinstance(node, ast.Subscript): + return None + value = node.value + name = value.attr if isinstance(value, ast.Attribute) else getattr(value, "id", None) + if name not in _RESPONSE_NAMES: + return None + return node.slice + + +def _return_annotation(func: Callable[..., Any]) -> ast.expr | None: + try: + source = inspect.getsource(func) + except (OSError, TypeError): + return None + if source[:1].isspace(): + # A method. Dedenting fails on a body holding a string literal that is + # indented less than the method, so nest the source in a block instead. + source = "if True:\n" + source + for node in ast.walk(ast.parse(source)): + if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)): + return node.returns + return None + + +def annotated_response(func: Callable[..., Any], context: str) -> AnnotatedResponse | None: + """The success shapes `func` declares through `Response[T]`, if it declares any. + + Error arms (`Response[DetailResponse]`, `Response[ValidationErrorResponse]`) and + untyped members (a bare `Response`, `HttpResponse`) are ignored. A type argument + that cannot be evaluated, such as one imported only under `TYPE_CHECKING`, makes + the whole annotation unusable, since what remains would understate the response. + """ + func = inspect.unwrap(func) + returns = _return_annotation(func) + if returns is None: + return None + + arguments = [_response_argument(member) for member in _union_members(returns)] + success = [node for node in arguments if node is not None and not _is_error_arm(node)] + if not success: + return None + + namespace = dict(getattr(func, "__globals__", {})) + found: list[Any] = [] + for node in success: + expression = ast.unparse(node) + try: + evaluated = eval(expression, namespace) # noqa: S307 -- our own annotations + except Exception as exc: + warn(f"could not evaluate Response[{expression}] for {context}: {exc!r}") + return None + if evaluated is not None and evaluated not in found: + found.append(evaluated) + return AnnotatedResponse(types=tuple(found)) + + +def response_type(hints: Sequence[Any]) -> Any: + """A single type hint for the success shapes, as `resolve_type_hint` accepts.""" + if len(hints) == 1: + return hints[0] + return Union[tuple(hints)] # noqa: UP007 -- built from a runtime sequence + + +_SEQUENCE_ORIGINS = ( + list, + set, + frozenset, + tuple, + collections.abc.Collection, + collections.abc.Iterable, + collections.abc.MutableSequence, + collections.abc.Sequence, +) +_MAPPING_ORIGINS = (dict, defaultdict, collections.abc.Mapping, collections.abc.MutableMapping) + + +def _type_name(hint: Any) -> str: + """A component name for `hint`: `FooResponse`, `ListFooResponse`, `FooOrBar`, ...""" + origin = typing.get_origin(hint) + args = typing.get_args(hint) + if origin is Union or origin is types.UnionType: + return "Or".join(_type_name(arg) for arg in args if arg is not type(None)) + if origin in _SEQUENCE_ORIGINS: + return "List" + (_type_name(args[0]) if args else "Any") + if origin in _MAPPING_ORIGINS: + return "Dict" + (_type_name(args[1]) if len(args) == 2 else "Any") + name = getattr(hint, "__name__", None) + if hint is Any or not isinstance(name, str): + return "Any" + name = name.lstrip("_") + return name[:1].upper() + name[1:] + + +def _inline_serializers_named(name: str) -> list[type[_RawSchema]]: + """Every inline response serializer made under `name` so far. + + `inline_sentry_response_serializer` makes a `_RawSchema` subclass per call, and + endpoints make theirs at import time, so this sees every declared name.""" + return [cls for cls in _RawSchema.__subclasses__() if cls.__name__ == name] + + +def inferred_response_serializer(hint: Any) -> type: + """The inline response serializer describing `hint`, named after it. + + An `@extend_schema` declaration that already describes the same hint under the + same name is reused, so both describe one component. A different hint that + happens to share the name gets a distinct one rather than overwriting it. + """ + base = _type_name(hint) + if not is_typeddict(hint) and not base.endswith("Response"): + base = f"{base}Response" + name = base + for suffix in range(2, 100): + existing = _inline_serializers_named(name) + if not existing: + return inline_sentry_response_serializer(name, hint) + for serializer in existing: + if serializer.typeSchema == hint: + return serializer + name = f"{base}{suffix}" + raise RuntimeError(f"too many response components named {base}") diff --git a/src/sentry/apidocs/internal/schema.py b/src/sentry/apidocs/internal/schema.py new file mode 100644 index 000000000000..0ef9b49825b1 --- /dev/null +++ b/src/sentry/apidocs/internal/schema.py @@ -0,0 +1,103 @@ +from __future__ import annotations + +from collections.abc import Callable +from typing import Any, cast + +from drf_spectacular.drainage import get_override +from drf_spectacular.openapi import AutoSchema +from drf_spectacular.plumbing import force_instance, safe_ref +from rest_framework.generics import GenericAPIView + +from sentry.apidocs.internal.response_annotations import ( + UNDOCUMENTED_EXTENSION, + annotated_response, + inferred_response_serializer, + response_type, +) +from sentry.apidocs.omissions import DEPRECATION_REASONS_OVERRIDE +from sentry.apidocs.schema import SentrySchema + + +class InternalSchema(SentrySchema): + """`SentrySchema`, describing the responses no `@extend_schema` declares as well. + + An undeclared response is read from the handler's `Response[T]` annotation. Without + one, drf-spectacular guesses it from the view's serializer if the view has one, and + otherwise it is marked `x-sentry-undocumented`. + """ + + def _map_serializer( + self, serializer: Any, direction: Any, bypass_extensions: bool = False + ) -> Any: + """Mark the fields `@sentry_schema_serializer(deprecate=...)` declares deprecated. + + This build runs with omissions off, and `SentrySchema` then maps a serializer as + if its decorator declared nothing, deprecations included: that is the baseline the + published build compares against. This build omits nothing, but keeps them.""" + schema = super()._map_serializer(serializer, direction, bypass_extensions) + properties = schema.get("properties", {}) + instance = cast("Callable[[object], object]", force_instance)(serializer) + deprecated = get_override(instance, DEPRECATION_REASONS_OVERRIDE, {}) + for name in deprecated or {}: + if name in properties: + # As drf-spectacular marks one, keeping a `$ref` in its own context. + properties[name] = safe_ref({**properties[name], "deprecated": True}) + return schema + + def get_response_serializers(self) -> Any: + annotated = annotated_response(self.view_func, f"{self.method} {self.path}") + if annotated is not None: + if not annotated.types: + return {"204": None} + return inferred_response_serializer(response_type(annotated.types)) + if _guesses_serializer(self.view): + return super().get_response_serializers() + if self.method == "DELETE": + return {"204": None} + return {"200": {UNDOCUMENTED_EXTENSION: True}} + + def _get_serializer(self) -> Any: + """Skip drf-spectacular's serializer guess for views that give it nothing to go on. + + It reports an error for each, which a build describing every undocumented + endpoint would otherwise report hundreds of times over.""" + if not _guesses_serializer(self.view): + return None + return cast("Callable[..., Any]", super()._get_serializer)() + + +def _guesses_serializer(view: Any) -> bool: + """Whether drf-spectacular can derive a serializer from the view itself.""" + return ( + isinstance(view, GenericAPIView) + or callable(getattr(view, "get_serializer", None)) + or callable(getattr(view, "get_serializer_class", None)) + or hasattr(view, "serializer_class") + ) + + +# The internal variant of each schema class seen, so each is made once per process. +_internal_schema_classes: dict[type[AutoSchema], type[AutoSchema]] = {} + + +def internal_schema_class(schema_class: type[AutoSchema]) -> type[AutoSchema]: + """`schema_class`, with `InternalSchema` inserted directly above `SentrySchema`. + + `@extend_schema` subclasses the schema class when it decorates, and its overrides + fall back to `super()` only for what it does not declare. Below them, a declared + response keeps taking precedence over an inferred one. Schema classes that do not + build on `SentrySchema` are left alone. + """ + if schema_class not in _internal_schema_classes: + _internal_schema_classes[schema_class] = _with_internal_schema(schema_class) + return _internal_schema_classes[schema_class] + + +def _with_internal_schema(schema_class: type[AutoSchema]) -> type[AutoSchema]: + if schema_class is SentrySchema: + return InternalSchema + if not issubclass(schema_class, SentrySchema) or issubclass(schema_class, InternalSchema): + return schema_class + # C3 linearization puts InternalSchema after every class of `schema_class` that + # subclasses SentrySchema, and before SentrySchema itself. + return type(f"Internal{schema_class.__name__}", (schema_class, InternalSchema), {}) diff --git a/src/sentry/apidocs/spectacular_ports.py b/src/sentry/apidocs/spectacular_ports.py index b57ce91e6073..30a71c49c4f7 100644 --- a/src/sentry/apidocs/spectacular_ports.py +++ b/src/sentry/apidocs/spectacular_ports.py @@ -29,17 +29,20 @@ # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. import collections +import contextlib import inspect import typing from collections import defaultdict -from collections.abc import Sequence +from collections.abc import Iterator, Sequence +from contextvars import ContextVar +from dataclasses import dataclass from enum import Enum from types import UnionType from typing import Any, Literal, Union, is_typeddict from typing import get_type_hints as _get_type_hints import drf_spectacular -from drf_spectacular.drainage import get_override +from drf_spectacular.drainage import get_override, warn from drf_spectacular.plumbing import ( UnableToProceedError, build_basic_type, @@ -120,6 +123,55 @@ def build_array_type( return drf_build_array_type(schema=schema, min_length=min_length, max_length=max_length) +@dataclass(frozen=True) +class TypeHintPolicy: + """How `resolve_type_hint` treats the hints that builds describe differently. + + The defaults are the published build's. The internal build + (`sentry.apidocs.internal`) switches them on for its duration. + """ + + # Describe a TypedDict as a closed object (`additionalProperties: false`), since it + # enumerates every key it can carry. + closed_typed_dicts: bool = False + # Limit the null member of an optional union of several types to null itself. + # `{"type": "object", "nullable": true}` alone also accepts any dictionary. + null_only_union_member: bool = False + # Replace a hint that cannot be resolved with an untyped placeholder, carrying the + # hint under `x-sentry-unresolved`, instead of failing the build. + unresolvable_placeholder: bool = False + + +_policy: ContextVar[TypeHintPolicy] = ContextVar("type_hint_policy", default=TypeHintPolicy()) + + +@contextlib.contextmanager +def type_hint_policy(policy: TypeHintPolicy) -> Iterator[None]: + token = _policy.set(policy) + try: + yield + finally: + _policy.reset(token) + + +UNRESOLVED_EXTENSION = "x-sentry-unresolved" + + +def resolve_type_hint_lenient(hint: Any, context: str) -> Any: + """`resolve_type_hint`, but a policy may replace an unresolvable hint with a placeholder. + + The placeholder marks exactly where a contract is not expressible yet, such as a + plain class used as a response. Without the policy, an unresolvable hint fails the + build: in the published spec it is a bug.""" + try: + return resolve_type_hint(hint) + except UnableToProceedError: + if not _policy.get().unresolvable_placeholder: + raise + warn(f"could not resolve {hint!r} for {context}; emitting an untyped placeholder") + return {UNRESOLVED_EXTENSION: repr(hint)} + + # The TypedDicts whose fields are being resolved right now, outermost first. _typed_dicts_in_progress: list[Any] = [] @@ -164,7 +216,12 @@ def resolve_type_hint(hint: TypeHint) -> TypeHint: max_length=len(args), min_length=len(args), ) - elif origin is dict or origin is defaultdict or origin is collections.abc.Mapping: + elif origin in ( + dict, + defaultdict, + collections.abc.Mapping, + collections.abc.MutableMapping, + ): schema = build_basic_type(OpenApiTypes.OBJECT) if args and args[1] is not typing.Any and schema is not None: schema["additionalProperties"] = _resolve_at(args[1], "additionalProperties") @@ -198,16 +255,20 @@ def resolve_type_hint(hint: TypeHint) -> TypeHint: return _cycle_placeholder(hint) excluded_fields = _typed_dict_exclusions(hint, excluded_fields) + closed: dict[str, Any] = ( + {"additionalProperties": False} if _policy.get().closed_typed_dicts else {} + ) _typed_dicts_in_progress.append(hint) try: return build_object_type( properties={ - k: _resolve_at(v, "properties", k) + k: _resolve_at_lenient(v, f"{hint.__name__}.{k}", "properties", k) for k, v in get_type_hints(hint).items() if k not in excluded_fields }, description=inspect.cleandoc(hint.__doc__ or ""), required=[h for h in hint.__required_keys__ if h not in excluded_fields], + **closed, ) finally: _typed_dicts_in_progress.pop() @@ -236,11 +297,18 @@ def resolve_type_hint(hint: TypeHint) -> TypeHint: # - https://github.com/tfranzel/drf-spectacular/issues/925 # - https://github.com/OAI/OpenAPI-Specification/issues/1368. if len(args) > 2: - schema["anyOf"].append({"type": "object", "nullable": True}) + null_schema: dict[str, Any] = {"type": "object", "nullable": True} + if _policy.get().null_only_union_member: + null_schema["enum"] = [None] + schema["anyOf"].append(null_schema) else: schema["nullable"] = True return schema - elif origin is collections.abc.Iterable: + elif origin in ( + collections.abc.Iterable, + collections.abc.Collection, + collections.abc.MutableSequence, + ): return build_array_type(_resolve_at(args[0], "items")) else: raise UnableToProceedError(hint) @@ -252,6 +320,12 @@ def _resolve_at(hint: Any, *keys: str) -> Any: return resolve_type_hint(hint) +def _resolve_at_lenient(hint: Any, context: str, *keys: str) -> Any: + """`_resolve_at` with `resolve_type_hint_lenient`.""" + with descended(*keys): + return resolve_type_hint_lenient(hint, context) + + def _typed_dict_exclusions(hint: Any, excluded_fields: Any) -> list[str]: """The fields a TypedDict leaves out, recording the ones it declares as omissions. diff --git a/src/sentry/apidocs/utils.py b/src/sentry/apidocs/utils.py index 811c7ed5714b..80f6315828e9 100644 --- a/src/sentry/apidocs/utils.py +++ b/src/sentry/apidocs/utils.py @@ -29,6 +29,9 @@ class _RawSchema: Used by `utils.inline_sentry_response_serializer` """ + # Set on each class `inline_sentry_response_serializer` creates. + typeSchema: ResponseTypeHint + def __init__(self, t: ResponseTypeHint) -> None: self.typeSchema = t diff --git a/tests/sentry/apidocs/internal/__init__.py b/tests/sentry/apidocs/internal/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/tests/sentry/apidocs/internal/test_generator.py b/tests/sentry/apidocs/internal/test_generator.py new file mode 100644 index 000000000000..30577031c911 --- /dev/null +++ b/tests/sentry/apidocs/internal/test_generator.py @@ -0,0 +1,352 @@ +from __future__ import annotations + +from collections.abc import Callable +from typing import Any, TypedDict, cast +from unittest import mock + +import pytest +from django.core.exceptions import ImproperlyConfigured +from django.urls import re_path +from drf_spectacular.settings import spectacular_settings +from drf_spectacular.utils import extend_schema +from drf_spectacular.validation import validate_schema +from rest_framework import serializers +from rest_framework.request import Request +from rest_framework.response import Response + +from sentry.api.api_owners import ApiOwner +from sentry.api.api_publish_status import ApiPublishStatus +from sentry.api.base import Endpoint +from sentry.apidocs.hooks import ( + CustomEndpointEnumerator, + CustomGenerator, + custom_postprocessing_hook, + custom_preprocessing_hook, +) +from sentry.apidocs.internal import ( + PATH_ALIASES_EXTENSION, + PUBLISH_STATUS_EXTENSION, + UNDOCUMENTED_EXTENSION, + InternalGenerator, +) +from sentry.apidocs.internal.generator import ( + InternalEndpointEnumerator, + _build, + _internal_build, + internal_postprocessing_hook, + internal_preprocessing_hook, +) +from sentry.apidocs.omissions import sentry_schema_serializer +from sentry.apidocs.utils import SentryApiBuildError, inline_sentry_response_serializer + +# The published build writes API ownership stats to a tracked file; tests must not. +no_ownership_file = mock.patch("sentry.apidocs.hooks.__write_ownership_data") + +NOTES_REGEX = ( + "^api/0/organizations/(?P[^/]+)/" + "(?:issues|groups)/(?P[^/]+)/(?:notes|comments)/$" +) +PREFIX = "/api/0/organizations/{organization_id_or_slug}" + + +def _endpoint(path: str, status: ApiPublishStatus) -> tuple[Any, Any, str, Any]: + view_class = type( + "FakeEndpoint", + (), + {"owner": ApiOwner.CRONS, "publish_status": {"GET": status}, "servers": None}, + ) + callback = type("FakeCallback", (), {"view_class": view_class}) + return (path, path, "GET", callback) + + +ENDPOINTS = [ + _endpoint("/api/0/public/", ApiPublishStatus.PUBLIC), + _endpoint("/api/0/public-experimental/", ApiPublishStatus.PUBLIC_EXPERIMENTAL), + _endpoint("/api/0/experimental/", ApiPublishStatus.EXPERIMENTAL), + _endpoint("/api/0/private/", ApiPublishStatus.PRIVATE), +] + + +def _documented_operation(operation_id: str) -> dict[str, Any]: + return { + "tags": ["Events"], + "description": "Documented", + "operationId": operation_id, + "parameters": [], + } + + +class TestEnumerator: + def test_published_build_renders_alternations_as_var(self) -> None: + assert ( + CustomEndpointEnumerator().get_path_from_regex(NOTES_REGEX) + == f"{PREFIX}/{{var}}/{{issue_id}}/{{var}}/" + ) + + def test_spells_first_branch_and_records_the_rest(self) -> None: + with _internal_build(): + path = InternalEndpointEnumerator().get_path_from_regex(NOTES_REGEX) + build = _build() + + assert path == f"{PREFIX}/issues/{{issue_id}}/notes/" + assert build.path_aliases == { + path: [ + f"{PREFIX}/issues/{{issue_id}}/notes/", + f"{PREFIX}/issues/{{issue_id}}/comments/", + f"{PREFIX}/groups/{{issue_id}}/notes/", + f"{PREFIX}/groups/{{issue_id}}/comments/", + ] + } + assert build.public_spellings == {path: f"{PREFIX}/{{var}}/{{issue_id}}/{{var}}/"} + + def test_leaves_alternations_inside_parameters(self) -> None: + regex = ( + "^api/0/projects/(?P[^/]+)/(?P[^/]+)/" + "events/(?P(?:\\d+|[A-Fa-f0-9-]{32,36}|latest))/$" + ) + + with _internal_build(): + path = InternalEndpointEnumerator().get_path_from_regex(regex) + assert _build().path_aliases == {} + + assert path == ( + "/api/0/projects/{organization_id_or_slug}/{project_id_or_slug}/events/{event_id}/" + ) + + def test_runs_only_inside_the_internal_build(self) -> None: + with pytest.raises(ImproperlyConfigured): + InternalEndpointEnumerator().get_path_from_regex(NOTES_REGEX) + + +@no_ownership_file +class TestPreprocessing: + def test_published_build_admits_published_methods(self, _write: Any) -> None: + filtered = custom_preprocessing_hook(ENDPOINTS) + + assert [path for path, *_ in filtered] == ["/api/0/public/", "/api/0/public-experimental/"] + + def test_admits_every_method_and_records_its_status(self, _write: Any) -> None: + with _internal_build(): + filtered = internal_preprocessing_hook(ENDPOINTS) + publish_status = _build().publish_status + + assert [path for path, *_ in filtered] == [ + "/api/0/public/", + "/api/0/public-experimental/", + "/api/0/experimental/", + "/api/0/private/", + ] + assert publish_status == { + ("/api/0/public/", "get"): ApiPublishStatus.PUBLIC, + ("/api/0/public-experimental/", "get"): ApiPublishStatus.PUBLIC_EXPERIMENTAL, + ("/api/0/experimental/", "get"): ApiPublishStatus.EXPERIMENTAL, + ("/api/0/private/", "get"): ApiPublishStatus.PRIVATE, + } + + def test_excludes_legacy_issue_routes_by_public_spelling(self, _write: Any) -> None: + # The exclusions are written against the published build's `{var}` spelling. + endpoints = [ + _endpoint("/api/0/issues/{issue_id}/", ApiPublishStatus.PUBLIC), + _endpoint(f"{PREFIX}/issues/{{issue_id}}/", ApiPublishStatus.PUBLIC), + ] + + with _internal_build(): + _build().public_spellings["/api/0/issues/{issue_id}/"] = "/api/0/{var}/{issue_id}/" + filtered = internal_preprocessing_hook(endpoints) + + assert [path for path, *_ in filtered] == [f"{PREFIX}/issues/{{issue_id}}/"] + + +class TestPostprocessing: + def test_stamps_status_and_skips_reference_checks_on_unpublished(self) -> None: + result = { + "components": {"schemas": {}}, + "paths": { + "/api/0/public/": {"get": _documented_operation("public")}, + # No tag and no description would fail the published checks. + "/api/0/private/": {"get": {"operationId": "private", "parameters": []}}, + }, + } + + with _internal_build(): + _build().publish_status = { + ("/api/0/public/", "get"): ApiPublishStatus.PUBLIC, + ("/api/0/private/", "get"): ApiPublishStatus.PRIVATE, + } + processed = internal_postprocessing_hook(result, None) + + assert processed["paths"]["/api/0/public/"]["get"][PUBLISH_STATUS_EXTENSION] == "public" + assert processed["paths"]["/api/0/private/"]["get"][PUBLISH_STATUS_EXTENSION] == "private" + + def test_still_checks_published_operations(self) -> None: + result = { + "components": {"schemas": {}}, + "paths": {"/api/0/public/": {"get": {"operationId": "public", "parameters": []}}}, + } + + with _internal_build(), pytest.raises(SentryApiBuildError): + _build().publish_status = {("/api/0/public/", "get"): ApiPublishStatus.PUBLIC} + internal_postprocessing_hook(result, None) + + def test_stamps_path_aliases(self) -> None: + notes = f"{PREFIX}/issues/{{issue_id}}/notes/" + comments = f"{PREFIX}/issues/{{issue_id}}/comments/" + result = { + "components": {"schemas": {}}, + "paths": {notes: {"get": {"operationId": "notes", "parameters": []}}}, + } + + with _internal_build(): + _build().path_aliases[notes] = [notes, comments] + _build().publish_status = {(notes, "get"): ApiPublishStatus.PRIVATE} + processed = internal_postprocessing_hook(result, None) + + assert processed["paths"][notes]["get"][PATH_ALIASES_EXTENSION] == [notes, comments] + + def test_published_build_stamps_nothing(self) -> None: + result = { + "components": {"schemas": {}}, + "paths": {"/api/0/public/": {"get": _documented_operation("public")}}, + } + + operation = custom_postprocessing_hook(result, None)["paths"]["/api/0/public/"]["get"] + + assert PUBLISH_STATUS_EXTENSION not in operation + assert PATH_ALIASES_EXTENSION not in operation + + +def test_build_swaps_hooks_and_restores_settings() -> None: + before = ( + list(spectacular_settings.PREPROCESSING_HOOKS), + list(spectacular_settings.POSTPROCESSING_HOOKS), + spectacular_settings.TITLE, + ) + + with _internal_build(): + assert spectacular_settings.PREPROCESSING_HOOKS[0] is internal_preprocessing_hook + assert internal_postprocessing_hook in spectacular_settings.POSTPROCESSING_HOOKS + assert custom_preprocessing_hook not in spectacular_settings.PREPROCESSING_HOOKS + assert custom_postprocessing_hook not in spectacular_settings.POSTPROCESSING_HOOKS + + assert ( + list(spectacular_settings.PREPROCESSING_HOOKS), + list(spectacular_settings.POSTPROCESSING_HOOKS), + spectacular_settings.TITLE, + ) == before + + +class BoxItem(TypedDict): + id: str + + +@sentry_schema_serializer(omit_from_public_schema={"secret": "Not for the public reference."}) +class BoxDeclaredItem(TypedDict): + name: str + secret: str + + +@sentry_schema_serializer(deprecate={"legacyName": "Use name instead."}) +class BoxPatchSerializer(serializers.Serializer[Any]): + name = serializers.CharField(help_text="The name.") + legacyName = serializers.CharField(help_text="The old name.", required=False) + + +class BoxEndpoint(Endpoint): + owner = ApiOwner.CRONS + permission_classes = () + publish_status = { + "GET": ApiPublishStatus.PRIVATE, + "PUT": ApiPublishStatus.PUBLIC, + "PATCH": ApiPublishStatus.EXPERIMENTAL, + "POST": ApiPublishStatus.PRIVATE, + "DELETE": ApiPublishStatus.PRIVATE, + } + idempotent_methods = frozenset({"PUT"}) + + def get(self, request: Request) -> Response[BoxItem]: + raise NotImplementedError + + @extend_schema( + tags=["Events"], + responses={200: inline_sentry_response_serializer("BoxDeclaredItem", BoxDeclaredItem)}, + ) + def put(self, request: Request) -> Response[BoxItem]: + """ + Update a box item + + A declared response takes precedence over the annotation. + """ + raise NotImplementedError + + @extend_schema(request=BoxPatchSerializer) + def patch(self, request: Request) -> Response[None]: + raise NotImplementedError + + def post(self, request: Request) -> Response: + raise NotImplementedError + + def delete(self, request: Request) -> Response: + raise NotImplementedError + + +PATTERNS = [re_path(r"^api/0/box/(?:items|things)/$", BoxEndpoint.as_view())] + + +def _json_schema(operation: dict[str, Any], code: str) -> dict[str, Any]: + return operation["responses"][code]["content"]["application/json"]["schema"] + + +@no_ownership_file +def test_internal_build_end_to_end(_write: Any) -> None: + schema = InternalGenerator(patterns=PATTERNS).get_schema(request=None, public=True) + cast("Callable[[dict[str, Any]], None]", validate_schema)(schema) + + operations = schema["paths"]["/api/0/box/items/"] + assert {method: op[PUBLISH_STATUS_EXTENSION] for method, op in operations.items()} == { + "get": "private", + "put": "public", + "patch": "experimental", + "post": "private", + "delete": "private", + } + assert operations["get"][PATH_ALIASES_EXTENSION] == ["/api/0/box/items/", "/api/0/box/things/"] + # Read from the annotation, and closed: a TypedDict lists every key it carries. + assert _json_schema(operations["get"], "200") == {"$ref": "#/components/schemas/BoxItem"} + assert schema["components"]["schemas"]["BoxItem"] == { + "type": "object", + "properties": {"id": {"type": "string"}}, + "required": ["id"], + "additionalProperties": False, + } + assert _json_schema(operations["put"], "200") == { + "$ref": "#/components/schemas/BoxDeclaredItem" + } + # Nothing is omitted: the backend sends `secret`, so the internal spec describes it. + assert schema["components"]["schemas"]["BoxDeclaredItem"]["properties"].keys() == { + "name", + "secret", + } + assert operations["patch"]["responses"] == {"204": {"description": "No response body"}} + # Omitting nothing still marks what is deprecated. + assert schema["components"]["schemas"]["PatchedBoxPatch"]["properties"]["legacyName"] == { + "type": "string", + "description": "The old name.", + "deprecated": True, + } + assert _json_schema(operations["post"], "200") == {UNDOCUMENTED_EXTENSION: True} + assert operations["delete"]["responses"] == {"204": {"description": "No response body"}} + + +@no_ownership_file +def test_published_build_of_the_same_routes_is_unchanged(_write: Any) -> None: + with mock.patch.object( + spectacular_settings, "PREPROCESSING_HOOKS", [custom_preprocessing_hook] + ): + schema = CustomGenerator(patterns=PATTERNS).get_schema(request=None, public=True) + + (operations,) = schema["paths"].values() + assert list(operations) == ["put"] + assert PUBLISH_STATUS_EXTENSION not in operations["put"] + declared_item = schema["components"]["schemas"]["BoxDeclaredItem"] + assert "additionalProperties" not in declared_item + assert declared_item["properties"].keys() == {"name"} diff --git a/tests/sentry/apidocs/internal/test_response_annotations.py b/tests/sentry/apidocs/internal/test_response_annotations.py new file mode 100644 index 000000000000..f06f8ff77712 --- /dev/null +++ b/tests/sentry/apidocs/internal/test_response_annotations.py @@ -0,0 +1,160 @@ +from __future__ import annotations + +from typing import TYPE_CHECKING, Any, TypedDict + +from rest_framework.response import Response + +from sentry.apidocs.internal.response_annotations import ( + annotated_response, + inferred_response_serializer, + response_type, +) +from sentry.apidocs.response_types import DetailResponse, ValidationErrorResponse +from sentry.apidocs.utils import inline_sentry_response_serializer + +if TYPE_CHECKING: + from sentry.apidocs.response_types import DetailResponse as OnlyTypeChecked + + +class AnnotatedItem(TypedDict): + id: str + + +class AnnotatedOther(TypedDict): + name: str + + +def single(request: Any) -> Response[AnnotatedItem]: + raise NotImplementedError + + +def with_error_arms( + request: Any, +) -> Response[AnnotatedItem] | Response[DetailResponse] | Response[ValidationErrorResponse]: + raise NotImplementedError + + +def two_success_arms(request: Any) -> Response[AnnotatedItem] | Response[AnnotatedOther]: + raise NotImplementedError + + +def only_error_arms(request: Any) -> Response[DetailResponse]: + raise NotImplementedError + + +def no_content(request: Any) -> Response[None]: + raise NotImplementedError + + +def bare_member(request: Any) -> Response | Response[list[AnnotatedItem]]: + raise NotImplementedError + + +def bare(request: Any) -> Response: + raise NotImplementedError + + +def unannotated(request: Any): # type: ignore[no-untyped-def] # the missing annotation is under test + raise NotImplementedError + + +def type_checking_only(request: Any) -> Response[OnlyTypeChecked]: + raise NotImplementedError + + +class Handlers: + def get(self, request: Any) -> Response[AnnotatedItem]: + message = """ +less indented than the method +""" + raise NotImplementedError(message) + + +def test_single_arm() -> None: + annotated = annotated_response(single, "GET /single/") + assert annotated is not None + assert annotated.types == (AnnotatedItem,) + + +def test_error_arms_are_ignored() -> None: + annotated = annotated_response(with_error_arms, "GET /errors/") + assert annotated is not None + assert annotated.types == (AnnotatedItem,) + + +def test_every_success_arm_is_kept() -> None: + annotated = annotated_response(two_success_arms, "GET /two/") + assert annotated is not None + assert annotated.types == (AnnotatedItem, AnnotatedOther) + assert response_type(annotated.types) == AnnotatedItem | AnnotatedOther + + +def test_only_error_arms_declare_nothing() -> None: + assert annotated_response(only_error_arms, "GET /only-errors/") is None + + +def test_no_content() -> None: + annotated = annotated_response(no_content, "DELETE /no-content/") + assert annotated is not None + assert annotated.types == () + + +def test_bare_member_is_ignored() -> None: + annotated = annotated_response(bare_member, "GET /bare-member/") + assert annotated is not None + assert annotated.types == (list[AnnotatedItem],) + + +def test_untyped_handlers_declare_nothing() -> None: + assert annotated_response(bare, "GET /bare/") is None + assert annotated_response(unannotated, "GET /unannotated/") is None + + +def test_argument_imported_only_for_type_checking_declares_nothing() -> None: + assert annotated_response(type_checking_only, "GET /type-checking/") is None + + +def test_method_holding_less_indented_string() -> None: + annotated = annotated_response(Handlers().get, "GET /handlers/") + assert annotated is not None + assert annotated.types == (AnnotatedItem,) + + +class InferredNamed(TypedDict): + id: str + + +class InferredReused(TypedDict): + id: str + + +class InferredClash(TypedDict): + id: str + + +def test_inferred_serializer_names() -> None: + assert inferred_response_serializer(InferredNamed).__name__ == "InferredNamed" + assert inferred_response_serializer(list[InferredNamed]).__name__ == "ListInferredNamedResponse" + assert ( + inferred_response_serializer(dict[str, InferredNamed]).__name__ + == "DictInferredNamedResponse" + ) + assert ( + inferred_response_serializer(InferredNamed | AnnotatedOther).__name__ + == "InferredNamedOrAnnotatedOtherResponse" + ) + + +def test_inferred_serializer_reuses_declared_one() -> None: + declared = inline_sentry_response_serializer("ListInferredReusedResponse", list[InferredReused]) + + assert inferred_response_serializer(list[InferredReused]) is declared + + +def test_inferred_serializer_avoids_a_clashing_name() -> None: + inline_sentry_response_serializer("InferredClash", list[InferredClash]) + + inferred = inferred_response_serializer(InferredClash) + + assert inferred.__name__ == "InferredClash2" + assert inferred.typeSchema is InferredClash # type: ignore[attr-defined] diff --git a/tests/sentry/apidocs/test_extensions.py b/tests/sentry/apidocs/test_extensions.py index 1bb3dc6eb027..7194b0645da4 100644 --- a/tests/sentry/apidocs/test_extensions.py +++ b/tests/sentry/apidocs/test_extensions.py @@ -1,12 +1,14 @@ from __future__ import annotations -from collections.abc import Mapping +from collections.abc import Iterator, Mapping, MutableSequence, Sequence from enum import Enum +from types import SimpleNamespace from typing import Any, Literal, TypedDict import pytest from django.http import HttpRequest from drf_spectacular.openapi import AutoSchema +from drf_spectacular.plumbing import UnableToProceedError from drf_spectacular.utils import extend_schema, extend_schema_serializer from jsonschema.exceptions import ValidationError from openapi_schema_validator import OAS30Validator @@ -18,8 +20,14 @@ RestrictedJsonFieldExtension, SentryInlineResponseSerializerExtension, SentryResponseSerializerExtension, + TokenAuthExtension, +) +from sentry.apidocs.internal.generator import INTERNAL_TYPE_HINTS +from sentry.apidocs.spectacular_ports import ( + UNRESOLVED_EXTENSION, + resolve_type_hint, + type_hint_policy, ) -from sentry.apidocs.spectacular_ports import resolve_type_hint from sentry.apidocs.utils import inline_sentry_response_serializer from sentry.investigations.endpoints.organization_investigation_block_execution_details import ( OrganizationInvestigationBlockExecutionDetailsEndpoint, @@ -51,6 +59,25 @@ class NestedDict(TypedDict): zz: str +class UnionSuccess(TypedDict): + status: Literal["success"] + url: str + + +class UnionFailure(TypedDict): + status: Literal["failure"] + reason: str + + +class PlainClassResponse: + value: str + + +class LenientResponse(TypedDict): + resolved: str + unresolved: PlainClassResponse + + class BasicSerializerOptional(TypedDict, total=False): a: int @@ -280,3 +307,131 @@ def post(self, request: HttpRequest) -> None: validator = OAS30Validator({**request_schema, "components": schema["components"]}) assert validator.is_valid(payload) is valid assert serializer(data=payload).is_valid() is valid + + +@pytest.fixture +def internal_type_hints() -> Iterator[None]: + with type_hint_policy(INTERNAL_TYPE_HINTS): + yield + + +class ScopedPermission: + scope_map = {"GET": ["org:read", "project:read"]} + + +class ScopelessPermission: + pass + + +def test_token_auth_tolerates_permissions_without_scope_map() -> None: + auto_schema = SimpleNamespace( + method="GET", + view=SimpleNamespace(get_permissions=lambda: [ScopelessPermission(), ScopedPermission()]), + ) + + requirement = TokenAuthExtension(None).get_security_requirement(auto_schema) # type: ignore[arg-type] + + assert requirement == {"auth_token": ["org:read", "project:read"]} + + +@pytest.mark.parametrize( + "hint", + [Sequence[int], MutableSequence[int]], +) +def test_abstract_sequences_resolve_as_arrays(hint: Any) -> None: + assert resolve_type_hint(hint) == {"type": "array", "items": {"type": "integer"}} + + +def test_abstract_mappings_resolve_as_objects() -> None: + assert resolve_type_hint(Mapping[str, int]) == { + "type": "object", + "additionalProperties": {"type": "integer"}, + } + + +def test_public_build_leaves_typed_dicts_open() -> None: + assert "additionalProperties" not in resolve_type_hint(NestedDict) + + +@pytest.mark.usefixtures("internal_type_hints") +def test_internal_build_closes_typed_dicts() -> None: + assert resolve_type_hint(NestedDict) == { + "type": "object", + "properties": {"zz": {"type": "string"}}, + "required": ["zz"], + "additionalProperties": False, + } + + +def test_public_build_fails_on_unresolvable_field() -> None: + serializer = inline_sentry_response_serializer("LenientResponse", LenientResponse) + + with pytest.raises(UnableToProceedError): + SentryInlineResponseSerializerExtension(serializer).map_serializer(AutoSchema(), "response") + + +@pytest.mark.usefixtures("internal_type_hints") +def test_internal_build_marks_unresolvable_field() -> None: + serializer = inline_sentry_response_serializer("LenientResponse", LenientResponse) + + schema = SentryInlineResponseSerializerExtension(serializer).map_serializer( + AutoSchema(), "response" + ) + + assert schema["properties"]["resolved"] == {"type": "string"} + assert UNRESOLVED_EXTENSION in schema["properties"]["unresolved"] + + +@pytest.mark.parametrize( + "value, valid", + [ + (None, True), + ({"status": "success", "url": "https://example.com"}, True), + ({"status": "failure", "reason": "unavailable"}, True), + ({}, False), + ({"unexpected": True}, False), + ({"status": "success", "reason": "unavailable"}, False), + ({"status": "failure", "reason": "unavailable", "extra": True}, False), + ("success", False), + ], +) +@pytest.mark.usefixtures("internal_type_hints") +def test_internal_nullable_union(value: Any, valid: bool) -> None: + serializer = inline_sentry_response_serializer( + "NullableUnion", UnionSuccess | UnionFailure | None + ) + schema = SentryInlineResponseSerializerExtension(serializer).map_serializer( + AutoSchema(), "response" + ) + + assert schema["anyOf"][-1] == {"type": "object", "nullable": True, "enum": [None]} + OAS30Validator.check_schema(schema) + assert OAS30Validator(schema).is_valid(value) is valid + + +@pytest.mark.usefixtures("internal_type_hints") +def test_internal_nullable_numeric_union() -> None: + serializer = inline_sentry_response_serializer("NullableNumber", int | float | None) + schema = SentryInlineResponseSerializerExtension(serializer).map_serializer( + AutoSchema(), "response" + ) + + OAS30Validator.check_schema(schema) + validator = OAS30Validator(schema) + assert validator.is_valid(None) + assert validator.is_valid(1) + assert validator.is_valid(1.5) + assert not validator.is_valid({}) + + +@pytest.mark.usefixtures("internal_type_hints") +def test_internal_nullable_dictionary() -> None: + serializer = inline_sentry_response_serializer("NullableDictionary", dict[str, Any] | None) + schema = SentryInlineResponseSerializerExtension(serializer).map_serializer( + AutoSchema(), "response" + ) + + validator = OAS30Validator(schema) + assert validator.is_valid(None) + assert validator.is_valid({"arbitrary": True}) + assert "enum" not in schema From ba526f9bd00fc66f963d5f5c72b516c64cef9fc6 Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Mon, 2 Nov 2026 16:17:04 -0500 Subject: [PATCH 2/7] feat(apidocs): Stamp permission classes on internal spec operations Consumers that filter operations by the permissions guarding them need the class names, which only the view classes have. Each operation the internal build admits now carries its view's permission_classes as x-sentry-permissions, with ones composed by &, | and ~ unwrapped to the classes they compose. --- src/sentry/apidocs/internal/__init__.py | 7 +- src/sentry/apidocs/internal/generator.py | 51 +++++++++++--- .../sentry/apidocs/internal/test_generator.py | 67 ++++++++++++++++--- 3 files changed, 104 insertions(+), 21 deletions(-) diff --git a/src/sentry/apidocs/internal/__init__.py b/src/sentry/apidocs/internal/__init__.py index f704585c1e02..38493f3634f2 100644 --- a/src/sentry/apidocs/internal/__init__.py +++ b/src/sentry/apidocs/internal/__init__.py @@ -13,8 +13,9 @@ described, since the backend sends them. With no omissions, it builds once, without the published build's second build checking them. - Every method is admitted, whatever its publish status, and stamped with it as - `x-sentry-publish-status`. The published reference's own checks (tags, - descriptions, unique summaries) skip unpublished operations. + `x-sentry-publish-status`, and with the permission classes guarding it as + `x-sentry-permissions`. The published reference's own checks (tags, descriptions, + unique summaries) skip unpublished operations. - A literal alternation in a route, such as `(?:issues|groups)`, is spelled as its first branch rather than `{var}`, so routes that differ only there stay apart. Every path such a route answers on is listed under `x-sentry-path-aliases`. @@ -31,6 +32,7 @@ from sentry.apidocs.internal.generator import ( PATH_ALIASES_EXTENSION, + PERMISSIONS_EXTENSION, PUBLISH_STATUS_EXTENSION, InternalGenerator, ) @@ -40,6 +42,7 @@ __all__ = ( "InternalGenerator", "PATH_ALIASES_EXTENSION", + "PERMISSIONS_EXTENSION", "PUBLISH_STATUS_EXTENSION", "UNDOCUMENTED_EXTENSION", "UNRESOLVED_EXTENSION", diff --git a/src/sentry/apidocs/internal/generator.py b/src/sentry/apidocs/internal/generator.py index b1fb2a88176a..6de2815b4d7b 100644 --- a/src/sentry/apidocs/internal/generator.py +++ b/src/sentry/apidocs/internal/generator.py @@ -12,6 +12,7 @@ from drf_spectacular.generators import SchemaGenerator from drf_spectacular.openapi import AutoSchema from drf_spectacular.settings import spectacular_settings +from rest_framework.permissions import OperandHolder, SingleOperandHolder from sentry.api.api_publish_status import ApiPublishStatus @@ -29,6 +30,9 @@ PUBLISH_STATUS_EXTENSION = "x-sentry-publish-status" PATH_ALIASES_EXTENSION = "x-sentry-path-aliases" +# The class names in the view's `permission_classes`. Absent from `APPEND_PATHS` +# operations, which have no view to read them from. +PERMISSIONS_EXTENSION = "x-sentry-permissions" INTERNAL_TYPE_HINTS = TypeHintPolicy( closed_typed_dicts=True, @@ -47,6 +51,14 @@ _ALTERNATION = re.compile(r"\(\?:([a-z-]+(?:\|[a-z-]+)+)\)") +@dataclass(frozen=True) +class _Admitted: + """What preprocessing reads off an admitted operation's view class.""" + + status: ApiPublishStatus + permissions: tuple[str, ...] + + @dataclass class _InternalBuild: # Each path spelled from a literal alternation, keyed by the spelling kept, listing @@ -54,8 +66,8 @@ class _InternalBuild: path_aliases: dict[str, list[str]] = field(default_factory=dict) # The same paths as the published build spells them, with `{var}` for an alternation. public_spellings: dict[str, str] = field(default_factory=dict) - # The publish status of each admitted operation, keyed by (path, lowercased method). - publish_status: dict[tuple[str, str], ApiPublishStatus] = field(default_factory=dict) + # Each admitted operation, keyed by (path, lowercased method). + admitted: dict[tuple[str, str], _Admitted] = field(default_factory=dict) _current: ContextVar[_InternalBuild] = ContextVar("internal_build") @@ -139,23 +151,46 @@ def internal_preprocessing_hook(endpoints: Any) -> Any: admit=lambda status: True, public_path=lambda path: build.public_spellings.get(path, path), ) - build.publish_status = { - (path, method.lower()): callback.view_class.publish_status[method] + build.admitted = { + (path, method.lower()): _Admitted( + status=callback.view_class.publish_status[method], + permissions=_permission_class_names(callback.view_class), + ) for path, _path_regex, method, callback in admitted } return admitted +def _permission_class_names(view_class: type) -> tuple[str, ...]: + names: set[str] = set() + for permission in getattr(view_class, "permission_classes", ()): + names |= _composed_names(permission) + return tuple(sorted(names)) + + +def _composed_names(permission: Any) -> set[str]: + """The classes a `permission_classes` entry guards on. + + DRF composes permissions with `&`, `|` and `~` into holders rather than classes, and + naming a holder would lose the classes it composes.""" + if isinstance(permission, OperandHolder): + return _composed_names(permission.op1_class) | _composed_names(permission.op2_class) + if isinstance(permission, SingleOperandHolder): + return _composed_names(permission.op1_class) + return {permission.__name__} + + def internal_postprocessing_hook(result: Any, generator: Any, **kwargs: Any) -> Any: - """Stamp each operation's publish status and aliases, then postprocess as published.""" + """Stamp what preprocessing read and each path's aliases, then postprocess as published.""" build = _build() # Before the published build's postprocessing, which can rewrite path keys. for path, operations in result["paths"].items(): aliases = build.path_aliases.get(path) for method, operation in operations.items(): - status = build.publish_status.get((path, method)) - if status is not None: - operation[PUBLISH_STATUS_EXTENSION] = status.value + admitted = build.admitted.get((path, method)) + if admitted is not None: + operation[PUBLISH_STATUS_EXTENSION] = admitted.status.value + operation[PERMISSIONS_EXTENSION] = list(admitted.permissions) if aliases is not None: operation[PATH_ALIASES_EXTENSION] = list(aliases) return custom_postprocessing_hook(result, generator, in_reference=_in_reference, **kwargs) diff --git a/tests/sentry/apidocs/internal/test_generator.py b/tests/sentry/apidocs/internal/test_generator.py index 30577031c911..65820d13d391 100644 --- a/tests/sentry/apidocs/internal/test_generator.py +++ b/tests/sentry/apidocs/internal/test_generator.py @@ -11,6 +11,7 @@ from drf_spectacular.utils import extend_schema from drf_spectacular.validation import validate_schema from rest_framework import serializers +from rest_framework.permissions import BasePermission from rest_framework.request import Request from rest_framework.response import Response @@ -25,12 +26,14 @@ ) from sentry.apidocs.internal import ( PATH_ALIASES_EXTENSION, + PERMISSIONS_EXTENSION, PUBLISH_STATUS_EXTENSION, UNDOCUMENTED_EXTENSION, InternalGenerator, ) from sentry.apidocs.internal.generator import ( InternalEndpointEnumerator, + _Admitted, _build, _internal_build, internal_postprocessing_hook, @@ -49,11 +52,30 @@ PREFIX = "/api/0/organizations/{organization_id_or_slug}" -def _endpoint(path: str, status: ApiPublishStatus) -> tuple[Any, Any, str, Any]: +class Scoped(BasePermission): + pass + + +class Impersonated(BasePermission): + pass + + +class Staff(BasePermission): + pass + + +def _endpoint( + path: str, status: ApiPublishStatus, permission_classes: tuple[Any, ...] = () +) -> tuple[Any, Any, str, Any]: view_class = type( "FakeEndpoint", (), - {"owner": ApiOwner.CRONS, "publish_status": {"GET": status}, "servers": None}, + { + "owner": ApiOwner.CRONS, + "publish_status": {"GET": status}, + "servers": None, + "permission_classes": permission_classes, + }, ) callback = type("FakeCallback", (), {"view_class": view_class}) return (path, path, "GET", callback) @@ -128,7 +150,7 @@ def test_published_build_admits_published_methods(self, _write: Any) -> None: def test_admits_every_method_and_records_its_status(self, _write: Any) -> None: with _internal_build(): filtered = internal_preprocessing_hook(ENDPOINTS) - publish_status = _build().publish_status + publish_status = {key: admitted.status for key, admitted in _build().admitted.items()} assert [path for path, *_ in filtered] == [ "/api/0/public/", @@ -156,6 +178,17 @@ def test_excludes_legacy_issue_routes_by_public_spelling(self, _write: Any) -> N assert [path for path, *_ in filtered] == [f"{PREFIX}/issues/{{issue_id}}/"] + def test_records_the_classes_composed_permissions_guard_on(self, _write: Any) -> None: + endpoint = _endpoint( + "/api/0/tokens/", ApiPublishStatus.PRIVATE, (Scoped & ~Impersonated, Staff | Scoped) + ) + + with _internal_build(): + internal_preprocessing_hook([endpoint]) + admitted = _build().admitted[("/api/0/tokens/", "get")] + + assert admitted.permissions == ("Impersonated", "Scoped", "Staff") + class TestPostprocessing: def test_stamps_status_and_skips_reference_checks_on_unpublished(self) -> None: @@ -169,14 +202,23 @@ def test_stamps_status_and_skips_reference_checks_on_unpublished(self) -> None: } with _internal_build(): - _build().publish_status = { - ("/api/0/public/", "get"): ApiPublishStatus.PUBLIC, - ("/api/0/private/", "get"): ApiPublishStatus.PRIVATE, + _build().admitted = { + ("/api/0/public/", "get"): _Admitted(ApiPublishStatus.PUBLIC, ("Scoped",)), + ("/api/0/private/", "get"): _Admitted(ApiPublishStatus.PRIVATE, ()), } processed = internal_postprocessing_hook(result, None) - assert processed["paths"]["/api/0/public/"]["get"][PUBLISH_STATUS_EXTENSION] == "public" - assert processed["paths"]["/api/0/private/"]["get"][PUBLISH_STATUS_EXTENSION] == "private" + public = processed["paths"]["/api/0/public/"]["get"] + private = processed["paths"]["/api/0/private/"]["get"] + assert (public[PUBLISH_STATUS_EXTENSION], public[PERMISSIONS_EXTENSION]) == ( + "public", + ["Scoped"], + ) + # An empty list records that the view declares none, unlike an absent extension. + assert (private[PUBLISH_STATUS_EXTENSION], private[PERMISSIONS_EXTENSION]) == ( + "private", + [], + ) def test_still_checks_published_operations(self) -> None: result = { @@ -185,7 +227,7 @@ def test_still_checks_published_operations(self) -> None: } with _internal_build(), pytest.raises(SentryApiBuildError): - _build().publish_status = {("/api/0/public/", "get"): ApiPublishStatus.PUBLIC} + _build().admitted = {("/api/0/public/", "get"): _Admitted(ApiPublishStatus.PUBLIC, ())} internal_postprocessing_hook(result, None) def test_stamps_path_aliases(self) -> None: @@ -198,7 +240,7 @@ def test_stamps_path_aliases(self) -> None: with _internal_build(): _build().path_aliases[notes] = [notes, comments] - _build().publish_status = {(notes, "get"): ApiPublishStatus.PRIVATE} + _build().admitted = {(notes, "get"): _Admitted(ApiPublishStatus.PRIVATE, ())} processed = internal_postprocessing_hook(result, None) assert processed["paths"][notes]["get"][PATH_ALIASES_EXTENSION] == [notes, comments] @@ -212,6 +254,7 @@ def test_published_build_stamps_nothing(self) -> None: operation = custom_postprocessing_hook(result, None)["paths"]["/api/0/public/"]["get"] assert PUBLISH_STATUS_EXTENSION not in operation + assert PERMISSIONS_EXTENSION not in operation assert PATH_ALIASES_EXTENSION not in operation @@ -253,7 +296,7 @@ class BoxPatchSerializer(serializers.Serializer[Any]): class BoxEndpoint(Endpoint): owner = ApiOwner.CRONS - permission_classes = () + permission_classes = (Scoped,) publish_status = { "GET": ApiPublishStatus.PRIVATE, "PUT": ApiPublishStatus.PUBLIC, @@ -310,6 +353,7 @@ def test_internal_build_end_to_end(_write: Any) -> None: "delete": "private", } assert operations["get"][PATH_ALIASES_EXTENSION] == ["/api/0/box/items/", "/api/0/box/things/"] + assert operations["get"][PERMISSIONS_EXTENSION] == ["Scoped"] # Read from the annotation, and closed: a TypedDict lists every key it carries. assert _json_schema(operations["get"], "200") == {"$ref": "#/components/schemas/BoxItem"} assert schema["components"]["schemas"]["BoxItem"] == { @@ -347,6 +391,7 @@ def test_published_build_of_the_same_routes_is_unchanged(_write: Any) -> None: (operations,) = schema["paths"].values() assert list(operations) == ["put"] assert PUBLISH_STATUS_EXTENSION not in operations["put"] + assert PERMISSIONS_EXTENSION not in operations["put"] declared_item = schema["components"]["schemas"]["BoxDeclaredItem"] assert "additionalProperties" not in declared_item assert declared_item["properties"].keys() == {"name"} From 2495526a4adb7a1a402eca31bb3624f214d40039 Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Tue, 3 Nov 2026 15:53:00 -0500 Subject: [PATCH 3/7] ref(apidocs): Note how the replacement internal schema is bound to its view DRF's ViewInspector.__set__ binds an assigned schema to the view instance, the same way drf-spectacular's create_view swaps in @extend_schema classes. Co-Authored-By: Claude Opus 5.5 --- src/sentry/apidocs/internal/generator.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/sentry/apidocs/internal/generator.py b/src/sentry/apidocs/internal/generator.py index 6de2815b4d7b..1012d9c58fff 100644 --- a/src/sentry/apidocs/internal/generator.py +++ b/src/sentry/apidocs/internal/generator.py @@ -139,6 +139,8 @@ def create_view(self, callback: Any, method: str, request: Any = None) -> Any: if isinstance(schema, AutoSchema): schema_class = internal_schema_class(type(schema)) if schema_class is not type(schema): + # Assigning through DRF's `ViewInspector` descriptor binds the new schema + # to `view`, as drf-spectacular's own `create_view` relies on. view.schema = schema_class() return view From 635abeebda00d7649495e0be9c6afa9480efb827 Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Tue, 3 Nov 2026 15:53:25 -0500 Subject: [PATCH 4/7] fix(apidocs): Drop the issue var parameter from internal spec operations Issue endpoints declare IssueParams.ISSUES_OR_GROUPS, a `var` path parameter for the published build's `{var}` spelling of (?:issues|groups), which the published postprocessing drops along with the `{var}`. The internal build spells the path `issues/` and kept the parameter, so 16 issue operations declared a required `var` that is not in their path. Co-Authored-By: Claude Opus 5.5 --- src/sentry/apidocs/internal/generator.py | 17 +++++++++++++- .../sentry/apidocs/internal/test_generator.py | 22 +++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/src/sentry/apidocs/internal/generator.py b/src/sentry/apidocs/internal/generator.py index 1012d9c58fff..9b77e55bfc4a 100644 --- a/src/sentry/apidocs/internal/generator.py +++ b/src/sentry/apidocs/internal/generator.py @@ -49,6 +49,7 @@ # Literal alternations such as `(?:issues|groups)` or `(?:user-feedback|user-reports)`. _ALTERNATION = re.compile(r"\(\?:([a-z-]+(?:\|[a-z-]+)+)\)") +_VARIABLE = re.compile(r"\{([^}]+)\}") @dataclass(frozen=True) @@ -185,9 +186,13 @@ def _composed_names(permission: Any) -> set[str]: def internal_postprocessing_hook(result: Any, generator: Any, **kwargs: Any) -> Any: """Stamp what preprocessing read and each path's aliases, then postprocess as published.""" build = _build() + paths = result["paths"] # Before the published build's postprocessing, which can rewrite path keys. - for path, operations in result["paths"].items(): + for path, operations in paths.items(): aliases = build.path_aliases.get(path) + # Issue endpoints declare a `var` parameter for the published build's `{var}` + # spelling of `(?:issues|groups)`, which it drops along with the `{var}`. + spelled_out = _variables(build.public_spellings.get(path, path)) - _variables(path) for method, operation in operations.items(): admitted = build.admitted.get((path, method)) if admitted is not None: @@ -195,9 +200,19 @@ def internal_postprocessing_hook(result: Any, generator: Any, **kwargs: Any) -> operation[PERMISSIONS_EXTENSION] = list(admitted.permissions) if aliases is not None: operation[PATH_ALIASES_EXTENSION] = list(aliases) + if spelled_out and "parameters" in operation: + operation["parameters"] = [ + parameter + for parameter in operation["parameters"] + if not (parameter.get("in") == "path" and parameter.get("name") in spelled_out) + ] return custom_postprocessing_hook(result, generator, in_reference=_in_reference, **kwargs) +def _variables(path: str) -> set[str]: + return set(_VARIABLE.findall(path)) + + def _in_reference(operation: Mapping[str, Any]) -> bool: """Whether the published reference would carry the operation. diff --git a/tests/sentry/apidocs/internal/test_generator.py b/tests/sentry/apidocs/internal/test_generator.py index 65820d13d391..f2accc8c7876 100644 --- a/tests/sentry/apidocs/internal/test_generator.py +++ b/tests/sentry/apidocs/internal/test_generator.py @@ -245,6 +245,28 @@ def test_stamps_path_aliases(self) -> None: assert processed["paths"][notes]["get"][PATH_ALIASES_EXTENSION] == [notes, comments] + def test_drops_the_parameter_for_a_spelled_out_alternation(self) -> None: + # Issue endpoints declare `var`, the published build's `{var}` for `(?:issues|groups)`. + path = f"{PREFIX}/issues/{{issue_id}}/" + var = {"in": "path", "name": "var", "required": True, "schema": {"type": "string"}} + issue_id = { + "in": "path", + "name": "issue_id", + "required": True, + "schema": {"type": "string"}, + } + result = { + "components": {"schemas": {}}, + "paths": {path: {"get": {"operationId": "issue", "parameters": [var, issue_id]}}}, + } + + with _internal_build(): + _build().public_spellings[path] = f"{PREFIX}/{{var}}/{{issue_id}}/" + _build().admitted = {(path, "get"): _Admitted(ApiPublishStatus.PRIVATE, ())} + processed = internal_postprocessing_hook(result, None) + + assert processed["paths"][path]["get"]["parameters"] == [issue_id] + def test_published_build_stamps_nothing(self) -> None: result = { "components": {"schemas": {}}, From 66955e74cd38ea44cd7ab377566473a693a9b936 Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Tue, 3 Nov 2026 15:53:45 -0500 Subject: [PATCH 5/7] fix(apidocs): Keep view methods beside hand-written APPEND_PATHS operations drf-spectacular appends APPEND_PATHS over whole path items, so a hand-written path item dropped every method its view serves that the item does not describe. The published build only describes the hand-written ones anyway; the internal one dropped POST on organization sentry-app-installations. The internal build now appends them method by method, hand-written still winning. Co-Authored-By: Claude Opus 5.5 --- src/sentry/apidocs/internal/generator.py | 10 ++- .../sentry/apidocs/internal/test_generator.py | 68 ++++++++++++++----- 2 files changed, 61 insertions(+), 17 deletions(-) diff --git a/src/sentry/apidocs/internal/generator.py b/src/sentry/apidocs/internal/generator.py index 9b77e55bfc4a..ca6ba6ec4cb4 100644 --- a/src/sentry/apidocs/internal/generator.py +++ b/src/sentry/apidocs/internal/generator.py @@ -1,6 +1,7 @@ from __future__ import annotations import contextlib +import copy import itertools import re from collections.abc import Callable, Iterator, Mapping, Sequence @@ -69,6 +70,8 @@ class _InternalBuild: public_spellings: dict[str, str] = field(default_factory=dict) # Each admitted operation, keyed by (path, lowercased method). admitted: dict[tuple[str, str], _Admitted] = field(default_factory=dict) + # The `APPEND_PATHS` setting, which postprocessing appends in drf-spectacular's place. + append_paths: Mapping[str, Mapping[str, Any]] = field(default_factory=dict) _current: ContextVar[_InternalBuild] = ContextVar("internal_build") @@ -187,6 +190,10 @@ def internal_postprocessing_hook(result: Any, generator: Any, **kwargs: Any) -> """Stamp what preprocessing read and each path's aliases, then postprocess as published.""" build = _build() paths = result["paths"] + # drf-spectacular appends `APPEND_PATHS` over whole path items, which drops the methods + # a view serves beyond the hand-written ones. The hand-written ones still win. + for path, operations in build.append_paths.items(): + paths.setdefault(path, {}).update(copy.deepcopy(operations)) # Before the published build's postprocessing, which can rewrite path keys. for path, operations in paths.items(): aliases = build.path_aliases.get(path) @@ -223,12 +230,13 @@ def _in_reference(operation: Mapping[str, Any]) -> bool: @contextlib.contextmanager def _internal_build() -> Iterator[None]: - token = _current.set(_InternalBuild()) + token = _current.set(_InternalBuild(append_paths=spectacular_settings.APPEND_PATHS)) try: with ( _spectacular_settings( TITLE=_TITLE, DESCRIPTION=_DESCRIPTION, + APPEND_PATHS={}, PREPROCESSING_HOOKS=_swapped( spectacular_settings.PREPROCESSING_HOOKS, custom_preprocessing_hook, diff --git a/tests/sentry/apidocs/internal/test_generator.py b/tests/sentry/apidocs/internal/test_generator.py index f2accc8c7876..c3cc7357cdf5 100644 --- a/tests/sentry/apidocs/internal/test_generator.py +++ b/tests/sentry/apidocs/internal/test_generator.py @@ -267,6 +267,35 @@ def test_drops_the_parameter_for_a_spelled_out_alternation(self) -> None: assert processed["paths"][path]["get"]["parameters"] == [issue_id] + def test_appends_hand_written_operations_beside_the_views_others(self) -> None: + path = "/api/0/installations/" + hand_written = _documented_operation("hand-written") + result = { + "components": {"schemas": {}}, + "paths": { + path: { + "get": {"operationId": "generated", "parameters": []}, + "post": {"operationId": "create", "parameters": []}, + } + }, + } + + with _internal_build(): + _build().append_paths = {path: {"get": hand_written}} + _build().admitted = { + (path, "get"): _Admitted(ApiPublishStatus.PRIVATE, ()), + (path, "post"): _Admitted(ApiPublishStatus.PRIVATE, ()), + } + processed = internal_postprocessing_hook(result, None) + + operations = processed["paths"][path] + assert {method: op["operationId"] for method, op in operations.items()} == { + "get": "hand-written", + "post": "create", + } + # Stamped on a copy, not on the operation in the settings. + assert PUBLISH_STATUS_EXTENSION not in hand_written + def test_published_build_stamps_nothing(self) -> None: result = { "components": {"schemas": {}}, @@ -281,23 +310,30 @@ def test_published_build_stamps_nothing(self) -> None: def test_build_swaps_hooks_and_restores_settings() -> None: - before = ( - list(spectacular_settings.PREPROCESSING_HOOKS), - list(spectacular_settings.POSTPROCESSING_HOOKS), - spectacular_settings.TITLE, - ) + append_paths = {"/api/0/hand-written/": {"get": _documented_operation("hand-written")}} + with mock.patch.object(spectacular_settings, "APPEND_PATHS", append_paths): + before = ( + list(spectacular_settings.PREPROCESSING_HOOKS), + list(spectacular_settings.POSTPROCESSING_HOOKS), + spectacular_settings.TITLE, + spectacular_settings.APPEND_PATHS, + ) + + with _internal_build(): + assert spectacular_settings.PREPROCESSING_HOOKS[0] is internal_preprocessing_hook + assert internal_postprocessing_hook in spectacular_settings.POSTPROCESSING_HOOKS + assert custom_preprocessing_hook not in spectacular_settings.PREPROCESSING_HOOKS + assert custom_postprocessing_hook not in spectacular_settings.POSTPROCESSING_HOOKS + # Appended by postprocessing instead, method by method. + assert spectacular_settings.APPEND_PATHS == {} + assert _build().append_paths is append_paths - with _internal_build(): - assert spectacular_settings.PREPROCESSING_HOOKS[0] is internal_preprocessing_hook - assert internal_postprocessing_hook in spectacular_settings.POSTPROCESSING_HOOKS - assert custom_preprocessing_hook not in spectacular_settings.PREPROCESSING_HOOKS - assert custom_postprocessing_hook not in spectacular_settings.POSTPROCESSING_HOOKS - - assert ( - list(spectacular_settings.PREPROCESSING_HOOKS), - list(spectacular_settings.POSTPROCESSING_HOOKS), - spectacular_settings.TITLE, - ) == before + assert ( + list(spectacular_settings.PREPROCESSING_HOOKS), + list(spectacular_settings.POSTPROCESSING_HOOKS), + spectacular_settings.TITLE, + spectacular_settings.APPEND_PATHS, + ) == before class BoxItem(TypedDict): From abe70c042771bd996665e54351baf4ffd75ac963 Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Tue, 3 Nov 2026 16:13:38 -0500 Subject: [PATCH 6/7] fix(apidocs): Keep a published method's generated operation over a hand-written one The published build appends APPEND_PATHS, then _fix_issue_paths moves the generated issue operations onto the hand-written paths, so the published reference describes GET issues/{issue_id}/tags/{key}/values/ with its generated operation. The internal build spells that path issues/ up front, and the hand-written operation won instead, with another operationId, parameters and response schema. A published method's generated operation now wins here too; hand-written ones still stand in for the rest. Co-Authored-By: Claude Opus 5.5 --- src/sentry/apidocs/internal/generator.py | 11 +++++++++-- tests/sentry/apidocs/internal/test_generator.py | 15 +++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/src/sentry/apidocs/internal/generator.py b/src/sentry/apidocs/internal/generator.py index ca6ba6ec4cb4..a9557f05034c 100644 --- a/src/sentry/apidocs/internal/generator.py +++ b/src/sentry/apidocs/internal/generator.py @@ -191,9 +191,16 @@ def internal_postprocessing_hook(result: Any, generator: Any, **kwargs: Any) -> build = _build() paths = result["paths"] # drf-spectacular appends `APPEND_PATHS` over whole path items, which drops the methods - # a view serves beyond the hand-written ones. The hand-written ones still win. + # a view serves beyond the hand-written ones. Those win, except over the generated + # operation of a published method: the published build's `_fix_issue_paths` moves that + # one over them, as for issue tag values, so the published reference describes it. for path, operations in build.append_paths.items(): - paths.setdefault(path, {}).update(copy.deepcopy(operations)) + generated = paths.setdefault(path, {}) + for method, operation in operations.items(): + admitted = build.admitted.get((path, method)) + if method in generated and admitted is not None and admitted.status.is_published: + continue + generated[method] = copy.deepcopy(operation) # Before the published build's postprocessing, which can rewrite path keys. for path, operations in paths.items(): aliases = build.path_aliases.get(path) diff --git a/tests/sentry/apidocs/internal/test_generator.py b/tests/sentry/apidocs/internal/test_generator.py index c3cc7357cdf5..0c3c141d7fd4 100644 --- a/tests/sentry/apidocs/internal/test_generator.py +++ b/tests/sentry/apidocs/internal/test_generator.py @@ -296,6 +296,21 @@ def test_appends_hand_written_operations_beside_the_views_others(self) -> None: # Stamped on a copy, not on the operation in the settings. assert PUBLISH_STATUS_EXTENSION not in hand_written + def test_a_published_methods_generated_operation_wins_over_a_hand_written_one(self) -> None: + # As the published build's `_fix_issue_paths` moves it over the hand-written one. + path = f"{PREFIX}/issues/{{issue_id}}/tags/{{key}}/values/" + result = { + "components": {"schemas": {}}, + "paths": {path: {"get": _documented_operation("generated")}}, + } + + with _internal_build(): + _build().append_paths = {path: {"get": _documented_operation("hand-written")}} + _build().admitted = {(path, "get"): _Admitted(ApiPublishStatus.PUBLIC, ())} + processed = internal_postprocessing_hook(result, None) + + assert processed["paths"][path]["get"]["operationId"] == "generated" + def test_published_build_stamps_nothing(self) -> None: result = { "components": {"schemas": {}}, From 1aa8b7124152b585aa0b147b43ab5bf01ba15df0 Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Wed, 4 Nov 2026 11:38:03 -0500 Subject: [PATCH 7/7] ref(apidocs): Drop the hooks' no-op default callbacks `_in_reference` always returned True and `_public_spelling` returned its argument; each existed only as the published build's default. Default `in_reference` and `public_path` to None instead, which leaves the published build's behavior unchanged. Co-Authored-By: Claude Opus 5.5 --- src/sentry/apidocs/hooks.py | 33 ++++++++++++++------------------- 1 file changed, 14 insertions(+), 19 deletions(-) diff --git a/src/sentry/apidocs/hooks.py b/src/sentry/apidocs/hooks.py index f80916f2d9c1..f3201949dd5b 100644 --- a/src/sentry/apidocs/hooks.py +++ b/src/sentry/apidocs/hooks.py @@ -198,21 +198,17 @@ def _is_published(status: ApiPublishStatus) -> bool: return status.is_published -def _public_spelling(path: str) -> str: - return path - - def custom_preprocessing_hook( # TODO: organize method, rename endpoints: Any, *, admit: Callable[[ApiPublishStatus], bool] = _is_published, - public_path: Callable[[str], str] = _public_spelling, + public_path: Callable[[str], str] | None = None, ) -> Any: """Check every endpoint, collect its metadata, and keep the methods `admit` lets in. - `public_path` gives the spelling of a path that the exclusions and the ownership - allowlist are written against. A build that spells paths otherwise, such as the - internal one (`sentry.apidocs.internal`), supplies both.""" + The exclusions and the ownership allowlist are written against the published build's + spelling of each path. A build that spells paths otherwise, such as the internal one + (`sentry.apidocs.internal`), passes `public_path` to give that spelling.""" _ENDPOINT_SERVERS.clear() _EXPERIMENTAL_OPERATIONS.clear() _OPERATION_SILOS.clear() @@ -221,6 +217,8 @@ def custom_preprocessing_hook( # TODO: organize method, rename filtered = [] ownership_data: dict[ApiOwner, dict[ApiPublishStatus, set[str]]] = {} for path, path_regex, method, callback in endpoints: + spelling = path if public_path is None else public_path(path) + # Collect servers from endpoint class for postprocessing endpoint_servers = getattr(callback.view_class, "servers", None) if endpoint_servers is not None: @@ -237,7 +235,7 @@ def custom_preprocessing_hook( # TODO: organize method, rename # Fail if endpoint is unowned if owner_team == ApiOwner.UNOWNED: - if public_path(path) not in API_OWNERSHIP_ALLOWLIST_DONT_MODIFY: + if spelling not in API_OWNERSHIP_ALLOWLIST_DONT_MODIFY: raise SentryApiBuildError( f"Endpoint {callback.view_class} is missing the attribute owner: ApiOwner. \n" + "If you can't find your team in ApiOwners feel free to add the associated github group. ", @@ -262,7 +260,7 @@ def custom_preprocessing_hook( # TODO: organize method, rename f"Endpoint {callback.view_class} lists safe methods in idempotent_methods: {sorted(idempotent_methods & _SAFE_METHODS)}. Safe methods are always idempotent; only list non-safe methods such as POST.", ) - if any(public_path(path).startswith(p) for p in EXCLUSION_PATH_PREFIXES): + if any(spelling.startswith(p) for p in EXCLUSION_PATH_PREFIXES): pass else: @@ -361,22 +359,19 @@ def _validate_request_body( ) -def _in_reference(operation: Mapping[str, Any]) -> bool: - return True - - def custom_postprocessing_hook( result: dict[str, Any], generator: Any, *, - in_reference: Callable[[Mapping[str, Any]], bool] = _in_reference, + in_reference: Callable[[Mapping[str, Any]], bool] | None = None, **kwargs: Any, ) -> dict[str, Any]: """Stamp operation metadata, then check what the published reference needs. - Operations `in_reference` rejects skip those checks. The published build has none; - the internal one (`sentry.apidocs.internal`) rejects its unpublished operations, - which it describes only for their shapes.""" + The published build describes only what the reference carries, so every operation + gets those checks. A build that describes more, such as the internal one + (`sentry.apidocs.internal`), passes `in_reference` to pick the operations that get + them; it describes its unpublished operations only for their shapes.""" # Stamp before alias canonicalization so metadata stays with its operation. for path, operations in result["paths"].items(): for method, operation in operations.items(): @@ -432,7 +427,7 @@ def custom_postprocessing_hook( endpoint_name = f"'{method_info['operationId']}'" - if not in_reference(method_info): + if in_reference is not None and not in_reference(method_info): continue summary = method_info.get("summary")