fix: the transformed value should always be returned #24758
Conversation
shawkins
force-pushed
the
iss24757
branch
2 times, most recently
from
b8b0214
to
6be114c
Compare
| @@ -132,14 +132,13 @@ ConfigValue getConfigValue(String name, ConfigSourceInterceptorContext context) | |||
|
|
|||
| Optional<String> configValue = ofNullable(config.getValue()); | |||
|
|
|||
| if (config.getName().equals(name)) { | |||
There was a problem hiding this comment.
@vmuzikar this check is reponsible for the inconsistency. In the case that the config is explicitly set, then the non-transformed value will be returned. So in this case for kc.proxy
--proxy=none will return none
omitted it will return false
In removing / moving this check it highlights that this inconistency exists in other places - the isDevModeDatabase check, the DefaultHostnameProvider check of whether http is enabled (it worked before only because the default is false), and when calling the Config.getRawValue - that last one could be a SmallRye issue as raw in that context does not appear to actually refer to the raw config value - https://github.com/smallrye/smallrye-config/blob/5a96de2849c87d144586df07dd4529e28d7e3b85/implementation/src/main/java/io/smallrye/config/SmallRyeConfig.java#L370
There may be some other places as well - so the question is how much should this cleanup be persued or should using the kc. config name return the untransformed value instead - which seems to have been the original expectation in most places. Or should we directly use the quarkus property keys where applicable, which would be unambiguous?
There was a problem hiding this comment.
Thanks for looking into this, @shawkins!
From my perspective, this was a plain oversight that transformers are NOT applied when a config option is explicitly specified by user. @pedroigor Could you please confirm this behaviour is not intentional for some reason?
Luckily, this affects "only" the cases when we fetch the Keycloak config options directly, does not affect the cases when a Keycloak option is mapped to a Quarkus option which is then fetched by some Quarkus code (e.g. quarkus.http.proxy.proxy-address-forwarding in case of kc.proxy).
With regards to the raw values, feels like a SmallRye bug to me. Should we file an issue for it?
should using the kc. config name return the untransformed value instead
I don't think so. I don't see a reason why we should transform values only if mapped to some other Quarkus option. Feels counterintuitive. We should leverage the raw values instead.
Or should we directly use the quarkus property keys where applicable, which would be unambiguous?
I don't think so either. Feels like workaround to me. And not all options map to a Quarkus option, transformers should work for those too.
There was a problem hiding this comment.
@shawkins It is indeed a very sensitive code path and something we should improve. It is pretty much what @vmuzikar stated and I can confirm the behavior is expected.
@vmuzikar It is not by luck that it only affects kc options. The reason is that unmapped options does not have property mappers in our side bound to them.
With regards to the raw values, feels like a SmallRye bug to me. Should we file an issue for it?
Before doing it, let's make sure it is really an issue as it is very likely to be related to how we are handling/mapping values in our configuration layer.
I don't think so. I don't see a reason why we should transform values only if mapped to some other Quarkus option. Feels counterintuitive. We should leverage the raw values instead.
It is not only about transforming values when a property is mapped to a Quarkus option. We also have cases where transformation relies on other kc options too.
I don't think so either. Feels like workaround to me. And not all options map to a Quarkus option, transformers should work for those too.
Yeah, we shouldn't reference Quarkus properties at all in our providers/code but rely on our own options. And I think this is the solution we are missing to improve how we are mapping properties. I tried different approaches (including what is being proposed here) but as you can see it breaks the expected behavior.
Perhaps we can sync on this one and look at it together. It has been a while since I don't touch that area and working with someone else on it will be helpful to find a better/right solution.
shawkins
force-pushed
the
iss24757
branch
3 times, most recently
from
cce0e26
to
8072630
Compare
|
Opened the smallrye issue as smallrye/smallrye-config#1053 |
closes keycloak#24757 Signed-off-by: Steve Hawkins <[email protected]>
ghost
added
the
Unreported flaky test detectedIf the below 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.exportimport.ExportImportTest#testDirRealmExportImportKeycloak CI - Java Distribution IT (ubuntu-latest - temurin - 19) org.keycloak.testsuite.exportimport.ExportImportTest#testSingleFileRealmExportImportKeycloak CI - Java Distribution IT (ubuntu-latest - temurin - 19) org.keycloak.testsuite.exportimport.ExportImportTest#testImportWithNullAuthenticatorConfigAndNoDefaultBrowserFlowKeycloak CI - Java Distribution IT (ubuntu-latest - temurin - 19) org.keycloak.testsuite.exportimport.ExportImportTest#testSingleFileRealmWithoutBuiltinsImportKeycloak CI - Java Distribution IT (ubuntu-latest - temurin - 19) org.keycloak.testsuite.exportimport.ExportImportTest#testExportUserProfileConfigKeycloak CI - Java Distribution IT (ubuntu-latest - temurin - 19) org.keycloak.testsuite.exportimport.ExportImportTest#testDirFullExportImportKeycloak CI - Java Distribution IT (ubuntu-latest - temurin - 19) org.keycloak.testsuite.exportimport.ExportImportTest#testDirRealmExportImportKeycloak CI - Java Distribution IT (windows-latest - temurin - 19) org.keycloak.testsuite.exportimport.ExportImportTest#testSingleFileRealmExportImportKeycloak CI - Java Distribution IT (windows-latest - temurin - 19) org.keycloak.testsuite.exportimport.ExportImportTest#testImportWithNullAuthenticatorConfigAndNoDefaultBrowserFlowKeycloak CI - Java Distribution IT (windows-latest - temurin - 19) org.keycloak.testsuite.exportimport.ExportImportTest#testSingleFileRealmWithoutBuiltinsImportKeycloak CI - Java Distribution IT (windows-latest - temurin - 19) org.keycloak.testsuite.exportimport.ExportImportTest#testExportUserProfileConfigKeycloak CI - Java Distribution IT (windows-latest - temurin - 19) org.keycloak.testsuite.exportimport.ExportImportTest#testDirFullExportImportKeycloak CI - Java Distribution IT (windows-latest - temurin - 19) |
|
Superceded by #24812 |
There is an inconsistency in the mapping logic such that when you explicitly set a property, the untransformed value can be returned. This change is to always return the transformed value.
closes #24757