Skip to content

Commit

Permalink
Review: Extracted some logic to new class LayerDefinitions, since it …
Browse files Browse the repository at this point in the history 
…felt that too many low level aspects of converting layers to predicates were covered by LayeredArchitecture, i.e. mixing of different abstraction levels. Also replaced copy & paste test with ignores by DataProvider tests for the two essential aspects, reporting violations and the description. The description of predicates was not covered by any test. Moved the quotes "'%s'" to the description of definedBy(packages), since an arbitrary predicate should have the freedom to decide, if it wants to be quoted or not. Also the description "..foo'. '..bar.." without quotes around made no sense, so you had to look in two separate places to understand the connection.
  • Loading branch information
@codecholeric
codecholeric committed Oct 20, 2019
1 parent 17bd747 commit 28b32a0
Show file tree
Hide file tree
Showing 2 changed files with 101 additions and 76 deletions.
100 changes: 66 additions & 34 deletions archunit/src/main/java/com/tngtech/archunit/library/Architectures.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.HashSet;
import java.util.Iterator;
import java.util.LinkedHashMap;
import java.util.LinkedHashSet;
import java.util.List;
Expand All @@ -45,6 +46,7 @@
import static com.google.common.base.Strings.isNullOrEmpty;
import static com.google.common.collect.Lists.newArrayList;
import static com.tngtech.archunit.PublicAPI.Usage.ACCESS;
import static com.tngtech.archunit.base.DescribedPredicate.alwaysFalse;
import static com.tngtech.archunit.core.domain.Dependency.Predicates.dependency;
import static com.tngtech.archunit.core.domain.Dependency.Predicates.dependencyOrigin;
import static com.tngtech.archunit.core.domain.JavaClass.Predicates.equivalentTo;
Expand All @@ -55,6 +57,7 @@
import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.classes;
import static java.lang.System.lineSeparator;
import static java.util.Arrays.asList;
import static java.util.Collections.singleton;

