typehandlerlibrary qa, jdrueckert feedback

especially take out pmd guardlog suppresses. fix part of them
in a catch even if not necessary from a performance point of
view. and create a pull request towards teraconfig to not
use the guardlog rule any more:
MovingBlocks/TeraConfig#23

Co-authored-by: jdrueckert <jd.rueckert@googlemail.com>

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

@@ -21,8 +21,9 @@ 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.1")
-    pmd("net.sourceforge.pmd:pmd-core:6.55.0")
-    pmd("net.sourceforge.pmd:pmd-java:6.55.0")
+    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

@@ -22,9 +22,6 @@
  * 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);
@@ -87,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();
     }
@@ -105,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();
     }
@@ -143,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

@@ -14,7 +14,6 @@
  * 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);

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

@@ -310,9 +310,6 @@ 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

@@ -31,9 +31,6 @@ 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,7 +60,6 @@ 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,9 +80,6 @@ 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,6 +24,7 @@
 public class CollectionTypeHandlerFactory implements TypeHandlerFactory {
     private static final Logger LOGGER = LoggerFactory.getLogger(CollectionTypeHandlerFactory.class);
 
+    // PMD and intellij cannot judge if this is good or not. suppress.
     @SuppressWarnings("PMD.UnusedLocalVariable") private ConstructorLibrary constructorLibrary;
 
     public CollectionTypeHandlerFactory(ConstructorLibrary constructorLibrary) {

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

@@ -5,6 +5,9 @@
 
 import java.nio.ByteBuffer;
 
+// TODO: critical assessment if this should be suppressed or remediated (jdrueckert)
+// giving back a pointer to storage permits called to change the value without security control. difficult if the caller is untrusted.
+// in case of game, good for performance: https://www.appsloveworld.com/java/100/140/reasoning-behind-arrayisstoreddirectly-rule-of-pmd.
 @SuppressWarnings({"PMD.ArrayIsStoredDirectly", "PMD.MethodReturnsInternalArray"})
 public class PersistedBytes extends AbstractPersistedData {
 

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

@@ -9,6 +9,9 @@
 import java.util.Arrays;
 import java.util.Iterator;
 
+// TODO: critical assessment if this should be suppressed or remediated (jdrueckert)
+// giving back a pointer to storage permits called to change the value without security control. difficult if the caller is untrusted. in
+// case of game, good for performance: https://www.appsloveworld.com/java/100/140/reasoning-behind-arrayisstoreddirectly-rule-of-pmd.
 @SuppressWarnings({"PMD.ArrayIsStoredDirectly", "PMD.MethodReturnsInternalArray"})
 public class PersistedBooleanArray extends AbstractPersistedArray {
 

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

@@ -65,7 +65,10 @@ private RecursiveType(T data, RecursiveType<T>... children) {
         }
     }
 
-    @edu.umd.cs.findbugs.annotations.SuppressFBWarnings
+    @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() {

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

@@ -37,10 +37,6 @@
 import java.util.function.Function;
 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 final InMemoryPersistedDataSerializer serializer = new InMemoryPersistedDataSerializer();
 

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

@@ -25,10 +25,6 @@
 import static org.mockito.Mockito.times;
 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);
 
@@ -117,6 +113,11 @@ private static class MultiTypeClass<T, U> {
         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;