From 709434eebc106c48b115550a0826ae12a61fa1fb Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Mon, 21 Aug 2023 19:04:10 +1000 Subject: [PATCH 01/10] WW-5336 Tidy up FreemarkerManager --- .../views/freemarker/FreemarkerManager.java | 40 +++++++++++++------ 1 file changed, 27 insertions(+), 13 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/views/freemarker/FreemarkerManager.java b/core/src/main/java/org/apache/struts2/views/freemarker/FreemarkerManager.java index 7f72d0b1f..62c7cc9ae 100644 --- a/core/src/main/java/org/apache/struts2/views/freemarker/FreemarkerManager.java +++ b/core/src/main/java/org/apache/struts2/views/freemarker/FreemarkerManager.java @@ -24,16 +24,24 @@ import com.opensymphony.xwork2.inject.Container; import com.opensymphony.xwork2.inject.Inject; import com.opensymphony.xwork2.util.ClassLoaderUtil; import com.opensymphony.xwork2.util.ValueStack; -import freemarker.cache.*; +import freemarker.cache.ClassTemplateLoader; +import freemarker.cache.FileTemplateLoader; +import freemarker.cache.MultiTemplateLoader; +import freemarker.cache.TemplateLoader; +import freemarker.cache.WebappTemplateLoader; import freemarker.core.HTMLOutputFormat; -import freemarker.core.OutputFormat; import freemarker.core.TemplateClassResolver; import freemarker.ext.jsp.TaglibFactory; import freemarker.ext.servlet.HttpRequestHashModel; import freemarker.ext.servlet.HttpRequestParametersHashModel; import freemarker.ext.servlet.HttpSessionHashModel; import freemarker.ext.servlet.ServletContextHashModel; -import freemarker.template.*; +import freemarker.template.Configuration; +import freemarker.template.ObjectWrapper; +import freemarker.template.TemplateException; +import freemarker.template.TemplateExceptionHandler; +import freemarker.template.TemplateModel; +import freemarker.template.Version; import freemarker.template.utility.StringUtil; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -51,7 +59,13 @@ import java.io.File; import java.io.IOException; import java.io.InputStream; import java.text.SimpleDateFormat; -import java.util.*; +import java.util.Calendar; +import java.util.Collections; +import java.util.GregorianCalendar; +import java.util.HashMap; +import java.util.Map; +import java.util.Properties; +import java.util.Set; /** *

@@ -176,27 +190,27 @@ public class FreemarkerManager { public void setEncoding(String encoding) { this.encoding = encoding; } - + @Inject(StrutsConstants.STRUTS_FREEMARKER_WRAPPER_ALT_MAP) public void setWrapperAltMap(String val) { altMapWrapper = "true".equals(val); } - + @Inject(StrutsConstants.STRUTS_FREEMARKER_BEANWRAPPER_CACHE) public void setCacheBeanWrapper(String val) { cacheBeanWrapper = "true".equals(val); } - + @Inject(StrutsConstants.STRUTS_FREEMARKER_MRU_MAX_STRONG_SIZE) public void setMruMaxStrongSize(String size) { mruMaxStrongSize = Integer.parseInt(size); } - + @Inject(value = StrutsConstants.STRUTS_FREEMARKER_TEMPLATES_CACHE_UPDATE_DELAY, required = false) public void setTemplateUpdateDelay(String delay) { templateUpdateDelay = delay; } - + @Inject public void setContainer(Container container) { Map map = new HashMap<>(); @@ -281,8 +295,8 @@ public class FreemarkerManager { loadSettings(servletContext); } - /** - * Sets the Freemarker Configuration's template loader with the FreemarkerThemeTemplateLoader + /** + * Sets the Freemarker Configuration's template loader with the FreemarkerThemeTemplateLoader * at the top. * * @param templateLoader the template loader @@ -293,7 +307,7 @@ public class FreemarkerManager { themeTemplateLoader.init(templateLoader); config.setTemplateLoader(themeTemplateLoader); } - + /** * Create the instance of the freemarker Configuration object. *

@@ -543,7 +557,7 @@ public class FreemarkerManager { protected void populateContext(ScopesHashModel model, ValueStack stack, Object action, HttpServletRequest request, HttpServletResponse response) { // put the same objects into the context that the velocity result uses - Map standard = ContextUtil.getStandardContext(stack, request, response); + Map standard = ContextUtil.getStandardContext(stack, request, response); model.putAll(standard); // support for JSP exception pages, exposing the servlet or JSP exception From e8614a99dd69fac81af1ac6fec4e413faef970ab Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Mon, 21 Aug 2023 19:06:01 +1000 Subject: [PATCH 02/10] WW-5336 Deprecate OgnlTool --- .../java/org/apache/struts2/util/StrutsUtil.java | 16 +++++++++++++++- .../apache/struts2/views/jsp/ui/OgnlTool.java | 10 +++++++--- core/src/main/resources/struts-beans.xml | 2 +- 3 files changed, 23 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/util/StrutsUtil.java b/core/src/main/java/org/apache/struts2/util/StrutsUtil.java index 36cfabcc9..f9fc43bf3 100644 --- a/core/src/main/java/org/apache/struts2/util/StrutsUtil.java +++ b/core/src/main/java/org/apache/struts2/util/StrutsUtil.java @@ -52,6 +52,7 @@ public class StrutsUtil { protected HttpServletResponse response; protected Map classes = new Hashtable<>(); protected OgnlTool ognl; + protected OgnlUtil ognl; protected ValueStack stack; private UrlHelper urlHelper; @@ -61,7 +62,7 @@ public class StrutsUtil { this.stack = stack; this.request = request; this.response = response; - this.ognl = stack.getActionContext().getContainer().getInstance(OgnlTool.class); + this.ognl = stack.getActionContext().getContainer().getInstance(OgnlUtil.class); this.urlHelper = stack.getActionContext().getContainer().getInstance(UrlHelper.class); this.objectFactory = stack.getActionContext().getContainer().getInstance(ObjectFactory.class); } @@ -124,6 +125,17 @@ public class StrutsUtil { return stack.findValue(expression, Class.forName(className)); } + public Object findValue(String expr, Object context) { + try { + return ognl.getValue(expr, ActionContext.getContext().getContextMap(), context); + } catch (OgnlException e) { + if (e.getReason() instanceof SecurityException) { + LOG.error(format("Could not evaluate this expression due to security constraints: [{0}]", expr), e); + } + return null; + } + } + public String getText(String text) { return (String) stack.findValue("getText('" + text.replace('\'', '"') + "')"); } @@ -186,6 +198,7 @@ public class StrutsUtil { } else { key = ognl.findValue(listKey, element); } + key = findValue(listKey, element); Object value = null; @@ -200,6 +213,7 @@ public class StrutsUtil { if ((value != null) && (selectedItems != null) && selectedItems.contains(value)) { isSelected = true; } + value = findValue(listValue, element); selectList.add(new ListEntry(key, value, isSelected)); } diff --git a/core/src/main/java/org/apache/struts2/views/jsp/ui/OgnlTool.java b/core/src/main/java/org/apache/struts2/views/jsp/ui/OgnlTool.java index e77665210..bb9d2b5b8 100644 --- a/core/src/main/java/org/apache/struts2/views/jsp/ui/OgnlTool.java +++ b/core/src/main/java/org/apache/struts2/views/jsp/ui/OgnlTool.java @@ -19,16 +19,16 @@ package org.apache.struts2.views.jsp.ui; import com.opensymphony.xwork2.ActionContext; -import ognl.OgnlException; - import com.opensymphony.xwork2.inject.Inject; import com.opensymphony.xwork2.ognl.OgnlUtil; +import ognl.OgnlException; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; /** - * FIXME: remove? + * @deprecated since 6.3.0. Use {@link org.apache.struts2.util.StrutsUtil} instead. */ +@Deprecated public class OgnlTool { private static final Logger LOG = LogManager.getLogger(OgnlTool.class); @@ -43,6 +43,10 @@ public class OgnlTool { this.ognlUtil = ognlUtil; } + /** + * @deprecated since 6.3.0. Use {@link org.apache.struts2.util.StrutsUtil#findValue(String, Object)} instead. + */ + @Deprecated public Object findValue(String expr, Object context) { try { return ognlUtil.getValue(expr, ActionContext.getContext().getContextMap(), context); diff --git a/core/src/main/resources/struts-beans.xml b/core/src/main/resources/struts-beans.xml index 7f98e4db5..9d8de3bfb 100644 --- a/core/src/main/resources/struts-beans.xml +++ b/core/src/main/resources/struts-beans.xml @@ -198,7 +198,7 @@ - + From 12cd92cb62d6d644781995d0d26e9c76e64cff3e Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Mon, 21 Aug 2023 19:08:12 +1000 Subject: [PATCH 03/10] WW-5336 Clean up StrutsUtil --- .../org/apache/struts2/util/StrutsUtil.java | 135 ++++++++---------- .../apache/struts2/util/StrutsUtilTest.java | 36 +++-- 2 files changed, 81 insertions(+), 90 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/util/StrutsUtil.java b/core/src/main/java/org/apache/struts2/util/StrutsUtil.java index f9fc43bf3..70396c46b 100644 --- a/core/src/main/java/org/apache/struts2/util/StrutsUtil.java +++ b/core/src/main/java/org/apache/struts2/util/StrutsUtil.java @@ -18,14 +18,16 @@ */ package org.apache.struts2.util; +import com.opensymphony.xwork2.ActionContext; import com.opensymphony.xwork2.ObjectFactory; +import com.opensymphony.xwork2.ognl.OgnlUtil; import com.opensymphony.xwork2.util.ClassLoaderUtil; import com.opensymphony.xwork2.util.TextParseUtil; import com.opensymphony.xwork2.util.ValueStack; +import ognl.OgnlException; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.apache.struts2.StrutsException; -import org.apache.struts2.views.jsp.ui.OgnlTool; import org.apache.struts2.views.util.UrlHelper; import javax.servlet.RequestDispatcher; @@ -39,7 +41,16 @@ import java.io.PrintWriter; import java.io.StringWriter; import java.io.UnsupportedEncodingException; import java.net.URLEncoder; -import java.util.*; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collection; +import java.util.Hashtable; +import java.util.List; +import java.util.Map; + +import static java.text.MessageFormat.format; +import static java.util.Collections.emptyList; +import static java.util.Collections.singletonList; /** * Struts base utility class, for use in Velocity and Freemarker templates @@ -50,13 +61,12 @@ public class StrutsUtil { protected HttpServletRequest request; protected HttpServletResponse response; - protected Map classes = new Hashtable<>(); - protected OgnlTool ognl; + protected Map> classes = new Hashtable<>(); protected OgnlUtil ognl; protected ValueStack stack; - private UrlHelper urlHelper; - private ObjectFactory objectFactory; + private final UrlHelper urlHelper; + private final ObjectFactory objectFactory; public StrutsUtil(ValueStack stack, HttpServletRequest request, HttpServletResponse response) { this.stack = stack; @@ -67,16 +77,14 @@ public class StrutsUtil { this.objectFactory = stack.getActionContext().getContainer().getInstance(ObjectFactory.class); } - public Object bean(Object aName) throws Exception { - String name = aName.toString(); - Class c = classes.get(name); - - if (c == null) { - c = ClassLoaderUtil.loadClass(name, StrutsUtil.class); - classes.put(name, c); + public Object bean(Object name) throws Exception { + String className = name.toString(); + Class clazz = classes.get(className); + if (clazz == null) { + clazz = ClassLoaderUtil.loadClass(className, StrutsUtil.class); + classes.put(className, clazz); } - - return objectFactory.buildBean(c, stack.getContext()); + return objectFactory.buildBean(clazz, stack.getContext()); } public boolean isTrue(String expression) { @@ -89,30 +97,20 @@ public class StrutsUtil { } public String include(Object aName) throws Exception { - try { - RequestDispatcher dispatcher = request.getRequestDispatcher(aName.toString()); - - if (dispatcher == null) { - throw new IllegalArgumentException("Cannot find included file " + aName); - } - - ResponseWrapper responseWrapper = new ResponseWrapper(response); - - dispatcher.include(request, responseWrapper); - - return responseWrapper.getData(); - } - catch (Exception e) { - LOG.debug("Cannot include {}", aName, e); - throw e; + RequestDispatcher dispatcher = request.getRequestDispatcher(aName.toString()); + if (dispatcher == null) { + throw new IllegalArgumentException("Cannot find included file " + aName); } + ResponseWrapper responseWrapper = new ResponseWrapper(response); + dispatcher.include(request, responseWrapper); + return responseWrapper.getData(); } public String urlEncode(String s) { try { return URLEncoder.encode(s, "UTF-8"); } catch (UnsupportedEncodingException e) { - LOG.debug("Cannot encode URL [{}]", s, e); + LOG.debug(format("Cannot encode URL [{0}]", s), e); return s; } } @@ -144,7 +142,7 @@ public class StrutsUtil { * @return the url ContextPath. An empty string if one does not exist. */ public String getContext() { - return (request == null)? "" : request.getContextPath(); + return request == null ? "" : request.getContextPath(); } public String translateVariables(String expression) { @@ -168,8 +166,13 @@ public class StrutsUtil { * to use as the value of the ListEntry * @return a List of ListEntry */ - public List makeSelectList(String selectedList, String list, String listKey, String listValue) { - List selectList = new ArrayList(); + public List makeSelectList(String selectedList, String list, String listKey, String listValue) { + List selectList = new ArrayList<>(); + + Collection items = (Collection) stack.findValue(list); + if (items == null) { + return selectList; + } Collection selectedItems = null; @@ -181,42 +184,27 @@ public class StrutsUtil { } else if (i instanceof Collection) { selectedItems = (Collection) i; } else { - // treat it is a single item - selectedItems = new ArrayList(); - selectedItems.add(i); + selectedItems = singletonList(i); } } - Collection items = (Collection) stack.findValue(list); - - if (items != null) { - for (Object element : items) { - Object key; - - if ((listKey == null) || (listKey.length() == 0)) { - key = element; - } else { - key = ognl.findValue(listKey, element); - } + for (Object element : items) { + Object key; + if (listKey == null || listKey.isEmpty()) { + key = element; + } else { key = findValue(listKey, element); - - Object value = null; - - if ((listValue == null) || (listValue.length() == 0)) { - value = element; - } else { - value = ognl.findValue(listValue, element); - } - - boolean isSelected = false; - - if ((value != null) && (selectedItems != null) && selectedItems.contains(value)) { - isSelected = true; - } - value = findValue(listValue, element); - - selectList.add(new ListEntry(key, value, isSelected)); } + + Object value; + if (listValue == null || listValue.isEmpty()) { + value = element; + } else { + value = findValue(listValue, element); + } + boolean isSelected = value != null && selectedItems != null && selectedItems.contains(value); + + selectList.add(new ListEntry(key, value, isSelected)); } return selectList; @@ -227,14 +215,13 @@ public class StrutsUtil { } public long toLong(int anInt) { - return (long) anInt; + return anInt; } public long toLong(String aLong) { - if (aLong == null) { + if (aLong == null || aLong.isEmpty()) { return 0; } - return Long.parseLong(aLong); } @@ -247,14 +234,7 @@ public class StrutsUtil { } public String toStringSafe(Object obj) { - try { - if (obj != null) { - return String.valueOf(obj); - } - return ""; - } catch (Exception e) { - return "Exception thrown: " + e; - } + return obj == null ? "" : obj.toString(); } static class ResponseWrapper extends HttpServletResponseWrapper { @@ -271,7 +251,6 @@ public class StrutsUtil { public String getData() { writer.flush(); - return strout.toString(); } diff --git a/core/src/test/java/org/apache/struts2/util/StrutsUtilTest.java b/core/src/test/java/org/apache/struts2/util/StrutsUtilTest.java index 4cf0abb82..c456ab7a3 100644 --- a/core/src/test/java/org/apache/struts2/util/StrutsUtilTest.java +++ b/core/src/test/java/org/apache/struts2/util/StrutsUtilTest.java @@ -32,6 +32,8 @@ import javax.servlet.ServletResponse; import java.util.ArrayList; import java.util.List; +import static org.assertj.core.api.Assertions.assertThat; + /** * Test case for StrutsUtil. * @@ -146,8 +148,8 @@ public class StrutsUtilTest extends StrutsInternalTestCase { public void testMakeSelectListMethod() { - String[] selectedList = new String[] { "Car", "Airplane", "Bus" }; - List list = new ArrayList(); + String[] selectedList = new String[]{"Car", "Airplane", "Bus"}; + List list = new ArrayList<>(); list.add("Lorry"); list.add("Car"); list.add("Helicopter"); @@ -155,18 +157,18 @@ public class StrutsUtilTest extends StrutsInternalTestCase { stack.getContext().put("mySelectedList", selectedList); stack.getContext().put("myList", list); - List listMade = strutsUtil.makeSelectList("#mySelectedList", "#myList", null, null); + List listMade = strutsUtil.makeSelectList("#mySelectedList", "#myList", null, null); assertEquals(listMade.size(), 3); - assertEquals(((ListEntry)listMade.get(0)).getKey(), "Lorry"); - assertEquals(((ListEntry)listMade.get(0)).getValue(), "Lorry"); - assertFalse(((ListEntry) listMade.get(0)).getIsSelected()); - assertEquals(((ListEntry)listMade.get(1)).getKey(), "Car"); - assertEquals(((ListEntry)listMade.get(1)).getValue(), "Car"); - assertTrue(((ListEntry) listMade.get(1)).getIsSelected()); - assertEquals(((ListEntry)listMade.get(2)).getKey(), "Helicopter"); - assertEquals(((ListEntry)listMade.get(2)).getValue(), "Helicopter"); - assertFalse(((ListEntry) listMade.get(2)).getIsSelected()); + assertEquals(listMade.get(0).getKey(), "Lorry"); + assertEquals(listMade.get(0).getValue(), "Lorry"); + assertFalse(listMade.get(0).getIsSelected()); + assertEquals(listMade.get(1).getKey(), "Car"); + assertEquals(listMade.get(1).getValue(), "Car"); + assertTrue(listMade.get(1).getIsSelected()); + assertEquals(listMade.get(2).getKey(), "Helicopter"); + assertEquals(listMade.get(2).getValue(), "Helicopter"); + assertFalse(listMade.get(2).getIsSelected()); } public void testToInt() { @@ -178,12 +180,22 @@ public class StrutsUtilTest extends StrutsInternalTestCase { assertEquals(strutsUtil.toLong(11), 11L); } + public void testStringToLong() { + assertEquals(11L, strutsUtil.toLong("11")); + assertEquals(0L, strutsUtil.toLong(null)); + assertEquals(0L, strutsUtil.toLong("")); + } public void testToString() { assertEquals(strutsUtil.toString(1), "1"); assertEquals(strutsUtil.toString(11L), "11"); } + public void testToStringSafe() { + assertEquals("1", strutsUtil.toStringSafe(1)); + assertEquals("", strutsUtil.toStringSafe(null)); + } + public void testTranslateVariables() { stack.push(new Object() { public String getFoo() { From 85e8d819ef6002208c0fa8e8d5b8f74a892e8e27 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Mon, 21 Aug 2023 19:10:40 +1000 Subject: [PATCH 04/10] WW-5336 Deprecate OGNL in template context --- .../java/org/apache/struts2/views/util/ContextUtil.java | 6 +++--- core/src/main/resources/struts.vm | 2 +- .../org/apache/struts2/views/velocity/VelocityManager.java | 6 +++++- .../apache/struts2/views/velocity/VelocityManagerTest.java | 3 +-- 4 files changed, 10 insertions(+), 7 deletions(-) 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 941321c4d..97c148f37 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 @@ -21,7 +21,6 @@ package org.apache.struts2.views.util; import com.opensymphony.xwork2.ActionInvocation; import com.opensymphony.xwork2.util.ValueStack; import org.apache.struts2.util.StrutsUtil; -import org.apache.struts2.views.jsp.ui.OgnlTool; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; @@ -48,8 +47,9 @@ public class ContextUtil { map.put(SESSION, req.getSession(false)); map.put(BASE, req.getContextPath()); map.put(STACK, stack); - map.put(OGNL, stack.getActionContext().getContainer().getInstance(OgnlTool.class)); - map.put(STRUTS, new StrutsUtil(stack, req, res)); + StrutsUtil util = new StrutsUtil(stack, req, res); + map.put(STRUTS, util); + map.put(OGNL, util); // Deprecated since 6.3.0 ActionInvocation invocation = stack.getActionContext().getActionInvocation(); if (invocation != null) { diff --git a/core/src/main/resources/struts.vm b/core/src/main/resources/struts.vm index 0b198e990..a92eb70d9 100644 --- a/core/src/main/resources/struts.vm +++ b/core/src/main/resources/struts.vm @@ -33,5 +33,5 @@ #end #macro(property $object $property) -$!{ognl.findValue($property, $object)} +$!{struts.findValue($property, $object)} #end diff --git a/plugins/velocity/src/main/java/org/apache/struts2/views/velocity/VelocityManager.java b/plugins/velocity/src/main/java/org/apache/struts2/views/velocity/VelocityManager.java index 8c8ee17bf..67f6e6919 100644 --- a/plugins/velocity/src/main/java/org/apache/struts2/views/velocity/VelocityManager.java +++ b/plugins/velocity/src/main/java/org/apache/struts2/views/velocity/VelocityManager.java @@ -58,6 +58,8 @@ import static java.util.stream.Collectors.joining; import static java.util.stream.Collectors.toList; import static org.apache.struts2.views.util.ContextUtil.STRUTS; +import static org.apache.struts2.views.util.ContextUtil.OGNL; + /** * Manages the environment for Velocity result types */ @@ -141,7 +143,9 @@ public class VelocityManager { List chainedContexts = prepareChainedContexts(req, res, stack.getContext()); Context context = new StrutsVelocityContext(chainedContexts, stack); ContextUtil.getStandardContext(stack, req, res).forEach(context::put); - context.put(STRUTS, new VelocityStrutsUtil(velocityEngine, context, stack, req, res)); + VelocityStrutsUtil util = new VelocityStrutsUtil(velocityEngine, context, stack, req, res); + context.put(STRUTS, util); + context.put(OGNL, util); // Deprecated since 6.3.0 return context; } diff --git a/plugins/velocity/src/test/java/org/apache/struts2/views/velocity/VelocityManagerTest.java b/plugins/velocity/src/test/java/org/apache/struts2/views/velocity/VelocityManagerTest.java index d550e9efb..b86ff2eec 100644 --- a/plugins/velocity/src/test/java/org/apache/struts2/views/velocity/VelocityManagerTest.java +++ b/plugins/velocity/src/test/java/org/apache/struts2/views/velocity/VelocityManagerTest.java @@ -22,7 +22,6 @@ import com.opensymphony.xwork2.ActionContext; import com.opensymphony.xwork2.util.ValueStack; import org.apache.struts2.ServletActionContext; import org.apache.struts2.junit.StrutsJUnit4TestCase; -import org.apache.struts2.views.jsp.ui.OgnlTool; import org.apache.velocity.context.Context; import org.apache.velocity.tools.ToolContext; import org.junit.After; @@ -95,7 +94,7 @@ public class VelocityManagerTest extends StrutsJUnit4TestCase { assertNotNull(context); assertThat(context.get("struts")).isInstanceOf(VelocityStrutsUtil.class); - assertThat(context.get("ognl")).isInstanceOf(OgnlTool.class); + assertThat(context.get("ognl")).isInstanceOf(VelocityStrutsUtil.class); // Deprecated since 6.3.0 assertThat(context.get("stack")).isInstanceOf(ValueStack.class); assertThat(context.get("request")).isInstanceOf(HttpServletRequest.class); assertThat(context.get("response")).isInstanceOf(HttpServletResponse.class); From 0f068d37aea4755a5b301e77d311e89aee05017a Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Tue, 22 Aug 2023 00:11:24 +1000 Subject: [PATCH 05/10] WW-5336 Reduce cognitive complexity #makeSelectList --- .../org/apache/struts2/util/StrutsUtil.java | 60 ++++++++++--------- .../apache/struts2/util/StrutsUtilTest.java | 35 +++++++++-- 2 files changed, 60 insertions(+), 35 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/util/StrutsUtil.java b/core/src/main/java/org/apache/struts2/util/StrutsUtil.java index 70396c46b..1d2c4079b 100644 --- a/core/src/main/java/org/apache/struts2/util/StrutsUtil.java +++ b/core/src/main/java/org/apache/struts2/util/StrutsUtil.java @@ -174,42 +174,44 @@ public class StrutsUtil { return selectList; } - Collection selectedItems = null; - - Object i = stack.findValue(selectedList); - - if (i != null) { - if (i.getClass().isArray()) { - selectedItems = Arrays.asList((Object[]) i); - } else if (i instanceof Collection) { - selectedItems = (Collection) i; - } else { - selectedItems = singletonList(i); - } - } - + Collection selectedItems = getSelectedItems(selectedList); for (Object element : items) { - Object key; - if (listKey == null || listKey.isEmpty()) { - key = element; - } else { - key = findValue(listKey, element); - } - - Object value; - if (listValue == null || listValue.isEmpty()) { - value = element; - } else { - value = findValue(listValue, element); - } - boolean isSelected = value != null && selectedItems != null && selectedItems.contains(value); - + Object key = computeKey(listKey, element); + Object value = computeValue(listValue, element); + boolean isSelected = value != null && selectedItems.contains(value); selectList.add(new ListEntry(key, value, isSelected)); } return selectList; } + private Collection getSelectedItems(String selectedListName) { + Object i = stack.findValue(selectedListName); + if (i == null) { + return emptyList(); + } + if (i.getClass().isArray()) { + return Arrays.asList((Object[]) i); + } else if (i instanceof Collection) { + return (Collection) i; + } + return singletonList(i); + } + + private Object computeKey(String listKey, Object element) { + if (listKey == null || listKey.isEmpty()) { + return element; + } + return findValue(listKey, element); + } + + private Object computeValue(String listValue, Object element) { + if (listValue == null || listValue.isEmpty()) { + return element; + } + return findValue(listValue, element); + } + public int toInt(long aLong) { return (int) aLong; } diff --git a/core/src/test/java/org/apache/struts2/util/StrutsUtilTest.java b/core/src/test/java/org/apache/struts2/util/StrutsUtilTest.java index c456ab7a3..f6a0debe9 100644 --- a/core/src/test/java/org/apache/struts2/util/StrutsUtilTest.java +++ b/core/src/test/java/org/apache/struts2/util/StrutsUtilTest.java @@ -29,7 +29,7 @@ import org.springframework.mock.web.MockRequestDispatcher; import javax.servlet.RequestDispatcher; import javax.servlet.ServletRequest; import javax.servlet.ServletResponse; -import java.util.ArrayList; +import java.util.Arrays; import java.util.List; import static org.assertj.core.api.Assertions.assertThat; @@ -147,12 +147,9 @@ public class StrutsUtilTest extends StrutsInternalTestCase { } - public void testMakeSelectListMethod() { + public void testMakeSelectList() { String[] selectedList = new String[]{"Car", "Airplane", "Bus"}; - List list = new ArrayList<>(); - list.add("Lorry"); - list.add("Car"); - list.add("Helicopter"); + List list = Arrays.asList("Lorry", "Car", "Helicopter"); stack.getContext().put("mySelectedList", selectedList); stack.getContext().put("myList", list); @@ -171,6 +168,32 @@ public class StrutsUtilTest extends StrutsInternalTestCase { assertFalse(listMade.get(2).getIsSelected()); } + public void testMakeSelectListCollection() { + List selectedList = Arrays.asList("Car", "Airplane", "Bus"); + List list = Arrays.asList("Lorry", "Car", "Helicopter"); + + stack.getContext().put("mySelectedList", selectedList); + stack.getContext().put("myList", list); + + List listMade = strutsUtil.makeSelectList("#mySelectedList", "#myList", null, null); + + assertEquals(listMade.size(), 3); + assertEquals(listMade.get(0).getKey(), "Lorry"); + assertEquals(listMade.get(0).getValue(), "Lorry"); + assertFalse(listMade.get(0).getIsSelected()); + assertEquals(listMade.get(1).getKey(), "Car"); + assertEquals(listMade.get(1).getValue(), "Car"); + assertTrue(listMade.get(1).getIsSelected()); + assertEquals(listMade.get(2).getKey(), "Helicopter"); + assertEquals(listMade.get(2).getValue(), "Helicopter"); + assertFalse(listMade.get(2).getIsSelected()); + } + + public void testMakeSelectListNonExistent() { + List listMade = strutsUtil.makeSelectList("#mySelectedList", "#nonexistent", null, null); + assertThat(listMade).isEmpty(); + } + public void testToInt() { assertEquals(strutsUtil.toInt(11L), 11); } From 6b13e8ac3aadbdd08b6afac989df16c83be6f0ac Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Tue, 22 Aug 2023 11:32:42 +1000 Subject: [PATCH 06/10] WW-5336 Switch to HashMap as concurrency handling not required --- core/src/main/java/org/apache/struts2/util/StrutsUtil.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/util/StrutsUtil.java b/core/src/main/java/org/apache/struts2/util/StrutsUtil.java index 1d2c4079b..727d67a26 100644 --- a/core/src/main/java/org/apache/struts2/util/StrutsUtil.java +++ b/core/src/main/java/org/apache/struts2/util/StrutsUtil.java @@ -44,7 +44,7 @@ import java.net.URLEncoder; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; -import java.util.Hashtable; +import java.util.HashMap; import java.util.List; import java.util.Map; @@ -61,7 +61,7 @@ public class StrutsUtil { protected HttpServletRequest request; protected HttpServletResponse response; - protected Map> classes = new Hashtable<>(); + protected Map> classes = new HashMap<>(); protected OgnlUtil ognl; protected ValueStack stack; From 35b0a6887fc2299c51fa5da9d8f2fd873086228f Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Tue, 22 Aug 2023 11:55:26 +1000 Subject: [PATCH 07/10] WW-5336 Update JavaDoc VelocityManager --- .../views/velocity/VelocityManager.java | 22 ++++--------------- 1 file changed, 4 insertions(+), 18 deletions(-) diff --git a/plugins/velocity/src/main/java/org/apache/struts2/views/velocity/VelocityManager.java b/plugins/velocity/src/main/java/org/apache/struts2/views/velocity/VelocityManager.java index 67f6e6919..b702d2249 100644 --- a/plugins/velocity/src/main/java/org/apache/struts2/views/velocity/VelocityManager.java +++ b/plugins/velocity/src/main/java/org/apache/struts2/views/velocity/VelocityManager.java @@ -29,7 +29,6 @@ import org.apache.struts2.ServletActionContext; import org.apache.struts2.StrutsConstants; import org.apache.struts2.StrutsException; import org.apache.struts2.views.TagLibraryDirectiveProvider; -import org.apache.struts2.views.jsp.ui.OgnlTool; import org.apache.struts2.views.util.ContextUtil; import org.apache.velocity.VelocityContext; import org.apache.velocity.app.Velocity; @@ -56,9 +55,8 @@ import java.util.Properties; import static java.lang.String.format; import static java.util.stream.Collectors.joining; import static java.util.stream.Collectors.toList; -import static org.apache.struts2.views.util.ContextUtil.STRUTS; - import static org.apache.struts2.views.util.ContextUtil.OGNL; +import static org.apache.struts2.views.util.ContextUtil.STRUTS; /** * Manages the environment for Velocity result types @@ -103,27 +101,15 @@ public class VelocityManager { } /** - * @return a reference to the VelocityEngine used by all struts velocity thingies with the exception of - * directly accessed *.vm pages + * @return a reference to the VelocityEngine used by all Struts Velocity results except directly + * accessed *.vm pages (unless otherwise configured) */ public VelocityEngine getVelocityEngine() { return velocityEngine; } /** - *

- * This method is responsible for creating the standard VelocityContext used by all WW2 velocity views. The - * following context parameters are defined: - *

- * - * + * This method is responsible for creating the standard VelocityContext used by all Struts Velocity views. * * @param stack the current {@link ValueStack} * @param req the current HttpServletRequest From 3b515b1a781e5335a29cea33b7c74bf8a82aefcc Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Tue, 22 Aug 2023 16:34:29 +1000 Subject: [PATCH 08/10] WW-5336 Fix visibility warnings --- .../test/java/org/apache/struts2/util/StrutsUtilTest.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/core/src/test/java/org/apache/struts2/util/StrutsUtilTest.java b/core/src/test/java/org/apache/struts2/util/StrutsUtilTest.java index f6a0debe9..de1ce844d 100644 --- a/core/src/test/java/org/apache/struts2/util/StrutsUtilTest.java +++ b/core/src/test/java/org/apache/struts2/util/StrutsUtilTest.java @@ -268,7 +268,7 @@ public class StrutsUtilTest extends StrutsInternalTestCase { // === internal class to assist in testing - static class InternalMockHttpServletRequest extends MockHttpServletRequest { + protected static class InternalMockHttpServletRequest extends MockHttpServletRequest { InternalMockRequestDispatcher dispatcher = null; public RequestDispatcher getRequestDispatcher(String path) { dispatcher = new InternalMockRequestDispatcher(path); @@ -280,8 +280,8 @@ public class StrutsUtilTest extends StrutsInternalTestCase { } } - static class InternalMockRequestDispatcher extends MockRequestDispatcher { - private String url; + protected static class InternalMockRequestDispatcher extends MockRequestDispatcher { + private final String url; boolean included = false; public InternalMockRequestDispatcher(String url) { super(url); From 215541146d894b70e7315377b64def355533e248 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Tue, 22 Aug 2023 16:35:29 +1000 Subject: [PATCH 09/10] WW-5336 Correct assertions and add test case --- .../apache/struts2/util/StrutsUtilTest.java | 57 ++++++++++++------- 1 file changed, 35 insertions(+), 22 deletions(-) diff --git a/core/src/test/java/org/apache/struts2/util/StrutsUtilTest.java b/core/src/test/java/org/apache/struts2/util/StrutsUtilTest.java index de1ce844d..bd1de3313 100644 --- a/core/src/test/java/org/apache/struts2/util/StrutsUtilTest.java +++ b/core/src/test/java/org/apache/struts2/util/StrutsUtilTest.java @@ -30,6 +30,7 @@ import javax.servlet.RequestDispatcher; import javax.servlet.ServletRequest; import javax.servlet.ServletResponse; import java.util.Arrays; +import java.util.LinkedHashMap; import java.util.List; import static org.assertj.core.api.Assertions.assertThat; @@ -148,7 +149,7 @@ public class StrutsUtilTest extends StrutsInternalTestCase { public void testMakeSelectList() { - String[] selectedList = new String[]{"Car", "Airplane", "Bus"}; + String[] selectedList = new String[]{"Car", "Airplane", "Bus"}; // Array List list = Arrays.asList("Lorry", "Car", "Helicopter"); stack.getContext().put("mySelectedList", selectedList); @@ -156,20 +157,15 @@ public class StrutsUtilTest extends StrutsInternalTestCase { List listMade = strutsUtil.makeSelectList("#mySelectedList", "#myList", null, null); - assertEquals(listMade.size(), 3); - assertEquals(listMade.get(0).getKey(), "Lorry"); - assertEquals(listMade.get(0).getValue(), "Lorry"); - assertFalse(listMade.get(0).getIsSelected()); - assertEquals(listMade.get(1).getKey(), "Car"); - assertEquals(listMade.get(1).getValue(), "Car"); - assertTrue(listMade.get(1).getIsSelected()); - assertEquals(listMade.get(2).getKey(), "Helicopter"); - assertEquals(listMade.get(2).getValue(), "Helicopter"); - assertFalse(listMade.get(2).getIsSelected()); + LinkedHashMap expectedItems = new LinkedHashMap<>(); + expectedItems.put("Lorry", false); + expectedItems.put("Car", true); + expectedItems.put("Helicopter", false); + makeSelectListCommonAssertions(listMade, expectedItems); } public void testMakeSelectListCollection() { - List selectedList = Arrays.asList("Car", "Airplane", "Bus"); + List selectedList = Arrays.asList("Airplane", "Helicopter", "Bus"); // Collection List list = Arrays.asList("Lorry", "Car", "Helicopter"); stack.getContext().put("mySelectedList", selectedList); @@ -177,16 +173,33 @@ public class StrutsUtilTest extends StrutsInternalTestCase { List listMade = strutsUtil.makeSelectList("#mySelectedList", "#myList", null, null); - assertEquals(listMade.size(), 3); - assertEquals(listMade.get(0).getKey(), "Lorry"); - assertEquals(listMade.get(0).getValue(), "Lorry"); - assertFalse(listMade.get(0).getIsSelected()); - assertEquals(listMade.get(1).getKey(), "Car"); - assertEquals(listMade.get(1).getValue(), "Car"); - assertTrue(listMade.get(1).getIsSelected()); - assertEquals(listMade.get(2).getKey(), "Helicopter"); - assertEquals(listMade.get(2).getValue(), "Helicopter"); - assertFalse(listMade.get(2).getIsSelected()); + LinkedHashMap expectedItems = new LinkedHashMap<>(); + expectedItems.put("Lorry", false); + expectedItems.put("Car", false); + expectedItems.put("Helicopter", true); + makeSelectListCommonAssertions(listMade, expectedItems); + } + + public void testMakeSelectListSingleton() { + String selectedItem = "Lorry"; // Singleton + List list = Arrays.asList("Lorry", "Car", "Helicopter"); + + stack.getContext().put("mySelectedList", selectedItem); + stack.getContext().put("myList", list); + + List listMade = strutsUtil.makeSelectList("#mySelectedList", "#myList", null, null); + + LinkedHashMap expectedItems = new LinkedHashMap<>(); + expectedItems.put("Lorry", true); + expectedItems.put("Car", false); + expectedItems.put("Helicopter", false); + makeSelectListCommonAssertions(listMade, expectedItems); + } + + private void makeSelectListCommonAssertions(List listMade, LinkedHashMap expectedItems) { + assertThat(listMade).extracting("key").containsExactly(expectedItems.keySet().toArray()); + assertThat(listMade).extracting("value").containsExactly(expectedItems.keySet().toArray()); + assertThat(listMade).extracting("isSelected").containsExactly(expectedItems.values().toArray()); } public void testMakeSelectListNonExistent() { From 6e5a1a07f0c6ca6b1238fb0ef414ffdd74e22af9 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Tue, 22 Aug 2023 16:37:04 +1000 Subject: [PATCH 10/10] WW-5336 Move XML comment to be clearer --- core/src/main/resources/struts-beans.xml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/core/src/main/resources/struts-beans.xml b/core/src/main/resources/struts-beans.xml index 9d8de3bfb..fc1fb2ee7 100644 --- a/core/src/main/resources/struts-beans.xml +++ b/core/src/main/resources/struts-beans.xml @@ -198,7 +198,8 @@ - + +