fix tagging edge cases

add comment and use Arrays.asList()

brain fart between camelCase and snake_case

logstash-core-event-java/src/main/java/org/logstash/Event.java

@@ -15,6 +15,7 @@ import org.logstash.ackedqueue.Queueable;
 import java.io.IOException;
 import java.io.Serializable;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Date;
 import java.util.HashMap;
 import java.util.List;
@@ -76,9 +77,20 @@ public class Event implements Cloneable, Serializable, Queueable {
         this.metadata_accessors = new Accessors(this.metadata);
 
         this.cancelled = false;
-        this.timestamp = initTimestamp(data.get(TIMESTAMP));
+
+        Object providedTimestamp = data.get(TIMESTAMP);
+        // keep reference to the parsedTimestamp for tagging below
+        Timestamp parsedTimestamp = initTimestamp(providedTimestamp);
+        this.timestamp = (parsedTimestamp == null) ? Timestamp.now() : parsedTimestamp;
+
         this.data.put(TIMESTAMP, this.timestamp);
         this.accessors = new Accessors(this.data);
+
+        // the tag() method has to be called after the Accessors initialization
+        if (parsedTimestamp == null) {
+            tag(TIMESTAMP_FAILURE_TAG);
+            this.setField(TIMESTAMP_FAILURE_FIELD, providedTimestamp);
+        }
     }
 
     public Map<String, Object> getData() {
@@ -338,22 +350,36 @@ public class Event implements Cloneable, Serializable, Queueable {
             logger.warn("Error parsing " + TIMESTAMP + " string value=" + o.toString());
         }
 
-        tag(TIMESTAMP_FAILURE_TAG);
-        this.data.put(TIMESTAMP_FAILURE_FIELD, o);
-
-        return Timestamp.now();
+        return null;
     }
 
     public void tag(String tag) {
-        List<Object> tags = (List<Object>) this.data.get("tags");
-        if (tags == null) {
-            tags = new ArrayList<>();
-            this.data.put("tags", tags);
+        List<Object> tags;
+        Object _tags = this.getField("tags");
+
+        // short circuit the null case where we know we won't need deduplication step below at the end
+        if (_tags == null) {
+            setField("tags", Arrays.asList(tag));
+            return;
         }
 
+        // assign to tags var the proper List of either the existing _tags List or a new List containing whatever non-List item was in the tags field
+        if (_tags instanceof List) {
+            tags = (List<Object>) _tags;
+        } else {
+            // tags field has a value but not in a List, convert in into a List
+            tags = new ArrayList<>();
+            tags.add(_tags);
+        }
+
+        // now make sure the tags list does not already contain the tag
+        // TODO: we should eventually look into using alternate data structures to do more efficient dedup but that will require properly defining the tagging API too
         if (!tags.contains(tag)) {
             tags.add(tag);
         }
+
+        // set that back as a proper BiValue
+        this.setField("tags", tags);
     }
 
     public byte[] serialize() throws IOException {

logstash-core-event-java/src/test/java/org/logstash/EventTest.java

@@ -1,5 +1,6 @@
 package org.logstash;
 
+import org.junit.Assert;
 import org.junit.Test;
 
 import java.io.IOException;
@@ -266,4 +267,28 @@ public class EventTest {
     public void testFromJsonWithPartialInvalidJsonArray() throws Exception {
         Event.fromJson("[{\"foo\":\"bar\"}, 1]");
     }
+
+    @Test
+    public void testTagOnEmptyTagsField() throws Exception {
+        Event e = new Event();
+        e.tag("foo");
+
+        List<String> tags = (List<String>)e.getField("tags");
+        assertEquals(tags.size(), 1);
+        assertEquals(tags.get(0), "foo");
+    }
+
+    @Test
+    public void testTagOnExistingTagsField() throws Exception {
+        Map data = new HashMap();
+        data.put("tags", "foo");
+        Event e = new Event(data);
+        e.tag("bar");
+
+        List<String> tags = (List<String>)e.getField("tags");
+        assertEquals(tags.size(), 2);
+        assertEquals(tags.get(0), "foo");
+        assertEquals(tags.get(1), "bar");
+      }
+
 }

logstash-core/lib/logstash/filters/base.rb

@@ -195,14 +195,20 @@ class LogStash::Filters::Base < LogStash::Plugin
     # note below that the tags array field needs to be updated then reassigned to the event.
     # this is important because a construct like event["tags"].delete(tag) will not work
     # in the current Java event implementation. see https://github.com/elastic/logstash/issues/4140
+
+    tags = event.get("tags")
+    return unless tags
+
+    tags = Array(tags)
     @remove_tag.each do |tag|
-      tags = event.get("tags")
-      break if tags.nil? || tags.empty?
+      break if tags.empty?
+
       tag = event.sprintf(tag)
       @logger.debug? and @logger.debug("filters/#{self.class.name}: removing tag", :tag => tag)
       tags.delete(tag)
-      event.set("tags", tags)
     end
+
+    event.set("tags", tags)
   end # def filter_matched
 
   protected

logstash-core/lib/logstash/util/decorators.rb

@@ -33,17 +33,20 @@ module LogStash::Util
     end
 
     # tags is an array of string. sprintf syntax can be used.
-    def add_tags(tags, event, pluginname)
-      tags.each do |tag|
-        tag = event.sprintf(tag)
-        self.logger.debug? and self.logger.debug("#{pluginname}: adding tag", "tag" => tag)
+    def add_tags(new_tags, event, pluginname)
+      tags = event.get("tags")
+      tags = tags.nil? ? [] : Array(tags)
+
+      new_tags.each do |new_tag|
+        new_tag = event.sprintf(new_tag)
+        self.logger.debug? and self.logger.debug("#{pluginname}: adding tag", "tag" => new_tag)
         # note below that the tags array field needs to be updated then reassigned to the event.
         # this is important because a construct like event["tags"] << tag will not work
         # in the current Java event implementation. see https://github.com/elastic/logstash/issues/4140
-        tags = event.get("tags") || []
-        tags << tag
-        event.set("tags", tags)
+        tags << new_tag  #unless tags.include?(new_tag)
       end
+
+      event.set("tags", tags)
     end
 
   end # module LogStash::Util::Decorators

logstash-core/spec/logstash/filters/base_spec.rb

@@ -98,6 +98,39 @@ describe LogStash::Filters::NOOP do
     end
   end
 
+  describe "tags parsing with one tag as string value" do
+    config <<-CONFIG
+    filter {
+      noop {
+        add_tag => ["bar"]
+      }
+    }
+    CONFIG
+
+    sample("type" => "noop") do
+      insist { subject.get("tags") } == ["bar"]
+    end
+
+    sample("type" => "noop", "tags" => "foo") do
+      insist { subject.get("tags") } == ["foo", "bar"]
+    end
+  end
+
+  describe "tags parsing with duplicate tags" do
+    config <<-CONFIG
+    filter {
+      noop {
+        add_tag => ["foo"]
+      }
+    }
+    CONFIG
+
+    sample("type" => "noop", "tags" => "foo") do
+      # this is completely weird but seems to be already expected in other specs
+      insist { subject.get("tags") } == ["foo", "foo"]
+    end
+  end
+
   describe "tags parsing with multiple tags" do
     config <<-CONFIG
     filter {
@@ -133,6 +166,18 @@ describe LogStash::Filters::NOOP do
     }
     CONFIG
 
+    sample("type" => "noop", "tags" => "foo") do
+      insist { subject.get("tags") } == ["foo"]
+    end
+
+    sample("type" => "noop", "tags" => "t2") do
+      insist { subject.get("tags") } == []
+    end
+
+    sample("type" => "noop", "tags" => ["t2"]) do
+      insist { subject.get("tags") } == []
+    end
+
     sample("type" => "noop", "tags" => ["t4"]) do
       insist { subject.get("tags") } == ["t4"]
     end