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 d0a4b3247da..208a965caec 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 @@ -268,14 +268,13 @@ public class Picocli { // first validate the advertised property names // - this allows for efficient resolution of wildcard values and checking spi options Configuration.getPropertyNames().forEach(name -> { - if (!name.startsWith(MicroProfileConfigProvider.NS_KEYCLOAK_PREFIX)) { - return; // there are canonical mappings to kc. values - no need to consider alternative forms - } - if (!options.includeRuntime) { - checkRuntimeSpiOptions(name, ignoredRunTime); - } - if (PropertyMappers.isMaybeSpiBuildTimeProperty(name)) { - ambiguousSpi.add(name); + if (name.startsWith(PropertyMappers.KC_SPI_PREFIX)) { + if (!options.includeRuntime) { + checkRuntimeSpiOptions(name, ignoredRunTime); + } + if (PropertyMappers.isMaybeSpiBuildTimeProperty(name)) { + ambiguousSpi.add(name); + } } PropertyMapper mapper = PropertyMappers.getMapper(name); if (mapper == null) { @@ -288,8 +287,10 @@ public class Picocli { secondClassOptions.put(name, forKey.getFrom()); } } - if (!mapper.hasWildcard()) { - return; // non-wildcard options will be validated in the next pass + if (!mapper.hasWildcard() // non-wildcard options will be validated in the next pass + // validate only canonical mappings to kc. values - no need to consider alternative forms + || !name.startsWith(MicroProfileConfigProvider.NS_KEYCLOAK_PREFIX)) { + return; } validateProperty(abstractCommand, options, ignoredRunTime, disabledBuildTime, disabledRunTime, deprecatedInUse, missingOption, disabledMappers.contains(mapper), forKey, unnecessary); @@ -335,7 +336,11 @@ public class Picocli { warn("The following SPI options are using the legacy format and are not being treated as build time options. Please use the new format with the appropriate -- separators to resolve this ambiguity: " + String.join("\n", ambiguousSpi)); } secondClassOptions.forEach((key, firstClass) -> { - warn("Please use the first-class option `%s` instead of `%s`".formatted(firstClass, key)); + if (Configuration.getConfigValue(firstClass).getConfigSourceName() != null) { + warn("With the first-class option `%s` set, you should remove the usage of `%s`".formatted(firstClass, key)); + } else { + warn("Please use the first-class option `%s` instead of `%s`".formatted(firstClass, key)); + } }); if (!unnecessary.isEmpty()) { info("The following options were specified, but are typically not relevant for this command: " + String.join("\n", unnecessary)); @@ -444,9 +449,6 @@ public class Picocli { } private static void checkRuntimeSpiOptions(String key, final List ignoredRunTime) { - if (!key.startsWith(PropertyMappers.KC_SPI_PREFIX)) { - return; - } boolean buildTimeOption = PropertyMappers.isSpiBuildTimeProperty(key); if (!buildTimeOption) { 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 ca7e602ed58..d716c806910 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 @@ -22,6 +22,7 @@ import org.keycloak.quarkus.runtime.Messages; import org.keycloak.quarkus.runtime.configuration.Configuration; import org.keycloak.quarkus.runtime.configuration.IgnoredArtifacts; import org.keycloak.quarkus.runtime.configuration.PersistedConfigSource; +import org.keycloak.quarkus.runtime.configuration.mappers.PropertyMapper; import io.quarkus.bootstrap.runner.RunnerClassLoader; import io.quarkus.runtime.LaunchMode; @@ -104,7 +105,7 @@ public final class Build extends AbstractCommand { if (Start.NAME.equals(cmd) || Build.NAME.equals(cmd)) { executionError(spec.commandLine(), Messages.devProfileNotAllowedError(cmd)); } - } else if (Configuration.getConfigValue(DB).getConfigSourceOrdinal() == 0) { + } else if (Configuration.getConfigValue(DB).getConfigSourceOrdinal() == PropertyMapper.DEFAULT_VALUE_ORDINAL) { picocli.warn("Usage of the default value for the db option in the production profile is deprecated. Please explicitly set the db instead."); } } diff --git a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/mappers/PropertyMapper.java b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/mappers/PropertyMapper.java index 54b95d3fe57..1b51ad8ac34 100644 --- a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/mappers/PropertyMapper.java +++ b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/mappers/PropertyMapper.java @@ -60,6 +60,7 @@ import static org.keycloak.quarkus.runtime.configuration.MicroProfileConfigProvi public class PropertyMapper { + public static final int DEFAULT_VALUE_ORDINAL = -1; protected final Option option; private final String to; private Function enabled; @@ -116,12 +117,18 @@ public class PropertyMapper { *

* In preference order we are looking for: *

-     *  [ {@link #from} ] ---> [ {@link #mapFrom} ] ---> [ {@link #getDefaultValue()} ] ---> [ {@link #to} ]
-     * (explicit)     (derived)           (fallback)         (fallback)
+     *  [ {@link #from} ] ---> [ {@link #mapFrom} ] ---> [ {@link #getDefaultValue()} ]
+     * (explicit)      (derived)           (fallback)
      * 
*

* - * 2. Transform found value + * 2. Use to + *

+ * If we are looking for the `to` value, and no value was found from step 1 or it is effectively a default value, + * then use the `to` value if it was set by the user. + *

+ * + * 3. Transform found value *

* If we found a value for the attribute name, it needs to be transformed via {@link #transformValue} method. How to transform it? *

    @@ -129,6 +136,10 @@ public class PropertyMapper { *
  • If the value contains an expression, expand it using SmallRye logic *
  • Finally the returned {@link ConfigValue} is made to match what was requested - with the name, value, rawValue, and ordinal set appropriately. *
+ * + * 4. Use to + *

+ * If no value is found or mapped, return the `to` value. */ ConfigValue getConfigValue(String name, ConfigSourceInterceptorContext context) { String from = getFrom(); @@ -137,6 +148,7 @@ public class PropertyMapper { // we don't want the NestedPropertyMappingInterceptor to restart the chain here, so we force a proceed // this ensures that mapFrom transformers, and regular transformers are applied exclusively - not chained ConfigValue config = convertValue(NestedPropertyMappingInterceptor.proceed(context, from)); + Optional directValue = null; boolean parentValue = false; if (mapFrom != null && (config == null || config.getValue() == null)) { @@ -147,12 +159,21 @@ public class PropertyMapper { parentValue = true; } + // instead of using a default or mapping from a default, check if the value is already set and use that instead + // a warning should be emitted about this in picocli + if (!name.equals(from) && (config == null || config.getValue() == null || (parentValue && config.getConfigSourceOrdinal() == DEFAULT_VALUE_ORDINAL))) { + directValue = Optional.ofNullable(context.proceed(name)); + if (directValue.filter(c -> c.getValue() != null && Configuration.isUserModifiable(c)).isPresent()) { + return directValue.get(); + } + } + if (config != null && config.getValue() != null) { config = transformValue(name, config, context, parentValue); } else { String defaultValue = this.option.getDefaultValue().map(Option::getDefaultValueString).orElse(null); config = transformValue(name, new ConfigValueBuilder().withName(name) - .withValue(defaultValue).withRawValue(defaultValue).build(), + .withValue(defaultValue).withRawValue(defaultValue).withConfigSourceOrdinal(DEFAULT_VALUE_ORDINAL).build(), context, false); } @@ -161,6 +182,9 @@ public class PropertyMapper { } // now try any defaults from quarkus + if (directValue != null) { + return directValue.orElse(null); + } return context.proceed(name); } diff --git a/quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/cli/PicocliTest.java b/quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/cli/PicocliTest.java index e966a4974f1..ec84f71fc3b 100644 --- a/quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/cli/PicocliTest.java +++ b/quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/cli/PicocliTest.java @@ -756,7 +756,26 @@ public class PicocliTest extends AbstractConfigurationTest { public void derivedPropertyUsage() { NonRunningPicocli nonRunningPicocli = pseudoLaunch("start-dev", "--hostname=localhost", "--spi-hostname-v2-hostname=second-class"); assertEquals(CommandLine.ExitCode.OK, nonRunningPicocli.exitCode); - assertThat(nonRunningPicocli.getOutString(), containsString("Please use the first-class option `kc.hostname` instead of `kc.spi-hostname-v2-hostname`")); + assertThat(nonRunningPicocli.getOutString(), containsString("With the first-class option `kc.hostname` set, you should remove the usage of `kc.spi-hostname-v2-hostname`")); + } + + @Test + public void quarkusPropertyInQuarkusPropertiesWarning() { + putEnvVar("QUARKUS_HTTP_PORT", "9090"); + NonRunningPicocli nonRunningPicocli = pseudoLaunch("start-dev"); + assertEquals(CommandLine.ExitCode.OK, nonRunningPicocli.exitCode); + assertThat(nonRunningPicocli.getOutString(), + containsString("Please use the first-class option `kc.http-port` instead of `quarkus.http.port`")); + } + + @Test + public void quarkusPropertyInQuarkusPropertiesWarningWhenKcOptionSet() { + putEnvVar("QUARKUS_HTTP_PORT", "9090"); + // When kc.http-port is explicitly set, should still warn about quarkus.http.port + NonRunningPicocli nonRunningPicocli = pseudoLaunch("start-dev", "--http-port=7070"); + assertEquals(CommandLine.ExitCode.OK, nonRunningPicocli.exitCode); + assertThat(nonRunningPicocli.getOutString(), + containsString("With the first-class option `kc.http-port` set, you should remove the usage of `quarkus.http.port`")); } @Test diff --git a/quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/configuration/ConfigurationTest.java b/quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/configuration/ConfigurationTest.java index c27464e1bb2..6c04f9b2d3b 100644 --- a/quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/configuration/ConfigurationTest.java +++ b/quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/configuration/ConfigurationTest.java @@ -664,6 +664,25 @@ public class ConfigurationTest extends AbstractConfigurationTest { assertEquals("200k", config.getConfigValue("quarkus.http.limits.max-header-size").getValue()); } + @Test + public void testQuarkusPropertyTakesPrecedenceOverDefault() { + putEnvVar("QUARKUS_HTTP_PORT", "9090"); + ConfigArgsConfigSource.setCliArgs(""); + SmallRyeConfig config = createConfig(); + assertEquals("9090", config.getConfigValue("quarkus.http.port").getValue()); + // the kc.http-port still returns the default since no kc.* option was explicitly set + assertEquals("8080", config.getConfigValue("kc.http-port").getValue()); + } + + @Test + public void testKcPropertyTakesPrecedenceOverQuarkusProperty() { + // Explicitly setting kc.http-port should override the quarkus.properties value (9090) + ConfigArgsConfigSource.setCliArgs("--http-port=7070"); + SmallRyeConfig config = createConfig(); + assertEquals("7070", config.getConfigValue("quarkus.http.port").getValue()); + assertEquals("7070", config.getConfigValue("kc.http-port").getValue()); + } + private static Config.Scope cacheEmbeddedConfiguration() { return initConfig(CacheEmbeddedConfigProviderSpi.SPI_NAME, DefaultCacheEmbeddedConfigProviderFactory.PROVIDER_ID); } diff --git a/quarkus/runtime/src/test/resources/conf/quarkus.properties b/quarkus/runtime/src/test/resources/conf/quarkus.properties index 7baeeca7e37..e193987f676 100644 --- a/quarkus/runtime/src/test/resources/conf/quarkus.properties +++ b/quarkus/runtime/src/test/resources/conf/quarkus.properties @@ -2,9 +2,6 @@ # Logging configuration. INFO is the default level for most of the categories quarkus.log.min-level=TRACE -quarkus.log.level = INFO -quarkus.log.category."org.jboss.resteasy.resteasy_jaxrs.i18n".level=WARN -quarkus.log.category."org.infinispan.transaction.lookup.JBossStandaloneJTAManagerLookup".level=WARN # For test nested properties quarkus.datasource.foo = jdbc:h2:file:${kc.home.dir:${kc.db.url.path:~}}/data/keycloakdb