fix(investigations): Raise apply errors and type ActionInvalid's messages

Author: wedamija

src/sentry/investigations/endpoints/organization_investigation_action_request_apply.py

@@ -22,7 +22,11 @@
 )
 from sentry.investigations.endpoints.validators import ActionRequestApplyValidator
 from sentry.investigations.models import Investigation
-from sentry.investigations.services.action_kinds.base import ApplyErrorCode, CannotApplyReason
+from sentry.investigations.services.action_kinds.base import (
+    ApplyErrorCode,
+    ApplyFailed,
+    CannotApplyReason,
+)
 from sentry.investigations.services.action_requests import apply_action_request
 from sentry.models.organization import Organization
 
@@ -79,18 +83,19 @@ def post(
         validator = ActionRequestApplyValidator(data=request.data)
         if not validator.is_valid():
             return Response(validator.errors, status=status.HTTP_400_BAD_REQUEST)
-        result = apply_action_request(
-            investigation=investigation,
-            action_request_id=action_request_id,
-            request=request,
-            actor_id=actor_id,
-            dry_run=validator.validated_data["dry_run"],
-        )
-        if result.error is not None:
-            return Response(result.error, status=_STATUS.get(result.error["code"], 409))
+        try:
+            action_request = apply_action_request(
+                investigation=investigation,
+                action_request_id=action_request_id,
+                request=request,
+                actor_id=actor_id,
+                dry_run=validator.validated_data["dry_run"],
+            )
+        except ApplyFailed as failed:
+            return Response(failed.error, status=_STATUS.get(failed.error["code"], 409))
         body: InvestigationActionRequestApplyResponse = {
             "actionRequest": serialize(
-                result.action_request,
+                action_request,
                 request.user,
                 InvestigationActionRequestSerializer(access=request.access),
             ),

src/sentry/investigations/services/action_kinds/base.py

@@ -1,10 +1,10 @@
 from __future__ import annotations
 
 import abc
-from collections.abc import Sequence
+from collections.abc import Mapping, Sequence
 from dataclasses import dataclass
 from enum import StrEnum
-from typing import Any, NotRequired, TypedDict
+from typing import NotRequired, TypedDict
 
 from django.contrib.auth.models import AnonymousUser
 from rest_framework.request import Request
@@ -65,10 +65,10 @@ class ActionRequestError(TypedDict):
     messages: NotRequired[list[str]]
 
 
-@dataclass(frozen=True)
-class ApplyResult:
-    action_request: InvestigationActionRequest
-    error: ApplyError | None = None
+class ApplyFailed(Exception):
+    def __init__(self, error: ApplyError) -> None:
+        super().__init__(error["detail"])
+        self.error = error
 
 
 @dataclass(frozen=True)
@@ -91,11 +91,11 @@ def __init__(self, current: dict[str, SettingValue]) -> None:
 
 
 class ActionInvalid(Exception):
-    """The change wouldn't save. `errors` are the validator's errors."""
+    """The change wouldn't save, and why."""
 
-    def __init__(self, errors: dict[str, Any] | list[Any]) -> None:
+    def __init__(self, messages: list[str]) -> None:
         super().__init__("The action is invalid.")
-        self.errors = errors
+        self.messages = messages
 
 
 class ActionKind(abc.ABC):
@@ -121,7 +121,7 @@ def check(self, context: ApplyContext) -> None:
         cannot_apply_reasons can leave this as is.
         """
 
-    def apply(self, context: ApplyContext) -> dict[str, object]:
+    def apply(self, context: ApplyContext) -> Mapping[str, object]:
         """Make the change, and return what the activity feed should show about it. Raise
         ActionStale or ActionInvalid instead of updating the request; the caller records it.
         """

src/sentry/investigations/services/action_kinds/update_detector.py

@@ -27,7 +27,11 @@
     InvestigationActionRequest,
     InvestigationOrchestrationRun,
 )
-from sentry.investigations.services.action_kinds.base import ActionKind, CannotApplyReason
+from sentry.investigations.services.action_kinds.base import (
+    ActionKind,
+    CannotApplyReason,
+    SettingValue,
+)
 from sentry.models.organization import Organization
 from sentry.snuba.models import SnubaQuery
 from sentry.users.models.user import User
@@ -124,8 +128,6 @@ def trim(cls, action: dict[str, Any]) -> dict[str, Any] | None:
         }
 
 
-DetectorValue = int | float | str | None
-
 _THRESHOLD_RESULTS: dict[
     Literal["highThreshold", "mediumThreshold", "resolutionThreshold"], DetectorPriorityLevel
 ] = {
@@ -211,12 +213,12 @@ def load(cls, detector: Detector, snuba_query: SnubaQuery) -> DetectorState:
         )
         return cls(data_source=data_source, condition_group=condition_group)
 
-    def values(self) -> dict[str, DetectorValue]:
+    def values(self) -> dict[str, SettingValue]:
         """The monitor's current settings, keyed like Seer's changes. A threshold is left out
         unless exactly one condition sets it.
         """
 
-        values: dict[str, DetectorValue] = {
+        values: dict[str, SettingValue] = {
             "timeWindowSeconds": self.data_source["timeWindow"],
             "query": self.data_source["query"],
             "aggregate": self.data_source["aggregate"],

src/sentry/investigations/services/action_requests.py

@@ -20,12 +20,13 @@
 from sentry.investigations.services.action_kinds import get_action_kind, shared_cannot_apply_reason
 from sentry.investigations.services.action_kinds.base import (
     ActionInvalid,
+    ActionKind,
     ActionRequestError,
     ActionStale,
     ApplyContext,
     ApplyError,
     ApplyErrorCode,
-    ApplyResult,
+    ApplyFailed,
     CannotApplyReason,
 )
 
@@ -229,11 +230,11 @@ def supersede_block_action_requests(investigation: Investigation, block_keys: Se
     )
 
 
-# The usual detail for a reason; anything a kind refuses gets the last one.
 _REASON_DETAILS = {
     CannotApplyReason.INVESTIGATION_ARCHIVED: "Archived investigations are read-only.",
     CannotApplyReason.UNSUPPORTED_ACTION: "This request has no action to apply.",
 }
+_DEFAULT_REASON_DETAIL = "This action can't be applied."
 
 
 def _not_suggested_error(action_request: InvestigationActionRequest) -> ApplyError:
@@ -254,22 +255,39 @@ def _not_suggested_error(action_request: InvestigationActionRequest) -> ApplyErr
     }
 
 
-def _error_messages(errors: object) -> list[str]:
-    if isinstance(errors, dict):
-        return [message for value in errors.values() for message in _error_messages(value)]
-    if isinstance(errors, list):
-        return [message for value in errors for message in _error_messages(value)]
-    return [str(errors)]
+def _check_can_apply(
+    investigation: Investigation, action_request: InvestigationActionRequest, request: Request
+) -> ActionKind:
+    """Return the request's kind, or raise ApplyFailed if the viewer can't apply it."""
+
+    reason = shared_cannot_apply_reason(investigation, action_request)
+    if reason == CannotApplyReason.NOT_SUGGESTED:
+        raise ApplyFailed(_not_suggested_error(action_request))
+    if reason is None:
+        kind = get_action_kind(action_request.action_kind)
+        assert kind is not None
+        # The same check that decides canApply guards the endpoint, so a kind can't forget it.
+        reason = kind.cannot_apply_reasons(
+            [action_request],
+            investigations={investigation.id: investigation},
+            access=request.access,
+            user=request.user,
+        )[action_request.id]
+        if reason is None:
+            return kind
+    raise ApplyFailed(
+        {"detail": _REASON_DETAILS.get(reason, _DEFAULT_REASON_DETAIL), "code": reason}
+    )
 
 
 def _apply(
     *,
     investigation: Investigation,
-    action_request_id: str,
+    action_request: InvestigationActionRequest,
     request: Request,
     actor_id: int,
     dry_run: bool,
-) -> ApplyResult:
+) -> InvestigationActionRequest:
     investigations = Investigation.objects.select_related("organization")
     runs = InvestigationOrchestrationRun.objects.all()
     action_requests = InvestigationActionRequest.objects.all()
@@ -279,31 +297,9 @@ def _apply(
         action_requests = action_requests.select_for_update()
 
     locked = investigations.get(id=investigation.id)
-    try:
-        run = runs.get(investigation=locked)
-    except InvestigationOrchestrationRun.DoesNotExist:
-        raise ResourceDoesNotExist
-    try:
-        action_request = action_requests.get(id=action_request_id, investigation=locked)
-    except (InvestigationActionRequest.DoesNotExist, ValueError):
-        raise ResourceDoesNotExist
-
-    reason = shared_cannot_apply_reason(locked, action_request)
-    if reason == CannotApplyReason.NOT_SUGGESTED:
-        return ApplyResult(action_request, _not_suggested_error(action_request))
-    kind = get_action_kind(action_request.action_kind)
-    if reason is None and kind is not None:
-        # The same check that decides canApply guards the endpoint, so a kind can't forget it.
-        reason = kind.cannot_apply_reasons(
-            [action_request],
-            investigations={locked.id: locked},
-            access=request.access,
-            user=request.user,
-        ).get(action_request.id, CannotApplyReason.UNSUPPORTED_ACTION)
-    if reason is not None:
-        detail = _REASON_DETAILS.get(reason, "This action can't be applied.")
-        return ApplyResult(action_request, {"detail": detail, "code": reason})
-    assert kind is not None
+    run = runs.get(investigation=locked)
+    action_request = action_requests.get(id=action_request.id)
+    kind = _check_can_apply(locked, action_request, request)
 
     context = ApplyContext(
         investigation=locked,
@@ -315,7 +311,7 @@ def _apply(
     try:
         if dry_run:
             kind.check(context)
-            return ApplyResult(action_request)
+            return action_request
         details = kind.apply(context)
     except ActionStale as stale:
         if not dry_run:
@@ -325,23 +321,21 @@ def _apply(
                 state=InvestigationActionRequestState.SUPERSEDED,
                 error=ActionRequestError(code=ApplyErrorCode.STALE, current=stale.current),
             )
-        return ApplyResult(
-            action_request,
+        raise ApplyFailed(
             {
                 "detail": "This changed since Seer suggested it.",
                 "code": ApplyErrorCode.STALE,
                 "current": stale.current,
-            },
+            }
         )
     except ActionInvalid as invalid:
         # The request stays suggested, so the user can try again.
-        return ApplyResult(
-            action_request,
+        raise ApplyFailed(
             {
                 "detail": "The change wasn't valid.",
                 "code": ApplyErrorCode.VALIDATION_FAILED,
-                "messages": _error_messages(invalid.errors),
-            },
+                "messages": invalid.messages,
+            }
         )
     action_request.update(
         state=InvestigationActionRequestState.APPLIED,
@@ -350,7 +344,7 @@ def _apply(
         applied_details=details,
         error=None,
     )
-    return ApplyResult(action_request)
+    return action_request
 
 
 def apply_action_request(
@@ -360,21 +354,33 @@ def apply_action_request(
     request: Request,
     actor_id: int,
     dry_run: bool = False,
-) -> ApplyResult:
-    """Apply a suggested action as the requesting user. A dry run only says whether it would
-    work, and doesn't take locks or change anything.
+) -> InvestigationActionRequest:
+    """Apply a suggested action as the requesting user, or raise ApplyFailed. A dry run only
+    says whether it would work, and doesn't take locks or change anything.
     """
 
+    try:
+        action_request = InvestigationActionRequest.objects.get(
+            id=action_request_id, investigation=investigation
+        )
+    except (InvestigationActionRequest.DoesNotExist, ValueError):
+        raise ResourceDoesNotExist
+
     using = router.db_for_write(InvestigationActionRequest)
     with transaction.atomic(using=using):
-        result = _apply(
-            investigation=investigation,
-            action_request_id=action_request_id,
-            request=request,
-            actor_id=actor_id,
-            dry_run=dry_run,
-        )
-        if dry_run:
-            # Even if a check writes by mistake, a dry run keeps nothing.
-            transaction.set_rollback(True, using=using)
-        return result
+        try:
+            return _apply(
+                investigation=investigation,
+                action_request=action_request,
+                request=request,
+                actor_id=actor_id,
+                dry_run=dry_run,
+            )
+        except ApplyFailed as failed:
+            error = failed
+        finally:
+            if dry_run:
+                # The detector validator can create environments while validating.
+                transaction.set_rollback(True, using=using)
+    # Raise once the transaction commits, so a stale request stays superseded.
+    raise error

tests/sentry/investigations/endpoints/test_organization_investigation_action_request_apply.py

@@ -207,24 +207,6 @@ def test_a_dry_run_runs_the_same_checks(self) -> None:
         assert response.status_code == 409
         assert response.data["code"] == "not_suggested"
 
-    def test_a_dry_run_changes_nothing(self) -> None:
-        action_request = self.create_investigation_action_request(
-            investigation=self.investigation,
-            key="task",
-            action_kind="seer_task",
-            action_payload={"kind": "seer_task", "prompt": "Go"},
-        )
-        version = self.investigation.version
-
-        response = self.apply(action_request, dryRun=True)
-
-        assert response.status_code == 400
-        assert response.data["code"] == "unsupported_action"
-        action_request.refresh_from_db()
-        self.investigation.refresh_from_db()
-        assert action_request.state == InvestigationActionRequestState.SUGGESTED
-        assert self.investigation.version == version
-
     def test_rejects_an_unknown_field(self) -> None:
         response = self.apply(self.suggest(), dryRn=True)
 
@@ -350,6 +332,16 @@ def test_a_dry_run_only_checks(self) -> None:
         self.action_request.refresh_from_db()
         assert self.action_request.state == InvestigationActionRequestState.SUGGESTED
 
+    def test_a_dry_run_keeps_nothing_a_check_writes(self) -> None:
+        title = self.action_request.title
+        self.kind.check_title = "Changed by the check"
+
+        response = self.post(dryRun=True)
+
+        assert response.status_code == 200, response.data
+        self.action_request.refresh_from_db()
+        assert self.action_request.title == title
+
     def test_applying_records_what_the_kind_returns(self) -> None:
         response = self.post()
 
@@ -386,7 +378,7 @@ def test_a_stale_dry_run_changes_nothing(self) -> None:
         assert self.action_request.error is None
 
     def test_an_invalid_action_stays_suggested(self) -> None:
-        self.kind.raises = ActionInvalid({"window": ["Pick a supported window."]})
+        self.kind.raises = ActionInvalid(["Pick a supported window."])
 
         response = self.post()