Copilot commented on code in PR #16736:
URL: https://github.com/apache/grails-core/pull/16736#discussion_r4352952619
##########
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/config/ConfigurationBuilder.groovy:
##########
@@ -425,6 +430,152 @@ abstract class ConfigurationBuilder<B, C> {
}
+ private Map resolveMapProperty(String propertyPath, Map fallBackValue) {
+ Map directValue
+ RuntimeException conversionFailure = null
+ try {
+ directValue = propertyResolver.getProperty(propertyPath, Map)
+ } catch (ConversionFailedException | ConverterNotFoundException e) {
+ conversionFailure = e
+ directValue = null
+ }
+ if (directValue != null && !directValue.isEmpty()) {
+ return directValue
+ }
+ if (propertyResolver instanceof ConfigurableEnvironment) {
+ ConfigurableEnvironment environment = (ConfigurableEnvironment)
propertyResolver
+ String prefix = propertyPath + '.'
+ Map<String, Object> result = new LinkedHashMap<>()
+ Set<String> resolvedNames = new HashSet<>()
+ for (PropertySource<?> propertySource :
environment.propertySources) {
+ if (propertySource instanceof EnumerablePropertySource) {
+ for (String name : ((EnumerablePropertySource<?>)
propertySource).propertyNames) {
+ if (!name.startsWith(prefix) ||
!resolvedNames.add(name)) {
+ continue
+ }
Review Comment:
The rebuild logic only considers raw enumerable property names that
literally start with `propertyPath + '.'`. This will miss common Spring
`Environment` sources where the enumerable names don’t match canonical dotted
notation (notably `SystemEnvironmentPropertySource` with uppercase/underscore
keys), even though
`environment.getProperty(\"dataSource.properties.maximumPoolSize\")` would
resolve via relaxed lookup. As a result, flattened map binding may still fail
for env-var/system-property style inputs. Consider adding normalization for
env-var style keys (upper + underscores) before the prefix check, or using a
binder mechanism that supports relaxed names for map binding if available in
this module.
##########
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/config/ConfigurationBuilder.groovy:
##########
@@ -425,6 +430,152 @@ abstract class ConfigurationBuilder<B, C> {
}
+ private Map resolveMapProperty(String propertyPath, Map fallBackValue) {
+ Map directValue
+ RuntimeException conversionFailure = null
+ try {
+ directValue = propertyResolver.getProperty(propertyPath, Map)
+ } catch (ConversionFailedException | ConverterNotFoundException e) {
+ conversionFailure = e
+ directValue = null
+ }
+ if (directValue != null && !directValue.isEmpty()) {
+ return directValue
+ }
+ if (propertyResolver instanceof ConfigurableEnvironment) {
+ ConfigurableEnvironment environment = (ConfigurableEnvironment)
propertyResolver
+ String prefix = propertyPath + '.'
+ Map<String, Object> result = new LinkedHashMap<>()
+ Set<String> resolvedNames = new HashSet<>()
+ for (PropertySource<?> propertySource :
environment.propertySources) {
+ if (propertySource instanceof EnumerablePropertySource) {
+ for (String name : ((EnumerablePropertySource<?>)
propertySource).propertyNames) {
+ if (!name.startsWith(prefix) ||
!resolvedNames.add(name)) {
+ continue
+ }
Review Comment:
When direct map binding is null/empty, this scans every enumerable key of
every `PropertySource`. In real apps `systemProperties` / `systemEnvironment`
can be very large, and this method may be invoked multiple times during
configuration building (for each map-typed property). Consider caching rebuilt
maps by `propertyPath` within the `ConfigurationBuilder` instance, or
restricting scanning to property sources likely to contain application config
(while preserving correctness/precedence), to avoid repeated O(totalKeys)
traversals at startup.
##########
grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/jdbc/connections/DataSourceConnectionSourceFactorySpec.groovy:
##########
@@ -18,14 +18,305 @@
*/
package org.grails.datastore.gorm.jdbc.connections
+import com.zaxxer.hikari.HikariDataSource
+import groovy.util.ConfigObject
import org.grails.datastore.mapping.core.DatastoreUtils
+import org.grails.datastore.mapping.core.exceptions.ConfigurationException
import org.grails.datastore.mapping.core.connections.ConnectionSource
+import org.springframework.core.convert.converter.Converter
+import org.springframework.core.convert.support.DefaultConversionService
+import org.springframework.core.env.MapPropertySource
+import org.springframework.core.env.StandardEnvironment
import org.springframework.jdbc.datasource.LazyConnectionDataSourceProxy
import org.springframework.jdbc.datasource.TransactionAwareDataSourceProxy
import spock.lang.Specification
class DataSourceConnectionSourceFactorySpec extends Specification {
+ void "create binds flattened default data source pool properties from the
Spring Environment"() {
+ given:
+ def factory = new DataSourceConnectionSourceFactory()
+ def environment = new StandardEnvironment()
+ environment.propertySources.addFirst(new
MapPropertySource('application.yml', [
+ 'dataSource.url': 'jdbc:h2:mem:factoryEnvironmentTest',
+ 'dataSource.driverClassName': 'org.h2.Driver',
+ 'dataSource.username': 'sa',
+ 'dataSource.password': '',
+ 'dataSource.type': 'com.zaxxer.hikari.HikariDataSource',
+ 'dataSource.properties.maximumPoolSize': 16,
+ 'dataSource.properties.minimumIdle': 8,
+ 'dataSource.properties.connectionTimeout': 2000,
+
'dataSource.properties.dataSourceProperties.connectionCollation':
'utf8mb4_unicode_ci'
+ ]))
+
+ when:
+ def connectionSource = factory.create(ConnectionSource.DEFAULT,
environment)
+
+ then:
+ connectionSource.settings.url == 'jdbc:h2:mem:factoryEnvironmentTest'
+ connectionSource.settings.username == 'sa'
+ connectionSource.settings.password == ''
+ connectionSource.source instanceof TransactionAwareDataSourceProxy
+ def lazyDataSource = ((TransactionAwareDataSourceProxy)
connectionSource.source).targetDataSource
+ lazyDataSource instanceof LazyConnectionDataSourceProxy
+ def dataSource = ((LazyConnectionDataSourceProxy)
lazyDataSource).targetDataSource
+ dataSource instanceof HikariDataSource
+ dataSource.maximumPoolSize == 16
+ dataSource.minimumIdle == 8
+ dataSource.connectionTimeout == 2000
+ dataSource.dataSourceProperties.getProperty('connectionCollation') ==
'utf8mb4_unicode_ci'
+
+ cleanup:
+ connectionSource?.close()
+ }
+
+ void "create binds flattened named data source pool properties from the
Spring Environment"() {
+ given:
+ def factory = new DataSourceConnectionSourceFactory()
+ def environment = new StandardEnvironment()
+ environment.propertySources.addFirst(new
MapPropertySource('application.yml', [
+ 'dataSources.secondary.url':
'jdbc:h2:mem:factoryNamedEnvironmentTest',
+ 'dataSources.secondary.type': HikariDataSource.name,
+ 'dataSources.secondary.properties.maximumPoolSize': 24
+ ]))
+
+ when:
+ def connectionSource = factory.create('secondary', environment)
+
+ then:
+ connectionSource.name == 'secondary'
+ def dataSource =
connectionSource.source.targetDataSource.targetDataSource
+ dataSource instanceof HikariDataSource
Review Comment:
These tests assume a specific proxy layering
(`TransactionAwareDataSourceProxy` → `LazyConnectionDataSourceProxy` → target).
That makes the spec brittle if the factory’s proxy chain changes (even if
behavior remains correct). Prefer asserting/unwraping proxies step-by-step (as
done in the first test) or adding a small helper in the spec to unwrap to the
underlying `DataSource` defensively before asserting it’s a `HikariDataSource`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
