Skip to content

fix(config): bind nested settings maps under Spring 7 #16160

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jamesfredley wants to merge 1 commit into
base: 8.0.x
Choose a base branch
from
+644 −17
Open

fix(config): bind nested settings maps under Spring 7 #16160

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
219 changes: 202 additions & 17 deletions ...tore-core/src/main/groovy/org/grails/datastore/mapping/config/ConfigurationBuilder.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@
*/
package org.grails.datastore.mapping.config

import java.beans.Introspector
import java.beans.PropertyDescriptor
import java.lang.reflect.InvocationTargetException
import java.lang.reflect.Method
import java.lang.reflect.Modifier

Expand All @@ -28,6 +31,7 @@ import groovy.transform.builder.SimpleStrategy
import groovy.util.logging.Slf4j

import org.springframework.core.convert.ConversionFailedException
import org.springframework.core.convert.ConverterNotFoundException
import org.springframework.core.env.PropertyResolver
import org.springframework.util.ClassUtils
import org.springframework.util.ReflectionUtils
Expand Down Expand Up @@ -393,23 +397,12 @@ abstract class ConfigurationBuilder<B, C> {
try {
value = propertyResolver.getProperty(propertyPathForArg, argType, fallBackValue)
} catch (ConversionFailedException e) {
if (argType.isEnum()) {
value = propertyResolver.getProperty(propertyPathForArg, String)
if (value != null) {
try {
value = Enum.valueOf((Class) argType, value.toUpperCase())
} catch (Throwable e2) {
// ignore e2 and throw original
throw new ConfigurationException("Invalid value for setting [$propertyPathForArg]: $e.message", e)
}
}
else {
throw new ConfigurationException("Invalid value for setting [$propertyPathForArg]: $e.message", e)
}
}
else {
throw new ConfigurationException("Invalid value for setting [$propertyPathForArg]: $e.message", e)
}
value = handleConversionException(e, argType, propertyPathForArg, fallBackValue)
} catch (ConverterNotFoundException e) {
// Spring 7 nested-map conversion fallback: handle types with
// @Builder(builderStrategy = SimpleStrategy) where Spring cannot
// auto-convert from Map. Independent of the Groovy version.

@jdaugherty jdaugherty Aug 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Builder is RUNTIME-retained on Groovy 5.0.8, so an argType annotated with @Builder(builderStrategy = SimpleStrategy) is intercepted by the argType.getAnnotation(Builder) branch earlier in buildRecurse and never reaches this getProperty call. The types that actually land here are ones with no runtime-visible @Builder (like the spec's synthetic settings classes). Two asks:

  1. Reword this comment (and the handleConverterNotFoundException javadoc) so it doesn't name @Builder(SimpleStrategy) types as the case being handled here.
  2. ConfigurationBuilder fails to bind nested settings under Spring 7 #16159 states the Hibernate and connection-source settings trees are real consumers, but HibernateConnectionSourceSettings, its nested types, ConnectionSourceSettings, and MultiTenancySettings are all annotated and bind through the recursion branch (the pre-existing specs in this file exercise that path and passed on 8.0.x before this change). Which shipped configuration reproduces the startup failure on 8.0.x? Worth capturing in the issue/PR so the affected surface is clear.

value = handleConverterNotFoundException(e, argType, propertyPathForArg, fallBackValue)
}
if (value != null) {
log.debug('Resolved value [{}] for setting [{}]', value, propertyPathForArg)
Expand Down Expand Up @@ -463,4 +456,196 @@ abstract class ConfigurationBuilder<B, C> {
protected void startBuild(Object builder, String configurationPath) {
// no-op
}
/**
* Handle ConversionFailedException - for enums, try case-insensitive conversion
*/
private Object handleConversionException(ConversionFailedException e, Class argType, String propertyPathForArg, Object fallBackValue) {
if (argType.isEnum()) {
def value = propertyResolver.getProperty(propertyPathForArg, String)
if (value != null) {
try {
return Enum.valueOf((Class) argType, value.toUpperCase())
} catch (IllegalArgumentException e2) {
throw new ConfigurationException("Invalid value for setting [$propertyPathForArg]: $e.message", e)
}
}
else {
throw new ConfigurationException("Invalid value for setting [$propertyPathForArg]: $e.message", e)
}
}
else {
ConverterNotFoundException converterNotFoundException = findConverterNotFoundException(e)
if (converterNotFoundException != null) {
return handleConverterNotFoundException(converterNotFoundException, argType, propertyPathForArg, fallBackValue)
}
throw new ConfigurationException("Invalid value for setting [$propertyPathForArg]: $e.message", e)
}
}

private static ConverterNotFoundException findConverterNotFoundException(Throwable exception) {
Throwable cause = exception
while (cause != null) {
if (cause instanceof ConverterNotFoundException) {
return (ConverterNotFoundException) cause
}
cause = cause.getCause()
}
return null
}

/**
* Handle ConverterNotFoundException - for nested configuration types,
* try to instantiate and populate from Map. This handles Spring 7 compatibility where
* Spring can't auto-convert from LinkedHashMap to these types. This is independent of the
* Groovy version and is required regardless of @Builder annotation retention.
*/
@CompileDynamic
private Object handleConverterNotFoundException(ConverterNotFoundException e, Class argType, String propertyPathForArg, Object fallBackValue, Object rawValue = null) {
if (rawValue == null) {
try {
// Use Object.class to prevent Spring's MapToMapConverter from deep-converting values
rawValue = propertyResolver.getProperty(propertyPathForArg, Object)
} catch (ConfigurationException e2) {
throw e2
} catch (Exception e2) {
throw new ConfigurationException("Cannot read configuration for path [$propertyPathForArg]: $e2.message", e2)
}
}

if (rawValue instanceof Map) {
try {
Map<String, PropertyDescriptor> writableProperties = [:]
Introspector.getBeanInfo(argType).propertyDescriptors.each { PropertyDescriptor property ->
if (property.name != 'metaClass' && property.writeMethod != null) {
writableProperties[property.name] = property
}
}

def instance = argType.getDeclaredConstructor().newInstance()
if (fallBackValue != null && argType.isInstance(fallBackValue)) {
// A map-backed settings type carries arbitrary entries as well as declared
// properties, so the inherited entries have to come across too or overriding
// one nested value would silently drop the rest.
if (instance instanceof Map && fallBackValue instanceof Map) {
((Map) instance).putAll((Map) fallBackValue)
}
writableProperties.values().each { PropertyDescriptor property ->
if (property.readMethod != null && property.readMethod.parameterCount == 0) {
Object fallbackPropertyValue = property.readMethod.invoke(fallBackValue)
property.writeMethod.invoke(instance, [fallbackPropertyValue] as Object[])
}
}
}

boolean mapBacked = instance instanceof Map
Set<String> resolvedProperties = [] as Set<String>
((Map) rawValue).each { key, val ->
String propertyName = key.toString()
PropertyDescriptor property = writableProperties[propertyName]
if (property != null) {
Object fallBackPropertyValue = getFallBackValue(fallBackValue, propertyName)
Object value = resolveMapValue(property.propertyType, "$propertyPathForArg.$propertyName", fallBackPropertyValue, val)
property.writeMethod.invoke(instance, [value] as Object[])
resolvedProperties.add(propertyName)
return
}
int nestedPropertySeparator = propertyName.indexOf('.')
if (nestedPropertySeparator > 0) {
String nestedPropertyName = propertyName.substring(0, nestedPropertySeparator)
PropertyDescriptor nestedProperty = writableProperties[nestedPropertyName]
if (nestedProperty != null) {
if (resolvedProperties.add(nestedPropertyName)) {
Object fallBackPropertyValue = getFallBackValue(fallBackValue, nestedPropertyName)
Object value = resolveMapValue(nestedProperty.propertyType, "$propertyPathForArg.$nestedPropertyName", fallBackPropertyValue, val)
nestedProperty.writeMethod.invoke(instance, [value] as Object[])
}
return
}
}
// Types that are themselves a Map (HibernateSettings extends LinkedHashMap, for
// example) exist precisely to carry arbitrary keys such as hibernate.hbm2ddl.auto,
// so an entry that is not a declared bean property belongs in the map rather than
// being rejected. Only types with a fixed set of properties reject unknown keys.
if (mapBacked) {
((Map) instance).put(key, val)
return
}
Comment on lines +569 to +572
throw new ConfigurationException("Unknown setting [$propertyPathForArg.$propertyName]")
}
return instance
} catch (ConfigurationException e2) {
throw e2
} catch (InvocationTargetException e2) {
Throwable cause = e2.targetException
if (cause instanceof Error) {
throw (Error) cause
}
if (cause instanceof ConfigurationException) {
throw (ConfigurationException) cause
}
throw new ConfigurationException("Invalid value for setting [$propertyPathForArg]: $cause.message", cause)
} catch (Exception e2) {
throw new ConfigurationException("Invalid value for setting [$propertyPathForArg]: $e2.message", e2)
}
}

if (rawValue != null) {
throw new ConfigurationException("Invalid value for setting [$propertyPathForArg]: cannot convert value [$rawValue] to required type [$argType.name]", e)
}

// If we have a fallback value, return it
if (fallBackValue != null) {
return fallBackValue
}

if (e != null) {
throw new ConfigurationException("Invalid value for setting [$propertyPathForArg]: $e.message", e)
}
return null
}

private Object resolveClassValue(String propertyPath) {
Object rawValue = propertyResolver.getProperty(propertyPath, Object)
if (rawValue instanceof Class) {
return rawValue
}
String className = rawValue instanceof CharSequence ? rawValue.toString().trim() : null
if (!className) {
return null
}
ClassLoader classLoader = Thread.currentThread().contextClassLoader ?: getClass().classLoader
try {
return ClassUtils.forName(className, classLoader)
} catch (ClassNotFoundException | LinkageError e) {
throw new ConfigurationException("Invalid class name [$className] for setting [$propertyPath]: ${e.message}", e)
}
}

private Object resolveMapValue(Class propertyType, String propertyPath, Object fallBackValue, Object rawValue) {
// Class-typed entries must use the same thread context class loader route as the
// top-level Class handling above, because the resolver's String->Class converter
// resolves against the framework class loader and silently leaves an
// application-defined class (hibernate.configClass, for example) unbound.
if (propertyType == Class) {

@jdaugherty jdaugherty Aug 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This branch and resolveClassValue aren't exercised by the new specs: MapBackedSettings.configClass is declared as String, and no test settings type has a Class-typed property, so the guard listed in the PR table ("Resolve Class entries via the thread context class loader") has no regression test. Could you add coverage for a nested Class-typed property — a Class literal value, a String class name, and an invalid class name?

While adding that, worth pinning the fallback behavior: the top-level argType == Class handling falls back to fallBackValue when no usable value is found, but resolveClassValue returns null, so a value that is neither a Class nor a CharSequence would silently clear an inherited fallback through the setter instead of retaining it (or erroring).

return resolveClassValue(propertyPath)
}
if (rawValue instanceof Map && !propertyType.isInstance(rawValue)) {
return handleConverterNotFoundException(null, propertyType, propertyPath, fallBackValue, rawValue)
}
try {
Object value = propertyResolver.getProperty(propertyPath, propertyType)
Object rawPropertyValue = propertyResolver.getProperty(propertyPath, Object)

@jdaugherty jdaugherty Aug 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The raw lookup runs even when the typed lookup on the previous line already returned a non-null value, where its result is discarded. It's only needed in the value == null case, so it can move inside that branch and save a resolver pass per nested scalar.

if (value == null && rawPropertyValue instanceof Map) {
if (propertyType.isInstance(rawPropertyValue)) {
return rawPropertyValue
}
return handleConverterNotFoundException(null, propertyType, propertyPath, fallBackValue, rawPropertyValue)
}
return value
} catch (ConversionFailedException e) {
return handleConversionException(e, propertyType, propertyPath, fallBackValue)
} catch (ConverterNotFoundException e) {
return handleConverterNotFoundException(e, propertyType, propertyPath, fallBackValue)
}
}
}
Loading
Loading