fix: provide better error messages for list options (keycloak#25918)

closes: keycloak#25235

Signed-off-by: Steve Hawkins <[email protected]>

quarkus/config-api/src/main/java/org/keycloak/config/DatabaseOptions.java

@@ -19,7 +19,7 @@ public class DatabaseOptions {
             .category(OptionCategory.DATABASE)
             .description("The database vendor.")
             .defaultValue("dev-file")
-            .expectedValues(Database::getDatabaseAliases)
+            .expectedValues(Database.getDatabaseAliases())
             .buildTime(true)
             .build();
 

quarkus/config-api/src/main/java/org/keycloak/config/FeatureOptions.java

@@ -10,18 +10,18 @@
 
 public class FeatureOptions {
 
-    public static final Option<List> FEATURES = new OptionBuilder("features", List.class, Profile.Feature.class)
+    public static final Option<List<String>> FEATURES = OptionBuilder.listOptionBuilder("features", String.class)
             .category(OptionCategory.FEATURE)
             .description("Enables a set of one or more features.")
             .defaultValue(Optional.empty())
-            .expectedValues(() -> getFeatureValues(true))
+            .expectedValues(getFeatureValues(true))
             .buildTime(true)
             .build();
 
-    public static final Option<List> FEATURES_DISABLED = new OptionBuilder("features-disabled", List.class, Profile.Feature.class)
+    public static final Option<List<String>> FEATURES_DISABLED = OptionBuilder.listOptionBuilder("features-disabled", String.class)
             .category(OptionCategory.FEATURE)
             .description("Disables a set of one or more features.")
-            .expectedValues(() -> getFeatureValues(false))
+            .expectedValues(getFeatureValues(false))
             .buildTime(true)
             .build();
 

quarkus/config-api/src/main/java/org/keycloak/config/HttpOptions.java

@@ -1,6 +1,9 @@
 package org.keycloak.config;
 
 import java.io.File;
+import java.util.Arrays;
+import java.util.List;
+
 import org.keycloak.common.crypto.FipsMode;
 
 public class HttpOptions {
@@ -54,10 +57,10 @@ public enum ClientAuth {
             .description("The cipher suites to use. If none is given, a reasonable default is selected.")
             .build();
 
-    public static final Option<String> HTTPS_PROTOCOLS = new OptionBuilder<>("https-protocols", String.class)
+    public static final Option<List<String>> HTTPS_PROTOCOLS = OptionBuilder.listOptionBuilder("https-protocols", String.class)
             .category(OptionCategory.HTTP)
             .description("The list of protocols to explicitly enable.")
-            .defaultValue("TLSv1.3,TLSv1.2")
+            .defaultValue(Arrays.asList("TLSv1.3,TLSv1.2"))
             .build();
 
     public static final Option<File> HTTPS_CERTIFICATE_FILE = new OptionBuilder<>("https-certificate-file", File.class)

quarkus/config-api/src/main/java/org/keycloak/config/LoggingOptions.java

@@ -30,11 +30,11 @@ public static List<String> getAvailableHandlerNames() {
                 .toList();
     }
 
-    public static final Option<List> LOG = new OptionBuilder("log", List.class, Handler.class)
+    public static final Option<List<Handler>> LOG = OptionBuilder.listOptionBuilder("log", Handler.class)
             .category(OptionCategory.LOGGING)
             .description("Enable one or more log handlers in a comma-separated list.")
-            .expectedValues(() -> getAvailableHandlerNames())
-            .defaultValue(DEFAULT_LOG_HANDLER)
+            .expectedValues(getAvailableHandlerNames())
+            .defaultValue(Arrays.asList(DEFAULT_LOG_HANDLER))
             .build();
 
     public enum Level {
@@ -53,9 +53,9 @@ public String toString() {
         }
     }
 
-    public static final Option<String> LOG_LEVEL = new OptionBuilder<>("log-level", String.class)
+    public static final Option<List<String>> LOG_LEVEL = OptionBuilder.listOptionBuilder("log-level", String.class)
             .category(OptionCategory.LOGGING)
-            .defaultValue(DEFAULT_LOG_LEVEL.toString())
+            .defaultValue(Arrays.asList(DEFAULT_LOG_LEVEL.toString()))
             .description("The log level of the root category or a comma-separated list of individual categories and their levels. For the root category, you don't need to specify a category.")
             .build();
 

quarkus/config-api/src/main/java/org/keycloak/config/Option.java

@@ -2,7 +2,7 @@
 
 import java.util.List;
 import java.util.Optional;
-import java.util.function.Supplier;
+import java.util.stream.Collectors;
 
 public class Option<T> {
 
@@ -13,10 +13,10 @@ public class Option<T> {
     private final boolean buildTime;
     private final String description;
     private final Optional<T> defaultValue;
-    private final Supplier<List<String>> expectedValues;
+    private final List<String> expectedValues;
     private final DeprecatedMetadata deprecatedMetadata;
 
-    public Option(Class<T> type, String key, OptionCategory category, boolean hidden, boolean buildTime, String description, Optional<T> defaultValue, Supplier<List<String>> expectedValues, DeprecatedMetadata deprecatedMetadata) {
+    public Option(Class<T> type, String key, OptionCategory category, boolean hidden, boolean buildTime, String description, Optional<T> defaultValue, List<String> expectedValues, DeprecatedMetadata deprecatedMetadata) {
         this.type = type;
         this.key = key;
         this.category = category;
@@ -53,7 +53,7 @@ public Optional<T> getDefaultValue() {
     }
 
     public List<String> getExpectedValues() {
-        return expectedValues.get();
+        return expectedValues;
     }
 
     public Optional<DeprecatedMetadata> getDeprecatedMetadata() {
@@ -90,4 +90,14 @@ private static String getDescriptionByCategorySupportLevel(String description, O
 
         return description;
     }
+
+    public static String getDefaultValueString(Object value) {
+        if (value == null) {
+            return null;
+        }
+        if (value instanceof List) {
+            return ((List<?>) value).stream().map(String::valueOf).collect(Collectors.joining(","));
+        }
+        return String.valueOf(value);
+    }
 }

quarkus/config-api/src/main/java/org/keycloak/config/OptionBuilder.java

@@ -1,56 +1,54 @@
 package org.keycloak.config;
 
+import java.util.Collection;
 import java.util.List;
+import java.util.Map;
 import java.util.Optional;
 import java.util.Set;
-import java.util.function.Supplier;
 import java.util.stream.Collectors;
 
 public class OptionBuilder<T> {
 
-    private  static final Supplier<List<String>> EMPTY_VALUES_SUPPLIER = List::of;
-    private  static final Supplier<List<String>> BOOLEAN_TYPE_VALUES = new Supplier<List<String>>() {
-        List<String> values = List.of(Boolean.TRUE.toString(), Boolean.FALSE.toString());
-
-        @Override 
-        public List<String> get() {
-            return values;
-        }
-    };
+    private static final List<String> BOOLEAN_TYPE_VALUES = List.of(Boolean.TRUE.toString(), Boolean.FALSE.toString());
 
     private final Class<T> type;
-    private final Class<T> auxiliaryType;
     private final String key;
     private OptionCategory category;
     private boolean hidden;
     private boolean build;
     private String description;
     private Optional<T> defaultValue;
-    private Supplier<List<String>> expectedValues;
+    private List<String> expectedValues = List.of();
     private DeprecatedMetadata deprecatedMetadata;
 
+    public static <A> OptionBuilder<List<A>> listOptionBuilder(String key, Class<A> type) {
+        return new OptionBuilder(key, List.class, type);
+    }
+
     public OptionBuilder(String key, Class<T> type) {
         this(key, type, null);
     }
 
-    public OptionBuilder(String key, Class<T> type, Class<T> auxiliaryType) {
+    private OptionBuilder(String key, Class<T> type, Class<?> auxiliaryType) {
         this.type = type;
-        this.auxiliaryType = auxiliaryType;
+        if (type.isArray() || ((Collection.class.isAssignableFrom(type) || Map.class.isAssignableFrom(type)) && type != java.util.List.class)) {
+            throw new IllegalArgumentException("Non-List multi-valued options are not yet supported");
+        }
         this.key = key;
         category = OptionCategory.GENERAL;
         hidden = false;
         build = false;
         description = null;
-        defaultValue = Boolean.class.equals(type) ? Optional.of((T) Boolean.FALSE) : Optional.empty();
-        expectedValues = EMPTY_VALUES_SUPPLIER;
-        if (Boolean.class.equals(type)) {
-            expectedValues(BOOLEAN_TYPE_VALUES);
+        Class<?> expected = type;
+        if (auxiliaryType != null) {
+            expected = auxiliaryType;
         }
-        if (Enum.class.isAssignableFrom(type)) {
-            expectedValues((Class<? extends Enum>) type);
+        defaultValue = Boolean.class.equals(expected) ? Optional.of((T) Boolean.FALSE) : Optional.empty();
+        if (Boolean.class.equals(expected)) {
+            expectedValues(BOOLEAN_TYPE_VALUES);
         }
-        if (auxiliaryType != null && Enum.class.isAssignableFrom(auxiliaryType)) {
-            expectedValues((Class<? extends Enum>) auxiliaryType);
+        if (Enum.class.isAssignableFrom(expected)) {
+            expectedValues((Class<? extends Enum>) expected);
         }
     }
 
@@ -84,32 +82,18 @@ public OptionBuilder<T> defaultValue(T defaultV) {
         return this;
     }
 
-    public OptionBuilder<T> expectedValues(Supplier<List<String>> expected) {
+    public OptionBuilder<T> expectedValues(List<String> expected) {
         this.expectedValues = expected;
         return this;
     }
 
     public OptionBuilder<T> expectedValues(Class<? extends Enum> expected) {
-        this.expectedValues = new Supplier<>() {
-            List<String> values = List.of(expected.getEnumConstants()).stream().map(Object::toString).collect(Collectors.toList());
-
-            @Override
-            public List<String> get() {
-                return values;
-            }
-        };
+        this.expectedValues = List.of(expected.getEnumConstants()).stream().map(Object::toString).collect(Collectors.toList());
         return this;
     }
 
     public OptionBuilder<T> expectedValues(T ... expected) {
-        this.expectedValues = new Supplier<>() {
-            List<String> values = List.of(expected).stream().map(v -> v.toString()).collect(Collectors.toList());
-
-            @Override
-            public List<String> get() {
-                return values;
-            }
-        };
+        this.expectedValues = List.of(expected).stream().map(v -> v.toString()).collect(Collectors.toList());
         return this;
     }
 
@@ -135,11 +119,7 @@ public OptionBuilder<T> deprecated(String note, Set<String> newOptionsKeys) {
 
 
     public Option<T> build() {
-        if (auxiliaryType != null) {
-            return new MultiOption<T>(type, auxiliaryType, key, category, hidden, build, description, defaultValue, expectedValues, deprecatedMetadata);
-        } else {
-            return new Option<T>(type, key, category, hidden, build, description, defaultValue, expectedValues, deprecatedMetadata);
-        }
+        return new Option<T>(type, key, category, hidden, build, description, defaultValue, expectedValues, deprecatedMetadata);
     }
 
 }

quarkus/config-api/src/main/java/org/keycloak/config/SecurityOptions.java

@@ -10,7 +10,7 @@ public class SecurityOptions {
 
     public static final Option<FipsMode> FIPS_MODE = new OptionBuilder<>("fips-mode", FipsMode.class)
             .category(OptionCategory.SECURITY)
-            .expectedValues(SecurityOptions::getFipsModeValues)
+            .expectedValues(getFipsModeValues())
             .buildTime(true)
             .description("Sets the FIPS mode. If '" + FipsMode.NON_STRICT + "' is set, FIPS is enabled but on non-approved mode. For full FIPS compliance, set '" + FipsMode.STRICT + "' to run on approved mode. "
                     + "This option defaults to '" + FipsMode.DISABLED + "' when '" + Profile.Feature.FIPS.getKey() + "' feature is disabled, which is by default. "

quarkus/config-api/src/main/java/org/keycloak/config/TruststoreOptions.java

@@ -2,9 +2,11 @@
 
 import org.keycloak.common.enums.HostnameVerificationPolicy;
 
+import java.util.List;
+
 public class TruststoreOptions {
 
-    public static final Option<String> TRUSTSTORE_PATHS = new OptionBuilder<>("truststore-paths", String.class)
+    public static final Option<List<String>> TRUSTSTORE_PATHS = OptionBuilder.listOptionBuilder("truststore-paths", String.class)
             .category(OptionCategory.TRUSTSTORE)
             .description("List of pkcs12 (p12 or pfx file extensions), PEM files, or directories containing those files that will be used as a system truststore.")
             .build();

quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/Picocli.java

@@ -56,7 +56,7 @@
 import org.eclipse.microprofile.config.spi.ConfigSource;
 import org.jboss.logging.Logger;
 import org.keycloak.config.DeprecatedMetadata;
-import org.keycloak.config.MultiOption;
+import org.keycloak.config.Option;
 import org.keycloak.config.OptionCategory;
 import org.keycloak.quarkus.runtime.cli.command.AbstractCommand;
 import org.keycloak.quarkus.runtime.cli.command.Build;
@@ -399,7 +399,7 @@ private static boolean hasConfigChanges(CommandLine cmdCommand) {
             if (runtimeValue == null && isNotBlank(persistedValue)) {
                 PropertyMapper<?> mapper = PropertyMappers.getMapper(propertyName);
 
-                if (mapper != null && persistedValue.equals(mapper.getDefaultValue().map(Object::toString).orElse(null))) {
+                if (mapper != null && persistedValue.equals(Option.getDefaultValueString(mapper.getDefaultValue().orElse(null)))) {
                     // same as default
                     continue;
                 }
@@ -608,14 +608,11 @@ public Iterator<String> iterator() {
                         .hidden(mapper.isHidden());
 
                 if (mapper.getDefaultValue().isPresent()) {
-                    optBuilder.defaultValue(mapper.getDefaultValue().get().toString());
+                    optBuilder.defaultValue(Option.getDefaultValueString(mapper.getDefaultValue().get()));
                 }
 
                 if (mapper.getType() != null) {
                     optBuilder.type(mapper.getType());
-                    if (mapper.getOption() instanceof MultiOption) {
-                        optBuilder.auxiliaryTypes(((MultiOption<?>) mapper.getOption()).getAuxiliaryType());
-                    }
                 } else {
                     optBuilder.type(String.class);
                 }
@@ -639,7 +636,7 @@ private static String getDecoratedOptionDescription(PropertyMapper<?> mapper) {
         }
 
         mapper.getDefaultValue()
-                .map(d -> d.toString().replaceAll("%", "%%")) // escape formats
+                .map(d -> Option.getDefaultValueString(d).replaceAll("%", "%%")) // escape formats
                 .map(d -> " Default: " + d + ".")
                 .ifPresent(transformedDesc::append);