sbglasius commented on PR #16296: URL: https://github.com/apache/grails-core/pull/16296#issuecomment-5597317106
@bkoehm @jdaugherty — `60598510cf` addresses James's review (five inline replies above). Two things for Brian, and then the data behind the "is this Grails-only behaviour" question, since you are both circling it. **Brian, on your r3925544370 — the "not mutated" case is already in.** It landed in `2e08c1fc15`, before your comment; I think it just got lost in the noise. Both specs carry a guard feature (*marshalling does not widen access on the shared property descriptor cache*) that reads the cached descriptor back after marshalling and fails if the flag was flipped — I verified it bites by temporarily restoring `makeAccessible`. So nothing is pending from you there. The cost is also lower than you generously accepted. It is not one extra lookup per property per marshal — only the path that would otherwise mutate pays it, and after `60598510cf` that path is narrower again: a getter now has to have no public type *anywhere* in its hierarchy before anything is widened. **Matrei's bullet 1, in plain terms:** to read `getUsername()` on an anonymous `UserDetails`, the marshaller walks to the same method as declared by the public `UserDetails` interface and invokes that. Java dispatches it to the anonymous class's override anyway, so the value is right and nothing is modified. Only when there is no publicly reachable declaration does it take a throwaway copy of the method, widen the copy, and invoke that — leaving the shared one untouched. That is the whole mechanism. **On whether this is Grails inventing behaviour.** I probed it rather than argue it. Same source, two compilers: | | Groovy 4.0.33 | Groovy 5.1.1 | |---|---|---| | anonymous class is public | **yes** | **no** | | `thing.name` via the metaclass | works | **works** | | `thing.getName()` via the metaclass | works | **works** | | raw `Method.invoke` | works | **IllegalAccessException** | | JDK `Introspector` | works | **works** — resolves to the interface | So ordinary Groovy is not broken by the change; only raw `Method.invoke` against the declaring class is, which is what our marshallers were doing. Groovy's own MOP, the JDK's `Introspector`, and Spring's `BeanWrapperImpl$BeanPropertyHandler` (which calls `ReflectionUtils.makeAccessible` on the cached read method — confirmed in the 7.0.9 bytecode) all handle this case. This PR does the same thing, more conservatively than Spring does, since we widen a copy rather than the cached member. James, that is also the answer to your question about bullet 1: **Java-based code is not already working.** Pre-fix, the plain-Java fixtures in this PR — an anonymous implementation, a package-private class, and a package-private class with no interface — all threw `IllegalAccessException` from `GenericJavaBeanMarshaller`, because javac has always compiled anonymous classes as non-public. Bullet 1 was only ever about the *field* loop, where Java skipped a public field that Groovy emitted; this PR aligned that. So Groovy 5 moved *towards* javac here rather than away from it, and the getter path was latently broken for Java beans on every branch. The warning stays as the removal seam you asked for. -- 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]
