Revert "chore(crons): Move monitor update logic to be entirely inside the serializer (#97794)"

getsentry-bot · wedamija · getsentry-bot · commit 06fc0efb9d69 · 2025-08-14T23:10:02.000Z
This reverts commit 5fd8341. Co-authored-by: wedamija <6288560+wedamija@users.noreply.github.com>
diff --git a/src/sentry/monitors/endpoints/base_monitor_details.py b/src/sentry/monitors/endpoints/base_monitor_details.py
@@ -1,25 +1,39 @@
 from __future__ import annotations
 
 from django.db import router, transaction
-from django.db.models import QuerySet
+from django.db.models import F, QuerySet
+from django.db.models.functions import TruncMinute
 from django.utils.crypto import get_random_string
-from rest_framework import serializers
 from rest_framework.request import Request
 from rest_framework.response import Response
 
 from sentry import audit_log, quotas
 from sentry.api.base import BaseEndpointMixin
+from sentry.api.exceptions import ParameterValidationError
 from sentry.api.helpers.environments import get_environments
 from sentry.api.serializers import serialize
 from sentry.constants import ObjectStatus
 from sentry.deletions.models.scheduleddeletion import RegionScheduledDeletion
 from sentry.models.environment import Environment
 from sentry.models.project import Project
 from sentry.models.rule import Rule, RuleActivity, RuleActivityType
-from sentry.monitors.models import Monitor, MonitorEnvironment, MonitorStatus
+from sentry.monitors.models import (
+    CheckInStatus,
+    Monitor,
+    MonitorCheckIn,
+    MonitorEnvironment,
+    MonitorStatus,
+)
 from sentry.monitors.serializers import MonitorSerializer
+from sentry.monitors.utils import (
+    create_issue_alert_rule,
+    get_checkin_margin,
+    get_max_runtime,
+    update_issue_alert_rule,
+)
 from sentry.monitors.validators import MonitorValidator
 from sentry.utils.auth import AuthenticatedHttpRequest
+from sentry.utils.outcomes import Outcome
 
 
 class MonitorDetailsMixin(BaseEndpointMixin):
