matrei commented on code in PR #16736:
URL: https://github.com/apache/grails-core/pull/16736#discussion_r4387442818


##########
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
+                        }
+                        Object value = readEnvironmentValue(environment, 
propertySource, name)
+                        if (value == null) {
+                            continue
+                        }
+                        String[] segments = 
name.substring(prefix.length()).split('\\.')

Review Comment:
   YAML lists reach this loop as indexed names. `NavigableMapPropertySource` 
lists `connectionInitSqls: [SET A=1, SET B=2]` as 
`dataSource.properties.connectionInitSqls[0]` and `[1]`. Splitting on `.` keeps 
`connectionInitSqls[0]` and `connectionInitSqls[1]` as literal keys instead of 
building one list:
   
   ```
   properties=[connectionInitSqls[0]:SET A=1, connectionInitSqls[1]:SET B=2, 
dataSourceProperties:[cachePrepStmts:true]]
   ```
   
   With DBCP2 this stops startup. `BasicDataSource.getConnectionInitSqls()` 
returns `Collections.emptyList()` while unset, so `DataSourceBuilder.bind` 
writing `connectionInitSqls[0]` throws `UnsupportedOperationException` from 
`AbstractNestablePropertyAccessor.processKeyedProperty`. I reproduced this with 
a `DataSource` that has the same getter and setter as DBCP2's. 
`disconnectionSqlCodes` has the same getter shape. On 8.0.0 these keys were 
dropped along with the rest of the map. The same values passed as a list in a 
direct map bind fine.
   
   Collecting `name[n]` segments into a `List` would fix it. 
`preparePropertyResolver` has the same gap, so this applies whichever way the 
fix goes. A feature with a YAML list under `dataSource.properties` would cover 
it.



##########
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()) {

Review Comment:
   This is the precedence point from the thread on L465, one level up. This 
early return lets a map stored at the exact path in a lower source win over 
flattened children in a higher one. With `low` holding `dataSource.properties: 
[maximumPoolSize: 20, minimumIdle: 2]` and `high` holding 
`dataSource.properties.maximumPoolSize: 50`:
   
   ```
   properties=[maximumPoolSize:20, minimumIdle:2]
   ```
   
   The direct read can't tell which source the map came from. Fixing it 
probably means reading the map from each source inside the walk, which already 
handles map values, instead of reading it once before the walk. It's fine to 
leave out of scope if no real config source puts a map at the exact path, but 
then a comment here should say so.



-- 
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]

Reply via email to