From 020ea0e36dd7eba4ee1e97b6d6b5710f181d3815 Mon Sep 17 00:00:00 2001 From: Peter Gafert Date: Sat, 26 Aug 2023 15:58:58 +0200 Subject: [PATCH] Review: I think to be consistent we need to offer both the positive and the negative form of `beAccessedByMethodsThat`, just like the other methods. Signed-off-by: Peter Gafert --- .../lang/conditions/ArchConditions.java | 11 ++-- .../lang/syntax/FieldsShouldInternal.java | 7 ++- .../lang/syntax/elements/FieldsShould.java | 20 +++++++ .../syntax/elements/FieldsShouldTest.java | 54 ++++++++++++++----- 4 files changed, 73 insertions(+), 19 deletions(-) diff --git a/archunit/src/main/java/com/tngtech/archunit/lang/conditions/ArchConditions.java b/archunit/src/main/java/com/tngtech/archunit/lang/conditions/ArchConditions.java index 060b3a97e..7931dfd3d 100644 --- a/archunit/src/main/java/com/tngtech/archunit/lang/conditions/ArchConditions.java +++ b/archunit/src/main/java/com/tngtech/archunit/lang/conditions/ArchConditions.java @@ -1275,13 +1275,16 @@ public static ArchCondition onlyBeCalledByConstructorsThat(Describ } @PublicAPI(usage = ACCESS) - public static ArchCondition notBeAccessedByMethodsThat(DescribedPredicate predicate) { - return new ArchCondition("not be accessed by methods that " + predicate.getDescription()) { + public static ArchCondition beAccessedByMethodsThat(DescribedPredicate predicate) { + return new ArchCondition("be accessed by methods that " + predicate.getDescription()) { @Override public void check(JavaField field, ConditionEvents events) { field.getAccessesToSelf().stream() - .filter(access -> access.getOrigin() instanceof JavaMethod && predicate.test((JavaMethod) access.getOrigin())) - .forEach(violation -> events.add(SimpleConditionEvent.violated(field, violation.getDescription()))); + .filter(access -> access.getOrigin() instanceof JavaMethod) + .forEach(access -> { + boolean satisfied = predicate.test((JavaMethod) access.getOrigin()); + events.add(new SimpleConditionEvent(field, satisfied, access.getDescription())); + }); } }; } diff --git a/archunit/src/main/java/com/tngtech/archunit/lang/syntax/FieldsShouldInternal.java b/archunit/src/main/java/com/tngtech/archunit/lang/syntax/FieldsShouldInternal.java index aa2448d72..7651bceef 100644 --- a/archunit/src/main/java/com/tngtech/archunit/lang/syntax/FieldsShouldInternal.java +++ b/archunit/src/main/java/com/tngtech/archunit/lang/syntax/FieldsShouldInternal.java @@ -95,8 +95,13 @@ public FieldsShouldInternal notHaveRawType(DescribedPredicate return addCondition(not(ArchConditions.haveRawType(predicate))); } + @Override + public FieldsShouldInternal beAccessedByMethodsThat(DescribedPredicate predicate) { + return addCondition(ArchConditions.beAccessedByMethodsThat(predicate)); + } + @Override public FieldsShouldInternal notBeAccessedByMethodsThat(DescribedPredicate predicate) { - return addCondition(ArchConditions.notBeAccessedByMethodsThat(predicate)); + return addCondition(not(ArchConditions.beAccessedByMethodsThat(predicate))); } } diff --git a/archunit/src/main/java/com/tngtech/archunit/lang/syntax/elements/FieldsShould.java b/archunit/src/main/java/com/tngtech/archunit/lang/syntax/elements/FieldsShould.java index 4e7118db1..2b5e6e1e5 100644 --- a/archunit/src/main/java/com/tngtech/archunit/lang/syntax/elements/FieldsShould.java +++ b/archunit/src/main/java/com/tngtech/archunit/lang/syntax/elements/FieldsShould.java @@ -157,11 +157,31 @@ public interface FieldsShould exten @PublicAPI(usage = ACCESS) CONJUNCTION notHaveRawType(DescribedPredicate predicate); + /** + * Asserts that fields are accessed by at least one method matching the given predicate. + * This mostly makes sense in the negated form + * 

+     * {@link ArchRuleDefinition#noFields() noFields()}.{@link GivenFields#should() should()}.{@link #beAccessedByMethodsThat(DescribedPredicate)}
+     *
+ * In this form it is equivalent to + * 

+     * {@link ArchRuleDefinition#fields()} fields()}.{@link GivenFields#should() should()}.{@link #notBeAccessedByMethodsThat(DescribedPredicate)}
+     *
+ * but might be useful for chaining multiple conditions via {@link FieldsShouldConjunction#andShould()}. + * + * @param predicate A predicate determining the methods this field should not be accessed by + * @return A syntax element that can either be used as working rule, or to continue specifying a more complex rule + * @see #notBeAccessedByMethodsThat(DescribedPredicate) + */ + @PublicAPI(usage = ACCESS) + CONJUNCTION beAccessedByMethodsThat(DescribedPredicate predicate); + /** * Asserts that fields are not accessed by methods matching the given predicate. * * @param predicate A predicate determining the methods this field should not be accessed by * @return A syntax element that can either be used as working rule, or to continue specifying a more complex rule + * @see #beAccessedByMethodsThat(DescribedPredicate) */ @PublicAPI(usage = ACCESS) CONJUNCTION notBeAccessedByMethodsThat(DescribedPredicate predicate); diff --git a/archunit/src/test/java/com/tngtech/archunit/lang/syntax/elements/FieldsShouldTest.java b/archunit/src/test/java/com/tngtech/archunit/lang/syntax/elements/FieldsShouldTest.java index b62985a10..51604103f 100644 --- a/archunit/src/test/java/com/tngtech/archunit/lang/syntax/elements/FieldsShouldTest.java +++ b/archunit/src/test/java/com/tngtech/archunit/lang/syntax/elements/FieldsShouldTest.java @@ -6,7 +6,7 @@ import java.util.Set; import com.google.common.collect.ImmutableList; -import com.tngtech.archunit.base.DescribedPredicate; +import com.tngtech.archunit.core.importer.ClassFileImporter; import com.tngtech.archunit.lang.ArchRule; import com.tngtech.archunit.lang.EvaluationResult; import com.tngtech.java.junit.dataprovider.DataProvider; @@ -18,12 +18,18 @@ import static com.google.common.collect.Iterables.getOnlyElement; import static com.tngtech.archunit.core.domain.JavaClass.Predicates.equivalentTo; import static com.tngtech.archunit.core.domain.TestUtils.importClasses; +import static com.tngtech.archunit.core.domain.properties.HasName.Predicates.name; +import static com.tngtech.archunit.lang.conditions.ArchPredicates.have; import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.fields; +import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.noFields; import static com.tngtech.archunit.lang.syntax.elements.GivenMembersTest.areNoFieldsWithType; import static com.tngtech.archunit.lang.syntax.elements.GivenMembersTest.assertViolation; import static com.tngtech.archunit.lang.syntax.elements.MembersShouldTest.parseMembers; +import static com.tngtech.archunit.testutil.Assertions.assertThatRule; import static com.tngtech.java.junit.dataprovider.DataProviders.$; import static com.tngtech.java.junit.dataprovider.DataProviders.$$; +import static com.tngtech.java.junit.dataprovider.DataProviders.testForEach; +import static java.util.regex.Pattern.quote; import static org.assertj.core.api.Assertions.assertThat; @RunWith(DataProviderRunner.class) @@ -37,18 +43,13 @@ public void complex_field_syntax() { .should().beAnnotatedWith(A.class) .andShould().notBePublic() .orShould().haveRawType(Map.class) - .andShould().notBeAccessedByMethodsThat(DescribedPredicate.alwaysTrue()) .evaluate(importClasses(ClassWithVariousMembers.class)); assertViolation(result); - String fieldAFailures = getOnlyElement(result.filterDescriptionsMatching(s -> s.contains(FIELD_A)).getFailureReport().getDetails()); - assertThat(fieldAFailures) + String failure = getOnlyElement(result.getFailureReport().getDetails()); + assertThat(failure) .containsPattern(String.format("%s.* does not have raw type %s", FIELD_A, Map.class.getName())) .containsPattern(String.format("%s.* is not annotated with @%s", FIELD_A, A.class.getSimpleName())); - - String fieldDFailures = - getOnlyElement(result.filterDescriptionsMatching(description -> description.contains(FIELD_D)).getFailureReport().getDetails()); - assertThat(fieldDFailures).containsPattern(String.format("%s.* gets field .*%s", METHOD_A, FIELD_D)); } @DataProvider @@ -77,13 +78,42 @@ public void property_predicates(ArchRule rule, Collection expectedViolat assertThat(actualFields).hasSameElementsAs(expectedViolatingFields); } + @DataProvider + public static Object[][] data_be_accessed_by_methods() { + return testForEach( + fields().should().notBeAccessedByMethodsThat(have(name("toFind"))), + noFields().should().beAccessedByMethodsThat(have(name("toFind"))) + ); + } + + @Test + @UseDataProvider + public void test_be_accessed_by_methods(ArchRule ruleCheckingAccessToMethod) { + class ClassWithAccessedField { + String field; + } + @SuppressWarnings("unused") + class AccessFromMethod { + void toFind(ClassWithAccessedField target) { + target.field = "changed"; + } + + void toIgnore(ClassWithAccessedField target) { + target.field = "changed"; + } + } + + assertThatRule(ruleCheckingAccessToMethod) + .checking(new ClassFileImporter().importClasses(ClassWithAccessedField.class, AccessFromMethod.class)) + .hasOnlyOneViolationMatching(".*" + quote(AccessFromMethod.class.getName()) + ".*toFind.*") + .hasNoViolationMatching(".*toIgnore.*"); + } + private static final String FIELD_A = "fieldA"; private static final String FIELD_B = "fieldB"; private static final String FIELD_C = "fieldC"; private static final String FIELD_D = "fieldD"; - private static final String METHOD_A = "methodA()"; - @SuppressWarnings({"unused"}) private static class ClassWithVariousMembers { private final String fieldA = "A"; @@ -91,10 +121,6 @@ private static class ClassWithVariousMembers { protected static final Object fieldB = 'B'; public List fieldC; static Map fieldD; - - public void methodA() { - Map tmp = fieldD; - } } private @interface A {