hive-forge: page past the forge's 50-row clamp; show list --json truncation
The forge clamps page_size to 50. branches, pr reviews, the review bodies merged into `comments <pr>`, and view's comment list each sent one request, so anything past the first page was dropped silently. labels::repo_labels did loop, but asked for 100/page and stopped on `len < 100`, so a clamped full page of 50 ended the walk. Add verbs::all_pages: requests 50/page until a short page, capped at 50 pages with a stderr warning when the cap is hit. Use it in all of the above; repo-labels now calls labels::repo_labels instead of its own copy of the loop. list --json now prints the same "N of M shown" stderr trailer as the pretty output; its paging and stdout shape are unchanged. Closes #5051
This commit is contained in:
parent
52f589780f
commit
0e5c61d12c
9 changed files with 164 additions and 90 deletions
|
|
@ -4,7 +4,7 @@ use anyhow::Result;
|
|||
use clap::Args as ClapArgs;
|
||||
|
||||
use crate::client::Client;
|
||||
use crate::verbs::refuse_json;
|
||||
use crate::verbs::{all_pages, refuse_json};
|
||||
|
||||
#[derive(ClapArgs)]
|
||||
pub struct Args {
|
||||
|
|
@ -15,11 +15,15 @@ pub struct Args {
|
|||
pub fn run(client: &Client, args: Args) -> Result<()> {
|
||||
refuse_json(client.json_mode(), "branches", "only the plain name list")?;
|
||||
let (owner, name) = client.owner_repo()?;
|
||||
let (_, branches) = client
|
||||
.api()
|
||||
.repo_list_branches(owner, name)
|
||||
.page_size(100)
|
||||
.send()?;
|
||||
let branches = all_pages(|page, size| {
|
||||
let (_, batch) = client
|
||||
.api()
|
||||
.repo_list_branches(owner, name)
|
||||
.page(page)
|
||||
.page_size(size)
|
||||
.send()?;
|
||||
Ok(batch)
|
||||
})?;
|
||||
for branch in &branches {
|
||||
let Some(n) = branch.name.as_deref() else {
|
||||
continue;
|
||||
|
|
|
|||
|
|
@ -38,7 +38,7 @@ use time::OffsetDateTime;
|
|||
use crate::client::{Client, index};
|
||||
use crate::notify;
|
||||
use crate::verbs::{
|
||||
MAX_LIMIT, PAGE_SIZE, clamp_limit, comment_reactions, parse_rfc3339, print_json,
|
||||
MAX_LIMIT, PAGE_SIZE, all_pages, clamp_limit, comment_reactions, parse_rfc3339, print_json,
|
||||
reaction_summary, rfc3339,
|
||||
};
|
||||
|
||||
|
|
@ -355,12 +355,16 @@ fn fetch_review_bodies(client: &Client, number: u64) -> Vec<Value> {
|
|||
let Ok(idx) = index(number) else {
|
||||
return Vec::new();
|
||||
};
|
||||
let reviews = client
|
||||
.api()
|
||||
.repo_list_pull_reviews(owner, name, idx)
|
||||
.send()
|
||||
.map(|(_, reviews)| reviews)
|
||||
.unwrap_or_default();
|
||||
let reviews = all_pages(|page, size| {
|
||||
let (_, batch) = client
|
||||
.api()
|
||||
.repo_list_pull_reviews(owner, name, idx)
|
||||
.page(page)
|
||||
.page_size(size)
|
||||
.send()?;
|
||||
Ok(batch)
|
||||
})
|
||||
.unwrap_or_default();
|
||||
reviews
|
||||
.into_iter()
|
||||
.filter_map(|r| {
|
||||
|
|
|
|||
|
|
@ -7,7 +7,7 @@ use forgejo_api::structs::{DeleteLabelsOption, IssueLabelsOption, IssueListLabel
|
|||
use serde_json::json;
|
||||
|
||||
use crate::client::{Client, index};
|
||||
use crate::verbs::print_json;
|
||||
use crate::verbs::{all_pages, print_json};
|
||||
|
||||
#[derive(ClapArgs)]
|
||||
pub struct Args {
|
||||
|
|
@ -105,37 +105,24 @@ pub fn run(client: &Client, args: Args) -> Result<()> {
|
|||
}
|
||||
|
||||
/// The repo's whole label set, for name → id resolution. `pub(crate)` so
|
||||
/// other verbs (`issue create`, `pr create`, `list`) can resolve a label
|
||||
/// name without duplicating the lookup.
|
||||
/// other verbs (`issue create`, `pr create`, `list`, `repo-labels`) share
|
||||
/// one lookup.
|
||||
///
|
||||
/// Paginated, where this used to ask for one page of 100 — **a page size
|
||||
/// with no page loop is a lie the size of the page.** Truncation here is
|
||||
/// worse than it looks: it doesn't drop a result, it makes a *valid*
|
||||
/// label past the cut fail to resolve, and then prints an "available
|
||||
/// labels" list that is itself incomplete, so the error argues for the
|
||||
/// typo. Same root as the `list` trailer bug (assuming one request
|
||||
/// returns the whole population), opposite direction — that one was a
|
||||
/// false positive, this is a false negative.
|
||||
/// Every page, not one: a page size with no page loop silently truncates
|
||||
/// the set, and a truncated set makes a *valid* label past the cut fail to
|
||||
/// resolve, with an "available labels" list that is itself incomplete, so
|
||||
/// the error argues for the typo.
|
||||
pub(crate) fn repo_labels(client: &Client) -> Result<Vec<Label>> {
|
||||
/// Generous cap so a misbehaving server can't spin us forever.
|
||||
const MAX_PAGES: u32 = 50;
|
||||
const PAGE: u32 = 100;
|
||||
let (owner, name) = client.owner_repo()?;
|
||||
let mut all = Vec::new();
|
||||
for page in 1..=MAX_PAGES {
|
||||
all_pages(|page, size| {
|
||||
let (_, batch) = client
|
||||
.api()
|
||||
.issue_list_labels(owner, name, IssueListLabelsQuery::default())
|
||||
.page(page)
|
||||
.page_size(PAGE)
|
||||
.page_size(size)
|
||||
.send()?;
|
||||
let short = batch.len() < PAGE as usize;
|
||||
all.extend(batch);
|
||||
if short {
|
||||
break;
|
||||
}
|
||||
}
|
||||
Ok(all)
|
||||
Ok(batch)
|
||||
})
|
||||
}
|
||||
|
||||
/// Resolve label names to ids, hard-erroring if any name doesn't match an
|
||||
|
|
|
|||
|
|
@ -173,10 +173,20 @@ pub fn run(client: &Client, args: Args) -> Result<()> {
|
|||
.page_size(u32::try_from(args.limit).unwrap_or(u32::MAX))
|
||||
.send()?;
|
||||
let count = issues.len() as u64;
|
||||
let note = trailer(
|
||||
headers.x_total_count.and_then(|t| u64::try_from(t).ok()),
|
||||
count,
|
||||
args.page,
|
||||
args.limit,
|
||||
);
|
||||
let mut items = serde_json::to_value(issues)?;
|
||||
if client.json_mode() {
|
||||
hoist_merge_state(&mut items);
|
||||
return print_json(&items);
|
||||
print_json(&items)?;
|
||||
if let Some(msg) = note {
|
||||
eprintln!("{msg}");
|
||||
}
|
||||
return Ok(());
|
||||
}
|
||||
for item in items.as_array().into_iter().flatten() {
|
||||
let number = item.get("number").and_then(Value::as_u64);
|
||||
|
|
@ -192,12 +202,7 @@ pub fn run(client: &Client, args: Args) -> Result<()> {
|
|||
blocks.as_deref().map_or(0, blocking_open_count),
|
||||
);
|
||||
}
|
||||
if let Some(msg) = trailer(
|
||||
headers.x_total_count.and_then(|t| u64::try_from(t).ok()),
|
||||
count,
|
||||
args.page,
|
||||
args.limit,
|
||||
) {
|
||||
if let Some(msg) = note {
|
||||
eprintln!("{msg}");
|
||||
}
|
||||
Ok(())
|
||||
|
|
|
|||
|
|
@ -120,9 +120,99 @@ pub(crate) fn clamp_limit(requested: u64) -> (u64, bool) {
|
|||
(limit, limit < requested)
|
||||
}
|
||||
|
||||
/// [`PAGE_SIZE`] as the `u32` the generated client's `.page_size()` takes.
|
||||
const PAGE_SIZE_U32: u32 = 50;
|
||||
|
||||
/// Runaway cap for [`all_pages`]: at most `MAX_PAGES * PAGE_SIZE` rows.
|
||||
const MAX_PAGES: u32 = 50;
|
||||
|
||||
/// Every row of a paged forge listing. `fetch(page, page_size)` sends one
|
||||
/// request; the walk stops at the first page shorter than `page_size`.
|
||||
/// The page size is the forge's clamp, not a larger request: a clamped
|
||||
/// full page would read as short and end the walk after one page.
|
||||
/// Stopping at [`MAX_PAGES`] prints a warning on stderr, so a capped
|
||||
/// result is never presented as the whole population.
|
||||
pub(crate) fn all_pages<T>(fetch: impl FnMut(u32, u32) -> Result<Vec<T>>) -> Result<Vec<T>> {
|
||||
let (rows, capped) = walk_pages(MAX_PAGES, fetch)?;
|
||||
if capped {
|
||||
eprintln!(
|
||||
"hive-forge: stopped after {MAX_PAGES} pages ({} rows); the result may be incomplete",
|
||||
rows.len()
|
||||
);
|
||||
}
|
||||
Ok(rows)
|
||||
}
|
||||
|
||||
/// [`all_pages`]' loop, with the cap as a parameter. Returns the rows and
|
||||
/// whether the walk hit `max_pages` without seeing a short page.
|
||||
fn walk_pages<T>(
|
||||
max_pages: u32,
|
||||
mut fetch: impl FnMut(u32, u32) -> Result<Vec<T>>,
|
||||
) -> Result<(Vec<T>, bool)> {
|
||||
let mut rows = Vec::new();
|
||||
for page in 1..=max_pages {
|
||||
let batch = fetch(page, PAGE_SIZE_U32)?;
|
||||
let short = batch.len() < PAGE_SIZE_U32 as usize;
|
||||
rows.extend(batch);
|
||||
if short {
|
||||
return Ok((rows, false));
|
||||
}
|
||||
}
|
||||
Ok((rows, true))
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod page_limit_tests {
|
||||
use super::{MAX_LIMIT, PAGE_SIZE, clamp_limit};
|
||||
use anyhow::Result;
|
||||
|
||||
use super::{MAX_LIMIT, PAGE_SIZE, PAGE_SIZE_U32, clamp_limit, walk_pages};
|
||||
|
||||
/// A fake listing of `total` rows, served `page_size` at a time.
|
||||
fn listing(total: usize) -> impl FnMut(u32, u32) -> Result<Vec<usize>> {
|
||||
move |page, size| {
|
||||
let (page, size) = (page as usize, size as usize);
|
||||
Ok(((page - 1) * size..(page * size).min(total)).collect())
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn page_size_u32_matches_page_size() {
|
||||
assert_eq!(u64::from(PAGE_SIZE_U32), PAGE_SIZE);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn walk_pages_collects_every_row_past_the_first_page() {
|
||||
let (rows, capped) = walk_pages(50, listing(86)).unwrap();
|
||||
assert_eq!(rows, (0..86).collect::<Vec<_>>());
|
||||
assert!(!capped);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn walk_pages_stops_on_the_empty_page_after_an_exact_multiple() {
|
||||
let mut calls = 0;
|
||||
let mut inner = listing(100);
|
||||
let (rows, capped) = walk_pages(50, |p, s| {
|
||||
calls += 1;
|
||||
inner(p, s)
|
||||
})
|
||||
.unwrap();
|
||||
assert_eq!(rows.len(), 100);
|
||||
assert_eq!(calls, 3);
|
||||
assert!(!capped);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn walk_pages_reports_hitting_the_cap() {
|
||||
let (rows, capped) = walk_pages(2, listing(500)).unwrap();
|
||||
assert_eq!(rows.len(), 100);
|
||||
assert!(capped);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn walk_pages_propagates_a_fetch_error() {
|
||||
let err = walk_pages::<usize>(5, |_, _| anyhow::bail!("boom")).unwrap_err();
|
||||
assert_eq!(err.to_string(), "boom");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn clamp_limit_passes_small_requests_through() {
|
||||
|
|
|
|||
|
|
@ -7,7 +7,7 @@ use forgejo_api::structs::CreatePullReviewOptions;
|
|||
use serde_json::{Value, json};
|
||||
|
||||
use crate::client::{Client, index};
|
||||
use crate::verbs::print_json;
|
||||
use crate::verbs::{all_pages, print_json};
|
||||
|
||||
#[derive(ClapArgs)]
|
||||
pub struct Args {
|
||||
|
|
@ -97,10 +97,16 @@ fn fetch_inline_comments(client: &Client, pr: u64, review_id: i64) -> Vec<Value>
|
|||
/// List all reviews for a PR, dispatching to the appropriate output mode.
|
||||
fn list_reviews(client: &Client, number: u64) -> Result<()> {
|
||||
let (owner, name) = client.owner_repo()?;
|
||||
let (_, reviews) = client
|
||||
.api()
|
||||
.repo_list_pull_reviews(owner, name, index(number)?)
|
||||
.send()?;
|
||||
let idx = index(number)?;
|
||||
let reviews = all_pages(|page, size| {
|
||||
let (_, batch) = client
|
||||
.api()
|
||||
.repo_list_pull_reviews(owner, name, idx)
|
||||
.page(page)
|
||||
.page_size(size)
|
||||
.send()?;
|
||||
Ok(batch)
|
||||
})?;
|
||||
let reviews = serde_json::to_value(reviews)?;
|
||||
let reviews = reviews.as_array().cloned().unwrap_or_default();
|
||||
if client.json_mode() {
|
||||
|
|
|
|||
|
|
@ -5,16 +5,10 @@
|
|||
|
||||
use anyhow::Result;
|
||||
use clap::Args as ClapArgs;
|
||||
use forgejo_api::structs::{IssueListLabelsQuery, Label};
|
||||
use forgejo_api::structs::Label;
|
||||
|
||||
use crate::client::Client;
|
||||
use crate::verbs::print_json;
|
||||
|
||||
/// Page size on the label list endpoint.
|
||||
const PAGE_SIZE: u32 = 50;
|
||||
|
||||
/// Runaway cap on label pagination — same ceiling the raw client used.
|
||||
const MAX_PAGES: u32 = 10;
|
||||
use crate::verbs::{labels, print_json};
|
||||
|
||||
#[derive(ClapArgs)]
|
||||
pub struct Args {
|
||||
|
|
@ -23,23 +17,7 @@ pub struct Args {
|
|||
}
|
||||
|
||||
pub fn run(client: &Client, args: Args) -> Result<()> {
|
||||
let (owner, name) = client.owner_repo()?;
|
||||
// Repos can carry more than one page of labels; paginate so the list
|
||||
// is complete rather than capped at the first page.
|
||||
let mut labels: Vec<Label> = Vec::new();
|
||||
for page in 1..=MAX_PAGES {
|
||||
let (_, batch) = client
|
||||
.api()
|
||||
.issue_list_labels(owner, name, IssueListLabelsQuery::default())
|
||||
.page(page)
|
||||
.page_size(PAGE_SIZE)
|
||||
.send()?;
|
||||
let short = batch.len() < PAGE_SIZE as usize;
|
||||
labels.extend(batch);
|
||||
if short {
|
||||
break;
|
||||
}
|
||||
}
|
||||
let labels = labels::repo_labels(client)?;
|
||||
let filtered: Vec<&Label> = labels
|
||||
.iter()
|
||||
.filter(|l| {
|
||||
|
|
|
|||
|
|
@ -7,7 +7,7 @@ use forgejo_api::structs::{IssueGetCommentsQuery, StateType};
|
|||
|
||||
use crate::client::{Client, index};
|
||||
use crate::notify;
|
||||
use crate::verbs::{attachment_line, issue_reactions, reaction_summary, refuse_json};
|
||||
use crate::verbs::{all_pages, attachment_line, issue_reactions, reaction_summary, refuse_json};
|
||||
|
||||
#[derive(ClapArgs)]
|
||||
pub struct Args {
|
||||
|
|
@ -62,16 +62,16 @@ pub fn run(client: &Client, args: Args) -> Result<()> {
|
|||
if let Some(summary) = reaction_summary(&reactions) {
|
||||
println!("[reactions: {summary}]");
|
||||
}
|
||||
let (_, comments) = client
|
||||
.api()
|
||||
.issue_get_comments(
|
||||
owner,
|
||||
name,
|
||||
index(args.number)?,
|
||||
IssueGetCommentsQuery::default(),
|
||||
)
|
||||
.page_size(50)
|
||||
.send()?;
|
||||
let idx = index(args.number)?;
|
||||
let comments = all_pages(|page, size| {
|
||||
let (_, batch) = client
|
||||
.api()
|
||||
.issue_get_comments(owner, name, idx, IssueGetCommentsQuery::default())
|
||||
.page(page)
|
||||
.page_size(size)
|
||||
.send()?;
|
||||
Ok(batch)
|
||||
})?;
|
||||
if !comments.is_empty() {
|
||||
println!();
|
||||
println!("---");
|
||||
|
|
|
|||
Loading…
Reference in a new issue