api: fix duplicate tag bug in ArrayTagSet (#1074)

It was possible to provide a list of `Tag`s to an `ArrayTagSet` with duplicate values. The duplicates would not be de-duped at merge time if the duplicate keys in the list were lexically less than that of existing tags. Merge now checks the last written tag value to make sure we don't write a duplicate key. --------- Co-authored-by: Brian Harrington <[email protected]>
Netflix · Aug 29, 2023 · 8d8e2b5 · 8d8e2b5
1 parent a4c0fe0
commit 8d8e2b5
Show file tree

Hide file tree

Showing 2 changed files with 171 additions and 5 deletions.
diff --git a/spectator-api/src/main/java/com/netflix/spectator/api/ArrayTagSet.java b/spectator-api/src/main/java/com/netflix/spectator/api/ArrayTagSet.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2014-2020 Netflix, Inc.
+ * Copyright 2014-2023 Netflix, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -336,9 +336,9 @@ private static void insertionSort(String[] ts, int length) {
 
   /**
    * Merge and dedup any entries in {@code ts} that have the same key. The last entry
-   * with a given key will get selected.
+   * with a given key will get selected. Each list must be sorted by key before processing.
    */
-  private static int merge(String[] dst, String[] srcA, int lengthA, String[] srcB, int lengthB) {
+  static int merge(String[] dst, String[] srcA, int lengthA, String[] srcB, int lengthB) {
     int i = 0;
     int ai = 0;
     int bi = 0;
@@ -350,13 +350,20 @@ private static int merge(String[] dst, String[] srcA, int lengthA, String[] srcB
       String bv = srcB[bi + 1];
       int cmp = ak.compareTo(bk);
       if (cmp < 0) {
+        // srcA should already have been deduped as it comes from the tag list
         dst[i++] = ak;
         dst[i++] = av;
         ai += 2;
       } else if (cmp > 0) {
+        // Choose last value for a given key if there are duplicates. It is possible srcB
+        // will contain duplicates if the user supplied a type like a list.
+        int j = bi + 2;
+        for (; j < lengthB && bk.equals(srcB[j]); j += 2) {
+          bv = srcB[j + 1];
+        }
         dst[i++] = bk;
         dst[i++] = bv;
-        bi += 2;
+        bi = j;
       } else {
         // Newer tags should override, use source B if there are duplicate keys.
         // If source B has duplicates, then use the last value for the given key.

diff --git a/spectator-api/src/test/java/com/netflix/spectator/api/ArrayTagSetTest.java b/spectator-api/src/test/java/com/netflix/spectator/api/ArrayTagSetTest.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2014-2020 Netflix, Inc.
+ * Copyright 2014-2023 Netflix, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -641,6 +641,48 @@ public void testCreateWithSortedMapCustomComparatorNaturalOrder() {
     Assertions.assertEquals(Arrays.asList(Tag.of("a", "v1"), Tag.of("b", "v2"), Tag.of("c", "v3"), Tag.of("d", "v4")), tagList);
   }
 
+  @Test
+  public void addReplace() {
+    ArrayTagSet ts1 = ArrayTagSet.create("k1", "v1");
+    ArrayTagSet ts2 = ArrayTagSet.create("k1", "v2");
+    ArrayTagSet expected = ArrayTagSet.create("k1", "v2");
+    Assertions.assertEquals(expected, ts1.addAll(ts2));
+
+    List<Tag> list = new ArrayList<>();
+    list.add(new BasicTag("k1", "v3"));
+    list.add(new BasicTag("k1", "v2"));
+    Assertions.assertEquals(expected, ts1.addAll(list));
+
+    list.clear();
+    list.add(new BasicTag("k1", "v3"));
+    list.add(new BasicTag("k1", "v2"));
+    Assertions.assertEquals(expected, ArrayTagSet.create(new HashMap<>()).addAll(list));
+
+    Assertions.assertEquals(expected, expected.addAll(new ArrayList<>()));
+
+    Assertions.assertEquals(expected, expected.addAll(new HashMap<>()));
+
+    Assertions.assertEquals(expected, ts1.addAll(new BadTagList("k1", "v2")));
+
+    Assertions.assertEquals(expected, ts1.addAll(new TagIterator("k1", "v2")));
+
+    Assertions.assertEquals(expected, ts1.addAll(new Tag[] { new BasicTag("k1", "v2")}));
+
+    ConcurrentHashMap<String, String> cMap = new ConcurrentHashMap<>();
+    cMap.put("k1", "v2");
+    Assertions.assertEquals(expected, ts1.addAll(cMap));
+
+    HashMap<String, String> hMap = new HashMap<>();
+    hMap.put("k1", "v2");
+    Assertions.assertEquals(expected, ts1.addAll(hMap));
+
+    Assertions.assertEquals(expected, ts1.add("k1", "v2"));
+
+    Assertions.assertEquals(expected, ts1.add(new BasicTag("k1", "v2")));
+
+    Assertions.assertEquals(expected, ArrayTagSet.create("k1", "v1", "k1", "v2"));
+  }
+
   @Test
   public void testAddBeginning() {
     ArrayTagSet tags = ArrayTagSet.create("a", "v1", "b", "v2", "c", "v3");
@@ -675,4 +717,121 @@ public void testAddUpdatesExistingWithSameTag() {
     ArrayTagSet updated = tags.add("c", "v3");
     Assertions.assertSame(tags, updated);
   }
+
+  @Test
+  public void mergeDuplicatesTwoKeys() {
+    String[] dst = new String[8];
+    String[] srcA = {"a", "2"};
+    String[] srcB = {"a", "1", "b", "1", "b", "2"};
+
+    int n = ArrayTagSet.merge(dst, srcA, srcA.length, srcB, srcB.length);
+    Assertions.assertArrayEquals(new String[] {"a", "1", "b", "2", null, null, null, null}, dst);
+    Assertions.assertEquals(4, n);
+  }
+
+  @Test
+  public void mergeDuplicatesSrcB() {
+    String[] dst = new String[8];
+    String[] srcA = {"b", "1"};
+    String[] srcB = {"a", "1", "a", "2"};
+    int n = ArrayTagSet.merge(dst, srcA, srcA.length, srcB, srcB.length);
+    Assertions.assertArrayEquals(new String[] {"a", "2", "b", "1", null, null, null, null}, dst);
+    Assertions.assertEquals(4, n);
+  }
+
+  @Test
+  public void mergeDuplicatesBoth() {
+    String[] dst = new String[8];
+    String[] srcA = {"a", "1"};
+    String[] srcB = {"a", "2", "a", "3"};
+    int n = ArrayTagSet.merge(dst, srcA, srcA.length, srcB, srcB.length);
+    Assertions.assertArrayEquals(new String[] {"a", "3", null, null, null, null, null, null}, dst);
+    Assertions.assertEquals(2, n);
+  }
+
+  @Test
+  public void addAllDuplicates() {
+    ArrayTagSet existing = ArrayTagSet.create("a", "foo");
+    List<Tag> tags = Arrays.asList(
+        Tag.of("b", "bar"),
+        Tag.of("b", "ERROR")
+    );
+    ArrayTagSet updated = existing.addAll(tags);
+    assertDistinct(updated);
+
+    // <= 1.6.9 would throw here as the two values in the array list would be merged
+    // into the result without checking for duplicates.
+    existing = ArrayTagSet.create("b", "foo");
+    tags = Arrays.asList(
+        Tag.of("a", "bar"),
+        Tag.of("a", "ERROR")
+    );
+    updated = existing.addAll(tags);
+    assertDistinct(updated);
+
+    existing = ArrayTagSet.create("b", "foo");
+    tags = Arrays.asList(
+        Tag.of("a", "bar"),
+        Tag.of("b", "boo"),
+        Tag.of("a", "ERROR")
+    );
+    updated = existing.addAll(tags);
+    assertDistinct(updated);
+  }
+
+  class TagIterator implements Iterable<Tag> {
+    String[] tags;
+    TagIterator(String... tags) {
+      this.tags = tags;
+    }
+
+    @Override
+    public Iterator<Tag> iterator() {
+      return new Iterator<Tag>() {
+        int i = 0;
+
+        @Override
+        public boolean hasNext() {
+          return i < tags.length;
+        }
+
+        @Override
+        public Tag next() {
+          return new BasicTag(tags[i++], tags[i++]);
+        }
+      };
+    }
+  }
+
+  class BadTagList implements TagList {
+
+    String[] tags;
+    BadTagList(String... tags) {
+      this.tags = tags;
+    }
+
+    @Override
+    public String getKey(int i) {
+      return tags[i * 2];
+    }
+
+    @Override
+    public String getValue(int i) {
+      return tags[(i * 2) + 1];
+    }
+
+    @Override
+    public int size() {
+      return tags.length / 2;
+    }
+  }
+
+  private void assertDistinct(TagList tags) {
+    try {
+      StreamSupport.stream(tags.spliterator(), false)
+          .collect(Collectors.toMap(Tag::key, Tag::value));
+    } catch (Exception e) {
+      throw new AssertionError("failed to convert tags to map", e);
+    }
+  }
 }