Skip to content

perf: skip unmodifiableMap wrapper when hookHints is empty #1953

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
toddbaert merged 5 commits into from
Jun 10, 2026
+33 −1
Merged

perf: skip unmodifiableMap wrapper when hookHints is empty #1953

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d583cd2
perf: skip unmodifiableMap wrapper when hookHints is empty
tobias-ibounig-dt Jun 5, 2026
796d5b7
ensure hookHints non null via builder, added test
tobias-ibounig-dt Jun 8, 2026
8a9fc31
test null does not throw exception
tobias-ibounig-dt Jun 9, 2026
aad8866
spotless
tobias-ibounig-dt Jun 9, 2026
a5b5795
Merge branch 'main' into perf/pr-2-hook-hints
toddbaert Jun 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions src/main/java/dev/openfeature/sdk/FlagEvaluationOptions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package dev.openfeature.sdk;

import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
Expand All @@ -19,4 +20,13 @@ public class FlagEvaluationOptions {

@Builder.Default
Map<String, Object> hookHints = new HashMap<>();

public static class FlagEvaluationOptionsBuilder {
/** Sets hook hints, normalizing null to an empty map. */
public FlagEvaluationOptionsBuilder hookHints(Map<String, Object> hookHints) {
this.hookHints$value = hookHints != null ? hookHints : Collections.emptyMap();
this.hookHints$set = true;
return this;
}
}
}
3 changes: 2 additions & 1 deletion src/main/java/dev/openfeature/sdk/OpenFeatureClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@
value = {"REC_CATCH_EXCEPTION"},
justification = "We don't want to allow any exception to reach the user. "
+ "Instead, we return an evaluation result with the appropriate error code.")
private <T> FlagEvaluationDetails<T> evaluateFlag(

Check failure on line 163 in src/main/java/dev/openfeature/sdk/OpenFeatureClient.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Refactor this method to reduce its Cognitive Complexity from 16 to the 15 allowed. 


See more on https://sonarcloud.io/project/issues?id=open-feature_java-sdk&issues=AZ6W5WbNNP0RdSucRHUl&open=AZ6W5WbNNP0RdSucRHUl&pullRequest=1953
FlagValueType type, String key, T defaultValue, EvaluationContext ctx, FlagEvaluationOptions options) {
FlagEvaluationDetails<T> details = null;
HookSupportData hookSupportData = new HookSupportData();
Expand All @@ -172,7 +172,8 @@
flagOptions = options;
}

hookSupportData.hints = Collections.unmodifiableMap(flagOptions.getHookHints());
var hookHints = flagOptions.getHookHints();
hookSupportData.hints = hookHints.isEmpty() ? Collections.emptyMap() : Collections.unmodifiableMap(hookHints);
Comment on lines +175 to +176

@gemini-code-assist gemini-code-assist Bot Jun 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If flagOptions.getHookHints() returns null (which can happen if hookHints is explicitly set to null via the builder in FlagEvaluationOptions), calling hookHints.isEmpty() will throw a NullPointerException.

To ensure robust defensive programming, we should handle the null case gracefully by defaulting to Collections.emptyMap().

Suggested change
var hookHints = flagOptions.getHookHints();
hookSupportData.hints = hookHints.isEmpty() ? Collections.emptyMap() : Collections.unmodifiableMap(hookHints);
var hookHints = flagOptions.getHookHints();
hookSupportData.hints = hookHints == null || hookHints.isEmpty() ? Collections.emptyMap() : Collections.unmodifiableMap(hookHints);

@chrfwow chrfwow Jun 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is something that should be handled in the builder, but idk if we can modify this, since I assume it will be a lombok builder. Can we add a test to make sure we won't have a NPE?

@tobias-ibounig-dt tobias-ibounig-dt Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ensured in builder that it can't be null

var context = new LayeredEvaluationContext(
openfeatureApi.getEvaluationContext(),
openfeatureApi.getTransactionContext(),
Expand Down
21 changes: 21 additions & 0 deletions src/test/java/dev/openfeature/sdk/HookSpecTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -510,6 +510,27 @@ void missing_hook_hints() {
assertTrue(feo.getHookHints().isEmpty());
}

@Test
void null_hook_hints_does_not_throw() {
Hook hook = mockBooleanHook();
FeatureProvider provider = mock(FeatureProvider.class);
when(provider.getBooleanEvaluation(any(), any(), any()))
.thenReturn(ProviderEvaluation.<Boolean>builder().value(true).build());

api.setProviderAndWait(provider);
Client client = api.getClient();

assertThatCode(() -> client.getBooleanValue(
"key",
false,
new ImmutableContext(),
FlagEvaluationOptions.builder()
.hook(hook)
.hookHints(null)
.build()))
.doesNotThrowAnyException();
}

@Test
void flag_eval_hook_order() {
Hook hook = mockBooleanHook();
Expand Down
Loading