From 8b4b6b92b9a134e382b7cccd77d4ccda8c882f7e Mon Sep 17 00:00:00 2001 From: soloturn Date: Sat, 18 Nov 2023 10:43:58 +0100 Subject: [PATCH] qa: fix typehandlerlibrary spotbugs findings (#5154) * kotlin jps updated itself * update spotbugs version to consume AsserThrows error fix (https://github.com/spotbugs/spotbugs/issues/2667) * parametrize logs * make fields final * remove unused logger * suppress PMD.ArrayIsStoredDirectly and PMD.MethodReturnsInternalArray until proper security assessment (https://github.com/MovingBlocks/Terasology/issues/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 --- .idea/kotlinc.xml | 2 +- .idea/misc.xml | 1 - build-logic/build.gradle.kts | 2 +- .../main/kotlin/terasology-metrics.gradle.kts | 12 ++++- .../persistence/serializers/Serializer.java | 6 +-- .../persistence/typeHandling/Serializer.java | 4 +- .../coreTypes/EnumTypeHandler.java | 2 +- .../ObjectFieldMapTypeHandlerFactory.java | 3 -- .../typeHandling/inMemory/PersistedBytes.java | 2 + .../arrays/PersistedBooleanArray.java | 2 + .../typeHandling/FutureTypeHandlerTest.java | 52 ++++++++++--------- .../typeHandling/InMemorySerializerTest.java | 33 ++---------- .../typeHandling/TypeHandlerLibraryTest.java | 9 ++-- .../factories/BytesTypeHandlerTest.java | 4 +- .../ObjectFieldMapTypeHandlerFactoryTest.java | 10 ++++ 15 files changed, 70 insertions(+), 74 deletions(-) diff --git a/.idea/kotlinc.xml b/.idea/kotlinc.xml index 7e340a776a6..f8467b458e4 100644 --- a/.idea/kotlinc.xml +++ b/.idea/kotlinc.xml @@ -1,6 +1,6 @@ - \ No newline at end of file diff --git a/.idea/misc.xml b/.idea/misc.xml index 0064cb9181f..a823531acb0 100644 --- a/.idea/misc.xml +++ b/.idea/misc.xml @@ -1,4 +1,3 @@ - diff --git a/build-logic/build.gradle.kts b/build-logic/build.gradle.kts index 63d286777ec..3e9f710485e 100644 --- a/build-logic/build.gradle.kts +++ b/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")) diff --git a/build-logic/src/main/kotlin/terasology-metrics.gradle.kts b/build-logic/src/main/kotlin/terasology-metrics.gradle.kts index 5cf9a7e7449..0b3a3b28810 100644 --- a/build-logic/src/main/kotlin/terasology-metrics.gradle.kts +++ b/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") 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 e4a0586cc04..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 @@ -84,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(); } @@ -102,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(); } @@ -140,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 7cd7a19059b..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 @@ -18,8 +18,8 @@ public class Serializer { private static final Logger logger = LoggerFactory.getLogger(Serializer.class); - private ClassMetadata classMetadata; - private Map, TypeHandler> fieldHandlers; + private final ClassMetadata classMetadata; + private final Map, TypeHandler> fieldHandlers; public Serializer(ClassMetadata classMetadata, Map, TypeHandler> fieldHandlers) { this.fieldHandlers = fieldHandlers; 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 cd0bc4ba319..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 @@ -16,7 +16,7 @@ public class EnumTypeHandler extends TypeHandler { private static final Logger logger = LoggerFactory.getLogger(EnumTypeHandler.class); - private Class enumType; + private final Class enumType; private Map caseInsensitiveLookup = Maps.newHashMap(); public EnumTypeHandler(Class enumType) { diff --git a/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/coreTypes/factories/ObjectFieldMapTypeHandlerFactory.java b/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/coreTypes/factories/ObjectFieldMapTypeHandlerFactory.java index 7da6f3f4851..488f6404997 100644 --- a/subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/coreTypes/factories/ObjectFieldMapTypeHandlerFactory.java +++ b/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; 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 3585432edff..057beede8a6 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; +// 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; 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 214308cbfc8..13658863a06 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; +// 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; 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 bd485d52815..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 @@ -27,30 +27,6 @@ public class FutureTypeHandlerTest { private final TypeHandlerLibrary typeHandlerLibrary = Mockito.spy(new TypeHandlerLibrary(reflections)); - private static final class RecursiveType { - final T data; - final List> children; - - @SafeVarargs - private RecursiveType(T data, RecursiveType... children) { - this.data = data; - this.children = Lists.newArrayList(children); - } - } - - private class ResultCaptor 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>>> resultCaptor = new ResultCaptor<>(); @@ -77,4 +53,32 @@ public void testRecursiveType() { assertEquals(typeHandler, future.typeHandler); } + + private static final class RecursiveType { + final T data; + final List> children; + + @SafeVarargs + private RecursiveType(T data, RecursiveType... 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 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; + } + } } 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 8d77751f98b..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,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 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 Assertions.assertEquals(typeGetter.getGetter().apply(entry), typeGetter.getGetter().apply(data)) diff --git a/subsystems/TypeHandlerLibrary/src/test/java/org/terasology/persistence/typeHandling/TypeHandlerLibraryTest.java b/subsystems/TypeHandlerLibrary/src/test/java/org/terasology/persistence/typeHandling/TypeHandlerLibraryTest.java index 6d027341def..09659522ebe 100644 --- a/subsystems/TypeHandlerLibrary/src/test/java/org/terasology/persistence/typeHandling/TypeHandlerLibraryTest.java +++ b/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 handler = typeHandlerLibrary.getTypeHandler(AMappedContainer.class).get(); @@ -93,4 +89,7 @@ void testGetBaseTypeHandler() { } private enum AnEnum { } + + @MappedContainer + private static class AMappedContainer { } } diff --git a/subsystems/TypeHandlerLibrary/src/test/java/org/terasology/persistence/typeHandling/coreTypes/factories/BytesTypeHandlerTest.java b/subsystems/TypeHandlerLibrary/src/test/java/org/terasology/persistence/typeHandling/coreTypes/factories/BytesTypeHandlerTest.java index f0d9690bbb7..dff71f2ae62 100644 --- a/subsystems/TypeHandlerLibrary/src/test/java/org/terasology/persistence/typeHandling/coreTypes/factories/BytesTypeHandlerTest.java +++ b/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 serialize(T obj, TypeHandler typeHandler) { 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 c6e73e3bd7a..738dbfb45b5 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 @@ -112,4 +112,14 @@ private static class MultiTypeClass { 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 { + private T t; + private List list; + } }