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/.github/workflows/ci.yml b/.github/workflows/ci.yml index b31e0a4..b6bf4ee 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -32,6 +32,11 @@ jobs: - name: Run cargo check run: cargo check + + - name: Collect the release archives' license notices + # The release jobs run only on main, so this catches a dependency change that breaks the + # script on its pull request rather than at release time. + run: dev/license-notices "$RUNNER_TEMP/licenses" test: name: Test Suite runs-on: ubuntu-latest @@ -129,10 +134,14 @@ jobs: # This combines the intel and m1 binaries into a single binary lipo -create -output target/codeowners target/aarch64-apple-darwin/release/codeowners target/x86_64-apple-darwin/release/codeowners - # Creates artifact for homebrew. -C means run from `target` directory - tar -czf target/codeowners-mac.tar.gz -C target codeowners + # Notices that some dependencies' licenses require redistributions of the binary to carry + dev/license-notices target/licenses + + # Creates artifact for homebrew. -C means run from `target` directory, which keeps the + # binary at the archive root, where dotslash-config.json looks for it + tar -czf target/codeowners-mac.tar.gz -C target codeowners licenses - # This tarball is a binary that is executable + # This tarball holds the executable binary and the license notices gh release upload $NEW_VERSION target/codeowners-mac.tar.gz env: @@ -158,8 +167,12 @@ jobs: cross build --release --target aarch64-unknown-linux-gnu - name: Upload linux binaries run: | - tar -czf target/x86_64-unknown-linux-gnu.tar.gz -C target/x86_64-unknown-linux-gnu/release codeowners - tar -czf target/aarch64-unknown-linux-gnu.tar.gz -C target/aarch64-unknown-linux-gnu/release codeowners + # Notices that some dependencies' licenses require redistributions of the binary to carry. + # Each -C keeps the binary at its archive's root, where dotslash-config.json looks for it. + dev/license-notices target/x86_64-unknown-linux-gnu/release/licenses + dev/license-notices target/aarch64-unknown-linux-gnu/release/licenses + tar -czf target/x86_64-unknown-linux-gnu.tar.gz -C target/x86_64-unknown-linux-gnu/release codeowners licenses + tar -czf target/aarch64-unknown-linux-gnu.tar.gz -C target/aarch64-unknown-linux-gnu/release codeowners licenses gh release upload $NEW_VERSION target/x86_64-unknown-linux-gnu.tar.gz gh release upload $NEW_VERSION target/aarch64-unknown-linux-gnu.tar.gz env: 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..bec715a 100644 --- a/README.md +++ b/README.md @@ -47,6 +47,8 @@ You can run `codeowners` without installing a platform-specific binary by using 2. Download the latest DotSlash text file from a release, for example [https://github.com/rubyatscale/codeowners-rs/releases](https://github.com/rubyatscale/codeowners-rs/releases). 3. Execute the downloaded file with DotSlash; it will fetch and run the correct binary. +Release archives after v0.5.0 also hold a `licenses` directory next to the `codeowners` binary. It has the license notices that the dependencies listed in `dev/license-notices` require redistributions of the binary to include. + ### Option B: From source with Cargo Requires Rust toolchain. @@ -214,6 +216,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 to raise this key in the config file in use. Raise it if a large team file hits the limit; real files have at most a few thousand nodes. It does not apply to the config file itself. Raising it also raises how much memory a single file can take: a file crafted to waste memory can make `codeowners` use about 280 MB more at the default and about 700 MB more at 50,000, measured on macOS, while ordinary files add almost nothing. 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/dev/license-notices b/dev/license-notices new file mode 100755 index 0000000..94987f8 --- /dev/null +++ b/dev/license-notices @@ -0,0 +1,66 @@ +#!/usr/bin/env bash +# Copies, verbatim, the license files of the crates listed below into /-/ for +# the release archives, since their licenses require redistributions of the binary to carry those +# notices. Versions come from Cargo.lock. +# +# Usage: dev/license-notices +# +# must not exist yet, so an archive never carries notices left over from other versions. Fails +# if a listed crate is no longer a dependency or ships no license files, so whoever drops or +# replaces one updates the list. +# +# Keep this bash 3.2 compatible: the mac release job runs it with macOS's /bin/bash, while the +# pull request check runs it on ubuntu. + +set -euo pipefail +shopt -s nullglob nocaseglob + +crates=( + # serde-saphyr's parser. Its LICENSE requires redistributions to include its notice and the ones + # it inherited, which it keeps in .licenses/. + granit-parser + # serde-saphyr decodes reader input with it, through encoding_rs_io. Its WHATWG-derived data is + # under BSD-3-Clause, which requires binary redistributions to reproduce that notice. + encoding_rs +) + +if [[ $# -ne 1 ]]; then + echo "usage: $0 " >&2 + exit 2 +fi +out=$1 +if [[ -e $out ]]; then + echo "$0: $out already exists" >&2 + exit 1 +fi + +manifest="$(dirname "$0")/../Cargo.toml" +metadata=$(cargo metadata --format-version 1 --locked --manifest-path "$manifest") +mkdir -p "$out" + +for crate in "${crates[@]}"; do + # One "\t" line per version of the crate in Cargo.lock. + packages=$(jq -r --arg name "$crate" \ + '.packages[] | select(.name == $name) | "\(.version)\t\(.manifest_path)"' <<<"$metadata") + if [[ -z $packages ]]; then + echo "$0: $crate is no longer a dependency. Update the crate list in $0." >&2 + exit 1 + fi + + while IFS=$'\t' read -r version manifest_path; do + src=$(dirname "$manifest_path") + # The license files at the crate root, plus the .licenses directory that granit-parser's LICENSE + # links to. Copying keeps their relative paths, so those links still resolve. + notices=("$src"/{LICENSE,LICENCE,COPYRIGHT,NOTICE,COPYING}*) + if [[ -d $src/.licenses ]]; then + notices+=("$src/.licenses") + fi + if [[ ${#notices[@]} -eq 0 ]]; then + echo "$0: $src has no license files. Check $crate's license and update $0." >&2 + exit 1 + fi + + mkdir "$out/$crate-$version" + cp -RL "${notices[@]}" "$out/$crate-$version" + done <<<"$packages" +done 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..4894dac 100644 --- a/src/config.rs +++ b/src/config.rs @@ -1,36 +1,56 @@ use serde::Deserialize; -use std::{fs::File, path::Path}; +use std::path::{Path, PathBuf}; + +/// `Config.path` for a config not read with `load_from_path`: the usual location, relative to the project root. +pub(crate) const DEFAULT_PATH: &str = "config/code_ownership.yml"; #[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, + + /// The file [`Config::load_from_path`] read this config from, or `config/code_ownership.yml` for a config + /// deserialized another way. The hint on a file over `yaml_max_nodes` names it. + #[serde(skip_deserializing, default = "default_path")] + pub path: PathBuf, } #[allow(dead_code)] @@ -91,19 +111,38 @@ fn default_codeowners_path() -> String { ".github".to_string() } +fn default_yaml_max_nodes() -> usize { + crate::yaml::DEFAULT_MAX_NODES +} + +fn default_path() -> PathBuf { + PathBuf::from(DEFAULT_PATH) +} + 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. + let config: Self = crate::yaml::from_slice(&bytes, crate::yaml::DEFAULT_MAX_NODES) + .map_err(|e| format!("Can't parse config file: {} ({})", path.to_string_lossy(), e))?; + Ok(Self { + path: path.to_path_buf(), + ..config + }) + } + + /// `yaml_max_nodes`, with this config's file relative to `project_root` when it is inside it. + pub(crate) fn node_limit(&self, project_root: &Path) -> crate::yaml::NodeLimit { + crate::yaml::NodeLimit { + max_nodes: self.yaml_max_nodes, + config_path: crate::path_utils::relative_to_buf(project_root, &self.path), + } } } #[cfg(test)] mod tests { - use std::{ - error::Error, - fs::{self, File}, - }; + use std::{error::Error, fs}; use indoc::indoc; use tempfile::tempdir; @@ -120,8 +159,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 +173,53 @@ 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(()) + } + + #[test] + fn test_node_limit_names_the_config_file() -> Result<(), Box> { + let temp_dir = tempdir()?; + fs::create_dir(temp_dir.path().join("config"))?; + let config_path = temp_dir.path().join("config/alt.yml"); + let config_str = indoc! {" + --- + owned_globs: + - \"**/*.rb\" + yaml_max_nodes: 30000 + "}; + fs::write(&config_path, config_str)?; + let config = Config::load_from_path(&config_path)?; + assert_eq!( + config.node_limit(temp_dir.path()), + crate::yaml::NodeLimit { + max_nodes: 30_000, + config_path: PathBuf::from("config/alt.yml"), + } + ); + // A config file outside the project root is named in full, and a config not read from a file names the default. + assert_eq!(config.node_limit(Path::new("/elsewhere")).config_path, config_path); + let unread: Config = crate::yaml::from_slice(b"owned_globs: []\n", crate::yaml::DEFAULT_MAX_NODES)?; + assert_eq!( + unread.node_limit(temp_dir.path()).config_path, + Path::new("config/code_ownership.yml") + ); Ok(()) } @@ -149,8 +234,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 +249,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 +264,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 +280,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..a59d855 100644 --- a/src/ownership/codeowners_file_parser.rs +++ b/src/ownership/codeowners_file_parser.rs @@ -1,6 +1,7 @@ use crate::{ ownership::{FileGenerator, TeamOwnership}, project::Team, + yaml::NodeLimit, }; use fast_glob::glob_match; use memoize::memoize; @@ -14,6 +15,7 @@ pub struct Parser { pub project_root: PathBuf, pub codeowners_file_path: PathBuf, pub team_file_globs: Vec, + pub node_limit: NodeLimit, } impl Parser { @@ -40,7 +42,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.node_limit.clone()); let result: HashMap> = file_inputs .par_iter() @@ -65,13 +67,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, node_limit: NodeLimit) -> 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(), &node_limit) { Ok(team) => team, Err(e) => { eprintln!("Error parsing team file: {e:?}, path: {}", path.display()); @@ -495,6 +497,35 @@ 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()], + node_limit: NodeLimit { + max_nodes: yaml_max_nodes, + ..Default::default() + }, + }; + 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..eb58ee2 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], + node_limit: crate::yaml::NodeLimit, 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(), + node_limit, }; 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..2aa2450 100644 --- a/src/ownership/file_owner_resolver.rs +++ b/src/ownership/file_owner_resolver.rs @@ -8,7 +8,7 @@ use std::{ use fast_glob::glob_match; use glob::glob; -use crate::{config::Config, project::Team, project_file_builder::build_project_file_without_cache}; +use crate::{config::Config, project::Team, project_file_builder::build_project_file_without_cache, yaml::NodeLimit}; use super::{FileOwner, mapper::Source}; @@ -20,7 +20,11 @@ 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.node_limit(project_root), + )?; let teams = &loaded.teams; let teams_by_name = &loaded.teams_by_name; @@ -105,9 +109,9 @@ struct LoadedTeams { teams_by_name: HashMap, } -type TeamCacheKey = (PathBuf, Vec); +type TeamCacheKey = (PathBuf, Vec, NodeLimit); -// 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 +126,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, + node_limit: NodeLimit, +) -> std::result::Result, String> { + let key = (project_root, team_file_globs, node_limit); let generation = { let cache = team_cache(); if let Some(loaded) = cache.loaded.get(&key) { @@ -132,7 +140,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 +186,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], node_limit: &NodeLimit) -> std::result::Result { let mut teams: Vec = Vec::new(); let mut skipped_team_file = false; for glob_str in team_file_globs { @@ -193,7 +201,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(), node_limit) { Ok(team) => teams.push(team), Err(e) => { eprintln!("Error parsing team file: {e:?}, path: {}", path.display()); @@ -266,7 +274,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.node_limit(project_root)) { 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 +385,8 @@ mod tests { ignore_dirs: vec![], executable_name: "codeowners".to_string(), codeowners_path: ".github".to_string(), + yaml_max_nodes: crate::yaml::DEFAULT_MAX_NODES, + path: PathBuf::from(crate::config::DEFAULT_PATH), } } @@ -678,7 +688,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()], + NodeLimit::default(), + ); let loaded = || { Arc::new(LoadedTeams { teams: Vec::new(), @@ -826,4 +840,34 @@ 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() { + // A clear between the two lookups would hide a key that ignores the limit. + let _guard = TEAM_CACHE_TEST_LOCK.lock().unwrap_or_else(|poisoned| poisoned.into_inner()); + 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..4c6bb0a 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(crate) fn from_team_file_path(absolute_path: PathBuf, node_limit: &crate::yaml::NodeLimit) -> Result> { + let bytes = std::fs::read(&absolute_path).change_context(Error::Io)?; + let deserializer: deserializers::Team = crate::yaml::from_project_slice(&bytes, node_limit).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::NodeLimit::default()) + } + + #[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..769a185 100644 --- a/src/project_builder.rs +++ b/src/project_builder.rs @@ -16,6 +16,7 @@ use crate::{ project::{DirectoryCodeownersFile, Error, Package, PackageType, Project, ProjectFile, Team, VendoredGem, deserializers}, project_file_builder::ProjectFileBuilder, tracked_files, + yaml::NodeLimit, }; type AbsolutePath = PathBuf; @@ -212,6 +213,7 @@ impl<'a> ProjectBuilder<'a> { Vec, ); + let node_limit = self.config.node_limit(&self.base_path); let (project_files, packages, vendored_gems, directory_codeowners, teams): Accumulator = entry_types .into_par_iter() .try_fold( @@ -242,7 +244,7 @@ impl<'a> ProjectBuilder<'a> { } } EntryType::RubyPackage(absolute_path, relative_path) => { - match ruby_package_owner(&absolute_path) + match ruby_package_owner(&absolute_path, &node_limit) .attach_with(|| format!("Failed to read ruby package: {}", absolute_path.display())) { Ok(Some(owner)) => { @@ -282,7 +284,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(), &node_limit) .change_context(Error::Io) .attach_with(|| format!("Failed to read team file: {}", absolute_path.display()))?; team_files.push(team); @@ -336,9 +338,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, node_limit: &NodeLimit) -> 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, node_limit).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 +371,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 +393,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(), &NodeLimit::default()).unwrap(); assert_eq!(owner, Some("TeamA".to_string())); } @@ -394,7 +403,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(), &NodeLimit::default()).unwrap(); assert_eq!(owner, Some("TeamB".to_string())); } @@ -404,7 +413,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(), &NodeLimit::default()); assert!(result.is_err()); } @@ -414,7 +423,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(), &NodeLimit::default()).unwrap(); assert_eq!(owner, Some("TeamA".to_string())); } @@ -424,7 +433,71 @@ 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(), &NodeLimit::default()).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(), &NodeLimit::default()).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(), &NodeLimit::default()).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(), &NodeLimit::default()).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 node_limit = NodeLimit { + max_nodes: 5, + config_path: PathBuf::from("config/alt.yml"), + }; + let error = ruby_package_owner(temp_file.path(), &node_limit).unwrap_err(); + assert!( + format!("{error:?}").contains("raise `yaml_max_nodes` (now 5) in config/alt.yml to allow larger files"), + "{error:?}" + ); + } } diff --git a/src/runner/api.rs b/src/runner/api.rs index bf4e9c9..f9073ef 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.node_limit(&run_config.project_root), file_paths, ) .map_err(Error::Io)?; diff --git a/src/yaml.rs b/src/yaml.rs new file mode 100644 index 0000000..57db4fc --- /dev/null +++ b/src/yaml.rs @@ -0,0 +1,339 @@ +//! 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}; +use std::path::PathBuf; + +/// 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; + +/// `yaml_max_nodes` for team files and package.yml, and the config file to raise it in, which the hint names. +#[derive(Clone, Debug, PartialEq, Eq, Hash)] +pub(crate) struct NodeLimit { + pub(crate) max_nodes: usize, + pub(crate) config_path: PathBuf, +} + +#[cfg(test)] +impl Default for NodeLimit { + fn default() -> Self { + Self { + max_nodes: DEFAULT_MAX_NODES, + config_path: PathBuf::from(crate::config::DEFAULT_PATH), + } + } +} + +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. + // At the default limit, the event cap is serde-saphyr's own default. + 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 and the config file. +pub(crate) fn from_project_slice(bytes: &[u8], node_limit: &NodeLimit) -> Result> { + from_slice(bytes, node_limit.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 {}) in {} to allow larger files", + node_limit.max_nodes, + node_limit.config_path.display() + )) + } 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(), &NodeLimit::default()) 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(), &NodeLimit::default()) 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_is_serde_saphyr_default_at_default_max_nodes() { + // No file reaches the event cap before another cap unless the limit is 0, where every multiplier gives 0, so no + // parse can pin the multiplier. + let budget = options(DEFAULT_MAX_NODES).budget.unwrap(); + assert_eq!(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 node_limit = NodeLimit { + max_nodes: 0, + ..Default::default() + }; + let Err(report) = from_project_slice::(b"owner: TeamA\n", &node_limit) 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(), &NodeLimit::default()) 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(), &NodeLimit::default()) 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", &NodeLimit::default()) 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..a233f13 --- /dev/null +++ b/tests/yaml_max_nodes_test.rs @@ -0,0 +1,113 @@ +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; + +const PACKAGE_YML: &str = "ruby/packages/payroll_flow/package.yml"; +const TEAM_FILE: &str = "config/teams/payroll.yml"; + +// 30,000 ignored list items put each of `large_files` over the default `yaml_max_nodes` of 20,000. +fn project_with_large_yaml_files(large_files: &[&str], yaml_max_nodes: Option) -> TempDir { + let temp_dir = setup_fixture_repo(Path::new("tests/fixtures/valid_project")); + let root = temp_dir.path(); + 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(); + + for large_file in large_files { + let path = root.join(large_file); + let yaml = fs::read_to_string(&path).unwrap(); + fs::write(&path, format!("{yaml}ignored: [{}]\n", vec!["x"; 30_000].join(", "))).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(&[PACKAGE_YML, TEAM_FILE], 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(&[PACKAGE_YML, TEAM_FILE], 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(()) +} + +#[test] +fn test_hint_names_the_config_file_in_use() -> Result<(), Box> { + let hint = "raise `yaml_max_nodes` (now 20000) in config/alt.yml to allow larger files"; + // generate stops at the first large file it reads, so each project has only one. + for large_file in [PACKAGE_YML, TEAM_FILE] { + let temp_dir = project_with_large_yaml_files(&[large_file], None); + let root = temp_dir.path(); + fs::rename(root.join("config/code_ownership.yml"), root.join("config/alt.yml"))?; + let with_alt_config = |args: &[&str]| codeowners(root, &[["--config-path", "config/alt.yml"].as_slice(), args].concat()); + + with_alt_config(&["generate"]) + .assert() + .failure() + .stderr(predicate::str::contains(hint)); + with_alt_config(&["for-file", "ruby/packages/payroll_flow/app/flow.rb"]) + .assert() + .stderr(predicate::str::contains(hint)); + // --from-codeowners reads only team files. + if large_file == TEAM_FILE { + with_alt_config(&["for-file", "--from-codeowners", "ruby/packages/payroll_flow/app/flow.rb"]) + .assert() + .stderr(predicate::str::contains(hint)); + } + } + + Ok(()) +}