sbglasius commented on PR #16281: URL: https://github.com/apache/grails-core/pull/16281#issuecomment-5495489484
Thanks @jdaugherty — this was a genuinely useful review. All three points addressed in 1d0096c, and I verified each independently first. No production code changed. **1. `attrs` was the uncovered half — and the finding goes further than stated.** Probing a replica with and without this PR's fix, on Groovy 4.0.30 and 5.0.6: ``` without the fix: dotted and subscript assign -> setGspTagSyntaxCall() called, map empty (both Groovy versions) with the fix: dotted and subscript assign -> map entry written, flag untouched (both Groovy versions) ``` So the setter-backed write change is caused by **this PR**, not by Groovy 5 — it is a second Grails 7 delta alongside the `metaClass` read, and it was undocumented. Now covered in §28.3 and pinned by `testWritingAnAttributeNamedAfterASetterNoLongerInvokesThatSetter`. For the static case I took the "document and test the actual behavior" option you suggested: `testStaticCompilationWritesTheFieldRatherThanTheMap` asserts the field is written and the map is not, and that the subscript form does the reverse. Both directions assert, so neither half is vacuous. **2. You are right that my controller test did not exercise the mechanism, and my earlier claim that it did was wrong.** `SimpleMapDataBindingSource.getIdentifierValue()` is literally `map['id']`, so submitting an `id` meant the fallback never ran; the only failing condition under your mutation was the assertion duplicating the unit test. Rewritten to submit *only* an `identifier` parameter with no `id`, which forces the fallback. Mutation-checked: with `getIdentifier()` reverted to prefer `identifier`, it now fails on ``` Condition not satisfied: model.commandObject == null [commandObject:org.grails.web.commandobjects.DomainClassCommandObject : 1] ``` — the resolution itself. A second test keeps the `id` + `identifier` case to show `id` wins. Worth noting this also means the originally-cited `?id=1&identifier=2` scenario was never exploitable, since `getIdentifierValue()` short-circuits on `id`. The real exposure was `?identifier=2` with no `id` at all. The revert was still correct. **3. Description updated.** It now states that `getIdentifier()` reads only `id` and explains the `0093ab2` revert. **Docs**: the §28.3 NOTE now covers both maps and names the silent `attrs` case; "every other parameter name is unaffected" is corrected to reads only; the `metaClass` paragraph gains the write side (`params.metaClass = 'mc'` throws `GroovyCastException`, `params['metaClass'] = 'mc'` stores an entry), pinned by an assertion. **Nit**: taken, now `assertTrue theMap.metaClass instanceof MetaClass`. **Follow-ups filed** rather than widened into this PR: - #16289 — the `gradle/test-config.gradle` convention never adds the Jupiter engine, so the silent-zero-tests failure can recur anywhere the transitive edge through `grails-testing-support-core` is narrowed. Your framing. - #16290 — `LazyMetaPropertyMap`, `DirtyCheckingMap`, `JSONObject`. Re-verified: `:grails-web-common:test`, `:grails-core:test`, `:grails-taglib:test`, `:grails-test-suite-web:test`, `:grails-controllers:test` all `--rerun-tasks`, and `aggregateViolations --rerun-tasks` clean on all four analysers. -- 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]
