From 75f84945fc9912c3df9e4e1033f4a409d9910910 Mon Sep 17 00:00:00 2001
From: ljacqu <ljacqu@users.noreply.github.com>
Date: Sun, 26 Mar 2017 13:20:40 +0200
Subject: [PATCH] Misc code householding - Checkstyle config: allow todo
 comments with issue number - Create consistency tests across all classes,
 ensuring: unique class names, users of expiring collectors implement
 HasCleanup, non-private fields are only constants - Fix tag replacement in
 PlayerListener for {DISPLAYNAME}

---
 .checkstyle.xml                               |   4 +-
 .../authme/debug/HasPermissionChecker.java    |   3 +-
 .../xephi/authme/listener/PlayerListener.java |   2 +-
 .../service/PasswordRecoveryService.java      |   9 +-
 .../authme/util/expiring/ExpiringMap.java     |   9 +-
 .../authme/util/expiring/TimedCounter.java    |   6 +-
 .../xephi/authme/ClassesConsistencyTest.java  | 182 ++++++++++++++++++
 .../authme/util/expiring/ExpiringMapTest.java |   2 +-
 .../util/expiring/TimedCounterTest.java       |   4 +-
 9 files changed, 210 insertions(+), 11 deletions(-)
 create mode 100644 src/test/java/fr/xephi/authme/ClassesConsistencyTest.java

diff --git a/.checkstyle.xml b/.checkstyle.xml
index 017242e36..9a91018c8 100644
--- a/.checkstyle.xml
+++ b/.checkstyle.xml
@@ -28,7 +28,8 @@
             <property name="ignorePattern" value="^package.*|^import.*|a href|href|http://|https://|ftp://"/>
         </module>
         <module name="TodoComment">
-            <property name="format" value="TODO|FIXME"/>
+            <!-- Allow comments which have an issue number as they are accounted for in the issue tracker -->
+            <property name="format" value="TODO(?! #\d+:)|FIXME"/>
         </module>
         <module name="GenericWhitespace"/>
         <module name="AvoidStarImport"/>
@@ -156,6 +157,7 @@
             <property name="scope" value="private"/>
             <property name="allowMissingThrowsTags" value="true"/>
             <property name="minLineCount" value="16"/>
+            <property name="allowedAnnotations" value="Override, Test, SectionComments, EventHandler"/>
             <property name="tokens" value="METHOD_DEF, ANNOTATION_FIELD_DEF"/> <!-- exclude CTOR_DEF -->
         </module>
         <!-- TODO Checkstyle/#4089: need "allowedAnnotations" property to skip @Comment fields
diff --git a/src/main/java/fr/xephi/authme/command/executable/authme/debug/HasPermissionChecker.java b/src/main/java/fr/xephi/authme/command/executable/authme/debug/HasPermissionChecker.java
index 0df27bccb..436ef180c 100644
--- a/src/main/java/fr/xephi/authme/command/executable/authme/debug/HasPermissionChecker.java
+++ b/src/main/java/fr/xephi/authme/command/executable/authme/debug/HasPermissionChecker.java
@@ -1,5 +1,6 @@
 package fr.xephi.authme.command.executable.authme.debug;
 
+import com.google.common.collect.ImmutableList;
 import fr.xephi.authme.permission.AdminPermission;
 import fr.xephi.authme.permission.DefaultPermission;
 import fr.xephi.authme.permission.PermissionNode;
