diff --git a/common/src/main/java/com/datastrato/gravitino/config/ConfigEntry.java b/common/src/main/java/com/datastrato/gravitino/config/ConfigEntry.java index 21876108ee1..3892f044c4b 100644 --- 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 seq, Function converter) { public ConfigEntry> toSequence() { ConfigEntry> conf = new ConfigEntry<>(key, version, doc, alternatives, isPublic, isDeprecated); - conf.setValueConverter((String str) -> strToSeq(str, valueConverter)); - conf.setStringConverter((List val) -> seqToStr(val, stringConverter)); + + Function 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 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 index f6f299b218c..8a42e4045a1 100644 --- 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,6 +122,7 @@ public void testCheckValue() { testConfDefault.writeTo(configMap, Lists.newArrayList(-10, 2)); Assertions.assertThrows( IllegalArgumentException.class, () -> testConfDefault.readFrom(configMap)); + ConfigEntry> testConfNoDefault = new ConfigBuilder("gravitino.test.no.default") .stringConf() @@ -130,6 +130,19 @@ public void testCheckValue() { .checkValue(Objects::nonNull, "error") .create(); Assertions.assertThrows( - NullPointerException.class, () -> testConfNoDefault.readFrom(configMap)); + IllegalArgumentException.class, () -> testConfNoDefault.readFrom(configMap)); + + // To test checkValue before calling `toSequence` + ConfigEntry> 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 index f92faee88da..894542ce90f 100644 --- 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 ROLE_CACHE_EVICTION_INTERVAL_MS = diff --git a/core/src/test/java/com/datastrato/gravitino/authorization/TestAccessControlManagerForPermissions.java b/core/src/test/java/com/datastrato/gravitino/authorization/TestAccessControlManagerForPermissions.java index 8fab7127f09..f30f84ee944 100644 --- a/core/src/test/java/com/datastrato/gravitino/authorization/TestAccessControlManagerForPermissions.java +++ b/core/src/test/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);