Repository navigation
fix: introduces the concept of a synthetic option - #47399
Conversation
|
@shawkins It is not clear to me when to use the |
It's more about that synthetic option does not existing at all - we're only using the satisfy the design of needing a from Option for a PropertyMapper.
Hidden already has a meaning (it exists as an option, but is not shown in help), and in some cases may be something that we actually want to allow a user to specify - for example customizing the db dialect has come up a couple of times even though the option is hidden.
It's mostly about the design, and safety checks, around the PropertyMapper class. We for example make sure that after sanitizing that no Option is duplicated. |
There was a problem hiding this comment.
I have mostly questions on how things work. But I got this log message when running the build command
task: [copy-datset] ./bin/kc.sh build --db=postgres --metrics-enabled=true --event-metrics-user-enabled=true --health-enabled=true --tracing-enabled=false
[copy-datset] INFO: The following run time options were found, but will be ignored during build time: kc.db, kc.db, kc.db, kc.db, kc.db, kc.db
[copy-datset]
[copy-datset] Updating the configuration and installing your custom providers, if any. Please wait.
[copy-datset] 2026-03-25 10:55:50,067 INFO [io.quarkus.deployment.QuarkusAugmentor] (main) Quarkus augmentation completed in 4341ms
[copy-datset] Server configuration updated and persisted. Run the following command to review the configuration:
[copy-datset]
[copy-datset] kc.sh show-config
[copy-datset]
This log entry, The following run time options were found, but will be ignored during build time: kc.db, kc.db, kc.db, kc.db, kc.db, kc.db seems weird to me.
| .transformer((name, value, context) -> transformTlsUserProperty(vendorValues, name, value, jdbcPropertyKey)) | ||
| private static PropertyMapper<?> setInputTlsJdbcProperty(Option<?> from, String jdbcPropertyKey, Collection<Database.Vendor> vendorValues) { | ||
| return fromOption(from) 8000 | ||
| .mapFrom(from, (name, value, context) -> transformTlsUserProperty(vendorValues, name, value, jdbcPropertyKey)) |
There was a problem hiding this comment.
Can we replace mapFrom with transformer here? I guess the answer is no except if you create a new fromOption to convert the option to synthetic.
There was a problem hiding this comment.
Can we replace mapFrom with transformer here? I guess the answer is no except if you create a new fromOption to convert the option to synthetic.
mapFrom and a transformer are a little different.
- a transformer gets the from value by proceeding past the property mapping interceptor - it's looking for a "real" value
- mapFrom restarts the interceptor chain to look for the parent value - so it can be a derived value
| private static PropertyMapper<?> setTlsJdbcProperty(Option<String> option, String jdbcPropertyKey, Map<Database.Vendor, String> vendorValues) { | ||
| return fromOption(option) | ||
| private static PropertyMapper<?> setTlsJdbcProperty(String jdbcPropertyKey, Map<Database.Vendor, String> vendorValues) { | ||
| return fromOption(SYNTHETIC_RUNTIME_DB_OPTION) |
There was a problem hiding this comment.
Why do we need SYNTHETIC_RUNTIME_DB_OPTION if you convert to synthetic when from == mapFrom?
There was a problem hiding this comment.
Because the option carries the other metadata like whether it's a build time option - DB is a build time option, but we wan the tls options to be runtime.
There was a problem hiding this comment.
What would happen if we use DB?
Would the values of quarkus.datasource.jdbc.additional-jdbc-properties.* be persisted during build time? I'll try to find out :)
There was a problem hiding this comment.
It doesn't seem to cause any issues in my test, as db-tls-mode can still be changed at runtime.
There was a problem hiding this comment.
I believe that was the cause of the mssql test failures in the CI initially.
Thank you for the explanation. |
Good catch, I'll address that. |
My main goal here was to eliminate / consolidate the ones that have been newly added. After this change, it would be good to review any of the existing hidden option to see if we'd want to remove or make them synthetic instead. |
Sounds good. On a related topic, would it make sense for the |
I touched on this in the pr description. Having it on the Option was more logical to differentiate things like the Option created for representing a feature. |
Agree to disagree 😄 |
| @@ -85,11 +84,11 @@ public List<PropertyMapper<?>> getPropertyMappers() { | |||
| .to("quarkus.log.console.json.log-format") | |||
| .paramLabel("format") | |||
| .build(), | |||
| fromParentOption(LoggingOptions.LOG_SERVICE_NAME) | |||
| fromOption(LoggingOptions.LOG_SERVICE_NAME).mapFrom(LoggingOptions.LOG_SERVICE_NAME) | |||
There was a problem hiding this comment.
Hmm, wouldn't it be better to keep the fromParentOption notion? It reads a little bit weird to set fromOption and mapFrom to the same option. Can we do it underhood?
There are some other teams that introduce new options, so it'd be good to have it simplistic - and I think this would help to simplify it.
@shawkins WDYT?
There was a problem hiding this comment.
I tried to explain this in the description:
Because there are several mapFrom signatures, I didn't add additional convienence methods.
Another way to go is to not require an option for the propertymapper builder - but in several places in the builder logic we assume that it is there.
Or as @pruivo mentioned we could change the meaning of a transformer for synthetic options - since I already modified the getConfigValue logic for synthetics, this is probably the way to go if we don't want to repeat the option. edit: to clarify to do this correctly we would need to keep the synthetic flag on an Option (to account for things like the feature option) then introduce another flag (possibly also called synthetic) on the PropertyMapper to override the handling when dealing with a "real option" that has multiple mappings. However unless we allow the propertymapper to override whether we're dealing with a runtime vs buildtime option, we still have to create a new Option to represent the database mappings.
| fromOption(DatabaseOptions.DB_CONNECT_TIMEOUT) | ||
| .to(CONNECT_TIMEOUT) | ||
| .mapFrom(DatabaseOptions.DB_CONNECT_TIMEOUT, (name, value, context) | ||
| -> durationToMillis(value)) | ||
| .isEnabled(DatabasePropertyMappers::isMysqlConnectTimeoutEnabled) | ||
|
|
||
| fromOption(DatabaseOptions.DB_MARIADB_CONNECT_TIMEOUT) | ||
| fromOption(DatabaseOptions.DB_CONNECT_TIMEOUT) | ||
| .to(CONNECT_TIMEOUT) | ||
| .mapFrom(DatabaseOptions.DB_CONNECT_TIMEOUT, (name, value, context) | ||
| -> durationToMillis(value)) | ||
| .isEnabled(DatabasePropertyMappers::isMariadbConnectTimeoutEnabled) | ||
| .build(), | ||
| fromOption(DatabaseOptions.DB_ORACLE_CONNECT_TIMEOUT) | ||
| fromOption(DatabaseOptions.DB_CONNECT_TIMEOUT) |
There was a problem hiding this comment.
Yep, maybe it'd be good in the future to have multiple to()s, even with some isEnabled arg to simplify this, right?
There was a problem hiding this comment.
In the code shown above it's a simplier case as the to's are the same - in fact we could combine those currently if we just changed the isEnabled and mapFrom logic.
For the true multiple mapping scenario it can make sense, but is a pretty big undertaking in terms of building logic [1] and potentially to how the propertymappers are stored / accessed:
[1]
.to(option1)
.mapFrom(...)
.isEnabled(...)
...
.endTo()
.to(option2)
...
There was a problem hiding this comment.
Yes, it'd also bring multiple complexities. Can be considered into the future.
I assumed this will be put on hold as well. |
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.broker.KcOidcBrokerTest#loginWithExistingUserWithBruteForceEnabledKeycloak CI - Java Distribution IT (windows-latest - temurin - 21) |
|
Rebased and removed the hold label. |
| @@ -104,7 +91,7 @@ public List<PropertyMapper<?>> getPropertyMappers() { | |||
| .to(PG_TARGET_SERVER_TYPE) | |||
| .isEnabled(DatabasePropertyMappers::isPostgresqlTargetServerTypeEnabled) | |||
| .build(), | |||
| fromOption(DatabaseOptions.DB_MSSQL_SEND_STRING_PARAMETER_AS_UNICODE) | |||
| fromOption(SYNTHETIC_RUNTIME_DB_OPTION).mapFrom(DB, (name, value, context) -> "false") | |||
There was a problem hiding this comment.
This sets the sendStringParametersAsUnicode for every named datasource. I tried a test which passes on the current main branch but fails on your branch:
diff --git a/quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/configuration/DatasourcesConfigurationTest.java b/quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/configuration/DatasourcesConfigurationTest.java
index 158a9bccb5..f1a3171562 100644
--- a/quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/configuration/DatasourcesConfigurationTest.java
+++ b/quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/configuration/DatasourcesConfigurationTest.java
@@ -595,6 +595,11 @@ public class DatasourcesConfigurationTest extends AbstractConfigurationTest {
assertNullAdditionalJdbcProperty(config, "users", trustStorePasswordProperty);
assertNullAdditionalJdbcProperty(config, "users", trustStoreTypeProperty);
+ // MSSQL-specific property should not leak into non-MSSQL named datasources
+ if (!"mssql".equals(dbKind)) {
+ assertNullAdditionalJdbcProperty(config, "users", "sendStringParametersAsUnicode");
+ }
+
// oracle has a different protocol for TLS
if ("oracle".equals(dbKind)) {
dbUrl = dbUrl.replace("jdbc:oracle:thin:@//", "jdbc:oracle:thin:@tcps://");
run
mvn test -pl quarkus/runtime -Dtest="DatasourcesConfigurationTest#testPostgresTLSOptions" -Dmaven.build.cache.enabled=false
...
[ERROR] DatasourcesConfigurationTest.testPostgresTLSOptions:517->doDatabaseTlsOptionTest:600->AbstractConfigurationTest.assertNullAdditionalJdbcProperty:166 quarkus.datasource."users".jdbc.additional-jdbc-properties.sendStringParametersAsUnicode
Expected: is null
but: was "false"There was a problem hiding this comment.
Thank you for catching this. This is related to something that we talked about with #46603 - which is that isEnabled does not work with wildcard options.
I've added a commit that refines this.
We had also talked about a follow-up to remove this usage of isEnabled - I'll see how hard that is and add that to the pr if it's straight-forward.
closes: keycloak#47385 Signed-off-by: Steve Hawkins <shawkins@redhat.com> # Conflicts: # quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/mappers/DatabasePropertyMappers.java # quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/mappers/PropertyMapper.java # quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/configuration/ConfigurationTest.java
also removing the disabled mapper interceptor Signed-off-by: Steve Hawkins <shawkins@redhat.com>
Signed-off-by: Steve Hawkins <shawkins@redhat.com>
Signed-off-by: Steve Hawkins <shawkins@redhat.com>
* fix: introduces the concept of a synthetic option closes: keycloak#47385 Signed-off-by: Steve Hawkins <shawkins@redhat.com> # Conflicts: # quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/mappers/DatabasePropertyMappers.java # quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/mappers/PropertyMapper.java # quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/configuration/ConfigurationTest.java * correcting the db synthetic property also removing the disabled mapper interceptor Signed-off-by: Steve Hawkins <shawkins@redhat.com> * addressing review comments Signed-off-by: Steve Hawkins <shawkins@redhat.com> * moving datasource mappers using isEnabled out of wildcards Signed-off-by: Steve Hawkins <shawkins@redhat.com> --------- Signed-off-by: Steve Hawkins <shawkins@redhat.com>
closes: #47385
Introduces the concept of a synthetic option to replace the usage of hidden options.
I associated the flag with the Option for scenarios like fromFeature - but it could be associated with the PropertyMapper instead.
When a propertymapper uses a synthetic option it will not consult that as a from value. Previously the random properties used by logging, or the named option created by fromFeature, or the new database options was consulted - it was just assumed it wouldn't be used. It is also now excluded from the cli.
An option is synthetic if that is explicitly set (such as with fromFeature) or implicitly if PropertyMapper option and mapFrom are the same. Because there are several mapFrom signatures, I didn't add additional convienence methods.
Ideally something like DB_ORACLE_TLS_TRANSPORT shouldn't exist, but removing it would require tweaking the URL construction logic (for example like what was done for h2 where more of it moved into the mapper, rather than being handled in the Database class).