Skip to content

Commit b5a443f

Browse files
chrfwowchrfwow
committed
cache hooks
Signed-off-by: christian.lutnik <[email protected]>
1 parent 64a81fb commit b5a443f

File tree

12 files changed

+303
-69
lines changed

12 files changed

+303
-69
lines changed

‎src/main/java/dev/openfeature/sdk/FeatureProvider.java‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,12 @@ default List<Hook> getProviderHooks() {
1515
return new ArrayList<>();
1616
}
1717

18+
/**
19+
* Returns all hooks that support the given flag value type.
20+
*
21+
* @param flagType the flag value type to support
22+
* @return a list of hooks that support the given flag value type
23+
*/
1824
default List<Hook> getProviderHooks(FlagValueType flagType) {
1925
var allHooks = getProviderHooks();
2026
var filteredHooks = new ArrayList<Hook>(allHooks.size());

‎src/main/java/dev/openfeature/sdk/HookSupport.java‎

Lines changed: 26 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22

33
import java.util.ArrayList;
44
import java.util.Collection;
5-
import java.util.Collections;
65
import java.util.List;
76
import java.util.Optional;
87
import java.util.concurrent.ConcurrentLinkedQueue;
@@ -19,22 +18,34 @@ class HookSupport {
1918
* Sets the {@link Hook}-{@link HookContext}-{@link Pair} list in the given data object with {@link HookContext}
2019
* set to null. Filters hooks by supported {@link FlagValueType}.
2120
*
22-
* @param hookSupportData the data object to modify
23-
* @param hooks the hooks to set
24-
* @param type the flag value type to filter unsupported hooks
21+
* @param hookSupportData the data object to modify
22+
* @param providerHooks the hooks filtered for the proper flag value type from the respective layer
23+
* @param flagOptionsHooks the hooks filtered for the proper flag value type from the respective layer
24+
* @param clientHooks the hooks filtered for the proper flag value type from the respective layer
25+
* @param apiHooks the hooks filtered for the proper flag value type from the respective layer
2526
*/
2627
public void setHooks(
2728
HookSupportData hookSupportData,
2829
List<Hook> providerHooks,
2930
List<Hook> flagOptionsHooks,
3031
ConcurrentLinkedQueue<Hook> clientHooks,
31-
ConcurrentLinkedQueue<Hook> apiHooks
32-
) {
33-
var lengthEstimate = providerHooks.size()
34-
+ flagOptionsHooks.size()
35-
+ clientHooks.size()
36-
+ apiHooks.size();
37-
List<Pair<Hook, HookContext>> hookContextPairs = new ArrayList<>(lengthEstimate);
32+
ConcurrentLinkedQueue<Hook> apiHooks) {
33+
var lengthEstimate = 0;
34+
35+
if (providerHooks != null) {
36+
lengthEstimate += providerHooks.size();
37+
}
38+
if (flagOptionsHooks != null) {
39+
lengthEstimate += flagOptionsHooks.size();
40+
}
41+
if (clientHooks != null) {
42+
lengthEstimate += clientHooks.size();
43+
}
44+
if (apiHooks != null) {
45+
lengthEstimate += apiHooks.size();
46+
}
47+
48+
ArrayList<Pair<Hook, HookContext>> hookContextPairs = new ArrayList<>(lengthEstimate);
3849

3950
addAll(hookContextPairs, providerHooks);
4051
addAll(hookContextPairs, flagOptionsHooks);
@@ -45,7 +56,7 @@ public void setHooks(
4556
}
4657

4758
private void addAll(List<Pair<Hook, HookContext>> accumulator, Collection<Hook> toAdd) {
48-
if(toAdd.isEmpty()){
59+
if (toAdd == null || toAdd.isEmpty()) {
4960
return;
5061
}
5162

@@ -89,10 +100,9 @@ public void updateEvaluationContext(HookSupportData hookSupportData, EvaluationC
89100

90101
public void executeBeforeHooks(HookSupportData data) {
91102
// These traverse backwards from normal.
92-
List<Pair<Hook, HookContext>> reversedHooks = new ArrayList<>(data.getHooks());
93-
Collections.reverse(reversedHooks);
94-
95-
for (Pair<Hook, HookContext> hookContextPair : reversedHooks) {
103+
var hooks = data.getHooks();
104+
for (int i = hooks.size() - 1; i >= 0; i--) {
105+
var hookContextPair = hooks.get(i);
96106
var hook = hookContextPair.getKey();
97107
var hookContext = hookContextPair.getValue();
98108

‎src/main/java/dev/openfeature/sdk/HookSupportData.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
package dev.openfeature.sdk;
22

3-
import java.util.List;
3+
import java.util.ArrayList;
44
import java.util.Map;
55
import lombok.Getter;
66

@@ -10,7 +10,7 @@
1010
@Getter
1111
class HookSupportData {
1212

13-
List<Pair<Hook, HookContext>> hooks;
13+
ArrayList<Pair<Hook, HookContext>> hooks;
1414
EvaluationContext evaluationContext;
1515
Map<String, Object> hints;
1616

‎src/main/java/dev/openfeature/sdk/OpenFeatureAPI.java‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,7 @@
44
import dev.openfeature.sdk.internal.AutoCloseableLock;
55
import dev.openfeature.sdk.internal.AutoCloseableReentrantReadWriteLock;
66
import java.util.ArrayList;
7-
import java.util.Arrays;
8-
import java.util.Collection;
7+
import java.util.HashSet;
98
import java.util.List;
109
import java.util.Optional;
1110
import java.util.Set;
@@ -314,7 +313,7 @@ public void addHooks(Hook... hooks) {
314313
var current = hooks[i];
315314
for (int j = 0; j < types.length; j++) {
316315
var type = types[j];
317-
if(current.supportsFlagValueType(type)) {
316+
if (current.supportsFlagValueType(type)) {
318317
this.apiHooks.get(type).add(current);
319318
}
320319
}
@@ -327,15 +326,16 @@ public void addHooks(Hook... hooks) {
327326
* @return A list of {@link Hook}s.
328327
*/
329328
public List<Hook> getHooks() {
330-
var allHooks = new ArrayList<Hook>();
329+
// Hooks can be duplicated if they support multiple FlagValueTypes
330+
var allHooks = new HashSet<Hook>();
331331
for (var queue : this.apiHooks.values()) {
332332
allHooks.addAll(queue);
333333
}
334-
return allHooks;
334+
return new ArrayList<>(allHooks);
335335
}
336336

337337
/**
338-
* Fetch the hooks associated to this client.
338+
* Fetch the hooks associated to this client, that support the given FlagValueType.
339339
*
340340
* @return A list of {@link Hook}s.
341341
*/

‎src/main/java/dev/openfeature/sdk/OpenFeatureClient.java‎

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,9 @@
88
import dev.openfeature.sdk.internal.ObjectUtils;
99
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
1010
import java.util.ArrayList;
11-
import java.util.Arrays;
1211
import java.util.Collections;
1312
import java.util.HashMap;
13+
import java.util.HashSet;
1414
import java.util.List;
1515
import java.util.Map;
1616
import java.util.Objects;
@@ -31,11 +31,11 @@
3131
*/
3232
@Slf4j
3333
@SuppressWarnings({
34-
"PMD.DataflowAnomalyAnalysis",
35-
"PMD.BeanMembersShouldSerialize",
36-
"PMD.UnusedLocalVariable",
37-
"unchecked",
38-
"rawtypes"
34+
"PMD.DataflowAnomalyAnalysis",
35+
"PMD.BeanMembersShouldSerialize",
36+
"PMD.UnusedLocalVariable",
37+
"unchecked",
38+
"rawtypes"
3939
})
4040
@Deprecated() // TODO: eventually we will make this non-public. See issue #872
4141
public class OpenFeatureClient implements Client {
@@ -148,11 +148,11 @@ public OpenFeatureClient addHooks(Hook... hooks) {
148148
*/
149149
@Override
150150
public List<Hook> getHooks() {
151-
var allHooks = new ArrayList<Hook>();
151+
var allHooks = new HashSet<Hook>();
152152
for (var queue : this.clientHooks.values()) {
153153
allHooks.addAll(queue);
154154
}
155-
return allHooks;
155+
return new ArrayList<>(allHooks);
156156
}
157157

158158
/**
@@ -197,8 +197,7 @@ private <T> FlagEvaluationDetails<T> evaluateFlag(
197197
provider.getProviderHooks(type),
198198
flagOptions.getHooks(type),
199199
clientHooks.get(type),
200-
openfeatureApi.getHooks(type)
201-
);
200+
openfeatureApi.getHooks(type));
202201

203202
var sharedHookContext =
204203
new SharedHookContext(key, type, this.getMetadata(), provider.getMetadata(), defaultValue);

‎src/test/java/dev/openfeature/sdk/DeveloperExperienceTest.java‎

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
import static org.assertj.core.api.Assertions.assertThat;
44
import static org.junit.jupiter.api.Assertions.assertEquals;
55
import static org.junit.jupiter.api.Assertions.assertFalse;
6+
import static org.junit.jupiter.api.Assertions.assertTrue;
67
import static org.mockito.ArgumentMatchers.any;
78
import static org.mockito.Mockito.times;
89
import static org.mockito.Mockito.verify;
@@ -14,6 +15,7 @@
1415
import java.util.List;
1516
import java.util.Map;
1617
import java.util.Optional;
18+
import java.util.concurrent.atomic.AtomicBoolean;
1719
import lombok.SneakyThrows;
1820
import org.junit.jupiter.api.BeforeEach;
1921
import org.junit.jupiter.api.Test;
@@ -101,9 +103,10 @@ void brokenProvider() {
101103
void providerLockedPerTransaction() {
102104

103105
final String defaultValue = "string-value";
104-
final OpenFeatureAPI api = new OpenFeatureAPI();
105-
var provider1 = TestProvider.builder().initsToReady();
106-
var provider2 = TestProvider.builder().initsToReady();
106+
final OpenFeatureAPI testApi = new OpenFeatureAPI();
107+
final var provider1 = TestProvider.builder().initsToReady();
108+
final var provider2 = TestProvider.builder().initsToReady();
109+
final var wasHookCalled = new AtomicBoolean(false);
107110

108111
class MutatingHook implements Hook {
109112

@@ -112,24 +115,27 @@ class MutatingHook implements Hook {
112115
// change the provider during a before hook - this should not impact the evaluation in progress
113116
public Optional before(HookContext ctx, Map hints) {
114117

115-
api.setProviderAndWait(provider2);
116-
118+
testApi.setProviderAndWait(provider2);
119+
wasHookCalled.set(true);
117120
return Optional.empty();
118121
}
119122
}
120123

121-
final Client client = api.getClient();
122-
api.setProviderAndWait(provider1);
123-
api.addHooks(new MutatingHook());
124+
final Client client = testApi.getClient();
125+
testApi.setProviderAndWait(provider1);
126+
testApi.addHooks(new MutatingHook());
124127

125128
// if provider is changed during an evaluation transaction it should proceed with the original provider
126129
client.getStringValue("val", defaultValue);
127130
assertEquals(1, provider1.getFlagEvaluations().size());
131+
assertEquals(0, provider2.getFlagEvaluations().size());
132+
assertTrue(wasHookCalled.get());
128133

129-
api.clearHooks();
134+
testApi.clearHooks();
130135

131136
// subsequent evaluations should now use new provider set by hook
132137
client.getStringValue("val", defaultValue);
138+
assertEquals(1, provider1.getFlagEvaluations().size());
133139
assertEquals(1, provider2.getFlagEvaluations().size());
134140
}
135141

‎src/test/java/dev/openfeature/sdk/FlagEvaluationSpecTest.java‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
import static org.mockito.Mockito.spy;
1515
import static org.mockito.Mockito.times;
1616
import static org.mockito.Mockito.verify;
17+
import static org.mockito.Mockito.when;
1718

1819
import dev.openfeature.sdk.e2e.Flag;
1920
import dev.openfeature.sdk.exceptions.GeneralError;
@@ -143,14 +144,16 @@ void provider_metadata() {
143144
void hook_addition() {
144145
Hook h1 = mock(Hook.class);
145146
Hook h2 = mock(Hook.class);
147+
when(h1.supportsFlagValueType(any())).thenReturn(true);
148+
when(h2.supportsFlagValueType(any())).thenReturn(true);
146149
api.addHooks(h1);
147150

148151
assertEquals(1, api.getHooks().size());
149152
assertEquals(h1, api.getHooks().get(0));
150153

151154
api.addHooks(h2);
152155
assertEquals(2, api.getHooks().size());
153-
assertEquals(h2, api.getHooks().get(1));
156+
assertTrue(api.getHooks().contains(h2));
154157
}
155158

156159
@Specification(
@@ -175,6 +178,8 @@ void hookRegistration() {
175178
Client c = _client();
176179
Hook m1 = mock(Hook.class);
177180
Hook m2 = mock(Hook.class);
181+
when(m1.supportsFlagValueType(any())).thenReturn(true);
182+
when(m2.supportsFlagValueType(any())).thenReturn(true);
178183
c.addHooks(m1);
179184
c.addHooks(m2);
180185
List<Hook> hooks = c.getHooks();

‎src/test/java/dev/openfeature/sdk/HookSupportTest.java‎

Lines changed: 27 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@
77
import static org.mockito.Mockito.when;
88

99
import dev.openfeature.sdk.fixtures.HookFixtures;
10-
import java.util.Arrays;
1110
import java.util.Collections;
1211
import java.util.HashMap;
1312
import java.util.List;
@@ -37,7 +36,12 @@ void shouldMergeEvaluationContextsOnBeforeHooksCorrectly() {
3736

3837
var sharedContext = getBaseHookContextForType(FlagValueType.STRING);
3938
var hookSupportData = new HookSupportData();
40-
hookSupport.setHooks(hookSupportData, List.of(hook1, hook2), Collections.emptyList(), new ConcurrentLinkedQueue<>(), new ConcurrentLinkedQueue<>());
39+
hookSupport.setHooks(
40+
hookSupportData,
41+
List.of(hook1, hook2),
42+
Collections.emptyList(),
43+
new ConcurrentLinkedQueue<>(),
44+
new ConcurrentLinkedQueue<>());
4145
hookSupport.setHookContexts(hookSupportData, sharedContext);
4246
hookSupport.updateEvaluationContext(hookSupportData, baseEvalContext);
4347

@@ -50,14 +54,18 @@ void shouldMergeEvaluationContextsOnBeforeHooksCorrectly() {
5054
assertThat(result.getValue("baseKey").asString()).isEqualTo("baseValue");
5155
}
5256

53-
/* @ParameterizedTest
54-
@EnumSource(value = FlagValueType.class)
57+
@Test
5558
@DisplayName("should always call generic hook")
56-
void shouldAlwaysCallGenericHook(FlagValueType flagValueType) {
59+
void shouldAlwaysCallGenericHook() {
5760
Hook<?> genericHook = mockGenericHook();
5861

5962
var hookSupportData = new HookSupportData();
60-
hookSupport.setHooks(hookSupportData, List.of(genericHook), flagValueType);
63+
hookSupport.setHooks(
64+
hookSupportData,
65+
List.of(genericHook),
66+
Collections.emptyList(),
67+
new ConcurrentLinkedQueue<>(),
68+
new ConcurrentLinkedQueue<>());
6169

6270
callAllHooks(hookSupportData);
6371

@@ -73,7 +81,12 @@ void shouldAlwaysCallGenericHook(FlagValueType flagValueType) {
7381
void shouldPassDataAcrossStages(FlagValueType flagValueType) {
7482
var testHook = new TestHookWithData();
7583
var hookSupportData = new HookSupportData();
76-
hookSupport.setHooks(hookSupportData, List.of(testHook), flagValueType);
84+
hookSupport.setHooks(
85+
hookSupportData,
86+
List.of(testHook),
87+
Collections.emptyList(),
88+
new ConcurrentLinkedQueue<>(),
89+
new ConcurrentLinkedQueue<>());
7790
hookSupport.setHookContexts(hookSupportData, getBaseHookContextForType(flagValueType));
7891

7992
hookSupport.executeBeforeHooks(hookSupportData);
@@ -99,14 +112,19 @@ void shouldIsolateDataBetweenHooks(FlagValueType flagValueType) {
99112
var testHook2 = new TestHookWithData(2);
100113

101114
var hookSupportData = new HookSupportData();
102-
hookSupport.setHooks(hookSupportData, List.of(testHook1, testHook2), flagValueType);
115+
hookSupport.setHooks(
116+
hookSupportData,
117+
List.of(testHook1, testHook2),
118+
Collections.emptyList(),
119+
new ConcurrentLinkedQueue<>(),
120+
new ConcurrentLinkedQueue<>());
103121
hookSupport.setHookContexts(hookSupportData, getBaseHookContextForType(flagValueType));
104122

105123
callAllHooks(hookSupportData);
106124

107125
assertHookData(testHook1, 1, "before", "after", "finallyAfter", "error");
108126
assertHookData(testHook2, 2, "before", "after", "finallyAfter", "error");
109-
}*/
127+
}
110128

111129
private static void callAllHooks(HookSupportData hookSupportData) {
112130
hookSupport.executeBeforeHooks(hookSupportData);

0 commit comments

Comments
 (0)