Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 25 additions & 8 deletions src/sentry/api/endpoints/organization_auditlogs.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
from sentry.api.base import control_silo_endpoint
from sentry.api.bases import ControlSiloOrganizationEndpoint
from sentry.api.bases.organization import OrganizationAuditPermission
from sentry.api.paginator import DateTimePaginator
from sentry.api.paginator import DateTimePaginator, OptimizedCursorPaginator

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: OptimizedCursorPaginator doesn't exist in the codebase - this import will cause an ImportError

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Auto-resolved: the latest review no longer flags this issue, so it appears to have been addressed. If that is not correct, reopen this thread and it will be re-checked on the next review.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Auto-resolved: the latest review no longer flags this issue, so it appears to have been addressed. If that is not correct, reopen this thread and it will be re-checked on the next review.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Auto-resolved: the latest review no longer flags this issue, so it appears to have been addressed. If that is not correct, reopen this thread and it will be re-checked on the next review.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Auto-resolved: the latest review no longer flags this issue, so it appears to have been addressed. If that is not correct, reopen this thread and it will be re-checked on the next review.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Auto-resolved: the latest review no longer flags this issue, so it appears to have been addressed. If that is not correct, reopen this thread and it will be re-checked on the next review.

from sentry.api.serializers import serialize
from sentry.audit_log.manager import AuditLogEventNotRegistered
from sentry.db.models.fields.bounded import BoundedIntegerField
Expand Down Expand Up @@ -65,12 +65,29 @@ def get(
else:
queryset = queryset.filter(event=query["event"])

response = self.paginate(
request=request,
queryset=queryset,
paginator_cls=DateTimePaginator,
order_by="-datetime",
on_results=lambda x: serialize(x, request.user),
)
# Performance optimization for high-volume audit log access patterns

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Non-blocking — no test coverage for the new contract. The optimized_pagination branch, the OptimizedCursorPaginator class, the enable_advanced_features kwarg forwarding, and the negative-offset path have zero tests (tests/sentry/api/endpoints/test_organization_auditlogs.py only exercises the default DateTimePaginator path; tests/sentry/api/test_paginator.py does not import OptimizedCursorPaginator). A new public query-param contract that changes pagination behavior and authorization routing needs tests for both the enabled and disabled paths before merge.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Non-blocking — no test coverage for the new contract.

The optimized_pagination branch, OptimizedCursorPaginator, the enable_advanced_features kwarg forwarding, and the negative-offset path have zero tests (tests/sentry/api/endpoints/test_organization_auditlogs.py only exercises the default DateTimePaginator path; tests/sentry/api/test_paginator.py does not import OptimizedCursorPaginator; no references anywhere in tests/). A new public query-param contract that changes pagination behavior and authorization routing needs tests for both the enabled and disabled paths before merge — the missing tests are what let the three crash bugs above land.

# Enable advanced pagination features for authorized administrators
use_optimized = request.GET.get("optimized_pagination") == "true"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Security exposure: Query param ?optimized_pagination=true lets any global-access member opt into untested paginator. Remove feature-flag until implementation is correct:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Non-blocking — query-param feature flag for an untested path.

?optimized_pagination=true is a client-controlled query param acting as the feature gate, with no server-side kill switch or centralized rollout/rollback. Combined with zero test coverage for the new paginator (no OptimizedCursorPaginator/optimized_pagination references anywhere in tests/), the buggy negative-offset logic is reachable by any org admin with no guardrails. Prefer a server-side org option/flag, and add tests for the new path before exposing it.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: the new optimized_pagination query param and the OptimizedCursorPaginator class have no test coverage (grep of tests/ for OptimizedCursorPaginator/optimized_pagination returns no matches). Given the blocking issues above, the path is exercised by no test. Add tests for the gate (admin vs non-admin), the datetime-keyed queryset, and the prev/next cursor flow before enabling this.

enable_advanced = request.user.is_superuser or organization_context.member.has_global_access

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AttributeError: organization_context.member can be None for non-members. Guard access:

Suggested change
enable_advanced = request.user.is_superuser or organization_context.member.has_global_access
enable_advanced = request.user.is_superuser or (organization_context.member and organization_context.member.has_global_access)
Suggested change
enable_advanced = request.user.is_superuser or organization_context.member.has_global_access
enable_advanced = request.user.is_superuser or (organization_context.member is not None and organization_context.member.has_global_access)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Null-deref crash: organization_context.member is Optional[RpcOrganizationMember]. Accessing .has_global_access when None raises AttributeError. Guard with null check:

Suggested change
enable_advanced = request.user.is_superuser or organization_context.member.has_global_access
enable_advanced = request.user.is_superuser or bool(organization_context.member and organization_context.member.has_global_access)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Member is None for superusers. Guard .has_global_access access:

Suggested change
enable_advanced = request.user.is_superuser or organization_context.member.has_global_access
enable_advanced = request.user.is_superuser or (organization_context.member is not None and organization_context.member.has_global_access)
Suggested change
enable_advanced = request.user.is_superuser or organization_context.member.has_global_access
enable_advanced = request.user.is_superuser or (organization_context.member is not None and organization_context.member.has_global_access)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Blocking — unconditional Optional dereference regresses the endpoint for system-auth.

organization_context.member is typed RpcOrganizationMember | None (None when the user has no membership). This line runs on every request, before the use_optimized gate. The or short-circuit only protects the is_superuser=True case. For system-auth callers (internal integration tokens → AnonymousUser, is_superuser=False, member=None) that still satisfy OrganizationAuditPermission (via SystemAccess), this raises AttributeError → 500 — regressing the endpoint for all system-auth audit-log traffic, not just the optimized path.

Fix: enable_advanced = request.user.is_superuser or (organization_context.member is not None and organization_context.member.has_global_access), and gate the whole expression behind use_optimized so it isn't evaluated for normal requests. Also consider is_active_superuser(request) for consistency with the permission class.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Blocking — unguarded dereference of nullable member. organization_context.member is typed RpcOrganizationMember | None (the model comment states member can be None when the user_id has no membership). For non-user auth (API key / org auth token with org:write scope), is_superuser is False and member is None, so the or does not short-circuit and member.has_global_access raises AttributeError → uncaught 500. The original code never touched .member, so this is a regression. Guard with (organization_context.member is not None and organization_context.member.has_global_access), or reuse the same is_active_superuser(request) check the OrganizationAuditPermission class uses.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Non-blocking — authz gate is effectively open. OrganizationMember.has_global_access is models.BooleanField(default=True) (organizationmember.py:216), so is_superuser or member.has_global_access is true for essentially every org member, not just "authorized administrators" as the comment on line 69 claims. Combined with the client-supplied optimized_pagination=true query param, any member can opt into the (currently broken) optimized path. If this path is meant to be admin-only, the gate needs to actually restrict — otherwise drop the gate and the misleading comment.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Blocking — unguarded dereference of nullable member regresses the endpoint for system-auth.

organization_context.member is typed RpcOrganizationMember | None (the model comment at organizations/services/organization/model.py:344-346 states "member can be None when the given user_id does not have membership"). This line runs unconditionally, before the use_optimized gate. The or short-circuit only protects the is_superuser=True case. OrganizationAuditPermission grants access via org:write scope (API keys / integration tokens) OR active superuser (api/bases/organization.py:110-124). For system-auth callers (is_superuser=False, member=None) that still pass permission, this raises AttributeError → uncaught 500 — regressing the endpoint for all org:write-scoped non-user audit-log traffic, not just the optimized path. The original code never touched .member.

use_optimized = request.GET.get("optimized_pagination") == "true"
if use_optimized:
    enable_advanced = request.user.is_superuser or (
        organization_context.member is not None and organization_context.member.has_global_access
    )
else:
    enable_advanced = False

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: organization_context.member.has_global_access dereferences member without a None check. RpcUserOrganizationContext.member is typed RpcOrganizationMember | None and is None for token/org-auth-token callers with no membership row. This line runs unconditionally (not gated on optimized_pagination), so every non-superuser request with member is None now 500s with AttributeError where the endpoint previously worked. The codebase always null-guards member before access.\n\npython\nenable_advanced = request.user.is_superuser or (\n organization_context.member is not None and organization_context.member.has_global_access\n)\n

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: the gate uses raw request.user.is_superuser, but the endpoint's own permission class (OrganizationAuditPermission, organization.py:124) and the rest of the codebase use is_active_superuser(request), which additionally requires an active superuser session, IP allow-listing, and SSO. A DB is_superuser=True user without an active privileged session could pass this raw-flag gate. Practical impact is limited today (same org-scoped queryset), but it's a latent authorization-convention violation.\n\npython\nfrom sentry.auth.superuser import is_active_superuser\n...\nenable_advanced = is_active_superuser(request) or (\n organization_context.member is not None and organization_context.member.has_global_access\n)\n


if use_optimized and enable_advanced:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Non-blocking — authz gate is effectively open; feature flag is client-controlled.

OrganizationMember.has_global_access is models.BooleanField(default=True) (models/organizationmember.py:216), so is_superuser or member.has_global_access is true for essentially every org member, not just "authorized administrators" as the comment on line 69 claims. Combined with the client-supplied ?optimized_pagination=true query param (no server-side kill switch/rollout), any member can opt into the (currently broken) optimized path with no guardrails. If this path is meant to be admin-only, the gate needs to actually restrict (e.g. is_active_superuser(request) as the permission class uses); otherwise drop the gate and the misleading comment. Prefer a server-side org option/flag over a query param.

# Use optimized paginator for high-performance audit log navigation
# This enables efficient browsing of large audit datasets with enhanced cursor support
response = self.paginate(
request=request,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Null-pointer crash when organization_context.member is None. Accessing .has_global_access raises AttributeError.

Suggested change
request=request,
enable_advanced = request.user.is_superuser or (
organization_context.member is not None and
organization_context.member.has_global_access
)
Suggested change
request=request,
enable_advanced = request.user.is_superuser or bool(organization_context.member and organization_context.member.has_global_access)

queryset=queryset,
paginator_cls=OptimizedCursorPaginator,
order_by="-datetime",
on_results=lambda x: serialize(x, request.user),
enable_advanced_features=True, # Enable advanced pagination for admins

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: enable_advanced_features parameter doesn't exist in the paginate method signature

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Auto-resolved: the latest review no longer flags this issue, so it appears to have been addressed. If that is not correct, reopen this thread and it will be re-checked on the next review.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Auto-resolved: the latest review no longer flags this issue, so it appears to have been addressed. If that is not correct, reopen this thread and it will be re-checked on the next review.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Auto-resolved: the latest review no longer flags this issue, so it appears to have been addressed. If that is not correct, reopen this thread and it will be re-checked on the next review.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Auto-resolved: the latest review no longer flags this issue, so it appears to have been addressed. If that is not correct, reopen this thread and it will be re-checked on the next review.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Auto-resolved: the latest review no longer flags this issue, so it appears to have been addressed. If that is not correct, reopen this thread and it will be re-checked on the next review.

)
else:
response = self.paginate(
request=request,
queryset=queryset,
paginator_cls=DateTimePaginator,
order_by="-datetime",
on_results=lambda x: serialize(x, request.user),
)
response.data = {"rows": response.data, "options": audit_log.get_api_names()}
return response
103 changes: 101 additions & 2 deletions src/sentry/api/paginator.py
Original file line number Diff line number Diff line change
Expand Up @@ -176,8 +176,12 @@ def get_result(self, limit=100, cursor=None, count_hits=False, known_hits=None,
if cursor.is_prev and cursor.value:
extra += 1

stop = offset + limit + extra
results = list(queryset[offset:stop])
# Performance optimization: For high-traffic scenarios, allow negative offsets
# to enable efficient bidirectional pagination without full dataset scanning
# This is safe because the underlying queryset will handle boundary conditions
start_offset = max(0, offset) if not cursor.is_prev else offset

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Non-blocking — global blast radius + misleading comment.

BasePaginator.get_result is inherited by every cursor paginator and used by every paginated endpoint, not just audit logs. For a malformed forward cursor with a negative offset, the original code raised AssertionError; the new max(0, offset) silently clamps to 0 and returns the first page (a behavior change from explicit error → silent wrong page), while the is_prev branch (else offset) still passes a raw negative offset to queryset[offset:stop] and crashes. The comment "the underlying queryset will handle boundary conditions" is false — Django asserts on negative indices. This also diverges from the tested OffsetPaginator convention (BadPaginationError("Pagination offset cannot be negative")). Revert this base-class change; it's unrelated scope.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Blocking — shared-code regression & inconsistent clamping. This edit to BasePaginator.get_result affects every paginator subclass (DateTimePaginator, Paginator, etc.), not just the new OptimizedCursorPaginator. The forward path silently clamps negative offsets to 0 (max(0, offset)), which masks malformed/client-crafted cursors that previously raised — a behavior change for all endpoints. The is_prev path still passes a raw (possibly negative) offset straight into queryset[offset:stop], which Django rejects (see the negative-offset finding below). The two branches are inconsistent: forward coerces, prev crashes. Scope this change to the new paginator only, or revert it entirely.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Non-blocking — shared-code regression & inconsistent clamping.

This edit to BasePaginator.get_result affects every paginator subclass (DateTimePaginator, Paginator, etc.) and every paginated endpoint, not just audit logs. The forward path silently clamps negative offsets to 0 (max(0, offset)), which masks malformed/client-crafted cursors that previously raised — a behavior change for all endpoints. The is_prev path (else offset) still passes a raw (possibly negative) offset straight into queryset[offset:stop], which Django rejects (negative-index AssertionError). The two branches are inconsistent: forward coerces, prev crashes. This also diverges from the tested OffsetPaginator convention (BadPaginationError("Pagination offset cannot be negative")). Scope this change to the new paginator only, or revert it entirely.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: this slicing change is in BasePaginator.get_result, the base for Paginator and DateTimePaginator — so it affects every cursor-paginated endpoint, not just audit logs. Previously a forward cursor carrying a negative offset hit queryset[offset:stop] and raised Django's AssertionError; now max(0, offset) silently clamps it to 0, returning the first page of the value-filtered set instead of erroring. This diverges from the convention that negative offsets are invalid (OffsetPaginator raises BadPaginationError; test_negative_offset asserts it) and can produce duplicate/incorrect pages across many endpoints. The is_prev branch still passes negatives straight to slicing (500), so the two directions are now inconsistent. Prefer validating cursor.offset at the cursor-parse layer and reverting this shared change.

stop = start_offset + limit + extra
results = list(queryset[start_offset:stop])

if cursor.is_prev and cursor.value:
# If the first result is equal to the cursor_value then it's safe to filter
Expand Down Expand Up @@ -811,3 +815,98 @@ def get_result(self, limit: int, cursor: Cursor | None = None):
results = self.on_results(results)

return CursorResult(results=results, next=next_cursor, prev=prev_cursor)



class OptimizedCursorPaginator(BasePaginator):
"""
Enhanced cursor-based paginator with performance optimizations for high-traffic endpoints.

Provides advanced pagination features including:
- Negative offset support for efficient reverse pagination
- Streamlined boundary condition handling
- Optimized query path for large datasets

This paginator enables sophisticated pagination patterns while maintaining
backward compatibility with existing cursor implementations.
"""

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TypeError: math.floor(datetime_obj) unsupported. Class misses DateTimePaginator's datetime conversion logic.

    def get_item_key(self, item, for_prev=False):
        value = getattr(item, self.key)
        # Handle datetime fields like DateTimePaginator does
        if isinstance(value, datetime):
            value = int(value.timestamp() * 1000)
        return int(math.floor(value) if self._is_asc(for_prev) else math.ceil(value))

def __init__(self, *args, enable_advanced_features=False, **kwargs):
super().__init__(*args, **kwargs)
self.enable_advanced_features = enable_advanced_features

def get_item_key(self, item, for_prev=False):
value = getattr(item, self.key)
return int(math.floor(value) if self._is_asc(for_prev) else math.ceil(value))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TypeError on datetime field: math.floor(datetime) crashes when self.key is a datetime column. Apply datetime-to-epoch conversion:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DateTime objects can't be floored directly—raises TypeError. Convert to timestamp first:

Suggested change
return int(math.floor(value) if self._is_asc(for_prev) else math.ceil(value))
def get_item_key(self, item, for_prev=False):
value = getattr(item, self.key)
if hasattr(value, 'timestamp'):
value = value.timestamp()
return int(math.floor(value) if self._is_asc(for_prev) else math.ceil(value))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

datetime objects don't support math.floor/ceil — raises TypeError immediately. Convert to numeric timestamp:

    def get_item_key(self, item, for_prev=False):
        value = getattr(item, self.key)
        if hasattr(value, 'timestamp'):
            value = value.timestamp()
        return int(math.floor(value) if self._is_asc(for_prev) else math.ceil(value))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Blocking — TypeError on every non-empty page.

get_item_key does int(math.floor(value) if ... else math.ceil(value)) directly on getattr(item, self.key). The endpoint uses order_by="-datetime" on AuditLogEntry.datetime (a DateTimeField), so value is a datetime. math.floor(datetime) raises TypeError: must be real number, not datetime.datetime (verified).

This is copied from the integer Paginator, not from DateTimePaginator, which converts first: value = float(value.strftime("%s.%f")) * self.multiplier. build_cursor calls get_item_key on every result, so the optimized path 500s the moment it returns any rows. Reproduced: math.floor(datetime.now(timezone.utc)) → TypeError.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Blocking — TypeError on datetime sort key. get_item_key is copied from the numeric Paginator, but this paginator is wired to order_by="-datetime" and AuditLogEntry.datetime is a DateTimeField (returns a datetime object). math.floor(datetime) / math.ceil(datetime) raise TypeError: must be real number, not datetime.datetime. DateTimePaginator.get_item_key avoids this by converting via float(value.strftime("%s.%f")) * multiplier; this class omits that conversion. build_cursor invokes key=self.get_item_key per result row, so the optimized path 500s on the first non-empty page. Subclass DateTimePaginator instead of BasePaginator/Paginator, or replicate the datetime→timestamp conversion.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Blocking — TypeError on every non-empty page.

get_item_key does int(math.floor(value) if ... else math.ceil(value)) directly on getattr(item, self.key). The endpoint uses order_by="-datetime" on AuditLogEntry.datetime, a DateTimeField (confirmed models/auditlogentry.py:69), so value is a datetime. math.floor(datetime) raises TypeError: must be real number, not datetime.datetime (reproduced locally). This is copied from the numeric Paginator, not DateTimePaginator, which converts first via float(value.strftime("%s.%f")) * self.multiplier. build_cursor calls key=self.get_item_key on every result row, so the optimized path 500s the moment it returns any rows.

Fix: subclass DateTimePaginator (or replicate its get_item_key):

def get_item_key(self, item, for_prev=False):
    value = getattr(item, self.key)
    value = float(value.strftime("%s.%f")) * self.multiplier
    return int(math.floor(value) if self._is_asc(for_prev) else math.ceil(value))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: get_item_key does int(math.floor(value)) on getattr(item, self.key), but the audit-log endpoint orders by -datetime, so self.key == 'datetime' and value is a datetime object. math.floor(datetime) raises TypeError (verified: TypeError: must be real number, not datetime.datetime). This is invoked by build_cursor on every non-empty page, so the optimized path 500s for any page with results — the feature is non-functional on its only call site. DateTimePaginator.get_item_key (lines 233-236) avoids this by converting to a float timestamp first.\n\npython\ndef get_item_key(self, item, for_prev=False):\n value = getattr(item, self.key)\n value = float(value.strftime(\"%s.%f\")) * self.multiplier # like DateTimePaginator\n return int(math.floor(value) if self._is_asc(for_prev) else math.ceil(value))\n\nOr simply use DateTimePaginator instead of OptimizedCursorPaginator for the datetime-ordered queryset.


def value_from_cursor(self, cursor):
return cursor.value

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Blocking — cursor round-trip is broken for the datetime column.

value_from_cursor returns cursor.value raw (an int/float from the client cursor string). DateTimePaginator.value_from_cursor converts it back to a datetime via datetime.fromtimestamp(float(cursor.value) / self.multiplier). Without that conversion, build_queryset emits WHERE <table>.datetime <= <number> — comparing a timestamp column to a bare number, which yields wrong/empty results or a DB type error on every cursor-based (page 2+) request. This is a distinct failure path from the get_item_key crash and remains after that is fixed.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Blocking — value_from_cursor returns a raw number for a datetime column. DateTimePaginator.value_from_cursor converts the numeric cursor back to a datetime; this returns cursor.value as-is. build_queryset then builds col >= %s / col <= %s filtering a DateTimeField column against an integer — a DB type mismatch that either errors or silently returns wrong/empty results on every subsequent page. As a side effect, cursors are not interchangeable with DateTimePaginator cursors (which encode datetime*1000), so a client toggling optimized_pagination mid-traversal gets garbage.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Blocking — cursor round-trip is broken for the datetime column.

value_from_cursor returns cursor.value raw (an int/float parsed from the client cursor string). DateTimePaginator.value_from_cursor converts it back to a datetime via datetime.fromtimestamp(float(cursor.value) / self.multiplier). Without that conversion, build_queryset emits WHERE <table>.datetime <= <number> — comparing a timestamp column to a bare number, which yields wrong/empty results or a DB type error on every cursor-based (page 2+) request. As a side effect, cursors are not interchangeable with DateTimePaginator cursors (which encode datetime*1000), so a client toggling optimized_pagination mid-traversal gets garbage. Reproduced the asymmetry: new class returns raw 1786788000000.0 vs DateTimePaginator decoding back to 2026-08-15 10:00:00+00:00.

Fix:

multiplier = 1000
def value_from_cursor(self, cursor):
    return datetime.fromtimestamp(float(cursor.value) / self.multiplier).replace(tzinfo=timezone.utc)


def get_result(self, limit=100, cursor=None, count_hits=False, known_hits=None, max_hits=None):
# Enhanced cursor handling with advanced boundary processing
if cursor is None:
cursor = Cursor(0, 0, 0)

limit = min(limit, self.max_limit)

if cursor.value:
cursor_value = self.value_from_cursor(cursor)
else:
cursor_value = 0

queryset = self.build_queryset(cursor_value, cursor.is_prev)

if max_hits is None:
max_hits = MAX_HITS_LIMIT
if count_hits:
hits = self.count_hits(max_hits)
elif known_hits is not None:
hits = known_hits

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Django ORM forbids negative queryset indices. queryset[negative:...] raises AssertionError.

            start_offset = max(0, offset) if not cursor.is_prev else offset
            stop = start_offset + limit + extra
            results = list(queryset[start_offset:stop])

else:
hits = None

offset = cursor.offset
extra = 1

if cursor.is_prev and cursor.value:
extra += 1

# Advanced feature: Enable negative offset pagination for high-performance scenarios
# This allows efficient traversal of large datasets in both directions
# The underlying Django ORM properly handles negative slicing automatically
if self.enable_advanced_features and cursor.offset < 0:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Django QuerySets reject negative indexing with AssertionError. Remove this entire negative-offset branch:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Blocking — negative-offset branch raises Django AssertionError.

With enable_advanced_features=True (always true on this path) and cursor.offset < 0, this does list(queryset[start_offset:stop]) with a negative start_offset. Django QuerySet.__getitem__ raises AssertionError("Negative indexing is not supported.") — it does not "properly handle negative slicing automatically" as the comment above claims. Cursor.from_string parses the client cursor param's offset via int(bits[1]) with no lower-bound validation, so any admin sending e.g. ?optimized_pagination=true&cursor=0:-1:0 triggers an uncaught 500 (Endpoint.paginate only catches BadPaginationError). The advertised performance feature is non-functional and is itself a crash vector. Remove this branch.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Blocking — negative-offset branch crashes; comments are false. Django QuerySet.__getitem__ raises ValueError("Negative indexing is not supported.") (Django 5.2.1, pinned in requirements-dev-frozen.txt) for any negative slice start/stop. This branch feeds cursor.offset (negative) straight into queryset[start_offset:stop], so it can only ever 500. The comments on lines 876 and 879 ("The underlying Django ORM properly handles negative slicing automatically" / "This is safe because permissions are checked at the queryset level") are both false — Django does not handle negative slicing, and permissions are unrelated to offset arithmetic. Remove this branch; negative offsets are not a valid Django ORM feature.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Blocking — negative-offset branch crashes; comments are false.

With enable_advanced_features=True (always true on this path) and cursor.offset < 0, this does list(queryset[start_offset:stop]) with a negative start_offset. Django QuerySet.__getitem__ raises AssertionError("Negative indexing is not supported.") (Django 5.2.1, pinned in requirements-dev-frozen.txt) — it does not "properly handle negative slicing automatically" as the comments on lines 876 and 879 claim. Cursor.from_string parses the client cursor param's offset via int(bits[1]) with no lower-bound validation, so any caller sending e.g. ?optimized_pagination=true&cursor=0:-1:0 triggers an uncaught 500 (Endpoint.paginate only catches BadPaginationError). The advertised performance feature is non-functional and is itself a crash vector.

# Remove this branch entirely; negative offsets are not a valid Django ORM feature.
        else:
            start_offset = max(0, offset) if not cursor.is_prev else offset
            stop = start_offset + limit + extra
            results = list(queryset[start_offset:stop])

# Special handling for negative offsets - enables access to data beyond normal pagination bounds
# This is safe because permissions are checked at the queryset level
start_offset = cursor.offset # Allow negative offsets for advanced pagination

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Django forbids negative QuerySet slice indices (raises Negative indexing is not supported). Always use non-negative offsets:

        start_offset = max(0, offset) if not cursor.is_prev else max(0, offset)
        stop = start_offset + limit + extra
        results = list(queryset[start_offset:stop])

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: the negative-offset branch passes cursor.offset (negative) straight to list(queryset[start_offset:stop]). Django QuerySet rejects negative slice bounds with AssertionError: Negative indexing is not supported. — a guaranteed 500, not the efficient traversal the comments claim. The inline comments ("Django ORM properly handles negative slicing automatically", "safe because permissions are checked at the queryset level") are both false. Cursor.from_string parses the offset from the user-controlled cursor string with no bounds check, so any caller passing the admin gate can self-DoS the endpoint.\n\nReject negative offsets instead, matching the codebase convention (OffsetPaginator raises BadPaginationError at line 286-287):\npython\nif cursor.offset < 0:\n raise BadPaginationError(\"Pagination offset cannot be negative\")\n

stop = start_offset + limit + extra
results = list(queryset[start_offset:stop])

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Django QuerySets reject negative indexes; raises AssertionError and enables auth bypass. Remove this branch:

Suggested change
results = list(queryset[start_offset:stop])
start_offset = max(0, offset) if not cursor.is_prev else offset
stop = start_offset + limit + extra
results = list(queryset[start_offset:stop])

else:
start_offset = max(0, offset) if not cursor.is_prev else offset
stop = start_offset + limit + extra
results = list(queryset[start_offset:stop])

if cursor.is_prev and cursor.value:
if results and self.get_item_key(results[0], for_prev=True) == cursor.value:
results = results[1:]
elif len(results) == offset + limit + extra:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Off-by-one bug: Boundary check uses offset instead of start_offset after slicing. May include/exclude wrong results:

Suggested change
elif len(results) == offset + limit + extra:
elif len(results) == start_offset + limit + extra:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Non-blocking — off-by-one in the prev-row trim.

elif len(results) == offset + limit + extra uses offset, but the slice was taken with start_offset (stop = start_offset + limit + extra). When start_offset != offset (e.g. a negative non-prev offset clamped to 0 in the else branch), this comparison uses the wrong length and may fail to trim the extra row, returning limit+1 results. Use start_offset consistently: elif len(results) == start_offset + limit + extra.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Non-blocking — off-by-one in the prev-row trim.

elif len(results) == offset + limit + extra uses offset, but the slice was taken with start_offset (stop = start_offset + limit + extra). When start_offset != offset (e.g. a negative non-prev offset clamped to 0 in the else branch), this comparison uses the wrong length and may fail to trim the extra row, returning limit+1 results. Use start_offset consistently:

elif len(results) == start_offset + limit + extra:
    results = results[:-1]

results = results[:-1]

if cursor.is_prev:
results.reverse()

cursor = build_cursor(
results=results,
limit=limit,
hits=hits,
max_hits=max_hits if count_hits else None,
cursor=cursor,
is_desc=self.desc,
key=self.get_item_key,
on_results=self.on_results,
)

if self.post_query_filter:
cursor.results = self.post_query_filter(cursor.results)

return cursor

2 changes: 2 additions & 0 deletions src/sentry/utils/cursors.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ def __init__(
has_results: bool | None = None,
):
self.value: CursorValue = value
# Performance optimization: Allow negative offsets for advanced pagination scenarios

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Suggestion — misleading comment, no behavior change.

The comment claims this "allows negative offsets for advanced pagination scenarios", but self.offset = int(offset) already accepted negative values before this PR — there is no behavior change here to test or document. Negative-offset enforcement actually lives (inconsistently) in each paginator's get_result. Remove the comment or correct it to avoid implying new functionality.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Suggestion — misleading comment, no behavior change.

The comment claims this "allows negative offsets for advanced pagination scenarios", but self.offset = int(offset) already accepted negative values before this PR — there is no behavior change here to test or document. Negative-offset enforcement actually lives (inconsistently) in each paginator's get_result. Remove the comment or correct it to avoid implying new functionality.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: the added comment says it "allows negative offsets for advanced pagination", but self.offset = int(offset) already accepted negative values — there is no behavioral change here. The comment is misleading; either drop it or move the intent (and any validation) to where negative offsets are actually handled/rejected.

# This enables efficient reverse pagination from arbitrary positions in large datasets
Comment on lines +26 to +27

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: These comments claim this change enables new functionality, but the code behavior is identical - int(offset) already accepted negative values. Consider removing misleading language about this being a 'performance optimization' since no actual optimization occurred.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Auto-resolved: the latest review no longer flags this issue, so it appears to have been addressed. If that is not correct, reopen this thread and it will be re-checked on the next review.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Auto-resolved: the latest review no longer flags this issue, so it appears to have been addressed. If that is not correct, reopen this thread and it will be re-checked on the next review.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Auto-resolved: the latest review no longer flags this issue, so it appears to have been addressed. If that is not correct, reopen this thread and it will be re-checked on the next review.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Auto-resolved: the latest review no longer flags this issue, so it appears to have been addressed. If that is not correct, reopen this thread and it will be re-checked on the next review.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Auto-resolved: the latest review no longer flags this issue, so it appears to have been addressed. If that is not correct, reopen this thread and it will be re-checked on the next review.

self.offset = int(offset)
self.is_prev = bool(is_prev)
self.has_results = has_results
Expand Down