From 73809eae54e8a3fbcc6ee03d49379848cf7f99ed Mon Sep 17 00:00:00 2001 From: JCgH4164838Gh792C124B5 <43964333+JCgH4164838Gh792C124B5@users.noreply.github.com> Date: Sat, 21 Sep 2019 23:29:24 -0400 Subject: [PATCH] Changed the styling of log outputs (as suggested by A. Mashchenko). Changed the log outputs to output full stacktraces (as suggested by A. Mashchenko and L. Lenart). Updated existing DefaultStaticContentLoaderTest and created a new DefaultDispatcherErrorHandlerTest to supply tests that exercise the new code paths and demonstrate expected logging. --- .../DefaultDispatcherErrorHandler.java | 8 +- .../DefaultStaticContentLoader.java | 4 +- .../DefaultDispatcherErrorHandlerTest.java | 138 ++++++++++++++++++ .../DefaultStaticContentLoaderTest.java | 62 ++++++++ 4 files changed, 206 insertions(+), 6 deletions(-) create mode 100644 core/src/test/java/org/apache/struts2/dispatcher/DefaultDispatcherErrorHandlerTest.java diff --git a/core/src/main/java/org/apache/struts2/dispatcher/DefaultDispatcherErrorHandler.java b/core/src/main/java/org/apache/struts2/dispatcher/DefaultDispatcherErrorHandler.java index 5f1fd2272..551ff1e6a 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/DefaultDispatcherErrorHandler.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/DefaultDispatcherErrorHandler.java @@ -97,10 +97,10 @@ public class DefaultDispatcherErrorHandler implements DispatcherErrorHandler { response.sendError(code, e.getMessage()); } catch (IOException e1) { // we're already sending an error, not much else we can do if more stuff breaks - LOG.warn("Unable to send error response, code: {}! (IOException): {}", code, e1.toString()); + LOG.warn("Unable to send error response, code: {};", code, e1); } catch (IllegalStateException ise) { // Log illegalstate instead of passing unrecoverable exception to calling thread - LOG.warn("Unable to send error response, code: {}! isCommited: {}. (IllegalStateException): {}", code, response.isCommitted(), ise.toString()); + LOG.warn("Unable to send error response, code: {}; isCommited: {};", code, response.isCommitted(), ise); } } @@ -126,10 +126,10 @@ public class DefaultDispatcherErrorHandler implements DispatcherErrorHandler { response.sendError(code, "Unable to show problem report:\n" + exp + "\n\n" + LocationUtils.getLocation(exp)); } catch (IOException ex) { // we're already sending an error, not much else we can do if more stuff breaks - LOG.warn("Unable to send error response, code: {}! (IOException): {}", code, ex); // Stacktrace with DevMode + LOG.warn("Unable to send error response, code: {};", code, ex); } catch (IllegalStateException ise) { // Log illegalstate instead of passing unrecoverable exception to calling thread - LOG.warn("Unable to send error response, code: {}! isCommited: {}. (IllegalStateException): {}", code, response.isCommitted(), ise); // Stacktrace with DevMode + LOG.warn("Unable to send error response, code: {}; isCommited: {};", code, response.isCommitted(), ise); } } } diff --git a/core/src/main/java/org/apache/struts2/dispatcher/DefaultStaticContentLoader.java b/core/src/main/java/org/apache/struts2/dispatcher/DefaultStaticContentLoader.java index 79c417918..42f17160e 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/DefaultStaticContentLoader.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/DefaultStaticContentLoader.java @@ -222,10 +222,10 @@ public class DefaultStaticContentLoader implements StaticContentLoader { response.sendError(HttpServletResponse.SC_NOT_FOUND); } catch (IOException e1) { // we're already sending an error, not much else we can do if more stuff breaks - LOG.warn("Unable to send error response, code: {}! (IOException): {}", HttpServletResponse.SC_NOT_FOUND, e1.toString()); + LOG.warn("Unable to send error response, code: {};", HttpServletResponse.SC_NOT_FOUND, e1); } catch (IllegalStateException ise) { // Log illegalstate instead of passing unrecoverable exception to calling thread - LOG.warn("Unable to send error response, code: {}! isCommited: {}. (IllegalStateException): {}", HttpServletResponse.SC_NOT_FOUND, response.isCommitted(), ise.toString()); + LOG.warn("Unable to send error response, code: {}; isCommited: {};", HttpServletResponse.SC_NOT_FOUND, response.isCommitted(), ise); } } diff --git a/core/src/test/java/org/apache/struts2/dispatcher/DefaultDispatcherErrorHandlerTest.java b/core/src/test/java/org/apache/struts2/dispatcher/DefaultDispatcherErrorHandlerTest.java new file mode 100644 index 000000000..e89dd3913 --- /dev/null +++ b/core/src/test/java/org/apache/struts2/dispatcher/DefaultDispatcherErrorHandlerTest.java @@ -0,0 +1,138 @@ +/* + * 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 java.io.IOException; +import java.util.Collections; +import org.apache.struts2.StrutsInternalTestCase; + +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import org.apache.struts2.views.freemarker.FreemarkerManager; +import static org.easymock.EasyMock.anyInt; +import static org.easymock.EasyMock.anyString; +import static org.easymock.EasyMock.createMock; +import static org.easymock.EasyMock.expect; +import static org.easymock.EasyMock.expectLastCall; +import static org.easymock.EasyMock.replay; + +public class DefaultDispatcherErrorHandlerTest extends StrutsInternalTestCase { + private HttpServletRequest requestMock; + private HttpServletResponse responseMock; + + /** + * Test to exercise the code path and prove handleError() will output + * the desired log warning when an IOException is thrown with devMode false. + */ + public void testHandleErrorIOException() { + DefaultDispatcherErrorHandler defaultDispatcherErrorHandler = new DefaultDispatcherErrorHandler(); + defaultDispatcherErrorHandler.setDevMode("false"); + defaultDispatcherErrorHandler.setFreemarkerManager(dispatcher.getContainer().getInstance(FreemarkerManager.class)); + defaultDispatcherErrorHandler.init(dispatcher.servletContext); + Exception fakeException = new Exception("Fake Exception, devMode false"); + try { + requestMock.setAttribute("javax.servlet.error.exception", fakeException); + expectLastCall(); + requestMock.setAttribute("javax.servlet.jsp.jspException", fakeException); + expectLastCall(); + responseMock.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR, fakeException.getMessage()); + expectLastCall().andStubThrow(new IOException("Fake IO Exception (SC_INTERNAL_SERVER_ERROR, devMode false)")); + replay(responseMock); + } catch (IOException ioe) { + fail("Mock sendError call setup failed. Ex: " + ioe); + } + defaultDispatcherErrorHandler.handleError(requestMock, responseMock, HttpServletResponse.SC_INTERNAL_SERVER_ERROR, fakeException); + } + + /** + * Test to exercise the code path and prove handleError() will output + * the desired log warning when an IOException is thrown with devMode true. + */ + public void testHandleErrorIOExceptionDevMode() { + DefaultDispatcherErrorHandler defaultDispatcherErrorHandler = new DefaultDispatcherErrorHandler(); + defaultDispatcherErrorHandler.setDevMode("true"); + defaultDispatcherErrorHandler.setFreemarkerManager(dispatcher.getContainer().getInstance(FreemarkerManager.class)); + defaultDispatcherErrorHandler.init(dispatcher.servletContext); + Exception fakeException = new Exception("Fake Exception, devMode true"); + try { + responseMock.setContentType("text/html"); + expectLastCall().andStubThrow(new IllegalStateException("Fake IllegalState Exception (report write)")); // Fake error during report write + responseMock.sendError(anyInt(), anyString()); + expectLastCall().andStubThrow(new IOException("Fake IO Exception (SC_INTERNAL_SERVER_ERROR, devMode true)")); + replay(responseMock); + } catch (IOException ioe) { + fail("Mock sendError call setup failed. Ex: " + ioe); + } + defaultDispatcherErrorHandler.handleError(requestMock, responseMock, HttpServletResponse.SC_INTERNAL_SERVER_ERROR, fakeException); + } + + /** + * Test to exercise the code path and prove handleError() will output + * the desired log warning when an IllegalStateException is thrown with devMode false. + */ + public void testHandleErrorIllegalStateException() { + DefaultDispatcherErrorHandler defaultDispatcherErrorHandler = new DefaultDispatcherErrorHandler(); + defaultDispatcherErrorHandler.setDevMode("false"); + defaultDispatcherErrorHandler.setFreemarkerManager(dispatcher.getContainer().getInstance(FreemarkerManager.class)); + defaultDispatcherErrorHandler.init(dispatcher.servletContext); + Exception fakeException = new Exception("Fake Exception, devMode false"); + try { + requestMock.setAttribute("javax.servlet.error.exception", fakeException); + expectLastCall(); + requestMock.setAttribute("javax.servlet.jsp.jspException", fakeException); + expectLastCall(); + expect(responseMock.isCommitted()).andStubReturn(Boolean.TRUE); + responseMock.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR, fakeException.getMessage()); + expectLastCall().andStubThrow(new IllegalStateException("Fake IllegalState Exception (SC_INTERNAL_SERVER_ERROR, devMode false)")); + replay(responseMock); + } catch (IOException ioe) { + fail("Mock sendError call setup failed. Ex: " + ioe); + } + defaultDispatcherErrorHandler.handleError(requestMock, responseMock, HttpServletResponse.SC_INTERNAL_SERVER_ERROR, fakeException); + } + + /** + * Test to exercise the code path and prove handleError() will output + * the desired log warning when an IllegalStateException is thrown with devMode true. + */ + public void testHandleErrorIllegalStateExceptionDevMode() { + DefaultDispatcherErrorHandler defaultDispatcherErrorHandler = new DefaultDispatcherErrorHandler(); + defaultDispatcherErrorHandler.setDevMode("true"); + defaultDispatcherErrorHandler.setFreemarkerManager(dispatcher.getContainer().getInstance(FreemarkerManager.class)); + defaultDispatcherErrorHandler.init(dispatcher.servletContext); + Exception fakeException = new Exception("Fake Exception, devMode true"); + try { + expect(responseMock.isCommitted()).andStubReturn(Boolean.TRUE); + responseMock.setContentType("text/html"); + expectLastCall().andStubThrow(new IllegalStateException("Fake IllegalState Exception (report write)")); // Fake error during report write + responseMock.sendError(anyInt(), anyString()); + expectLastCall().andStubThrow(new IllegalStateException("Fake IllegalState Exception (SC_INTERNAL_SERVER_ERROR, devMode true)")); + replay(responseMock); + } catch (IOException ioe) { + fail("Mock sendError call setup failed. Ex: " + ioe); + } + defaultDispatcherErrorHandler.handleError(requestMock, responseMock, HttpServletResponse.SC_INTERNAL_SERVER_ERROR, fakeException); + } + + protected void setUp() { + requestMock = (HttpServletRequest) createMock(HttpServletRequest.class); + responseMock = (HttpServletResponse) createMock(HttpServletResponse.class); + dispatcher = initDispatcher(Collections.emptyMap()); + } +} diff --git a/core/src/test/java/org/apache/struts2/dispatcher/DefaultStaticContentLoaderTest.java b/core/src/test/java/org/apache/struts2/dispatcher/DefaultStaticContentLoaderTest.java index e8d077df0..8f8eda1eb 100644 --- a/core/src/test/java/org/apache/struts2/dispatcher/DefaultStaticContentLoaderTest.java +++ b/core/src/test/java/org/apache/struts2/dispatcher/DefaultStaticContentLoaderTest.java @@ -18,11 +18,22 @@ */ package org.apache.struts2.dispatcher; +import java.io.IOException; import org.apache.struts2.StrutsInternalTestCase; import java.util.List; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import static org.easymock.EasyMock.createMock; +import static org.easymock.EasyMock.expect; +import static org.easymock.EasyMock.expectLastCall; +import static org.easymock.EasyMock.replay; public class DefaultStaticContentLoaderTest extends StrutsInternalTestCase { + private HttpServletRequest requestMock; + private HttpServletResponse responseMock; + private HostConfig hostConfigMock; + private DefaultStaticContentLoader defaultStaticContentLoader; public void testParsePackages() throws Exception { @@ -50,4 +61,55 @@ public class DefaultStaticContentLoaderTest extends StrutsInternalTestCase { assertEquals(result4.get(3), "foo/bar/package4/"); } + /** + * Test to exercise the code path and prove findStaticResource() will output + * the desired log warning when an IOException is thrown. + */ + public void testFindStaticResourceIOException() { + expect(requestMock.getDateHeader("If-Modified-Since")).andStubReturn(0L); + try { + responseMock.sendError(HttpServletResponse.SC_NOT_FOUND); + expectLastCall().andStubThrow(new IOException("Fake IO Exception (SC_NOT_FOUND)")); + replay(responseMock); + } catch (IOException ioe) { + fail("Mock sendError call setup failed. Ex: " + ioe); + } + try { + defaultStaticContentLoader.findStaticResource("/static/fake.html", requestMock, responseMock); + } catch (IOException ioe) { + fail("DefaultStaticContentLoader.findStaticResource() call failed. Ex: " + ioe); + } + } + + /** + * Test to exercise the code path and prove findStaticResource() will output + * the desired log warning when an IllegalStateException is thrown. + */ + public void testFindStaticResourceIllegalStateException() { + expect(requestMock.getDateHeader("If-Modified-Since")).andStubReturn(0L); + try { + expect(responseMock.isCommitted()).andStubReturn(Boolean.TRUE); + responseMock.sendError(HttpServletResponse.SC_NOT_FOUND); + expectLastCall().andStubThrow(new IllegalStateException("Fake IllegalState Exception (SC_NOT_FOUND)")); + replay(responseMock); + } catch (IOException ioe) { + fail("Mock sendError call setup failed. Ex: " + ioe); + } + try { + defaultStaticContentLoader.findStaticResource("/static/fake.html", requestMock, responseMock); + } catch (IOException ioe) { + fail("DefaultStaticContentLoader.findStaticResource() call failed. Ex: " + ioe); + } + } + + protected void setUp() { + requestMock = (HttpServletRequest) createMock(HttpServletRequest.class); + responseMock = (HttpServletResponse) createMock(HttpServletResponse.class); + hostConfigMock = (HostConfig) createMock(HostConfig.class); + expect(hostConfigMock.getInitParameter("packages")).andStubReturn(null); + expect(hostConfigMock.getInitParameter("loggerFactory")).andStubReturn(null); + defaultStaticContentLoader = new DefaultStaticContentLoader(); + defaultStaticContentLoader.setHostConfig(hostConfigMock); + defaultStaticContentLoader.setEncoding("UTF-8"); + } }