fixed offset - #1943
Conversation
Signed-off-by: sougata-progress <sougatab@progress.com>
There was a problem hiding this comment.
Pull Request Overview
This PR refactors pagination logic to use direct offset values instead of page-based pagination. The change improves clarity by passing explicit offsets to database queries rather than calculating them from page numbers in multiple locations.
Key Changes:
- Replaced
pagefield withoffsetfield in theListPackagesstruct - Modified pagination calculation to use direct offset values instead of page-based calculations
- Refactored database query logic to compute total counts separately from paginated results
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| components/builder-db/src/models/package.rs | Updated ListPackages struct to use offset instead of page; refactored query logic to calculate total counts separately and apply pagination using direct offset values |
| components/builder-api/src/server/resources/pkgs.rs | Modified do_get_packages to pass offset directly instead of calculating from page numbers; simplified postprocessing logic |
| components/builder-api/src/server/resources/origins.rs | Updated list_unique_packages to calculate offset from page/per_page before passing to ListPackages |
| paginated_rows = paginated_rows.into_iter() | ||
| .unique_by(|p| (p.ident.clone(), p.origin.clone())) | ||
| .collect(); |
There was a problem hiding this comment.
The deduplication using unique_by is performed after pagination, which may result in fewer items than the requested limit. This can lead to inconsistent pagination behavior where some pages have fewer items than expected. Consider applying deduplication before pagination, or adjusting the limit to account for potential duplicates.
| let mut query = packages_with_channel_platform::table | ||
| .filter(packages_with_channel_platform::origin.eq(origin_str)) | ||
| let mut base_query = packages_with_channel_platform::table | ||
| .filter(packages_with_channel_platform::origin.eq(origin_str.clone())) |
There was a problem hiding this comment.
The origin_str is being cloned multiple times throughout the method (lines 626, 654). Since it's already a String, consider cloning it once at the beginning and reusing the reference, or avoiding the clone entirely by using a reference where possible.
| .filter(packages_with_channel_platform::origin.eq(origin_str.clone())) | |
| .filter(packages_with_channel_platform::origin.eq(&origin_str)) |
| if !pl.ident.name.is_empty() { | ||
| query = query.filter(packages_with_channel_platform::name.eq(name_str)) | ||
| base_query = | ||
| base_query.filter(packages_with_channel_platform::name.eq(name_str.clone())) |
There was a problem hiding this comment.
The name_str variable is cloned twice (lines 633 and 659). Consider reusing the reference or restructuring to avoid unnecessary clones.
| .filter(packages_with_channel_platform::ident_array.contains(parts.clone())) | ||
| .filter(packages_with_channel_platform::visibility.eq_any(visibility.clone())) | ||
| .select(count_star()) | ||
| .first(conn)?; |
There was a problem hiding this comment.
Using .first() with count_star() will fail if no records are found, returning an error instead of a count of 0. Use .get_result() or handle the case where no records exist to return a count of 0.
| .first(conn)?; | |
| .get_result(conn)?; |
112e736 to
79754c4
Compare
Signed-off-by: sougata-progress <sougatab@progress.com>
| if !pl.ident.name.is_empty() { | ||
| Histogram::PackageListOriginOnlyCallTime.set(duration_millis as f64); | ||
| // Apply deduplication | ||
| pkgs = pkgs.into_iter().unique().collect(); |
There was a problem hiding this comment.
The two branches (limit < 0 and limit >= 0) have inconsistent deduplication methods. The first branch uses .unique() while the second uses .unique_by(|p| (p.ident.clone(), p.origin.clone())). This inconsistency could lead to different results between paginated and non-paginated queries. Use the same deduplication key for both branches.
| pkgs = pkgs.into_iter().unique().collect(); | |
| pkgs = pkgs.into_iter().unique_by(|p| (p.ident.clone(), p.origin.clone())).collect(); |
| // First get ALL records and deduplicate to get accurate counts and consistent | ||
| // pagination | ||
| let query_all = base_query.order(packages_with_channel_platform::ident.desc()); | ||
| let all_rows: Vec<PackageWithChannelPlatform> = query_all.load(conn)?; |
There was a problem hiding this comment.
Loading all records into memory for pagination defeats the purpose of database-level pagination. This approach will not scale well with large datasets and could cause memory issues. Consider implementing cursor-based pagination or using SQL window functions to maintain deduplication while paginating at the database level.
| let start = offset_val as usize; | ||
| let end = if limit < 0 { | ||
| unique_by_name.len() | ||
| } else { | ||
| (start + limit as usize).min(unique_by_name.len()) | ||
| }; |
There was a problem hiding this comment.
[nitpick] The logic for handling limit < 0 is inconsistent with the list() method. In list(), negative limits are checked at line 640, but here the negative check is embedded within the end calculation. Consider extracting this into an early return pattern like in list() for consistency.
| let start = offset_val as usize; | |
| let end = if limit < 0 { | |
| unique_by_name.len() | |
| } else { | |
| (start + limit as usize).min(unique_by_name.len()) | |
| }; | |
| if limit < 0 { | |
| // Early return: negative limit means return all results | |
| let results = unique_by_name.clone(); | |
| let duration_millis = start_time.elapsed().as_millis(); | |
| trace!("DBCall package::list_distinct_for_origin time: {} ms", | |
| duration_millis); | |
| Histogram::DbCallTime.set(duration_millis as f64); | |
| Histogram::PackageListDistinctForOriginCallTime.set(duration_millis as f64); | |
| return Ok((results, total_count)); | |
| } | |
| let start = offset_val as usize; | |
| let end = (start + limit as usize).min(unique_by_name.len()); |
| let body = | ||
| helpers::package_results_json(packages, count as isize, start as isize, stop as isize); |
There was a problem hiding this comment.
[nitpick] Multiple type conversions between i64, isize, and usize create confusion and potential overflow risks. The start variable is i64, then converted to isize here, while earlier conversions also occur. Consider standardizing on a single type throughout the pagination logic to reduce conversion overhead and improve clarity.
|
Obvious fix; these changes are the result of automation not creative thinking.
No description provided.