Add example illustrating how more advanced method-to-method dependenc…

…ies can be checked

Based on an example provided in
#235, this example aims to help
people who are working on similar use cases in their projects.

Signed-off-by: Per Lundberg <[email protected]>

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<SingletonClass> 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();
+    }
+}

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();
+    }
+}

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;
+    }
+}

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();
+    }
+}

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<JavaMethod> onlyBeCalledFromWhitelistedOrigins( String... whitelistedOrigins ) {
+        return new ArchCondition<JavaMethod>( "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<String> result = Arrays.stream( whitelistedOrigins )
+                                    .filter( o -> call.getOrigin().getFullName().startsWith( o ) )
+                                    .findFirst();
+
+                            return !result.isPresent();
+                        } )
+                        .forEach( call -> events.add( SimpleConditionEvent.violated( method, call.getDescription() ) ) );
+            }
+        };
+    }
+}