codeconsole commented on PR #15700: URL: https://github.com/apache/grails-core/pull/15700#issuecomment-4572637187
## Updated based on review Pushed an amended commit with the following tweaks (verified by an independent multi-agent review of the original PR): ### Test coverage improved The original test (now renamed *"constraints closure does not overwrite mapping properties (standard DSL flow)"*) passed even with the fix reverted — both closures route through `builder.invokeMethod` and reuse the same instance, so `target.propertyConfigs` stays empty for that flow and the changed merge path is never exercised. The new spec adds **"mapping property in builder.properties is preserved when target.propertyConfigs has a different instance for the same key"** which: - Populates `builder.properties['name']` via the mapping closure (the normal `invokeMethod` path) - Independently populates `entity.propertyConfigs['name']` with a separate instance (simulating the constraint-evaluator / `Entity.property(name, Map)` direct-API path that actually triggers the bug in a full Grails app) - Asserts the builder's mapping-configured instance is the one returned by `getProperties()` **Verified:** this new test FAILS against the pre-fix `properties.putAll(target.propertyConfigs)` code and PASSES with the `putIfAbsent` fix. The TCK-based MongoDB integration spec was attempted but does not exercise the bug — the TCK uses `DefaultGrailsApplication` directly and skips the constraint-evaluator wiring that populates `propertyConfigs` in a real Grails-app boot, so it cannot reproduce the failure. Also added **"target.propertyConfigs entries fill in for keys the mapping closure never touched"** to lock in that constraint-only configuration for properties the mapping never mentions still propagates correctly. ### Code comment added Added javadoc to `getProperties()` documenting: - The precedence rule (mapping-side instance wins when both maps have the same key) - Why the asymmetry exists (dual-store legacy) - A link back to this issue for the structural cleanup that would let the method go away ### Known follow-up The dual-store design (`builder.properties` vs `Entity.propertyConfigs`) is the underlying root cause — this PR papers over a symptom. A future major-release cleanup should collapse the two into a single canonical store. Tracking would happen in a separate issue. -- 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]
