qa: fix typehandlerlibrary spotbugs findings

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