From 9af1f91c1ada819bcb0fe1951c0f6eaa60b44eea Mon Sep 17 00:00:00 2001 From: Andrea Aime Date: Wed, 18 Nov 2015 19:01:02 +0100 Subject: [PATCH] [GEOS-7318] A HTTPErrorCodeException gets ignored when wrapped inside a ServiceException --- .../java/org/geoserver/ows/Dispatcher.java | 95 ++++++++----------- .../org/geoserver/ows/DispatcherTest.java | 12 ++- .../java/org/geoserver/ows/HelloWorld.java | 12 ++- .../org/geoserver/ows/applicationContext.xml | 1 + 4 files changed, 63 insertions(+), 57 deletions(-) diff --git a/src/ows/src/main/java/org/geoserver/ows/Dispatcher.java b/src/ows/src/main/java/org/geoserver/ows/Dispatcher.java index 58f5ff1a942..d3e9ca632a9 100644 --- a/src/ows/src/main/java/org/geoserver/ows/Dispatcher.java +++ b/src/ows/src/main/java/org/geoserver/ows/Dispatcher.java @@ -1607,7 +1607,9 @@ Map readOpPost(BufferedReader input) throws Exception { void exception(Throwable t, Service service, Request request) { Throwable current = t; - while (current != null && !(current instanceof ClientStreamAbortedException) && !(isSecurityException(current))) { + while (current != null && !(current instanceof ClientStreamAbortedException) + && !isSecurityException(current) + && !(current instanceof HttpErrorCodeException)) { if(current instanceof SAXException) current = ((SAXException) current).getException(); else @@ -1617,46 +1619,53 @@ void exception(Throwable t, Service service, Request request) { logger.log(Level.FINER, "Client has closed stream", t); return; } - if ( isSecurityException(current)) + if ( isSecurityException(current)) { throw (RuntimeException) current; - - - //unwind the exception stack until we find one we know about - Throwable cause = t; - while( cause != null ) { - if ( cause instanceof ServiceException ) { - break; - } - if ( cause instanceof HttpErrorCodeException ) { - break; - } - if ( isSecurityException(cause) ) { - break; - } - - cause = cause.getCause(); - } - - if ( cause == null ) { - //did not fine a "special" exception, create a service exception - // by default - cause = new ServiceException(t); } - if (!(cause instanceof HttpErrorCodeException)) { - logger.log(Level.SEVERE, "", t); - } else { - int errorCode = ((HttpErrorCodeException)cause).getErrorCode(); + if (current instanceof HttpErrorCodeException) { + HttpErrorCodeException ece = (HttpErrorCodeException) current; + int errorCode = ece.getErrorCode(); if (errorCode < 199 || errorCode > 299) { logger.log(Level.FINE, "", t); + } else { + logger.log(Level.FINER, "", t); } - else{ + + try { + if(ece.getMessage() != null) { + request.getHttpResponse().sendError(ece.getErrorCode(),ece.getMessage()); + } else { + request.getHttpResponse().sendError(ece.getErrorCode()); + } + if (ece.getErrorCode() < 400) { + // gwc returns an HttpErrorCodeException for 304s + // we don't want to flag these as errors for upstream filters, ie the monitoring extension + t = null; + } + } catch (IOException e) { + // means the resposne was already commited something logger.log(Level.FINER, "", t); } - } - + } else { + logger.log(Level.SEVERE, "", t); - if ( cause instanceof ServiceException ) { + //unwind the exception stack until we find one we know about + Throwable cause = t; + while( cause != null ) { + if ( cause instanceof ServiceException ) { + break; + } + + cause = cause.getCause(); + } + + if ( cause == null ) { + // did not fine a "special" exception, create a service exception by default + cause = new ServiceException(t); + } + + // at this point we're sure it'a service exception ServiceException se = (ServiceException) cause; if ( cause != t ) { //copy the message, code + locator, but set cause equal to root @@ -1665,28 +1674,6 @@ void exception(Throwable t, Service service, Request request) { handleServiceException(se,service,request); } - else if ( cause instanceof HttpErrorCodeException ) { - //TODO: log the exception stack trace - - //set the error code - HttpErrorCodeException ece = (HttpErrorCodeException) cause; - try { - if(ece.getMessage() != null) { - request.getHttpResponse().sendError(ece.getErrorCode(),ece.getMessage()); - } else { - request.getHttpResponse().sendError(ece.getErrorCode()); - } - if (ece.getErrorCode() < 400) { - // gwc returns an HttpErrorCodeException for 304s - // we don't want to flag these as errors for upstream filters, ie the monitoring extension - t = null; - } - } - catch (IOException e) { - //means the resposne was already commited - //TODO: something - } - } request.error = t; } diff --git a/src/ows/src/test/java/org/geoserver/ows/DispatcherTest.java b/src/ows/src/test/java/org/geoserver/ows/DispatcherTest.java index 32e125c5466..2909d2f4362 100644 --- a/src/ows/src/test/java/org/geoserver/ows/DispatcherTest.java +++ b/src/ows/src/test/java/org/geoserver/ows/DispatcherTest.java @@ -342,6 +342,14 @@ public int available(){ } public void testHttpErrorCodeException() throws Exception { + assertHttpErrorCode("httpErrorCodeException"); + } + + public void testWrappedHttpErrorCodeException() throws Exception { + assertHttpErrorCode("wrappedHttpErrorCodeException"); + } + + private void assertHttpErrorCode(String requestType) throws Exception { URL url = getClass().getResource("applicationContext.xml"); FileSystemXmlApplicationContext context = new FileSystemXmlApplicationContext(url.toString()); @@ -373,7 +381,7 @@ public void setCharacterEncoding(String encoding) { CodeExpectingHttpServletResponse response = new CodeExpectingHttpServletResponse(new MockHttpServletResponse()); request.setupAddParameter("service", "hello"); - request.setupAddParameter("request", "httpErrorCodeException"); + request.setupAddParameter("request", requestType); request.setupAddParameter("version", "1.0.0"); request.setRequestURI( @@ -382,7 +390,7 @@ public void setCharacterEncoding(String encoding) { dispatcher.handleRequest(request, response); assertEquals(HttpServletResponse.SC_NO_CONTENT, response.getStatusCode()); - } + } /** * Assert that if the service bean implements the optional {@link DirectInvocationService} diff --git a/src/ows/src/test/java/org/geoserver/ows/HelloWorld.java b/src/ows/src/test/java/org/geoserver/ows/HelloWorld.java index 9c4e60be2ae..59c56ba44f5 100644 --- a/src/ows/src/test/java/org/geoserver/ows/HelloWorld.java +++ b/src/ows/src/test/java/org/geoserver/ows/HelloWorld.java @@ -1,4 +1,4 @@ -/* (c) 2014 Open Source Geospatial Foundation - all rights reserved +/* (c) 2014 - 2015 Open Source Geospatial Foundation - all rights reserved * (c) 2001 - 2013 OpenPlans * This code is licensed under the GPL 2.0 license, available at the root * application directory. @@ -7,6 +7,8 @@ import javax.servlet.http.HttpServletResponse; +import org.geoserver.platform.ServiceException; + public class HelloWorld { Message message; @@ -17,4 +19,12 @@ public Message hello(Message message) { public void httpErrorCodeException() { throw new HttpErrorCodeException( HttpServletResponse.SC_NO_CONTENT ); } + + public void wrappedHttpErrorCodeException() { + try { + throw new HttpErrorCodeException( HttpServletResponse.SC_NO_CONTENT ); + } catch(Exception e) { + throw new ServiceException("Wrapping code error", e); + } + } } diff --git a/src/ows/src/test/java/org/geoserver/ows/applicationContext.xml b/src/ows/src/test/java/org/geoserver/ows/applicationContext.xml index 961b25159ce..34649d2a4e1 100644 --- a/src/ows/src/test/java/org/geoserver/ows/applicationContext.xml +++ b/src/ows/src/test/java/org/geoserver/ows/applicationContext.xml @@ -37,6 +37,7 @@ hello httpErrorCodeException + wrappedHttpErrorCodeException