Skip to content

Commit d80a30e

Browse files
dougqhdevflow.devflow-routing-intake
andauthored
Make the TagMap Entry pathway null-tolerant (#11963)
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> Mark tag keys @nonnull on the TagMap write/create surface A tag has no valid null key, so put/set/getAndSet/create now take @nonnull tag. This completes the null contract alongside the value/Entry side: keys are strict (null = a bug), values/Entries on the Entry pathway are tolerant (null = no tag). Scoped to the write/create surface; read/lookup keys (getString, remove, getXxxOrDefault) are left for a possible follow-up. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Brace single-statement null-check ifs per style convention Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: devflow.devflow-routing-intake <devflow.devflow-routing-intake@kubernetes.us1.ddbuild.io>
1 parent 9b16f91 commit d80a30e

2 files changed

Lines changed: 118 additions & 31 deletions

File tree

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

Lines changed: 52 additions & 31 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...
@@ -147,50 +149,53 @@ static Ledger ledger(int size) {
147149
* are implemented more efficiently.
148150
*/
149151
@Deprecated
150-
Object put(String tag, Object value);
152+
Object put(@Nonnull 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(@Nonnull 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(@Nonnull String tag, @Nonnull CharSequence value);
161163

162-
void set(String tag, boolean value);
164+
void set(@Nonnull String tag, boolean value);
163165

164-
void set(String tag, int value);
166+
void set(@Nonnull String tag, int value);
165167

166-
void set(String tag, long value);
168+
void set(@Nonnull String tag, long value);
167169

168-
void set(String tag, float value);
170+
void set(@Nonnull String tag, float value);
169171

170-
void set(String tag, double value);
172+
void set(@Nonnull 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 */
175-
Entry getAndSet(String tag, Object value);
178+
Entry getAndSet(@Nonnull String tag, Object value);
176179

177-
Entry getAndSet(String tag, CharSequence value);
180+
Entry getAndSet(@Nonnull String tag, CharSequence value);
178181

179-
Entry getAndSet(String tag, boolean value);
182+
Entry getAndSet(@Nonnull String tag, boolean value);
180183

181-
Entry getAndSet(String tag, int value);
184+
Entry getAndSet(@Nonnull String tag, int value);
182185

183-
Entry getAndSet(String tag, long value);
186+
Entry getAndSet(@Nonnull String tag, long value);
184187

185-
Entry getAndSet(String tag, float value);
188+
Entry getAndSet(@Nonnull String tag, float value);
186189

187-
Entry getAndSet(String tag, double value);
190+
Entry getAndSet(@Nonnull 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,40 +373,49 @@ 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 */
372-
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);
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
382+
public static final Entry create(@Nonnull String tag, Object value) {
383+
if (value == null) {
384+
return null;
385+
}
386+
if (value instanceof CharSequence && ((CharSequence) value).length() == 0) {
387+
return null;
388+
}
389+
return TagMap.Entry.newAnyEntry(tag, value);
377390
}
378391

379392
/** If value is non-null, returns a new TagMap.Entry If value is null or empty, returns null */
380-
public static final Entry create(String tag, CharSequence value) {
393+
@Nullable
394+
public static final Entry create(@Nonnull String tag, CharSequence value) {
381395
// NOTE: From the static typing, we know that value is not a primitive box
382396

383397
return (value == null || value.length() == 0)
384398
? null
385399
: TagMap.Entry.newObjectEntry(tag, value);
386400
}
387401

388-
public static final Entry create(String tag, boolean value) {
402+
public static final Entry create(@Nonnull String tag, boolean value) {
389403
return TagMap.Entry.newBooleanEntry(tag, value);
390404
}
391405

392-
public static final Entry create(String tag, int value) {
406+
public static final Entry create(@Nonnull String tag, int value) {
393407
return TagMap.Entry.newIntEntry(tag, value);
394408
}
395409

396-
public static final Entry create(String tag, long value) {
410+
public static final Entry create(@Nonnull String tag, long value) {
397411
return TagMap.Entry.newLongEntry(tag, value);
398412
}
399413

400-
public static final Entry create(String tag, float value) {
414+
public static final Entry create(@Nonnull String tag, float value) {
401415
return TagMap.Entry.newFloatEntry(tag, value);
402416
}
403417

404-
public static final Entry create(String tag, double value) {
418+
public static final Entry create(@Nonnull String tag, double value) {
405419
return TagMap.Entry.newDoubleEntry(tag, value);
406420
}
407421

@@ -1357,6 +1371,9 @@ public Object put(String tag, Object value) {
13571371

13581372
@Override
13591373
public void set(TagMap.EntryReader newEntryReader) {
1374+
if (newEntryReader == null) {
1375+
return;
1376+
}
13601377
this.getAndSet(newEntryReader.entry());
13611378
}
13621379

@@ -1397,6 +1414,10 @@ public void set(String tag, double value) {
13971414

13981415
@Override
13991416
public Entry getAndSet(Entry newEntry) {
1417+
if (newEntry == null) {
1418+
return null;
1419+
}
1420+
14001421
this.checkWriteAccess();
14011422

14021423
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)