perf: batch the CODEOWNERS query in validate_files (parked pending #125)

src/runner.rs

@@ -143,36 +143,71 @@ impl Runner {
     }
 
     fn validate_files(&self, file_paths: Vec<String>) -> 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<String> = 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<String>, Vec<String>) = 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<String> = 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()
             };
         }

tests/validate_files_test.rs

@@ -376,3 +376,80 @@ fn test_validate_respects_unowned_globs() -> Result<(), Box<dyn Error>> {
 
     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<dyn Error>> {
+    // 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<dyn Error>> {
+    // 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<dyn Error>> {
+    // 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(())
+}