This is an automated email from the ASF dual-hosted git repository. lukaszlenart pushed a commit to branch WW-5659-lazy-params-request-scoping in repository https://gitbox.apache.org/repos/asf/struts.git
commit 83c52c9f077519d7f65ed0f7260568b9e31b6cfc Author: Lukasz Lenart <[email protected]> AuthorDate: Mon Jul 27 11:39:55 2026 +0200 WW-5659 test(core): exercise real lazy param resolution in dynamic upload tests --- .../interceptor/ActionFileUploadInterceptor.java | 13 +++++ .../ActionFileUploadInterceptorTest.java | 62 +++++++++++++++------- ...7-WW-5659-lazy-params-request-scoping-design.md | 26 ++++----- 3 files changed, 67 insertions(+), 34 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/interceptor/ActionFileUploadInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/ActionFileUploadInterceptor.java index 82ff798e3..c898275f0 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/ActionFileUploadInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/ActionFileUploadInterceptor.java @@ -198,6 +198,19 @@ import java.util.List; * } * </pre> * + * <p> + * Dynamic parameters are resolved into a fresh {@link UploadPolicy} for each invocation, so the + * interceptor itself is never modified per request and concurrent uploads cannot observe each + * other's policy. An expression that cannot be resolved does not relax validation: the policy is + * marked unresolved and affected uploads are rejected. A lazily resolved {@code disabled} param + * only takes effect if the interceptor's params holder extends {@link DisableParams} — there is + * deliberately no fallback to the interceptor instance. Likewise, an interceptor that overrides + * {@link ConditionalInterceptor#shouldIntercept(ActionInvocation) shouldIntercept} to read its own + * lazily-injected fields would see only config-time values, since resolution never touches the + * interceptor; {@code ActionFileUploadInterceptor} does not override {@code shouldIntercept}, so + * this does not affect it. + * </p> + * * @see WithLazyParams * @see UploadedFilesAware * @see AbstractFileUploadInterceptor diff --git a/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java b/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java index ab31c3d18..abdc12763 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java @@ -648,11 +648,10 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { ActionContext.getContext().getValueStack().push(action); ActionContext.getContext().withServletRequest(createMultipartRequestMaxFiles()); - // Simulate WithLazyParams injection by manually setting the parameters - // In real execution, DefaultActionInvocation.invoke() would call LazyParamInjector - interceptor.setAllowedTypes(action.getAllowedMimeTypes()); - - interceptor.intercept(mai); + // Exercise the real resolution path: LazyParamInjector resolves ${allowedMimeTypes} + // into a fresh UploadPolicy instead of mutating the shared interceptor. + UploadPolicy policy = injectDynamicUploadPolicy(interceptor, ActionContext.getContext(), true, false, false); + interceptor.intercept(mai, policy); List<UploadedFile> files = action.getUploadFiles(); @@ -686,8 +685,8 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { ActionContext.getContext().getValueStack().push(action1); ActionContext.getContext().withServletRequest(createMultipartRequestMaxFiles()); - interceptor.setAllowedTypes(action1.getAllowedMimeTypes()); - interceptor.intercept(mai1); + UploadPolicy policy1 = injectDynamicUploadPolicy(interceptor, ActionContext.getContext(), true, false, false); + interceptor.intercept(mai1, policy1); assertThat(action1.getUploadFiles()).isNotNull().hasSize(1); assertThat(action1.getUploadFiles().get(0).getContentType()).isEqualTo("text/plain"); @@ -715,8 +714,8 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { ActionContext.getContext().withServletRequest(createMultipartRequestMaxFiles()); // Simulate new parameter evaluation for second request - interceptor.setAllowedTypes(action2.getAllowedMimeTypes()); - interceptor.intercept(mai2); + UploadPolicy policy2 = injectDynamicUploadPolicy(interceptor, ActionContext.getContext(), true, false, false); + interceptor.intercept(mai2, policy2); assertThat(action2.getUploadFiles()).isNotNull().hasSize(1); assertThat(action2.getUploadFiles().get(0).getContentType()).isEqualTo("text/html"); @@ -746,8 +745,8 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { ActionContext.getContext().getValueStack().push(action); ActionContext.getContext().withServletRequest(createMultipartRequestMaxFiles()); - interceptor.setAllowedExtensions(action.getAllowedExtensions()); - interceptor.intercept(mai); + UploadPolicy policy = injectDynamicUploadPolicy(interceptor, ActionContext.getContext(), false, true, false); + interceptor.intercept(mai, policy); List<UploadedFile> files = action.getUploadFiles(); @@ -785,8 +784,8 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { ActionContext.getContext().getValueStack().push(action); ActionContext.getContext().withServletRequest(createMultipartRequestMaxFiles()); - interceptor.setMaximumSize(action.getMaxFileSize()); - interceptor.intercept(mai); + UploadPolicy policy = injectDynamicUploadPolicy(interceptor, ActionContext.getContext(), false, false, true); + interceptor.intercept(mai, policy); // File should be rejected due to size assertThat(action.hasFieldErrors()).isTrue(); @@ -819,9 +818,8 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { ActionContext.getContext().getValueStack().push(action); ActionContext.getContext().withServletRequest(createMultipartRequestMaxFiles()); - interceptor.setAllowedTypes(action.getAllowedMimeTypes()); - interceptor.setAllowedExtensions(action.getAllowedExtensions()); - interceptor.intercept(mai); + UploadPolicy policy = injectDynamicUploadPolicy(interceptor, ActionContext.getContext(), true, true, false); + interceptor.intercept(mai, policy); List<UploadedFile> files = action.getUploadFiles(); @@ -856,8 +854,8 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { ActionContext.getContext().getValueStack().push(action); ActionContext.getContext().withServletRequest(createMultipartRequestMaxFiles()); - interceptor.setAllowedTypes(action.getAllowedMimeTypes()); - interceptor.intercept(mai); + UploadPolicy policy = injectDynamicUploadPolicy(interceptor, ActionContext.getContext(), true, false, false); + interceptor.intercept(mai, policy); List<UploadedFile> files = action.getUploadFiles(); @@ -1074,6 +1072,34 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { } } + /** + * Resolves the given flags into a fresh {@link UploadPolicy} via the real + * {@link WithLazyParams.LazyParamInjector} path, using the already-bound {@code context} + * (and its ValueStack, with the action already pushed by the caller) rather than fabricating + * a new one. This mirrors what {@code DefaultActionInvocation} does at request time, so tests + * exercise the actual resolution instead of hand-calling a setter on the shared interceptor. + */ + private UploadPolicy injectDynamicUploadPolicy(ActionFileUploadInterceptor actionFileUploadInterceptor, + ActionContext context, + boolean includeAllowedTypes, + boolean includeAllowedExtensions, + boolean includeMaximumSize) { + Map<String, String> params = new HashMap<>(); + if (includeAllowedTypes) { + params.put("allowedTypes", "${allowedMimeTypes}"); + } + if (includeAllowedExtensions) { + params.put("allowedExtensions", "${allowedExtensions}"); + } + if (includeMaximumSize) { + params.put("maximumSize", "${maxFileSize}"); + } + + WithLazyParams.LazyParamInjector injector = new WithLazyParams.LazyParamInjector(context.getValueStack()); + container.inject(injector); + return injector.resolveInto(actionFileUploadInterceptor.newLazyParams(), params, context); + } + private String runUploadAttempt(ActionFileUploadInterceptor actionFileUploadInterceptor, MyDynamicFileUploadAction action, MockHttpServletRequest uploadRequest) throws Exception { diff --git a/docs/superpowers/specs/2026-07-27-WW-5659-lazy-params-request-scoping-design.md b/docs/superpowers/specs/2026-07-27-WW-5659-lazy-params-request-scoping-design.md index 20e8a7c96..397b79be3 100644 --- a/docs/superpowers/specs/2026-07-27-WW-5659-lazy-params-request-scoping-design.md +++ b/docs/superpowers/specs/2026-07-27-WW-5659-lazy-params-request-scoping-design.md @@ -290,19 +290,15 @@ Under this design: `unresolved(paramName)` is called. - A WARN is logged naming interceptor, param and expression. -`UploadPolicy.unresolved(param)` then has to decide whether the seeded value is usable. Two -cases exist, and they are distinguishable: - -- **A genuine static fallback.** The interceptor's own `<interceptor>` definition carried a - literal value for that param and the `<interceptor-ref>` overrode it with an expression, - so the seed is a real value (e.g. `image/png`). That value applies and validation - proceeds normally. -- **No fallback.** The only configuration for that param is the expression itself, so - `buildInterceptor` seeded the holder with the literal `${...}` text — as a set containing - the string `"${uploadConfig.allowedMimeTypes}"`, which matches no content type. - -The rule: `unresolved(param)` marks the dimension unusable **only if** the seeded value for -that dimension still contains `${`. Otherwise a genuine static fallback exists and is used. +`UploadPolicy.unresolved(param)` records the parameter and marks the whole policy unusable, +regardless of the seeded value. A static fallback therefore applies only when the param is absent +from the lazy map entirely, i.e. pure static configuration, which never triggers `unresolved`. + +The seed-introspection alternative — honouring a seeded value that does not itself contain +`${` — was rejected during planning: it is not implementable deterministically, because +`maximumSize` is seeded as a `Long` and cannot carry a `${...}` literal, so the rule would behave +differently per parameter type. + A dimension marked unusable causes `acceptFile` to reject the file with a dedicated message rather than the opaque one produced by matching content types against literal `${...}` text. This needs a new bundle key — `struts.messages.error.upload.policy.unresolved` — added to @@ -324,9 +320,7 @@ silently never runs. New tests must follow the existing style. `newLazyParams()` still returns the configured values — the singleton was never written. - `disabled` request-scoping: concurrent invocations resolving different `disabled` values; assert only the intended one is skipped. -- Fail-closed: unresolved expression with no static fallback → rejected with the new - message; unresolved *with* a static fallback (literal on the `<interceptor>` definition, - expression on the `<interceptor-ref>`) → the static value applies and validation proceeds. +- Fail-closed: unresolved expression → rejected with the new message. - `ConditionalInterceptor` interaction: a `WithLazyParams` interceptor that is also conditional still honours a custom `shouldIntercept`. - Migrate existing dynamic tests (`testDynamicParameterEvaluation` and friends) off the
