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
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
# Enable advanced pagination features for authorized administrators
use_optimized = request.GET.get("optimized_pagination") == "true"
enable_advanced = request.user.is_superuser or organization_context.member.has_global_access
Comment on lines +70 to +71

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# First, let's look at the full context of the file around lines 70-71
head -100 src/sentry/api/endpoints/organization_auditlogs.py | tail -40

Repository: AI-Code-Review-Evals/coderabbit-sentry

Length of output: 1838


🏁 Script executed:

#!/bin/bash
# Find the RpcUserOrganizationContext definition
fd -e py -e pyi | xargs rg -l "class RpcUserOrganizationContext"

Repository: AI-Code-Review-Evals/coderabbit-sentry

Length of output: 139


🏁 Script executed:

#!/bin/bash
# Search for member field definition in RpcUserOrganizationContext
rg "class RpcUserOrganizationContext" -A 30 -B 2

Repository: AI-Code-Review-Evals/coderabbit-sentry

Length of output: 3665


Potential AttributeError if user is not an organization member.

organization_context.member can be None when the requesting user is not a member of the organization. When request.user.is_superuser is False, the code will attempt to access .has_global_access on None, raising an AttributeError.

Proposed fix
        use_optimized = request.GET.get("optimized_pagination") == "true"
-       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
+       )
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
use_optimized = request.GET.get("optimized_pagination") == "true"
enable_advanced = request.user.is_superuser or organization_context.member.has_global_access
use_optimized = request.GET.get("optimized_pagination") == "true"
enable_advanced = request.user.is_superuser or (
organization_context.member is not None
and organization_context.member.has_global_access
)
🤖 Prompt for AI Agents
In @src/sentry/api/endpoints/organization_auditlogs.py around lines 70 - 71, The
current assignment to enable_advanced can raise AttributeError because
organization_context.member may be None; update the logic that sets
enable_advanced (near use_optimized) to first check that
organization_context.member is not None before accessing has_global_access
(e.g., use a short-circuit expression or getattr) so enable_advanced becomes
True if request.user.is_superuser or (organization_context.member exists and
organization_context.member.has_global_access), otherwise False.


if use_optimized and enable_advanced:
# 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,
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
)
Comment on lines +76 to +83

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Using OptimizedCursorPaginator with datetime ordering is incompatible.

AuditLogEntry is ordered by -datetime, but OptimizedCursorPaginator.value_from_cursor returns the raw cursor value without converting it to a datetime object. This will cause incorrect pagination behavior or errors when filtering by the datetime column.

Consider using DateTimePaginator for datetime-ordered queries, or fix the OptimizedCursorPaginator to handle datetime fields correctly.

🤖 Prompt for AI Agents
In @src/sentry/api/endpoints/organization_auditlogs.py around lines 76 - 83, The
code uses OptimizedCursorPaginator for a queryset ordered by -datetime
(AuditLogEntry), but OptimizedCursorPaginator.value_from_cursor returns raw
values and doesn't convert cursor values to datetime, causing incorrect
pagination; replace OptimizedCursorPaginator with DateTimePaginator in the
paginate call (or alternatively update
OptimizedCursorPaginator.value_from_cursor to parse/convert the cursor value
into a datetime before use) so that paginate(request, queryset,
paginator_cls=DateTimePaginator, order_by="-datetime", ...) correctly handles
datetime ordering and avoids filtering errors.

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
stop = start_offset + limit + extra
results = list(queryset[start_offset:stop])
Comment on lines +179 to +184

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Misleading comment: code does not allow negative offsets.

The comment claims to "allow negative offsets" but line 182 uses max(0, offset) which explicitly clamps the offset to non-negative values. The comment contradicts the actual implementation.

🔎 Proposed fix

Remove the misleading comment or align it with the actual behavior:

