jdaugherty commented on code in PR #16160:
URL: https://github.com/apache/grails-core/pull/16160#discussion_r3812921693
##########
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/config/ConfigurationBuilder.groovy:
##########
@@ -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.
Review Comment:
`@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. #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.
##########
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/config/ConfigurationBuilder.groovy:
##########
@@ -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
+ }
+ 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) {
+ 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)
Review Comment:
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.
##########
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/config/ConfigurationBuilder.groovy:
##########
@@ -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
+ }
+ 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) {
Review Comment:
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).
--
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]
