Skip to content

Commit 59bb586

Browse files
codecholericcodecholeric
committed
fix confusing behavior of only{Call/Access}...That... syntax
To make assertions about access targets is in general not trivial. From the byte code we can only infer that the method `x` with some signature was called on target class `y`. This does not give us enough information to make assertions about the method itself (thus ArchUnit distinguishes an `AccessTarget` having a name and an owner where it is declared from a full `JavaMember` which can also have e.g. annotations, an information that cannot be derived from the mere byte code access). Syntax elements of the form `only{Call/Access}...That...`, which want to apply a predicate to some subclass of `JavaMember`, must thus try to resolve the actual call of a target first (by calling `target.resolve()` which will in some cases even report multiple targets, but can also return no target at all, e.g. if the target class has not been imported and thus there is no information about the full method). So far these methods have asserted that at least one element of the resolved targets satisfies the predicate. Unfortunately this will fail and report a violation whenever the target has not been imported, which is a somewhat confusing behavior. Originally I thought it would be good, because if one wants to make assertions about the resolved targets, one should ensure all targets are present and if not failure is okay. But on the one hand the error message is pretty confusing (which by itself would be fixable), on the other hand there are simply classes that cannot be correctly imported, for example array types. Since this behavior is confusing I have decided to let the test pass if the target cannot be resolved. This could lead to tests wrongly passing if not all necessary classes are imported, but in the end it is an user decision what to import and hard to reason about from ArchUnit's point of view (also now that `resolveMissingClassesFromClasspath` is `true` by default, it should also happen way less often, that necessary classes are forgotten for the import). Signed-off-by: Peter Gafert <[email protected]>
1 parent 5cd395e commit 59bb586

File tree

6 files changed

+66
-18
lines changed

6 files changed

+66
-18
lines changed

