Gracefully handle validate when an unknown pack is referenced (#131)

* adding failing tests

* adding anyhow to simply result errors

* avoiding panics on validation

Cargo.toml

@@ -28,6 +28,7 @@ name = "packs"
 path = "src/lib.rs"
 
 [dependencies]
+anyhow = { version = "1.0.75", features = [] } # for error handling
 clap = { version = "4.2.1", features = ["derive"] } # cli
 clap_derive = "4.2.0" # cli
 itertools = "0.10.5" # tools for iterating over iterable things

src/packs/checker.rs

@@ -285,11 +285,12 @@ fn validate(configuration: &Configuration) -> Vec<String> {
         }),
     ];
 
-    let validation_errors: Vec<String> = validators
+    let mut validation_errors: Vec<String> = validators
         .iter()
         .filter_map(|v| v.validate(configuration))
         .flatten()
         .collect();
+    validation_errors.dedup();
     debug!("Finished validators against packages");
 
     validation_errors

src/packs/checker/architecture.rs

@@ -5,6 +5,7 @@ use super::{
 use crate::packs::checker::Reference;
 use crate::packs::pack::Pack;
 use crate::packs::{Configuration, Violation};
+use anyhow::{bail, Result};
 
 #[derive(Default, Clone)]
 pub struct Layers {
@@ -16,7 +17,7 @@ impl Layers {
         &self,
         referencing_layer: &String,
         defining_layer: &String,
-    ) -> bool {
+    ) -> Result<bool> {
         let referencing_layer_index = self
             .layers
             .iter()
@@ -27,13 +28,11 @@ impl Layers {
 
         match (referencing_layer_index, defining_layer_index) {
             (Some(referencing_layer_index), Some(defining_layer_index)) => {
-                referencing_layer_index <= defining_layer_index
+                Ok(referencing_layer_index <= defining_layer_index)
             }
             _ => {
-                panic!(
-                    "Could not find one of layer `{}` or layer `{}` in `packwerk.yml`",
-                    referencing_layer, defining_layer
-                )
+                bail!("Could not find one of layer `{}` or layer `{}` in `packwerk.yml`",
+                    referencing_layer, defining_layer)
             }
         }
     }
@@ -42,23 +41,40 @@ impl Layers {
 impl ValidatorInterface for Checker {
     fn validate(&self, configuration: &Configuration) -> Option<Vec<String>> {
         let mut error_messages: Vec<String> = vec![];
-        for pack_dependency in
-            configuration.pack_set.all_pack_dependencies(configuration)
-        {
-            let (from_pack, to_pack) =
-                (pack_dependency.from_pack, pack_dependency.to_pack);
-            if !dependency_permitted(configuration, from_pack, to_pack) {
-                let error_message = format!(
-                    "Invalid 'dependencies' in '{}/package.yml'. '{}/package.yml' has a layer type of '{},' which cannot rely on '{},' which has a layer type of '{}.' `architecture_layers` can be found in packwerk.yml",
-                    from_pack.relative_path.display(),
-                    from_pack.relative_path.display(),
-                    from_pack.layer.clone().unwrap(),
-                    to_pack.name,
-                    to_pack.layer.clone().unwrap(),
-                );
-                error_messages.push(error_message);
+        match configuration.pack_set.all_pack_dependencies(configuration) {
+            Ok(dependencies) => {
+                for pack_dependency in dependencies {
+                    let (from_pack, to_pack) =
+                        (pack_dependency.from_pack, pack_dependency.to_pack);
+                    match dependency_permitted(
+                        configuration,
+                        from_pack,
+                        to_pack,
+                    ) {
+                        Ok(true) => continue,
+                        Ok(false) => {
+                            let error_message = format!(
+                                "Invalid 'dependencies' in '{}/package.yml'. '{}/package.yml' has a layer type of '{},' which cannot rely on '{},' which has a layer type of '{}.' `architecture_layers` can be found in packwerk.yml",
+                                from_pack.relative_path.display(),
+                                from_pack.relative_path.display(),
+                                from_pack.layer.clone().unwrap(),
+                                to_pack.name,
+                                to_pack.layer.clone().unwrap(),
+                            );
+                            error_messages.push(error_message);
+                        }
+                        Err(error) => {
+                            error_messages.push(error.to_string());
+                            return Some(error_messages);
+                        }
+                    }
+                }
+            }
+            Err(error) => {
+                error_messages.push(error.to_string());
             }
         }
+
         if error_messages.is_empty() {
             None
         } else {
@@ -71,15 +87,15 @@ fn dependency_permitted(
     configuration: &Configuration,
     from_pack: &Pack,
     to_pack: &Pack,
-) -> bool {
+) -> Result<bool> {
     if from_pack.enforce_architecture().is_false() {
-        return true;
+        return Ok(true);
     }
 
     let (from_pack_layer, to_pack_layer) = (&from_pack.layer, &to_pack.layer);
 
     if from_pack_layer.is_none() || to_pack_layer.is_none() {
-        return true;
+        return Ok(true);
     }
 
     let (from_pack_layer, to_pack_layer) = (
@@ -131,7 +147,10 @@ impl CheckerInterface for Checker {
 
         match (&referencing_pack.layer, &defining_pack.layer) {
             (Some(referencing_layer), Some(defining_layer)) => {
-                if self.layers.can_depend_on(referencing_layer, defining_layer)
+                if self
+                    .layers
+                    .can_depend_on(referencing_layer, defining_layer)
+                    .unwrap()
                 {
                     return None;
                 }
@@ -433,7 +452,7 @@ mod tests {
         };
 
         let result = dependency_permitted(&configuration, &from_pack, &to_pack);
-        assert_eq!(result, test_case.expected_result);
+        assert_eq!(result.unwrap(), test_case.expected_result);
     }
 
     #[test]

src/packs/checker/dependency.rs

@@ -38,10 +38,19 @@ impl ValidatorInterface for Checker {
         };
         let mut error_messages: Vec<String> = vec![];
 
-        for pack_dependency in
-            configuration.pack_set.all_pack_dependencies(configuration)
-        {
-            add_edge(pack_dependency.from_pack, pack_dependency.to_pack);
+        match configuration.pack_set.all_pack_dependencies(configuration) {
+            Ok(pack_dependencies) => {
+                for pack_dependency in pack_dependencies {
+                    add_edge(
+                        pack_dependency.from_pack,
+                        pack_dependency.to_pack,
+                    );
+                }
+            }
+            Err(msg) => {
+                error_messages.push(msg.to_string());
+                return Some(error_messages);
+            }
         }
 
         let mut sccs = vec![];

src/packs/pack_set.rs

@@ -1,3 +1,4 @@
+use anyhow::{bail, Result};
 use std::{
     collections::{HashMap, HashSet},
     path::{Path, PathBuf},
@@ -90,15 +91,15 @@ impl PackSet {
         )
     }
 
-    pub fn for_pack(&self, pack_name: &str) -> Result<&Pack, &'static str> {
+    pub fn for_pack(&self, pack_name: &str) -> Result<&Pack> {
         // Trim trailing slash on pack_name.
         // Since often the input arg here comes from the command line,
         // a command line auto-completer may add a trailing slash.
         let pack_name = pack_name.trim_end_matches('/');
         if let Some(pack) = self.indexed_packs.get(pack_name) {
             Ok(pack)
         } else {
-            Err("No pack found.")
+            bail!("No pack found '{}'", pack_name)
         }
     }
 
@@ -114,20 +115,23 @@ impl PackSet {
     pub fn all_pack_dependencies<'a>(
         &'a self,
         configuration: &'a Configuration,
-    ) -> Vec<PackDependency> {
+    ) -> Result<Vec<PackDependency>> {
         let mut pack_refs: Vec<PackDependency> = Vec::new();
         for from_pack in &configuration.pack_set.packs {
             for dependency_pack_name in &from_pack.dependencies {
-                let to_pack = configuration
-                    .pack_set
-                    .for_pack(dependency_pack_name)
-                    .unwrap_or_else(|_| panic!("{} has '{}' in its dependencies, but that pack cannot be found. Try `packs list-packs` to debug.",
-                                               from_pack.yml.to_string_lossy(),
-                                               dependency_pack_name));
-                pack_refs.push(PackDependency { from_pack, to_pack });
+                match configuration.pack_set.for_pack(dependency_pack_name) {
+                    Ok(to_pack) => {
+                        pack_refs.push(PackDependency { from_pack, to_pack })
+                    }
+                    Err(_) => {
+                        bail!("{} has '{}' in its dependencies, but that pack cannot be found. Try `packs list-packs` to debug.",
+                               from_pack.yml.to_string_lossy(),
+                               dependency_pack_name);
+                    }
+                }
             }
         }
-        pack_refs
+        Ok(pack_refs)
     }
 }
 

tests/fixtures/references_unknown_pack/config/initializers/inflections.rb

@@ -0,0 +1,6 @@
+ActiveSupport::Inflector.inflections do |do_not_couple_implementation_to_this_string|
+  do_not_couple_implementation_to_this_string.acronym 'API'
+
+  # Using single vs double quotes inconsistently
+  do_not_couple_implementation_to_this_string.acronym "CSV"
+end

tests/fixtures/references_unknown_pack/packs/feature_flags/app/services/feature_flags.rb

@@ -0,0 +1,2 @@
+# feature_flags, a utility pack, should not rely on Payments, a product pack
+Payments

tests/fixtures/references_unknown_pack/packs/feature_flags/package.yml

@@ -0,0 +1,2 @@
+enforce_architecture: true
+layer: product

tests/fixtures/references_unknown_pack/packs/payments/app/services/payments.rb

@@ -0,0 +1,2 @@
+module Payments
+end

tests/fixtures/references_unknown_pack/packs/payments/package.yml

@@ -0,0 +1,4 @@
+enforce_architecture: true
+dependencies:
+  - packs/unknown-pack
+

tests/fixtures/references_unknown_pack/packwerk.yml

@@ -0,0 +1,29 @@
+# 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'
+
+architecture_layers:
+  - tooling
+  - deprecated
+  - product
+  - utilities

tests/validate_test.rs

@@ -54,3 +54,18 @@ Invalid 'dependencies' in 'packs/foo/package.yml'. 'packs/foo/package.yml' has a
     common::teardown();
     Ok(())
 }
+
+#[test]
+fn test_validate_with_referencing_unknown_pack() -> Result<(), Box<dyn Error>> {
+    Command::cargo_bin("packs")?
+        .arg("--project-root")
+        .arg("tests/fixtures/references_unknown_pack")
+        .arg("--debug")
+        .arg("validate")
+        .assert()
+        .failure()
+        .stdout(predicate::str::contains("has \'packs/unknown-pack\' in its dependencies, but that pack cannot be found"));
+
+    common::teardown();
+    Ok(())
+}