From c34764e3cf1d73e151d2f04407e42a3b20b1bc70 Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Fri, 15 Aug 2025 10:45:44 -0700 Subject: [PATCH 1/2] Revert "Revert "chore(crons): Move monitor update logic to be entirely inside the serializer (#97794)"" This reverts commit 06fc0efb9d6906ae7ae36b386b70782036d89911. --- .../endpoints/base_monitor_details.py | 126 +------- src/sentry/monitors/validators.py | 148 ++++++++- tests/sentry/monitors/test_validators.py | 306 ++++++++++++++++++ 3 files changed, 450 insertions(+), 130 deletions(-) diff --git a/src/sentry/monitors/endpoints/base_monitor_details.py b/src/sentry/monitors/endpoints/base_monitor_details.py index f2e101d79e988e..74193f34a41757 100644 --- a/src/sentry/monitors/endpoints/base_monitor_details.py +++ b/src/sentry/monitors/endpoints/base_monitor_details.py @@ -1,15 +1,14 @@ from __future__ import annotations from django.db import router, transaction -from django.db.models import F, QuerySet -from django.db.models.functions import TruncMinute +from django.db.models import QuerySet 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 @@ -17,23 +16,10 @@ 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 ( - CheckInStatus, - Monitor, - MonitorCheckIn, - MonitorEnvironment, - MonitorStatus, -) +from sentry.monitors.models import Monitor, 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): @@ -57,118 +43,26 @@ 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={ - "name": monitor.name, - "slug": monitor.slug, - "status": monitor.status, - "config": monitor.config, - "project": project, - }, + instance=monitor, context={ "organization": project.organization, "access": request.access, + "request": request, "monitor": monitor, }, ) if not validator.is_valid(): return self.respond(validator.errors, 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) + try: + updated_monitor = validator.save() + except serializers.ValidationError as e: + return self.respond(e.detail, status=400) - return self.respond(serialize(monitor, request.user)) + return self.respond(serialize(updated_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 index 601361282f5aeb..606111a1fade3e 100644 --- a/src/sentry/monitors/validators.py +++ b/src/sentry/monitors/validators.py @@ -1,15 +1,17 @@ import re -from typing import Literal +from typing import Any, 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 quotas +from sentry import audit_log, quotas from sentry.api.fields.actor import ActorField from sentry.api.fields.empty_integer import EmptyIntegerField from sentry.api.fields.sentry_slug import SentrySerializerSlugField @@ -18,11 +20,25 @@ 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, ScheduleType +from sentry.monitors.models import ( + CheckInStatus, + Monitor, + MonitorCheckIn, + MonitorEnvironment, + 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, signal_monitor_created +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.utils.dates import AVAILABLE_TIMEZONES from sentry.utils.outcomes import Outcome @@ -162,7 +178,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.get("config") + self.instance = self.parent.instance.config self.partial = self.parent.partial def validate_schedule_type(self, value): @@ -300,7 +316,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.get("slug"): + if self.instance and value == self.instance.slug: return value if Monitor.objects.filter( @@ -309,14 +325,6 @@ 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"] @@ -361,6 +369,118 @@ 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 index 75a3395289b525..0991531f253345 100644 --- a/tests/sentry/monitors/test_validators.py +++ b/tests/sentry/monitors/test_validators.py @@ -298,3 +298,309 @@ def test_create_with_is_muted(self): monitor = validator.save() assert monitor.is_muted is True + + +class MonitorValidatorUpdateTest(MonitorTestCase): + def setUp(self): + super().setUp() + self.login_as(self.user) + + self.monitor = Monitor.objects.create( + organization_id=self.organization.id, + project_id=self.project.id, + name="Test Monitor", + slug="test-monitor", + config={ + "schedule": "0 * * * *", + "schedule_type": ScheduleType.CRONTAB, + "checkin_margin": 5, + "max_runtime": 30, + }, + ) + self.team = self.create_team(organization=self.organization) + self.request = RequestFactory().get("/") + self.request.user = self.user + + # Create a mock access object + self.access = MagicMock() + self.access.has_project_scope.return_value = True + + def test_update_name(self): + """Test updating monitor name.""" + validator = MonitorValidator( + instance=self.monitor, + data={"name": "Updated Monitor Name"}, + partial=True, + context={ + "organization": self.organization, + "access": self.access, + "request": self.request, + }, + ) + assert validator.is_valid() + + updated_monitor = validator.save() + assert updated_monitor.name == "Updated Monitor Name" + assert updated_monitor.slug == "test-monitor" # Slug unchanged + + def test_update_slug(self): + """Test updating monitor slug.""" + validator = MonitorValidator( + instance=self.monitor, + data={"slug": "new-monitor-slug"}, + partial=True, + context={ + "organization": self.organization, + "access": self.access, + "request": self.request, + }, + ) + assert validator.is_valid() + + updated_monitor = validator.save() + assert updated_monitor.slug == "new-monitor-slug" + assert updated_monitor.name == "Test Monitor" # Name unchanged + + def test_update_config(self): + """Test updating monitor config.""" + new_config = { + "schedule": "*/30 * * * *", + "schedule_type": "crontab", + "checkin_margin": 10, + "max_runtime": 60, + "timezone": "America/New_York", + } + validator = MonitorValidator( + instance=self.monitor, + data={"config": new_config}, + partial=True, + context={ + "organization": self.organization, + "access": self.access, + "request": self.request, + }, + ) + assert validator.is_valid() + + updated_monitor = validator.save() + assert updated_monitor.config["schedule"] == "*/30 * * * *" + assert updated_monitor.config["checkin_margin"] == 10 + assert updated_monitor.config["max_runtime"] == 60 + assert updated_monitor.config["timezone"] == "America/New_York" + + def test_update_owner_to_user(self): + """Test updating monitor owner to a user.""" + validator = MonitorValidator( + instance=self.monitor, + data={"owner": f"user:{self.user.id}"}, + partial=True, + context={ + "organization": self.organization, + "access": self.access, + "request": self.request, + }, + ) + assert validator.is_valid(), validator.errors + + updated_monitor = validator.save() + assert updated_monitor.owner_user_id == self.user.id + assert updated_monitor.owner_team_id is None + + def test_update_owner_to_team(self): + """Test updating monitor owner to a team.""" + validator = MonitorValidator( + instance=self.monitor, + data={"owner": f"team:{self.team.id}"}, + partial=True, + context={ + "organization": self.organization, + "access": self.access, + "request": self.request, + }, + ) + assert validator.is_valid() + + updated_monitor = validator.save() + assert updated_monitor.owner_user_id is None + assert updated_monitor.owner_team_id == self.team.id + + def test_update_owner_to_none(self): + """Test removing monitor owner.""" + # First set an owner + self.monitor.update(owner_user_id=self.user.id) + + validator = MonitorValidator( + instance=self.monitor, + data={"owner": None}, + partial=True, + context={ + "organization": self.organization, + "access": self.access, + "request": self.request, + }, + ) + assert validator.is_valid() + + updated_monitor = validator.save() + assert updated_monitor.owner_user_id is None + assert updated_monitor.owner_team_id is None + + def test_update_is_muted(self): + """Test updating is_muted field.""" + validator = MonitorValidator( + instance=self.monitor, + data={"is_muted": True}, + partial=True, + context={ + "organization": self.organization, + "access": self.access, + "request": self.request, + }, + ) + assert validator.is_valid() + + updated_monitor = validator.save() + assert updated_monitor.is_muted is True + + def test_update_status_to_disabled(self): + """Test updating monitor status to disabled.""" + validator = MonitorValidator( + instance=self.monitor, + data={"status": "disabled"}, + partial=True, + context={ + "organization": self.organization, + "access": self.access, + "request": self.request, + "monitor": self.monitor, + }, + ) + assert validator.is_valid() + + updated_monitor = validator.save() + assert updated_monitor.status == ObjectStatus.DISABLED + + @patch("sentry.quotas.backend.check_assign_monitor_seat") + def test_update_status_to_active_with_quota_check(self, mock_check_seat): + """Test updating monitor status to active checks quota.""" + # Start with disabled monitor + self.monitor.update(status=ObjectStatus.DISABLED) + + mock_result = MagicMock() + mock_result.assignable = True + mock_check_seat.return_value = mock_result + + validator = MonitorValidator( + instance=self.monitor, + data={"status": "active"}, + partial=True, + context={ + "organization": self.organization, + "access": self.access, + "request": self.request, + "monitor": self.monitor, + }, + ) + assert validator.is_valid() + + updated_monitor = validator.save() + assert updated_monitor.status == ObjectStatus.ACTIVE + mock_check_seat.assert_called_once_with(self.monitor) + + @patch("sentry.quotas.backend.check_assign_monitor_seat") + def test_update_status_to_active_quota_exceeded(self, mock_check_seat): + """Test updating monitor status to active fails when quota exceeded.""" + # Start with disabled monitor + self.monitor.update(status=ObjectStatus.DISABLED) + + mock_result = MagicMock() + mock_result.assignable = False + mock_result.reason = "Monitor quota exceeded" + mock_check_seat.return_value = mock_result + + validator = MonitorValidator( + instance=self.monitor, + data={"status": "active"}, + partial=True, + context={ + "organization": self.organization, + "access": self.access, + "request": self.request, + "monitor": self.monitor, + }, + ) + assert not validator.is_valid() + assert "Monitor quota exceeded" in str(validator.errors["status"]) + + def test_update_multiple_fields(self): + """Test updating multiple fields at once.""" + validator = MonitorValidator( + instance=self.monitor, + data={ + "name": "New Name", + "slug": "new-slug", + "is_muted": True, + "owner": f"team:{self.team.id}", + }, + partial=True, + context={ + "organization": self.organization, + "access": self.access, + "request": self.request, + }, + ) + assert validator.is_valid() + + updated_monitor = validator.save() + assert updated_monitor.name == "New Name" + assert updated_monitor.slug == "new-slug" + assert updated_monitor.is_muted is True + assert updated_monitor.owner_team_id == self.team.id + + def test_update_slug_already_exists(self): + """Test updating slug to one that already exists fails.""" + # Create another monitor with target slug + Monitor.objects.create( + organization_id=self.organization.id, + project_id=self.project.id, + name="Another Monitor", + slug="existing-slug", + config={ + "schedule": "0 * * * *", + "schedule_type": ScheduleType.CRONTAB, + }, + ) + + validator = MonitorValidator( + instance=self.monitor, + data={"slug": "existing-slug"}, + partial=True, + context={ + "organization": self.organization, + "access": self.access, + "request": self.request, + }, + ) + assert not validator.is_valid() + assert 'The slug "existing-slug" is already in use.' in str(validator.errors["slug"]) + + def test_update_preserves_unchanged_fields(self): + """Test that update preserves fields that aren't being updated.""" + original_config = self.monitor.config.copy() + + validator = MonitorValidator( + instance=self.monitor, + data={"name": "Just Update Name"}, + partial=True, + context={ + "organization": self.organization, + "access": self.access, + "request": self.request, + }, + ) + assert validator.is_valid() + + updated_monitor = validator.save() + assert updated_monitor.name == "Just Update Name" + assert updated_monitor.slug == "test-monitor" + assert updated_monitor.config == original_config From daee0202fecf4071bd6e62b96af4eaf5ff7dae17 Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Fri, 15 Aug 2025 11:47:02 -0700 Subject: [PATCH 2/2] fix problem with dropping keys from config --- src/sentry/monitors/validators.py | 12 +++++++- tests/sentry/monitors/test_validators.py | 35 ++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 1 deletion(-) diff --git a/src/sentry/monitors/validators.py b/src/sentry/monitors/validators.py index 606111a1fade3e..f5d238b6a2bd0b 100644 --- a/src/sentry/monitors/validators.py +++ b/src/sentry/monitors/validators.py @@ -1,6 +1,7 @@ import re from typing import Any, Literal +import jsonschema import sentry_sdk from cronsim import CronSimError from django.core.exceptions import ValidationError @@ -23,6 +24,7 @@ from sentry.models.project import Project from sentry.monitors.constants import MAX_MARGIN, MAX_THRESHOLD, MAX_TIMEOUT from sentry.monitors.models import ( + MONITOR_CONFIG, CheckInStatus, Monitor, MonitorCheckIn, @@ -402,7 +404,15 @@ def update(self, instance, validated_data): if "is_muted" in validated_data: params["is_muted"] = validated_data["is_muted"] if "config" in validated_data: - params["config"] = validated_data["config"] + merged_config = instance.config.copy() + merged_config.update(validated_data["config"]) + + try: + jsonschema.validate(merged_config, MONITOR_CONFIG) + except jsonschema.ValidationError as e: + raise serializers.ValidationError({"config": f"Invalid config: {e.message}"}) + + params["config"] = merged_config if "status" in params: # Attempt to assign a monitor seat diff --git a/tests/sentry/monitors/test_validators.py b/tests/sentry/monitors/test_validators.py index 0991531f253345..ef059701289ca1 100644 --- a/tests/sentry/monitors/test_validators.py +++ b/tests/sentry/monitors/test_validators.py @@ -604,3 +604,38 @@ def test_update_preserves_unchanged_fields(self): assert updated_monitor.name == "Just Update Name" assert updated_monitor.slug == "test-monitor" assert updated_monitor.config == original_config + + def test_partial_config_update_preserves_existing_fields(self): + """Test that partial config updates preserve fields not included in the update.""" + # Set up a monitor with a complete config + original_config = { + "schedule": "0 * * * *", + "schedule_type": ScheduleType.CRONTAB, + "checkin_margin": 5, + "max_runtime": 30, + "timezone": "UTC", + "failure_issue_threshold": 3, + "recovery_threshold": 1, + } + self.monitor.update(config=original_config) + + # Update only the schedule - other fields should be preserved + validator = MonitorValidator( + instance=self.monitor, + data={"config": {"schedule": "*/30 * * * *", "schedule_type": "crontab"}}, + partial=True, + context={ + "organization": self.organization, + "access": self.access, + "request": self.request, + }, + ) + assert validator.is_valid() + + updated_monitor = validator.save() + assert updated_monitor.config["schedule"] == "*/30 * * * *" + assert updated_monitor.config["checkin_margin"] == 5 + assert updated_monitor.config["max_runtime"] == 30 + assert updated_monitor.config["timezone"] == "UTC" + assert updated_monitor.config["failure_issue_threshold"] == 3 + assert updated_monitor.config["recovery_threshold"] == 1