diff --git a/.github/dependabot.yml b/.github/dependabot.yml index 17aafe5..6ec0feb 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -8,6 +8,10 @@ updates: cargo: patterns: - "*" + # serde-saphyr patches only its latest release, and its releases can change how YAML parses, + # so its bumps get their own pull request instead of waiting on unrelated breakage. + exclude-patterns: + - "serde-saphyr" cooldown: default-days: 7 - package-ecosystem: "github-actions" diff --git a/Cargo.lock b/Cargo.lock index eac3781..3fe878a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -11,6 +11,17 @@ dependencies = [ "memchr", ] +[[package]] +name = "annotate-snippets" +version = "0.12.12" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c86cd1c51b95d71dde52bca69ed225008f6ff4c8cc825b08042aa1ef823e1980" +dependencies = [ + "anstyle", + "memchr", + "unicode-width 0.2.2", +] + [[package]] name = "anstream" version = "0.6.18" @@ -66,6 +77,12 @@ version = "1.0.104" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "330a5ed07fa54e4702c9d6c4174f74427fc0ef6e214bbd677ae50a5099946470" +[[package]] +name = "arraydeque" +version = "0.5.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7d902e3d592a523def97af8f317b08ce16b7ab854c1985a0c671e6f15cebc236" + [[package]] name = "arrayvec" version = "0.7.6" @@ -189,8 +206,8 @@ dependencies = [ "regex", "rusty-hook", "serde", + "serde-saphyr", "serde_json", - "serde_yaml", "similar", "tempfile", "tracing", @@ -203,6 +220,12 @@ version = "1.0.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "acbf1af155f9b9ef647e42cdc158db4b64a1b61f743629225fde6f3e0be2a7c7" +[[package]] +name = "core_detect" +version = "1.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7f8f80099a98041a3d1622845c271458a2d73e688351bf3cb999266764b81d48" + [[package]] name = "crossbeam-channel" version = "0.5.17" @@ -256,6 +279,29 @@ version = "1.8.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7fcaabb2fef8c910e7f4c7ce9f67a1283a1715879a7c230ca9d6d1ae31f16d91" +[[package]] +name = "encoding_rs" +version = "0.8.42" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8e985e0451871ad22fb8d2b6b076e2028a502a0d3950998c2c5c0a4f9b5d9679" +dependencies = [ + "cfg-if", + "core_detect", + "multiversion_no_op", + "rustversion", + "scopeguard", + "simdutf8", +] + +[[package]] +name = "encoding_rs_io" +version = "0.1.8" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fba3fe847045ecff794b9c138293a80db914678c453ad63fbf0c6a9eb6e00b22" +dependencies = [ + "encoding_rs", +] + [[package]] name = "enum_dispatch" version = "0.3.13" @@ -275,15 +321,9 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "a2d328fc287c61314c4a61af7cfdcbd7e678e39778488c7cb13ec133ce0f4059" dependencies = [ "fsio", - "indexmap 1.9.3", + "indexmap", ] -[[package]] -name = "equivalent" -version = "1.0.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5443807d6dff69373d433ab9ef5378ad8df50ca6298caf15de6e52e24aaf54d5" - [[package]] name = "errno" version = "0.3.13" @@ -340,7 +380,7 @@ version = "0.2.21" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "14dbbfd5c71d70241ecf9e6f13737f7b5ce823821063188d7e46c41d371eebd5" dependencies = [ - "unicode-width", + "unicode-width 0.1.10", ] [[package]] @@ -375,16 +415,20 @@ dependencies = [ ] [[package]] -name = "hashbrown" -version = "0.12.3" +name = "granit-parser" +version = "2.0.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8a9ee70c43aaf417c914396645a0fa852624801b24ebb7ae78fe8272889ac888" +checksum = "da55feca5d1625466bae068f9fd4b87892caeae70c3fe671ddb0f50d41aebf37" +dependencies = [ + "arraydeque", + "smallvec", +] [[package]] name = "hashbrown" -version = "0.15.1" +version = "0.12.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3a9bfc1af68b1726ea47d3d5109de126281def866b33970e10fbab11b5dafab3" +checksum = "8a9ee70c43aaf417c914396645a0fa852624801b24ebb7ae78fe8272889ac888" [[package]] name = "heck" @@ -415,17 +459,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "bd070e393353796e801d209ad339e89596eb4c8d430d18ede6a1cced8fafbd99" dependencies = [ "autocfg", - "hashbrown 0.12.3", -] - -[[package]] -name = "indexmap" -version = "2.6.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "707907fe3c25f5424cce2cb7e1cbcafee6bdbe735ca90ef77c29e84591e5b9da" -dependencies = [ - "equivalent", - "hashbrown 0.15.1", + "hashbrown", ] [[package]] @@ -519,6 +553,12 @@ dependencies = [ "syn 1.0.109", ] +[[package]] +name = "multiversion_no_op" +version = "1.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "743fb55ba31b18fb1ecef6bdc9aa2743314978ac084044301a7eee33fb99a20d" + [[package]] name = "nias" version = "0.5.0" @@ -720,12 +760,6 @@ dependencies = [ "toml", ] -[[package]] -name = "ryu" -version = "1.0.13" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f91339c0467de62360649f8d3e185ca8de4224ff281f66000de5eb2a77a79041" - [[package]] name = "same-file" version = "1.0.6" @@ -735,6 +769,12 @@ dependencies = [ "winapi-util", ] +[[package]] +name = "scopeguard" +version = "1.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "94143f37725109f92c262ed2cf5e59bce7498c01bcc1502d7b9afe439a4e9f49" + [[package]] name = "semver" version = "1.0.17" @@ -751,6 +791,20 @@ dependencies = [ "serde_derive", ] +[[package]] +name = "serde-saphyr" +version = "2.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a253edfdea34032da21af84e16fcc94f4c7a6ba237f9f3bc2dbf43cea8535c44" +dependencies = [ + "annotate-snippets", + "encoding_rs_io", + "granit-parser", + "num-traits", + "serde_core", + "smallvec", +] + [[package]] name = "serde_core" version = "1.0.229" @@ -784,19 +838,6 @@ dependencies = [ "zmij", ] -[[package]] -name = "serde_yaml" -version = "0.9.34+deprecated" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6a8b1a1a2ebf674015cc02edccce75287f1a0130d394307b36743c2f5d504b47" -dependencies = [ - "indexmap 2.6.0", - "itoa", - "ryu", - "serde", - "unsafe-libyaml", -] - [[package]] name = "sharded-slab" version = "0.1.4" @@ -806,6 +847,12 @@ dependencies = [ "lazy_static", ] +[[package]] +name = "simdutf8" +version = "0.1.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e3a9fe34e3e7a50316060351f37187a3f546bce95496156754b601a5fa71b76e" + [[package]] name = "similar" version = "3.2.0" @@ -972,10 +1019,10 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c0edd1e5b14653f783770bce4a4dabb4a5108a5370a5f5d8cfe8710c361f6c8b" [[package]] -name = "unsafe-libyaml" -version = "0.2.11" +name = "unicode-width" +version = "0.2.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "673aac59facbab8a9007c7f6108d11f63b603f7cabff99fabf650fea5c32b861" +checksum = "b4ac048d71ede7ee76d585517add45da530660ef4390e49b098733c6e897f254" [[package]] name = "utf8parse" diff --git a/Cargo.toml b/Cargo.toml index 1f4937b..6bcf46c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -38,7 +38,9 @@ rayon = "1.10.0" regex = "1.11.1" serde = { version = "1.0.219", features = ["derive"] } serde_json = "1.0.143" -serde_yaml = "0.9.34" +# Deserialize only: `default-features = false` leaves out the serializer and YAML comment +# capture, which we do not use. +serde-saphyr = { version = "2.0.0", default-features = false, features = ["deserialize"] } similar = "3.2.0" tempfile = "3.21.0" tracing = "0.1.41" diff --git a/README.md b/README.md index ebbafcf..433c198 100644 --- a/README.md +++ b/README.md @@ -214,6 +214,7 @@ codeowners gv --no-cache - `cache_directory` (default: `'tmp/cache/codeowners'`) - `ignore_dirs` (default includes: `.git`, `node_modules`, `tmp`, etc.) - `executable_name` (default: `'codeowners'`): Customize the command name shown in validation error messages. Useful when using `codeowners-rs` via wrappers like the [code_ownership](https://github.com/rubyatscale/code_ownership) Ruby gem. +- `yaml_max_nodes` (default: `20000`): The most YAML nodes (mappings, sequences and scalars) allowed in one team file or `package.yml`. A file over it, or over the limits on total scalar bytes and anchor copies that grow with it, fails with a `budget breached` error and a hint naming this key. Raise it if a large team file hits the limit; real files have at most a few thousand nodes. The memory a crafted file can make `codeowners` use grows with it too: about 290 MB at the default and about 710 MB at 50,000, measured on macOS. It does not apply to `config/code_ownership.yml` itself. Separately, any of these files, or the config, fails if it nests more than 16 levels deep, counting the top level. That limit can't be raised. Example configuration with custom executable name: diff --git a/src/common_test.rs b/src/common_test.rs index 57a8bd8..fdb0ffa 100644 --- a/src/common_test.rs +++ b/src/common_test.rs @@ -1,10 +1,6 @@ #[cfg(test)] pub mod tests { - use std::{ - error::Error, - fs::{self, File}, - path::PathBuf, - }; + use std::{error::Error, fs, path::PathBuf}; use indoc::indoc; @@ -111,8 +107,7 @@ pub mod tests { fs::write(test_config.temp_dir_path.join(&project_file.relative_path), &project_file.content)?; } - let config_file = File::open(test_config.temp_dir_path.join(test_config.relative_code_ownership_config_yml_path))?; - let config: Config = serde_yaml::from_reader(config_file)?; + let config = Config::load_from_path(&test_config.temp_dir_path.join(test_config.relative_code_ownership_config_yml_path))?; let codeowners_file_path = &test_config.temp_dir_path.join(".github/CODEOWNERS"); let cache: Cache = NoopCache::default().into(); diff --git a/src/config.rs b/src/config.rs index 620185f..3d7002a 100644 --- a/src/config.rs +++ b/src/config.rs @@ -1,36 +1,48 @@ use serde::Deserialize; -use std::{fs::File, path::Path}; +use std::path::Path; #[derive(Deserialize, Debug, Clone)] pub struct Config { + #[serde(deserialize_with = "crate::yaml::strings_or_nulls")] pub owned_globs: Vec, - #[serde(default = "ruby_package_paths")] + #[serde(default = "ruby_package_paths", deserialize_with = "crate::yaml::strings_or_nulls")] pub ruby_package_paths: Vec, - #[serde(alias = "js_package_paths", default = "javascript_package_paths")] + #[serde( + alias = "js_package_paths", + default = "javascript_package_paths", + deserialize_with = "crate::yaml::strings_or_nulls" + )] pub javascript_package_paths: Vec, - #[serde(default = "team_file_glob")] + #[serde(default = "team_file_glob", deserialize_with = "crate::yaml::strings_or_nulls")] pub team_file_glob: Vec, - #[serde(default = "unowned_globs")] + #[serde(default = "unowned_globs", deserialize_with = "crate::yaml::strings_or_nulls")] pub unowned_globs: Vec, - #[serde(alias = "unbuilt_gems_path", default = "vendored_gems_path")] + #[serde( + alias = "unbuilt_gems_path", + default = "vendored_gems_path", + deserialize_with = "crate::yaml::string_or_null" + )] pub vendored_gems_path: String, - #[serde(default = "default_cache_directory")] + #[serde(default = "default_cache_directory", deserialize_with = "crate::yaml::string_or_null")] pub cache_directory: String, - #[serde(default = "default_ignore_dirs")] + #[serde(default = "default_ignore_dirs", deserialize_with = "crate::yaml::strings_or_nulls")] pub ignore_dirs: Vec, - #[serde(default = "default_executable_name")] + #[serde(default = "default_executable_name", deserialize_with = "crate::yaml::string_or_null")] pub executable_name: String, - #[serde(default = "default_codeowners_path")] + #[serde(default = "default_codeowners_path", deserialize_with = "crate::yaml::string_or_null")] pub codeowners_path: String, + + #[serde(default = "default_yaml_max_nodes")] + pub yaml_max_nodes: usize, } #[allow(dead_code)] @@ -91,19 +103,22 @@ fn default_codeowners_path() -> String { ".github".to_string() } +fn default_yaml_max_nodes() -> usize { + crate::yaml::DEFAULT_MAX_NODES +} + impl Config { pub fn load_from_path(path: &Path) -> std::result::Result { - let file = File::open(path).map_err(|e| format!("Can't open config file: {} ({})", path.to_string_lossy(), e))?; - serde_yaml::from_reader(file).map_err(|e| format!("Can't parse config file: {} ({})", path.to_string_lossy(), e)) + let bytes = std::fs::read(path).map_err(|e| format!("Can't open config file: {} ({})", path.to_string_lossy(), e))?; + // The config can't raise its own limit, and at a few hundred nodes it never needs to. + crate::yaml::from_slice(&bytes, crate::yaml::DEFAULT_MAX_NODES) + .map_err(|e| format!("Can't parse config file: {} ({})", path.to_string_lossy(), e)) } } #[cfg(test)] mod tests { - use std::{ - error::Error, - fs::{self, File}, - }; + use std::{error::Error, fs}; use indoc::indoc; use tempfile::tempdir; @@ -120,8 +135,7 @@ mod tests { - \"{app,components,config,frontend,lib,packs,spec}/**/*.{rb,rake,js,jsx,ts,tsx,json,yml}\" "}; fs::write(&config_path, config_str)?; - let config_file = File::open(&config_path)?; - let config: Config = serde_yaml::from_reader(config_file)?; + let config = Config::load_from_path(&config_path)?; assert_eq!( config.owned_globs, vec!["{app,components,config,frontend,lib,packs,spec}/**/*.{rb,rake,js,jsx,ts,tsx,json,yml}"] @@ -135,6 +149,23 @@ mod tests { ); assert_eq!(config.vendored_gems_path, "vendored/"); assert_eq!(config.executable_name, "codeowners generate"); + assert_eq!(config.yaml_max_nodes, 20_000); + Ok(()) + } + + #[test] + fn test_parse_config_with_custom_yaml_max_nodes() -> Result<(), Box> { + let temp_dir = tempdir()?; + let config_path = temp_dir.path().join("config.yml"); + let config_str = indoc! {" + --- + owned_globs: + - \"**/*.rb\" + yaml_max_nodes: 200000 + "}; + fs::write(&config_path, config_str)?; + let config = Config::load_from_path(&config_path)?; + assert_eq!(config.yaml_max_nodes, 200_000); Ok(()) } @@ -149,8 +180,7 @@ mod tests { executable_name: my-custom-codeowners "}; fs::write(&config_path, config_str)?; - let config_file = File::open(&config_path)?; - let config: Config = serde_yaml::from_reader(config_file)?; + let config = Config::load_from_path(&config_path)?; assert_eq!(config.executable_name, "my-custom-codeowners"); Ok(()) } @@ -165,8 +195,7 @@ mod tests { - \"**/*.rb\" "}; fs::write(&config_path, config_str)?; - let config_file = File::open(&config_path)?; - let config: Config = serde_yaml::from_reader(config_file)?; + let config = Config::load_from_path(&config_path)?; assert_eq!(config.executable_name, "codeowners generate"); Ok(()) } @@ -181,8 +210,7 @@ mod tests { - \"**/*.rb\" "}; fs::write(&config_path, config_str)?; - let config_file = File::open(&config_path)?; - let config: Config = serde_yaml::from_reader(config_file)?; + let config = Config::load_from_path(&config_path)?; assert_eq!(config.codeowners_path, ".github"); Ok(()) } @@ -198,9 +226,67 @@ mod tests { codeowners_path: docs "}; fs::write(&config_path, config_str)?; - let config_file = File::open(&config_path)?; - let config: Config = serde_yaml::from_reader(config_file)?; + let config = Config::load_from_path(&config_path)?; assert_eq!(config.codeowners_path, "docs"); Ok(()) } + + #[test] + fn test_parse_config_with_empty_values() -> Result<(), Box> { + let temp_dir = tempdir()?; + let config_path = temp_dir.path().join("config.yml"); + let config_str = indoc! {" + --- + owned_globs: + - \"**/*.rb\" + - + ruby_package_paths: + - + javascript_package_paths: + - + team_file_glob: + - + unowned_globs: + - + ignore_dirs: + - + vendored_gems_path: + cache_directory: + executable_name: + codeowners_path: + "}; + fs::write(&config_path, config_str)?; + let config = Config::load_from_path(&config_path)?; + assert_eq!(config.owned_globs, vec!["**/*.rb", ""]); + for globs in [ + &config.ruby_package_paths, + &config.javascript_package_paths, + &config.team_file_glob, + &config.unowned_globs, + &config.ignore_dirs, + ] { + assert_eq!(globs, &[""]); + } + for value in [ + &config.vendored_gems_path, + &config.cache_directory, + &config.executable_name, + &config.codeowners_path, + ] { + assert_eq!(value, ""); + } + Ok(()) + } + + #[test] + fn test_load_from_path_reports_parse_error_on_one_line() -> Result<(), Box> { + let temp_dir = tempdir()?; + let config_path = temp_dir.path().join("config.yml"); + fs::write(&config_path, "owned_globs: {app: true}\n")?; + let error = Config::load_from_path(&config_path).unwrap_err(); + assert!(error.starts_with("Can't parse config file: "), "{error}"); + assert!(error.contains("line 1"), "{error}"); + assert!(!error.contains('\n'), "{error}"); + Ok(()) + } } diff --git a/src/lib.rs b/src/lib.rs index 13d53a4..eb35bd2 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -9,3 +9,4 @@ pub mod project_builder; pub mod project_file_builder; pub mod runner; pub(crate) mod tracked_files; +pub(crate) mod yaml; diff --git a/src/ownership/codeowners_file_parser.rs b/src/ownership/codeowners_file_parser.rs index fd25691..b14dbaf 100644 --- a/src/ownership/codeowners_file_parser.rs +++ b/src/ownership/codeowners_file_parser.rs @@ -14,6 +14,7 @@ pub struct Parser { pub project_root: PathBuf, pub codeowners_file_path: PathBuf, pub team_file_globs: Vec, + pub yaml_max_nodes: usize, } impl Parser { @@ -40,7 +41,7 @@ impl Parser { 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()); + let teams_by_name = teams_by_github_team_name(self.absolute_team_files_globs(), self.yaml_max_nodes); let result: HashMap> = file_inputs .par_iter() @@ -65,13 +66,13 @@ impl Parser { } #[memoize] -fn teams_by_github_team_name(team_file_glob: Vec) -> HashMap { +fn teams_by_github_team_name(team_file_glob: Vec, yaml_max_nodes: usize) -> HashMap { let mut teams = HashMap::new(); for glob in team_file_glob { match glob::glob(&glob) { Ok(paths) => { for path in paths.filter_map(Result::ok) { - let team = match Team::from_team_file_path(path.clone()) { + let team = match Team::from_team_file_path(path.clone(), yaml_max_nodes) { Ok(team) => team, Err(e) => { eprintln!("Error parsing team file: {e:?}, path: {}", path.display()); @@ -495,6 +496,32 @@ mod tests { Ok(()) } + #[test] + fn test_teams_from_files_paths_is_keyed_on_yaml_max_nodes() -> Result<(), Box> { + let temp_dir = tempfile::tempdir()?; + let root = temp_dir.path(); + fs::create_dir_all(root.join("config/teams"))?; + fs::write( + root.join("config/teams/payroll.yml"), + "name: Payroll\ngithub:\n team: '@PayrollTeam'\n", + )?; + fs::write(root.join("CODEOWNERS"), "# Team-specific owned globs\n/app/**/* @PayrollTeam\n")?; + let owner_of = |yaml_max_nodes: usize| -> Result, Box> { + let parser = Parser { + project_root: root.to_path_buf(), + codeowners_file_path: root.join("CODEOWNERS"), + team_file_globs: vec!["config/teams/**/*.yml".to_string()], + yaml_max_nodes, + }; + let teams = parser.teams_from_files_paths(&[PathBuf::from("app/a.rb")])?; + Ok(teams["app/a.rb"].as_ref().map(|team| team.name.clone())) + }; + + assert_eq!(owner_of(crate::yaml::DEFAULT_MAX_NODES)?, Some("Payroll".to_string())); + assert_eq!(owner_of(3)?, None, "a team file over the limit is skipped"); + Ok(()) + } + #[test] fn test_unignore_regex() -> Result<(), Box> { let un_ignore = Regex::new(r"^# \/")?; diff --git a/src/ownership/codeowners_query.rs b/src/ownership/codeowners_query.rs index 562bb48..3486e25 100644 --- a/src/ownership/codeowners_query.rs +++ b/src/ownership/codeowners_query.rs @@ -8,6 +8,7 @@ pub(crate) fn teams_for_files_from_codeowners( project_root: &Path, codeowners_file_path: &Path, team_file_globs: &[String], + yaml_max_nodes: usize, file_paths: &[String], ) -> Result>, String> { let relative_file_paths: Vec = file_paths @@ -26,6 +27,7 @@ pub(crate) fn teams_for_files_from_codeowners( codeowners_file_path: codeowners_file_path.to_path_buf(), project_root: project_root.to_path_buf(), team_file_globs: team_file_globs.to_vec(), + yaml_max_nodes, }; parser.teams_from_files_paths(&relative_file_paths).map_err(|e| e.to_string()) diff --git a/src/ownership/file_owner_resolver.rs b/src/ownership/file_owner_resolver.rs index cc7f9fa..42da95b 100644 --- a/src/ownership/file_owner_resolver.rs +++ b/src/ownership/file_owner_resolver.rs @@ -20,7 +20,7 @@ pub fn find_file_owners(project_root: &Path, config: &Config, file_path: &Path) }; let relative_file_path = crate::path_utils::relative_to_buf(project_root, &absolute_file_path); - let loaded = loaded_teams(teams_cache_root(project_root), config.team_file_glob.clone())?; + let loaded = loaded_teams(teams_cache_root(project_root), config.team_file_glob.clone(), config.yaml_max_nodes)?; let teams = &loaded.teams; let teams_by_name = &loaded.teams_by_name; @@ -105,9 +105,9 @@ struct LoadedTeams { teams_by_name: HashMap, } -type TeamCacheKey = (PathBuf, Vec); +type TeamCacheKey = (PathBuf, Vec, usize); -// Parsing every team file dominates a lookup, so load them once per project root and glob list and share +// Parsing every team file dominates a lookup, so load them once per project root, glob list and node limit and share // the result across threads for the life of the process. static TEAM_CACHE: LazyLock> = LazyLock::new(Default::default); @@ -122,8 +122,12 @@ fn team_cache() -> MutexGuard<'static, TeamCache> { TEAM_CACHE.lock().unwrap_or_else(PoisonError::into_inner) } -fn loaded_teams(project_root: PathBuf, team_file_globs: Vec) -> std::result::Result, String> { - let key = (project_root, team_file_globs); +fn loaded_teams( + project_root: PathBuf, + team_file_globs: Vec, + yaml_max_nodes: usize, +) -> std::result::Result, String> { + let key = (project_root, team_file_globs, yaml_max_nodes); let generation = { let cache = team_cache(); if let Some(loaded) = cache.loaded.get(&key) { @@ -132,7 +136,7 @@ fn loaded_teams(project_root: PathBuf, team_file_globs: Vec) -> std::res cache.generation }; - let load = load_teams(&key.0, &key.1)?; + let load = load_teams(&key.0, &key.1, key.2)?; let teams_by_name = build_teams_by_name_map(&load.teams); let loaded = Arc::new(LoadedTeams { teams: load.teams, @@ -178,7 +182,7 @@ struct TeamLoad { skipped_team_file: bool, } -fn load_teams(project_root: &Path, team_file_globs: &[String]) -> std::result::Result { +fn load_teams(project_root: &Path, team_file_globs: &[String], yaml_max_nodes: usize) -> std::result::Result { let mut teams: Vec = Vec::new(); let mut skipped_team_file = false; for glob_str in team_file_globs { @@ -193,7 +197,7 @@ fn load_teams(project_root: &Path, team_file_globs: &[String]) -> std::result::R continue; } }; - match Team::from_team_file_path(path.clone()) { + match Team::from_team_file_path(path.clone(), yaml_max_nodes) { Ok(team) => teams.push(team), Err(e) => { eprintln!("Error parsing team file: {e:?}, path: {}", path.display()); @@ -266,7 +270,7 @@ fn nearest_package_owner( if glob_list_matches(rel_str, &config.ruby_package_paths) { let pkg_yml = current.join("package.yml"); if pkg_yml.exists() { - match crate::project_builder::ruby_package_owner(&pkg_yml) { + match crate::project_builder::ruby_package_owner(&pkg_yml, config.yaml_max_nodes) { Ok(owner) => { if let Some(team) = owner.and_then(|owner| teams_by_name.get(&owner)) { let package_path = parent_rel.join("package.yml"); @@ -377,6 +381,7 @@ mod tests { ignore_dirs: vec![], executable_name: "codeowners".to_string(), codeowners_path: ".github".to_string(), + yaml_max_nodes: crate::yaml::DEFAULT_MAX_NODES, } } @@ -678,7 +683,11 @@ mod tests { #[test] fn test_clear_team_cache_during_a_load_is_not_undone_by_its_result() { let _guard = TEAM_CACHE_TEST_LOCK.lock().unwrap_or_else(|poisoned| poisoned.into_inner()); - let key: TeamCacheKey = (PathBuf::from("/clear-during-load"), vec!["config/teams/**/*.yml".to_string()]); + let key: TeamCacheKey = ( + PathBuf::from("/clear-during-load"), + vec!["config/teams/**/*.yml".to_string()], + crate::yaml::DEFAULT_MAX_NODES, + ); let loaded = || { Arc::new(LoadedTeams { teams: Vec::new(), @@ -826,4 +835,32 @@ mod tests { assert_eq!(owner_of(&config_b), Some("Billing".to_string())); assert_eq!(owner_of(&config_a), Some("Payroll".to_string())); } + + #[test] + fn test_team_cache_is_keyed_on_yaml_max_nodes() { + let td = tempdir().unwrap(); + let root = td.path(); + fs::create_dir_all(root.join("config/teams")).unwrap(); + fs::write( + root.join("config/teams/payroll.yml"), + "name: Payroll\ngithub:\n team: '@PayrollTeam'\nowned_globs:\n - app/**/*\n", + ) + .unwrap(); + let config_with_max_nodes = |yaml_max_nodes: usize| crate::config::Config { + yaml_max_nodes, + ..build_config_for_temp("frontend/**/*", "packs/**/*", "vendored") + }; + let owner_of = |config: &crate::config::Config| { + find_file_owners(root, config, Path::new("app/a.rb")) + .unwrap() + .first() + .map(|owner| owner.team.name.clone()) + }; + + assert_eq!( + owner_of(&config_with_max_nodes(crate::yaml::DEFAULT_MAX_NODES)), + Some("Payroll".to_string()) + ); + assert_eq!(owner_of(&config_with_max_nodes(3)), None, "a team file over the limit is skipped"); + } } diff --git a/src/project.rs b/src/project.rs index 12b09a6..3deb58e 100644 --- a/src/project.rs +++ b/src/project.rs @@ -2,7 +2,6 @@ use core::fmt; use std::{ collections::HashMap, fmt::Display, - fs::File, path::{Path, PathBuf}, }; @@ -44,9 +43,9 @@ pub struct Team { } impl Team { - pub fn from_team_file_path(absolute_path: PathBuf) -> Result> { - let file = File::open(&absolute_path).change_context(Error::Io)?; - let deserializer: deserializers::Team = serde_yaml::from_reader(file).change_context(Error::SerdeYaml)?; + pub fn from_team_file_path(absolute_path: PathBuf, yaml_max_nodes: usize) -> Result> { + let bytes = std::fs::read(&absolute_path).change_context(Error::Io)?; + let deserializer: deserializers::Team = crate::yaml::from_project_slice(&bytes, yaml_max_nodes).change_context(Error::SerdeYaml)?; Ok(Self { path: absolute_path.to_owned(), name: deserializer.name, @@ -117,6 +116,7 @@ pub mod deserializers { #[derive(Deserialize)] pub struct Github { + #[serde(deserialize_with = "crate::yaml::string_or_null")] pub team: String, #[serde(default = "bool_false")] pub do_not_add_to_codeowners_file: bool, @@ -124,20 +124,25 @@ pub mod deserializers { #[derive(Deserialize)] pub struct Ruby { - #[serde(default = "empty_string_vec")] + #[serde(default = "empty_string_vec", deserialize_with = "crate::yaml::strings_or_nulls")] pub owned_gems: Vec, } #[derive(Deserialize)] pub struct Team { + #[serde(deserialize_with = "crate::yaml::string_or_null")] pub name: String, pub github: Github, pub ruby: Option, - #[serde(default = "empty_string_vec")] + #[serde(default = "empty_string_vec", deserialize_with = "crate::yaml::strings_or_nulls")] pub owned_globs: Vec, - #[serde(alias = "unowned_globs", default = "empty_string_vec")] + #[serde( + alias = "unowned_globs", + default = "empty_string_vec", + deserialize_with = "crate::yaml::strings_or_nulls" + )] pub subtracted_globs: Vec, } @@ -200,6 +205,8 @@ impl Project { #[cfg(test)] mod tests { + use indoc::indoc; + use super::*; #[test] @@ -229,4 +236,72 @@ mod tests { assert_eq!(map.get("a").unwrap().name, vg1.name); assert_eq!(map.get("b").unwrap().name, vg2.name); } + + fn team_from_yaml(yaml: &str) -> Result> { + let temp_file = tempfile::NamedTempFile::new().unwrap(); + std::fs::write(temp_file.path(), yaml).unwrap(); + Team::from_team_file_path(temp_file.path().to_path_buf(), crate::yaml::DEFAULT_MAX_NODES) + } + + #[test] + fn test_team_from_team_file_path_accepts_empty_values() { + let yaml = indoc! {" + name: + github: + team: + do_not_add_to_codeowners_file: true + owned_globs: + - app/payroll/** + - + subtracted_globs: + - + ruby: + owned_gems: + - + "}; + + let team = team_from_yaml(yaml).unwrap(); + assert_eq!(team.name, ""); + assert_eq!(team.github_team, ""); + assert!(team.avoid_ownership); + assert_eq!(team.owned_globs, vec!["app/payroll/**".to_string(), "".to_string()]); + assert_eq!(team.subtracted_globs, vec!["".to_string()]); + assert_eq!(team.owned_gems, vec!["".to_string()]); + } + + #[test] + fn test_team_from_team_file_path_ignores_duplicate_unknown_keys() { + let yaml = indoc! {" + name: Payroll + github: + team: '@acme/payroll' + members: + - a + members: + - b + slack_channel: '#a' + slack_channel: '#b' + "}; + + let team = team_from_yaml(yaml).unwrap(); + assert_eq!(team.github_team, "@acme/payroll"); + } + + #[test] + fn test_team_from_team_file_path_reads_only_true_and_false_as_booleans() { + let yaml = "name: Payroll\ngithub:\n team: '@acme/payroll'\n do_not_add_to_codeowners_file: True\n"; + assert!(team_from_yaml(yaml).unwrap().avoid_ownership); + + let yaml = "name: Payroll\ngithub:\n team: '@acme/payroll'\n do_not_add_to_codeowners_file: yes\n"; + assert!(team_from_yaml(yaml).is_err()); + } + + #[test] + fn test_team_from_team_file_path_reports_missing_field_once() { + let error = team_from_yaml("name: Payroll\n").unwrap_err(); + + let message = format!("{error:?}"); + assert!(message.contains("missing field `github`"), "{message}"); + assert_eq!(message.matches("missing field").count(), 1, "{message}"); + } } diff --git a/src/project_builder.rs b/src/project_builder.rs index e0d06ee..591a65b 100644 --- a/src/project_builder.rs +++ b/src/project_builder.rs @@ -212,6 +212,7 @@ impl<'a> ProjectBuilder<'a> { Vec, ); + let yaml_max_nodes = self.config.yaml_max_nodes; let (project_files, packages, vendored_gems, directory_codeowners, teams): Accumulator = entry_types .into_par_iter() .try_fold( @@ -242,7 +243,7 @@ impl<'a> ProjectBuilder<'a> { } } EntryType::RubyPackage(absolute_path, relative_path) => { - match ruby_package_owner(&absolute_path) + match ruby_package_owner(&absolute_path, yaml_max_nodes) .attach_with(|| format!("Failed to read ruby package: {}", absolute_path.display())) { Ok(Some(owner)) => { @@ -282,7 +283,7 @@ impl<'a> ProjectBuilder<'a> { }); } EntryType::TeamFile(absolute_path, _relative_path) => { - let team = Team::from_team_file_path(absolute_path.clone()) + let team = Team::from_team_file_path(absolute_path.clone(), yaml_max_nodes) .change_context(Error::Io) .attach_with(|| format!("Failed to read team file: {}", absolute_path.display()))?; team_files.push(team); @@ -336,9 +337,14 @@ fn matches_globs(path: &Path, globs: &[String]) -> bool { } } -pub(crate) fn ruby_package_owner(path: &Path) -> Result, Report> { - let file = File::open(path).change_context(Error::Io)?; - let deserializer: deserializers::RubyPackage = serde_yaml::from_reader(file).change_context(Error::SerdeYaml)?; +pub(crate) fn ruby_package_owner(path: &Path, yaml_max_nodes: usize) -> Result, Report> { + let bytes = std::fs::read(path).change_context(Error::Io)?; + // An empty, comment-only or `---`-only file is a null document, which serde-saphyr 2.1 no longer reads as a struct. + let package: Option = + crate::yaml::from_project_slice(&bytes, yaml_max_nodes).change_context(Error::SerdeYaml)?; + let Some(deserializer) = package else { + return Ok(None); + }; let top_level_owner = deserializer.owner; let metadata_owner = deserializer.metadata.and_then(|metadata| metadata.owner); @@ -364,6 +370,8 @@ fn javascript_package_owner(path: &Path) -> Result, Report #[cfg(test)] mod tests { + use indoc::indoc; + use super::*; const OWNED_GLOB: &str = "{app,components,config,frontend,lib,packs,spec,danger,script}/**/*.{rb,arb,erb,rake,js,jsx,ts,tsx}"; @@ -384,7 +392,7 @@ mod tests { let temp_file = tempfile::NamedTempFile::new().unwrap(); std::fs::write(temp_file.path(), yaml).unwrap(); - let owner = ruby_package_owner(temp_file.path()).unwrap(); + let owner = ruby_package_owner(temp_file.path(), crate::yaml::DEFAULT_MAX_NODES).unwrap(); assert_eq!(owner, Some("TeamA".to_string())); } @@ -394,7 +402,7 @@ mod tests { let temp_file = tempfile::NamedTempFile::new().unwrap(); std::fs::write(temp_file.path(), yaml).unwrap(); - let owner = ruby_package_owner(temp_file.path()).unwrap(); + let owner = ruby_package_owner(temp_file.path(), crate::yaml::DEFAULT_MAX_NODES).unwrap(); assert_eq!(owner, Some("TeamB".to_string())); } @@ -404,7 +412,7 @@ mod tests { let temp_file = tempfile::NamedTempFile::new().unwrap(); std::fs::write(temp_file.path(), yaml).unwrap(); - let result = ruby_package_owner(temp_file.path()); + let result = ruby_package_owner(temp_file.path(), crate::yaml::DEFAULT_MAX_NODES); assert!(result.is_err()); } @@ -414,7 +422,7 @@ mod tests { let temp_file = tempfile::NamedTempFile::new().unwrap(); std::fs::write(temp_file.path(), yaml).unwrap(); - let owner = ruby_package_owner(temp_file.path()).unwrap(); + let owner = ruby_package_owner(temp_file.path(), crate::yaml::DEFAULT_MAX_NODES).unwrap(); assert_eq!(owner, Some("TeamA".to_string())); } @@ -424,7 +432,67 @@ mod tests { let temp_file = tempfile::NamedTempFile::new().unwrap(); std::fs::write(temp_file.path(), yaml).unwrap(); - let owner = ruby_package_owner(temp_file.path()).unwrap(); + let owner = ruby_package_owner(temp_file.path(), crate::yaml::DEFAULT_MAX_NODES).unwrap(); assert_eq!(owner, None); } + + #[test] + fn test_ruby_package_owner_empty_file_has_no_owner() { + for yaml in [ + "", + "\u{feff}", + "# no keys yet\n", + "---\n", + "--- # no keys yet\n", + "~\n", + "null\n", + "...\n", + ] { + let temp_file = tempfile::NamedTempFile::new().unwrap(); + std::fs::write(temp_file.path(), yaml).unwrap(); + + let owner = ruby_package_owner(temp_file.path(), crate::yaml::DEFAULT_MAX_NODES).expect(yaml); + assert_eq!(owner, None, "{yaml:?}"); + } + } + + #[test] + fn test_ruby_package_owner_ignores_duplicate_unknown_keys() { + let yaml = indoc! {" + enforce_dependencies: true + enforce_privacy: true + enforce_privacy: false + metadata: + slack: '#a' + slack: '#b' + owner: TeamA + "}; + let temp_file = tempfile::NamedTempFile::new().unwrap(); + std::fs::write(temp_file.path(), yaml).unwrap(); + + let owner = ruby_package_owner(temp_file.path(), crate::yaml::DEFAULT_MAX_NODES).unwrap(); + assert_eq!(owner, Some("TeamA".to_string())); + } + + #[test] + fn test_ruby_package_owner_duplicate_owner_last_wins() { + let yaml = "owner: TeamA\nowner: TeamB\n"; + let temp_file = tempfile::NamedTempFile::new().unwrap(); + std::fs::write(temp_file.path(), yaml).unwrap(); + + let owner = ruby_package_owner(temp_file.path(), crate::yaml::DEFAULT_MAX_NODES).unwrap(); + assert_eq!(owner, Some("TeamB".to_string())); + } + + #[test] + fn test_ruby_package_owner_over_the_node_limit_names_yaml_max_nodes() { + let temp_file = tempfile::NamedTempFile::new().unwrap(); + std::fs::write(temp_file.path(), "owner: TeamA\nignored: [a, b, c, d, e]\n").unwrap(); + + let error = ruby_package_owner(temp_file.path(), 5).unwrap_err(); + assert!( + format!("{error:?}").contains("raise `yaml_max_nodes` (now 5) in config/code_ownership.yml to allow larger files"), + "{error:?}" + ); + } } diff --git a/src/runner/api.rs b/src/runner/api.rs index bf4e9c9..8de27bf 100644 --- a/src/runner/api.rs +++ b/src/runner/api.rs @@ -69,6 +69,7 @@ pub fn teams_for_files_from_codeowners( &run_config.project_root, &codeowners_file_path, &config.team_file_glob, + config.yaml_max_nodes, file_paths, ) .map_err(Error::Io)?; diff --git a/src/yaml.rs b/src/yaml.rs new file mode 100644 index 0000000..0a7d6dd --- /dev/null +++ b/src/yaml.rs @@ -0,0 +1,306 @@ +//! Options keep serde_yaml 0.9's semantics where serde-saphyr allows it: these files are written for Ruby's Psych, +//! and most package.yml keys are ones we never read, so rejecting a file over one would abort the whole run. +use error_stack::Report; +use serde::de::DeserializeOwned; +use serde::{Deserialize, Deserializer}; +use serde_saphyr::{DuplicateKeyPolicy, MergeKeyPolicy, NonFiniteFloatPolicy, Options, budget::BudgetBreach}; + +/// Default for the `yaml_max_nodes` config key. Real files have at most a few thousand nodes. +pub(crate) const DEFAULT_MAX_NODES: usize = 20_000; + +fn options(max_nodes: usize) -> Options { + serde_saphyr::options! { + // LastWins re-buffers nested mappings once per level, so the default budget lets a 500 KB file take gigabytes. + // Real files nest about 5 deep; the depth limit also prevents stack overflow. + // The other caps grow with `yaml_max_nodes`, so raising it is all a larger file needs. + budget: serde_saphyr::budget! { + max_depth: 16, + max_nodes: max_nodes, + max_events: max_nodes.saturating_mul(50), + max_total_scalar_bytes: max_nodes.saturating_mul(256), + max_recorded_anchor_events: max_nodes.saturating_mul(5), + max_recorded_anchor_bytes: max_nodes.saturating_mul(64), + }, + // The default rejects a repeated key anywhere, including in values we ignore. Last wins, as in Psych. + duplicate_keys: DuplicateKeyPolicy::LastWins, + // serde_yaml treated `<<` as an ordinary key. + merge_keys: MergeKeyPolicy::AsOrdinary, + // Ignored values are still parsed, and the default rejects `.inf`, `.nan` and `1e999` in them. + non_finite_float_policy: NonFiniteFloatPolicy::PassThrough, + // serde_yaml neither decoded nor validated `!!binary`. + ignore_binary_tag_for_string: true, + // serde_yaml rejected YAML 1.1 booleans such as `yes` and `off`. + strict_booleans: true, + // Snippets add several lines to each error, which serde_yaml kept to one. + with_snippet: false, + } +} + +/// Takes bytes, not a reader: `from_reader` transcodes after a BOM, replacing invalid UTF-8 and accepting UTF-16, unlike serde_yaml. +pub(crate) fn from_slice(bytes: &[u8], max_nodes: usize) -> Result { + serde_saphyr::from_slice_with_options(bytes, options(max_nodes)) +} + +/// [`from_slice`] for team files and package.yml, whose size limits come from the `yaml_max_nodes` config key: a file +/// over them gets a hint naming that key. +pub(crate) fn from_project_slice(bytes: &[u8], max_nodes: usize) -> Result> { + from_slice(bytes, max_nodes).map_err(|error| { + let mut original = &error; + while let serde_saphyr::Error::AliasError { error, .. } = original { + original = error; + } + let too_large = matches!( + original, + serde_saphyr::Error::Budget { + breach: BudgetBreach::Nodes { .. } + | BudgetBreach::Events { .. } + | BudgetBreach::ScalarBytes { .. } + | BudgetBreach::RecordedAnchorEvents { .. } + | BudgetBreach::RecordedAnchorBytes { .. }, + .. + } + ); + let report = Report::new(YamlError(error)); + if too_large { + report.attach(format!( + "raise `yaml_max_nodes` (now {max_nodes}) in config/code_ownership.yml to allow larger files" + )) + } else { + report + } + }) +} + +/// A serde-saphyr error with no `source()`. An alias error's message already includes the error it wraps, and +/// error_stack would print that error again as its source. +#[derive(Debug)] +pub(crate) struct YamlError(pub(crate) serde_saphyr::Error); + +impl std::fmt::Display for YamlError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + std::fmt::Display::fmt(&self.0, f) + } +} + +impl core::error::Error for YamlError {} + +/// Reads an empty, `~` or `null` value as `""` instead of failing. serde_yaml accepted these, keeping `~` and `null` as text. +pub(crate) fn string_or_null<'de, D: Deserializer<'de>>(deserializer: D) -> Result { + Ok(Option::::deserialize(deserializer)?.unwrap_or_default()) +} + +/// [`string_or_null`] for each item of a list. Reads an empty, `~` or `null` list as empty itself, since serde-saphyr 2.1 +/// stops doing that for a `Vec` (serde-saphyr#233), and `unowned_globs:` with no value is common. +pub(crate) fn strings_or_nulls<'de, D: Deserializer<'de>>(deserializer: D) -> Result, D::Error> { + Ok(Option::>>::deserialize(deserializer)? + .unwrap_or_default() + .into_iter() + .map(Option::unwrap_or_default) + .collect()) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::project::deserializers::RubyPackage; + + fn owner(yaml: &[u8]) -> Result, serde_saphyr::Error> { + from_slice::(yaml, DEFAULT_MAX_NODES).map(|package| package.owner) + } + + #[test] + fn test_ignores_unusual_values_in_unknown_keys() { + for unknown in [ + "<<: 5", + "error_budget: .inf", + "sample_rate: .nan", + "huge: 1e999", + "blob: !!binary not base64", + "legacy: !ruby/object:OpenStruct {table: {}}", + ] { + let yaml = format!("owner: TeamA\n{unknown}\n"); + assert_eq!(owner(yaml.as_bytes()).expect(unknown), Some("TeamA".to_string()), "{unknown}"); + } + } + + #[test] + fn test_reads_empty_list_as_empty() { + #[derive(Deserialize)] + struct Globs { + #[serde(deserialize_with = "strings_or_nulls")] + globs: Vec, + } + + for yaml in ["globs:\n", "globs: ~\n", "globs: null\n", "globs: []\n"] { + let parsed: Globs = from_slice(yaml.as_bytes(), DEFAULT_MAX_NODES).expect(yaml); + assert!(parsed.globs.is_empty(), "{yaml:?}"); + } + } + + #[test] + fn test_strings_or_nulls_reads_null_as_empty_list() { + use serde::de::IntoDeserializer; + // serde-saphyr 2.0 would read null as an empty list even without the helper; serde's unit deserializer does not. + let null: serde::de::value::UnitDeserializer = ().into_deserializer(); + assert_eq!(strings_or_nulls(null).unwrap(), Vec::::new()); + } + + #[test] + fn test_does_not_apply_merge_keys() { + let yaml = "defaults: &defaults\n owner: TeamA\n<<: *defaults\nmetadata:\n <<: {owner: TeamA}\n"; + let package: RubyPackage = from_slice(yaml.as_bytes(), DEFAULT_MAX_NODES).unwrap(); + assert_eq!(package.owner, None); + assert_eq!(package.metadata.and_then(|metadata| metadata.owner), None); + } + + #[test] + fn test_reads_utf8_bom_before_several_keys() { + assert_eq!( + owner("\u{feff}enforce_dependencies: true\nowner: TeamA\n".as_bytes()).unwrap(), + Some("TeamA".to_string()) + ); + } + + #[test] + fn test_rejects_invalid_utf8_after_bom() { + assert!(owner(b"\xef\xbb\xbfowner: Caf\xe9\n").is_err()); + } + + #[test] + fn test_rejects_deeply_nested_values() { + let nested = |lists: usize| format!("owner: TeamA\nignored: {}{}\n", "[".repeat(lists), "]".repeat(lists)); + + // 16 levels, counting the top-level mapping, are allowed. + assert_eq!(owner(nested(15).as_bytes()).unwrap(), Some("TeamA".to_string())); + assert!(matches!( + owner(nested(16).as_bytes()), + Err(serde_saphyr::Error::Budget { + breach: BudgetBreach::Depth { .. }, + .. + }) + )); + + // Raising `yaml_max_nodes` does not raise the depth limit, so its breach gets no hint. + let Err(report) = from_project_slice::(nested(16).as_bytes(), DEFAULT_MAX_NODES) else { + panic!("17 levels should be rejected"); + }; + assert!(!format!("{report:?}").contains("yaml_max_nodes"), "{report:?}"); + } + + #[test] + fn test_rejects_too_many_nodes() { + let owner_with_items = |count: usize, max_nodes: usize| { + let yaml = format!("owner: TeamA\nignored: [{}]\n", vec!["x"; count].join(", ")); + from_slice::(yaml.as_bytes(), max_nodes).map(|package| package.owner) + }; + + assert_eq!(owner_with_items(10_000, DEFAULT_MAX_NODES).unwrap(), Some("TeamA".to_string())); + assert!(matches!( + owner_with_items(100_000, DEFAULT_MAX_NODES), + Err(serde_saphyr::Error::Budget { .. }) + )); + assert_eq!(owner_with_items(100_000, 200_000).unwrap(), Some("TeamA".to_string())); + assert!(matches!(owner_with_items(1_000, 500), Err(serde_saphyr::Error::Budget { .. }))); + } + + #[test] + fn test_anchor_copy_limits_grow_with_max_nodes() { + // Ten nested anchored lists copy each of their 10,001 items into ten anchor buffers. + let nested_anchors = format!( + "owner: TeamA\nignored: {}{}{}\n", + (0..10).map(|i| format!("&a{i} [")).collect::(), + vec!["x"; 10_001].join(", "), + "]".repeat(10) + ); + // The escape makes the scalar owned, so its 1.3 MB count toward the anchor-copy byte limit. + let owned_anchor = format!("owner: TeamA\nbig: &a \"{}\\t\"\n", "x".repeat(1_300_000)); + + for yaml in [&nested_anchors, &owned_anchor] { + let Err(report) = from_project_slice::(yaml.as_bytes(), DEFAULT_MAX_NODES) else { + panic!("should exceed an anchor-copy limit at the default yaml_max_nodes"); + }; + assert!( + matches!( + &report.current_context().0, + serde_saphyr::Error::Budget { + breach: BudgetBreach::RecordedAnchorEvents { .. } | BudgetBreach::RecordedAnchorBytes { .. }, + .. + } + ), + "{report:?}" + ); + assert!(format!("{report:?}").contains("raise `yaml_max_nodes`"), "{report:?}"); + assert_eq!( + from_slice::(yaml.as_bytes(), 40_000).unwrap().owner, + Some("TeamA".to_string()) + ); + } + } + + #[test] + fn test_event_limit_grows_with_max_nodes() { + // A node yields up to about three parser events, so a fixed event cap such as serde-saphyr's default of 1,000,000 + // would reject files that a raised `yaml_max_nodes` allows. + let budget = options(700_000).budget.unwrap(); + assert!(budget.max_events > serde_saphyr::Budget::default().max_events); + } + + #[test] + fn test_event_limit_breach_names_yaml_max_nodes() { + // The event cap is reached before the node cap only at a limit of 0. + let Err(report) = from_project_slice::(b"owner: TeamA\n", 0) else { + panic!("a yaml_max_nodes of 0 should reject every file"); + }; + assert!( + matches!( + &report.current_context().0, + serde_saphyr::Error::Budget { + breach: BudgetBreach::Events { .. }, + .. + } + ), + "{report:?}" + ); + assert!(format!("{report:?}").contains("raise `yaml_max_nodes` (now 0)"), "{report:?}"); + } + + #[test] + fn test_scalar_byte_limit_grows_with_max_nodes() { + let yaml = format!("owner: TeamA\nignored: [{}]\n", vec!["x".repeat(400); 15_000].join(", ")); + + let Err(report) = from_project_slice::(yaml.as_bytes(), DEFAULT_MAX_NODES) else { + panic!("15,000 strings of 400 bytes should exceed the default scalar-byte limit"); + }; + assert!(matches!( + &report.current_context().0, + serde_saphyr::Error::Budget { + breach: BudgetBreach::ScalarBytes { .. }, + .. + } + )); + assert!(format!("{report:?}").contains("raise `yaml_max_nodes`"), "{report:?}"); + assert_eq!( + from_slice::(yaml.as_bytes(), 40_000).unwrap().owner, + Some("TeamA".to_string()) + ); + + let aliased = format!( + "owner: TeamA\nbig: &a \"{}\"\nignored: [{}]\n", + "x".repeat(1_000_000), + ["*a"; 8].join(", ") + ); + let Err(report) = from_project_slice::(aliased.as_bytes(), DEFAULT_MAX_NODES) else { + panic!("eight aliases of a 1 MB string should exceed the default scalar-byte limit"); + }; + assert!(format!("{report:?}").contains("raise `yaml_max_nodes`"), "{report:?}"); + } + + #[test] + fn test_reports_an_alias_error_once() { + let Err(report) = from_project_slice::(b"x: &x [a]\nowner: *x\n", DEFAULT_MAX_NODES) else { + panic!("a sequence is not a valid owner"); + }; + let message = format!("{report:?}"); + assert_eq!(message.matches("expected string scalar").count(), 1, "{message}"); + } +} diff --git a/tests/yaml_max_nodes_test.rs b/tests/yaml_max_nodes_test.rs new file mode 100644 index 0000000..ab681f1 --- /dev/null +++ b/tests/yaml_max_nodes_test.rs @@ -0,0 +1,87 @@ +use assert_cmd::Command; +use predicates::prelude::*; +use std::{error::Error, fs, path::Path}; +use tempfile::TempDir; + +mod common; + +use common::git_add_all_files; +use common::setup_fixture_repo; + +// 30,000 ignored list items put a package.yml and a team file over the default `yaml_max_nodes` of 20,000. +fn project_with_large_yaml_files(yaml_max_nodes: Option) -> TempDir { + let temp_dir = setup_fixture_repo(Path::new("tests/fixtures/valid_project")); + let root = temp_dir.path(); + let ignored = format!("ignored: [{}]\n", vec!["x"; 30_000].join(", ")); + + fs::write( + root.join("ruby/packages/payroll_flow/package.yml"), + format!("owner: Payroll\n{ignored}"), + ) + .unwrap(); + fs::create_dir_all(root.join("ruby/packages/payroll_flow/app")).unwrap(); + fs::write(root.join("ruby/packages/payroll_flow/app/flow.rb"), "class Flow; end\n").unwrap(); + + let team_path = root.join("config/teams/payroll.yml"); + let team = fs::read_to_string(&team_path).unwrap(); + fs::write(&team_path, format!("{team}{ignored}")).unwrap(); + + if let Some(yaml_max_nodes) = yaml_max_nodes { + let config_path = root.join("config/code_ownership.yml"); + let config = fs::read_to_string(&config_path).unwrap(); + fs::write(&config_path, format!("{config}\nyaml_max_nodes: {yaml_max_nodes}\n")).unwrap(); + } + + git_add_all_files(root); + temp_dir +} + +fn codeowners(project_root: &Path, args: &[&str]) -> Command { + let mut cmd = Command::cargo_bin("codeowners").unwrap(); + cmd.arg("--project-root").arg(project_root).arg("--no-cache").args(args); + cmd +} + +#[test] +fn test_default_yaml_max_nodes_rejects_large_files() -> Result<(), Box> { + let temp_dir = project_with_large_yaml_files(None); + let root = temp_dir.path(); + + codeowners(root, &["generate"]) + .assert() + .failure() + .stderr(predicate::str::contains("budget breached").and(predicate::str::contains("raise `yaml_max_nodes` (now 20000)"))); + codeowners(root, &["for-file", "ruby/packages/payroll_flow/app/flow.rb"]) + .assert() + .stderr(predicate::str::contains("Error reading ruby package").and(predicate::str::contains("budget breached"))); + // generate stops at whichever large file it reads first, but --from-codeowners reads only team files. + codeowners(root, &["for-file", "--from-codeowners", "ruby/packages/payroll_flow/app/flow.rb"]) + .assert() + .stderr( + predicate::str::contains("Error parsing team file") + .and(predicate::str::contains("budget breached")) + .and(predicate::str::contains("raise `yaml_max_nodes` (now 20000)")), + ); + + Ok(()) +} + +#[test] +fn test_raised_yaml_max_nodes_accepts_large_files() -> Result<(), Box> { + let temp_dir = project_with_large_yaml_files(Some(100_000)); + let root = temp_dir.path(); + + codeowners(root, &["generate"]).assert().success().stderr(""); + codeowners(root, &["for-file", "ruby/packages/payroll_flow/app/flow.rb"]) + .assert() + .success() + .stdout(predicate::str::contains("Team: Payroll")) + .stderr(""); + codeowners(root, &["for-file", "--from-codeowners", "ruby/packages/payroll_flow/app/flow.rb"]) + .assert() + .success() + .stdout(predicate::str::contains("Team: Payroll")) + .stderr(""); + + Ok(()) +}