From 46b7aa9c327b0702d9ab77db53c7ff96ac71c53a Mon Sep 17 00:00:00 2001 From: Anuraag Agrawal Date: Tue, 31 Aug 2021 12:35:26 +0900 Subject: [PATCH] Fix parsing of unclean map values in Config. --- .../api/config/ConfigValueParsers.java | 43 ++-- .../api/config/ConfigTest.java | 31 ++- .../config/ConfigPropertiesAdapterTest.java | 194 ++++++++++++++++++ 3 files changed, 233 insertions(+), 35 deletions(-) create mode 100644 javaagent-tooling/src/test/java/io/opentelemetry/javaagent/tooling/config/ConfigPropertiesAdapterTest.java diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/config/ConfigValueParsers.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/config/ConfigValueParsers.java index 2ec4d9d84e89..b2e00b61b848 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/config/ConfigValueParsers.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/config/ConfigValueParsers.java @@ -6,36 +6,45 @@ package io.opentelemetry.instrumentation.api.config; import java.time.Duration; +import java.util.AbstractMap; import java.util.Arrays; import java.util.Collections; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.concurrent.TimeUnit; +import java.util.stream.Collectors; final class ConfigValueParsers { static List parseList(String value) { - String[] tokens = value.split(",", -1); - // Remove whitespace from each item. - for (int i = 0; i < tokens.length; i++) { - tokens[i] = tokens[i].trim(); - } - return Collections.unmodifiableList(Arrays.asList(tokens)); + return Collections.unmodifiableList(filterBlanksAndNulls(value.split(","))); } static Map parseMap(String value) { - Map result = new LinkedHashMap<>(); - for (String token : value.split(",", -1)) { - token = token.trim(); - String[] parts = token.split("=", -1); - if (parts.length != 2) { - throw new IllegalArgumentException( - "Invalid map config part, should be formatted key1=value1,key2=value2: " + value); - } - result.put(parts[0], parts[1]); - } - return Collections.unmodifiableMap(result); + return parseList(value).stream() + .map(keyValuePair -> filterBlanksAndNulls(keyValuePair.split("=", 2))) + .map( + splitKeyValuePairs -> { + if (splitKeyValuePairs.size() != 2) { + throw new IllegalArgumentException( + "Invalid map property, should be formatted key1=value1,key2=value2: " + value); + } + return new AbstractMap.SimpleImmutableEntry<>( + splitKeyValuePairs.get(0), splitKeyValuePairs.get(1)); + }) + // If duplicate keys, prioritize later ones similar to duplicate system properties on a + // Java command line. + .collect( + Collectors.toMap( + Map.Entry::getKey, Map.Entry::getValue, (first, next) -> next, LinkedHashMap::new)); + } + + private static List filterBlanksAndNulls(String[] values) { + return Arrays.stream(values) + .map(String::trim) + .filter(s -> !s.isEmpty()) + .collect(Collectors.toList()); } static Duration parseDuration(String value) { diff --git a/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/config/ConfigTest.java b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/config/ConfigTest.java index acc74dc4d604..8132cbaf00bb 100644 --- a/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/config/ConfigTest.java +++ b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/config/ConfigTest.java @@ -9,15 +9,15 @@ import static java.util.Collections.emptyList; import static java.util.Collections.singletonList; import static java.util.Collections.singletonMap; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.entry; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; import java.time.Duration; -import java.util.HashMap; import java.util.List; -import java.util.Map; import java.util.TreeSet; import java.util.stream.Stream; import org.junit.jupiter.api.Test; @@ -150,17 +150,19 @@ void shouldGetMap() { Config.newBuilder() .addProperty("prop.map", "one=1, two=2") .addProperty("prop.wrong", "one=1, but not two!") + .addProperty("prop.trailing", "one=1,") .build(); - assertEquals(map("one", "1", "two", "2"), config.getMap("prop.map")); - assertEquals( - map("one", "1", "two", "2"), config.getMap("prop.map", singletonMap("three", "3"))); - assertTrue(config.getMap("prop.wrong").isEmpty()); - assertEquals( - singletonMap("three", "3"), config.getMap("prop.wrong", singletonMap("three", "3"))); - assertTrue(config.getMap("prop.missing").isEmpty()); - assertEquals( - singletonMap("three", "3"), config.getMap("prop.missing", singletonMap("three", "3"))); + assertThat(config.getMap("prop.map")).containsOnly(entry("one", "1"), entry("two", "2")); + assertThat(config.getMap("prop.map", singletonMap("three", "3"))) + .containsOnly(entry("one", "1"), entry("two", "2")); + assertThat(config.getMap("prop.wrong")).isEmpty(); + assertThat(config.getMap("prop.wrong", singletonMap("three", "3"))) + .containsOnly(entry("three", "3")); + assertThat(config.getMap("prop.missing")).isEmpty(); + assertThat(config.getMap("prop.missing", singletonMap("three", "3"))) + .containsOnly(entry("three", "3")); + assertThat(config.getMap("prop.trailing")).containsOnly(entry("one", "1")); } @ParameterizedTest @@ -216,11 +218,4 @@ public Stream provideArguments(ExtensionContext context) { Arguments.of(asList("disabled-env", "test-env"), true, false)); } } - - public static Map map(String k1, String v1, String k2, String v2) { - Map map = new HashMap<>(); - map.put(k1, v1); - map.put(k2, v2); - return map; - } } diff --git a/javaagent-tooling/src/test/java/io/opentelemetry/javaagent/tooling/config/ConfigPropertiesAdapterTest.java b/javaagent-tooling/src/test/java/io/opentelemetry/javaagent/tooling/config/ConfigPropertiesAdapterTest.java new file mode 100644 index 000000000000..a95c23067d17 --- /dev/null +++ b/javaagent-tooling/src/test/java/io/opentelemetry/javaagent/tooling/config/ConfigPropertiesAdapterTest.java @@ -0,0 +1,194 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.tooling.config; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.assertj.core.api.Assertions.entry; + +import io.opentelemetry.instrumentation.api.config.Config; +import io.opentelemetry.sdk.autoconfigure.ConfigProperties; +import io.opentelemetry.sdk.autoconfigure.ConfigurationException; +import java.time.Duration; +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; +import org.junit.jupiter.api.Disabled; +import org.junit.jupiter.api.Test; + +// Copied from +// https://github.com/open-telemetry/opentelemetry-java/blob/main/sdk-extensions/autoconfigure/src/test/java/io/opentelemetry/sdk/autoconfigure/ConfigPropertiesTest.java +class ConfigPropertiesAdapterTest { + + @Test + void allValid() { + Map properties = new HashMap<>(); + properties.put("string", "str"); + properties.put("int", "10"); + properties.put("long", "20"); + properties.put("double", "5.4"); + properties.put("list", "cat,dog,bear"); + properties.put("map", "cat=meow,dog=bark,bear=growl"); + properties.put("duration", "1s"); + + ConfigProperties config = createConfig(properties); + assertThat(config.getString("string")).isEqualTo("str"); + assertThat(config.getInt("int")).isEqualTo(10); + assertThat(config.getLong("long")).isEqualTo(20); + assertThat(config.getDouble("double")).isEqualTo(5.4); + assertThat(config.getCommaSeparatedValues("list")).containsExactly("cat", "dog", "bear"); + assertThat(config.getCommaSeparatedMap("map")) + .containsExactly(entry("cat", "meow"), entry("dog", "bark"), entry("bear", "growl")); + assertThat(config.getDuration("duration")).isEqualTo(Duration.ofSeconds(1)); + } + + @Test + void allMissing() { + ConfigProperties config = createConfig(Collections.emptyMap()); + assertThat(config.getString("string")).isNull(); + assertThat(config.getInt("int")).isNull(); + assertThat(config.getLong("long")).isNull(); + assertThat(config.getDouble("double")).isNull(); + assertThat(config.getCommaSeparatedValues("list")).isEmpty(); + assertThat(config.getCommaSeparatedMap("map")).isEmpty(); + assertThat(config.getDuration("duration")).isNull(); + } + + @Test + void allEmpty() { + Map properties = new HashMap<>(); + properties.put("string", ""); + properties.put("int", ""); + properties.put("long", ""); + properties.put("double", ""); + properties.put("list", ""); + properties.put("map", ""); + properties.put("duration", ""); + + ConfigProperties config = createConfig(properties); + assertThat(config.getString("string")).isEmpty(); + assertThat(config.getInt("int")).isNull(); + assertThat(config.getLong("long")).isNull(); + assertThat(config.getDouble("double")).isNull(); + assertThat(config.getCommaSeparatedValues("list")).isEmpty(); + assertThat(config.getCommaSeparatedMap("map")).isEmpty(); + assertThat(config.getDuration("duration")).isNull(); + } + + @Test + @Disabled("Currently invalid values are not handled the same as the SDK") + void invalidInt() { + assertThatThrownBy(() -> createConfig(Collections.singletonMap("int", "bar")).getInt("int")) + .isInstanceOf(ConfigurationException.class) + .hasMessage("Invalid value for property int=bar. Must be a integer."); + assertThatThrownBy( + () -> createConfig(Collections.singletonMap("int", "999999999999999")).getInt("int")) + .isInstanceOf(ConfigurationException.class) + .hasMessage("Invalid value for property int=999999999999999. Must be a integer."); + } + + @Test + @Disabled("Currently invalid values are not handled the same as the SDK") + void invalidLong() { + assertThatThrownBy(() -> createConfig(Collections.singletonMap("long", "bar")).getLong("long")) + .isInstanceOf(ConfigurationException.class) + .hasMessage("Invalid value for property long=bar. Must be a long."); + assertThatThrownBy( + () -> + createConfig(Collections.singletonMap("long", "99223372036854775807")) + .getLong("long")) + .isInstanceOf(ConfigurationException.class) + .hasMessage("Invalid value for property long=99223372036854775807. Must be a long."); + } + + @Test + @Disabled("Currently invalid values are not handled the same as the SDK") + void invalidDouble() { + assertThatThrownBy( + () -> createConfig(Collections.singletonMap("double", "bar")).getDouble("double")) + .isInstanceOf(ConfigurationException.class) + .hasMessage("Invalid value for property double=bar. Must be a double."); + assertThatThrownBy( + () -> createConfig(Collections.singletonMap("double", "1.0.1")).getDouble("double")) + .isInstanceOf(ConfigurationException.class) + .hasMessage("Invalid value for property double=1.0.1. Must be a double."); + } + + @Test + void uncleanList() { + assertThat( + createConfig(Collections.singletonMap("list", " a ,b,c , d,, ,")) + .getCommaSeparatedValues("list")) + .containsExactly("a", "b", "c", "d"); + } + + @Test + void uncleanMap() { + assertThat( + createConfig(Collections.singletonMap("map", " a=1 ,b=2,c = 3 , d= 4,, ,")) + .getCommaSeparatedMap("map")) + .containsExactly(entry("a", "1"), entry("b", "2"), entry("c", "3"), entry("d", "4")); + } + + @Test + @Disabled("Currently invalid values are not handled the same as the SDK") + void invalidMap() { + assertThatThrownBy( + () -> + createConfig(Collections.singletonMap("map", "a=1,b=")).getCommaSeparatedMap("map")) + .isInstanceOf(ConfigurationException.class) + .hasMessage("Invalid map property: map=a=1,b="); + assertThatThrownBy( + () -> + createConfig(Collections.singletonMap("map", "a=1,b")).getCommaSeparatedMap("map")) + .isInstanceOf(ConfigurationException.class) + .hasMessage("Invalid map property: map=a=1,b"); + assertThatThrownBy( + () -> + createConfig(Collections.singletonMap("map", "a=1,=b")).getCommaSeparatedMap("map")) + .isInstanceOf(ConfigurationException.class) + .hasMessage("Invalid map property: map=a=1,=b"); + } + + @Test + @Disabled("Currently invalid values are not handled the same as the SDK") + void invalidDuration() { + assertThatThrownBy( + () -> + createConfig(Collections.singletonMap("duration", "1a1ms")).getDuration("duration")) + .isInstanceOf(ConfigurationException.class) + .hasMessage("Invalid duration property duration=1a1ms. Expected number, found: 1a1"); + assertThatThrownBy( + () -> createConfig(Collections.singletonMap("duration", "9mm")).getDuration("duration")) + .isInstanceOf(ConfigurationException.class) + .hasMessage("Invalid duration property duration=9mm. Invalid duration string, found: mm"); + } + + @Test + void durationUnitParsing() { + assertThat(createConfig(Collections.singletonMap("duration", "1")).getDuration("duration")) + .isEqualTo(Duration.ofMillis(1)); + assertThat(createConfig(Collections.singletonMap("duration", "2ms")).getDuration("duration")) + .isEqualTo(Duration.ofMillis(2)); + assertThat(createConfig(Collections.singletonMap("duration", "3s")).getDuration("duration")) + .isEqualTo(Duration.ofSeconds(3)); + assertThat(createConfig(Collections.singletonMap("duration", "4m")).getDuration("duration")) + .isEqualTo(Duration.ofMinutes(4)); + assertThat(createConfig(Collections.singletonMap("duration", "5h")).getDuration("duration")) + .isEqualTo(Duration.ofHours(5)); + assertThat(createConfig(Collections.singletonMap("duration", "6d")).getDuration("duration")) + .isEqualTo(Duration.ofDays(6)); + // Check Space handling + assertThat(createConfig(Collections.singletonMap("duration", "7 ms")).getDuration("duration")) + .isEqualTo(Duration.ofMillis(7)); + assertThat(createConfig(Collections.singletonMap("duration", "8 ms")).getDuration("duration")) + .isEqualTo(Duration.ofMillis(8)); + } + + private static ConfigProperties createConfig(Map properties) { + return new ConfigPropertiesAdapter(Config.newBuilder().readProperties(properties).build()); + } +}