Skip to content

Commit

Permalink
ref(crons): Switch to using owner_{user,team}_id over actor (#68825)
Browse files Browse the repository at this point in the history
In my initial implementation of GH-68623 I hadn't realized that `Actor`
was being removed. This updates our usages to use the new
`owner_user_id` and `owner_team_id` (Waiting on GH-68809)
  • Loading branch information
evanpurkhiser authored Apr 12, 2024
1 parent 3ae6392 commit 9c99a10
Show file tree
Hide file tree
Showing 9 changed files with 70 additions and 26 deletions.
24 changes: 24 additions & 0 deletions src/sentry/models/actor.py
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,30 @@ def from_actor_identifier(cls, actor_identifier: int | str | None) -> ActorTuple
except IndexError as e:
raise serializers.ValidationError(f"Unable to resolve actor identifier: {e}")

@classmethod
def from_id(cls, user_id: int | None, team_id: int | None) -> ActorTuple | None:
from sentry.models.team import Team
from sentry.models.user import User

if user_id and team_id:
raise ValueError("user_id and team_id may not both be specified")
if user_id and not team_id:
return cls(user_id, User)
if team_id and not user_id:
return cls(team_id, Team)

return None

@classmethod
def from_ids(cls, user_ids: Sequence[int], team_ids: Sequence[int]) -> Sequence[ActorTuple]:
from sentry.models.team import Team
from sentry.models.user import User

return [
*[cls(user_id, User) for user_id in user_ids],
*[cls(team_id, Team) for team_id in team_ids],
]

def resolve(self) -> Team | RpcUser:
return fetch_actor_by_id(self.type, self.id)

Expand Down
10 changes: 9 additions & 1 deletion src/sentry/monitors/endpoints/base_monitor_details.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
from sentry.models.project import Project
from sentry.models.rule import Rule, RuleActivity, RuleActivityType
from sentry.models.scheduledeletion import RegionScheduledDeletion
from sentry.models.team import Team
from sentry.models.user import User
from sentry.monitors.models import (
CheckInStatus,
Monitor,
Expand Down Expand Up @@ -94,7 +96,13 @@ def update_monitor(
if "is_muted" in result:
params["is_muted"] = result["is_muted"]
if "owner" in result:
params["owner_actor_id"] = result["owner"].id if result["owner"] else None
owner = result["owner"]
params["owner_user_id"] = None
params["owner_team_id"] = None
if owner and owner.type == User:
params["owner_user_id"] = owner.id
elif owner and owner.type == Team:
params["owner_team_id"] = owner.id
if "config" in result:
params["config"] = result["config"]

Expand Down
16 changes: 11 additions & 5 deletions src/sentry/monitors/endpoints/organization_monitor_index.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@
from sentry.db.models.query import in_iexact
from sentry.models.environment import Environment
from sentry.models.organization import Organization
from sentry.models.team import Team
from sentry.models.user import User
from sentry.monitors.models import (
Monitor,
MonitorEnvironment,
Expand Down Expand Up @@ -259,16 +261,20 @@ def post(self, request: Request, organization) -> Response:

result = validator.validated_data

if result.get("owner"):
owner_id = result.get("owner").id
else:
owner_id = None
owner = result.get("owner")
owner_user_id = None
owner_team_id = None
if owner and owner.type == User:
owner_user_id = owner.id
elif owner and owner.type == Team:
owner_team_id = owner.id

try:
monitor = Monitor.objects.create(
project_id=result["project"].id,
organization_id=organization.id,
owner_actor_id=owner_id,
owner_user_id=owner_user_id,
owner_team_id=owner_team_id,
name=result["name"],
slug=result.get("slug"),
status=result["status"],
Expand Down
5 changes: 5 additions & 0 deletions src/sentry/monitors/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
from sentry.db.models.fields.slug import SentrySlugField
from sentry.db.models.utils import slugify_instance
from sentry.locks import locks
from sentry.models.actor import ActorTuple
from sentry.models.environment import Environment
from sentry.models.rule import Rule, RuleSource
from sentry.monitors.constants import MAX_SLUG_LENGTH
Expand Down Expand Up @@ -306,6 +307,10 @@ def save(self, *args, **kwargs):
)
return super().save(*args, **kwargs)

@property
def owner_actor(self) -> ActorTuple | None:
return ActorTuple.from_id(self.owner_user_id, self.owner_team_id)

@property
def schedule(self) -> CrontabSchedule | IntervalSchedule:
schedule_type = self.config["schedule_type"]
Expand Down
15 changes: 7 additions & 8 deletions src/sentry/monitors/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

from sentry.api.serializers import ProjectSerializerResponse, Serializer, register, serialize
from sentry.api.serializers.models.actor import ActorSerializer, ActorSerializerResponse
from sentry.models.actor import Actor, ActorTuple
from sentry.models.actor import ActorTuple
from sentry.models.project import Project
from sentry.monitors.utils import fetch_associated_groups
from sentry.monitors.validators import IntervalNames
Expand Down Expand Up @@ -181,14 +181,13 @@ def get_attrs(self, item_list, user, **kwargs):
for project, serialized_project in zip(projects, serialize(list(projects), user))
}

actors = Actor.objects.filter(id__in=[i.owner_actor_id for i in item_list])
actor_tuples = [a.get_actor_tuple() for a in actors]

actors_serialized = serialize(
ActorTuple.resolve_many(actor_tuples), user, ActorSerializer()
actors = ActorTuple.from_ids(
[m.owner_user_id for m in item_list if m.owner_user_id],
[m.owner_team_id for m in item_list if m.owner_team_id],
)
actors_serialized = serialize(ActorTuple.resolve_many(actors), user, ActorSerializer())
actor_data = {
actor.id: serialized_actor for actor, serialized_actor in zip(actors, actors_serialized)
actor: serialized_actor for actor, serialized_actor in zip(actors, actors_serialized)
}

monitor_environments = (
Expand Down Expand Up @@ -218,7 +217,7 @@ def get_attrs(self, item_list, user, **kwargs):
item: {
"project": projects_data[item.project_id] if item.project_id else None,
"environments": environment_data[item.id],
"owner": actor_data.get(item.owner_actor_id),
"owner": actor_data.get(item.owner_actor),
}
for item in item_list
}
Expand Down
1 change: 0 additions & 1 deletion src/sentry/monitors/validators.py
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,6 @@ class MonitorValidator(CamelSnakeSerializer):
required=False,
allow_null=True,
help_text="The ID of the team or user that owns the monitor. (eg. user:51 or team:6)",
as_actor=True,
)
is_muted = serializers.BooleanField(
required=False,
Expand Down
5 changes: 2 additions & 3 deletions src/sentry/testutils/cases.py
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,6 @@
from sentry.issues.ingest import send_issue_occurrence_to_eventstream
from sentry.mail import mail_adapter
from sentry.mediators.project_rules.creator import Creator
from sentry.models.actor import get_actor_for_user
from sentry.models.apitoken import ApiToken
from sentry.models.authprovider import AuthProvider as AuthProviderModel
from sentry.models.commit import Commit
Expand Down Expand Up @@ -3061,8 +3060,8 @@ def setUp(self):

class MonitorTestCase(APITestCase):
def _create_monitor(self, **kwargs):
if "owner_actor_id" not in kwargs:
kwargs["owner_actor_id"] = get_actor_for_user(self.user).id
if "owner_user_id" not in kwargs:
kwargs["owner_user_id"] = self.user.id

return Monitor.objects.create(
organization_id=self.organization.id,
Expand Down
16 changes: 10 additions & 6 deletions tests/sentry/monitors/endpoints/test_base_monitor_details.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
import pytest

from sentry.constants import ObjectStatus
from sentry.models.actor import get_actor_for_team, get_actor_for_user
from sentry.models.environment import Environment
from sentry.models.rule import Rule, RuleActivity, RuleActivityType
from sentry.models.scheduledeletion import RegionScheduledDeletion
Expand Down Expand Up @@ -171,8 +170,10 @@ def test_owner_user(self):
}

def test_owner_team(self):
team_actor = get_actor_for_team(self.team)
monitor = self._create_monitor(owner_actor_id=team_actor.id)
monitor = self._create_monitor(
owner_user_id=None,
owner_team_id=self.team.id,
)
resp = self.get_success_response(self.organization.slug, monitor.slug)

assert resp.data["owner"] == {
Expand Down Expand Up @@ -220,7 +221,8 @@ def test_slug(self):

def test_owner(self):
monitor = self._create_monitor()
assert monitor.owner_actor_id == get_actor_for_user(self.user).id
assert monitor.owner_user_id == self.user.id
assert monitor.owner_team_id is None

resp = self.get_success_response(
self.organization.slug, monitor.slug, method="PUT", **{"owner": f"team:{self.team.id}"}
Expand All @@ -229,7 +231,8 @@ def test_owner(self):
assert resp.data["owner"]["name"] == self.team.name

monitor = Monitor.objects.get(id=monitor.id)
assert monitor.owner_actor_id == get_actor_for_team(self.team).id
assert monitor.owner_team_id == self.team.id
assert monitor.owner_user_id is None

# Clear owner
resp = self.get_success_response(
Expand All @@ -238,7 +241,8 @@ def test_owner(self):
assert resp.data["owner"] is None

monitor = Monitor.objects.get(id=monitor.id)
assert monitor.owner_actor_id is None
assert monitor.owner_user_id is None
assert monitor.owner_team_id is None

# Validate error cases
resp = self.get_error_response(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
from rest_framework.exceptions import ErrorDetail

from sentry.constants import ObjectStatus
from sentry.models.actor import get_actor_for_user
from sentry.models.rule import Rule, RuleSource
from sentry.monitors.models import Monitor, MonitorStatus, MonitorType, ScheduleType
from sentry.quotas.base import SeatAssignmentResult
Expand Down Expand Up @@ -298,7 +297,8 @@ def test_simple(self, mock_record):
assert monitor.project_id == self.project.id
assert monitor.name == "My Monitor"
assert monitor.status == ObjectStatus.ACTIVE
assert monitor.owner_actor_id == get_actor_for_user(self.user).id
assert monitor.owner_user_id == self.user.id
assert monitor.owner_team_id is None
assert monitor.type == MonitorType.CRON_JOB
assert monitor.config == {
"schedule_type": ScheduleType.CRONTAB,
Expand Down

0 comments on commit 9c99a10

Please sign in to comment.