[ https://issues.apache.org/jira/browse/WW-5382?focusedWorklogId=897547&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-897547 ]
ASF GitHub Bot logged work on WW-5382: -------------------------------------- Author: ASF GitHub Bot Created on: 01/Jan/24 17:09 Start Date: 01/Jan/24 17:09 Worklog Time Spent: 10m Work Description: kusalk commented on code in PR #826: URL: https://github.com/apache/struts/pull/826#discussion_r1439057874 ########## core/src/main/java/com/opensymphony/xwork2/XWorkJUnit4TestCase.java: ########## @@ -51,10 +51,6 @@ public void setUp() throws Exception { @After public void tearDown() throws Exception { XWorkTestCaseHelper.tearDown(configurationManager); - configurationManager = null; Review Comment: Not necessary ########## core/src/main/java/org/apache/struts2/dispatcher/Dispatcher.java: ########## @@ -1068,22 +1100,17 @@ public ConfigurationManager getConfigurationManager() { * @return Our dependency injection container */ public Container getContainer() { - if (ContainerHolder.get() != null) { - return ContainerHolder.get(); - } - ConfigurationManager mgr = getConfigurationManager(); - if (mgr == null) { - throw new IllegalStateException("The configuration manager shouldn't be null"); - } else { - Configuration config = mgr.getConfiguration(); - if (config == null) { - throw new IllegalStateException("Unable to load configuration"); - } else { - Container container = config.getContainer(); - ContainerHolder.store(container); - return container; + if (ContainerHolder.get() == null) { + try { + ContainerHolder.store(getConfigurationManager().getConfiguration().getContainer()); + } catch (NullPointerException e) { + throw new IllegalStateException("ConfigurationManager and/or Configuration should not be null", e); } } + if (injectedContainer != ContainerHolder.get()) { Review Comment: This is the core change of this PR - we compare the retrieved container instance against the one we last injected this with, and if it has changed, reinject ########## core/src/main/java/org/apache/struts2/dispatcher/Dispatcher.java: ########## @@ -988,18 +1031,7 @@ protected boolean isMultipartRequest(HttpServletRequest request) { * @return a multi part request object */ protected MultiPartRequest getMultiPartRequest() { - MultiPartRequest mpr = null; Review Comment: Remnants from before the bean aliasing functionality existed I presume? ########## plugins/sitemesh/src/main/java/org/apache/struts2/sitemesh/OldDecorator2NewStrutsDecorator.java: ########## Review Comment: Same deal with line-endings ########## core/src/main/java/org/apache/struts2/dispatcher/MockDispatcher.java: ########## Review Comment: Deleted this class in favour of partial mocks using Mockito spy - still not ideal but a step in the right direction ########## plugins/portlet/src/main/java/org/apache/struts2/portlet/dispatcher/Jsr168Dispatcher.java: ########## Review Comment: (Changed line-endings to `LF` for this file) ########## core/src/test/java/org/apache/struts2/StrutsInternalTestCase.java: ########## @@ -38,22 +40,22 @@ public abstract class StrutsInternalTestCase extends XWorkTestCase { /** * Sets up the configuration settings, XWork configuration, and * message resources - * + * * @throws java.lang.Exception */ @Override protected void setUp() throws Exception { - super.setUp(); Review Comment: These super calls were just wasted computation as everything was replaced right after ########## core/src/main/java/org/apache/struts2/util/StrutsTestCaseHelper.java: ########## @@ -52,8 +50,16 @@ public static Dispatcher initDispatcher(ServletContext ctx, Map<String,String> p return du; } - public static void tearDown() throws Exception { - Dispatcher.setInstance(null); + public static void tearDown(Dispatcher dispatcher) { + if (dispatcher != null && dispatcher.getConfigurationManager() != null) { Review Comment: Moved this here for better reuse ########## core/src/main/java/com/opensymphony/xwork2/config/impl/DefaultConfiguration.java: ########## @@ -325,12 +325,8 @@ public Class<? extends Configuration> type() { } protected ActionContext setContext(Container cont) { - ActionContext context = ActionContext.getContext(); - if (context == null) { Review Comment: This method is only called twice, and it is always null on the first call, and never null on the second execution. I can't see why we wouldn't want to replace the bootstrap context once the final context has been constructed. Although it is also unclear if the ActionContext is even used after this point - i.e. for loading the PackageProviders. The ActionContext is cleared once the PackageProviders are loaded. ########## core/src/main/java/org/apache/struts2/dispatcher/Dispatcher.java: ########## @@ -192,6 +194,11 @@ public class Dispatcher { * Store ConfigurationManager instance, set on init. */ protected ConfigurationManager configurationManager; + private ObjectFactory objectFactory; Review Comment: We should save some computation by injecting these beans once when the Dispatcher is initialised instead of constantly retrieving them from the container ########## core/src/test/java/org/apache/struts2/dispatcher/DispatcherTest.java: ########## @@ -18,46 +18,65 @@ */ package org.apache.struts2.dispatcher; -import com.mockobjects.dynamic.C; -import com.mockobjects.dynamic.Mock; import com.opensymphony.xwork2.ActionContext; import com.opensymphony.xwork2.LocalizedTextProvider; import com.opensymphony.xwork2.ObjectFactory; import com.opensymphony.xwork2.StubValueStack; -import com.opensymphony.xwork2.config.Configuration; +import com.opensymphony.xwork2.config.ConfigurationException; import com.opensymphony.xwork2.config.ConfigurationManager; import com.opensymphony.xwork2.config.entities.InterceptorMapping; import com.opensymphony.xwork2.config.entities.InterceptorStackConfig; import com.opensymphony.xwork2.config.entities.PackageConfig; import com.opensymphony.xwork2.inject.Container; +import com.opensymphony.xwork2.inject.ContainerBuilder; import com.opensymphony.xwork2.interceptor.Interceptor; import com.opensymphony.xwork2.mock.MockActionInvocation; import com.opensymphony.xwork2.mock.MockActionProxy; +import com.opensymphony.xwork2.test.StubConfigurationProvider; +import com.opensymphony.xwork2.util.location.LocatableProperties; import org.apache.struts2.ServletActionContext; import org.apache.struts2.StrutsConstants; -import org.apache.struts2.StrutsInternalTestCase; +import org.apache.struts2.StrutsJUnit4InternalTestCase; import org.apache.struts2.dispatcher.mapper.ActionMapping; import org.apache.struts2.dispatcher.multipart.MultiPartRequestWrapper; import org.apache.struts2.util.ObjectFactoryDestroyable; +import org.junit.Test; import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.mock.web.MockHttpSession; import org.springframework.mock.web.MockServletContext; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; -import java.util.Collections; +import java.nio.charset.StandardCharsets; import java.util.HashMap; import java.util.HashSet; import java.util.Locale; import java.util.Map; import java.util.Set; +import static java.nio.charset.StandardCharsets.UTF_8; +import static java.util.Collections.emptyMap; +import static java.util.Collections.singletonMap; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyBoolean; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + /** * Test case for Dispatcher. */ -public class DispatcherTest extends StrutsInternalTestCase { +public class DispatcherTest extends StrutsJUnit4InternalTestCase { Review Comment: Upgraded to JUnit4 and replaced older mocks with Mockito ########## core/src/test/java/org/apache/struts2/dispatcher/DispatcherTest.java: ########## @@ -421,196 +397,201 @@ public void testServiceActionCreatesNewProxyIfDifferentMapping() throws Exceptio * Verify proper default (true) handleExceptionState for Dispatcher and that * it properly reflects a manually configured change to false. */ + @Test public void testHandleException() { - Dispatcher du = initDispatcher(new HashMap<>()); - assertTrue("Default Dispatcher handleException state not true ?", du.isHandleException()); + assertTrue("Default Dispatcher handleException state not true ?", dispatcher.isHandleException()); - Dispatcher du2 = initDispatcher(new HashMap<String, String>() {{ - put(StrutsConstants.STRUTS_HANDLE_EXCEPTION, "false"); - }}); - assertFalse("Modified Dispatcher handleException state not false ?", du2.isHandleException()); + initDispatcher(singletonMap(StrutsConstants.STRUTS_HANDLE_EXCEPTION, "false")); + assertFalse("Modified Dispatcher handleException state not false ?", dispatcher.isHandleException()); } /** * Verify proper default (false) devMode for Dispatcher and that * it properly reflects a manually configured change to true. */ + @Test public void testDevMode() { - Dispatcher du = initDispatcher(new HashMap<>()); - assertFalse("Default Dispatcher devMode state not false ?", du.isDevMode()); + assertFalse("Default Dispatcher devMode state not false ?", dispatcher.isDevMode()); - Dispatcher du2 = initDispatcher(new HashMap<String, String>() {{ - put(StrutsConstants.STRUTS_DEVMODE, "true"); - }}); - assertTrue("Modified Dispatcher devMode state not true ?", du2.isDevMode()); + initDispatcher(singletonMap(StrutsConstants.STRUTS_DEVMODE, "true")); + assertTrue("Modified Dispatcher devMode state not true ?", dispatcher.isDevMode()); } + @Test public void testGetLocale_With_DefaultLocale_FromConfiguration() { // Given - Mock mock = new Mock(HttpServletRequest.class); + HttpServletRequest request = mock(HttpServletRequest.class); MockHttpSession mockHttpSession = new MockHttpSession(); - mock.expectAndReturn("getCharacterEncoding", "utf-8"); // From Dispatcher prepare(). - mock.expectAndReturn("getHeader", "X-Requested-With", ""); // From Dispatcher prepare(). - mock.expectAndReturn("getParameterMap", new HashMap<String, Object>()); // From Dispatcher prepare(). - mock.expectAndReturn("getSession", false, mockHttpSession); // From Dispatcher prepare(). - mock.expectAndReturn("getSession", true, mockHttpSession); // From createTestContextMap(). - HttpServletRequest request = (HttpServletRequest) mock.proxy(); + when(request.getCharacterEncoding()).thenReturn(UTF_8.name()); + when(request.getHeader("X-Requested-With")).thenReturn(""); + when(request.getParameterMap()).thenReturn(emptyMap()); + when(request.getSession(anyBoolean())).thenReturn(mockHttpSession); HttpServletResponse response = new MockHttpServletResponse(); - Dispatcher testDispatcher = initDispatcher(new HashMap<String, String>() {{ - put(StrutsConstants.STRUTS_I18N_ENCODING, "utf-8"); - // Not setting a Struts Locale here, so we should receive the default "de_DE" from the test configuration. - }}); + // Not setting a Struts Locale here, so we should receive the default "de_DE" from the test configuration. + initDispatcher(singletonMap(StrutsConstants.STRUTS_I18N_ENCODING, UTF_8.name())); // When - testDispatcher.prepare(request, response); - ActionContext context = ActionContext.of(createTestContextMap(testDispatcher, request, response)); + dispatcher.prepare(request, response); + ActionContext context = ActionContext.of(createTestContextMap(dispatcher, request, response)); // Then assertEquals(Locale.GERMANY, context.getLocale()); // Expect the Dispatcher defaultLocale value "de_DE" from the test configuration. - mock.verify(); } + @Test public void testGetLocale_With_DefaultLocale_fr_CA() { // Given - Mock mock = new Mock(HttpServletRequest.class); + HttpServletRequest request = mock(HttpServletRequest.class); MockHttpSession mockHttpSession = new MockHttpSession(); - mock.expectAndReturn("getCharacterEncoding", "utf-8"); // From Dispatcher prepare(). - mock.expectAndReturn("getHeader", "X-Requested-With", ""); // From Dispatcher prepare(). - mock.expectAndReturn("getParameterMap", new HashMap<String, Object>()); // From Dispatcher prepare(). - mock.expectAndReturn("getSession", false, mockHttpSession); // From Dispatcher prepare(). - mock.expectAndReturn("getSession", true, mockHttpSession); // From createTestContextMap(). - HttpServletRequest request = (HttpServletRequest) mock.proxy(); + when(request.getCharacterEncoding()).thenReturn(UTF_8.name()); + when(request.getHeader("X-Requested-With")).thenReturn(""); + when(request.getParameterMap()).thenReturn(emptyMap()); + when(request.getSession(anyBoolean())).thenReturn(mockHttpSession); HttpServletResponse response = new MockHttpServletResponse(); - Dispatcher testDispatcher = initDispatcher(new HashMap<String, String>() {{ - put(StrutsConstants.STRUTS_I18N_ENCODING, "utf-8"); + initDispatcher(new HashMap<String, String>() {{ + put(StrutsConstants.STRUTS_I18N_ENCODING, UTF_8.name()); put(StrutsConstants.STRUTS_LOCALE, Locale.CANADA_FRENCH.toString()); // Set the Dispatcher defaultLocale to fr_CA. }}); // When - testDispatcher.prepare(request, response); - ActionContext context = ActionContext.of(createTestContextMap(testDispatcher, request, response)); + dispatcher.prepare(request, response); + ActionContext context = ActionContext.of(createTestContextMap(dispatcher, request, response)); // Then assertEquals(Locale.CANADA_FRENCH, context.getLocale()); // Expect the Dispatcher defaultLocale value. - mock.verify(); } + @Test public void testGetLocale_With_BadDefaultLocale_RequestLocale_en_UK() { // Given - Mock mock = new Mock(HttpServletRequest.class); + HttpServletRequest request = mock(HttpServletRequest.class); MockHttpSession mockHttpSession = new MockHttpSession(); - mock.expectAndReturn("getCharacterEncoding", "utf-8"); // From Dispatcher prepare(). - mock.expectAndReturn("getHeader", "X-Requested-With", ""); // From Dispatcher prepare(). - mock.expectAndReturn("getLocale", Locale.UK); // From Dispatcher prepare(). - mock.expectAndReturn("getParameterMap", new HashMap<String, Object>()); // From Dispatcher prepare(). - mock.expectAndReturn("getSession", false, mockHttpSession); // From Dispatcher prepare(). - mock.expectAndReturn("getSession", true, mockHttpSession); // From createTestContextMap(). - mock.expectAndReturn("getLocale", Locale.UK); // From createTestContextMap(). - HttpServletRequest request = (HttpServletRequest) mock.proxy(); + when(request.getCharacterEncoding()).thenReturn(UTF_8.name()); + when(request.getHeader("X-Requested-With")).thenReturn(""); + when(request.getParameterMap()).thenReturn(emptyMap()); + when(request.getSession(anyBoolean())).thenReturn(mockHttpSession); + when(request.getLocale()).thenReturn(Locale.UK); HttpServletResponse response = new MockHttpServletResponse(); - Dispatcher testDispatcher = initDispatcher(new HashMap<String, String>() {{ - put(StrutsConstants.STRUTS_I18N_ENCODING, "utf-8"); + initDispatcher(new HashMap<String, String>() {{ + put(StrutsConstants.STRUTS_I18N_ENCODING, UTF_8.name()); put(StrutsConstants.STRUTS_LOCALE, "This_is_not_a_valid_Locale_string"); // Set Dispatcher defaultLocale to an invalid value. }}); // When - testDispatcher.prepare(request, response); - ActionContext context = ActionContext.of(createTestContextMap(testDispatcher, request, response)); + dispatcher.prepare(request, response); + ActionContext context = ActionContext.of(createTestContextMap(dispatcher, request, response)); // Then assertEquals(Locale.UK, context.getLocale()); // Expect the request set value from Mock. - mock.verify(); } + @Test public void testGetLocale_With_BadDefaultLocale_And_RuntimeException() { // Given - Mock mock = new Mock(HttpServletRequest.class); + HttpServletRequest request = mock(HttpServletRequest.class); MockHttpSession mockHttpSession = new MockHttpSession(); - mock.expectAndReturn("getCharacterEncoding", "utf-8"); // From Dispatcher prepare(). - mock.expectAndReturn("getHeader", "X-Requested-With", ""); // From Dispatcher prepare(). - mock.expectAndReturn("getLocale", Locale.UK); // From Dispatcher prepare(). - mock.expectAndReturn("getParameterMap", new HashMap<String, Object>()); // From Dispatcher prepare(). - mock.expectAndReturn("getSession", false, mockHttpSession); // From Dispatcher prepare(). - mock.expectAndReturn("getSession", true, mockHttpSession); // From createTestContextMap(). - mock.expectAndThrow("getLocale", new IllegalStateException("Test theoretical state preventing HTTP Request Locale access")); // From createTestContextMap(). - HttpServletRequest request = (HttpServletRequest) mock.proxy(); + when(request.getCharacterEncoding()).thenReturn(UTF_8.name()); + when(request.getHeader("X-Requested-With")).thenReturn(""); + when(request.getParameterMap()).thenReturn(emptyMap()); + when(request.getSession(anyBoolean())).thenReturn(mockHttpSession); + when(request.getLocale()).thenReturn(Locale.UK); HttpServletResponse response = new MockHttpServletResponse(); - Dispatcher testDispatcher = initDispatcher(new HashMap<String, String>() {{ - put(StrutsConstants.STRUTS_I18N_ENCODING, "utf-8"); + initDispatcher(new HashMap<String, String>() {{ + put(StrutsConstants.STRUTS_I18N_ENCODING, UTF_8.name()); put(StrutsConstants.STRUTS_LOCALE, "This_is_not_a_valid_Locale_string"); // Set the Dispatcher defaultLocale to an invalid value. }}); // When - testDispatcher.prepare(request, response); - ActionContext context = ActionContext.of(createTestContextMap(testDispatcher, request, response)); + dispatcher.prepare(request, response); + when(request.getLocale()).thenThrow(new IllegalStateException("Test theoretical state preventing HTTP Request Locale access")); + ActionContext context = ActionContext.of(createTestContextMap(dispatcher, request, response)); // Then assertEquals(Locale.getDefault(), context.getLocale()); // Expect the system default value, when BOTH Dispatcher default Locale AND request access fail. - mock.verify(); } + @Test public void testGetLocale_With_NullDefaultLocale() { // Given - Mock mock = new Mock(HttpServletRequest.class); + HttpServletRequest request = mock(HttpServletRequest.class); MockHttpSession mockHttpSession = new MockHttpSession(); - mock.expectAndReturn("getCharacterEncoding", "utf-8"); // From Dispatcher prepare(). - mock.expectAndReturn("getHeader", "X-Requested-With", ""); // From Dispatcher prepare(). - mock.expectAndReturn("getLocale", Locale.CANADA_FRENCH); // From Dispatcher prepare(). - mock.expectAndReturn("getParameterMap", new HashMap<String, Object>()); // From Dispatcher prepare(). - mock.expectAndReturn("getSession", false, mockHttpSession); // From Dispatcher prepare(). - mock.expectAndReturn("getSession", true, mockHttpSession); // From createTestContextMap(). - mock.expectAndReturn("getLocale", Locale.CANADA_FRENCH); // From createTestContextMap(). - HttpServletRequest request = (HttpServletRequest) mock.proxy(); + when(request.getCharacterEncoding()).thenReturn(UTF_8.name()); + when(request.getHeader("X-Requested-With")).thenReturn(""); + when(request.getParameterMap()).thenReturn(emptyMap()); + when(request.getSession(anyBoolean())).thenReturn(mockHttpSession); + when(request.getLocale()).thenReturn(Locale.CANADA_FRENCH); HttpServletResponse response = new MockHttpServletResponse(); - Dispatcher testDispatcher = initDispatcher(new HashMap<String, String>() {{ - put(StrutsConstants.STRUTS_I18N_ENCODING, "utf-8"); - // Attempting to set StrutsConstants.STRUTS_LOCALE to null here via parameters causes an NPE. - }}); + // Attempting to set StrutsConstants.STRUTS_LOCALE to null here via parameters causes an NPE. + initDispatcher(singletonMap(StrutsConstants.STRUTS_I18N_ENCODING, UTF_8.name())); - testDispatcher.setDefaultLocale(null); // Force a null Struts default locale, otherwise we receive the default "de_DE" from the test configuration. + dispatcher.setDefaultLocale(null); // Force a null Struts default locale, otherwise we receive the default "de_DE" from the test configuration. // When - testDispatcher.prepare(request, response); - ActionContext context = ActionContext.of(createTestContextMap(testDispatcher, request, response)); + dispatcher.prepare(request, response); + ActionContext context = ActionContext.of(createTestContextMap(dispatcher, request, response)); // Then assertEquals(Locale.CANADA_FRENCH, context.getLocale()); // Expect the request set value from Mock. - mock.verify(); } + @Test public void testGetLocale_With_NullDefaultLocale_And_RuntimeException() { // Given - Mock mock = new Mock(HttpServletRequest.class); + HttpServletRequest request = mock(HttpServletRequest.class); MockHttpSession mockHttpSession = new MockHttpSession(); - mock.expectAndReturn("getCharacterEncoding", "utf-8"); // From Dispatcher prepare(). - mock.expectAndReturn("getHeader", "X-Requested-With", ""); // From Dispatcher prepare(). - mock.expectAndReturn("getLocale", Locale.CANADA_FRENCH); // From Dispatcher prepare(). - mock.expectAndReturn("getParameterMap", new HashMap<String, Object>()); // From Dispatcher prepare(). - mock.expectAndReturn("getSession", false, mockHttpSession); // From Dispatcher prepare(). - mock.expectAndReturn("getSession", true, mockHttpSession); // From createTestContextMap(). - mock.expectAndThrow("getLocale", new IllegalStateException("Test some theoretical state preventing HTTP Request Locale access")); // From createTestContextMap(). - HttpServletRequest request = (HttpServletRequest) mock.proxy(); + when(request.getCharacterEncoding()).thenReturn(UTF_8.name()); + when(request.getHeader("X-Requested-With")).thenReturn(""); + when(request.getParameterMap()).thenReturn(emptyMap()); + when(request.getSession(anyBoolean())).thenReturn(mockHttpSession); + when(request.getLocale()).thenReturn(Locale.CANADA_FRENCH); HttpServletResponse response = new MockHttpServletResponse(); - Dispatcher testDispatcher = initDispatcher(new HashMap<String, String>() {{ - put(StrutsConstants.STRUTS_I18N_ENCODING, "utf-8"); - // Attempting to set StrutsConstants.STRUTS_LOCALE to null via parameters causes an NPE. - }}); + // Attempting to set StrutsConstants.STRUTS_LOCALE to null via parameters causes an NPE. + initDispatcher(singletonMap(StrutsConstants.STRUTS_I18N_ENCODING, UTF_8.name())); - testDispatcher.setDefaultLocale(null); // Force a null Struts default locale, otherwise we receive the default "de_DE" from the test configuration. + dispatcher.setDefaultLocale(null); // Force a null Struts default locale, otherwise we receive the default "de_DE" from the test configuration. // When - testDispatcher.prepare(request, response); - ActionContext context = ActionContext.of(createTestContextMap(testDispatcher, request, response)); + dispatcher.prepare(request, response); + when(request.getLocale()).thenThrow(new IllegalStateException("Test theoretical state preventing HTTP Request Locale access")); + ActionContext context = ActionContext.of(createTestContextMap(dispatcher, request, response)); // Then assertEquals(Locale.getDefault(), context.getLocale()); // Expect the system default value when Mock request access fails. - mock.verify(); + } + + @Test + public void dispatcherReinjectedAfterReload() { Review Comment: New test for reinjection functionality Issue Time Tracking ------------------- Worklog Id: (was: 897547) Time Spent: 1h (was: 50m) > Stale configuration persists after configuration reload > ------------------------------------------------------- > > Key: WW-5382 > URL: https://issues.apache.org/jira/browse/WW-5382 > Project: Struts 2 > Issue Type: Bug > Components: Core > Affects Versions: 6.3.0 > Reporter: Kusal Kithul-Godage > Priority: Major > Fix For: 6.4.0 > > Time Spent: 1h > Remaining Estimate: 0h > -- This message was sent by Atlassian Jira (v8.20.10#820010)