Skip to content

Commit 96f5708

Browse files
author
Anuraag Agrawal
authored
Fix parsing of unclean map values in Config. (#4032)
1 parent 6533596 commit 96f5708

File tree

3 files changed

+233
-35
lines changed

3 files changed

+233
-35
lines changed

‎instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/config/ConfigValueParsers.java‎

Lines changed: 26 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -6,36 +6,45 @@
66
package io.opentelemetry.instrumentation.api.config;
77

88
import java.time.Duration;
9+
import java.util.AbstractMap;
910
import java.util.Arrays;
1011
import java.util.Collections;
1112
import java.util.LinkedHashMap;
1213
import java.util.List;
1314
import java.util.Map;
1415
import java.util.concurrent.TimeUnit;
16+
import java.util.stream.Collectors;
1517

1618
final class ConfigValueParsers {
1719

1820
static List<String> parseList(String value) {
19-
String[] tokens = value.split(",", -1);
20-
// Remove whitespace from each item.
21-
for (int i = 0; i < tokens.length; i++) {
22-
tokens[i] = tokens[i].trim();
23-
}
24-
return Collections.unmodifiableList(Arrays.asList(tokens));
21+
return Collections.unmodifiableList(filterBlanksAndNulls(value.split(",")));
2522
}
2623

2724
static Map<String, String> parseMap(String value) {
28-
Map<String, String> result = new LinkedHashMap<>();
29-
for (String token : value.split(",", -1)) {
30-
token = token.trim();
31-
String[] parts = token.split("=", -1);
32-
if (parts.length != 2) {
33-
throw new IllegalArgumentException(
34-
"Invalid map config part, should be formatted key1=value1,key2=value2: " + value);
35-
}
36-
result.put(parts[0], parts[1]);
37-
}
38-
return Collections.unmodifiableMap(result);
25+
return parseList(value).stream()
26+
.map(keyValuePair -> filterBlanksAndNulls(keyValuePair.split("=", 2)))
27+
.map(
28+
splitKeyValuePairs -> {
29+
if (splitKeyValuePairs.size() != 2) {
30+
throw new IllegalArgumentException(
31+
"Invalid map property, should be formatted key1=value1,key2=value2: " + value);
32+
}
33+
return new AbstractMap.SimpleImmutableEntry<>(
34+
splitKeyValuePairs.get(0), splitKeyValuePairs.get(1));
35+
})
36+
// If duplicate keys, prioritize later ones similar to duplicate system properties on a
37+
// Java command line.
38+
.collect(
39+
Collectors.toMap(
40+
Map.Entry::getKey, Map.Entry::getValue, (first, next) -> next, LinkedHashMap::new));
41+
}
42+
43+
private static List<String> filterBlanksAndNulls(String[] values) {
44+
return Arrays.stream(values)
45+
.map(String::trim)
46+
.filter(s -> !s.isEmpty())
47+
.collect(Collectors.toList());
3948
}
4049

4150
static Duration parseDuration(String value) {

‎instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/config/ConfigTest.java‎

Lines changed: 13 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -9,15 +9,15 @@
99
import static java.util.Collections.emptyList;
1010
import static java.util.Collections.singletonList;
1111
import static java.util.Collections.singletonMap;
12+
import static org.assertj.core.api.Assertions.assertThat;
13+
import static org.assertj.core.api.Assertions.entry;
1214
import static org.junit.jupiter.api.Assertions.assertEquals;
1315
import static org.junit.jupiter.api.Assertions.assertFalse;
1416
import static org.junit.jupiter.api.Assertions.assertNull;
1517
import static org.junit.jupiter.api.Assertions.assertTrue;
1618

1719
import java.time.Duration;
18-
import java.util.HashMap;
1920
import java.util.List;
20-
import java.util.Map;
2121
import java.util.TreeSet;
2222
import java.util.stream.Stream;
2323
import org.junit.jupiter.api.Test;
@@ -150,17 +150,19 @@ void shouldGetMap() {
150150
Config.newBuilder()
151151
.addProperty("prop.map", "one=1, two=2")
152152
.addProperty("prop.wrong", "one=1, but not two!")
153+
.addProperty("prop.trailing", "one=1,")
153154
.build();
154155

155-
assertEquals(map("one", "1", "two", "2"), config.getMap("prop.map"));
156-
assertEquals(
157-
map("one", "1", "two", "2"), config.getMap("prop.map", singletonMap("three", "3")));
158-
assertTrue(config.getMap("prop.wrong").isEmpty());
159-
assertEquals(
160-
singletonMap("three", "3"), config.getMap("prop.wrong", singletonMap("three", "3")));
161-
assertTrue(config.getMap("prop.missing").isEmpty());
162-
assertEquals(
163-
singletonMap("three", "3"), config.getMap("prop.missing", singletonMap("three", "3")));
156+
assertThat(config.getMap("prop.map")).containsOnly(entry("one", "1"), entry("two", "2"));
157+
assertThat(config.getMap("prop.map", singletonMap("three", "3")))
158+
.containsOnly(entry("one", "1"), entry("two", "2"));
159+
assertThat(config.getMap("prop.wrong")).isEmpty();
160+
assertThat(config.getMap("prop.wrong", singletonMap("three", "3")))
161+
.containsOnly(entry("three", "3"));
162+
assertThat(config.getMap("prop.missing")).isEmpty();
163+
assertThat(config.getMap("prop.missing", singletonMap("three", "3")))
164+
.containsOnly(entry("three", "3"));
165+
assertThat(config.getMap("prop.trailing")).containsOnly(entry("one", "1"));
164166
}
165167

166168
@ParameterizedTest
@@ -216,11 +218,4 @@ public Stream<? extends Arguments> provideArguments(ExtensionContext context) {
216218
Arguments.of(asList("disabled-env", "test-env"), true, false));
217219
}
218220
}
219-
220-
public static Map<String, String> map(String k1, String v1, String k2, String v2) {
221-
Map<String, String> map = new HashMap<>();
222-
map.put(k1, v1);
223-
map.put(k2, v2);
224-
return map;
225-
}
226221
}

