fix: adding type validation and lazily adding cli options (#43467)

* fix: adding type validation and lazily adding cli options

closes: #43466

Signed-off-by: Steve Hawkins <shawkins@redhat.com>

* consolidating empty value checking

Signed-off-by: Steve Hawkins <shawkins@redhat.com>

* stripping the smallrye code if possible

Signed-off-by: Steve Hawkins <shawkins@redhat.com>

---------

Signed-off-by: Steve Hawkins <shawkins@redhat.com>

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

@@ -9,6 +9,7 @@ import java.util.stream.Collectors;
 
 public class Option<T> {
     private final Class<T> type;
+    private final Class<?> componentType;
     private final String key;
     private final OptionCategory category;
     private final boolean hidden;
@@ -24,7 +25,7 @@ public class Option<T> {
 
     public Option(Class<T> type, String key, OptionCategory category, boolean hidden, boolean buildTime, String description,
                   Optional<T> defaultValue, List<String> expectedValues, boolean strictExpectedValues, boolean caseInsensitiveExpectedValues,
-                  DeprecatedMetadata deprecatedMetadata, Set<String> connectedOptions, String wildcardKey) {
+                  DeprecatedMetadata deprecatedMetadata, Set<String> connectedOptions, String wildcardKey, Class<?> componentType) {
         this.type = type;
         this.key = key;
         this.category = category;
@@ -38,6 +39,7 @@ public class Option<T> {
         this.deprecatedMetadata = deprecatedMetadata;
         this.connectedOptions = connectedOptions;
         this.wildcardKey = wildcardKey;
+        this.componentType = componentType;
     }
 
     public Class<T> getType() {
@@ -168,4 +170,8 @@ public class Option<T> {
     public static String transformEnumValue(String value) {
         return CaseFormat.UPPER_UNDERSCORE.to(CaseFormat.LOWER_HYPHEN, value);
     }
+
+    public Class<?> getComponentType() {
+        return componentType;
+    }
 }

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

@@ -194,7 +194,7 @@ public class OptionBuilder<T> {
             }
         }
 
-        return new Option<T>(type, key, category, hidden, build, description, defaultValue, expectedValues, strictExpectedValues, caseInsensitiveExpectedValues, deprecatedMetadata, connectedOptions, wildcardKey);
+        return new Option<T>(type, key, category, hidden, build, description, defaultValue, expectedValues, strictExpectedValues, caseInsensitiveExpectedValues, deprecatedMetadata, connectedOptions, wildcardKey, expected);
     }
 
 }

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

@@ -39,12 +39,9 @@ import java.util.Map;
 import java.util.Optional;
 import java.util.Properties;
 import java.util.Set;
-import java.util.function.Consumer;
 import java.util.function.Function;
-import java.util.stream.Collectors;
 
 import org.keycloak.common.profile.ProfileException;
-import org.keycloak.common.util.CollectionUtil;
 import org.keycloak.config.DeprecatedMetadata;
 import org.keycloak.config.Option;
 import org.keycloak.config.OptionCategory;
@@ -77,6 +74,7 @@ import picocli.CommandLine.Help.Ansi;
 import picocli.CommandLine.Help.Ansi.Style;
 import picocli.CommandLine.Help.ColorScheme;
 import picocli.CommandLine.IFactory;
+import picocli.CommandLine.MissingParameterException;
 import picocli.CommandLine.Model.ArgGroupSpec;
 import picocli.CommandLine.Model.CommandSpec;
 import picocli.CommandLine.Model.ISetter;
@@ -99,8 +97,6 @@ public class Picocli {
     }
 
     private final ExecutionExceptionHandler errorHandler = new ExecutionExceptionHandler();
-    private Set<PropertyMapper<?>> allowedMappers;
-    private final List<String> unrecognizedArgs = new ArrayList<>();
     private Optional<AbstractCommand> parsedCommand = Optional.empty();
     private boolean warnedTimestampChanged;
 
@@ -114,17 +110,65 @@ public class Picocli {
         return colorMode;
     }
 
+    private boolean isHelpRequested(ParseResult result) {
+        if (result.isUsageHelpRequested()) {
+            return true;
+        }
+
+        return result.subcommands().stream().anyMatch(this::isHelpRequested);
+    }
+
     public void parseAndRun(List<String> cliArgs) {
-        // perform two passes over the cli args. First without option validation to determine the current command, then with option validation enabled
-        CommandLine cmd = createCommandLine(spec -> {}).setUnmatchedArgumentsAllowed(true);
+        List<String> unrecognizedArgs = new ArrayList<>();
+        CommandLine cmd = createCommandLine(unrecognizedArgs);
+
         String[] argArray = cliArgs.toArray(new String[0]);
 
         try {
-            ParseResult result = cmd.parseArgs(argArray); // process the cli args first to init the config file and perform validation
+            ParseResult result = cmd.parseArgs(argArray);
+
             var commandLineList = result.asCommandLineList();
 
-            // recreate the command specifically for the current
-            cmd = createCommandLineForCommand(cliArgs, commandLineList);
+            CommandLine cl = commandLineList.get(commandLineList.size() - 1);
+
+            AbstractCommand currentCommand = null;
+            if (cl.getCommand() instanceof AbstractCommand ac) {
+                currentCommand = ac;
+            }
+            initConfig(cliArgs, currentCommand);
+
+            if (!unrecognizedArgs.isEmpty()) {
+                IncludeOptions options = Optional.ofNullable(currentCommand).map(c -> getIncludeOptions(cliArgs, c, c.getName())).orElse(new IncludeOptions());
+                Set<OptionCategory> allowedCategories = Set.copyOf(Optional.ofNullable(currentCommand).map(AbstractCommand::getOptionCategories).orElse(List.of()));
+                // TODO: further refactor this as these args should be the source for ConfigArgsConfigSource
+                unrecognizedArgs.removeIf(arg -> {
+                    boolean hasArg = false;
+                    if (arg.contains("=")) {
+                        arg = arg.substring(0, arg.indexOf("="));
+                        hasArg = true;
+                    }
+                    PropertyMapper<?> mapper = PropertyMappers.getMapperByCliKey(arg);
+                    if (mapper != null) {
+                        if (!allowedCategories.contains(mapper.getCategory()) || (mapper.isBuildTime() && !options.includeBuildTime) || (mapper.isRunTime() && !options.includeRuntime)) {
+                            return false;
+                        }
+                        if (!hasArg) {
+                            addCommandOptions(cliArgs, cl);
+                            throw new MissingParameterException(cl, cl.getCommandSpec().optionsMap().get(arg), null);
+                        }
+                        return true;
+                    }
+                    return false;
+                });
+                if (!unrecognizedArgs.isEmpty()) {
+                    addCommandOptions(cliArgs, cl);
+                    throw new KcUnmatchedArgumentException(cl, unrecognizedArgs);
+                }
+            }
+
+            if (isHelpRequested(result)) {
+                addCommandOptions(cliArgs, cl);
+            }
 
             int exitCode = cmd.execute(argArray);
 
@@ -136,53 +180,6 @@ public class Picocli {
         }
     }
 
-    private CommandLine createCommandLineForCommand(List<String> cliArgs, List<CommandLine> commandLineList) {
-        return createCommandLine(spec -> {
-            // use the incoming commandLineList from the initial parsing to determine the current command
-            CommandSpec currentSpec = spec;
-
-            // add help to the root and all commands as it is not inherited
-            addHelp(currentSpec);
-
-            for (CommandLine commandLine : commandLineList.subList(1, commandLineList.size())) {
-                CommandLine subCommand = currentSpec.subcommands().get(commandLine.getCommandName());
-                if (subCommand == null) {
-                    currentSpec = null;
-                    break;
-                }
-
-                currentSpec = subCommand.getCommandSpec();
-
-                currentSpec.addUnmatchedArgsBinding(CommandLine.Model.UnmatchedArgsBinding.forStringArrayConsumer(new ISetter() {
-                    @Override
-                    public <T> T set(T value) {
-                        if (value != null) {
-                            unrecognizedArgs.addAll(Arrays.asList((String[]) value));
-                        }
-                        return null; // doesn't matter
-                    }
-                }));
-
-                addHelp(currentSpec);
-            }
-
-            AbstractCommand currentCommand = null;
-            CommandLine commandLine = null;
-            if (currentSpec != null) {
-                commandLine = currentSpec.commandLine();
-
-                if (commandLine != null && commandLine.getCommand() instanceof AbstractCommand ac) {
-                    currentCommand = ac;
-                }
-            }
-            // init the config before adding options to properly sanitize mappers
-            initConfig(cliArgs, currentCommand);
-            if (commandLine != null) {
-                addCommandOptions(cliArgs, commandLine);
-            }
-        });
-    }
-
     public Optional<AbstractCommand> getParsedCommand() {
         return parsedCommand;
     }
@@ -218,17 +215,6 @@ public class Picocli {
      * @param abstractCommand
      */
     public void validateConfig(List<String> cliArgs, AbstractCommand abstractCommand) {
-        unrecognizedArgs.removeIf(arg -> {
-            if (arg.contains("=")) {
-                arg = arg.substring(0, arg.indexOf("="));
-            }
-            PropertyMapper<?> mapper = PropertyMappers.getMapperByCliKey(arg);
-            return mapper != null && mapper.hasWildcard() && allowedMappers.contains(mapper);
-        });
-        if (!unrecognizedArgs.isEmpty()) {
-            throw new KcUnmatchedArgumentException(abstractCommand.getCommandLine().orElseThrow(), unrecognizedArgs);
-        }
-
         if (cliArgs.contains(OPTIMIZED_BUILD_OPTION_LONG) && !wasBuildEverRun()) {
             throw new PropertyException(Messages.optimizedUsedForFirstStartup());
         }
@@ -601,7 +587,30 @@ public class Picocli {
         return properties;
     }
 
-    public CommandLine createCommandLine(Consumer<CommandSpec> consumer) {
+    private void updateSpecHelpAndUnmatched(CommandSpec spec, List<String> unrecognizedArgs) {
+        try {
+            spec.addOption(OptionSpec.builder(Help.OPTION_NAMES)
+                    .usageHelp(true)
+                    .description("This help message.")
+                    .build());
+        } catch (DuplicateOptionAnnotationsException e) {
+            // Completion is inheriting mixinStandardHelpOptions = true
+        }
+
+        spec.addUnmatchedArgsBinding(CommandLine.Model.UnmatchedArgsBinding.forStringArrayConsumer(new ISetter() {
+            @Override
+            public <T> T set(T value) {
+                if (value != null) {
+                    unrecognizedArgs.addAll(Arrays.asList((String[]) value));
+                }
+                return null; // doesn't matter
+            }
+        }));
+
+        spec.subcommands().values().forEach(c -> updateSpecHelpAndUnmatched(c.getCommandSpec(), unrecognizedArgs));
+    }
+
+    CommandLine createCommandLine(List<String> unrecognizedArgs) {
         CommandSpec spec = CommandSpec.forAnnotatedObject(new Main(), new IFactory() {
             @Override
             public <K> K create(Class<K> cls) throws Exception {
@@ -612,7 +621,7 @@ public class Picocli {
                 return result;
             }
         }).name(Environment.getCommand());
-        consumer.accept(spec);
+        updateSpecHelpAndUnmatched(spec, unrecognizedArgs);
 
         CommandLine cmd = new CommandLine(spec);
         cmd.setExpandAtFiles(false);
@@ -634,17 +643,6 @@ public class Picocli {
         return new PrintWriter(System.out, true);
     }
 
-    private static void addHelp(CommandSpec currentSpec) {
-        try {
-            currentSpec.addOption(OptionSpec.builder(Help.OPTION_NAMES)
-                    .usageHelp(true)
-                    .description("This help message.")
-                    .build());
-        } catch (DuplicateOptionAnnotationsException e) {
-            // Completion is inheriting mixinStandardHelpOptions = true
-        }
-    }
-
     private IncludeOptions getIncludeOptions(List<String> cliArgs, AbstractCommand abstractCommand, String commandName) {
         IncludeOptions result = new IncludeOptions();
         if (abstractCommand == null) {
@@ -689,10 +687,6 @@ public class Picocli {
         }
 
         addMappedOptionsToArgGroups(commandLine, mappers);
-
-        if (CollectionUtil.isEmpty(allowedMappers)) {
-            allowedMappers = mappers.values().stream().flatMap(List::stream).collect(Collectors.toUnmodifiableSet());
-        }
     }
 
     private static <T extends Map<OptionCategory, List<PropertyMapper<?>>>> void combinePropertyMappers(T origMappers, T additionalMappers) {

quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/mappers/PropertyMapper.java

@@ -31,6 +31,8 @@ import java.util.function.BiFunction;
 import java.util.function.BooleanSupplier;
 import java.util.function.Consumer;
 import java.util.function.Function;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
 import java.util.stream.Stream;
 
 import org.keycloak.common.Profile;
@@ -42,6 +44,7 @@ import org.keycloak.quarkus.runtime.cli.PropertyException;
 import org.keycloak.quarkus.runtime.cli.ShortErrorMessageHandler;
 import org.keycloak.quarkus.runtime.cli.command.AbstractCommand;
 import org.keycloak.quarkus.runtime.configuration.ConfigArgsConfigSource;
+import org.keycloak.quarkus.runtime.configuration.Configuration;
 import org.keycloak.quarkus.runtime.configuration.KcEnvConfigSource;
 import org.keycloak.quarkus.runtime.configuration.KeycloakConfigSourceProvider;
 import org.keycloak.quarkus.runtime.configuration.NestedPropertyMappingInterceptor;
@@ -595,6 +598,24 @@ public class PropertyMapper<T> {
                 continue;
             }
             try {
+                if (option.getComponentType() != String.class && option.getExpectedValues().isEmpty()) {
+                    if (v.isEmpty()) {
+                        throw new PropertyException("Invalid empty value for option %s".formatted(getOptionAndSourceMessage(configValue)));
+                    }
+                    try {
+                        Configuration.getConfig().convert(v, option.getComponentType());
+                    } catch (Exception e) {
+                        // strip the smallrye code if possible
+                        String message = e.getMessage();
+                        Pattern p = Pattern.compile("SRCFG\\d+: (.*)$");
+                        Matcher m = p.matcher(message);
+                        if (m.find()) {
+                            message = m.group(1);
+                        }
+                        throw new PropertyException("Invalid value for option %s: %s".formatted(getOptionAndSourceMessage(configValue), message));
+                    }
+                }
+
                 singleValidator.accept(configValue, v);
             } catch (PropertyException e) {
                 if (!result.isEmpty()) {

quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/cli/PicocliTest.java

@@ -187,7 +187,24 @@ public class PicocliTest extends AbstractConfigurationTest {
         NonRunningPicocli nonRunningPicocli = pseudoLaunch("start-dev", "--http-port=a");
         assertEquals(CommandLine.ExitCode.USAGE, nonRunningPicocli.exitCode);
         assertThat(nonRunningPicocli.getErrString(),
-                containsString("Invalid value for option '--http-port': 'a' is not an int"));
+                containsString("Invalid value for option '--http-port': Expected an integer value, got \"a\""));
+    }
+
+    @Test
+    public void testInvalidArgumentTypeEnv() {
+        putEnvVar("KC_HTTP_PORT", "a");
+        NonRunningPicocli nonRunningPicocli = pseudoLaunch("start-dev");
+        assertEquals(CommandLine.ExitCode.USAGE, nonRunningPicocli.exitCode);
+        assertThat(nonRunningPicocli.getErrString(),
+                containsString("Invalid value for option 'KC_HTTP_PORT': Expected an integer value, got \"a\""));
+    }
+
+    @Test
+    public void testEmptyValue() {
+        NonRunningPicocli nonRunningPicocli = pseudoLaunch("start-dev", "--http-port=");
+        assertEquals(CommandLine.ExitCode.USAGE, nonRunningPicocli.exitCode);
+        assertThat(nonRunningPicocli.getErrString(),
+                containsString("Invalid empty value for option '--http-port'"));
     }
 
     @Test
@@ -804,7 +821,7 @@ public class PicocliTest extends AbstractConfigurationTest {
 
         NonRunningPicocli nonRunningPicocli = pseudoLaunch("start-dev", "--log=%s".formatted(logHandlerOptionsName), "--log-%s-async=true".formatted(logHandlerOptionsName), "--log-%s-async-queue-length=invalid".formatted(logHandlerOptionsName));
         assertEquals(CommandLine.ExitCode.USAGE, nonRunningPicocli.exitCode);
-        assertThat(nonRunningPicocli.getErrString(), containsString("Invalid value for option '--log-%s-async-queue-length': 'invalid' is not an int".formatted(logHandlerOptionsName)));
+        assertThat(nonRunningPicocli.getErrString(), containsString("Invalid value for option '--log-%s-async-queue-length': Expected an integer value, got \"invalid\"".formatted(logHandlerOptionsName)));
 
         onAfter();
         nonRunningPicocli = pseudoLaunch("start-dev", "--log=%s".formatted(logHandlerName), "--log-%s-async-queue-length=768".formatted(logHandlerOptionsName));
@@ -858,7 +875,7 @@ public class PicocliTest extends AbstractConfigurationTest {
 
         var nonRunningPicocli = pseudoLaunch("start-dev", "--log=%s".formatted(handlerName), "--log-%s-async=true".formatted(handlerName), "--log-%s-async-queue-length=invalid".formatted(handlerName));
         assertEquals(CommandLine.ExitCode.USAGE, nonRunningPicocli.exitCode);
-        assertThat(nonRunningPicocli.getErrString(), containsString("Invalid value for option '--log-%s-async-queue-length': 'invalid' is not an int".formatted(handlerName)));
+        assertThat(nonRunningPicocli.getErrString(), containsString("Invalid value for option '--log-%s-async-queue-length': Expected an integer value, got \"invalid\"".formatted(handlerName)));
     }
 
     @Test

quarkus/tests/integration/src/test/java/org/keycloak/it/cli/dist/CacheEmbeddedMtlsDistTest.java

@@ -99,7 +99,7 @@ public class CacheEmbeddedMtlsDistTest {
 
         // test blank
         result = dist.run("start-dev", "--cache=ispn", "--cache-embedded-mtls-enabled=true", "--%s=".formatted(key));
-        result.assertError("Invalid value for option '--%s': '' is not an int".formatted(key));
+        result.assertError("Invalid empty value for option '--%s'".formatted(key));
     }
 
     @Test