diff --git a/bugsink/api_pagination.py b/bugsink/api_pagination.py new file mode 100644 index 0000000..eb5e41e --- /dev/null +++ b/bugsink/api_pagination.py @@ -0,0 +1,30 @@ +from rest_framework.pagination import CursorPagination +from rest_framework.exceptions import ValidationError + + +class AscDescCursorPagination(CursorPagination): + """ + Cursor-based paginator that supports `?order=asc|desc`. + Each view sets: + base_ordering = ("field",) or ("field1", "field2") + default_direction = "asc" | "desc" + page_size = + """ + + base_ordering = None + default_direction = "desc" + + def get_ordering(self, request, queryset, view): + order_param = request.query_params.get("order") + if order_param and order_param not in ("asc", "desc"): + raise ValidationError({"order": ["Must be 'asc' or 'desc'."]}) + + direction = order_param or self.default_direction + + if self.base_ordering is None: + raise RuntimeError("AscDescCursorPagination requires base_ordering to be set.") + + ordering = [] + for field in self.base_ordering: + ordering.append(f"-{field}" if direction == "desc" else field) + return ordering diff --git a/events/api_views.py b/events/api_views.py index 188bcef..21cfbdd 100644 --- a/events/api_views.py +++ b/events/api_views.py @@ -3,11 +3,21 @@ from rest_framework import viewsets from rest_framework.exceptions import ValidationError from bugsink.utils import assert_ +from bugsink.api_pagination import AscDescCursorPagination from .models import Event from .serializers import EventListSerializer, EventDetailSerializer +class EventPagination(AscDescCursorPagination): + # Cursor pagination requires an indexed, mostly-stable ordering field. We use `digest_order`: we require + # ?issue= and have a composite (issue_id, digest_order) index, so ORDER BY digest_order after filtering by + # issue is fast and cursor-stable. (also note that digest_order comes in in-order). + base_ordering = ("digest_order",) + page_size = 250 + default_direction = "desc" # newest first by default, aligned with UI + + class EventViewSet(viewsets.ReadOnlyModelViewSet): """ LIST requires: ?issue= @@ -17,6 +27,7 @@ class EventViewSet(viewsets.ReadOnlyModelViewSet): """ queryset = Event.objects.all() # router requirement for basename inference serializer_class = EventListSerializer + pagination_class = EventPagination def filter_queryset(self, queryset): query_params = self.request.query_params @@ -24,14 +35,7 @@ class EventViewSet(viewsets.ReadOnlyModelViewSet): if "issue" not in query_params: raise ValidationError({"issue": ["This field is required."]}) - order = query_params.get("order", "desc") - if order not in ("asc", "desc"): - raise ValidationError({"order": ["Must be 'asc' or 'desc'."]}) - - ordering = "digest_order" if order == "asc" else "-digest_order" - - # (issue, digest_order) is a db-index - return queryset.filter(issue=query_params["issue"]).order_by(ordering) + return queryset.filter(issue=query_params["issue"]) def get_object(self): """ diff --git a/events/test_api.py b/events/test_api.py index c6fcf64..37d1f40 100644 --- a/events/test_api.py +++ b/events/test_api.py @@ -5,7 +5,10 @@ from rest_framework.test import APIClient from projects.models import Project from bsmain.models import AuthToken from events.factories import create_event +from events.api_views import EventViewSet + from issues.factories import get_or_create_issue +from events.factories import create_event_data class EventApiTests(DjangoTestCase): @@ -65,3 +68,50 @@ class EventApiTests(DjangoTestCase): ids = [item["id"] for item in response.json()["results"]] self.assertEqual(ids[0], str(e0.id)) self.assertEqual(ids[1], str(e1.id)) + + +class EventPaginationTests(DjangoTestCase): + def setUp(self): + self.client = APIClient() + token = AuthToken.objects.create() + self.client.credentials(HTTP_AUTHORIZATION=f"Bearer {token.token}") + self.old_size = EventViewSet.pagination_class.page_size + EventViewSet.pagination_class.page_size = 2 + + def tearDown(self): + EventViewSet.pagination_class.page_size = self.old_size + + def _make_events(self, issue, n=5): + events = [] + for i in range(n): + ev = create_event(issue=issue) + events.append(ev) + return events + + def _ids(self, resp): + return [row["id"] for row in resp.json()["results"]] + + def test_digest_order_desc_two_pages(self): + proj = Project.objects.create(name="P") + issue = get_or_create_issue(project=proj, event_data=create_event_data(exception_type="root"))[0] + events = self._make_events(issue, 5) + + # default (desc) → events 5,4 on page 1; 3,2 on page 2 + r1 = self.client.get(reverse("api:event-list"), {"issue": str(issue.id)}) + self.assertEqual(self._ids(r1), [str(events[4].id), str(events[3].id)]) + + r2 = self.client.get(r1.json()["next"]) + self.assertEqual(self._ids(r2), [str(events[2].id), str(events[1].id)]) + + def test_digest_order_asc_two_pages(self): + proj = Project.objects.create(name="P2") + issue = get_or_create_issue(project=proj, event_data=create_event_data(exception_type="root2"))[0] + events = self._make_events(issue, 5) + + # asc → events 1,2 on page 1; 3,4 on page 2 + r1 = self.client.get(reverse("api:event-list"), + {"issue": str(issue.id), "order": "asc"}) + self.assertEqual(self._ids(r1), [str(events[0].id), str(events[1].id)]) + + r2 = self.client.get(r1.json()["next"]) + self.assertEqual(self._ids(r2), [str(events[2].id), str(events[3].id)]) diff --git a/issues/api_views.py b/issues/api_views.py index 23ad92b..8f79c96 100644 --- a/issues/api_views.py +++ b/issues/api_views.py @@ -1,11 +1,58 @@ from django.shortcuts import get_object_or_404 from rest_framework import viewsets +from rest_framework.pagination import CursorPagination from rest_framework.exceptions import ValidationError from .models import Issue from .serializers import IssueSerializer +class IssuesCursorPagination(CursorPagination): + """ + Cursor paginator for /issues supporting ?sort=… and ?order=asc|desc. + + Sort modes are named after the *primary* column: + - sort=digest_order → unique per project → no tie-breakers needed + - sort=last_seen → timestamp → tie-breaker on id + + Direction applies to primary *and beyond* (i.e. all fields in the list). + The view MUST filter by project; ordering is handled here. + """ + # Cursor pagination requires an indexed, mostly-stable ordering. Stable mode: sort=digest_order (default). We + # require ?project= and have a composite (project_id, digest_order) index, so ORDER BY digest_order after + # filtering by project is fast and cursor-stable. + + # We also offer a "recent" mode: sort=last_seen. This is not stable, as new events can come in mid-cursor, and + # reshuffle things causing misses or duplicates. However, this is the desired UX for a "recent activity" view. + # i.e. the typical usage would in fact just be to get the "first page" of recent activity. + page_size = 250 + default_direction = "asc" + default_sort = "digest_order" + + VALID_SORTS = ("digest_order", "last_seen") + VALID_ORDERS = ("asc", "desc") + + def get_ordering(self, request, queryset, view): + sort = request.query_params.get("sort", self.default_sort) + if sort not in self.VALID_SORTS: + raise ValidationError({"sort": ["Must be 'digest_order' or 'last_seen'."]}) + + order = request.query_params.get("order", self.default_direction) + if order not in self.VALID_ORDERS: + raise ValidationError({"order": ["Must be 'asc' or 'desc'."]}) + + desc = (order == "desc") + + if sort == "digest_order": + # Unique per project; stable cursor once filtered by project. + return ["-digest_order" if desc else "digest_order"] + + # sort == "last_seen": timestamp needs a deterministic tie-breaker. + if desc: + return ["-last_seen", "-id"] + return ["last_seen", "id"] + + class IssueViewSet(viewsets.ReadOnlyModelViewSet): """ LIST requires: ?project= @@ -15,6 +62,7 @@ class IssueViewSet(viewsets.ReadOnlyModelViewSet): """ queryset = Issue.objects.filter(is_deleted=False) # hide soft-deleted issues; also satisfies router serializer_class = IssueSerializer + pagination_class = IssuesCursorPagination def get_queryset(self): return self.queryset @@ -24,19 +72,12 @@ class IssueViewSet(viewsets.ReadOnlyModelViewSet): if self.action != "list": return queryset - query_params = self.request.query_params - - project = query_params.get("project") + project = self.request.query_params.get("project") if not project: - # the below until we have a UI for cross-project Issue listing, i.e. #190 + # the below at least until we have a UI for cross-project Issue listing, i.e. #190 raise ValidationError({"project": ["This field is required."]}) - order = query_params.get("order", "desc") - if order not in ("asc", "desc"): - raise ValidationError({"order": ["Must be 'asc' or 'desc'."]}) - - ordering = "last_seen" if order == "asc" else "-last_seen" - return queryset.filter(project=project).order_by(ordering) + return queryset.filter(project=project) def get_object(self): """ diff --git a/issues/test_api.py b/issues/test_api.py index 2b84bd4..68b4b6b 100644 --- a/issues/test_api.py +++ b/issues/test_api.py @@ -10,6 +10,8 @@ from issues.models import Issue from issues.factories import get_or_create_issue from events.factories import create_event_data +from issues.api_views import IssueViewSet + class IssueApiTests(DjangoTestCase): def setUp(self): @@ -38,20 +40,20 @@ class IssueApiTests(DjangoTestCase): self.assertEqual(response.status_code, 400) self.assertEqual({"project": ["This field is required."]}, response.json()) - def test_list_by_project_default_desc(self): + def test_list_by_project_default_asc(self): response = self.client.get(reverse("api:issue-list"), {"project": str(self.project.id)}) self.assertEqual(response.status_code, 200) ids = [row["id"] for row in response.json()["results"]] - self.assertEqual(ids[0], str(self.issue1.id)) - self.assertEqual(ids[1], str(self.issue0.id)) - - def test_list_by_project_order_asc(self): - response = self.client.get(reverse("api:issue-list"), {"project": str(self.project.id), "order": "asc"}) - self.assertEqual(response.status_code, 200) - ids = [row["id"] for row in response.json()["results"]] self.assertEqual(ids[0], str(self.issue0.id)) self.assertEqual(ids[1], str(self.issue1.id)) + def test_list_by_project_order_desc(self): + response = self.client.get(reverse("api:issue-list"), {"project": str(self.project.id), "order": "desc"}) + self.assertEqual(response.status_code, 200) + ids = [row["id"] for row in response.json()["results"]] + self.assertEqual(ids[0], str(self.issue1.id)) + self.assertEqual(ids[1], str(self.issue0.id)) + def test_list_rejects_bad_order(self): response = self.client.get(reverse("api:issue-list"), {"project": str(self.project.id), "order": "sideways"}) self.assertEqual(response.status_code, 400) @@ -74,3 +76,93 @@ class IssueApiTests(DjangoTestCase): url = reverse("api:issue-detail", args=[self.issue0.id]) response = self.client.get(url) self.assertEqual(response.status_code, 404) + + def test_list_rejects_bad_sort(self): + r = self.client.get( + reverse("api:issue-list"), + {"project": str(self.project.id), "sort": "nope"}, + ) + self.assertEqual(r.status_code, 400) + self.assertEqual(r.json(), {"sort": ["Must be 'digest_order' or 'last_seen'."]}) + + +class IssuePaginationTests(DjangoTestCase): + last_seen_deltas = [3, 1, 4, 0, 2] + + def setUp(self): + self.client = APIClient() + token = AuthToken.objects.create() + self.client.credentials(HTTP_AUTHORIZATION=f"Bearer {token.token}") + self.old_size = IssueViewSet.pagination_class.page_size + IssueViewSet.pagination_class.page_size = 2 + + def tearDown(self): + IssueViewSet.pagination_class.page_size = self.old_size + + def _make_issues(self): + proj = Project.objects.create(name="P") + base = timezone.now().replace(microsecond=0) + issues = [] + for i, delta in enumerate(self.last_seen_deltas): + data = create_event_data(exception_type=f"E{i}") + iss = get_or_create_issue(project=proj, event_data=data)[0] + iss.digest_order = i + 1 + iss.last_seen = base + timezone.timedelta(minutes=delta) + iss.save(update_fields=["digest_order", "last_seen"]) + issues.append(iss) + return proj, issues + + def _ids(self, resp): + return [row["id"] for row in resp.json()["results"]] + + def _idx_by_last_seen(self, issues, minutes): + return issues[self.last_seen_deltas.index(minutes)].id + + def _idx_by_digest(self, issues, n): + return issues[n - 1].id # digest_order = n + + def test_digest_order_asc(self): + proj, issues = self._make_issues() + r1 = self.client.get( + reverse("api:issue-list"), + {"project": str(proj.id), "sort": "digest_order", "order": "asc"}) + + self.assertEqual(self._ids(r1), [str(self._idx_by_digest(issues, 1)), str(self._idx_by_digest(issues, 2))]) + + r2 = self.client.get(r1.json()["next"]) + self.assertEqual(self._ids(r2), [str(self._idx_by_digest(issues, 3)), str(self._idx_by_digest(issues, 4))]) + + def test_digest_order_desc(self): + proj, issues = self._make_issues() + r1 = self.client.get( + reverse("api:issue-list"), {"project": str(proj.id), "sort": "digest_order", "order": "desc"}) + + self.assertEqual(self._ids(r1), [str(self._idx_by_digest(issues, 5)), str(self._idx_by_digest(issues, 4))]) + + r2 = self.client.get(r1.json()["next"]) + self.assertEqual(self._ids(r2), [str(self._idx_by_digest(issues, 3)), str(self._idx_by_digest(issues, 2))]) + + def test_last_seen_asc(self): + proj, issues = self._make_issues() + r1 = self.client.get( + reverse("api:issue-list"), {"project": str(proj.id), "sort": "last_seen", "order": "asc"}) + + self.assertEqual( + self._ids(r1), [str(self._idx_by_last_seen(issues, 0)), str(self._idx_by_last_seen(issues, 1))]) + + r2 = self.client.get(r1.json()["next"]) + self.assertEqual(self._ids(r2), + [str(self._idx_by_last_seen(issues, 2)), str(self._idx_by_last_seen(issues, 3))]) + + def test_last_seen_desc(self): + proj, issues = self._make_issues() + + r1 = self.client.get( + reverse("api:issue-list"), {"project": str(proj.id), "sort": "last_seen", "order": "desc"}) + + self.assertEqual( + self._ids(r1), [str(self._idx_by_last_seen(issues, 4)), str(self._idx_by_last_seen(issues, 3))]) + + r2 = self.client.get(r1.json()["next"]) + self.assertEqual( + self._ids(r2), [str(self._idx_by_last_seen(issues, 2)), str(self._idx_by_last_seen(issues, 1))]) diff --git a/projects/api_views.py b/projects/api_views.py index 7691ad3..111bf0a 100644 --- a/projects/api_views.py +++ b/projects/api_views.py @@ -1,5 +1,6 @@ from django.shortcuts import get_object_or_404 from rest_framework import viewsets +from bugsink.api_pagination import AscDescCursorPagination from .models import Project from .serializers import ( @@ -9,6 +10,14 @@ from .serializers import ( ) +class ProjectPagination(AscDescCursorPagination): + # Cursor pagination requires an indexed, mostly-stable ordering field. We use `name`, which is indexed; for Teams, + # updates are rare and the table is small, so "requirement met in practice though not in theory". + base_ordering = ("name",) + page_size = 250 + default_direction = "asc" + + class ProjectViewSet(viewsets.ModelViewSet): """ /api/canonical/0/projects/ @@ -20,6 +29,7 @@ class ProjectViewSet(viewsets.ModelViewSet): """ queryset = Project.objects.all() http_method_names = ["get", "post", "patch", "head", "options"] + pagination_class = ProjectPagination def filter_queryset(self, queryset): if self.action != "list": @@ -34,8 +44,7 @@ class ProjectViewSet(viewsets.ModelViewSet): if team_id: qs = qs.filter(team=team_id) - # Explicit ordering aligned with UI - return qs.order_by("name") + return qs def get_object(self): # Pure PK lookup (bypass filter_queryset) diff --git a/teams/api_views.py b/teams/api_views.py index 966ce65..1e22606 100644 --- a/teams/api_views.py +++ b/teams/api_views.py @@ -1,5 +1,8 @@ from django.shortcuts import get_object_or_404 from rest_framework import viewsets + +from bugsink.api_pagination import AscDescCursorPagination + from .models import Team from .serializers import ( TeamListSerializer, @@ -8,6 +11,14 @@ from .serializers import ( ) +class TeamPagination(AscDescCursorPagination): + # Cursor pagination requires an indexed, mostly-stable ordering field. We use `name`, which is indexed; for Teams, + # updates are rare and the table is small, so "requirement met in practice though not in theory". + base_ordering = ("name",) + page_size = 250 + default_direction = "asc" + + class TeamViewSet(viewsets.ModelViewSet): """ /api/canonical/0/teams/ @@ -19,12 +30,7 @@ class TeamViewSet(viewsets.ModelViewSet): """ queryset = Team.objects.all() http_method_names = ["get", "post", "patch", "head", "options"] - - def filter_queryset(self, queryset): - if self.action != "list": - return queryset - # Explicit ordering aligned with UI - return queryset.order_by("name") + pagination_class = TeamPagination def get_object(self): # Pure PK lookup (bypass filter_queryset)