From f0557a25909ec83215f9cdb6dd64debc1ac77f8a Mon Sep 17 00:00:00 2001 From: Per Lundberg Date: Wed, 30 Jun 2021 10:35:44 +0300 Subject: [PATCH] Add example illustrating how more advanced method-to-method dependencies can be checked Based on an example provided in https://github.com/TNG/ArchUnit/issues/235, this example aims to help people who are working on similar use cases in their projects. Signed-off-by: Per Lundberg --- .../example/singleton/SingletonClass.java | 25 +++++++ .../SingletonClassInvalidConsumer.java | 23 ++++++ .../SingletonClassValidConsumer.java | 34 +++++++++ ...gletonClassWhitelistedInvalidConsumer.java | 15 ++++ .../archunit/exampletest/SingletonTest.java | 73 +++++++++++++++++++ .../a81a2b54-5a18-4145-b544-7a580aba0425 | 1 - 6 files changed, 170 insertions(+), 1 deletion(-) create mode 100644 example-plain/src/main/java/com/tngtech/archunit/example/singleton/SingletonClass.java create mode 100644 example-plain/src/main/java/com/tngtech/archunit/example/singleton/SingletonClassInvalidConsumer.java create mode 100644 example-plain/src/main/java/com/tngtech/archunit/example/singleton/SingletonClassValidConsumer.java create mode 100644 example-plain/src/main/java/com/tngtech/archunit/example/singleton/SingletonClassWhitelistedInvalidConsumer.java create mode 100644 example-plain/src/test/java/com/tngtech/archunit/exampletest/SingletonTest.java diff --git a/example-plain/src/main/java/com/tngtech/archunit/example/singleton/SingletonClass.java b/example-plain/src/main/java/com/tngtech/archunit/example/singleton/SingletonClass.java new file mode 100644 index 0000000..c055039 --- /dev/null +++ b/example-plain/src/main/java/com/tngtech/archunit/example/singleton/SingletonClass.java @@ -0,0 +1,25 @@ +package com.tngtech.archunit.example.singleton; + +import java.util.function.Supplier; + +import com.google.common.base.Suppliers; + +/** + * @author Per Lundberg + */ +public class SingletonClass { + + private static final Supplier INSTANCE = Suppliers.memoize( SingletonClass::new ); + + private SingletonClass() { + // Private constructor to prevent construction + } + + public void doSomething() { + throw new UnsupportedOperationException(); + } + + public static SingletonClass getInstance() { + return INSTANCE.get(); + } +} diff --git a/example-plain/src/main/java/com/tngtech/archunit/example/singleton/SingletonClassInvalidConsumer.java b/example-plain/src/main/java/com/tngtech/archunit/example/singleton/SingletonClassInvalidConsumer.java new file mode 100644 index 0000000..83fd58b --- /dev/null +++ b/example-plain/src/main/java/com/tngtech/archunit/example/singleton/SingletonClassInvalidConsumer.java @@ -0,0 +1,23 @@ +package com.tngtech.archunit.example.singleton; + +/** + * An example of how {@link SingletonClass} is used incorrectly. This is expected to be detected by + * the corresponding ArchUnit test. + * + * @author Per Lundberg + */ +public class SingletonClassInvalidConsumer { + + void doSomething() { + // This pattern (of calling getInstance() in the midst of a method) is both unadvisable and + // dangerous for the following reasons: + // + // - It makes it impossible to override the dependency for tests, which can lead to + // unnecessarily excessive object mocking. + // + // - It postpones object initialization to an unnecessarily late stage (runtime instead of + // startup-time). Problems with classes that cannot be instantiated risks being "hidden" + // until runtime, defeating the purpose of the "fail fast" philosophy. + SingletonClass.getInstance().doSomething(); + } +} diff --git a/example-plain/src/main/java/com/tngtech/archunit/example/singleton/SingletonClassValidConsumer.java b/example-plain/src/main/java/com/tngtech/archunit/example/singleton/SingletonClassValidConsumer.java new file mode 100644 index 0000000..059f892 --- /dev/null +++ b/example-plain/src/main/java/com/tngtech/archunit/example/singleton/SingletonClassValidConsumer.java @@ -0,0 +1,34 @@ +package com.tngtech.archunit.example.singleton; + +import java.util.function.Supplier; + +import com.google.common.base.Suppliers; + +/** + * An example of how {@link SingletonClass} is used correctly + * + * @author Per Lundberg + */ +public class SingletonClassValidConsumer { + + private static SingletonClassValidConsumer instance; + + private final SingletonClass singletonClass; + + public SingletonClassValidConsumer( SingletonClass singletonClass ) { + this.singletonClass = singletonClass; + } + + public static SingletonClassValidConsumer getInstance() { + // Valid way to call getInstance() on another class: + // + // - We retrieve the instance for the dependency in our own getInstance() method. It is + // passed to our constructor as a constructor parameter, making it easy to override it for + // tests. + if ( instance == null ) { + instance = new SingletonClassValidConsumer( SingletonClass.getInstance() ); + } + + return instance; + } +} diff --git a/example-plain/src/main/java/com/tngtech/archunit/example/singleton/SingletonClassWhitelistedInvalidConsumer.java b/example-plain/src/main/java/com/tngtech/archunit/example/singleton/SingletonClassWhitelistedInvalidConsumer.java new file mode 100644 index 0000000..437a301 --- /dev/null +++ b/example-plain/src/main/java/com/tngtech/archunit/example/singleton/SingletonClassWhitelistedInvalidConsumer.java @@ -0,0 +1,15 @@ +package com.tngtech.archunit.example.singleton; + +/** + * An example of how {@link SingletonClass} is used incorrectly, but explicitly whitelisted. + * + * @author Per Lundberg + */ +public class SingletonClassWhitelistedInvalidConsumer { + + void doSomething() { + // This pattern (of calling getInstance() in the midst of a method) is both unadvisable and + // dangerous for reasons described in SingleTonClassInvalidConsumer + SingletonClass.getInstance().doSomething(); + } +} diff --git a/example-plain/src/test/java/com/tngtech/archunit/exampletest/SingletonTest.java b/example-plain/src/test/java/com/tngtech/archunit/exampletest/SingletonTest.java new file mode 100644 index 0000000..5cb66cd --- /dev/null +++ b/example-plain/src/test/java/com/tngtech/archunit/exampletest/SingletonTest.java @@ -0,0 +1,73 @@ +package com.tngtech.archunit.exampletest; + +import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.methods; + +import java.util.Arrays; +import java.util.Objects; +import java.util.Optional; + +import org.junit.Test; + +import com.tngtech.archunit.core.domain.JavaClasses; +import com.tngtech.archunit.core.domain.JavaMethod; +import com.tngtech.archunit.core.domain.JavaModifier; +import com.tngtech.archunit.core.importer.ClassFileImporter; +import com.tngtech.archunit.example.singleton.SingletonClass; +import com.tngtech.archunit.lang.ArchCondition; +import com.tngtech.archunit.lang.ConditionEvents; +import com.tngtech.archunit.lang.SimpleConditionEvent; + +/** + * @author Per Lundberg + */ +public class SingletonTest { + private static final JavaClasses classes = new ClassFileImporter().importPackagesOf( SingletonClass.class); + + @Test + public void getInstance_is_not_used_from_inside_methods() { + methods() + .that().haveName( "getInstance" ) + .and().areStatic() + + // Note: this is a convoluted way to say "no parameters". + .and().haveRawParameterTypes( new String[0] ) + + .should( onlyBeCalledFromWhitelistedOrigins( + // The class below will trigger a violation unless present as a parameter here. + "com.tngtech.archunit.example.singleton.SingletonClassWhitelistedInvalidConsumer" + ) ) + .because( "" + + "getInstance() calls should not be spread out through the methods of a class. This makes it hard/impossible " + + "to override the dependencies for tests, and also means the dependencies are much harder to identify when " + + "quickly looking at the code. Instead, move all getInstance() calls to the INSTANCE supplier and pass the " + + "dependency to the constructor that way. If this is impossible for a particular case, add the class name to " + + "the whitelist in " + getClass().getName() ) + .check( classes ); + } + + private ArchCondition onlyBeCalledFromWhitelistedOrigins( String... whitelistedOrigins ) { + return new ArchCondition( "only be called by whitelisted methods" ) { + @Override + public void check( JavaMethod method, ConditionEvents events ) { + method.getCallsOfSelf().stream() + // TODO: Add your own exceptions as needed here, if you have particular + // TODO: use cases where getInstance() calls are permissible. + // Static getInstance() methods are always allowed to call getInstance. This + // does not break dependency injection and does not come with significant + // design flaws. + .filter( call -> !( Objects.equals( call.getOrigin().getName(), "getInstance" ) && call.getOrigin().getModifiers().contains( JavaModifier.STATIC ) ) ) + + // Anything not handled by the exceptions above must be explicitly listed in + // the whitelistedOrigins parameter. + .filter( call -> { + Optional result = Arrays.stream( whitelistedOrigins ) + .filter( o -> call.getOrigin().getFullName().startsWith( o ) ) + .findFirst(); + + return result.isEmpty(); + } ) + .forEach( call -> events.add( SimpleConditionEvent.violated( method, call.getDescription() ) ) ); + } + }; + } +} diff --git a/example-plain/src/test/resources/frozen/a81a2b54-5a18-4145-b544-7a580aba0425 b/example-plain/src/test/resources/frozen/a81a2b54-5a18-4145-b544-7a580aba0425 index 7c6acee..85a4afd 100644 --- a/example-plain/src/test/resources/frozen/a81a2b54-5a18-4145-b544-7a580aba0425 +++ b/example-plain/src/test/resources/frozen/a81a2b54-5a18-4145-b544-7a580aba0425 @@ -22,6 +22,5 @@ Method has return type in (ComplexServiceAnnotation.java:0) Method calls method <[Lcom.tngtech.archunit.example.layers.service.ServiceType;.clone()> in (ServiceType.java:3) Method has return type <[Lcom.tngtech.archunit.example.layers.service.ServiceType;> in (ServiceType.java:0) -Method has return type <[Lcom.tngtech.archunit.example.layers.service.ServiceType;> in (ServiceType.java:0) Method calls method in (ServiceViolatingDaoRules.java:27) Method is annotated with in (ServiceViolatingProxyRules.java:0) \ No newline at end of file