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(()) +}