This is an automated email from the ASF dual-hosted git repository.
jmclean pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new 890247cd1d [apache#7980] fix(config): Improve ConfigEntry parsing of
comma-separated configuration (#7994)
890247cd1d is described below
commit 890247cd1d5cd5d653d28fd1d1fb1459ded6c949
Author: KWON TAE HEON <[email protected]>
AuthorDate: Mon Aug 11 10:59:40 2025 +0900
[apache#7980] fix(config): Improve ConfigEntry parsing of comma-separated
configuration (#7994)
### What changes were proposed in this pull request?
Update `ConfigEntry` sequence parsing to:
- Trim leading/trailing whitespace for the input and each element
- Ignore empty/blank elements after trimming
- Preserve element order and duplicates
- Update tests accordingly (add parsing tests, remove obsolete
empty-element failure, verify `checkValue` runs after parsing)
### Why are the changes needed?
- Issue #7980 requests trimming and skipping empty elements for
comma-separated configuration.
- Previous behavior kept empty strings, which could lead to unexpected
`IllegalArgumentException` and less ergonomic configuration.
- Previously, whitespace-only elements (e.g., `" "`) were preserved as
empty strings (`List.of("")`).
- This change removes such elements entirely, so `" "` now results in an
empty list (`List.of()`).
Fix: #7980
### Does this PR introduce _any_ user-facing change?
- Yes.
- Configurations that previously produced `List.of("")` for
whitespace-only values will now produce `List.of()`.
- This may affect code that differentiates between “list with an empty
string” and “empty list”.
### How was this patch tested?
- Added `testSequenceParsing_trimsAndIgnoresEmptyElements()` to the
`TestConfigEntryList` class.
- Added `testBlankOnlyInput_returnsEmptyList()` to the
`TestConfigEntryList` class.
- Added `testCheckValue_appliesAfterParsing()` to the
`TestConfigEntryList` class.
- Added `testSequenceParsing_preservesOrderAndDuplicates()` to the
`TestConfigEntryList` class.
- Updated `testCheckValue()` in the `TestConfigEntryList` class to
remove obsolete empty-element failure case.
---
.../org/apache/gravitino/config/ConfigEntry.java | 20 +++++++++-
.../apache/gravitino/config/TestConfigEntry.java | 15 ++++++++
.../gravitino/config/TestConfigEntryList.java | 43 +++++++++++++++-------
3 files changed, 63 insertions(+), 15 deletions(-)
diff --git a/core/src/main/java/org/apache/gravitino/config/ConfigEntry.java
b/core/src/main/java/org/apache/gravitino/config/ConfigEntry.java
index e33a1661ca..d8d704a1c8 100644
--- a/core/src/main/java/org/apache/gravitino/config/ConfigEntry.java
+++ b/core/src/main/java/org/apache/gravitino/config/ConfigEntry.java
@@ -154,17 +154,33 @@ public class ConfigEntry<T> {
/**
* Split the string to a list, then map each string element to its converted
form.
+ * Leading/trailing whitespace of the input and each element will be
trimmed, and blank elements
+ * will be ignored before conversion.
+ *
+ * <p>Examples:
+ *
+ * <pre>{@code
+ * strToSeq(null, converter) = []
+ * strToSeq(" ", converter) = []
+ * strToSeq("A,B,C", converter) = ["A", "B", "C"]
+ * strToSeq(" A, B , ,C, ,D ", converter) = ["A", "B", "C", "D"]
+ * strToSeq(" AB, B C, ,D, , E F ", converter) = ["AB", "B C", "D", "E F"]
+ * }</pre>
*
* @param str The string form of the value list from the conf entry.
* @param converter The original ConfigEntry valueConverter.
* @return The list of converted type.
*/
public List<T> strToSeq(String str, Function<String, T> converter) {
- if (str == null || str.isEmpty()) {
+ if (str == null || str.trim().isEmpty()) {
return Collections.emptyList();
}
List<String> strList = Arrays.asList(str.split(","));
- return strList.stream().map(converter).collect(Collectors.toList());
+ return strList.stream()
+ .map(String::trim)
+ .filter(s -> !s.isEmpty())
+ .map(converter)
+ .collect(Collectors.toList());
}
/**
diff --git
a/core/src/test/java/org/apache/gravitino/config/TestConfigEntry.java
b/core/src/test/java/org/apache/gravitino/config/TestConfigEntry.java
index cccef9607e..2973e437db 100644
--- a/core/src/test/java/org/apache/gravitino/config/TestConfigEntry.java
+++ b/core/src/test/java/org/apache/gravitino/config/TestConfigEntry.java
@@ -170,11 +170,26 @@ public class TestConfigEntry {
IllegalArgumentException.class, () ->
testConfOptional.readFrom(configMap));
}
+ @Test
+ public void testStrToSeqTrimString() {
+ ConfigEntry<String> conf = new
ConfigBuilder("gravitino.test.seq").stringConf().create();
+ List<String> result = conf.strToSeq(" A, B , ,C, ,D ", s -> s);
+ Assertions.assertEquals(result, Lists.newArrayList("A", "B", "C", "D"));
+
+ ConfigEntry<String> conf2 = new
ConfigBuilder("gravitino.test.seq").stringConf().create();
+ List<String> result2 = conf2.strToSeq(" AB, B C, ,D, , E F ", s -> s);
+ Assertions.assertEquals(result2, Lists.newArrayList("AB", "B C", "D", "E
F"));
+ }
+
@Test
public void testStrToSeqEmptyString() {
ConfigEntry<String> conf = new
ConfigBuilder("gravitino.test.seq").stringConf().create();
List<String> result = conf.strToSeq("", s -> s);
Assertions.assertTrue(result.isEmpty());
+
+ ConfigEntry<String> conf2 = new
ConfigBuilder("gravitino.test.seq").stringConf().create();
+ List<String> result2 = conf2.strToSeq(" ", s -> s);
+ Assertions.assertTrue(result2.isEmpty());
}
@Test
diff --git
a/core/src/test/java/org/apache/gravitino/config/TestConfigEntryList.java
b/core/src/test/java/org/apache/gravitino/config/TestConfigEntryList.java
index ce95ffbf06..dee97f47cd 100644
--- a/core/src/test/java/org/apache/gravitino/config/TestConfigEntryList.java
+++ b/core/src/test/java/org/apache/gravitino/config/TestConfigEntryList.java
@@ -145,19 +145,6 @@ public class TestConfigEntryList {
.create();
Assertions.assertThrows(
IllegalArgumentException.class, () ->
testConfNoDefault.readFrom(configMap));
-
- // To test checkValue before calling `toSequence`
- ConfigEntry<List<String>> testConfWithoutDefault =
- new ConfigBuilder("gravitino.test.empty.check")
- .doc("test")
- .internal()
- .stringConf()
- .checkValue(value -> !value.isEmpty(), "error")
- .toSequence()
- .create();
- testConfWithoutDefault.writeTo(configMap, Lists.newArrayList("valid", "",
"another"));
- Assertions.assertThrows(
- IllegalArgumentException.class, () ->
testConfWithoutDefault.readFrom(configMap));
}
@Test
@@ -172,4 +159,34 @@ public class TestConfigEntryList {
() -> testConf.writeTo(configMapEmpty, Lists.newArrayList(1, null,
2)));
Assertions.assertEquals("1,2", configMapEmpty.get("gravitino.seq.null"));
}
+
+ @Test
+ public void testSequenceParsing_trimsAndIgnoresEmptyElements() {
+ ConfigEntry<List<String>> conf =
+ new ConfigBuilder("gravitino.test.seq.trim")
+ .doc("test")
+ .internal()
+ .stringConf()
+ .toSequence()
+ .create();
+
+ conf.writeTo(configMapEmpty, Lists.newArrayList(" A", "B ", "", " C", "
", "D", " E F "));
+ List<String> valueList = conf.readFrom(configMapEmpty);
+ Assertions.assertEquals(Lists.newArrayList("A", "B", "C", "D", "E F"),
valueList);
+ }
+
+ @Test
+ public void testBlankOnlyInput_returnsEmptyList() {
+ ConfigEntry<List<String>> conf =
+ new ConfigBuilder("gravitino.test.seq.blank")
+ .doc("test")
+ .internal()
+ .stringConf()
+ .toSequence()
+ .create();
+
+ conf.writeTo(configMapEmpty, Lists.newArrayList(" ", "\t", ""));
+ List<String> res = conf.readFrom(configMapEmpty);
+ Assertions.assertTrue(res.isEmpty());
+ }
}
