fixup: fix all Collections.toMap

Signed-off-by: Todd Baert <todd.baert@dynatrace.com>

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,26 +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()
-                // 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())),
+                        (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,10 +48,10 @@ 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) {
 
@@ -90,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();
     }
 
@@ -139,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

@@ -275,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());
@@ -298,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,7 +98,7 @@ 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((String)null), res.getValue("nullKey"));
     }
 
     @Test

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.
+    }
 }