typehandlerlibrary qa

as a workaround, add dependency for spotbugs annotations, and create a
[ticket there so the spotbugs gradle plugin would include the dependency]
(spotbugs/spotbugs-gradle-plugin#1018).

see here a motivation for inner type last rule. if we leave the rule we
should respect it, otherwise disable the rule.
https://stackoverflow.com/questions/25158939/what-motivation-is-behind-checkstyle-inner-type-last-rule

especially in tests just quite the warnings, in most cases the code is
deliberate as it is.

see #3859 and see bugs:
* pmd/pmd#4731
* spotbugs/spotbugs-gradle-plugin#1018
* spotbugs/spotbugs#2667

build-logic/build.gradle.kts

@@ -46,7 +46,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.1")
     implementation("org.sonarsource.scanner.gradle:sonarqube-gradle-plugin:3.3")
 
     api(kotlin("test"))

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

@@ -14,6 +14,9 @@ plugins {
 }
 
 dependencies {
+    // 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.0")
     pmd("net.sourceforge.pmd:pmd-core:6.55.0")
     pmd("net.sourceforge.pmd:pmd-java:6.55.0")
 

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

@@ -22,6 +22,9 @@
  * methods that use {@link #serializeToPersisted(Object, TypeInfo)} and {@link #deserializeFromPersisted(PersistedData,
  * TypeInfo)}.
  */
+// log statements after an if are marked as false positive, suppress.
+// see pmd bug: https://github.com/pmd/pmd/issues/4731
+@SuppressWarnings("PMD.GuardLogStatementJavaUtil")
 public final class Serializer<D extends PersistedData> {
 
     private static final Logger logger = LoggerFactory.getLogger(Serializer.class);

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

@@ -14,12 +14,13 @@
  * A serializer provides low-level serialization support for a type, using a mapping of type handlers for each field of that type.
  *
  */
+@SuppressWarnings("PMD.GuardLogStatementJavaUtil")
 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/TypeHandlerLibrary.java

@@ -310,6 +310,9 @@ public <T> TypeHandler<T> getBaseTypeHandler(TypeInfo<T> typeInfo) {
         return new RuntimeDelegatingTypeHandler<>(delegateHandler, typeInfo, context);
     }
 
+    // log after else is false positive, suppress.
+    // see bug: https://github.com/pmd/pmd/issues/4731
+    @SuppressWarnings("PMD.GuardLogStatementJavaUtil")
     private Map<FieldMetadata<?, ?>, TypeHandler> getFieldHandlerMap(ClassMetadata<?, ?> type) {
         Map<FieldMetadata<?, ?>, TypeHandler> handlerMap = Maps.newHashMap();
         for (FieldMetadata<?, ?> field : type.getFields()) {

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) {
@@ -31,6 +31,9 @@ public PersistedData serializeNonNull(T value, PersistedDataSerializer serialize
         return serializer.serialize(value.toString());
     }
 
+    // log after else is false positive, suppress.
+    // see bug: https://github.com/pmd/pmd/issues/4731
+    @SuppressWarnings("PMD.GuardLogStatementJavaUtil")
     @Override
     public Optional<T> deserialize(PersistedData data) {
         if (data.isString()) {

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

@@ -60,6 +60,7 @@ private PersistedData serializeEntry(Map.Entry<K, V> entry, PersistedDataSeriali
         return serializer.serialize(result);
     }
 
+    @SuppressWarnings("PMD.GuardLogStatementJavaUtil")
     @Override
     public Optional<Map<K, V>> deserialize(PersistedData data) {
 

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

@@ -80,6 +80,9 @@ private String getFieldName(Field field) {
         return serializedName.value();
     }
 
+    // log after else is false positive, suppress.
+    // see bug: https://github.com/pmd/pmd/issues/4731
+    @SuppressWarnings("PMD.GuardLogStatementJavaUtil")
     @Override
     public Optional<T> deserialize(PersistedData data) {
         if (!data.isValueMap()) {

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

@@ -24,7 +24,7 @@
 public class CollectionTypeHandlerFactory implements TypeHandlerFactory {
     private static final Logger LOGGER = LoggerFactory.getLogger(CollectionTypeHandlerFactory.class);
 
-    private ConstructorLibrary constructorLibrary;
+    @SuppressWarnings("PMD.UnusedLocalVariable") private ConstructorLibrary constructorLibrary;
 
     public CollectionTypeHandlerFactory(ConstructorLibrary constructorLibrary) {
         this.constructorLibrary = constructorLibrary;

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,7 @@
 
 import java.nio.ByteBuffer;
 
+@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,7 @@
 import java.util.Arrays;
 import java.util.Iterator;
 
+@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,29 @@ 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
+    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

@@ -38,8 +38,11 @@
 import java.util.stream.Stream;
 
 
+// spotbugs does thinks Assertions.assertThrows does not throw the exception, even if
+// test is successful. see bug: https://github.com/spotbugs/spotbugs/issues/2667.
+@edu.umd.cs.findbugs.annotations.SuppressFBWarnings
 class InMemorySerializerTest {
-    private InMemoryPersistedDataSerializer serializer = new InMemoryPersistedDataSerializer();
+    private final InMemoryPersistedDataSerializer serializer = new InMemoryPersistedDataSerializer();
 
     public static Stream<Arguments> types() {
         return Stream.of(
@@ -123,30 +126,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 +285,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 +314,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 +448,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

@@ -20,6 +20,10 @@
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
 
+// both, pmd and spotbugs complain about not used private fields, suppress in
+// the static test class, but fields are checked. suppress.
+@edu.umd.cs.findbugs.annotations.SuppressFBWarnings
+@SuppressWarnings("PMD.UnusedPrivateField")
 public class ObjectFieldMapTypeHandlerFactoryTest {
     private final TypeHandlerLibrary typeHandlerLibrary = mock(TypeHandlerLibrary.class);
 
@@ -30,11 +34,6 @@ public class ObjectFieldMapTypeHandlerFactoryTest {
     private final TypeHandlerContext context =
             new TypeHandlerContext(typeHandlerLibrary, mock(SerializationSandbox.class));
 
-    private static class SomeClass<T> {
-        private T t;
-        private List<T> list;
-    }
-
     @Test
     public void testObject() {
         Optional<TypeHandler<SomeClass<Integer>>> typeHandler =
@@ -48,4 +47,9 @@ public void testObject() {
 
         verify(typeHandlerLibrary).getTypeHandler(eq(new TypeInfo<List<Integer>>() { }.getType()));
     }
+
+    private static class SomeClass<T> {
+        private T t;
+        private List<T> list;
+    }
 }