jackson stream read constraints: code-based defaults (#16854)

* Revert "Apply Jackson stream read constraints defaults at runtime (#16832)"

This reverts commit cc608eb.

* jackson stream read constraints: code-based defaults

refactors stream read constraints to couple default values with their
associated overrides, which allows us to have more descriptive logging
that includes provenance of the value that has been applied.

Author: yaauie

config/jvm.options

@@ -71,11 +71,11 @@
 # text values with sizes less than or equal to this limit will be treated as invalid.
 # This value should be higher than `logstash.jackson.stream-read-constraints.max-number-length`.
 # The jackson library defaults to 20000000 or 20MB, whereas Logstash defaults to 200MB or 200000000 characters.
--Dlogstash.jackson.stream-read-constraints.max-string-length=200000000
+#-Dlogstash.jackson.stream-read-constraints.max-string-length=200000000
 #
 # Sets the maximum number length (in chars or bytes, depending on input context).
 # The jackson library defaults to 1000, whereas Logstash defaults to 10000.
--Dlogstash.jackson.stream-read-constraints.max-number-length=10000
+#-Dlogstash.jackson.stream-read-constraints.max-number-length=10000
 #
 # Sets the maximum nesting depth. The depth is a count of objects and arrays that have not
 # been closed, `{` and `[` respectively.

logstash-core/src/main/java/org/logstash/jackson/StreamReadConstraintsUtil.java

@@ -5,10 +5,16 @@
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 
-import java.util.*;
+import javax.annotation.Nullable;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.Properties;
+import java.util.Set;
 import java.util.function.BiConsumer;
-import java.util.function.BiFunction;
-import java.util.function.Function;
 import java.util.stream.Collectors;
 
 public class StreamReadConstraintsUtil {
@@ -18,38 +24,51 @@ public class StreamReadConstraintsUtil {
 
     private StreamReadConstraints configuredStreamReadConstraints;
 
-    // Provide default values for Jackson constraints in the case they are
-    // not specified in configuration file.
-    private static final Map<Override, Integer> JACKSON_DEFAULTS = Map.of(
-        Override.MAX_STRING_LENGTH, 200_000_000,
-        Override.MAX_NUMBER_LENGTH, 10_000,
-        Override.MAX_NESTING_DEPTH, 1_000
-    );
-
     enum Override {
-        MAX_STRING_LENGTH(StreamReadConstraints.Builder::maxStringLength, StreamReadConstraints::getMaxStringLength),
-        MAX_NUMBER_LENGTH(StreamReadConstraints.Builder::maxNumberLength, StreamReadConstraints::getMaxNumberLength),
-        MAX_NESTING_DEPTH(StreamReadConstraints.Builder::maxNestingDepth, StreamReadConstraints::getMaxNestingDepth),
+        MAX_STRING_LENGTH(200_000_000, StreamReadConstraints.Builder::maxStringLength, StreamReadConstraints::getMaxStringLength),
+        MAX_NUMBER_LENGTH(10_000, StreamReadConstraints.Builder::maxNumberLength, StreamReadConstraints::getMaxNumberLength),
+        MAX_NESTING_DEPTH(1_000, StreamReadConstraints.Builder::maxNestingDepth, StreamReadConstraints::getMaxNestingDepth),
         ;
 
         static final String PROP_PREFIX = "logstash.jackson.stream-read-constraints.";
 
         final String propertyName;
+        final int defaultValue;
         private final IntValueApplicator applicator;
         private final IntValueObserver observer;
 
-        Override(final IntValueApplicator applicator,
+        Override(final int defaultValue,
+                 final IntValueApplicator applicator,
                  final IntValueObserver observer) {
             this.propertyName = PROP_PREFIX + this.name().toLowerCase().replace('_', '-');
+            this.defaultValue = defaultValue;
             this.applicator = applicator;
             this.observer = observer;
         }
 
+        int resolve(final Integer specifiedValue) {
+            return Objects.requireNonNullElse(specifiedValue, defaultValue);
+        }
+
+        void apply(final StreamReadConstraints.Builder constraintsBuilder,
+                   @Nullable final Integer specifiedValue) {
+            this.applicator.applyIntValue(constraintsBuilder, resolve(specifiedValue));
+        }
+
+        int observe(final StreamReadConstraints constraints) {
+            return this.observer.observeIntValue(constraints);
+        }
+
         @FunctionalInterface
-        interface IntValueObserver extends Function<StreamReadConstraints, Integer> {}
+        interface IntValueObserver {
+            int observeIntValue(final StreamReadConstraints constraints);
+        }
 
         @FunctionalInterface
-        interface IntValueApplicator extends BiFunction<StreamReadConstraints.Builder, Integer, StreamReadConstraints.Builder> {}
+        interface IntValueApplicator {
+            @SuppressWarnings("UnusedReturnValue")
+            StreamReadConstraints.Builder applyIntValue(final StreamReadConstraints.Builder constraintsBuilder, final int value);
+        }
     }
 
     /**
@@ -86,9 +105,7 @@ StreamReadConstraints get() {
         if (configuredStreamReadConstraints == null) {
             final StreamReadConstraints.Builder builder = StreamReadConstraints.defaults().rebuild();
 
-            // Apply the Jackson defaults first, then the overrides from config
-            JACKSON_DEFAULTS.forEach((override, value) -> override.applicator.apply(builder, value));
-            eachOverride((override, value) -> override.applicator.apply(builder, value));
+            eachOverride((override, specifiedValue) -> override.apply(builder, specifiedValue));
 
             this.configuredStreamReadConstraints = builder.build();
         }
@@ -106,11 +123,14 @@ public void validateIsGlobalDefault() {
     private void validate(final StreamReadConstraints streamReadConstraints) {
         final List<String> fatalIssues = new ArrayList<>();
         eachOverride((override, specifiedValue) -> {
-            final Integer effectiveValue = override.observer.apply(streamReadConstraints);
-            if (Objects.equals(specifiedValue, effectiveValue)) {
-                logger.info("Jackson default value override `{}` configured to `{}`", override.propertyName, specifiedValue);
+            final int effectiveValue = override.observe(streamReadConstraints);
+            final int expectedValue = override.resolve(specifiedValue);
+
+            if (!Objects.equals(effectiveValue, expectedValue)) {
+                fatalIssues.add(String.format("`%s` (expected: `%s`, actual: `%s`)", override.propertyName, expectedValue, effectiveValue));
             } else {
-                fatalIssues.add(String.format("`%s` (expected: `%s`, actual: `%s`)", override.propertyName, specifiedValue, effectiveValue));
+                final String reason = Objects.nonNull(specifiedValue) ? "system properties" : "logstash default";
+                logger.info("Jackson default value override `{}` configured to `{}` ({})", override.propertyName, effectiveValue, reason);
             }
         });
         for (String unsupportedProperty : getUnsupportedProperties()) {
@@ -124,13 +144,14 @@ private void validate(final StreamReadConstraints streamReadConstraints) {
     void eachOverride(BiConsumer<Override,Integer> overrideIntegerBiConsumer) {
         for (Override override : Override.values()) {
             final String propValue = this.propertyOverrides.get(override.propertyName);
-            if (propValue != null) {
-                try {
-                    int intValue = Integer.parseInt(propValue);
-                    overrideIntegerBiConsumer.accept(override, intValue);
-                } catch (IllegalArgumentException e) {
-                    throw new IllegalArgumentException(String.format("System property `%s` must be positive integer value. Received: `%s`", override.propertyName, propValue), e);
-                }
+
+            try {
+                final Integer explicitValue = Optional.ofNullable(propValue)
+                        .map(Integer::parseInt)
+                        .orElse(null);
+                overrideIntegerBiConsumer.accept(override, explicitValue);
+            } catch (IllegalArgumentException e) {
+                throw new IllegalArgumentException(String.format("System property `%s` must be positive integer value. Received: `%s`", override.propertyName, propValue), e);
             }
         }
     }

logstash-core/src/test/java/org/logstash/ObjectMappersTest.java

@@ -49,4 +49,9 @@ public void testLog4jOMRegisterRubyBasicObjectSerializersFirst() {
         assertNotNull(found);
         assertTrue("RubyBasicObjectSerializer must be registered before others non-default serializers", found instanceof RubyBasicObjectSerializer);
     }
+
+    @Test
+    public void testStreamReadConstraintsApplication() {
+        ObjectMappers.CONFIGURED_STREAM_READ_CONSTRAINTS.validateIsGlobalDefault();
+    }
 }

logstash-core/src/test/java/org/logstash/jackson/StreamReadConstraintsUtilTest.java

@@ -7,13 +7,17 @@
 import org.apache.logging.log4j.test.appender.ListAppender;
 import org.junit.Before;
 import org.junit.ClassRule;
+import org.junit.Rule;
 import org.junit.Test;
 
 import java.util.Arrays;
 import java.util.List;
+import java.util.Objects;
 import java.util.Properties;
 import java.util.function.BiConsumer;
+
 import org.apache.logging.log4j.Level;
+import org.logstash.ObjectMappers;
 
 import static org.assertj.core.api.Assertions.*;
 import static org.logstash.jackson.StreamReadConstraintsUtil.Override.*;
@@ -23,13 +27,12 @@ public class StreamReadConstraintsUtilTest {
     @ClassRule
     public static final LoggerContextRule LOGGER_CONTEXT_RULE = new LoggerContextRule("log4j2-log-stream-read-constraints.xml");
 
+    @Rule
+    public final DefaultsRestorer defaultsRestorer = new DefaultsRestorer();
+
     private ListAppender listAppender;
     private Logger observedLogger;
 
-    private static final int DEFAULT_MAX_STRING_LENGTH = 200_000_000;
-    private static final int DEFAULT_MAX_NUMBER_LENGTH = 10_000;
-    private static final int DEFAULT_MAX_NESTING_DEPTH = 1_000;
-
     @Before
     public void setUpLoggingListAppender() {
         int i = 1+16;
@@ -39,6 +42,7 @@ public void setUpLoggingListAppender() {
 
     @Test
     public void configuresDefaultsByDefault() {
+        Objects.requireNonNull(ObjectMappers.CBOR_MAPPER); // force static init
         StreamReadConstraintsUtil.fromSystemProperties().validateIsGlobalDefault();
     }
 
@@ -55,8 +59,8 @@ public void configuresMaxStringLength() {
             assertThat(configuredConstraints).as("inherited defaults")
                     .returns(defaults.getMaxDocumentLength(), from(StreamReadConstraints::getMaxDocumentLength))
                     .returns(defaults.getMaxNameLength(), from(StreamReadConstraints::getMaxNameLength))
-                    .returns(DEFAULT_MAX_NESTING_DEPTH, from(StreamReadConstraints::getMaxNestingDepth))
-                    .returns(DEFAULT_MAX_NUMBER_LENGTH, from(StreamReadConstraints::getMaxNumberLength));
+                    .returns(MAX_NESTING_DEPTH.defaultValue, from(StreamReadConstraints::getMaxNestingDepth))
+                    .returns(MAX_NUMBER_LENGTH.defaultValue, from(StreamReadConstraints::getMaxNumberLength));
 
             assertThatThrownBy(configuredUtil::validateIsGlobalDefault).isInstanceOf(IllegalStateException.class).hasMessageContaining(MAX_STRING_LENGTH.propertyName);
 
@@ -98,8 +102,8 @@ public void configuresMaxNumberLength() {
             assertThat(configuredConstraints).as("inherited defaults")
                     .returns(defaults.getMaxDocumentLength(), from(StreamReadConstraints::getMaxDocumentLength))
                     .returns(defaults.getMaxNameLength(), from(StreamReadConstraints::getMaxNameLength))
-                    .returns(DEFAULT_MAX_NESTING_DEPTH, from(StreamReadConstraints::getMaxNestingDepth))
-                    .returns(DEFAULT_MAX_STRING_LENGTH, from(StreamReadConstraints::getMaxStringLength));
+                    .returns(MAX_NESTING_DEPTH.defaultValue, from(StreamReadConstraints::getMaxNestingDepth))
+                    .returns(MAX_STRING_LENGTH.defaultValue, from(StreamReadConstraints::getMaxStringLength));
 
             assertThatThrownBy(configuredUtil::validateIsGlobalDefault).isInstanceOf(IllegalStateException.class).hasMessageContaining(MAX_NUMBER_LENGTH.propertyName);
 
@@ -141,8 +145,8 @@ public void configuresMaxNestingDepth() {
             assertThat(configuredConstraints).as("inherited defaults")
                     .returns(defaults.getMaxDocumentLength(), from(StreamReadConstraints::getMaxDocumentLength))
                     .returns(defaults.getMaxNameLength(), from(StreamReadConstraints::getMaxNameLength))
-                    .returns(DEFAULT_MAX_STRING_LENGTH, from(StreamReadConstraints::getMaxStringLength))
-                    .returns(DEFAULT_MAX_NUMBER_LENGTH, from(StreamReadConstraints::getMaxNumberLength));
+                    .returns(MAX_STRING_LENGTH.defaultValue, from(StreamReadConstraints::getMaxStringLength))
+                    .returns(MAX_NUMBER_LENGTH.defaultValue, from(StreamReadConstraints::getMaxNumberLength));
 
             assertThatThrownBy(configuredUtil::validateIsGlobalDefault).isInstanceOf(IllegalStateException.class).hasMessageContaining(MAX_NESTING_DEPTH.propertyName);
 
@@ -187,8 +191,6 @@ public void validatesApplication() {
             configuredUtil.validateIsGlobalDefault();
         });
 
-        System.out.format("OK%n");
-
         assertLogObserved(Level.INFO, "override `" + MAX_NESTING_DEPTH.propertyName + "` configured to `1010`");
         assertLogObserved(Level.INFO, "override `" + MAX_STRING_LENGTH.propertyName + "` configured to `1011`");
         assertLogObserved(Level.INFO, "override `" + MAX_NUMBER_LENGTH.propertyName + "` configured to `1110`");
@@ -198,28 +200,17 @@ public void validatesApplication() {
     }
 
     @Test
-    public void usesJacksonDefaultsWhenNoConfig() {
-        StreamReadConstraintsUtil util = new StreamReadConstraintsUtil(new Properties(), this.observedLogger);
-        StreamReadConstraints constraints = util.get();
-
-        assertThat(constraints)
-            .returns(DEFAULT_MAX_STRING_LENGTH, from(StreamReadConstraints::getMaxStringLength))
-            .returns(DEFAULT_MAX_NUMBER_LENGTH, from(StreamReadConstraints::getMaxNumberLength))
-            .returns(DEFAULT_MAX_NESTING_DEPTH, from(StreamReadConstraints::getMaxNestingDepth));
-    }
-
-    @Test
-    public void configOverridesDefault() {
-        Properties props = new Properties();
-        props.setProperty("logstash.jackson.stream-read-constraints.max-string-length", "100");
+    public void validatesApplicationWithDefaults() {
+        final Properties properties = new Properties(); // empty -- no overrides
 
-        StreamReadConstraintsUtil util = new StreamReadConstraintsUtil(props, this.observedLogger);
-        StreamReadConstraints constraints = util.get();
+        fromProperties(properties, (configuredUtil, defaults) -> {
+            configuredUtil.applyAsGlobalDefault();
+            configuredUtil.validateIsGlobalDefault();
 
-        assertThat(constraints)
-            .returns(100, from(StreamReadConstraints::getMaxStringLength))
-            .returns(DEFAULT_MAX_NUMBER_LENGTH, from(StreamReadConstraints::getMaxNumberLength))
-            .returns(DEFAULT_MAX_NESTING_DEPTH, from(StreamReadConstraints::getMaxNestingDepth));
+            assertLogObserved(Level.INFO, "override `" + MAX_NESTING_DEPTH.propertyName + "` configured to `" + MAX_NESTING_DEPTH.defaultValue + "` (logstash default)");
+            assertLogObserved(Level.INFO, "override `" + MAX_STRING_LENGTH.propertyName + "` configured to `" + MAX_STRING_LENGTH.defaultValue + "` (logstash default)");
+            assertLogObserved(Level.INFO, "override `" + MAX_NUMBER_LENGTH.propertyName + "` configured to `" + MAX_NUMBER_LENGTH.defaultValue + "` (logstash default)");
+        });
     }
 
     private void assertLogObserved(final Level level, final String... messageFragments) {
@@ -230,13 +221,33 @@ private void assertLogObserved(final Level level, final String... messageFragmen
                 .anySatisfy(logEvent -> assertThat(logEvent.getMessage().getFormattedMessage()).contains(messageFragments));
     }
 
-    private synchronized void fromProperties(final Properties properties, BiConsumer<StreamReadConstraintsUtil, StreamReadConstraints> defaultsConsumer) {
+    private synchronized void fromProperties(final Properties properties,
+                                             final BiConsumer<StreamReadConstraintsUtil, StreamReadConstraints> defaultsConsumer) {
         final StreamReadConstraints defaults = StreamReadConstraints.defaults();
-        try {
-            final StreamReadConstraintsUtil util = new StreamReadConstraintsUtil(properties, this.observedLogger);
-            defaultsConsumer.accept(util, defaults);
-        } finally {
-            StreamReadConstraints.overrideDefaultStreamReadConstraints(defaults);
+        final StreamReadConstraintsUtil util = new StreamReadConstraintsUtil(properties, this.observedLogger);
+
+        defaultsConsumer.accept(util, defaults);
+    }
+
+    /**
+     * A TestRule to snapshot the current defaults from jackson prior to test execution, and to
+     * ensure that snapshot is restored after test execution has completed.
+     */
+    public static class DefaultsRestorer extends org.junit.rules.ExternalResource {
+        private StreamReadConstraints snapshot;
+
+        public StreamReadConstraints getSnapshot() {
+            return this.snapshot;
+        }
+
+        @Override
+        protected void before() throws Throwable {
+            this.snapshot = StreamReadConstraints.defaults();
+        }
+
+        @Override
+        protected void after() {
+            StreamReadConstraints.overrideDefaultStreamReadConstraints(this.snapshot);
         }
     }
 }