Repository navigation
fix: allowing the general use of synthetic wildcards - #48223
Conversation
There was a problem hiding this comment.
Test failures look related. I found a missing throw :)
0183809 to
c4c7f3b
Compare
There was a problem hiding this comment.
LGTM, I just have a suggestion and a question 👍
| @@ -458,31 +407,33 @@ private static String transformPoolMaxLifetime(String db, ConfigSourceIntercepto | |||
| }; | |||
| } | |||
|
|
|||
| public static final class Datasources { | |||
| public static final class Datasources extends org.keycloak.config.DatabaseOptions.Datasources { | |||
There was a problem hiding this comment.
I believe the reason this and the DatabaseOptions class still exist is just to keep the Option.setWildcardKey method as protected and directly mutating the Option - which we otherwise try to avoid.
I did try though to minimize the public surface area.
|
|
||
| Map<String, Option<?>> cachedDatasourceOptions = new HashMap<>(); | ||
| cachedDatasourceOptions.put(DB.getKey(), DB_KIND); | ||
| mappers.stream().map(PropertyMapper::getOption).forEach(o -> getDatasourceOption(o, cachedDatasourceOptions)); |
There was a problem hiding this comment.
It is unnecessary to populate the map as you can do getDatasourceOption(o, cachedDatasourceOptions) inside the loop below.
There was a problem hiding this comment.
Right, I didn't fully finish decoupling the logic. Should be better now.
|
|
||
| Map<String, Option<?>> cachedDatasourceOptions = new HashMap<>(); | ||
| cachedDatasourceOptions.put(DB.getKey(), DB_KIND); | ||
| mappers.stream().map(PropertyMapper::getOption).forEach(o -> cachedDatasourceOptions.computeIfAbsent(o.getKey(), k -> getDatasourceOption(o))); |
There was a problem hiding this comment.
I meant this line can be removed and the cachedDatasourceOptions.get(String) below replaced with cachedDatasourceOptions.computeIfAbsent(Option.getKey(), ignored -> getDatasourceOption(Option));
There was a problem hiding this comment.
I understand, but I'm willing to have an extra iteration over the mappers to keep the knowledge of adding entries to the cache localized.
| @@ -412,7 +413,7 @@ void checkPersistenceUnits(List<PersistenceXmlDescriptorBuildItem> descriptors) | |||
| .filter(descriptor -> !descriptor.getName().equals(DEFAULT_PERSISTENCE_UNIT)) // not default persistence unit | |||
| .map(KeycloakProcessor::getDatasourceNameFromPersistenceXml) | |||
| .filter(this::missingDbKind) | |||
| .map(datasourceName -> DatabaseOptions.Datasources.getNamedKey(DatabaseOptions.DB, datasourceName).orElseThrow()).toList(); | |||
| .map(datasourceName -> PropertyMappers.getWildcardPropertyMapper(DatabaseOptions.DB_KIND).orElseThrow().getFrom(datasourceName)).toList(); | |||
There was a problem hiding this comment.
I don't have problem with this, but I don't think previous state where db-kind-<datasource> https://www.keycloak.org/server/all-config?q=db-kind was shown was fine. IIUC kc.db-kind-<datasource> is used in app props, sys props or as KC_... in env vars, but the db-kind-<datasource> is used in keycloak.conf and with -- as CLI option. So db-kind<datasource> is the part that is shared and documented.
e027bb9 to
3d827f4
Compare
| DatabasePropertyMappers.getDatasourceOptionValue(DatabaseOptions.DB_SQL_JPA_DEBUG, datasourceName) | ||
| .ifPresent(f -> unitProperties.put(AvailableSettings.USE_SQL_COMMENTS, f)); |
There was a problem hiding this comment.
@shawkins just to quickly check previously USE_SQL_COMMENTS was only set when the value was true. Now any explicitly set value is passed through to Hibernate. Is this semantic change intentional?
There was a problem hiding this comment.
Yes, I think this is the appropriate behavior. I would expect for example that I explictly set --db-debug-jpql-datasource=false, that debugging would be turned off.
The line of thinking before may have been not to override the presistence xml if the first-class option were set to its default (false). However that really implies there isn't a default, or the meaning of false needs to be documented clearly.
There was a problem hiding this comment.
Sorry for the late review, looks good.
| (getter, namedProperty, alias) -> "jdbc:oracle:thin:%s//%s:%s/%s".formatted( | ||
| DatabaseOptions.DatabaseTlsMode.fromCliValue(getProperty(DatabaseOptions.DB_TLS_MODE, getter, | ||
| DatabaseOptions.DatabaseTlsMode.DISABLED.toCliValue())) == DatabaseTlsMode.DISABLED ? "@" : "@tcps:", | ||
| getProperty(DatabaseOptions.DB_URL_HOST, getter, "localhost"), | ||
| getProperty(DatabaseOptions.DB_URL_PORT, getter, "1521"), | ||
| getProperty(DatabaseOptions.DB_URL_DATABASE, getter, "keycloak")), |
There was a problem hiding this comment.
This is an old problem but Oracle is missing handling of DatabaseOptions.DB_URL_PROPERTIES. But it's not a regression, it was like that before too, so approving regardless.
There was a problem hiding this comment.
Right, that was noted in another pr, but I don't think a follow-up was created - adding now so we don't forget this time: #49256
also simplifying datasources logic closes: keycloak#48214 Signed-off-by: Steve Hawkins <shawkins@redhat.com>
also simplifying datasources logic closes: keycloak#48214 Signed-off-by: Steve Hawkins <shawkins@redhat.com>
also simplifying datasources logic
closes: #48214