diff --git a/components/builder-api/src/server/resources/origins.rs b/components/builder-api/src/server/resources/origins.rs index 379c72ec34..d4e489ceab 100644 --- a/components/builder-api/src/server/resources/origins.rs +++ b/components/builder-api/src/server/resources/origins.rs @@ -842,7 +842,7 @@ async fn list_unique_packages(req: HttpRequest, visibility: helpers::visibility_for_optional_session(&req, opt_session_id, &origin), - page: page as i64, + offset: ((page as i64).saturating_sub(1)) * (per_page as i64), limit: per_page as i64, }; match Package::distinct_for_origin(&lpr, &mut conn) { diff --git a/components/builder-api/src/server/resources/pkgs.rs b/components/builder-api/src/server/resources/pkgs.rs index 24fa55e2a8..2245d55f9e 100644 --- a/components/builder-api/src/server/resources/pkgs.rs +++ b/components/builder-api/src/server/resources/pkgs.rs @@ -852,22 +852,19 @@ pub fn postprocess_extended_package_list(_req: &HttpRequest, let start = if pagination.range < 0 { 0 } else { - let (page, per_page) = helpers::extract_pagination_in_pages(pagination); - let safe_page = page.max(1); - safe_page.checked_sub(1) - .and_then(|p| p.checked_mul(per_page)) - .unwrap_or_default() + pagination.range as i64 }; - let pkg_count = packages.len() as isize; + let pkg_count = packages.len() as i64; let stop = match pkg_count { 0 => count, - _ => (start + pkg_count - 1) as i64, + _ => start + pkg_count - 1, }; debug!("postprocessing extended package list, start: {}, stop: {}, total_count: {}", start, stop, count); - let body = helpers::package_results_json(packages, count as isize, start, stop as isize); + let body = + helpers::package_results_json(packages, count as isize, start as isize, stop as isize); let mut response = if count as isize > (stop as isize + 1) { HttpResponse::PartialContent() @@ -891,17 +888,23 @@ fn do_get_packages(req: &HttpRequest, Err(_) => None, }; - let (page, per_page) = helpers::extract_pagination_in_pages(pagination); - let limit = if pagination.range < 0 { -1 } else { per_page }; + let (offset, limit) = if pagination.range < 0 { + // When range is negative, return all packages + (0i64, -1i64) + } else { + // Use range directly as offset + (pagination.range as i64, helpers::PAGINATION_RANGE_MAX as i64) + }; let mut conn = req_state(req).db.get_conn().map_err(Error::DbError)?; - let lpr = ListPackages { ident: BuilderPackageIdent(ident.clone()), - visibility: helpers::visibility_for_optional_session(req, - opt_session_id, - &ident.origin), - page: page as i64, - limit: limit as i64, }; + let lpr = ListPackages { ident: BuilderPackageIdent(ident.clone()), + visibility: + helpers::visibility_for_optional_session(req, + opt_session_id, + &ident.origin), + offset, + limit }; if pagination.distinct { match Package::list_distinct(&lpr, &mut conn).map_err(Error::DieselError) { diff --git a/components/builder-db/src/models/package.rs b/components/builder-db/src/models/package.rs index 6a8a806103..6482a89470 100644 --- a/components/builder-db/src/models/package.rs +++ b/components/builder-db/src/models/package.rs @@ -344,7 +344,7 @@ pub struct UpdatePackageVisibility { pub struct ListPackages { pub ident: BuilderPackageIdent, pub visibility: Vec, - pub page: i64, + pub offset: i64, pub limit: i64, } @@ -619,72 +619,77 @@ impl Package { let name_str = pl.ident.name.clone(); let parts = pl.ident.clone().parts(); let visibility = pl.visibility.clone(); - let page = pl.page; + let offset_val = pl.offset; let limit = pl.limit; - 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())) .into_boxed(); + // We need the into_boxed above to be able to conditionally filter and not break the // typesystem. 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())) }; - let mut pkgs = if pl.limit < 0 { - let query = - query.filter(packages_with_channel_platform::ident_array.contains(parts.clone())) - .filter(packages_with_channel_platform::visibility.eq_any(visibility.clone())) - .order(packages_with_channel_platform::ident.desc()); - let pkgs: std::vec::Vec = query.get_results(conn)?; - pkgs - } else { - // Use window function with COUNT(*) OVER() for efficient pagination - // This prevents loading all data into memory and avoids slice index panics - let offset_val = (page.saturating_sub(1)) * limit; - - let query_with_pagination = - query.filter(packages_with_channel_platform::ident_array.contains(parts.clone())) - .filter(packages_with_channel_platform::visibility.eq_any(visibility.clone())) - .order(packages_with_channel_platform::ident.desc()) - .limit(limit) - .offset(offset_val); - - let paginated_rows: Vec = query_with_pagination.load(conn)?; - - // Apply deduplication consistently using unique_by for package identity - let unique_rows: Vec = - paginated_rows.into_iter() - .unique_by(|p| (p.ident.clone(), p.origin.clone())) - .collect(); - - unique_rows - }; + base_query = base_query + .filter(packages_with_channel_platform::ident_array.contains(parts.clone())) + .filter(packages_with_channel_platform::visibility.eq_any(visibility.clone())); - // helpful trick when debugging queries, this has Debug trait: - // diesel::query_builder::debug_query::(&query) + if pl.limit < 0 { + // No pagination - return all records + let query = base_query.order(packages_with_channel_platform::ident.desc()); + let mut pkgs: std::vec::Vec = query.get_results(conn)?; - let duration_millis = start_time.elapsed().as_millis(); - Histogram::DbCallTime.set(duration_millis as f64); - Histogram::PackageListCallTime.set(duration_millis as f64); - - // Package list for a whole origin is still not very - // performant, and we want to track that - if !pl.ident.name.is_empty() { - Histogram::PackageListOriginOnlyCallTime.set(duration_millis as f64); + // Apply deduplication + pkgs = pkgs.into_iter().unique().collect(); + let count = pkgs.len() as i64; + Ok((pkgs, count)) } else { - Histogram::PackageListOriginNameCallTime.set(duration_millis as f64); + // 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 = query_all.load(conn)?; + + // Apply deduplication BEFORE pagination to ensure consistent page sizes + let deduplicated_rows: Vec = + all_rows.into_iter() + .unique_by(|p| (p.ident.clone(), p.origin.clone())) + .collect(); + + let total_count = deduplicated_rows.len() as i64; + + // Now apply pagination to deduplicated results + let start = offset_val as usize; + let end = if limit > 0 { + (start + limit as usize).min(deduplicated_rows.len()) + } else { + deduplicated_rows.len() + }; + + let paginated_rows = if start >= deduplicated_rows.len() { + Vec::new() + } else { + deduplicated_rows[start..end].to_vec() + }; + + let duration_millis = start_time.elapsed().as_millis(); + Histogram::DbCallTime.set(duration_millis as f64); + Histogram::PackageListCallTime.set(duration_millis as f64); + + // Package list for a whole origin is still not very + // performant, and we want to track that + if !pl.ident.name.is_empty() { + Histogram::PackageListOriginOnlyCallTime.set(duration_millis as f64); + } else { + Histogram::PackageListOriginNameCallTime.set(duration_millis as f64); + } + + trace!(target: "habitat_builder_api::server::resources::pkgs::versions", "Package::list for {:?}, returned {} items out of {} total", pl.ident, paginated_rows.len(), total_count); + + Ok((paginated_rows, total_count)) } - - trace!(target: "habitat_builder_api::server::resources::pkgs::versions", "Package::list for {:?}, returned {} items", pl.ident, pkgs.len()); - - // TODO: Look for a performant Postgresql fix - // and possibly rethink the channels design - pkgs = pkgs.into_iter().unique().collect(); - trace!(target: "habitat_builder_api::server::resources::pkgs::versions", "Package::list for {:?} after de-dup has {} items", pl.ident, pkgs.len()); - - let new_count = pkgs.len() as i64; - Ok((pkgs, new_count)) } pub fn list_distinct(pl: &ListPackages, @@ -698,7 +703,7 @@ impl Package { let name_str = pl.ident.name.clone(); let parts = pl.ident.clone().parts(); let visibility = pl.visibility.clone(); - let page = pl.page; + let offset_val = pl.offset; let limit = pl.limit; let mut count_query = @@ -733,7 +738,7 @@ impl Package { } let limit_i64 = limit; - let offset_i64 = (page.saturating_sub(1)) * limit; + let offset_i64 = offset_val; let rows: Vec<(String, String)> = page_query.limit(limit_i64).offset(offset_i64).load(conn)?; @@ -771,9 +776,11 @@ impl Package { // Extract cloned copies out of pl let origin_str = pl.ident.origin.clone(); let visibility = pl.visibility.clone(); - let page = pl.page; + let offset_val = pl.offset; let limit = pl.limit; + // This method already properly deduplicates BEFORE pagination + // which is the correct approach. Keep the existing logic but with cleaner implementation. let base_query = origin_package_settings::table .filter(origin_package_settings::origin.eq(origin_str)) .filter(origin_package_settings::visibility.eq_any(visibility)) @@ -782,20 +789,23 @@ impl Package { .order(origin_package_settings::name.asc()) .into_boxed(); - // helpful trick when debugging queries, this has Debug trait: - // diesel::query_builder::debug_query::(&base_query) - + // Get all records and deduplicate in memory (this ensures consistent pagination) let all_rows: Vec = base_query.load(conn)?; let unique_by_name: Vec = all_rows.into_iter() .unique_by(|pkg| pkg.name.clone()) .collect(); let total_count = unique_by_name.len() as i64; - let start = ((page.saturating_sub(1)) * limit) as usize; - let end = (start + limit as usize).min(unique_by_name.len()); - let results = if limit < 0 { - unique_by_name.clone() - } else if start >= unique_by_name.len() { + + // Apply pagination to deduplicated results + 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()) + }; + + let results = if start >= unique_by_name.len() { Vec::new() } else { unique_by_name[start..end].to_vec()