[I] ConfigurationBuilder Spring 7 nested-map conversion: nine defects, two still open [grails-core]

["jamesfredley (via GitHub)" <gi...@apache.org>]

jamesfredley opened a new issue, #16159:
URL: https://github.com/apache/grails-core/issues/16159

   `ConfigurationBuilder` gained a Spring 7 compatibility path: when Spring cannot convert a configuration `Map` into a type annotated `@Builder(builderStrategy = SimpleStrategy)`, the builder instantiates the target and populates it from the Map. That path currently ships on the Groovy 6 canary branch (#15558), where it is 261 of that branch's insertions despite its own docstring stating it is *"independent of the Groovy version"*.
   
   An attempt to extract it onto `9.0.x`, where it belongs, ran it through two independent code reviewers over four rounds. The gap on trunk is real - with trunk's original `ConfigurationBuilder` and only the new spec applied, six scenarios fail with:
   
   ```
   Expected exception of type 'ConfigurationException',
     but got 'org.springframework.core.convert.ConverterNotFoundException'
   ```
   
   `9.0.x` resolves `org.springframework:spring-core:7.0.8` via Spring Boot 4.1.0, so the affected Spring version is present.
   
   However, the review surfaced nine distinct defects in the implementation. Seven were fixed and covered with tests; two remain open and need someone with Hibernate configuration-binding knowledge.
   
   ## Fixed, with regression tests
   
   | # | Defect | Consequence |
   |---|---|---|
   | 1 | Every non-enum `ConversionFailedException` triggered the Map fallback without checking the cause chain contained `ConverterNotFoundException` | A converter that deliberately rejects a Map could be bypassed |
   | 2 | A surrounding `catch (Throwable)` swallowed `ConfigurationException` | Unknown-key and malformed-value failures were masked by the original exception |
   | 3 | Raw-value resolution failures were logged and discarded, leaving the population-failure flag null | A non-null fallback was returned, silently accepting configuration whose lookup had failed |
   | 4 | Population built a fresh instance without copying the fallback | Every field the caller did not specify was lost, breaking `buildRecurse` per-field inheritance for named connection settings |
   | 5 | Map entries were assigned raw | Converters and case-insensitive enum handling were bypassed, so `multiTenancy.mode: database` was rejected where `DATABASE` worked |
   | 6 | Any key that was not a declared bean property was rejected | `HibernateSettings extends LinkedHashMap<String, String>` exists to carry arbitrary keys, so `hibernate.hbm2ddl.auto` would be rejected as an unknown setting |
   | 7 | `Method.invoke(instance, value)` hit the Java null-varargs pitfall | An explicit null could not clear an inherited value |
   
   Coverage went from 10 to 19 specs in `ConfigurationBuilderSpec`, module total 108 to 117, all passing on Groovy 5.
   
   ## Open, not fixed
   
   **A. Flattened descendant keys are rejected.** `DatastoreUtils.createPropertyResolver` flattens nested configuration, so a parent map contains both immediate children and flattened descendants - `hibernate.jpa` holds both `compliance` and `compliance.cascade`. Since `JpaSettings` is not map-backed, the flattened key reaches the strict unknown-key branch and is rejected, so configuration nested more than one level deep fails to build.
   
   **B. Fallback inheritance does not recurse.** When a fallback object has a nested child and the configuration overrides only part of that child, the nested resolve runs with a null fallback. The previously copied child is replaced, silently losing its unspecified fallback fields. Inheritance therefore works only at the first manually populated level.
   
   Both need validation against real Hibernate and multi-datasource configuration rather than unit-level mocks, which is why they are being reported rather than patched speculatively.
   
   ## Why this matters now
   
   Defects 1-7 and both open items are **live on the canary branch today**, since that is where this code currently resides. Items A and B in particular can reject valid configuration or silently reset inherited settings at application startup.
   
   ## Suggested handling
   
   1. Fix A and B with integration coverage over genuinely nested Hibernate configuration.
   2. Land the whole thing on `9.0.x` as a Spring 7 compatibility fix.
   3. Drop it from #15558, which then carries only genuine Groovy 6 workarounds and roughly halves in size.
   
   The seven fixes plus the 9 added specs exist on the local branch `fix/spring7-nested-map-conversion` and can be pushed if wanted as a starting point.
   


-- 
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: notifications-unsubscribe@grails.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] ConfigurationBuilder fails to bind nested settings under Spring 7 [grails-core]

["jdaugherty (via GitHub)" <gi...@apache.org>]

jdaugherty closed issue #16159: ConfigurationBuilder fails to bind nested settings under Spring 7
URL: https://github.com/apache/grails-core/issues/16159


-- 
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: notifications-unsubscribe@grails.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] ConfigurationBuilder Spring 7 nested-map conversion: nine defects, two still open [grails-core]

["jamesfredley (via GitHub)" <gi...@apache.org>]

jamesfredley commented on issue #16159:
URL: https://github.com/apache/grails-core/issues/16159#issuecomment-5316301171

   Retargeted the fix (#16160) from `9.0.x` to **`8.0.x`**.
   
   This is not 9.0-only. `8.0.x` and `9.0.x` both pin Spring Boot `4.1.0` (so `spring-core` 7.0.8) and Groovy `5.0.8`, and `ConfigurationBuilder.groovy` plus its spec are byte-identical on the two branches - so the Spring 7 nested-map gap exists on `8.0.x` as well. Landing it at the oldest maintained branch lets it flow up through `8.0.x` -> `8.1.x` -> `9.0.x` instead of being fixed only on the newest line.
   
   Re-verified on the `8.0.x` base after rebasing: `:grails-datastore-core:test --rerun-tasks` BUILD SUCCESSFUL, `tests=120 failures=0`, `ConfigurationBuilderSpec tests=22`. The rebased diff is byte-identical to the reviewed content, so the review verdict carries over unchanged.


-- 
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: notifications-unsubscribe@grails.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org