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


##########
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('\\.')
+                        Map<String, Object> current = result
+                        for (int segmentIndex = 0; segmentIndex < 
segments.length - 1; segmentIndex++) {
+                            String segment = segments[segmentIndex]
+                            Object child = current.get(segment)
+                            if (!(child instanceof Map)) {

Review Comment:
   Fixed in 84e8282825. The map is rebuilt one source at a time in precedence 
order, and a path already set by a higher source is skipped whatever its shape, 
including a lower map or `${missing}`. Added `create keeps a higher precedence 
scalar over a lower flattened descendant` next to the precedence feature, and 
`Test Spring Environment rebuilds map-backed builder settings with source 
precedence` covers the `flush.mode` case.



##########
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/config/ConfigurationBuilder.groovy:
##########
@@ -258,7 +261,7 @@ abstract class ConfigurationBuilder<B, C> {
                         }
 
                         if (newBuilder instanceof Map) {
-                            Map subMap = 
propertyResolver.getProperty(propertyPath, Map, Collections.emptyMap())
+                            Map subMap = resolveMapProperty(propertyPath, 
Collections.emptyMap())

Review Comment:
   Added in 84e8282825. `Test Spring Environment rebuilds map-backed builder 
settings with source precedence` uses a `StandardEnvironment` and a map-backed 
`@Builder(SimpleStrategy)` fixture, and asserts both the declared `configClass` 
binding and the rebuilt nested entries. `Test DatastoreUtils resolver retains 
existing map-backed builder binding` shows that resolver is unchanged. The 
description now notes that map-backed settings such as `HibernateSettings` 
receive these keys from the Environment.



##########
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('\\.')
+                        Map<String, Object> current = result
+                        for (int segmentIndex = 0; segmentIndex < 
segments.length - 1; segmentIndex++) {
+                            String segment = segments[segmentIndex]
+                            Object child = current.get(segment)
+                            if (!(child instanceof Map)) {
+                                child = new LinkedHashMap<String, Object>()
+                                current.put(segment, child)
+                            }
+                            current = (Map<String, Object>) child
+                        }
+                        String leaf = segments[segments.length - 1]
+                        if (value instanceof Map) {
+                            Map<String, Object> copied = copyConfigMap((Map<?, 
?>) value)
+                            Object existing = current.get(leaf)
+                            if (existing instanceof Map) {
+                                mergeMissing((Map<String, Object>) existing, 
copied)
+                            }
+                            else {
+                                current.put(leaf, copied)
+                            }
+                            claimMapPaths(resolvedNames, name, copied)
+                        }
+                        else if (!(current.get(leaf) instanceof Map)) {
+                            current.put(leaf, value)
+                        }
+                    }
+                }
+            }
+            if (!result.isEmpty()) {
+                return result
+            }
+        }
+        if (conversionFailure != null) {
+            throw conversionFailure
+        }
+        return directValue != null ? directValue : fallBackValue
+    }
+
+    /**
+     * Read one flattened entry without forcing it to String. A descendant may 
already be a
+     * Map, as Hibernate listener settings are, and String conversion throws 
before that map
+     * can be bound.
+     */
+    private static Object readEnvironmentValue(ConfigurableEnvironment 
environment, PropertySource<?> propertySource, String name) {
+        try {
+            return environment.getProperty(name, Object)

Review Comment:
   That read is gone in 84e8282825. `readEnvironmentValue` takes the raw value 
from the highest source, and only a String goes through 
`Environment.getProperty`, so the unresolvable-placeholder setting is kept. The 
comment now says that.



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