From 24e60416f69e972579d8f17563a41814af15fba5 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sat, 9 Nov 2024 10:48:02 +0100 Subject: [PATCH 1/3] WW-5486 Fixes exposing params added by ServletDispatcherResult --- .../org/apache/struts2/ActionContext.java | 2 ++ .../apache/struts2/dispatcher/Dispatcher.java | 1 - .../result/ServletDispatcherResultTest.java | 28 +++++++++++-------- 3 files changed, 19 insertions(+), 12 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/ActionContext.java b/core/src/main/java/org/apache/struts2/ActionContext.java index febedd04fa..bad8f67057 100644 --- a/core/src/main/java/org/apache/struts2/ActionContext.java +++ b/core/src/main/java/org/apache/struts2/ActionContext.java @@ -24,6 +24,7 @@ import jakarta.servlet.jsp.PageContext; import org.apache.struts2.action.Action; import org.apache.struts2.conversion.impl.ConversionData; +import org.apache.struts2.dispatcher.DispatcherConstants; import org.apache.struts2.dispatcher.HttpParameters; import org.apache.struts2.dispatcher.mapper.ActionMapping; import org.apache.struts2.inject.Container; @@ -316,6 +317,7 @@ public String getActionName() { */ public ActionContext withParameters(HttpParameters parameters) { put(PARAMETERS, parameters); + put(DispatcherConstants.PARAMETERS, parameters); return this; } diff --git a/core/src/main/java/org/apache/struts2/dispatcher/Dispatcher.java b/core/src/main/java/org/apache/struts2/dispatcher/Dispatcher.java index deab70ce11..cb85639d1b 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/Dispatcher.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/Dispatcher.java @@ -851,7 +851,6 @@ public Map createContextMap(Map requestMap, .with(DispatcherConstants.REQUEST, requestMap) .with(DispatcherConstants.SESSION, sessionMap) .with(DispatcherConstants.APPLICATION, applicationMap) - .with(DispatcherConstants.PARAMETERS, parameters) .getContextMap(); AttributeMap attrMap = new AttributeMap(extraContext); diff --git a/core/src/test/java/org/apache/struts2/result/ServletDispatcherResultTest.java b/core/src/test/java/org/apache/struts2/result/ServletDispatcherResultTest.java index 219f3d2180..22a6312ea1 100644 --- a/core/src/test/java/org/apache/struts2/result/ServletDispatcherResultTest.java +++ b/core/src/test/java/org/apache/struts2/result/ServletDispatcherResultTest.java @@ -20,16 +20,19 @@ import com.mockobjects.dynamic.C; import com.mockobjects.dynamic.Mock; +import jakarta.servlet.RequestDispatcher; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; import org.apache.struts2.ActionContext; -import org.apache.struts2.mock.MockActionInvocation; -import org.apache.struts2.util.ValueStackFactory; import org.apache.struts2.ServletActionContext; import org.apache.struts2.StrutsInternalTestCase; import org.apache.struts2.StrutsStatics; +import org.apache.struts2.dispatcher.DispatcherConstants; +import org.apache.struts2.dispatcher.Parameter; +import org.apache.struts2.mock.MockActionInvocation; +import org.apache.struts2.util.ValueStackFactory; -import jakarta.servlet.RequestDispatcher; -import jakarta.servlet.http.HttpServletRequest; -import jakarta.servlet.http.HttpServletResponse; +import java.util.Map; public class ServletDispatcherResultTest extends StrutsInternalTestCase implements StrutsStatics { @@ -53,8 +56,7 @@ public void testInclude() { try { view.execute(null); } catch (Exception e) { - e.printStackTrace(); - fail(); + fail(e.getMessage()); } dispatcherMock.verify(); @@ -86,8 +88,7 @@ public void testSimple() { try { view.execute(null); } catch (Exception e) { - e.printStackTrace(); - fail(); + fail(e.getMessage()); } dispatcherMock.verify(); @@ -123,12 +124,17 @@ public void testWithParameter() { try { view.execute(mockActionInvocation); } catch (Exception e) { - e.printStackTrace(); - fail(); + fail(e.getMessage()); } assertTrue(mockActionInvocation.getInvocationContext().getParameters().contains("bar")); assertEquals("1", mockActionInvocation.getInvocationContext().getParameters().get("bar").getValue()); + + // See https://issues.apache.org/jira/browse/WW-5486 + Map contextMap = (Map) mockActionInvocation.getInvocationContext().getContextMap().get(DispatcherConstants.PARAMETERS); + assertTrue(contextMap.containsKey("bar")); + assertEquals("1", contextMap.get("bar").getValue()); + dispatcherMock.verify(); requestMock.verify(); dispatcherMock.verify(); From cf260074df0db634048a084201ad2e07ff8e672b Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Wed, 13 Nov 2024 09:04:33 +0100 Subject: [PATCH 2/3] WW-5486 Simplifies test to avoid using mocks --- .../result/ServletDispatcherResultTest.java | 121 +++++++----------- 1 file changed, 44 insertions(+), 77 deletions(-) diff --git a/core/src/test/java/org/apache/struts2/result/ServletDispatcherResultTest.java b/core/src/test/java/org/apache/struts2/result/ServletDispatcherResultTest.java index 22a6312ea1..064307969a 100644 --- a/core/src/test/java/org/apache/struts2/result/ServletDispatcherResultTest.java +++ b/core/src/test/java/org/apache/struts2/result/ServletDispatcherResultTest.java @@ -18,125 +18,92 @@ */ package org.apache.struts2.result; -import com.mockobjects.dynamic.C; -import com.mockobjects.dynamic.Mock; -import jakarta.servlet.RequestDispatcher; -import jakarta.servlet.http.HttpServletRequest; -import jakarta.servlet.http.HttpServletResponse; import org.apache.struts2.ActionContext; -import org.apache.struts2.ServletActionContext; import org.apache.struts2.StrutsInternalTestCase; import org.apache.struts2.StrutsStatics; -import org.apache.struts2.dispatcher.DispatcherConstants; -import org.apache.struts2.dispatcher.Parameter; import org.apache.struts2.mock.MockActionInvocation; +import org.apache.struts2.util.ValueStack; import org.apache.struts2.util.ValueStackFactory; - -import java.util.Map; +import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.mock.web.MockHttpServletResponse; public class ServletDispatcherResultTest extends StrutsInternalTestCase implements StrutsStatics { - public void testInclude() { + private MockHttpServletRequest request; + private MockHttpServletResponse response; + private MockActionInvocation invocation; + private ValueStack stack; + + public void testForward() { ServletDispatcherResult view = new ServletDispatcherResult(); view.setLocation("foo.jsp"); - Mock dispatcherMock = new Mock(RequestDispatcher.class); - dispatcherMock.expect("include", C.ANY_ARGS); - - Mock requestMock = new Mock(HttpServletRequest.class); - requestMock.expectAndReturn("getAttribute", "struts.actiontag.invocation", null); - requestMock.expectAndReturn("getRequestDispatcher", C.args(C.eq("foo.jsp")), dispatcherMock.proxy()); + request.setAttribute("struts.actiontag.invocation", null); + request.setAttribute("jakarta.servlet.include.servlet_path", null); + request.setRequestURI("foo.jsp"); - Mock responseMock = new Mock(HttpServletResponse.class); - responseMock.expectAndReturn("isCommitted", Boolean.TRUE); - - ServletActionContext.setRequest((HttpServletRequest) requestMock.proxy()); - ServletActionContext.setResponse((HttpServletResponse) responseMock.proxy()); + response.setCommitted(Boolean.FALSE); try { - view.execute(null); + view.execute(invocation); } catch (Exception e) { fail(e.getMessage()); } - dispatcherMock.verify(); - requestMock.verify(); - dispatcherMock.verify(); + assertEquals("foo.jsp", response.getForwardedUrl()); } - public void testSimple() { + public void testInclude() { ServletDispatcherResult view = new ServletDispatcherResult(); view.setLocation("foo.jsp"); - Mock dispatcherMock = new Mock(RequestDispatcher.class); - dispatcherMock.expect("forward", C.ANY_ARGS); - - Mock requestMock = new Mock(HttpServletRequest.class); - requestMock.expectAndReturn("getAttribute", "struts.actiontag.invocation", null); - requestMock.expectAndReturn("getAttribute", "jakarta.servlet.include.servlet_path", null); - requestMock.expectAndReturn("getRequestDispatcher", C.args(C.eq("foo.jsp")), dispatcherMock.proxy()); - requestMock.expect("setAttribute", C.ANY_ARGS); // this is a bad mock, but it works - requestMock.expect("setAttribute", C.ANY_ARGS); // this is a bad mock, but it works - requestMock.matchAndReturn("getRequestURI", "foo.jsp"); - - Mock responseMock = new Mock(HttpServletResponse.class); - responseMock.expectAndReturn("isCommitted", Boolean.FALSE); - - ServletActionContext.setRequest((HttpServletRequest) requestMock.proxy()); - ServletActionContext.setResponse((HttpServletResponse) responseMock.proxy()); + request.setAttribute("struts.actiontag.invocation", null); + response.setCommitted(Boolean.TRUE); + request.setRequestURI("foo.jsp"); try { - view.execute(null); + view.execute(invocation); } catch (Exception e) { fail(e.getMessage()); } - dispatcherMock.verify(); - requestMock.verify(); - dispatcherMock.verify(); + assertEquals("foo.jsp", response.getIncludedUrl()); } public void testWithParameter() { ServletDispatcherResult view = container.inject(ServletDispatcherResult.class); view.setLocation("foo.jsp?bar=1"); - Mock dispatcherMock = new Mock(RequestDispatcher.class); - dispatcherMock.expect("forward", C.ANY_ARGS); - - Mock requestMock = new Mock(HttpServletRequest.class); - requestMock.expectAndReturn("getAttribute", "struts.actiontag.invocation", null); - requestMock.expectAndReturn("getAttribute", "jakarta.servlet.include.servlet_path", null); - requestMock.expectAndReturn("getRequestDispatcher", C.args(C.eq("foo.jsp?bar=1")), dispatcherMock.proxy()); - requestMock.expect("setAttribute", C.ANY_ARGS); // this is a bad mock, but it works - requestMock.expect("setAttribute", C.ANY_ARGS); // this is a bad mock, but it works - requestMock.matchAndReturn("getRequestURI", "foo.jsp"); - - Mock responseMock = new Mock(HttpServletResponse.class); - responseMock.expectAndReturn("isCommitted", Boolean.FALSE); - - ServletActionContext.setRequest((HttpServletRequest) requestMock.proxy()); - ServletActionContext.setResponse((HttpServletResponse) responseMock.proxy()); - - MockActionInvocation mockActionInvocation = new MockActionInvocation(); - mockActionInvocation.setInvocationContext(ActionContext.getContext()); - mockActionInvocation.setStack(container.getInstance(ValueStackFactory.class).createValueStack()); - try { - view.execute(mockActionInvocation); + view.execute(invocation); } catch (Exception e) { fail(e.getMessage()); } - assertTrue(mockActionInvocation.getInvocationContext().getParameters().contains("bar")); - assertEquals("1", mockActionInvocation.getInvocationContext().getParameters().get("bar").getValue()); + assertTrue(invocation.getInvocationContext().getParameters().contains("bar")); + assertEquals("1", invocation.getInvocationContext().getParameters().get("bar").getValue()); // See https://issues.apache.org/jira/browse/WW-5486 - Map contextMap = (Map) mockActionInvocation.getInvocationContext().getContextMap().get(DispatcherConstants.PARAMETERS); - assertTrue(contextMap.containsKey("bar")); - assertEquals("1", contextMap.get("bar").getValue()); + assertEquals("1", stack.findString("#parameters.bar")); + } - dispatcherMock.verify(); - requestMock.verify(); - dispatcherMock.verify(); + @Override + public void setUp() throws Exception { + super.setUp(); + invocation = new MockActionInvocation(); + request = new MockHttpServletRequest(); + response = new MockHttpServletResponse(); + stack = container.getInstance(ValueStackFactory.class).createValueStack(); + invocation.setStack(stack); + + stack.getActionContext() + .withServletRequest(request) + .withServletResponse(response) + .withActionInvocation(invocation) + .withValueStack(stack) + .bind(); + + invocation.setInvocationContext(ActionContext.getContext()); } + } From a49b5a36164528ee6798a5e359f9e0689beac25a Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Wed, 13 Nov 2024 09:59:52 +0100 Subject: [PATCH 3/3] WW-5486 Removes try-catches --- .../result/ServletDispatcherResultTest.java | 24 +++++-------------- 1 file changed, 6 insertions(+), 18 deletions(-) diff --git a/core/src/test/java/org/apache/struts2/result/ServletDispatcherResultTest.java b/core/src/test/java/org/apache/struts2/result/ServletDispatcherResultTest.java index 064307969a..2947e71e53 100644 --- a/core/src/test/java/org/apache/struts2/result/ServletDispatcherResultTest.java +++ b/core/src/test/java/org/apache/struts2/result/ServletDispatcherResultTest.java @@ -34,7 +34,7 @@ public class ServletDispatcherResultTest extends StrutsInternalTestCase implemen private MockActionInvocation invocation; private ValueStack stack; - public void testForward() { + public void testForward() throws Exception { ServletDispatcherResult view = new ServletDispatcherResult(); view.setLocation("foo.jsp"); @@ -44,16 +44,12 @@ public void testForward() { response.setCommitted(Boolean.FALSE); - try { - view.execute(invocation); - } catch (Exception e) { - fail(e.getMessage()); - } + view.execute(invocation); assertEquals("foo.jsp", response.getForwardedUrl()); } - public void testInclude() { + public void testInclude() throws Exception { ServletDispatcherResult view = new ServletDispatcherResult(); view.setLocation("foo.jsp"); @@ -61,24 +57,16 @@ public void testInclude() { response.setCommitted(Boolean.TRUE); request.setRequestURI("foo.jsp"); - try { - view.execute(invocation); - } catch (Exception e) { - fail(e.getMessage()); - } + view.execute(invocation); assertEquals("foo.jsp", response.getIncludedUrl()); } - public void testWithParameter() { + public void testWithParameter() throws Exception { ServletDispatcherResult view = container.inject(ServletDispatcherResult.class); view.setLocation("foo.jsp?bar=1"); - try { - view.execute(invocation); - } catch (Exception e) { - fail(e.getMessage()); - } + view.execute(invocation); assertTrue(invocation.getInvocationContext().getParameters().contains("bar")); assertEquals("1", invocation.getInvocationContext().getParameters().get("bar").getValue());