From 13dbeeefeafcc75c15fe8e515c9ebf1675e228ad Mon Sep 17 00:00:00 2001 From: mdxabu Date: Sun, 6 Jul 2025 16:35:16 +0530 Subject: [PATCH 01/19] Added hook data Signed-off-by: mdxabu --- .../java/dev/openfeature/sdk/HookContext.java | 9 +- .../java/dev/openfeature/sdk/HookData.java | 75 ++++++++++++ .../java/dev/openfeature/sdk/HookSupport.java | 14 ++- .../dev/openfeature/sdk/HookContextTest.java | 50 +++++++- .../dev/openfeature/sdk/HookDataTest.java | 112 ++++++++++++++++++ .../dev/openfeature/sdk/HookSupportTest.java | 53 +++++++-- 6 files changed, 301 insertions(+), 12 deletions(-) create mode 100644 src/main/java/dev/openfeature/sdk/HookData.java create mode 100644 src/test/java/dev/openfeature/sdk/HookDataTest.java diff --git a/src/main/java/dev/openfeature/sdk/HookContext.java b/src/main/java/dev/openfeature/sdk/HookContext.java index e14eeb643..b06d2e3fa 100644 --- a/src/main/java/dev/openfeature/sdk/HookContext.java +++ b/src/main/java/dev/openfeature/sdk/HookContext.java @@ -11,7 +11,7 @@ * @param the type for the flag being evaluated */ @Value -@Builder +@Builder(toBuilder = true) @With public class HookContext { @NonNull String flagKey; @@ -25,6 +25,12 @@ public class HookContext { ClientMetadata clientMetadata; Metadata providerMetadata; + /** + * Hook data provides a way for hooks to maintain state across their execution stages. + * Each hook instance gets its own isolated data store. + */ + HookData hookData; + /** * Builds a {@link HookContext} instances from request data. * @@ -51,6 +57,7 @@ public static HookContext from( .providerMetadata(providerMetadata) .ctx(ctx) .defaultValue(defaultValue) + .hookData(null) // Explicitly set to null for backward compatibility .build(); } } diff --git a/src/main/java/dev/openfeature/sdk/HookData.java b/src/main/java/dev/openfeature/sdk/HookData.java new file mode 100644 index 000000000..5c22ea799 --- /dev/null +++ b/src/main/java/dev/openfeature/sdk/HookData.java @@ -0,0 +1,75 @@ +package dev.openfeature.sdk; + +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentMap; + +/** + * Hook data provides a way for hooks to maintain state across their execution stages. + * Each hook instance gets its own isolated data store that persists only for the duration + * of a single flag evaluation. + */ +public interface HookData { + + /** + * Sets a value for the given key. + * + * @param key the key to store the value under + * @param value the value to store + */ + void set(String key, Object value); + + /** + * Gets the value for the given key. + * + * @param key the key to retrieve the value for + * @return the value, or null if not found + */ + Object get(String key); + + /** + * Gets the value for the given key, cast to the specified type. + * + * @param the type to cast to + * @param key the key to retrieve the value for + * @param type the class to cast to + * @return the value cast to the specified type, or null if not found + * @throws ClassCastException if the value cannot be cast to the specified type + */ + T get(String key, Class type); + + /** + * Default implementation using ConcurrentHashMap for thread safety. + */ + static HookData create() { + return new DefaultHookData(); + } + + /** + * Default thread-safe implementation of HookData. + */ + class DefaultHookData implements HookData { + private final ConcurrentMap data = new ConcurrentHashMap<>(); + + @Override + public void set(String key, Object value) { + data.put(key, value); + } + + @Override + public Object get(String key) { + return data.get(key); + } + + @Override + public T get(String key, Class type) { + Object value = data.get(key); + if (value == null) { + return null; + } + if (!type.isInstance(value)) { + throw new ClassCastException("Value for key '" + key + "' is not of type " + type.getName()); + } + return type.cast(value); + } + } +} diff --git a/src/main/java/dev/openfeature/sdk/HookSupport.java b/src/main/java/dev/openfeature/sdk/HookSupport.java index 73518ee8e..df6115c57 100644 --- a/src/main/java/dev/openfeature/sdk/HookSupport.java +++ b/src/main/java/dev/openfeature/sdk/HookSupport.java @@ -2,6 +2,7 @@ import java.util.ArrayList; import java.util.Collections; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Optional; @@ -87,10 +88,21 @@ private EvaluationContext callBeforeHooks( List reversedHooks = new ArrayList<>(hooks); Collections.reverse(reversedHooks); EvaluationContext context = hookCtx.getCtx(); + + // Create hook data for each hook instance + Map hookDataMap = new HashMap<>(); + for (Hook hook : reversedHooks) { + if (hook.supportsFlagValueType(flagValueType)) { + hookDataMap.put(hook, HookData.create()); + } + } + for (Hook hook : reversedHooks) { if (hook.supportsFlagValueType(flagValueType)) { + // Create a new context with this hook's data + HookContext contextWithHookData = hookCtx.withHookData(hookDataMap.get(hook)); Optional optional = - Optional.ofNullable(hook.before(hookCtx, hints)).orElse(Optional.empty()); + Optional.ofNullable(hook.before(contextWithHookData, hints)).orElse(Optional.empty()); if (optional.isPresent()) { context = context.merge(optional.get()); } diff --git a/src/test/java/dev/openfeature/sdk/HookContextTest.java b/src/test/java/dev/openfeature/sdk/HookContextTest.java index 2196b8b1f..fcf9715e5 100644 --- a/src/test/java/dev/openfeature/sdk/HookContextTest.java +++ b/src/test/java/dev/openfeature/sdk/HookContextTest.java @@ -1,6 +1,6 @@ package dev.openfeature.sdk; -import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.*; import static org.mockito.Mockito.mock; import org.junit.jupiter.api.Test; @@ -29,4 +29,52 @@ void metadata_field_is_type_metadata() { "The before stage MUST run before flag resolution occurs. It accepts a hook context (required) and hook hints (optional) as parameters. It has no return value.") @Test void not_applicable_for_dynamic_context() {} + + @Test + void shouldCreateHookContextWithHookData() { + HookData hookData = HookData.create(); + hookData.set("test", "value"); + + HookContext context = HookContext.builder() + .flagKey("test-flag") + .type(FlagValueType.STRING) + .defaultValue("default") + .ctx(new ImmutableContext()) + .hookData(hookData) + .build(); + + assertNotNull(context.getHookData()); + assertEquals("value", context.getHookData().get("test")); + } + + @Test + void shouldCreateHookContextWithoutHookData() { + HookContext context = HookContext.builder() + .flagKey("test-flag") + .type(FlagValueType.STRING) + .defaultValue("default") + .ctx(new ImmutableContext()) + .build(); + + assertNull(context.getHookData()); + } + + @Test + void shouldCreateHookContextWithHookDataUsingWith() { + HookContext originalContext = HookContext.builder() + .flagKey("test-flag") + .type(FlagValueType.STRING) + .defaultValue("default") + .ctx(new ImmutableContext()) + .build(); + + HookData hookData = HookData.create(); + hookData.set("timing", System.currentTimeMillis()); + + HookContext contextWithHookData = originalContext.withHookData(hookData); + + assertNull(originalContext.getHookData()); + assertNotNull(contextWithHookData.getHookData()); + assertNotNull(contextWithHookData.getHookData().get("timing")); + } } diff --git a/src/test/java/dev/openfeature/sdk/HookDataTest.java b/src/test/java/dev/openfeature/sdk/HookDataTest.java new file mode 100644 index 000000000..d9e3dcc59 --- /dev/null +++ b/src/test/java/dev/openfeature/sdk/HookDataTest.java @@ -0,0 +1,112 @@ +package dev.openfeature.sdk; + +import static org.junit.jupiter.api.Assertions.*; + +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.TimeUnit; +import org.junit.jupiter.api.Test; + +class HookDataTest { + + @Test + void shouldStoreAndRetrieveValues() { + HookData hookData = HookData.create(); + + hookData.set("key1", "value1"); + hookData.set("key2", 42); + hookData.set("key3", true); + + assertEquals("value1", hookData.get("key1")); + assertEquals(42, hookData.get("key2")); + assertEquals(true, hookData.get("key3")); + } + + @Test + void shouldReturnNullForMissingKeys() { + HookData hookData = HookData.create(); + + assertNull(hookData.get("nonexistent")); + } + + @Test + void shouldSupportTypeSafeRetrieval() { + HookData hookData = HookData.create(); + + hookData.set("string", "hello"); + hookData.set("integer", 123); + hookData.set("boolean", false); + + assertEquals("hello", hookData.get("string", String.class)); + assertEquals(Integer.valueOf(123), hookData.get("integer", Integer.class)); + assertEquals(Boolean.FALSE, hookData.get("boolean", Boolean.class)); + } + + @Test + void shouldReturnNullForMissingKeysWithType() { + HookData hookData = HookData.create(); + + assertNull(hookData.get("missing", String.class)); + } + + @Test + void shouldThrowClassCastExceptionForWrongType() { + HookData hookData = HookData.create(); + + hookData.set("string", "not a number"); + + assertThrows(ClassCastException.class, () -> { + hookData.get("string", Integer.class); + }); + } + + @Test + void shouldOverwriteExistingValues() { + HookData hookData = HookData.create(); + + hookData.set("key", "original"); + assertEquals("original", hookData.get("key")); + + hookData.set("key", "updated"); + assertEquals("updated", hookData.get("key")); + } + + @Test + void shouldBeThreadSafe() throws InterruptedException { + HookData hookData = HookData.create(); + int threadCount = 10; + int operationsPerThread = 100; + CountDownLatch latch = new CountDownLatch(threadCount); + ExecutorService executor = Executors.newFixedThreadPool(threadCount); + + for (int i = 0; i < threadCount; i++) { + final int threadId = i; + executor.submit(() -> { + try { + for (int j = 0; j < operationsPerThread; j++) { + String key = "thread-" + threadId + "-key-" + j; + String value = "thread-" + threadId + "-value-" + j; + hookData.set(key, value); + assertEquals(value, hookData.get(key)); + } + } finally { + latch.countDown(); + } + }); + } + + assertTrue(latch.await(10, TimeUnit.SECONDS)); + executor.shutdown(); + } + + @Test + void shouldSupportNullValues() { + HookData hookData = HookData.create(); + + hookData.set("nullKey", null); + assertNull(hookData.get("nullKey")); + assertNull(hookData.get("nullKey", String.class)); + } +} + diff --git a/src/test/java/dev/openfeature/sdk/HookSupportTest.java b/src/test/java/dev/openfeature/sdk/HookSupportTest.java index 02a8ff90c..2cf220de9 100644 --- a/src/test/java/dev/openfeature/sdk/HookSupportTest.java +++ b/src/test/java/dev/openfeature/sdk/HookSupportTest.java @@ -23,8 +23,14 @@ void shouldMergeEvaluationContextsOnBeforeHooksCorrectly() { Map attributes = new HashMap<>(); attributes.put("baseKey", new Value("baseValue")); EvaluationContext baseContext = new ImmutableContext(attributes); - HookContext hookContext = new HookContext<>( - "flagKey", FlagValueType.STRING, "defaultValue", baseContext, () -> "client", () -> "provider"); + HookContext hookContext = HookContext.builder() + .flagKey("flagKey") + .type(FlagValueType.STRING) + .defaultValue("defaultValue") + .ctx(baseContext) + .clientMetadata(() -> "client") + .providerMetadata(() -> "provider") + .build(); Hook hook1 = mockStringHook(); Hook hook2 = mockStringHook(); when(hook1.before(any(), any())).thenReturn(Optional.of(evaluationContextWithValue("bla", "blubber"))); @@ -47,13 +53,14 @@ void shouldAlwaysCallGenericHook(FlagValueType flagValueType) { HookSupport hookSupport = new HookSupport(); EvaluationContext baseContext = new ImmutableContext(); IllegalStateException expectedException = new IllegalStateException("All fine, just a test"); - HookContext hookContext = new HookContext<>( - "flagKey", - flagValueType, - createDefaultValue(flagValueType), - baseContext, - () -> "client", - () -> "provider"); + HookContext hookContext = HookContext.builder() + .flagKey("flagKey") + .type(flagValueType) + .defaultValue(createDefaultValue(flagValueType)) + .ctx(baseContext) + .clientMetadata(() -> "client") + .providerMetadata(() -> "provider") + .build(); hookSupport.beforeHooks( flagValueType, hookContext, Collections.singletonList(genericHook), Collections.emptyMap()); @@ -105,4 +112,32 @@ private EvaluationContext evaluationContextWithValue(String key, String value) { EvaluationContext baseContext = new ImmutableContext(attributes); return baseContext; } + + private static class TestHook implements Hook { + boolean beforeCalled = false; + boolean afterCalled = false; + boolean errorCalled = false; + boolean finallyCalled = false; + + @Override + public Optional before(HookContext ctx, Map hints) { + beforeCalled = true; + return Optional.empty(); + } + + @Override + public void after(HookContext ctx, FlagEvaluationDetails details, Map hints) { + afterCalled = true; + } + + @Override + public void error(HookContext ctx, Exception error, Map hints) { + errorCalled = true; + } + + @Override + public void finallyAfter(HookContext ctx, FlagEvaluationDetails details, Map hints) { + finallyCalled = true; + } + } } From 549e97cd9fba8aef6b67d433a93077a96c24f11d Mon Sep 17 00:00:00 2001 From: mdxabu Date: Sun, 6 Jul 2025 17:37:06 +0530 Subject: [PATCH 02/19] to support null values while maintaining thread safety, you must avoid ConcurrentHashMap and use a synchronized HashMap instead Signed-off-by: mdxabu --- src/main/java/dev/openfeature/sdk/HookData.java | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/main/java/dev/openfeature/sdk/HookData.java b/src/main/java/dev/openfeature/sdk/HookData.java index 5c22ea799..a7429cc78 100644 --- a/src/main/java/dev/openfeature/sdk/HookData.java +++ b/src/main/java/dev/openfeature/sdk/HookData.java @@ -1,5 +1,8 @@ package dev.openfeature.sdk; +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; @@ -47,8 +50,8 @@ static HookData create() { /** * Default thread-safe implementation of HookData. */ - class DefaultHookData implements HookData { - private final ConcurrentMap data = new ConcurrentHashMap<>(); + public class DefaultHookData implements HookData { + private final Map data = Collections.synchronizedMap(new HashMap<>()); @Override public void set(String key, Object value) { From c06c679967be28dfb1746cfe4d68d2477f595cfe Mon Sep 17 00:00:00 2001 From: mdxabu Date: Tue, 8 Jul 2025 15:35:52 +0530 Subject: [PATCH 03/19] Build successfully runned:spotless plugin Signed-off-by: mdxabu --- src/main/java/dev/openfeature/sdk/HookData.java | 2 -- src/main/java/dev/openfeature/sdk/HookSupport.java | 4 ++-- src/test/java/dev/openfeature/sdk/HookDataTest.java | 1 - src/test/java/dev/openfeature/sdk/HookSupportTest.java | 3 ++- 4 files changed, 4 insertions(+), 6 deletions(-) diff --git a/src/main/java/dev/openfeature/sdk/HookData.java b/src/main/java/dev/openfeature/sdk/HookData.java index a7429cc78..e952992d1 100644 --- a/src/main/java/dev/openfeature/sdk/HookData.java +++ b/src/main/java/dev/openfeature/sdk/HookData.java @@ -3,8 +3,6 @@ import java.util.Collections; import java.util.HashMap; import java.util.Map; -import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.ConcurrentMap; /** * Hook data provides a way for hooks to maintain state across their execution stages. diff --git a/src/main/java/dev/openfeature/sdk/HookSupport.java b/src/main/java/dev/openfeature/sdk/HookSupport.java index df6115c57..15cfd03c3 100644 --- a/src/main/java/dev/openfeature/sdk/HookSupport.java +++ b/src/main/java/dev/openfeature/sdk/HookSupport.java @@ -101,8 +101,8 @@ private EvaluationContext callBeforeHooks( if (hook.supportsFlagValueType(flagValueType)) { // Create a new context with this hook's data HookContext contextWithHookData = hookCtx.withHookData(hookDataMap.get(hook)); - Optional optional = - Optional.ofNullable(hook.before(contextWithHookData, hints)).orElse(Optional.empty()); + Optional optional = Optional.ofNullable(hook.before(contextWithHookData, hints)) + .orElse(Optional.empty()); if (optional.isPresent()) { context = context.merge(optional.get()); } diff --git a/src/test/java/dev/openfeature/sdk/HookDataTest.java b/src/test/java/dev/openfeature/sdk/HookDataTest.java index d9e3dcc59..cc748fe54 100644 --- a/src/test/java/dev/openfeature/sdk/HookDataTest.java +++ b/src/test/java/dev/openfeature/sdk/HookDataTest.java @@ -109,4 +109,3 @@ void shouldSupportNullValues() { assertNull(hookData.get("nullKey", String.class)); } } - diff --git a/src/test/java/dev/openfeature/sdk/HookSupportTest.java b/src/test/java/dev/openfeature/sdk/HookSupportTest.java index 2cf220de9..8b51ea1cc 100644 --- a/src/test/java/dev/openfeature/sdk/HookSupportTest.java +++ b/src/test/java/dev/openfeature/sdk/HookSupportTest.java @@ -136,7 +136,8 @@ public void error(HookContext ctx, Exception error, Map } @Override - public void finallyAfter(HookContext ctx, FlagEvaluationDetails details, Map hints) { + public void finallyAfter( + HookContext ctx, FlagEvaluationDetails details, Map hints) { finallyCalled = true; } } From ce1a60b35f4928e15448957db9dd3020f6a5ac1b Mon Sep 17 00:00:00 2001 From: Simon Schrottner Date: Tue, 9 Sep 2025 17:31:24 +0200 Subject: [PATCH 04/19] fixup: fix comment Co-authored-by: alexandraoberaigner <82218944+alexandraoberaigner@users.noreply.github.com> Signed-off-by: Simon Schrottner --- src/main/java/dev/openfeature/sdk/HookData.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/dev/openfeature/sdk/HookData.java b/src/main/java/dev/openfeature/sdk/HookData.java index e952992d1..52299185e 100644 --- a/src/main/java/dev/openfeature/sdk/HookData.java +++ b/src/main/java/dev/openfeature/sdk/HookData.java @@ -39,7 +39,7 @@ public interface HookData { T get(String key, Class type); /** - * Default implementation using ConcurrentHashMap for thread safety. + * Default implementation is using a synchronized map. */ static HookData create() { return new DefaultHookData(); From bc883c5eb558519c992c9a619413a24a5c133186 Mon Sep 17 00:00:00 2001 From: Simon Schrottner Date: Tue, 9 Sep 2025 17:50:26 +0200 Subject: [PATCH 05/19] fixup: remove synchronized map Co-authored-by: alexandraoberaigner <82218944+alexandraoberaigner@users.noreply.github.com> Signed-off-by: Simon Schrottner --- src/main/java/dev/openfeature/sdk/HookData.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/dev/openfeature/sdk/HookData.java b/src/main/java/dev/openfeature/sdk/HookData.java index 52299185e..81aab52c8 100644 --- a/src/main/java/dev/openfeature/sdk/HookData.java +++ b/src/main/java/dev/openfeature/sdk/HookData.java @@ -49,7 +49,7 @@ static HookData create() { * Default thread-safe implementation of HookData. */ public class DefaultHookData implements HookData { - private final Map data = Collections.synchronizedMap(new HashMap<>()); + private final Map data = new HashMap<>(); @Override public void set(String key, Object value) { From 1339f2fff6e86a7c00933da92a43f660e981137c Mon Sep 17 00:00:00 2001 From: Simon Schrottner Date: Tue, 9 Sep 2025 17:54:09 +0200 Subject: [PATCH 06/19] fixup: removed unneeded import Signed-off-by: Simon Schrottner --- src/main/java/dev/openfeature/sdk/HookData.java | 1 - 1 file changed, 1 deletion(-) diff --git a/src/main/java/dev/openfeature/sdk/HookData.java b/src/main/java/dev/openfeature/sdk/HookData.java index 81aab52c8..4709d6abf 100644 --- a/src/main/java/dev/openfeature/sdk/HookData.java +++ b/src/main/java/dev/openfeature/sdk/HookData.java @@ -1,6 +1,5 @@ package dev.openfeature.sdk; -import java.util.Collections; import java.util.HashMap; import java.util.Map; From c66e2c875f6f0f859925baf977ec98c37b733336 Mon Sep 17 00:00:00 2001 From: Simon Schrottner Date: Tue, 9 Sep 2025 18:51:30 +0200 Subject: [PATCH 07/19] fixup: spotless Signed-off-by: Simon Schrottner --- src/main/java/dev/openfeature/sdk/HookData.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/dev/openfeature/sdk/HookData.java b/src/main/java/dev/openfeature/sdk/HookData.java index 4709d6abf..edd55eede 100644 --- a/src/main/java/dev/openfeature/sdk/HookData.java +++ b/src/main/java/dev/openfeature/sdk/HookData.java @@ -38,7 +38,7 @@ public interface HookData { T get(String key, Class type); /** - * Default implementation is using a synchronized map. + * Default implementation is using a synchronized map. */ static HookData create() { return new DefaultHookData(); From d9ebe63c33e423cd5d46676fd3374f4b5a7c3dfd Mon Sep 17 00:00:00 2001 From: Abu Date: Wed, 10 Sep 2025 12:01:40 +0530 Subject: [PATCH 08/19] Update src/main/java/dev/openfeature/sdk/HookData.java Co-authored-by: chrfwow Signed-off-by: Abu --- src/main/java/dev/openfeature/sdk/HookData.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/dev/openfeature/sdk/HookData.java b/src/main/java/dev/openfeature/sdk/HookData.java index edd55eede..e0d941045 100644 --- a/src/main/java/dev/openfeature/sdk/HookData.java +++ b/src/main/java/dev/openfeature/sdk/HookData.java @@ -38,7 +38,7 @@ public interface HookData { T get(String key, Class type); /** - * Default implementation is using a synchronized map. + * Default implementation uses a HashMap. */ static HookData create() { return new DefaultHookData(); From f3ea94dfa749bf217a6b729d427cc5451af78304 Mon Sep 17 00:00:00 2001 From: Abu Date: Wed, 10 Sep 2025 12:01:54 +0530 Subject: [PATCH 09/19] Update src/main/java/dev/openfeature/sdk/HookData.java Co-authored-by: chrfwow Signed-off-by: Abu --- src/main/java/dev/openfeature/sdk/HookData.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/dev/openfeature/sdk/HookData.java b/src/main/java/dev/openfeature/sdk/HookData.java index e0d941045..16b2726d0 100644 --- a/src/main/java/dev/openfeature/sdk/HookData.java +++ b/src/main/java/dev/openfeature/sdk/HookData.java @@ -45,7 +45,7 @@ static HookData create() { } /** - * Default thread-safe implementation of HookData. + * Default implementation of HookData. */ public class DefaultHookData implements HookData { private final Map data = new HashMap<>(); From 81f2e4f11dc01f55514c8c2b46e0152bb70f836d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexandra=C2=A0Oberaigner?= Date: Wed, 10 Sep 2025 20:42:40 +0200 Subject: [PATCH 10/19] feat: add support for hook data sharing between stages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Alexandra Oberaigner --- .../java/dev/openfeature/sdk/HookSupport.java | 66 ++++++--- .../openfeature/sdk/OpenFeatureClient.java | 16 +- .../dev/openfeature/sdk/HookSupportTest.java | 138 +++++++++++++++--- 3 files changed, 171 insertions(+), 49 deletions(-) diff --git a/src/main/java/dev/openfeature/sdk/HookSupport.java b/src/main/java/dev/openfeature/sdk/HookSupport.java index 15cfd03c3..748e90e28 100644 --- a/src/main/java/dev/openfeature/sdk/HookSupport.java +++ b/src/main/java/dev/openfeature/sdk/HookSupport.java @@ -6,9 +6,11 @@ import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.function.BiConsumer; import java.util.function.Consumer; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; +import org.apache.commons.lang3.tuple.Pair; @Slf4j @RequiredArgsConstructor @@ -16,52 +18,63 @@ class HookSupport { public EvaluationContext beforeHooks( - FlagValueType flagValueType, HookContext hookCtx, List hooks, Map hints) { - return callBeforeHooks(flagValueType, hookCtx, hooks, hints); + FlagValueType flagValueType, HookContext hookCtx, List> hookDataPairs, Map hints) { + return callBeforeHooks(flagValueType, hookCtx, hookDataPairs, hints); } public void afterHooks( FlagValueType flagValueType, HookContext hookContext, FlagEvaluationDetails details, - List hooks, + List> hookDataPairs, Map hints) { - executeHooksUnchecked(flagValueType, hooks, hook -> hook.after(hookContext, details, hints)); + executeHooksUnchecked(flagValueType, hookDataPairs, hookContext, (hook, ctx) -> hook.after(ctx, details, hints)); } public void afterAllHooks( FlagValueType flagValueType, HookContext hookCtx, FlagEvaluationDetails details, - List hooks, + List> hookDataPairs, Map hints) { - executeHooks(flagValueType, hooks, "finally", hook -> hook.finallyAfter(hookCtx, details, hints)); + executeHooks(flagValueType, hookDataPairs, hookCtx, "finally", (hook, ctx) -> hook.finallyAfter(ctx, details, hints)); } public void errorHooks( FlagValueType flagValueType, HookContext hookCtx, Exception e, - List hooks, + List> hookDataPairs, Map hints) { - executeHooks(flagValueType, hooks, "error", hook -> hook.error(hookCtx, e, hints)); + executeHooks(flagValueType, hookDataPairs, hookCtx, "error", (hook, ctx) -> hook.error(ctx, e, hints)); + } + + public List> getHookDataPairs(List hooks) { + var pairs = new ArrayList>(); + for (Hook hook : hooks) { + pairs.add(Pair.of(hook, HookData.create())); + } + return pairs; } private void executeHooks( - FlagValueType flagValueType, List hooks, String hookMethod, Consumer> hookCode) { - if (hooks != null) { - for (Hook hook : hooks) { + FlagValueType flagValueType, List> hookDataPairs, HookContext hookContext, String hookMethod, BiConsumer, HookContext> hookCode) { + if (hookDataPairs != null) { + for (Pair hookDataPair : hookDataPairs) { + Hook hook = hookDataPair.getLeft(); + HookData hookData = hookDataPair.getRight(); if (hook.supportsFlagValueType(flagValueType)) { - executeChecked(hook, hookCode, hookMethod); + executeChecked(hook, hookData, hookContext, hookCode, hookMethod); } } } } // before, error, and finally hooks shouldn't throw - private void executeChecked(Hook hook, Consumer> hookCode, String hookMethod) { + private void executeChecked(Hook hook, HookData hookData, HookContext hookContext, BiConsumer, HookContext> hookCode, String hookMethod) { try { - hookCode.accept(hook); + var hookCtxWithData = hookContext.withHookData(hookData); + hookCode.accept(hook, hookCtxWithData); } catch (Exception exception) { log.error( "Unhandled exception when running {} hook {} (only 'after' hooks should throw)", @@ -72,23 +85,26 @@ private void executeChecked(Hook hook, Consumer> hookCode, String } // after hooks can throw in order to do validation - private void executeHooksUnchecked(FlagValueType flagValueType, List hooks, Consumer> hookCode) { - if (hooks != null) { - for (Hook hook : hooks) { + private void executeHooksUnchecked(FlagValueType flagValueType, List> hookDataPairs, HookContext hookContext, BiConsumer, HookContext> hookCode) { + if (hookDataPairs != null) { + for (Pair hookDataPair : hookDataPairs) { + Hook hook = hookDataPair.getLeft(); + HookData hookData = hookDataPair.getRight(); if (hook.supportsFlagValueType(flagValueType)) { - hookCode.accept(hook); + var hookCtxWithData = hookContext.withHookData(hookData); + hookCode.accept(hook, hookCtxWithData); } } } } private EvaluationContext callBeforeHooks( - FlagValueType flagValueType, HookContext hookCtx, List hooks, Map hints) { + FlagValueType flagValueType, HookContext hookCtx, List> hookDataPairs, Map hints) { // These traverse backwards from normal. - List reversedHooks = new ArrayList<>(hooks); + List> reversedHooks = new ArrayList<>(hookDataPairs); Collections.reverse(reversedHooks); EvaluationContext context = hookCtx.getCtx(); - + /* // Create hook data for each hook instance Map hookDataMap = new HashMap<>(); for (Hook hook : reversedHooks) { @@ -96,11 +112,15 @@ private EvaluationContext callBeforeHooks( hookDataMap.put(hook, HookData.create()); } } + */ + + for (Pair hookDataPair : reversedHooks) { + Hook hook = hookDataPair.getLeft(); + HookData hookData = hookDataPair.getRight(); - for (Hook hook : reversedHooks) { if (hook.supportsFlagValueType(flagValueType)) { // Create a new context with this hook's data - HookContext contextWithHookData = hookCtx.withHookData(hookDataMap.get(hook)); + HookContext contextWithHookData = hookCtx.withHookData(hookData); Optional optional = Optional.ofNullable(hook.before(contextWithHookData, hints)) .orElse(Optional.empty()); if (optional.isPresent()) { diff --git a/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java b/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java index b5522b66a..785602089 100644 --- a/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java +++ b/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java @@ -19,6 +19,7 @@ import java.util.function.Consumer; import lombok.Getter; import lombok.extern.slf4j.Slf4j; +import org.apache.commons.lang3.tuple.Pair; /** * OpenFeature Client implementation. @@ -164,7 +165,8 @@ private FlagEvaluationDetails evaluateFlag( var hints = Collections.unmodifiableMap(flagOptions.getHookHints()); FlagEvaluationDetails details = null; - List mergedHooks = null; + List mergedHooks; + List> hookDataPairs = null; HookContext afterHookContext = null; try { @@ -175,7 +177,7 @@ private FlagEvaluationDetails evaluateFlag( mergedHooks = ObjectUtils.merge( provider.getProviderHooks(), flagOptions.getHooks(), clientHooks, openfeatureApi.getMutableHooks()); - + hookDataPairs = hookSupport.getHookDataPairs(mergedHooks); var mergedCtx = hookSupport.beforeHooks( type, HookContext.from( @@ -185,7 +187,7 @@ private FlagEvaluationDetails evaluateFlag( provider.getMetadata(), mergeEvaluationContext(ctx), defaultValue), - mergedHooks, + hookDataPairs, hints); afterHookContext = @@ -207,9 +209,9 @@ private FlagEvaluationDetails evaluateFlag( var error = ExceptionUtils.instantiateErrorByErrorCode(details.getErrorCode(), details.getErrorMessage()); enrichDetailsWithErrorDefaults(defaultValue, details); - hookSupport.errorHooks(type, afterHookContext, error, mergedHooks, hints); + hookSupport.errorHooks(type, afterHookContext, error, hookDataPairs, hints); } else { - hookSupport.afterHooks(type, afterHookContext, details, mergedHooks, hints); + hookSupport.afterHooks(type, afterHookContext, details, hookDataPairs, hints); } } catch (Exception e) { if (details == null) { @@ -222,9 +224,9 @@ private FlagEvaluationDetails evaluateFlag( } details.setErrorMessage(e.getMessage()); enrichDetailsWithErrorDefaults(defaultValue, details); - hookSupport.errorHooks(type, afterHookContext, e, mergedHooks, hints); + hookSupport.errorHooks(type, afterHookContext, e, hookDataPairs, hints); } finally { - hookSupport.afterAllHooks(type, afterHookContext, details, mergedHooks, hints); + hookSupport.afterAllHooks(type, afterHookContext, details, hookDataPairs, hints); } return details; diff --git a/src/test/java/dev/openfeature/sdk/HookSupportTest.java b/src/test/java/dev/openfeature/sdk/HookSupportTest.java index 8b51ea1cc..2985c9c7f 100644 --- a/src/test/java/dev/openfeature/sdk/HookSupportTest.java +++ b/src/test/java/dev/openfeature/sdk/HookSupportTest.java @@ -9,8 +9,10 @@ import java.util.Arrays; import java.util.Collections; import java.util.HashMap; +import java.util.List; import java.util.Map; import java.util.Optional; +import org.apache.commons.lang3.tuple.Pair; import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; @@ -38,7 +40,7 @@ void shouldMergeEvaluationContextsOnBeforeHooksCorrectly() { HookSupport hookSupport = new HookSupport(); EvaluationContext result = hookSupport.beforeHooks( - FlagValueType.STRING, hookContext, Arrays.asList(hook1, hook2), Collections.emptyMap()); + FlagValueType.STRING, hookContext, hookSupport.getHookDataPairs(Arrays.asList(hook1, hook2)), Collections.emptyMap()); assertThat(result.getValue("bla").asString()).isEqualTo("blubber"); assertThat(result.getValue("foo").asString()).isEqualTo("bar"); @@ -51,6 +53,7 @@ void shouldMergeEvaluationContextsOnBeforeHooksCorrectly() { void shouldAlwaysCallGenericHook(FlagValueType flagValueType) { Hook genericHook = mockGenericHook(); HookSupport hookSupport = new HookSupport(); + var hookDataPairs = hookSupport.getHookDataPairs(Collections.singletonList(genericHook)); EvaluationContext baseContext = new ImmutableContext(); IllegalStateException expectedException = new IllegalStateException("All fine, just a test"); HookContext hookContext = HookContext.builder() @@ -63,24 +66,24 @@ void shouldAlwaysCallGenericHook(FlagValueType flagValueType) { .build(); hookSupport.beforeHooks( - flagValueType, hookContext, Collections.singletonList(genericHook), Collections.emptyMap()); + flagValueType, hookContext, hookDataPairs, Collections.emptyMap()); hookSupport.afterHooks( flagValueType, hookContext, FlagEvaluationDetails.builder().build(), - Collections.singletonList(genericHook), + hookDataPairs, Collections.emptyMap()); hookSupport.afterAllHooks( flagValueType, hookContext, FlagEvaluationDetails.builder().build(), - Collections.singletonList(genericHook), + hookDataPairs, Collections.emptyMap()); hookSupport.errorHooks( flagValueType, hookContext, expectedException, - Collections.singletonList(genericHook), + hookDataPairs, Collections.emptyMap()); verify(genericHook).before(any(), any()); @@ -89,6 +92,89 @@ void shouldAlwaysCallGenericHook(FlagValueType flagValueType) { verify(genericHook).error(any(), any(), any()); } + @ParameterizedTest + @EnumSource(value = FlagValueType.class) + @DisplayName("should allow hooks to store and retrieve data across stages") + void shouldPassDataAcrossStages(FlagValueType flagValueType) { + HookSupport hookSupport = new HookSupport(); + HookContext hookContext = getObjectHookContext(flagValueType); + + TestHookWithData testHook = new TestHookWithData("test-key", "value"); + var pairs = hookSupport.getHookDataPairs(List.of(testHook)); + + callAllHooks(flagValueType, hookSupport, hookContext, pairs); + + assertHookData(testHook, "value"); + } + + @ParameterizedTest + @EnumSource(value = FlagValueType.class) + @DisplayName("should isolate data between different hook instances") + void shouldIsolateDataBetweenHooks(FlagValueType flagValueType) { + HookSupport hookSupport = new HookSupport(); + HookContext hookContext = getObjectHookContext(flagValueType); + + TestHookWithData testHook1 = new TestHookWithData("test-key", "value-1"); + TestHookWithData testHook2 = new TestHookWithData("test-key", "value-2"); + var pairs = hookSupport.getHookDataPairs(List.of(testHook1, testHook2)); + + callAllHooks(flagValueType, hookSupport, hookContext, pairs); + + assertHookData(testHook1, "value-1"); + assertHookData(testHook2, "value-2"); + } + + @ParameterizedTest + @EnumSource(value = FlagValueType.class) + @DisplayName("should isolate data between the same hook instances") + void shouldIsolateDataBetweenSameHooks(FlagValueType flagValueType) { + + HookSupport hookSupport = new HookSupport(); + HookContext hookContext = getObjectHookContext(flagValueType); + + TestHookWithData testHook = new TestHookWithData("test-key", "value-1"); + + // run hooks first time + var pairs = hookSupport.getHookDataPairs(List.of(testHook)); + callAllHooks(flagValueType, hookSupport, hookContext, pairs); + assertHookData(testHook, "value-1"); + + // re-run with different value, will throw if HookData contains already data + testHook.value = "value-2"; + callAllHooks(flagValueType, hookSupport, hookContext, pairs); + + assertHookData(testHook, "value-2"); + } + + private HookContext getObjectHookContext(FlagValueType flagValueType) { + EvaluationContext baseContext = new ImmutableContext(); + HookContext hookContext = HookContext.builder() + .flagKey("flagKey") + .type(flagValueType) + .defaultValue(createDefaultValue(flagValueType)) + .ctx(baseContext) + .clientMetadata(() -> "client") + .providerMetadata(() -> "provider") + .build(); + return hookContext; + } + + private static void assertHookData(TestHookWithData testHook1, String expected) { + assertThat(testHook1.onBeforeValue).isEqualTo(expected); + assertThat(testHook1.onFinallyAfterValue).isEqualTo(expected); + assertThat(testHook1.onAfterValue).isEqualTo(expected); + assertThat(testHook1.onErrorValue).isEqualTo(expected); + } + + private static void callAllHooks(FlagValueType flagValueType, HookSupport hookSupport, HookContext hookContext, + List> pairs) { + hookSupport.beforeHooks(flagValueType, hookContext, pairs, Collections.emptyMap()); + hookSupport.afterHooks(flagValueType, hookContext, new FlagEvaluationDetails<>(), pairs, Collections.emptyMap()); + hookSupport.errorHooks(flagValueType, hookContext, new Exception(), pairs, Collections.emptyMap()); + hookSupport.afterAllHooks(flagValueType, hookContext, new FlagEvaluationDetails<>(), pairs, Collections.emptyMap()); + } + + private Object createDefaultValue(FlagValueType flagValueType) { switch (flagValueType) { case INTEGER: @@ -113,32 +199,46 @@ private EvaluationContext evaluationContextWithValue(String key, String value) { return baseContext; } - private static class TestHook implements Hook { - boolean beforeCalled = false; - boolean afterCalled = false; - boolean errorCalled = false; - boolean finallyCalled = false; + private class TestHookWithData implements Hook { + + private final String key; + Object value; + + Object onBeforeValue; + Object onAfterValue; + Object onErrorValue; + Object onFinallyAfterValue; + + TestHookWithData(String key, Object value) { + this.key = key; + this.value = value; + } @Override - public Optional before(HookContext ctx, Map hints) { - beforeCalled = true; + public Optional before(HookContext ctx, Map hints) { + var storedValue = ctx.getHookData().get(key); + if (storedValue != null) { + throw new Error("Hook data isolation violated! Data is already set."); + } + ctx.getHookData().set(key, value); + onBeforeValue = ctx.getHookData().get(key); return Optional.empty(); } @Override - public void after(HookContext ctx, FlagEvaluationDetails details, Map hints) { - afterCalled = true; + public void after(HookContext ctx, FlagEvaluationDetails details, Map hints) { + onAfterValue = ctx.getHookData().get(key); } @Override - public void error(HookContext ctx, Exception error, Map hints) { - errorCalled = true; + public void error(HookContext ctx, Exception error, Map hints) { + onErrorValue = ctx.getHookData().get(key); } @Override - public void finallyAfter( - HookContext ctx, FlagEvaluationDetails details, Map hints) { - finallyCalled = true; + public void finallyAfter(HookContext ctx, FlagEvaluationDetails details, Map hints) { + onFinallyAfterValue = ctx.getHookData().get(key); } + } } From 6d4f70054154e97d08fb081afa9b5822809896dc Mon Sep 17 00:00:00 2001 From: Simon Schrottner Date: Wed, 10 Sep 2025 21:30:35 +0200 Subject: [PATCH 11/19] fixup: tests Signed-off-by: Simon Schrottner --- .../java/dev/openfeature/sdk/HookSupport.java | 41 +++++++++++++---- .../openfeature/sdk/OpenFeatureClient.java | 21 +++------ .../dev/openfeature/sdk/HookSupportTest.java | 44 +++++++++++-------- 3 files changed, 64 insertions(+), 42 deletions(-) diff --git a/src/main/java/dev/openfeature/sdk/HookSupport.java b/src/main/java/dev/openfeature/sdk/HookSupport.java index 748e90e28..1a169a303 100644 --- a/src/main/java/dev/openfeature/sdk/HookSupport.java +++ b/src/main/java/dev/openfeature/sdk/HookSupport.java @@ -2,12 +2,10 @@ import java.util.ArrayList; import java.util.Collections; -import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Optional; import java.util.function.BiConsumer; -import java.util.function.Consumer; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; import org.apache.commons.lang3.tuple.Pair; @@ -18,7 +16,10 @@ class HookSupport { public EvaluationContext beforeHooks( - FlagValueType flagValueType, HookContext hookCtx, List> hookDataPairs, Map hints) { + FlagValueType flagValueType, + HookContext hookCtx, + List> hookDataPairs, + Map hints) { return callBeforeHooks(flagValueType, hookCtx, hookDataPairs, hints); } @@ -28,7 +29,8 @@ public void afterHooks( FlagEvaluationDetails details, List> hookDataPairs, Map hints) { - executeHooksUnchecked(flagValueType, hookDataPairs, hookContext, (hook, ctx) -> hook.after(ctx, details, hints)); + executeHooksUnchecked( + flagValueType, hookDataPairs, hookContext, (hook, ctx) -> hook.after(ctx, details, hints)); } public void afterAllHooks( @@ -37,7 +39,12 @@ public void afterAllHooks( FlagEvaluationDetails details, List> hookDataPairs, Map hints) { - executeHooks(flagValueType, hookDataPairs, hookCtx, "finally", (hook, ctx) -> hook.finallyAfter(ctx, details, hints)); + executeHooks( + flagValueType, + hookDataPairs, + hookCtx, + "finally", + (hook, ctx) -> hook.finallyAfter(ctx, details, hints)); } public void errorHooks( @@ -58,7 +65,11 @@ public List> getHookDataPairs(List hooks) { } private void executeHooks( - FlagValueType flagValueType, List> hookDataPairs, HookContext hookContext, String hookMethod, BiConsumer, HookContext> hookCode) { + FlagValueType flagValueType, + List> hookDataPairs, + HookContext hookContext, + String hookMethod, + BiConsumer, HookContext> hookCode) { if (hookDataPairs != null) { for (Pair hookDataPair : hookDataPairs) { Hook hook = hookDataPair.getLeft(); @@ -71,7 +82,12 @@ private void executeHooks( } // before, error, and finally hooks shouldn't throw - private void executeChecked(Hook hook, HookData hookData, HookContext hookContext, BiConsumer, HookContext> hookCode, String hookMethod) { + private void executeChecked( + Hook hook, + HookData hookData, + HookContext hookContext, + BiConsumer, HookContext> hookCode, + String hookMethod) { try { var hookCtxWithData = hookContext.withHookData(hookData); hookCode.accept(hook, hookCtxWithData); @@ -85,7 +101,11 @@ private void executeChecked(Hook hook, HookData hookData, HookContext hoo } // after hooks can throw in order to do validation - private void executeHooksUnchecked(FlagValueType flagValueType, List> hookDataPairs, HookContext hookContext, BiConsumer, HookContext> hookCode) { + private void executeHooksUnchecked( + FlagValueType flagValueType, + List> hookDataPairs, + HookContext hookContext, + BiConsumer, HookContext> hookCode) { if (hookDataPairs != null) { for (Pair hookDataPair : hookDataPairs) { Hook hook = hookDataPair.getLeft(); @@ -99,7 +119,10 @@ private void executeHooksUnchecked(FlagValueType flagValueType, List> hookDataPairs, Map hints) { + FlagValueType flagValueType, + HookContext hookCtx, + List> hookDataPairs, + Map hints) { // These traverse backwards from normal. List> reversedHooks = new ArrayList<>(hookDataPairs); Collections.reverse(reversedHooks); diff --git a/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java b/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java index 785602089..cfed14ef1 100644 --- a/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java +++ b/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java @@ -170,25 +170,16 @@ private FlagEvaluationDetails evaluateFlag( HookContext afterHookContext = null; try { - var stateManager = openfeatureApi.getFeatureProviderStateManager(this.domain); + final var stateManager = openfeatureApi.getFeatureProviderStateManager(this.domain); // provider must be accessed once to maintain a consistent reference - var provider = stateManager.getProvider(); - var state = stateManager.getState(); - + final var provider = stateManager.getProvider(); + final var state = stateManager.getState(); + afterHookContext = HookContext.from( + key, type, this.getMetadata(), provider.getMetadata(), mergeEvaluationContext(ctx), defaultValue); mergedHooks = ObjectUtils.merge( provider.getProviderHooks(), flagOptions.getHooks(), clientHooks, openfeatureApi.getMutableHooks()); hookDataPairs = hookSupport.getHookDataPairs(mergedHooks); - var mergedCtx = hookSupport.beforeHooks( - type, - HookContext.from( - key, - type, - this.getMetadata(), - provider.getMetadata(), - mergeEvaluationContext(ctx), - defaultValue), - hookDataPairs, - hints); + var mergedCtx = hookSupport.beforeHooks(type, afterHookContext, hookDataPairs, hints); afterHookContext = HookContext.from(key, type, this.getMetadata(), provider.getMetadata(), mergedCtx, defaultValue); diff --git a/src/test/java/dev/openfeature/sdk/HookSupportTest.java b/src/test/java/dev/openfeature/sdk/HookSupportTest.java index 2985c9c7f..51fe29e38 100644 --- a/src/test/java/dev/openfeature/sdk/HookSupportTest.java +++ b/src/test/java/dev/openfeature/sdk/HookSupportTest.java @@ -40,7 +40,10 @@ void shouldMergeEvaluationContextsOnBeforeHooksCorrectly() { HookSupport hookSupport = new HookSupport(); EvaluationContext result = hookSupport.beforeHooks( - FlagValueType.STRING, hookContext, hookSupport.getHookDataPairs(Arrays.asList(hook1, hook2)), Collections.emptyMap()); + FlagValueType.STRING, + hookContext, + hookSupport.getHookDataPairs(Arrays.asList(hook1, hook2)), + Collections.emptyMap()); assertThat(result.getValue("bla").asString()).isEqualTo("blubber"); assertThat(result.getValue("foo").asString()).isEqualTo("bar"); @@ -65,8 +68,7 @@ void shouldAlwaysCallGenericHook(FlagValueType flagValueType) { .providerMetadata(() -> "provider") .build(); - hookSupport.beforeHooks( - flagValueType, hookContext, hookDataPairs, Collections.emptyMap()); + hookSupport.beforeHooks(flagValueType, hookContext, hookDataPairs, Collections.emptyMap()); hookSupport.afterHooks( flagValueType, hookContext, @@ -79,12 +81,7 @@ void shouldAlwaysCallGenericHook(FlagValueType flagValueType) { FlagEvaluationDetails.builder().build(), hookDataPairs, Collections.emptyMap()); - hookSupport.errorHooks( - flagValueType, - hookContext, - expectedException, - hookDataPairs, - Collections.emptyMap()); + hookSupport.errorHooks(flagValueType, hookContext, expectedException, hookDataPairs, Collections.emptyMap()); verify(genericHook).before(any(), any()); verify(genericHook).after(any(), any(), any()); @@ -102,7 +99,7 @@ void shouldPassDataAcrossStages(FlagValueType flagValueType) { TestHookWithData testHook = new TestHookWithData("test-key", "value"); var pairs = hookSupport.getHookDataPairs(List.of(testHook)); - callAllHooks(flagValueType, hookSupport, hookContext, pairs); + callAllHooks(flagValueType, hookSupport, hookContext, testHook); assertHookData(testHook, "value"); } @@ -135,13 +132,12 @@ void shouldIsolateDataBetweenSameHooks(FlagValueType flagValueType) { TestHookWithData testHook = new TestHookWithData("test-key", "value-1"); // run hooks first time - var pairs = hookSupport.getHookDataPairs(List.of(testHook)); - callAllHooks(flagValueType, hookSupport, hookContext, pairs); + callAllHooks(flagValueType, hookSupport, hookContext, testHook); assertHookData(testHook, "value-1"); // re-run with different value, will throw if HookData contains already data testHook.value = "value-2"; - callAllHooks(flagValueType, hookSupport, hookContext, pairs); + callAllHooks(flagValueType, hookSupport, hookContext, testHook); assertHookData(testHook, "value-2"); } @@ -166,15 +162,28 @@ private static void assertHookData(TestHookWithData testHook1, String expected) assertThat(testHook1.onErrorValue).isEqualTo(expected); } - private static void callAllHooks(FlagValueType flagValueType, HookSupport hookSupport, HookContext hookContext, + private static void callAllHooks( + FlagValueType flagValueType, + HookSupport hookSupport, + HookContext hookContext, + TestHookWithData testHook) { + var pairs = hookSupport.getHookDataPairs(List.of(testHook)); + callAllHooks(flagValueType, hookSupport, hookContext, pairs); + } + + private static void callAllHooks( + FlagValueType flagValueType, + HookSupport hookSupport, + HookContext hookContext, List> pairs) { hookSupport.beforeHooks(flagValueType, hookContext, pairs, Collections.emptyMap()); - hookSupport.afterHooks(flagValueType, hookContext, new FlagEvaluationDetails<>(), pairs, Collections.emptyMap()); + hookSupport.afterHooks( + flagValueType, hookContext, new FlagEvaluationDetails<>(), pairs, Collections.emptyMap()); hookSupport.errorHooks(flagValueType, hookContext, new Exception(), pairs, Collections.emptyMap()); - hookSupport.afterAllHooks(flagValueType, hookContext, new FlagEvaluationDetails<>(), pairs, Collections.emptyMap()); + hookSupport.afterAllHooks( + flagValueType, hookContext, new FlagEvaluationDetails<>(), pairs, Collections.emptyMap()); } - private Object createDefaultValue(FlagValueType flagValueType) { switch (flagValueType) { case INTEGER: @@ -239,6 +248,5 @@ public void error(HookContext ctx, Exception error, Map hints) { public void finallyAfter(HookContext ctx, FlagEvaluationDetails details, Map hints) { onFinallyAfterValue = ctx.getHookData().get(key); } - } } From a4fc956378dff50d5df69f936dca960aafbc2cb0 Mon Sep 17 00:00:00 2001 From: Simon Schrottner Date: Thu, 11 Sep 2025 11:00:00 +0200 Subject: [PATCH 12/19] fixup: pr comments and extraction of own pair class Signed-off-by: Simon Schrottner --- .../java/dev/openfeature/sdk/HookSupport.java | 16 +++------ .../openfeature/sdk/OpenFeatureClient.java | 19 +++++----- src/main/java/dev/openfeature/sdk/Pair.java | 36 +++++++++++++++++++ .../dev/openfeature/sdk/HookDataTest.java | 32 ----------------- .../dev/openfeature/sdk/HookSupportTest.java | 16 ++++----- 5 files changed, 57 insertions(+), 62 deletions(-) create mode 100644 src/main/java/dev/openfeature/sdk/Pair.java diff --git a/src/main/java/dev/openfeature/sdk/HookSupport.java b/src/main/java/dev/openfeature/sdk/HookSupport.java index 1a169a303..f21499af5 100644 --- a/src/main/java/dev/openfeature/sdk/HookSupport.java +++ b/src/main/java/dev/openfeature/sdk/HookSupport.java @@ -8,7 +8,6 @@ import java.util.function.BiConsumer; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; -import org.apache.commons.lang3.tuple.Pair; @Slf4j @RequiredArgsConstructor @@ -56,10 +55,12 @@ public void errorHooks( executeHooks(flagValueType, hookDataPairs, hookCtx, "error", (hook, ctx) -> hook.error(ctx, e, hints)); } - public List> getHookDataPairs(List hooks) { + public List> getHookDataPairs(List hooks, FlagValueType flagValueType) { var pairs = new ArrayList>(); for (Hook hook : hooks) { - pairs.add(Pair.of(hook, HookData.create())); + if (hook.supportsFlagValueType(flagValueType)) { + pairs.add(Pair.of(hook, HookData.create())); + } } return pairs; } @@ -127,15 +128,6 @@ private EvaluationContext callBeforeHooks( List> reversedHooks = new ArrayList<>(hookDataPairs); Collections.reverse(reversedHooks); EvaluationContext context = hookCtx.getCtx(); - /* - // Create hook data for each hook instance - Map hookDataMap = new HashMap<>(); - for (Hook hook : reversedHooks) { - if (hook.supportsFlagValueType(flagValueType)) { - hookDataMap.put(hook, HookData.create()); - } - } - */ for (Pair hookDataPair : reversedHooks) { Hook hook = hookDataPair.getLeft(); diff --git a/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java b/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java index cfed14ef1..4d21b58f2 100644 --- a/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java +++ b/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java @@ -19,7 +19,6 @@ import java.util.function.Consumer; import lombok.Getter; import lombok.extern.slf4j.Slf4j; -import org.apache.commons.lang3.tuple.Pair; /** * OpenFeature Client implementation. @@ -167,21 +166,21 @@ private FlagEvaluationDetails evaluateFlag( FlagEvaluationDetails details = null; List mergedHooks; List> hookDataPairs = null; - HookContext afterHookContext = null; + HookContext hookContext = null; try { final var stateManager = openfeatureApi.getFeatureProviderStateManager(this.domain); // provider must be accessed once to maintain a consistent reference final var provider = stateManager.getProvider(); final var state = stateManager.getState(); - afterHookContext = HookContext.from( + hookContext = HookContext.from( key, type, this.getMetadata(), provider.getMetadata(), mergeEvaluationContext(ctx), defaultValue); mergedHooks = ObjectUtils.merge( provider.getProviderHooks(), flagOptions.getHooks(), clientHooks, openfeatureApi.getMutableHooks()); - hookDataPairs = hookSupport.getHookDataPairs(mergedHooks); - var mergedCtx = hookSupport.beforeHooks(type, afterHookContext, hookDataPairs, hints); + hookDataPairs = hookSupport.getHookDataPairs(mergedHooks, type); + var mergedCtx = hookSupport.beforeHooks(type, hookContext, hookDataPairs, hints); - afterHookContext = + hookContext = HookContext.from(key, type, this.getMetadata(), provider.getMetadata(), mergedCtx, defaultValue); // "short circuit" if the provider is in NOT_READY or FATAL state @@ -200,9 +199,9 @@ private FlagEvaluationDetails evaluateFlag( var error = ExceptionUtils.instantiateErrorByErrorCode(details.getErrorCode(), details.getErrorMessage()); enrichDetailsWithErrorDefaults(defaultValue, details); - hookSupport.errorHooks(type, afterHookContext, error, hookDataPairs, hints); + hookSupport.errorHooks(type, hookContext, error, hookDataPairs, hints); } else { - hookSupport.afterHooks(type, afterHookContext, details, hookDataPairs, hints); + hookSupport.afterHooks(type, hookContext, details, hookDataPairs, hints); } } catch (Exception e) { if (details == null) { @@ -215,9 +214,9 @@ private FlagEvaluationDetails evaluateFlag( } details.setErrorMessage(e.getMessage()); enrichDetailsWithErrorDefaults(defaultValue, details); - hookSupport.errorHooks(type, afterHookContext, e, hookDataPairs, hints); + hookSupport.errorHooks(type, hookContext, e, hookDataPairs, hints); } finally { - hookSupport.afterAllHooks(type, afterHookContext, details, hookDataPairs, hints); + hookSupport.afterAllHooks(type, hookContext, details, hookDataPairs, hints); } return details; diff --git a/src/main/java/dev/openfeature/sdk/Pair.java b/src/main/java/dev/openfeature/sdk/Pair.java new file mode 100644 index 000000000..5fb03ec79 --- /dev/null +++ b/src/main/java/dev/openfeature/sdk/Pair.java @@ -0,0 +1,36 @@ +package dev.openfeature.sdk; + +class Pair { + private final K key; + private final V value; + + private Pair(K key, V value) { + this.key = key; + this.value = value; + } + + public K getKey() { + return key; + } + + public V getValue() { + return value; + } + + public K getLeft() { + return key; + } + + public V getRight() { + return value; + } + + @Override + public String toString() { + return "Pair{" + "key=" + key + ", value=" + value + '}'; + } + + public static Pair of(K key, V value) { + return new Pair<>(key, value); + } +} diff --git a/src/test/java/dev/openfeature/sdk/HookDataTest.java b/src/test/java/dev/openfeature/sdk/HookDataTest.java index cc748fe54..eacbeeb78 100644 --- a/src/test/java/dev/openfeature/sdk/HookDataTest.java +++ b/src/test/java/dev/openfeature/sdk/HookDataTest.java @@ -2,10 +2,6 @@ import static org.junit.jupiter.api.Assertions.*; -import java.util.concurrent.CountDownLatch; -import java.util.concurrent.ExecutorService; -import java.util.concurrent.Executors; -import java.util.concurrent.TimeUnit; import org.junit.jupiter.api.Test; class HookDataTest { @@ -72,34 +68,6 @@ void shouldOverwriteExistingValues() { assertEquals("updated", hookData.get("key")); } - @Test - void shouldBeThreadSafe() throws InterruptedException { - HookData hookData = HookData.create(); - int threadCount = 10; - int operationsPerThread = 100; - CountDownLatch latch = new CountDownLatch(threadCount); - ExecutorService executor = Executors.newFixedThreadPool(threadCount); - - for (int i = 0; i < threadCount; i++) { - final int threadId = i; - executor.submit(() -> { - try { - for (int j = 0; j < operationsPerThread; j++) { - String key = "thread-" + threadId + "-key-" + j; - String value = "thread-" + threadId + "-value-" + j; - hookData.set(key, value); - assertEquals(value, hookData.get(key)); - } - } finally { - latch.countDown(); - } - }); - } - - assertTrue(latch.await(10, TimeUnit.SECONDS)); - executor.shutdown(); - } - @Test void shouldSupportNullValues() { HookData hookData = HookData.create(); diff --git a/src/test/java/dev/openfeature/sdk/HookSupportTest.java b/src/test/java/dev/openfeature/sdk/HookSupportTest.java index 51fe29e38..85a25facb 100644 --- a/src/test/java/dev/openfeature/sdk/HookSupportTest.java +++ b/src/test/java/dev/openfeature/sdk/HookSupportTest.java @@ -12,7 +12,6 @@ import java.util.List; import java.util.Map; import java.util.Optional; -import org.apache.commons.lang3.tuple.Pair; import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; @@ -25,9 +24,10 @@ void shouldMergeEvaluationContextsOnBeforeHooksCorrectly() { Map attributes = new HashMap<>(); attributes.put("baseKey", new Value("baseValue")); EvaluationContext baseContext = new ImmutableContext(attributes); + FlagValueType valueType = FlagValueType.STRING; HookContext hookContext = HookContext.builder() .flagKey("flagKey") - .type(FlagValueType.STRING) + .type(valueType) .defaultValue("defaultValue") .ctx(baseContext) .clientMetadata(() -> "client") @@ -40,9 +40,9 @@ void shouldMergeEvaluationContextsOnBeforeHooksCorrectly() { HookSupport hookSupport = new HookSupport(); EvaluationContext result = hookSupport.beforeHooks( - FlagValueType.STRING, + valueType, hookContext, - hookSupport.getHookDataPairs(Arrays.asList(hook1, hook2)), + hookSupport.getHookDataPairs(Arrays.asList(hook1, hook2), valueType), Collections.emptyMap()); assertThat(result.getValue("bla").asString()).isEqualTo("blubber"); @@ -56,7 +56,7 @@ void shouldMergeEvaluationContextsOnBeforeHooksCorrectly() { void shouldAlwaysCallGenericHook(FlagValueType flagValueType) { Hook genericHook = mockGenericHook(); HookSupport hookSupport = new HookSupport(); - var hookDataPairs = hookSupport.getHookDataPairs(Collections.singletonList(genericHook)); + var hookDataPairs = hookSupport.getHookDataPairs(Collections.singletonList(genericHook), flagValueType); EvaluationContext baseContext = new ImmutableContext(); IllegalStateException expectedException = new IllegalStateException("All fine, just a test"); HookContext hookContext = HookContext.builder() @@ -97,7 +97,7 @@ void shouldPassDataAcrossStages(FlagValueType flagValueType) { HookContext hookContext = getObjectHookContext(flagValueType); TestHookWithData testHook = new TestHookWithData("test-key", "value"); - var pairs = hookSupport.getHookDataPairs(List.of(testHook)); + var pairs = hookSupport.getHookDataPairs(List.of(testHook), flagValueType); callAllHooks(flagValueType, hookSupport, hookContext, testHook); @@ -113,7 +113,7 @@ void shouldIsolateDataBetweenHooks(FlagValueType flagValueType) { TestHookWithData testHook1 = new TestHookWithData("test-key", "value-1"); TestHookWithData testHook2 = new TestHookWithData("test-key", "value-2"); - var pairs = hookSupport.getHookDataPairs(List.of(testHook1, testHook2)); + var pairs = hookSupport.getHookDataPairs(List.of(testHook1, testHook2), flagValueType); callAllHooks(flagValueType, hookSupport, hookContext, pairs); @@ -167,7 +167,7 @@ private static void callAllHooks( HookSupport hookSupport, HookContext hookContext, TestHookWithData testHook) { - var pairs = hookSupport.getHookDataPairs(List.of(testHook)); + var pairs = hookSupport.getHookDataPairs(List.of(testHook), flagValueType); callAllHooks(flagValueType, hookSupport, hookContext, pairs); } From 0dee9295d07e10fcfba67930a2a99c730dfabb40 Mon Sep 17 00:00:00 2001 From: Simon Schrottner Date: Thu, 11 Sep 2025 11:55:41 +0200 Subject: [PATCH 13/19] feat: add HookContext decorator Signed-off-by: Simon Schrottner --- .../java/dev/openfeature/sdk/HookContext.java | 57 +++++++++---------- .../openfeature/sdk/HookContextWithData.java | 50 ++++++++++++++++ .../sdk/HookContextWithoutData.java | 33 +++++++++++ .../java/dev/openfeature/sdk/HookSupport.java | 6 +- .../dev/openfeature/sdk/HookContextTest.java | 2 +- .../dev/openfeature/sdk/HookSupportTest.java | 2 +- 6 files changed, 115 insertions(+), 35 deletions(-) create mode 100644 src/main/java/dev/openfeature/sdk/HookContextWithData.java create mode 100644 src/main/java/dev/openfeature/sdk/HookContextWithoutData.java diff --git a/src/main/java/dev/openfeature/sdk/HookContext.java b/src/main/java/dev/openfeature/sdk/HookContext.java index b06d2e3fa..37b15d459 100644 --- a/src/main/java/dev/openfeature/sdk/HookContext.java +++ b/src/main/java/dev/openfeature/sdk/HookContext.java @@ -1,38 +1,14 @@ package dev.openfeature.sdk; -import lombok.Builder; -import lombok.NonNull; -import lombok.Value; -import lombok.With; +import dev.openfeature.sdk.HookContextWithoutData.HookContextWithoutDataBuilder; /** - * A data class to hold immutable context that {@link Hook} instances use. + * A interface to hold immutable context that {@link Hook} instances use. * - * @param the type for the flag being evaluated */ -@Value -@Builder(toBuilder = true) -@With -public class HookContext { - @NonNull String flagKey; - - @NonNull FlagValueType type; - - @NonNull T defaultValue; - - @NonNull EvaluationContext ctx; - - ClientMetadata clientMetadata; - Metadata providerMetadata; - +public interface HookContext { /** - * Hook data provides a way for hooks to maintain state across their execution stages. - * Each hook instance gets its own isolated data store. - */ - HookData hookData; - - /** - * Builds a {@link HookContext} instances from request data. + * Builds a {@link HookContextWithoutData} instances from request data. * * @param key feature flag key * @param type flag value type @@ -43,14 +19,14 @@ public class HookContext { * @param type that the flag is evaluating against * @return resulting context for hook */ - public static HookContext from( + static HookContext from( String key, FlagValueType type, ClientMetadata clientMetadata, Metadata providerMetadata, EvaluationContext ctx, T defaultValue) { - return HookContext.builder() + return HookContextWithoutData.builder() .flagKey(key) .type(type) .clientMetadata(clientMetadata) @@ -60,4 +36,25 @@ public static HookContext from( .hookData(null) // Explicitly set to null for backward compatibility .build(); } + + /** + * Returns a builder for our default HookContext object. + */ + static HookContextWithoutDataBuilder builder() { + return HookContextWithoutData.builder(); + } + + String getFlagKey(); + + FlagValueType getType(); + + T getDefaultValue(); + + EvaluationContext getCtx(); + + ClientMetadata getClientMetadata(); + + Metadata getProviderMetadata(); + + HookData getHookData(); } diff --git a/src/main/java/dev/openfeature/sdk/HookContextWithData.java b/src/main/java/dev/openfeature/sdk/HookContextWithData.java new file mode 100644 index 000000000..137477c11 --- /dev/null +++ b/src/main/java/dev/openfeature/sdk/HookContextWithData.java @@ -0,0 +1,50 @@ +package dev.openfeature.sdk; + +class HookContextWithData implements HookContext { + private final HookContext context; + private final HookData data; + + private HookContextWithData(HookContext context, HookData data) { + this.context = context; + this.data = data; + } + + public static HookContextWithData of(HookContext context, HookData data) { + return new HookContextWithData<>(context, data); + } + + @Override + public String getFlagKey() { + return context.getFlagKey(); + } + + @Override + public FlagValueType getType() { + return context.getType(); + } + + @Override + public T getDefaultValue() { + return context.getDefaultValue(); + } + + @Override + public EvaluationContext getCtx() { + return context.getCtx(); + } + + @Override + public ClientMetadata getClientMetadata() { + return context.getClientMetadata(); + } + + @Override + public Metadata getProviderMetadata() { + return context.getProviderMetadata(); + } + + @Override + public HookData getHookData() { + return data; + } +} diff --git a/src/main/java/dev/openfeature/sdk/HookContextWithoutData.java b/src/main/java/dev/openfeature/sdk/HookContextWithoutData.java new file mode 100644 index 000000000..cf06f101a --- /dev/null +++ b/src/main/java/dev/openfeature/sdk/HookContextWithoutData.java @@ -0,0 +1,33 @@ +package dev.openfeature.sdk; + +import lombok.Builder; +import lombok.NonNull; +import lombok.Value; +import lombok.With; + +/** + * A data class to hold immutable context that {@link Hook} instances use. + * + * @param the type for the flag being evaluated + */ +@Value +@Builder +@With +class HookContextWithoutData implements HookContext { + @NonNull String flagKey; + + @NonNull FlagValueType type; + + @NonNull T defaultValue; + + @NonNull EvaluationContext ctx; + + ClientMetadata clientMetadata; + Metadata providerMetadata; + + /** + * Hook data provides a way for hooks to maintain state across their execution stages. + * Each hook instance gets its own isolated data store. + */ + HookData hookData; +} diff --git a/src/main/java/dev/openfeature/sdk/HookSupport.java b/src/main/java/dev/openfeature/sdk/HookSupport.java index f21499af5..b31afe2a2 100644 --- a/src/main/java/dev/openfeature/sdk/HookSupport.java +++ b/src/main/java/dev/openfeature/sdk/HookSupport.java @@ -90,7 +90,7 @@ private void executeChecked( BiConsumer, HookContext> hookCode, String hookMethod) { try { - var hookCtxWithData = hookContext.withHookData(hookData); + var hookCtxWithData = HookContextWithData.of(hookContext, hookData); hookCode.accept(hook, hookCtxWithData); } catch (Exception exception) { log.error( @@ -112,7 +112,7 @@ private void executeHooksUnchecked( Hook hook = hookDataPair.getLeft(); HookData hookData = hookDataPair.getRight(); if (hook.supportsFlagValueType(flagValueType)) { - var hookCtxWithData = hookContext.withHookData(hookData); + var hookCtxWithData = HookContextWithData.of(hookContext, hookData); hookCode.accept(hook, hookCtxWithData); } } @@ -135,7 +135,7 @@ private EvaluationContext callBeforeHooks( if (hook.supportsFlagValueType(flagValueType)) { // Create a new context with this hook's data - HookContext contextWithHookData = hookCtx.withHookData(hookData); + HookContext contextWithHookData = HookContextWithData.of(hookCtx, hookData); Optional optional = Optional.ofNullable(hook.before(contextWithHookData, hints)) .orElse(Optional.empty()); if (optional.isPresent()) { diff --git a/src/test/java/dev/openfeature/sdk/HookContextTest.java b/src/test/java/dev/openfeature/sdk/HookContextTest.java index fcf9715e5..9b8773ce2 100644 --- a/src/test/java/dev/openfeature/sdk/HookContextTest.java +++ b/src/test/java/dev/openfeature/sdk/HookContextTest.java @@ -71,7 +71,7 @@ void shouldCreateHookContextWithHookDataUsingWith() { HookData hookData = HookData.create(); hookData.set("timing", System.currentTimeMillis()); - HookContext contextWithHookData = originalContext.withHookData(hookData); + HookContext contextWithHookData = HookContextWithData.of(originalContext, hookData); assertNull(originalContext.getHookData()); assertNotNull(contextWithHookData.getHookData()); diff --git a/src/test/java/dev/openfeature/sdk/HookSupportTest.java b/src/test/java/dev/openfeature/sdk/HookSupportTest.java index 85a25facb..67ec03d94 100644 --- a/src/test/java/dev/openfeature/sdk/HookSupportTest.java +++ b/src/test/java/dev/openfeature/sdk/HookSupportTest.java @@ -25,7 +25,7 @@ void shouldMergeEvaluationContextsOnBeforeHooksCorrectly() { attributes.put("baseKey", new Value("baseValue")); EvaluationContext baseContext = new ImmutableContext(attributes); FlagValueType valueType = FlagValueType.STRING; - HookContext hookContext = HookContext.builder() + HookContext hookContext = HookContextWithoutData.builder() .flagKey("flagKey") .type(valueType) .defaultValue("defaultValue") From 1de7aea6a9ffc4d8b42b95ebde26ac7b21f3c6f8 Mon Sep 17 00:00:00 2001 From: Simon Schrottner Date: Thu, 11 Sep 2025 12:30:58 +0200 Subject: [PATCH 14/19] fixup: add default method for interface Signed-off-by: Simon Schrottner --- src/main/java/dev/openfeature/sdk/HookContext.java | 5 +++-- .../java/dev/openfeature/sdk/HookContextWithoutData.java | 6 ------ 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/src/main/java/dev/openfeature/sdk/HookContext.java b/src/main/java/dev/openfeature/sdk/HookContext.java index 37b15d459..b87f49e3d 100644 --- a/src/main/java/dev/openfeature/sdk/HookContext.java +++ b/src/main/java/dev/openfeature/sdk/HookContext.java @@ -33,7 +33,6 @@ static HookContext from( .providerMetadata(providerMetadata) .ctx(ctx) .defaultValue(defaultValue) - .hookData(null) // Explicitly set to null for backward compatibility .build(); } @@ -56,5 +55,7 @@ static HookContextWithoutDataBuilder builder() { Metadata getProviderMetadata(); - HookData getHookData(); + default HookData getHookData() { + return null; + } } diff --git a/src/main/java/dev/openfeature/sdk/HookContextWithoutData.java b/src/main/java/dev/openfeature/sdk/HookContextWithoutData.java index cf06f101a..a08699363 100644 --- a/src/main/java/dev/openfeature/sdk/HookContextWithoutData.java +++ b/src/main/java/dev/openfeature/sdk/HookContextWithoutData.java @@ -24,10 +24,4 @@ class HookContextWithoutData implements HookContext { ClientMetadata clientMetadata; Metadata providerMetadata; - - /** - * Hook data provides a way for hooks to maintain state across their execution stages. - * Each hook instance gets its own isolated data store. - */ - HookData hookData; } From 52cbaa740679d0e8abb9f5d967b099be97452361 Mon Sep 17 00:00:00 2001 From: Simon Schrottner Date: Thu, 11 Sep 2025 13:05:16 +0200 Subject: [PATCH 15/19] feat: performance boost Signed-off-by: Simon Schrottner --- .../java/dev/openfeature/sdk/HookContext.java | 3 ++ .../sdk/HookContextWithoutData.java | 38 ++++++++++++++++++- .../dev/openfeature/sdk/ImmutableContext.java | 2 + .../openfeature/sdk/OpenFeatureClient.java | 13 ++++--- .../dev/openfeature/sdk/HookContextTest.java | 8 +--- 5 files changed, 50 insertions(+), 14 deletions(-) diff --git a/src/main/java/dev/openfeature/sdk/HookContext.java b/src/main/java/dev/openfeature/sdk/HookContext.java index b87f49e3d..67bf897d7 100644 --- a/src/main/java/dev/openfeature/sdk/HookContext.java +++ b/src/main/java/dev/openfeature/sdk/HookContext.java @@ -18,7 +18,10 @@ public interface HookContext { * @param defaultValue Fallback value * @param type that the flag is evaluating against * @return resulting context for hook + * + * @deprecated this should not be instantiated outside the SDK anymore */ + @Deprecated static HookContext from( String key, FlagValueType type, diff --git a/src/main/java/dev/openfeature/sdk/HookContextWithoutData.java b/src/main/java/dev/openfeature/sdk/HookContextWithoutData.java index a08699363..9d0721ff7 100644 --- a/src/main/java/dev/openfeature/sdk/HookContextWithoutData.java +++ b/src/main/java/dev/openfeature/sdk/HookContextWithoutData.java @@ -1,18 +1,24 @@ package dev.openfeature.sdk; +import lombok.AccessLevel; import lombok.Builder; +import lombok.Data; +import lombok.Getter; import lombok.NonNull; +import lombok.Setter; import lombok.Value; import lombok.With; +import lombok.experimental.NonFinal; /** * A data class to hold immutable context that {@link Hook} instances use. * * @param the type for the flag being evaluated */ -@Value +@Data @Builder @With +@Setter(AccessLevel.PRIVATE) class HookContextWithoutData implements HookContext { @NonNull String flagKey; @@ -20,8 +26,38 @@ class HookContextWithoutData implements HookContext { @NonNull T defaultValue; + @Setter(AccessLevel.PACKAGE) @NonNull EvaluationContext ctx; ClientMetadata clientMetadata; Metadata providerMetadata; + + /** + * Builds a {@link HookContextWithoutData} instances from request data. + * + * @param key feature flag key + * @param type flag value type + * @param clientMetadata info on which client is calling + * @param providerMetadata info on the provider + * @param ctx Evaluation Context for the request + * @param defaultValue Fallback value + * @param type that the flag is evaluating against + * @return resulting context for hook + */ + static HookContextWithoutData from( + String key, + FlagValueType type, + ClientMetadata clientMetadata, + Metadata providerMetadata, + EvaluationContext ctx, + T defaultValue) { + return HookContextWithoutData.builder() + .flagKey(key) + .type(type) + .clientMetadata(clientMetadata) + .providerMetadata(providerMetadata) + .ctx(ImmutableContext.EMPTY) + .defaultValue(defaultValue) + .build(); + } } diff --git a/src/main/java/dev/openfeature/sdk/ImmutableContext.java b/src/main/java/dev/openfeature/sdk/ImmutableContext.java index 8560c369e..e4916dfca 100644 --- a/src/main/java/dev/openfeature/sdk/ImmutableContext.java +++ b/src/main/java/dev/openfeature/sdk/ImmutableContext.java @@ -20,6 +20,8 @@ @SuppressWarnings("PMD.BeanMembersShouldSerialize") public final class ImmutableContext implements EvaluationContext { + public static final ImmutableContext EMPTY = new ImmutableContext(); + @Delegate(excludes = DelegateExclusions.class) private final ImmutableStructure structure; diff --git a/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java b/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java index 4d21b58f2..6fbd1a428 100644 --- a/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java +++ b/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java @@ -166,22 +166,23 @@ private FlagEvaluationDetails evaluateFlag( FlagEvaluationDetails details = null; List mergedHooks; List> hookDataPairs = null; - HookContext hookContext = null; + HookContextWithoutData hookContext = null; try { final var stateManager = openfeatureApi.getFeatureProviderStateManager(this.domain); // provider must be accessed once to maintain a consistent reference final var provider = stateManager.getProvider(); final var state = stateManager.getState(); - hookContext = HookContext.from( - key, type, this.getMetadata(), provider.getMetadata(), mergeEvaluationContext(ctx), defaultValue); + hookContext = HookContextWithoutData.from( + key, type, this.getMetadata(), provider.getMetadata(), null, defaultValue); + + hookContext.setCtx(mergeEvaluationContext(ctx)); + mergedHooks = ObjectUtils.merge( provider.getProviderHooks(), flagOptions.getHooks(), clientHooks, openfeatureApi.getMutableHooks()); hookDataPairs = hookSupport.getHookDataPairs(mergedHooks, type); var mergedCtx = hookSupport.beforeHooks(type, hookContext, hookDataPairs, hints); - - hookContext = - HookContext.from(key, type, this.getMetadata(), provider.getMetadata(), mergedCtx, defaultValue); + hookContext.setCtx(mergedCtx); // "short circuit" if the provider is in NOT_READY or FATAL state if (ProviderState.NOT_READY.equals(state)) { diff --git a/src/test/java/dev/openfeature/sdk/HookContextTest.java b/src/test/java/dev/openfeature/sdk/HookContextTest.java index 9b8773ce2..7f67e751b 100644 --- a/src/test/java/dev/openfeature/sdk/HookContextTest.java +++ b/src/test/java/dev/openfeature/sdk/HookContextTest.java @@ -35,13 +35,7 @@ void shouldCreateHookContextWithHookData() { HookData hookData = HookData.create(); hookData.set("test", "value"); - HookContext context = HookContext.builder() - .flagKey("test-flag") - .type(FlagValueType.STRING) - .defaultValue("default") - .ctx(new ImmutableContext()) - .hookData(hookData) - .build(); + HookContext context = HookContextWithData.of(null, hookData); assertNotNull(context.getHookData()); assertEquals("value", context.getHookData().get("test")); From 0b224dedc681cc09b66b09bff940b796e0154ab9 Mon Sep 17 00:00:00 2001 From: Simon Schrottner Date: Thu, 11 Sep 2025 14:01:22 +0200 Subject: [PATCH 16/19] feat: performance improvements Signed-off-by: Simon Schrottner --- .../sdk/HookContextWithoutData.java | 3 --- .../java/dev/openfeature/sdk/HookSupport.java | 24 +++++++------------ .../sdk/benchmark/AllocationBenchmark.java | 24 +++++++++++++++++++ 3 files changed, 33 insertions(+), 18 deletions(-) diff --git a/src/main/java/dev/openfeature/sdk/HookContextWithoutData.java b/src/main/java/dev/openfeature/sdk/HookContextWithoutData.java index 9d0721ff7..9d92de341 100644 --- a/src/main/java/dev/openfeature/sdk/HookContextWithoutData.java +++ b/src/main/java/dev/openfeature/sdk/HookContextWithoutData.java @@ -3,12 +3,9 @@ import lombok.AccessLevel; import lombok.Builder; import lombok.Data; -import lombok.Getter; import lombok.NonNull; import lombok.Setter; -import lombok.Value; import lombok.With; -import lombok.experimental.NonFinal; /** * A data class to hold immutable context that {@link Hook} instances use. diff --git a/src/main/java/dev/openfeature/sdk/HookSupport.java b/src/main/java/dev/openfeature/sdk/HookSupport.java index b31afe2a2..e9ebbbe58 100644 --- a/src/main/java/dev/openfeature/sdk/HookSupport.java +++ b/src/main/java/dev/openfeature/sdk/HookSupport.java @@ -75,9 +75,7 @@ private void executeHooks( for (Pair hookDataPair : hookDataPairs) { Hook hook = hookDataPair.getLeft(); HookData hookData = hookDataPair.getRight(); - if (hook.supportsFlagValueType(flagValueType)) { - executeChecked(hook, hookData, hookContext, hookCode, hookMethod); - } + executeChecked(hook, hookData, hookContext, hookCode, hookMethod); } } } @@ -111,10 +109,8 @@ private void executeHooksUnchecked( for (Pair hookDataPair : hookDataPairs) { Hook hook = hookDataPair.getLeft(); HookData hookData = hookDataPair.getRight(); - if (hook.supportsFlagValueType(flagValueType)) { - var hookCtxWithData = HookContextWithData.of(hookContext, hookData); - hookCode.accept(hook, hookCtxWithData); - } + var hookCtxWithData = HookContextWithData.of(hookContext, hookData); + hookCode.accept(hook, hookCtxWithData); } } } @@ -133,14 +129,12 @@ private EvaluationContext callBeforeHooks( Hook hook = hookDataPair.getLeft(); HookData hookData = hookDataPair.getRight(); - if (hook.supportsFlagValueType(flagValueType)) { - // Create a new context with this hook's data - HookContext contextWithHookData = HookContextWithData.of(hookCtx, hookData); - Optional optional = Optional.ofNullable(hook.before(contextWithHookData, hints)) - .orElse(Optional.empty()); - if (optional.isPresent()) { - context = context.merge(optional.get()); - } + // Create a new context with this hook's data + HookContext contextWithHookData = HookContextWithData.of(hookCtx, hookData); + Optional optional = + Optional.ofNullable(hook.before(contextWithHookData, hints)).orElse(Optional.empty()); + if (optional.isPresent()) { + context = context.merge(optional.get()); } } return context; diff --git a/src/test/java/dev/openfeature/sdk/benchmark/AllocationBenchmark.java b/src/test/java/dev/openfeature/sdk/benchmark/AllocationBenchmark.java index 5bc89d03d..9fe043722 100644 --- a/src/test/java/dev/openfeature/sdk/benchmark/AllocationBenchmark.java +++ b/src/test/java/dev/openfeature/sdk/benchmark/AllocationBenchmark.java @@ -54,6 +54,30 @@ public Optional before(HookContext ctx, Map() { + @Override + public Optional before(HookContext ctx, Map hints) { + return Optional.ofNullable(new ImmutableContext()); + } + }); + client.addHooks(new Hook() { + @Override + public Optional before(HookContext ctx, Map hints) { + return Optional.ofNullable(new ImmutableContext()); + } + }); + client.addHooks(new Hook() { + @Override + public Optional before(HookContext ctx, Map hints) { + return Optional.ofNullable(new ImmutableContext()); + } + }); + client.addHooks(new Hook() { + @Override + public Optional before(HookContext ctx, Map hints) { + return Optional.ofNullable(new ImmutableContext()); + } + }); Map invocationAttrs = new HashMap<>(); invocationAttrs.put("invoke", new Value(3)); From 711e0beeb8ad3aaf655e59338ddef9ad1116dc53 Mon Sep 17 00:00:00 2001 From: Simon Schrottner Date: Thu, 11 Sep 2025 14:41:40 +0200 Subject: [PATCH 17/19] fixup: lazy init Signed-off-by: Simon Schrottner --- src/main/java/dev/openfeature/sdk/HookData.java | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/main/java/dev/openfeature/sdk/HookData.java b/src/main/java/dev/openfeature/sdk/HookData.java index 16b2726d0..7877cee57 100644 --- a/src/main/java/dev/openfeature/sdk/HookData.java +++ b/src/main/java/dev/openfeature/sdk/HookData.java @@ -48,21 +48,27 @@ static HookData create() { * Default implementation of HookData. */ public class DefaultHookData implements HookData { - private final Map data = new HashMap<>(); + private Map data; @Override public void set(String key, Object value) { + if(data == null) { + data = new HashMap<>(); + } data.put(key, value); } @Override public Object get(String key) { + if(data == null) { + return null; + } return data.get(key); } @Override public T get(String key, Class type) { - Object value = data.get(key); + Object value = get(key); if (value == null) { return null; } From c3f8f3678b4a52521e03536f1f72db0ea88092f5 Mon Sep 17 00:00:00 2001 From: Simon Schrottner Date: Thu, 11 Sep 2025 15:44:27 +0200 Subject: [PATCH 18/19] fixup: pr comments Signed-off-by: Simon Schrottner --- .../java/dev/openfeature/sdk/HookContext.java | 11 +---------- .../sdk/HookContextWithoutData.java | 18 +++--------------- .../java/dev/openfeature/sdk/HookData.java | 6 +++--- .../dev/openfeature/sdk/OpenFeatureClient.java | 6 ++++-- src/main/java/dev/openfeature/sdk/Pair.java | 8 -------- .../dev/openfeature/sdk/HookContextTest.java | 2 +- 6 files changed, 12 insertions(+), 39 deletions(-) diff --git a/src/main/java/dev/openfeature/sdk/HookContext.java b/src/main/java/dev/openfeature/sdk/HookContext.java index 67bf897d7..e88e812a6 100644 --- a/src/main/java/dev/openfeature/sdk/HookContext.java +++ b/src/main/java/dev/openfeature/sdk/HookContext.java @@ -4,7 +4,6 @@ /** * A interface to hold immutable context that {@link Hook} instances use. - * */ public interface HookContext { /** @@ -18,7 +17,6 @@ public interface HookContext { * @param defaultValue Fallback value * @param type that the flag is evaluating against * @return resulting context for hook - * * @deprecated this should not be instantiated outside the SDK anymore */ @Deprecated @@ -29,14 +27,7 @@ static HookContext from( Metadata providerMetadata, EvaluationContext ctx, T defaultValue) { - return HookContextWithoutData.builder() - .flagKey(key) - .type(type) - .clientMetadata(clientMetadata) - .providerMetadata(providerMetadata) - .ctx(ctx) - .defaultValue(defaultValue) - .build(); + return new HookContextWithoutData<>(key, type, defaultValue, ctx, clientMetadata, providerMetadata); } /** diff --git a/src/main/java/dev/openfeature/sdk/HookContextWithoutData.java b/src/main/java/dev/openfeature/sdk/HookContextWithoutData.java index 9d92de341..6e5394ee1 100644 --- a/src/main/java/dev/openfeature/sdk/HookContextWithoutData.java +++ b/src/main/java/dev/openfeature/sdk/HookContextWithoutData.java @@ -36,25 +36,13 @@ class HookContextWithoutData implements HookContext { * @param type flag value type * @param clientMetadata info on which client is calling * @param providerMetadata info on the provider - * @param ctx Evaluation Context for the request * @param defaultValue Fallback value * @param type that the flag is evaluating against * @return resulting context for hook */ static HookContextWithoutData from( - String key, - FlagValueType type, - ClientMetadata clientMetadata, - Metadata providerMetadata, - EvaluationContext ctx, - T defaultValue) { - return HookContextWithoutData.builder() - .flagKey(key) - .type(type) - .clientMetadata(clientMetadata) - .providerMetadata(providerMetadata) - .ctx(ImmutableContext.EMPTY) - .defaultValue(defaultValue) - .build(); + String key, FlagValueType type, ClientMetadata clientMetadata, Metadata providerMetadata, T defaultValue) { + return new HookContextWithoutData<>( + key, type, defaultValue, ImmutableContext.EMPTY, clientMetadata, providerMetadata); } } diff --git a/src/main/java/dev/openfeature/sdk/HookData.java b/src/main/java/dev/openfeature/sdk/HookData.java index 7877cee57..c7c644a93 100644 --- a/src/main/java/dev/openfeature/sdk/HookData.java +++ b/src/main/java/dev/openfeature/sdk/HookData.java @@ -47,12 +47,12 @@ static HookData create() { /** * Default implementation of HookData. */ - public class DefaultHookData implements HookData { + class DefaultHookData implements HookData { private Map data; @Override public void set(String key, Object value) { - if(data == null) { + if (data == null) { data = new HashMap<>(); } data.put(key, value); @@ -60,7 +60,7 @@ public void set(String key, Object value) { @Override public Object get(String key) { - if(data == null) { + if (data == null) { return null; } return data.get(key); diff --git a/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java b/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java index 6fbd1a428..10c359e3e 100644 --- a/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java +++ b/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java @@ -173,9 +173,11 @@ private FlagEvaluationDetails evaluateFlag( // provider must be accessed once to maintain a consistent reference final var provider = stateManager.getProvider(); final var state = stateManager.getState(); - hookContext = HookContextWithoutData.from( - key, type, this.getMetadata(), provider.getMetadata(), null, defaultValue); + hookContext = + HookContextWithoutData.from(key, type, this.getMetadata(), provider.getMetadata(), defaultValue); + // we are setting the evaluation context one after the other, so that we have a hook context in each + // possible exception case. hookContext.setCtx(mergeEvaluationContext(ctx)); mergedHooks = ObjectUtils.merge( diff --git a/src/main/java/dev/openfeature/sdk/Pair.java b/src/main/java/dev/openfeature/sdk/Pair.java index 5fb03ec79..bc6614093 100644 --- a/src/main/java/dev/openfeature/sdk/Pair.java +++ b/src/main/java/dev/openfeature/sdk/Pair.java @@ -9,14 +9,6 @@ private Pair(K key, V value) { this.value = value; } - public K getKey() { - return key; - } - - public V getValue() { - return value; - } - public K getLeft() { return key; } diff --git a/src/test/java/dev/openfeature/sdk/HookContextTest.java b/src/test/java/dev/openfeature/sdk/HookContextTest.java index 7f67e751b..123052b7d 100644 --- a/src/test/java/dev/openfeature/sdk/HookContextTest.java +++ b/src/test/java/dev/openfeature/sdk/HookContextTest.java @@ -35,7 +35,7 @@ void shouldCreateHookContextWithHookData() { HookData hookData = HookData.create(); hookData.set("test", "value"); - HookContext context = HookContextWithData.of(null, hookData); + HookContextWithData context = HookContextWithData.of(mock(HookContext.class), hookData); assertNotNull(context.getHookData()); assertEquals("value", context.getHookData().get("test")); From 445403d4f0835f5be33b735bfbd2364acc617972 Mon Sep 17 00:00:00 2001 From: Simon Schrottner Date: Fri, 12 Sep 2025 17:52:55 +0200 Subject: [PATCH 19/19] fixup: rename pair variables Signed-off-by: Simon Schrottner --- src/main/java/dev/openfeature/sdk/Pair.java | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/main/java/dev/openfeature/sdk/Pair.java b/src/main/java/dev/openfeature/sdk/Pair.java index bc6614093..a43834f4b 100644 --- a/src/main/java/dev/openfeature/sdk/Pair.java +++ b/src/main/java/dev/openfeature/sdk/Pair.java @@ -1,25 +1,25 @@ package dev.openfeature.sdk; class Pair { - private final K key; - private final V value; + private final K left; + private final V right; - private Pair(K key, V value) { - this.key = key; - this.value = value; + private Pair(K left, V value) { + this.left = left; + this.right = value; } public K getLeft() { - return key; + return left; } public V getRight() { - return value; + return right; } @Override public String toString() { - return "Pair{" + "key=" + key + ", value=" + value + '}'; + return "Pair{" + "key=" + left + ", value=" + right + '}'; } public static Pair of(K key, V value) {