-
Notifications
You must be signed in to change notification settings - Fork 0
Enhanced Pagination Performance for High-Volume Audit Logs #15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chainAlign BasePaginator comments with actual offset behavior This block claims we “allow negative offsets” and that the queryset “will handle boundary conditions,” but for non‑prev cursors we now explicitly clamp negatives to zero: start_offset = max(0, offset) if not cursor.is_prev else offsetSo forward pagination no longer passes negative offsets to the ORM (which is good), and prev‑cursor behavior is unchanged. To avoid future confusion (and over‑stating ORM capabilities), I’d suggest tightening the comment to describe what the code actually does: - # 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
+ # For safety, clamp negative offsets to 0 on forward cursors.
+ # Prev cursors retain the raw offset (which is expected to be non-negative).
+ start_offset = max(0, offset) if not cursor.is_prev else offsetThis keeps the functional improvement while documenting realistic expectations of 🌐 Web query: 💡 Result: Django QuerySet slice bounds must be non‑negative — negative indexing/slices are not supported (Django raises "Negative indexing is not supported."). [1][2] Sources:
🏁 Script executed: #!/bin/bash
# Check for OptimizedCursorPaginator class and verify the negative slicing concern
rg -n "class OptimizedCursorPaginator" src/sentry/api/paginator.pyLength of output: 50 🏁 Script executed: #!/bin/bash
# Get the content around lines 821-911 to see the OptimizedCursorPaginator implementation
sed -n '800,920p' src/sentry/api/paginator.py | head -120Length of output: 4208 🏁 Script executed: #!/bin/bash
# Check if there's any negative offset handling in the new code
rg -n -A5 -B5 "enable_advanced_features" src/sentry/api/paginator.pyLength of output: 1343 🏁 Script executed: #!/bin/bash
# Verify the current state of lines 179-184
sed -n '175,190p' src/sentry/api/paginator.pyLength of output: 831 Address misleading comments in BasePaginator and remove unsupported negative slicing in OptimizedCursorPaginator The review comment for BasePaginator is valid. Django QuerySet slice bounds must be non‑negative — negative indexing/slices are not supported (Django raises "Negative indexing is not supported.") The current comments mislead by claiming the ORM "will handle boundary conditions," when the code actually clamps negative offsets to 0 for forward cursors. Additionally, there is a critical issue in OptimizedCursorPaginator (lines 877–882): the code attempts to pass negative offsets directly to Django slicing when Required fixes:
🤖 Prompt for AI Agents |
||
|
|
||
| if cursor.is_prev and cursor.value: | ||
| # If the first result is equal to the cursor_value then it's safe to filter | ||
|
|
@@ -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 | ||
|
|
||
| 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]) | ||
| 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 | ||
|
Comment on lines
+821
to
+911
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chainNegative slicing on Django QuerySets is unsafe; avoid using negative offsets directly The advanced path in if self.enable_advanced_features and cursor.offset < 0:
start_offset = cursor.offset # may be negative
stop = start_offset + limit + extra
results = list(queryset[start_offset:stop])On Django, QuerySet slicing does not support negative indices; using a negative start or stop raises an error rather than “handling boundary conditions.” This means:
Given the risk, it’s safer to avoid negative indices entirely in this paginator until a proven design is in place. A minimal, safe fix is to drop the negative-slice branch and delegate to the existing, well-tested logic in class OptimizedCursorPaginator(BasePaginator):
@@
- 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])
- 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
+ def get_result(self, limit=100, cursor=None, count_hits=False, known_hits=None, max_hits=None):
+ """
+ For now, delegate to BasePaginator.get_result so behavior matches the
+ existing cursor paginator. The `enable_advanced_features` flag is
+ accepted but not used until a safe, tested optimized path is implemented.
+ """
+ return super().get_result(
+ limit=limit,
+ cursor=cursor,
+ count_hits=count_hits,
+ known_hits=known_hits,
+ max_hits=max_hits,
+ )This preserves public API (including the 🌐 Web query: 💡 Result: It raises an error. Django QuerySet slicing does not support negative indices — a slice with a negative start or stop (e.g. MyModel.objects.all()[-10:10]) raises ValueError("Negative indexing is not supported."). [1][2] Sources:
Critical issue confirmed: negative QuerySet slicing will crash The web search confirms Django QuerySets raise The misleading comments ("Django ORM properly handles negative slicing automatically") compound the risk by misdirecting future maintainers. The suggested fix is appropriate: remove the unsafe negative-slice branch and delegate to 🤖 Prompt for AI Agents |
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
Optimized paginator wiring looks good; consider guarding
organization_context.memberThe conditional swap to
OptimizedCursorPaginatoris well-scoped: it’s opt-in viaoptimized_pagination=trueand further gated to admins/global-access users, while preserving the existing DateTime-based path for everyone else.One thing to double‑check:
organization_context.memberis assumed to be non‑null when the user isn’t a superuser:If there are legitimate flows where an authenticated user can reach this endpoint with
member is None(e.g., service accounts or partial org contexts), this will raiseAttributeError. If that’s possible, consider making this more defensive:Otherwise, it’s worth adding a brief comment that
memberis guaranteed to be non-null here byOrganizationAuditPermissionto document the assumption.Also applies to: 68-91
🏁 Script executed:
Length of output: 530
Length of output: 2154
🏁 Script executed:
Length of output: 2356
🏁 Script executed:
Length of output: 778
🏁 Script executed:
Length of output: 1635
Guard
organization_context.memberagainst None to prevent AttributeErrorThe concern is valid.
RpcUserOrganizationContext.memberis explicitly typed asRpcOrganizationMember | None, with the model's docstring stating: "member can be None when the given user_id does not have membership with the given organization."At line 70, the code accesses
organization_context.member.has_global_accesswithout a null check:OrganizationAuditPermissiondoesn't enforce thatmembermust be non-null—it validates based on scopes and superuser status. A non-superuser with sufficient scopes but no organization membership can reach this endpoint and trigger anAttributeError.Apply the defensive fix:
Same issue exists at lines 68–91.
🤖 Prompt for AI Agents