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 73c7ec350e3cda786c541fc71d3c0e4ac6c8b24d Author: Lukasz Lenart <[email protected]> AuthorDate: Mon Jul 27 11:23:16 2026 +0200 WW-5659 test(core): cover both lazy-params skip branches and per-invocation disabled The two skip branches in DefaultActionInvocation#invokeWithLazyParams had no coverage: deleting either left the whole suite green. The only tests reaching that method used LazyFoo/LazyFooWithStackParams, which declare no disabled param, and MockLazyParams did not extend DisableParams, so the holder branch was unreachable and shouldIntercept was always true. Make MockLazyParams extend DisableParams and add two action configs that isolate one branch each: - LazyFooLazilyDisabled passes disabled as an interceptor-ref param, so it reaches InterceptorMapping#getParams(), resolves onto the holder, and exercises the DisableParams branch. - LazyFooStaticallyDisabled sets disabled on the interceptor definition instead. InterceptorBuilder only puts interceptor-ref params into the mapping, so the holder never sees it and it can only be honoured through ConditionalInterceptor#shouldIntercept. Verified by deleting each branch in turn: each deletion fails exactly the one test that targets it, and no other. Also make testDisabledIsResolvedPerInvocation earn its name. It previously asserted only that newLazyParams() returns a fresh object, never resolving anything, and built a MyDynamicFileUploadAction it never used. It now routes two actions through a real LazyParamInjector#resolveInto of a disabled=${uploadDisabled} param and pins that one invocation's resolved flag survives the other's, and that neither reaches the interceptor singleton. --- .../struts2/DefaultActionInvocationTest.java | 53 ++++++++++++++++++++++ .../ActionFileUploadInterceptorTest.java | 47 ++++++++++++++++--- .../apache/struts2/mock/MockLazyInterceptor.java | 7 +-- core/src/test/resources/xwork-sample.xml | 21 +++++++++ core/src/test/resources/xwork-test-default.xml | 5 ++ 5 files changed, 124 insertions(+), 9 deletions(-) diff --git a/core/src/test/java/org/apache/struts2/DefaultActionInvocationTest.java b/core/src/test/java/org/apache/struts2/DefaultActionInvocationTest.java index 6a8e9fc21..398fa7f77 100644 --- a/core/src/test/java/org/apache/struts2/DefaultActionInvocationTest.java +++ b/core/src/test/java/org/apache/struts2/DefaultActionInvocationTest.java @@ -42,6 +42,7 @@ import java.util.concurrent.Semaphore; import java.util.concurrent.TimeUnit; import static org.apache.struts2.ognl.OgnlUtilTest.createOgnlUtil; +import static org.assertj.core.api.Assertions.assertThat; /** @@ -423,6 +424,58 @@ public class DefaultActionInvocationTest extends XWorkTestCase { assertEquals("static value", action.getBlah()); } + /** + * Regression for WW-5659: a {@code disabled} param resolved lazily from the value stack must skip + * the interceptor for that invocation. It arrives through the interceptor mapping's params, so it + * lands on the per-invocation holder and is honoured there, never on the shared interceptor. + */ + public void testInvokeWithLazyParamsSkipsLazilyDisabledInterceptor() throws Exception { + HashMap<String, Object> params = new HashMap<>(); + params.put("blah", "true"); + + ActionContext extraContext = ActionContext.of() + .withParameters(HttpParameters.create(params).build()); + + DefaultActionInvocation defaultActionInvocation = new DefaultActionInvocation(extraContext.getContextMap(), true); + container.inject(defaultActionInvocation); + + ActionProxy actionProxy = actionProxyFactory.createActionProxy("", "LazyFooLazilyDisabled", null, extraContext.getContextMap()); + defaultActionInvocation.init(actionProxy); + defaultActionInvocation.invoke(); + + SimpleAction action = (SimpleAction) defaultActionInvocation.getAction(); + + // the rest of the stack still ran, so the params interceptor applied blah... + assertThat(action.getBlah()).isEqualTo("true"); + // ...but the lazy interceptor was skipped, so it never applied its foo param to the action + assertThat(action.getName()).isNull(); + } + + /** + * Regression for WW-5659: {@code disabled} configured on the interceptor definition never reaches + * the params holder, so it can only be honoured through {@link org.apache.struts2.interceptor.ConditionalInterceptor#shouldIntercept}. + * The lazy path must still consult it. + */ + public void testInvokeWithLazyParamsSkipsStaticallyDisabledInterceptor() throws Exception { + HashMap<String, Object> params = new HashMap<>(); + params.put("blah", "dynamic value"); + + ActionContext extraContext = ActionContext.of() + .withParameters(HttpParameters.create(params).build()); + + DefaultActionInvocation defaultActionInvocation = new DefaultActionInvocation(extraContext.getContextMap(), true); + container.inject(defaultActionInvocation); + + ActionProxy actionProxy = actionProxyFactory.createActionProxy("", "LazyFooStaticallyDisabled", null, extraContext.getContextMap()); + defaultActionInvocation.init(actionProxy); + defaultActionInvocation.invoke(); + + SimpleAction action = (SimpleAction) defaultActionInvocation.getAction(); + + assertThat(action.getBlah()).isEqualTo("dynamic value"); + assertThat(action.getName()).isNull(); + } + public void testInvokeWithAsyncManager() throws Exception { DefaultActionInvocation dai = new DefaultActionInvocation(new HashMap<>(), false); dai.stack = container.getInstance(ValueStackFactory.class).createValueStack(); 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 c2cf40719..e9bca76cc 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java @@ -888,6 +888,7 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { private String allowedMimeTypes; private String allowedExtensions; private Long maxFileSize; + private String uploadDisabled; @Override public void withUploadedFiles(List<UploadedFile> uploadedFiles) { @@ -921,6 +922,14 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { public void setMaxFileSize(Long maxFileSize) { this.maxFileSize = maxFileSize; } + + public String getUploadDisabled() { + return uploadDisabled; + } + + public void setUploadDisabled(String uploadDisabled) { + this.uploadDisabled = uploadDisabled; + } } public void testUploadPolicyParsesAndCopies() { @@ -1028,17 +1037,43 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { ActionFileUploadInterceptor interceptor = new ActionFileUploadInterceptor(); container.inject(interceptor); - MyDynamicFileUploadAction action = new MyDynamicFileUploadAction(); - action.setAllowedMimeTypes("text/plain"); - container.inject(action); + MyDynamicFileUploadAction disablingAction = new MyDynamicFileUploadAction(); + disablingAction.setUploadDisabled("true"); + container.inject(disablingAction); - UploadPolicy policy = interceptor.newLazyParams(); - policy.setDisabled("true"); + MyDynamicFileUploadAction enablingAction = new MyDynamicFileUploadAction(); + enablingAction.setUploadDisabled("false"); + container.inject(enablingAction); + + UploadPolicy disabledPolicy = resolveDisabled(interceptor, disablingAction); + UploadPolicy enabledPolicy = resolveDisabled(interceptor, enablingAction); - assertThat(policy.isDisabled()).isTrue(); + // the second resolution must not have cleared the first invocation's flag... + assertThat(disabledPolicy.isDisabled()).isTrue(); + assertThat(enabledPolicy.isDisabled()).isFalse(); + // ...and neither resolution may reach the shared interceptor assertThat(interceptor.newLazyParams().isDisabled()).isFalse(); } + private UploadPolicy resolveDisabled(ActionFileUploadInterceptor actionFileUploadInterceptor, + MyDynamicFileUploadAction action) { + ValueStack valueStack = container.getInstance(ValueStackFactory.class).createValueStack(); + valueStack.push(action); + + ActionContext context = ActionContext.of(valueStack.getContext()) + .withContainer(container) + .withValueStack(valueStack) + .bind(); + try { + WithLazyParams.LazyParamInjector injector = new WithLazyParams.LazyParamInjector(valueStack); + container.inject(injector); + return injector.resolveInto(actionFileUploadInterceptor.newLazyParams(), + Map.of("disabled", "${uploadDisabled}"), context); + } finally { + ActionContext.clear(); + } + } + private String runUploadAttempt(ActionFileUploadInterceptor actionFileUploadInterceptor, MyDynamicFileUploadAction action, MockHttpServletRequest uploadRequest) throws Exception { diff --git a/core/src/test/java/org/apache/struts2/mock/MockLazyInterceptor.java b/core/src/test/java/org/apache/struts2/mock/MockLazyInterceptor.java index 75d3d021a..f875c043c 100644 --- a/core/src/test/java/org/apache/struts2/mock/MockLazyInterceptor.java +++ b/core/src/test/java/org/apache/struts2/mock/MockLazyInterceptor.java @@ -21,15 +21,16 @@ package org.apache.struts2.mock; import org.apache.struts2.ActionInvocation; import org.apache.struts2.SimpleAction; import org.apache.struts2.interceptor.AbstractInterceptor; -import org.apache.struts2.interceptor.InterceptorParams; +import org.apache.struts2.interceptor.DisableParams; import org.apache.struts2.interceptor.WithLazyParams; public class MockLazyInterceptor extends AbstractInterceptor implements WithLazyParams<MockLazyInterceptor.MockLazyParams> { /** - * Per-invocation holder, seeded from the configured values. + * Per-invocation holder, seeded from the configured values. Extends {@link DisableParams} so a + * lazily resolved {@code disabled} param applies to a single invocation. */ - public static class MockLazyParams implements InterceptorParams { + public static class MockLazyParams extends DisableParams { private String foo = ""; private String bar = ""; diff --git a/core/src/test/resources/xwork-sample.xml b/core/src/test/resources/xwork-sample.xml index 5bc189d82..5615e8ef2 100644 --- a/core/src/test/resources/xwork-sample.xml +++ b/core/src/test/resources/xwork-sample.xml @@ -72,6 +72,27 @@ </interceptor-ref> </action> + <!-- disabled resolved lazily per invocation: reaches the interceptor mapping's params, + so it lands on the params holder and not on the shared interceptor --> + <action name="LazyFooLazilyDisabled" class="org.apache.struts2.SimpleAction"> + <result name="error" type="void"/> + <interceptor-ref name="params"/> + <interceptor-ref name="lazy"> + <param name="foo">should not be applied</param> + <param name="disabled">${blah}</param> + </interceptor-ref> + </action> + + <!-- disabled configured statically on the interceptor definition: invisible to the params + holder, honoured only via ConditionalInterceptor#shouldIntercept --> + <action name="LazyFooStaticallyDisabled" class="org.apache.struts2.SimpleAction"> + <result name="error" type="void"/> + <interceptor-ref name="params"/> + <interceptor-ref name="lazyStaticallyDisabled"> + <param name="foo">should not be applied</param> + </interceptor-ref> + </action> + <action name="WildCard" class="org.apache.struts2.SimpleAction"> <param name="foo">17</param> <param name="bar">23</param> diff --git a/core/src/test/resources/xwork-test-default.xml b/core/src/test/resources/xwork-test-default.xml index 31f92d9d6..87931c7d2 100644 --- a/core/src/test/resources/xwork-test-default.xml +++ b/core/src/test/resources/xwork-test-default.xml @@ -42,6 +42,11 @@ <param name="foo">expectedFoo</param> </interceptor> <interceptor name="lazy" class="org.apache.struts2.mock.MockLazyInterceptor"/> + <!-- disabled at interceptor-definition level, so it never reaches InterceptorMapping#getParams() + and is only visible through ConditionalInterceptor#shouldIntercept --> + <interceptor name="lazyStaticallyDisabled" class="org.apache.struts2.mock.MockLazyInterceptor"> + <param name="disabled">true</param> + </interceptor> <interceptor-stack name="defaultStack"> <interceptor-ref name="staticParams"/>
