Merge pull request #305 from brharrington/tag-set-varargs

fix corrupt tag set when using varargs
Netflix · Jun 8, 2016 · adbc097 · adbc097
2 parents d494c2d + 5ae39a2
commit adbc097
Show file tree

Hide file tree

Showing 2 changed files with 42 additions and 4 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
@@ -167,14 +167,19 @@ ArrayTagSet addAll(Map<String, String> ts) {
 
   /** Add a collection of tags to the set. */
   ArrayTagSet addAll(String[] ts) {
+    if (ts.length % 2 != 0) {
+      throw new IllegalArgumentException("array length must be even, (length=" + ts.length + ")");
+    }
+
     if (ts.length == 0) {
       return this;
     } else {
       int tsLength = ts.length / 2;
       Tag[] newTags = new Tag[length + tsLength];
       System.arraycopy(tags, 0, newTags, 0, length);
       for (int i = 0; i < tsLength; ++i) {
-        newTags[length + i] = new BasicTag(ts[i], ts[i + 1]);
+        final int j = i * 2;
+        newTags[length + i] = new BasicTag(ts[j], ts[j + 1]);
       }
       return dedup(newTags);
     }

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
@@ -121,6 +121,34 @@ public void testCreateFromArrayTagSet() {
     Assert.assertEquals(ts1, ts2);
   }
 
+  @Test
+  public void testCreateFromVarargs2() {
+    Map<String, String> m = new HashMap<>();
+    m.put("k1", "v1");
+    m.put("k2", "v2");
+    ArrayTagSet ts1 = ArrayTagSet.create(m);
+    ArrayTagSet ts2 = ArrayTagSet.create("k1", "v1", "k2", "v2");
+    Assert.assertEquals(ts1, ts2);
+  }
+
+  @Test
+  public void testCreateFromVarargs5() {
+    Map<String, String> m = new HashMap<>();
+    m.put("a", "1");
+    m.put("b", "2");
+    m.put("c", "3");
+    m.put("d", "4");
+    m.put("e", "5");
+    ArrayTagSet ts1 = ArrayTagSet.create(m);
+    ArrayTagSet ts2 = ArrayTagSet.create("a", "1", "b", "2", "c", "3", "d", "4", "e", "5");
+    Assert.assertEquals(ts1, ts2);
+  }
+
+  @Test(expected = IllegalArgumentException.class)
+  public void testCreateFromVarargsOdd() {
+    ArrayTagSet.create("a", "1", "b");
+  }
+
   @Test
   public void testCreateFromEmptyIterable() {
     Assert.assertEquals(ArrayTagSet.EMPTY, ArrayTagSet.create(Collections.emptyList()));
@@ -162,7 +190,8 @@ public void testCreateFromMultiValueMap() {
     Map<String, String> tags = new HashMap<>();
     tags.put("k1", "v1");
     tags.put("k2", "v2");
-    Assert.assertEquals(ArrayTagSet.create("k1", "v1").addAll(ArrayTagSet.create("k2", "v2")), ArrayTagSet.create(tags));
+    Assert.assertEquals(ArrayTagSet.create("k1", "v1")
+        .addAll(ArrayTagSet.create("k2", "v2")), ArrayTagSet.create(tags));
   }
 
   @Test(expected = NullPointerException.class)
@@ -218,7 +247,9 @@ public void testMergeSingleValueAsList() {
   public void testMergeMultipleValuesAsList() {
     ArrayList<Tag> prefix = new ArrayList<>();
     ArrayTagSet initial = ArrayTagSet.create("k3", "v3");
-    ArrayTagSet expected = ArrayTagSet.create("k1", "v1").addAll(ArrayTagSet.create("k2", "v2")).addAll(ArrayTagSet.create("k3", "v3"));
+    ArrayTagSet expected = ArrayTagSet.create("k1", "v1")
+        .addAll(ArrayTagSet.create("k2", "v2"))
+        .addAll(ArrayTagSet.create("k3", "v3"));
 
     prefix.add(new BasicTag("k1", "v1"));
     prefix.add(new BasicTag("k2", "v2"));
@@ -254,7 +285,9 @@ public void testMergeSingleValueAsMap() {
   public void testMergeMultipleValuesAsMap() {
     Map<String, String> extra = new HashMap<>();
     ArrayTagSet initial = ArrayTagSet.create("k3", "v3");
-    ArrayTagSet expected = ArrayTagSet.create("k1", "v1").addAll(ArrayTagSet.create("k2", "v2")).addAll(ArrayTagSet.create("k3", "v3"));
+    ArrayTagSet expected = ArrayTagSet.create("k1", "v1")
+        .addAll(ArrayTagSet.create("k2", "v2"))
+        .addAll(ArrayTagSet.create("k3", "v3"));
 
     extra.put("k1", "v1");
     extra.put("k2", "v2");