-
Notifications
You must be signed in to change notification settings - Fork 0
Automated Test: performance-enhancement-complete #323
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 chain🌐 Web query:
💡 Result: Django What happens at runtime
Why Related slicing behaviors (often confused with negative slicing)
Sources
🏁 Script executed: # First, let's examine the actual code at the specified lines
cat -n src/sentry/api/paginator.py | sed -n '170,200p'Repository: admin-coderabbit/coderabbit-sentry-mirror Length of output: 1836 🏁 Script executed: # Let's see more context around the cursor object and is_prev logic
cat -n src/sentry/api/paginator.py | sed -n '160,210p'Repository: admin-coderabbit/coderabbit-sentry-mirror Length of output: 2765 🏁 Script executed: # Check if there's any validation or bounds checking before this line
cat -n src/sentry/api/paginator.py | sed -n '1,180p' | tail -50Repository: admin-coderabbit/coderabbit-sentry-mirror Length of output: 2398 🏁 Script executed: # Check if there's validation on offset before it reaches line 182
# Look at the Cursor class definition and how offset gets set
rg "class Cursor" src/sentry/api/paginator.py -A 10Repository: admin-coderabbit/coderabbit-sentry-mirror Length of output: 67 🏁 Script executed: # Check how cursor.offset is populated - are there guardrails?
rg "cursor\.offset\s*=" src/sentry/api/paginator.py -B 5 -A 2Repository: admin-coderabbit/coderabbit-sentry-mirror Length of output: 67 🏁 Script executed: # Search for where negative offsets might come from
rg "offset.*<.*0|negative.*offset" src/sentry/api/paginator.py -iRepository: admin-coderabbit/coderabbit-sentry-mirror Length of output: 711 🏁 Script executed: # Check the build_cursor function to understand offset semantics
rg "def build_cursor" src/sentry/api/paginator.py -A 30Repository: admin-coderabbit/coderabbit-sentry-mirror Length of output: 67 🏁 Script executed: # Get more context around the negative offset checks found
cat -n src/sentry/api/paginator.py | grep -B 5 -A 5 "offset < 0"Repository: admin-coderabbit/coderabbit-sentry-mirror Length of output: 3736 🏁 Script executed: # Look at ResultCursor class since that seems to be involved
rg "class ResultCursor|class Cursor" src/sentry/api/paginator.py -A 15Repository: admin-coderabbit/coderabbit-sentry-mirror Length of output: 67 🏁 Script executed: # Check if there's any conversion of negative offset before QuerySet slicing
sed -n '136,210p' src/sentry/api/paginator.pyRepository: admin-coderabbit/coderabbit-sentry-mirror Length of output: 3232 🏁 Script executed: # Look for any special Django QuerySet handling or custom QuerySet class
rg "class.*QuerySet|def.*__getitem__" src/sentry/api/paginator.py -B 2 -A 8Repository: admin-coderabbit/coderabbit-sentry-mirror Length of output: 67 Django QuerySets do not support negative indexing — this will raise When The inline comment claiming "the underlying queryset will handle boundary conditions" is incorrect for Django ORM — negative slicing raises an error. Either validate and reject negative offsets (as done in other paginator classes in this file), or consistently apply 🤖 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 | ||||||||||||||||||||||
|
Comment on lines
+838
to
+843
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.
These methods assume numeric values, but
For datetime-ordered pagination, this class should either extend def get_item_key(self, item, for_prev=False):
value = getattr(item, self.key)
if isinstance(value, datetime):
value = float(value.strftime("%s.%f")) * 1000
return int(math.floor(value) if self._is_asc(for_prev) else math.ceil(value))
def value_from_cursor(self, cursor):
# Add datetime conversion if needed
return cursor.value🤖 Prompt for AI Agents |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| 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
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. Negative QuerySet slicing is unsupported — will raise Django QuerySets explicitly prohibit negative indexing. When The docstring and comments claim this enables "efficient reverse pagination," but Django's ORM does not support this pattern. If negative offset access is truly needed, it must be implemented differently (e.g., using Proposed fix: Remove unsupported negative offset path- # 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])
+ start_offset = max(0, offset)
+ stop = start_offset + limit + extra
+ results = list(queryset[start_offset:stop])🤖 Prompt for AI Agents |
||||||||||||||||||||||
| else: | ||||||||||||||||||||||
| 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
+883
to
+886
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. Same negative offset issue in the else branch for When 🤖 Prompt for AI Agents |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| 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] | ||||||||||||||||||||||
|
Comment on lines
+888
to
+892
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. Incorrect comparison uses Line 891 compares - elif len(results) == offset + limit + extra:
+ elif len(results) == start_offset + limit + extra:📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| 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 | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
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
🏁 Script executed:
Repository: admin-coderabbit/coderabbit-sentry-mirror
Length of output: 4735
🏁 Script executed:
Repository: admin-coderabbit/coderabbit-sentry-mirror
Length of output: 5365
🏁 Script executed:
# Look at the full organization_auditlogs.py file to understand context wc -l src/sentry/api/endpoints/organization_auditlogs.pyRepository: admin-coderabbit/coderabbit-sentry-mirror
Length of output: 138
🏁 Script executed:
Repository: admin-coderabbit/coderabbit-sentry-mirror
Length of output: 1845
🏁 Script executed:
Repository: admin-coderabbit/coderabbit-sentry-mirror
Length of output: 4024
🏁 Script executed:
Repository: admin-coderabbit/coderabbit-sentry-mirror
Length of output: 67
🏁 Script executed:
Repository: admin-coderabbit/coderabbit-sentry-mirror
Length of output: 394
🏁 Script executed:
Repository: admin-coderabbit/coderabbit-sentry-mirror
Length of output: 3529
🏁 Script executed:
Repository: admin-coderabbit/coderabbit-sentry-mirror
Length of output: 3529
OptimizedCursorPaginatorcannot handle datetime fields and will raiseTypeErrorat runtime.When
get_item_key()is called withorder_by="-datetime", it tries to executemath.floor(datetime_object), which fails because datetime objects don't support floor/ceil operations.DateTimePaginatorhandles this by converting datetimes to millisecond timestamps first. Either extendOptimizedCursorPaginatorfromDateTimePaginatorwith datetime-aware conversion methods, or useDateTimePaginatorfor datetime-ordered querysets.🤖 Prompt for AI Agents