From 1d38fa88cd6c742cfe415175759be97936216fbc Mon Sep 17 00:00:00 2001 From: Steven Hawkins Date: Mon, 21 Oct 2024 12:15:33 -0400 Subject: [PATCH] fix: remove erroneous spi warnings (#33648) closes: #34057 Signed-off-by: Steve Hawkins --- .../quarkus/runtime/KeycloakMain.java | 3 +- .../keycloak/quarkus/runtime/cli/Picocli.java | 58 +++++++++++---- .../runtime/cli/command/AbstractCommand.java | 7 +- .../cli/command/AbstractStartCommand.java | 13 +--- .../quarkus/runtime/cli/command/Build.java | 4 +- .../quarkus/runtime/cli/PicocliTest.java | 71 ++++++++++++------- 6 files changed, 103 insertions(+), 53 deletions(-) diff --git a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/KeycloakMain.java b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/KeycloakMain.java index 987749a7ca..f1263646e6 100644 --- a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/KeycloakMain.java +++ b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/KeycloakMain.java @@ -97,7 +97,8 @@ public class KeycloakMain implements QuarkusApplication { try { PropertyMappers.sanitizeDisabledMappers(); - Picocli.validateConfig(cliArgs, new Start()); + PrintWriter outStream = new PrintWriter(System.out, true); + Picocli.validateConfig(cliArgs, new Start(), outStream); } catch (PropertyException | ProfileException e) { handleUsageError(e.getMessage(), e.getCause()); return; 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 88cec42ace..7e6fb6be01 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 @@ -58,7 +58,6 @@ import java.util.regex.Pattern; import java.util.stream.Collectors; import org.eclipse.microprofile.config.spi.ConfigSource; -import org.jboss.logging.Logger; import org.keycloak.common.profile.ProfileException; import org.keycloak.config.DeprecatedMetadata; import org.keycloak.config.Option; @@ -81,7 +80,9 @@ import org.keycloak.quarkus.runtime.configuration.QuarkusPropertiesConfigSource; import org.keycloak.quarkus.runtime.configuration.mappers.PropertyMappers; import org.keycloak.quarkus.runtime.configuration.mappers.PropertyMapper; import org.keycloak.quarkus.runtime.Environment; +import org.keycloak.quarkus.runtime.KeycloakMain; +import io.quarkus.bootstrap.runner.QuarkusEntryPoint; import io.smallrye.config.ConfigValue; import picocli.CommandLine; @@ -89,6 +90,9 @@ import picocli.CommandLine.ParameterException; import picocli.CommandLine.ParseResult; import picocli.CommandLine.DuplicateOptionAnnotationsException; import picocli.CommandLine.Help.Ansi; +import picocli.CommandLine.Help.Ansi.Style; +import picocli.CommandLine.Help.ColorScheme; +import picocli.CommandLine.IFactory; import picocli.CommandLine.Model.CommandSpec; import picocli.CommandLine.Model.ISetter; import picocli.CommandLine.Model.OptionSpec; @@ -349,8 +353,9 @@ public class Picocli { * * @param cliArgs * @param abstractCommand + * @param outWriter */ - public static void validateConfig(List cliArgs, AbstractCommand abstractCommand) { + public static void validateConfig(List cliArgs, AbstractCommand abstractCommand, PrintWriter outWriter) { IncludeOptions options = getIncludeOptions(cliArgs, abstractCommand, abstractCommand.getName()); if (!options.includeBuildTime && !options.includeRuntime) { @@ -427,24 +432,22 @@ public class Picocli { } } - Logger logger = Logger.getLogger(Picocli.class); // logger can't be instantiated in a class field - if (!ignoredBuildTime.isEmpty()) { throw new PropertyException(format("The following build time options have values that differ from what is persisted - the new values will NOT be used until another build is run: %s\n", String.join(", ", ignoredBuildTime))); } else if (!ignoredRunTime.isEmpty()) { - logger.warn(format("The following run time options were found, but will be ignored during build time: %s\n", - String.join(", ", ignoredRunTime))); + warn(format("The following run time options were found, but will be ignored during build time: %s\n", + String.join(", ", ignoredRunTime)), outWriter); } if (!disabledBuildTime.isEmpty()) { - outputDisabledProperties(disabledBuildTime, true, logger); + outputDisabledProperties(disabledBuildTime, true, outWriter); } else if (!disabledRunTime.isEmpty()) { - outputDisabledProperties(disabledRunTime, false, logger); + outputDisabledProperties(disabledRunTime, false, outWriter); } if (!deprecatedInUse.isEmpty()) { - logger.warn("The following used options or option values are DEPRECATED and will be removed or their behaviour changed in a future release:\n" + String.join("\n", deprecatedInUse) + "\nConsult the Release Notes for details."); + warn("The following used options or option values are DEPRECATED and will be removed or their behaviour changed in a future release:\n" + String.join("\n", deprecatedInUse) + "\nConsult the Release Notes for details.", outWriter); } } finally { DisabledMappersInterceptor.enable(disabledMappersInterceptorEnabled); @@ -543,11 +546,16 @@ public class Picocli { } disabledInUse.add(sb.toString()); } + + private static void warn(String text, PrintWriter outwriter) { + ColorScheme defaultColorScheme = picocli.CommandLine.Help.defaultColorScheme(Help.Ansi.AUTO); + outwriter.println(defaultColorScheme.apply("WARNING: ", Arrays.asList(Style.fg_yellow, Style.bold)) + text); + } - private static void outputDisabledProperties(Set properties, boolean build, Logger logger) { - logger.warn(format("The following used %s time options are UNAVAILABLE and will be ignored during %s time:\n %s", + private static void outputDisabledProperties(Set properties, boolean build, PrintWriter outWriter) { + warn(format("The following used %s time options are UNAVAILABLE and will be ignored during %s time:\n %s", build ? "build" : "run", build ? "run" : "build", - String.join("\n", properties))); + String.join("\n", properties)), outWriter); } private static boolean hasConfigChanges(CommandLine cmdCommand) { @@ -671,7 +679,16 @@ public class Picocli { } public CommandLine createCommandLine(Consumer consumer) { - CommandSpec spec = CommandSpec.forAnnotatedObject(new Main()).name(Environment.getCommand()); + CommandSpec spec = CommandSpec.forAnnotatedObject(new Main(), new IFactory() { + @Override + public K create(Class cls) throws Exception { + K result = CommandLine.defaultFactory().create(cls); + if (result instanceof AbstractCommand ac) { + ac.setPicocli(Picocli.this); + } + return result; + } + }).name(Environment.getCommand()); consumer.accept(spec); CommandLine cmd = new CommandLine(spec); @@ -681,13 +698,17 @@ public class Picocli { cmd.setHelpFactory(new HelpFactory()); cmd.getHelpSectionMap().put(SECTION_KEY_COMMAND_LIST, new SubCommandListRenderer()); cmd.setErr(getErrWriter()); - + cmd.setOut(getOutWriter()); return cmd; } protected PrintWriter getErrWriter() { return new PrintWriter(System.err, true); } + + protected PrintWriter getOutWriter() { + return new PrintWriter(System.out, true); + } private static void addHelp(CommandSpec currentSpec) { try { @@ -961,4 +982,13 @@ public class Picocli { } } } + + public void start(CommandLine cmd) { + KeycloakMain.start((ExecutionExceptionHandler) cmd.getExecutionExceptionHandler(), cmd.getErr(), cmd.getParseResult().originalArgs().toArray(new String[0])); + } + + public void build() throws Throwable { + QuarkusEntryPoint.main(); + } + } diff --git a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/command/AbstractCommand.java b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/command/AbstractCommand.java index c93e99a932..6daa65a65e 100644 --- a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/command/AbstractCommand.java +++ b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/command/AbstractCommand.java @@ -34,6 +34,7 @@ public abstract class AbstractCommand { @Spec protected CommandSpec spec; + protected Picocli picocli; protected void executionError(CommandLine cmd, String message) { executionError(cmd, message, null); @@ -65,7 +66,7 @@ public abstract class AbstractCommand { } protected void validateConfig() { - Picocli.validateConfig(ConfigArgsConfigSource.getAllCliArgs(), this); + Picocli.validateConfig(ConfigArgsConfigSource.getAllCliArgs(), this, spec.commandLine().getOut()); } public abstract String getName(); @@ -73,5 +74,9 @@ public abstract class AbstractCommand { public CommandLine getCommandLine() { return spec.commandLine(); } + + public void setPicocli(Picocli picocli) { + this.picocli = picocli; + } } diff --git a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/command/AbstractStartCommand.java b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/command/AbstractStartCommand.java index f50e3aa791..ae3051190f 100644 --- a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/command/AbstractStartCommand.java +++ b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/command/AbstractStartCommand.java @@ -25,7 +25,6 @@ import org.keycloak.quarkus.runtime.cli.ExecutionExceptionHandler; import org.keycloak.quarkus.runtime.configuration.ConfigArgsConfigSource; import org.keycloak.quarkus.runtime.configuration.mappers.HostnameV2PropertyMappers; import org.keycloak.quarkus.runtime.configuration.mappers.HttpPropertyMappers; -import org.keycloak.url.HostnameV2ProviderFactory; import java.util.EnumSet; import java.util.List; @@ -38,8 +37,6 @@ import static org.keycloak.quarkus.runtime.configuration.Configuration.getRawPer public abstract class AbstractStartCommand extends AbstractCommand implements Runnable { public static final String OPTIMIZED_BUILD_OPTION_LONG = "--optimized"; - private boolean skipStart; - @Override public void run() { Environment.setParsedCommand(this); @@ -52,10 +49,8 @@ public abstract class AbstractStartCommand extends AbstractCommand implements Ru if (ConfigArgsConfigSource.getAllCliArgs().contains(OPTIMIZED_BUILD_OPTION_LONG) && !wasBuildEverRun()) { executionError(spec.commandLine(), Messages.optimizedUsedForFirstStartup()); } - - if (!skipStart) { - KeycloakMain.start((ExecutionExceptionHandler) cmd.getExecutionExceptionHandler(), cmd.getErr(), cmd.getParseResult().originalArgs().toArray(new String[0])); - } + + picocli.start(cmd); } protected void doBeforeRun() { @@ -76,8 +71,4 @@ public abstract class AbstractStartCommand extends AbstractCommand implements Ru return EnumSet.of(OptionCategory.IMPORT, OptionCategory.EXPORT); } - public void setSkipStart(boolean skipStart) { - this.skipStart = skipStart; - } - } diff --git a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/command/Build.java b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/command/Build.java index fc5bb63b34..4fd368432e 100644 --- a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/command/Build.java +++ b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/command/Build.java @@ -79,7 +79,7 @@ public final class Build extends AbstractCommand implements Runnable { configureBuildClassLoader(); beforeReaugmentationOnWindows(); - QuarkusEntryPoint.main(); + picocli.build(); if (!isDevProfile()) { println(spec.commandLine(), "Server configuration updated and persisted. Run the following command to review the configuration:\n"); @@ -133,7 +133,7 @@ public final class Build extends AbstractCommand implements Runnable { private void cleanTempResources() { if (!LaunchMode.current().isDevOrTest()) { // only needed for dev/testing purposes - getHomePath().resolve("quarkus-artifact.properties").toFile().delete(); + Optional.ofNullable(getHomePath()).ifPresent(path -> path.resolve("quarkus-artifact.properties").toFile().delete()); } } diff --git a/quarkus/runtime/src/test/java/or/keycloak/quarkus/runtime/cli/PicocliTest.java b/quarkus/runtime/src/test/java/or/keycloak/quarkus/runtime/cli/PicocliTest.java index 6a6546be7d..5897ef8404 100644 --- a/quarkus/runtime/src/test/java/or/keycloak/quarkus/runtime/cli/PicocliTest.java +++ b/quarkus/runtime/src/test/java/or/keycloak/quarkus/runtime/cli/PicocliTest.java @@ -19,17 +19,16 @@ package or.keycloak.quarkus.runtime.cli; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.not; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNull; import java.io.PrintWriter; import java.io.StringWriter; -import java.util.Arrays; import java.util.List; import org.junit.Test; import org.keycloak.quarkus.runtime.cli.Picocli; -import org.keycloak.quarkus.runtime.cli.command.AbstractStartCommand; import org.keycloak.quarkus.runtime.configuration.ConfigArgsConfigSource; import org.keycloak.quarkus.runtime.configuration.test.AbstractConfigurationTest; @@ -43,20 +42,34 @@ public class PicocliTest extends AbstractConfigurationTest { private class NonRunningPicocli extends Picocli { final StringWriter err = new StringWriter(); + final StringWriter out = new StringWriter(); SmallRyeConfig config; int exitCode = Integer.MAX_VALUE; String getErrString() { - // normalize line endings - TODO: could also normalize non-printable chars - // but for now those are part of the expected output - return System.lineSeparator().equals("\n") ? err.toString() - : err.toString().replace(System.lineSeparator(), "\n"); + return normalize(err); + } + + // normalize line endings - TODO: could also normalize non-printable chars + // but for now those are part of the expected output + String normalize(StringWriter writer) { + return System.lineSeparator().equals("\n") ? writer.toString() + : writer.toString().replace(System.lineSeparator(), "\n"); + } + + String getOutString() { + return normalize(out); } @Override protected PrintWriter getErrWriter() { return new PrintWriter(err, true); } + + @Override + protected PrintWriter getOutWriter() { + return new PrintWriter(out, true); + } @Override protected void exitOnFailure(int exitCode, CommandLine cmd) { @@ -67,33 +80,29 @@ public class PicocliTest extends AbstractConfigurationTest { throw new AssertionError("Should not reaugment"); }; - @Override - protected int run(CommandLine cmd, String[] argArray) { - skipStart(cmd); - return super.run(cmd, argArray); - } - - private void skipStart(CommandLine cmd) { - for (CommandLine sub : cmd.getSubcommands().values()) { - if (sub.getCommand() instanceof AbstractStartCommand) { - ((AbstractStartCommand) (sub.getCommand())).setSkipStart(true); - } - skipStart(sub); - } - } - @Override public void parseAndRun(List cliArgs) { - ConfigArgsConfigSource.setCliArgs(cliArgs.toArray(String[]::new)); config = createConfig(); super.parseAndRun(cliArgs); } + + @Override + public void start(CommandLine cmd) { + // skip + } + + @Override + public void build() throws Throwable { + // skip + } }; NonRunningPicocli pseudoLaunch(String... args) { NonRunningPicocli nonRunningPicocli = new NonRunningPicocli(); - nonRunningPicocli.parseAndRun(Arrays.asList(args)); + ConfigArgsConfigSource.setCliArgs(args); + var cliArgs = Picocli.parseArgs(args); + nonRunningPicocli.parseAndRun(cliArgs); return nonRunningPicocli; } @@ -205,5 +214,19 @@ public class PicocliTest extends AbstractConfigurationTest { assertThat(nonRunningPicocli.getErrString(), containsString( "Option: '--db postgres' is not expected to contain whitespace, please remove any unnecessary quoting/escaping")); } - + + @Test + public void spiRuntimeAllowedWithStart() { + NonRunningPicocli nonRunningPicocli = pseudoLaunch("start", "--http-enabled=true", "--spi-something-pass=changeme"); + assertEquals(CommandLine.ExitCode.OK, nonRunningPicocli.exitCode); + assertThat(nonRunningPicocli.getOutString(), not(containsString("kc.spi-something-pass"))); + } + + @Test + public void spiRuntimeWarnWithBuild() { + NonRunningPicocli nonRunningPicocli = pseudoLaunch("build", "--spi-something-pass=changeme"); + assertEquals(CommandLine.ExitCode.OK, nonRunningPicocli.exitCode); + assertThat(nonRunningPicocli.getOutString(), containsString("The following run time options were found, but will be ignored during build time: kc.spi-something-pass")); + } + }