Sentry / PR #26

Enhanced Pagination Performance for High-Volume Audit Logs

ai-code-review-evaluation/sentry-greptile#1
Golden recall: 100% Extra findings: 3

Golden Comments (3/3 found)

# Comment Severity Status FriendlyReviewer Detail
#1 Django querysets do not support negative slicing HIGH ✓ Found "The negative-offset branch in get_result does not work because Django QuerySet does not support negative slicing (raises AssertionError), and it is unreachable because cursor offsets are always non-negative — dead code that would crash if ever reached" (paginator.py L877)
#2 When requests are authenticated with API keys or org auth tokens (which have user_id=None), organization_context.member is None; line 71 accesses organization_context.member.has_global_access without checking for None, causing an AttributeError crash when optimized_pagination=true is used HIGH ✓ Found "organization_context.member may be None (per the RPC model); the privilege gate dereferences it without a null check, causing AttributeError for non-superuser callers without a membership, resulting in a 500" (organization_auditlogs.py L71)
#3 get_item_key assumes a numeric key, but the paginator is used with order_by=-datetime in the audit logs endpoint; calling math.floor/math.ceil on a datetime will raise a TypeError HIGH ✓ Found "OptimizedCursorPaginator cannot paginate datetime keys: get_item_key calls math.floor on a datetime object (raises TypeError) and value_from_cursor returns an int that build_queryset compares against a datetime column (PostgreSQL operator error). Every request using the optimized path 500s" (paginator.py L838)

Supplementary Findings (3 findings)

Finding File Severity Legitimate?
Cursor encoding incompatible with the DateTimePaginator path (drops sub-second precision, skips the multiplier=1000 conversion) paginator.py:842 HIGH ✓ Breaks backward compatibility: cursors emitted by one path cannot be consumed by the other
optimized_pagination flag unvalidated and client-triggerable via the query string organization_auditlogs.py:70 HIGH ✓ Any caller can trigger a reliable 500; partly a consequence of the broken paginator, but the unvalidated flag is a real 500 path
No-op change to the shared BasePaginator.get_result touching ~128 dependents, with a misleading comment paginator.py:182 LOW ✓ Borderline: functionally a no-op, but needless churn and a misleading comment on shared code

Honest Analysis

FriendlyReviewer found all 3 golden bugs of this PR — recall 100%. The Martian Code dataset's golden reference for this PR contains 3 comments, and all of them are found, each with a precise remediation. The dataset rates all three goldens HIGH, and FriendlyReviewer flagged each as high (🔴), noting that each guarantees a 500 on the affected path.

On the negative-slicing golden (#1), FriendlyReviewer's candidate is actually stronger than the reference comment: it establishes not only that Django QuerySets reject negative slices (AssertionError), but that the branch is unreachable because cursor offsets are always non-negative — dead code that would crash if ever hit. On the datetime golden (#3), the candidate bundles the connected value_from_cursor defect (an int compared against a PostgreSQL datetime column) into the same finding — same function, same root cause, so the match is direct and the diagnosis is complete.

The three supplementary findings are all defensible. The cursor-encoding incompatibility (L842) is a genuine backward-compatibility break that the dataset never credits: consumers toggling the optimized_pagination flag would emit cursors that the other path cannot consume. The unvalidated client-triggerable flag (L70) is real but partly downstream of the broken paginator — the interesting part is that the serializer never validates the flag, so any caller can force the crashing path. The third (L182) is admittedly borderline: the BasePaginator hunk is a functional no-op, and the finding is mostly about churn and a misleading comment on shared code used by ~128 dependents.

Overall this is a clean result: the extras are all outside the reference set, but they are specific, localizable, and legitimate. A review that only found the three goldens would have missed the backward-compat break — the trade-off of the wide net is a small amount of benign noise.

FriendlyReviewer's full review

As posted on the PR — the complete output from FriendlyReviewer.

friendly-reviewer bot commented now

Hi there 👋

🌥️ Tech 🌥️ Feat
5 high, 1 low 1 issue

The optimized pagination implementation for the audit-log endpoint is fundamentally broken and cannot be merged. The new OptimizedCursorPaginator has no datetime handling, which causes a TypeError on the first page and an SQL type error on subsequent pages, guaranteeing a 500 on every request that opts in. It also emits cursor values incompatible with the existing DateTimePaginator path, breaking backward compatibility for consumers toggling the flag. The negative-offset branch is unreachable and would crash with Django's AssertionError if ever hit. The endpoint additionally dereferences a possibly-None member, and the optimized_pagination flag is unvalidated and client-triggerable. A change to the shared BasePaginator slicing is a functional no-op that needlessly touches ~128 dependents. The MR must be reworked (or reverted) and covered by tests before it is considered for merge.

These might need a close look

  • 🔴 src/sentry/api/paginator.py (L838)
    OptimizedCursorPaginator cannot paginate datetime keys: get_item_key calls math.floor on a datetime object (raises TypeError) and value_from_cursor returns an int that build_queryset compares against a datetime column (PostgreSQL operator error). Every request using the optimized path 500s.
  • 🔴 src/sentry/api/paginator.py (L877)
    The negative-offset branch in get_result does not work because Django QuerySet does not support negative slicing (raises AssertionError), and it is unreachable because cursor offsets are always non-negative. It is dead code that would crash if ever reached.
  • 🔴 src/sentry/api/paginator.py (L842)
    Cursor encoding is incompatible with the DateTimePaginator path: the optimized paginator drops sub-second precision and does not apply the multiplier=1000 conversion, so cursors emitted by one path cannot be consumed by the other, breaking backward compatibility for consumers toggling the optimized_pagination flag.
  • 🔴 src/sentry/api/endpoints/organization_auditlogs.py (L71)
    organization_context.member may be None (per the RPC model); the privilege gate dereferences it without a null check, causing AttributeError for non-superuser callers without a membership, resulting in a 500.
  • 🔴 src/sentry/api/endpoints/organization_auditlogs.py (L70)
    The optimized_pagination flag is read directly from the query string and not validated by the serializer, so any caller passing ?optimized_pagination=true triggers the broken paginator path and a reliable 500. It should be allowlisted and validated, or the paginator fixed and tested.
Small things (take or leave)
  • 🔵 src/sentry/api/paginator.py (L182)
    The change to BasePaginator.get_result is a functional no-op because cursor offsets are always >=0, yet it modifies shared code used by ~128 dependents and adds a misleading comment. Revert this hunk.

✅ Feature-level checklist

Not there yet

  • ❌ Enhanced Pagination Performance for High-Volume Audit Logs
    The implementation fails to provide the promised performance enhancement; instead it introduces guaranteed crashes (TypeError/SQL type errors) on the audit-log endpoint, an incompatible cursor format, and additional client-triggerable 500 paths. The requirement is not met and the change must be reworked or reverted.