From 5ca36848536f927d9b1dd48f38eec788942bdd13 Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Thu, 25 Jan 2024 14:32:47 +0100 Subject: [PATCH 1/2] [CSAPI]: Log more details on invalid requests Previously, just the exception message was logged. For some exceptions, this is quite non-descript, for example: 2024-01-25 06:17:14.061 DEBUG [SWAPool-1] - Invalid request (BAD_PAYLOAD): Invalid payload: Invalid XML: Received event END_DOCUMENT, instead of START_ELEMENT or END_ELEMENT. To solve this error, a backtrace is needed to figure out what was expected exactly. This particular exception message could probably be improved by itself, but this commit solves this more generically, since there might be other exceptions suffering from the same issue. This commit changes the handling of InvalidRequestExceptions to use the existing `logError()` function. This logs details of the request and a backtrace. To ensure that this does not log "Internal server error" but keep the existing "Invalid request", `logError()` is refactored to accept an error prefix. --- .../impl/service/consys/RestApiServlet.java | 20 ++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/sensorhub-service-consys/src/main/java/org/sensorhub/impl/service/consys/RestApiServlet.java b/sensorhub-service-consys/src/main/java/org/sensorhub/impl/service/consys/RestApiServlet.java index d2473d7a2..403ce9226 100644 --- a/sensorhub-service-consys/src/main/java/org/sensorhub/impl/service/consys/RestApiServlet.java +++ b/sensorhub-service-consys/src/main/java/org/sensorhub/impl/service/consys/RestApiServlet.java @@ -51,7 +51,7 @@ public abstract class RestApiServlet extends HttpServlet { static final String LOG_REQUEST_MSG = "{} {}{} (from ip={}, user={})"; static final String INTERNAL_ERROR_MSG = "Internal server error"; - static final String INTERNAL_ERROR_LOG_MSG = INTERNAL_ERROR_MSG + " while processing request " + LOG_REQUEST_MSG; + static final String ERROR_LOG_MSG = "{} while processing request " + LOG_REQUEST_MSG; static final String ACCESS_DENIED_ERROR_MSG = "Permission denied"; static final String JSON_CONTENT_TYPE = "application/json"; @@ -379,18 +379,24 @@ public Object createWebSocket(ServletUpgradeRequest req, ServletUpgradeResponse protected void logRequest(HttpServletRequest req) { if (log.isInfoEnabled()) - logRequestInfo(req, null); + logRequestInfo(req, null, null); } protected void logError(HttpServletRequest req, Throwable e) + { + logError(req, null, e); + } + + + protected void logError(HttpServletRequest req, String error_prefix, Throwable e) { if (log.isErrorEnabled()) - logRequestInfo(req, e); + logRequestInfo(req, error_prefix, e); } - protected void logRequestInfo(HttpServletRequest req, Throwable error) + protected void logRequestInfo(HttpServletRequest req, String error_prefix, Throwable error) { String method = req.getMethod(); String url = req.getRequestURI(); @@ -416,7 +422,7 @@ protected void logRequestInfo(HttpServletRequest req, Throwable error) query = "?" + req.getQueryString(); if (error != null) - log.error(INTERNAL_ERROR_LOG_MSG, method, url, query, ip, user, error); + log.error(ERROR_LOG_MSG, error_prefix, method, url, query, ip, user, error); else log.info(LOG_REQUEST_MSG, method, url, query, ip, user); } @@ -459,8 +465,8 @@ public void sendError(int code, String msg, HttpServletRequest req, HttpServletR protected void handleInvalidRequestException(HttpServletRequest req, HttpServletResponse resp, InvalidRequestException e) { - log.debug("Invalid request ({}): {}", e.getErrorCode(), e.getMessage()); - + logError(req, String.format("Invalid request (%s)", e.getErrorCode()), e); + switch (e.getErrorCode()) { case UNSUPPORTED_OPERATION: From 5a7084359e9b11fec1547ece08bb52548dc62f42 Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Thu, 25 Jan 2024 14:49:51 +0100 Subject: [PATCH 2/2] [CSAPI]: Preserve exception cause on invalid payload For some exceptions, the cause backtrace provides necessary details for debugging the actual cause of the issue, since the message is not always detailed enough. --- .../org/sensorhub/impl/service/consys/ServiceErrors.java | 8 +++++++- .../impl/service/consys/resource/BaseResourceHandler.java | 4 ++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/sensorhub-service-consys/src/main/java/org/sensorhub/impl/service/consys/ServiceErrors.java b/sensorhub-service-consys/src/main/java/org/sensorhub/impl/service/consys/ServiceErrors.java index 14b53ef87..3150bba63 100644 --- a/sensorhub-service-consys/src/main/java/org/sensorhub/impl/service/consys/ServiceErrors.java +++ b/sensorhub-service-consys/src/main/java/org/sensorhub/impl/service/consys/ServiceErrors.java @@ -42,7 +42,13 @@ public static InvalidRequestException badRequest(String msg) public static InvalidRequestException invalidPayload(String msg) { - return new InvalidRequestException(ErrorCode.BAD_PAYLOAD, msg); + return invalidPayload(msg, null); + } + + + public static InvalidRequestException invalidPayload(String msg, Throwable cause) + { + return new InvalidRequestException(ErrorCode.BAD_PAYLOAD, msg, cause); } diff --git a/sensorhub-service-consys/src/main/java/org/sensorhub/impl/service/consys/resource/BaseResourceHandler.java b/sensorhub-service-consys/src/main/java/org/sensorhub/impl/service/consys/resource/BaseResourceHandler.java index 93fa8048d..39e61c0aa 100644 --- a/sensorhub-service-consys/src/main/java/org/sensorhub/impl/service/consys/resource/BaseResourceHandler.java +++ b/sensorhub-service-consys/src/main/java/org/sensorhub/impl/service/consys/resource/BaseResourceHandler.java @@ -355,7 +355,7 @@ protected void create(final RequestContext ctx) throws IOException } catch (ResourceParseException e) { - throw ServiceErrors.invalidPayload("Invalid payload: " + e.getMessage()); + throw ServiceErrors.invalidPayload("Invalid payload: " + e.getMessage(), e); } } @@ -399,7 +399,7 @@ protected void update(final RequestContext ctx, final String id) throws IOExcept } catch (ResourceParseException e) { - throw ServiceErrors.invalidPayload("Invalid payload: " + e.getMessage()); + throw ServiceErrors.invalidPayload("Invalid payload: " + e.getMessage(), e); } // update in datastore