diff --git a/pom.xml b/pom.xml index f28352814..d2fbe954a 100644 --- a/pom.xml +++ b/pom.xml @@ -53,7 +53,7 @@ com.github.spotbugs spotbugs - 4.7.3 + 4.8.0 provided @@ -365,7 +365,7 @@ com.github.spotbugs spotbugs - 4.7.3 + 4.8.0 diff --git a/spotbugs-exclusions.xml b/spotbugs-exclusions.xml index 8105db97e..66032ad08 100644 --- a/spotbugs-exclusions.xml +++ b/spotbugs-exclusions.xml @@ -26,6 +26,26 @@ + + Added in spotbugs 4.8.0 - EventProvider shares a name with something from the standard lib (confusing), but change would be breaking + + + + + Added in spotbugs 4.8.0 - Metadata shares a name with something from the standard lib (confusing), but change would be breaking + + + + + Added in spotbugs 4.8.0 - Reason shares a name with something from the standard lib (confusing), but change would be breaking + + + + + Added in spotbugs 4.8.0 - FlagValueType.STRING shares a name with something from the standard lib (confusing), but change would be breaking + + + diff --git a/src/main/java/dev/openfeature/sdk/AbstractStructure.java b/src/main/java/dev/openfeature/sdk/AbstractStructure.java new file mode 100644 index 000000000..a7d7c2eae --- /dev/null +++ b/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 attributes; + + AbstractStructure() { + this.attributes = new HashMap<>(); + } + + AbstractStructure(Map attributes) { + this.attributes = attributes; + } + + /** + * Get all values as their underlying primitives types. + * + * @return all attributes on the structure into a Map + */ + @Override + public Map 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); + } +} diff --git a/src/main/java/dev/openfeature/sdk/ImmutableStructure.java b/src/main/java/dev/openfeature/sdk/ImmutableStructure.java index 731a55b1e..7ea1ef654 100644 --- a/src/main/java/dev/openfeature/sdk/ImmutableStructure.java +++ b/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 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 attributes) { - Map 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 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 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); } } diff --git a/src/main/java/dev/openfeature/sdk/MutableStructure.java b/src/main/java/dev/openfeature/sdk/MutableStructure.java index 343fe2c94..3c4f34dd0 100644 --- a/src/main/java/dev/openfeature/sdk/MutableStructure.java +++ b/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 attributes; +public class MutableStructure extends AbstractStructure { public MutableStructure() { - this.attributes = new HashMap<>(); + super(); } public MutableStructure(Map attributes) { - this.attributes = new HashMap<>(attributes); + super(attributes); } @Override @@ -92,20 +89,4 @@ public MutableStructure add(String key, List value) { public Map asMap() { return new HashMap<>(this.attributes); } - - /** - * Get all values, with primitives types. - * - * @return all attributes on the structure into a Map - */ - @Override - public Map asObjectMap() { - return attributes - .entrySet() - .stream() - .collect(Collectors.toMap( - Map.Entry::getKey, - e -> convertValue(getValue(e.getKey())) - )); - } } diff --git a/src/main/java/dev/openfeature/sdk/Structure.java b/src/main/java/dev/openfeature/sdk/Structure.java index 46274e70e..6ceaf5926 100644 --- a/src/main/java/dev/openfeature/sdk/Structure.java +++ b/src/main/java/dev/openfeature/sdk/Structure.java @@ -48,12 +48,17 @@ public interface Structure { Map 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 Map merge(Function 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)); } } diff --git a/src/main/java/dev/openfeature/sdk/Value.java b/src/main/java/dev/openfeature/sdk/Value.java index 8be50179f..59e4a9cf5 100644 --- a/src/main/java/dev/openfeature/sdk/Value.java +++ b/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 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) { diff --git a/src/test/java/dev/openfeature/sdk/ImmutableStructureTest.java b/src/test/java/dev/openfeature/sdk/ImmutableStructureTest.java index d7453452e..491b5069f 100644 --- a/src/test/java/dev/openfeature/sdk/ImmutableStructureTest.java +++ b/src/test/java/dev/openfeature/sdk/ImmutableStructureTest.java @@ -122,4 +122,11 @@ void GettingAMissingValueShouldReturnNull() { assertEquals(expected, structure.asObjectMap()); } + + @Test + void constructorHandlesNullValue() { + HashMap attrs = new HashMap<>(); + attrs.put("null", null); + new ImmutableStructure(attrs); + } } diff --git a/src/test/java/dev/openfeature/sdk/StructureTest.java b/src/test/java/dev/openfeature/sdk/StructureTest.java index 8f1889114..16747ee06 100644 --- a/src/test/java/dev/openfeature/sdk/StructureTest.java +++ b/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 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))); } } diff --git a/src/test/java/dev/openfeature/sdk/ValueTest.java b/src/test/java/dev/openfeature/sdk/ValueTest.java index 53513afd1..816190ab8 100644 --- a/src/test/java/dev/openfeature/sdk/ValueTest.java +++ b/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. + } }