From ef5158ec2436c2b44742f1f9362f91c70c352922 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sun, 8 Nov 2026 10:03:57 +0100 Subject: [PATCH 1/3] WW-5849 docs: design a forward result to replace action chaining Co-Authored-By: Claude Opus 5.5 --- ...026-11-08-WW-5849-action-forward-design.md | 196 ++++++++++++++++++ 1 file changed, 196 insertions(+) create mode 100644 docs/superpowers/specs/2026-11-08-WW-5849-action-forward-design.md diff --git a/docs/superpowers/specs/2026-11-08-WW-5849-action-forward-design.md b/docs/superpowers/specs/2026-11-08-WW-5849-action-forward-design.md new file mode 100644 index 0000000000..50cf85d314 --- /dev/null +++ b/docs/superpowers/specs/2026-11-08-WW-5849-action-forward-design.md @@ -0,0 +1,196 @@ +# Action forward: a replacement for action chaining + +Date: 2026-11-08 · Status: draft, under discussion · Ticket: [WW-5849](https://issues.apache.org/jira/browse/WW-5849) + +## Why + +On 2026-10-25 the "Is anyone using action chaining?" thread on user@ asked whether chaining is still +in use, ahead of a possible deprecation. Several users replied that they rely on it, one of them with +a large configuration migrated from Struts 1, and James Chaplin gave the reason worth designing for: + +> it can be convenient to allow for a sort of "branching flow" where depending on state, the action +> result could chain/flow from one action to another directly (without having to make a round-trip +> back to the user's browser). I don't think any of the alternatives can achieve an equivalent result. + +He is right: `redirectAction` costs a round-trip, and "move the logic into a service" is a rewrite, +not a migration. So removing chaining without a replacement is off the table. + +But today's chaining does far more than James needs, and the extra reach is what makes it hard to +follow and to reason about. The thought experiment behind this design: *if Struts had no chaining, +what would we add to give James his server-side branch?* The answer is the `forward` result below. +`chain` is then deprecated in 7.x and removed in 8.0.0. + +## What chaining does today (the problem) + +Verified against `main` at `2f7666e98`. + +**Which actions can be chained to** +- Any action in any namespace, by name. `namespace`, `actionName` and `method` on + `ActionChainResult` go through `TextParseUtil.translateVariables` against the value stack, so the + target can be computed from data. `method` is still checked against `allowedMethods` + (`DefaultActionProxy.java:191`). +- The target cannot opt in or out. The only opt-out is source-side (`Unchainable`), implemented only + by `DefaultTextProvider`. +- Loop detection is an exact `namespace/action!method` history match; `skipActions` disables it per + action name, ignoring namespace. +- The Convention plugin chains implicitly: action `foo` returning `bar` with no configured result + chains to `foo-bar` in the same package (`ConventionUnknownHandler.java:310-318`). + +**How data passes — three implicit channels** +1. `ChainingInterceptor` copies every readable property of every non-`Unchainable` object in the + value stack onto the target action (opt-out via `includes`/`excludes`; opt-in annotation gating + via `struts.chaining.requireAnnotations`, default `false`, which reuses `@StrutsParameter`). +2. The value stack is shared: the source action stays in the stack below the target, so expressions + evaluated during the target can resolve against it. +3. The original HTTP parameters are passed on and the target's `params` interceptor binds them again, + after the copy. + +**Quirks** +- `ChainingInterceptor.shouldCopyStack` is true whenever `invocation.getResult()` is null, which is + every fresh invocation — not only chained ones. `` builds its stack from the page's + (`ActionComponent.java:187`), so the copy very likely runs for tag-invoked actions too (not yet + pinned by a test). +- In `defaultStack`, `prepare` runs before `chain`, so `prepare()` sees pre-copy state. +- Setting `excludes` drops the default `actionErrors`/`actionMessages`/`fieldErrors` excludes. + +## Goals + +- Server-side transfer from one action to another within the same request, chosen by the source + action's result code — James's "branching flow". +- Both boundaries explicit: **which actions can be forwarded to** (target opts in) and **what data + crosses** (named on both sides). +- A **mechanical** migration from `chain`: no action redesign, but each target gets the opt-in and + each result that needs data gets a hand-off list (see "Migration aid"). Not config-only — the + explicit boundaries are worth that cost. Many chains need no hand-off at all, + because request parameters re-bind. +- Ships in a 7.x minor, alongside `chain`, so users migrate before anything is deprecated. + +## Non-goals + +- Changing `chain`'s behaviour in 7.x. It keeps working exactly as today until removal. +- Fixing the `chain` quirks above. They go away with `chain` in 8.0.0. +- Replacing `redirectAction`, `dispatcher`, or ``. + +## Anti-goals (would make this a failure even if it works) + +- Any implicit data channel: property copying by name, a shared value stack, or values flowing in + because both sides happen to share a property name. +- A target computed from request data. +- An action becoming a forward target without its author choosing it. +- Reusing `@StrutsParameter` for the hand-off. HTTP input and framework hand-off are different trust + channels and must stay separately gated. + +## Design — decided + +### The `forward` result + +A new result type registered in `struts-default.xml`. Running it starts a new action invocation for +the target in the same request; the target's own result renders, as with `chain` today. + +1. **Static target.** `actionName`, `namespace`, and `method` are literals from configuration — no + `${...}` evaluation. Branching is done with result codes, one `forward` result per branch. +2. **Target opts in — a target-side allowlist.** An action can only be forwarded to if it is marked + as a forward target; everything else is denied by default, the same model as `@StrutsParameter` + for request parameters. Two equivalent forms, both recorded on `ActionConfig`: + - a class-level annotation (working name `@Forwardable`) that is **not** `@Inherited`, so a + `BaseAction` cannot opt in every subclass; + - an attribute on `` in XML, for classes the app does not own or annotate. + + No marker interface (inherited, so one `implements` on a base class reopens every action) and no + negative marker such as `NotForwardable` (absence already means "not forwardable"; a deny marker + is the `Unchainable` pattern this design leaves behind). The allowlist names targets, not + source→target pairs; restricting sources (e.g. `@Forwardable(from = …)`) is a possible later + extension, out of scope now. Rationale for per-class opt-in even with explicit hand-off: a + forward bypasses everything mapped to the target's URL (`web.xml` security constraints, servlet + filters, container auth); only the target's interceptors run. + + Forwarding to an unmarked action throws at run time; see "left to the builder" for config-time + validation. +3. **Fresh value stack and own context.** The target gets a new value stack (the source action is not + in it) and its own `ActionContext`: fresh conversion errors, its own `ActionInvocation`, the same + session, application, locale and request parameters. The target's result renders against the + target's stack only. +4. **Request parameters are re-bound** like a servlet forward: the target's `params` interceptor binds + the original HTTP parameters under the target's own `@StrutsParameter` rules, exactly as for a + direct request. +5. **Explicit hand-off, named on both sides.** The source's `forward` result lists the values it + passes, each evaluated against the source action. The target declares which properties accept a + forwarded value, with a dedicated annotation (not `@StrutsParameter`). Only values named on both + sides are set; a value the source passes but the target does not accept is dropped and logged. +6. **Hand-off is applied after `params`**, by a dedicated interceptor placed directly after `params` + in every shipped stack (`defaultStack`, `paramsPrepareParamsStack` after the second `params`, and + the REST and Bean Validation plugin stacks). When both channels set the same property, the + server-side hand-off wins. If a forward carries hand-off values and the target's stack lacks that + interceptor, the forward throws rather than silently dropping them. + - The accepting annotation covers **top-level setters on the action only** in this version: no + nested paths, no ModelDriven model. A value that fails type conversion is dropped and logged at + WARN. This keeps the new channel narrower than `@StrutsParameter`; it can be widened + later if users need it. +7. **Action errors, field errors and messages do not carry over** unless passed explicitly like any + other value. +8. **Depth limit, not name-history matching.** A constant caps nested forwards per request (counting + `forward` only, not `` or redirects); default 5; exceeding it throws `StrutsException`. + No `skipActions` equivalent. +9. **Only explicit triggers.** No interceptor copies data, and `` is unaffected. +10. **The target runs its full interceptor stack**, including security and HTTP-method interceptors, + which see the original request. + +### Convention plugin + +The implicit `foo` → `foo-bar` transfer becomes an **implicit `forward`**, applied only when `foo-bar` +has opted in as a forward target. If `foo-bar` exists but has not opted in, Convention does not +forward (it keeps looking for a result as it would if `foo-bar` didn't exist). Convention reads the +opt-in from `ActionConfig`. + +The implicit forward carries **no hand-off list**, so a Convention flow that relied on the chain copy +gets only request parameters. That is an accepted, documented behaviour change; Convention users add +the opt-in and, where they need data, an explicit `forward` result (Convention's `@Result` accepts +`forward` and its params). + +### Migration aid + +During the deprecation phase (rollout step 2), `chain` logs — once per chain, at a level users can +enable without noise — which properties `ChainingInterceptor` actually copied onto which target. +Users derive each `forward` result's hand-off list from real traffic instead of reading code. The +migration guide walks through: opt in the target, add the hand-off list from the log, drop the +`chain` interceptor reference. + +### Rollout + +1. **7.6.0** — ship `forward` with a new `struts-7.6.dtd` (the only grammar change is the opt-in + attribute on ``; `struts-7.5.dtd` is frozen once 7.5.0 is released; apps that opt in via + XML switch their DOCTYPE), the target opt-in, the hand-off annotation, the depth constant, + Convention's implicit forward, docs (struts-site), and a `chain` → `forward` migration guide. + `chain` unchanged. +2. **7.7.0** — deprecate `ActionChainResult`, `ChainingInterceptor`, and `Unchainable` with + `@Deprecated(since = "…", forRemoval = true)`; deprecate the `struts.chaining.*` constants; file a + WW Task at fix version 8.0.0 for removal. One release after `forward` ships, so early adopters can + report problems before everyone sees deprecation warnings. +3. **8.0.0** — remove `chain` and the `chain` interceptor from `struts-default.xml` (`chainStack`, + `paramsPrepareParamsStack`, `defaultStack`) and from the REST and Bean Validation plugin stacks; + remove the `ActionChainResult` unwinding loop in `DefaultActionInvocation.getResult()` and its + contract in `ActionInvocation`; remove the `struts.chaining.*` constants, including + `AliasInterceptor`'s use of `struts.chaining.requireAnnotations` (decide its replacement then). + Convention's implicit chain path goes too. + +Each step is its own WW ticket; step 1 is WW-5849. + +## Constraints + +- Struts code style and process: `Struts*` prefix for default implementations, no `XWork*` names, + no boolean flag arguments, `@Deprecated(since, forRemoval = true)` plus a removal ticket. +- Tests are JUnit 3 (`XWorkTestCase`) or JUnit 4; match the file being extended. No JUnit 5. +- Must not depend on the REST plugin's optional XML dependency or any optional module. + +## Left to the builder + +- Names: the result type (`forward` is the working name; note the existing `dispatcher` result is a + servlet forward to a resource, so docs must distinguish them), the target opt-in marker, the + hand-off annotation, the result param that lists passed values, and the depth constant and its + default. +- Whether forwarding to a non-opted-in target can also be caught at configuration time. +- How hand-off values are carried between invocations (an `ActionContext` entry is the obvious place) + and how type conversion applies to them. +- Whether the forward result reuses `ActionProxyFactory.createActionProxy` and how it creates the + fresh value stack. +- Interaction with `execAndWait`, `token`/`tokenSession`, and `store` interceptors in the target. From c819fce46e5731b0ff4c71eececc1bde98e233ab Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sun, 8 Nov 2026 10:09:43 +0100 Subject: [PATCH 2/3] WW-5849 docs: detect forward cycles at startup Static forward targets make the forward graph known from configuration, so cycles can be reported when the configuration is built instead of on a user's request. The per-request depth limit stays as the runtime backstop. A per-request time budget was considered instead of the depth limit and rejected: nested forwards overflow the stack long before any useful budget expires, a running action cannot be safely aborted, and a load-dependent limit fails under load and cannot be pinned by a test. Co-Authored-By: Claude Opus 5.5 --- .../specs/2026-11-08-WW-5849-action-forward-design.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/superpowers/specs/2026-11-08-WW-5849-action-forward-design.md b/docs/superpowers/specs/2026-11-08-WW-5849-action-forward-design.md index 50cf85d314..9e8191d75d 100644 --- a/docs/superpowers/specs/2026-11-08-WW-5849-action-forward-design.md +++ b/docs/superpowers/specs/2026-11-08-WW-5849-action-forward-design.md @@ -131,6 +131,11 @@ the target in the same request; the target's own result renders, as with `chain` 8. **Depth limit, not name-history matching.** A constant caps nested forwards per request (counting `forward` only, not `` or redirects); default 5; exceeding it throws `StrutsException`. No `skipActions` equivalent. + - **Cycle check at startup.** Because targets are static, the configuration defines the full + forward graph (explicit `forward` results plus Convention's implicit forwards). When the + configuration is built, cycles in that graph are detected and logged as a warning naming the + actions involved; in devMode they fail startup. Runtime result codes are unknown at startup, + so the depth limit stays as the runtime backstop. 9. **Only explicit triggers.** No interceptor copies data, and `` is unaffected. 10. **The target runs its full interceptor stack**, including security and HTTP-method interceptors, which see the original request. From 9a9d578adca593b3d25ed491b317b7aaa977548b Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sun, 8 Nov 2026 10:29:02 +0100 Subject: [PATCH 3/3] WW-5849 docs: log unopted chain targets as a migration aid During the 7.7.0 deprecation phase, chain also logs each target that has not opted in as a forward target, next to the copied properties, so users know which actions to mark before switching. Evolving chain in place towards the forward design (keeping the result type name and flipping its behaviour step by step) was considered and rejected. The per-action work for users is the same in both approaches; in-place evolution saves only the type="chain" -> type="forward" rename, at the cost of several mode constants and their test matrix until 8.0.0, a breaking change to struts.chaining.requireAnnotations (shared with AliasInterceptor), a value-stack isolation step that cannot be phased, and a result type whose meaning changes across releases. Co-Authored-By: Claude Opus 5.5 --- .../2026-11-08-WW-5849-action-forward-design.md | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/docs/superpowers/specs/2026-11-08-WW-5849-action-forward-design.md b/docs/superpowers/specs/2026-11-08-WW-5849-action-forward-design.md index 9e8191d75d..7e56afed2f 100644 --- a/docs/superpowers/specs/2026-11-08-WW-5849-action-forward-design.md +++ b/docs/superpowers/specs/2026-11-08-WW-5849-action-forward-design.md @@ -155,10 +155,16 @@ the opt-in and, where they need data, an explicit `forward` result (Convention's ### Migration aid During the deprecation phase (rollout step 2), `chain` logs — once per chain, at a level users can -enable without noise — which properties `ChainingInterceptor` actually copied onto which target. -Users derive each `forward` result's hand-off list from real traffic instead of reading code. The -migration guide walks through: opt in the target, add the hand-off list from the log, drop the -`chain` interceptor reference. +enable without noise — two things: + +- which properties `ChainingInterceptor` actually copied onto which target, so users derive each + `forward` result's hand-off list from real traffic instead of reading code; +- each chain target that has not opted in as a forward target, so users know which actions need the + opt-in before switching. + +`chain` itself does not change behaviour in 7.x; the logs are the only addition. The migration guide +walks through: opt in the targets the log names, add the hand-off list from the log, change +`type="chain"` to `type="forward"`, drop the `chain` interceptor reference. ### Rollout