jamesfredley commented on PR #15557:
URL: https://github.com/apache/grails-core/pull/15557#issuecomment-4604383973
## Controller action parameter scope under `indy=false`: approaches evaluated
For the record, here is the full set of approaches we identified for the
parameterized-controller-action `MissingPropertyException` under
`-PgrailsIndy=false`, what was tried, and what was not.
**Confirmed root cause:** for a parameterized action,
`ControllerActionTransformer` generates a zero-arg dispatch wrapper that binds
the request params to **local variables** and delegates via `this.action(p1,
p2)`. Groovy 5's `OptimizingStatementWriter` compiles that delegating call as a
`__$stMC` fast/slow pair; the **slow path** - taken whenever the controller
metaclass is non-standard, i.e. in any live app (Spring Security, plugins, ...)
- re-resolves the bound locals as dynamic property reads
(`this.getProperty("p1")`), throwing `MissingPropertyException`. The fast path
(and any isolated script, which keeps the standard metaclass) loads the locals
correctly, which is why this only failed in a running app. (The earlier
hypothesis that the `TryCatchStatement` re-parenting / `VariableScopeVisitor`
not rebinding was responsible turned out to be incorrect.)
### Tried
| # | Approach | Result |
|---|---|---|
| T1 | Reuse the original method's `BlockStatement` as the try-body when
exception-wrapping (avoid re-parenting a fresh block) | Fails - re-parenting
was never the cause |
| T11 | Don't exception-wrap the parameterized original; only wrap the
generated zero-arg dispatch wrapper | Fails - the defect is in the wrapper's
delegating call, not the original method |
| T12 | Drop `setMethodTarget(...)` on the `this.action(...)` delegating
call | Fails - the `__$stMC` fork is still emitted and the slow path still uses
`getProperty` |
| T13 | Re-run `VariableScopeVisitor` (`processVariableScopes`) on the
generated wrapper | Fails - classic codegen resolves variables via
`CompileStack`, not the AST scope |
| **T14** | **Tag the controller `ClassNode` with
`OptimizingStatementWriter.ClassNodeSkip`** to suppress the fast/slow fork |
**Works** - a single correct path is emitted; all 5 re-enabled
`grails-test-examples` modules pass `integrationTest` under both `indy=false`
and `indy=true`, and the `:grails-controllers:test` unit suite passes |
### Identified but not tried
| Approach | Why not |
|---|---|
| Set `accessedVariable` on the delegating-call argument
`VariableExpression`s | Classic `AsmClassGenerator` ignores `accessedVariable`
(it resolves by name via `CompileStack`), so it cannot help - the same reason
T13 fails |
| Per-statement opt-out via
`OptimizingStatementWriter.StatementMeta(optimize=false)` | Not a usable API
surface: `StatementMeta.optimize` is private and `OptVisitor.addMeta` forces it
back to `true` |
| Inline the original action body into the wrapper instead of delegating |
Higher risk - returns, exception wrapping, parameter scope, and command-object
behaviour would all become the transformer's responsibility |
| `@CompileStatic` the generated wrapper | The request-binding logic
(`params.int(...)`, etc.) is not statically type-checkable |
| Force the fast path globally (`disabledStandardMetaClass` / set `__$stMC`)
| Global side effects; not a per-controller, production-safe option |
**Chosen: T14.** It is the minimal, public, per-controller switch -
`ClassNodeSkip` is a documented Groovy API and
`OptimizingStatementWriter.setNodeMeta(...)` returns early when it is present -
and it is a workaround for the upstream Groovy defect rather than a fix of it,
to be removed once `OptimizingStatementWriter` is fixed upstream. Standalone
reproducer:
https://github.com/jamesfredley/groovy5-controller-action-param-scope-bug
--
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]