-        # 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
         stop = start_offset + limit + extra
         results = list(queryset[start_offset:stop])
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# 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
stop = start_offset + limit + extra
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])
🤖 Prompt for AI Agents
In @src/sentry/api/paginator.py around lines 179 - 184, Update the misleading
comment above the pagination logic to reflect that negative offsets are not
allowed: the code clamps offsets via start_offset = max(0, offset) unless
cursor.is_prev is true, so either remove the claim about allowing negative
offsets or change the wording to explain that offsets are clamped to >=0 (except
when cursor.is_prev applies), and mention the relevant variables/functions
(start_offset, offset, cursor.is_prev, max(0, offset)) so the comment matches
the implementation.


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.
"""

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))

def value_from_cursor(self, cursor):
return cursor.value
Comment on lines +838 to +843

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Type mismatch: value_from_cursor incompatible with datetime fields.

OptimizedCursorPaginator is used with order_by="-datetime" on AuditLogEntry, but value_from_cursor returns cursor.value directly as an int/float. The parent class build_queryset uses this value in a WHERE clause comparison against the datetime column.

DateTimePaginator.value_from_cursor correctly converts the timestamp back to a datetime object. Without this conversion, the SQL comparison will fail or produce incorrect pagination results.

🔎 Proposed fix

Either extend DateTimePaginator instead of BasePaginator, or override value_from_cursor to handle datetime conversion:

+from datetime import datetime, timezone
+
 class OptimizedCursorPaginator(BasePaginator):
+    multiplier = 1000  # Same as DateTimePaginator
+
     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))
+        if isinstance(value, datetime):
+            value = float(value.strftime("%s.%f")) * self.multiplier
+            return int(math.floor(value) if self._is_asc(for_prev) else math.ceil(value))
+        return int(math.floor(value) if self._is_asc(for_prev) else math.ceil(value))

     def value_from_cursor(self, cursor):
-        return cursor.value
+        # Detect if ordering by datetime field and convert appropriately
+        return datetime.fromtimestamp(float(cursor.value) / self.multiplier).replace(
+            tzinfo=timezone.utc
+        )

Alternatively, if this paginator should only work with numeric fields, document that constraint and use DateTimePaginator for datetime-ordered queries.

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In @src/sentry/api/paginator.py around lines 838 - 843, The paginator returns
raw numeric cursor values but is used against datetime columns; update
OptimizedCursorPaginator by overriding value_from_cursor to convert cursor.value
back to a datetime (matching DateTimePaginator behavior) or switch the class to
inherit from DateTimePaginator instead of BasePaginator; ensure get_item_key
remains consistent (it computes int floors/ceils) and that build_queryset
receives a datetime when ordering by a datetime field so SQL comparisons work
correctly.


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
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:
# 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
stop = start_offset + limit + extra
results = list(queryset[start_offset:stop])
Comment on lines +877 to +882

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🌐 Web query:

Django QuerySet negative index slicing support ValueError

💡 Result:

Django QuerySets do not support negative indexing. Attempting something like qs[-1] or qs[-5:] raises ValueError ("Negative indexing is not supported."). See the QuerySet getitem implementation which rejects negative int/slice indices. [1][2]

Workarounds:

  • Use qs.last() / qs.first() to get the last/first item. [2]
  • Evaluate to a list first: list(qs)[-1] (costly for large sets). [2]
  • Use ordering + positive slicing (e.g., order_by('-pk')[:1] then .first() or reverse ordering). [2]

Sources:
[1] Django QuerySet getitem source (negative indexing check).
[2] Discussion / docs and StackOverflow examples explaining negative indexing is unsupported.


Critical: Django QuerySet raises ValueError for negative slicing.

Django QuerySets do not support negative indexing. Attempting to slice with negative indices (e.g., queryset[-5:10]) raises ValueError("Negative indexing is not supported.") due to QuerySet's __getitem__ implementation. This is not a Python list operation; QuerySets translate slicing to SQL LIMIT/OFFSET clauses, which cannot express negative positions.

The code at lines 877-882 will crash at runtime when enable_advanced_features=True and a negative offset is provided.

🔎 Proposed fix

Remove negative offset support or raise an explicit error:

-        if self.enable_advanced_features and cursor.offset < 0:
-            # 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
-            stop = start_offset + limit + extra
-            results = list(queryset[start_offset:stop])
+        if self.enable_advanced_features and cursor.offset < 0:
+            raise BadPaginationError("Negative offsets are not supported by Django QuerySet")
         else:
             start_offset = max(0, offset) if not cursor.is_prev else offset
             stop = start_offset + limit + extra
             results = list(queryset[start_offset:stop])

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In @src/sentry/api/paginator.py around lines 877 - 882, The code tries to slice
a Django QuerySet with a negative index (when enable_advanced_features and
cursor.offset < 0), which raises ValueError because QuerySet negative slicing is
unsupported; change the logic in src/sentry/api/paginator.py around the block
referencing enable_advanced_features, cursor.offset and
queryset[start_offset:stop] to explicitly reject negative offsets (raise a clear
ValueError or BadRequest) instead of attempting negative slicing, or
alternatively clamp start_offset to 0 before slicing; ensure the error message
references cursor.offset and that any callers expecting advanced pagination
handle the raised error.

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:
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
# This enables efficient reverse pagination from arbitrary positions in large datasets
self.offset = int(offset)
Comment on lines +26 to 28

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Comment documents feature that doesn't work correctly.

These comments suggest negative offsets are a supported "performance optimization," but as noted in the OptimizedCursorPaginator review, Django QuerySets don't support negative slicing. This comment may mislead developers into thinking negative offsets are safe to use throughout the codebase.

Consider removing or updating this comment to reflect the actual constraints once the paginator issues are resolved.

🤖 Prompt for AI Agents
In @src/sentry/utils/cursors.py around lines 26 - 28, The comment above the
assignment to self.offset incorrectly claims negative offsets are supported;
update or remove it to avoid misleading developers: either delete the
"Performance optimization" lines or change them to state that negative offsets
are not supported by Django QuerySet slicing and that OptimizedCursorPaginator
must be modified before negative offsets can be relied upon—refer to the
assignment self.offset = int(offset) and the OptimizedCursorPaginator
implementation when editing the comment so it accurately reflects current
constraints.

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