diff --git a/src/packs/checker.rs b/src/packs/checker.rs index 52239106..72dd7465 100644 --- a/src/packs/checker.rs +++ b/src/packs/checker.rs @@ -34,6 +34,7 @@ use super::reference_extractor::get_all_references; #[derive(PartialEq, Clone, Eq, Hash, Debug)] pub struct ViolationIdentifier { pub violation_type: String, + pub strict: bool, pub file: String, pub constant_name: String, pub referencing_pack_name: String, @@ -119,12 +120,7 @@ impl CheckAllResult { if !self.strict_mode_violations.is_empty() { for v in self.strict_mode_violations.iter() { - let error_message = format!("{} cannot have {} violations on {} because strict mode is enabled for {} violations in the enforcing pack's package.yml file", - v.referencing_pack_name, - v.violation_type, - v.defining_pack_name, - v.violation_type - ); + let error_message = build_strict_violation_message(v); writeln!(f, "{}", error_message)?; } } @@ -147,7 +143,6 @@ struct CheckAllBuilder<'a> { } struct FoundViolations { - checkers: Vec>, absolute_paths: HashSet, violations: HashSet, } @@ -178,7 +173,7 @@ impl<'a> CheckAllBuilder<'a> { .cloned() .collect(), strict_mode_violations: self - .build_strict_mode_violations(recorded_violations)? + .build_strict_mode_violations() .into_iter() .cloned() .collect(), @@ -248,37 +243,13 @@ impl<'a> CheckAllBuilder<'a> { Ok(stale_violations) } - fn build_strict_mode_violations( - &mut self, - recorded_violations: &'a HashSet, - ) -> anyhow::Result> { - let indexed_checkers: HashMap< - String, - &Box, - > = self - .found_violations - .checkers - .iter() - .map(|checker| (checker.violation_type(), checker)) - .collect(); - - recorded_violations + fn build_strict_mode_violations(&self) -> Vec<&'a ViolationIdentifier> { + self.found_violations + .violations .iter() - .try_fold(vec![], |mut acc, violation| { - let checker = indexed_checkers - .get(&violation.violation_type) - .context(format!( - "Checker for violation type {} not found", - violation.violation_type - ))?; - - if checker - .is_strict_mode_violation(violation, self.configuration)? - { - acc.push(violation); - } - Ok(acc) - }) + .filter(|v| v.identifier.strict) + .map(|v| &v.identifier) + .collect() } } @@ -295,7 +266,6 @@ pub(crate) fn check_all( let violations: HashSet = get_all_violations(configuration, &absolute_paths, &checkers)?; let found_violations = FoundViolations { - checkers, absolute_paths, violations, }; @@ -322,6 +292,16 @@ fn validate(configuration: &Configuration) -> Vec { validation_errors } +pub(crate) fn build_strict_violation_message( + violation_identifier: &ViolationIdentifier, +) -> String { + format!("{} cannot have {} violations on {} because strict mode is enabled for {} violations in the enforcing pack's package.yml file", + violation_identifier.referencing_pack_name, + violation_identifier.violation_type, + violation_identifier.defining_pack_name, + violation_identifier.violation_type,) +} + pub(crate) fn validate_all( configuration: &Configuration, ) -> anyhow::Result<()> { @@ -348,8 +328,24 @@ pub(crate) fn update(configuration: &Configuration) -> anyhow::Result<()> { &checkers, )?; + let strict_violations = &violations + .iter() + .filter(|v| v.identifier.strict) + .collect::>(); + if !strict_violations.is_empty() { + for violation in strict_violations { + let strict_message = + build_strict_violation_message(&violation.identifier); + println!("{}", strict_message); + } + println!( + "{} strict mode violation(s) detected. These violations must be fixed for `check` to succeed.", + &strict_violations.len() + ); + } package_todo::write_violations_to_disk(configuration, violations); println!("Successfully updated package_todo.yml files!"); + Ok(()) } diff --git a/src/packs/checker/architecture.rs b/src/packs/checker/architecture.rs index ff7229fc..30e0f031 100644 --- a/src/packs/checker/architecture.rs +++ b/src/packs/checker/architecture.rs @@ -171,6 +171,7 @@ impl CheckerInterface for Checker { let file = reference.relative_referencing_file.clone(); let identifier = ViolationIdentifier { violation_type, + strict: referencing_pack.enforce_architecture().is_strict(), file, constant_name: reference.constant_name.clone(), referencing_pack_name: referencing_pack_name.clone(), @@ -250,7 +251,7 @@ mod tests { }, expected_violation: Some(build_expected_violation( "packs/foo/app/services/foo.rb:3:1\nArchitecture violation: `::Bar` belongs to `packs/bar` (whose layer is `product`) cannot be accessed from `packs/foo` (whose layer is `utilities`)".to_string(), - "architecture".to_string())), + "architecture".to_string(), false)), ..Default::default() }; test_check(&checker_with_layers(), &mut test_checker) @@ -275,7 +276,32 @@ mod tests { }, expected_violation: Some(build_expected_violation( "packs/foo/app/services/foo.rb:3:1\nArchitecture violation: `::Bar` belongs to `packs/bar` (whose layer is `product`) cannot be accessed from `packs/foo` (whose layer is `utilities`)".to_string(), - "architecture".to_string())), + "architecture".to_string(), false)), + ..Default::default() + }; + test_check(&checker_with_layers(), &mut test_checker) + } + + #[test] + fn reference_is_a_strict_violation() -> anyhow::Result<()> { + let mut test_checker = TestChecker { + reference: None, + configuration: None, + referenced_constant_name: Some(String::from("::Bar")), + defining_pack: Some(Pack { + name: "packs/bar".to_owned(), + layer: Some("product".to_string()), + ..default_defining_pack() + }), + referencing_pack: Pack { + name: "packs/foo".to_owned(), + enforce_architecture: Some(CheckerSetting::Strict), + layer: Some("utilities".to_string()), + ..default_referencing_pack() + }, + expected_violation: Some(build_expected_violation( + "packs/foo/app/services/foo.rb:3:1\nArchitecture violation: `::Bar` belongs to `packs/bar` (whose layer is `product`) cannot be accessed from `packs/foo` (whose layer is `utilities`)".to_string(), + "architecture".to_string(), true)), ..Default::default() }; test_check(&checker_with_layers(), &mut test_checker) diff --git a/src/packs/checker/common_test.rs b/src/packs/checker/common_test.rs index 414b8a0d..37c1a1fa 100644 --- a/src/packs/checker/common_test.rs +++ b/src/packs/checker/common_test.rs @@ -28,10 +28,12 @@ pub mod tests { pub fn build_expected_violation( message: String, violation_type: String, + strict: bool, ) -> Violation { build_expected_violation_with_constant( message, violation_type, + strict, String::from("::Bar"), ) } @@ -39,14 +41,16 @@ pub mod tests { pub fn build_expected_violation_with_constant( message: String, violation_type: String, + strict: bool, constant_name: String, ) -> Violation { Violation { message, identifier: ViolationIdentifier { violation_type, + strict, file: String::from("packs/foo/app/services/foo.rb"), - constant_name: constant_name, + constant_name, referencing_pack_name: String::from("packs/foo"), defining_pack_name: String::from("packs/bar"), }, diff --git a/src/packs/checker/dependency.rs b/src/packs/checker/dependency.rs index 5e32f2fc..505b68e5 100644 --- a/src/packs/checker/dependency.rs +++ b/src/packs/checker/dependency.rs @@ -165,6 +165,7 @@ impl CheckerInterface for Checker { let file = reference.relative_referencing_file.clone(); let identifier = ViolationIdentifier { violation_type, + strict: referencing_pack.enforce_dependencies().is_strict(), file, constant_name: reference.constant_name.clone(), referencing_pack_name: referencing_pack_name.clone(), @@ -248,7 +249,29 @@ mod tests { ..default_referencing_pack()}, expected_violation: Some(build_expected_violation( "packs/foo/app/services/foo.rb:3:1\nDependency violation: `::Bar` belongs to `packs/bar`, but `packs/foo/package.yml` does not specify a dependency on `packs/bar`.".to_string(), - "dependency".to_string())), + "dependency".to_string(), false)), + ..Default::default() + }; + test_check(&Checker {}, &mut test_checker) + } + + #[test] + fn test_with_strict_violation() -> anyhow::Result<()> { + let mut test_checker = TestChecker { + reference: None, + configuration: None, + referenced_constant_name: Some(String::from("::Bar")), + defining_pack: Some(Pack { + name: "packs/bar".to_owned(), + ..default_defining_pack() + }), + referencing_pack: Pack{ + relative_path: PathBuf::from("packs/foo"), + enforce_dependencies: Some(CheckerSetting::Strict), + ..default_referencing_pack()}, + expected_violation: Some(build_expected_violation( + "packs/foo/app/services/foo.rb:3:1\nDependency violation: `::Bar` belongs to `packs/bar`, but `packs/foo/package.yml` does not specify a dependency on `packs/bar`.".to_string(), + "dependency".to_string(), true)), ..Default::default() }; test_check(&Checker {}, &mut test_checker) diff --git a/src/packs/checker/folder_visibility.rs b/src/packs/checker/folder_visibility.rs index edcb415b..5d57d484 100644 --- a/src/packs/checker/folder_visibility.rs +++ b/src/packs/checker/folder_visibility.rs @@ -35,6 +35,7 @@ impl CheckerInterface for Checker { ); let identifier = ViolationIdentifier { violation_type: self.violation_type(), + strict: defining_pack.enforce_folder_visibility().is_strict(), file: reference.relative_referencing_file.clone(), constant_name: reference.constant_name.clone(), referencing_pack_name: referencing_pack.name.clone(), @@ -117,7 +118,28 @@ mod tests { ..default_referencing_pack()}, expected_violation: Some(build_expected_violation( "packs/foo/app/services/foo.rb:3:1\nFolder Visibility violation: `::Bar` belongs to `packs/bar`, which is not visible to `packs/foo` as it is not a sibling pack or parent pack.".to_string(), - "folder_visibility".to_string())), + "folder_visibility".to_string(), false)), + ..Default::default() + }; + test_check(&Checker {}, &mut test_checker) + } + #[test] + fn test_with_strict_violation() -> anyhow::Result<()> { + let mut test_checker = TestChecker { + reference: None, + configuration: None, + referenced_constant_name: Some(String::from("::Bar")), + defining_pack: Some(Pack { + name: "packs/bar".to_owned(), + enforce_folder_visibility: Some(CheckerSetting::Strict), + ..default_defining_pack() + }), + referencing_pack: Pack{ + relative_path: PathBuf::from("packs/foo"), + ..default_referencing_pack()}, + expected_violation: Some(build_expected_violation( + "packs/foo/app/services/foo.rb:3:1\nFolder Visibility violation: `::Bar` belongs to `packs/bar`, which is not visible to `packs/foo` as it is not a sibling pack or parent pack.".to_string(), + "folder_visibility".to_string(), true)), ..Default::default() }; test_check(&Checker {}, &mut test_checker) diff --git a/src/packs/checker/privacy.rs b/src/packs/checker/privacy.rs index cd6fe4c0..e09c545b 100644 --- a/src/packs/checker/privacy.rs +++ b/src/packs/checker/privacy.rs @@ -102,6 +102,7 @@ impl CheckerInterface for Checker { let file = reference.relative_referencing_file.clone(); let identifier = ViolationIdentifier { violation_type, + strict: defining_pack.enforce_privacy().is_strict(), file, constant_name: reference.constant_name.clone(), referencing_pack_name: referencing_pack_name.clone(), @@ -201,7 +202,29 @@ mod tests { referencing_pack: default_referencing_pack(), expected_violation: Some(build_expected_violation( String::from("packs/foo/app/services/foo.rb:3:1\nPrivacy violation: `::Bar` is private to `packs/bar`, but referenced from `packs/foo`"), - String::from("privacy"), + String::from("privacy"), false, + )), + ..Default::default() + }; + test_check(&Checker {}, &mut test_checker) + } + + #[test] + fn test_with_strict_privacy_violation() -> anyhow::Result<()> { + let mut test_checker = TestChecker { + reference: None, + configuration: None, + referenced_constant_name: Some(String::from("::Bar")), + defining_pack: Some(Pack { + name: "packs/bar".to_owned(), + enforce_privacy: Some(CheckerSetting::Strict), + ignored_private_constants: HashSet::from([String::from("::Taco")]), + ..default_defining_pack() + }), + referencing_pack: default_referencing_pack(), + expected_violation: Some(build_expected_violation( + String::from("packs/foo/app/services/foo.rb:3:1\nPrivacy violation: `::Bar` is private to `packs/bar`, but referenced from `packs/foo`"), + String::from("privacy"), true, )), ..Default::default() }; @@ -376,7 +399,7 @@ mod tests { referencing_pack: default_referencing_pack(), expected_violation: Some(build_expected_violation_with_constant( String::from("packs/foo/app/services/foo.rb:3:1\nPrivacy violation: `::Bar::BarChild` is private to `packs/bar`, but referenced from `packs/foo`"), - String::from("privacy"), + String::from("privacy"), false, String::from("::Bar::BarChild") )), ..Default::default() diff --git a/src/packs/checker/visibility.rs b/src/packs/checker/visibility.rs index c349f94e..dc72d6bf 100644 --- a/src/packs/checker/visibility.rs +++ b/src/packs/checker/visibility.rs @@ -64,6 +64,7 @@ impl CheckerInterface for Checker { let file = reference.relative_referencing_file.clone(); let identifier = ViolationIdentifier { violation_type, + strict: defining_pack.enforce_visibility().is_strict(), file, constant_name: reference.constant_name.clone(), referencing_pack_name: referencing_pack_name.clone(), @@ -144,7 +145,29 @@ mod tests { ..default_referencing_pack()}, expected_violation: Some(build_expected_violation( "packs/foo/app/services/foo.rb:3:1\nVisibility violation: `::Bar` belongs to `packs/bar`, which is not visible to `packs/foo`".to_string(), - "visibility".to_string())), + "visibility".to_string(), false)), + ..Default::default() + }; + test_check(&Checker {}, &mut test_checker) + } + + #[test] + fn test_with_strict_violation() -> anyhow::Result<()> { + let mut test_checker = TestChecker { + reference: None, + configuration: None, + referenced_constant_name: Some(String::from("::Bar")), + defining_pack: Some(Pack { + name: "packs/bar".to_owned(), + enforce_visibility: Some(CheckerSetting::Strict), + ..default_defining_pack() + }), + referencing_pack: Pack{ + relative_path: PathBuf::from("packs/foo"), + ..default_referencing_pack()}, + expected_violation: Some(build_expected_violation( + "packs/foo/app/services/foo.rb:3:1\nVisibility violation: `::Bar` belongs to `packs/bar`, which is not visible to `packs/foo`".to_string(), + "visibility".to_string(), true)), ..Default::default() }; test_check(&Checker {}, &mut test_checker) diff --git a/src/packs/pack.rs b/src/packs/pack.rs index a54dcdea..89177794 100644 --- a/src/packs/pack.rs +++ b/src/packs/pack.rs @@ -151,6 +151,7 @@ impl Pack { for file in &violation_group.files { let identifier = ViolationIdentifier { violation_type: violation_type.clone(), + strict: false, file: file.clone(), constant_name: constant_name.clone(), referencing_pack_name: self.name.clone(), @@ -623,4 +624,39 @@ owner: Foobar vec![root.join("app/company_data"), root.join("app/services")]; assert_eq!(expected, actual) } + + #[test] + fn test_all_recorded_violations() -> anyhow::Result<()> { + let root = test_util::get_absolute_root( + "tests/fixtures/contains_package_todo", + ); + let pack = Pack::from_path( + root.join("packs/foo/package.yml").as_path(), + root.as_path(), + )?; + + let mut actual = pack.all_violations(); + actual.sort_by(|a, b| a.file.cmp(&b.file)); + + let expected = vec![ + ViolationIdentifier { + violation_type: "dependency".to_string(), + strict: false, + file: "packs/foo/app/services/foo.rb".to_string(), + constant_name: "::Bar".to_string(), + referencing_pack_name: "packs/foo".to_string(), + defining_pack_name: "packs/bar".to_string(), + }, + ViolationIdentifier { + violation_type: "dependency".to_string(), + strict: false, + file: "packs/foo/app/services/other_foo.rb".to_string(), + constant_name: "::Bar".to_string(), + referencing_pack_name: "packs/foo".to_string(), + defining_pack_name: "packs/bar".to_string(), + }, + ]; + assert_eq!(expected, actual); + Ok(()) + } } diff --git a/src/packs/package_todo.rs b/src/packs/package_todo.rs index 1fefca61..5c0f480b 100644 --- a/src/packs/package_todo.rs +++ b/src/packs/package_todo.rs @@ -141,6 +141,9 @@ pub fn write_violations_to_disk( let mut violations_by_responsible_pack: HashMap> = HashMap::new(); for violation in violations { + if violation.identifier.strict { + continue; + } let referencing_pack_name = violation.identifier.referencing_pack_name.to_owned(); violations_by_responsible_pack diff --git a/tests/fixtures/contains_strict_violations/package.yml b/tests/fixtures/contains_strict_violations/package.yml new file mode 100644 index 00000000..e69de29b diff --git a/tests/fixtures/contains_strict_violations/packs/bar/app/services/bar.rb b/tests/fixtures/contains_strict_violations/packs/bar/app/services/bar.rb new file mode 100644 index 00000000..50031503 --- /dev/null +++ b/tests/fixtures/contains_strict_violations/packs/bar/app/services/bar.rb @@ -0,0 +1,2 @@ +module Bar +end diff --git a/tests/fixtures/contains_strict_violations/packs/bar/package.yml b/tests/fixtures/contains_strict_violations/packs/bar/package.yml new file mode 100644 index 00000000..827dac40 --- /dev/null +++ b/tests/fixtures/contains_strict_violations/packs/bar/package.yml @@ -0,0 +1 @@ +enforce_privacy: strict diff --git a/tests/fixtures/contains_strict_violations/packs/foo/app/services/foo.rb b/tests/fixtures/contains_strict_violations/packs/foo/app/services/foo.rb new file mode 100644 index 00000000..ff251421 --- /dev/null +++ b/tests/fixtures/contains_strict_violations/packs/foo/app/services/foo.rb @@ -0,0 +1,5 @@ +module Foo + def calls_bar_with_stated_dependency + Bar + end +end diff --git a/tests/fixtures/contains_strict_violations/packs/foo/package.yml b/tests/fixtures/contains_strict_violations/packs/foo/package.yml new file mode 100644 index 00000000..9f59634b --- /dev/null +++ b/tests/fixtures/contains_strict_violations/packs/foo/package.yml @@ -0,0 +1,3 @@ +enforce_dependencies: true +dependencies: + - packs/bar diff --git a/tests/fixtures/contains_strict_violations/packwerk.yml b/tests/fixtures/contains_strict_violations/packwerk.yml new file mode 100644 index 00000000..51f2f3b4 --- /dev/null +++ b/tests/fixtures/contains_strict_violations/packwerk.yml @@ -0,0 +1,23 @@ +# See: Setting up the configuration file +# https://github.com/Shopify/packwerk/blob/main/USAGE.md#setting-up-the-configuration-file + +# List of patterns for folder paths to include +# include: +# - "**/*.{rb,rake,erb}" + +# List of patterns for folder paths to exclude +# exclude: +# - "{bin,node_modules,script,tmp,vendor}/**/*" + +# Patterns to find package configuration files +# package_paths: "**/" + +# List of custom associations, if any +# custom_associations: +# - "cache_belongs_to" + +# Whether or not you want the cache enabled (disabled by default) +cache: false + +# Where you want the cache to be stored (default below) +# cache_directory: 'tmp/cache/packwerk' diff --git a/tests/update_test.rs b/tests/update_test.rs index 0fb3a5bd..ac639d36 100644 --- a/tests/update_test.rs +++ b/tests/update_test.rs @@ -187,3 +187,31 @@ packs/bar: Ok(()) } + +#[test] +fn test_update_with_strict_violations() -> anyhow::Result<()> { + let path = Path::new( + "tests/fixtures/contains_strict_violations/packs/foo/package_todo.yml", + ); + let _ignore = std::fs::remove_file(path); + + Command::cargo_bin("packs")? + .arg("--project-root") + .arg("tests/fixtures/contains_strict_violations") + .arg("update") + .assert() + .success() + .stdout(predicate::str::contains( + "packs/foo cannot have privacy violations on packs/bar because strict mode is enabled for privacy violations in the enforcing pack's package.yml file", + )) + .stdout(predicate::str::contains("1 strict mode violation(s) detected.")) + .stdout(predicate::str::contains( + "Successfully updated package_todo.yml files!", + )); + + assert!( + !path.exists(), + "todo should not be created for strict violations" + ); + Ok(()) +}