qa: fix typehandlerlibrary spotbugs findings (#5154)

* kotlin jps updated itself
* update spotbugs version to consume AsserThrows error fix (spotbugs/spotbugs#2667)
* parametrize logs
* make fields final
* remove unused logger
* suppress PMD.ArrayIsStoredDirectly and PMD.MethodReturnsInternalArray until proper security assessment (#5176)
* respect inner type last rule (https://stackoverflow.com/questions/25158939/what-motivation-is-behind-checkstyle-inner-type-last-rule)
* replace assertEquals with assertArrayEquals for arrays
* suppress warning for unused field in ObjectFieldMapTypeHandlerFactoryTest (Direct member access is not expected. TypeHandler factory dynamically loads type handlers on type handler creation based on member types of input class TypeInfo.)

Co-authored-by: jdrueckert <[email protected]>

build-logic/build.gradle.kts

@@ -47,7 +47,7 @@ dependencies {
     implementation("org.terasology.gestalt:gestalt-module:7.1.0")
 
     // plugins we configure
-    implementation("com.github.spotbugs.snom:spotbugs-gradle-plugin:5.2.0")
+    implementation("com.github.spotbugs.snom:spotbugs-gradle-plugin:5.2.3")
     implementation("org.sonarsource.scanner.gradle:sonarqube-gradle-plugin:3.3")
 
     api(kotlin("test"))

build-logic/src/main/kotlin/terasology-metrics.gradle.kts

@@ -13,9 +13,17 @@ plugins {
     id("org.sonarqube")
 }
 
+// give test dependencies access to compileOnly dependencies to emulate providedCompile
+// only because of spotbugs-annotations in below dependencies.
+configurations.testImplementation.get().extendsFrom(configurations.compileOnly.get())
+
 dependencies {
-    pmd("net.sourceforge.pmd:pmd-core:6.55.0")
-    pmd("net.sourceforge.pmd:pmd-java:6.55.0")
+    // spotbugs annotations to suppress warnings are not included via spotbugs plugin
+    // see bug: https://github.com/spotbugs/spotbugs-gradle-plugin/issues/1018
+    compileOnly("com.github.spotbugs:spotbugs-annotations:4.8.1")
+    pmd("net.sourceforge.pmd:pmd-ant:7.0.0-rc4")
+    pmd("net.sourceforge.pmd:pmd-core:7.0.0-rc4")
+    pmd("net.sourceforge.pmd:pmd-java:7.0.0-rc4")
 
     testRuntimeOnly("ch.qos.logback:logback-classic:1.2.11") {
         because("runtime: to configure logging during tests with logback.xml")

subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/serializers/Serializer.java

@@ -84,7 +84,7 @@ public <T> Optional<T> deserialize(TypeInfo<T> type, InputStream inputStream) {
             D persistedData = reader.read(inputStream);
             return deserializeFromPersisted(persistedData, type);
         } catch (IOException e) {
-            logger.error("Cannot deserialize type [" + type + "]", e);
+            logger.error("Cannot deserialize type [{}]", type, e);
         }
         return Optional.empty();
     }
@@ -102,7 +102,7 @@ public <T> Optional<T> deserialize(TypeInfo<T> type, byte[] bytes) {
             D persistedData = reader.read(bytes);
             return deserializeFromPersisted(persistedData, type);
         } catch (IOException e) {
-            logger.error("Cannot deserialize type [" + type + "]", e);
+            logger.error("Cannot deserialize type [{}]", type, e);
         }
         return Optional.empty();
     }
@@ -140,7 +140,7 @@ public <T> void serialize(T object, TypeInfo<T> type, OutputStream outputStream)
             try {
                 writer.writeTo(persistedData.get(), outputStream);
             } catch (IOException e) {
-                logger.error("Cannot serialize [" + type + "]", e);
+                logger.error("Cannot serialize [{}]", type, e);
             }
         } else {
             logger.error("Cannot serialize [{}]", type);

subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/Serializer.java

@@ -18,8 +18,8 @@ public class Serializer {
 
     private static final Logger logger = LoggerFactory.getLogger(Serializer.class);
 
-    private ClassMetadata<?, ?> classMetadata;
-    private Map<FieldMetadata<?, ?>, TypeHandler> fieldHandlers;
+    private final ClassMetadata<?, ?> classMetadata;
+    private final Map<FieldMetadata<?, ?>, TypeHandler> fieldHandlers;
 
     public Serializer(ClassMetadata<?, ?> classMetadata, Map<FieldMetadata<?, ?>, TypeHandler> fieldHandlers) {
         this.fieldHandlers = fieldHandlers;

subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/coreTypes/EnumTypeHandler.java

@@ -16,7 +16,7 @@
 public class EnumTypeHandler<T extends Enum> extends TypeHandler<T> {
 
     private static final Logger logger = LoggerFactory.getLogger(EnumTypeHandler.class);
-    private Class<T> enumType;
+    private final Class<T> enumType;
     private Map<String, T> caseInsensitiveLookup = Maps.newHashMap();
 
     public EnumTypeHandler(Class<T> enumType) {

subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/coreTypes/factories/ObjectFieldMapTypeHandlerFactory.java

@@ -3,8 +3,6 @@
 package org.terasology.persistence.typeHandling.coreTypes.factories;
 
 import com.google.common.collect.Maps;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
 import org.terasology.persistence.typeHandling.TypeHandler;
 import org.terasology.persistence.typeHandling.TypeHandlerContext;
 import org.terasology.persistence.typeHandling.TypeHandlerFactory;
@@ -23,7 +21,6 @@
 import java.util.Optional;
 
 public class ObjectFieldMapTypeHandlerFactory implements TypeHandlerFactory {
-    private static final Logger LOGGER = LoggerFactory.getLogger(ObjectFieldMapTypeHandlerFactory.class);
 
     private ConstructorLibrary constructorLibrary;
 

subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/inMemory/PersistedBytes.java

@@ -5,6 +5,8 @@
 
 import java.nio.ByteBuffer;
 
+// TODO, see https://github.com/MovingBlocks/Terasology/issues/5176 for reasoning.
+@SuppressWarnings({"PMD.ArrayIsStoredDirectly", "PMD.MethodReturnsInternalArray"})
 public class PersistedBytes extends AbstractPersistedData {
 
     private final byte[] bytes;

subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/inMemory/arrays/PersistedBooleanArray.java

@@ -9,6 +9,8 @@
 import java.util.Arrays;
 import java.util.Iterator;
 
+// TODO, see https://github.com/MovingBlocks/Terasology/issues/5176 for reasoning.
+@SuppressWarnings({"PMD.ArrayIsStoredDirectly", "PMD.MethodReturnsInternalArray"})
 public class PersistedBooleanArray extends AbstractPersistedArray {
 
     private final boolean[] booleans;

subsystems/TypeHandlerLibrary/src/test/java/org/terasology/persistence/typeHandling/FutureTypeHandlerTest.java

@@ -27,30 +27,6 @@ public class FutureTypeHandlerTest {
     private final TypeHandlerLibrary typeHandlerLibrary =
             Mockito.spy(new TypeHandlerLibrary(reflections));
 
-    private static final class RecursiveType<T> {
-        final T data;
-        final List<RecursiveType<T>> children;
-
-        @SafeVarargs
-        private RecursiveType(T data, RecursiveType<T>... children) {
-            this.data = data;
-            this.children = Lists.newArrayList(children);
-        }
-    }
-
-    private class ResultCaptor<T> implements Answer {
-        private T result = null;
-        public T getResult() {
-            return result;
-        }
-
-        @Override
-        public T answer(InvocationOnMock invocationOnMock) throws Throwable {
-            result = (T) invocationOnMock.callRealMethod();
-            return result;
-        }
-    }
-
     @Test
     public void testRecursiveType() {
         ResultCaptor<Optional<TypeHandler<RecursiveType<Integer>>>> resultCaptor = new ResultCaptor<>();
@@ -77,4 +53,32 @@ public void testRecursiveType() {
 
         assertEquals(typeHandler, future.typeHandler);
     }
+
+    private static final class RecursiveType<T> {
+        final T data;
+        final List<RecursiveType<T>> children;
+
+        @SafeVarargs
+        private RecursiveType(T data, RecursiveType<T>... children) {
+            this.data = data;
+            this.children = Lists.newArrayList(children);
+        }
+    }
+
+    @edu.umd.cs.findbugs.annotations.SuppressFBWarnings(
+            value = "SIC_INNER_SHOULD_BE_STATIC",
+            justification = "Test code is not performance-relevant, flagged inner ResultCaptor class is a mock with dynamic behavior" +
+                    " and cannot be static.")
+    private class ResultCaptor<T> implements Answer {
+        private T result = null;
+        public T getResult() {
+            return result;
+        }
+
+        @Override
+        public T answer(InvocationOnMock invocationOnMock) throws Throwable {
+            result = (T) invocationOnMock.callRealMethod();
+            return result;
+        }
+    }
 }

subsystems/TypeHandlerLibrary/src/test/java/org/terasology/persistence/typeHandling/InMemorySerializerTest.java

@@ -37,9 +37,8 @@
 import java.util.function.Function;
 import java.util.stream.Stream;
 
-
 class InMemorySerializerTest {
-    private InMemoryPersistedDataSerializer serializer = new InMemoryPersistedDataSerializer();
+    private final InMemoryPersistedDataSerializer serializer = new InMemoryPersistedDataSerializer();
 
     public static Stream<Arguments> types() {
         return Stream.of(
@@ -123,30 +122,6 @@ void serializeStrings() {
         Assertions.assertThrows(ClassCastException.class, data::getAsDouble);
     }
 
-    //TODO remove it
-    public void template(PersistedData data) {
-        Assertions.assertFalse(data.isString());
-        Assertions.assertFalse(data.isArray());
-        Assertions.assertFalse(data.isNull());
-        Assertions.assertFalse(data.isNumber());
-        Assertions.assertFalse(data.isBoolean());
-        Assertions.assertFalse(data.isBytes());
-        Assertions.assertFalse(data.isValueMap());
-
-        Assertions.assertThrows(IllegalStateException.class, data::getAsValueMap);
-        Assertions.assertThrows(IllegalStateException.class, data::getAsArray);
-
-        Assertions.assertThrows(DeserializationException.class, data::getAsByteBuffer);
-        Assertions.assertThrows(DeserializationException.class, data::getAsBytes);
-
-        Assertions.assertThrows(ClassCastException.class, data::getAsString);
-        Assertions.assertThrows(ClassCastException.class, data::getAsBoolean);
-        Assertions.assertThrows(ClassCastException.class, data::getAsInteger);
-        Assertions.assertThrows(ClassCastException.class, data::getAsLong);
-        Assertions.assertThrows(ClassCastException.class, data::getAsFloat);
-        Assertions.assertThrows(ClassCastException.class, data::getAsDouble);
-    }
-
     @Test
     void serializeOneAsStrings() {
         PersistedData data = serializer.serialize(new String[]{"foo"});
@@ -306,7 +281,7 @@ void serializeBytes() {
 
         Assertions.assertEquals(PersistedBytes.class, data.getClass());
         Assertions.assertTrue(data.isBytes());
-        Assertions.assertEquals(value, data.getAsBytes());
+        Assertions.assertArrayEquals(value, data.getAsBytes());
         Assertions.assertEquals(ByteBuffer.wrap(value), data.getAsByteBuffer());
 
         Assertions.assertFalse(data.isString());
@@ -335,7 +310,7 @@ void serializeByteBuffer() {
         Assertions.assertEquals(PersistedBytes.class, data.getClass());
         Assertions.assertTrue(data.isBytes());
 
-        Assertions.assertEquals(value, data.getAsBytes());
+        Assertions.assertArrayEquals(value, data.getAsBytes());
         Assertions.assertEquals(ByteBuffer.wrap(value), data.getAsByteBuffer());
 
         Assertions.assertFalse(data.isString());
@@ -469,7 +444,7 @@ private void checkValueArray(PersistedData data, PersistedData entry, Set<TypeGe
         Assertions.assertEquals(PersistedValueArray.class, data.getClass());
 
         Assertions.assertEquals(entry, data.getAsArray().getArrayItem(0));
-        typeGetters.stream()
+        typeGetters
                 .forEach(typeGetter ->
                         Assertions.assertEquals(typeGetter.getGetter().apply(entry),
                                 typeGetter.getGetter().apply(data))

subsystems/TypeHandlerLibrary/src/test/java/org/terasology/persistence/typeHandling/TypeHandlerLibraryTest.java

@@ -22,19 +22,15 @@
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
 class TypeHandlerLibraryTest {
-    private static Reflections reflections;
     private static TypeHandlerLibrary typeHandlerLibrary;
 
     @BeforeAll
     public static void setup() {
-        reflections = new Reflections(TypeHandlerLibraryTest.class.getClassLoader());
+        Reflections reflections = new Reflections(TypeHandlerLibraryTest.class.getClassLoader());
         typeHandlerLibrary = new TypeHandlerLibrary(reflections);
         TypeHandlerLibrary.populateBuiltInHandlers(typeHandlerLibrary);
     }
 
-    @MappedContainer
-    private static class AMappedContainer { }
-
     @Test
     public void testMappedContainerHandler() {
         TypeHandler<AMappedContainer> handler = typeHandlerLibrary.getTypeHandler(AMappedContainer.class).get();
@@ -93,4 +89,7 @@ void testGetBaseTypeHandler() {
     }
 
     private enum AnEnum { }
+
+    @MappedContainer
+    private static class AMappedContainer { }
 }

subsystems/TypeHandlerLibrary/src/test/java/org/terasology/persistence/typeHandling/coreTypes/factories/BytesTypeHandlerTest.java

@@ -39,10 +39,10 @@ void byteArraySerializeDeserialize() {
         byte[] expectedObj = new byte[]{(byte) 0xFF};
 
         PersistedBytes data = serialize(expectedObj, new ByteArrayTypeHandler());
-        Assertions.assertEquals(expectedObj, data.getAsBytes());
+        Assertions.assertArrayEquals(expectedObj, data.getAsBytes());
 
         byte[] obj = deserialize(data, new ByteArrayTypeHandler());
-        Assertions.assertEquals(expectedObj, obj);
+        Assertions.assertArrayEquals(expectedObj, obj);
     }
 
     private <R extends PersistedData, T> R serialize(T obj, TypeHandler<T> typeHandler) {

subsystems/TypeHandlerLibrary/src/test/java/org/terasology/persistence/typeHandling/coreTypes/factories/ObjectFieldMapTypeHandlerFactoryTest.java

@@ -112,4 +112,14 @@ private static class MultiTypeClass<T, U> {
         private T t;
         private U u;
     }
+
+    @edu.umd.cs.findbugs.annotations.SuppressFBWarnings(
+            value = "UUF_UNUSED_FIELD",
+            justification = "Direct member access is not expected. TypeHandler factory dynamically loads type handlers on type handler" +
+                    " creation based on member types of input class TypeInfo. ")
+    @SuppressWarnings("PMD.UnusedPrivateField")
+    private static class SomeClass<T> {
+        private T t;
+        private List<T> list;
+    }
 }