From 0455fbae0dde71c298173f2081189301f0dd0ccd Mon Sep 17 00:00:00 2001 From: Perry Hertler Date: Thu, 20 Aug 2026 11:55:02 -0500 Subject: [PATCH] perf: batch the CODEOWNERS query in validate_files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit validate_files called team_for_file_from_codeowners once per path. That helper wraps the path in a one-element slice and hands it to the batch query, which reloads the config and re-reads and re-parses the entire CODEOWNERS file every time — parse_codeowners_entries is not memoized, unlike teams_by_github_team_name right beside it. Against an 18k-line CODEOWNERS that cost ~9.5ms per path, linearly: a 2000-file changeset spent 22s, of which ~20s was re-parsing the same file 2000 times. Now the paths are filtered once and handed to the batch query in a single call, which already parallelizes internally. The batch returns a map keyed by project-relative path, so the lookup key has to match what the query computes. Relativization therefore goes through path_utils::relative_to rather than a second hand-rolled copy of strip_prefix; if the two ever diverged, lookups would miss silently and report owned files as unowned. Unowned files are still reported using the caller's original path string, and still in input order, so absolute paths render as before. No IO error behavior change. The batched call cannot attribute a failure to a single path, but that is unobservable here: the only error the query reports is a non-UTF-8 path, and these have already been through to_string_lossy. A missing or unreadable CODEOWNERS is not an error on this path either — the parser logs it and yields no entries, so every path is reported unowned. Both were true before this change. Tests cover the map lookup specifically: several unowned paths in one call, the same path passed twice (one collapsed key), and an absolute plus a relative path to the same file (one key, two original strings). Co-Authored-By: Claude Fable 5 --- src/runner.rs | 87 +++++++++++++++++++++++------------- tests/validate_files_test.rs | 77 +++++++++++++++++++++++++++++++ 2 files changed, 134 insertions(+), 30 deletions(-) diff --git a/src/runner.rs b/src/runner.rs index 5562979..4d4d001 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -143,36 +143,71 @@ impl Runner { } fn validate_files(&self, file_paths: Vec) -> RunResult { - let mut unowned_files = Vec::new(); - let mut io_errors = Vec::new(); - // Filter files based on owned_globs and unowned_globs configuration // Only validate files that match owned_globs and don't match unowned_globs - let filtered_paths: Vec = file_paths + // + // Each surviving path is kept alongside its project-relative form: the + // relative form is what the CODEOWNERS query is keyed by, while the + // original is what gets reported back to the caller. + let (original_paths, relative_paths): (Vec, Vec) = file_paths .into_iter() - .filter(|file_path| { - // Convert to relative path for glob matching - let path = Path::new(file_path); - let relative_path = if path.is_absolute() { - path.strip_prefix(&self.run_config.project_root).unwrap_or(path) - } else { - path - }; + .filter_map(|file_path| { + // Relativize with the same helper the CODEOWNERS query uses. The query + // keys its result map by this form, so the two must agree exactly -- + // otherwise the lookups below miss silently and report owned files as + // unowned. + let relative_path = crate::path_utils::relative_to(&self.run_config.project_root, Path::new(&file_path)); // Mirror the filtering applied by ProjectBuilder when walking the project - matches_globs(relative_path, &self.config.owned_globs) && !matches_globs(relative_path, &self.config.unowned_globs) + if matches_globs(relative_path, &self.config.owned_globs) && !matches_globs(relative_path, &self.config.unowned_globs) { + let relative = relative_path.to_string_lossy().into_owned(); + Some((file_path, relative)) + } else { + None + } }) - .collect(); + .unzip(); - debug_span!("per_file_query").in_scope(|| { - for file_path in filtered_paths { - match team_for_file_from_codeowners(&self.run_config, &file_path) { - Ok(Some(_)) => {} - Ok(None) => unowned_files.push(file_path), - Err(err) => io_errors.push(format!("{}: {}", file_path, err)), - } + if relative_paths.is_empty() { + return RunResult::default(); + } + + // One batched query for every path, rather than one query per path. The + // per-path version re-read and re-parsed the entire CODEOWNERS file every + // time, because `parse_codeowners_entries` is not memoized. The batch + // function already parallelizes across the paths it is given. + // + // This calls the inner query rather than the `runner::api` wrapper on + // purpose: the wrapper reloads the config on every call, which is the other + // half of the per-path cost. + let teams = match debug_span!("per_file_query").in_scope(|| { + crate::ownership::codeowners_query::teams_for_files_from_codeowners( + &self.run_config.project_root, + &self.codeowners_file_path, + &self.config.team_file_glob, + &relative_paths, + ) + }) { + Ok(teams) => teams, + // Kept for completeness rather than because it fires: the only failure the + // query reports is a non-UTF-8 path, and these have already been through + // `to_string_lossy`. Note that an unreadable CODEOWNERS is not an error on + // this path at all -- the parser logs it and yields no entries, so every + // path is reported unowned instead. + Err(err) => { + return RunResult { + io_errors: vec![err], + ..Default::default() + }; } - }); + }; + + // Report the caller's original path string, not the relative key, so absolute + // paths render as the caller wrote them. + let unowned_files: Vec = std::iter::zip(original_paths, &relative_paths) + .filter(|(_, relative)| teams.get(relative.as_str()).is_none_or(Option::is_none)) + .map(|(original, _)| original) + .collect(); if !unowned_files.is_empty() { let validation_errors = std::iter::once("Unowned files detected:".to_string()) @@ -181,14 +216,6 @@ impl Runner { return RunResult { validation_errors, - io_errors, - ..Default::default() - }; - } - - if !io_errors.is_empty() { - return RunResult { - io_errors, ..Default::default() }; } diff --git a/tests/validate_files_test.rs b/tests/validate_files_test.rs index 541b5bf..d17262c 100644 --- a/tests/validate_files_test.rs +++ b/tests/validate_files_test.rs @@ -376,3 +376,80 @@ fn test_validate_respects_unowned_globs() -> Result<(), Box> { Ok(()) } + +// The three tests below cover the batched CODEOWNERS query specifically: results come +// back in a map keyed by project-relative path, so they exercise multiple keys, a +// collapsed duplicate key, and two different original strings sharing one key. + +#[test] +fn test_validate_reports_every_unowned_file_in_one_invocation() -> Result<(), Box> { + // More than one unowned path in a single call, mixed with an owned one. The batch + // returns a map, so this checks no entry is dropped and the owned file stays absent. + run_codeowners( + "valid_project", + &[ + "validate", + "ruby/app/first_unowned.rb", + "ruby/app/models/payroll.rb", + "ruby/app/second_unowned.rb", + ], + false, + OutputStream::Stdout, + predicate::str::contains("ruby/app/first_unowned.rb") + .and(predicate::str::contains("ruby/app/second_unowned.rb")) + .and(predicate::str::contains("models/payroll.rb").not()), + )?; + + Ok(()) +} + +#[test] +fn test_validate_handles_the_same_path_passed_twice() -> Result<(), Box> { + // Duplicate paths collapse to a single key in the results map. The file must still + // be reported rather than lost to the dedup. + run_codeowners( + "valid_project", + &["validate", "ruby/app/unowned.rb", "ruby/app/unowned.rb"], + false, + OutputStream::Stdout, + predicate::str::contains("ruby/app/unowned.rb").and(predicate::str::contains("Unowned")), + )?; + + Ok(()) +} + +#[test] +fn test_validate_reports_absolute_and_relative_paths_as_given() -> Result<(), Box> { + // An absolute and a relative path to the same file share one relative key. Each must + // be echoed back in the form the caller supplied, not regenerated from the key. + let fixture_root = std::path::Path::new("tests/fixtures/valid_project"); + let temp_dir = setup_fixture_repo(fixture_root); + let project_root = temp_dir.path(); + + // Must exist on disk to be canonicalized below, and absent from CODEOWNERS so it + // comes back unowned. + std::fs::write(project_root.join("ruby/app/unowned.rb"), "# no owner")?; + git_add_all_files(project_root); + + // Canonicalized to match the project root, which the CLI canonicalizes. A + // non-canonical absolute path fails to relativize and is then dropped by the + // owned_globs filter before it ever reaches the query. + let absolute = project_root.join("ruby/app/unowned.rb").canonicalize()?; + + Command::cargo_bin("codeowners")? + .arg("--project-root") + .arg(project_root) + .arg("--no-cache") + .arg("validate") + .arg(absolute.to_str().unwrap()) + .arg("ruby/app/unowned.rb") + .assert() + .failure() + // The absolute form, reported verbatim. + .stdout(predicate::str::contains(absolute.to_str().unwrap())) + // The relative form. The leading indent distinguishes it from the absolute + // line, which also ends in this same substring. + .stdout(predicate::str::contains(" ruby/app/unowned.rb")); + + Ok(()) +}