@@ -43,26 +57,118 @@ def update_monitor(
         """
         Update a monitor.
         """
+        # set existing values as validator will overwrite
+        existing_config = monitor.config
+        existing_margin = existing_config.get("checkin_margin")
+        existing_max_runtime = existing_config.get("max_runtime")
+
         validator = MonitorValidator(
             data=request.data,
             partial=True,
-            instance=monitor,
+            instance={
+                "name": monitor.name,
+                "slug": monitor.slug,
+                "status": monitor.status,
+                "config": monitor.config,
+                "project": project,
+            },
             context={
                 "organization": project.organization,
                 "access": request.access,
-                "request": request,
                 "monitor": monitor,
             },
         )
         if not validator.is_valid():
             return self.respond(validator.errors, status=400)
 
-        try:
-            updated_monitor = validator.save()
-        except serializers.ValidationError as e:
-            return self.respond(e.detail, status=400)
+        result = validator.save()
+
+        params = {}
+        if "name" in result:
+            params["name"] = result["name"]
+        if "slug" in result:
+            params["slug"] = result["slug"]
+        if "status" in result:
+            params["status"] = result["status"]
+        if "is_muted" in result:
+            params["is_muted"] = result["is_muted"]
+        if "owner" in result:
+            owner = result["owner"]
+            params["owner_user_id"] = None
+            params["owner_team_id"] = None
+            if owner and owner.is_user:
+                params["owner_user_id"] = owner.id
+            elif owner and owner.is_team:
+                params["owner_team_id"] = owner.id
+        if "config" in result:
+            params["config"] = result["config"]
+
+            # update timeouts + expected next check-in, as appropriate
+            checkin_margin = result["config"].get("checkin_margin")
+            if checkin_margin != existing_margin:
+                MonitorEnvironment.objects.filter(monitor_id=monitor.id).update(
+                    next_checkin_latest=F("next_checkin") + get_checkin_margin(checkin_margin)
+                )
+
+            max_runtime = result["config"].get("max_runtime")
+            if max_runtime != existing_max_runtime:
+                MonitorCheckIn.objects.filter(
+                    monitor_id=monitor.id, status=CheckInStatus.IN_PROGRESS
+                ).update(timeout_at=TruncMinute(F("date_added")) + get_max_runtime(max_runtime))
+
+        if "project" in result and result["project"].id != monitor.project_id:
+            raise ParameterValidationError("existing monitors may not be moved between projects")
+
+        # Attempt to assign a monitor seat
+        if params["status"] == ObjectStatus.ACTIVE and monitor.status != ObjectStatus.ACTIVE:
+            outcome = quotas.backend.assign_monitor_seat(monitor)
+            # The MonitorValidator checks if a seat assignment is available.
+            # This protects against a race condition
+            if outcome != Outcome.ACCEPTED:
+                raise ParameterValidationError("Failed to enable monitor, please try again")
+
+        # Attempt to unassign the monitor seat
+        if params["status"] == ObjectStatus.DISABLED and monitor.status != ObjectStatus.DISABLED:
+            quotas.backend.disable_monitor_seat(monitor)
+
+        # Update monitor slug in billing
+        if "slug" in result:
+            quotas.backend.update_monitor_slug(monitor.slug, params["slug"], monitor.project_id)
+
+        if params:
+            monitor.update(**params)
+            self.create_audit_entry(
+                request=request,
+                organization=project.organization,
+                target_object=monitor.id,
+                event=audit_log.get_event_id("MONITOR_EDIT"),
+                data=monitor.get_audit_log_data(),
+            )
+
+        # Update alert rule after in case slug or name changed
+        if "alert_rule" in result:
+            # Check to see if rule exists
+            issue_alert_rule = monitor.get_issue_alert_rule()
+            # If rule exists, update as necessary
+            if issue_alert_rule:
+                issue_alert_rule_id = update_issue_alert_rule(
+                    request, project, monitor, issue_alert_rule, result["alert_rule"]
+                )
+            # If rule does not exist, create
+            else:
+                issue_alert_rule_id = create_issue_alert_rule(
+                    request, project, monitor, result["alert_rule"]
+                )
+
+            if issue_alert_rule_id:
+                # If config is not sent, use existing config to update alert_rule_id
+                if "config" not in params:
+                    params["config"] = monitor.config
+
+                params["config"]["alert_rule_id"] = issue_alert_rule_id
+                monitor.update(**params)
 
-        return self.respond(serialize(updated_monitor, request.user))
+        return self.respond(serialize(monitor, request.user))
 
     def delete_monitor(self, request: Request, project: Project, monitor: Monitor) -> Response:
         """
diff --git a/src/sentry/monitors/validators.py b/src/sentry/monitors/validators.py
@@ -1,17 +1,15 @@
 import re
-from typing import Any, Literal
+from typing import Literal
 
 import sentry_sdk
 from cronsim import CronSimError
 from django.core.exceptions import ValidationError
-from django.db.models import F
-from django.db.models.functions import TruncMinute
 from django.utils import timezone
 from drf_spectacular.types import OpenApiTypes
 from drf_spectacular.utils import extend_schema_field, extend_schema_serializer
 from rest_framework import serializers
 
-from sentry import audit_log, quotas
+from sentry import quotas
 from sentry.api.fields.actor import ActorField
 from sentry.api.fields.empty_integer import EmptyIntegerField
 from sentry.api.fields.sentry_slug import SentrySerializerSlugField
@@ -20,25 +18,11 @@
 from sentry.constants import ObjectStatus
 from sentry.db.models import BoundedPositiveIntegerField
 from sentry.db.models.fields.slug import DEFAULT_SLUG_MAX_LENGTH
-from sentry.models.project import Project
 from sentry.monitors.constants import MAX_MARGIN, MAX_THRESHOLD, MAX_TIMEOUT
-from sentry.monitors.models import (
-    CheckInStatus,
-    Monitor,
-    MonitorCheckIn,
-    MonitorEnvironment,
-    ScheduleType,
-)
+from sentry.monitors.models import CheckInStatus, Monitor, ScheduleType
 from sentry.monitors.schedule import get_next_schedule, get_prev_schedule
 from sentry.monitors.types import CrontabSchedule, slugify_monitor_slug
-from sentry.monitors.utils import (
-    create_issue_alert_rule,
-    get_checkin_margin,
-    get_max_runtime,
-    signal_monitor_created,
-    update_issue_alert_rule,
-)
-from sentry.utils.audit import create_audit_entry
+from sentry.monitors.utils import create_issue_alert_rule, signal_monitor_created
 from sentry.utils.dates import AVAILABLE_TIMEZONES
 from sentry.utils.outcomes import Outcome
 
@@ -178,7 +162,7 @@ def bind(self, *args, **kwargs):
         super().bind(*args, **kwargs)
         # Inherit instance data when used as a nested serializer
         if self.parent.instance:
-            self.instance = self.parent.instance.config
+            self.instance = self.parent.instance.get("config")
         self.partial = self.parent.partial
 
     def validate_schedule_type(self, value):
@@ -316,7 +300,7 @@ def validate_slug(self, value):
 
         value = slugify_monitor_slug(value)
         # Ignore if slug is equal to current value
-        if self.instance and value == self.instance.slug:
+        if self.instance and value == self.instance.get("slug"):
             return value
 
         if Monitor.objects.filter(
@@ -325,6 +309,14 @@ def validate_slug(self, value):
             raise ValidationError(f'The slug "{value}" is already in use.')
         return value
 
+    def update(self, instance, validated_data):
+        config = instance.get("config", {})
+        config.update(validated_data.get("config", {}))
+        instance.update(validated_data)
+        if "config" in instance or "config" in validated_data:
+            instance["config"] = config
+        return instance
+
     def create(self, validated_data):
         project = validated_data.get("project", self.context.get("project"))
         organization = self.context["organization"]
@@ -369,118 +361,6 @@ def create(self, validated_data):
                 monitor.update(config=config)
         return monitor
 
-    def update(self, instance, validated_data):
-        """
-        Update an existing Monitor instance.
-        """
-        if "project" in validated_data and validated_data["project"].id != instance.project_id:
-            raise serializers.ValidationError(
-                {"detail": {"message": "existing monitors may not be moved between projects"}}
-            )
-
-        existing_config = instance.config.copy()
-        existing_margin = existing_config.get("checkin_margin")
-        existing_max_runtime = existing_config.get("max_runtime")
-        existing_slug = instance.slug
-
-        params: dict[str, Any] = {}
-        if "owner" in validated_data:
-            owner = validated_data["owner"]
-            params["owner_user_id"] = None
-            params["owner_team_id"] = None
-            if owner and owner.is_user:
-                params["owner_user_id"] = owner.id
-            elif owner and owner.is_team:
-                params["owner_team_id"] = owner.id
-
-        if "name" in validated_data:
-            params["name"] = validated_data["name"]
-        if "slug" in validated_data:
-            params["slug"] = validated_data["slug"]
-        if "status" in validated_data:
-            params["status"] = validated_data["status"]
-        if "is_muted" in validated_data:
-            params["is_muted"] = validated_data["is_muted"]
-        if "config" in validated_data:
-            params["config"] = validated_data["config"]
-
-        if "status" in params:
-            # Attempt to assign a monitor seat
-            if params["status"] == ObjectStatus.ACTIVE and instance.status != ObjectStatus.ACTIVE:
-                outcome = quotas.backend.assign_monitor_seat(instance)
-                # The MonitorValidator checks if a seat assignment is available.
-                # This protects against a race condition
-                if outcome != Outcome.ACCEPTED:
-                    raise serializers.ValidationError(
-                        {"status": "Failed to enable monitor due to quota limits"}
-                    )
-
-            # Attempt to unassign the monitor seat
-            if (
-                params["status"] == ObjectStatus.DISABLED
-                and instance.status != ObjectStatus.DISABLED
-            ):
-                quotas.backend.disable_monitor_seat(instance)
-
-        if params:
-            instance.update(**params)
-            create_audit_entry(
-                request=self.context["request"],
-                organization_id=instance.organization_id,
-                target_object=instance.id,
-                event=audit_log.get_event_id("MONITOR_EDIT"),
-                data=instance.get_audit_log_data(),
-            )
-
-        # Update monitor slug in billing
-        if "slug" in params:
-            quotas.backend.update_monitor_slug(existing_slug, params["slug"], instance.project_id)
-
-        if "config" in validated_data:
-            new_config = validated_data["config"]
-            checkin_margin = new_config.get("checkin_margin")
-            if checkin_margin != existing_margin:
-                MonitorEnvironment.objects.filter(monitor_id=instance.id).update(
-                    next_checkin_latest=F("next_checkin") + get_checkin_margin(checkin_margin)
-                )
-
-            max_runtime = new_config.get("max_runtime")
-            if max_runtime != existing_max_runtime:
-                MonitorCheckIn.objects.filter(
-                    monitor_id=instance.id, status=CheckInStatus.IN_PROGRESS
-                ).update(timeout_at=TruncMinute(F("date_added")) + get_max_runtime(max_runtime))
-
-        # Update alert rule after in case slug or name changed
-        if "alert_rule" in validated_data:
-            alert_rule_data = validated_data["alert_rule"]
-            request = self.context.get("request")
-            if not request:
-                return instance
-
-            project = Project.objects.get(id=instance.project_id)
-
-            # Check to see if rule exists
-            issue_alert_rule = instance.get_issue_alert_rule()
-            # If rule exists, update as necessary
-            if issue_alert_rule:
-                issue_alert_rule_id = update_issue_alert_rule(
-                    request, project, instance, issue_alert_rule, alert_rule_data
-                )
-            # If rule does not exist, create
-            else:
-                # Type assertion for mypy - create_issue_alert_rule expects AuthenticatedHttpRequest
-                # but in tests we might have a regular Request object
-                issue_alert_rule_id = create_issue_alert_rule(
-                    request, project, instance, alert_rule_data
-                )
-
-            if issue_alert_rule_id:
-                # If config is not sent, use existing config to update alert_rule_id
-                instance.config["alert_rule_id"] = issue_alert_rule_id
-                instance.update(config=instance.config)
-
-        return instance
-
 
 class TraceContextValidator(serializers.Serializer):
     trace_id = serializers.UUIDField(format="hex")
diff --git a/tests/sentry/monitors/test_validators.py b/tests/sentry/monitors/test_validators.py
