fixup: pr feedback

toddbaert · toddbaert · commit f3e06863c60d · 2024-10-23T10:40:20.000-04:00
Signed-off-by: Todd Baert &lt;todd.baert@dynatrace.com&gt;
diff --git a/benchmark.txt b/benchmark.txt
diff --git a/src/main/java/dev/openfeature/sdk/EvaluationContext.java b/src/main/java/dev/openfeature/sdk/EvaluationContext.java
@@ -1,5 +1,10 @@
 package dev.openfeature.sdk;
 
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Map.Entry;
+import java.util.function.Function;
+
 /**
  * The EvaluationContext is a container for arbitrary contextual data
  * that can be used as a basis for dynamic evaluation.
@@ -19,4 +24,40 @@ public interface EvaluationContext extends Structure {
      * @return resulting merged context
      */
     EvaluationContext merge(EvaluationContext overridingContext);
+
+    /**
+     * Recursively merges the base Value map with the overriding map.
+     * 
+     * @param <T> Structure type
+     * @param newStructure function to create the right structure(s) for Values
+     * @param base base map to merge
+     * @param overriding overriding map to merge
+     * @return resulting merged map
+     */
+    static <T extends Structure> Map<String, Value> mergeMaps(Function<Map<String, Value>, Structure> newStructure,
+            Map<String, Value> base,
+            Map<String, Value> overriding) {
+
+        if (base == null || base.isEmpty()) {
+            return overriding;
+        }
+        if (overriding == null || overriding.isEmpty()) {
+            return base;
+        }
+
+        final Map<String, Value> merged = new HashMap<>(base);
+        for (Entry<String, Value> overridingEntry : overriding.entrySet()) {
+            String key = overridingEntry.getKey();
+            if (overridingEntry.getValue().isStructure() && merged.containsKey(key) && merged.get(key).isStructure()) {
+                Structure mergedValue = merged.get(key).asStructure();
+                Structure overridingValue = overridingEntry.getValue().asStructure();
+                Map<String, Value> newMap = mergeMaps(newStructure, mergedValue.asUnmodifiableMap(),
+                        overridingValue.asUnmodifiableMap());
+                merged.put(key, new Value(newStructure.apply(newMap)));
+            } else {
+                merged.put(key, overridingEntry.getValue());
+            }
+        }
+        return merged;
+    }
 }
diff --git a/src/main/java/dev/openfeature/sdk/ImmutableContext.java b/src/main/java/dev/openfeature/sdk/ImmutableContext.java
@@ -11,7 +11,8 @@
 /**
  * The EvaluationContext is a container for arbitrary contextual data
  * that can be used as a basis for dynamic evaluation.
- * The ImmutableContext is an EvaluationContext implementation which is threadsafe, and whose attributes can
+ * The ImmutableContext is an EvaluationContext implementation which is
+ * threadsafe, and whose attributes can
  * not be modified after instantiation.
  */
 @ToString
