Skip to content

Commit fc7bc19

Browse files
dougqhclaude
andcommitted
Make the TagMap Entry pathway null-tolerant
create(Object)/create(CharSequence) may return null for a null or empty value, and the Entry sinks -- getAndSet(Entry) / set(EntryReader) -- treat a null Entry as a no-op, so a null/empty value flows through the Entry pathway as "no tag" without any caller guarding. create(Object) now applies the empty-CharSequence check by runtime type, so the null/empty => absent convention holds regardless of the static type at the call site. The strict (key,value) setters keep their contract -- their values are now @nonnull -- so null tolerance is scoped to the Entry pathway. The annotations make the split self-describing. Fixes a latent NPE by construction: RemoteHostnameAdder sets create(TRACER_HOST, hostname) guarding only null, not empty, so an empty hostname previously NPE'd on set(null). Caller-side guard cleanup (incl. the redundant #11958 guard) is left to a follow-up. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent d3d2e12 commit fc7bc19

2 files changed

Lines changed: 90 additions & 11 deletions

File tree

internal-api/src/main/java/datadog/trace/api/TagMap.java

Lines changed: 24 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@
1616
import java.util.function.Function;
1717
import java.util.stream.Stream;
1818
import java.util.stream.StreamSupport;
19+
import javax.annotation.Nonnull;
20+
import javax.annotation.Nullable;
1921

2022
/**
2123
* A super simple hash map designed for...
@@ -150,14 +152,14 @@ static Ledger ledger(int size) {
150152
Object put(String tag, Object value);
151153

152154
/** Sets value without returning prior value - more efficient than {@link Map#put} */
153-
void set(String tag, Object value);
155+
void set(String tag, @Nonnull Object value);
154156

155157
/**
156158
* Similar to {@link TagMap#set(String, Object)} but more efficient when working with
157159
* CharSequences and Strings. Depending on this situation, this methods avoids having to do type
158160
* resolution later on
159161
*/
160-
void set(String tag, CharSequence value);
162+
void set(String tag, @Nonnull CharSequence value);
161163

162164
void set(String tag, boolean value);
163165

