[#3216] fix(core): Fix the issue of method checkValue in the Config…

…Option (#3231) ### What changes were proposed in this pull request? Fix the issue of method `checkValue` in the ConfigOption. If we use `checkValue` in the wrong position, it won't work. ### Why are the changes needed? Fix: #3216 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? UT. Co-authored-by: qqqttt123 <[email protected]> Co-authored-by: Heng Qin <[email protected]>
apache · Apr 30, 2024 · 1fef0c7 · 1fef0c7
1 parent 6f90a24
commit 1fef0c7
Show file tree

Hide file tree

Showing 4 changed files with 40 additions and 5 deletions.
diff --git a/common/src/main/java/com/datastrato/gravitino/config/ConfigEntry.java b/common/src/main/java/com/datastrato/gravitino/config/ConfigEntry.java
@@ -171,8 +171,25 @@ public String seqToStr(List<T> seq, Function<T, String> converter) {
   public ConfigEntry<List<T>> toSequence() {
     ConfigEntry<List<T>> conf =
         new ConfigEntry<>(key, version, doc, alternatives, isPublic, isDeprecated);
-    conf.setValueConverter((String str) -> strToSeq(str, valueConverter));
-    conf.setStringConverter((List<T> val) -> seqToStr(val, stringConverter));
+
+    Function<String, T> elementConverter;
+    if (validator == null) {
+      elementConverter = valueConverter;
+    } else {
+      elementConverter =
+          value -> {
+            if (value == null) {
+              validator.accept(null);
+            }
+            T convertedValue = valueConverter.apply(value);
+            validator.accept(convertedValue);
+            return convertedValue;
+          };
+    }
+
+    conf.setValueConverter((String str) -> strToSeq(str, elementConverter));
+    conf.setStringConverter((List<T> val) -> val == null ? null : seqToStr(val, stringConverter));
+
     return conf;
   }
 

diff --git a/common/src/test/java/com/datastrato/gravitino/config/TestConfigEntryList.java b/common/src/test/java/com/datastrato/gravitino/config/TestConfigEntryList.java
@@ -39,7 +39,6 @@ public void testConfWithDefaultValue() {
             .doc("test")
             .internal()
             .stringConf()
-            .checkValue(value -> value == null, "error")
             .toSequence()
             .checkValue(valueList -> valueList.stream().allMatch("test-string"::equals), "error")
             .createWithDefault(Lists.newArrayList("test-string", "test-string", "test-string"));
@@ -123,13 +122,27 @@ public void testCheckValue() {
     testConfDefault.writeTo(configMap, Lists.newArrayList(-10, 2));
     Assertions.assertThrows(
         IllegalArgumentException.class, () -> testConfDefault.readFrom(configMap));
+
     ConfigEntry<List<String>> testConfNoDefault =
         new ConfigBuilder("gravitino.test.no.default")
             .stringConf()
             .toSequence()
             .checkValue(Objects::nonNull, "error")
             .create();
     Assertions.assertThrows(
-        NullPointerException.class, () -> testConfNoDefault.readFrom(configMap));
+        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(""));
+    Assertions.assertThrows(
+        IllegalArgumentException.class, () -> testConfWithoutDefault.readFrom(configMap));
   }
 }
diff --git a/core/src/main/java/com/datastrato/gravitino/Configs.java b/core/src/main/java/com/datastrato/gravitino/Configs.java
@@ -254,8 +254,11 @@ public interface Configs {
           .doc("The admins of Gravitino service")
           .version(ConfigConstants.VERSION_0_5_0)
           .stringConf()
-          .checkValue(StringUtils::isNotBlank, ConfigConstants.NOT_BLANK_ERROR_MSG)
           .toSequence()
+          .checkValue(
+              valueList ->
+                  valueList != null && valueList.stream().allMatch(StringUtils::isNotBlank),
+              ConfigConstants.NOT_BLANK_ERROR_MSG)
           .create();
 
   ConfigEntry<Long> ROLE_CACHE_EVICTION_INTERVAL_MS =

diff --git a/...t/java/com/datastrato/gravitino/authorization/TestAccessControlManagerForPermissions.java b/...t/java/com/datastrato/gravitino/authorization/TestAccessControlManagerForPermissions.java
@@ -5,6 +5,7 @@
 package com.datastrato.gravitino.authorization;
 
 import com.datastrato.gravitino.Config;
+import com.datastrato.gravitino.Configs;
 import com.datastrato.gravitino.Entity;
 import com.datastrato.gravitino.EntityStore;
 import com.datastrato.gravitino.GravitinoEnv;
@@ -92,6 +93,7 @@ public class TestAccessControlManagerForPermissions {
   @BeforeAll
   public static void setUp() throws Exception {
     config = new Config(false) {};
+    config.set(Configs.SERVICE_ADMINS, Lists.newArrayList("admin"));
 
     entityStore = new TestMemoryEntityStore.InMemoryEntityStore();
     entityStore.initialize(config);