From 6378dcbac203788c150a212a4e40753fb3fe9015 Mon Sep 17 00:00:00 2001 From: Steven Hawkins Date: Wed, 24 Jul 2024 04:23:23 -0400 Subject: [PATCH] fix: additional consolidation / refinement of argument parsing (#31448) closes: #26339 Signed-off-by: Steve Hawkins --- .../keycloak/quarkus/runtime/cli/Picocli.java | 60 ++++++------------- .../configuration/ConfigArgsConfigSource.java | 3 +- .../ConfigArgsConfigSourceTest.java | 7 +++ .../it/junit5/extension/CLITestExtension.java | 27 +++------ 4 files changed, 36 insertions(+), 61 deletions(-) diff --git a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/Picocli.java b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/Picocli.java index b3e24747d8..e5fcdc5dd0 100644 --- a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/Picocli.java +++ b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/Picocli.java @@ -66,7 +66,6 @@ import org.keycloak.config.OptionCategory; import org.keycloak.quarkus.runtime.cli.command.AbstractCommand; import org.keycloak.quarkus.runtime.cli.command.BootstrapAdmin; import org.keycloak.quarkus.runtime.cli.command.Build; -import org.keycloak.quarkus.runtime.cli.command.ImportRealmMixin; import org.keycloak.quarkus.runtime.cli.command.Main; import org.keycloak.quarkus.runtime.cli.command.ShowConfig; import org.keycloak.quarkus.runtime.cli.command.Start; @@ -100,7 +99,6 @@ public final class Picocli { public static final String ARG_PREFIX = "--"; public static final String ARG_SHORT_PREFIX = "-"; public static final String NO_PARAM_LABEL = "none"; - private static final String ARG_KEY_VALUE_SEPARATOR = "="; private static class IncludeOptions { boolean includeRuntime; @@ -277,11 +275,9 @@ public final class Picocli { public void accept(String key, String value) { PropertyMapper mapper = PropertyMappers.getMapper(key); - if (mapper != null && mapper.isBuildTime()) { - return; + if (mapper == null || mapper.isRunTime()) { + properties.add(key + "=" + maskValue(key, value)); } - - properties.add(key + "=" + maskValue(key, value)); } }, arg -> { properties.add(arg); @@ -301,11 +297,9 @@ public final class Picocli { parseConfigArgs(cliArgs, (k, v) -> { PropertyMapper mapper = PropertyMappers.getMapper(k); - if (mapper == null || mapper.isRunTime()) { - return; + if (mapper != null && mapper.isBuildTime()) { + configArgsList.add(k + "=" + v); } - - configArgsList.add(k + "=" + v); }, ignored -> {}); int exitCode = cmd.execute(configArgsList.toArray(new String[0])); @@ -855,42 +849,26 @@ public final class Picocli { // makes sure cli args are available to the config source ConfigArgsConfigSource.setCliArgs(rawArgs); - List args = new ArrayList<>(List.of(rawArgs)); - Iterator iterator = args.iterator(); - while (iterator.hasNext()) { - String arg = iterator.next(); - - if (arg.startsWith("--spi") || arg.startsWith("-D")) { - // TODO: ignore properties for providers for now, need to fetch them from the providers, otherwise CLI will complain about invalid options - // also ignores system properties as they are set when starting the JVM - // change this once we are able to obtain properties from providers - iterator.remove(); - - if (!arg.contains(ARG_KEY_VALUE_SEPARATOR)) { - if (!iterator.hasNext()) { - if (arg.startsWith("--spi")) { - throw new PropertyException(format("spi argument %s requires a value.", arg)); - } - return args; - } - String next = iterator.next(); - - if (!next.startsWith("--")) { - // ignore the value if the arg is using space as separator - iterator.remove(); - } - } + // TODO: ignore properties for providers for now, need to fetch them from the providers, otherwise CLI will complain about invalid options + // also ignores system properties as they are set when starting the JVM + // change this once we are able to obtain properties from providers + List args = new ArrayList<>(); + ConfigArgsConfigSource.parseConfigArgs(List.of(rawArgs), (arg, value) -> { + if (!arg.startsWith("--spi") && !arg.startsWith("-D")) { + args.add(arg + "=" + value); } - } - + }, arg -> { + if (arg.startsWith("--spi")) { + throw new PropertyException(format("spi argument %s requires a value.", arg)); + } + if (!arg.startsWith("-D")) { + args.add(arg); + } + }); return args; } - private static boolean isRuntimeOption(String arg) { - return arg.startsWith(ImportRealmMixin.IMPORT_REALM); - } - private static void checkChangesInBuildOptionsDuringAutoBuild() { if (Configuration.isOptimized()) { List> buildOptions = stream(Configuration.getPropertyNames(true).spliterator(), false) diff --git a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/ConfigArgsConfigSource.java b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/ConfigArgsConfigSource.java index b37ff0f4a4..f48654d060 100644 --- a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/ConfigArgsConfigSource.java +++ b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/ConfigArgsConfigSource.java @@ -156,8 +156,7 @@ public class ConfigArgsConfigSource extends PropertiesConfigSource { // 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)) { + if (mapper != null || SHORT_OPTIONS_ACCEPTING_VALUE.contains(key) || arg.startsWith("--spi")) { i++; // consume next as a value to the key value = args.get(i); } else { diff --git a/quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/configuration/ConfigArgsConfigSourceTest.java b/quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/configuration/ConfigArgsConfigSourceTest.java index d5bc35b12f..e3b29040db 100644 --- a/quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/configuration/ConfigArgsConfigSourceTest.java +++ b/quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/configuration/ConfigArgsConfigSourceTest.java @@ -33,5 +33,12 @@ public class ConfigArgsConfigSourceTest { 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); } + + @Test + public void testParseArgsWithSpi() { + List values = new ArrayList<>(); + ConfigArgsConfigSource.parseConfigArgs(Arrays.asList("--spi-some-thing-enabled=value", "--spi-some-thing-else", "other-value"), (key, value) -> values.add(key+'='+value), ignored -> {}); + assertEquals(Arrays.asList("--spi-some-thing-enabled=value", "--spi-some-thing-else=other-value"), values); + } } diff --git a/quarkus/tests/junit5/src/main/java/org/keycloak/it/junit5/extension/CLITestExtension.java b/quarkus/tests/junit5/src/main/java/org/keycloak/it/junit5/extension/CLITestExtension.java index 6a9eb03d3d..4a181a1052 100644 --- a/quarkus/tests/junit5/src/main/java/org/keycloak/it/junit5/extension/CLITestExtension.java +++ b/quarkus/tests/junit5/src/main/java/org/keycloak/it/junit5/extension/CLITestExtension.java @@ -46,7 +46,6 @@ import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.List; -import java.util.regex.Pattern; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -60,7 +59,6 @@ import static org.keycloak.quarkus.runtime.cli.command.Main.CONFIG_FILE_SHORT_NA public class CLITestExtension extends QuarkusMainTestExtension { private static final String SYS_PROPS = "sys-props"; - private static final String KEY_VALUE_SEPARATOR = "[=]"; private KeycloakDistribution dist; private DatabaseContainer databaseContainer; private InfinispanContainer infinispanContainer; @@ -73,24 +71,17 @@ public class CLITestExtension extends QuarkusMainTestExtension { getStore(context).put(SYS_PROPS, new HashMap<>(System.getProperties())); if (launch != null && distConfig == null) { - for (String arg : launch.value()) { - if (arg.contains(CONFIG_FILE_SHORT_NAME) || arg.contains(CONFIG_FILE_LONG_NAME)) { - Pattern kvSeparator = Pattern.compile(KEY_VALUE_SEPARATOR); - String[] cfKeyValue = kvSeparator.split(arg); - setProperty(KeycloakPropertiesConfigSource.KEYCLOAK_CONFIG_FILE_PROP, cfKeyValue[1]); + ConfigArgsConfigSource.parseConfigArgs(List.of(launch.value()), (arg, value) -> { + if (arg.equals(CONFIG_FILE_SHORT_NAME) || arg.equals(CONFIG_FILE_LONG_NAME)) { + setProperty(KeycloakPropertiesConfigSource.KEYCLOAK_CONFIG_FILE_PROP, value); } else if (arg.startsWith("-D")) { - // allow setting system properties from JVM tests - int keyValueSeparator = arg.indexOf('='); - - if (keyValueSeparator == -1) { - continue; - } - - String name = arg.substring(2, keyValueSeparator); - String value = arg.substring(keyValueSeparator + 1); - setProperty(name, value); + setProperty(arg, value); } - } + }, arg -> { + if (arg.startsWith("-D")) { + setProperty(arg, ""); + } + }); } configureDatabase(context);