diff --git a/archunit-example/example-plain/src/main/java/com/tngtech/archunit/example/singleton/SingletonClass.java b/archunit-example/example-plain/src/main/java/com/tngtech/archunit/example/singleton/SingletonClass.java new file mode 100644 index 0000000000..c0550394ae --- /dev/null +++ b/archunit-example/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/archunit-example/example-plain/src/main/java/com/tngtech/archunit/example/singleton/SingletonClassInvalidConsumer.java b/archunit-example/example-plain/src/main/java/com/tngtech/archunit/example/singleton/SingletonClassInvalidConsumer.java new file mode 100644 index 0000000000..83fd58b335 --- /dev/null +++ b/archunit-example/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/archunit-example/example-plain/src/main/java/com/tngtech/archunit/example/singleton/SingletonClassValidConsumer.java b/archunit-example/example-plain/src/main/java/com/tngtech/archunit/example/singleton/SingletonClassValidConsumer.java new file mode 100644 index 0000000000..059f89201e --- /dev/null +++ b/archunit-example/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/archunit-example/example-plain/src/main/java/com/tngtech/archunit/example/singleton/SingletonClassWhitelistedInvalidConsumer.java b/archunit-example/example-plain/src/main/java/com/tngtech/archunit/example/singleton/SingletonClassWhitelistedInvalidConsumer.java new file mode 100644 index 0000000000..437a3010c2 --- /dev/null +++ b/archunit-example/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/archunit-example/example-plain/src/test/java/com/tngtech/archunit/exampletest/SingletonTest.java b/archunit-example/example-plain/src/test/java/com/tngtech/archunit/exampletest/SingletonTest.java new file mode 100644 index 0000000000..0bfad5196d --- /dev/null +++ b/archunit-example/example-plain/src/test/java/com/tngtech/archunit/exampletest/SingletonTest.java @@ -0,0 +1,75 @@ +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 org.junit.experimental.categories.Category; + +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 + */ +@Category(Example.class) +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.isPresent(); + } ) + .forEach( call -> events.add( SimpleConditionEvent.violated( method, call.getDescription() ) ) ); + } + }; + } +}