This is an automated email from the ASF dual-hosted git repository. joerghoh pushed a commit to branch SLING-13413 in repository https://gitbox.apache.org/repos/asf/sling-org-apache-sling-servlets-resolver.git
commit 13e724bd68d25858653d97400bb757a918aade06 Author: Joerg Hoh <[email protected]> AuthorDate: Mon Nov 9 19:41:20 2026 +0100 SLING-13413 simple error handling for non-(JSON|HTML) requests --- .../defaults/DefaultErrorHandlerServlet.java | 28 +++-- .../DefaultErrorHandlerEmptyResponseTest.java | 125 +++++++++++++++++++++ .../DefaultErrorHandlerJakartaServletTest.java | 80 +++++++++++-- .../defaults/DefaultErrorHandlerServletTest.java | 86 ++++++++++++-- .../sling/servlets/resolver/it/TestServlet.java | 4 +- 5 files changed, 293 insertions(+), 30 deletions(-) diff --git a/src/main/java/org/apache/sling/servlets/resolver/internal/defaults/DefaultErrorHandlerServlet.java b/src/main/java/org/apache/sling/servlets/resolver/internal/defaults/DefaultErrorHandlerServlet.java index 382da57..ed638ab 100644 --- a/src/main/java/org/apache/sling/servlets/resolver/internal/defaults/DefaultErrorHandlerServlet.java +++ b/src/main/java/org/apache/sling/servlets/resolver/internal/defaults/DefaultErrorHandlerServlet.java @@ -45,7 +45,10 @@ import org.slf4j.LoggerFactory; * The <code>DefaultErrorHandlerServlet</code> * * This is the default error handler servlet registered at the end of the - * global search path + * global search path. + * + * See also the DefaultErrorHandler in Sling Engine. + * */ @SuppressWarnings("serial") @Component( @@ -77,14 +80,25 @@ public class DefaultErrorHandlerServlet extends GenericServlet { statusMessage = statusToString(statusCode); } - // properly consider the 'Accept' header conditions to decide whether to send json or html back - if (req instanceof HttpServletRequest - && JSON_CONTENT_TYPE.equals(new JakartaMediaRangeList((HttpServletRequest) req) - .prefer(HTML_CONTENT_TYPE, JSON_CONTENT_TYPE))) { + // parse the accept header to retrieve the preferred content-type and decide on the error handler + String contentType = HTML_CONTENT_TYPE; + if (req instanceof HttpServletRequest) { + contentType = + new JakartaMediaRangeList((HttpServletRequest) req).prefer(HTML_CONTENT_TYPE, JSON_CONTENT_TYPE); + } + + if (JSON_CONTENT_TYPE.equals(contentType)) { renderJson(req, res, statusMessage, requestUri, servletName, statusCode); - } else { - // default to HTML rendering + } else if (HTML_CONTENT_TYPE.equals(contentType)) { renderHtml(req, res, statusMessage, requestUri, servletName, statusCode); + } else if (!res.isCommitted()) { + // for all non-committed requests which are not JSON or HTML just clear the buffer and set the + // statuscode; do not output anything. + res.resetBuffer(); + ((HttpServletResponse) res).setStatus(statusCode); + res.setContentLength(0); + } else { + log.warn("Response already committed, unable to change status to {} or clear response body", statusCode); } } diff --git a/src/test/java/org/apache/sling/servlets/resolver/internal/defaults/DefaultErrorHandlerEmptyResponseTest.java b/src/test/java/org/apache/sling/servlets/resolver/internal/defaults/DefaultErrorHandlerEmptyResponseTest.java new file mode 100644 index 0000000..b198d59 --- /dev/null +++ b/src/test/java/org/apache/sling/servlets/resolver/internal/defaults/DefaultErrorHandlerEmptyResponseTest.java @@ -0,0 +1,125 @@ +/* + * 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.sling.servlets.resolver.internal.defaults; + +import java.io.IOException; +import java.util.List; + +import jakarta.servlet.RequestDispatcher; +import jakarta.servlet.ServletConfig; +import jakarta.servlet.ServletContext; +import jakarta.servlet.ServletException; +import jakarta.servlet.http.HttpServletResponse; +import org.apache.sling.api.SlingConstants; +import org.apache.sling.api.SlingHttpServletRequest; +import org.apache.sling.api.SlingJakartaHttpServletRequest; +import org.apache.sling.api.request.builder.Builders; +import org.apache.sling.api.request.builder.SlingHttpServletResponseResult; +import org.apache.sling.api.request.builder.SlingJakartaHttpServletResponseResult; +import org.apache.sling.api.resource.Resource; +import org.apache.sling.api.wrappers.JavaxToJakartaRequestWrapper; +import org.apache.sling.api.wrappers.JavaxToJakartaResponseWrapper; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; +import org.junit.runners.Parameterized.Parameters; +import org.mockito.Mockito; + +import static org.junit.Assert.assertEquals; + +/** + * Verifies empty error responses when neither HTML nor JSON is acceptable. + */ +@SuppressWarnings("deprecation") +@RunWith(Parameterized.class) +public class DefaultErrorHandlerEmptyResponseTest { + + @Parameters(name = "Accept={0}") + public static List<Object[]> data() { + return List.of( + new Object[] {"text/plain"}, new Object[] {"application/xml"}, new Object[] {"*/*;q=0"}, new Object[] { + "text/*;q=0,application/*;q=0,*/*" + }); + } + + private final String accept; + private DefaultErrorHandlerServlet errorServlet; + + public DefaultErrorHandlerEmptyResponseTest(String accept) { + this.accept = accept; + } + + @Before + public void setUp() throws ServletException { + ServletContext context = Mockito.mock(ServletContext.class); + Mockito.when(context.getServerInfo()).thenReturn("Test Server Info"); + ServletConfig config = Mockito.mock(ServletConfig.class); + Mockito.when(config.getServletContext()).thenReturn(context); + errorServlet = new DefaultErrorHandlerServlet(); + errorServlet.init(config); + } + + /** + * Returns an empty Jakarta response when Accept negotiation selects neither HTML nor JSON, + * even if the response content type is already HTML. Sets the error status and zero content + * length without obtaining a writer. + */ + @Test + public void testEmptyErrorResponse() throws IOException { + SlingJakartaHttpServletRequest request = Mockito.spy( + Builders.newRequestBuilder(Mockito.mock(Resource.class)).buildJakartaRequest()); + Mockito.when(request.getHeader("Accept")).thenReturn(accept); + request.setAttribute(RequestDispatcher.ERROR_STATUS_CODE, HttpServletResponse.SC_INTERNAL_SERVER_ERROR); + SlingJakartaHttpServletResponseResult response = + Mockito.spy(Builders.newResponseBuilder().buildJakartaResponseResult()); + response.setContentType("text/html"); + + errorServlet.service(request, response); + + assertEquals(HttpServletResponse.SC_INTERNAL_SERVER_ERROR, response.getStatus()); + assertEquals("", response.getOutputAsString()); + assertEquals(0, response.getContentLength()); + Mockito.verify(response, Mockito.never()).getWriter(); + } + + /** + * Verifies the same empty-body, error-status, and content-length behavior through the + * javax-to-Jakarta wrappers, without obtaining a response writer. + */ + @Test + public void testEmptyErrorResponseWithJavaxWrapper() throws IOException { + SlingHttpServletRequest request = Mockito.spy( + Builders.newRequestBuilder(Mockito.mock(Resource.class)).build()); + Mockito.when(request.getHeader("Accept")).thenReturn(accept); + request.setAttribute(SlingConstants.ERROR_STATUS, HttpServletResponse.SC_INTERNAL_SERVER_ERROR); + SlingHttpServletResponseResult response = + Mockito.spy(Builders.newResponseBuilder().build()); + response.setContentType("text/html"); + + errorServlet.service( + JavaxToJakartaRequestWrapper.toJakartaRequest(request), + JavaxToJakartaResponseWrapper.toJakartaResponse(response)); + + assertEquals(HttpServletResponse.SC_INTERNAL_SERVER_ERROR, response.getStatus()); + assertEquals("", response.getOutputAsString()); + assertEquals(0, response.getContentLength()); + Mockito.verify(response, Mockito.never()).getWriter(); + } +} diff --git a/src/test/java/org/apache/sling/servlets/resolver/internal/defaults/DefaultErrorHandlerJakartaServletTest.java b/src/test/java/org/apache/sling/servlets/resolver/internal/defaults/DefaultErrorHandlerJakartaServletTest.java index f923d4d..bf6d45f 100644 --- a/src/test/java/org/apache/sling/servlets/resolver/internal/defaults/DefaultErrorHandlerJakartaServletTest.java +++ b/src/test/java/org/apache/sling/servlets/resolver/internal/defaults/DefaultErrorHandlerJakartaServletTest.java @@ -82,10 +82,11 @@ public class DefaultErrorHandlerJakartaServletTest { final SlingJakartaHttpServletRequest request = Builders.newRequestBuilder(resource).buildJakartaRequest(); - // mock a request that accepts a json response - MockErrorSlingHttpServletRequest req = - new MockErrorSlingHttpServletRequest(request, "application/json,*/*;q=0.9"); - assertJsonErrorResponse(req); + for (String accept : new String[] { + "application/json,*/*;q=0.9", "application/json,text/html", "application/json;q=0.5,*/*;q=0" + }) { + assertJsonErrorResponse(new MockErrorSlingHttpServletRequest(request, accept)); + } } /** @@ -115,13 +116,21 @@ public class DefaultErrorHandlerJakartaServletTest { @Test public void testHtmlErrorResponse() throws IOException, ServletException { + for (String accept : new String[] { + "text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8", + "text/html;q=0.8,application/json;q=0.2", + "text/html;q=0.2,text/*;q=0.1,application/json;q=0.15" + }) { + assertHtmlErrorResponse(accept); + } + } + + private void assertHtmlErrorResponse(String accept) throws IOException, ServletException { final Resource resource = Mockito.mock(Resource.class); final SlingJakartaHttpServletRequest request = Builders.newRequestBuilder(resource).buildJakartaRequest(); - // mock a request that accepts an html response - SlingJakartaHttpServletRequest req = new MockErrorSlingHttpServletRequest( - request, "text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8"); + SlingJakartaHttpServletRequest req = new MockErrorSlingHttpServletRequest(request, accept); MockErrorSlingHttpServletResponse res = new MockErrorSlingHttpServletResponse( Builders.newResponseBuilder().buildJakartaResponseResult(), false); @@ -141,6 +150,44 @@ public class DefaultErrorHandlerJakartaServletTest { assertTrue(responseOutput.contains("Test Exception")); } + @Test + public void testEmptyErrorResponseClearsBufferedBody() throws IOException, ServletException { + final Resource resource = Mockito.mock(Resource.class); + final SlingJakartaHttpServletRequest request = + Builders.newRequestBuilder(resource).buildJakartaRequest(); + MockErrorSlingHttpServletResponse res = new MockErrorSlingHttpServletResponse( + Builders.newResponseBuilder().buildJakartaResponseResult(), false); + res.setContentLength(123); + res.getWriter().write("incomplete response"); + + DefaultErrorHandlerServlet errorServlet = new DefaultErrorHandlerServlet(); + errorServlet.init(new MockServletConfig()); + errorServlet.service(new MockErrorSlingHttpServletRequest(request, "text/plain"), res); + + assertEquals(HttpServletResponse.SC_INTERNAL_SERVER_ERROR, res.getStatus()); + assertEquals("", res.getOutput().toString()); + assertEquals("0", res.getHeader("Content-Length")); + } + + @Test + public void testCommittedUnacceptableResponseIsNotChanged() throws IOException, ServletException { + final Resource resource = Mockito.mock(Resource.class); + final SlingJakartaHttpServletRequest request = + Builders.newRequestBuilder(resource).buildJakartaRequest(); + MockErrorSlingHttpServletResponse res = new MockErrorSlingHttpServletResponse( + Builders.newResponseBuilder().buildJakartaResponseResult(), true); + res.setContentType("text/plain"); + res.getWriter().write("committed response"); + + DefaultErrorHandlerServlet errorServlet = new DefaultErrorHandlerServlet(); + errorServlet.init(new MockServletConfig()); + errorServlet.service(new MockErrorSlingHttpServletRequest(request, "text/plain"), res); + + assertEquals(HttpServletResponse.SC_OK, res.getStatus()); + assertEquals("text/plain", res.getContentType()); + assertEquals("committed response", res.getOutput().toString()); + } + /** * Mock impl to simulate enough of a servlet context to satisfy what is used * by DefaultErrorHandlerServlet @@ -176,7 +223,7 @@ public class DefaultErrorHandlerJakartaServletTest { private static final class MockErrorSlingHttpServletResponse extends SlingJakartaHttpServletResponseWrapper { private PrintWriter writer; - private StringWriter strWriter; + private final StringWriter strWriter = new StringWriter(); private boolean committed; public MockErrorSlingHttpServletResponse( @@ -192,7 +239,20 @@ public class DefaultErrorHandlerJakartaServletTest { @Override public void reset() { - // no-op + super.reset(); + strWriter.getBuffer().setLength(0); + } + + @Override + public void resetBuffer() { + super.resetBuffer(); + strWriter.getBuffer().setLength(0); + } + + @Override + public void setContentLength(int length) { + super.setContentLength(length); + setIntHeader("Content-Length", length); } @Override @@ -201,8 +261,6 @@ public class DefaultErrorHandlerJakartaServletTest { // the flush() calls, so we can't use it to verify // content in the response. if (this.writer == null) { - strWriter = new StringWriter(); - this.strWriter.getBuffer(); this.writer = new PrintWriter(strWriter); } return this.writer; diff --git a/src/test/java/org/apache/sling/servlets/resolver/internal/defaults/DefaultErrorHandlerServletTest.java b/src/test/java/org/apache/sling/servlets/resolver/internal/defaults/DefaultErrorHandlerServletTest.java index 0bd3a7c..94545f1 100644 --- a/src/test/java/org/apache/sling/servlets/resolver/internal/defaults/DefaultErrorHandlerServletTest.java +++ b/src/test/java/org/apache/sling/servlets/resolver/internal/defaults/DefaultErrorHandlerServletTest.java @@ -88,10 +88,11 @@ public class DefaultErrorHandlerServletTest { final SlingHttpServletRequest request = Builders.newRequestBuilder(resource).build(); - // mock a request that accepts a json response - MockErrorSlingHttpServletRequest req = - new MockErrorSlingHttpServletRequest(request, "application/json,*/*;q=0.9"); - assertJsonErrorResponse(req); + for (String accept : new String[] { + "application/json,*/*;q=0.9", "application/json,text/html", "application/json;q=0.5,*/*;q=0" + }) { + assertJsonErrorResponse(new MockErrorSlingHttpServletRequest(request, accept)); + } } /** @@ -120,13 +121,21 @@ public class DefaultErrorHandlerServletTest { @Test public void testHtmlErrorResponse() throws IOException, ServletException { + for (String accept : new String[] { + "text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8", + "text/html;q=0.8,application/json;q=0.2", + "text/html;q=0.2,text/*;q=0.1,application/json;q=0.15" + }) { + assertHtmlErrorResponse(accept); + } + } + + private void assertHtmlErrorResponse(String accept) throws IOException, ServletException { final Resource resource = Mockito.mock(Resource.class); final SlingHttpServletRequest request = Builders.newRequestBuilder(resource).build(); - // mock a request that accepts an html response - SlingHttpServletRequest req = new MockErrorSlingHttpServletRequest( - request, "text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8"); + SlingHttpServletRequest req = new MockErrorSlingHttpServletRequest(request, accept); MockErrorSlingHttpServletResponse res = new MockErrorSlingHttpServletResponse( Builders.newResponseBuilder().build(), false); @@ -148,6 +157,50 @@ public class DefaultErrorHandlerServletTest { assertTrue(responseOutput.contains("Test Exception")); } + @Test + public void testEmptyErrorResponseClearsBufferedBody() throws IOException, ServletException { + final Resource resource = Mockito.mock(Resource.class); + final SlingHttpServletRequest request = + Builders.newRequestBuilder(resource).build(); + MockErrorSlingHttpServletResponse res = new MockErrorSlingHttpServletResponse( + Builders.newResponseBuilder().build(), false); + res.setContentLength(123); + res.getWriter().write("incomplete response"); + + DefaultErrorHandlerServlet errorServlet = new DefaultErrorHandlerServlet(); + errorServlet.init(new MockServletConfig()); + errorServlet.service( + JavaxToJakartaRequestWrapper.toJakartaRequest( + new MockErrorSlingHttpServletRequest(request, "text/plain")), + JavaxToJakartaResponseWrapper.toJakartaResponse(res)); + + assertEquals(HttpServletResponse.SC_INTERNAL_SERVER_ERROR, res.getStatus()); + assertEquals("", res.getOutput().toString()); + assertEquals("0", res.getHeader("Content-Length")); + } + + @Test + public void testCommittedUnacceptableResponseIsNotChanged() throws IOException, ServletException { + final Resource resource = Mockito.mock(Resource.class); + final SlingHttpServletRequest request = + Builders.newRequestBuilder(resource).build(); + MockErrorSlingHttpServletResponse res = new MockErrorSlingHttpServletResponse( + Builders.newResponseBuilder().build(), true); + res.setContentType("text/plain"); + res.getWriter().write("committed response"); + + DefaultErrorHandlerServlet errorServlet = new DefaultErrorHandlerServlet(); + errorServlet.init(new MockServletConfig()); + errorServlet.service( + JavaxToJakartaRequestWrapper.toJakartaRequest( + new MockErrorSlingHttpServletRequest(request, "text/plain")), + JavaxToJakartaResponseWrapper.toJakartaResponse(res)); + + assertEquals(HttpServletResponse.SC_OK, res.getStatus()); + assertEquals("text/plain", res.getContentType()); + assertEquals("committed response", res.getOutput().toString()); + } + /** * Mock impl to simulate enough of a servlet context to satisfy what is used * by DefaultErrorHandlerServlet @@ -183,7 +236,7 @@ public class DefaultErrorHandlerServletTest { private static final class MockErrorSlingHttpServletResponse extends SlingHttpServletResponseWrapper { private PrintWriter writer; - private StringWriter strWriter; + private final StringWriter strWriter = new StringWriter(); private boolean committed; public MockErrorSlingHttpServletResponse(final SlingHttpServletResponse response, final boolean committed) { @@ -198,7 +251,20 @@ public class DefaultErrorHandlerServletTest { @Override public void reset() { - // no-op + super.reset(); + strWriter.getBuffer().setLength(0); + } + + @Override + public void resetBuffer() { + super.resetBuffer(); + strWriter.getBuffer().setLength(0); + } + + @Override + public void setContentLength(int length) { + super.setContentLength(length); + setIntHeader("Content-Length", length); } @Override @@ -207,8 +273,6 @@ public class DefaultErrorHandlerServletTest { // the flush() calls, so we can't use it to verify // content in the response. if (this.writer == null) { - strWriter = new StringWriter(); - this.strWriter.getBuffer(); this.writer = new PrintWriter(strWriter); } return this.writer; diff --git a/src/test/java/org/apache/sling/servlets/resolver/it/TestServlet.java b/src/test/java/org/apache/sling/servlets/resolver/it/TestServlet.java index 4c492fb..2cff8a8 100644 --- a/src/test/java/org/apache/sling/servlets/resolver/it/TestServlet.java +++ b/src/test/java/org/apache/sling/servlets/resolver/it/TestServlet.java @@ -48,7 +48,9 @@ public class TestServlet extends HttpServlet { @Override public void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException { - resp.sendError(IM_A_TEAPOT, SERVED_BY_PREFIX + name); + resp.setStatus(IM_A_TEAPOT); + resp.setContentType("text/plain"); + resp.getWriter().write(SERVED_BY_PREFIX + name); } @Override
