Skip to content

Commit e8fb3a1

Browse files
leoromanovskyjustinabrahms
leoromanovsky
and
justinabrahms
authored
fix: Allow empty string as a valid targeting key (#1807)
* Allow empty string as a valid targeting key The OpenFeature spec defines targeting key as "a string" with no restriction on empty strings. The TARGETING_KEY_MISSING error code is for when a key is "not provided" (null), not when it's an empty string. Previously, MutableContext rejected empty strings by checking `!targetingKey.trim().isEmpty()`, treating them the same as null. This made it impossible to distinguish between "not provided" and "explicitly set to empty string". Changes: - Remove isEmpty check from constructor and setTargetingKey() - Empty string is now stored and returned by getTargetingKey() - Null still results in no targeting key being set - Updated tests to reflect new behavior - Added explicit tests for empty string targeting key Signed-off-by: Leo Romanovsky <leo.romanovsky@datadoghq.com> * Also update ImmutableContext and add whitespace tests - Update ImmutableContext to allow empty string targeting keys, matching the MutableContext behavior for consistency - Add tests for whitespace-only targeting keys to explicitly document this is now permitted - Update merge behavior tests to reflect that empty string targeting keys now override existing values Signed-off-by: Leo Romanovsky <leo.romanovsky@datadoghq.com> * Combine empty and whitespace setter tests per review feedback Signed-off-by: Leo Romanovsky <leo.romanovsky@datadoghq.com> * Fix EvalContextTest merge tests for whitespace targeting keys Signed-off-by: Leo Romanovsky <leo.romanovsky@datadoghq.com> * Add test for setTargetingKey(null) to improve coverage Signed-off-by: Leo Romanovsky <leo.romanovsky@datadoghq.com> * Add nullability test for ImmutableContext constructor Per review feedback, add test to verify that passing null to the ImmutableContext constructor results in no targeting key being set. Signed-off-by: Leo Romanovsky <leo.romanovsky@datadoghq.com> * Fix ambiguous constructor call in ImmutableContext test Cast null to String to disambiguate between ImmutableContext(String) and ImmutableContext(Map) constructors. Signed-off-by: Leo Romanovsky <leo.romanovsky@datadoghq.com> --------- Signed-off-by: Leo Romanovsky <leo.romanovsky@datadoghq.com> Co-authored-by: Justin Abrahms <justin@abrah.ms>
1 parent d3df294 commit e8fb3a1

5 files changed

Lines changed: 75 additions & 13 deletions

File tree

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,12 +55,13 @@ public ImmutableContext(Map<String, Value> attributes) {
5555

5656
/**
5757
* Create an immutable context with given targetingKey and attributes provided.
58+
* Empty string is a valid targeting key value.
5859
*
5960
* @param targetingKey targeting key
6061
* @param attributes evaluation context attributes
6162
*/
6263
public ImmutableContext(String targetingKey, Map<String, Value> attributes) {
63-
if (targetingKey != null && !targetingKey.isBlank()) {
64+
if (targetingKey != null) {
6465
this.structure = new ImmutableStructure(targetingKey, attributes);
6566
} else {
6667
this.structure = new ImmutableStructure(attributes);

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

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -36,14 +36,14 @@ public MutableContext(Map<String, Value> attributes) {
3636

3737
/**
3838
* Create a mutable context with given targetingKey and attributes provided. TargetingKey should be non-null
39-
* and non-empty to be accepted.
39+
* to be accepted. Empty string is a valid targeting key value.
4040
*
4141
* @param targetingKey targeting key
4242
* @param attributes evaluation context attributes
4343
*/
4444
public MutableContext(String targetingKey, Map<String, Value> attributes) {
4545
this.structure = new MutableStructure(new HashMap<>(attributes));
46-
if (targetingKey != null && !targetingKey.trim().isEmpty()) {
46+
if (targetingKey != null) {
4747
this.structure.attributes.put(TARGETING_KEY, new Value(targetingKey));
4848
}
4949
}
@@ -85,10 +85,11 @@ public MutableContext add(String key, List<Value> value) {
8585
}
8686

8787
/**
88-
* Override or set targeting key for this mutable context. Value should be non-null and non-empty to be accepted.
88+
* Override or set targeting key for this mutable context. Value should be non-null to be accepted.
89+
* Empty string is a valid targeting key value.
8990
*/
9091
public MutableContext setTargetingKey(String targetingKey) {
91-
if (targetingKey != null && !targetingKey.trim().isEmpty()) {
92+
if (targetingKey != null) {
9293
this.add(TARGETING_KEY, targetingKey);
9394
}
9495
return this;

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -173,9 +173,10 @@ void Immutable_context_merge_targeting_key() {
173173
ctxMerged = ctx1.merge(ctx2);
174174
assertEquals(key2, ctxMerged.getTargetingKey());
175175

176+
// Whitespace is now a valid targeting key and should override
176177
ctx2 = new ImmutableContext(" ", new HashMap<>());
177178
ctxMerged = ctx1.merge(ctx2);
178-
assertEquals(key1, ctxMerged.getTargetingKey());
179+
assertEquals(" ", ctxMerged.getTargetingKey());
179180
}
180181

181182
@Test
@@ -200,9 +201,10 @@ void merge_targeting_key() {
200201
ctxMerged = ctx1.merge(ctx2);
201202
assertEquals(key2, ctxMerged.getTargetingKey());
202203

204+
// Whitespace is now a valid targeting key and should override
203205
ctx2.setTargetingKey(" ");
204206
ctxMerged = ctx1.merge(ctx2);
205-
assertEquals(key2, ctxMerged.getTargetingKey());
207+
assertEquals(" ", ctxMerged.getTargetingKey());
206208
}
207209

208210
@Test

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

Lines changed: 25 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -54,16 +54,17 @@ void shouldChangeTargetingKeyFromOverridingContext() {
5454
assertEquals("overriding_key", merge.getTargetingKey());
5555
}
5656

57-
@DisplayName("targeting key should not be changed from the overriding context if missing")
57+
@DisplayName("targeting key should be changed from the overriding context even if empty string")
5858
@Test
59-
void shouldRetainTargetingKeyWhenOverridingContextTargetingKeyValueIsEmpty() {
59+
void shouldOverrideTargetingKeyWhenOverridingContextTargetingKeyIsEmptyString() {
6060
HashMap<String, Value> attributes = new HashMap<>();
6161
attributes.put("key1", new Value("val1"));
6262
attributes.put("key2", new Value("val2"));
6363
EvaluationContext ctx = new ImmutableContext("targeting_key", attributes);
6464
EvaluationContext overriding = new ImmutableContext("");
6565
EvaluationContext merge = ctx.merge(overriding);
66-
assertEquals("targeting_key", merge.getTargetingKey());
66+
// Empty string is a valid targeting key and should override
67+
assertEquals("", merge.getTargetingKey());
6768
}
6869

6970
@DisplayName("missing targeting key should return null")
@@ -73,6 +74,27 @@ void missingTargetingKeyShould() {
7374
assertNull(ctx.getTargetingKey());
7475
}
7576

77+
@DisplayName("null targeting key in constructor should result in no targeting key")
78+
@Test
79+
void nullTargetingKeyInConstructorShouldResultInNoTargetingKey() {
80+
EvaluationContext ctx = new ImmutableContext((String) null);
81+
assertNull(ctx.getTargetingKey());
82+
}
83+
84+
@DisplayName("empty string is a valid targeting key")
85+
@Test
86+
void emptyStringIsValidTargetingKey() {
87+
EvaluationContext ctx = new ImmutableContext("");
88+
assertEquals("", ctx.getTargetingKey());
89+
}
90+
91+
@DisplayName("whitespace-only string is a valid targeting key")
92+
@Test
93+
void whitespaceOnlyStringIsValidTargetingKey() {
94+
EvaluationContext ctx = new ImmutableContext(" ");
95+
assertEquals(" ", ctx.getTargetingKey());
96+
}
97+
7698
@DisplayName("Merge should retain all the attributes from the existing context when overriding context is null")
7799
@Test
78100
void mergeShouldReturnAllTheValuesFromTheContextWhenOverridingContextIsNull() {

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

Lines changed: 39 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -40,16 +40,17 @@ void shouldChangeTargetingKeyFromOverridingContext() {
4040
assertEquals("overriding_key", merge.getTargetingKey());
4141
}
4242

43-
@DisplayName("targeting key should not changed from the overriding context if missing")
43+
@DisplayName("targeting key should be changed from the overriding context even if empty string")
4444
@Test
45-
void shouldRetainTargetingKeyWhenOverridingContextTargetingKeyValueIsEmpty() {
45+
void shouldOverrideTargetingKeyWhenOverridingContextTargetingKeyIsEmptyString() {
4646
HashMap<String, Value> attributes = new HashMap<>();
4747
attributes.put("key1", new Value("val1"));
4848
attributes.put("key2", new Value("val2"));
4949
EvaluationContext ctx = new MutableContext("targeting_key", attributes);
5050
EvaluationContext overriding = new MutableContext("");
5151
EvaluationContext merge = ctx.merge(overriding);
52-
assertEquals("targeting_key", merge.getTargetingKey());
52+
// Empty string is a valid targeting key and should override
53+
assertEquals("", merge.getTargetingKey());
5354
}
5455

5556
@DisplayName("missing targeting key should return null")
@@ -59,6 +60,41 @@ void missingTargetingKeyShould() {
5960
assertEquals(null, ctx.getTargetingKey());
6061
}
6162

63+
@DisplayName("setTargetingKey with null should not set targeting key")
64+
@Test
65+
void setTargetingKeyWithNullShouldNotSet() {
66+
MutableContext ctx = new MutableContext("original");
67+
ctx.setTargetingKey(null);
68+
// null should not override the existing targeting key
69+
assertEquals("original", ctx.getTargetingKey());
70+
}
71+
72+
@DisplayName("empty string is a valid targeting key via constructor")
73+
@Test
74+
void emptyStringIsValidTargetingKeyViaConstructor() {
75+
EvaluationContext ctx = new MutableContext("");
76+
assertEquals("", ctx.getTargetingKey());
77+
}
78+
79+
@DisplayName("empty and whitespace-only strings are valid targeting keys via setter")
80+
@Test
81+
void emptyAndWhitespaceAreValidTargetingKeysViaSetter() {
82+
MutableContext ctx = new MutableContext();
83+
84+
ctx.setTargetingKey("");
85+
assertEquals("", ctx.getTargetingKey());
86+
87+
ctx.setTargetingKey(" ");
88+
assertEquals(" ", ctx.getTargetingKey());
89+
}
90+
91+
@DisplayName("whitespace-only string is a valid targeting key via constructor")
92+
@Test
93+
void whitespaceOnlyStringIsValidTargetingKeyViaConstructor() {
94+
EvaluationContext ctx = new MutableContext(" ");
95+
assertEquals(" ", ctx.getTargetingKey());
96+
}
97+
6298
@DisplayName("Merge should retain all the attributes from the existing context when overriding context is null")
6399
@Test
64100
void mergeShouldReturnAllTheValuesFromTheContextWhenOverridingContextIsNull() {

0 commit comments

Comments
 (0)