From 5b0a18e6e362c5b373a39c703c55b0cc1c7d76dd Mon Sep 17 00:00:00 2001 From: Pablo Diaz Date: Tue, 10 Nov 2015 12:14:52 +0100 Subject: [PATCH 1/5] Send internal errors to logger --- .../restrouter/handlers/DefaultKaryonErrorHandler.java | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/main/java/scmspain/karyon/restrouter/handlers/DefaultKaryonErrorHandler.java b/src/main/java/scmspain/karyon/restrouter/handlers/DefaultKaryonErrorHandler.java index 87dd01c..6bf4324 100644 --- a/src/main/java/scmspain/karyon/restrouter/handlers/DefaultKaryonErrorHandler.java +++ b/src/main/java/scmspain/karyon/restrouter/handlers/DefaultKaryonErrorHandler.java @@ -4,6 +4,8 @@ import io.netty.buffer.ByteBuf; import io.netty.handler.codec.http.HttpResponseStatus; import io.reactivex.netty.protocol.http.server.HttpServerRequest; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import rx.Observable; import scmspain.karyon.restrouter.exception.CannotSerializeException; import scmspain.karyon.restrouter.exception.InvalidAcceptHeaderException; @@ -16,6 +18,7 @@ * handle the generated exception */ public class DefaultKaryonErrorHandler implements ErrorHandler { + private static final Logger L = LoggerFactory.getLogger(DefaultKaryonErrorHandler.class); @Override public Observable handleError( @@ -29,7 +32,7 @@ public Observable handleError( return Observable.just(new RestRouterErrorDTO("Path: " + request.getPath() + " NOT FOUND")); - } else if (throwable instanceof ParamAnnotationException) { + } else if (throwable instanceof ParamAnnotationException) { statusCode.set(HttpResponseStatus.BAD_REQUEST); return Observable.empty(); @@ -54,6 +57,9 @@ public Observable handleError( } else { statusCode.set(HttpResponseStatus.INTERNAL_SERVER_ERROR); + + L.error("Internal server error", throwable); + return Observable.error(throwable); } } From 417f713a433a39cc5a9e9177ffba22ce120d7b75 Mon Sep 17 00:00:00 2001 From: Pablo Diaz Date: Wed, 11 Nov 2015 16:24:28 +0100 Subject: [PATCH 2/5] Changed the logger to handle errors with all type of endpoints --- .../java/scmspain/karyon/restrouter/RestRouterHandler.java | 6 ++++-- .../restrouter/handlers/DefaultKaryonErrorHandler.java | 6 ------ 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/src/main/java/scmspain/karyon/restrouter/RestRouterHandler.java b/src/main/java/scmspain/karyon/restrouter/RestRouterHandler.java index 3c9e05b..8a1baea 100644 --- a/src/main/java/scmspain/karyon/restrouter/RestRouterHandler.java +++ b/src/main/java/scmspain/karyon/restrouter/RestRouterHandler.java @@ -8,6 +8,8 @@ import io.reactivex.netty.protocol.http.server.RequestHandler; import org.apache.commons.lang.StringUtils; import org.commonjava.mimeparse.MIMEParse; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import rx.Observable; import scmspain.karyon.restrouter.exception.CannotSerializeException; import scmspain.karyon.restrouter.exception.InvalidAcceptHeaderException; @@ -28,6 +30,7 @@ import java.util.stream.Stream; public class RestRouterHandler implements RequestHandler { + private static final Logger L = LoggerFactory.getLogger(RestRouterHandler.class); private SerializeManager serializerManager; private RestUriRouter restUriRouter; private DefaultKaryonErrorHandler defaultKaryonErrorHandler = new DefaultKaryonErrorHandler(); @@ -87,10 +90,9 @@ public Observable handle(HttpServerRequest request, } - return result; + return result.doOnError(throwable -> L.error("Internal Server Error", throwable)); } - private Observable handleSupported(Route route, HttpServerRequest request, HttpServerResponse response) { diff --git a/src/main/java/scmspain/karyon/restrouter/handlers/DefaultKaryonErrorHandler.java b/src/main/java/scmspain/karyon/restrouter/handlers/DefaultKaryonErrorHandler.java index 6bf4324..129b76f 100644 --- a/src/main/java/scmspain/karyon/restrouter/handlers/DefaultKaryonErrorHandler.java +++ b/src/main/java/scmspain/karyon/restrouter/handlers/DefaultKaryonErrorHandler.java @@ -4,8 +4,6 @@ import io.netty.buffer.ByteBuf; import io.netty.handler.codec.http.HttpResponseStatus; import io.reactivex.netty.protocol.http.server.HttpServerRequest; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; import rx.Observable; import scmspain.karyon.restrouter.exception.CannotSerializeException; import scmspain.karyon.restrouter.exception.InvalidAcceptHeaderException; @@ -18,7 +16,6 @@ * handle the generated exception */ public class DefaultKaryonErrorHandler implements ErrorHandler { - private static final Logger L = LoggerFactory.getLogger(DefaultKaryonErrorHandler.class); @Override public Observable handleError( @@ -57,9 +54,6 @@ public Observable handleError( } else { statusCode.set(HttpResponseStatus.INTERNAL_SERVER_ERROR); - - L.error("Internal server error", throwable); - return Observable.error(throwable); } } From d152aa6f660089447562e25a913479dc4e349858 Mon Sep 17 00:00:00 2001 From: Pablo Diaz Date: Thu, 12 Nov 2015 18:29:41 +0100 Subject: [PATCH 3/5] added some context to error --- .../scmspain/karyon/restrouter/RestRouterHandler.java | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/main/java/scmspain/karyon/restrouter/RestRouterHandler.java b/src/main/java/scmspain/karyon/restrouter/RestRouterHandler.java index 8a1baea..49b3010 100644 --- a/src/main/java/scmspain/karyon/restrouter/RestRouterHandler.java +++ b/src/main/java/scmspain/karyon/restrouter/RestRouterHandler.java @@ -90,7 +90,16 @@ public Observable handle(HttpServerRequest request, } - return result.doOnError(throwable -> L.error("Internal Server Error", throwable)); + return result.doOnError(throwable -> logError(throwable, request)); + } + + private void logError(Throwable throwable, HttpServerRequest request) { + String method = request.getHttpMethod().name(); + String path = request.getPath(); + String message = String.format("Internal server error requesting [%s %s]", method, path); + + L.error(message, throwable); + } private Observable handleSupported(Route route, From 77a03136c65e4d78ef004f66c045d5e1e004dc99 Mon Sep 17 00:00:00 2001 From: Pablo Diaz Date: Mon, 16 Nov 2015 11:20:19 +0100 Subject: [PATCH 4/5] Changed error implementation, now it doesnt propagate the error to karyon to be able to intercept the output --- .../karyon/restrouter/RestRouterHandler.java | 40 +++++++++++++------ .../karyon/restrouter/RestRouterScanner.java | 4 +- ...java => KaryonRestRouterErrorHandler.java} | 7 +++- .../restrouter/RestRouterHandlerTest.java | 9 +++-- 4 files changed, 40 insertions(+), 20 deletions(-) rename src/main/java/scmspain/karyon/restrouter/handlers/{DefaultKaryonErrorHandler.java => KaryonRestRouterErrorHandler.java} (90%) diff --git a/src/main/java/scmspain/karyon/restrouter/RestRouterHandler.java b/src/main/java/scmspain/karyon/restrouter/RestRouterHandler.java index 49b3010..e7010b4 100644 --- a/src/main/java/scmspain/karyon/restrouter/RestRouterHandler.java +++ b/src/main/java/scmspain/karyon/restrouter/RestRouterHandler.java @@ -3,6 +3,8 @@ import com.google.common.net.HttpHeaders; import com.google.common.net.MediaType; import io.netty.buffer.ByteBuf; +import io.netty.handler.codec.http.HttpMethod; +import io.netty.handler.codec.http.HttpResponseStatus; import io.reactivex.netty.protocol.http.server.HttpServerRequest; import io.reactivex.netty.protocol.http.server.HttpServerResponse; import io.reactivex.netty.protocol.http.server.RequestHandler; @@ -13,7 +15,7 @@ import rx.Observable; import scmspain.karyon.restrouter.exception.CannotSerializeException; import scmspain.karyon.restrouter.exception.InvalidAcceptHeaderException; -import scmspain.karyon.restrouter.handlers.DefaultKaryonErrorHandler; +import scmspain.karyon.restrouter.handlers.KaryonRestRouterErrorHandler; import scmspain.karyon.restrouter.handlers.ErrorHandler; import scmspain.karyon.restrouter.handlers.RestRouterErrorDTO; import scmspain.karyon.restrouter.serializer.SerializeManager; @@ -30,12 +32,14 @@ import java.util.stream.Stream; public class RestRouterHandler implements RequestHandler { - private static final Logger L = LoggerFactory.getLogger(RestRouterHandler.class); private SerializeManager serializerManager; private RestUriRouter restUriRouter; - private DefaultKaryonErrorHandler defaultKaryonErrorHandler = new DefaultKaryonErrorHandler(); + private KaryonRestRouterErrorHandler karyonRestRouterErrorHandler = new KaryonRestRouterErrorHandler(); private ErrorHandler errorHandler; + private static final Logger L = LoggerFactory.getLogger(RestRouterHandler.class); + + /** * Creates an instance * @param restUriRouter the rest uri router @@ -90,18 +94,30 @@ public Observable handle(HttpServerRequest request, } - return result.doOnError(throwable -> logError(throwable, request)); - } + return result.onErrorResumeNext(throwable -> { + response.setStatus(HttpResponseStatus.INTERNAL_SERVER_ERROR); - private void logError(Throwable throwable, HttpServerRequest request) { - String method = request.getHttpMethod().name(); - String path = request.getPath(); - String message = String.format("Internal server error requesting [%s %s]", method, path); + logError(throwable, request); - L.error(message, throwable); + return Observable.empty(); + }); + } + private void logError(Throwable throwable, HttpServerRequest request) { + try { + HttpMethod method = request.getHttpMethod(); + String methodStr = method != null ? method.name() : ""; + String path = request.getPath(); + String message = String.format("Internal server error requesting [%s %s]", methodStr, path); + + L.error(message, throwable); + } catch (Throwable t) { + // Just to be sure we don't generate additional errors + L.error("Internal server error", throwable); + } } + private Observable handleSupported(Route route, HttpServerRequest request, HttpServerResponse response) { @@ -133,7 +149,7 @@ private Observable handleSupported(Route route, // If RouteNotFound is not handle it will be handled here resultObs = resultObs.onErrorResumeNext(throwable -> { - return defaultKaryonErrorHandler.handleError(request, throwable, response::setStatus); + return karyonRestRouterErrorHandler.handleError(request, throwable, response::setStatus); }); Serializer serializer = serializerManager.getSerializer(contentType) @@ -153,7 +169,7 @@ private Observable handleCustomSerialization(Route route .process(request, response); resultObs = resultObs.onErrorResumeNext(throwable -> { - return defaultKaryonErrorHandler.handleError(request, throwable, response::setStatus) + return karyonRestRouterErrorHandler.handleError(request, throwable, response::setStatus) .map(this::serializeErrorDto) .flatMap(response::writeStringAndFlush); }); diff --git a/src/main/java/scmspain/karyon/restrouter/RestRouterScanner.java b/src/main/java/scmspain/karyon/restrouter/RestRouterScanner.java index f2702a1..bc0e0a2 100644 --- a/src/main/java/scmspain/karyon/restrouter/RestRouterScanner.java +++ b/src/main/java/scmspain/karyon/restrouter/RestRouterScanner.java @@ -84,13 +84,13 @@ private void validate(SerializeManager serializeManager) { } if(!serializeManager.getSupportedMediaTypes().containsAll(produces)) { - RuntimeException e = new RuntimeException("Existe una route con produces y no hay serializacion para alguno de los media types"); + RuntimeException e = new RuntimeException("Exists a route with a produces that cannot be handle"); logger.error("error", e); throw e; } if(!produces.isEmpty() && custom) { - RuntimeException e = new RuntimeException("Route con produces y custom"); + RuntimeException e = new RuntimeException("There is a route which is not serializable annotated with produce"); logger.error("error", e); throw e; } diff --git a/src/main/java/scmspain/karyon/restrouter/handlers/DefaultKaryonErrorHandler.java b/src/main/java/scmspain/karyon/restrouter/handlers/KaryonRestRouterErrorHandler.java similarity index 90% rename from src/main/java/scmspain/karyon/restrouter/handlers/DefaultKaryonErrorHandler.java rename to src/main/java/scmspain/karyon/restrouter/handlers/KaryonRestRouterErrorHandler.java index 129b76f..67cd1f2 100644 --- a/src/main/java/scmspain/karyon/restrouter/handlers/DefaultKaryonErrorHandler.java +++ b/src/main/java/scmspain/karyon/restrouter/handlers/KaryonRestRouterErrorHandler.java @@ -4,6 +4,8 @@ import io.netty.buffer.ByteBuf; import io.netty.handler.codec.http.HttpResponseStatus; import io.reactivex.netty.protocol.http.server.HttpServerRequest; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import rx.Observable; import scmspain.karyon.restrouter.exception.CannotSerializeException; import scmspain.karyon.restrouter.exception.InvalidAcceptHeaderException; @@ -15,7 +17,8 @@ * Default error handler, this is the error generated when the defined {@link ErrorHandler} cannot * handle the generated exception */ -public class DefaultKaryonErrorHandler implements ErrorHandler { +public class KaryonRestRouterErrorHandler implements ErrorHandler { + private static final Logger L = LoggerFactory.getLogger(KaryonRestRouterErrorHandler.class); @Override public Observable handleError( @@ -53,7 +56,7 @@ public Observable handleError( new RestRouterErrorDTO("Cannot serialize the response in the given accept: " + request.getHeaders().get(HttpHeaders.ACCEPT))); } else { - statusCode.set(HttpResponseStatus.INTERNAL_SERVER_ERROR); + return Observable.error(throwable); } } diff --git a/src/test/java/scmspain/karyon/restrouter/RestRouterHandlerTest.java b/src/test/java/scmspain/karyon/restrouter/RestRouterHandlerTest.java index d267275..3c9ba85 100644 --- a/src/test/java/scmspain/karyon/restrouter/RestRouterHandlerTest.java +++ b/src/test/java/scmspain/karyon/restrouter/RestRouterHandlerTest.java @@ -5,6 +5,7 @@ import com.google.common.net.HttpHeaders; import io.netty.buffer.ByteBuf; import io.netty.buffer.PooledByteBufAllocator; +import io.netty.handler.codec.http.HttpMethod; import io.netty.handler.codec.http.HttpResponseStatus; import io.reactivex.netty.protocol.http.server.HttpRequestHeaders; import io.reactivex.netty.protocol.http.server.HttpResponseHeaders; @@ -370,6 +371,8 @@ public void testWhenAcceptIsEmptyAndSupportedAreJsonAndXmlAndTestAndProducesCust given(routeHandler.process(request, response)) .willReturn(Observable.error(new RuntimeException("test"))); + given(request.getHttpMethod()) + .willReturn(HttpMethod.GET); // When RestRouterHandler restRouterHandler = new RestRouterHandler(restUriRouter, serializerManager, errorHandler); @@ -377,14 +380,12 @@ public void testWhenAcceptIsEmptyAndSupportedAreJsonAndXmlAndTestAndProducesCust responseBody.subscribe(subscriber); - List throwableList = subscriber.getOnErrorEvents(); - // Then + subscriber.assertNoErrors(); verify(routeHandler).process(request, response); verify(serializer, never()).serialize(any(), any()); verify(response.getHeaders(), never()).setHeader(any(), any()); - - assertThat(throwableList, hasItem(Matchers.isA(RuntimeException.class))); + verify(response).setStatus(HttpResponseStatus.INTERNAL_SERVER_ERROR); } @Test From 4023738a129982048ee982e55f420e4bec949fc5 Mon Sep 17 00:00:00 2001 From: Pablo Diaz Date: Mon, 16 Nov 2015 15:09:05 +0100 Subject: [PATCH 5/5] Added more info to error --- .../karyon/restrouter/RestRouterHandler.java | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/src/main/java/scmspain/karyon/restrouter/RestRouterHandler.java b/src/main/java/scmspain/karyon/restrouter/RestRouterHandler.java index e7010b4..b9180ca 100644 --- a/src/main/java/scmspain/karyon/restrouter/RestRouterHandler.java +++ b/src/main/java/scmspain/karyon/restrouter/RestRouterHandler.java @@ -15,8 +15,8 @@ import rx.Observable; import scmspain.karyon.restrouter.exception.CannotSerializeException; import scmspain.karyon.restrouter.exception.InvalidAcceptHeaderException; -import scmspain.karyon.restrouter.handlers.KaryonRestRouterErrorHandler; import scmspain.karyon.restrouter.handlers.ErrorHandler; +import scmspain.karyon.restrouter.handlers.KaryonRestRouterErrorHandler; import scmspain.karyon.restrouter.handlers.RestRouterErrorDTO; import scmspain.karyon.restrouter.serializer.SerializeManager; import scmspain.karyon.restrouter.serializer.SerializeWriter; @@ -26,6 +26,7 @@ import scmspain.karyon.restrouter.transport.http.RouteNotFound; import javax.inject.Inject; +import java.util.List; import java.util.Optional; import java.util.Set; import java.util.stream.Collectors; @@ -107,8 +108,11 @@ private void logError(Throwable throwable, HttpServerRequest request) { try { HttpMethod method = request.getHttpMethod(); String methodStr = method != null ? method.name() : ""; - String path = request.getPath(); - String message = String.format("Internal server error requesting [%s %s]", methodStr, path); + String path = request.getPath() + "?" + request.getQueryString(); + + String requestHeaders = getRequestHeaders(request); + + String message = String.format("Internal server error requesting [%s %s] [HEADERS=> %s]", methodStr, path, requestHeaders); L.error(message, throwable); } catch (Throwable t) { @@ -117,6 +121,12 @@ private void logError(Throwable throwable, HttpServerRequest request) { } } + private String getRequestHeaders(HttpServerRequest request) { + return request.getHeaders().entries().stream() + .map(entry -> entry.getKey() + ": " + entry.getValue()) + .reduce((entry1, entry2) -> entry1 + ", " + entry2) + .orElse(""); + } private Observable handleSupported(Route route, HttpServerRequest request,