Make the TagMap Entry pathway null-tolerant

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

@@ -16,6 +16,8 @@
 import java.util.function.Function;
 import java.util.stream.Stream;
 import java.util.stream.StreamSupport;
+import javax.annotation.Nonnull;
+import javax.annotation.Nullable;
 
 /**
  * A super simple hash map designed for...
@@ -147,50 +149,53 @@ static Ledger ledger(int size) {
    * are implemented more efficiently.
    */
   @Deprecated
-  Object put(String tag, Object value);
+  Object put(@Nonnull String tag, Object value);
 
   /** Sets value without returning prior value - more efficient than {@link Map#put} */
-  void set(String tag, Object value);
+  void set(@Nonnull String tag, @Nonnull Object value);
 
   /**
    * Similar to {@link TagMap#set(String, Object)} but more efficient when working with
    * CharSequences and Strings. Depending on this situation, this methods avoids having to do type
    * resolution later on
    */
-  void set(String tag, CharSequence value);
+  void set(@Nonnull String tag, @Nonnull CharSequence value);
 
-  void set(String tag, boolean value);
+  void set(@Nonnull String tag, boolean value);
 
-  void set(String tag, int value);
+  void set(@Nonnull String tag, int value);
 
-  void set(String tag, long value);
+  void set(@Nonnull String tag, long value);
 
-  void set(String tag, float value);
+  void set(@Nonnull String tag, float value);
 
-  void set(String tag, double value);
+  void set(@Nonnull String tag, double value);
 
-  void set(EntryReader newEntry);
+  /** A null reader is a no-op (see the null-tolerance contract on {@link #getAndSet(Entry)}). */
+  void set(@Nullable EntryReader newEntry);
 
   /** sets the value while returning the prior Entry */
-  Entry getAndSet(String tag, Object value);
+  Entry getAndSet(@Nonnull String tag, Object value);
 
-  Entry getAndSet(String tag, CharSequence value);
+  Entry getAndSet(@Nonnull String tag, CharSequence value);
 
-  Entry getAndSet(String tag, boolean value);
+  Entry getAndSet(@Nonnull String tag, boolean value);
 
-  Entry getAndSet(String tag, int value);
+  Entry getAndSet(@Nonnull String tag, int value);
 
-  Entry getAndSet(String tag, long value);
+  Entry getAndSet(@Nonnull String tag, long value);
 
-  Entry getAndSet(String tag, float value);
+  Entry getAndSet(@Nonnull String tag, float value);
 
-  Entry getAndSet(String tag, double value);
+  Entry getAndSet(@Nonnull String tag, double value);
 
   /**
-   * TagMap specific method that places an Entry directly into an optimized TagMap avoiding need to
-   * allocate a new Entry object
+   * Places an Entry directly into the map, avoiding a new Entry allocation. Null-tolerant: a null
+   * {@code newEntry} is a no-op returning null, so an Entry producer (e.g. {@link
+   * Entry#create(String, Object)} for a null/empty value) can emit "no tag" without the caller
+   * filtering. Contrast the strict {@link Nonnull} {@code set(String, value)} setters.
    */
-  Entry getAndSet(Entry newEntry);
+  Entry getAndSet(@Nullable Entry newEntry);
 
   void putAll(Map<? extends String, ? extends Object> map);
 
@@ -368,40 +373,49 @@ final class Entry extends EntryChange implements Map.Entry<String, Object>, Entr
      */
     static final byte ANY = 0;
 