@@ -25,7 +26,7 @@ import java.util.function.BiFunction;
 class HasPermissionChecker implements DebugSection {
 
     static final List<Class<? extends PermissionNode>> PERMISSION_NODE_CLASSES =
-        Arrays.asList(AdminPermission.class, PlayerPermission.class, PlayerStatePermission.class);
+        ImmutableList.of(AdminPermission.class, PlayerPermission.class, PlayerStatePermission.class);
 
     @Inject
     private PermissionsManager permissionsManager;
diff --git a/src/main/java/fr/xephi/authme/listener/PlayerListener.java b/src/main/java/fr/xephi/authme/listener/PlayerListener.java
index ddd3e430d..8f87d1fad 100644
--- a/src/main/java/fr/xephi/authme/listener/PlayerListener.java
+++ b/src/main/java/fr/xephi/authme/listener/PlayerListener.java
@@ -175,7 +175,7 @@ public class PlayerListener implements Listener {
         String customJoinMessage = settings.getProperty(RegistrationSettings.CUSTOM_JOIN_MESSAGE);
         if (!customJoinMessage.isEmpty()) {
             event.setJoinMessage(customJoinMessage.replace("{PLAYERNAME}", player.getName())
-                .replace("{DISPLAYNAME]", player.getDisplayName()));
+                .replace("{DISPLAYNAME}", player.getDisplayName()));
         }
 
         if (!settings.getProperty(RegistrationSettings.DELAY_JOIN_MESSAGE)) {
diff --git a/src/main/java/fr/xephi/authme/service/PasswordRecoveryService.java b/src/main/java/fr/xephi/authme/service/PasswordRecoveryService.java
index 26f48daa2..f24b45f35 100644
--- a/src/main/java/fr/xephi/authme/service/PasswordRecoveryService.java
+++ b/src/main/java/fr/xephi/authme/service/PasswordRecoveryService.java
@@ -2,6 +2,7 @@ package fr.xephi.authme.service;
 
 import fr.xephi.authme.ConsoleLogger;
 import fr.xephi.authme.datasource.DataSource;
+import fr.xephi.authme.initialization.HasCleanup;
 import fr.xephi.authme.initialization.Reloadable;
 import fr.xephi.authme.mail.EmailService;
 import fr.xephi.authme.message.MessageKey;
@@ -25,7 +26,7 @@ import static fr.xephi.authme.settings.properties.EmailSettings.RECOVERY_PASSWOR
 /**
  * Manager for password recovery.
  */
-public class PasswordRecoveryService implements Reloadable {
+public class PasswordRecoveryService implements Reloadable, HasCleanup {
 
     @Inject
     private CommonService commonService;
@@ -163,4 +164,10 @@ public class PasswordRecoveryService implements Reloadable {
         successfulRecovers.setExpiration(
             commonService.getProperty(SecuritySettings.PASSWORD_CHANGE_TIMEOUT), TimeUnit.MINUTES);
     }
+
+    @Override
+    public void performCleanup() {
+        emailCooldown.removeExpiredEntries();
+        successfulRecovers.removeExpiredEntries();
+    }
 }
diff --git a/src/main/java/fr/xephi/authme/util/expiring/ExpiringMap.java b/src/main/java/fr/xephi/authme/util/expiring/ExpiringMap.java
index e920805c2..3bf19351b 100644
--- a/src/main/java/fr/xephi/authme/util/expiring/ExpiringMap.java
+++ b/src/main/java/fr/xephi/authme/util/expiring/ExpiringMap.java
@@ -24,7 +24,7 @@ import java.util.concurrent.TimeUnit;
  */
 public class ExpiringMap<K, V> {
 
-    protected final Map<K, ExpiringEntry<V>> entries = new ConcurrentHashMap<>();
+    private final Map<K, ExpiringEntry<V>> entries = new ConcurrentHashMap<>();
     private long expirationMillis;
 
     /**
@@ -105,6 +105,13 @@ public class ExpiringMap<K, V> {
         return entries.isEmpty();
     }
 
+    /**
+     * @return the internal map
+     */
+    protected Map<K, ExpiringEntry<V>> getEntries() {
+        return entries;
+    }
+
     /**
      * Class holding a value paired with an expiration timestamp.
      *
diff --git a/src/main/java/fr/xephi/authme/util/expiring/TimedCounter.java b/src/main/java/fr/xephi/authme/util/expiring/TimedCounter.java
index e5ef899d6..d6d8bba1d 100644
--- a/src/main/java/fr/xephi/authme/util/expiring/TimedCounter.java
+++ b/src/main/java/fr/xephi/authme/util/expiring/TimedCounter.java
@@ -44,13 +44,13 @@ public class TimedCounter<K> extends ExpiringMap<K, Integer> {
      * @param key the key to increment the counter for
      */
     public void decrement(K key) {
-        ExpiringEntry<Integer> e = entries.get(key);
+        ExpiringEntry<Integer> e = getEntries().get(key);
 
         if (e != null) {
             if (e.getValue() <= 0) {
                 remove(key);
             } else {
-                entries.put(key, new ExpiringEntry<>(e.getValue() - 1, e.getExpiration()));
+                getEntries().put(key, new ExpiringEntry<>(e.getValue() - 1, e.getExpiration()));
             }
         }
     }
@@ -62,7 +62,7 @@ public class TimedCounter<K> extends ExpiringMap<K, Integer> {
      */
     public int total() {
         long currentTime = System.currentTimeMillis();
-        return entries.values().stream()
+        return getEntries().values().stream()
             .filter(entry -> currentTime <= entry.getExpiration())
             .map(ExpiringEntry::getValue)
             .reduce(0, Integer::sum);
diff --git a/src/test/java/fr/xephi/authme/ClassesConsistencyTest.java b/src/test/java/fr/xephi/authme/ClassesConsistencyTest.java
new file mode 100644
index 000000000..cb570caaa
--- /dev/null
+++ b/src/test/java/fr/xephi/authme/ClassesConsistencyTest.java
@@ -0,0 +1,182 @@
+package fr.xephi.authme;
+
+import ch.jalu.configme.properties.Property;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import fr.xephi.authme.datasource.Columns;
+import fr.xephi.authme.initialization.HasCleanup;
+import fr.xephi.authme.listener.PlayerListener;
+import fr.xephi.authme.process.register.executors.RegistrationMethod;
+import fr.xephi.authme.security.crypts.Whirlpool;
+import fr.xephi.authme.util.expiring.ExpiringMap;
+import fr.xephi.authme.util.expiring.ExpiringSet;
+import fr.xephi.authme.util.expiring.TimedCounter;
+import org.junit.Test;
+
+import java.io.File;
+import java.lang.reflect.Field;
+import java.lang.reflect.Modifier;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+import static org.hamcrest.Matchers.equalTo;
+import static org.junit.Assert.assertThat;
+import static org.junit.Assert.fail;
+
+/**
+ * Contains consistency tests across all AuthMe classes.
+ */
+public class ClassesConsistencyTest {
+
+    /** Contains all production classes. */
+    private static final List<Class<?>> ALL_CLASSES =
+        new ClassCollector(TestHelper.SOURCES_FOLDER, TestHelper.PROJECT_ROOT).collectClasses();
+
+    /** Expiring structure types. */
+    private static final Set<Class<?>> EXPIRING_STRUCTURES = ImmutableSet.of(
+        ExpiringSet.class, ExpiringMap.class, TimedCounter.class);
+
+    /** Immutable types, which are allowed to be used in non-private constants. */
+    private static final Set<Class<?>> IMMUTABLE_TYPES = ImmutableSet.of(
+        /* JDK */
+        int.class, long.class, float.class, String.class, File.class, Enum.class, collectionsUnmodifiableList(),
+        /* AuthMe */
+        Property.class, RegistrationMethod.class,
+        /* Guava */
+        ImmutableMap.class, ImmutableList.class);
+
+    /** Classes excluded from the field visibility test. */
+    private static final Set<Class<?>> CLASSES_EXCLUDED_FROM_VISIBILITY_TEST = ImmutableSet.of(
+        Whirlpool.class, // not our implementation, so we don't touch it
+        Columns.class, // uses non-final String constants, which is safe
+        PlayerListener.class // TODO #1145: Remove public field in favor of dedicated methods
+    );
+
+    /**
+     * Checks that there aren't two classes with the same name; this is confusing and should be avoided.
+     */
+    @Test
+    public void shouldNotHaveSameName() {
+        // given
+        Set<String> names = new HashSet<>();
+
+        // when / then
+        for (Class<?> clazz : ALL_CLASSES) {
+            if (!names.add(clazz.getSimpleName())) {
+                fail("Class with name '" + clazz.getSimpleName() + "' already encountered!");
+            }
+        }
+    }
+
+    /**
+     * Checks that fields of classes are either private or static final fields of an immutable type.
+     */
+    @Test
+    public void shouldHaveNonPrivateConstantsOnly() {
+        // given / when
+        Set<String> invalidFields = ALL_CLASSES.stream()
+            .filter(clz -> !CLASSES_EXCLUDED_FROM_VISIBILITY_TEST.contains(clz))
+            .map(Class::getDeclaredFields)
+            .flatMap(Arrays::stream)
+            .filter(f -> hasIllegalFieldVisibility(f))
+            .map(f -> formatField(f))
+            .collect(Collectors.toSet());
+
+        // then
+        if (!invalidFields.isEmpty()) {
+            fail("Found " + invalidFields.size() + " fields with non-private, mutable fields:\n- "
+                + String.join("\n- ", invalidFields));
+        }
+    }
+
+    private static boolean hasIllegalFieldVisibility(Field field) {
+        final int modifiers = field.getModifiers();
+        if (Modifier.isPrivate(modifiers)) {
+            return false;
+        } else if (!Modifier.isStatic(modifiers) || !Modifier.isFinal(modifiers)) {
+            return true;
+        }
+
+        // Field is non-private, static and final
+        Class<?> valueType;
+        if (Collection.class.isAssignableFrom(field.getType()) || Map.class.isAssignableFrom(field.getType())) {
+            // For collections/maps, need to check the actual type to ensure it's an unmodifiable implementation
+            Object value = ReflectionTestUtils.getFieldValue(field, null);
+            valueType = value.getClass();
+        } else {
+             valueType = field.getType();
+        }
+
+        // Field is static, final, and not private -> check that it is immutable type
+        return IMMUTABLE_TYPES.stream()
+            .noneMatch(immutableType -> immutableType.isAssignableFrom(valueType));
+    }
+
+    /**
+     * Prints out the field with (most of) its modifiers.
+     *
+     * @param field the field to format
+     * @return description of the field
+     */
+    private static String formatField(Field field) {
+        String modifiersText = "";
+        int modifiers = field.getModifiers();
+        if (Modifier.isPublic(modifiers)) {
+            modifiersText += "public ";
+        } else if (Modifier.isProtected(modifiers)) {
+            modifiersText += "protected ";
+        } else if (Modifier.isPrivate(modifiers)) {
+            modifiersText += "private ";
+        }
+
+        if (Modifier.isStatic(modifiers)) {
+            modifiersText += "static ";
+        }
+        if (Modifier.isFinal(modifiers)) {
+            modifiersText += "final ";
+        }
+
+        return String.format("[%s] %s %s %s", field.getDeclaringClass().getSimpleName(), modifiersText.trim(),
+            field.getType().getSimpleName(), field.getName());
+    }
+
+    /**
+     * Checks that classes with expiring collections (such as {@link ExpiringMap}) implement the {@link HasCleanup}
+     * interface to regularly clean up expired data.
+     */
+    @Test
+    public void shouldImplementHasCleanup() {
+        // given / when / then
+        for (Class<?> clazz : ALL_CLASSES) {
+            if (hasExpiringCollectionAsField(clazz)) {
+                assertThat("Class '" + clazz.getSimpleName() + "' has expiring collections, should implement HasCleanup",
+                    HasCleanup.class.isAssignableFrom(clazz), equalTo(true));
+                // System.out.println("Successful check for " + clazz);
+            }
+        }
+    }
+
+    private static boolean hasExpiringCollectionAsField(Class<?> clazz) {
+        for (Field field : clazz.getDeclaredFields()) {
+            if (EXPIRING_STRUCTURES.stream().anyMatch(t -> t.isAssignableFrom(field.getType()))) {
+                return true;
+            }
+        }
+        return false;
+    }
+
+    /**
+     * @return the concrete class of the unmodifiable list as returned by {@link Collections#unmodifiableList(List)}.
+     */
+    private static Class<?> collectionsUnmodifiableList() {
+        return Collections.unmodifiableList(new ArrayList<>()).getClass();
+    }
+}
diff --git a/src/test/java/fr/xephi/authme/util/expiring/ExpiringMapTest.java b/src/test/java/fr/xephi/authme/util/expiring/ExpiringMapTest.java
index cdace3065..ac9a5a719 100644
--- a/src/test/java/fr/xephi/authme/util/expiring/ExpiringMapTest.java
+++ b/src/test/java/fr/xephi/authme/util/expiring/ExpiringMapTest.java
@@ -76,7 +76,7 @@ public class ExpiringMapTest {
         map.removeExpiredEntries();
 
         // then
-        Map<Integer, ?> internalMap = map.entries;
+        Map<Integer, ?> internalMap = map.getEntries();
         assertThat(internalMap.keySet(), containsInAnyOrder(64, 25));
     }
 
diff --git a/src/test/java/fr/xephi/authme/util/expiring/TimedCounterTest.java b/src/test/java/fr/xephi/authme/util/expiring/TimedCounterTest.java
index abf012744..c2b65eed3 100644
--- a/src/test/java/fr/xephi/authme/util/expiring/TimedCounterTest.java
+++ b/src/test/java/fr/xephi/authme/util/expiring/TimedCounterTest.java
@@ -56,8 +56,8 @@ public class TimedCounterTest {
     public void shouldSumUpEntries() {
         // given
         TimedCounter<String> counter = new TimedCounter<>(90, TimeUnit.SECONDS);
-        counter.entries.put("expired", new ExpiringMap.ExpiringEntry<>(800, 0));
-        counter.entries.put("expired2", new ExpiringMap.ExpiringEntry<>(24, System.currentTimeMillis() - 100));
+        counter.getEntries().put("expired", new ExpiringMap.ExpiringEntry<>(800, 0));
+        counter.getEntries().put("expired2", new ExpiringMap.ExpiringEntry<>(24, System.currentTimeMillis() - 100));
         counter.put("other", 10);
         counter.put("Another", 4);