fix: duplicate results in cl/issue paginated queries. #1595
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors query logic in issue and CL storage implementations to remove redundant database joins and improve code clarity. The changes eliminate unnecessary LEFT JOIN operations on labels and assignees that were causing duplicate rows, simplifying the pagination logic.
Key changes:
- Removed unnecessary LEFT JOIN operations for labels and assignees in list queries
- Simplified pagination logic to directly return total count instead of wrapping in tuples
- Added early-exit optimization for empty CL lists
- Added "debug-print" feature to sea-orm dependency
- Fixed formatting for pgp dependency
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| jupiter/src/storage/issue_storage.rs | Removed redundant joins and DISTINCT clause, simplified pagination return logic |
| jupiter/src/storage/cl_storage.rs | Similar refactoring as issue_storage plus added early-exit for empty results and improved variable naming |
| jupiter/Cargo.toml | Added debug-print feature to sea-orm and fixed pgp formatting |
| .map(|m| (m, num_pages))?; | ||
| let total = paginator.num_items().await?; | ||
| let issues = paginator.fetch_page(page.page - 1).await?; | ||
|
|
There was a problem hiding this comment.
When issues is empty, this code will still execute three unnecessary database queries (lines 61-86) for labels, assignees, and conversations with empty ID lists. Consider adding an early return if issues.is_empty() { return Ok((vec![], 0)); } before line 59, similar to the optimization added in cl_storage.rs at line 86.
| if issues.is_empty() { | |
| return Ok((vec![], total)); | |
| } |
| .filter(cond) | ||
| .distinct(); | ||
| .distinct() | ||
| .order_by_asc(mega_issue::Column::Id); |
There was a problem hiding this comment.
Adding .order_by_asc(mega_issue::Column::Id) before apply_sort() creates a potential issue: if the user specifies a custom sort via apply_sort(), the query will have two ORDER BY clauses (first by ID, then by the user's chosen field). While SeaORM will generate SQL with both ordering criteria, this means results will be sorted by ID first, then by the user's field as a secondary sort, which is likely not the intended behavior. Consider either: (1) adding the ID ordering conditionally only when no custom sort is specified, or (2) documenting this as intentional tie-breaking behavior.
| .filter(cond) | ||
| .distinct(); | ||
| .distinct() | ||
| .order_by_asc(mega_cl::Column::Id); |
There was a problem hiding this comment.
Adding .order_by_asc(mega_cl::Column::Id) before apply_sort() creates a potential issue: if the user specifies a custom sort via apply_sort(), the query will have two ORDER BY clauses (first by ID, then by the user's chosen field). While SeaORM will generate SQL with both ordering criteria, this means results will be sorted by ID first, then by the user's field as a secondary sort, which is likely not the intended behavior. Consider either: (1) adding the ID ordering conditionally only when no custom sort is specified, or (2) documenting this as intentional tie-breaking behavior.
| "sqlx-sqlite", | ||
| "runtime-tokio-rustls", | ||
| "macros", | ||
| "debug-print", |
There was a problem hiding this comment.
The debug-print feature for sea-orm should not be enabled in production builds as it logs all SQL queries, which can impact performance and potentially expose sensitive data in logs. This feature is typically intended for development/debugging only. Consider using a separate dev-dependencies section or Cargo feature flag to enable this only in non-production builds.
42eaf2a
2 participants
No description provided.