From 511dbe2d889c2623ef522978902f6e4c51897d20 Mon Sep 17 00:00:00 2001 From: soloturn Date: Wed, 1 Nov 2023 01:42:19 +0100 Subject: [PATCH] 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: https://github.com/MovingBlocks/TeraConfig/pull/23 Co-authored-by: jdrueckert --- .idea/misc.xml | 3 +-- .../src/main/kotlin/terasology-metrics.gradle.kts | 5 +++-- .../terasology/persistence/serializers/Serializer.java | 9 +++------ .../terasology/persistence/typeHandling/Serializer.java | 1 - .../persistence/typeHandling/TypeHandlerLibrary.java | 3 --- .../typeHandling/coreTypes/EnumTypeHandler.java | 3 --- .../typeHandling/coreTypes/GenericMapTypeHandler.java | 1 - .../coreTypes/ObjectFieldMapTypeHandler.java | 3 --- .../factories/CollectionTypeHandlerFactory.java | 1 + .../typeHandling/inMemory/PersistedBytes.java | 2 ++ .../inMemory/arrays/PersistedBooleanArray.java | 2 ++ .../persistence/typeHandling/FutureTypeHandlerTest.java | 5 ++++- .../persistence/typeHandling/InMemorySerializerTest.java | 4 ---- .../factories/ObjectFieldMapTypeHandlerFactoryTest.java | 9 +++++---- 14 files changed, 21 insertions(+), 30 deletions(-) diff --git a/.idea/misc.xml b/.idea/misc.xml index 12fcfb1f537..5fe35f6097d 100644 --- a/.idea/misc.xml +++ b/.idea/misc.xml @@ -1,4 +1,3 @@ - @@ -15,5 +14,5 @@ - + \ No newline at end of file diff --git a/build-logic/src/main/kotlin/terasology-metrics.gradle.kts b/build-logic/src/main/kotlin/terasology-metrics.gradle.kts index f97157cfc39..259a7adcc22 100644 --- a/build-logic/src/main/kotlin/terasology-metrics.gradle.kts +++ b/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") diff --git a/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/serializers/Serializer.java b/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/serializers/Serializer.java index f3aea703012..c9ffdd342a9 100644 --- a/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/serializers/Serializer.java +++ b/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 { private static final Logger logger = LoggerFactory.getLogger(Serializer.class); @@ -87,7 +84,7 @@ public Optional deserialize(TypeInfo 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 Optional deserialize(TypeInfo 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 void serialize(T object, TypeInfo 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); diff --git a/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/Serializer.java b/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/Serializer.java index 77982e35059..1361e671f7f 100644 --- a/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/Serializer.java +++ b/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); diff --git a/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/TypeHandlerLibrary.java b/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/TypeHandlerLibrary.java index 7738930899f..3395590985b 100644 --- a/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/TypeHandlerLibrary.java +++ b/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/TypeHandlerLibrary.java @@ -310,9 +310,6 @@ public TypeHandler getBaseTypeHandler(TypeInfo 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, TypeHandler> getFieldHandlerMap(ClassMetadata type) { Map, TypeHandler> handlerMap = Maps.newHashMap(); for (FieldMetadata field : type.getFields()) { diff --git a/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/coreTypes/EnumTypeHandler.java b/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/coreTypes/EnumTypeHandler.java index ff6f69dca55..193a5d3393d 100644 --- a/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/coreTypes/EnumTypeHandler.java +++ b/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 deserialize(PersistedData data) { if (data.isString()) { diff --git a/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/coreTypes/GenericMapTypeHandler.java b/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/coreTypes/GenericMapTypeHandler.java index 747d10016d1..ebec6d4dd64 100644 --- a/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/coreTypes/GenericMapTypeHandler.java +++ b/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/coreTypes/GenericMapTypeHandler.java @@ -60,7 +60,6 @@ private PersistedData serializeEntry(Map.Entry entry, PersistedDataSeriali return serializer.serialize(result); } - @SuppressWarnings("PMD.GuardLogStatementJavaUtil") @Override public Optional> deserialize(PersistedData data) { diff --git a/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/coreTypes/ObjectFieldMapTypeHandler.java b/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/coreTypes/ObjectFieldMapTypeHandler.java index fc82ede9ea6..519ed719f0d 100644 --- a/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/coreTypes/ObjectFieldMapTypeHandler.java +++ b/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 deserialize(PersistedData data) { if (!data.isValueMap()) { diff --git a/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/coreTypes/factories/CollectionTypeHandlerFactory.java b/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/coreTypes/factories/CollectionTypeHandlerFactory.java index 08444ad7353..49c7349b985 100644 --- a/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/coreTypes/factories/CollectionTypeHandlerFactory.java +++ b/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) { diff --git a/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/inMemory/PersistedBytes.java b/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/inMemory/PersistedBytes.java index fbd0bb79098..a573848215f 100644 --- a/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/inMemory/PersistedBytes.java +++ b/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/inMemory/PersistedBytes.java @@ -5,6 +5,8 @@ import java.nio.ByteBuffer; +// 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 { diff --git a/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/inMemory/arrays/PersistedBooleanArray.java b/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/inMemory/arrays/PersistedBooleanArray.java index f1090b694ed..981dccfeb39 100644 --- a/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/inMemory/arrays/PersistedBooleanArray.java +++ b/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; +// 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 { diff --git a/subsystems/TypeHandlerLibrary/src/test/java/org/terasology/persistence/typeHandling/FutureTypeHandlerTest.java b/subsystems/TypeHandlerLibrary/src/test/java/org/terasology/persistence/typeHandling/FutureTypeHandlerTest.java index 6f215679c10..5e89fcd7ec0 100644 --- a/subsystems/TypeHandlerLibrary/src/test/java/org/terasology/persistence/typeHandling/FutureTypeHandlerTest.java +++ b/subsystems/TypeHandlerLibrary/src/test/java/org/terasology/persistence/typeHandling/FutureTypeHandlerTest.java @@ -65,7 +65,10 @@ private RecursiveType(T data, RecursiveType... 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 implements Answer { private T result = null; public T getResult() { diff --git a/subsystems/TypeHandlerLibrary/src/test/java/org/terasology/persistence/typeHandling/InMemorySerializerTest.java b/subsystems/TypeHandlerLibrary/src/test/java/org/terasology/persistence/typeHandling/InMemorySerializerTest.java index 51756bb403b..8da2f82f4b1 100644 --- a/subsystems/TypeHandlerLibrary/src/test/java/org/terasology/persistence/typeHandling/InMemorySerializerTest.java +++ b/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(); diff --git a/subsystems/TypeHandlerLibrary/src/test/java/org/terasology/persistence/typeHandling/coreTypes/factories/ObjectFieldMapTypeHandlerFactoryTest.java b/subsystems/TypeHandlerLibrary/src/test/java/org/terasology/persistence/typeHandling/coreTypes/factories/ObjectFieldMapTypeHandlerFactoryTest.java index 236e6114b69..fae71214903 100644 --- a/subsystems/TypeHandlerLibrary/src/test/java/org/terasology/persistence/typeHandling/coreTypes/factories/ObjectFieldMapTypeHandlerFactoryTest.java +++ b/subsystems/TypeHandlerLibrary/src/test/java/org/terasology/persistence/typeHandling/coreTypes/factories/ObjectFieldMapTypeHandlerFactoryTest.java @@ -20,10 +20,6 @@ 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); @@ -48,6 +44,11 @@ public void testObject() { verify(typeHandlerLibrary).getTypeHandler(eq(new TypeInfo>() { }.getType())); } + @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 { private T t; private List list;