This is an automated email from the ASF dual-hosted git repository. cziegeler pushed a commit to branch feature/SLING-13355-non-html-error-handler in repository https://gitbox.apache.org/repos/asf/sling-org-apache-sling-engine.git
commit 827de5e64e5cee067f0a3993584a3190d3561fdf Author: Carsten Ziegeler <[email protected]> AuthorDate: Wed Nov 11 11:32:45 2026 +0100 SLING-13355 Support a dedicated error handler for non-HTML requests Error handlers registered with the service property sling.error.handler=nonhtml are used for requests whose Accept header neither contains text/html nor */*. The highest ranked one is used. A default implementation returning just the status with an empty body is provided. Co-authored-by: Copilot <[email protected]> --- .../sling/engine/impl/DefaultErrorHandler.java | 95 +++++- .../engine/impl/DefaultNonHtmlErrorHandler.java | 53 ++++ .../engine/impl/SlingRequestProcessorImpl.java | 26 ++ .../sling/engine/impl/DefaultErrorHandlerTest.java | 341 +++++++++++++++++++++ .../impl/DefaultNonHtmlErrorHandlerTest.java | 70 +++++ .../engine/impl/SlingRequestProcessorImplTest.java | 110 +++++++ 6 files changed, 693 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/apache/sling/engine/impl/DefaultErrorHandler.java b/src/main/java/org/apache/sling/engine/impl/DefaultErrorHandler.java index 3cf96c4..54208c5 100644 --- a/src/main/java/org/apache/sling/engine/impl/DefaultErrorHandler.java +++ b/src/main/java/org/apache/sling/engine/impl/DefaultErrorHandler.java @@ -20,6 +20,10 @@ package org.apache.sling.engine.impl; import java.io.IOException; import java.io.PrintWriter; +import java.util.Comparator; +import java.util.Map; +import java.util.Optional; +import java.util.TreeMap; import jakarta.servlet.RequestDispatcher; import jakarta.servlet.http.HttpServletRequest; @@ -29,11 +33,13 @@ import org.apache.sling.api.SlingHttpServletResponse; import org.apache.sling.api.SlingJakartaHttpServletRequest; import org.apache.sling.api.SlingJakartaHttpServletResponse; import org.apache.sling.api.request.ResponseUtil; +import org.apache.sling.api.request.header.JakartaMediaRangeList; import org.apache.sling.api.servlets.ErrorHandler; import org.apache.sling.api.servlets.JakartaErrorHandler; import org.apache.sling.api.wrappers.JakartaToJavaxRequestWrapper; import org.apache.sling.api.wrappers.JakartaToJavaxResponseWrapper; import org.osgi.framework.ServiceReference; +import org.osgi.util.converter.Converters; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -48,14 +54,28 @@ public class DefaultErrorHandler implements JakartaErrorHandler { /** default log */ private final Logger log = LoggerFactory.getLogger(getClass()); + // TODO - move this to API + private static final String HTML_CONTENT_TYPE = "text/html"; + + private static final String ALL_CONTENT_TYPES = "*/*"; + + // TODO - move this to API + static final String NON_HTML = "nonhtml"; + /** Use this if not null, and if that fails output a report about that failure */ private volatile JakartaErrorHandler delegate; + /** Use this if not null for non html responses */ + private volatile JakartaErrorHandler nonHtmlDelegate; + private volatile JakartaErrorHandler errorHandler; private volatile JakartaErrorHandler jakartaErrorHandler; private volatile ServiceReference<?> errorHandlerRef; private volatile ServiceReference<?> jakartaErrorHandlerRef; + private final Map<ServiceReference<JakartaErrorHandler>, JakartaErrorHandler> jakartaErrorHandlers = + new TreeMap<>(Comparator.reverseOrder()); + @SuppressWarnings("deprecation") public synchronized void setDelegate(final ServiceReference<?> ref, final ErrorHandler eh) { if (eh != null) { @@ -133,6 +153,29 @@ public class DefaultErrorHandler implements JakartaErrorHandler { sendError(originalStatus, originalMessage, null, request, response); } + protected JakartaErrorHandler getErrorHandler(final SlingJakartaHttpServletRequest request) { + final JakartaErrorHandler nonHtml = this.nonHtmlDelegate; + if (nonHtml != null && !acceptsHtml(request)) { + return nonHtml; + } + return this.delegate; + } + + /** + * A request accepts html if it explicitly accepts {@code text/html} or {@code *}{@code /*}. + * A missing accept header and partial wildcards like {@code text/*} are not considered html. + * Only the accept header is evaluated: request parameters are not read, as parameter + * parsing might itself be the cause of the error being handled. + */ + private static boolean acceptsHtml(final SlingJakartaHttpServletRequest request) { + final String accept = request.getHeader(JakartaMediaRangeList.HEADER_ACCEPT); + if (accept == null || accept.trim().isEmpty()) { + return false; + } + // TODO - do we want to treat */* as html? + return new JakartaMediaRangeList(accept).prefer(HTML_CONTENT_TYPE, ALL_CONTENT_TYPES) != null; + } + // ---------- ErrorHandler interface (default implementation) -------------- /** @@ -153,7 +196,7 @@ public class DefaultErrorHandler implements JakartaErrorHandler { final SlingJakartaHttpServletResponse response) throws IOException { // If we have a delegate let it handle the error - final JakartaErrorHandler local = this.delegate; + final JakartaErrorHandler local = this.getErrorHandler(request); if (local != null) { try { local.handleError(status, message, request, response); @@ -198,7 +241,7 @@ public class DefaultErrorHandler implements JakartaErrorHandler { throws IOException { final int status = HttpServletResponse.SC_INTERNAL_SERVER_ERROR; // If we have a delegate let it handle the error - final JakartaErrorHandler local = this.delegate; + final JakartaErrorHandler local = this.getErrorHandler(request); if (local != null) { try { local.handleError(throwable, request, response); @@ -263,4 +306,52 @@ public class DefaultErrorHandler implements JakartaErrorHandler { // close the response (SLING-2724) pw.close(); } + + private boolean matchesName(final ServiceReference<JakartaErrorHandler> ref, final String name) { + final String[] names = Converters.standardConverter() + .convert(ref.getProperty(SlingRequestProcessorImpl.ERROR_HANDLER_NAME)) + .to(String[].class); + + for (final String n : names) { + if (name.equals(n)) { + return true; + } + } + return false; + } + + /** + * Set the non html error handler + */ + private void setNonHtmlErrorHandler() { + final Optional<Map.Entry<ServiceReference<JakartaErrorHandler>, JakartaErrorHandler>> first = + jakartaErrorHandlers.entrySet().stream() + .filter(entry -> matchesName(entry.getKey(), DefaultErrorHandler.NON_HTML)) + .findFirst(); + if (first.isPresent()) { + this.nonHtmlDelegate = first.get().getValue(); + } else { + this.nonHtmlDelegate = null; + } + } + + /** + * Add a named delegate for handling non-HTML errors. + * @param ref the service reference for the delegate + * @param handler the error handler instance for the delegate + */ + public synchronized void addNamedDelegate( + final ServiceReference<JakartaErrorHandler> ref, final JakartaErrorHandler handler) { + this.jakartaErrorHandlers.put(ref, handler); + this.setNonHtmlErrorHandler(); + } + + /** + * Remove a named delegate for handling non-HTML errors. + * @param ref the service reference for the delegate + */ + public synchronized void removeNamedDelegate(final ServiceReference<JakartaErrorHandler> ref) { + this.jakartaErrorHandlers.remove(ref); + this.setNonHtmlErrorHandler(); + } } diff --git a/src/main/java/org/apache/sling/engine/impl/DefaultNonHtmlErrorHandler.java b/src/main/java/org/apache/sling/engine/impl/DefaultNonHtmlErrorHandler.java new file mode 100644 index 0000000..5968ba2 --- /dev/null +++ b/src/main/java/org/apache/sling/engine/impl/DefaultNonHtmlErrorHandler.java @@ -0,0 +1,53 @@ +/* + * 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.engine.impl; + +import java.io.IOException; + +import jakarta.servlet.http.HttpServletResponse; +import org.apache.sling.api.SlingJakartaHttpServletRequest; +import org.apache.sling.api.SlingJakartaHttpServletResponse; +import org.apache.sling.api.servlets.JakartaErrorHandler; +import org.osgi.service.component.annotations.Component; + +@Component( + service = JakartaErrorHandler.class, + property = {SlingRequestProcessorImpl.ERROR_HANDLER_NAME + "=" + DefaultErrorHandler.NON_HTML}) +public class DefaultNonHtmlErrorHandler implements JakartaErrorHandler { + + @Override + public void handleError( + int status, + String message, + SlingJakartaHttpServletRequest request, + SlingJakartaHttpServletResponse response) + throws IOException { + response.setStatus(status); + response.setContentLength(0); + // commit the response + response.flushBuffer(); + } + + @Override + public void handleError( + Throwable throwable, SlingJakartaHttpServletRequest request, SlingJakartaHttpServletResponse response) + throws IOException { + this.handleError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR, null, request, response); + } +} diff --git a/src/main/java/org/apache/sling/engine/impl/SlingRequestProcessorImpl.java b/src/main/java/org/apache/sling/engine/impl/SlingRequestProcessorImpl.java index 1cd2ac5..21309b2 100644 --- a/src/main/java/org/apache/sling/engine/impl/SlingRequestProcessorImpl.java +++ b/src/main/java/org/apache/sling/engine/impl/SlingRequestProcessorImpl.java @@ -85,6 +85,9 @@ import org.slf4j.LoggerFactory; configurationPid = Config.PID) public class SlingRequestProcessorImpl implements SlingRequestProcessor { + // TODO - move this to API + static final String ERROR_HANDLER_NAME = "sling.error.handler"; + /** default log */ private final Logger log = LoggerFactory.getLogger(SlingRequestProcessorImpl.class); @@ -156,6 +159,7 @@ public class SlingRequestProcessorImpl implements SlingRequestProcessor { @Reference( name = "JakartaErrorHandler", + target = "(!(" + ERROR_HANDLER_NAME + "=*))", cardinality = ReferenceCardinality.OPTIONAL, policy = ReferencePolicy.DYNAMIC, policyOption = ReferencePolicyOption.GREEDY) @@ -167,6 +171,28 @@ public class SlingRequestProcessorImpl implements SlingRequestProcessor { this.errorHandler.setDelegate(ref, (JakartaErrorHandler) null); } + /** + * Add named error handlers + * @param handler The JakartaErrorHandler instance to add as a named delegate. + * @param ref The ServiceReference associated with the JakartaErrorHandler instance. + */ + @Reference( + target = "(" + ERROR_HANDLER_NAME + "=*)", + cardinality = ReferenceCardinality.MULTIPLE, + policy = ReferencePolicy.DYNAMIC) + void setNamedJakartaErrorHandler( + final JakartaErrorHandler handler, final ServiceReference<JakartaErrorHandler> ref) { + this.errorHandler.addNamedDelegate(ref, handler); + } + + /** + * Remove named error handlers + * @param ref The ServiceReference associated with the JakartaErrorHandler instance to remove. + */ + void unsetNamedJakartaErrorHandler(final ServiceReference<JakartaErrorHandler> ref) { + this.errorHandler.removeNamedDelegate(ref); + } + @SuppressWarnings("deprecation") @Reference( name = "ErrorHandler", diff --git a/src/test/java/org/apache/sling/engine/impl/DefaultErrorHandlerTest.java b/src/test/java/org/apache/sling/engine/impl/DefaultErrorHandlerTest.java index 7ba3b55..9fb92e8 100644 --- a/src/test/java/org/apache/sling/engine/impl/DefaultErrorHandlerTest.java +++ b/src/test/java/org/apache/sling/engine/impl/DefaultErrorHandlerTest.java @@ -18,20 +18,31 @@ */ package org.apache.sling.engine.impl; +import java.io.IOException; import java.io.PrintWriter; import java.io.StringWriter; +import java.util.Hashtable; import org.apache.sling.api.SlingJakartaHttpServletRequest; import org.apache.sling.api.SlingJakartaHttpServletResponse; import org.apache.sling.api.request.RequestProgressTracker; +import org.apache.sling.api.servlets.JakartaErrorHandler; +import org.apache.sling.testing.mock.osgi.junit.OsgiContext; import org.junit.Before; +import org.junit.Rule; import org.junit.Test; +import org.osgi.framework.Constants; +import org.osgi.framework.ServiceReference; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.when; /** @@ -42,6 +53,9 @@ import static org.mockito.Mockito.when; */ public class DefaultErrorHandlerTest { + @Rule + public final OsgiContext context = new OsgiContext(); + private DefaultErrorHandler handler; private SlingJakartaHttpServletRequest request; private SlingJakartaHttpServletResponse response; @@ -115,4 +129,331 @@ public class DefaultErrorHandlerTest { responseBody.flush(); assertTrue(responseBody.toString().isEmpty()); } + + // ---------- named / non-html delegate selection ---------- + + @SuppressWarnings("unchecked") + private ServiceReference<JakartaErrorHandler> registerNamed( + final JakartaErrorHandler eh, final int ranking, final Object names) { + final Hashtable<String, Object> props = new Hashtable<>(); + props.put(SlingRequestProcessorImpl.ERROR_HANDLER_NAME, names); + props.put(Constants.SERVICE_RANKING, ranking); + return (ServiceReference<JakartaErrorHandler>) context.bundleContext() + .registerService(JakartaErrorHandler.class, eh, props) + .getReference(); + } + + private void accept(final String accept) { + when(request.getHeader("Accept")).thenReturn(accept); + } + + @Test + public void testNonHtmlRequestUsesNonHtmlDelegateForStatus() throws Exception { + final JakartaErrorHandler main = mock(JakartaErrorHandler.class); + final JakartaErrorHandler nonHtml = mock(JakartaErrorHandler.class); + handler.setDelegate(null, main); + handler.addNamedDelegate(registerNamed(nonHtml, 0, DefaultErrorHandler.NON_HTML), nonHtml); + accept("application/json"); + + handler.handleError(404, "not found", request, response); + + verify(nonHtml).handleError(404, "not found", request, response); + verifyNoInteractions(main); + } + + @Test + public void testNonHtmlRequestUsesNonHtmlDelegateForThrowable() throws Exception { + final JakartaErrorHandler main = mock(JakartaErrorHandler.class); + final JakartaErrorHandler nonHtml = mock(JakartaErrorHandler.class); + handler.setDelegate(null, main); + handler.addNamedDelegate(registerNamed(nonHtml, 0, DefaultErrorHandler.NON_HTML), nonHtml); + accept("application/json"); + + final Exception cause = new IllegalStateException("boom"); + handler.handleError(cause, request, response); + + verify(nonHtml).handleError(cause, request, response); + verifyNoInteractions(main); + } + + @Test + public void testHtmlRequestUsesMainDelegateEvenIfNonHtmlDelegateExists() throws Exception { + final JakartaErrorHandler main = mock(JakartaErrorHandler.class); + final JakartaErrorHandler nonHtml = mock(JakartaErrorHandler.class); + handler.setDelegate(null, main); + handler.addNamedDelegate(registerNamed(nonHtml, 0, DefaultErrorHandler.NON_HTML), nonHtml); + accept("text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8"); + + handler.handleError(500, "error", request, response); + + verify(main).handleError(500, "error", request, response); + verifyNoInteractions(nonHtml); + } + + @Test + public void testMissingAcceptHeaderIsTreatedAsNonHtml() throws Exception { + final JakartaErrorHandler main = mock(JakartaErrorHandler.class); + final JakartaErrorHandler nonHtml = mock(JakartaErrorHandler.class); + handler.setDelegate(null, main); + handler.addNamedDelegate(registerNamed(nonHtml, 0, DefaultErrorHandler.NON_HTML), nonHtml); + + handler.handleError(404, null, request, response); + + verify(nonHtml).handleError(404, null, request, response); + verifyNoInteractions(main); + } + + @Test + public void testBlankAcceptHeaderIsTreatedAsNonHtml() throws Exception { + final JakartaErrorHandler main = mock(JakartaErrorHandler.class); + final JakartaErrorHandler nonHtml = mock(JakartaErrorHandler.class); + handler.setDelegate(null, main); + handler.addNamedDelegate(registerNamed(nonHtml, 0, DefaultErrorHandler.NON_HTML), nonHtml); + accept(" "); + + handler.handleError(404, null, request, response); + + verify(nonHtml).handleError(404, null, request, response); + verifyNoInteractions(main); + } + + @Test + public void testMissingAcceptHeaderWithoutNonHtmlDelegateUsesMainDelegate() throws Exception { + final JakartaErrorHandler main = mock(JakartaErrorHandler.class); + handler.setDelegate(null, main); + + handler.handleError(404, null, request, response); + + verify(main).handleError(404, null, request, response); + } + + @Test + public void testWildcardAcceptIsTreatedAsHtml() throws Exception { + final JakartaErrorHandler main = mock(JakartaErrorHandler.class); + final JakartaErrorHandler nonHtml = mock(JakartaErrorHandler.class); + handler.setDelegate(null, main); + handler.addNamedDelegate(registerNamed(nonHtml, 0, DefaultErrorHandler.NON_HTML), nonHtml); + accept("*/*"); + + handler.handleError(404, null, request, response); + + verify(main).handleError(404, null, request, response); + verifyNoInteractions(nonHtml); + } + + @Test + public void testWildcardWithLowerQualityIsTreatedAsHtml() throws Exception { + final JakartaErrorHandler main = mock(JakartaErrorHandler.class); + final JakartaErrorHandler nonHtml = mock(JakartaErrorHandler.class); + handler.setDelegate(null, main); + handler.addNamedDelegate(registerNamed(nonHtml, 0, DefaultErrorHandler.NON_HTML), nonHtml); + accept("application/json, */*;q=0.1"); + + handler.handleError(404, null, request, response); + + verify(main).handleError(404, null, request, response); + verifyNoInteractions(nonHtml); + } + + @Test + public void testTextWildcardAcceptIsTreatedAsNonHtml() throws Exception { + final JakartaErrorHandler main = mock(JakartaErrorHandler.class); + final JakartaErrorHandler nonHtml = mock(JakartaErrorHandler.class); + handler.setDelegate(null, main); + handler.addNamedDelegate(registerNamed(nonHtml, 0, DefaultErrorHandler.NON_HTML), nonHtml); + accept("text/*"); + + handler.handleError(404, null, request, response); + + verify(nonHtml).handleError(404, null, request, response); + verifyNoInteractions(main); + } + + @Test + public void testHtmlWithLowerQualityIsStillTreatedAsHtml() throws Exception { + final JakartaErrorHandler main = mock(JakartaErrorHandler.class); + final JakartaErrorHandler nonHtml = mock(JakartaErrorHandler.class); + handler.setDelegate(null, main); + handler.addNamedDelegate(registerNamed(nonHtml, 0, DefaultErrorHandler.NON_HTML), nonHtml); + accept("application/json, text/html;q=0.1"); + + handler.handleError(404, null, request, response); + + verify(main).handleError(404, null, request, response); + verifyNoInteractions(nonHtml); + } + + @Test + public void testRequestParametersAreNotEvaluated() throws Exception { + final JakartaErrorHandler main = mock(JakartaErrorHandler.class); + final JakartaErrorHandler nonHtml = mock(JakartaErrorHandler.class); + handler.setDelegate(null, main); + handler.addNamedDelegate(registerNamed(nonHtml, 0, DefaultErrorHandler.NON_HTML), nonHtml); + accept("application/json"); + // parameter parsing might be the cause of the error being handled + when(request.getParameter(any())).thenThrow(new IllegalStateException("parameter parsing failed")); + + handler.handleError(400, null, request, response); + + verify(nonHtml).handleError(400, null, request, response); + verifyNoInteractions(main); + verify(request, never()).getParameter(any()); + } + + @Test + public void testNonHtmlRequestWithoutNonHtmlDelegateFallsBackToMainDelegate() throws Exception { + final JakartaErrorHandler main = mock(JakartaErrorHandler.class); + handler.setDelegate(null, main); + accept("application/json"); + + handler.handleError(404, null, request, response); + + verify(main).handleError(404, null, request, response); + } + + @Test + public void testNonHtmlRequestWithoutAnyDelegateUsesBuiltInResponse() throws Exception { + accept("application/json"); + + handler.handleError(404, "not found", request, response); + + verify(response).setStatus(404); + responseBody.flush(); + assertTrue(responseBody.toString().contains("not found")); + } + + @Test + public void testNonHtmlDelegateWithoutMainDelegate() throws Exception { + final JakartaErrorHandler nonHtml = mock(JakartaErrorHandler.class); + handler.addNamedDelegate(registerNamed(nonHtml, 0, DefaultErrorHandler.NON_HTML), nonHtml); + + accept("application/json"); + handler.handleError(404, null, request, response); + verify(nonHtml).handleError(404, null, request, response); + + // html requests must not be routed to the non-html delegate + accept("text/html"); + handler.handleError(403, null, request, response); + verify(nonHtml, never()).handleError(403, null, request, response); + verify(response).setStatus(403); + } + + @Test + public void testNamedDelegateWithOtherNameIsIgnored() throws Exception { + final JakartaErrorHandler main = mock(JakartaErrorHandler.class); + final JakartaErrorHandler other = mock(JakartaErrorHandler.class); + handler.setDelegate(null, main); + handler.addNamedDelegate(registerNamed(other, 100, "something-else"), other); + accept("application/json"); + + handler.handleError(404, null, request, response); + + verify(main).handleError(404, null, request, response); + verifyNoInteractions(other); + } + + @Test + public void testNamedDelegateWithMultipleNamesIncludingNonHtml() throws Exception { + final JakartaErrorHandler nonHtml = mock(JakartaErrorHandler.class); + handler.addNamedDelegate( + registerNamed(nonHtml, 0, new String[] {"something-else", DefaultErrorHandler.NON_HTML}), nonHtml); + accept("application/json"); + + handler.handleError(404, null, request, response); + + verify(nonHtml).handleError(404, null, request, response); + } + + @Test + public void testHighestRankedNonHtmlDelegateWinsRegardlessOfBindOrder() throws Exception { + final JakartaErrorHandler low = mock(JakartaErrorHandler.class); + final JakartaErrorHandler high = mock(JakartaErrorHandler.class); + final JakartaErrorHandler mid = mock(JakartaErrorHandler.class); + handler.addNamedDelegate(registerNamed(low, -10, DefaultErrorHandler.NON_HTML), low); + handler.addNamedDelegate(registerNamed(high, 100, DefaultErrorHandler.NON_HTML), high); + handler.addNamedDelegate(registerNamed(mid, 10, DefaultErrorHandler.NON_HTML), mid); + accept("application/json"); + + handler.handleError(404, null, request, response); + + verify(high).handleError(404, null, request, response); + verifyNoInteractions(low, mid); + } + + @Test + public void testOlderNonHtmlDelegateWinsOnEqualRanking() throws Exception { + final JakartaErrorHandler first = mock(JakartaErrorHandler.class); + final JakartaErrorHandler second = mock(JakartaErrorHandler.class); + final ServiceReference<JakartaErrorHandler> firstRef = registerNamed(first, 0, DefaultErrorHandler.NON_HTML); + final ServiceReference<JakartaErrorHandler> secondRef = registerNamed(second, 0, DefaultErrorHandler.NON_HTML); + // bind in reverse registration order + handler.addNamedDelegate(secondRef, second); + handler.addNamedDelegate(firstRef, first); + accept("application/json"); + + handler.handleError(404, null, request, response); + + verify(first).handleError(404, null, request, response); + verifyNoInteractions(second); + } + + @Test + public void testRemovingNonHtmlDelegateFallsBackToNextRankedAndThenToMain() throws Exception { + final JakartaErrorHandler main = mock(JakartaErrorHandler.class); + final JakartaErrorHandler low = mock(JakartaErrorHandler.class); + final JakartaErrorHandler high = mock(JakartaErrorHandler.class); + handler.setDelegate(null, main); + final ServiceReference<JakartaErrorHandler> lowRef = registerNamed(low, 0, DefaultErrorHandler.NON_HTML); + final ServiceReference<JakartaErrorHandler> highRef = registerNamed(high, 10, DefaultErrorHandler.NON_HTML); + handler.addNamedDelegate(lowRef, low); + handler.addNamedDelegate(highRef, high); + accept("application/json"); + + handler.removeNamedDelegate(highRef); + handler.handleError(404, null, request, response); + verify(low).handleError(404, null, request, response); + verifyNoInteractions(high); + + handler.removeNamedDelegate(lowRef); + handler.handleError(410, null, request, response); + verify(main).handleError(410, null, request, response); + verify(low, never()).handleError(410, null, request, response); + } + + @Test + public void testRemovingUnknownNamedDelegateIsHarmless() throws Exception { + final JakartaErrorHandler nonHtml = mock(JakartaErrorHandler.class); + final JakartaErrorHandler unknown = mock(JakartaErrorHandler.class); + handler.addNamedDelegate(registerNamed(nonHtml, 0, DefaultErrorHandler.NON_HTML), nonHtml); + accept("application/json"); + + handler.removeNamedDelegate(registerNamed(unknown, 0, DefaultErrorHandler.NON_HTML)); + handler.handleError(404, null, request, response); + + verify(nonHtml).handleError(404, null, request, response); + } + + @Test + public void testFailingNonHtmlDelegateFallsBackToBuiltInResponse() throws Exception { + final JakartaErrorHandler main = mock(JakartaErrorHandler.class); + final JakartaErrorHandler nonHtml = mock(JakartaErrorHandler.class); + doThrow(new IOException("failed")) + .when(nonHtml) + .handleError( + anyInt(), + any(), + any(SlingJakartaHttpServletRequest.class), + any(SlingJakartaHttpServletResponse.class)); + handler.setDelegate(null, main); + handler.addNamedDelegate(registerNamed(nonHtml, 0, DefaultErrorHandler.NON_HTML), nonHtml); + accept("application/json"); + + handler.handleError(404, "not found", request, response); + + verify(response).reset(); + verify(response).setStatus(404); + verifyNoInteractions(main); + responseBody.flush(); + assertTrue(responseBody.toString().contains("not found")); + } } diff --git a/src/test/java/org/apache/sling/engine/impl/DefaultNonHtmlErrorHandlerTest.java b/src/test/java/org/apache/sling/engine/impl/DefaultNonHtmlErrorHandlerTest.java new file mode 100644 index 0000000..d659255 --- /dev/null +++ b/src/test/java/org/apache/sling/engine/impl/DefaultNonHtmlErrorHandlerTest.java @@ -0,0 +1,70 @@ +/* + * 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.engine.impl; + +import org.apache.sling.api.SlingJakartaHttpServletRequest; +import org.apache.sling.api.SlingJakartaHttpServletResponse; +import org.junit.Before; +import org.junit.Test; +import org.mockito.InOrder; + +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.inOrder; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; + +public class DefaultNonHtmlErrorHandlerTest { + + private DefaultNonHtmlErrorHandler handler; + private SlingJakartaHttpServletRequest request; + private SlingJakartaHttpServletResponse response; + + @Before + public void setup() { + handler = new DefaultNonHtmlErrorHandler(); + request = mock(SlingJakartaHttpServletRequest.class); + response = mock(SlingJakartaHttpServletResponse.class); + } + + @Test + public void testStatusIsSentWithEmptyBody() throws Exception { + handler.handleError(404, "secret detail", request, response); + + final InOrder order = inOrder(response); + order.verify(response).setStatus(404); + order.verify(response).setContentLength(0); + order.verify(response).flushBuffer(); + verify(response, never()).getWriter(); + verify(response, never()).getOutputStream(); + verify(response, never()).setContentType(anyString()); + } + + @Test + public void testThrowableIsSentAsInternalServerErrorWithEmptyBody() throws Exception { + handler.handleError(new IllegalStateException("secret detail"), request, response); + + final InOrder order = inOrder(response); + order.verify(response).setStatus(500); + order.verify(response).setContentLength(0); + order.verify(response).flushBuffer(); + verify(response, never()).getWriter(); + verify(response, never()).getOutputStream(); + } +} diff --git a/src/test/java/org/apache/sling/engine/impl/SlingRequestProcessorImplTest.java b/src/test/java/org/apache/sling/engine/impl/SlingRequestProcessorImplTest.java index 8f38a40..35a7693 100644 --- a/src/test/java/org/apache/sling/engine/impl/SlingRequestProcessorImplTest.java +++ b/src/test/java/org/apache/sling/engine/impl/SlingRequestProcessorImplTest.java @@ -21,6 +21,7 @@ package org.apache.sling.engine.impl; import java.io.PrintWriter; import java.io.StringWriter; import java.lang.reflect.Field; +import java.util.Collections; import jakarta.servlet.Servlet; import jakarta.servlet.ServletRequest; @@ -33,22 +34,29 @@ import org.apache.sling.api.request.RequestProgressTracker; import org.apache.sling.api.resource.Resource; import org.apache.sling.api.resource.ResourceMetadata; import org.apache.sling.api.resource.ResourceResolver; +import org.apache.sling.api.servlets.JakartaErrorHandler; import org.apache.sling.api.servlets.ServletResolver; import org.apache.sling.engine.impl.filter.FilterHandle; import org.apache.sling.engine.impl.filter.ServletFilterManager; import org.apache.sling.engine.impl.filter.ServletFilterManager.FilterChainType; import org.apache.sling.engine.impl.parameters.SlingParameterParseException; +import org.apache.sling.testing.mock.osgi.junit.OsgiContext; import org.jetbrains.annotations.NotNull; import org.junit.Before; +import org.junit.Rule; import org.junit.Test; +import org.osgi.framework.Constants; +import org.osgi.framework.ServiceRegistration; import static jakarta.servlet.http.HttpServletResponse.SC_BAD_REQUEST; import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.when; /** @@ -59,6 +67,9 @@ import static org.mockito.Mockito.when; */ public class SlingRequestProcessorImplTest { + @Rule + public final OsgiContext context = new OsgiContext(); + private SlingRequestProcessorImpl processor; private ServletFilterManager filterManager; private SlingJakartaHttpServletRequest request; @@ -173,4 +184,103 @@ public class SlingRequestProcessorImplTest { when(resourceMetadata.getResolutionPathInfo()).thenReturn(path); return resource; } + + // ---------- error handler service wiring (DS reference targets) ---------- + + private static SlingJakartaHttpServletRequest anyRequest() { + return any(SlingJakartaHttpServletRequest.class); + } + + private static SlingJakartaHttpServletResponse anyResponse() { + return any(SlingJakartaHttpServletResponse.class); + } + + private SlingRequestProcessorImpl activateProcessorWithOsgi() { + context.registerService(ServletResolver.class, mock(ServletResolver.class)); + context.registerService(ServletFilterManager.class, filterManager); + context.registerService(RequestProcessorMBeanImpl.class, mock(RequestProcessorMBeanImpl.class)); + return context.registerInjectActivateService(SlingRequestProcessorImpl.class); + } + + @Test + public void testNamedErrorHandlerIsNotBoundAsDefaultErrorHandler() throws Exception { + final JakartaErrorHandler main = mock(JakartaErrorHandler.class); + final JakartaErrorHandler nonHtml = mock(JakartaErrorHandler.class); + context.registerService(JakartaErrorHandler.class, main, Constants.SERVICE_RANKING, 0); + // higher ranking, but must not replace the default handler as it is a named one + context.registerService( + JakartaErrorHandler.class, + nonHtml, + Constants.SERVICE_RANKING, + 1000, + SlingRequestProcessorImpl.ERROR_HANDLER_NAME, + DefaultErrorHandler.NON_HTML); + final SlingRequestProcessorImpl osgiProcessor = activateProcessorWithOsgi(); + + when(request.getHeader("Accept")).thenReturn("text/html"); + osgiProcessor.handleError(404, "not found", request, response); + + verify(main).handleError(eq(404), eq("not found"), anyRequest(), anyResponse()); + verifyNoInteractions(nonHtml); + } + + @Test + public void testNamedNonHtmlErrorHandlerIsUsedForNonHtmlRequests() throws Exception { + final JakartaErrorHandler main = mock(JakartaErrorHandler.class); + context.registerService(JakartaErrorHandler.class, main); + final SlingRequestProcessorImpl osgiProcessor = activateProcessorWithOsgi(); + + // registered after activation: dynamic binding + final JakartaErrorHandler nonHtml = mock(JakartaErrorHandler.class); + context.registerService( + JakartaErrorHandler.class, + nonHtml, + SlingRequestProcessorImpl.ERROR_HANDLER_NAME, + DefaultErrorHandler.NON_HTML); + + when(request.getHeader("Accept")).thenReturn("application/json"); + osgiProcessor.handleError(404, "not found", request, response); + + verify(nonHtml).handleError(eq(404), eq("not found"), anyRequest(), anyResponse()); + verifyNoInteractions(main); + } + + @Test + public void testUnregisteringNonHtmlErrorHandlerFallsBackToDefaultErrorHandler() throws Exception { + final JakartaErrorHandler main = mock(JakartaErrorHandler.class); + final JakartaErrorHandler nonHtml = mock(JakartaErrorHandler.class); + context.registerService(JakartaErrorHandler.class, main); + final ServiceRegistration<JakartaErrorHandler> reg = context.bundleContext() + .registerService( + JakartaErrorHandler.class, + nonHtml, + new java.util.Hashtable<>(Collections.singletonMap( + SlingRequestProcessorImpl.ERROR_HANDLER_NAME, DefaultErrorHandler.NON_HTML))); + final SlingRequestProcessorImpl osgiProcessor = activateProcessorWithOsgi(); + + reg.unregister(); + when(request.getHeader("Accept")).thenReturn("application/json"); + osgiProcessor.handleError(404, "not found", request, response); + + verify(main).handleError(eq(404), eq("not found"), anyRequest(), anyResponse()); + verifyNoInteractions(nonHtml); + } + + @Test + public void testDefaultNonHtmlErrorHandlerComponentIsUsedForNonHtmlRequests() throws Exception { + final JakartaErrorHandler main = mock(JakartaErrorHandler.class); + context.registerService(JakartaErrorHandler.class, main); + // registered with the component properties from its DS metadata + context.registerInjectActivateService(DefaultNonHtmlErrorHandler.class); + final SlingRequestProcessorImpl osgiProcessor = activateProcessorWithOsgi(); + + when(request.getHeader("Accept")).thenReturn("application/json"); + osgiProcessor.handleError(404, "not found", request, response); + + verify(response).setStatus(404); + verify(response).setContentLength(0); + verifyNoInteractions(main); + responseBody.flush(); + assertTrue(responseBody.toString().isEmpty()); + } }