@@ -169,7 +171,8 @@ static Ledger ledger(int size) {
169171

170172
void set(String tag, double value);
171173

172-
void set(EntryReader newEntry);
174+
/** A null reader is a no-op (see the null-tolerance contract on {@link #getAndSet(Entry)}). */
175+
void set(@Nullable EntryReader newEntry);
173176

174177
/** sets the value while returning the prior Entry */
175178
Entry getAndSet(String tag, Object value);
@@ -187,10 +190,12 @@ static Ledger ledger(int size) {
187190
Entry getAndSet(String tag, double value);
188191

189192
/**
190-
* TagMap specific method that places an Entry directly into an optimized TagMap avoiding need to
191-
* allocate a new Entry object
193+
* Places an Entry directly into the map, avoiding a new Entry allocation. Null-tolerant: a null
194+
* {@code newEntry} is a no-op returning null, so an Entry producer (e.g. {@link
195+
* Entry#create(String, Object)} for a null/empty value) can emit "no tag" without the caller
196+
* filtering. Contrast the strict {@link Nonnull} {@code set(String, value)} setters.
192197
*/
193-
Entry getAndSet(Entry newEntry);
198+
Entry getAndSet(@Nullable Entry newEntry);
194199

195200
void putAll(Map<? extends String, ? extends Object> map);
196201

@@ -368,15 +373,20 @@ final class Entry extends EntryChange implements Map.Entry<String, Object>, Entr
368373
*/
369374
static final byte ANY = 0;
370375

371-
/** If value is non-null, returns a new TagMap.Entry If value is null, returns null */
376+
/**
377+
* Entry for {@code (tag, value)}, or null when {@code value} is null or an empty {@code
378+
* CharSequence} -- checked by runtime type, so an empty String passed as {@code Object} skips
379+
* the same as via the {@link #create(String, CharSequence)} overload.
380+
*/
381+
@Nullable
372382
public static final Entry create(String tag, Object value) {
373-
// NOTE: From the static typing, it is possible that value is a primitive box, so need to call
374-
// Any variant
375-
376-
return (value == null) ? null : TagMap.Entry.newAnyEntry(tag, value);
383+
if (value == null) return null;
384+
if (value instanceof CharSequence && ((CharSequence) value).length() == 0) return null;
385+
return TagMap.Entry.newAnyEntry(tag, value);
377386
}
378387

379388
/** If value is non-null, returns a new TagMap.Entry If value is null or empty, returns null */
389+
@Nullable
380390
public static final Entry create(String tag, CharSequence value) {
381391
// NOTE: From the static typing, we know that value is not a primitive box
382392

@@ -1357,6 +1367,7 @@ public Object put(String tag, Object value) {
13571367

13581368
@Override
13591369
public void set(TagMap.EntryReader newEntryReader) {
1370+
if (newEntryReader == null) return;
13601371
this.getAndSet(newEntryReader.entry());
13611372
}
13621373

@@ -1397,6 +1408,8 @@ public void set(String tag, double value) {
13971408

13981409
@Override
13991410
public Entry getAndSet(Entry newEntry) {
1411+
if (newEntry == null) return null;
1412+
14001413
this.checkWriteAccess();
14011414

14021415
Object[] thisBuckets = this.buckets;
Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
package datadog.trace.api;
2+
3+
import static org.junit.jupiter.api.Assertions.assertEquals;
4+
import static org.junit.jupiter.api.Assertions.assertFalse;
5+
import static org.junit.jupiter.api.Assertions.assertNotNull;
6+
import static org.junit.jupiter.api.Assertions.assertNull;
7+
import static org.junit.jupiter.api.Assertions.assertTrue;
8+
9+
import org.junit.jupiter.api.Test;
10+
11+
/**
12+
* Pins the Entry-pathway null-tolerance contract: {@code create(...)} returns null for a null or
13+
* empty value, and the {@code Entry} sinks ({@code set(Entry)} / {@code getAndSet(Entry)}) treat
14+
* null as a no-op -- so a null/empty value flows through as "no tag" without any caller guarding.
15+
*/
16+
class TagMapNullToleranceTest {
17+
18+
@Test
19+
void createReturnsNullForNullOrEmptyValue() {
20+
// CharSequence overload: null or empty -> null
21+
assertNull(TagMap.Entry.create("k", (CharSequence) null));
22+
assertNull(TagMap.Entry.create("k", ""));
23+
24+
// Object overload: null -> null, and an empty String typed as Object -> null (runtime check,
25+
// so the convention holds regardless of the static type at the call site)
26+
assertNull(TagMap.Entry.create("k", (Object) null));
27+
assertNull(TagMap.Entry.create("k", (Object) ""));
28+
29+
// Non-empty values still produce an entry, whatever the static type
30+
assertNotNull(TagMap.Entry.create("k", "v"));
31+
assertNotNull(TagMap.Entry.create("k", (Object) "v"));
32+
// Non-CharSequence Object has no notion of "empty" -> always an entry
33+
assertNotNull(TagMap.Entry.create("k", (Object) Integer.valueOf(0)));
34+
}
35+
36+
@Test
37+
void getAndSetToleratesNullEntry() {
38+
TagMap map = TagMap.create();
39+
assertNull(map.getAndSet((TagMap.Entry) null), "null entry is a no-op returning null");
40+
assertEquals(0, map.size(), "no tag added");
41+
}
42+
43+
@Test
44+
void setToleratesNullReader() {
45+
TagMap map = TagMap.create();
46+
map.set((TagMap.EntryReader) null); // must not throw
47+
assertEquals(0, map.size(), "no tag added");
48+
}
49+
50+
@Test
51+
void nullOrEmptyFlowsThroughTheEntryPathwayAsNoTag() {
52+
TagMap map = TagMap.create();
53+
54+
// The seamless case: create(...) -> set(Entry) with a null/empty value, no caller guard.
55+
map.set(TagMap.Entry.create("empty", ""));
56+
map.set(TagMap.Entry.create("emptyObj", (Object) ""));
57+
map.set(TagMap.Entry.create("nul", (CharSequence) null));
58+
assertEquals(0, map.size(), "null/empty values leave no tags behind");
59+
assertFalse(map.containsKey("empty"));
60+
61+
// A real value still lands.
62+
map.set(TagMap.Entry.create("present", "v"));
63+
assertEquals(1, map.size());
64+
assertTrue(map.containsKey("present"));
65+
}
66+
}

0 commit comments

Comments
 (0)