From f326ce32772710dc3959fdab779b85cf91b96180 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sat, 4 Nov 2023 15:50:59 +0100 Subject: [PATCH 1/8] WW-5333 Refactors AttributeMap --- .../org/apache/struts2/components/Set.java | 16 +- .../{util => dispatcher}/AttributeMap.java | 72 ++-- .../apache/struts2/dispatcher/Dispatcher.java | 11 +- .../dispatcher/DispatcherConstants.java | 33 ++ .../apache/struts2/dispatcher/RequestMap.java | 4 +- .../debugging/DebuggingInterceptor.java | 16 +- .../apache/struts2/views/jsp/TagUtils.java | 2 +- .../struts2/views/util/ContextUtil.java | 7 +- .../struts2/dispatcher/AttributeMapTest.java | 363 ++++++++++++++++++ .../portlet/dispatcher/Jsr168Dispatcher.java | 13 +- 10 files changed, 475 insertions(+), 62 deletions(-) rename core/src/main/java/org/apache/struts2/{util => dispatcher}/AttributeMap.java (63%) create mode 100644 core/src/main/java/org/apache/struts2/dispatcher/DispatcherConstants.java create mode 100644 core/src/test/java/org/apache/struts2/dispatcher/AttributeMapTest.java diff --git a/core/src/main/java/org/apache/struts2/components/Set.java b/core/src/main/java/org/apache/struts2/components/Set.java index 8cb1ca461..cca990ea8 100644 --- a/core/src/main/java/org/apache/struts2/components/Set.java +++ b/core/src/main/java/org/apache/struts2/components/Set.java @@ -20,6 +20,7 @@ package org.apache.struts2.components; import java.io.Writer; +import org.apache.struts2.dispatcher.DispatcherConstants; import org.apache.struts2.views.annotations.StrutsTag; import org.apache.struts2.views.annotations.StrutsTagAttribute; @@ -53,16 +54,11 @@ import com.opensymphony.xwork2.util.ValueStack; * * * * * @@ -107,16 +103,16 @@ public class Set extends ContextBean { body=""; - if ("application".equalsIgnoreCase(scope)) { + if (DispatcherConstants.APPLICATION.equalsIgnoreCase(scope)) { stack.setValue("#application['" + getVar() + "']", o); - } else if ("session".equalsIgnoreCase(scope)) { + } else if (DispatcherConstants.SESSION.equalsIgnoreCase(scope)) { stack.setValue("#session['" + getVar() + "']", o); - } else if ("request".equalsIgnoreCase(scope)) { + } else if (DispatcherConstants.REQUEST.equalsIgnoreCase(scope)) { stack.setValue("#request['" + getVar() + "']", o); - } else if ("page".equalsIgnoreCase(scope)) { + } else if (DispatcherConstants.PAGE.equalsIgnoreCase(scope)) { stack.setValue("#attr['" + getVar() + "']", o, false); } else { - // Default scope is action. Note: The action acope handling also adds the var to the page scope. + // Default scope is action. Note: The action scope handling also adds the var to the page scope. stack.getContext().put(getVar(), o); stack.setValue("#attr['" + getVar() + "']", o, false); } diff --git a/core/src/main/java/org/apache/struts2/util/AttributeMap.java b/core/src/main/java/org/apache/struts2/dispatcher/AttributeMap.java similarity index 63% rename from core/src/main/java/org/apache/struts2/util/AttributeMap.java rename to core/src/main/java/org/apache/struts2/dispatcher/AttributeMap.java index ea088b4af..dbdc686bb 100644 --- a/core/src/main/java/org/apache/struts2/util/AttributeMap.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/AttributeMap.java @@ -16,14 +16,16 @@ * specific language governing permissions and limitations * under the License. */ -package org.apache.struts2.util; +package org.apache.struts2.dispatcher; -import org.apache.struts2.ServletActionContext; +import org.apache.struts2.StrutsStatics; import javax.servlet.jsp.PageContext; +import java.util.AbstractMap; import java.util.Collection; import java.util.Collections; import java.util.Map; +import java.util.Objects; import java.util.Set; /** @@ -38,17 +40,16 @@ import java.util.Set; *
  • Session scope
    • *
  • Application scope
    • * - * + *

    * A object is searched in the order above, starting from page and ending at application scope. - * */ -public class AttributeMap implements Map { +public class AttributeMap extends AbstractMap { protected static final String UNSUPPORTED = "method makes no sense for a simplified map"; - Map context; + private final Map context; - public AttributeMap(Map context) { + public AttributeMap(Map context) { this.context = context; } @@ -73,18 +74,22 @@ public class AttributeMap implements Map { } @Override - public Set entrySet() { - return Collections.EMPTY_SET; + public Set> entrySet() { + return Collections.unmodifiableSet(this.context.entrySet()); } @Override public Object get(Object key) { + if (key == null) { + return null; + } + PageContext pc = getPageContext(); if (pc == null) { - Map request = (Map) context.get("request"); - Map session = (Map) context.get("session"); - Map application = (Map) context.get("application"); + RequestMap request = (RequestMap) context.get(DispatcherConstants.REQUEST); + SessionMap session = (SessionMap) context.get(DispatcherConstants.SESSION); + ApplicationMap application = (ApplicationMap) context.get(DispatcherConstants.APPLICATION); if ((request != null) && (request.get(key) != null)) { return request.get(key); @@ -94,26 +99,23 @@ public class AttributeMap implements Map { return application.get(key); } } else { - try { - return pc.findAttribute(key.toString()); - } catch (NullPointerException npe) { - return null; - } + return pc.findAttribute(key.toString()); } return null; } @Override - public Set keySet() { - return Collections.EMPTY_SET; + public Set keySet() { + return Collections.unmodifiableSet(this.context.keySet()); } @Override - public Object put(Object key, Object value) { + public Object put(String key, Object value) { PageContext pc = getPageContext(); if (pc != null) { - pc.setAttribute(key.toString(), value); + pc.setAttribute(key, value); + return value; } return null; @@ -135,21 +137,21 @@ public class AttributeMap implements Map { } @Override - public Collection values() { - return Collections.EMPTY_SET; + public Collection values() { + return Collections.unmodifiableCollection(this.context.values()); } private PageContext getPageContext() { - return (PageContext) context.get(ServletActionContext.PAGE_CONTEXT); + return (PageContext) context.get(StrutsStatics.PAGE_CONTEXT); } @Override public String toString() { return "AttributeMap {" + - "request=" + toStringSafe(context.get("request")) + - ", session=" + toStringSafe(context.get("session")) + - ", application=" + toStringSafe(context.get("application")) + - '}'; + "request=" + toStringSafe(context.get(DispatcherConstants.REQUEST)) + + ", session=" + toStringSafe(context.get(DispatcherConstants.SESSION)) + + ", application=" + toStringSafe(context.get(DispatcherConstants.APPLICATION)) + + '}'; } private String toStringSafe(Object obj) { @@ -163,4 +165,18 @@ public class AttributeMap implements Map { } } + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (!(o instanceof AttributeMap)) return false; + if (!super.equals(o)) return false; + AttributeMap that = (AttributeMap) o; + return Objects.equals(context, that.context); + } + + @Override + public int hashCode() { + return Objects.hash(super.hashCode(), context); + } + } 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 51ae95d27..e378633e2 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/Dispatcher.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/Dispatcher.java @@ -67,7 +67,6 @@ import org.apache.struts2.config.StrutsXmlConfigurationProvider; import org.apache.struts2.dispatcher.mapper.ActionMapping; import org.apache.struts2.dispatcher.multipart.MultiPartRequest; import org.apache.struts2.dispatcher.multipart.MultiPartRequestWrapper; -import org.apache.struts2.util.AttributeMap; import org.apache.struts2.util.ObjectFactoryDestroyable; import org.apache.struts2.util.fs.JBossFileManager; @@ -779,14 +778,14 @@ public class Dispatcher { .withServletResponse(response) .withServletContext(servletContext) // helpers to get access to request/session/application scope - .with("request", requestMap) - .with("session", sessionMap) - .with("application", applicationMap) - .with("parameters", parameters) + .with(DispatcherConstants.REQUEST, requestMap) + .with(DispatcherConstants.SESSION, sessionMap) + .with(DispatcherConstants.APPLICATION, applicationMap) + .with(DispatcherConstants.PARAMETERS, parameters) .getContextMap(); AttributeMap attrMap = new AttributeMap(extraContext); - extraContext.put("attr", attrMap); + extraContext.put(DispatcherConstants.ATTRIBUTES, attrMap); return extraContext; } diff --git a/core/src/main/java/org/apache/struts2/dispatcher/DispatcherConstants.java b/core/src/main/java/org/apache/struts2/dispatcher/DispatcherConstants.java new file mode 100644 index 000000000..7ccd220cb --- /dev/null +++ b/core/src/main/java/org/apache/struts2/dispatcher/DispatcherConstants.java @@ -0,0 +1,33 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.struts2.dispatcher; + +public final class DispatcherConstants { + + public static final String REQUEST = "request"; + public static final String RESPONSE = "response"; + public static final String SESSION = "session"; + public static final String APPLICATION = "application"; + public static final String PARAMETERS = "parameters"; + public static final String ATTRIBUTES = "attr"; + public static final String PAGE = "page"; + + private DispatcherConstants() { + } +} diff --git a/core/src/main/java/org/apache/struts2/dispatcher/RequestMap.java b/core/src/main/java/org/apache/struts2/dispatcher/RequestMap.java index a75dffb75..b8e5b8e67 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/RequestMap.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/RequestMap.java @@ -32,8 +32,9 @@ public class RequestMap extends AbstractMap implements Serializa private static final long serialVersionUID = -7675640869293787926L; + private final HttpServletRequest request; + private Set> entries; - private HttpServletRequest request; /** * Saves the request to use as the backing for getting and setting values @@ -44,7 +45,6 @@ public class RequestMap extends AbstractMap implements Serializa this.request = request; } - /** * Removes all attributes from the request as well as clears entries in this map. */ diff --git a/core/src/main/java/org/apache/struts2/interceptor/debugging/DebuggingInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/debugging/DebuggingInterceptor.java index 62f871690..7f116829e 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/debugging/DebuggingInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/debugging/DebuggingInterceptor.java @@ -29,8 +29,10 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.apache.struts2.ServletActionContext; import org.apache.struts2.StrutsConstants; +import org.apache.struts2.dispatcher.DispatcherConstants; import org.apache.struts2.dispatcher.Parameter; import org.apache.struts2.dispatcher.PrepareOperations; +import org.apache.struts2.dispatcher.RequestMap; import org.apache.struts2.views.freemarker.FreemarkerManager; import org.apache.struts2.views.freemarker.FreemarkerResult; @@ -97,11 +99,13 @@ public class DebuggingInterceptor extends AbstractInterceptor { private final static Logger LOG = LogManager.getLogger(DebuggingInterceptor.class); - private String[] ignorePrefixes = new String[]{"org.apache.struts.", - "com.opensymphony.xwork2.", "xwork."}; - private String[] _ignoreKeys = new String[]{"application", "session", - "parameters", "request"}; - private HashSet ignoreKeys = new HashSet<>(Arrays.asList(_ignoreKeys)); + private final String[] ignorePrefixes = new String[]{"org.apache.struts.", "com.opensymphony.xwork2.", "xwork."}; + private final HashSet ignoreKeys = new HashSet<>(Arrays.asList( + DispatcherConstants.APPLICATION, + DispatcherConstants.SESSION, + DispatcherConstants.PARAMETERS, + DispatcherConstants.REQUEST + )); private final static String XML_MODE = "xml"; private final static String CONSOLE_MODE = "console"; @@ -319,7 +323,7 @@ public class DebuggingInterceptor extends AbstractInterceptor { } } writer.endNode(); - Map requestMap = (Map) ctx.get("request"); + RequestMap requestMap = (RequestMap) ctx.get(DispatcherConstants.REQUEST); serializeIt(requestMap, "request", writer, filterValueStack(requestMap)); serializeIt(ctx.getSession(), "session", writer, new ArrayList<>()); diff --git a/core/src/main/java/org/apache/struts2/views/jsp/TagUtils.java b/core/src/main/java/org/apache/struts2/views/jsp/TagUtils.java index f813e00d1..044f5d1af 100644 --- a/core/src/main/java/org/apache/struts2/views/jsp/TagUtils.java +++ b/core/src/main/java/org/apache/struts2/views/jsp/TagUtils.java @@ -24,7 +24,7 @@ import com.opensymphony.xwork2.util.ValueStack; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.apache.struts2.ServletActionContext; -import org.apache.struts2.util.AttributeMap; +import org.apache.struts2.dispatcher.AttributeMap; import javax.servlet.http.HttpServletRequest; import javax.servlet.jsp.PageContext; diff --git a/core/src/main/java/org/apache/struts2/views/util/ContextUtil.java b/core/src/main/java/org/apache/struts2/views/util/ContextUtil.java index 4ab3c2786..0f1c85aae 100644 --- a/core/src/main/java/org/apache/struts2/views/util/ContextUtil.java +++ b/core/src/main/java/org/apache/struts2/views/util/ContextUtil.java @@ -20,6 +20,7 @@ package org.apache.struts2.views.util; import com.opensymphony.xwork2.ActionInvocation; import com.opensymphony.xwork2.util.ValueStack; +import org.apache.struts2.dispatcher.DispatcherConstants; import org.apache.struts2.util.StrutsUtil; import javax.servlet.http.HttpServletRequest; @@ -31,9 +32,9 @@ import java.util.Map; * Value Stack's Context related Utilities. */ public class ContextUtil { - public static final String REQUEST = "request"; - public static final String RESPONSE = "response"; - public static final String SESSION = "session"; + public static final String REQUEST = DispatcherConstants.REQUEST; + public static final String RESPONSE = DispatcherConstants.RESPONSE; + public static final String SESSION = DispatcherConstants.SESSION; public static final String BASE = "base"; public static final String STACK = "stack"; public static final String STRUTS = "struts"; diff --git a/core/src/test/java/org/apache/struts2/dispatcher/AttributeMapTest.java b/core/src/test/java/org/apache/struts2/dispatcher/AttributeMapTest.java new file mode 100644 index 000000000..7878150e3 --- /dev/null +++ b/core/src/test/java/org/apache/struts2/dispatcher/AttributeMapTest.java @@ -0,0 +1,363 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.struts2.dispatcher; + +import org.apache.struts2.StrutsStatics; +import org.junit.Test; +import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.mock.web.MockHttpSession; +import org.springframework.mock.web.MockPageContext; +import org.springframework.mock.web.MockServletContext; + +import javax.servlet.ServletContext; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpSession; +import javax.servlet.jsp.PageContext; +import java.util.Collection; +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; + +import static org.hamcrest.CoreMatchers.hasItem; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; + +public class AttributeMapTest { + + @Test + public void shouldRetrievePageContextAttribute() { + // given + PageContext pc = new MockPageContext(); + pc.setAttribute("attr", "value"); + + Map context = new HashMap() {{ + put(StrutsStatics.PAGE_CONTEXT, pc); + }}; + + // when + AttributeMap am = new AttributeMap(context); + Object value = am.get("attr"); + + // then + assertEquals("value", value); + } + + @Test + public void shouldPutAttribute() { + // given + PageContext pc = new MockPageContext(); + + Map context = new HashMap() {{ + put(StrutsStatics.PAGE_CONTEXT, pc); + }}; + + // when + AttributeMap am = new AttributeMap(context); + Object value = am.put("attr", "value"); + + // then + assertEquals("value", value); + assertEquals("value", pc.getAttribute("attr")); + } + + @Test + public void shouldRetrieveRequestAttribute() { + // given + HttpServletRequest request = new MockHttpServletRequest(); + request.setAttribute("attr", "value"); + + Map context = new HashMap() {{ + put(DispatcherConstants.REQUEST, new RequestMap(request)); + }}; + + // when + AttributeMap am = new AttributeMap(context); + Object value = am.get("attr"); + + // then + assertEquals("value", value); + } + + @Test + public void shouldRetrieveSessionAttribute() { + // given + HttpSession session = new MockHttpSession(); + session.setAttribute("attr", "value"); + + MockHttpServletRequest request = new MockHttpServletRequest(); + request.setSession(session); + + Map context = new HashMap() {{ + put(DispatcherConstants.SESSION, new SessionMap(request)); + }}; + + // when + AttributeMap am = new AttributeMap(context); + Object value = am.get("attr"); + + // then + assertEquals("value", value); + } + + @Test + public void shouldRetrieveApplicationAttribute() { + // given + ServletContext sc = new MockServletContext(); + sc.setAttribute("attr", "value"); + + Map context = new HashMap() {{ + put(DispatcherConstants.APPLICATION, new ApplicationMap(sc)); + }}; + + // when + AttributeMap am = new AttributeMap(context); + Object value = am.get("attr"); + + // then + assertEquals("value", value); + } + + @Test + public void shouldReturnNullIfKeyIsNull() { + // given + // when + AttributeMap am = new AttributeMap(new HashMap<>()); + Object value = am.get(null); + + // then + assertNull(value); + } + + @Test + public void shouldThrowExceptionOnRemove() { + // given + PageContext pc = new MockPageContext(); + pc.setAttribute("attr", "value"); + + Map context = new HashMap() {{ + put(StrutsStatics.PAGE_CONTEXT, pc); + }}; + + // when + AttributeMap am = new AttributeMap(context); + + // then + Object value = am.get("attr"); + assertEquals("value", value); + + assertThrows(UnsupportedOperationException.class, () -> am.remove("attr")); + } + + @Test + public void shouldThrowExceptionOnClear() { + // given + PageContext pc = new MockPageContext(); + pc.setAttribute("attr", "value"); + + Map context = new HashMap() {{ + put(StrutsStatics.PAGE_CONTEXT, pc); + }}; + + // when + AttributeMap am = new AttributeMap(context); + Object value = am.get("attr"); + + // then + assertEquals("value", value); + + // when + assertThrows(UnsupportedOperationException.class, am::clear); + } + + @Test + public void shouldThrowExceptionOnIsEmpty() { + // given + PageContext pc = new MockPageContext(); + pc.setAttribute("attr", "value"); + + Map context = new HashMap() {{ + put(StrutsStatics.PAGE_CONTEXT, pc); + }}; + + // when + AttributeMap am = new AttributeMap(context); + Object value = am.get("attr"); + + // then + assertEquals("value", value); + + // when + assertThrows(UnsupportedOperationException.class, am::isEmpty); + } + + @Test + public void shouldThrowExceptionOnContainsValue() { + // given + PageContext pc = new MockPageContext(); + pc.setAttribute("attr", "value"); + + Map context = new HashMap() {{ + put(StrutsStatics.PAGE_CONTEXT, pc); + }}; + + // when + AttributeMap am = new AttributeMap(context); + Object value = am.get("attr"); + + // then + assertEquals("value", value); + + // when + assertThrows(UnsupportedOperationException.class, () -> am.containsValue("attr")); + } + + @Test + public void shouldThrowExceptionOnPutAll() { + // given + PageContext pc = new MockPageContext(); + pc.setAttribute("attr", "value"); + + Map context = new HashMap() {{ + put(StrutsStatics.PAGE_CONTEXT, pc); + }}; + + Map values = Collections.emptyMap(); + + // when + AttributeMap am = new AttributeMap(context); + Object value = am.get("attr"); + + // then + assertEquals("value", value); + + // when + assertThrows(UnsupportedOperationException.class, () -> am.putAll(values)); + } + + @Test + public void shouldThrowExceptionOnKeySet() { + // given + PageContext pc = new MockPageContext(); + pc.setAttribute("attr", "value"); + + Map context = new HashMap() {{ + put(StrutsStatics.PAGE_CONTEXT, pc); + }}; + + // when + AttributeMap am = new AttributeMap(context); + Object value = am.get("attr"); + + // then + assertEquals("value", value); + + // when + assertEquals(am.keySet(), context.keySet()); + } + + @Test + public void shouldThrowExceptionOnSize() { + // given + PageContext pc = new MockPageContext(); + pc.setAttribute("attr", "value"); + + Map context = new HashMap() {{ + put(StrutsStatics.PAGE_CONTEXT, pc); + }}; + + // when + AttributeMap am = new AttributeMap(context); + Object value = am.get("attr"); + + // then + assertEquals("value", value); + + // when + assertThrows(UnsupportedOperationException.class, am::size); + } + + @Test + public void shouldGetAllValues() { + // given + PageContext pc = new MockPageContext(); + pc.setAttribute("attr", "value"); + + Map context = new HashMap() {{ + put(StrutsStatics.PAGE_CONTEXT, pc); + }}; + + // when + AttributeMap am = new AttributeMap(context); + Object value = am.get("attr"); + + // then + assertEquals("value", value); + + // when + Collection values = am.values(); + + // then + assertThat(values, hasItem(pc)); + } + + @Test + public void shouldGetEntrySet() { + // given + PageContext pc = new MockPageContext(); + pc.setAttribute("attr", "value"); + + Map context = new HashMap() {{ + put(StrutsStatics.PAGE_CONTEXT, pc); + }}; + + // when + AttributeMap am = new AttributeMap(context); + Object value = am.get("attr"); + + // then + assertEquals("value", value); + + // then + assertEquals(context.entrySet(), am.entrySet()); + } + + @Test + public void shouldContainsKey() { + // given + PageContext pc = new MockPageContext(); + pc.setAttribute("attr", "value"); + + Map context = new HashMap() {{ + put(StrutsStatics.PAGE_CONTEXT, pc); + }}; + + // when + AttributeMap am = new AttributeMap(context); + + // then + assertTrue(am.containsKey("attr")); + + Object value = am.get("attr"); + assertEquals("value", value); + } + +} \ No newline at end of file diff --git a/plugins/portlet/src/main/java/org/apache/struts2/portlet/dispatcher/Jsr168Dispatcher.java b/plugins/portlet/src/main/java/org/apache/struts2/portlet/dispatcher/Jsr168Dispatcher.java index cc7b00c5b..394ef6042 100644 --- a/plugins/portlet/src/main/java/org/apache/struts2/portlet/dispatcher/Jsr168Dispatcher.java +++ b/plugins/portlet/src/main/java/org/apache/struts2/portlet/dispatcher/Jsr168Dispatcher.java @@ -33,6 +33,7 @@ import org.apache.struts2.StrutsException; import org.apache.struts2.StrutsStatics; import org.apache.struts2.dispatcher.ApplicationMap; import org.apache.struts2.dispatcher.Dispatcher; +import org.apache.struts2.dispatcher.DispatcherConstants; import org.apache.struts2.dispatcher.HttpParameters; import org.apache.struts2.dispatcher.RequestMap; import org.apache.struts2.dispatcher.SessionMap; @@ -48,7 +49,7 @@ import org.apache.struts2.portlet.context.PortletActionContext; import org.apache.struts2.portlet.servlet.PortletServletContext; import org.apache.struts2.portlet.servlet.PortletServletRequest; import org.apache.struts2.portlet.servlet.PortletServletResponse; -import org.apache.struts2.util.AttributeMap; +import org.apache.struts2.dispatcher.AttributeMap; import javax.portlet.ActionRequest; import javax.portlet.ActionResponse; @@ -377,7 +378,7 @@ public class Jsr168Dispatcher extends GenericPortlet implements StrutsStatics { container.inject(servletRequest); // ServletActionContext - Map extraContext = ActionContext.of(new HashMap()) + Map extraContext = ActionContext.of(new HashMap<>()) .withServletRequest(servletRequest) .withServletResponse(servletResponse) .withServletContext(servletContext) @@ -392,10 +393,10 @@ public class Jsr168Dispatcher extends GenericPortlet implements StrutsStatics { .with(PORTLET_NAMESPACE, portletNamespace) .with(DEFAULT_ACTION_FOR_MODE, actionMap.get(request.getPortletMode())) // helpers to get access to request/session/application scope - .with("request", requestMap) - .with("session", sessionMap) - .with("application", applicationMap) - .with("parameters", parameterMap) + .with(DispatcherConstants.REQUEST, requestMap) + .with(DispatcherConstants.SESSION, sessionMap) + .with(DispatcherConstants.APPLICATION, applicationMap) + .with(DispatcherConstants.PARAMETERS, parameterMap) .with(MODE_NAMESPACE_MAP, modeMap) .with(PortletConstants.DEFAULT_ACTION_MAP, actionMap) .with(PortletConstants.PHASE, phase) From fadfe24c5f77b9531f0eef185424e1e7c505ee7d Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 13 Nov 2023 07:15:36 +0100 Subject: [PATCH 2/8] Uses the new notifications@ list for all the messages form Github --- .asf.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.asf.yaml b/.asf.yaml index 2a7cde95d..673c6e25c 100644 --- a/.asf.yaml +++ b/.asf.yaml @@ -4,9 +4,9 @@ notifications: # Send all issue emails (new, closed, comments) to issues@ issues: issues@struts.apache.org # Send new/closed PR notifications to commits@ - pullrequests_status: commits@struts.apache.org + pullrequests_status: notifications@struts.apache.org # Send individual PR comments/reviews to issues@ - pullrequests_comment: issues@struts.apache.org + pullrequests_comment: notifications@struts.apache.org # Link opened PRs with JIRA jira_options: link label worklog From a2b4744c3a0bfe517f1135f0407ded5b45f5bb96 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Mon, 13 Nov 2023 23:58:41 +1100 Subject: [PATCH 3/8] WW-5363 Velocity: read chained contexts before ValueStack --- .../views/velocity/StrutsVelocityContext.java | 52 +++++++++++++------ 1 file changed, 35 insertions(+), 17 deletions(-) diff --git a/plugins/velocity/src/main/java/org/apache/struts2/views/velocity/StrutsVelocityContext.java b/plugins/velocity/src/main/java/org/apache/struts2/views/velocity/StrutsVelocityContext.java index cca2b5dc3..2c5477ba1 100644 --- a/plugins/velocity/src/main/java/org/apache/struts2/views/velocity/StrutsVelocityContext.java +++ b/plugins/velocity/src/main/java/org/apache/struts2/views/velocity/StrutsVelocityContext.java @@ -24,6 +24,7 @@ import org.apache.velocity.VelocityContext; import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import java.util.function.Function; public class StrutsVelocityContext extends VelocityContext { @@ -54,36 +55,53 @@ public class StrutsVelocityContext extends VelocityContext { /** * @deprecated please use {@link #StrutsVelocityContext(List, ValueStack)} */ - @Deprecated() + @Deprecated public StrutsVelocityContext(VelocityContext[] chainedContexts, ValueStack stack) { this(new ArrayList<>(Arrays.asList(chainedContexts)), stack); } + @Override public boolean internalContainsKey(String key) { return internalGet(key) != null; } + @Override public Object internalGet(String key) { - Object val = super.internalGet(key); - if (val != null) { - return val; - } - if (stack != null) { - val = stack.findValue(key); - if (val != null) { - return val; - } - val = stack.getContext().get(key); + for (Function contextGet : contextGetterList()) { + Object val = contextGet.apply(key); if (val != null) { return val; } } - if (chainedContexts != null) { - for (VelocityContext chainedContext : chainedContexts) { - val = chainedContext.internalGet(key); - if (val != null) { - return val; - } + return null; + } + + protected List> contextGetterList() { + return Arrays.asList(super::internalGet, this::chainedContextGet, this::stackGet, this::stackContextGet); + } + + protected Object stackGet(String key) { + if (stack == null) { + return null; + } + return stack.findValue(key); + } + + protected Object stackContextGet(String key) { + if (stack == null) { + return null; + } + return stack.getContext().get(key); + } + + protected Object chainedContextGet(String key) { + if (chainedContexts == null) { + return null; + } + for (VelocityContext chainedContext : chainedContexts) { + Object val = chainedContext.internalGet(key); + if (val != null) { + return val; } } return null; From cf9e53573ed0fc46f8bf83c299afe99098de5465 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Tue, 14 Nov 2023 01:45:43 +1100 Subject: [PATCH 4/8] WW-5363 Add test coverage --- .../velocity/StrutsVelocityContextTest.java | 88 +++++++++++++++++++ 1 file changed, 88 insertions(+) create mode 100644 plugins/velocity/src/test/java/org/apache/struts2/views/velocity/StrutsVelocityContextTest.java diff --git a/plugins/velocity/src/test/java/org/apache/struts2/views/velocity/StrutsVelocityContextTest.java b/plugins/velocity/src/test/java/org/apache/struts2/views/velocity/StrutsVelocityContextTest.java new file mode 100644 index 000000000..9d78ae1d5 --- /dev/null +++ b/plugins/velocity/src/test/java/org/apache/struts2/views/velocity/StrutsVelocityContextTest.java @@ -0,0 +1,88 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.struts2.views.velocity; + +import com.opensymphony.xwork2.util.ValueStack; +import org.apache.velocity.VelocityContext; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.mockito.Mock; +import org.mockito.junit.MockitoJUnit; +import org.mockito.junit.MockitoRule; + +import java.util.HashMap; +import java.util.Map; + +import static java.util.Collections.singletonList; +import static org.junit.Assert.assertEquals; +import static org.mockito.Mockito.when; + +public class StrutsVelocityContextTest { + + @Rule + public MockitoRule mockitoRule = MockitoJUnit.rule(); + + private StrutsVelocityContext strutsVelocityContext; + + @Mock + private VelocityContext chainedContext; + + @Mock + private ValueStack stack; + + private Map stackContext; + + @Before + public void setUp() throws Exception { + stackContext = new HashMap<>(); + when(stack.getContext()).thenReturn(stackContext); + strutsVelocityContext = new StrutsVelocityContext(singletonList(chainedContext), stack); + } + + @Test + public void getChainedValue() { + when(chainedContext.internalGet("foo")).thenReturn("bar"); + assertEquals("bar", strutsVelocityContext.internalGet("foo")); + } + + @Test + public void getStackValue() { + when(stack.findValue("foo")).thenReturn("bar"); + assertEquals("bar", strutsVelocityContext.internalGet("foo")); + } + + @Test + public void getStackContextValue() { + stackContext.put("foo", "bar"); + assertEquals("bar", strutsVelocityContext.internalGet("foo")); + } + + @Test + public void getValuePrecedence() { + when(chainedContext.internalGet("foo")).thenReturn("bar"); + assertEquals("bar", strutsVelocityContext.internalGet("foo")); + + when(stack.findValue("foo")).thenReturn("baz"); + assertEquals("bar", strutsVelocityContext.internalGet("foo")); + + stackContext.put("foo", "qux"); + assertEquals("bar", strutsVelocityContext.internalGet("foo")); + } +} From 6c98663f84c79f51c2dd77d27271066002f043a8 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Tue, 14 Nov 2023 02:10:16 +1100 Subject: [PATCH 5/8] WW-5363 Fix super#internalGet --- .../views/velocity/StrutsVelocityContext.java | 6 +++++- .../velocity/StrutsVelocityContextTest.java | 19 ++++++++++++++----- 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/plugins/velocity/src/main/java/org/apache/struts2/views/velocity/StrutsVelocityContext.java b/plugins/velocity/src/main/java/org/apache/struts2/views/velocity/StrutsVelocityContext.java index 2c5477ba1..99be98b15 100644 --- a/plugins/velocity/src/main/java/org/apache/struts2/views/velocity/StrutsVelocityContext.java +++ b/plugins/velocity/src/main/java/org/apache/struts2/views/velocity/StrutsVelocityContext.java @@ -77,7 +77,11 @@ public class StrutsVelocityContext extends VelocityContext { } protected List> contextGetterList() { - return Arrays.asList(super::internalGet, this::chainedContextGet, this::stackGet, this::stackContextGet); + return Arrays.asList(this::superGet, this::chainedContextGet, this::stackGet, this::stackContextGet); + } + + protected Object superGet(String key) { + return super.internalGet(key); } protected Object stackGet(String key) { diff --git a/plugins/velocity/src/test/java/org/apache/struts2/views/velocity/StrutsVelocityContextTest.java b/plugins/velocity/src/test/java/org/apache/struts2/views/velocity/StrutsVelocityContextTest.java index 9d78ae1d5..9f1280842 100644 --- a/plugins/velocity/src/test/java/org/apache/struts2/views/velocity/StrutsVelocityContextTest.java +++ b/plugins/velocity/src/test/java/org/apache/struts2/views/velocity/StrutsVelocityContextTest.java @@ -74,15 +74,24 @@ public class StrutsVelocityContextTest { assertEquals("bar", strutsVelocityContext.internalGet("foo")); } + @Test + public void getSuperValue() { + strutsVelocityContext.put("foo", "bar"); + assertEquals("bar", strutsVelocityContext.internalGet("foo")); + } + @Test public void getValuePrecedence() { - when(chainedContext.internalGet("foo")).thenReturn("bar"); - assertEquals("bar", strutsVelocityContext.internalGet("foo")); + stackContext.put("foo", "quux"); + assertEquals("quux", strutsVelocityContext.internalGet("foo")); - when(stack.findValue("foo")).thenReturn("baz"); - assertEquals("bar", strutsVelocityContext.internalGet("foo")); + when(stack.findValue("foo")).thenReturn("qux"); + assertEquals("qux", strutsVelocityContext.internalGet("foo")); - stackContext.put("foo", "qux"); + when(chainedContext.internalGet("foo")).thenReturn("baz"); + assertEquals("baz", strutsVelocityContext.internalGet("foo")); + + strutsVelocityContext.put("foo", "bar"); assertEquals("bar", strutsVelocityContext.internalGet("foo")); } } From 29d471e2ddae9b63340a564860253e96349f7161 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Tue, 14 Nov 2023 02:11:49 +1100 Subject: [PATCH 6/8] WW-5363 Improve code coverage --- .../struts2/views/velocity/StrutsVelocityContextTest.java | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/plugins/velocity/src/test/java/org/apache/struts2/views/velocity/StrutsVelocityContextTest.java b/plugins/velocity/src/test/java/org/apache/struts2/views/velocity/StrutsVelocityContextTest.java index 9f1280842..1405637b3 100644 --- a/plugins/velocity/src/test/java/org/apache/struts2/views/velocity/StrutsVelocityContextTest.java +++ b/plugins/velocity/src/test/java/org/apache/struts2/views/velocity/StrutsVelocityContextTest.java @@ -28,10 +28,12 @@ import org.mockito.junit.MockitoJUnit; import org.mockito.junit.MockitoRule; import java.util.HashMap; +import java.util.List; import java.util.Map; import static java.util.Collections.singletonList; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; import static org.mockito.Mockito.when; public class StrutsVelocityContextTest { @@ -94,4 +96,10 @@ public class StrutsVelocityContextTest { strutsVelocityContext.put("foo", "bar"); assertEquals("bar", strutsVelocityContext.internalGet("foo")); } + + @Test + public void nullArgs() { + strutsVelocityContext = new StrutsVelocityContext((List) null, null); + assertNull(strutsVelocityContext.internalGet("foo")); + } } From 2188d115918961d84804ae218e377620438b1c4e Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 13 Nov 2023 16:27:15 +0100 Subject: [PATCH 7/8] Send Jenkins notifications to the notifications@ list --- Jenkinsfile | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Jenkinsfile b/Jenkinsfile index e00cefb97..b80a9fcd8 100644 --- a/Jenkinsfile +++ b/Jenkinsfile @@ -189,7 +189,7 @@ pipeline { failure { script { emailext( - to: "commits@struts.apache.org", + to: "notifications@struts.apache.org", recipientProviders: [[$class: 'DevelopersRecipientProvider']], from: "Mr. Jenkins ", subject: "Jenkins job ${env.JOB_NAME}#${env.BUILD_NUMBER} failed", @@ -212,7 +212,7 @@ Director of Continuous Integration unstable { script { emailext( - to: "commits@struts.apache.org", + to: "notifications@struts.apache.org", recipientProviders: [[$class: 'DevelopersRecipientProvider']], from: "Mr. Jenkins ", subject: "Jenkins job ${env.JOB_NAME}#${env.BUILD_NUMBER} unstable", @@ -235,7 +235,7 @@ Director of Continuous Integration fixed { script { emailext( - to: "commits@struts.apache.org", + to: "notifications@struts.apache.org", recipientProviders: [[$class: 'DevelopersRecipientProvider']], from: 'Mr. Jenkins ', subject: "Jenkins job ${env.JOB_NAME}#${env.BUILD_NUMBER} back to normal", From 0504e7076c3a94945e168d72e06ef3d15a98a0dc Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Wed, 15 Nov 2023 16:06:24 +1100 Subject: [PATCH 8/8] WW-5363 Remove redundant method from VelocityManager --- .../views/velocity/StrutsVelocityContext.java | 9 +-------- .../views/velocity/StrutsVelocityContextTest.java | 15 --------------- 2 files changed, 1 insertion(+), 23 deletions(-) diff --git a/plugins/velocity/src/main/java/org/apache/struts2/views/velocity/StrutsVelocityContext.java b/plugins/velocity/src/main/java/org/apache/struts2/views/velocity/StrutsVelocityContext.java index 99be98b15..4241f6ead 100644 --- a/plugins/velocity/src/main/java/org/apache/struts2/views/velocity/StrutsVelocityContext.java +++ b/plugins/velocity/src/main/java/org/apache/struts2/views/velocity/StrutsVelocityContext.java @@ -77,7 +77,7 @@ public class StrutsVelocityContext extends VelocityContext { } protected List> contextGetterList() { - return Arrays.asList(this::superGet, this::chainedContextGet, this::stackGet, this::stackContextGet); + return Arrays.asList(this::superGet, this::chainedContextGet, this::stackGet); } protected Object superGet(String key) { @@ -91,13 +91,6 @@ public class StrutsVelocityContext extends VelocityContext { return stack.findValue(key); } - protected Object stackContextGet(String key) { - if (stack == null) { - return null; - } - return stack.getContext().get(key); - } - protected Object chainedContextGet(String key) { if (chainedContexts == null) { return null; diff --git a/plugins/velocity/src/test/java/org/apache/struts2/views/velocity/StrutsVelocityContextTest.java b/plugins/velocity/src/test/java/org/apache/struts2/views/velocity/StrutsVelocityContextTest.java index 1405637b3..6cd38c8aa 100644 --- a/plugins/velocity/src/test/java/org/apache/struts2/views/velocity/StrutsVelocityContextTest.java +++ b/plugins/velocity/src/test/java/org/apache/struts2/views/velocity/StrutsVelocityContextTest.java @@ -27,9 +27,7 @@ import org.mockito.Mock; import org.mockito.junit.MockitoJUnit; import org.mockito.junit.MockitoRule; -import java.util.HashMap; import java.util.List; -import java.util.Map; import static java.util.Collections.singletonList; import static org.junit.Assert.assertEquals; @@ -49,12 +47,8 @@ public class StrutsVelocityContextTest { @Mock private ValueStack stack; - private Map stackContext; - @Before public void setUp() throws Exception { - stackContext = new HashMap<>(); - when(stack.getContext()).thenReturn(stackContext); strutsVelocityContext = new StrutsVelocityContext(singletonList(chainedContext), stack); } @@ -70,12 +64,6 @@ public class StrutsVelocityContextTest { assertEquals("bar", strutsVelocityContext.internalGet("foo")); } - @Test - public void getStackContextValue() { - stackContext.put("foo", "bar"); - assertEquals("bar", strutsVelocityContext.internalGet("foo")); - } - @Test public void getSuperValue() { strutsVelocityContext.put("foo", "bar"); @@ -84,9 +72,6 @@ public class StrutsVelocityContextTest { @Test public void getValuePrecedence() { - stackContext.put("foo", "quux"); - assertEquals("quux", strutsVelocityContext.internalGet("foo")); - when(stack.findValue("foo")).thenReturn("qux"); assertEquals("qux", strutsVelocityContext.internalGet("foo"));