fix: preservation of the command line in the --optimized suggestion (#26163)

closes #26140

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

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

@@ -192,7 +192,7 @@ public final class Picocli {
     private static List<String> getSanitizedRuntimeCliOptions() {
         List<String> properties = new ArrayList<>();
 
-        parseConfigArgs(new BiConsumer<String, String>() {
+        parseConfigArgs(ConfigArgsConfigSource.getAllCliArgs(), new BiConsumer<String, String>() {
             @Override
             public void accept(String key, String value) {
                 PropertyMapper<?> mapper = PropertyMappers.getMapper(key);
@@ -203,6 +203,8 @@ public final class Picocli {
 
                 properties.add(key + "=" + formatValue(key, value));
             }
+        }, arg -> {
+            properties.add(arg);
         });
 
         return properties;
@@ -225,7 +227,7 @@ public final class Picocli {
         exitCode = cmd.execute(configArgsList.toArray(new String[0]));
 
         if(!isDevMode() && exitCode == cmd.getCommandSpec().exitCodeOnSuccess()) {
-            cmd.getOut().printf("Next time you run the server, just run:%n%n\t%s %s %s %s%n%n", Environment.getCommand(), commandName, OPTIMIZED_BUILD_OPTION_LONG, String.join(" ", getSanitizedRuntimeCliOptions()));
+            cmd.getOut().printf("Next time you run the server, just run:%n%n\t%s %s %s%n%n", Environment.getCommand(), String.join(" ", getSanitizedRuntimeCliOptions()), OPTIMIZED_BUILD_OPTION_LONG);
         }
 
         return exitCode;

quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/ConfigArgsConfigSource.java

@@ -17,7 +17,6 @@
 
 package org.keycloak.quarkus.runtime.configuration;
 
-import static java.util.Arrays.asList;
 import static org.keycloak.quarkus.runtime.cli.Picocli.ARG_SHORT_PREFIX;
 import static org.keycloak.quarkus.runtime.configuration.Configuration.OPTION_PART_SEPARATOR_CHAR;
 import static org.keycloak.quarkus.runtime.configuration.MicroProfileConfigProvider.NS_KEYCLOAK_PREFIX;
@@ -26,34 +25,37 @@ import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import java.util.function.BiConsumer;
+import java.util.function.Consumer;
 import java.util.regex.Pattern;
 
 import io.smallrye.config.PropertiesConfigSource;
 
+import org.keycloak.quarkus.runtime.cli.command.Main;
 import org.keycloak.quarkus.runtime.configuration.mappers.PropertyMapper;
 import org.keycloak.quarkus.runtime.configuration.mappers.PropertyMappers;
-import org.keycloak.utils.StringUtil;
 
 /**
  * <p>A configuration source for mapping configuration arguments to their corresponding properties so that they can be recognized
  * when building and running the server.
- * 
+ *
  * <p>The mapping is based on the system property {@code kc.config.args}, where the value is a comma-separated list of
  * the arguments passed during build or runtime. E.g: "--http-enabled=true,--http-port=8180,--database-vendor=postgres".
- * 
+ *
- * <p>Each argument is going to be mapped to its corresponding configuration property by prefixing the key with the {@link MicroProfileConfigProvider#NS_KEYCLOAK} namespace. 
+ * <p>Each argument is going to be mapped to its corresponding configuration property by prefixing the key with the {@link MicroProfileConfigProvider#NS_KEYCLOAK} namespace.
  */
 public class ConfigArgsConfigSource extends PropertiesConfigSource {
 
+    public static final Set<String> SHORT_OPTIONS_ACCEPTING_VALUE = Set.of(Main.PROFILE_SHORT_NAME, Main.CONFIG_FILE_SHORT_NAME);
+
     public static final String CLI_ARGS = "kc.config.args";
     public static final String NAME = "CliConfigSource";
     private static final String ARG_SEPARATOR = ";;";
-    private static final Pattern ARG_SPLIT = Pattern.compile(";;");
     private static final Pattern ARG_KEY_VALUE_SPLIT = Pattern.compile("=");
 
     protected ConfigArgsConfigSource() {
-        super(parseArgument(), NAME, 600);
+        super(parseArguments(), NAME, 600);
     }
 
     public static void setCliArgs(String[] args) {
@@ -98,16 +100,16 @@ public class ConfigArgsConfigSource extends PropertiesConfigSource {
         return properties.get(propertyName.replace(OPTION_PART_SEPARATOR_CHAR, '.'));
     }
 
-    private static Map<String, String> parseArgument() {
+    private static Map<String, String> parseArguments() {
         String rawArgs = getRawConfigArgs();
-        
+
         if (rawArgs == null || "".equals(rawArgs.trim())) {
             return Collections.emptyMap();
         }
 
         Map<String, String> properties = new HashMap<>();
 
-        parseConfigArgs(new BiConsumer<String, String>() {
+        parseConfigArgs(getAllCliArgs(), new BiConsumer<String, String>() {
             @Override
             public void accept(String key, String value) {
                 key = NS_KEYCLOAK_PREFIX + key.substring(2);
@@ -126,56 +128,48 @@ public class ConfigArgsConfigSource extends PropertiesConfigSource {
                     properties.put(mapper.getFrom(), value);
                 }
             }
-        });
+        }, ignored -> {});
 
         return properties;
     }
 
-    public static void parseConfigArgs(BiConsumer<String, String> cliArgConsumer) {
+    public static void parseConfigArgs(List<String> args, BiConsumer<String, String> valueArgConsumer, Consumer<String> unaryConsumer) {
-        // init here because the class might be loaded by CL without init
+        for (int i = 0; i < args.size(); i++) {
-        List<String> ignoredArgs = asList("--verbose", "-v", "--help", "-h");
+            String arg = args.get(i);
-        String rawArgs = getRawConfigArgs();
-
-        if (StringUtil.isBlank(rawArgs)) {
-            return;
-        }
-
-        String[] args = ARG_SPLIT.split(rawArgs);
-
-        for (int i = 0; i < args.length; i++) {
-            String arg = args[i];
-
-            if (ignoredArgs.contains(arg)) {
-                continue;
-            }
 
             if (!arg.startsWith(ARG_SHORT_PREFIX)) {
+                unaryConsumer.accept(arg);
                 continue;
             }
 
             String[] keyValue = ARG_KEY_VALUE_SPLIT.split(arg, 2);
             String key = keyValue[0];
 
-            if ("".equals(key.trim())) {
-                throw new IllegalArgumentException("Invalid argument key");
-            }
-
             String value;
 
             if (keyValue.length == 1) {
-                if (args.length <= i + 1) {
+                if (args.size() <= i + 1) {
+                    unaryConsumer.accept(arg);
                     continue;
                 }
-                value = args[i + 1];
+                PropertyMapper<?> mapper = PropertyMappers.getMapper(key);
-            } else if (keyValue.length == 2) {
+                // the weaknesses here:
+                // - needs to know all of the short name options that accept a value
+                // - does not know all of the picocli parsing rules. picocli will accept -cffile, and short option grouping - that's not accounted for
+                // - does not understand spi options, they will be assumed to be unary
+                if (mapper != null || SHORT_OPTIONS_ACCEPTING_VALUE.contains(key)) {
+                    i++; // consume next as a value to the key
+                    value = args.get(i);
+                } else {
+                    unaryConsumer.accept(arg);
+                    continue;
+                }
+            } else {
                 // the argument has a simple value. Eg.: key=pair
                 value = keyValue[1];
-            } else {
-                // to support cases like --db-url=jdbc:mariadb://localhost/kc?a=1
-                value = arg.substring(key.length() + 1);
             }
 
-            cliArgConsumer.accept(key, value);
+            valueArgConsumer.accept(key, value);
         }
     }
 }

quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/configuration/ConfigArgsConfigSourceTest.java

@@ -0,0 +1,37 @@
+/*
+ * Copyright 2024 Red Hat, Inc. and/or its affiliates
+ * and other contributors as indicated by the @author tags.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.keycloak.quarkus.runtime.configuration;
+
+import org.junit.Test;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+
+import static org.junit.Assert.assertEquals;
+
+public class ConfigArgsConfigSourceTest {
+
+    @Test
+    public void testParseArgs() {
+        List<String> values = new ArrayList<>();
+        ConfigArgsConfigSource.parseConfigArgs(Arrays.asList("--key=value", "-cf", "file", "command", "arg", "--db", "value"), (key, value) -> values.add(key+'='+value), values::add);
+        assertEquals(Arrays.asList("--key=value", "-cf=file", "command", "arg", "--db=value"), values);
+    }
+
+}

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

@@ -38,7 +38,7 @@ import static org.keycloak.quarkus.runtime.cli.command.AbstractStartCommand.OPTI
 public class StartAutoBuildDistTest {
 
     @Test
-    @Launch({ "start", "--http-enabled=true", "--hostname-strict=false" })
+    @Launch({ "--verbose", "start", "--http-enabled=true", "--hostname-strict=false" })
     @Order(1)
     void testStartAutoBuild(LaunchResult result) {
         CLIResult cliResult = (CLIResult) result;
@@ -47,7 +47,7 @@ public class StartAutoBuildDistTest {
         cliResult.assertMessage("Server configuration updated and persisted. Run the following command to review the configuration:");
         cliResult.assertMessage(KeycloakDistribution.SCRIPT_CMD + " show-config");
         cliResult.assertMessage("Next time you run the server, just run:");
-        cliResult.assertMessage(KeycloakDistribution.SCRIPT_CMD + " start " + OPTIMIZED_BUILD_OPTION_LONG + " --http-enabled=true --hostname-strict=false");
+        cliResult.assertMessage(KeycloakDistribution.SCRIPT_CMD + " --verbose start --http-enabled=true --hostname-strict=false " + OPTIMIZED_BUILD_OPTION_LONG);
         assertFalse(cliResult.getOutput().contains("--cache"));
         cliResult.assertStarted();
     }

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

@@ -96,7 +96,7 @@ public class StartCommandDistTest {
         cliResult.assertMessage("Server configuration updated and persisted. Run the following command to review the configuration:");
         cliResult.assertMessage(KeycloakDistribution.SCRIPT_CMD + " show-config");
         cliResult.assertMessage("Next time you run the server, just run:");
-        cliResult.assertMessage(KeycloakDistribution.SCRIPT_CMD + " start " + OPTIMIZED_BUILD_OPTION_LONG + " --http-enabled=true --hostname-strict=false");
+        cliResult.assertMessage(KeycloakDistribution.SCRIPT_CMD + " start --http-enabled=true --hostname-strict=false " + OPTIMIZED_BUILD_OPTION_LONG);
         assertFalse(cliResult.getOutput().contains("--metrics-enabled"));
         cliResult.assertStarted();
     }