fix: use to values ahead of keycloak defaults

closes: #46728

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

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<String> ignoredRunTime) {
-        if (!key.startsWith(PropertyMappers.KC_SPI_PREFIX)) {
-            return;
-        }
         boolean buildTimeOption = PropertyMappers.isSpiBuildTimeProperty(key);
 
         if (!buildTimeOption) {

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.");
         }
     }

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<T> {
 
+    public static final int DEFAULT_VALUE_ORDINAL = -1;
     protected final Option<T> option;
     private final String to;
     private Function<AbstractCommand, Boolean> enabled;
@@ -116,12 +117,18 @@ public class PropertyMapper<T> {
      * <p>
      * In preference order we are looking for:
      * <pre>
-     *  [ {@link #from} ] ---> [ {@link #mapFrom} ] ---> [ {@link #getDefaultValue()} ] ---> [ {@link #to} ]
-     * (explicit)     (derived)           (fallback)         (fallback)
+     *  [ {@link #from} ] ---> [ {@link #mapFrom} ] ---> [ {@link #getDefaultValue()} ]
+     * (explicit)      (derived)           (fallback)
      * </pre>
      * <p>
      *
-     * <b>2. Transform found value</b>
+     * <b>2. Use to</b>
+     * <p>
+     * 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.
+     * <p>
+     *
+     * <b>3. Transform found value</b>
      * <p>
      * If we found a value for the attribute name, it needs to be transformed via {@link #transformValue} method. How to transform it?
      * <ul>
@@ -129,6 +136,10 @@ public class PropertyMapper<T> {
      *   <li>If the value contains an expression, expand it using SmallRye logic
      *   <li>Finally the returned {@link ConfigValue} is made to match what was requested - with the name, value, rawValue, and ordinal set appropriately.
      * </ul>
+     *
+     * <b>4. Use to</b>
+     * <p>
+     * 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<T> {
         // 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<ConfigValue> directValue = null;
 
         boolean parentValue = false;
         if (mapFrom != null && (config == null || config.getValue() == null)) {
@@ -147,12 +159,21 @@ public class PropertyMapper<T> {
             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<T> {
         }
 
         // now try any defaults from quarkus
+        if (directValue != null) {
+            return directValue.orElse(null);
+        }
         return context.proceed(name);
     }
 

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

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);
     }

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