fix(forge): validate list's label + milestone filters, and paginate both
A filter value the forge cannot resolve is DISCARDED, not rejected, so a typo does not narrow the result set -- it returns the unfiltered one. That does not waste a query, it inverts the answer: "is anything open in this milestone" comes back as every open issue and reads as yes, and a duplicate check gets a list that never narrowed. `list` now resolves both before querying. Labels reuse the write side's resolver; the ids are discarded because this endpoint filters by name, so resolution here is a spell-check rather than a lookup -- reusing it keeps the message identical to the one the write side has always produced. Milestones accept a title or an id and are checked against the ALL-state set: filtering on a closed milestone is a normal query, and validating against open-only would reject exactly the retrospective ones. Both fetchers paginate. `repo_labels` asked for one page of 100 and treated it as the population -- the inverse of the trailer bug, same root: a valid label past the cut fails to resolve, and the error then prints an "available labels" list that is itself truncated, so the message argues for the typo. `--assignee` / `--author` stay unvalidated on purpose: someone who has left still legitimately appears on old issues, so a login that is not a current member is not necessarily a typo. Also drops the docs paragraph claiming unknown labels are silently dropped on the write side; that has not been true since the resolver landed.
This commit is contained in:
atlas
parent
433b294099
commit
0aa9a854bc
4 changed files with 206 additions and 36 deletions
|
|
@ -276,20 +276,25 @@ to discover valid label names before triaging or to audit the label set.
|
|||
- Do NOT use raw `curl` for forge access -- the CLI handles auth,
|
||||
error checking, and output formatting.
|
||||
- `issue-create --label <name>` / `pr-create --label <name>` are
|
||||
repeatable and take the same spelling `labels <n> add` does. Unknown
|
||||
names are silently dropped (matching `labels add`'s existing
|
||||
behavior) rather than erroring, so a typo just means the label
|
||||
doesn't land — check `hive-forge repo-labels` if one's missing. On
|
||||
repeatable and take the same spelling `labels <n> add` does. **An
|
||||
unknown name is an error, not a silent drop** — the command fails
|
||||
listing the names that didn't resolve plus every label the repo has,
|
||||
so it's fixable from the message without a second call. On
|
||||
`pr-create --agit`, labels are applied as a follow-up call once the
|
||||
PR number is parsed back out of the push output (the AGit push
|
||||
itself has no label field), so they're silently skipped if that
|
||||
parse fails — same fallback as the deferred multi-line body.
|
||||
- `list --milestone <name>` takes a milestone **name or id**, is
|
||||
repeatable, and the forge *discards* one it doesn't recognise. So a
|
||||
typo returns the **unfiltered** list rather than an empty one — the
|
||||
failure looks like "this milestone contains everything", not like an
|
||||
error. Confirm the spelling with `hive-forge milestone`. (Same
|
||||
silent-discard shape as unknown labels above.)
|
||||
- `list --label <name>` / `list --milestone <name>` are repeatable and
|
||||
validated the same way, for a sharper reason: the forge **discards** a
|
||||
filter value it doesn't recognise, so a typo returns the
|
||||
**unfiltered** list rather than an empty one. That doesn't waste a
|
||||
query, it inverts the answer — "is anything open in this milestone"
|
||||
comes back as every open issue. `--milestone` takes a title or an id,
|
||||
and closed milestones count as valid (filtering on a shipped one is a
|
||||
normal query).
|
||||
- ⚠️ `--assignee` / `--author` are deliberately **not** validated: someone
|
||||
who has left still legitimately appears on old issues, so a login that
|
||||
isn't a current member is not necessarily a typo.
|
||||
- `list --limit N` is a *request*: the forge clamps page size to its own
|
||||
`api.MAX_RESPONSE_ITEMS` (50 by default), so `--limit 400` returns at
|
||||
most 50 rows. The stderr trailer reports the real total from the
|
||||
|
|
|
|||
|
|
@ -90,18 +90,38 @@ pub fn run(client: &Client, args: Args) -> Result<()> {
|
|||
Ok(())
|
||||
}
|
||||
|
||||
/// First page (100) of the repo's label set, for name → id resolution.
|
||||
/// `pub(crate)` so other creation verbs (`issue-create`, `pr-create`) can
|
||||
/// resolve a `--label` name to the id the create-payload structs need
|
||||
/// without duplicating the lookup.
|
||||
/// 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.
|
||||
///
|
||||
/// 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.
|
||||
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 (_, labels) = client
|
||||
.api()
|
||||
.issue_list_labels(owner, name, IssueListLabelsQuery::default())
|
||||
.page_size(100)
|
||||
.send()?;
|
||||
Ok(labels)
|
||||
let mut all = Vec::new();
|
||||
for page in 1..=MAX_PAGES {
|
||||
let (_, batch) = client
|
||||
.api()
|
||||
.issue_list_labels(owner, name, IssueListLabelsQuery::default())
|
||||
.page(page)
|
||||
.page_size(PAGE)
|
||||
.send()?;
|
||||
let short = batch.len() < PAGE as usize;
|
||||
all.extend(batch);
|
||||
if short {
|
||||
break;
|
||||
}
|
||||
}
|
||||
Ok(all)
|
||||
}
|
||||
|
||||
/// Resolve label names to ids, hard-erroring if any name doesn't match an
|
||||
|
|
|
|||
|
|
@ -24,7 +24,7 @@ use forgejo_api::structs::{
|
|||
use serde_json::Value;
|
||||
|
||||
use crate::client::Client;
|
||||
use crate::verbs::print_json;
|
||||
use crate::verbs::{labels, milestone, print_json};
|
||||
|
||||
/// What kind of items to return. Maps onto Forgejo's `type` query
|
||||
/// parameter: `issues` / `pulls`, or no filter at all for `both`
|
||||
|
|
@ -89,12 +89,15 @@ pub struct Args {
|
|||
#[arg(long)]
|
||||
mention: Option<String>,
|
||||
/// Filter to items carrying any of these label names. Repeatable.
|
||||
/// Validated client-side: a name the forge can't resolve is dropped
|
||||
/// from the filter rather than rejected, which returns MORE results
|
||||
/// than asked for, not fewer.
|
||||
#[arg(long = "label")]
|
||||
labels: Vec<String>,
|
||||
/// Filter to items in any of these milestones, by name or id.
|
||||
/// Repeatable. A name that doesn't exist is discarded by the forge
|
||||
/// rather than rejected — so a typo returns the UNFILTERED list, not
|
||||
/// an empty one. Check the spelling against `milestone list`.
|
||||
/// Filter to items in any of these milestones, by title or id.
|
||||
/// Repeatable. Validated client-side against the repo's milestones
|
||||
/// (closed ones included), since the forge would silently discard a
|
||||
/// name it can't resolve and return the UNFILTERED list.
|
||||
#[arg(long = "milestone")]
|
||||
milestones: Vec<String>,
|
||||
/// Full-text search over title AND body, server-side. Composes with
|
||||
|
|
@ -117,6 +120,29 @@ pub struct Args {
|
|||
|
||||
pub fn run(client: &Client, args: Args) -> Result<()> {
|
||||
let (owner, name) = client.owner_repo()?;
|
||||
// Validate the name-based filters BEFORE querying. The forge
|
||||
// *discards* a label or milestone it can't resolve instead of
|
||||
// rejecting it, so a typo doesn't narrow the result set — it returns
|
||||
// the UNFILTERED one. That doesn't waste a query, it inverts the
|
||||
// answer: "is anything open in this milestone" comes back as every
|
||||
// open issue and reads as yes, and a duplicate check gets a list that
|
||||
// never narrowed and concludes there isn't one.
|
||||
//
|
||||
// The ids are discarded on purpose — this endpoint filters by name,
|
||||
// so resolution here is a spell-check, not a lookup. `resolve_ids` is
|
||||
// reused rather than reimplemented so the message stays identical to
|
||||
// the one the write side has always produced.
|
||||
//
|
||||
// Costs one extra round-trip per filtered invocation, and only when
|
||||
// the filter is actually used.
|
||||
if !args.labels.is_empty() {
|
||||
let all = labels::repo_labels(client)?;
|
||||
labels::resolve_ids(&all, &args.labels)?;
|
||||
}
|
||||
if !args.milestones.is_empty() {
|
||||
let all = milestone::repo_milestones(client, "all")?;
|
||||
milestone::ensure_filters_resolve(&all, &args.milestones)?;
|
||||
}
|
||||
let query = IssueListIssuesQuery {
|
||||
state: Some(args.state.query_state()),
|
||||
// The forge parses `labels` as a comma-separated list of names.
|
||||
|
|
|
|||
|
|
@ -1,10 +1,10 @@
|
|||
//! `milestone list|create|close` — manage milestones. Default action:
|
||||
//! list.
|
||||
|
||||
use anyhow::{Context, Result};
|
||||
use anyhow::{Context, Result, bail};
|
||||
use clap::{Args as ClapArgs, Subcommand};
|
||||
use forgejo_api::structs::{
|
||||
CreateMilestoneOption, EditMilestoneOption, IssueGetMilestonesListQuery,
|
||||
CreateMilestoneOption, EditMilestoneOption, IssueGetMilestonesListQuery, Milestone,
|
||||
};
|
||||
use serde_json::{Value, json};
|
||||
use time::OffsetDateTime;
|
||||
|
|
@ -42,19 +42,80 @@ enum Action {
|
|||
},
|
||||
}
|
||||
|
||||
/// The repo's milestones in `state` (`open` / `closed` / `all`), all
|
||||
/// pages. `pub(crate)` so `list` can validate a `--milestone` filter
|
||||
/// against them.
|
||||
///
|
||||
/// Paginated for the reason `labels::repo_labels` spells out: a page size
|
||||
/// with no page loop silently truncates the population, and a validator
|
||||
/// reading a truncated set rejects legitimate input.
|
||||
pub(crate) fn repo_milestones(client: &Client, state: &str) -> Result<Vec<Milestone>> {
|
||||
/// Generous cap so a misbehaving server can't spin us forever.
|
||||
const MAX_PAGES: u32 = 50;
|
||||
const PAGE: u32 = 50;
|
||||
let (owner, name) = client.owner_repo()?;
|
||||
let mut all = Vec::new();
|
||||
for page in 1..=MAX_PAGES {
|
||||
let query = IssueGetMilestonesListQuery {
|
||||
state: Some(state.to_owned()),
|
||||
name: None,
|
||||
};
|
||||
let (_, batch) = client
|
||||
.api()
|
||||
.issue_get_milestones_list(owner, name, query)
|
||||
.page(page)
|
||||
.page_size(PAGE)
|
||||
.send()?;
|
||||
let short = batch.len() < PAGE as usize;
|
||||
all.extend(batch);
|
||||
if short {
|
||||
break;
|
||||
}
|
||||
}
|
||||
Ok(all)
|
||||
}
|
||||
|
||||
/// Check every `--milestone` filter value names a real milestone,
|
||||
/// erroring with the ones that don't plus what is available.
|
||||
///
|
||||
/// Accepts a title **or** a numeric id, because Forgejo's filter does.
|
||||
///
|
||||
/// ⚠️ Callers must pass the `all`-state set, not the open one: filtering
|
||||
/// on a *closed* milestone is a normal query ("what shipped in v1"), and
|
||||
/// validating against open-only would reject exactly the retrospective
|
||||
/// queries that need it most. Same reasoning that keeps `--assignee` and
|
||||
/// `--author` unvalidated — a name being no longer current is not a typo.
|
||||
pub(crate) fn ensure_filters_resolve(all: &[Milestone], values: &[String]) -> Result<()> {
|
||||
let unresolved: Vec<&str> = values
|
||||
.iter()
|
||||
.filter(|v| {
|
||||
!all.iter().any(|m| {
|
||||
m.title.as_deref() == Some(v.as_str())
|
||||
|| v.parse::<i64>().is_ok_and(|id| m.id == Some(id))
|
||||
})
|
||||
})
|
||||
.map(String::as_str)
|
||||
.collect();
|
||||
if !unresolved.is_empty() {
|
||||
let available: Vec<&str> = all.iter().filter_map(|m| m.title.as_deref()).collect();
|
||||
bail!(
|
||||
"unresolved milestone(s): {} — available milestones: {}",
|
||||
unresolved.join(", "),
|
||||
if available.is_empty() {
|
||||
"(none)".to_owned()
|
||||
} else {
|
||||
available.join(", ")
|
||||
}
|
||||
);
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
pub fn run(client: &Client, args: Args) -> Result<()> {
|
||||
let (owner, name) = client.owner_repo()?;
|
||||
match args.action.unwrap_or(Action::List) {
|
||||
Action::List => {
|
||||
let query = IssueGetMilestonesListQuery {
|
||||
state: Some("open".to_owned()),
|
||||
name: None,
|
||||
};
|
||||
let (_, milestones) = client
|
||||
.api()
|
||||
.issue_get_milestones_list(owner, name, query)
|
||||
.page_size(50)
|
||||
.send()?;
|
||||
let milestones = repo_milestones(client, "open")?;
|
||||
let trimmed: Vec<Value> = milestones
|
||||
.iter()
|
||||
.map(|m| {
|
||||
|
|
@ -112,3 +173,61 @@ pub fn run(client: &Client, args: Args) -> Result<()> {
|
|||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::{Milestone, ensure_filters_resolve};
|
||||
|
||||
/// Struct literal rather than `serde_json::from_value`: the JSON
|
||||
/// fixture compiled and then failed at *runtime* on a required field
|
||||
/// (`closed_at`), which is a worse trade than listing the fields —
|
||||
/// this way a field the upstream crate adds is a compile error, and
|
||||
/// the neighbouring `labels.rs` fixture already does it this way.
|
||||
fn milestone(id: i64, title: &str) -> Milestone {
|
||||
Milestone {
|
||||
closed_at: None,
|
||||
closed_issues: None,
|
||||
created_at: None,
|
||||
description: None,
|
||||
due_on: None,
|
||||
id: Some(id),
|
||||
open_issues: None,
|
||||
state: None,
|
||||
title: Some(title.to_owned()),
|
||||
updated_at: None,
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn titles_and_ids_both_resolve() {
|
||||
let all = vec![milestone(1, "v1"), milestone(2, "v2")];
|
||||
ensure_filters_resolve(&all, &["v2".to_owned(), "1".to_owned()]).unwrap();
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn unresolved_value_errors_listing_the_typo_and_available_titles() {
|
||||
let all = vec![milestone(1, "v1"), milestone(2, "v2")];
|
||||
let err = ensure_filters_resolve(&all, &["v3".to_owned()]).unwrap_err();
|
||||
let msg = err.to_string();
|
||||
assert!(msg.contains("v3"), "missing the typo: {msg}");
|
||||
assert!(
|
||||
msg.contains("v1") && msg.contains("v2"),
|
||||
"missing what is available: {msg}"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_numeric_value_is_an_id_not_a_title() {
|
||||
// "2" must not match the milestone *titled* "2" by accident of
|
||||
// parsing, nor resolve because some milestone exists — an id
|
||||
// filter that silently matches nothing is the whole bug.
|
||||
let all = vec![milestone(1, "v1")];
|
||||
assert!(ensure_filters_resolve(&all, &["2".to_owned()]).is_err());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn unresolved_on_a_repo_with_no_milestones_says_none() {
|
||||
let err = ensure_filters_resolve(&[], &["v1".to_owned()]).unwrap_err();
|
||||
assert!(err.to_string().contains("(none)"));
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue