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..eb278a4348ca --- /dev/null +++ b/src/sentry/apidocs/omission_apply.py @@ -0,0 +1,196 @@ +"""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 ( + 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 + + +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..8da82273cfb3 --- /dev/null +++ b/src/sentry/apidocs/omission_guard.py @@ -0,0 +1,433 @@ +"""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) + + +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) + 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 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 + 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/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/apidocs/schema.py b/src/sentry/apidocs/schema.py index 25b046c53d3a..edd0b1482180 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 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 class SentrySchema(AutoSchema): @@ -26,3 +45,147 @@ 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: + 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) + 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)} + 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(): + 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 _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} + + +@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/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]), diff --git a/tests/sentry/apidocs/test_omission_guard.py b/tests/sentry/apidocs/test_omission_guard.py new file mode 100644 index 000000000000..20b3d9242dfb --- /dev/null +++ b/tests/sentry/apidocs/test_omission_guard.py @@ -0,0 +1,408 @@ +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"}] + + +@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 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