From c9a97b38e187431010afcb1ae9eee66375c8ad19 Mon Sep 17 00:00:00 2001 From: Matt Gaunt-Seo Date: Tue, 10 Mar 2026 18:17:53 -0700 Subject: [PATCH] Reorg utils and stop using pub(crate) for repo_helpers. --- src/api/types.rs | 2 +- src/commands/bugs.rs | 129 +---------------- src/commands/mod.rs | 1 - src/commands/repo_helpers.rs | 88 ------------ src/commands/repos.rs | 2 +- src/commands/scans.rs | 4 +- src/output.rs | 2 +- src/{utils.rs => utils/datetime.rs} | 25 ---- src/utils/mod.rs | 3 + src/utils/pagination.rs | 29 ++++ src/utils/repos.rs | 215 ++++++++++++++++++++++++++++ 11 files changed, 255 insertions(+), 245 deletions(-) delete mode 100644 src/commands/repo_helpers.rs rename src/{utils.rs => utils/datetime.rs} (79%) create mode 100644 src/utils/mod.rs create mode 100644 src/utils/pagination.rs create mode 100644 src/utils/repos.rs diff --git a/src/api/types.rs b/src/api/types.rs index fc549d0..b619f30 100644 --- a/src/api/types.rs +++ b/src/api/types.rs @@ -1,7 +1,7 @@ use clap::builder::PossibleValue; use crate::output::Formattable; -use crate::utils::{format_date, format_datetime}; +use crate::utils::datetime::{format_date, format_datetime}; // Re-export generated types as the public API for this crate. pub use super::generated::types::{ diff --git a/src/commands/bugs.rs b/src/commands/bugs.rs index cce0010..02239f7 100644 --- a/src/commands/bugs.rs +++ b/src/commands/bugs.rs @@ -8,9 +8,10 @@ use crate::api::types::{ dismissal_reason_label, review_state_label, Bug, BugDismissalReason, BugId, BugReviewState, IntroducedIn, RepoId, }; -use crate::commands::repo_helpers::resolve_repo_id; use crate::output::{output_list, SectionRenderer}; -use crate::utils::{format_datetime, page_to_offset}; +use crate::utils::datetime::format_datetime; +use crate::utils::pagination::page_to_offset; +use crate::utils::repos::resolve_repo_id; /// Return only bugs where `isSecurityVulnerability` is `true`. fn filter_vulns_only(bugs: &[Bug]) -> Vec { @@ -414,130 +415,6 @@ pub async fn handle(command: &BugCommands, cli: &crate::Cli) -> Result<()> { #[cfg(test)] mod tests { use super::*; - use crate::api::types::Repo; - use crate::commands::repo_helpers::validate_owner_repo_format; - use crate::commands::repo_helpers::{match_repo_by_name, resolve_repo_id_from_repos}; - - // ── validate_owner_repo_format ─────────────────────────────────── - - #[test] - fn valid_owner_repo() { - assert!(validate_owner_repo_format("usedetail/cli").is_ok()); - } - - #[test] - fn rejects_empty_owner() { - assert!(validate_owner_repo_format("/cli").is_err()); - } - - #[test] - fn rejects_empty_repo() { - assert!(validate_owner_repo_format("usedetail/").is_err()); - } - - #[test] - fn rejects_multiple_slashes() { - assert!(validate_owner_repo_format("a/b/c").is_err()); - } - - #[test] - fn rejects_slash_only() { - assert!(validate_owner_repo_format("/").is_err()); - } - - // ── match_repo_by_name ─────────────────────────────────────────── - - fn sample_repos() -> Vec { - vec![ - serde_json::from_value(serde_json::json!({ - "id": "repo_1", "name": "cli", "ownerName": "usedetail", - "fullName": "usedetail/cli", "visibility": "public", - "primaryBranch": "main", "orgId": "org_1", "orgName": "Detail" - })) - .unwrap(), - serde_json::from_value(serde_json::json!({ - "id": "repo_2", "name": "cli", "ownerName": "acme", - "fullName": "acme/cli", "visibility": "private", - "primaryBranch": "main", "orgId": "org_2", "orgName": "Acme" - })) - .unwrap(), - serde_json::from_value(serde_json::json!({ - "id": "repo_3", "name": "web", "ownerName": "usedetail", - "fullName": "usedetail/web", "visibility": "public", - "primaryBranch": "main", "orgId": "org_1", "orgName": "Detail" - })) - .unwrap(), - ] - } - - #[test] - fn match_single_repo_by_name() { - let repos = sample_repos(); - let id = match_repo_by_name("web", &repos).unwrap(); - assert_eq!(id.to_string(), "repo_3"); - } - - #[test] - fn match_no_repo_returns_error() { - let repos = sample_repos(); - let err = match_repo_by_name("nonexistent", &repos).unwrap_err(); - assert!(err.to_string().contains("not found")); - } - - #[test] - fn match_multiple_repos_returns_error_with_names() { - let repos = sample_repos(); - let err = match_repo_by_name("cli", &repos).unwrap_err(); - let msg = err.to_string(); - assert!(msg.contains("Multiple repositories")); - assert!(msg.contains("usedetail/cli")); - assert!(msg.contains("acme/cli")); - } - - #[test] - fn match_empty_repo_list() { - let err = match_repo_by_name("cli", &[]).unwrap_err(); - assert!(err.to_string().contains("not found")); - } - - // ── resolve_repo_id_from_repos ────────────────────────────────── - - #[test] - fn resolve_owner_repo_exact_match() { - let repos = sample_repos(); - let id = resolve_repo_id_from_repos(&repos, "usedetail/cli").unwrap(); - assert_eq!(id.to_string(), "repo_1"); - } - - #[test] - fn resolve_owner_repo_not_found_has_access_hint() { - let repos = sample_repos(); - let err = resolve_repo_id_from_repos(&repos, "usedetail/missing").unwrap_err(); - assert!(err - .to_string() - .contains("Make sure you have access to this repository")); - } - - #[test] - fn resolve_owner_repo_invalid_format_rejected() { - let repos = sample_repos(); - let err = resolve_repo_id_from_repos(&repos, "usedetail/cli/extra").unwrap_err(); - assert!(err.to_string().contains("Invalid repository format")); - } - - #[test] - fn resolve_bare_repo_name_unique_match() { - let repos = sample_repos(); - let id = resolve_repo_id_from_repos(&repos, "web").unwrap(); - assert_eq!(id.to_string(), "repo_3"); - } - - #[test] - fn resolve_bare_repo_name_ambiguous_returns_error() { - let repos = sample_repos(); - let err = resolve_repo_id_from_repos(&repos, "cli").unwrap_err(); - assert!(err.to_string().contains("Multiple repositories")); - } // ── validate_close_flags ───────────────────────────────────────── diff --git a/src/commands/mod.rs b/src/commands/mod.rs index d8ba20b..f9f69a2 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -1,6 +1,5 @@ pub mod auth; pub mod bugs; -pub mod repo_helpers; pub mod repos; pub mod satisfying_sort; pub mod scans; diff --git a/src/commands/repo_helpers.rs b/src/commands/repo_helpers.rs deleted file mode 100644 index f24dc4f..0000000 --- a/src/commands/repo_helpers.rs +++ /dev/null @@ -1,88 +0,0 @@ -use anyhow::{bail, Context, Result}; - -use crate::api::client::ApiClient; -use crate::api::types::{Repo, RepoId}; - -/// Page size used when paginating through repos to resolve identifiers. -const REPO_PAGE_SIZE: u32 = 100; - -/// Fetch all repos by paginating through the API. -async fn fetch_all_repos(client: &ApiClient) -> Result> { - let mut all_repos = Vec::new(); - let mut offset = 0; - - loop { - let repos = client - .list_repos(REPO_PAGE_SIZE, offset) - .await - .context("Failed to fetch repositories while resolving identifier")?; - - let page_size = repos.repos.len(); - all_repos.extend(repos.repos); - - if page_size < usize::try_from(REPO_PAGE_SIZE).unwrap_or(0) { - break; - } - offset += REPO_PAGE_SIZE; - } - - Ok(all_repos) -} - -/// Validate that a slash-containing identifier has exactly one slash with -/// non-empty owner and repo parts. -pub(crate) fn validate_owner_repo_format(identifier: &str) -> Result<()> { - let parts: Vec<&str> = identifier.split('/').collect(); - if parts.len() != 2 || parts[0].is_empty() || parts[1].is_empty() { - bail!( - "Invalid repository format. Please use owner/repo (e.g., 'usedetail/cli') or just the repo name. Run 'detail repos list' to see your repositories." - ); - } - Ok(()) -} - -/// Given a bare repo name and the full list of accessible repos, return the -/// matching repo ID — or a helpful error when zero or multiple repos match. -pub(crate) fn match_repo_by_name(name: &str, repos: &[Repo]) -> Result { - let matching: Vec<_> = repos.iter().filter(|r| r.name == name).collect(); - - match matching.len() { - 0 => bail!( - "Repository '{name}' not found. Run 'detail repos list' to see your repositories." - ), - 1 => Ok(matching[0].id.clone()), - _ => { - let repo_list: Vec = matching - .iter() - .map(|r| format!(" - {}", r.full_name)) - .collect(); - bail!( - "Multiple repositories with name '{}' found:\n{}\n\nPlease specify using owner/repo format (e.g., '{}').", - name, - repo_list.join("\n"), - matching[0].full_name - ) - } - } -} - -/// Resolve owner/repo or repo name to repo ID, searching across all accessible repos. -pub(crate) async fn resolve_repo_id(client: &ApiClient, repo_identifier: &str) -> Result { - let repos = fetch_all_repos(client).await?; - resolve_repo_id_from_repos(&repos, repo_identifier) -} - -pub(crate) fn resolve_repo_id_from_repos(repos: &[Repo], repo_identifier: &str) -> Result { - if repo_identifier.contains('/') { - validate_owner_repo_format(repo_identifier)?; - repos - .iter() - .find(|r| r.full_name == repo_identifier) - .map(|r| r.id.clone()) - .context(format!( - "Repository '{repo_identifier}' not found. Make sure you have access to this repository." - )) - } else { - match_repo_by_name(repo_identifier, repos) - } -} diff --git a/src/commands/repos.rs b/src/commands/repos.rs index 813fb61..c7d867c 100644 --- a/src/commands/repos.rs +++ b/src/commands/repos.rs @@ -5,7 +5,7 @@ use clap::Subcommand; use console::{style, Term}; use crate::output::output_list; -use crate::utils::page_to_offset; +use crate::utils::pagination::page_to_offset; #[derive(Subcommand)] pub enum RepoCommands { diff --git a/src/commands/scans.rs b/src/commands/scans.rs index a708ad0..16b570d 100644 --- a/src/commands/scans.rs +++ b/src/commands/scans.rs @@ -1,9 +1,9 @@ use anyhow::{Context, Result}; use clap::Subcommand; -use crate::commands::repo_helpers::resolve_repo_id; use crate::output::output_list; -use crate::utils::page_to_offset; +use crate::utils::pagination::page_to_offset; +use crate::utils::repos::resolve_repo_id; #[derive(Subcommand)] pub enum ScanCommands { diff --git a/src/output.rs b/src/output.rs index 74f4a9a..2503ee7 100644 --- a/src/output.rs +++ b/src/output.rs @@ -9,7 +9,7 @@ use console::{style, Term}; use serde::Serialize; use termimad::crossterm::style::Attribute; -use crate::utils::page_to_offset; +use crate::utils::pagination::page_to_offset; static MARKDOWN_SKIN: LazyLock = LazyLock::new(|| { let mut skin = termimad::MadSkin::default(); diff --git a/src/utils.rs b/src/utils/datetime.rs similarity index 79% rename from src/utils.rs rename to src/utils/datetime.rs index ade57cb..9a951de 100644 --- a/src/utils.rs +++ b/src/utils/datetime.rs @@ -1,10 +1,5 @@ use chrono::Local; -/// Convert page number and limit to offset for pagination -pub const fn page_to_offset(page: u32, limit: u32) -> u32 { - (page - 1).saturating_mul(limit) -} - /// Conversion factor from milliseconds to seconds const MS_TO_SECONDS: i64 = 1000; @@ -38,26 +33,6 @@ mod tests { .to_string() } - #[test] - fn page_to_offset_first_page() { - assert_eq!(page_to_offset(1, 50), 0); - } - - #[test] - fn page_to_offset_second_page() { - assert_eq!(page_to_offset(2, 50), 50); - } - - #[test] - fn page_to_offset_custom_limit() { - assert_eq!(page_to_offset(3, 10), 20); - } - - #[test] - fn page_to_offset_limit_one() { - assert_eq!(page_to_offset(5, 1), 4); - } - #[test] fn format_date_unix_epoch() { assert_eq!(format_date(0), expected_local(0, "%Y-%m-%d")); diff --git a/src/utils/mod.rs b/src/utils/mod.rs new file mode 100644 index 0000000..ab49a10 --- /dev/null +++ b/src/utils/mod.rs @@ -0,0 +1,3 @@ +pub mod datetime; +pub mod pagination; +pub mod repos; diff --git a/src/utils/pagination.rs b/src/utils/pagination.rs new file mode 100644 index 0000000..fe79f0e --- /dev/null +++ b/src/utils/pagination.rs @@ -0,0 +1,29 @@ +/// Convert page number and limit to offset for pagination +pub const fn page_to_offset(page: u32, limit: u32) -> u32 { + (page - 1).saturating_mul(limit) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn page_to_offset_first_page() { + assert_eq!(page_to_offset(1, 50), 0); + } + + #[test] + fn page_to_offset_second_page() { + assert_eq!(page_to_offset(2, 50), 50); + } + + #[test] + fn page_to_offset_custom_limit() { + assert_eq!(page_to_offset(3, 10), 20); + } + + #[test] + fn page_to_offset_limit_one() { + assert_eq!(page_to_offset(5, 1), 4); + } +} diff --git a/src/utils/repos.rs b/src/utils/repos.rs new file mode 100644 index 0000000..0c9e99a --- /dev/null +++ b/src/utils/repos.rs @@ -0,0 +1,215 @@ +use anyhow::{bail, Context, Result}; + +use crate::api::client::ApiClient; +use crate::api::types::{Repo, RepoId}; + +/// Page size used when paginating through repos to resolve identifiers. +const REPO_PAGE_SIZE: u32 = 100; + +/// Fetch all repos by paginating through the API. +pub async fn fetch_all_repos(client: &ApiClient) -> Result> { + let mut all_repos = Vec::new(); + let mut offset = 0; + + loop { + let repos = client + .list_repos(REPO_PAGE_SIZE, offset) + .await + .context("Failed to fetch repositories while resolving identifier")?; + + let page_size = repos.repos.len(); + all_repos.extend(repos.repos); + + if page_size < usize::try_from(REPO_PAGE_SIZE).unwrap_or(0) { + break; + } + offset += REPO_PAGE_SIZE; + } + + Ok(all_repos) +} + +/// Validate that a slash-containing identifier has exactly one slash with +/// non-empty owner and repo parts. +pub fn validate_owner_repo_format(identifier: &str) -> Result<()> { + let parts: Vec<&str> = identifier.split('/').collect(); + if parts.len() != 2 || parts[0].is_empty() || parts[1].is_empty() { + bail!( + "Invalid repository format. Please use owner/repo (e.g., 'usedetail/cli') or just the repo name. Run 'detail repos list' to see your repositories." + ); + } + Ok(()) +} + +/// Given a bare repo name and the full list of accessible repos, return the +/// matching repo ID — or a helpful error when zero or multiple repos match. +pub fn match_repo_by_name(name: &str, repos: &[Repo]) -> Result { + let matching: Vec<_> = repos.iter().filter(|r| r.name == name).collect(); + + match matching.len() { + 0 => bail!( + "Repository '{name}' not found. Run 'detail repos list' to see your repositories." + ), + 1 => Ok(matching[0].id.clone()), + _ => { + let repo_list: Vec = matching + .iter() + .map(|r| format!(" - {}", r.full_name)) + .collect(); + bail!( + "Multiple repositories with name '{}' found:\n{}\n\nPlease specify using owner/repo format (e.g., '{}').", + name, + repo_list.join("\n"), + matching[0].full_name + ) + } + } +} + +/// Resolve owner/repo or repo name to repo ID, searching across all accessible repos. +pub async fn resolve_repo_id(client: &ApiClient, repo_identifier: &str) -> Result { + let repos = fetch_all_repos(client).await?; + resolve_repo_id_from_repos(&repos, repo_identifier) +} + +pub fn resolve_repo_id_from_repos(repos: &[Repo], repo_identifier: &str) -> Result { + if repo_identifier.contains('/') { + validate_owner_repo_format(repo_identifier)?; + repos + .iter() + .find(|r| r.full_name == repo_identifier) + .map(|r| r.id.clone()) + .context(format!( + "Repository '{repo_identifier}' not found. Make sure you have access to this repository." + )) + } else { + match_repo_by_name(repo_identifier, repos) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::api::types::Repo; + + fn sample_repos() -> Vec { + vec![ + serde_json::from_value(serde_json::json!({ + "id": "repo_1", "name": "cli", "ownerName": "usedetail", + "fullName": "usedetail/cli", "visibility": "public", + "primaryBranch": "main", "orgId": "org_1", "orgName": "Detail" + })) + .unwrap(), + serde_json::from_value(serde_json::json!({ + "id": "repo_2", "name": "cli", "ownerName": "acme", + "fullName": "acme/cli", "visibility": "private", + "primaryBranch": "main", "orgId": "org_2", "orgName": "Acme" + })) + .unwrap(), + serde_json::from_value(serde_json::json!({ + "id": "repo_3", "name": "web", "ownerName": "usedetail", + "fullName": "usedetail/web", "visibility": "public", + "primaryBranch": "main", "orgId": "org_1", "orgName": "Detail" + })) + .unwrap(), + ] + } + + // ── validate_owner_repo_format ─────────────────────────────────── + + #[test] + fn valid_owner_repo() { + assert!(validate_owner_repo_format("usedetail/cli").is_ok()); + } + + #[test] + fn rejects_empty_owner() { + assert!(validate_owner_repo_format("/cli").is_err()); + } + + #[test] + fn rejects_empty_repo() { + assert!(validate_owner_repo_format("usedetail/").is_err()); + } + + #[test] + fn rejects_multiple_slashes() { + assert!(validate_owner_repo_format("a/b/c").is_err()); + } + + #[test] + fn rejects_slash_only() { + assert!(validate_owner_repo_format("/").is_err()); + } + + // ── match_repo_by_name ─────────────────────────────────────────── + + #[test] + fn match_single_repo_by_name() { + let repos = sample_repos(); + let id = match_repo_by_name("web", &repos).unwrap(); + assert_eq!(id.to_string(), "repo_3"); + } + + #[test] + fn match_no_repo_returns_error() { + let repos = sample_repos(); + let err = match_repo_by_name("nonexistent", &repos).unwrap_err(); + assert!(err.to_string().contains("not found")); + } + + #[test] + fn match_multiple_repos_returns_error_with_names() { + let repos = sample_repos(); + let err = match_repo_by_name("cli", &repos).unwrap_err(); + let msg = err.to_string(); + assert!(msg.contains("Multiple repositories")); + assert!(msg.contains("usedetail/cli")); + assert!(msg.contains("acme/cli")); + } + + #[test] + fn match_empty_repo_list() { + let err = match_repo_by_name("cli", &[]).unwrap_err(); + assert!(err.to_string().contains("not found")); + } + + // ── resolve_repo_id_from_repos ────────────────────────────────── + + #[test] + fn resolve_owner_repo_exact_match() { + let repos = sample_repos(); + let id = resolve_repo_id_from_repos(&repos, "usedetail/cli").unwrap(); + assert_eq!(id.to_string(), "repo_1"); + } + + #[test] + fn resolve_owner_repo_not_found_has_access_hint() { + let repos = sample_repos(); + let err = resolve_repo_id_from_repos(&repos, "usedetail/missing").unwrap_err(); + assert!(err + .to_string() + .contains("Make sure you have access to this repository")); + } + + #[test] + fn resolve_owner_repo_invalid_format_rejected() { + let repos = sample_repos(); + let err = resolve_repo_id_from_repos(&repos, "usedetail/cli/extra").unwrap_err(); + assert!(err.to_string().contains("Invalid repository format")); + } + + #[test] + fn resolve_bare_repo_name_unique_match() { + let repos = sample_repos(); + let id = resolve_repo_id_from_repos(&repos, "web").unwrap(); + assert_eq!(id.to_string(), "repo_3"); + } + + #[test] + fn resolve_bare_repo_name_ambiguous_returns_error() { + let repos = sample_repos(); + let err = resolve_repo_id_from_repos(&repos, "cli").unwrap_err(); + assert!(err.to_string().contains("Multiple repositories")); + } +}