Gh680 jsonproviders no auto start (#681)

Issue #680: no auto-start of JsonProviders
- remove Lifecycle from the JsonProvider interface to prevent Logstash from auto-starting the provider
- declare the start/stop method initially defined by the Lifecycle interface in the JsonProvider interface
- now that start() is called only once, parse the JSON string during start()

Issue #679: remaining characters after reading JSON string
- raise an error if some characters remain after reading a JsonNode out of the string

src/main/java/net/logstash/logback/composite/AbstractPatternJsonProvider.java

@@ -67,28 +67,33 @@ public String getPattern() {
 
     public void setPattern(final String pattern) {
         this.pattern = pattern;
-        parse();
     }
 
     @Override
     public void setJsonFactory(JsonFactory jsonFactory) {
         this.jsonFactory = Objects.requireNonNull(jsonFactory);
-        parse();
     }
 
+    @Override
+    public void start() {
+        if (jsonFactory == null) {
+            throw new IllegalStateException("JsonFactory has not been set");
+        }
+        initializeNodeWriter();
+
+        super.start();
+    }
+
+
     /**
      * Parses the pattern into a {@link NodeWriter}.
-     * We do this when the properties are set instead of on {@link #start()},
-     * because {@link #start()} is called by logstash's xml parser
-     * before the Formatter has had an opportunity to set the jsonFactory.
      */
-    private void parse() {
-        if (pattern != null && jsonFactory != null) {
-            AbstractJsonPatternParser<Event> parser = createParser(this.jsonFactory);
-            parser.setOmitEmptyFields(omitEmptyFields);
-            nodeWriter = parser.parse(pattern);
-        }
+    private void initializeNodeWriter() {
+        AbstractJsonPatternParser<Event> parser = createParser(this.jsonFactory);
+        parser.setOmitEmptyFields(omitEmptyFields);
+        this.nodeWriter = parser.parse(pattern);
     }
+
 
     /**
      * When {@code true}, fields whose values are considered empty ({@link AbstractJsonPatternParser#isEmptyValue(Object)}})

src/main/java/net/logstash/logback/composite/GlobalCustomFieldsJsonProvider.java

@@ -18,12 +18,13 @@
 import java.io.IOException;
 import java.util.Iterator;
 import java.util.Map.Entry;
+import java.util.Objects;
 
 import ch.qos.logback.core.spi.DeferredProcessingAware;
 import com.fasterxml.jackson.core.JsonFactory;
 import com.fasterxml.jackson.core.JsonGenerator;
-import com.fasterxml.jackson.core.JsonParser;
 import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.node.ObjectNode;
 
 public class GlobalCustomFieldsJsonProvider<Event extends DeferredProcessingAware> extends AbstractJsonProvider<Event> implements JsonFactoryAware {
 
@@ -36,8 +37,12 @@ public class GlobalCustomFieldsJsonProvider<Event extends DeferredProcessingAwar
     /**
      * When non-null, the fields in this JsonNode will be embedded in the logstash json.
      */
-    private JsonNode customFieldsNode;
+    private ObjectNode customFieldsNode;
 
+    /**
+     * The factory used to convert the JSON string into a valid {@link ObjectNode} when custom
+     * fields are set as text instead of a pre-parsed Jackson ObjectNode.
+     */
     private JsonFactory jsonFactory;
 
     @Override
@@ -58,46 +63,90 @@ private void writeFieldsOfNode(JsonGenerator generator, JsonNode node) throws IO
         }
     }
 
+    /**
+     * Start the provider.
+     * 
+     * <p>The provider is started even when it fails to parse the {@link #customFields} JSON string.
+     * An ERROR status is emitted instead and no exception is thrown.
+     */
     @Override
     public void start() {
         initializeCustomFields();
         super.start();
     }
 
     private void initializeCustomFields() {
-        if (this.customFields != null && jsonFactory != null) {
-            try (JsonParser parser = this.jsonFactory.createParser(customFields)) {
-                this.customFieldsNode = parser.readValueAsTree();
-            } catch (IOException e) {
-                addError("Failed to parse custom fields [" + customFields + "]", e);
-            }
+        if (customFieldsNode != null || customFields == null) {
+            return;
+        }
+        if (jsonFactory == null) {
+            throw new IllegalStateException("JsonFactory has not been set");
+        }
+
+        try {
+            this.customFieldsNode = JsonReadingUtils.readFullyAsObjectNode(this.jsonFactory, this.customFields);
+        } catch (IOException e) {
+            addError("[customFields] is not a valid JSON object", e);
         }
     }
 
+    /**
+     * Set the custom fields as a JSON string.
+     * The string will be parsed when the provider is {@link #start()}.
+     * 
+     * @param customFields the custom fields as JSON string.
+     */
     public void setCustomFields(String customFields) {
-        this.customFields = customFields;
         if (isStarted()) {
-            initializeCustomFields();
+            throw new IllegalStateException("Configuration cannot be changed while the provider is started");
         }
+
+        this.customFields = customFields;
+        this.customFieldsNode = null;
     }
 
     public String getCustomFields() {
         return customFields;
     }
 
-    public JsonNode getCustomFieldsNode() {
+    public ObjectNode getCustomFieldsNode() {
         return this.customFieldsNode;
     }
-
+
+    /**
+     * Set the custom JSON fields.
+     * Must be a valid JsonNode that maps to a JSON object structure, i.e. an {@link ObjectNode}.
+     * 
+     * @param customFields a {@link JsonNode} whose properties must be added as custom fields.
+     * @deprecated use {@link #setCustomFieldsNode(ObjectNode)} instead.
+     * @throws IllegalArgumentException if the argument is not a {@link ObjectNode}.
+     */
+    @Deprecated
     public void setCustomFieldsNode(JsonNode customFields) {
-        this.customFieldsNode = customFields;
-        if (this.customFieldsNode != null && customFields == null) {
-            this.customFields = this.customFieldsNode.toString();
+        if (customFields != null && !(customFields instanceof ObjectNode)) {
+            throw new IllegalArgumentException("Must be an ObjectNode");
+        }
+        setCustomFieldsNode((ObjectNode) customFields);
+    }
+
+
+    /**
+     * Use the fields of the given {@link ObjectNode} (may be empty).
+     * 
+     * @param customFields the JSON object whose fields as added as custom fields
+     */
+    public void setCustomFieldsNode(ObjectNode customFields) {
+        if (isStarted()) {
+            throw new IllegalStateException("Configuration cannot be changed while the provider is started");
         }
+
+        this.customFieldsNode = customFields;
+        this.customFields = null;
     }
 
+
     @Override
     public void setJsonFactory(JsonFactory jsonFactory) {
-        this.jsonFactory = jsonFactory;
+        this.jsonFactory = Objects.requireNonNull(jsonFactory);
     }
 }

src/main/java/net/logstash/logback/composite/JsonProvider.java

@@ -21,15 +21,14 @@
 import ch.qos.logback.classic.spi.ILoggingEvent;
 import ch.qos.logback.core.spi.ContextAware;
 import ch.qos.logback.core.spi.DeferredProcessingAware;
-import ch.qos.logback.core.spi.LifeCycle;
 import com.fasterxml.jackson.core.JsonGenerator;
 
 /**
  * Contributes to the JSON output being written for the given Event.
  *
  * @param <Event> type of event ({@link ILoggingEvent} or {@link IAccessEvent}).
  */
-public interface JsonProvider<Event extends DeferredProcessingAware> extends LifeCycle, ContextAware {
+public interface JsonProvider<Event extends DeferredProcessingAware> extends ContextAware {
 
     /**
      * Writes information about the event, to the given generator.
@@ -53,4 +52,19 @@ public interface JsonProvider<Event extends DeferredProcessingAware> extends Lif
      */
     void prepareForDeferredProcessing(Event event);
 
+    /**
+     * Start the provider after all configuration properties are set.
+     */
+    void start();
+
+    /**
+     * Stop the provider
+     */
+    void stop();
+
+    /**
+     * Report whether the provider is started or not.
+     * @return {@code true} if the provider is started, {@code false} otherwise.
+     */
+    boolean isStarted();
 }

src/main/java/net/logstash/logback/composite/JsonReadingUtils.java

@@ -0,0 +1,95 @@
+/*
+ * Copyright 2013-2021 the original author or authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package net.logstash.logback.composite;
+
+import java.io.IOException;
+
+import com.fasterxml.jackson.core.JsonFactory;
+import com.fasterxml.jackson.core.JsonParseException;
+import com.fasterxml.jackson.core.JsonParser;
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.node.ObjectNode;
+
+/**
+ * Utilities for reading/parsing JSON string.
+ *
+ * <p>Note: This class is for internal use only and subject to backward incompatible change
+ * at any time.
+ *
+ * @author brenuart
+ */
+public class JsonReadingUtils {
+
+    private JsonReadingUtils() {
+        // utility class - prevent instantiation
+    }
+
+
+    /**
+     * Fully read the supplied JSON string into the equivalent {@link JsonNode}.
+     * 
+     * Throws a {@link JsonParseException} if the string is not fully read after a first valid JsonNode is found.
+     * This may happen for input like <em>10 foobar</em> that would otherwise return a NumericNode with value
+     * {@code 10} leaving <em>foobar</em> unread.
+     * 
+     * @param jsonFactory the {@link JsonFactory} from which to obtain a {@link JsonParser} to read the JSON string.
+     * @param json the JSON string to read
+     * @return the {@link JsonNode} corresponding to the input string or {@code null} if the string is null or empty.
+     * @throws IOException if there is either an underlying I/O problem or decoding issue
+     */
+    public static JsonNode readFully(JsonFactory jsonFactory, String json) throws IOException {
+        if (json == null) {
+            return null;
+        }
+
+        final String trimmedJson = json.trim();
+        try (JsonParser parser = jsonFactory.createParser(trimmedJson)) {
+            final JsonNode tree = parser.readValueAsTree();
+
+            if (parser.getCurrentLocation().getCharOffset() < trimmedJson.length()) {
+                /*
+                 * If the full trimmed string was not read, then the full trimmed string contains a json value plus other text.
+                 * For example, trimmedValue = '10 foobar', or 'true foobar', or '{"foo","bar"} baz'.
+                 * In these cases readTree will only read the first part, and will not read the remaining text.
+                 */
+                throw new JsonParseException(parser, "unexpected character");
+            }
+
+            return tree;
+        }
+    }
+
+
+    /**
+     * Fully read a JSON string into an {@link ObjectNode}, throwing a {@link JsonParseException} if the supplied string
+     * is not a valid JSON object representation.
+     * 
+     * @param jsonFactory the {@link JsonFactory} from which to obtain a {@link JsonParser} to read the JSON string.
+     * @param json the JSON string to read
+     * @return the {@link JsonNode} corresponding to the input string or {@code null} if the string is null or empty.
+     * @throws IOException if there is either an underlying I/O problem or decoding issue
+     * 
+     * @see JsonReadingUtils#readFully(JsonFactory, String)
+     */
+    public static ObjectNode readFullyAsObjectNode(JsonFactory jsonFactory, String json) throws IOException {
+        final JsonNode node = readFully(jsonFactory, json);
+
+        if (node != null && !(node instanceof ObjectNode)) {
+            throw new JsonParseException(null, "expected a JSON object representation");
+        }
+
+        return (ObjectNode) node;    }
+}

src/main/java/net/logstash/logback/composite/JsonWritingUtils.java

@@ -23,6 +23,12 @@
 import com.fasterxml.jackson.core.JsonGenerator;
 import com.fasterxml.jackson.databind.JsonMappingException;
 
+/**
+ * Utilities for writing JSON
+ *
+ * <p>Note: This class is for internal use only and subject to backward incompatible change
+ * at any time.
+ */
 public class JsonWritingUtils {
 
     /**