feat(open periods): Expect open periods to exist #99548
Conversation
github-actions
bot
added
the
Scope: Backend
|
✅ All tests passed in 19559.72s |
|
|
||
|
|
||
| @dataclass | ||
| class OpenPeriod: |
There was a problem hiding this comment.
This is replaced by GroupOpenPeriodResponse in the serializer
| group: Group, | ||
| query_start: datetime | None = None, | ||
| query_end: datetime | None = None, | ||
| offset: int | None = None, |
There was a problem hiding this comment.
I removed offset because I'm not longer using the GenericOffsetPaginator and the way it was used here (by doing a greater than on the id field) seemed like a security risk - if you passed a random offset you could access various open period results
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
|
||
| if offset and limit: | ||
| return open_periods[offset : offset + limit] | ||
| if not query_start: |
There was a problem hiding this comment.
👀 well, this is a lot simpler now.
| group_open_periods = group_open_periods.filter(date_ended__lte=query_end) | ||
|
|
||
| return open_periods | ||
| return group_open_periods[:limit] |
There was a problem hiding this comment.
TIL that django is smart and will add this to db query as the limit rather than getting all the data and filtering the results in memory. 🎉
| @@ -1,7 +1,5 @@ | |||
| import logging | |||
There was a problem hiding this comment.
unrelated to the changes in this file -- but thoughts on moving this model into src/sentry/issues/models since it's an issue platform model?
| if not target_group: | ||
| return Response( | ||
| {"detail": "Group not found. Could not query open periods."}, | ||
| status=status.HTTP_200_OK, |
There was a problem hiding this comment.
should this be a 404 (or another 4XX status) response since the group isn't found?
There was a problem hiding this comment.
You know that's what I thought too, but I based this off of https://github.com/getsentry/sentry/blob/master/src/sentry/workflow_engine/endpoints/organization_detector_index.py#L422-L426 I think, happy to change it though
| } | ||
|
|
||
|
|
||
| class GroupOpenPeriodResponse(TypedDict): |
There was a problem hiding this comment.
nice! 🎉 i feel like i should add these for all the workflow engine serializers.
| group=target_group, | ||
| query_start=start, | ||
| query_end=end, | ||
| limit=limit, |
There was a problem hiding this comment.
Bug: Pagination Parameter Validation and Usage Issues
The per_page query parameter's validation is incomplete: non-numeric input or non-positive values can lead to unhandled exceptions (500s), and assert is unreliable for validation in production. Additionally, per_page is incorrectly used to pre-limit results before OffsetPaginator, causing double-limiting and incorrect pagination behavior.
Now that we are no longer calling
get_open_periods_for_groupin theGroupDetailsEndpoint(#99519) we can clean up the code that constructs an open period when there isn't actually aGroupOpenPeriod(this code was used before we ran the backfill) to return the objects and serialize them normally. This PR also adds theidto the response. A near future PR will add a serializer forGroupOpenPeriodActivityand add that to the response as well.