-    /** If value is non-null, returns a new TagMap.Entry If value is null, returns null */
-    public static final Entry create(String tag, Object value) {
-      // NOTE: From the static typing, it is possible that value is a primitive box, so need to call
-      // Any variant
-
-      return (value == null) ? null : TagMap.Entry.newAnyEntry(tag, value);
+    /**
+     * Entry for {@code (tag, value)}, or null when {@code value} is null or an empty {@code
+     * CharSequence} -- checked by runtime type, so an empty String passed as {@code Object} skips
+     * the same as via the {@link #create(String, CharSequence)} overload.
+     */
+    @Nullable
+    public static final Entry create(@Nonnull String tag, Object value) {
+      if (value == null) {
+        return null;
+      }
+      if (value instanceof CharSequence && ((CharSequence) value).length() == 0) {
+        return null;
+      }
+      return TagMap.Entry.newAnyEntry(tag, value);
     }
 
     /** If value is non-null, returns a new TagMap.Entry If value is null or empty, returns null */
-    public static final Entry create(String tag, CharSequence value) {
+    @Nullable
+    public static final Entry create(@Nonnull String tag, CharSequence value) {
       // NOTE: From the static typing, we know that value is not a primitive box
 
       return (value == null || value.length() == 0)
           ? null
           : TagMap.Entry.newObjectEntry(tag, value);
     }
 
-    public static final Entry create(String tag, boolean value) {
+    public static final Entry create(@Nonnull String tag, boolean value) {
       return TagMap.Entry.newBooleanEntry(tag, value);
     }
 
-    public static final Entry create(String tag, int value) {
+    public static final Entry create(@Nonnull String tag, int value) {
       return TagMap.Entry.newIntEntry(tag, value);
     }
 
-    public static final Entry create(String tag, long value) {
+    public static final Entry create(@Nonnull String tag, long value) {
       return TagMap.Entry.newLongEntry(tag, value);
     }
 
-    public static final Entry create(String tag, float value) {
+    public static final Entry create(@Nonnull String tag, float value) {
       return TagMap.Entry.newFloatEntry(tag, value);
     }
 
-    public static final Entry create(String tag, double value) {
+    public static final Entry create(@Nonnull String tag, double value) {
       return TagMap.Entry.newDoubleEntry(tag, value);
     }
 
@@ -1357,6 +1371,9 @@ public Object put(String tag, Object value) {
 
   @Override
   public void set(TagMap.EntryReader newEntryReader) {
+    if (newEntryReader == null) {
+      return;
+    }
     this.getAndSet(newEntryReader.entry());
   }
 
@@ -1397,6 +1414,10 @@ public void set(String tag, double value) {
 
   @Override
   public Entry getAndSet(Entry newEntry) {
+    if (newEntry == null) {
+      return null;
+    }
+
     this.checkWriteAccess();
 
     Object[] thisBuckets = this.buckets;

internal-api/src/test/java/datadog/trace/api/TagMapNullToleranceTest.java

@@ -0,0 +1,66 @@
+package datadog.trace.api;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import org.junit.jupiter.api.Test;
+
+/**
+ * Pins the Entry-pathway null-tolerance contract: {@code create(...)} returns null for a null or
+ * empty value, and the {@code Entry} sinks ({@code set(Entry)} / {@code getAndSet(Entry)}) treat
+ * null as a no-op -- so a null/empty value flows through as "no tag" without any caller guarding.
+ */
+class TagMapNullToleranceTest {
+
+  @Test
+  void createReturnsNullForNullOrEmptyValue() {
+    // CharSequence overload: null or empty -> null
+    assertNull(TagMap.Entry.create("k", (CharSequence) null));
+    assertNull(TagMap.Entry.create("k", ""));
+
+    // Object overload: null -> null, and an empty String typed as Object -> null (runtime check,
+    // so the convention holds regardless of the static type at the call site)
+    assertNull(TagMap.Entry.create("k", (Object) null));
+    assertNull(TagMap.Entry.create("k", (Object) ""));
+
+    // Non-empty values still produce an entry, whatever the static type
+    assertNotNull(TagMap.Entry.create("k", "v"));
+    assertNotNull(TagMap.Entry.create("k", (Object) "v"));
+    // Non-CharSequence Object has no notion of "empty" -> always an entry
+    assertNotNull(TagMap.Entry.create("k", (Object) Integer.valueOf(0)));
+  }
+
+  @Test
+  void getAndSetToleratesNullEntry() {
+    TagMap map = TagMap.create();
+    assertNull(map.getAndSet((TagMap.Entry) null), "null entry is a no-op returning null");
+    assertEquals(0, map.size(), "no tag added");
+  }
+
+  @Test
+  void setToleratesNullReader() {
+    TagMap map = TagMap.create();
+    map.set((TagMap.EntryReader) null); // must not throw
+    assertEquals(0, map.size(), "no tag added");
+  }
+
+  @Test
+  void nullOrEmptyFlowsThroughTheEntryPathwayAsNoTag() {
+    TagMap map = TagMap.create();
+
+    // The seamless case: create(...) -> set(Entry) with a null/empty value, no caller guard.
+    map.set(TagMap.Entry.create("empty", ""));
+    map.set(TagMap.Entry.create("emptyObj", (Object) ""));
+    map.set(TagMap.Entry.create("nul", (CharSequence) null));
+    assertEquals(0, map.size(), "null/empty values leave no tags behind");
+    assertFalse(map.containsKey("empty"));
+
+    // A real value still lands.
+    map.set(TagMap.Entry.create("present", "v"));
+    assertEquals(1, map.size());
+    assertTrue(map.containsKey("present"));
+  }
+}