Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updating layer validations #183

Merged
merged 2 commits into from
Apr 23, 2024
Merged

Updating layer validations #183

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8ac763f
validate layers
perryqh Apr 22, 2024
768f27a
depend_on no longer used
perryqh Apr 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
261 changes: 147 additions & 114 deletions src/packs/checker/architecture.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,40 +35,43 @@ impl Layers {
}
}

impl Checker {
fn validate_pack(&self, pack: &Pack) -> Option<String> {
match &pack.layer {
Some(layer) => {
if self.layers.layers.contains(layer) {
None
} else {
Some(format!(
"Invalid 'layer' option in '{}'. `layer` must be one of the layers defined in `packwerk.yml`",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we do have this right, but it was just a bail vs. a first-class error right?

(Related to error message: Could not find one of layer {}or layer{}inpackwerk.yml...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. The referenced code above is an architecture layer validation message. The "could not..." is a "check" error.

&pack.relative_yml().to_string_lossy()
))
}
}
None => match &pack.enforce_architecture {
Some(setting) => {
if setting.is_false() {
None
} else {
Some(format!(
"'layer' must be specified in '{}' because `enforce_architecture` is true or strict.",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense... if a pack enforces architecture, it must specify a layer. Surprised we didn't have this before!

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh yeah, I double checked and it definitely just let it fly before!

pack.relative_yml().to_string_lossy()
))
}
}
None => None,
},
}
}
}

impl ValidatorInterface for Checker {
fn validate(&self, configuration: &Configuration) -> Option<Vec<String>> {
let mut error_messages: Vec<String> = vec![];
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());

for pack in &configuration.pack_set.packs {
if let Some(error_message) = self.validate_pack(pack) {
error_messages.push(error_message);
}
}

Expand All @@ -80,31 +83,6 @@ impl ValidatorInterface for Checker {
}
}

fn dependency_permitted(
configuration: &Configuration,
from_pack: &Pack,
to_pack: &Pack,
) -> Result<bool> {
if from_pack.enforce_architecture().is_false() {
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 Ok(true);
}

let (from_pack_layer, to_pack_layer) = (
from_pack_layer.as_ref().unwrap(),
to_pack_layer.as_ref().unwrap(),
);

configuration
.layers
.can_depend_on(from_pack_layer, to_pack_layer)
}

pub struct Checker {
pub layers: Layers,
}
Expand Down Expand Up @@ -367,89 +345,139 @@ mod tests {
}
}
}
fn package_yml_architecture_test(test_case: ArchitectureTestCase) {

fn validate_layers(
config_layers: Vec<String>,
package_layer: Option<String>,
package_enforce_layer: Option<CheckerSetting>,
) -> Option<Vec<String>> {
let root_pack = Pack {
name: String::from("."),
layer: None,
..Pack::default()
};

let from_pack = Pack {
name: test_case.from_pack_name,
layer: test_case.from_pack_layer,
enforce_architecture: test_case.from_pack_enforce_architecture,
dependencies: test_case.from_pack_dependencies,
let test_pack = Pack {
name: String::from("packs/foo"),
relative_path: PathBuf::from("packs/foo/package.yml"),
layer: package_layer,
enforce_architecture: package_enforce_layer,
..Pack::default()
};
let to_pack = Pack {
name: test_case.to_pack_name,
layer: test_case.to_pack_layer,
..Pack::default()
};

let configuration = Configuration {
pack_set: PackSet::build(
HashSet::from_iter(vec![
root_pack,
from_pack.clone(),
to_pack.clone(),
]),
HashSet::from_iter(vec![root_pack, test_pack]),
HashMap::new(),
)
.unwrap(),
..Configuration::default()
};
let checker = Checker {
layers: Layers {
layers: test_case.layers,
layers: config_layers,
},
..Configuration::default()
};

let result = dependency_permitted(&configuration, &from_pack, &to_pack);
assert_eq!(result.unwrap(), test_case.expected_result);
checker.validate(&configuration)
}

#[test]
fn package_yml_dependency_not_permitted() {
let test_case = ArchitectureTestCase::default();
package_yml_architecture_test(test_case);
fn validate_layers_strict_true() {
let result = validate_layers(
vec![String::from("product"), String::from("utilities")],
Some(String::from("product")),
Some(CheckerSetting::Strict),
);
assert_eq!(result, None);

let result = validate_layers(
vec![String::from("product"), String::from("utilities")],
Some(String::from("product")),
Some(CheckerSetting::True),
);
assert_eq!(result, None);
}

#[test]
fn package_yml_dependency_permitted_violation_not_enforced() {
let test_case = ArchitectureTestCase {
from_pack_enforce_architecture: Some(CheckerSetting::False),
expected_result: true,
..ArchitectureTestCase::default()
};
package_yml_architecture_test(test_case);
fn validate_layers_false_none() {
let result = validate_layers(
vec![String::from("product"), String::from("utilities")],
None,
Some(CheckerSetting::False),
);
assert_eq!(result, None);

let result = validate_layers(
vec![String::from("product"), String::from("utilities")],
None,
None,
);
assert_eq!(result, None);
}

#[test]
fn package_yml_dependency_permitted_violation_no_from_layer() {
let test_case = ArchitectureTestCase {
from_pack_layer: None,
expected_result: true,
..ArchitectureTestCase::default()
};
package_yml_architecture_test(test_case);
fn validate_layers_false_none_but_layer_specified() {
let result = validate_layers(
vec![String::from("product"), String::from("utilities")],
Some(String::from("product")),
Some(CheckerSetting::False),
);
assert_eq!(result, None);

let result = validate_layers(
vec![String::from("product"), String::from("utilities")],
Some(String::from("product")),
None,
);
assert_eq!(result, None);
}

#[test]
fn package_yml_dependency_permitted_violation_no_to_layer() {
let test_case = ArchitectureTestCase {
to_pack_layer: None,
expected_result: true,
..ArchitectureTestCase::default()
};
package_yml_architecture_test(test_case);
fn validate_layers_true_strict_with_no_layer() {
let result = validate_layers(
vec![String::from("product"), String::from("utilities")],
None,
Some(CheckerSetting::True),
);
assert_eq!(result, Some(vec![String::from("'layer' must be specified in 'packs/foo/package.yml/package.yml' because `enforce_architecture` is true or strict.")]));

let result = validate_layers(
vec![String::from("product"), String::from("utilities")],
None,
Some(CheckerSetting::Strict),
);
assert_eq!(result, Some(vec![String::from("'layer' must be specified in 'packs/foo/package.yml/package.yml' because `enforce_architecture` is true or strict.")]));
}

#[test]
fn package_yml_dependency_permitted_violation_valid_layer() {
let test_case = ArchitectureTestCase {
expected_result: true,
layers: vec![String::from("utilities"), String::from("product")],
..ArchitectureTestCase::default()
};
package_yml_architecture_test(test_case);
fn validate_layers_with_not_found_layer() {
let expected_error = Some(vec![String::from("Invalid 'layer' option in 'packs/foo/package.yml/package.yml'. `layer` must be one of the layers defined in `packwerk.yml`")]);

let result = validate_layers(
vec![String::from("product"), String::from("utilities")],
Some(String::from("not defined")),
Some(CheckerSetting::True),
);
assert_eq!(result, expected_error);

let result = validate_layers(
vec![String::from("product"), String::from("utilities")],
Some(String::from("not defined")),
Some(CheckerSetting::Strict),
);
assert_eq!(result, expected_error);

let result = validate_layers(
vec![String::from("product"), String::from("utilities")],
Some(String::from("not defined")),
Some(CheckerSetting::False),
);
assert_eq!(result, expected_error);

let result = validate_layers(
vec![String::from("product"), String::from("utilities")],
Some(String::from("not defined")),
None,
);
assert_eq!(result, expected_error);
}

#[test]
Expand All @@ -473,10 +501,15 @@ mod tests {
};

let error = checker.validate(&configuration);
let expected_message = vec![
String::from("Invalid 'dependencies' in 'packs/baz/package.yml'. 'packs/baz/package.yml' has a layer type of 'technical_services,' which cannot rely on 'packs/bar,' which has a layer type of 'admin.' `architecture_layers` can be found in packwerk.yml"),
String::from( "Invalid 'dependencies' in 'packs/foo/package.yml'. 'packs/foo/package.yml' has a layer type of 'product,' which cannot rely on 'packs/bar,' which has a layer type of 'admin.' `architecture_layers` can be found in packwerk.yml")
assert!(error.is_some());
let mut errors = error.unwrap();
errors.sort();

let expected_errors = vec![
"'layer' must be specified in 'packs/baz/package.yml' because `enforce_architecture` is true or strict.".to_string(),
"Invalid 'layer' option in 'packs/bar/package.yml'. `layer` must be one of the layers defined in `packwerk.yml`".to_string(),
"Invalid 'layer' option in 'packs/foo/package.yml'. `layer` must be one of the layers defined in `packwerk.yml`".to_string()
];
assert_eq!(error, Some(expected_message));
assert_eq!(errors, expected_errors);
}
}
2 changes: 1 addition & 1 deletion tests/fixtures/app_with_architecture_violations_in_yml/packs/bar/package.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
enforce_privacy: true
enforce_architecture: true
enforce_architecture: false
layer: admin
1 change: 0 additions & 1 deletion tests/fixtures/app_with_architecture_violations_in_yml/packs/baz/package.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
enforce_privacy: true
enforce_architecture: true
layer: technical_services
dependencies:
- packs/bar
2 changes: 1 addition & 1 deletion tests/fixtures/app_with_architecture_violations_in_yml/packs/foo/package.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
enforce_dependencies: true
enforce_privacy: true
enforce_architecture: true
layer: product
layer: not_a_layer
dependencies:
- packs/bar
6 changes: 2 additions & 4 deletions tests/validate_test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,10 @@ packs/foo, packs/bar",
#[test]
fn test_validate_architecture() -> Result<(), Box<dyn Error>> {
let expected_message_1 = String::from(
"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we still want the architecture validator to ensure that stated dependencies do not violate the architecture right?

Is your point that if there is a reference that the checker finds, then it's the reference that violates architecture – not the stated dependencies? And we already have that behavior in the system?

My only thought here is that even if there are zero references, it's still wrong to have a dependency that conceptually violates a checker that should be restricting that dependency from existing. Can you share more about why you feel like this should be removed?

Copy link
Contributor Author

@perryqh perryqh Apr 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback! There's a few concerns we have about one type of validation using another type's package.yml configuration.

  1. Maintaining separation between dependency definitions and architectural layer validations is beneficial. This separation allows teams to selectively apply different types of validations without worrying about interdependencies between them.

  2. If layer validation takes into account declared dependencies, the following scenario will come up:
    i) A package.yml specifies a legitimate dependency
    ii) Subsequently, a new layer validation is introduced involving the valid dependency
    iii) If layer validation flags the dependency as invalid, the dependency will need to be removed and both layer and dependency violations will be added to the associated package_todo.yml

  3. Layer validation against unnecessary dependencies is of limited value. If we're concerned about unnecessary dependencies, we could consider making them dependency validation errors.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with keeping what you have, but here's what I think:

  1. I see things slightly differently here. I don't consider the dependencies key to be a property/definition of the dependency checker. Instead, I see dependencies as being part of the fundamental properties of a pack, and both the dependency checker and the architectural checker can apply their independent constraints on top of that shared concept.
  2. This almost seems like a good thing to me and is conceptually correct – the new architecture constraint would dictate that the dependency is no longer permitted. If the user wants to ignore the subsequent dependency violations associated, they can add the dependency to ignored_dependencies.
  3. Agreed, it doesn't add much value. I think it's nice in so far as someone can trust that when they look at a package dependencies, it should never violate the architecture.

Ultimately though no big deal... we can try this out and iterate!

Invalid 'dependencies' in 'packs/baz/package.yml'. 'packs/baz/package.yml' has a layer type of 'technical_services,' which cannot rely on 'packs/bar,' which has a layer type of 'admin.' `architecture_layers` can be found in packwerk.yml",
"\'layer\' must be specified in \'packs/baz/package.yml\' because `enforce_architecture` is true or strict.",
);
let expected_message_2 = String::from(
"
Invalid 'dependencies' in 'packs/foo/package.yml'. 'packs/foo/package.yml' has a layer type of 'product,' which cannot rely on 'packs/bar,' which has a layer type of 'admin.' `architecture_layers` can be found in packwerk.yml",
"Invalid \'layer\' option in \'packs/foo/package.yml\'. `layer` must be one of the layers defined in `packwerk.yml`"
);

Command::cargo_bin("packs")
Expand Down
Loading