Skip to content

fixed offset #1943

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mwrock merged 2 commits into from
Nov 13, 2025
Merged

fixed offset #1943

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
79754c4
fixed offset
sougata-progress Nov 12, 2025
2281a77
deduplication fix
sougata-progress Nov 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion components/builder-api/src/server/resources/origins.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
35 changes: 19 additions & 16 deletions components/builder-api/src/server/resources/pkgs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Comment on lines +866 to +867

Copilot AI Nov 13, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copilot uses AI. Check for mistakes.

let mut response = if count as isize > (stop as isize + 1) {
HttpResponse::PartialContent()
Expand All @@ -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) {
Expand Down
142 changes: 76 additions & 66 deletions components/builder-db/src/models/package.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,7 +344,7 @@ pub struct UpdatePackageVisibility {
pub struct ListPackages {
pub ident: BuilderPackageIdent,
pub visibility: Vec<PackageVisibility>,
pub page: i64,
pub offset: i64,
pub limit: i64,
}

Expand Down Expand Up @@ -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()))

Copilot AI Nov 12, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
.filter(packages_with_channel_platform::origin.eq(origin_str.clone()))
.filter(packages_with_channel_platform::origin.eq(&origin_str))

Copilot uses AI. Check for mistakes.
.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()))

Copilot AI Nov 12, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name_str variable is cloned twice (lines 633 and 659). Consider reusing the reference or restructuring to avoid unnecessary clones.

Copilot uses AI. Check for mistakes.
};

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<PackageWithChannelPlatform> = 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<PackageWithChannelPlatform> = query_with_pagination.load(conn)?;

// Apply deduplication consistently using unique_by for package identity
let unique_rows: Vec<PackageWithChannelPlatform> =
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::<diesel::pg::Pg, _>(&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<PackageWithChannelPlatform> = 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();

Copilot AI Nov 13, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
pkgs = pkgs.into_iter().unique().collect();
pkgs = pkgs.into_iter().unique_by(|p| (p.ident.clone(), p.origin.clone())).collect();

Copilot uses AI. Check for mistakes.
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<PackageWithChannelPlatform> = query_all.load(conn)?;
Comment on lines +650 to +653

Copilot AI Nov 13, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

// Apply deduplication BEFORE pagination to ensure consistent page sizes
let deduplicated_rows: Vec<PackageWithChannelPlatform> =
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,
Expand All @@ -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 =
Expand Down Expand Up @@ -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)?;
Expand Down Expand Up @@ -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))
Expand All @@ -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::<diesel::pg::Pg, _>(&base_query)

// Get all records and deduplicate in memory (this ensures consistent pagination)
let all_rows: Vec<OriginPackageSettings> = base_query.load(conn)?;
let unique_by_name: Vec<OriginPackageSettings> = 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())
};
Comment on lines +801 to +806

Copilot AI Nov 13, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Suggested change
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());

Copilot uses AI. Check for mistakes.

let results = if start >= unique_by_name.len() {
Vec::new()
} else {
unique_by_name[start..end].to_vec()
Expand Down
Loading