Fix parsing of unclean map values in Config. (#4032)

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.Arrays.asList;
 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 @@ class ConfigTest {
         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 @@ class ConfigTest {
           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());
+  }
+}