From a2b4744c3a0bfe517f1135f0407ded5b45f5bb96 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Mon, 13 Nov 2023 23:58:41 +1100 Subject: [PATCH 1/4] 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 2/4] 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 3/4] 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 4/4] 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")); + } }