perf(api): add cursor pagination to api_keys/webhook_subscriptions/playbooks list endpoints - #1589
Conversation
…aybooks list endpoints (#1142) GET /v1/api_keys, GET /v1/webhook_subscriptions, and GET /v1/playbooks previously returned every row for the tenant with no pagination at all. Add a *_cursor StorageBackend method for each, following the existing list_action_receipts/list_action_receipts_cursor precedent: additive new methods alongside the originals, so no existing caller breaks (including grpc.rs's list_playbooks call, which keeps using the old method). Routes opt in via the established parse_cursor/paginated_response convention (?limit=&offset=&cursor=, X-Next-Cursor response header). Postgres has no equivalent to SQLite's implicit rowid, so a new migration adds an explicit rowid BIGSERIAL UNIQUE to the three tables, mirroring action_receipts/decisions/soc_alerts/soc_incidents which already had it for the same reason. list_agents, list_mcp_servers, list_approvals (offset-only) and list_detection_rules, list_policies, list_policy_audit_log, list_policy_templates (no pagination) remain for a follow-up; the policy.rs-owned endpoints in particular were left untouched since that file had unrelated concurrent edits in flight.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughAdds cursor-based pagination to three list endpoints ( ChangesCursor Pagination for List Endpoints
Sequence Diagram(s)sequenceDiagram
participant Client
participant RouteHandler as list_* Handler
participant Storage as SqlDbStorage
participant DB as SQLite/Postgres
Client->>RouteHandler: GET /v1/api_keys?limit=10&cursor=<token>
RouteHandler->>RouteHandler: parse_pagination, decode_cursor
RouteHandler->>Storage: list_api_keys_cursor(tenant_id, limit, offset, cursor)
Storage->>DB: SELECT ... WHERE rowid < cursor LIMIT limit+1
DB-->>Storage: up to limit+1 rows
Storage->>Storage: paginate_rows → (page, next_cursor)
Storage-->>RouteHandler: (Vec<ApiKeyRecord>, Option<i64>)
RouteHandler-->>Client: JSON body + X-Next-Cursor header (if next_cursor present)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces cursor-based pagination for the api_keys, webhook_subscriptions, and playbooks list endpoints, adding corresponding database queries, a Postgres migration for rowid columns, and route handler updates. The reviewer feedback suggests reducing database query duplication by extracting common logic into a macro, refactoring the list_playbooks route to use the standard parse_cursor helper for consistency, and moving a webhook-related integration test to the correct file.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| match pool { | ||
| DbPool::Sqlite(p) => { | ||
| let rows = sqlx::query(query) | ||
| .bind(tenant_id) | ||
| .bind(cursor) | ||
| .bind(cursor) | ||
| .bind(limit + 1) | ||
| .bind(if cursor.is_some() { 0 } else { offset }) | ||
| .fetch_all(p) | ||
| .await?; | ||
| super::paginate_rows(rows, limit) | ||
| } | ||
| #[cfg(feature = "postgres")] | ||
| DbPool::Postgres(p) => { | ||
| let pg_sql = crate::db::to_postgres_sql(query); | ||
| let rows = sqlx::query(&pg_sql) | ||
| .bind(tenant_id) | ||
| .bind(cursor) | ||
| .bind(cursor) | ||
| .bind(limit + 1) | ||
| .bind(if cursor.is_some() { 0 } else { offset }) | ||
| .fetch_all(p) | ||
| .await?; | ||
| super::paginate_rows(rows, limit) | ||
| } | ||
| } |
There was a problem hiding this comment.
There's significant code duplication across list_playbooks_cursor, list_api_keys_cursor, and list_webhook_subscriptions_cursor. The match pool { ... } block is nearly identical in all three functions.
To improve maintainability and reduce redundancy, consider extracting this common logic into a new macro in lib/storage/src/db/mod.rs, similar to the existing fetch_all_as! macro. This would centralize the logic for handling both Sqlite and Postgres database pools for paginated queries.
For example, you could create a fetch_all_for_pagination! macro and then simplify the cursor functions.
| pub async fn list_playbooks( | ||
| State(state): State<Arc<AppState>>, | ||
| TenantId(tenant_id): TenantId, | ||
| axum::extract::RawQuery(raw_query): axum::extract::RawQuery, | ||
| ) -> Result<impl IntoResponse, StatusError> { | ||
| let records = state | ||
| let (limit, offset) = crate::routes::parse_pagination(raw_query.as_deref()); | ||
| let cursor = match crate::routes::parse_filter(raw_query.as_deref(), "cursor") { | ||
| None => None, | ||
| Some(raw) => Some( | ||
| crate::routes::decode_cursor(&raw) | ||
| .ok_or_else(|| StatusError::bad_request("Invalid cursor"))?, | ||
| ), | ||
| }; | ||
|
|
||
| let (records, next_cursor) = state | ||
| .storage | ||
| .list_playbooks(&tenant_id) | ||
| .list_playbooks_cursor(&tenant_id, limit, offset, cursor) | ||
| .await | ||
| .map_err(|e| StatusError::internal(e.to_string()))?; | ||
| Ok(Json(records)) | ||
|
|
||
| Ok(crate::routes::paginated_response(&records, next_cursor)) | ||
| } |
There was a problem hiding this comment.
The implementation for parsing the cursor in list_playbooks is different from list_api_keys and list_webhook_subscriptions. Here, you are using parse_filter and decode_cursor directly, while the other handlers use a parse_cursor helper function.
For consistency and better maintainability, I recommend refactoring this to use parse_cursor, which seems to be the preferred abstraction. This would likely involve changing the function's return type from Result<impl IntoResponse, StatusError> to impl IntoResponse to match the other handlers and improve error logging consistency.
pub async fn list_playbooks(
State(state): State<Arc<AppState>>,
TenantId(tenant_id): TenantId,
axum::extract::RawQuery(raw_query): axum::extract::RawQuery,
) -> impl IntoResponse {
let (limit, offset) = crate::routes::parse_pagination(raw_query.as_deref());
let cursor = match crate::routes::parse_cursor(raw_query.as_deref()) {
Ok(c) => c,
Err(resp) => return *resp,
};
match state
.storage
.list_playbooks_cursor(&tenant_id, limit, offset, cursor)
.await
{
Ok((records, next_cursor)) => crate::routes::paginated_response(&records, next_cursor),
Err(e) => {
tracing::error!("Failed to list playbooks: {:?}", e);
StatusError::internal("Database error").into_response()
}
}
}| /// #1142: `GET /v1/webhook_subscriptions?limit=1` sets the | ||
| /// `x-next-cursor` response header end-to-end through the full route | ||
| /// handler when more rows exist than the requested page size, and a | ||
| /// follow-up request with `?cursor=<that value>` returns the remaining | ||
| /// row with no further `x-next-cursor`. | ||
| #[tokio::test] | ||
| async fn list_webhook_subscriptions_route_sets_next_cursor_header() { | ||
| let (state, tenant_id, _) = setup_state("webhook_subscriptions_cursor_header").await; | ||
| for i in 0..2 { | ||
| let _ = create_webhook_subscription( | ||
| State(state.clone()), | ||
| TenantId(tenant_id.clone()), | ||
| Json(CreateWebhookSubscriptionRequest { | ||
| url: format!("https://example.com/hook{i}"), | ||
| secret: None, | ||
| event_types: "alert,incident".to_string(), | ||
| min_severity: None, | ||
| format: None, | ||
| }), | ||
| ) | ||
| .await | ||
| .into_response(); | ||
| } | ||
|
|
||
| let response = list_webhook_subscriptions( | ||
| State(state.clone()), | ||
| TenantId(tenant_id.clone()), | ||
| axum::extract::RawQuery(Some("limit=1".to_string())), | ||
| ) | ||
| .await | ||
| .into_response(); | ||
| assert_eq!(response.status(), StatusCode::OK); | ||
| let next_cursor = response | ||
| .headers() | ||
| .get("x-next-cursor") | ||
| .expect("a second row exists beyond the page") | ||
| .to_str() | ||
| .unwrap() | ||
| .to_string(); | ||
| let body = to_bytes(response.into_body(), usize::MAX).await.unwrap(); | ||
| let page: Vec<WebhookSubscriptionRecord> = serde_json::from_slice(&body).unwrap(); | ||
| assert_eq!(page.len(), 1); | ||
|
|
||
| let response2 = list_webhook_subscriptions( | ||
| State(state), | ||
| TenantId(tenant_id), | ||
| axum::extract::RawQuery(Some(format!("limit=1&cursor={next_cursor}"))), | ||
| ) | ||
| .await | ||
| .into_response(); | ||
| assert_eq!(response2.status(), StatusCode::OK); | ||
| assert!(response2.headers().get("x-next-cursor").is_none()); | ||
| let body2 = to_bytes(response2.into_body(), usize::MAX).await.unwrap(); | ||
| let page2: Vec<WebhookSubscriptionRecord> = serde_json::from_slice(&body2).unwrap(); | ||
| assert_eq!(page2.len(), 1); | ||
| } |
There was a problem hiding this comment.
This test, list_webhook_subscriptions_route_sets_next_cursor_header, is for webhook subscriptions but it's located in src/src/routes/tenant.rs. It uses create_webhook_subscription and list_webhook_subscriptions, which are from webhooks.rs.
To improve code organization and make it easier to find relevant tests, this test should be moved to src/src/routes/webhooks.rs alongside other webhook-related tests.
1 participant
Summary
GET /v1/api_keys,GET /v1/webhook_subscriptions,GET /v1/playbookspreviously returned every row for the tenant, unbounded. Each now supports the existing cursor-pagination convention (?limit=&offset=&cursor=,X-Next-Cursorresponse header) already used by/v1/receipts,/v1/decisions,/v1/alerts,/v1/incidents,/v1/audit/events.list_api_keys_cursor/list_webhook_subscriptions_cursor/list_playbooks_cursortoStorageBackendalongside (not replacing) the existing non-cursor methods — mirrors the pre-existinglist_action_receipts/list_action_receipts_cursorpattern, so no existing caller breaks. In particularsrc/src/grpc.rs'slist_playbookscall is untouched.rowid(which the cursor queries order by), solib/storage/migrations_postgres/0002_pagination_rowid_columns.sqladds an explicitrowid BIGSERIAL UNIQUEto the three newly-paginated tables — mirroringaction_receipts/decisions/soc_alerts/soc_incidents, which already needed the same treatment.Why
Progress on #1142. Three list endpoints had zero pagination of any kind (not even offset-based) — every API key, webhook subscription, or playbook a tenant has ever created was returned in one response.
Scope (what's intentionally NOT in this PR)
list_agents,list_mcp_servers,list_approvalsalready have offset-based pagination; upgrading them to cursor-based is left for a follow-up.list_detection_ruleshas no pagination yet, same as the three converted here — left for a follow-up to keep this PR's diff reviewable.list_policies,list_policy_audit_log,list_policy_templates(all insrc/src/routes/policy.rs) are deliberately untouched — that file has unrelated work in flight elsewhere right now, and these endpoints can be picked up once that settles.Test plan
lib/storage/src/db/{webhooks,tenant,playbooks}.rs): pagination across multiple rows withnext_cursorset correctly, plus the established off-by-one boundary regression guard (a page that exactly matches the result-set size must not falsely claim a next page exists).src/src/routes/tenant.rs):list_webhook_subscriptions_route_sets_next_cursor_headerdrives the full HTTP handler end-to-end —?limit=1against 2 rows setsX-Next-Cursor, and following that cursor returns the remaining row with no further header.test_api_key_crud_route,test_webhook_subscription_crud_route,test_playbook_crud_routes) for the newRawQueryparameter — all still green.cargo check --features postgres(compile-only — no live Postgres in this environment, and thepostgresfeature isn't exercised by any CI job today either; same rigor level as the rest of this repo's existing Postgres scaffolding).cargo fmt --manifest-path src/Cargo.toml -- --checkcargo clippy --manifest-path src/Cargo.toml --all-targets -- -D warningscargo test --manifest-path src/Cargo.toml --workspace— full workspace green (815 tests across all crates, 3 ignored, 0 failed)Summary by CodeRabbit
New Features
limit,offset, andcursorquery parameters.Bug Fixes