@@ -22,7 +23,8 @@ public final class ImmutableContext implements EvaluationContext {
     private final ImmutableStructure structure;
 
     /**
-     * Create an immutable context with an empty targeting_key and attributes provided.
+     * Create an immutable context with an empty targeting_key and attributes
+     * provided.
      */
     public ImmutableContext() {
         this(new HashMap<>());
@@ -70,7 +72,8 @@ public String getTargetingKey() {
     }
 
     /**
-     * Merges this EvaluationContext object with the passed EvaluationContext, overriding in case of conflict.
+     * Merges this EvaluationContext object with the passed EvaluationContext,
+     * overriding in case of conflict.
      *
      * @param overridingContext overriding context
      * @return new, resulting merged context
@@ -85,16 +88,16 @@ public EvaluationContext merge(EvaluationContext overridingContext) {
         }
 
         return new ImmutableContext(
-                this.merge(ImmutableStructure::new, this.asUnmodifiableMap(), overridingContext.asUnmodifiableMap()));
+                EvaluationContext.mergeMaps(ImmutableStructure::new, this.asUnmodifiableMap(),
+                        overridingContext.asUnmodifiableMap()));
     }
 
     @SuppressWarnings("all")
     private static class DelegateExclusions {
         @ExcludeFromGeneratedCoverageReport
-        public <T extends Structure> Map<String, Value> merge(Function<Map<String, Value>, Structure> newStructure, 
+        public <T extends Structure> Map<String, Value> merge(Function<Map<String, Value>, Structure> newStructure,
                 Map<String, Value> base,
                 Map<String, Value> overriding) {
- 
             return null;
         }
     }
diff --git a/src/main/java/dev/openfeature/sdk/MutableContext.java b/src/main/java/dev/openfeature/sdk/MutableContext.java
@@ -33,7 +33,7 @@ public MutableContext(String targetingKey) {
     }
 
     public MutableContext(Map<String, Value> attributes) {
-        this("", new HashMap<>(attributes));
+        this(null, new HashMap<>(attributes));
     }
 
     /**
@@ -121,7 +121,7 @@ public EvaluationContext merge(EvaluationContext overridingContext) {
             return overridingContext;
         }
 
-        Map<String, Value> merged = this.merge(
+        Map<String, Value> merged = EvaluationContext.mergeMaps(
                 MutableStructure::new, this.asUnmodifiableMap(), overridingContext.asUnmodifiableMap());
         return new MutableContext(merged);
     }
diff --git a/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java b/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java
@@ -1,15 +1,24 @@
 package dev.openfeature.sdk;
 
-import dev.openfeature.sdk.exceptions.*;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.List;
+import java.util.function.Consumer;
+
+import dev.openfeature.sdk.exceptions.ExceptionUtils;
+import dev.openfeature.sdk.exceptions.FatalError;
+import dev.openfeature.sdk.exceptions.GeneralError;
+import dev.openfeature.sdk.exceptions.OpenFeatureError;
+import dev.openfeature.sdk.exceptions.ProviderNotReadyError;
 import dev.openfeature.sdk.internal.AutoCloseableLock;
 import dev.openfeature.sdk.internal.AutoCloseableReentrantReadWriteLock;
 import dev.openfeature.sdk.internal.ObjectUtils;
 import lombok.Getter;
 import lombok.extern.slf4j.Slf4j;
 
-import java.util.*;
-import java.util.function.Consumer;
-
 /**
  * OpenFeature Client implementation.
  * You should not instantiate this or reference this class.
@@ -19,8 +28,8 @@
  * @deprecated // TODO: eventually we will make this non-public. See issue #872
  */
 @Slf4j
-@SuppressWarnings({"PMD.DataflowAnomalyAnalysis", "PMD.BeanMembersShouldSerialize", "PMD.UnusedLocalVariable",
-    "unchecked", "rawtypes"})
+@SuppressWarnings({ "PMD.DataflowAnomalyAnalysis", "PMD.BeanMembersShouldSerialize", "PMD.UnusedLocalVariable",
+    "unchecked", "rawtypes" })
 @Deprecated() // TODO: eventually we will make this non-public. See issue #872
 public class OpenFeatureClient implements Client {
 
@@ -39,18 +48,18 @@ public class OpenFeatureClient implements Client {
      * Deprecated public constructor. Use OpenFeature.API.getClient() instead.
      *
      * @param openFeatureAPI Backing global singleton
-     * @param domain         An identifier which logically binds clients with providers (used by observability tools).
+     * @param domain         An identifier which logically binds clients with
+     *                       providers (used by observability tools).
      * @param version        Version of the client (used by observability tools).
      * @deprecated Do not use this constructor. It's for internal use only.
-     *     Clients created using it will not run event handlers.
-     *     Use the OpenFeatureAPI's getClient factory method instead.
+     *             Clients created using it will not run event handlers.
+     *             Use the OpenFeatureAPI's getClient factory method instead.
      */
     @Deprecated() // TODO: eventually we will make this non-public. See issue #872
     public OpenFeatureClient(
             OpenFeatureAPI openFeatureAPI,
             String domain,
-            String version
-    ) {
+            String version) {
         this.openfeatureApi = openFeatureAPI;
         this.domain = domain;
         this.version = version;
@@ -106,7 +115,7 @@ public EvaluationContext getEvaluationContext() {
     }
 
     private <T> FlagEvaluationDetails<T> evaluateFlag(FlagValueType type, String key, T defaultValue,
-                                                      EvaluationContext ctx, FlagEvaluationOptions options) {
+            EvaluationContext ctx, FlagEvaluationOptions options) {
         FlagEvaluationOptions flagOptions = ObjectUtils.defaultIfNull(options,
                 () -> FlagEvaluationOptions.builder().build());
         Map<String, Object> hints = Collections.unmodifiableMap(flagOptions.getHookHints());
@@ -182,29 +191,23 @@ private static <T> void enrichDetailsWithErrorDefaults(T defaultValue, FlagEvalu
      * @return merged evaluation context
      */
     private EvaluationContext mergeEvaluationContext(EvaluationContext invocationContext) {
-        // avoid any unnecessary context instantiations and stream usage here; this is call with every evaluation.
         final EvaluationContext apiContext = openfeatureApi.getEvaluationContext();
         final EvaluationContext clientContext = this.getEvaluationContext();
         final EvaluationContext transactionContext = openfeatureApi.getTransactionContext();
-        final List<EvaluationContext> contextsToMerge = new ArrayList<>();
-        if (apiContext != null) {
-            contextsToMerge.add(apiContext);
-        }
-        if (transactionContext != null) {
-            contextsToMerge.add(transactionContext);
-        }
-        if (clientContext != null) {
-            contextsToMerge.add(clientContext);
-        }
-        if (invocationContext != null) {
-            contextsToMerge.add(invocationContext);
-        }
+        return mergeContextMaps(apiContext, transactionContext, clientContext, invocationContext);
+    }
 
-        EvaluationContext merged = new ImmutableContext();
-        for (EvaluationContext evaluationContext : contextsToMerge) {
-            merged = merged.merge(evaluationContext);
+    private EvaluationContext mergeContextMaps(EvaluationContext... contexts) {
+        // avoid any unnecessary context instantiations and stream usage here; this is
+        // called with every evaluation.
+        Map merged = new HashMap<>();
+        for (EvaluationContext evaluationContext : contexts) {
+            if (evaluationContext != null && !evaluationContext.isEmpty()) {
+                merged = EvaluationContext.mergeMaps(ImmutableStructure::new, merged,
+                        evaluationContext.asUnmodifiableMap());
+            }
         }
-        return merged;
+        return new ImmutableContext(merged);
     }
 
     private <T> ProviderEvaluation<?> createProviderEvaluation(
@@ -241,7 +244,7 @@ public Boolean getBooleanValue(String key, Boolean defaultValue, EvaluationConte
 
     @Override
     public Boolean getBooleanValue(String key, Boolean defaultValue, EvaluationContext ctx,
-                                   FlagEvaluationOptions options) {
+            FlagEvaluationOptions options) {
         return getBooleanDetails(key, defaultValue, ctx, options).getValue();
     }
 
@@ -257,7 +260,7 @@ public FlagEvaluationDetails<Boolean> getBooleanDetails(String key, Boolean defa
 
     @Override
     public FlagEvaluationDetails<Boolean> getBooleanDetails(String key, Boolean defaultValue, EvaluationContext ctx,
-                                                            FlagEvaluationOptions options) {
+            FlagEvaluationOptions options) {
         return this.evaluateFlag(FlagValueType.BOOLEAN, key, defaultValue, ctx, options);
     }
 
@@ -273,7 +276,7 @@ public String getStringValue(String key, String defaultValue, EvaluationContext
 
     @Override
     public String getStringValue(String key, String defaultValue, EvaluationContext ctx,
-                                 FlagEvaluationOptions options) {
+            FlagEvaluationOptions options) {
         return getStringDetails(key, defaultValue, ctx, options).getValue();
     }
 
@@ -289,7 +292,7 @@ public FlagEvaluationDetails<String> getStringDetails(String key, String default
 
     @Override
     public FlagEvaluationDetails<String> getStringDetails(String key, String defaultValue, EvaluationContext ctx,
-                                                          FlagEvaluationOptions options) {
+            FlagEvaluationOptions options) {
         return this.evaluateFlag(FlagValueType.STRING, key, defaultValue, ctx, options);
     }
 
@@ -305,7 +308,7 @@ public Integer getIntegerValue(String key, Integer defaultValue, EvaluationConte
 
     @Override
     public Integer getIntegerValue(String key, Integer defaultValue, EvaluationContext ctx,
-                                   FlagEvaluationOptions options) {
+            FlagEvaluationOptions options) {
         return getIntegerDetails(key, defaultValue, ctx, options).getValue();
     }
 
@@ -321,7 +324,7 @@ public FlagEvaluationDetails<Integer> getIntegerDetails(String key, Integer defa
 
     @Override
     public FlagEvaluationDetails<Integer> getIntegerDetails(String key, Integer defaultValue, EvaluationContext ctx,
-                                                            FlagEvaluationOptions options) {
+            FlagEvaluationOptions options) {
         return this.evaluateFlag(FlagValueType.INTEGER, key, defaultValue, ctx, options);
     }
 
@@ -337,7 +340,7 @@ public Double getDoubleValue(String key, Double defaultValue, EvaluationContext
 
     @Override
     public Double getDoubleValue(String key, Double defaultValue, EvaluationContext ctx,
-                                 FlagEvaluationOptions options) {
+            FlagEvaluationOptions options) {
         return this.evaluateFlag(FlagValueType.DOUBLE, key, defaultValue, ctx, options).getValue();
     }
 
@@ -353,7 +356,7 @@ public FlagEvaluationDetails<Double> getDoubleDetails(String key, Double default
 
     @Override
     public FlagEvaluationDetails<Double> getDoubleDetails(String key, Double defaultValue, EvaluationContext ctx,
-                                                          FlagEvaluationOptions options) {
+            FlagEvaluationOptions options) {
         return this.evaluateFlag(FlagValueType.DOUBLE, key, defaultValue, ctx, options);
     }
 
@@ -369,7 +372,7 @@ public Value getObjectValue(String key, Value defaultValue, EvaluationContext ct
 
     @Override
     public Value getObjectValue(String key, Value defaultValue, EvaluationContext ctx,
-                                FlagEvaluationOptions options) {
+            FlagEvaluationOptions options) {
         return getObjectDetails(key, defaultValue, ctx, options).getValue();
     }
 
@@ -380,13 +383,13 @@ public FlagEvaluationDetails<Value> getObjectDetails(String key, Value defaultVa
 
     @Override
     public FlagEvaluationDetails<Value> getObjectDetails(String key, Value defaultValue,
-                                                         EvaluationContext ctx) {
+            EvaluationContext ctx) {
         return getObjectDetails(key, defaultValue, ctx, FlagEvaluationOptions.builder().build());
     }
 
     @Override
     public FlagEvaluationDetails<Value> getObjectDetails(String key, Value defaultValue, EvaluationContext ctx,
-                                                         FlagEvaluationOptions options) {
+            FlagEvaluationOptions options) {
         return this.evaluateFlag(FlagValueType.OBJECT, key, defaultValue, ctx, options);
     }
 
diff --git a/src/main/java/dev/openfeature/sdk/Structure.java b/src/main/java/dev/openfeature/sdk/Structure.java
@@ -5,8 +5,6 @@
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Set;
-import java.util.Map.Entry;
-import java.util.function.Function;
 import java.util.stream.Collectors;
 
 import static dev.openfeature.sdk.Value.objectToValue;
@@ -115,42 +113,6 @@ default Object convertValue(Value value) {
         throw new ValueNotConvertableError();
     }
 
-    /**
-     * Recursively merges the base map with the overriding map.
-     * 
-     * @param <T> Structure type
-     * @param newStructure function to create the right structure
-     * @param base base map to merge
-     * @param overriding overriding map to merge
-     * @return resulting merged map
-     */
-    default <T extends Structure> Map<String, Value> merge(Function<Map<String, Value>, Structure> newStructure,
-                                                           Map<String, Value> base,
-                                                           Map<String, Value> overriding) {
-        
-        if (base.isEmpty()) {
-            return overriding;
-        }
-        if (overriding.isEmpty()) {
-            return base;
-        }
-
-        final Map<String, Value> merged = new HashMap<>(base);
-        for (Entry<String, Value> overridingEntry : overriding.entrySet()) {
-            String key = overridingEntry.getKey();
-            if (overridingEntry.getValue().isStructure() && merged.containsKey(key) && merged.get(key).isStructure()) {
-                Structure mergedValue = merged.get(key).asStructure();
-                Structure overridingValue = overridingEntry.getValue().asStructure();
-                Map<String, Value> newMap = this.merge(newStructure, mergedValue.asUnmodifiableMap(),
-                        overridingValue.asUnmodifiableMap());
-                merged.put(key, new Value(newStructure.apply(newMap)));
-            } else {
-                merged.put(key, overridingEntry.getValue());
-            }
-        }
-        return merged;
-    }
-
     /**
      * Transform an object map to a {@link Structure} type.
      *

Original file line number	Diff line number	Diff line change
`@@ -33,7 +33,7 @@ public MutableContext(String targetingKey) {`
`33`	`33`	`}`
`34`	`34`
`35`	`35`	`public MutableContext(Map<String, Value> attributes) {`
`36`		`- this("", new HashMap<>(attributes));`
	`36`	`+ this(null, new HashMap<>(attributes));`
`37`	`37`	`}`
`38`	`38`
`39`	`39`	`/**`
`@@ -121,7 +121,7 @@ public EvaluationContext merge(EvaluationContext overridingContext) {`
`121`	`121`	`return overridingContext;`
`122`	`122`	`}`
`123`	`123`
`124`		`- Map<String, Value> merged = this.merge(`
	`124`	`+ Map<String, Value> merged = EvaluationContext.mergeMaps(`
`125`	`125`	`MutableStructure::new, this.asUnmodifiableMap(), overridingContext.asUnmodifiableMap());`
`126`	`126`	`return new MutableContext(merged);`
`127`	`127`	`}`