‎javaagent-tooling/src/test/java/io/opentelemetry/javaagent/tooling/config/ConfigPropertiesAdapterTest.java‎

Lines changed: 194 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,194 @@
1+
/*
2+
* Copyright The OpenTelemetry Authors
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
package io.opentelemetry.javaagent.tooling.config;
7+
8+
import static org.assertj.core.api.Assertions.assertThat;
9+
import static org.assertj.core.api.Assertions.assertThatThrownBy;
10+
import static org.assertj.core.api.Assertions.entry;
11+
12+
import io.opentelemetry.instrumentation.api.config.Config;
13+
import io.opentelemetry.sdk.autoconfigure.ConfigProperties;
14+
import io.opentelemetry.sdk.autoconfigure.ConfigurationException;
15+
import java.time.Duration;
16+
import java.util.Collections;
17+
import java.util.HashMap;
18+
import java.util.Map;
19+
import org.junit.jupiter.api.Disabled;
20+
import org.junit.jupiter.api.Test;
21+
22+
// Copied from
23+
// https://github.com/open-telemetry/opentelemetry-java/blob/main/sdk-extensions/autoconfigure/src/test/java/io/opentelemetry/sdk/autoconfigure/ConfigPropertiesTest.java
24+
class ConfigPropertiesAdapterTest {
25+
26+
@Test
27+
void allValid() {
28+
Map<String, String> properties = new HashMap<>();
29+
properties.put("string", "str");
30+
properties.put("int", "10");
31+
properties.put("long", "20");
32+
properties.put("double", "5.4");
33+
properties.put("list", "cat,dog,bear");
34+
properties.put("map", "cat=meow,dog=bark,bear=growl");
35+
properties.put("duration", "1s");
36+
37+
ConfigProperties config = createConfig(properties);
38+
assertThat(config.getString("string")).isEqualTo("str");
39+
assertThat(config.getInt("int")).isEqualTo(10);
40+
assertThat(config.getLong("long")).isEqualTo(20);
41+
assertThat(config.getDouble("double")).isEqualTo(5.4);
42+
assertThat(config.getCommaSeparatedValues("list")).containsExactly("cat", "dog", "bear");
43+
assertThat(config.getCommaSeparatedMap("map"))
44+
.containsExactly(entry("cat", "meow"), entry("dog", "bark"), entry("bear", "growl"));
45+
assertThat(config.getDuration("duration")).isEqualTo(Duration.ofSeconds(1));
46+
}
47+
48+
@Test
49+
void allMissing() {
50+
ConfigProperties config = createConfig(Collections.emptyMap());
51+
assertThat(config.getString("string")).isNull();
52+
assertThat(config.getInt("int")).isNull();
53+
assertThat(config.getLong("long")).isNull();
54+
assertThat(config.getDouble("double")).isNull();
55+
assertThat(config.getCommaSeparatedValues("list")).isEmpty();
56+
assertThat(config.getCommaSeparatedMap("map")).isEmpty();
57+
assertThat(config.getDuration("duration")).isNull();
58+
}
59+
60+
@Test
61+
void allEmpty() {
62+
Map<String, String> properties = new HashMap<>();
63+
properties.put("string", "");
64+
properties.put("int", "");
65+
properties.put("long", "");
66+
properties.put("double", "");
67+
properties.put("list", "");
68+
properties.put("map", "");
69+
properties.put("duration", "");
70+
71+
ConfigProperties config = createConfig(properties);
72+
assertThat(config.getString("string")).isEmpty();
73+
assertThat(config.getInt("int")).isNull();
74+
assertThat(config.getLong("long")).isNull();
75+
assertThat(config.getDouble("double")).isNull();
76+
assertThat(config.getCommaSeparatedValues("list")).isEmpty();
77+
assertThat(config.getCommaSeparatedMap("map")).isEmpty();
78+
assertThat(config.getDuration("duration")).isNull();
79+
}
80+
81+
@Test
82+
@Disabled("Currently invalid values are not handled the same as the SDK")
83+
void invalidInt() {
84+
assertThatThrownBy(() -> createConfig(Collections.singletonMap("int", "bar")).getInt("int"))
85+
.isInstanceOf(ConfigurationException.class)
86+
.hasMessage("Invalid value for property int=bar. Must be a integer.");
87+
assertThatThrownBy(
88+
() -> createConfig(Collections.singletonMap("int", "999999999999999")).getInt("int"))
89+
.isInstanceOf(ConfigurationException.class)
90+
.hasMessage("Invalid value for property int=999999999999999. Must be a integer.");
91+
}
92+
93+
@Test
94+
@Disabled("Currently invalid values are not handled the same as the SDK")
95+
void invalidLong() {
96+
assertThatThrownBy(() -> createConfig(Collections.singletonMap("long", "bar")).getLong("long"))
97+
.isInstanceOf(ConfigurationException.class)
98+
.hasMessage("Invalid value for property long=bar. Must be a long.");
99+
assertThatThrownBy(
100+
() ->
101+
createConfig(Collections.singletonMap("long", "99223372036854775807"))
102+
.getLong("long"))
103+
.isInstanceOf(ConfigurationException.class)
104+
.hasMessage("Invalid value for property long=99223372036854775807. Must be a long.");
105+
}
106+
107+
@Test
108+
@Disabled("Currently invalid values are not handled the same as the SDK")
109+
void invalidDouble() {
110+
assertThatThrownBy(
111+
() -> createConfig(Collections.singletonMap("double", "bar")).getDouble("double"))
112+
.isInstanceOf(ConfigurationException.class)
113+
.hasMessage("Invalid value for property double=bar. Must be a double.");
114+
assertThatThrownBy(
115+
() -> createConfig(Collections.singletonMap("double", "1.0.1")).getDouble("double"))
116+
.isInstanceOf(ConfigurationException.class)
117+
.hasMessage("Invalid value for property double=1.0.1. Must be a double.");
118+
}
119+
120+
@Test
121+
void uncleanList() {
122+
assertThat(
123+
createConfig(Collections.singletonMap("list", " a ,b,c , d,, ,"))
124+
.getCommaSeparatedValues("list"))
125+
.containsExactly("a", "b", "c", "d");
126+
}
127+
128+
@Test
129+
void uncleanMap() {
130+
assertThat(
131+
createConfig(Collections.singletonMap("map", " a=1 ,b=2,c = 3 , d= 4,, ,"))
132+
.getCommaSeparatedMap("map"))
133+
.containsExactly(entry("a", "1"), entry("b", "2"), entry("c", "3"), entry("d", "4"));
134+
}
135+
136+
@Test
137+
@Disabled("Currently invalid values are not handled the same as the SDK")
138+
void invalidMap() {
139+
assertThatThrownBy(
140+
() ->
141+
createConfig(Collections.singletonMap("map", "a=1,b=")).getCommaSeparatedMap("map"))
142+
.isInstanceOf(ConfigurationException.class)
143+
.hasMessage("Invalid map property: map=a=1,b=");
144+
assertThatThrownBy(
145+
() ->
146+
createConfig(Collections.singletonMap("map", "a=1,b")).getCommaSeparatedMap("map"))
147+
.isInstanceOf(ConfigurationException.class)
148+
.hasMessage("Invalid map property: map=a=1,b");
149+
assertThatThrownBy(
150+
() ->
151+
createConfig(Collections.singletonMap("map", "a=1,=b")).getCommaSeparatedMap("map"))
152+
.isInstanceOf(ConfigurationException.class)
153+
.hasMessage("Invalid map property: map=a=1,=b");
154+
}
155+
156+
@Test
157+
@Disabled("Currently invalid values are not handled the same as the SDK")
158+
void invalidDuration() {
159+
assertThatThrownBy(
160+
() ->
161+
createConfig(Collections.singletonMap("duration", "1a1ms")).getDuration("duration"))
162+
.isInstanceOf(ConfigurationException.class)
163+
.hasMessage("Invalid duration property duration=1a1ms. Expected number, found: 1a1");
164+
assertThatThrownBy(
165+
() -> createConfig(Collections.singletonMap("duration", "9mm")).getDuration("duration"))
166+
.isInstanceOf(ConfigurationException.class)
167+
.hasMessage("Invalid duration property duration=9mm. Invalid duration string, found: mm");
168+
}
169+
170+
@Test
171+
void durationUnitParsing() {
172+
assertThat(createConfig(Collections.singletonMap("duration", "1")).getDuration("duration"))
173+
.isEqualTo(Duration.ofMillis(1));
174+
assertThat(createConfig(Collections.singletonMap("duration", "2ms")).getDuration("duration"))
175+
.isEqualTo(Duration.ofMillis(2));
176+
assertThat(createConfig(Collections.singletonMap("duration", "3s")).getDuration("duration"))
177+
.isEqualTo(Duration.ofSeconds(3));
178+
assertThat(createConfig(Collections.singletonMap("duration", "4m")).getDuration("duration"))
179+
.isEqualTo(Duration.ofMinutes(4));
180+
assertThat(createConfig(Collections.singletonMap("duration", "5h")).getDuration("duration"))
181+
.isEqualTo(Duration.ofHours(5));
182+
assertThat(createConfig(Collections.singletonMap("duration", "6d")).getDuration("duration"))
183+
.isEqualTo(Duration.ofDays(6));
184+
// Check Space handling
185+
assertThat(createConfig(Collections.singletonMap("duration", "7 ms")).getDuration("duration"))
186+
.isEqualTo(Duration.ofMillis(7));
187+
assertThat(createConfig(Collections.singletonMap("duration", "8 ms")).getDuration("duration"))
188+
.isEqualTo(Duration.ofMillis(8));
189+
}
190+
191+
private static ConfigProperties createConfig(Map<String, String> properties) {
192+
return new ConfigPropertiesAdapter(Config.newBuilder().readProperties(properties).build());
193+
}
194+
}

0 commit comments

Comments
 (0)