sbglasius commented on PR #16281: URL: https://github.com/apache/grails-core/pull/16281#issuecomment-5507699909
Closing in favour of https://github.com/apache/grails-core/pull/16293, which takes the approach you asked for, @jdaugherty. Two things came out of taking another run at it. **The root cause is not what this PR's description claims.** I attributed it to `DefaultGroovyMethods` overload selection changing. It did not — the `getAt`/`putAt` signatures are byte-identical in 4.0.33 and 5.1.0. The change is in `MetaClassImpl`, for classes implementing `Map`: - `getProperty` in Groovy 4 did `if (isMap && !isStatic) return ((Map) object).get(name);` unconditionally, *before* any getter lookup. In Groovy 5 the getter lookup runs first, so a public getter wins. - `setProperty` in Groovy 4 used the setter if one existed and otherwise put into the map. In Groovy 5 the map-`put` branch is guarded by `(mp == null || !mp.isPublic() || isSpecialProperty(name))`, so a public getter-only property falls through to `throw new ReadOnlyPropertyException`. So the trigger is just: a public JavaBean accessor on a `Map` implementation shadows the entry of the same name. **Which means your point about static compilation was the decisive one.** You were right that documenting it was the wrong answer, and it goes further than "inconvenient": in `StaticTypesCallSiteWriter`, `makeGetPropertyWithGetter(...)` is tried *before* `writeMapDotProperty(...)`, and the mutator loop before the `Map.put` branch. The static compiler binds to the declared accessor and never reaches `getProperty`/`setProperty`, so no runtime hook — and no annotation or AST transform, to answer your Slack question — can fix that half. Only removing the accessor reaches both paths. The new PR does exactly that: `getRequest()` becomes `request()`, `getIdentifier()` is gone (`params.id`), `isGspTagSyntaxCall()` becomes `gspTagSyntaxCall()`. No `getProperty`/`setProperty`/`getAt`/`putAt` overrides at all. `setGspTagSyntaxCall(boolean)` is kept on purpose, so the write-side behaviour you flagged stays as it was in Grails 7. Your other review points are carried over: the command-object spec now submits only `identifier` with no `id` so the fallback actually runs (mutation-checked — it fails on the resolution, not on a duplicate assertion), the `attrs` half is covered including the static case, and the `junit-jupiter-engine` gap is fixed module-local with the convention-level fix tracked in #16289. Leaving this thread open would mean nine inline comments anchored to code that no longer exists, so it is cleaner to review the new PR fresh — but the analysis in your review is what produced it, and it is linked from there. -- 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]
