fix: null handling with Structure, Value (#663)

Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Michael Beemer <beeme1mr@users.noreply.github.com>
Co-authored-by: Giovanni Liva <giovanni.liva@dynatrace.com>

pom.xml

@@ -53,7 +53,7 @@
       <!-- used so that lombok can generate suppressions for spotbugs. It needs to find it on the relevant classpath -->
       <groupId>com.github.spotbugs</groupId>
       <artifactId>spotbugs</artifactId>
-      <version>4.7.3</version>
+      <version>4.8.0</version>
       <scope>provided</scope>
     </dependency>
 
@@ -365,7 +365,7 @@
           <dependency>
             <groupId>com.github.spotbugs</groupId>
             <artifactId>spotbugs</artifactId>
-            <version>4.7.3</version>
+            <version>4.8.0</version>
           </dependency>
         </dependencies>
         <executions>

spotbugs-exclusions.xml

@@ -26,6 +26,26 @@
         <Class name="dev.openfeature.sdk.OpenFeatureAPI"/>
         <Bug pattern="EI_EXPOSE_REP2"/>
     </And>
+    <And>
+        Added in spotbugs 4.8.0 - EventProvider shares a name with something from the standard lib (confusing), but change would be breaking
+        <Class name="dev.openfeature.sdk.EventProvider"/>
+        <Bug pattern="PI_DO_NOT_REUSE_PUBLIC_IDENTIFIERS_CLASS_NAMES"/>
+    </And>
+    <And>
+        Added in spotbugs 4.8.0 - Metadata shares a name with something from the standard lib (confusing), but change would be breaking
+        <Class name="dev.openfeature.sdk.Metadata"/>
+        <Bug pattern="PI_DO_NOT_REUSE_PUBLIC_IDENTIFIERS_CLASS_NAMES"/>
+    </And>
+    <And>
+        Added in spotbugs 4.8.0 - Reason shares a name with something from the standard lib (confusing), but change would be breaking
+        <Class name="dev.openfeature.sdk.Reason"/>
+        <Bug pattern="PI_DO_NOT_REUSE_PUBLIC_IDENTIFIERS_CLASS_NAMES"/>
+    </And>
+    <And>
+        Added in spotbugs 4.8.0 - FlagValueType.STRING shares a name with something from the standard lib (confusing), but change would be breaking
+        <Class name="dev.openfeature.sdk.FlagValueType"/>
+        <Bug pattern="PI_DO_NOT_REUSE_PUBLIC_IDENTIFIERS_FIELD_NAMES"/>
+    </And>
 
     <!-- Test class that should be excluded -->
     <Match>

src/main/java/dev/openfeature/sdk/AbstractStructure.java

@@ -0,0 +1,35 @@
+package dev.openfeature.sdk;
+
+import java.util.HashMap;
+import java.util.Map;
+
+@SuppressWarnings({ "PMD.BeanMembersShouldSerialize", "checkstyle:MissingJavadocType" })
+abstract class AbstractStructure implements Structure {
+
+    protected final Map<String, Value> attributes;
+
+    AbstractStructure() {
+        this.attributes = new HashMap<>();
+    }
+
+    AbstractStructure(Map<String, Value> attributes) {
+        this.attributes = attributes;
+    }
+
+    /**
+     * Get all values as their underlying primitives types.
+     *
+     * @return all attributes on the structure into a Map
+     */
+    @Override
+    public Map<String, Object> asObjectMap() {
+        return attributes
+                .entrySet()
+                .stream()
+                // custom collector, workaround for Collectors.toMap in JDK8
+                // https://bugs.openjdk.org/browse/JDK-8148463
+                .collect(HashMap::new,
+                        (accumulated, entry) -> accumulated.put(entry.getKey(), convertValue(entry.getValue())),
+                        HashMap::putAll);
+    }
+}

src/main/java/dev/openfeature/sdk/ImmutableStructure.java

@@ -1,32 +1,32 @@
 package dev.openfeature.sdk;
 
-import lombok.EqualsAndHashCode;
-import lombok.ToString;
-
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Map;
+import java.util.Optional;
 import java.util.Set;
-import java.util.stream.Collectors;
+
+import lombok.EqualsAndHashCode;
+import lombok.ToString;
 
 /**
- * {@link ImmutableStructure} represents a potentially nested object type which is used to represent
+ * {@link ImmutableStructure} represents a potentially nested object type which
+ * is used to represent
  * structured data.
- * The ImmutableStructure is a Structure implementation which is threadsafe, and whose attributes can
- * not be modified after instantiation.
+ * The ImmutableStructure is a Structure implementation which is threadsafe, and
+ * whose attributes can
+ * not be modified after instantiation. All references are clones.
  */
 @ToString
 @EqualsAndHashCode
-@SuppressWarnings({"PMD.BeanMembersShouldSerialize", "checkstyle:MissingJavadocType"})
-public final class ImmutableStructure implements Structure {
-
-    private final Map<String, Value> attributes;
+@SuppressWarnings({ "PMD.BeanMembersShouldSerialize", "checkstyle:MissingJavadocType" })
+public final class ImmutableStructure extends AbstractStructure {
 
     /**
      * create an immutable structure with the empty attributes.
      */
     public ImmutableStructure() {
-        this(new HashMap<>());
+        super();
     }
 
     /**
@@ -35,10 +35,14 @@ public ImmutableStructure() {
      * @param attributes attributes.
      */
     public ImmutableStructure(Map<String, Value> attributes) {
-        Map<String, Value> copy = attributes.entrySet()
+        super(new HashMap<>(attributes.entrySet()
                 .stream()
-                .collect(Collectors.toMap(Map.Entry::getKey, e -> e.getValue().clone()));
-        this.attributes = new HashMap<>(copy);
+                .collect(HashMap::new,
+                        (accumulated, entry) -> accumulated.put(entry.getKey(),
+                                Optional.ofNullable(entry.getValue())
+                                    .map(e -> e.clone())
+                                    .orElse(null)),
+                        HashMap::putAll)));
     }
 
     @Override
@@ -63,25 +67,11 @@ public Map<String, Value> asMap() {
         return attributes
                 .entrySet()
                 .stream()
-                .collect(Collectors.toMap(
-                        Map.Entry::getKey,
-                        e -> getValue(e.getKey())
-                ));
-    }
-
-    /**
-     * Get all values, with primitives types.
-     *
-     * @return all attributes on the structure into a Map
-     */
-    @Override
-    public Map<String, Object> asObjectMap() {
-        return attributes
-                .entrySet()
-                .stream()
-                .collect(Collectors.toMap(
-                        Map.Entry::getKey,
-                        e -> convertValue(getValue(e.getKey()))
-                ));
+                .collect(HashMap::new,
+                        (accumulated, entry) -> accumulated.put(entry.getKey(),
+                                Optional.ofNullable(entry.getValue())
+                                .map(e -> e.clone())
+                                .orElse(null)),
+                        HashMap::putAll);
     }
 }

src/main/java/dev/openfeature/sdk/MutableStructure.java

@@ -1,14 +1,13 @@
 package dev.openfeature.sdk;
 
-import lombok.EqualsAndHashCode;
-import lombok.ToString;
-
 import java.time.Instant;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
-import java.util.stream.Collectors;
+
+import lombok.EqualsAndHashCode;
+import lombok.ToString;
 
 /**
  * {@link MutableStructure} represents a potentially nested object type which is used to represent 
@@ -19,16 +18,14 @@
 @ToString
 @EqualsAndHashCode
 @SuppressWarnings({"PMD.BeanMembersShouldSerialize", "checkstyle:MissingJavadocType"})
-public class MutableStructure implements Structure {
-
-    protected final Map<String, Value> attributes;
+public class MutableStructure extends AbstractStructure {
 
     public MutableStructure() {
-        this.attributes = new HashMap<>();
+        super();
     }
 
     public MutableStructure(Map<String, Value> attributes) {
-        this.attributes = new HashMap<>(attributes);
+        super(attributes);
     }
 
     @Override
@@ -92,20 +89,4 @@ public <T> MutableStructure add(String key, List<Value> value) {
     public Map<String, Value> asMap() {
         return new HashMap<>(this.attributes);
     }
-
-    /**
-     * Get all values, with primitives types.
-     *
-     * @return all attributes on the structure into a Map
-     */
-    @Override
-    public Map<String, Object> asObjectMap() {
-        return attributes
-            .entrySet()
-            .stream()
-            .collect(Collectors.toMap(
-                Map.Entry::getKey,
-                e -> convertValue(getValue(e.getKey()))
-            ));
-    }
 }

src/main/java/dev/openfeature/sdk/Structure.java

@@ -48,12 +48,17 @@ public interface Structure {
     Map<String, Object> asObjectMap();
 
     /**
-     * convertValue is converting the object type Value in a primitive type.
+     * Converts the Value into its equivalent primitive type.
      *
      * @param value - Value object to convert
-     * @return an Object containing the primitive type.
+     * @return an Object containing the primitive type, or null.
      */
     default Object convertValue(Value value) {
+
+        if (value == null || value.isNull()) {
+            return null;
+        }
+
         if (value.isBoolean()) {
             return value.asBoolean();
         }
@@ -85,15 +90,14 @@ default Object convertValue(Value value) {
         if (value.isStructure()) {
             Structure s = value.asStructure();
             return s.asMap()
-                    .keySet()
+                    .entrySet()
                     .stream()
-                    .collect(
-                            Collectors.toMap(
-                                    key -> key,
-                                    key -> convertValue(s.getValue(key))
-                            )
-                    );
+                    .collect(HashMap::new,
+                            (accumulated, entry) -> accumulated.put(entry.getKey(),
+                                    convertValue(entry.getValue())),
+                            HashMap::putAll);
         }
+
         throw new ValueNotConvertableError();
     }
 
@@ -134,7 +138,9 @@ default <T extends Structure> Map<String, Value> merge(Function<Map<String, Valu
      */
     static Structure mapToStructure(Map<String, Object> map) {
         return new MutableStructure(map.entrySet().stream()
-            .filter(e -> e.getValue() != null)
-            .collect(Collectors.toMap(Map.Entry::getKey, e -> objectToValue(e.getValue()))));
+            .collect(HashMap::new,
+                        (accumulated, entry) -> accumulated.put(entry.getKey(),
+                        objectToValue(entry.getValue())),
+                        HashMap::putAll));
     }
 }

src/main/java/dev/openfeature/sdk/Value.java

@@ -19,11 +19,15 @@
  */
 @ToString
 @EqualsAndHashCode
-@SuppressWarnings({"PMD.BeanMembersShouldSerialize", "checkstyle:MissingJavadocType"})
+@SuppressWarnings({"PMD.BeanMembersShouldSerialize", "checkstyle:MissingJavadocType", "checkstyle:NoFinalizer"})
 public class Value implements Cloneable {
 
     private final Object innerObject;
 
+    protected final void finalize() {
+        // DO NOT REMOVE, spotbugs: CT_CONSTRUCTOR_THROW
+    }
+
     /**
      * Construct a new null Value.
      */
@@ -271,11 +275,7 @@ protected Value clone() {
             return new Value(copy);
         }
         if (this.isStructure()) {
-            Map<String, Value> copy = this.asStructure().asMap().entrySet().stream().collect(Collectors.toMap(
-                    Map.Entry::getKey,
-                    e -> e.getValue().clone()
-            ));
-            return new Value(new ImmutableStructure(copy));
+            return new Value(new ImmutableStructure(this.asStructure().asMap()));
         }
         if (this.isInstant()) {
             Instant copy = Instant.ofEpochMilli(this.asInstant().toEpochMilli());
@@ -294,7 +294,7 @@ public static Value objectToValue(Object object) {
         if (object instanceof Value) {
             return (Value) object;
         } else if (object == null) {
-            return null;
+            return new Value();
         } else if (object instanceof String) {
             return new Value((String) object);
         } else if (object instanceof Boolean) {

src/test/java/dev/openfeature/sdk/ImmutableStructureTest.java

@@ -122,4 +122,11 @@ void GettingAMissingValueShouldReturnNull() {
 
         assertEquals(expected, structure.asObjectMap());
     }
+
+    @Test
+    void constructorHandlesNullValue() {
+        HashMap<String, Value> attrs = new HashMap<>();
+        attrs.put("null", null);
+        new ImmutableStructure(attrs);
+    }
 }

src/test/java/dev/openfeature/sdk/StructureTest.java

@@ -98,6 +98,20 @@ void mapToStructureTest() {
         assertEquals(new Value(Instant.ofEpochSecond(0)), res.getValue("Instant"));
         assertEquals(new HashMap<>(), res.getValue("Map").asStructure().asMap());
         assertEquals(new Value(immutableContext), res.getValue("ImmutableContext"));
-        assertNull(res.getValue("nullKey"));
+        assertEquals(new Value(), res.getValue("nullKey"));
+    }
+
+    @Test
+    void asObjectHandlesNullValue() {
+        Map<String, Value> map = new HashMap<>();
+        map.put("null", new Value((String)null));
+        ImmutableStructure structure = new ImmutableStructure(map);
+        assertNull(structure.asObjectMap().get("null"));
+    }
+
+    @Test
+    void convertValueHandlesNullValue() {
+        ImmutableStructure structure = new ImmutableStructure();
+        assertNull(structure.convertValue(new Value((String)null)));
     }
 }

src/test/java/dev/openfeature/sdk/ValueTest.java

@@ -1,5 +1,6 @@
 package dev.openfeature.sdk;
 
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
@@ -142,4 +143,9 @@ class Something {}
 
         assertThrows(InstantiationException.class, ()-> new Value(list));
     }
+
+    @Test public void noOpFinalize() {
+        Value val = new Value();
+        assertDoesNotThrow(val::finalize); // does nothing, but we want to defined in and make it final.
+    }
 }