From 85124f42830da8dd36cab431e8f14bcb08e0913e Mon Sep 17 00:00:00 2001 From: Jeremy Stanley Date: Mon, 14 Sep 2026 13:03:02 -0700 Subject: [PATCH 1/3] feat(apidocs): Withhold enum values and verify schema omissions Withholding part of the public schema only worked per field, so a choice value had to be hidden by hand-writing a parameter constant that drifted from the serializer it duplicated. A key in omit_from_public_schema can now be `field.choice`. SentrySchema removes that value while mapping the serializer declaring it, covering its component and any query parameters built from it. Nested serializers and TypedDicts declare their own fields; a declaration on any other class is refused, naming the class it belongs on. The docs build then generates the schema again with omissions switched off and compares the two by name. Every difference must come from a declared omission at the place drf-spectacular put it, and every declared omission must show in the final schema, or the build fails. Co-Authored-By: Claude Opus 5 --- src/sentry/apidocs/hooks.py | 28 +- src/sentry/apidocs/omission_apply.py | 187 +++++++ src/sentry/apidocs/omission_guard.py | 424 ++++++++++++++++ src/sentry/apidocs/omission_paths.py | 136 +++++ src/sentry/apidocs/omissions.py | 163 ++++-- src/sentry/apidocs/schema.py | 147 +++++- src/sentry/apidocs/spectacular_ports.py | 44 +- tests/sentry/apidocs/test_omission_guard.py | 332 ++++++++++++ tests/sentry/apidocs/test_omission_paths.py | 533 ++++++++++++++++++++ tests/tools/test_flake8_plugin.py | 59 +++ tools/flake8_plugin.py | 59 ++- 11 files changed, 2040 insertions(+), 72 deletions(-) create mode 100644 src/sentry/apidocs/omission_apply.py create mode 100644 src/sentry/apidocs/omission_guard.py create mode 100644 src/sentry/apidocs/omission_paths.py create mode 100644 tests/sentry/apidocs/test_omission_guard.py create mode 100644 tests/sentry/apidocs/test_omission_paths.py diff --git a/src/sentry/apidocs/hooks.py b/src/sentry/apidocs/hooks.py index 7dee08d23191..0b1a4e03bef7 100644 --- a/src/sentry/apidocs/hooks.py +++ b/src/sentry/apidocs/hooks.py @@ -11,6 +11,13 @@ from sentry.api.api_publish_status import ApiPublishStatus from sentry.apidocs.api_ownership_allowlist_dont_modify import API_OWNERSHIP_ALLOWLIST_DONT_MODIFY from sentry.apidocs.build import OPENAPI_TAGS +from sentry.apidocs.omission_apply import OmissionError +from sentry.apidocs.omission_guard import ( + check_schema_omissions, + omissions_disabled, + omissions_enabled, + recording_omissions, +) from sentry.apidocs.utils import SentryApiBuildError HTTP_METHOD_NAME = Literal[ @@ -103,6 +110,23 @@ def get_path_from_regex(self, path_regex: str) -> str: class CustomGenerator(SchemaGenerator): endpoint_inspector_cls = CustomEndpointEnumerator + def get_schema(self, request: Any = None, public: bool = False) -> Any: + """Build the schema, then prove its omissions changed only what they declare. + + The baseline is a second build, on a fresh registry, with omissions off.""" + baseline_generator = type(self)( + patterns=self.patterns, urlconf=self.urlconf, api_version=self.api_version + ) + with omissions_disabled(): + baseline = SchemaGenerator.get_schema(baseline_generator, request, public) + with recording_omissions() as recorded: + schema = super().get_schema(request, public) + try: + check_schema_omissions(baseline, schema, recorded) + except OmissionError as exc: + raise SentryApiBuildError(str(exc)) from exc + return schema + # Collected during preprocessing, used in postprocessing _ENDPOINT_SERVERS: dict[str, list[dict[str, Any]]] = {} @@ -215,7 +239,9 @@ def _validate_request_body( # display body params without a description, so it's easy to miss them. # There is an edge case where a body param might be reference that we should ignore for now - if "description" not in param_data and "$ref" not in param_data: + # The baseline build restores omitted fields, which are exactly the ones + # allowed to lack a description, and it is never published. + if "description" not in param_data and "$ref" not in param_data and omissions_enabled(): raise SentryApiBuildError( f"""Body parameter '{body_param}' is missing a description for endpoint {endpoint_name}. diff --git a/src/sentry/apidocs/omission_apply.py b/src/sentry/apidocs/omission_apply.py new file mode 100644 index 000000000000..026acee0746d --- /dev/null +++ b/src/sentry/apidocs/omission_apply.py @@ -0,0 +1,187 @@ +"""Withholding declared choice values from a serializer's generated schema. + +drf-spectacular drops whole fields itself. This removes single choices from the +schema of exactly the serializer declaring them, then checks nothing else moved. +""" + +from __future__ import annotations + +import copy +from collections.abc import Iterator, Mapping, Set +from typing import Any + +from drf_spectacular.drainage import get_override + +from sentry.apidocs.omission_paths import VALUE, PathError, Resolved, resolve +from sentry.apidocs.omissions import OMISSION_REASONS_OVERRIDE + +_ABSENT = object() + + +class OmissionError(Exception): + """A declaration that cannot be applied to the generated schema exactly.""" + + +def choice_rules(serializer: Any) -> dict[str, Resolved]: + """The ``field.choice`` paths `serializer` declares, resolved against its fields.""" + declared = get_override(serializer, OMISSION_REASONS_OVERRIDE, {}) or {} + rules: dict[str, Resolved] = {} + errors: list[str] = [] + for path in sorted(path for path in declared if "." in path): + try: + resolved = resolve(serializer, path) + except PathError as exc: + errors.append(str(exc)) + continue + if resolved.kind != VALUE: + errors.append(f"{path!r} does not name a choice value") + continue + rules[path] = resolved + if errors: + raise OmissionError(f"{type(serializer).__name__}: {'; '.join(errors)}") + return rules + + +def withhold_values(schema: Mapping[str, Any], rules: Mapping[str, Resolved]) -> dict[str, Any]: + """A copy of a serializer's mapped schema with each rule's choice removed.""" + result = copy.deepcopy(dict(schema)) + properties = result.get("properties") + for path, rule in rules.items(): + field, value = rule.segments + if not isinstance(properties, dict) or not isinstance(properties.get(field), dict): + raise OmissionError(f"{path!r}: {field!r} is not in the generated schema") + prop = properties[field] + holder = _enum_holder(prop, path) + if value not in {str(entry) for entry in holder["enum"]}: + raise OmissionError(f"{path!r}: {value!r} is not in the generated enum") + holder["enum"] = [entry for entry in holder["enum"] if str(entry) != value] + strip_choice_description(prop, value) + strip_choice_description(holder, value) + if rule.withholds_default: + # A default only means anything for an optional field. + prop.pop("default", None) + holder.pop("default", None) + result["required"] = sorted({*result.get("required", []), field}) + return result + + +def _enum_holder(prop: dict[str, Any], path: str) -> dict[str, Any]: + """The schema carrying the enum, which for a list sits on its items.""" + for candidate in (prop, prop.get("items")): + if isinstance(candidate, dict) and isinstance(candidate.get("enum"), list): + return candidate + raise OmissionError(f"{path!r}: the generated schema has no enum to withhold from") + + +def strip_choice_description(holder: Any, value: str) -> None: + """Drop a withheld value from the generated choice listing. + + Every choice is named in the description, so the enum alone is not enough.""" + if not isinstance(holder, dict): + return + description = holder.get("description") + if not isinstance(description, str): + return + kept = [line for line in description.splitlines() if not line.startswith(f"* `{value}`")] + while kept and not kept[-1].strip(): + kept.pop() + holder["description"] = "\n".join(kept) + if not holder["description"]: + del holder["description"] + + +def check_withheld_values( + before: Mapping[str, Any], after: Mapping[str, Any], rules: Mapping[str, Resolved] +) -> None: + """Raise unless `after` differs from `before` only where `rules` allow. + + Written apart from withhold_values, so a mistake there cannot also pass here.""" + withheld: dict[str, set[str]] = {} + defaults: set[str] = set() + for rule in rules.values(): + field, value = rule.segments + withheld.setdefault(field, set()).add(value) + if rule.withholds_default: + defaults.add(field) + for key, old, new in _differences(before, after): + if not _allowed(key, old, new, withheld, defaults): + raise OmissionError( + f"withholding {sorted(rules)} changed {'/'.join(key)} from {_shown(old)} to " + f"{_shown(new)}, which no declaration covers" + ) + + +def check_required_parameters( + before: Mapping[tuple[str, str], Any], + after: Mapping[tuple[str, str], Any], + required: Set[tuple[str, str]], +) -> None: + """Raise unless `after` marks exactly the `required` parameters and changes + nothing else. Parameters are keyed by (name, location).""" + for key in sorted({*before, *after}): + old, new = before.get(key, _ABSENT), after.get(key, _ABSENT) + label = f"{key[1]}:{key[0]}" + if key in required: + expected = {**old, "required": True} if isinstance(old, Mapping) else old + if new != expected: + raise OmissionError( + f"withholding the default of {label} should only mark it required, but it " + f"changed from {_shown(old)} to {_shown(new)}" + ) + elif new != old: + raise OmissionError( + f"{label} changed from {_shown(old)} to {_shown(new)}, which no withheld " + f"default covers" + ) + + +def _differences( + old: Any, new: Any, key: tuple[str, ...] = () +) -> Iterator[tuple[tuple[str, ...], Any, Any]]: + """Every differing leaf, keyed by the property names leading to it.""" + if isinstance(old, Mapping) and isinstance(new, Mapping): + for name in sorted({*old, *new}, key=str): + yield from _differences( + old.get(name, _ABSENT), new.get(name, _ABSENT), (*key, str(name)) + ) + elif old != new: + yield key, old, new + + +def _allowed( + key: tuple[str, ...], + old: Any, + new: Any, + withheld: Mapping[str, Set[str]], + defaults: Set[str], +) -> bool: + if key == ("required",): + previous = old if isinstance(old, list) else [] + return isinstance(new, list) and set(new) == {*previous, *defaults} + if len(key) < 3 or key[0] != "properties" or key[1] not in withheld: + return False + field, rest = key[1], key[2:] + if rest[:1] == ("items",): + rest = rest[1:] + if rest == ("enum",): + return isinstance(old, list) and new == [e for e in old if str(e) not in withheld[field]] + if rest == ("description",): + return isinstance(old, str) and new == _without_choice_lines(old, withheld[field]) + if rest == ("default",): + return field in defaults and new is _ABSENT + return False + + +def _without_choice_lines(description: str, values: Set[str]) -> Any: + lines = [ + line + for line in description.splitlines() + if not any(line.startswith(f"* `{value}`") for value in values) + ] + while lines and not lines[-1].strip(): + lines.pop() + return "\n".join(lines) if lines else _ABSENT + + +def _shown(value: Any) -> str: + return "" if value is _ABSENT else repr(value) diff --git a/src/sentry/apidocs/omission_guard.py b/src/sentry/apidocs/omission_guard.py new file mode 100644 index 000000000000..f3ab14856b75 --- /dev/null +++ b/src/sentry/apidocs/omission_guard.py @@ -0,0 +1,424 @@ +"""Proving a schema build's omissions changed only what they declare. + +The docs build generates twice, once with omissions switched off, and compares +the two by name. Every difference must belong to an omission recorded where +drf-spectacular placed the serializer or TypedDict declaring it. +""" + +from __future__ import annotations + +import contextlib +from collections.abc import Iterator, Mapping, Set +from contextvars import ContextVar +from dataclasses import dataclass, field +from typing import Any + +from sentry.apidocs.omission_apply import ( + _ABSENT, + OmissionError, + _differences, + _shown, + _without_choice_lines, +) +from sentry.apidocs.omission_paths import Resolved + +# ("components", component name) or ("operations", operationId). +Location = tuple[str, str] +# Keys leading from a location's schema to a nested object, e.g. ("properties", "owner"). +Pointer = tuple[str, ...] + +_HTTP_METHODS = {"get", "put", "post", "delete", "options", "head", "patch", "trace"} +_REF_PREFIX = "#/components/schemas/" +_LISTED = 20 + + +@dataclass +class Declared: + """Everything declared by the serializers and TypedDicts placed at one location.""" + + omitted: set[str] = field(default_factory=set) + deprecated: set[str] = field(default_factory=set) + withheld: dict[str, set[str]] = field(default_factory=dict) + defaults: set[str] = field(default_factory=set) + # Fields omitted by a TypedDict inlined at each pointer within the location. + nested: dict[Pointer, set[str]] = field(default_factory=dict) + + +class _Disabled: + pass + + +_DISABLED = _Disabled() +_mode: ContextVar[dict[Location, Declared] | _Disabled | None] = ContextVar( + "sentry_apidocs_omission_mode", default=None +) +_placement: ContextVar[Location | None] = ContextVar( + "sentry_apidocs_omission_placement", default=None +) +_pointer: ContextVar[Pointer] = ContextVar("sentry_apidocs_omission_pointer", default=()) + + +def omissions_enabled() -> bool: + return not isinstance(_mode.get(), _Disabled) + + +@contextlib.contextmanager +def omissions_disabled() -> Iterator[None]: + token = _mode.set(_DISABLED) + try: + yield + finally: + _mode.reset(token) + + +@contextlib.contextmanager +def recording_omissions() -> Iterator[dict[Location, Declared]]: + recorded: dict[Location, Declared] = {} + token = _mode.set(recorded) + try: + yield recorded + finally: + _mode.reset(token) + + +@contextlib.contextmanager +def placed_at(location: Location) -> Iterator[None]: + """Serializers mapped inside this block land at the root of `location`.""" + placement, pointer = _placement.set(location), _pointer.set(()) + try: + yield + finally: + _pointer.reset(pointer) + _placement.reset(placement) + + +@contextlib.contextmanager +def descended(*keys: str) -> Iterator[None]: + """A type hint resolved inside this block lands at `keys` below the current pointer.""" + token = _pointer.set((*_pointer.get(), *keys)) + try: + yield + finally: + _pointer.reset(token) + + +def record( + serializer: str, + omitted: Set[str], + deprecated: Set[str], + rules: Mapping[str, Resolved], +) -> None: + """Note what a serializer declared at the location it is being mapped into.""" + declared = _declared_here(serializer, bool(omitted or deprecated or rules)) + if declared is None: + return + declared.omitted |= omitted + declared.deprecated |= deprecated + for rule in rules.values(): + field_name, value = rule.segments + declared.withheld.setdefault(field_name, set()).add(value) + if rule.withholds_default: + declared.defaults.add(field_name) + + +def record_typed_dict(typed_dict: str, omitted: Set[str]) -> None: + """Note the fields a TypedDict omits, at the pointer it is being inlined at.""" + declared = _declared_here(typed_dict, bool(omitted)) + if declared is not None: + declared.nested.setdefault(_pointer.get(), set()).update(omitted) + + +def _declared_here(declaring: str, declares: bool) -> Declared | None: + recorded = _mode.get() + if not isinstance(recorded, dict) or not declares: + return None + location = _placement.get() + if location is None: + raise OmissionError( + f"{declaring} declares omissions but was mapped outside any component or " + f"operation, so the build cannot check where they landed" + ) + return recorded.setdefault(location, Declared()) + + +def check_schema_omissions( + baseline: Mapping[str, Any], + schema: Mapping[str, Any], + recorded: Mapping[Location, Declared], +) -> None: + """Raise unless `schema` differs from `baseline` exactly as `recorded` declares.""" + before, after = _keyed(baseline), _keyed(schema) + unclaimed = [ + f"{'/'.join(key)}: {_shown(old)} -> {_shown(new)}" + for key, old, new in _differences(before, after) + if not _claimed(key, old, new, recorded, before, after) + ] + if unclaimed: + raise OmissionError( + "omissions changed parts of the schema nothing declares:\n " + + "\n ".join(unclaimed[:_LISTED]) + ) + missing = list(_missing(after, recorded)) + if missing: + raise OmissionError( + "declared omissions are not in the built schema:\n " + "\n ".join(missing[:_LISTED]) + ) + + +def _keyed(document: Mapping[str, Any]) -> dict[str, Any]: + """The document with operations keyed by operationId and parameters by location and name.""" + operations: dict[str, Any] = {} + path_items: dict[str, Any] = {} + for path, item in (document.get("paths") or {}).items(): + path_items[path] = {k: v for k, v in item.items() if k not in _HTTP_METHODS} + for method, operation in item.items(): + if method not in _HTTP_METHODS: + continue + operation_id = operation.get("operationId") or f"{method.upper()} {path}" + if operation_id in operations: + raise OmissionError( + f"operationId {operation_id!r} is used twice, so the builds cannot be compared" + ) + parameters = { + f"{p.get('in')}:{p.get('name')}": p for p in operation.get("parameters", []) + } + operations[operation_id] = { + **operation, + "parameters": parameters, + "x-route": f"{method.upper()} {path}", + } + rest = {k: v for k, v in document.items() if k != "paths"} + return {**rest, "paths": path_items, "operations": operations} + + +def _claimed( + key: tuple[str, ...], + old: Any, + new: Any, + recorded: Mapping[Location, Declared], + before: Mapping[str, Any], + after: Mapping[str, Any], +) -> bool: + if key[:2] == ("components", "schemas") and len(key) >= 3: + if len(key) == 3: + # A component only an omitted field used is no longer generated. + return new is _ABSENT and not _referenced(after, key[2]) + declared = recorded.get(("components", key[2])) + if declared is None: + return False + if _claimed_in_typed_dict(key[3:], old, new, declared): + return True + if len(key) >= 5 and key[3] == "properties" and key[4] in declared.deprecated: + # Compared as a whole property: drf-spectacular wraps a deprecated $ref in allOf. + return _marked_deprecated( + _property(before, key[2], key[4]), _property(after, key[2], key[4]) + ) + return _claimed_in_object(key[3:], old, new, declared) + if key[:1] == ("operations",) and len(key) >= 4 and key[2] == "parameters": + declared = recorded.get(("operations", key[1])) + return declared is not None and _claimed_in_parameter(key[3:], old, new, declared) + if key[:1] == ("operations",) and key[2:] == ("requestBody", "required"): + return _body_requirement_followed(key[1], old, new, recorded, before, after) + return False + + +def _claimed_in_typed_dict(rest: tuple[str, ...], old: Any, new: Any, declared: Declared) -> bool: + """A TypedDict's omission removes exactly its own fields at its own pointer.""" + for pointer, omitted in declared.nested.items(): + if rest[: len(pointer)] != pointer: + continue + inner = rest[len(pointer) :] + if len(inner) == 2 and inner[0] == "properties" and inner[1] in omitted: + return new is _ABSENT + if inner == ("required",): + previous = set(old) if isinstance(old, list) else set() + current = set(new) if isinstance(new, list) else set() + return bool(previous & omitted) and current == previous - omitted + return False + + +def _body_requirement_followed( + operation_id: str, + old: Any, + new: Any, + recorded: Mapping[Location, Declared], + before: Mapping[str, Any], + after: Mapping[str, Any], +) -> bool: + """drf-spectacular requires a body exactly while its schema has a writable + required field, so an omission that empties or fills that set flips the body.""" + component = _body_component(after["operations"][operation_id]) + if component is None or ("components", component) not in recorded: + return False + was, now = _has_writable_required(before, component), _has_writable_required(after, component) + return was != now and (old is True) == was and (new is True) == now + + +def _body_component(operation: Mapping[str, Any]) -> str | None: + """The one component every media type of the request body points at.""" + content = (operation.get("requestBody") or {}).get("content") or {} + refs = {(media.get("schema") or {}).get("$ref") for media in content.values()} + if len(refs) != 1: + return None + ref = refs.pop() + if not isinstance(ref, str) or not ref.startswith(_REF_PREFIX): + return None + return ref[len(_REF_PREFIX) :] + + +def _has_writable_required(document: Mapping[str, Any], name: str) -> bool: + component = document.get("components", {}).get("schemas", {}).get(name) or {} + properties = component.get("properties", {}) + return any( + not (properties.get(required) or {}).get("readOnly") + for required in component.get("required", []) + ) + + +def _claimed_in_object(rest: tuple[str, ...], old: Any, new: Any, declared: Declared) -> bool: + if rest == ("required",): + previous = set(old) if isinstance(old, list) else set() + current = set(new) if isinstance(new, list) else set() + return current == (previous - declared.omitted) | (declared.defaults & (previous | current)) + if len(rest) < 2 or rest[0] != "properties": + return False + field_name, inner = rest[1], rest[2:] + if field_name in declared.omitted: + return inner == () and new is _ABSENT + if field_name in declared.withheld: + return _claimed_choice( + inner, + old, + new, + declared.withheld[field_name], + field_name in declared.defaults, + ) + return False + + +def _property(document: Mapping[str, Any], component: str, field_name: str) -> Any: + schema = document.get("components", {}).get("schemas", {}).get(component) or {} + return (schema.get("properties") or {}).get(field_name, _ABSENT) + + +def _marked_deprecated(old: Any, new: Any) -> bool: + """Whether `new` is `old` marked deprecated the way drf-spectacular marks it: a + sibling key, or for a bare $ref an allOf wrapper, since a $ref ignores siblings.""" + if not isinstance(old, Mapping) or not isinstance(new, Mapping): + return False + if set(old) == {"$ref"}: + return dict(new) == {"allOf": [dict(old)], "deprecated": True} + return dict(new) == {**old, "deprecated": True} + + +def _claimed_in_parameter(rest: tuple[str, ...], old: Any, new: Any, declared: Declared) -> bool: + location, _, field_name = rest[0].partition(":") + inner = rest[1:] + if location != "query": + return False + if field_name in declared.omitted: + return inner == () and new is _ABSENT + if field_name in declared.deprecated: + return inner == ("deprecated",) and old is _ABSENT and new is True + if field_name not in declared.withheld: + return False + values = declared.withheld[field_name] + default_withheld = field_name in declared.defaults + if inner == ("required",): + return default_withheld and new is True + if inner == ("description",): + return isinstance(old, str) and new == _without_choice_lines(old, values) + if inner[:1] == ("schema",): + return _claimed_choice(inner[1:], old, new, values, default_withheld) + return False + + +def _claimed_choice( + inner: tuple[str, ...], old: Any, new: Any, values: Set[str], default_withheld: bool +) -> bool: + if inner[:1] == ("items",): + inner = inner[1:] + if inner == ("enum",): + return isinstance(old, list) and new == [e for e in old if str(e) not in values] + if inner == ("description",): + return isinstance(old, str) and new == _without_choice_lines(old, values) + if inner == ("default",): + return default_withheld and new is _ABSENT + return False + + +def _missing(after: Mapping[str, Any], recorded: Mapping[Location, Declared]) -> Iterator[str]: + """Declared omissions the finished schema does not show.""" + for (kind, name), declared in sorted(recorded.items()): + where = f"{kind}/{name}" + if kind == "components": + component = after.get("components", {}).get("schemas", {}).get(name) + if component is None: + continue + for pointer, omitted in sorted(declared.nested.items()): + node = _at(component, pointer) + present = omitted & set((node or {}).get("properties", {})) + for field_name in sorted(present): + yield f"{where}/{'/'.join(pointer)}: {field_name} is still present" + schemas = dict(component.get("properties", {})) + required = component.get("required", []) + marked = { + f for f, s in schemas.items() if isinstance(s, Mapping) and s.get("deprecated") + } + else: + operation = after.get("operations", {}).get(name) + if operation is None: + continue + parameters = operation.get("parameters", {}) + schemas = { + key.partition(":")[2]: parameter.get("schema") + for key, parameter in parameters.items() + if key.startswith("query:") + } + required = [ + key.partition(":")[2] + for key, parameter in parameters.items() + if key.startswith("query:") and parameter.get("required") + ] + marked = { + key.partition(":")[2] + for key, parameter in parameters.items() + if key.startswith("query:") and parameter.get("deprecated") + } + for field_name in sorted(declared.omitted & set(schemas)): + yield f"{where}: {field_name} is still present" + for field_name in sorted(declared.deprecated & set(schemas)): + if field_name not in marked: + yield f"{where}: {field_name} is not marked deprecated" + for field_name, values in sorted(declared.withheld.items()): + schema = schemas.get(field_name) + if not isinstance(schema, Mapping): + continue + enum = schema.get("enum", (schema.get("items") or {}).get("enum", [])) + for value in sorted(values & {str(entry) for entry in enum}): + yield f"{where}: {field_name} still offers {value!r}" + for field_name in sorted(declared.defaults & set(schemas)): + if field_name not in required: + yield f"{where}: {field_name} withholds its default but is not required" + + +def _at(node: Any, pointer: Pointer) -> Mapping[str, Any] | None: + for key in pointer: + if not isinstance(node, Mapping): + return None + node = node.get(key) + return node if isinstance(node, Mapping) else None + + +def _referenced(document: Mapping[str, Any], name: str) -> bool: + target = f"{_REF_PREFIX}{name}" + stack: list[Any] = [document] + while stack: + node = stack.pop() + if isinstance(node, Mapping): + if node.get("$ref") == target: + return True + stack.extend(node.values()) + elif isinstance(node, list): + stack.extend(node) + return False diff --git a/src/sentry/apidocs/omission_paths.py b/src/sentry/apidocs/omission_paths.py new file mode 100644 index 000000000000..534060ef6a5b --- /dev/null +++ b/src/sentry/apidocs/omission_paths.py @@ -0,0 +1,136 @@ +"""Naming a withheld part of the public schema: a dotted path resolved against +a serializer's fields, each field's class deciding what may follow it. +""" + +from __future__ import annotations + +from dataclasses import dataclass +from typing import Any + +from rest_framework import serializers + +# What a resolved path points at. A value can be withheld but not deprecated, +# because the generated enum has nowhere to carry per-entry metadata. +FIELD = "field" +VALUE = "value" + + +class PathError(ValueError): + """A path that cannot be parsed, or does not name anything.""" + + +@dataclass(frozen=True) +class Resolved: + """Where a path landed: the segments walked, and what the last one names.""" + + segments: tuple[str, ...] + kind: str + # Withholding the value a field falls back to says the default is going + # away, so the parameter must be sent explicitly and declares no default. + withholds_default: bool = False + + +def parse_path(path: str) -> tuple[str, ...]: + if not path or not path.strip(): + raise PathError("a path cannot be empty") + segments = path.split(".") + for segment in segments: + if not segment.strip(): + raise PathError( + f"{path!r} has an empty segment; write 'field' or 'field.choice' with no " + f"leading, trailing or doubled dots." + ) + return tuple(segments) + + +def _choices_of(field: Any) -> set[str] | None: + choices = getattr(field, "choices", None) + if choices is None: + return None + return {str(choice) for choice in choices} + + +def _descend(field: Any) -> Any | None: + """The serializer a segment descends into, skipping a list's child.""" + if isinstance(field, serializers.ListField): + field = field.child + if isinstance(field, serializers.ListSerializer): + field = field.child + return field if isinstance(field, serializers.BaseSerializer) else None + + +def _fields_of(node: Any) -> dict[str, Any] | None: + try: + return dict(node.fields) + except Exception: + return None + + +def resolve(serializer: Any, path: str) -> Resolved: + """Walk `path` against a constructed serializer, or raise PathError. + + Choices come from the built field, so a computed set resolves normally. + """ + segments = parse_path(path) + node: Any = serializer + for index, segment in enumerate(segments): + walked = ".".join(segments[: index + 1]) + fields = _fields_of(node) + if fields is None: + raise PathError( + f"{path!r}: {'.'.join(segments[:index])!r} has no fields to descend into" + ) + if segment not in fields: + raise PathError( + f"{path!r}: {walked!r} names nothing on " + f"{type(node).__name__}; it withholds nothing and should be deleted" + ) + field = fields[segment] + remaining = segments[index + 1 :] + if not remaining: + return Resolved(segments, FIELD) + + nested = _descend(field) + if nested is not None: + raise PathError(nested_field_message(path, type(nested).__name__, ".".join(remaining))) + choices = _choices_of(_unwrap_list(field)) + if choices is not None: + if len(remaining) > 1: + raise PathError(f"{path!r}: a choice value has no parts to address") + value = remaining[0] + if value not in choices: + raise PathError( + f"{path!r}: {walked!r} does not accept {value!r}; it withholds " + f"nothing and should be deleted" + ) + return Resolved(segments, VALUE, withholds_default=_is_default(field, value)) + if isinstance(field, (serializers.JSONField, serializers.DictField)): + raise PathError( + f"{path!r}: {walked!r} holds a dynamic mapping, so its contents " + f"are not statically addressable" + ) + raise PathError( + f"{path!r}: {walked!r} is a {type(field).__name__} and has no addressable parts" + ) + raise PathError(f"{path!r} resolved to nothing") + + +def _is_default(field: Any, value: str) -> bool: + default = getattr(_unwrap_list(field), "default", None) + return default is not None and str(default) == value + + +def _unwrap_list(field: Any) -> Any: + """A list's child, so `sort.-age` reaches a ListField(child=ChoiceField).""" + if isinstance(field, serializers.ListField): + return field.child + return field + + +def nested_field_message(path: str, shape: str, rest: str) -> str: + """Why a path into a nested serializer is refused, and where it belongs.""" + return ( + f"{path!r} reaches into {shape}, a shape other fields and endpoints can share. " + f"Field omissions are declared on the serializer that defines the field: " + f"declare {rest!r} on {shape}." + ) diff --git a/src/sentry/apidocs/omissions.py b/src/sentry/apidocs/omissions.py index f07840ce5bc7..53077a1d1e38 100644 --- a/src/sentry/apidocs/omissions.py +++ b/src/sentry/apidocs/omissions.py @@ -1,64 +1,153 @@ -"""Withholding a serializer field from the public API schema. - -Omitted fields vanish from the generated schema and every generated SDK, but are -still accepted at runtime. A missing description is not a reason to use this -- -add help_text instead; sentry.apidocs.hooks spells out when omission is correct. +"""Withholding part of a serializer from the public API schema. Omitted parts +vanish from every generated SDK but are still accepted at runtime. """ from __future__ import annotations from collections.abc import Callable, Sequence -from typing import TypeVar +from typing import TypeVar, get_type_hints from drf_spectacular.drainage import get_override, set_override +from rest_framework.serializers import BaseSerializer + +from sentry.apidocs.omission_paths import _descend, nested_field_message, parse_path -# Override key holding ``{field: reason}``. Read by the linter and by anything -# reporting on the public surface; drf-spectacular itself ignores it. +# Override keys holding ``{path: reason}``. Read by SentrySchema and by the +# linter; drf-spectacular itself ignores them. OMISSION_REASONS_OVERRIDE = "sentry_omission_reasons" +DEPRECATION_REASONS_OVERRIDE = "sentry_deprecation_reasons" T = TypeVar("T", bound=type) +def _check_reasons(declared: dict[str, str], keyword: str, remedy: str) -> None: + for path, reason in declared.items(): + parse_path(path) + if not reason or not reason.strip(): + raise ValueError( + f"{keyword}['{path}'] needs a reason explaining why the field is not " + f"part of the public API surface. {remedy}" + ) + + def sentry_schema_serializer( - *, omit_from_public_schema: dict[str, str], deprecate_fields: Sequence[str] | None = None + *, + omit_from_public_schema: dict[str, str] | None = None, + deprecate: dict[str, str] | None = None, + deprecate_fields: Sequence[str] | None = None, ) -> Callable[[T], T]: - """Withhold fields from the generated schema, recording why for each one. + """Withhold or deprecate parts of the generated schema, recording why. - Each value is the reason that field is not part of the public API surface. - ``deprecate_fields`` is passed through to drf-spectacular unchanged. + A key names a field, or ``field.choice`` to withhold one choice value. + ``deprecate_fields`` is the older list form. """ - if not omit_from_public_schema: + omit_from_public_schema = omit_from_public_schema or {} + deprecate = deprecate or {} + if not omit_from_public_schema and not deprecate and not deprecate_fields: raise ValueError( - "sentry_schema_serializer() requires at least one field in " - "omit_from_public_schema; remove the decorator instead." + "sentry_schema_serializer() requires at least one path in " + "omit_from_public_schema or deprecate; remove the decorator instead." ) - for field, reason in omit_from_public_schema.items(): - if not reason or not reason.strip(): - raise ValueError( - f"omit_from_public_schema['{field}'] needs a reason explaining why the " - f"field is not part of the public API surface. If the field is simply " - f"undocumented, add a help_text to it instead of omitting it." - ) + both = sorted(set(omit_from_public_schema) & set(deprecate)) + if both: + raise ValueError( + f"{both[0]!r} is both withheld and deprecated; a path that is absent from " + f"the schema cannot also be marked in it." + ) - def decorator(klass: T) -> T: - # Merge rather than replace: a class may carry exclude_fields from a - # stacked @extend_schema_serializer, and subclasses inherit the override. - existing = get_override(klass, "exclude_fields", []) or [] - merged = list(dict.fromkeys([*existing, *omit_from_public_schema])) - set_override(klass, "exclude_fields", merged) + _check_reasons( + omit_from_public_schema, + "omit_from_public_schema", + "If the field is simply undocumented, add a help_text to it instead of omitting it.", + ) + _check_reasons(deprecate, "deprecate", "Say what replaces it and when it goes away.") - reasons = {**(get_override(klass, OMISSION_REASONS_OVERRIDE, {}) or {})} - reasons.update(omit_from_public_schema) - set_override(klass, OMISSION_REASONS_OVERRIDE, reasons) + too_deep = sorted(path for path in omit_from_public_schema if path.count(".") > 1) + if too_deep: + raise ValueError( + f"omit_from_public_schema[{too_deep[0]!r}] is deeper than 'field.choice'. To " + f"withhold part of a nested serializer, declare it on that serializer." + ) + dotted_deprecations = sorted(path for path in deprecate if "." in path) + if dotted_deprecations: + raise ValueError( + f"deprecate[{dotted_deprecations[0]!r}] must name a whole field. The generated " + f"enum cannot mark one choice, and a nested field is deprecated on its own " + f"serializer." + ) - if deprecate_fields: - existing_deprecated = get_override(klass, "deprecate_fields", []) or [] - set_override( - klass, - "deprecate_fields", - list(dict.fromkeys([*existing_deprecated, *deprecate_fields])), + def decorator(klass: T) -> T: + if _is_response_serializer(klass): + returned = _returned_type_name(klass) + raise ValueError( + f"{klass.__name__} is a response Serializer, so its schema is the TypedDict " + f"its serialize() returns. Field omissions are declared on the class that " + f"defines the field: move this decorator to {returned or 'that TypedDict'}." ) + choice_paths = _deep(omit_from_public_schema) + if choice_paths and not issubclass(klass, BaseSerializer): + raise ValueError( + f"{klass.__name__} declares {choice_paths[0]!r}, but only a serializer has the " + f"choice fields such a path is resolved against." + ) + declared_fields = getattr(klass, "_declared_fields", {}) + for path in choice_paths: + field_name, _, rest = path.partition(".") + nested = _descend(declared_fields.get(field_name)) + if nested is not None: + raise ValueError(nested_field_message(path, type(nested).__name__, rest)) + _merge_reasons(klass, OMISSION_REASONS_OVERRIDE, omit_from_public_schema) + _merge_reasons(klass, DEPRECATION_REASONS_OVERRIDE, deprecate) + + # A field is dropped natively by drf-spectacular. A choice is withheld by + # SentrySchema while mapping this serializer. + _merge_native(klass, "exclude_fields", _shallow(omit_from_public_schema)) + _merge_native(klass, "deprecate_fields", _shallow(deprecate)) + if deprecate_fields: + _merge_native(klass, "deprecate_fields", list(deprecate_fields)) return klass return decorator + + +def _shallow(declared: dict[str, str]) -> list[str]: + return [path for path in declared if "." not in path] + + +def _deep(declared: dict[str, str]) -> list[str]: + return sorted(path for path in declared if "." in path) + + +def _is_response_serializer(klass: type) -> bool: + # Matched by name: importing sentry.api.serializers.base here would load models. + return any( + base.__module__ == "sentry.api.serializers.base" and base.__name__ == "Serializer" + for base in getattr(klass, "__mro__", ()) + ) + + +def _returned_type_name(klass: type) -> str | None: + """The name of what serialize() is typed to return, when it can be resolved yet.""" + try: + returned = get_type_hints(klass.serialize).get("return") # type: ignore[attr-defined] + except (NameError, TypeError, AttributeError): + return None + return getattr(returned, "__name__", None) + + +def _merge_native(klass: type, key: str, values: Sequence[str]) -> None: + """Merge rather than replace: a stacked decorator or a base class may have + set this already, and subclasses inherit the override.""" + if not values: + return + existing = get_override(klass, key, []) or [] + set_override(klass, key, list(dict.fromkeys([*existing, *values]))) + + +def _merge_reasons(klass: type, key: str, declared: dict[str, str]) -> None: + if not declared: + return + reasons = {**(get_override(klass, key, {}) or {})} + reasons.update(declared) + set_override(klass, key, reasons) diff --git a/src/sentry/apidocs/schema.py b/src/sentry/apidocs/schema.py index 25b046c53d3a..782d77bb6169 100644 --- a/src/sentry/apidocs/schema.py +++ b/src/sentry/apidocs/schema.py @@ -1,5 +1,24 @@ +import contextlib +import copy +from collections.abc import Iterator +from typing import Any + +from drf_spectacular.drainage import get_override from drf_spectacular.openapi import AutoSchema -from drf_spectacular.plumbing import get_doc +from drf_spectacular.plumbing import force_instance, get_doc, is_basic_serializer +from drf_spectacular.utils import OpenApiParameter + +from sentry.apidocs.omission_apply import ( + OmissionError, + check_required_parameters, + check_withheld_values, + choice_rules, + withhold_values, +) +from sentry.apidocs.omission_guard import omissions_enabled, placed_at, record +from sentry.apidocs.omission_paths import Resolved +from sentry.apidocs.omissions import DEPRECATION_REASONS_OVERRIDE, OMISSION_REASONS_OVERRIDE +from sentry.apidocs.utils import SentryApiBuildError class SentrySchema(AutoSchema): @@ -26,3 +45,129 @@ def get_description(self) -> str: # type: ignore[override] if len(docstring.splitlines()) > 1: return docstring return super().get_description() + + def resolve_serializer( + self, serializer: Any, direction: Any, bypass_extensions: bool = False + ) -> Any: + """Note which component the serializer is mapped into, for the build's check.""" + name = self._get_serializer_name(force_instance(serializer), direction, bypass_extensions) + with placed_at(("components", name)): + return super().resolve_serializer(serializer, direction, bypass_extensions) + + def _map_serializer( + self, serializer: Any, direction: Any, bypass_extensions: bool = False + ) -> Any: + """Withhold declared choices from the schema of the serializer declaring them. + + Components and exploded query parameters are both built here.""" + instance = force_instance(serializer) + if not omissions_enabled(): + with _decorator_exclusions_suspended(instance): + return super()._map_serializer(instance, direction, bypass_extensions) + + schema = super()._map_serializer(instance, direction, bypass_extensions) + rules = _choice_rules(instance) + try: + record( + type(instance).__name__, + _declared_fields(instance, OMISSION_REASONS_OVERRIDE), + _declared_fields(instance, DEPRECATION_REASONS_OVERRIDE), + rules, + ) + if not rules: + return schema + withheld = withhold_values(schema, rules) + check_withheld_values(schema, withheld, rules) + except OmissionError as exc: + raise SentryApiBuildError(f"{type(instance).__name__}: {exc}") from exc + return withheld + + def _process_override_parameters(self, direction: Any = "request") -> Any: + """Mark a parameter required when its serializer withholds the default. + + drf-spectacular takes `required` from the field, which still has one.""" + with placed_at(("operations", self.get_operation_id())): + result = super()._process_override_parameters(direction) + _lift_deprecated(result) + if not omissions_enabled(): + return result + + # Which declaration produced each parameter; a later one replaces an earlier one. + source: dict[tuple[str, str], Any] = {} + for parameter in self.get_override_parameters(): + if isinstance(parameter, OpenApiParameter): + source[parameter.name, parameter.location] = parameter + elif is_basic_serializer(parameter): + for name in force_instance(parameter).fields: + source[name, OpenApiParameter.QUERY] = parameter + serializers = {id(p): p for p in source.values() if not isinstance(p, OpenApiParameter)} + required: set[tuple[str, str]] = set() + for serializer in serializers.values(): + for rule in _choice_rules(serializer).values(): + key = (rule.segments[0], OpenApiParameter.QUERY) + if rule.withholds_default and source.get(key) is serializer and result.get(key): + required.add(key) + if not required: + return result + + before = copy.deepcopy(result) + for key in required: + result[key]["required"] = True + try: + check_required_parameters(before, result, required) + except OmissionError as exc: + raise SentryApiBuildError(str(exc)) from exc + return result + + +def _choice_rules(serializer: Any) -> dict[str, Resolved]: + try: + return choice_rules(force_instance(serializer)) + except OmissionError as exc: + raise SentryApiBuildError(str(exc)) from exc + + +def _lift_deprecated(parameters: dict[tuple[str, str], Any]) -> None: + """Mark a deprecated parameter on the parameter, where SDK generators read it. + + drf-spectacular leaves a deprecated serializer field's marker in its schema.""" + for parameter in parameters.values(): + schema = (parameter or {}).get("schema") + if isinstance(schema, dict) and schema.pop("deprecated", False): + parameter["deprecated"] = True + + +def _declared_fields(serializer: Any, key: str) -> set[str]: + """Whole fields the decorator declared under `key`, which drf-spectacular applies.""" + return {path for path in get_override(serializer, key, {}) or {} if "." not in path} + + +@contextlib.contextmanager +def _decorator_exclusions_suspended(instance: Any) -> Iterator[None]: + """Map `instance` as if the decorator had not excluded or deprecated its fields. + + The override is shadowed on the instance alone, so the class is untouched.""" + omitted = _declared_fields(instance, OMISSION_REASONS_OVERRIDE) + deprecated = _declared_fields(instance, DEPRECATION_REASONS_OVERRIDE) + if not omitted and not deprecated: + yield + return + own = vars(instance).get("_spectacular_annotation", _MISSING) + annotation = dict(getattr(instance, "_spectacular_annotation", {})) + annotation["exclude_fields"] = [ + f for f in annotation.get("exclude_fields", []) if f not in omitted + ] + annotation["deprecate_fields"] = [ + f for f in annotation.get("deprecate_fields", []) if f not in deprecated + ] + instance._spectacular_annotation = annotation + try: + yield + finally: + if own is _MISSING: + del instance._spectacular_annotation + else: + instance._spectacular_annotation = own + + +_MISSING = object() diff --git a/src/sentry/apidocs/spectacular_ports.py b/src/sentry/apidocs/spectacular_ports.py index 31f7f9989934..1fa1f622fa32 100644 --- a/src/sentry/apidocs/spectacular_ports.py +++ b/src/sentry/apidocs/spectacular_ports.py @@ -49,6 +49,8 @@ from drf_spectacular.types import OpenApiTypes from drf_spectacular.utils import _SchemaType +from sentry.apidocs.omission_guard import descended, omissions_enabled, record_typed_dict +from sentry.apidocs.omissions import OMISSION_REASONS_OVERRIDE from sentry.apidocs.utils import reload_module_with_type_checking_enabled # This function is ported from the drf-spectacular library method here: @@ -119,13 +121,15 @@ def resolve_type_hint(hint) -> Any: elif origin is None and inspect.isclass(hint) and issubclass(hint, tuple): # a convoluted way to catch NamedTuple. suggestions welcome. if get_type_hints(hint): - properties = {k: resolve_type_hint(v) for k, v in get_type_hints(hint).items()} + properties = { + k: _resolve_at(v, "properties", k) for k, v in get_type_hints(hint).items() + } else: properties = {k: build_basic_type(OpenApiTypes.ANY) for k in hint._fields} # type: ignore[attr-defined] return build_object_type(properties=properties, required=properties.keys()) elif origin is list or hint is list: return build_array_type( - resolve_type_hint(args[0]) if args else build_basic_type(OpenApiTypes.ANY) + _resolve_at(args[0], "items") if args else build_basic_type(OpenApiTypes.ANY) ) elif origin is tuple: return build_array_type( @@ -136,12 +140,12 @@ def resolve_type_hint(hint) -> Any: elif origin is dict or origin is defaultdict: schema = build_basic_type(OpenApiTypes.OBJECT) if args and args[1] is not typing.Any and schema is not None: - schema["additionalProperties"] = resolve_type_hint(args[1]) + schema["additionalProperties"] = _resolve_at(args[1], "additionalProperties") return schema elif origin is set: - return build_array_type(resolve_type_hint(args[0])) + return build_array_type(_resolve_at(args[0], "items")) elif origin is frozenset: - return build_array_type(resolve_type_hint(args[0])) + return build_array_type(_resolve_at(args[0], "items")) elif origin is Literal: # Literal only works for python >= 3.8 despite typing_extensions, because it # behaves slightly different w.r.t. __origin__ @@ -162,9 +166,10 @@ def resolve_type_hint(hint) -> Any: schema.update(basic_type) return schema elif is_typeddict(hint): + excluded_fields = _typed_dict_exclusions(hint, excluded_fields) return build_object_type( properties={ - k: resolve_type_hint(v) + k: _resolve_at(v, "properties", k) for k, v in get_type_hints(hint).items() if k not in excluded_fields }, @@ -179,7 +184,11 @@ def resolve_type_hint(hint) -> Any: # multiple types but errors with oneOf. # TODO(schew2381): Create issue in drf-spectacular to see if this # fix makes sense - schema = {"anyOf": [resolve_type_hint(arg) for arg in type_args]} + schema = { + "anyOf": [ + _resolve_at(arg, "anyOf", str(index)) for index, arg in enumerate(type_args) + ] + } else: schema = resolve_type_hint(type_args[0]) if type(None) in args and schema is not None: @@ -197,6 +206,25 @@ def resolve_type_hint(hint) -> Any: schema["nullable"] = True return schema elif origin is collections.abc.Iterable: - return build_array_type(resolve_type_hint(args[0])) + return build_array_type(_resolve_at(args[0], "items")) else: raise UnableToProceedError(hint) + + +def _resolve_at(hint: Any, *keys: str) -> Any: + """Resolve a hint nested at `keys` below its parent, for the build's omission check.""" + with descended(*keys): + return resolve_type_hint(hint) + + +def _typed_dict_exclusions(hint: Any, excluded_fields: Any) -> list[str]: + """The fields a TypedDict leaves out, recording the ones it declares as omissions. + + With omissions switched off, the declared ones are kept so the build can compare.""" + declared = { + path for path in get_override(hint, OMISSION_REASONS_OVERRIDE, {}) or {} if "." not in path + } + if not omissions_enabled(): + return [name for name in excluded_fields if name not in declared] + record_typed_dict(hint.__name__, declared) + return list(excluded_fields) diff --git a/tests/sentry/apidocs/test_omission_guard.py b/tests/sentry/apidocs/test_omission_guard.py new file mode 100644 index 000000000000..7a9f571142f4 --- /dev/null +++ b/tests/sentry/apidocs/test_omission_guard.py @@ -0,0 +1,332 @@ +from __future__ import annotations + +from collections.abc import Callable, Sequence +from typing import Any, TypedDict +from unittest import mock + +import pytest +from django.urls import path as url_path +from drf_spectacular.settings import spectacular_settings +from drf_spectacular.utils import OpenApiParameter, extend_schema +from rest_framework import serializers + +# Registers the extension mapping a response Serializer through its TypedDict, +# as the docs build does by importing it. +import sentry.apidocs.extensions # noqa: F401 +from sentry.api.base import Endpoint +from sentry.api.serializers.base import Serializer as ResponseSerializer +from sentry.apidocs.hooks import CustomGenerator +from sentry.apidocs.omissions import sentry_schema_serializer +from sentry.apidocs.utils import SentryApiBuildError +from sentry.conf.server import custom_parameter_sort + +Hook = Callable[..., Any] + + +@sentry_schema_serializer( + omit_from_public_schema={ + "internal_flag": "Internal.", + "kind.internal": "Internal.", + "mode.legacy": "Send mode explicitly.", + }, + deprecate={"old_name": "Use name."}, +) +class GuardedParams(serializers.Serializer): + kind = serializers.ChoiceField(choices=("public", "internal"), help_text="Kind.") + mode = serializers.ChoiceField(choices=("legacy", "current"), default="legacy") + internal_flag = serializers.BooleanField(required=False) + old_name = serializers.CharField(required=False, help_text="Old name.") + + +@sentry_schema_serializer(omit_from_public_schema={"kind.internal": "Internal."}) +class GuardedBody(serializers.Serializer): + kind = serializers.ChoiceField(choices=("public", "internal"), help_text="Kind.") + caption = serializers.CharField(help_text="Caption.") + + +class UnrelatedParams(serializers.Serializer): + query = serializers.CharField(required=False, help_text="Query.") + + +class GuardedEndpoint(Endpoint): + permission_classes = () + + @extend_schema(operation_id="guarded", parameters=[GuardedParams]) + def get(self, request): + pass + + @extend_schema(operation_id="guardedBody", request=GuardedBody) + def post(self, request): + pass + + +class UnrelatedEndpoint(Endpoint): + permission_classes = () + + @extend_schema(operation_id="unrelated", parameters=[UnrelatedParams]) + def get(self, request): + pass + + +def _build(hooks: Sequence[Hook] = ()) -> dict[str, Any]: + """Build the way the docs build does, with `hooks` as later postprocessing.""" + patterns = [ + url_path("guarded/", GuardedEndpoint.as_view()), + url_path("unrelated/", UnrelatedEndpoint.as_view()), + ] + with ( + mock.patch.object(spectacular_settings, "POSTPROCESSING_HOOKS", list(hooks)), + mock.patch.object(spectacular_settings, "SORT_OPERATION_PARAMETERS", custom_parameter_sort), + ): + return CustomGenerator(patterns=patterns).get_schema(request=None, public=True) + + +def _parameters(result: dict[str, Any], route: str) -> list[dict[str, Any]]: + return result["paths"][f"/{route}"]["get"]["parameters"] + + +def _parameter(result: dict[str, Any], route: str, name: str) -> dict[str, Any]: + return next(p for p in _parameters(result, route) if p["name"] == name) + + +def _withheld(result: dict[str, Any]) -> bool: + """Whether this build is the one with omissions applied.""" + return "internal" not in _parameter(result, "guarded/", "kind")["schema"]["enum"] + + +def test_a_build_with_every_kind_of_omission_passes_the_guard() -> None: + result = _build() + names = {p["name"] for p in _parameters(result, "guarded/")} + assert "internal_flag" not in names + assert _parameter(result, "guarded/", "kind")["schema"]["enum"] == ["public"] + assert _parameter(result, "guarded/", "mode")["required"] is True + # OpenAPI marks a deprecated parameter on the parameter, where SDK generators read it. + old_name = _parameter(result, "guarded/", "old_name") + assert old_name["deprecated"] is True + assert "deprecated" not in old_name["schema"] + body = result["components"]["schemas"]["GuardedBody"]["properties"]["kind"] + assert body["enum"] == ["public"] + + +def test_reordering_parameters_is_not_a_difference() -> None: + def reverse(result: dict[str, Any], **kwargs: Any) -> dict[str, Any]: + result["paths"]["/guarded/"]["get"]["parameters"].reverse() + return result + + _build([reverse]) + + +def test_a_withheld_choice_restored_by_a_later_step_fails_the_build() -> None: + def restore(result: dict[str, Any], **kwargs: Any) -> dict[str, Any]: + _parameter(result, "guarded/", "kind")["schema"]["enum"] = ["public", "internal"] + return result + + with pytest.raises(SentryApiBuildError) as exc: + _build([restore]) + assert "internal" in str(exc.value) + + +def test_an_omitted_field_restored_by_a_later_step_fails_the_build() -> None: + def restore(result: dict[str, Any], **kwargs: Any) -> dict[str, Any]: + parameters = _parameters(result, "guarded/") + if not any(p["name"] == "internal_flag" for p in parameters): + parameters.append( + {"in": "query", "name": "internal_flag", "schema": {"type": "boolean"}} + ) + return result + + with pytest.raises(SentryApiBuildError) as exc: + _build([restore]) + assert "internal_flag" in str(exc.value) + + +def test_an_omission_that_changes_another_operation_fails_the_build() -> None: + def spill(result: dict[str, Any], **kwargs: Any) -> dict[str, Any]: + if _withheld(result): + _parameter(result, "unrelated/", "query")["description"] = "Changed." + return result + + with pytest.raises(SentryApiBuildError) as exc: + _build([spill]) + assert "unrelated" in str(exc.value) + + +def test_an_omission_that_changes_another_field_of_its_component_fails_the_build() -> None: + def spill(result: dict[str, Any], **kwargs: Any) -> dict[str, Any]: + if _withheld(result): + result["components"]["schemas"]["GuardedBody"]["properties"]["caption"]["maxLength"] = 1 + return result + + with pytest.raises(SentryApiBuildError) as exc: + _build([spill]) + assert "caption" in str(exc.value) + + +def test_marking_a_parameter_required_that_no_default_rule_covers_fails_the_build() -> None: + def spill(result: dict[str, Any], **kwargs: Any) -> dict[str, Any]: + if _withheld(result): + _parameter(result, "guarded/", "old_name")["required"] = True + return result + + with pytest.raises(SentryApiBuildError) as exc: + _build([spill]) + assert "old_name" in str(exc.value) + + +@sentry_schema_serializer(omit_from_public_schema={"secret": "Internal."}) +class OnlyRequiredFieldOmittedBody(serializers.Serializer): + secret = serializers.CharField(help_text="Secret.") + note = serializers.CharField(required=False, help_text="Note.") + + +class OnlyRequiredFieldOmittedEndpoint(Endpoint): + permission_classes = () + + @extend_schema(operation_id="onlyRequiredOmitted", request=OnlyRequiredFieldOmittedBody) + def put(self, request): + pass + + +def _build_body(hooks: Sequence[Hook] = ()) -> dict[str, Any]: + patterns = [url_path("body/", OnlyRequiredFieldOmittedEndpoint.as_view())] + with mock.patch.object(spectacular_settings, "POSTPROCESSING_HOOKS", list(hooks)): + return CustomGenerator(patterns=patterns).get_schema(request=None, public=True) + + +def test_omitting_a_bodys_only_required_field_leaves_the_body_optional() -> None: + """drf-spectacular requires a body only while its serializer has a required field.""" + result = _build_body() + assert "required" not in result["paths"]["/body/"]["put"]["requestBody"] + assert ( + "secret" + not in result["components"]["schemas"]["OnlyRequiredFieldOmittedBody"]["properties"] + ) + + +def test_a_body_made_optional_without_an_omission_behind_it_fails_the_build() -> None: + def spill(result: dict[str, Any], **kwargs: Any) -> dict[str, Any]: + properties = result["components"]["schemas"]["OnlyRequiredFieldOmittedBody"]["properties"] + body = result["paths"]["/body/"]["put"]["requestBody"] + if "secret" not in properties: + body.pop("required", None) + body["description"] = "Changed." + return result + + with pytest.raises(SentryApiBuildError) as exc: + _build_body([spill]) + assert "description" in str(exc.value) + + +def test_an_explicit_parameter_is_outside_the_guard() -> None: + """A hand-written parameter is identical in both builds, so nothing to claim.""" + + class ExplicitEndpoint(Endpoint): + permission_classes = () + + @extend_schema( + operation_id="explicit", + parameters=[OpenApiParameter("kind", str, enum=["public", "internal"])], + ) + def get(self, request): + pass + + patterns = [url_path("explicit/", ExplicitEndpoint.as_view())] + with mock.patch.object(spectacular_settings, "POSTPROCESSING_HOOKS", []): + result = CustomGenerator(patterns=patterns).get_schema(request=None, public=True) + enum = result["paths"]["/explicit/"]["get"]["parameters"][0]["schema"]["enum"] + assert sorted(enum) == ["internal", "public"] + + +class NestedShape(serializers.Serializer): + token = serializers.CharField(help_text="Token.") + + +@sentry_schema_serializer(deprecate={"shape": "Use token directly."}) +class DeprecatedNestedBody(serializers.Serializer): + # No help_text: a bare $ref, which only the deprecated build wraps in allOf. + shape = NestedShape() + name = serializers.CharField(help_text="Name.") + + +class DeprecatedNestedEndpoint(Endpoint): + permission_classes = () + + @extend_schema(operation_id="deprecatedNested", request=DeprecatedNestedBody) + def post(self, request): + pass + + +@sentry_schema_serializer(omit_from_public_schema={"secret": "Internal."}) +class GuardedItem(TypedDict): + name: str + secret: str + + +class GuardedTypedResponse(TypedDict): + title: str + owner: GuardedItem + items: list[GuardedItem] + + +class GuardedTypedResponseSerializer(ResponseSerializer): + def serialize(self, obj: Any, attrs: Any, user: Any, **kwargs: Any) -> GuardedTypedResponse: + raise NotImplementedError + + +class GuardedTypedEndpoint(Endpoint): + permission_classes = () + + @extend_schema(operation_id="typed", responses={200: GuardedTypedResponseSerializer}) + def get(self, request): + pass + + +def _build_typed(hooks: Sequence[Hook] = ()) -> dict[str, Any]: + patterns = [url_path("typed/", GuardedTypedEndpoint.as_view())] + with mock.patch.object(spectacular_settings, "POSTPROCESSING_HOOKS", list(hooks)): + return CustomGenerator(patterns=patterns).get_schema(request=None, public=True) + + +def _typed_item(result: dict[str, Any], where: str) -> dict[str, Any]: + """The GuardedItem object inside the response, as the owner or a list item.""" + response = result["components"]["schemas"]["GuardedTypedResponse"]["properties"] + return response["owner"] if where == "owner" else response["items"]["items"] + + +def test_a_typed_dict_omission_passes_the_guard_wherever_it_is_nested() -> None: + result = _build_typed() + assert "secret" not in _typed_item(result, "owner")["properties"] + assert "secret" not in _typed_item(result, "items")["properties"] + + +@pytest.mark.parametrize("where", ("owner", "items")) +def test_a_typed_dict_omission_restored_by_a_later_step_fails_the_build(where: str) -> None: + def restore(result: dict[str, Any], **kwargs: Any) -> dict[str, Any]: + _typed_item(result, where)["properties"].setdefault("secret", {"type": "string"}) + return result + + with pytest.raises(SentryApiBuildError) as exc: + _build_typed([restore]) + assert "secret" in str(exc.value) + + +def test_a_typed_dict_omission_that_changes_a_sibling_field_fails_the_build() -> None: + def spill(result: dict[str, Any], **kwargs: Any) -> dict[str, Any]: + owner = _typed_item(result, "owner")["properties"] + if "secret" not in owner: + owner["name"]["maxLength"] = 1 + return result + + with pytest.raises(SentryApiBuildError) as exc: + _build_typed([spill]) + assert "name" in str(exc.value) + + +def test_deprecating_a_nested_serializer_field_passes_the_guard() -> None: + """drf-spectacular wraps a deprecated $ref as allOf, so it is not a leaf change.""" + patterns = [url_path("nested/", DeprecatedNestedEndpoint.as_view())] + with mock.patch.object(spectacular_settings, "POSTPROCESSING_HOOKS", []): + result = CustomGenerator(patterns=patterns).get_schema(request=None, public=True) + shape = result["components"]["schemas"]["DeprecatedNestedBody"]["properties"]["shape"] + assert shape["deprecated"] is True + assert shape["allOf"] == [{"$ref": "#/components/schemas/NestedShape"}] diff --git a/tests/sentry/apidocs/test_omission_paths.py b/tests/sentry/apidocs/test_omission_paths.py new file mode 100644 index 000000000000..89a99c0be836 --- /dev/null +++ b/tests/sentry/apidocs/test_omission_paths.py @@ -0,0 +1,533 @@ +from __future__ import annotations + +from typing import Any, TypedDict +from unittest import mock + +import pytest +from django.urls import path as url_path +from drf_spectacular.drainage import get_override +from drf_spectacular.settings import spectacular_settings +from drf_spectacular.utils import OpenApiParameter, extend_schema +from rest_framework import serializers + +import sentry.apidocs.omission_apply as omission_apply +from sentry.api.base import Endpoint +from sentry.api.serializers.base import Serializer as ResponseSerializer +from sentry.apidocs.omission_paths import FIELD, VALUE, PathError, Resolved, parse_path, resolve +from sentry.apidocs.omissions import ( + DEPRECATION_REASONS_OVERRIDE, + OMISSION_REASONS_OVERRIDE, + sentry_schema_serializer, +) +from sentry.apidocs.utils import SentryApiBuildError +from sentry.conf.server import custom_parameter_sort +from tests.sentry.apidocs import generate_schema + + +class Inner(serializers.Serializer): + token = serializers.CharField() + caption = serializers.CharField() + + +class Shape(serializers.Serializer): + name = serializers.CharField() + data_source = serializers.ChoiceField(choices=("discover", "events", "spans")) + config = Inner() + items = serializers.ListField(child=Inner()) + sort = serializers.ListField(child=serializers.ChoiceField(choices=("-age", "age"))) + options = serializers.JSONField() + + +# --- parsing and resolution --- + + +def test_a_flat_path_names_a_field() -> None: + assert resolve(Shape(), "name").kind == FIELD + + +def test_a_choice_field_resolves_the_next_segment_as_a_value() -> None: + assert resolve(Shape(), "data_source.discover").kind == VALUE + + +@pytest.mark.parametrize("path", ("config.token", "items.token")) +def test_a_path_into_a_nested_serializer_must_be_declared_on_that_serializer(path: str) -> None: + """The nested shape is its own component, shared by everything using it.""" + with pytest.raises(PathError) as exc: + resolve(Shape(), path) + assert "declare" in str(exc.value) + assert "Inner" in str(exc.value) + + +def test_a_list_of_choices_resolves_a_value() -> None: + assert resolve(Shape(), "sort.-age").kind == VALUE + + +@pytest.mark.parametrize( + "path,expected", + ( + ("name.nope", "has no addressable parts"), + ("data_source.nope", "does not accept"), + ("data_source.discover.deeper", "has no parts to address"), + ("options.anything", "dynamic mapping"), + ("config.nope", "declare"), + ("nope", "names nothing"), + ), +) +def test_an_unresolvable_path_is_reported(path: str, expected: str) -> None: + with pytest.raises(PathError) as exc: + resolve(Shape(), path) + assert expected in str(exc.value) + + +@pytest.mark.parametrize("path", ("", " ", "a..b", ".a", "a.")) +def test_a_malformed_path_is_rejected(path: str) -> None: + with pytest.raises(PathError): + parse_path(path) + + +def test_choices_come_from_the_built_field_not_source() -> None: + computed = [value.upper() for value in ("a", "b")] + + class Computed(serializers.Serializer): + kind = serializers.ChoiceField(choices=computed) + + assert resolve(Computed(), "kind.A").kind == VALUE + + +def test_withholding_the_default_value_says_so() -> None: + class Defaulted(serializers.Serializer): + kind = serializers.ChoiceField(choices=("a", "b"), default="a") + + resolved = resolve(Defaulted(), "kind.a") + assert resolved.kind == VALUE + assert resolved.withholds_default + + +def test_withholding_a_non_default_does_not_claim_the_default() -> None: + class Defaulted(serializers.Serializer): + kind = serializers.ChoiceField(choices=("a", "b"), default="a") + + assert not resolve(Defaulted(), "kind.b").withholds_default + + +# --- the two verbs on the decorator --- + + +def test_both_verbs_record_their_reasons() -> None: + @sentry_schema_serializer( + omit_from_public_schema={"data_source.discover": "Deprecated; use events."}, + deprecate={"name": "Use slug."}, + ) + class Declared(Shape): + pass + + assert get_override(Declared, OMISSION_REASONS_OVERRIDE) == { + "data_source.discover": "Deprecated; use events." + } + assert get_override(Declared, DEPRECATION_REASONS_OVERRIDE) == {"name": "Use slug."} + # A one-segment path is handled natively; a choice is not. + assert get_override(Declared, "deprecate_fields") == ["name"] + assert get_override(Declared, "exclude_fields", []) == [] + + +def test_a_path_cannot_be_both_withheld_and_deprecated() -> None: + with pytest.raises(ValueError) as exc: + sentry_schema_serializer(omit_from_public_schema={"name": "a"}, deprecate={"name": "b"}) + assert "both withheld and deprecated" in str(exc.value) + + +def test_deprecation_requires_a_reason() -> None: + with pytest.raises(ValueError) as exc: + sentry_schema_serializer(deprecate={"name": " "}) + assert "needs a reason" in str(exc.value) + + +def test_the_older_list_form_still_works() -> None: + @sentry_schema_serializer(deprecate_fields=["name"]) + class Legacy(Shape): + pass + + assert get_override(Legacy, "deprecate_fields") == ["name"] + + +# --- declarations the decorator refuses outright --- + + +def test_a_typed_dict_cannot_declare_a_dotted_path() -> None: + """Only a serializer has fields to resolve the rest of a path against.""" + + class Response(TypedDict): + kind: str + + with pytest.raises(ValueError) as exc: + sentry_schema_serializer(omit_from_public_schema={"kind.internal": "Internal."})(Response) + assert "serializer" in str(exc.value) + + +@pytest.mark.parametrize("path", ("data_source.discover", "config.token")) +def test_deprecation_names_a_whole_field(path: str) -> None: + with pytest.raises(ValueError) as exc: + sentry_schema_serializer(deprecate={path: "Use something else."}) + assert "whole field" in str(exc.value) + + +def test_a_path_is_a_field_or_a_field_and_one_choice() -> None: + with pytest.raises(ValueError): + sentry_schema_serializer(omit_from_public_schema={"config.token.more": "Internal."}) + + +@pytest.mark.parametrize("path", ("config.token", "items.token")) +def test_a_parent_cannot_omit_a_field_of_a_nested_serializer(path: str) -> None: + """Refused when the class is defined, naming the serializer to declare it on.""" + + class Parent(Shape): + pass + + with pytest.raises(ValueError) as exc: + sentry_schema_serializer(omit_from_public_schema={path: "Internal."})(Parent) + assert "declare 'token' on Inner" in str(exc.value) + + +class RefusedResponse(TypedDict): + name: str + secret: str + + +class RefusedResponseSerializer(ResponseSerializer): + def serialize(self, obj: Any, attrs: Any, user: Any, **kwargs: Any) -> RefusedResponse: + raise NotImplementedError + + +def test_a_response_serializer_cannot_declare_omissions() -> None: + """Its schema is the TypedDict serialize() returns, so its fields are declared there.""" + with pytest.raises(ValueError) as exc: + sentry_schema_serializer(omit_from_public_schema={"secret": "Internal."})( + RefusedResponseSerializer + ) + assert "TypedDict" in str(exc.value) + assert "RefusedResponse" in str(exc.value) + + +# --- resolving and withholding a serializer's choices --- + + +def test_only_choice_paths_become_rules() -> None: + @sentry_schema_serializer( + omit_from_public_schema={"name": "Internal.", "data_source.discover": "Deprecated."} + ) + class Mixed(Shape): + pass + + assert list(omission_apply.choice_rules(Mixed())) == ["data_source.discover"] + assert get_override(Mixed, "exclude_fields") == ["name"] + + +def test_a_serializer_without_declarations_has_no_rules() -> None: + assert omission_apply.choice_rules(Shape()) == {} + + +def test_a_withheld_value_is_stripped_from_the_description() -> None: + holder = {"description": "Pick one.\n\n* `a` - A\n* `b` - B"} + omission_apply.strip_choice_description(holder, "a") + assert holder["description"] == "Pick one.\n\n* `b` - B" + + +def test_withholding_from_a_list_of_choices_edits_its_items() -> None: + before = { + "properties": { + "sort": {"type": "array", "items": {"type": "string", "enum": ["-age", "age"]}} + } + } + rules = {"sort.-age": Resolved(("sort", "-age"), VALUE)} + after = omission_apply.withhold_values(before, rules) + omission_apply.check_withheld_values(before, after, rules) + assert after["properties"]["sort"]["items"]["enum"] == ["age"] + + +def test_withholding_the_default_requires_the_field_and_passes_the_check() -> None: + before = { + "properties": { + "kind": {"type": "string", "enum": ["legacy", "current"], "default": "legacy"} + } + } + rules = {"kind.legacy": Resolved(("kind", "legacy"), VALUE, withholds_default=True)} + after = omission_apply.withhold_values(before, rules) + omission_apply.check_withheld_values(before, after, rules) + assert after["properties"]["kind"] == {"type": "string", "enum": ["current"]} + assert after["required"] == ["kind"] + + +def test_a_value_absent_from_the_generated_enum_is_reported() -> None: + before = {"properties": {"kind": {"type": "string", "enum": ["public"]}}} + rules = {"kind.gone": Resolved(("kind", "gone"), VALUE)} + with pytest.raises(omission_apply.OmissionError) as exc: + omission_apply.withhold_values(before, rules) + assert "not in the generated enum" in str(exc.value) + + +# --- the check that a build changed exactly what was declared --- + +WITHHELD = {"kind.internal": Resolved(("kind", "internal"), VALUE)} + + +def _mapped() -> dict[str, Any]: + return { + "type": "object", + "properties": { + "kind": { + "type": "string", + "enum": ["public", "internal"], + "description": "Kind.\n\n* `public`\n* `internal`", + }, + "other": {"type": "string", "enum": ["public", "internal"], "default": "public"}, + }, + } + + +def test_withholding_leaves_the_input_untouched_and_passes_the_check() -> None: + before = _mapped() + after = omission_apply.withhold_values(before, WITHHELD) + omission_apply.check_withheld_values(before, after, WITHHELD) + assert after["properties"]["kind"]["enum"] == ["public"] + assert after["properties"]["kind"]["description"] == "Kind.\n\n* `public`" + assert before == _mapped() + + +def test_a_change_to_an_undeclared_field_fails_the_check() -> None: + after = _mapped() + after["properties"]["other"]["enum"] = ["public"] + with pytest.raises(omission_apply.OmissionError) as exc: + omission_apply.check_withheld_values(_mapped(), after, WITHHELD) + assert "other" in str(exc.value) + + +def test_withholding_more_than_was_declared_fails_the_check() -> None: + after = omission_apply.withhold_values(_mapped(), WITHHELD) + after["properties"]["kind"]["enum"] = [] + with pytest.raises(omission_apply.OmissionError): + omission_apply.check_withheld_values(_mapped(), after, WITHHELD) + + +def test_dropping_a_default_that_was_not_withheld_fails_the_check() -> None: + after = omission_apply.withhold_values(_mapped(), WITHHELD) + del after["properties"]["other"]["default"] + with pytest.raises(omission_apply.OmissionError): + omission_apply.check_withheld_values(_mapped(), after, WITHHELD) + + +# --- through schema generation --- + + +def _build(*routes: tuple[str, type[Endpoint]]) -> dict[str, Any]: + """Generate a schema for `routes` the way the docs build configures it: the + project's parameter ordering, and no enum extraction into components.""" + patterns = [url_path(route, view.as_view()) for route, view in routes] + with ( + mock.patch.object(spectacular_settings, "POSTPROCESSING_HOOKS", []), + mock.patch.object(spectacular_settings, "SORT_OPERATION_PARAMETERS", custom_parameter_sort), + ): + return generate_schema(None, patterns=patterns) + + +def _query(schema: dict[str, Any], route: str) -> dict[str, dict[str, Any]]: + parameters = schema["paths"][f"/{route}"]["get"]["parameters"] + return {p["name"]: p for p in parameters if p["in"] == "query"} + + +def test_a_rule_reaches_only_the_operation_whose_serializer_declares_it() -> None: + """Another endpoint with a same-named parameter must not be touched.""" + + @sentry_schema_serializer(omit_from_public_schema={"sort.internal": "Internal."}) + class DeclaringParams(serializers.Serializer): + sort = serializers.ChoiceField(choices=("age", "internal")) + + class UnrelatedParams(serializers.Serializer): + sort = serializers.ChoiceField(choices=("age", "internal")) + + class DeclaringEndpoint(Endpoint): + permission_classes = () + + @extend_schema(parameters=[DeclaringParams]) + def get(self, request): + pass + + class UnrelatedEndpoint(Endpoint): + permission_classes = () + + @extend_schema(parameters=[UnrelatedParams]) + def get(self, request): + pass + + schema = _build(("declaring/", DeclaringEndpoint), ("unrelated/", UnrelatedEndpoint)) + assert _query(schema, "declaring/")["sort"]["schema"]["enum"] == ["age"] + assert _query(schema, "unrelated/")["sort"]["schema"]["enum"] == ["age", "internal"] + + +def test_a_rule_applies_when_the_operation_drops_one_of_the_serializers_fields() -> None: + """A rule that cannot find its operation must not quietly publish the value.""" + + @sentry_schema_serializer(omit_from_public_schema={"kind.internal": "Internal."}) + class DroppedFieldParams(serializers.Serializer): + kind = serializers.ChoiceField(choices=("public", "internal")) + other = serializers.CharField(required=False) + + class DroppedFieldEndpoint(Endpoint): + permission_classes = () + + @extend_schema(parameters=[DroppedFieldParams, OpenApiParameter("other", exclude=True)]) + def get(self, request): + pass + + schema = _build(("dropped/", DroppedFieldEndpoint)) + assert "other" not in _query(schema, "dropped/") + assert _query(schema, "dropped/")["kind"]["schema"]["enum"] == ["public"] + + +def test_a_nested_rule_cannot_hide_a_field_from_another_endpoint_using_the_shape() -> None: + class OwnedShape(serializers.Serializer): + token = serializers.CharField(help_text="Token.") + caption = serializers.CharField(help_text="Caption.") + + with pytest.raises(ValueError) as exc: + + @sentry_schema_serializer(omit_from_public_schema={"only.token": "Internal."}) + class HoldsOwnedShape(serializers.Serializer): + only = OwnedShape(help_text="Only.") + + assert "declare 'token' on OwnedShape" in str(exc.value) + + +def test_a_withheld_choice_leaves_the_parameter_enum_and_description() -> None: + @sentry_schema_serializer(omit_from_public_schema={"kind.internal": "Internal."}) + class DescribedParams(serializers.Serializer): + kind = serializers.ChoiceField(choices=("public", "internal"), help_text="Kind.") + + class DescribedEndpoint(Endpoint): + permission_classes = () + + @extend_schema(parameters=[DescribedParams]) + def get(self, request): + pass + + kind = _query(_build(("described/", DescribedEndpoint)), "described/")["kind"] + assert kind["schema"]["enum"] == ["public"] + assert "* `public`" in kind["description"] + assert "internal" not in kind["description"] + + +def test_withholding_the_default_requires_the_parameter_and_orders_it_first() -> None: + @sentry_schema_serializer(omit_from_public_schema={"kind.legacy": "Send kind explicitly."}) + class DefaultedParams(serializers.Serializer): + a_optional = serializers.CharField(required=False) + kind = serializers.ChoiceField(choices=("legacy", "current"), default="legacy") + + class DefaultedEndpoint(Endpoint): + permission_classes = () + + @extend_schema(parameters=[DefaultedParams]) + def get(self, request): + pass + + schema = _build(("defaulted/", DefaultedEndpoint)) + parameters = schema["paths"]["/defaulted/"]["get"]["parameters"] + assert [p["name"] for p in parameters] == ["kind", "a_optional"] + assert parameters[0]["required"] is True + assert "default" not in parameters[0]["schema"] + + +def test_a_withheld_choice_leaves_a_request_body_component() -> None: + @sentry_schema_serializer(omit_from_public_schema={"kind.internal": "Internal."}) + class WithheldBody(serializers.Serializer): + kind = serializers.ChoiceField(choices=("public", "internal"), help_text="Kind.") + + class BodyEndpoint(Endpoint): + permission_classes = () + + @extend_schema(request=WithheldBody) + def post(self, request): + pass + + schema = _build(("body/", BodyEndpoint)) + assert schema["components"]["schemas"]["WithheldBody"]["properties"]["kind"]["enum"] == [ + "public" + ] + + +def test_a_rule_naming_nothing_fails_the_build() -> None: + @sentry_schema_serializer(omit_from_public_schema={"kind.missing": "Internal."}) + class GhostParams(serializers.Serializer): + kind = serializers.ChoiceField(choices=("public",)) + + class GhostEndpoint(Endpoint): + permission_classes = () + + @extend_schema(parameters=[GhostParams]) + def get(self, request): + pass + + with pytest.raises(SentryApiBuildError) as exc: + _build(("ghost/", GhostEndpoint)) + assert "missing" in str(exc.value) + + +def test_a_parameter_given_explicitly_is_not_marked_required_by_the_serializer() -> None: + """The explicit parameter replaced the serializer's field, so the rule is not its.""" + + @sentry_schema_serializer(omit_from_public_schema={"kind.legacy": "Send kind explicitly."}) + class OverriddenParams(serializers.Serializer): + kind = serializers.ChoiceField(choices=("legacy", "current"), default="legacy") + + class OverriddenEndpoint(Endpoint): + permission_classes = () + + @extend_schema( + parameters=[OverriddenParams, OpenApiParameter("kind", str, description="Kind.")] + ) + def get(self, request): + pass + + kind = _query(_build(("overridden/", OverriddenEndpoint)), "overridden/")["kind"] + assert kind["description"] == "Kind." + assert "required" not in kind + + +# --- the check that marking parameters required changed nothing else --- + +KIND = ("kind", "query") +OTHER = ("other", "query") + + +def _parameters() -> dict[tuple[str, str], Any]: + return { + KIND: {"name": "kind", "in": "query", "schema": {"type": "string", "enum": ["current"]}}, + OTHER: {"name": "other", "in": "query", "schema": {"type": "string"}}, + } + + +def test_marking_the_declared_parameter_passes_the_check() -> None: + after = _parameters() + after[KIND]["required"] = True + omission_apply.check_required_parameters(_parameters(), after, {KIND}) + + +def test_marking_an_undeclared_parameter_fails_the_check() -> None: + after = _parameters() + after[KIND]["required"] = True + after[OTHER]["required"] = True + with pytest.raises(omission_apply.OmissionError) as exc: + omission_apply.check_required_parameters(_parameters(), after, {KIND}) + assert "other" in str(exc.value) + + +def test_changing_more_than_required_on_a_declared_parameter_fails_the_check() -> None: + after = _parameters() + after[KIND]["required"] = True + after[KIND]["schema"] = {"type": "string"} + with pytest.raises(omission_apply.OmissionError): + omission_apply.check_required_parameters(_parameters(), after, {KIND}) + + +def test_leaving_a_declared_parameter_optional_fails_the_check() -> None: + with pytest.raises(omission_apply.OmissionError) as exc: + omission_apply.check_required_parameters(_parameters(), _parameters(), {KIND}) + assert "kind" in str(exc.value) diff --git a/tests/tools/test_flake8_plugin.py b/tests/tools/test_flake8_plugin.py index 3cfeb1bb76ed..74ed6ab6ce79 100644 --- a/tests/tools/test_flake8_plugin.py +++ b/tests/tools/test_flake8_plugin.py @@ -1637,3 +1637,62 @@ def get(self, request) -> Response[X]: return options.get("key", request.GET) """ assert _run_input(src, SHAPED) == [] + + +def _run_s023(src: str) -> list[str]: + return [e for e in _run(src, filename="src/sentry/apidocs/t.py") if "S023" in e] + + +def test_S023_a_deep_path_checks_only_its_first_segment() -> None: + src = """\ +@sentry_schema_serializer( + omit_from_public_schema={"data_source.discover": "Deprecated; use events."} +) +class S(serializers.Serializer): + data_source = serializers.ChoiceField(choices=("discover", "events")) +""" + assert _run_s023(src) == [] + + +def test_S023_a_deep_path_on_an_unknown_field_is_still_a_ghost() -> None: + src = """\ +@sentry_schema_serializer(omit_from_public_schema={"nope.discover": "why"}) +class S(serializers.Serializer): + data_source = serializers.ChoiceField(choices=("discover",)) +""" + errors = _run_s023(src) + assert len(errors) == 1 + assert "'nope'" in errors[0] + + +def test_S023_a_malformed_path_is_reported() -> None: + src = """\ +@sentry_schema_serializer(omit_from_public_schema={"a..b": "why"}) +class S(serializers.Serializer): + a = serializers.CharField() +""" + errors = _run_s023(src) + assert len(errors) == 1 + assert "not a usable path" in errors[0] + + +def test_S023_deprecate_needs_a_reason_too() -> None: + src = """\ +@sentry_schema_serializer(deprecate={"name": " "}) +class S(serializers.Serializer): + name = serializers.CharField() +""" + errors = _run_s023(src) + assert len(errors) == 1 + assert "needs a reason" in errors[0] + + +def test_S023_deprecate_reports_a_ghost() -> None: + src = """\ +@sentry_schema_serializer(deprecate={"gone": "Use slug."}) +class S(serializers.Serializer): + name = serializers.CharField() +""" + errors = _run_s023(src) + assert len(errors) == 1 + assert "deprecate='gone'" in errors[0] diff --git a/tools/flake8_plugin.py b/tools/flake8_plugin.py index 366fb0c27a9a..a9c9df0d4d8d 100644 --- a/tools/flake8_plugin.py +++ b/tools/flake8_plugin.py @@ -138,6 +138,10 @@ S023_ghost_msg = ( "S023 {}={!r} names no field on this class; it omits nothing and should be deleted." ) +S023_bad_path_msg = ( + "S023 {}={!r} is not a usable path. Write 'field' or 'field.choice', " + "with no leading, trailing or doubled dots." +) S024_msg = ( "S024 This module discovers .py files and parses them with ast, which is a " @@ -953,31 +957,36 @@ def _check_S023(self, node: ast.ClassDef) -> None: S023_ghost_msg.format("exclude_fields", field), ) ) - elif kw.arg == "omit_from_public_schema": - if not isinstance(kw.value, ast.Dict): - self.errors.append((dec.lineno, dec.col_offset, S023_not_mapping_msg)) - continue - for k, v in zip(kw.value.keys, kw.value.values): - if not isinstance(k, ast.Constant) or not isinstance(k.value, str): - continue - field = k.value - reason = _joined_str(v) - if reason is not None and not reason.strip(): - self.errors.append( - ( - dec.lineno, - dec.col_offset, - S023_blank_reason_msg.format(field), - ) - ) - if not open_class and field not in fields: - self.errors.append( - ( - dec.lineno, - dec.col_offset, - S023_ghost_msg.format("omit_from_public_schema", field), - ) - ) + elif kw.arg in ("omit_from_public_schema", "deprecate"): + self._check_paths(dec, kw, fields, open_class) + + def _check_paths( + self, dec: ast.Call, kw: ast.keyword, fields: set[str], open_class: bool + ) -> None: + """Reasons and grammar for a {path: reason} mapping. + + Only the first segment is a field here; the schema build resolves the rest.""" + if not isinstance(kw.value, ast.Dict): + self.errors.append((dec.lineno, dec.col_offset, S023_not_mapping_msg)) + return + assert kw.arg is not None + for k, v in zip(kw.value.keys, kw.value.values): + if not isinstance(k, ast.Constant) or not isinstance(k.value, str): + continue + path = k.value + segments = path.split(".") + if any(not segment.strip() for segment in segments): + self.errors.append( + (dec.lineno, dec.col_offset, S023_bad_path_msg.format(kw.arg, path)) + ) + continue + reason = _joined_str(v) + if reason is not None and not reason.strip(): + self.errors.append((dec.lineno, dec.col_offset, S023_blank_reason_msg.format(path))) + if not open_class and segments[0] not in fields: + self.errors.append( + (dec.lineno, dec.col_offset, S023_ghost_msg.format(kw.arg, segments[0])) + ) def _s024_visit_call(self, node: ast.Call) -> None: func = node.func From cb08f45a179d4476cb91c3aab026d3180132aae5 Mon Sep 17 00:00:00 2001 From: Jeremy Stanley Date: Mon, 14 Sep 2026 13:03:11 -0700 Subject: [PATCH 2/3] feat(apidocs): Declare /replay-count/ parameters from its validator The hand-written parameter constants drifted from the validator: they omitted the spans data source for weeks and documented query as optional although the validator requires it. The endpoint now passes its validator to the schema, withholding the deprecated discover and transactions data sources. Co-Authored-By: Claude Opus 5 --- src/sentry/apidocs/parameters.py | 23 ---------------- .../endpoints/organization_replay_count.py | 26 ++++++++++++++----- 2 files changed, 19 insertions(+), 30 deletions(-) diff --git a/src/sentry/apidocs/parameters.py b/src/sentry/apidocs/parameters.py index cec8e6efe474..031784fd58f9 100644 --- a/src/sentry/apidocs/parameters.py +++ b/src/sentry/apidocs/parameters.py @@ -6,8 +6,6 @@ from sentry import constants from sentry.api.helpers.projects import PROJECT_ID_OR_SLUG_SCHEMA -from sentry.search.eap.types import SupportedTraceItemType -from sentry.snuba.dataset import Dataset from sentry.snuba.sessions import STATS_PERIODS # NOTE: Please add new params by path vs query, then in alphabetical order @@ -1112,27 +1110,6 @@ class ReplayParams: description="""The ID of the replay deletion job you'd like to retrieve.""", ) - DATA_SOURCE = OpenApiParameter( - name="data_source", - location="query", - required=True, - type=OpenApiTypes.STR, - enum=[ - Dataset.Events.value, - Dataset.IssuePlatform.value, - SupportedTraceItemType.SPANS.value, - ], - description="The data source to query replays from.", - ) - - RETURN_IDS = OpenApiParameter( - name="returnIds", - location="query", - required=False, - type=OpenApiTypes.BOOL, - description="If true, return issue IDs rather than counts.", - ) - class NotificationParams: TRIGGER_TYPE = OpenApiParameter( diff --git a/src/sentry/replays/endpoints/organization_replay_count.py b/src/sentry/replays/endpoints/organization_replay_count.py index 94afa51ec269..601d29200a47 100644 --- a/src/sentry/replays/endpoints/organization_replay_count.py +++ b/src/sentry/replays/endpoints/organization_replay_count.py @@ -14,11 +14,10 @@ from sentry.api.bases.organization_events import OrganizationEventsEndpointBase from sentry.apidocs.constants import RESPONSE_BAD_REQUEST, RESPONSE_FORBIDDEN from sentry.apidocs.examples.replay_examples import ReplayExamples +from sentry.apidocs.omissions import sentry_schema_serializer from sentry.apidocs.parameters import ( GlobalParams, OrganizationParams, - ReplayParams, - VisibilityParams, ) from sentry.apidocs.response_types import DetailResponse from sentry.apidocs.utils import inline_sentry_response_serializer @@ -33,8 +32,20 @@ from sentry.types.ratelimit import RateLimit, RateLimitCategory +@sentry_schema_serializer( + omit_from_public_schema={ + "data_source.discover": "Deprecated 2026-07; use events. Send data_source explicitly.", + "data_source.transactions": "Deprecated 2026-07; use spans. Still accepted until blocked.", + } +) class ReplayCountQueryParamsValidator(serializers.Serializer): - query = serializers.CharField(required=True) + query = serializers.CharField( + required=True, + help_text="""Filters results by using [query syntax](/product/sentry-basics/search/). + +Example: `query=(transaction:foo AND release:abc) OR (transaction:[bar,baz] AND release:def)` +""", + ) data_source = serializers.ChoiceField( choices=( Dataset.Discover.value, @@ -44,8 +55,11 @@ class ReplayCountQueryParamsValidator(serializers.Serializer): SupportedTraceItemType.SPANS.value, ), default=Dataset.Discover.value, + help_text="The data source to query replays from.", + ) + returnIds = serializers.BooleanField( + default=False, help_text="If true, return issue IDs rather than counts." ) - returnIds = serializers.BooleanField(default=False) @cell_silo_endpoint @@ -83,9 +97,7 @@ class OrganizationReplayCountEndpoint(OrganizationEventsEndpointBase): GlobalParams.STATS_PERIOD, OrganizationParams.PROJECT, OrganizationParams.PROJECT_ID_OR_SLUG, - VisibilityParams.QUERY, - ReplayParams.DATA_SOURCE, - ReplayParams.RETURN_IDS, + ReplayCountQueryParamsValidator, ], responses={ 200: inline_sentry_response_serializer("ReplayCounts", dict[int, int]), From 9ef1f3cc6a64c1f0f131adbbdecbd9fbc57ed21f Mon Sep 17 00:00:00 2001 From: Jeremy Stanley Date: Mon, 14 Sep 2026 13:39:02 -0700 Subject: [PATCH 3/3] fix(apidocs): Check omissions inside unions and on replaced parameters The build's omission check compared union arms as one list, so an omission on a TypedDict inside a union was reported as undeclared and a missed one went unnoticed. Arms are now compared one by one. A serializer's declarations were recorded while it was exploded into query parameters, before an explicit OpenApiParameter could replace one of its fields, so the check held the replacement to the serializer's rule. They are now recorded once the operation's parameters are final, and only for the fields the serializer still provides. Co-Authored-By: Claude Opus 5 --- src/sentry/apidocs/omission_apply.py | 9 +++ src/sentry/apidocs/omission_guard.py | 13 +++- src/sentry/apidocs/schema.py | 32 +++++++-- tests/sentry/apidocs/test_omission_guard.py | 76 +++++++++++++++++++++ 4 files changed, 121 insertions(+), 9 deletions(-) diff --git a/src/sentry/apidocs/omission_apply.py b/src/sentry/apidocs/omission_apply.py index 026acee0746d..eb278a4348ca 100644 --- a/src/sentry/apidocs/omission_apply.py +++ b/src/sentry/apidocs/omission_apply.py @@ -144,6 +144,15 @@ def _differences( yield from _differences( old.get(name, _ABSENT), new.get(name, _ABSENT), (*key, str(name)) ) + elif ( + isinstance(old, list) + and isinstance(new, list) + and key[-1:] in (("anyOf",), ("oneOf",), ("allOf",)) + and len(old) == len(new) + ): + # Union arms are compared one by one, so a change inside one can be placed. + for index, (old_arm, new_arm) in enumerate(zip(old, new)): + yield from _differences(old_arm, new_arm, (*key, str(index))) elif old != new: yield key, old, new diff --git a/src/sentry/apidocs/omission_guard.py b/src/sentry/apidocs/omission_guard.py index f3ab14856b75..8da82273cfb3 100644 --- a/src/sentry/apidocs/omission_guard.py +++ b/src/sentry/apidocs/omission_guard.py @@ -62,6 +62,12 @@ def omissions_enabled() -> bool: return not isinstance(_mode.get(), _Disabled) +def in_operation() -> bool: + """Whether serializers mapped now become an operation's query parameters.""" + placement = _placement.get() + return placement is not None and placement[0] == "operations" + + @contextlib.contextmanager def omissions_disabled() -> Iterator[None]: token = _mode.set(_DISABLED) @@ -404,9 +410,12 @@ def _missing(after: Mapping[str, Any], recorded: Mapping[Location, Declared]) -> def _at(node: Any, pointer: Pointer) -> Mapping[str, Any] | None: for key in pointer: - if not isinstance(node, Mapping): + if isinstance(node, Mapping): + node = node.get(key) + elif isinstance(node, list) and key.isdigit() and int(key) < len(node): + node = node[int(key)] + else: return None - node = node.get(key) return node if isinstance(node, Mapping) else None diff --git a/src/sentry/apidocs/schema.py b/src/sentry/apidocs/schema.py index 782d77bb6169..edd0b1482180 100644 --- a/src/sentry/apidocs/schema.py +++ b/src/sentry/apidocs/schema.py @@ -15,7 +15,7 @@ choice_rules, withhold_values, ) -from sentry.apidocs.omission_guard import omissions_enabled, placed_at, record +from sentry.apidocs.omission_guard import in_operation, omissions_enabled, placed_at, record from sentry.apidocs.omission_paths import Resolved from sentry.apidocs.omissions import DEPRECATION_REASONS_OVERRIDE, OMISSION_REASONS_OVERRIDE from sentry.apidocs.utils import SentryApiBuildError @@ -68,12 +68,9 @@ def _map_serializer( schema = super()._map_serializer(instance, direction, bypass_extensions) rules = _choice_rules(instance) try: - record( - type(instance).__name__, - _declared_fields(instance, OMISSION_REASONS_OVERRIDE), - _declared_fields(instance, DEPRECATION_REASONS_OVERRIDE), - rules, - ) + if not in_operation(): + # Query parameters are recorded once explicit ones have replaced fields. + _record(instance, rules) if not rules: return schema withheld = withhold_values(schema, rules) @@ -101,6 +98,13 @@ def _process_override_parameters(self, direction: Any = "request") -> Any: for name in force_instance(parameter).fields: source[name, OpenApiParameter.QUERY] = parameter serializers = {id(p): p for p in source.values() if not isinstance(p, OpenApiParameter)} + try: + with placed_at(("operations", self.get_operation_id())): + for serializer in serializers.values(): + owned = {name for (name, _), src in source.items() if src is serializer} + _record(force_instance(serializer), _choice_rules(serializer), owned) + except OmissionError as exc: + raise SentryApiBuildError(str(exc)) from exc required: set[tuple[str, str]] = set() for serializer in serializers.values(): for rule in _choice_rules(serializer).values(): @@ -137,6 +141,20 @@ def _lift_deprecated(parameters: dict[tuple[str, str], Any]) -> None: parameter["deprecated"] = True +def _record(instance: Any, rules: dict[str, Resolved], owned: set[str] | None = None) -> None: + """Note `instance`'s declarations for the build's check, limited to `owned` fields.""" + + def kept(names: set[str]) -> set[str]: + return names if owned is None else names & owned + + record( + type(instance).__name__, + kept(_declared_fields(instance, OMISSION_REASONS_OVERRIDE)), + kept(_declared_fields(instance, DEPRECATION_REASONS_OVERRIDE)), + {path: rule for path, rule in rules.items() if owned is None or rule.segments[0] in owned}, + ) + + def _declared_fields(serializer: Any, key: str) -> set[str]: """Whole fields the decorator declared under `key`, which drf-spectacular applies.""" return {path for path in get_override(serializer, key, {}) or {} if "." not in path} diff --git a/tests/sentry/apidocs/test_omission_guard.py b/tests/sentry/apidocs/test_omission_guard.py index 7a9f571142f4..20b3d9242dfb 100644 --- a/tests/sentry/apidocs/test_omission_guard.py +++ b/tests/sentry/apidocs/test_omission_guard.py @@ -330,3 +330,79 @@ def test_deprecating_a_nested_serializer_field_passes_the_guard() -> None: shape = result["components"]["schemas"]["DeprecatedNestedBody"]["properties"]["shape"] assert shape["deprecated"] is True assert shape["allOf"] == [{"$ref": "#/components/schemas/NestedShape"}] + + +@sentry_schema_serializer(omit_from_public_schema={"secret": "Internal."}) +class UnionItem(TypedDict): + name: str + secret: str + + +class UnionResponse(TypedDict): + value: UnionItem | int + + +class UnionResponseSerializer(ResponseSerializer): + def serialize(self, obj: Any, attrs: Any, user: Any, **kwargs: Any) -> UnionResponse: + raise NotImplementedError + + +class UnionEndpoint(Endpoint): + permission_classes = () + + @extend_schema(operation_id="union", responses={200: UnionResponseSerializer}) + def get(self, request): + pass + + +def _build_union(hooks: Sequence[Hook] = ()) -> dict[str, Any]: + patterns = [url_path("union/", UnionEndpoint.as_view())] + with mock.patch.object(spectacular_settings, "POSTPROCESSING_HOOKS", list(hooks)): + return CustomGenerator(patterns=patterns).get_schema(request=None, public=True) + + +def _union_item(result: dict[str, Any]) -> dict[str, Any]: + value = result["components"]["schemas"]["UnionResponse"]["properties"]["value"] + return next(arm for arm in value["anyOf"] if "properties" in arm) + + +def test_a_typed_dict_omission_inside_a_union_passes_the_guard() -> None: + assert "secret" not in _union_item(_build_union())["properties"] + + +def test_a_typed_dict_omission_restored_inside_a_union_fails_the_build() -> None: + def restore(result: dict[str, Any], **kwargs: Any) -> dict[str, Any]: + _union_item(result)["properties"].setdefault("secret", {"type": "string"}) + return result + + with pytest.raises(SentryApiBuildError) as exc: + _build_union([restore]) + assert "secret is still present" in str(exc.value) + + +@sentry_schema_serializer(omit_from_public_schema={"mode.legacy": "Send mode explicitly."}) +class ReplacedDefaultParams(serializers.Serializer): + mode = serializers.ChoiceField(choices=("legacy", "current"), default="legacy") + + +class ReplacedDefaultEndpoint(Endpoint): + permission_classes = () + + @extend_schema( + operation_id="replacedDefault", + parameters=[ReplacedDefaultParams, OpenApiParameter("mode", str, description="Mode.")], + ) + def get(self, request): + pass + + +def test_a_field_replaced_by_an_explicit_parameter_is_not_checked_as_the_serializers() -> None: + """The explicit parameter is what the operation documents, so the rule is not its.""" + patterns = [url_path("replaced/", ReplacedDefaultEndpoint.as_view())] + with mock.patch.object(spectacular_settings, "POSTPROCESSING_HOOKS", []): + result = CustomGenerator(patterns=patterns).get_schema(request=None, public=True) + mode = next( + p for p in result["paths"]["/replaced/"]["get"]["parameters"] if p["name"] == "mode" + ) + assert mode["description"] == "Mode." + assert "required" not in mode