‎archunit-integration-test/src/test/java/com/tngtech/archunit/integration/ExamplesIntegrationTest.java

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -282,9 +282,6 @@ Stream<DynamicTest> ControllerRulesTest() {
282282
.by(callFromMethod(SomeController.class, "doSthController")
283283
.toMethod(ServiceViolatingDaoRules.class, "doSthService")
284284
.inLine(11))
285-
.by(callFromMethod(SomeEnum.class, "values")
286-
.toMethod(SomeEnum[].class, "clone")
287-
.inLine(3))
288285

289286
.ofRule(String.format("classes that reside in a package '..controller..' should "
290287
+ "only call constructors that are declared in a package '..controller..' or are annotated with @%s",
@@ -308,9 +305,6 @@ Stream<DynamicTest> ControllerRulesTest() {
308305
.by(callFromConstructor(UseCaseTwoController.class)
309306
.toConstructor(AbstractController.class)
310307
.inLine(6))
311-
.by(callFromMethod(SomeEnum.class, "values")
312-
.toMethod(SomeEnum[].class, "clone")
313-
.inLine(3))
314308

315309
.ofRule(String.format("classes that reside in a package '..controller..' should "
316310
+ "only access fields that are declared in a package '..controller..' or are annotated with @%s",
@@ -334,9 +328,6 @@ Stream<DynamicTest> ControllerRulesTest() {
334328
.by(callFromConstructor(UseCaseTwoController.class)
335329
.toConstructor(AbstractController.class)
336330
.inLine(6))
337-
.by(callFromMethod(SomeEnum.class, "values")
338-
.toMethod(SomeEnum[].class, "clone")
339-
.inLine(3))
340331

341332
.toDynamicTests();
342333
}

‎archunit/src/main/java/com/tngtech/archunit/base/DescribedPredicate.java

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
*/
1616
package com.tngtech.archunit.base;
1717

18+
import com.google.common.collect.Iterables;
1819
import com.tngtech.archunit.PublicAPI;
1920

2021
import static com.google.common.base.Preconditions.checkArgument;
@@ -128,6 +129,10 @@ public static <T> DescribedPredicate<T> not(final DescribedPredicate<? super T>
128129
return new NotPredicate<>(predicate);
129130
}
130131

132+
public static DescribedPredicate<Iterable<?>> empty() {
133+
return new EmptyPredicate();
134+
}
135+
131136
public static <T> DescribedPredicate<Iterable<T>> anyElementThat(final DescribedPredicate<? super T> predicate) {
132137
return new AnyElementPredicate<>(predicate);
133138
}
@@ -296,6 +301,17 @@ public boolean apply(T input) {
296301
}
297302
}
298303

304+
private static class EmptyPredicate extends DescribedPredicate<Iterable<?>> {
305+
EmptyPredicate() {
306+
super("empty");
307+
}
308+
309+
@Override
310+
public boolean apply(Iterable<?> input) {
311+
return Iterables.isEmpty(input);
312+
}
313+
}
314+
299315
private static class AnyElementPredicate<T> extends DescribedPredicate<Iterable<T>> {
300316
private final DescribedPredicate<T> predicate;
301317

‎archunit/src/main/java/com/tngtech/archunit/lang/conditions/ArchConditions.java

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@
6565

6666
import static com.tngtech.archunit.PublicAPI.Usage.ACCESS;
6767
import static com.tngtech.archunit.base.DescribedPredicate.anyElementThat;
68+
import static com.tngtech.archunit.base.DescribedPredicate.empty;
6869
import static com.tngtech.archunit.core.domain.Dependency.Functions.GET_ORIGIN_CLASS;
6970
import static com.tngtech.archunit.core.domain.Dependency.Functions.GET_TARGET_CLASS;
7071
import static com.tngtech.archunit.core.domain.Dependency.Predicates.dependencyOrigin;
@@ -169,7 +170,7 @@ public static ArchCondition<JavaClass> accessFieldWhere(DescribedPredicate<? sup
169170
public static ArchCondition<JavaClass> onlyAccessFieldsThat(final DescribedPredicate<? super JavaField> predicate) {
170171
ChainableFunction<JavaFieldAccess, FieldAccessTarget> getTarget = JavaAccess.Functions.Get.target();
171172
DescribedPredicate<JavaFieldAccess> accessPredicate = getTarget.then(FieldAccessTarget.Functions.RESOLVE)
172-
.is(anyElementThat(predicate.<JavaField>forSubType()));
173+
.is(anyElementThat(predicate.<JavaField>forSubType()).or(empty()));
173174
return new ClassOnlyAccessesCondition<>(accessPredicate, GET_FIELD_ACCESSES_FROM_SELF)
174175
.as("only access fields that " + predicate.getDescription());
175176
}
@@ -202,7 +203,7 @@ public static ArchCondition<JavaClass> callMethodWhere(final DescribedPredicate<
202203
public static ArchCondition<JavaClass> onlyCallMethodsThat(final DescribedPredicate<? super JavaMethod> predicate) {
203204
ChainableFunction<JavaMethodCall, MethodCallTarget> getTarget = JavaAccess.Functions.Get.target();
204205
DescribedPredicate<JavaMethodCall> callPredicate = getTarget.then(MethodCallTarget.Functions.RESOLVE)
205-
.is(anyElementThat(predicate.<JavaMethod>forSubType()));
206+
.is(anyElementThat(predicate.<JavaMethod>forSubType()).or(empty()));
206207
return new ClassOnlyAccessesCondition<>(callPredicate, GET_METHOD_CALLS_FROM_SELF)
207208
.as("only call methods that " + predicate.getDescription());
208209
}
@@ -235,7 +236,7 @@ public static ArchCondition<JavaClass> callConstructorWhere(final DescribedPredi
235236
public static ArchCondition<JavaClass> onlyCallConstructorsThat(final DescribedPredicate<? super JavaConstructor> predicate) {
236237
ChainableFunction<JavaConstructorCall, ConstructorCallTarget> getTarget = JavaAccess.Functions.Get.target();
237238
DescribedPredicate<JavaConstructorCall> callPredicate = getTarget.then(ConstructorCallTarget.Functions.RESOLVE)
238-
.is(anyElementThat(predicate.<JavaConstructor>forSubType()));
239+
.is(anyElementThat(predicate.<JavaConstructor>forSubType()).or(empty()));
239240
return new ClassOnlyAccessesCondition<>(callPredicate, GET_CONSTRUCTOR_CALLS_FROM_SELF)
240241
.as("only call constructors that " + predicate.getDescription());
241242
}
@@ -250,7 +251,7 @@ public static ArchCondition<JavaClass> callCodeUnitWhere(DescribedPredicate<? su
250251
public static ArchCondition<JavaClass> onlyCallCodeUnitsThat(final DescribedPredicate<? super JavaCodeUnit> predicate) {
251252
ChainableFunction<JavaCall<?>, CodeUnitCallTarget> getTarget = JavaAccess.Functions.Get.target();
252253
DescribedPredicate<JavaCall<?>> callPredicate = getTarget.then(CodeUnitCallTarget.Functions.RESOLVE)
253-
.is(anyElementThat(predicate.<JavaCodeUnit>forSubType()));
254+
.is(anyElementThat(predicate.<JavaCodeUnit>forSubType()).or(empty()));
254255
return new ClassOnlyAccessesCondition<>(callPredicate, GET_CALLS_FROM_SELF)
255256
.as("only call code units that " + predicate.getDescription());
256257
}
@@ -259,7 +260,7 @@ public static ArchCondition<JavaClass> onlyCallCodeUnitsThat(final DescribedPred
259260
public static ArchCondition<JavaClass> onlyAccessMembersThat(final DescribedPredicate<? super JavaMember> predicate) {
260261
ChainableFunction<JavaAccess<?>, AccessTarget> getTarget = JavaAccess.Functions.Get.target();
261262
DescribedPredicate<JavaAccess<?>> accessPredicate = getTarget.then(AccessTarget.Functions.RESOLVE)
262-
.is(anyElementThat(predicate.<JavaMember>forSubType()));
263+
.is(anyElementThat(predicate.<JavaMember>forSubType()).or(empty()));
263264
return new ClassOnlyAccessesCondition<>(accessPredicate, GET_ACCESSES_FROM_SELF)
264265
.as("only access members that " + predicate.getDescription());
265266
}

‎archunit/src/test/java/com/tngtech/archunit/base/DescribedPredicateTest.java

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
package com.tngtech.archunit.base;
22

3+
import java.util.Collections;
4+
35
import com.google.common.collect.ImmutableList;
46
import com.tngtech.java.junit.dataprovider.DataProvider;
57
import com.tngtech.java.junit.dataprovider.DataProviderRunner;
@@ -14,6 +16,7 @@
1416
import static com.tngtech.archunit.base.DescribedPredicate.describe;
1517
import static com.tngtech.archunit.base.DescribedPredicate.doNot;
1618
import static com.tngtech.archunit.base.DescribedPredicate.doesNot;
19+
import static com.tngtech.archunit.base.DescribedPredicate.empty;
1720
import static com.tngtech.archunit.base.DescribedPredicate.equalTo;
1821
import static com.tngtech.archunit.base.DescribedPredicate.greaterThan;
1922
import static com.tngtech.archunit.base.DescribedPredicate.greaterThanOrEqualTo;
@@ -181,6 +184,16 @@ public void onResultOf_works() {
181184
assertThat(equalTo(5).onResultOf(constant(6))).rejects(new Object());
182185
}
183186

187+
@Test
188+
public void empty_works() {
189+
assertThat(empty())
190+
.hasDescription("empty")
191+
.accepts(Collections.emptyList())
192+
.accepts(Collections.emptySet())
193+
.rejects(ImmutableList.of(1))
194+
.rejects(Collections.singleton(""));
195+
}
196+
184197
@Test
185198
public void anyElementThat_works() {
186199
assertThat(anyElementThat(equalTo(5)))
@@ -230,4 +243,4 @@ public String toString() {
230243
enum Foo {
231244
FIRST, SECOND, THIRD
232245
}
233-
}
246+
}

‎archunit/src/test/java/com/tngtech/archunit/lang/syntax/elements/ClassesShouldTest.java

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
import com.tngtech.archunit.lang.conditions.ArchConditions;
3333
import com.tngtech.archunit.lang.syntax.elements.testclasses.SomeClass;
3434
import com.tngtech.archunit.lang.syntax.elements.testclasses.WrongNamedClass;
35+
import com.tngtech.archunit.testutil.ArchConfigurationRule;
3536
import com.tngtech.java.junit.dataprovider.DataProvider;
3637
import com.tngtech.java.junit.dataprovider.DataProviderRunner;
3738
import com.tngtech.java.junit.dataprovider.UseDataProvider;
@@ -45,6 +46,7 @@
4546
import static com.tngtech.archunit.base.DescribedPredicate.greaterThanOrEqualTo;
4647
import static com.tngtech.archunit.base.DescribedPredicate.lessThan;
4748
import static com.tngtech.archunit.base.DescribedPredicate.lessThanOrEqualTo;
49+
import static com.tngtech.archunit.base.DescribedPredicate.not;
4850
import static com.tngtech.archunit.core.domain.JavaClass.Predicates.type;
4951
import static com.tngtech.archunit.core.domain.JavaClassTest.expectInvalidSyntaxUsageForClassInsteadOfInterface;
5052
import static com.tngtech.archunit.core.domain.JavaConstructor.CONSTRUCTOR_NAME;
@@ -69,11 +71,11 @@
6971
import static com.tngtech.archunit.lang.conditions.ArchConditions.notHaveModifier;
7072
import static com.tngtech.archunit.lang.conditions.ArchPredicates.are;
7173
import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.classes;
74+
import static com.tngtech.archunit.testutil.Assertions.assertThat;
7275
import static com.tngtech.java.junit.dataprovider.DataProviders.$;
7376
import static com.tngtech.java.junit.dataprovider.DataProviders.$$;
7477
import static com.tngtech.java.junit.dataprovider.DataProviders.testForEach;
7578
import static java.util.regex.Pattern.quote;
76-
import static org.assertj.core.api.Assertions.assertThat;
7779

7880
@RunWith(DataProviderRunner.class)
7981
public class ClassesShouldTest {
@@ -82,6 +84,9 @@ public class ClassesShouldTest {
8284
@Rule
8385
public final ExpectedException thrown = ExpectedException.none();
8486

87+
@Rule
88+
public final ArchConfigurationRule configurationRule = new ArchConfigurationRule();
89+
8590
@DataProvider
8691
public static Object[][] haveFullyQualifiedName_rules() {
8792
return $$(
@@ -1693,6 +1698,29 @@ public void notBeClass(ArchRule rule, Class<?> satisfied, Class<?> violated) {
16931698
.doesNotMatch(String.format(".*%s.* is .*", quote(satisfied.getName())));
16941699
}
16951700

1701+
@DataProvider
1702+
public static Object[][] onlyAccessRules_rules() {
1703+
return $$(
1704+
$(classes().should().onlyCallMethodsThat(are(not(declaredIn(ClassWithMethod.class)))), ClassCallingMethod.class),
1705+
$(classes().should(ArchConditions.onlyCallMethodsThat(are(not(declaredIn(ClassWithMethod.class))))), ClassCallingMethod.class),
1706+
$(classes().should().onlyCallConstructorsThat(are(not(declaredIn(ClassWithConstructor.class)))), ClassCallingConstructor.class),
1707+
$(classes().should(ArchConditions.onlyCallConstructorsThat(are(not(declaredIn(ClassWithConstructor.class))))), ClassCallingConstructor.class),
1708+
$(classes().should().onlyCallCodeUnitsThat(are(not(declaredIn(ClassWithMethod.class)))), ClassCallingMethod.class),
1709+
$(classes().should(ArchConditions.onlyCallCodeUnitsThat(are(not(declaredIn(ClassWithMethod.class))))), ClassCallingMethod.class),
1710+
$(classes().should().onlyAccessFieldsThat(are(not(declaredIn(ClassWithField.class)))), ClassAccessingField.class),
1711+
$(classes().should(ArchConditions.onlyAccessFieldsThat(are(not(declaredIn(ClassWithField.class))))), ClassAccessingField.class),
1712+
$(classes().should().onlyAccessMembersThat(are(not(declaredIn(ClassWithField.class)))), ClassAccessingField.class),
1713+
$(classes().should(ArchConditions.onlyAccessMembersThat(are(not(declaredIn(ClassWithField.class))))), ClassAccessingField.class));
1714+
}
1715+
1716+
@Test
1717+
@UseDataProvider("onlyAccessRules_rules")
1718+
public void onlyCall_should_report_success_if_targets_are_non_resolvable(ArchRule rule, Class<?> classCallingUnresolvableTarget) {
1719+
ArchConfiguration.get().setResolveMissingDependenciesFromClassPath(false);
1720+
1721+
assertThat(rule).checking(importClasses(classCallingUnresolvableTarget)).hasNoViolation();
1722+
}
1723+
16961724
static String locationPattern(Class<?> clazz) {
16971725
return String.format("\\(%s.java:0\\)", quote(clazz.getSimpleName()));
16981726
}

‎archunit/src/test/resources/archunit.properties

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,2 +1 @@
1-
resolveMissingDependenciesFromClassPath=true
2-
enableMd5InClassSources=true
1+
enableMd5InClassSources=true

0 commit comments

Comments
 (0)