Fix parsing of unclean map values in Config.

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<String> 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<String, String> parseMap(String value) {
- Map<String, String> 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<String> filterBlanksAndNulls(String[] values) {
+ return Arrays.stream(values)
+ .map(String::trim)
+ .filter(s -> !s.isEmpty())
+ .collect(Collectors.toList());
  }
 
  static Duration parseDuration(String value) {

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<? extends Arguments> provideArguments(ExtensionContext context) {
  Arguments.of(asList("disabled-env", "test-env"), true, false));
  }
  }
-
- public static Map<String, String> map(String k1, String v1, String k2, String v2) {
- Map<String, String> map = new HashMap<>();
- map.put(k1, v1);
- map.put(k2, v2);
- return map;
- }
 }

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<String, String> 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<String, String> 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<String, String> properties) {
+ return new ConfigPropertiesAdapter(Config.newBuilder().readProperties(properties).build());
+ }
+}