sbglasius opened a new pull request, #16293: URL: https://github.com/apache/grails-core/pull/16293
Fixes #16280 Supersedes #16281 ## Root cause The issue and #16281 both attributed this to a change in `DefaultGroovyMethods` overload selection. That is not what happened — the `getAt`/`putAt` signatures are byte-identical in Groovy 4.0.33 and 5.1.0, and selection among them did not change. The change is inside `MetaClassImpl`, for classes that implement `Map`: | | Groovy 4.0.33 | Groovy 5.1.0 | |---|---|---| | `getProperty` | `if (isMap && !isStatic) return ((Map) object).get(name);` — unconditionally, before any getter lookup | getter lookup runs first; the map `get` is only reached when there is no visible public getter | | `setProperty` | setter if one exists, **else** `((Map) object).put(name, newValue)` | the map-`put` branch is guarded by `(mp == null \|\| !mp.isPublic() \|\| isSpecialProperty(name))`, so a public getter-only bean property falls through to `throw new ReadOnlyPropertyException` | So the trigger is simply: **a public JavaBean accessor on a `Map` implementation now shadows the map entry of the same name.** The subscript form reaches the same code via `DefaultGroovyMethods.putAt(Object, String, Object)` → `InvokerHelper.setProperty`. `GrailsParameterMap` declared `getRequest()` and `getIdentifier()`, which is why request parameters named `request` and `identifier` became unreadable and threw on assignment. ## Fix Remove the colliding accessors. With no bean accessor of that name, Groovy 5 reaches the map on every path: - dynamic — `getProperty` → `method == null` → `isMap` → `Map.get`; `setProperty` → `mp == null` → `Map.put` - `@CompileStatic` — `StaticTypeCheckingVisitor.getTypeForMapPropertyExpression` types it as a map entry and `StaticTypesCallSiteWriter` emits `Map.get` / `Map.put` - subscript, both modes — DGM `getAt`/`putAt` land on the same `get`/`put` The whole production change is two methods renamed, one deleted, and five call sites. No `getProperty`, `setProperty`, `getAt` or `putAt` override. ### Why not the metaclass overrides in #16281 #16281 kept the accessors and overrode `getProperty`/`setProperty`/`getAt`/`putAt` on `AbstractTypeConvertingMap`. That cannot reach statically compiled call sites. In `StaticTypesCallSiteWriter`, `makeGetPropertyWithGetter(...)` is tried **before** `writeMapDotProperty(...)`, and the mutator loop runs before the `Map.put` branch — so `@CompileStatic` binds to the declared accessor and never consults the metaclass hooks. #16281 had to document that as a limitation. It also regressed `attrs.gspTagSyntaxCall = false` from "invoke the setter" (Grails 7) to "write a map entry". The same reasoning rules out an annotation plus AST transform: a local transform on the map class can only generate those same runtime hooks, and the static compiler at the *call site* still binds to the declared getter. It would add public API without fixing the static half. Thanks to @jdaugherty, whose review on #16281 established both of those points and redirected this to restoring the behaviour rather than documenting around it. ## API changes | Grails 7 | Grails 8 | |---|---| | `params.getRequest()` | `params.request()` | | `params.getIdentifier()` | `params.id` — it only ever read the `id` parameter | | `GroovyPageAttributes.isGspTagSyntaxCall()` | `gspTagSyntaxCall()` (internal class) | `getRequest()` gained a non-bean replacement rather than being deleted, because there is a real caller: `AbstractRequestBodyDataBindingSourceCreator` reads the request body from a `GrailsParameterMap` binding source. `getIdentifier()` had exactly one caller in the monorepo (`Controller.groovy`), now reading `GormProperties.IDENTITY` directly, so the `id`-only semantics are unchanged. `setGspTagSyntaxCall(boolean)` is deliberately retained. A write-only property reproduces Grails 7 exactly — the read addresses the map, the assignment invokes the setter — which avoids the regression #16281 introduced. `TagOutput` and `GroovyPage` rely on that setter. `AbstractTypeConvertingMap` gains a javadoc statement of the invariant so this does not recur: a subclass must not declare a no-argument `getX()`/`isX()`, because such an accessor cannot be worked around at runtime. ## Tests - `GrailsParameterMapTests` — 7 new cases: `identifier` and `request` via subscript and dotted syntax, values arriving from the request, absent keys reading `null`, `remove`/`clone`, `request()` still reachable, and a `@CompileStatic` helper pinning statically compiled read *and* write in both forms. - `GroovyPageAttributesTests` — 4 new cases covering the `attrs` side, including the static read and the setter-parity case. - `TypeConvertingMapTests` — one case at the base-class level. - `CommandObjectInstantiationSpec` — a request submitting **only** `identifier` (no `id`), so `SimpleMapDataBindingSource.getIdentifierValue()` returns null and the `Controller` fallback actually runs. This is the shape @jdaugherty asked for; the earlier version passed regardless of `getIdentifier()`. - `AbstractRequestBodyDataBindingSourceCreatorSpec` — covers the `GrailsParameterMap` branch that now calls `request()`. Every new test was mutation-checked. Reinstating the accessors makes `GrailsParameterMapTests` fail to **compile** with `Cannot set read-only property: identifier` — the exact static error #16281 documented — and with that one case removed so the file compiles, all 7 dynamic cases fail. Reinstating `isGspTagSyntaxCall()` fails the two `attrs` read cases. Pointing the `Controller` fallback at `identifier` fails the command-object spec on `model.commandObject == null`, the resolution itself. ### Build fix `grails-taglib` had no `junit-jupiter-engine` on its test runtime classpath. Its Spock specs ran (spock-core brings its own engine), but `GroovyPageAttributesTests` — the module's only plain JUnit 5 class — was never discovered. Added, module-local; the convention-level fix is tracked in #16289. ## Documentation `upgrading80x.adoc` §28.3 — the Groovy 5 change, the restored rule, and the two method renames. No caveat section is needed any more, because `params` is uniformly a map again in both compilation modes. -- 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]
