From 134f7efeb3384d27a25f96314a59f428fbdab92a Mon Sep 17 00:00:00 2001 From: Perry Hertler Date: Wed, 20 Aug 2025 17:18:14 -0500 Subject: [PATCH 01/12] stubbing out teams_for_files_from_codeowners --- src/ownership.rs | 7 +- src/ownership/codeowners_file_parser.rs | 75 ++---- src/runner.rs | 300 +++++++++++++++++++----- 3 files changed, 264 insertions(+), 118 deletions(-) diff --git a/src/ownership.rs b/src/ownership.rs index 8f109ef..ceda129 100644 --- a/src/ownership.rs +++ b/src/ownership.rs @@ -9,12 +9,11 @@ use std::{ }; use tracing::{info, instrument}; -pub(crate) mod codeowners_file_parser; -pub(crate) mod codeowners_query; mod file_generator; mod file_owner_finder; -pub mod file_owner_resolver; +pub mod for_file_fast; pub(crate) mod mapper; +pub(crate) mod codeowners_file_parser; mod validator; use crate::{ @@ -25,9 +24,9 @@ use crate::{ pub use validator::Errors as ValidatorErrors; use self::{ - codeowners_file_parser::parse_for_team, file_generator::FileGenerator, mapper::{JavascriptPackageMapper, Mapper, RubyPackageMapper, TeamFileMapper, TeamGemMapper, TeamGlobMapper, TeamYmlMapper}, + codeowners_file_parser::parse_for_team, validator::Validator, }; diff --git a/src/ownership/codeowners_file_parser.rs b/src/ownership/codeowners_file_parser.rs index 75ab6c7..d7d696d 100644 --- a/src/ownership/codeowners_file_parser.rs +++ b/src/ownership/codeowners_file_parser.rs @@ -4,7 +4,6 @@ use crate::{ }; use fast_glob::glob_match; use memoize::memoize; -use rayon::prelude::*; use regex::Regex; use std::{ collections::HashMap, @@ -23,57 +22,33 @@ pub struct Parser { } impl Parser { - pub fn teams_from_files_paths(&self, file_paths: &[PathBuf]) -> Result>, Box> { - let file_inputs: Vec<(String, String)> = file_paths - .iter() - .map(|path| { - let file_path_str = path - .to_str() - .ok_or(IoError::new(std::io::ErrorKind::InvalidInput, "Invalid file path"))?; - let original = file_path_str.to_string(); - let prefixed = if file_path_str.starts_with('/') { - original.clone() - } else { - format!("/{}", file_path_str) - }; - Ok((original, prefixed)) - }) - .collect::, IoError>>()?; - - if file_inputs.is_empty() { - return Ok(HashMap::new()); - } - - let codeowners_entries: Vec<(String, String)> = - build_codeowners_lines_in_priority(self.codeowners_file_path.to_string_lossy().into_owned()) - .iter() - .map(|line| { - line.split_once(' ') - .map(|(glob, team_name)| (glob.to_string(), team_name.to_string())) - .ok_or_else(|| IoError::new(std::io::ErrorKind::InvalidInput, "Invalid line")) - }) - .collect::>() - .map_err(|e| Box::new(e) as Box)?; - - let teams_by_name = teams_by_github_team_name(self.absolute_team_files_globs()); - - let result: HashMap> = file_inputs - .par_iter() - .map(|(key, prefixed)| { - let team = codeowners_entries - .iter() - .find(|(glob, _)| glob_match(glob, prefixed)) - .and_then(|(_, team_name)| teams_by_name.get(team_name).cloned()); - (key.clone(), team) - }) - .collect(); - - Ok(result) + pub fn teams_from_files_paths(&self, file_paths: &[PathBuf]) -> Result, Box> { + todo!() } - + pub fn team_from_file_path(&self, file_path: &Path) -> Result, Box> { - let teams = self.teams_from_files_paths(&[file_path.to_path_buf()])?; - Ok(teams.get(file_path.to_string_lossy().into_owned().as_str()).cloned().flatten()) + let file_path_str = file_path + .to_str() + .ok_or(IoError::new(std::io::ErrorKind::InvalidInput, "Invalid file path"))?; + let slash_prefixed = if file_path_str.starts_with("/") { + file_path_str.to_string() + } else { + format!("/{}", file_path_str) + }; + + let codeowners_lines_in_priorty = build_codeowners_lines_in_priority(self.codeowners_file_path.to_string_lossy().into_owned()); + for line in codeowners_lines_in_priorty { + let (glob, team_name) = line + .split_once(' ') + .ok_or(IoError::new(std::io::ErrorKind::InvalidInput, "Invalid line"))?; + if glob_match(glob, &slash_prefixed) { + let tbn = teams_by_github_team_name(self.absolute_team_files_globs()); + let team: Option = tbn.get(team_name.to_string().as_str()).cloned(); + return Ok(team); + } + } + + Ok(None) } fn absolute_team_files_globs(&self) -> Vec { diff --git a/src/runner.rs b/src/runner.rs index 0aef0c3..3e78699 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -1,33 +1,92 @@ -use std::{path::Path, process::Command}; +use core::fmt; +use std::{ + collections::HashMap, + fs::File, + path::{Path, PathBuf}, + process::Command, +}; -use error_stack::{Result, ResultExt}; +use error_stack::{Context, Result, ResultExt}; +use serde::{Deserialize, Serialize}; use crate::{ cache::{Cache, Caching, file::GlobalCache, noop::NoopCache}, config::Config, ownership::{FileOwner, Ownership}, + project::Team, project_builder::ProjectBuilder, }; -mod types; -pub use self::types::{Error, RunConfig, RunResult}; -mod api; -pub use self::api::*; +#[derive(Debug, Default, Serialize, Deserialize)] +pub struct RunResult { + pub validation_errors: Vec, + pub io_errors: Vec, + pub info_messages: Vec, +} +#[derive(Debug, Clone)] +pub struct RunConfig { + pub project_root: PathBuf, + pub codeowners_file_path: PathBuf, + pub config_path: PathBuf, + pub no_cache: bool, +} pub struct Runner { run_config: RunConfig, ownership: Ownership, cache: Cache, - config: Config, +} + +pub fn for_file(run_config: &RunConfig, file_path: &str, from_codeowners: bool) -> RunResult { + if from_codeowners { + return for_file_codeowners_only(run_config, file_path); + } + for_file_optimized(run_config, file_path) +} + +pub fn file_owner_for_file(run_config: &RunConfig, file_path: &str) -> Result, Error> { + let config = config_from_path(&run_config.config_path)?; + use crate::ownership::for_file_fast::find_file_owners; + let owners = find_file_owners(&run_config.project_root, &config, std::path::Path::new(file_path)).map_err(Error::Io)?; + Ok(owners.first().cloned()) +} + +pub fn team_for_file(run_config: &RunConfig, file_path: &str) -> Result, Error> { + let owner = file_owner_for_file(run_config, file_path)?; + Ok(owner.map(|fo| fo.team.clone())) } pub fn version() -> String { env!("CARGO_PKG_VERSION").to_string() } +pub fn for_team(run_config: &RunConfig, team_name: &str) -> RunResult { + run_with_runner(run_config, |runner| runner.for_team(team_name)) +} + +pub fn validate(run_config: &RunConfig, _file_paths: Vec) -> RunResult { + run_with_runner(run_config, |runner| runner.validate()) +} + +pub fn generate(run_config: &RunConfig, git_stage: bool) -> RunResult { + run_with_runner(run_config, |runner| runner.generate(git_stage)) +} + +pub fn generate_and_validate(run_config: &RunConfig, _file_paths: Vec, git_stage: bool) -> RunResult { + run_with_runner(run_config, |runner| runner.generate_and_validate(git_stage)) +} + +pub fn delete_cache(run_config: &RunConfig) -> RunResult { + run_with_runner(run_config, |runner| runner.delete_cache()) +} + +pub fn crosscheck_owners(run_config: &RunConfig) -> RunResult { + run_with_runner(run_config, |runner| runner.crosscheck_owners()) +} + pub type Runnable = fn(Runner) -> RunResult; -pub fn run(run_config: &RunConfig, runnable: F) -> RunResult +pub fn run_with_runner(run_config: &RunConfig, runnable: F) -> RunResult where F: FnOnce(Runner) -> RunResult, { @@ -43,12 +102,35 @@ where runnable(runner) } -pub(crate) fn config_from_path(path: &Path) -> Result { - match crate::config::Config::load_from_path(path) { - Ok(c) => Ok(c), - Err(msg) => Err(error_stack::Report::new(Error::Io(msg))), +impl RunResult { + pub fn has_errors(&self) -> bool { + !self.validation_errors.is_empty() || !self.io_errors.is_empty() } } + +#[derive(Debug)] +pub enum Error { + Io(String), + ValidationFailed, +} + +impl Context for Error {} +impl fmt::Display for Error { + fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Error::Io(msg) => fmt.write_str(msg), + Error::ValidationFailed => fmt.write_str("Error::ValidationFailed"), + } + } +} + +pub(crate) fn config_from_path(path: &PathBuf) -> Result { + let config_file = File::open(path) + .change_context(Error::Io(format!("Can't open config file: {}", &path.to_string_lossy()))) + .attach_printable(format!("Can't open config file: {}", &path.to_string_lossy()))?; + + serde_yaml::from_reader(config_file).change_context(Error::Io(format!("Can't parse config file: {}", &path.to_string_lossy()))) +} impl Runner { pub fn new(run_config: &RunConfig) -> Result { let config = config_from_path(&run_config.config_path)?; @@ -86,7 +168,6 @@ impl Runner { run_config: run_config.clone(), ownership, cache, - config, }) } @@ -174,75 +255,166 @@ impl Runner { pub fn crosscheck_owners(&self) -> RunResult { crate::crosscheck::crosscheck_owners(&self.run_config, &self.cache) } +} - pub fn owners_for_file(&self, file_path: &str) -> Result, Error> { - use crate::ownership::file_owner_resolver::find_file_owners; - let owners = find_file_owners(&self.run_config.project_root, &self.config, std::path::Path::new(file_path)).map_err(Error::Io)?; - Ok(owners) +fn for_file_codeowners_only(run_config: &RunConfig, file_path: &str) -> RunResult { + match team_for_file_from_codeowners(run_config, file_path) { + Ok(Some(team)) => { + let relative_team_path = team + .path + .strip_prefix(&run_config.project_root) + .unwrap_or(team.path.as_path()) + .to_string_lossy() + .to_string(); + RunResult { + info_messages: vec![format!( + "Team: {}\nGithub Team: {}\nTeam YML: {}\nDescription:\n- Owner inferred from codeowners file", + team.name, team.github_team, relative_team_path + )], + ..Default::default() + } + } + Ok(None) => RunResult::default(), + Err(err) => RunResult { + io_errors: vec![err.to_string()], + ..Default::default() + }, } +} - pub fn for_file_optimized(&self, file_path: &str) -> RunResult { - let file_owners = match self.owners_for_file(file_path) { - Ok(v) => v, - Err(err) => { - return RunResult { - io_errors: vec![err.to_string()], - ..Default::default() - }; - } - }; +// For an array of file paths, return a map of file path to its owning team +pub fn teams_for_files_from_codeowners(run_config: &RunConfig, file_paths: &[String]) -> Result, Error> { + let relative_file_paths: Vec = file_paths + .iter() + .map(|path| Path::new(path).strip_prefix(&run_config.project_root).unwrap_or(Path::new(path))) + .map(|path| path.to_path_buf()) + .collect(); - let info_messages: Vec = match file_owners.len() { - 0 => vec![format!("{}", FileOwner::default())], - 1 => vec![format!("{}", file_owners[0])], - _ => { - let mut error_messages = vec!["Error: file is owned by multiple teams!".to_string()]; - for file_owner in file_owners { - error_messages.push(format!("\n{}", file_owner)); - } - return RunResult { - validation_errors: error_messages, - ..Default::default() - }; - } - }; - RunResult { - info_messages, - ..Default::default() + let parser = build_codeowners_parser(run_config)?; + Ok(parser + .teams_from_files_paths(&relative_file_paths) + .map_err(|e| Error::Io(e.to_string()))?) +} + +fn build_codeowners_parser(run_config: &RunConfig) -> Result { + let config = config_from_path(&run_config.config_path)?; + Ok(crate::ownership::codeowners_file_parser::Parser { + codeowners_file_path: run_config.codeowners_file_path.clone(), + project_root: run_config.project_root.clone(), + team_file_globs: config.team_file_glob.clone(), + }) +} + +pub fn team_for_file_from_codeowners(run_config: &RunConfig, file_path: &str) -> Result, Error> { + let relative_file_path = Path::new(file_path) + .strip_prefix(&run_config.project_root) + .unwrap_or(Path::new(file_path)); + + let parser = build_codeowners_parser(run_config)?; + Ok(parser + .team_from_file_path(Path::new(relative_file_path)) + .map_err(|e| Error::Io(e.to_string()))?) +} + +fn for_file_optimized(run_config: &RunConfig, file_path: &str) -> RunResult { + let config = match config_from_path(&run_config.config_path) { + Ok(c) => c, + Err(err) => { + return RunResult { + io_errors: vec![err.to_string()], + ..Default::default() + }; } - } + }; - pub fn for_file_codeowners_only(&self, file_path: &str) -> RunResult { - match team_for_file_from_codeowners(&self.run_config, file_path) { - Ok(Some(team)) => { - let relative_team_path = crate::path_utils::relative_to(&self.run_config.project_root, team.path.as_path()) - .to_string_lossy() - .to_string(); - RunResult { - info_messages: vec![format!( - "Team: {}\nGithub Team: {}\nTeam YML: {}\nDescription:\n- Owner inferred from codeowners file", - team.name, team.github_team, relative_team_path - )], - ..Default::default() - } + use crate::ownership::for_file_fast::find_file_owners; + let file_owners = match find_file_owners(&run_config.project_root, &config, std::path::Path::new(file_path)) { + Ok(v) => v, + Err(err) => { + return RunResult { + io_errors: vec![err], + ..Default::default() + }; + } + }; + + let info_messages: Vec = match file_owners.len() { + 0 => vec![format!("{}", FileOwner::default())], + 1 => vec![format!("{}", file_owners[0])], + _ => { + let mut error_messages = vec!["Error: file is owned by multiple teams!".to_string()]; + for file_owner in file_owners { + error_messages.push(format!("\n{}", file_owner)); } - Ok(None) => RunResult::default(), - Err(err) => RunResult { - io_errors: vec![err.to_string()], + return RunResult { + validation_errors: error_messages, ..Default::default() - }, + }; } + }; + RunResult { + info_messages, + ..Default::default() } } -// removed free functions for for_file_* variants in favor of Runner methods - #[cfg(test)] mod tests { + use tempfile::tempdir; + + use crate::{common_test, ownership::mapper::Source}; + use super::*; #[test] fn test_version() { assert_eq!(version(), env!("CARGO_PKG_VERSION").to_string()); } + fn write_file(temp_dir: &Path, file_path: &str, content: &str) { + let file_path = temp_dir.join(file_path); + let _ = std::fs::create_dir_all(file_path.parent().unwrap()); + std::fs::write(file_path, content).unwrap(); + } + + #[test] + fn test_file_owners_for_file() { + let temp_dir = tempdir().unwrap(); + write_file( + temp_dir.path(), + "config/code_ownership.yml", + common_test::tests::DEFAULT_CODE_OWNERSHIP_YML, + ); + ["a", "b", "c"].iter().for_each(|name| { + let team_yml = format!("name: {}\ngithub:\n team: \"@{}\"\n members:\n - {}member\n", name, name, name); + write_file(temp_dir.path(), &format!("config/teams/{}.yml", name), &team_yml); + }); + write_file( + temp_dir.path(), + "app/consumers/deep/nesting/nestdir/deep_file.rb", + "# @team b\nclass DeepFile end;", + ); + + let run_config = RunConfig { + project_root: temp_dir.path().to_path_buf(), + codeowners_file_path: temp_dir.path().join(".github/CODEOWNERS").to_path_buf(), + config_path: temp_dir.path().join("config/code_ownership.yml").to_path_buf(), + no_cache: false, + }; + + let file_owner = file_owner_for_file(&run_config, "app/consumers/deep/nesting/nestdir/deep_file.rb") + .unwrap() + .unwrap(); + assert_eq!(file_owner.team.name, "b"); + assert_eq!(file_owner.team.github_team, "@b"); + assert!(file_owner.team.path.to_string_lossy().ends_with("config/teams/b.yml")); + assert_eq!(file_owner.sources.len(), 1); + assert_eq!(file_owner.sources, vec![Source::AnnotatedFile]); + + let team = team_for_file(&run_config, "app/consumers/deep/nesting/nestdir/deep_file.rb") + .unwrap() + .unwrap(); + assert_eq!(team.name, "b"); + assert_eq!(team.github_team, "@b"); + assert!(team.path.to_string_lossy().ends_with("config/teams/b.yml")); + } } From a5fc1f3ed4c6bc1458bfbbfbe9a83ee147070668 Mon Sep 17 00:00:00 2001 From: Perry Hertler Date: Wed, 20 Aug 2025 18:58:56 -0500 Subject: [PATCH 02/12] first try..slow --- src/ownership/codeowners_file_parser.rs | 86 ++++++++++++++++++------- src/runner.rs | 42 ++++++++++++ 2 files changed, 104 insertions(+), 24 deletions(-) diff --git a/src/ownership/codeowners_file_parser.rs b/src/ownership/codeowners_file_parser.rs index d7d696d..b2d0263 100644 --- a/src/ownership/codeowners_file_parser.rs +++ b/src/ownership/codeowners_file_parser.rs @@ -5,6 +5,7 @@ use crate::{ use fast_glob::glob_match; use memoize::memoize; use regex::Regex; +use rayon::prelude::*; use std::{ collections::HashMap, error::Error, @@ -23,32 +24,69 @@ pub struct Parser { impl Parser { pub fn teams_from_files_paths(&self, file_paths: &[PathBuf]) -> Result, Box> { - todo!() - } - - pub fn team_from_file_path(&self, file_path: &Path) -> Result, Box> { - let file_path_str = file_path - .to_str() - .ok_or(IoError::new(std::io::ErrorKind::InvalidInput, "Invalid file path"))?; - let slash_prefixed = if file_path_str.starts_with("/") { - file_path_str.to_string() - } else { - format!("/{}", file_path_str) - }; - - let codeowners_lines_in_priorty = build_codeowners_lines_in_priority(self.codeowners_file_path.to_string_lossy().into_owned()); - for line in codeowners_lines_in_priorty { - let (glob, team_name) = line - .split_once(' ') - .ok_or(IoError::new(std::io::ErrorKind::InvalidInput, "Invalid line"))?; - if glob_match(glob, &slash_prefixed) { - let tbn = teams_by_github_team_name(self.absolute_team_files_globs()); - let team: Option = tbn.get(team_name.to_string().as_str()).cloned(); - return Ok(team); - } + let mut file_inputs: Vec<(String, String)> = Vec::with_capacity(file_paths.len()); + for path in file_paths { + let file_path_str = path + .to_str() + .ok_or(IoError::new(std::io::ErrorKind::InvalidInput, "Invalid file path"))?; + let key = file_path_str.to_string(); + let slash_prefixed = if file_path_str.starts_with('/') { + file_path_str.to_string() + } else { + format!("/{}", file_path_str) + }; + file_inputs.push((key, slash_prefixed)); + } + + if file_inputs.is_empty() { + return Ok(HashMap::new()); + } + + let codeowners_lines_in_priority = build_codeowners_lines_in_priority( + self.codeowners_file_path.to_string_lossy().into_owned(), + ); + // Pre-parse lines once to avoid repeated split and to handle malformed lines early + let codeowners_entries: Vec<(String, String)> = codeowners_lines_in_priority + .iter() + .map(|line| { + line + .split_once(' ') + .map(|(glob, team_name)| (glob.to_string(), team_name.to_string())) + .ok_or_else(|| IoError::new(std::io::ErrorKind::InvalidInput, "Invalid line")) + }) + .collect::>() + .map_err(|e| Box::new(e) as Box)?; + let teams_by_name = teams_by_github_team_name(self.absolute_team_files_globs()); + + // Parallelize across files: for each file, scan lines in priority order + let result_pairs: Vec<(String, Team)> = file_inputs + .par_iter() + .filter_map(|(key, prefixed)| { + for (glob, team_name) in &codeowners_entries { + if glob_match(glob, prefixed) { + // Stop at first match (highest priority). If team missing, treat as unowned. + if let Some(team) = teams_by_name.get(team_name) { + return Some((key.clone(), team.clone())); + } else { + return None; + } + } + } + None + }) + .collect(); + + let mut result: HashMap = HashMap::with_capacity(result_pairs.len()); + for (k, t) in result_pairs { + result.insert(k, t); } - Ok(None) + Ok(result) + } + + pub fn team_from_file_path(&self, file_path: &Path) -> Result, Box> { + let teams = self.teams_from_files_paths(&[file_path.to_path_buf()])?; + Ok(teams.get(file_path.to_string_lossy().into_owned().as_str()).cloned()) } fn absolute_team_files_globs(&self) -> Vec { diff --git a/src/runner.rs b/src/runner.rs index 3e78699..0cceede 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -363,6 +363,7 @@ mod tests { use tempfile::tempdir; use crate::{common_test, ownership::mapper::Source}; + use ignore::{DirEntry, WalkBuilder, WalkParallel, WalkState}; use super::*; @@ -417,4 +418,45 @@ mod tests { assert_eq!(team.github_team, "@b"); assert!(team.path.to_string_lossy().ends_with("config/teams/b.yml")); } + + #[test] + fn test_teams_for_files_from_codeowners() { + let project_root = Path::new("/Users/perryhertler/workspace/zenpayroll"); + let codeowners_file_path = project_root.join(".github/CODEOWNERS"); + let config_path = project_root.join("config/code_ownership.yml"); + let run_config = RunConfig { + project_root: project_root.to_path_buf(), + codeowners_file_path: codeowners_file_path.to_path_buf(), + config_path: config_path.to_path_buf(), + no_cache: false, + }; + + // Collect all files in packs and frontend directories recursively + let mut file_paths = Vec::new(); + for dir in ["packs", "frontend"] { + let dir_path = project_root.join(dir); + if dir_path.exists() && dir_path.is_dir() { + for entry in WalkBuilder::new(&dir_path) + .filter_entry(|e| { + let name = e.file_name().to_str().unwrap_or(""); + !(name == "node_modules" || name == "dist" || name == ".git") + }) + .build() + .filter_map(|e| e.ok()) + .filter(|e| e.file_type().map(|t| t.is_file()).unwrap_or(false)) + .filter_map(|e| e.path().strip_prefix(project_root).ok().map(|p| p.to_string_lossy().to_string())) + { + file_paths.push(entry); + } + } + } + + let start_time = std::time::Instant::now(); + let teams = teams_for_files_from_codeowners(&run_config, &file_paths).unwrap(); + let end_time = std::time::Instant::now(); + println!("Time taken: {:?}", end_time.duration_since(start_time)); + println!("Teams: {:?}", teams); + assert_eq!(teams.len(), 1); + + } } From 21a3d298551616e7c00d86fdebe044a6554e5669 Mon Sep 17 00:00:00 2001 From: Perry Hertler Date: Wed, 20 Aug 2025 19:37:04 -0500 Subject: [PATCH 03/12] much faster --- src/ownership.rs | 4 +- src/ownership/codeowners_file_parser.rs | 79 +++++++++++-------------- src/runner.rs | 69 +++++++++------------ 3 files changed, 64 insertions(+), 88 deletions(-) diff --git a/src/ownership.rs b/src/ownership.rs index ceda129..88516f8 100644 --- a/src/ownership.rs +++ b/src/ownership.rs @@ -9,11 +9,11 @@ use std::{ }; use tracing::{info, instrument}; +pub(crate) mod codeowners_file_parser; mod file_generator; mod file_owner_finder; pub mod for_file_fast; pub(crate) mod mapper; -pub(crate) mod codeowners_file_parser; mod validator; use crate::{ @@ -24,9 +24,9 @@ use crate::{ pub use validator::Errors as ValidatorErrors; use self::{ + codeowners_file_parser::parse_for_team, file_generator::FileGenerator, mapper::{JavascriptPackageMapper, Mapper, RubyPackageMapper, TeamFileMapper, TeamGemMapper, TeamGlobMapper, TeamYmlMapper}, - codeowners_file_parser::parse_for_team, validator::Validator, }; diff --git a/src/ownership/codeowners_file_parser.rs b/src/ownership/codeowners_file_parser.rs index b2d0263..f2872cf 100644 --- a/src/ownership/codeowners_file_parser.rs +++ b/src/ownership/codeowners_file_parser.rs @@ -4,8 +4,8 @@ use crate::{ }; use fast_glob::glob_match; use memoize::memoize; -use regex::Regex; use rayon::prelude::*; +use regex::Regex; use std::{ collections::HashMap, error::Error, @@ -24,63 +24,50 @@ pub struct Parser { impl Parser { pub fn teams_from_files_paths(&self, file_paths: &[PathBuf]) -> Result, Box> { - let mut file_inputs: Vec<(String, String)> = Vec::with_capacity(file_paths.len()); - for path in file_paths { - let file_path_str = path - .to_str() - .ok_or(IoError::new(std::io::ErrorKind::InvalidInput, "Invalid file path"))?; - let key = file_path_str.to_string(); - let slash_prefixed = if file_path_str.starts_with('/') { - file_path_str.to_string() - } else { - format!("/{}", file_path_str) - }; - file_inputs.push((key, slash_prefixed)); - } + let file_inputs: Vec<(String, String)> = file_paths + .iter() + .map(|path| { + let file_path_str = path + .to_str() + .ok_or(IoError::new(std::io::ErrorKind::InvalidInput, "Invalid file path"))?; + let original = file_path_str.to_string(); + let prefixed = if file_path_str.starts_with('/') { + original.clone() + } else { + format!("/{}", file_path_str) + }; + Ok((original, prefixed)) + }) + .collect::, IoError>>()?; if file_inputs.is_empty() { return Ok(HashMap::new()); } - let codeowners_lines_in_priority = build_codeowners_lines_in_priority( - self.codeowners_file_path.to_string_lossy().into_owned(), - ); - // Pre-parse lines once to avoid repeated split and to handle malformed lines early - let codeowners_entries: Vec<(String, String)> = codeowners_lines_in_priority - .iter() - .map(|line| { - line - .split_once(' ') - .map(|(glob, team_name)| (glob.to_string(), team_name.to_string())) - .ok_or_else(|| IoError::new(std::io::ErrorKind::InvalidInput, "Invalid line")) - }) - .collect::>() - .map_err(|e| Box::new(e) as Box)?; + let codeowners_entries: Vec<(String, String)> = + build_codeowners_lines_in_priority(self.codeowners_file_path.to_string_lossy().into_owned()) + .iter() + .map(|line| { + line.split_once(' ') + .map(|(glob, team_name)| (glob.to_string(), team_name.to_string())) + .ok_or_else(|| IoError::new(std::io::ErrorKind::InvalidInput, "Invalid line")) + }) + .collect::>() + .map_err(|e| Box::new(e) as Box)?; + let teams_by_name = teams_by_github_team_name(self.absolute_team_files_globs()); - // Parallelize across files: for each file, scan lines in priority order - let result_pairs: Vec<(String, Team)> = file_inputs + let result: HashMap = file_inputs .par_iter() .filter_map(|(key, prefixed)| { - for (glob, team_name) in &codeowners_entries { - if glob_match(glob, prefixed) { - // Stop at first match (highest priority). If team missing, treat as unowned. - if let Some(team) = teams_by_name.get(team_name) { - return Some((key.clone(), team.clone())); - } else { - return None; - } - } - } - None + codeowners_entries + .iter() + .find(|(glob, _)| glob_match(glob, prefixed)) + .and_then(|(_, team_name)| teams_by_name.get(team_name)) + .map(|team| (key.clone(), team.clone())) }) .collect(); - let mut result: HashMap = HashMap::with_capacity(result_pairs.len()); - for (k, t) in result_pairs { - result.insert(k, t); - } - Ok(result) } diff --git a/src/runner.rs b/src/runner.rs index 0cceede..61eef4f 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -362,10 +362,8 @@ fn for_file_optimized(run_config: &RunConfig, file_path: &str) -> RunResult { mod tests { use tempfile::tempdir; - use crate::{common_test, ownership::mapper::Source}; - use ignore::{DirEntry, WalkBuilder, WalkParallel, WalkState}; - use super::*; + use crate::{common_test, ownership::mapper::Source}; #[test] fn test_version() { @@ -421,42 +419,33 @@ mod tests { #[test] fn test_teams_for_files_from_codeowners() { - let project_root = Path::new("/Users/perryhertler/workspace/zenpayroll"); - let codeowners_file_path = project_root.join(".github/CODEOWNERS"); - let config_path = project_root.join("config/code_ownership.yml"); - let run_config = RunConfig { - project_root: project_root.to_path_buf(), - codeowners_file_path: codeowners_file_path.to_path_buf(), - config_path: config_path.to_path_buf(), - no_cache: false, - }; - - // Collect all files in packs and frontend directories recursively - let mut file_paths = Vec::new(); - for dir in ["packs", "frontend"] { - let dir_path = project_root.join(dir); - if dir_path.exists() && dir_path.is_dir() { - for entry in WalkBuilder::new(&dir_path) - .filter_entry(|e| { - let name = e.file_name().to_str().unwrap_or(""); - !(name == "node_modules" || name == "dist" || name == ".git") - }) - .build() - .filter_map(|e| e.ok()) - .filter(|e| e.file_type().map(|t| t.is_file()).unwrap_or(false)) - .filter_map(|e| e.path().strip_prefix(project_root).ok().map(|p| p.to_string_lossy().to_string())) - { - file_paths.push(entry); - } - } - } - - let start_time = std::time::Instant::now(); - let teams = teams_for_files_from_codeowners(&run_config, &file_paths).unwrap(); - let end_time = std::time::Instant::now(); - println!("Time taken: {:?}", end_time.duration_since(start_time)); - println!("Teams: {:?}", teams); - assert_eq!(teams.len(), 1); - + let project_root = Path::new("tests/fixtures/valid_project"); + let file_paths = [ + "javascript/packages/items/item.ts", + "config/teams/payroll.yml", + "ruby/app/models/bank_account.rb", + "made/up/file.rb", + "ruby/ignored_files/git_ignored.rb", + ]; + let run_config = RunConfig { + project_root: project_root.to_path_buf(), + codeowners_file_path: project_root.join(".github/CODEOWNERS").to_path_buf(), + config_path: project_root.join("config/code_ownership.yml").to_path_buf(), + no_cache: false, + }; + let teams = + teams_for_files_from_codeowners(&run_config, &file_paths.iter().map(|s| s.to_string()).collect::>()).unwrap(); + assert_eq!(teams.len(), 3); + assert_eq!( + teams.get("javascript/packages/items/item.ts").map(|t| t.name.as_str()), + Some("Payroll") + ); + assert_eq!(teams.get("config/teams/payroll.yml").map(|t| t.name.as_str()), Some("Payroll")); + assert_eq!( + teams.get("ruby/app/models/bank_account.rb").map(|t| t.name.as_str()), + Some("Payments") + ); + assert_eq!(teams.get("made/up/file.rb").map(|t| t.name.as_str()), None); + assert_eq!(teams.get("ruby/ignored_files/git_ignored.rb").map(|t| t.name.as_str()), None); } } From 00c88d66af7010d6e68582bc3330f67a7b063793 Mon Sep 17 00:00:00 2001 From: Perry Hertler Date: Wed, 20 Aug 2025 19:53:27 -0500 Subject: [PATCH 04/12] bumping crates --- src/ownership/codeowners_file_parser.rs | 14 ++++----- src/runner.rs | 40 ++++++++++++++++++++----- 2 files changed, 40 insertions(+), 14 deletions(-) diff --git a/src/ownership/codeowners_file_parser.rs b/src/ownership/codeowners_file_parser.rs index f2872cf..75ab6c7 100644 --- a/src/ownership/codeowners_file_parser.rs +++ b/src/ownership/codeowners_file_parser.rs @@ -23,7 +23,7 @@ pub struct Parser { } impl Parser { - pub fn teams_from_files_paths(&self, file_paths: &[PathBuf]) -> Result, Box> { + pub fn teams_from_files_paths(&self, file_paths: &[PathBuf]) -> Result>, Box> { let file_inputs: Vec<(String, String)> = file_paths .iter() .map(|path| { @@ -57,14 +57,14 @@ impl Parser { let teams_by_name = teams_by_github_team_name(self.absolute_team_files_globs()); - let result: HashMap = file_inputs + let result: HashMap> = file_inputs .par_iter() - .filter_map(|(key, prefixed)| { - codeowners_entries + .map(|(key, prefixed)| { + let team = codeowners_entries .iter() .find(|(glob, _)| glob_match(glob, prefixed)) - .and_then(|(_, team_name)| teams_by_name.get(team_name)) - .map(|team| (key.clone(), team.clone())) + .and_then(|(_, team_name)| teams_by_name.get(team_name).cloned()); + (key.clone(), team) }) .collect(); @@ -73,7 +73,7 @@ impl Parser { pub fn team_from_file_path(&self, file_path: &Path) -> Result, Box> { let teams = self.teams_from_files_paths(&[file_path.to_path_buf()])?; - Ok(teams.get(file_path.to_string_lossy().into_owned().as_str()).cloned()) + Ok(teams.get(file_path.to_string_lossy().into_owned().as_str()).cloned().flatten()) } fn absolute_team_files_globs(&self) -> Vec { diff --git a/src/runner.rs b/src/runner.rs index 61eef4f..114ab04 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -283,7 +283,7 @@ fn for_file_codeowners_only(run_config: &RunConfig, file_path: &str) -> RunResul } // For an array of file paths, return a map of file path to its owning team -pub fn teams_for_files_from_codeowners(run_config: &RunConfig, file_paths: &[String]) -> Result, Error> { +pub fn teams_for_files_from_codeowners(run_config: &RunConfig, file_paths: &[String]) -> Result>, Error> { let relative_file_paths: Vec = file_paths .iter() .map(|path| Path::new(path).strip_prefix(&run_config.project_root).unwrap_or(Path::new(path))) @@ -435,17 +435,43 @@ mod tests { }; let teams = teams_for_files_from_codeowners(&run_config, &file_paths.iter().map(|s| s.to_string()).collect::>()).unwrap(); - assert_eq!(teams.len(), 3); + assert_eq!(teams.len(), 5); assert_eq!( - teams.get("javascript/packages/items/item.ts").map(|t| t.name.as_str()), + teams + .get("javascript/packages/items/item.ts") + .unwrap() + .as_ref() + .map(|t| t.name.as_str()), Some("Payroll") ); - assert_eq!(teams.get("config/teams/payroll.yml").map(|t| t.name.as_str()), Some("Payroll")); assert_eq!( - teams.get("ruby/app/models/bank_account.rb").map(|t| t.name.as_str()), + teams.get("config/teams/payroll.yml").unwrap().as_ref().map(|t| t.name.as_str()), + Some("Payroll") + ); + assert_eq!( + teams + .get("ruby/app/models/bank_account.rb") + .unwrap() + .as_ref() + .map(|t| t.name.as_str()), Some("Payments") ); - assert_eq!(teams.get("made/up/file.rb").map(|t| t.name.as_str()), None); - assert_eq!(teams.get("ruby/ignored_files/git_ignored.rb").map(|t| t.name.as_str()), None); + assert_eq!(teams.get("made/up/file.rb").unwrap().as_ref().map(|t| t.name.as_str()), None); + assert_eq!( + teams + .get("ruby/ignored_files/git_ignored.rb") + .unwrap() + .as_ref() + .map(|t| t.name.as_str()), + None + ); + assert_eq!( + teams + .get("ruby/ignored_files/git_ignored.rb") + .unwrap() + .as_ref() + .map(|t| t.name.as_str()), + None + ); } } From 81c80bd5e86f183b97d5ccb22b97fd6a16aa1f2a Mon Sep 17 00:00:00 2001 From: Perry Hertler Date: Thu, 21 Aug 2025 20:34:04 -0500 Subject: [PATCH 05/12] extracting separate concerns from runner --- src/ownership.rs | 1 + src/runner.rs | 253 +++++++++++++---------------------------------- 2 files changed, 69 insertions(+), 185 deletions(-) diff --git a/src/ownership.rs b/src/ownership.rs index 88516f8..5641dfc 100644 --- a/src/ownership.rs +++ b/src/ownership.rs @@ -10,6 +10,7 @@ use std::{ use tracing::{info, instrument}; pub(crate) mod codeowners_file_parser; +pub(crate) mod codeowners_query; mod file_generator; mod file_owner_finder; pub mod for_file_fast; diff --git a/src/runner.rs b/src/runner.rs index 114ab04..a27a7c3 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -1,92 +1,33 @@ -use core::fmt; -use std::{ - collections::HashMap, - fs::File, - path::{Path, PathBuf}, - process::Command, -}; +use std::{path::Path, process::Command}; -use error_stack::{Context, Result, ResultExt}; -use serde::{Deserialize, Serialize}; +use error_stack::{Result, ResultExt}; use crate::{ cache::{Cache, Caching, file::GlobalCache, noop::NoopCache}, config::Config, ownership::{FileOwner, Ownership}, - project::Team, project_builder::ProjectBuilder, }; -#[derive(Debug, Default, Serialize, Deserialize)] -pub struct RunResult { - pub validation_errors: Vec, - pub io_errors: Vec, - pub info_messages: Vec, -} -#[derive(Debug, Clone)] -pub struct RunConfig { - pub project_root: PathBuf, - pub codeowners_file_path: PathBuf, - pub config_path: PathBuf, - pub no_cache: bool, -} +mod types; +pub use self::types::{Error, RunConfig, RunResult}; +mod api; +pub use self::api::*; pub struct Runner { run_config: RunConfig, ownership: Ownership, cache: Cache, -} - -pub fn for_file(run_config: &RunConfig, file_path: &str, from_codeowners: bool) -> RunResult { - if from_codeowners { - return for_file_codeowners_only(run_config, file_path); - } - for_file_optimized(run_config, file_path) -} - -pub fn file_owner_for_file(run_config: &RunConfig, file_path: &str) -> Result, Error> { - let config = config_from_path(&run_config.config_path)?; - use crate::ownership::for_file_fast::find_file_owners; - let owners = find_file_owners(&run_config.project_root, &config, std::path::Path::new(file_path)).map_err(Error::Io)?; - Ok(owners.first().cloned()) -} - -pub fn team_for_file(run_config: &RunConfig, file_path: &str) -> Result, Error> { - let owner = file_owner_for_file(run_config, file_path)?; - Ok(owner.map(|fo| fo.team.clone())) + config: Config, } pub fn version() -> String { env!("CARGO_PKG_VERSION").to_string() } -pub fn for_team(run_config: &RunConfig, team_name: &str) -> RunResult { - run_with_runner(run_config, |runner| runner.for_team(team_name)) -} - -pub fn validate(run_config: &RunConfig, _file_paths: Vec) -> RunResult { - run_with_runner(run_config, |runner| runner.validate()) -} - -pub fn generate(run_config: &RunConfig, git_stage: bool) -> RunResult { - run_with_runner(run_config, |runner| runner.generate(git_stage)) -} - -pub fn generate_and_validate(run_config: &RunConfig, _file_paths: Vec, git_stage: bool) -> RunResult { - run_with_runner(run_config, |runner| runner.generate_and_validate(git_stage)) -} - -pub fn delete_cache(run_config: &RunConfig) -> RunResult { - run_with_runner(run_config, |runner| runner.delete_cache()) -} - -pub fn crosscheck_owners(run_config: &RunConfig) -> RunResult { - run_with_runner(run_config, |runner| runner.crosscheck_owners()) -} - pub type Runnable = fn(Runner) -> RunResult; -pub fn run_with_runner(run_config: &RunConfig, runnable: F) -> RunResult +pub fn run(run_config: &RunConfig, runnable: F) -> RunResult where F: FnOnce(Runner) -> RunResult, { @@ -102,35 +43,12 @@ where runnable(runner) } -impl RunResult { - pub fn has_errors(&self) -> bool { - !self.validation_errors.is_empty() || !self.io_errors.is_empty() - } -} - -#[derive(Debug)] -pub enum Error { - Io(String), - ValidationFailed, -} - -impl Context for Error {} -impl fmt::Display for Error { - fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result { - match self { - Error::Io(msg) => fmt.write_str(msg), - Error::ValidationFailed => fmt.write_str("Error::ValidationFailed"), - } +pub(crate) fn config_from_path(path: &Path) -> Result { + match crate::config::Config::load_from_path(path) { + Ok(c) => Ok(c), + Err(msg) => Err(error_stack::Report::new(Error::Io(msg))), } } - -pub(crate) fn config_from_path(path: &PathBuf) -> Result { - let config_file = File::open(path) - .change_context(Error::Io(format!("Can't open config file: {}", &path.to_string_lossy()))) - .attach_printable(format!("Can't open config file: {}", &path.to_string_lossy()))?; - - serde_yaml::from_reader(config_file).change_context(Error::Io(format!("Can't parse config file: {}", &path.to_string_lossy()))) -} impl Runner { pub fn new(run_config: &RunConfig) -> Result { let config = config_from_path(&run_config.config_path)?; @@ -168,6 +86,7 @@ impl Runner { run_config: run_config.clone(), ownership, cache, + config, }) } @@ -255,111 +174,75 @@ impl Runner { pub fn crosscheck_owners(&self) -> RunResult { crate::crosscheck::crosscheck_owners(&self.run_config, &self.cache) } -} -fn for_file_codeowners_only(run_config: &RunConfig, file_path: &str) -> RunResult { - match team_for_file_from_codeowners(run_config, file_path) { - Ok(Some(team)) => { - let relative_team_path = team - .path - .strip_prefix(&run_config.project_root) - .unwrap_or(team.path.as_path()) - .to_string_lossy() - .to_string(); - RunResult { - info_messages: vec![format!( - "Team: {}\nGithub Team: {}\nTeam YML: {}\nDescription:\n- Owner inferred from codeowners file", - team.name, team.github_team, relative_team_path - )], - ..Default::default() - } - } - Ok(None) => RunResult::default(), - Err(err) => RunResult { - io_errors: vec![err.to_string()], - ..Default::default() - }, + pub fn owners_for_file(&self, file_path: &str) -> Result, Error> { + use crate::ownership::for_file_fast::find_file_owners; + let owners = find_file_owners(&self.run_config.project_root, &self.config, std::path::Path::new(file_path)).map_err(Error::Io)?; + Ok(owners) } -} -// For an array of file paths, return a map of file path to its owning team -pub fn teams_for_files_from_codeowners(run_config: &RunConfig, file_paths: &[String]) -> Result>, Error> { - let relative_file_paths: Vec = file_paths - .iter() - .map(|path| Path::new(path).strip_prefix(&run_config.project_root).unwrap_or(Path::new(path))) - .map(|path| path.to_path_buf()) - .collect(); - - let parser = build_codeowners_parser(run_config)?; - Ok(parser - .teams_from_files_paths(&relative_file_paths) - .map_err(|e| Error::Io(e.to_string()))?) -} - -fn build_codeowners_parser(run_config: &RunConfig) -> Result { - let config = config_from_path(&run_config.config_path)?; - Ok(crate::ownership::codeowners_file_parser::Parser { - codeowners_file_path: run_config.codeowners_file_path.clone(), - project_root: run_config.project_root.clone(), - team_file_globs: config.team_file_glob.clone(), - }) -} - -pub fn team_for_file_from_codeowners(run_config: &RunConfig, file_path: &str) -> Result, Error> { - let relative_file_path = Path::new(file_path) - .strip_prefix(&run_config.project_root) - .unwrap_or(Path::new(file_path)); - - let parser = build_codeowners_parser(run_config)?; - Ok(parser - .team_from_file_path(Path::new(relative_file_path)) - .map_err(|e| Error::Io(e.to_string()))?) -} - -fn for_file_optimized(run_config: &RunConfig, file_path: &str) -> RunResult { - let config = match config_from_path(&run_config.config_path) { - Ok(c) => c, - Err(err) => { - return RunResult { - io_errors: vec![err.to_string()], - ..Default::default() - }; - } - }; + pub fn for_file_optimized(&self, file_path: &str) -> RunResult { + let file_owners = match self.owners_for_file(file_path) { + Ok(v) => v, + Err(err) => { + return RunResult { + io_errors: vec![err.to_string()], + ..Default::default() + }; + } + }; - use crate::ownership::for_file_fast::find_file_owners; - let file_owners = match find_file_owners(&run_config.project_root, &config, std::path::Path::new(file_path)) { - Ok(v) => v, - Err(err) => { - return RunResult { - io_errors: vec![err], - ..Default::default() - }; + let info_messages: Vec = match file_owners.len() { + 0 => vec![format!("{}", FileOwner::default())], + 1 => vec![format!("{}", file_owners[0])], + _ => { + let mut error_messages = vec!["Error: file is owned by multiple teams!".to_string()]; + for file_owner in file_owners { + error_messages.push(format!("\n{}", file_owner)); + } + return RunResult { + validation_errors: error_messages, + ..Default::default() + }; + } + }; + RunResult { + info_messages, + ..Default::default() } - }; + } - let info_messages: Vec = match file_owners.len() { - 0 => vec![format!("{}", FileOwner::default())], - 1 => vec![format!("{}", file_owners[0])], - _ => { - let mut error_messages = vec!["Error: file is owned by multiple teams!".to_string()]; - for file_owner in file_owners { - error_messages.push(format!("\n{}", file_owner)); + pub fn for_file_codeowners_only(&self, file_path: &str) -> RunResult { + match team_for_file_from_codeowners(&self.run_config, file_path) { + Ok(Some(team)) => { + let relative_team_path = team + .path + .strip_prefix(&self.run_config.project_root) + .unwrap_or(team.path.as_path()) + .to_string_lossy() + .to_string(); + RunResult { + info_messages: vec![format!( + "Team: {}\nGithub Team: {}\nTeam YML: {}\nDescription:\n- Owner inferred from codeowners file", + team.name, team.github_team, relative_team_path + )], + ..Default::default() + } } - return RunResult { - validation_errors: error_messages, + Ok(None) => RunResult::default(), + Err(err) => RunResult { + io_errors: vec![err.to_string()], ..Default::default() - }; + }, } - }; - RunResult { - info_messages, - ..Default::default() } } +// removed free functions for for_file_* variants in favor of Runner methods + #[cfg(test)] mod tests { + use std::path::Path; use tempfile::tempdir; use super::*; From c3d1451ef065f619298f2af1b6d829259662a455 Mon Sep 17 00:00:00 2001 From: Perry Hertler Date: Thu, 21 Aug 2025 21:06:45 -0500 Subject: [PATCH 06/12] updating README with usage examples --- src/runner.rs | 109 -------------------------------------------------- 1 file changed, 109 deletions(-) diff --git a/src/runner.rs b/src/runner.rs index a27a7c3..00e7f54 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -242,119 +242,10 @@ impl Runner { #[cfg(test)] mod tests { - use std::path::Path; - use tempfile::tempdir; - use super::*; - use crate::{common_test, ownership::mapper::Source}; #[test] fn test_version() { assert_eq!(version(), env!("CARGO_PKG_VERSION").to_string()); } - fn write_file(temp_dir: &Path, file_path: &str, content: &str) { - let file_path = temp_dir.join(file_path); - let _ = std::fs::create_dir_all(file_path.parent().unwrap()); - std::fs::write(file_path, content).unwrap(); - } - - #[test] - fn test_file_owners_for_file() { - let temp_dir = tempdir().unwrap(); - write_file( - temp_dir.path(), - "config/code_ownership.yml", - common_test::tests::DEFAULT_CODE_OWNERSHIP_YML, - ); - ["a", "b", "c"].iter().for_each(|name| { - let team_yml = format!("name: {}\ngithub:\n team: \"@{}\"\n members:\n - {}member\n", name, name, name); - write_file(temp_dir.path(), &format!("config/teams/{}.yml", name), &team_yml); - }); - write_file( - temp_dir.path(), - "app/consumers/deep/nesting/nestdir/deep_file.rb", - "# @team b\nclass DeepFile end;", - ); - - let run_config = RunConfig { - project_root: temp_dir.path().to_path_buf(), - codeowners_file_path: temp_dir.path().join(".github/CODEOWNERS").to_path_buf(), - config_path: temp_dir.path().join("config/code_ownership.yml").to_path_buf(), - no_cache: false, - }; - - let file_owner = file_owner_for_file(&run_config, "app/consumers/deep/nesting/nestdir/deep_file.rb") - .unwrap() - .unwrap(); - assert_eq!(file_owner.team.name, "b"); - assert_eq!(file_owner.team.github_team, "@b"); - assert!(file_owner.team.path.to_string_lossy().ends_with("config/teams/b.yml")); - assert_eq!(file_owner.sources.len(), 1); - assert_eq!(file_owner.sources, vec![Source::AnnotatedFile]); - - let team = team_for_file(&run_config, "app/consumers/deep/nesting/nestdir/deep_file.rb") - .unwrap() - .unwrap(); - assert_eq!(team.name, "b"); - assert_eq!(team.github_team, "@b"); - assert!(team.path.to_string_lossy().ends_with("config/teams/b.yml")); - } - - #[test] - fn test_teams_for_files_from_codeowners() { - let project_root = Path::new("tests/fixtures/valid_project"); - let file_paths = [ - "javascript/packages/items/item.ts", - "config/teams/payroll.yml", - "ruby/app/models/bank_account.rb", - "made/up/file.rb", - "ruby/ignored_files/git_ignored.rb", - ]; - let run_config = RunConfig { - project_root: project_root.to_path_buf(), - codeowners_file_path: project_root.join(".github/CODEOWNERS").to_path_buf(), - config_path: project_root.join("config/code_ownership.yml").to_path_buf(), - no_cache: false, - }; - let teams = - teams_for_files_from_codeowners(&run_config, &file_paths.iter().map(|s| s.to_string()).collect::>()).unwrap(); - assert_eq!(teams.len(), 5); - assert_eq!( - teams - .get("javascript/packages/items/item.ts") - .unwrap() - .as_ref() - .map(|t| t.name.as_str()), - Some("Payroll") - ); - assert_eq!( - teams.get("config/teams/payroll.yml").unwrap().as_ref().map(|t| t.name.as_str()), - Some("Payroll") - ); - assert_eq!( - teams - .get("ruby/app/models/bank_account.rb") - .unwrap() - .as_ref() - .map(|t| t.name.as_str()), - Some("Payments") - ); - assert_eq!(teams.get("made/up/file.rb").unwrap().as_ref().map(|t| t.name.as_str()), None); - assert_eq!( - teams - .get("ruby/ignored_files/git_ignored.rb") - .unwrap() - .as_ref() - .map(|t| t.name.as_str()), - None - ); - assert_eq!( - teams - .get("ruby/ignored_files/git_ignored.rb") - .unwrap() - .as_ref() - .map(|t| t.name.as_str()), - None - ); - } } From 9c0296d79c026ffb987a70d7d4e7c34205256fc8 Mon Sep 17 00:00:00 2001 From: Perry Hertler Date: Thu, 21 Aug 2025 21:30:10 -0500 Subject: [PATCH 07/12] DRY --- src/ownership.rs | 2 +- src/runner.rs | 7 ++----- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/src/ownership.rs b/src/ownership.rs index 5641dfc..8f109ef 100644 --- a/src/ownership.rs +++ b/src/ownership.rs @@ -13,7 +13,7 @@ pub(crate) mod codeowners_file_parser; pub(crate) mod codeowners_query; mod file_generator; mod file_owner_finder; -pub mod for_file_fast; +pub mod file_owner_resolver; pub(crate) mod mapper; mod validator; diff --git a/src/runner.rs b/src/runner.rs index 00e7f54..0aef0c3 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -176,7 +176,7 @@ impl Runner { } pub fn owners_for_file(&self, file_path: &str) -> Result, Error> { - use crate::ownership::for_file_fast::find_file_owners; + use crate::ownership::file_owner_resolver::find_file_owners; let owners = find_file_owners(&self.run_config.project_root, &self.config, std::path::Path::new(file_path)).map_err(Error::Io)?; Ok(owners) } @@ -215,10 +215,7 @@ impl Runner { pub fn for_file_codeowners_only(&self, file_path: &str) -> RunResult { match team_for_file_from_codeowners(&self.run_config, file_path) { Ok(Some(team)) => { - let relative_team_path = team - .path - .strip_prefix(&self.run_config.project_root) - .unwrap_or(team.path.as_path()) + let relative_team_path = crate::path_utils::relative_to(&self.run_config.project_root, team.path.as_path()) .to_string_lossy() .to_string(); RunResult { From 1c714904412dc9a855063bd56e145a6fc6a4413d Mon Sep 17 00:00:00 2001 From: Perry Hertler Date: Fri, 22 Aug 2025 09:29:59 -0500 Subject: [PATCH 08/12] json for-file --- Cargo.lock | 2 +- Cargo.toml | 2 +- src/cache/file.rs | 2 +- src/cli.rs | 10 +- src/main.rs | 2 +- src/ownership.rs | 10 +- src/project.rs | 6 +- src/project_builder.rs | 10 +- src/runner.rs | 201 ++++++++++++++++++++++++++++------ src/runner/api.rs | 6 +- src/runner/types.rs | 8 +- tests/invalid_project_test.rs | 59 +++++++++- tests/valid_project_test.rs | 50 ++++++++- 13 files changed, 309 insertions(+), 59 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 57a82a8..598809b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -179,7 +179,7 @@ checksum = "b94f61472cee1439c0b966b47e3aca9ae07e45d070759512cd390ea2bebc6675" [[package]] name = "codeowners" -version = "0.2.16" +version = "0.2.17" dependencies = [ "assert_cmd", "clap", diff --git a/Cargo.toml b/Cargo.toml index a11c487..5a884ec 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "codeowners" -version = "0.2.16" +version = "0.2.17" edition = "2024" [profile.release] diff --git a/src/cache/file.rs b/src/cache/file.rs index ba76126..9aacb76 100644 --- a/src/cache/file.rs +++ b/src/cache/file.rs @@ -62,7 +62,7 @@ impl Caching for GlobalCache { fn delete_cache(&self) -> Result<(), Error> { let cache_path = self.get_cache_path(); - dbg!("deleting", &cache_path); + tracing::debug!("Deleting cache file: {}", cache_path.display()); fs::remove_file(cache_path).change_context(Error::Io) } } diff --git a/src/cli.rs b/src/cli.rs index 4b6343e..1e72bce 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -17,10 +17,12 @@ enum Command { help = "Find the owner from the CODEOWNERS file and just return the team name and yml path" )] from_codeowners: bool, + #[arg(short, long, default_value = "false", help = "Output the result in JSON format")] + json: bool, name: String, }, - #[clap(about = "Finds code ownership information for a given team ", visible_alias = "t")] + #[clap(about = "Finds code ownership information for a given team", visible_alias = "t")] ForTeam { name: String }, #[clap( @@ -113,7 +115,11 @@ pub fn cli() -> Result { Command::Validate => runner::validate(&run_config, vec![]), Command::Generate { skip_stage } => runner::generate(&run_config, !skip_stage), Command::GenerateAndValidate { skip_stage } => runner::generate_and_validate(&run_config, vec![], !skip_stage), - Command::ForFile { name, from_codeowners } => runner::for_file(&run_config, &name, from_codeowners), + Command::ForFile { + name, + from_codeowners, + json, + } => runner::for_file(&run_config, &name, from_codeowners, json), Command::ForTeam { name } => runner::for_team(&run_config, &name), Command::DeleteCache => runner::delete_cache(&run_config), Command::CrosscheckOwners => runner::crosscheck_owners(&run_config), diff --git a/src/main.rs b/src/main.rs index d34ab1e..8afc20a 100644 --- a/src/main.rs +++ b/src/main.rs @@ -26,7 +26,7 @@ fn maybe_print_errors(result: RunResult) -> Result<(), RunnerError> { for msg in result.validation_errors { println!("{}", msg); } - process::exit(-1); + process::exit(1); } Ok(()) diff --git a/src/ownership.rs b/src/ownership.rs index 8f109ef..ff8cbc6 100644 --- a/src/ownership.rs +++ b/src/ownership.rs @@ -59,19 +59,21 @@ impl TeamOwnership { impl Display for FileOwner { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { let sources = if self.sources.is_empty() { - "Unowned".to_string() + "".to_string() } else { - self.sources + let sources_str = self + .sources .iter() .sorted_by_key(|source| source.to_string()) .map(|source| source.to_string()) .collect::>() - .join("\n- ") + .join("\n- "); + format!("\n- {}", sources_str) }; write!( f, - "Team: {}\nGithub Team: {}\nTeam YML: {}\nDescription:\n- {}", + "Team: {}\nGithub Team: {}\nTeam YML: {}\nDescription:{}", self.team.name, self.team.github_team, self.team_config_file_path, sources ) } diff --git a/src/project.rs b/src/project.rs index 5179371..c09b247 100644 --- a/src/project.rs +++ b/src/project.rs @@ -158,9 +158,9 @@ pub enum Error { impl fmt::Display for Error { fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result { match self { - Error::Io => fmt.write_str("Error::Io"), - Error::SerdeYaml => fmt.write_str("Error::SerdeYaml"), - Error::SerdeJson => fmt.write_str("Error::SerdeJson"), + Error::Io => fmt.write_str("IO operation failed"), + Error::SerdeYaml => fmt.write_str("YAML serialization/deserialization failed"), + Error::SerdeJson => fmt.write_str("JSON serialization/deserialization failed"), } } } diff --git a/src/project_builder.rs b/src/project_builder.rs index 45a9d88..1945b0a 100644 --- a/src/project_builder.rs +++ b/src/project_builder.rs @@ -52,8 +52,9 @@ impl<'a> ProjectBuilder<'a> { } } - #[instrument(level = "debug", skip_all)] + #[instrument(level = "debug", skip_all, fields(base_path = %self.base_path.display()))] pub fn build(&mut self) -> Result { + tracing::info!("Starting project build"); let mut builder = WalkBuilder::new(&self.base_path); builder.hidden(false); builder.follow_links(false); @@ -277,6 +278,13 @@ impl<'a> ProjectBuilder<'a> { .flat_map(|team| vec![(team.name.clone(), team.clone()), (team.github_team.clone(), team.clone())]) .collect(); + tracing::info!( + files_count = %project_files.len(), + teams_count = %teams.len(), + packages_count = %packages.len(), + "Project build completed successfully" + ); + Ok(Project { base_path: self.base_path.to_owned(), files: project_files, diff --git a/src/runner.rs b/src/runner.rs index 0aef0c3..f54a508 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -1,6 +1,7 @@ use std::{path::Path, process::Command}; use error_stack::{Result, ResultExt}; +use serde::Serialize; use crate::{ cache::{Cache, Caching, file::GlobalCache, noop::NoopCache}, @@ -181,61 +182,163 @@ impl Runner { Ok(owners) } - pub fn for_file_optimized(&self, file_path: &str) -> RunResult { + pub fn for_file_derived(&self, file_path: &str, json: bool) -> RunResult { let file_owners = match self.owners_for_file(file_path) { Ok(v) => v, Err(err) => { - return RunResult { - io_errors: vec![err.to_string()], - ..Default::default() - }; + return RunResult::from_io_error(Error::Io(err.to_string()), json); } }; - let info_messages: Vec = match file_owners.len() { - 0 => vec![format!("{}", FileOwner::default())], - 1 => vec![format!("{}", file_owners[0])], - _ => { + match file_owners.as_slice() { + [] => RunResult::from_file_owner(&FileOwner::default(), json), + [owner] => RunResult::from_file_owner(owner, json), + many => { let mut error_messages = vec!["Error: file is owned by multiple teams!".to_string()]; - for file_owner in file_owners { - error_messages.push(format!("\n{}", file_owner)); + for owner in many { + error_messages.push(format!("\n{}", owner)); } - return RunResult { - validation_errors: error_messages, - ..Default::default() - }; + RunResult::from_validation_errors(error_messages, json) } - }; - RunResult { - info_messages, - ..Default::default() } } - pub fn for_file_codeowners_only(&self, file_path: &str) -> RunResult { + pub fn for_file_codeowners_only(&self, file_path: &str, json: bool) -> RunResult { match team_for_file_from_codeowners(&self.run_config, file_path) { Ok(Some(team)) => { - let relative_team_path = crate::path_utils::relative_to(&self.run_config.project_root, team.path.as_path()) + let team_yml = crate::path_utils::relative_to(&self.run_config.project_root, team.path.as_path()) .to_string_lossy() .to_string(); - RunResult { - info_messages: vec![format!( - "Team: {}\nGithub Team: {}\nTeam YML: {}\nDescription:\n- Owner inferred from codeowners file", - team.name, team.github_team, relative_team_path - )], - ..Default::default() + let result = ForFileResult { + team_name: team.name.clone(), + github_team: team.github_team.clone(), + team_yml, + description: vec!["Owner inferred from codeowners file".to_string()], + }; + if json { + RunResult::json_info(result) + } else { + RunResult { + info_messages: vec![format!( + "Team: {}\nGithub Team: {}\nTeam YML: {}\nDescription:\n- {}", + result.team_name, + result.github_team, + result.team_yml, + result.description.join("\n- ") + )], + ..Default::default() + } + } + } + Ok(None) => RunResult::from_file_owner(&FileOwner::default(), json), + Err(err) => { + if json { + RunResult::json_io_error(Error::Io(err.to_string())) + } else { + RunResult { + io_errors: vec![err.to_string()], + ..Default::default() + } } } - Ok(None) => RunResult::default(), - Err(err) => RunResult { - io_errors: vec![err.to_string()], - ..Default::default() - }, } } } -// removed free functions for for_file_* variants in favor of Runner methods +#[derive(Debug, Clone, Serialize)] +pub struct ForFileResult { + pub team_name: String, + pub github_team: String, + pub team_yml: String, + pub description: Vec, +} + +impl RunResult { + pub fn has_errors(&self) -> bool { + !self.validation_errors.is_empty() || !self.io_errors.is_empty() + } + + fn from_io_error(error: Error, json: bool) -> Self { + if json { + Self::json_io_error(error) + } else { + Self { + io_errors: vec![error.to_string()], + ..Default::default() + } + } + } + + fn from_file_owner(file_owner: &FileOwner, json: bool) -> Self { + if json { + let description: Vec = if file_owner.sources.is_empty() { + vec![] + } else { + file_owner.sources.iter().map(|source| source.to_string()).collect() + }; + Self::json_info(ForFileResult { + team_name: file_owner.team.name.clone(), + github_team: file_owner.team.github_team.clone(), + team_yml: file_owner.team_config_file_path.clone(), + description, + }) + } else { + Self { + info_messages: vec![format!("{}", file_owner)], + ..Default::default() + } + } + } + + fn from_validation_errors(validation_errors: Vec, json: bool) -> Self { + if json { + Self::json_validation_error(validation_errors) + } else { + Self { + validation_errors, + ..Default::default() + } + } + } + + pub fn json_info(result: ForFileResult) -> Self { + let json = match serde_json::to_string_pretty(&result) { + Ok(json) => json, + Err(e) => return Self::json_io_error(Error::Io(e.to_string())), + }; + Self { + info_messages: vec![json], + ..Default::default() + } + } + + pub fn json_io_error(error: Error) -> Self { + let message = match error { + Error::Io(msg) => msg, + Error::ValidationFailed => "Error::ValidationFailed".to_string(), + }; + let json = match serde_json::to_string(&serde_json::json!({"error": message})).map_err(|e| Error::Io(e.to_string())) { + Ok(json) => json, + Err(e) => return Self::json_io_error(Error::Io(e.to_string())), + }; + Self { + io_errors: vec![json], + ..Default::default() + } + } + + pub fn json_validation_error(validation_errors: Vec) -> Self { + let json_obj = serde_json::json!({"validation_errors": validation_errors}); + let json = match serde_json::to_string_pretty(&json_obj) { + Ok(json) => json, + Err(e) => return Self::json_io_error(Error::Io(e.to_string())), + }; + Self { + validation_errors: vec![json], + ..Default::default() + } + } +} #[cfg(test)] mod tests { @@ -245,4 +348,36 @@ mod tests { fn test_version() { assert_eq!(version(), env!("CARGO_PKG_VERSION").to_string()); } + #[test] + fn test_json_info() { + let result = ForFileResult { + team_name: "team1".to_string(), + github_team: "team1".to_string(), + team_yml: "config/teams/team1.yml".to_string(), + description: vec!["file annotation".to_string()], + }; + let result = RunResult::json_info(result); + assert_eq!(result.info_messages.len(), 1); + assert_eq!( + result.info_messages[0], + "{\n \"team_name\": \"team1\",\n \"github_team\": \"team1\",\n \"team_yml\": \"config/teams/team1.yml\",\n \"description\": [\n \"file annotation\"\n ]\n}" + ); + } + + #[test] + fn test_json_io_error() { + let result = RunResult::json_io_error(Error::Io("unable to find file".to_string())); + assert_eq!(result.io_errors.len(), 1); + assert_eq!(result.io_errors[0], "{\"error\":\"unable to find file\"}"); + } + + #[test] + fn test_json_validation_error() { + let result = RunResult::json_validation_error(vec!["file has multiple owners".to_string()]); + assert_eq!(result.validation_errors.len(), 1); + assert_eq!( + result.validation_errors[0], + "{\n \"validation_errors\": [\n \"file has multiple owners\"\n ]\n}" + ); + } } diff --git a/src/runner/api.rs b/src/runner/api.rs index ea5fcaa..962d6b0 100644 --- a/src/runner/api.rs +++ b/src/runner/api.rs @@ -6,12 +6,12 @@ use crate::project::Team; use super::{Error, RunConfig, RunResult, Runner, config_from_path, run}; -pub fn for_file(run_config: &RunConfig, file_path: &str, from_codeowners: bool) -> RunResult { +pub fn for_file(run_config: &RunConfig, file_path: &str, from_codeowners: bool, json: bool) -> RunResult { run(run_config, |runner| { if from_codeowners { - runner.for_file_codeowners_only(file_path) + runner.for_file_codeowners_only(file_path, json) } else { - runner.for_file_optimized(file_path) + runner.for_file_derived(file_path, json) } }) } diff --git a/src/runner/types.rs b/src/runner/types.rs index 5ba38ea..c42fa02 100644 --- a/src/runner/types.rs +++ b/src/runner/types.rs @@ -11,12 +11,6 @@ pub struct RunResult { pub info_messages: Vec, } -impl RunResult { - pub fn has_errors(&self) -> bool { - !self.validation_errors.is_empty() || !self.io_errors.is_empty() - } -} - #[derive(Debug, Clone)] pub struct RunConfig { pub project_root: PathBuf, @@ -25,7 +19,7 @@ pub struct RunConfig { pub no_cache: bool, } -#[derive(Debug)] +#[derive(Debug, Serialize)] pub enum Error { Io(String), ValidationFailed, diff --git a/tests/invalid_project_test.rs b/tests/invalid_project_test.rs index d3191ce..90a001f 100644 --- a/tests/invalid_project_test.rs +++ b/tests/invalid_project_test.rs @@ -54,12 +54,30 @@ fn test_for_file() -> Result<(), Box> { Github Team: Unowned Team YML: Description: - - Unowned "}), )?; Ok(()) } +#[test] +fn test_for_file_json() -> Result<(), Box> { + run_codeowners( + "invalid_project", + &["for-file", "ruby/app/models/blockchain.rb", "--json"], + true, + OutputStream::Stdout, + predicate::eq(indoc! {r#" + { + "team_name": "Unowned", + "github_team": "Unowned", + "team_yml": "", + "description": [] + } + "#}), + )?; + Ok(()) +} + #[test] fn test_for_file_multiple_owners() -> Result<(), Box> { run_codeowners( @@ -85,3 +103,42 @@ fn test_for_file_multiple_owners() -> Result<(), Box> { )?; Ok(()) } + +#[test] +fn test_for_file_multiple_owners_json() -> Result<(), Box> { + run_codeowners( + "invalid_project", + &["for-file", "ruby/app/services/multi_owned.rb", "--json"], + false, + OutputStream::Stdout, + predicate::eq(indoc! {r#" + { + "validation_errors": [ + "Error: file is owned by multiple teams!", + "\nTeam: Payments\nGithub Team: @PaymentTeam\nTeam YML: config/teams/payments.yml\nDescription:\n- Owner annotation at the top of the file", + "\nTeam: Payroll\nGithub Team: @PayrollTeam\nTeam YML: config/teams/payroll.yml\nDescription:\n- Owner specified in `ruby/app/services/.codeowner`" + ] + } + "#}), + )?; + Ok(()) +} + +#[test] +fn test_for_file_nonexistent_json() -> Result<(), Box> { + run_codeowners( + "invalid_project", + &["for-file", "nonexistent/file.rb", "--json"], + true, + OutputStream::Stdout, + predicate::eq(indoc! {r#" + { + "team_name": "Unowned", + "github_team": "Unowned", + "team_yml": "", + "description": [] + } + "#}), + )?; + Ok(()) +} diff --git a/tests/valid_project_test.rs b/tests/valid_project_test.rs index 0e28554..ca2aa56 100644 --- a/tests/valid_project_test.rs +++ b/tests/valid_project_test.rs @@ -71,6 +71,28 @@ fn test_for_file() -> Result<(), Box> { Ok(()) } +#[test] +fn test_for_file_json() -> Result<(), Box> { + run_codeowners( + "valid_project", + &["for-file", "ruby/app/models/payroll.rb", "--json"], + true, + OutputStream::Stdout, + predicate::eq(indoc! {r#" + { + "team_name": "Payroll", + "github_team": "@PayrollTeam", + "team_yml": "config/teams/payroll.yml", + "description": [ + "Owner annotation at the top of the file" + ] + } + "#}), + )?; + + Ok(()) +} + #[test] fn test_for_file_full_path() -> Result<(), Box> { let project_root = Path::new("tests/fixtures/valid_project"); @@ -94,6 +116,33 @@ fn test_for_file_full_path() -> Result<(), Box> { Ok(()) } +#[test] +fn test_for_file_full_path_json() -> Result<(), Box> { + let project_root = Path::new("tests/fixtures/valid_project"); + let for_file_absolute_path = fs::canonicalize(project_root.join("ruby/app/models/payroll.rb"))?; + + Command::cargo_bin("codeowners")? + .arg("--project-root") + .arg(project_root) + .arg("--no-cache") + .arg("for-file") + .arg(for_file_absolute_path.to_str().unwrap()) + .arg("--json") + .assert() + .success() + .stdout(predicate::eq(indoc! {r#" + { + "team_name": "Payroll", + "github_team": "@PayrollTeam", + "team_yml": "config/teams/payroll.yml", + "description": [ + "Owner annotation at the top of the file" + ] + } + "#})); + Ok(()) +} + #[test] fn test_fast_for_file() -> Result<(), Box> { Command::cargo_bin("codeowners")? @@ -129,7 +178,6 @@ fn test_fast_for_file_with_ignored_file() -> Result<(), Box> { Github Team: Unowned Team YML: Description: - - Unowned "})); Ok(()) } From b90ed084238e6eb7d26b38187eb76171944e2aa2 Mon Sep 17 00:00:00 2001 From: Perry Hertler Date: Fri, 22 Aug 2025 19:18:44 -0500 Subject: [PATCH 09/12] fallback error --- src/runner.rs | 15 +++++++--- src/runner/api.rs | 75 ++++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 78 insertions(+), 12 deletions(-) diff --git a/src/runner.rs b/src/runner.rs index f54a508..abf573d 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -304,7 +304,7 @@ impl RunResult { pub fn json_info(result: ForFileResult) -> Self { let json = match serde_json::to_string_pretty(&result) { Ok(json) => json, - Err(e) => return Self::json_io_error(Error::Io(e.to_string())), + Err(e) => return Self::fallback_io_error(&e.to_string()), }; Self { info_messages: vec![json], @@ -317,9 +317,9 @@ impl RunResult { Error::Io(msg) => msg, Error::ValidationFailed => "Error::ValidationFailed".to_string(), }; - let json = match serde_json::to_string(&serde_json::json!({"error": message})).map_err(|e| Error::Io(e.to_string())) { + let json = match serde_json::to_string(&serde_json::json!({"error": message})) { Ok(json) => json, - Err(e) => return Self::json_io_error(Error::Io(e.to_string())), + Err(e) => return Self::fallback_io_error(&format!("JSON serialization failed: {}", e)), }; Self { io_errors: vec![json], @@ -331,13 +331,20 @@ impl RunResult { let json_obj = serde_json::json!({"validation_errors": validation_errors}); let json = match serde_json::to_string_pretty(&json_obj) { Ok(json) => json, - Err(e) => return Self::json_io_error(Error::Io(e.to_string())), + Err(e) => return Self::fallback_io_error(&format!("JSON serialization failed: {}", e)), }; Self { validation_errors: vec![json], ..Default::default() } } + + fn fallback_io_error(message: &str) -> Self { + Self { + io_errors: vec![format!("{{\"error\": \"{}\"}}", message.replace('"', "\\\""))], + ..Default::default() + } + } } #[cfg(test)] diff --git a/src/runner/api.rs b/src/runner/api.rs index 962d6b0..5328349 100644 --- a/src/runner/api.rs +++ b/src/runner/api.rs @@ -4,16 +4,13 @@ use std::path::Path; use crate::ownership::FileOwner; use crate::project::Team; -use super::{Error, RunConfig, RunResult, Runner, config_from_path, run}; +use super::{Error, ForFileResult, RunConfig, RunResult, Runner, config_from_path, run}; pub fn for_file(run_config: &RunConfig, file_path: &str, from_codeowners: bool, json: bool) -> RunResult { - run(run_config, |runner| { - if from_codeowners { - runner.for_file_codeowners_only(file_path, json) - } else { - runner.for_file_derived(file_path, json) - } - }) + if from_codeowners { + return for_file_codeowners_only_fast(run_config, file_path, json); + } + for_file_optimized(run_config, file_path, json) } pub fn for_team(run_config: &RunConfig, team_name: &str) -> RunResult { @@ -81,3 +78,65 @@ pub fn team_for_file_from_codeowners(run_config: &RunConfig, file_path: &str) -> .map_err(Error::Io)?; Ok(res) } + +// Fast path that avoids creating a full Runner for single file queries +fn for_file_optimized(run_config: &RunConfig, file_path: &str, json: bool) -> RunResult { + let config = match config_from_path(&run_config.config_path) { + Ok(c) => c, + Err(err) => { + return RunResult::from_io_error(Error::Io(err.to_string()), json); + } + }; + + use crate::ownership::file_owner_resolver::find_file_owners; + let file_owners = match find_file_owners(&run_config.project_root, &config, std::path::Path::new(file_path)) { + Ok(v) => v, + Err(err) => { + return RunResult::from_io_error(Error::Io(err), json); + } + }; + + match file_owners.as_slice() { + [] => RunResult::from_file_owner(&crate::ownership::FileOwner::default(), json), + [owner] => RunResult::from_file_owner(owner, json), + many => { + let mut error_messages = vec!["Error: file is owned by multiple teams!".to_string()]; + for owner in many { + error_messages.push(format!("\n{}", owner)); + } + RunResult::from_validation_errors(error_messages, json) + } + } +} + +fn for_file_codeowners_only_fast(run_config: &RunConfig, file_path: &str, json: bool) -> RunResult { + match team_for_file_from_codeowners(run_config, file_path) { + Ok(Some(team)) => { + let team_yml = crate::path_utils::relative_to(&run_config.project_root, team.path.as_path()) + .to_string_lossy() + .to_string(); + let result = ForFileResult { + team_name: team.name.clone(), + github_team: team.github_team.clone(), + team_yml, + description: vec!["Owner inferred from codeowners file".to_string()], + }; + if json { + RunResult::json_info(result) + } else { + RunResult { + info_messages: vec![format!( + "Team: {}\nGithub Team: {}\nTeam YML: {}\nDescription:\n- {}", + result.team_name, + result.github_team, + result.team_yml, + result.description.join("\n- ") + )], + ..Default::default() + } + } + } + Ok(None) => RunResult::from_file_owner(&crate::ownership::FileOwner::default(), json), + Err(err) => RunResult::from_io_error(Error::Io(format!("{}", err)), json), + } +} From c1478dddf533a05a5cc56407eb22b84764e5cbfb Mon Sep 17 00:00:00 2001 From: Perry Hertler Date: Sat, 23 Aug 2025 15:04:40 -0500 Subject: [PATCH 10/12] perf boost --- src/runner/api.rs | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/runner/api.rs b/src/runner/api.rs index 5328349..9728238 100644 --- a/src/runner/api.rs +++ b/src/runner/api.rs @@ -4,7 +4,7 @@ use std::path::Path; use crate::ownership::FileOwner; use crate::project::Team; -use super::{Error, ForFileResult, RunConfig, RunResult, Runner, config_from_path, run}; +use super::{Error, ForFileResult, RunConfig, RunResult, config_from_path, run}; pub fn for_file(run_config: &RunConfig, file_path: &str, from_codeowners: bool, json: bool) -> RunResult { if from_codeowners { @@ -37,10 +37,17 @@ pub fn crosscheck_owners(run_config: &RunConfig) -> RunResult { run(run_config, |runner| runner.crosscheck_owners()) } +// Returns all owners for a file without creating a Runner (performance optimized) +pub fn owners_for_file(run_config: &RunConfig, file_path: &str) -> error_stack::Result, Error> { + let config = config_from_path(&run_config.config_path)?; + use crate::ownership::file_owner_resolver::find_file_owners; + let owners = find_file_owners(&run_config.project_root, &config, std::path::Path::new(file_path)).map_err(Error::Io)?; + Ok(owners) +} + // Returns the highest priority owner for a file. More to come here. pub fn file_owner_for_file(run_config: &RunConfig, file_path: &str) -> error_stack::Result, Error> { - let runner = Runner::new(run_config)?; - let owners = runner.owners_for_file(file_path)?; + let owners = owners_for_file(run_config, file_path)?; Ok(owners.first().cloned()) } From 6270ac7340aaf290b72f5575b7cf785eb3bc63cd Mon Sep 17 00:00:00 2001 From: Perry Hertler Date: Sat, 23 Aug 2025 17:23:36 -0500 Subject: [PATCH 11/12] memoizing codeowners --- src/cache/file.rs | 2 +- src/ownership/codeowners_file_parser.rs | 22 +++++++++++----------- src/runner/api.rs | 15 ++++----------- 3 files changed, 16 insertions(+), 23 deletions(-) diff --git a/src/cache/file.rs b/src/cache/file.rs index 9aacb76..4132ab8 100644 --- a/src/cache/file.rs +++ b/src/cache/file.rs @@ -17,7 +17,7 @@ pub struct GlobalCache { file_owner_cache: Option>>>, } -const DEFAULT_CACHE_CAPACITY: usize = 10000; +const DEFAULT_CACHE_CAPACITY: usize = 50000; impl Caching for GlobalCache { fn get_file_owner(&self, path: &Path) -> Result, Error> { diff --git a/src/ownership/codeowners_file_parser.rs b/src/ownership/codeowners_file_parser.rs index 75ab6c7..1cce3f0 100644 --- a/src/ownership/codeowners_file_parser.rs +++ b/src/ownership/codeowners_file_parser.rs @@ -44,16 +44,7 @@ impl Parser { return Ok(HashMap::new()); } - let codeowners_entries: Vec<(String, String)> = - build_codeowners_lines_in_priority(self.codeowners_file_path.to_string_lossy().into_owned()) - .iter() - .map(|line| { - line.split_once(' ') - .map(|(glob, team_name)| (glob.to_string(), team_name.to_string())) - .ok_or_else(|| IoError::new(std::io::ErrorKind::InvalidInput, "Invalid line")) - }) - .collect::>() - .map_err(|e| Box::new(e) as Box)?; + let codeowners_entries = parse_codeowners_entries(self.codeowners_file_path.to_string_lossy().into_owned()); let teams_by_name = teams_by_github_team_name(self.absolute_team_files_globs()); @@ -111,7 +102,6 @@ fn teams_by_github_team_name(team_file_glob: Vec) -> HashMap Vec { let codeowners_file = match fs::read_to_string(codeowners_file_path) { Ok(codeowners_file) => codeowners_file, @@ -124,6 +114,16 @@ fn build_codeowners_lines_in_priority(codeowners_file_path: String) -> Vec Vec<(String, String)> { + build_codeowners_lines_in_priority(codeowners_file_path) + .iter() + .filter_map(|line| { + line.split_once(' ') + .map(|(glob, team_name)| (glob.to_string(), team_name.to_string())) + }) + .collect() +} + #[derive(Debug, Clone, PartialEq, Eq)] struct Section { heading: String, diff --git a/src/runner/api.rs b/src/runner/api.rs index 9728238..1389a1f 100644 --- a/src/runner/api.rs +++ b/src/runner/api.rs @@ -73,17 +73,10 @@ pub fn teams_for_files_from_codeowners( } pub fn team_for_file_from_codeowners(run_config: &RunConfig, file_path: &str) -> error_stack::Result, Error> { - let relative_file_path = crate::path_utils::relative_to(&run_config.project_root, Path::new(file_path)); - - let config = config_from_path(&run_config.config_path)?; - let res = crate::ownership::codeowners_query::team_for_file_from_codeowners( - &run_config.project_root, - &run_config.codeowners_file_path, - &config.team_file_glob, - Path::new(relative_file_path), - ) - .map_err(Error::Io)?; - Ok(res) + let result = teams_for_files_from_codeowners(run_config, &[file_path.to_string()])?; + // Since we only passed one file, there should be exactly one result + debug_assert_eq!(result.len(), 1); + Ok(result.into_values().next().flatten()) } // Fast path that avoids creating a full Runner for single file queries From 05f394865dd188a0533a20fdf6f2b8c7bf6c5669 Mon Sep 17 00:00:00 2001 From: Perry Hertler Date: Sat, 23 Aug 2025 17:33:11 -0500 Subject: [PATCH 12/12] DRY from codeowners --- src/ownership/codeowners_file_parser.rs | 13 +------------ src/ownership/codeowners_query.rs | 21 --------------------- src/runner/api.rs | 1 - 3 files changed, 1 insertion(+), 34 deletions(-) diff --git a/src/ownership/codeowners_file_parser.rs b/src/ownership/codeowners_file_parser.rs index 1cce3f0..c40410c 100644 --- a/src/ownership/codeowners_file_parser.rs +++ b/src/ownership/codeowners_file_parser.rs @@ -6,13 +6,7 @@ use fast_glob::glob_match; use memoize::memoize; use rayon::prelude::*; use regex::Regex; -use std::{ - collections::HashMap, - error::Error, - fs, - io::Error as IoError, - path::{Path, PathBuf}, -}; +use std::{collections::HashMap, error::Error, fs, io::Error as IoError, path::PathBuf}; use super::file_generator::compare_lines; @@ -62,11 +56,6 @@ impl Parser { Ok(result) } - pub fn team_from_file_path(&self, file_path: &Path) -> Result, Box> { - let teams = self.teams_from_files_paths(&[file_path.to_path_buf()])?; - Ok(teams.get(file_path.to_string_lossy().into_owned().as_str()).cloned().flatten()) - } - fn absolute_team_files_globs(&self) -> Vec { self.team_file_globs .iter() diff --git a/src/ownership/codeowners_query.rs b/src/ownership/codeowners_query.rs index 70846e4..562bb48 100644 --- a/src/ownership/codeowners_query.rs +++ b/src/ownership/codeowners_query.rs @@ -4,27 +4,6 @@ use std::path::{Path, PathBuf}; use crate::ownership::codeowners_file_parser::Parser; use crate::project::Team; -pub(crate) fn team_for_file_from_codeowners( - project_root: &Path, - codeowners_file_path: &Path, - team_file_globs: &[String], - file_path: &Path, -) -> Result, String> { - let relative_file_path = if file_path.is_absolute() { - crate::path_utils::relative_to_buf(project_root, file_path) - } else { - PathBuf::from(file_path) - }; - - let parser = Parser { - codeowners_file_path: codeowners_file_path.to_path_buf(), - project_root: project_root.to_path_buf(), - team_file_globs: team_file_globs.to_vec(), - }; - - parser.team_from_file_path(&relative_file_path).map_err(|e| e.to_string()) -} - pub(crate) fn teams_for_files_from_codeowners( project_root: &Path, codeowners_file_path: &Path, diff --git a/src/runner/api.rs b/src/runner/api.rs index 1389a1f..acaf8ae 100644 --- a/src/runner/api.rs +++ b/src/runner/api.rs @@ -1,5 +1,4 @@ use std::collections::HashMap; -use std::path::Path; use crate::ownership::FileOwner; use crate::project::Team;