diff --git a/src/sentry/api/base.py b/src/sentry/api/base.py index 1de6858d72bd..9d04c18503c6 100644 --- a/src/sentry/api/base.py +++ b/src/sentry/api/base.py @@ -268,6 +268,9 @@ class Endpoint(APIView): owner: ApiOwner = ApiOwner.UNOWNED publish_status: dict[HTTP_METHOD_NAME, ApiPublishStatus] = {} + # Non-safe methods that a client may resend after a timeout or 5xx without + # changing the outcome. Published as `x-sentry-idempotent`; see apidocs/README.md. + idempotent_methods: frozenset[HTTP_METHOD_NAME] = frozenset() rate_limits: RateLimitConfig | Callable[..., RateLimitConfig] = DEFAULT_RATE_LIMIT_CONFIG enforce_rate_limit: bool = settings.SENTRY_RATELIMITER_ENABLED servers: list[dict[str, Any]] | None = None diff --git a/src/sentry/api/endpoints/chunk.py b/src/sentry/api/endpoints/chunk.py index acc08153a121..92281ab1df86 100644 --- a/src/sentry/api/endpoints/chunk.py +++ b/src/sentry/api/endpoints/chunk.py @@ -132,6 +132,8 @@ class ChunkUploadEndpoint(OrganizationEndpoint): "GET": ApiPublishStatus.PRIVATE, "POST": ApiPublishStatus.PRIVATE, } + # Blobs are unique per checksum, so resending a chunk leaves the same single blob. + idempotent_methods = frozenset({"POST"}) owner = ApiOwner.OWNERS_INGEST authentication_classes = OrganizationEndpoint.authentication_classes + ( diff --git a/src/sentry/api/endpoints/debug_files.py b/src/sentry/api/endpoints/debug_files.py index bf6ec565f6a3..aac427d32140 100644 --- a/src/sentry/api/endpoints/debug_files.py +++ b/src/sentry/api/endpoints/debug_files.py @@ -985,6 +985,9 @@ class DifAssembleEndpoint(ProjectEndpoint): publish_status = { "POST": ApiPublishStatus.PRIVATE, } + # Assemble state is cached per checksum, so a resend reports it instead of + # starting a second job. + idempotent_methods = frozenset({"POST"}) permission_classes = (ProjectReleasePermission,) def post(self, request: Request, project: Project) -> Response: diff --git a/src/sentry/api/endpoints/organization_artifactbundle_assemble.py b/src/sentry/api/endpoints/organization_artifactbundle_assemble.py index bc1a5b42e55c..d67459f8d735 100644 --- a/src/sentry/api/endpoints/organization_artifactbundle_assemble.py +++ b/src/sentry/api/endpoints/organization_artifactbundle_assemble.py @@ -28,6 +28,9 @@ class OrganizationArtifactBundleAssembleEndpoint(OrganizationReleasesBaseEndpoin publish_status = { "POST": ApiPublishStatus.PRIVATE, } + # Assemble state is cached per checksum, so a resend reports it instead of + # starting a second job. + idempotent_methods = frozenset({"POST"}) def post(self, request: Request, organization) -> Response: """ diff --git a/src/sentry/apidocs/README.md b/src/sentry/apidocs/README.md index c0a295c8d279..36aff8341162 100644 --- a/src/sentry/apidocs/README.md +++ b/src/sentry/apidocs/README.md @@ -45,3 +45,24 @@ An organization's public locality URL remains runtime information supplied by bootstrap, configuration, organization listings (`links.regionUrl`), or optional discovery. This extension contains no deployment URLs, credentials, private cell addresses, cache policy, or automatic host selection. + +# Idempotent operations + +An operation carries `"x-sentry-idempotent": true` when its endpoint lists the +method in `idempotent_methods`: + +```python +class DifAssembleEndpoint(ProjectEndpoint): + publish_status = {"POST": ApiPublishStatus.PRIVATE} + idempotent_methods = frozenset({"POST"}) +``` + +It means a client may resend the identical request after a timeout or a 5xx +response without changing the outcome, so generated clients can retry it like a +`GET`. Only list methods that are not already safe (RFC 9110 §9.2.1): the build +fails for `GET`, `HEAD`, `OPTIONS`, `TRACE`, or a method the endpoint does not +declare in `publish_status`. A missing marker means "not known to be idempotent", +including `PUT` and `DELETE`, whose handlers do not all live up to RFC 9110 §9.2.2. + +Like routing, declarations are collected for every endpoint and reset on every +build. The marker appears only on operations that are in the generated schema. diff --git a/src/sentry/apidocs/hooks.py b/src/sentry/apidocs/hooks.py index 6075bd3fa6b7..d413163ac294 100644 --- a/src/sentry/apidocs/hooks.py +++ b/src/sentry/apidocs/hooks.py @@ -144,6 +144,13 @@ class RoutingMetadata(TypedDict): organizationParameter: NotRequired[str] +# (canonical path, lowercased method) pairs whose endpoint lists the method in +# `idempotent_methods`. Collected for unpublished endpoints too, like silos. +_IDEMPOTENT_OPERATIONS: set[tuple[str, str]] = set() + +# RFC 9110 9.2.1: these never change server state, so clients may always resend them. +_SAFE_METHODS = frozenset({"GET", "HEAD", "OPTIONS", "TRACE"}) + # Include non-spectacular endpoints so existing APPEND_PATHS operations can use # their backend declaration without changing which methods are published. _OPERATION_SILOS: dict[tuple[str, str], DeclaredSilo] = {} @@ -176,6 +183,7 @@ def custom_preprocessing_hook(endpoints: Any) -> Any: # TODO: organize method, _ENDPOINT_SERVERS.clear() _EXPERIMENTAL_OPERATIONS.clear() _OPERATION_SILOS.clear() + _IDEMPOTENT_OPERATIONS.clear() filtered = [] ownership_data: dict[ApiOwner, dict] = {} @@ -208,13 +216,26 @@ def custom_preprocessing_hook(endpoints: Any) -> Any: # TODO: organize method, f"All methods must declare a publish_status. Please add a valid publish status for Endpoint {callback.view_class} {method} method.", ) + idempotent_methods: frozenset[str] = getattr( + callback.view_class, "idempotent_methods", frozenset() + ) + if not idempotent_methods <= callback.view_class.publish_status.keys(): + raise SentryApiBuildError( + f"Endpoint {callback.view_class} declares idempotent_methods that it does not implement: {sorted(idempotent_methods - callback.view_class.publish_status.keys())}.", + ) + if idempotent_methods & _SAFE_METHODS: + raise SentryApiBuildError( + f"Endpoint {callback.view_class} lists safe methods in idempotent_methods: {sorted(idempotent_methods & _SAFE_METHODS)}. Safe methods are always idempotent; only list non-safe methods such as POST.", + ) + if any(path.startswith(p) for p in EXCLUSION_PATH_PREFIXES): pass else: - _OPERATION_SILOS[(_canonical_issue_path(path), method.lower())] = _declared_silo( - callback.view_class - ) + operation_key = (_canonical_issue_path(path), method.lower()) + _OPERATION_SILOS[operation_key] = _declared_silo(callback.view_class) + if method in idempotent_methods: + _IDEMPOTENT_OPERATIONS.add(operation_key) # endpoints that are documented via tooling status = callback.view_class.publish_status.get(method) if status is not None and status.is_published: @@ -299,10 +320,11 @@ def custom_postprocessing_hook(result: Any, generator: Any, **kwargs: Any) -> An # Stamp before alias canonicalization so metadata stays with its operation. for path, operations in result["paths"].items(): for method, operation in operations.items(): - routing: RoutingMetadata = { - "silo": _OPERATION_SILOS.get((_canonical_issue_path(path), method), "unknown") - } + operation_key = (_canonical_issue_path(path), method) + routing: RoutingMetadata = {"silo": _OPERATION_SILOS.get(operation_key, "unknown")} operation["x-sentry-routing"] = routing + if operation_key in _IDEMPOTENT_OPERATIONS: + operation["x-sentry-idempotent"] = True # Add servers override from endpoint class definitions for path, servers in _ENDPOINT_SERVERS.items(): diff --git a/tests/apidocs/test_hooks.py b/tests/apidocs/test_hooks.py index 20a8e5bb1ccb..b9e5f2b1eb2e 100644 --- a/tests/apidocs/test_hooks.py +++ b/tests/apidocs/test_hooks.py @@ -13,6 +13,7 @@ from sentry.apidocs.hooks import ( _ENDPOINT_SERVERS, _EXPERIMENTAL_OPERATIONS, + _IDEMPOTENT_OPERATIONS, _OPERATION_SILOS, EXPERIMENTAL_NOTICE, CustomEndpointEnumerator, @@ -169,6 +170,106 @@ def test_experimental_marker_stamped_on_operation(self) -> None: assert processed["paths"]["/api/0/public/"]["get"]["description"] == "A stable endpoint" +@patch("sentry.apidocs.hooks.__write_ownership_data") +class IdempotentOperationsTest(TestCase): + """Declared idempotent methods are collected for every endpoint and stamped as a marker.""" + + def setUp(self) -> None: + _IDEMPOTENT_OPERATIONS.clear() + _OPERATION_SILOS.clear() + + def tearDown(self) -> None: + _ENDPOINT_SERVERS.clear() + _EXPERIMENTAL_OPERATIONS.clear() + _IDEMPOTENT_OPERATIONS.clear() + _OPERATION_SILOS.clear() + + def _endpoints( + self, path: str, methods: dict[str, ApiPublishStatus], idempotent: frozenset[str] + ) -> list[tuple[Any, Any, str, Any]]: + view_class = type( + "FakeEndpoint", + (), + { + "owner": ApiOwner.CRONS, + "publish_status": methods, + "idempotent_methods": idempotent, + "servers": None, + }, + ) + callback = type("FakeCallback", (), {"view_class": view_class}) + return [(path, path, method, callback) for method in methods] + + def _operation(self, operation_id: str) -> dict[str, Any]: + return { + "tags": ["Events"], + "description": "An endpoint", + "operationId": operation_id, + "parameters": [], + } + + def test_only_declared_methods_are_marked(self, _write_ownership: Any) -> None: + custom_preprocessing_hook( + self._endpoints( + "/api/0/assemble/", + {"POST": ApiPublishStatus.PUBLIC, "PUT": ApiPublishStatus.PUBLIC}, + frozenset({"POST"}), + ) + ) + result = { + "components": {"schemas": {}}, + "paths": { + "/api/0/assemble/": { + "post": self._operation("assemble"), + "put": self._operation("update-assembly"), + }, + }, + } + + processed = custom_postprocessing_hook(result, None) + + operations = processed["paths"]["/api/0/assemble/"] + assert operations["post"]["x-sentry-idempotent"] is True + assert "x-sentry-idempotent" not in operations["put"] + + def test_unpublished_methods_are_collected(self, _write_ownership: Any) -> None: + filtered = custom_preprocessing_hook( + self._endpoints( + "/api/0/private-assemble/", + {"POST": ApiPublishStatus.PRIVATE}, + frozenset({"POST"}), + ) + ) + + assert filtered == [] + assert _IDEMPOTENT_OPERATIONS == {("/api/0/private-assemble/", "post")} + + def test_next_build_does_not_reuse_previous_markers(self, _write_ownership: Any) -> None: + _IDEMPOTENT_OPERATIONS.add(("/api/0/stale/", "post")) + + custom_preprocessing_hook( + self._endpoints("/api/0/fresh/", {"POST": ApiPublishStatus.PUBLIC}, frozenset()) + ) + + assert _IDEMPOTENT_OPERATIONS == set() + + def test_undeclared_method_raises(self, _write_ownership: Any) -> None: + endpoints = self._endpoints( + "/api/0/assemble/", {"POST": ApiPublishStatus.PUBLIC}, frozenset({"PUT"}) + ) + + with pytest.raises(SentryApiBuildError, match="does not implement"): + custom_preprocessing_hook(endpoints) + + def test_safe_method_raises(self, _write_ownership: Any) -> None: + endpoints = self._endpoints( + "/api/0/assemble/", {"GET": ApiPublishStatus.PUBLIC}, frozenset({"GET"}) + ) + + with pytest.raises(SentryApiBuildError, match="safe methods"): + custom_preprocessing_hook(endpoints) + + class SummaryUniquenessTest(TestCase): def _operation(self, summary: str) -> dict[str, Any]: return {