/**
* Offers convenience to assert typical architectures, like a {@link #layeredArchitecture()}.
Expand Down Expand Up @@ -95,19 +98,19 @@ public static LayeredArchitecture layeredArchitecture() {
}

public static final class LayeredArchitecture implements ArchRule {
private final Map<String, LayerDefinition> layerDefinitions;
private final LayerDefinitions layerDefinitions;
private final Set<LayerDependencySpecification> dependencySpecifications;
private final PredicateAggregator<Dependency> irrelevantDependenciesPredicate;
private final Optional<String> overriddenDescription;

private LayeredArchitecture() {
this(new LinkedHashMap<String, LayerDefinition>(),
this(new LayerDefinitions(),
new LinkedHashSet<LayerDependencySpecification>(),
new PredicateAggregator<Dependency>().thatORs(),
Optional.<String>absent());
}

private LayeredArchitecture(Map<String, LayerDefinition> layerDefinitions,
private LayeredArchitecture(LayerDefinitions layerDefinitions,
Set<LayerDependencySpecification> dependencySpecifications,
PredicateAggregator<Dependency> irrelevantDependenciesPredicate,
Optional<String> overriddenDescription) {
Expand All @@ -118,7 +121,7 @@ private LayeredArchitecture(Map<String, LayerDefinition> layerDefinitions,
}

private LayeredArchitecture addLayerDefinition(LayerDefinition definition) {
layerDefinitions.put(definition.name, definition);
layerDefinitions.add(definition);
return this;
}

Expand All @@ -140,7 +143,7 @@ public String getDescription() {
}

List<String> lines = newArrayList("Layered architecture consisting of");
for (LayerDefinition definition : layerDefinitions.values()) {
for (LayerDefinition definition : layerDefinitions) {
lines.add(definition.toString());
}
for (LayerDependencySpecification specification : dependencySpecifications) {
Expand All @@ -149,11 +152,16 @@ public String getDescription() {
return Joiner.on(lineSeparator()).join(lines);
}

@Override
public String toString() {
return getDescription();
}

@Override
@PublicAPI(usage = ACCESS)
public EvaluationResult evaluate(JavaClasses classes) {
EvaluationResult result = new EvaluationResult(this, Priority.MEDIUM);
for (LayerDefinition layerDefinition : layerDefinitions.values()) {
for (LayerDefinition layerDefinition : layerDefinitions) {
result.add(evaluateLayersShouldNotBeEmpty(classes, layerDefinition));
}
for (LayerDependencySpecification specification : dependencySpecifications) {
Expand All @@ -163,39 +171,21 @@ public EvaluationResult evaluate(JavaClasses classes) {
}

private EvaluationResult evaluateLayersShouldNotBeEmpty(JavaClasses classes, LayerDefinition layerDefinition) {
return classes().that(matchLayerDefinition(layerDefinition.name))
return classes().that(layerDefinitions.containsPredicateFor(layerDefinition.name))
.should(notBeEmptyFor(layerDefinition))
.evaluate(classes);
}

private EvaluationResult evaluateDependenciesShouldBeSatisfied(JavaClasses classes, LayerDependencySpecification specification) {

return classes().that(matchLayerDefinition(specification.layerName))
return classes().that(layerDefinitions.containsPredicateFor(specification.layerName))
.should(onlyHaveDependentsWhere(originMatchesIfDependencyIsRelevant(specification.layerName, specification.allowedAccessors)))
.evaluate(classes);
}

private DescribedPredicate<JavaClass> matchLayerDefinition(String layerName) {
return matchLayerDefinition(Collections.singleton(layerName));
}

private DescribedPredicate<JavaClass> matchLayerDefinition(final Collection<String> layerNames) {
return new DescribedPredicate<JavaClass>("any element") {
@Override
public boolean apply(JavaClass input) {
for (String layerName : layerNames) {
if (layerDefinitions.get(layerName).describedPredicate.apply(input)) {
return true;
}
}
return false;
}
};
}

private DescribedPredicate<Dependency> originMatchesIfDependencyIsRelevant(String ownLayer, Set<String> allowedAccessors) {
DescribedPredicate<Dependency> originPackageMatches =
dependencyOrigin(matchLayerDefinition(allowedAccessors)).or(dependencyOrigin(matchLayerDefinition(ownLayer)));
dependencyOrigin(layerDefinitions.containsPredicateFor(allowedAccessors)).or(dependencyOrigin(layerDefinitions.containsPredicateFor(ownLayer)));

return irrelevantDependenciesPredicate.isPresent() ?
originPackageMatches.or(irrelevantDependenciesPredicate.get()) :
Expand Down Expand Up @@ -274,13 +264,50 @@ public LayerDependencySpecification whereLayer(String name) {

private void checkLayerNamesExist(String... layerNames) {
for (String layerName : layerNames) {
checkArgument(layerDefinitions.containsKey(layerName), "There is no layer named '%s'", layerName);
checkArgument(layerDefinitions.containLayer(layerName), "There is no layer named '%s'", layerName);
}
}

private static final class LayerDefinitions implements Iterable<LayerDefinition> {
private final Map<String, LayerDefinition> layerDefinitions = new LinkedHashMap<>();

void add(LayerDefinition definition) {
layerDefinitions.put(definition.name, definition);
}

boolean containLayer(String layerName) {
return layerDefinitions.containsKey(layerName);
}

DescribedPredicate<JavaClass> containsPredicateFor(String layerName) {
return containsPredicateFor(singleton(layerName));
}

DescribedPredicate<JavaClass> containsPredicateFor(final Collection<String> layerNames) {
DescribedPredicate<JavaClass> result = alwaysFalse();
for (LayerDefinition definition : get(layerNames)) {
result = result.or(definition.containsPredicate());
}
return result;
}

private Iterable<LayerDefinition> get(Collection<String> layerNames) {
Set<LayerDefinition> result = new HashSet<>();
for (String layerName : layerNames) {
result.add(layerDefinitions.get(layerName));
}
return result;
}

@Override
public Iterator<LayerDefinition> iterator() {
return layerDefinitions.values().iterator();
}
}

public final class LayerDefinition {
private final String name;
private DescribedPredicate<JavaClass> describedPredicate;
private DescribedPredicate<JavaClass> containsPredicate;

private LayerDefinition(String name) {
checkState(!isNullOrEmpty(name), "Layer name must be present");
Expand All @@ -289,19 +316,24 @@ private LayerDefinition(String name) {

@PublicAPI(usage = ACCESS)
public LayeredArchitecture definedBy(DescribedPredicate<JavaClass> predicate) {
checkNotNull(predicate, "predicate must be present");
this.describedPredicate = predicate;
checkNotNull(predicate, "Supplied predicate must not be null");
this.containsPredicate = predicate;
return LayeredArchitecture.this.addLayerDefinition(this);
}

@PublicAPI(usage = ACCESS)
public LayeredArchitecture definedBy(String... packageIdentifiers) {
return definedBy(resideInAnyPackage(packageIdentifiers).as(Joiner.on("', '").join(packageIdentifiers)));
String description = String.format("'%s'", Joiner.on("', '").join(packageIdentifiers));
return definedBy(resideInAnyPackage(packageIdentifiers).as(description));
}

DescribedPredicate<JavaClass> containsPredicate() {
return containsPredicate;
}

@Override
public String toString() {
return String.format("layer '%s' ('%s')", name, describedPredicate);
return String.format("layer '%s' (%s)", name, containsPredicate);
}
}

Expand Down
77 changes: 35 additions & 42 deletions archunit/src/test/java/com/tngtech/archunit/library/ArchitecturesTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@

import com.google.common.base.Joiner;
import com.google.common.collect.ImmutableSet;
import com.tngtech.archunit.core.domain.JavaClass;
import com.tngtech.archunit.core.domain.JavaClasses;
import com.tngtech.archunit.core.importer.ClassFileImporter;
import com.tngtech.archunit.lang.ArchRule;
Expand Down Expand Up @@ -36,9 +35,11 @@
import org.junit.rules.ExpectedException;
import org.junit.runner.RunWith;

import static com.tngtech.archunit.core.domain.JavaClass.Predicates.resideInAnyPackage;
import static com.tngtech.archunit.core.domain.properties.HasName.Predicates.name;
import static com.tngtech.archunit.library.Architectures.layeredArchitecture;
import static com.tngtech.archunit.library.Architectures.onionArchitecture;
import static com.tngtech.java.junit.dataprovider.DataProviders.testForEach;
import static java.beans.Introspector.decapitalize;
import static java.lang.System.lineSeparator;
import static java.util.regex.Pattern.quote;
Expand All @@ -51,21 +52,39 @@ public class ArchitecturesTest {
@Rule
public final ExpectedException thrown = ExpectedException.none();

@Test
public void layered_architecture_description() {
LayeredArchitecture architecture = layeredArchitecture()
.layer("One").definedBy("some.pkg..")
.layer("Two").definedBy("first.any.pkg..", "second.any.pkg..")
.layer("Three").definedBy("..three..")
.whereLayer("One").mayNotBeAccessedByAnyLayer()
.whereLayer("Two").mayOnlyBeAccessedByLayers("One")
.whereLayer("Three").mayOnlyBeAccessedByLayers("One", "Two");
@DataProvider
public static Object[][] layeredArchitectureDefinitions() {
return testForEach(
layeredArchitecture()
.layer("One").definedBy("..library.testclasses.some.pkg..")
.layer("Two").definedBy("..library.testclasses.first.any.pkg..", "..library.testclasses.second.any.pkg..")
.layer("Three").definedBy("..library.testclasses..three..")
.whereLayer("One").mayNotBeAccessedByAnyLayer()
.whereLayer("Two").mayOnlyBeAccessedByLayers("One")
.whereLayer("Three").mayOnlyBeAccessedByLayers("One", "Two"),
layeredArchitecture()
.layer("One").definedBy(
resideInAnyPackage("..library.testclasses.some.pkg..")
.as("'..library.testclasses.some.pkg..'"))
.layer("Two").definedBy(
resideInAnyPackage("..library.testclasses.first.any.pkg..", "..library.testclasses.second.any.pkg..")
.as("'..library.testclasses.first.any.pkg..', '..library.testclasses.second.any.pkg..'"))
.layer("Three").definedBy(
resideInAnyPackage("..library.testclasses..three..")
.as("'..library.testclasses..three..'"))
.whereLayer("One").mayNotBeAccessedByAnyLayer()
.whereLayer("Two").mayOnlyBeAccessedByLayers("One")
.whereLayer("Three").mayOnlyBeAccessedByLayers("One", "Two"));
}

@Test
@UseDataProvider("layeredArchitectureDefinitions")
public void layered_architecture_description(LayeredArchitecture architecture) {
assertThat(architecture.getDescription()).isEqualTo(
"Layered architecture consisting of" + lineSeparator() +
"layer 'One' ('some.pkg..')" + lineSeparator() +
"layer 'Two' ('first.any.pkg..', 'second.any.pkg..')" + lineSeparator() +
"layer 'Three' ('..three..')" + lineSeparator() +
"layer 'One' ('..library.testclasses.some.pkg..')" + lineSeparator() +
"layer 'Two' ('..library.testclasses.first.any.pkg..', '..library.testclasses.second.any.pkg..')" + lineSeparator() +
"layer 'Three' ('..library.testclasses..three..')" + lineSeparator() +
"where layer 'One' may not be accessed by any layer" + lineSeparator() +
"where layer 'Two' may only be accessed by layers ['One']" + lineSeparator() +
"where layer 'Three' may only be accessed by layers ['One', 'Two']");
Expand Down Expand Up @@ -124,21 +143,14 @@ public void layered_architecture_defining_empty_layers_is_rejected() {
JavaClasses classes = new ClassFileImporter().importPackages(getClass().getPackage().getName() + ".testclasses");

EvaluationResult result = architecture.evaluate(classes);
assertThat(result.hasViolation()).isTrue();
assertThat(result.hasViolation()).as("result of evaluating empty layers has violation").isTrue();
assertPatternMatches(result.getFailureReport().getDetails(),
ImmutableSet.of(expectedEmptyLayer("Some"), expectedEmptyLayer("Other")));
}

@Test
public void layered_architecture_gathers_all_layer_violations() {
LayeredArchitecture architecture = layeredArchitecture()
.layer("One").definedBy(absolute("some.pkg.."))
.layer("Two").definedBy(absolute("first.any.pkg..", "second.any.pkg.."))
.layer("Three").definedBy(absolute("..three.."))
.whereLayer("One").mayNotBeAccessedByAnyLayer()
.whereLayer("Two").mayOnlyBeAccessedByLayers("One")
.whereLayer("Three").mayOnlyBeAccessedByLayers("One", "Two");

@UseDataProvider("layeredArchitectureDefinitions")
public void layered_architecture_gathers_all_layer_violations(LayeredArchitecture architecture) {
JavaClasses classes = new ClassFileImporter().importPackages(getClass().getPackage().getName() + ".testclasses");

EvaluationResult result = architecture.evaluate(classes);
Expand Down Expand Up @@ -206,25 +218,6 @@ public void layered_architecture_combines_multiple_ignores() {
assertThat(layeredArchitecture.evaluate(classes).hasViolation()).as("result has violation").isFalse();
}

@Test
public void layered_architecture_combines_multiple_ignores_using_predicate_definition() {
JavaClasses classes = new ClassFileImporter().importClasses(
FirstAnyPkgClass.class, SomePkgSubClass.class,
SecondThreeAnyClass.class, SomePkgClass.class);

LayeredArchitecture layeredArchitecture = layeredArchitecture()
.layer("One").definedBy(JavaClass.Predicates.simpleNameStartingWith("SomePkg"))
.whereLayer("One").mayNotBeAccessedByAnyLayer()
.ignoreDependency(FirstAnyPkgClass.class, SomePkgSubClass.class);

assertThat(layeredArchitecture.evaluate(classes).hasViolation()).as("result has violation").isTrue();

layeredArchitecture = layeredArchitecture
.ignoreDependency(SecondThreeAnyClass.class, SomePkgClass.class);

assertThat(layeredArchitecture.evaluate(classes).hasViolation()).as("result has violation").isFalse();
}

@Test
public void onion_architecture_description() {
OnionArchitecture architecture = onionArchitecture()
Expand Down

0 comments on commit 28b32a0

Please sign in to comment.