From 1507f76925757056a619c1fd0acc3c5006b99c60 Mon Sep 17 00:00:00 2001 From: David Cramer Date: Fri, 6 Jan 2023 09:54:36 -0800 Subject: [PATCH] fix: Correct API usage for Crons - Remove legacy docstrings and replace with better descriptions - Missing checkin details endpoint for Hybrid Cloud --- .../api/endpoints/monitor_checkin_details.py | 46 +++++++++++-------- src/sentry/api/endpoints/monitor_checkins.py | 15 +++--- src/sentry/api/endpoints/monitor_details.py | 19 ++------ src/sentry/api/urls.py | 5 ++ .../endpoints/test_monitor_checkin_details.py | 22 +++++++++ 5 files changed, 63 insertions(+), 44 deletions(-) diff --git a/src/sentry/api/endpoints/monitor_checkin_details.py b/src/sentry/api/endpoints/monitor_checkin_details.py index 6fd560660693dd..13368b2c9f5394 100644 --- a/src/sentry/api/endpoints/monitor_checkin_details.py +++ b/src/sentry/api/endpoints/monitor_checkin_details.py @@ -1,3 +1,5 @@ +from __future__ import annotations + from django.db import transaction from django.utils import timezone from drf_spectacular.utils import extend_schema @@ -41,7 +43,15 @@ class MonitorCheckInDetailsEndpoint(Endpoint): # TODO(dcramer): this code needs shared with other endpoints as its security focused # TODO(dcramer): this doesnt handle is_global roles - def convert_args(self, request: Request, monitor_id, checkin_id, *args, **kwargs): + def convert_args( + self, + request: Request, + monitor_id, + checkin_id, + organization_slug: str | None = None, + *args, + **kwargs, + ): try: monitor = Monitor.objects.get(guid=monitor_id) except Monitor.DoesNotExist: @@ -51,6 +61,10 @@ def convert_args(self, request: Request, monitor_id, checkin_id, *args, **kwargs if project.status != ProjectStatus.VISIBLE: raise ResourceDoesNotExist + if organization_slug: + if project.organization.slug != organization_slug: + return self.respond_invalid() + if hasattr(request.auth, "project_id") and project.id != request.auth.project_id: return self.respond(status=400) @@ -102,12 +116,7 @@ def convert_args(self, request: Request, monitor_id, checkin_id, *args, **kwargs ) def get(self, request: Request, project, monitor, checkin) -> Response: """ - Retrieve a check-in - ``````````````````` - - :pparam string monitor_id: the id of the monitor. - :pparam string checkin_id: the id of the check-in. - :auth: required + Retrieves details for a check-in. You may use `latest` for the `checkin_id` parameter in order to retrieve the most recent (by creation date) check-in which is still mutable (not marked as finished). @@ -128,7 +137,7 @@ def get(self, request: Request, project, monitor, checkin) -> Response: request=MonitorCheckInValidator, responses={ 200: inline_sentry_response_serializer( - "MonitorCheckIn2", MonitorCheckInSerializerResponse + "MonitorCheckIn", MonitorCheckInSerializerResponse ), 208: RESPONSE_ALREADY_REPORTED, 400: RESPONSE_BAD_REQUEST, @@ -136,18 +145,14 @@ def get(self, request: Request, project, monitor, checkin) -> Response: 403: RESPONSE_FORBIDDEN, 404: RESPONSE_NOTFOUND, }, - examples=[ - # OpenApiExample() - ], ) def put(self, request: Request, project, monitor, checkin) -> Response: """ - Update a check-in - ````````````````` + Updates a check-in. + + Once a check-in is finished (indicated via an `ok` or `error` status) it can no longer be changed. - :pparam string monitor_id: the id of the monitor. - :pparam string checkin_id: the id of the check-in. - :auth: required + If you simply wish to update that the task is still running, you can simply send an empty payload. You may use `latest` for the `checkin_id` parameter in order to retrieve the most recent (by creation date) check-in which is still mutable (not marked as finished). @@ -165,15 +170,16 @@ def put(self, request: Request, project, monitor, checkin) -> Response: current_datetime = timezone.now() params = {"date_updated": current_datetime} + if "status" in result: + params["status"] = getattr(CheckInStatus, result["status"].upper()) + if "duration" in result: params["duration"] = result["duration"] - else: + # if a duration is not defined and we're at a finish state, calculate one + elif params.get("status", checkin.status) in CheckInStatus.FINISHED_VALUES: duration = int((current_datetime - checkin.date_added).total_seconds() * 1000) params["duration"] = duration - if "status" in result: - params["status"] = getattr(CheckInStatus, result["status"].upper()) - with transaction.atomic(): checkin.update(**params) if checkin.status == CheckInStatus.ERROR: diff --git a/src/sentry/api/endpoints/monitor_checkins.py b/src/sentry/api/endpoints/monitor_checkins.py index 59926d3ff76429..b0c46d67396aaa 100644 --- a/src/sentry/api/endpoints/monitor_checkins.py +++ b/src/sentry/api/endpoints/monitor_checkins.py @@ -51,11 +51,7 @@ def get( self, request: Request, project, monitor, organization_slug: str | None = None ) -> Response: """ - Retrieve check-ins for a monitor - ```````````````````````````````` - - :pparam string monitor_id: the id of the monitor. - :auth: required + Retrieve a list of check-ins for a monitor """ # we don't allow read permission with DSNs if isinstance(request.auth, ProjectKey): @@ -100,11 +96,12 @@ def post( self, request: Request, project, monitor, organization_slug: str | None = None ) -> Response: """ - Create a new check-in for a monitor - ``````````````````````````````````` + Creates a new check-in for a monitor. + + If `status` is not present, it will be assumed that the check-in is starting, and be marked as `in_progress`. - :pparam string monitor_id: the id of the monitor. - :auth: required + To achieve a ping-like behavior, you can simply define `status` and optionally `duration` and + this check-in will be automatically marked as finished. Note: If a DSN is utilized for authentication, the response will be limited in details. """ diff --git a/src/sentry/api/endpoints/monitor_details.py b/src/sentry/api/endpoints/monitor_details.py index 3f8bb077404b29..002f7a7e997120 100644 --- a/src/sentry/api/endpoints/monitor_details.py +++ b/src/sentry/api/endpoints/monitor_details.py @@ -44,11 +44,7 @@ def get( self, request: Request, project, monitor, organization_slug: str | None = None ) -> Response: """ - Retrieve a monitor - `````````````````` - - :pparam string monitor_id: the id of the monitor. - :auth: required + Retrieves details for a monitor. """ if organization_slug: if project.organization.slug != organization_slug: @@ -74,11 +70,7 @@ def put( self, request: Request, project, monitor, organization_slug: str | None = None ) -> Response: """ - Update a monitor - ```````````````` - - :pparam string monitor_id: the id of the monitor. - :auth: required + Update a monitor. """ if organization_slug: if project.organization.slug != organization_slug: @@ -145,12 +137,9 @@ def delete( self, request: Request, project, monitor, organization_slug: str | None = None ) -> Response: """ - Delete a monitor - ```````````````` - - :pparam string monitor_id: the id of the monitor. - :auth: required + Delete a monitor. """ + if organization_slug: if project.organization.slug != organization_slug: return self.respond_invalid() diff --git a/src/sentry/api/urls.py b/src/sentry/api/urls.py index 9bc34e4c8a598d..827784bc0f6074 100644 --- a/src/sentry/api/urls.py +++ b/src/sentry/api/urls.py @@ -713,6 +713,11 @@ MonitorCheckInsEndpoint.as_view(), name="sentry-api-0-monitor-check-in-index-with-org", ), + url( + r"^(?P[^\/]+)/checkins/(?P[^\/]+)/$", + MonitorCheckInDetailsEndpoint.as_view(), + name="sentry-api-0-monitor-check-in-details-with-org", + ), url( r"^(?P[^\/]+)/stats/$", MonitorStatsEndpoint.as_view(), diff --git a/tests/sentry/api/endpoints/test_monitor_checkin_details.py b/tests/sentry/api/endpoints/test_monitor_checkin_details.py index e2fbffba90eacc..3b7c7ed1fd4556 100644 --- a/tests/sentry/api/endpoints/test_monitor_checkin_details.py +++ b/tests/sentry/api/endpoints/test_monitor_checkin_details.py @@ -16,6 +16,28 @@ def setUp(self): super().setUp() self.login_as(self.user) + def test_noop_in_progerss(self): + monitor = Monitor.objects.create( + organization_id=self.organization.id, + project_id=self.project.id, + next_checkin=timezone.now() - timedelta(minutes=1), + type=MonitorType.CRON_JOB, + config={"schedule": "* * * * *"}, + date_added=timezone.now() - timedelta(minutes=1), + ) + checkin = MonitorCheckIn.objects.create( + monitor=monitor, + project_id=self.project.id, + date_added=monitor.date_added, + status=CheckInStatus.IN_PROGRESS, + ) + + self.get_success_response(monitor.guid, checkin.guid) + + checkin = MonitorCheckIn.objects.get(id=checkin.id) + assert checkin.status == CheckInStatus.IN_PROGRESS + assert checkin.date_updated > checkin.date_added + def test_passing(self): monitor = Monitor.objects.create( organization_id=self.organization.id,