jdaugherty commented on PR #16160:
URL: https://github.com/apache/grails-core/pull/16160#issuecomment-5803768727
Pushed 8aee638. It addresses the four open review comments and a defect they
surfaced.
## Which shipped configuration reproduces this
`hibernate.hibernateEventListeners`. Configured as
```yaml
hibernate:
hibernateEventListeners:
listenerMap:
pre-load: someListener
```
Spring 7 throws at binding time:
```
ConverterNotFoundException: No converter found capable of converting from
type
[java.util.LinkedHashMap<?, ?>] to type
[org.grails.orm.hibernate.HibernateEventListeners]
```
`HibernateEventListeners` is a plain bean with no runtime `@Builder`, which
is why it reaches
this branch. The review comment on the `@Builder` retention is correct —
`javap` on
`groovy-5.0.8.jar` confirms `groovy.transform.builder.Builder` is
`RUNTIME`-retained, so
`argType.getAnnotation(Builder)` intercepts every `SimpleStrategy` type in
the recursion above
and those never reach the new handler. The Hibernate and connection-source
trees named in
\#16159 bind through the recursion, not here. The code comment and javadoc
are corrected to
describe plain settings beans instead.
## The fallback did not bind it
With the previous revision applied, that configuration produced a
`HibernateEventListeners`
with a null `listenerMap` — a loud startup exception became silent data loss.
This is the documented "known limitation", and the claim that
`DatastoreUtils.createPropertyResolver` is unaffected by it does not hold.
`createFlatConfig` only emits dotted keys for `ConfigObject` values; a plain
nested `Map` is
stored as a leaf. So
`getProperty('hibernate.hibernateEventListeners.listenerMap', Map)`
returns null while the value sits in the very map `resolveMapValue` was
handed and discarded.
`resolveMapValue` now binds the entry in hand when the typed lookup finds
nothing at the path
and the value is already assignable — the follow-up proposed in the
description. Values needing
conversion still route through the enum, `Class` and nested-map branches, so
the case-insensitive
enum handling is untouched.
## Review comments addressed
| Comment | Change |
|---|---|
| Comment and javadoc name `@Builder(SimpleStrategy)` as the case handled |
Reworded to plain settings beans; the recursion handles annotated types |
| `Class` branch and `resolveClassValue` have no regression test | Specs for
a class literal, a class name |
| `resolveClassValue` returns null where the top-level handling falls back |
Returns the fallback when no class, with a spec |
| Raw lookup runs even when the typed lookup succeeded | Moved inside the
`value == null` branch, one ralar |
| Map-backed entries stored under the original key object | Stored under the
normalized `String`, with a spec using a `GString` key |
Two further defects found while verifying:
- A flattened descendant key passed the **descendant's** value as the parent
property's raw
value. Not reachable through the real resolver's key ordering, but it
would bind a
grandchild map onto its parent. The descendant is now resolved from its
path.
- A setter rejecting a value reported the **parent's** path. It now reports
the property's own
path and the expected type, rather than surfacing `argument type mismatch`.
## Testing
`ConfigurationBuilderSpec` goes from 22 to 29. Five of the seven new specs
fail against the
previous revision; the two `Class`-name specs pass on both and exist to
close the coverage gap
rather than pin new behaviour.
`HibernateConnectionSourceSettingsBuilderSpec` gains the real
shipped-configuration case above.
```
:grails-datastore-core:test SUCCESS
:grails-datamapping-core:test SUCCESS
:grails-data-hibernate7-core:test + :grails-data-hibernate5-core:test
3036 tests, 0 failures, 24 skipped
:grails-data-mongodb-core:test --tests *MongoConnectionSource* SUCCESS
:grails-datastore-core:codeStyle :grails-data-hibernate7-core:codeStyle
SUCCESS
```
Neo4j is not in `settings.gradle` on `8.0.x`, so `Neo4jDriverConfigBuilder`
was not exercised.
PMD and SpotBugs tasks are not registered on this branch, so `codeStyle`
(CodeNarc + Checkstyle)
is the analysis gate.
## Description needs an update
The "Known limitation" paragraph is now wrong on two counts — the flattening
resolver *is*
affected, and the limitation is fixed rather than deferred. The impact table
in #16159 also
still attributes the failure to the `@Builder` settings trees. Both want
rewording before merge.
--
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]
