Skip to content

Commit 0c23de0

Browse files
codecholericcodecholeric
authored
Fix onlyxxx that behaving inconsistently for unresolvable targets TNG#348
So far the set of `should().only{Access/Call}...That...(..)` syntax methods have behaved a little strange when the target of the call was not imported from a class file. This happens for example for unresolvable types like array types. The basic problem is, that from the byte code point of view only certain properties about an `AccessTarget` (like `SomeClass.method()`) can be derived. For further details (e.g. is the method called annotated with a certain annotation) it is necessary to try and resolve the respective method that is targeted from the imported target `JavaClass`. This resolution can unfortunately fail, if the target class has not been imported by ArchUnit (or is unresolvable like an array or primitive type). So far these `only...` methods wanted that at least one of the resolved targets would satisfy the given predicate (e.g. `annotatedWith(..)`). If the target is unresolvable this assertion would always fail, reporting things like `[some.Array;.clone() is not annotated with ...`. But since it would always fail, it could at the same time complain about `[some.Array;.clone() is not annotated with ...` and `[some.Array;.clone() is annotated with ...` which is certainly super confusing (and plain wrong in the second case where `not(annotatedWith(..))` would be used; compare the example). This PR resolves this issue by counting unresolvable targets as always satisfying these predicates. This might lead to wrongly successful tests if classes are missing from the import, but in the end it is indeterminable for ArchUnit anyway, what classes are wanted / desired for the test and which classes are not wanted. It is also a fair point of view to say that unimported classes should not be considered when asserting `should().only...`.
2 parents b7bf88a + 59bb586 commit 0c23de0

File tree

8 files changed

+113
-71
lines changed

8 files changed

+113
-71
lines changed

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

-9
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

+16
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

+6-5
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

+14-1
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/core/domain/SourceCodeLocationTest.java

+46-52
Original file line numberDiff line numberDiff line change
@@ -1,52 +1,46 @@
1-
package com.tngtech.archunit.core.domain;
2-
3-
import java.util.ArrayList;
4-
5-
import com.tngtech.archunit.testutil.ArchConfigurationRule;
6-
import org.assertj.core.api.Assertions;
7-
import org.junit.Before;
8-
import org.junit.Rule;
9-
import org.junit.Test;
10-
11-
import static com.tngtech.archunit.core.domain.TestUtils.importClassWithContext;
12-
import static com.tngtech.archunit.testutil.Assertions.assertThat;
13-
14-
public class SourceCodeLocationTest {
15-
16-
@Rule
17-
public final ArchConfigurationRule configuration = new ArchConfigurationRule();
18-
19-
@Before
20-
public void setUp() {
21-
// We need this to create a JavaClass without source, i.e. a stub because the class is missing and cannot be resolved
22-
configuration.resolveAdditionalDependenciesFromClassPath(false);
23-
}
24-
25-
@Test
26-
public void format_location() {
27-
JavaClass classWithSource = importClassWithContext(Object.class);
28-
29-
assertThat(classWithSource.getSource()).as("source").isPresent();
30-
Assertions.assertThat(SourceCodeLocation.of(classWithSource, 7).toString()).isEqualTo("(Object.java:7)");
31-
32-
JavaClass classWithoutSource = getClassWithoutSource();
33-
34-
assertThat(classWithoutSource.getSource()).as("source").isAbsent();
35-
Assertions.assertThat(SourceCodeLocation.of(classWithoutSource, 7).toString()).isEqualTo(String.format("(%s.java:7)", classWithoutSource.getSimpleName()));
36-
}
37-
38-
private JavaClass getClassWithoutSource() {
39-
for (JavaAccess<?> javaAccess : importClassWithContext(SomeClass.class).getAccessesFromSelf()) {
40-
if (javaAccess.getTargetOwner().isEquivalentTo(ArrayList.class)) {
41-
return javaAccess.getTargetOwner();
42-
}
43-
}
44-
throw new RuntimeException("Could not create any java class without source");
45-
}
46-
47-
private static class SomeClass {
48-
public SomeClass() {
49-
new ArrayList<>();
50-
}
51-
}
52-
}
1+
package com.tngtech.archunit.core.domain;
2+
3+
import java.util.ArrayList;
4+
5+
import com.tngtech.archunit.testutil.ArchConfigurationRule;
6+
import org.assertj.core.api.Assertions;
7+
import org.junit.Rule;
8+
import org.junit.Test;
9+
10+
import static com.tngtech.archunit.core.domain.TestUtils.importClassWithContext;
11+
import static com.tngtech.archunit.testutil.Assertions.assertThat;
12+
13+
public class SourceCodeLocationTest {
14+
15+
// We need this to create a JavaClass without source, i.e. a stub because the class is missing and cannot be resolved
16+
@Rule
17+
public final ArchConfigurationRule configuration = new ArchConfigurationRule().resolveAdditionalDependenciesFromClassPath(false);
18+
19+
@Test
20+
public void format_location() {
21+
JavaClass classWithSource = importClassWithContext(Object.class);
22+
23+
assertThat(classWithSource.getSource()).as("source").isPresent();
24+
Assertions.assertThat(SourceCodeLocation.of(classWithSource, 7).toString()).isEqualTo("(Object.java:7)");
25+
26+
JavaClass classWithoutSource = getClassWithoutSource();
27+
28+
assertThat(classWithoutSource.getSource()).as("source").isAbsent();
29+
Assertions.assertThat(SourceCodeLocation.of(classWithoutSource, 7).toString()).isEqualTo(String.format("(%s.java:7)", classWithoutSource.getSimpleName()));
30+
}
31+
32+
private JavaClass getClassWithoutSource() {
33+
for (JavaAccess<?> javaAccess : importClassWithContext(SomeClass.class).getAccessesFromSelf()) {
34+
if (javaAccess.getTargetOwner().isEquivalentTo(ArrayList.class)) {
35+
return javaAccess.getTargetOwner();
36+
}
37+
}
38+
throw new RuntimeException("Could not create any java class without source");
39+
}
40+
41+
private static class SomeClass {
42+
public SomeClass() {
43+
new ArrayList<>();
44+
}
45+
}
46+
}

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

+29-1
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/java/com/tngtech/archunit/testutil/ArchConfigurationRule.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
import org.junit.rules.ExternalResource;
55

66
public class ArchConfigurationRule extends ExternalResource {
7-
private boolean resolveMissingDependenciesFromClassPath;
7+
private boolean resolveMissingDependenciesFromClassPath = ArchConfiguration.get().resolveMissingDependenciesFromClassPath();
88

99
public ArchConfigurationRule resolveAdditionalDependenciesFromClassPath(boolean enabled) {
1010
resolveMissingDependenciesFromClassPath = enabled;

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

+1-2
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)