Improve error code status (#1552)

* Fix undefined versions * Improve code status * Bad request -> warn
yuzutech · May 23, 2023 · e81900a · e81900a
1 parent 80601b4
commit e81900a
Show file tree

Hide file tree

Showing 10 changed files with 56 additions and 29 deletions.
diff --git a/server/src/main/java/io/kroki/server/Server.java b/server/src/main/java/io/kroki/server/Server.java
@@ -167,7 +167,7 @@ static void start(Vertx vertx, JsonObject config, Handler<AsyncResult<HttpServer
     // Default route
     Route route = router.route("/*");
     route.handler(routingContext -> routingContext.fail(404));
-    ErrorHandler errorHandler = new ErrorHandler(vertx, false);
+    ErrorHandler errorHandler = new ErrorHandler(vertx, config.getBoolean("KROKI_DISPLAY_EXCEPTION_DETAILS", false));
     route.failureHandler(errorHandler);
 
     server

diff --git a/server/src/main/java/io/kroki/server/action/CommandStatusHandler.java b/server/src/main/java/io/kroki/server/action/CommandStatusHandler.java
@@ -1,11 +1,13 @@
 package io.kroki.server.action;
 
+import io.kroki.server.error.BadRequestException;
+
 public interface CommandStatusHandler {
 
   default byte[] handle(int exitValue, byte[] stdout, byte[] stderr) {
     if (exitValue != 0) {
       String errorMessage = new String(stdout) + new String(stderr);
-      throw new IllegalStateException(errorMessage + " (exit code " + exitValue + ")");
+      throw new BadRequestException(errorMessage + " (exit code " + exitValue + ")");
     }
     return stdout;
   }

diff --git a/server/src/main/java/io/kroki/server/error/ErrorHandler.java b/server/src/main/java/io/kroki/server/error/ErrorHandler.java
@@ -14,12 +14,14 @@
 import org.owasp.html.PolicyFactory;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
+import org.slf4j.event.Level;
 
 import javax.imageio.ImageIO;
 import java.awt.image.BufferedImage;
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
 import java.util.List;
+import java.util.Objects;
 
 public class ErrorHandler implements io.vertx.ext.web.handler.ErrorHandler {
 
@@ -52,13 +54,15 @@ public ErrorHandler(Vertx vertx, boolean displayExceptionDetails) {
 
   @Override
   public void handle(RoutingContext context) {
-    HttpServerResponse response = context.response();
     Throwable failure = context.failure();
     int errorCode = context.statusCode();
-    String errorMessage = response.getStatusMessage();
-    String statusMessage = null;
+    final String errorMessage;
+    final String statusMessage;
     String htmlErrorMessage = null;
-    if (failure instanceof BadRequestException) {
+    if (errorCode == 404) {
+      statusMessage = "Not Found";
+      errorMessage = statusMessage;
+    } else if (failure instanceof BadRequestException) {
       errorCode = 400;
       errorMessage = failure.getMessage();
       statusMessage = "Bad Request";
@@ -70,32 +74,31 @@ public void handle(RoutingContext context) {
       htmlErrorMessage = ((ServiceUnavailableException) failure).getMessageHTML();
     } else if (failure instanceof IllegalStateException) {
       errorCode = 500;
-      errorMessage = failure.getMessage();
-      if (errorMessage == null) {
-        errorMessage = "Internal Server Error";
-      }
+      errorMessage = Objects.requireNonNullElse(failure.getMessage(), "Internal Server Error");
+      statusMessage = "Internal Server Error";
     } else {
-      if (errorCode < 400 || errorCode > 500) {
+      if (errorCode < 400 || errorCode > 599) {
+        // unexpected error code!
+        logger.warn("Unexpected error code in ErrorHandler. Got: " + errorCode + ". Error code must be within 400 and 599, fallback to 500");
         errorCode = 500;
       }
       if (displayExceptionDetails) {
-        errorMessage = failure.getMessage();
-      }
-      if (errorMessage == null || errorMessage.trim().isEmpty()) {
+        errorMessage = Objects.requireNonNullElse(failure.getMessage(), "Internal Server Error");
+      } else {
         errorMessage = "Internal Server Error";
       }
-    }
-    if (statusMessage == null) {
-      statusMessage = errorMessage;
+      statusMessage = "Internal Server Error";
     }
     handleError(new ErrorContext(context.request(), context.response(), statusMessage, new ErrorInfo(context.failure(), errorCode, errorMessage, htmlErrorMessage)));
   }
 
   public void handleError(ErrorContext errorContext) {
     HttpServerResponse response = errorContext.getResponse();
     response.setStatusMessage(errorContext.getStatusMessage());
-    logging.error(errorContext.getRequest(), errorContext.getErrorInfo());
-    response.setStatusCode(errorContext.getErrorCode());
+    int errorCode = errorContext.getErrorCode();
+    Level level = (errorCode >= 400 && errorCode <= 499) ? Level.WARN : Level.ERROR;
+    logging.log(level, errorContext.getRequest(), errorContext.getErrorInfo());
+    response.setStatusCode(errorCode);
     ErrorInfo errorInfo = errorContext.getErrorInfo();
     if (!sendErrorResponseMIME(response, errorInfo) && !sendErrorAcceptMIME(response, errorContext.getAcceptableMimes(), errorInfo)) {
       // fallback plain/text
@@ -128,7 +131,9 @@ private boolean sendError(HttpServerResponse response, String mime, ErrorInfo er
       StringBuilder stack = new StringBuilder();
       if (failure != null && displayExceptionDetails) {
         for (StackTraceElement elem : failure.getStackTrace()) {
-          stack.append(htmlSanitizer.sanitize("<li>" + elem.toString() + "</li>"));
+          stack.append("<li>");
+          stack.append(htmlSanitizer.sanitize( elem.toString()));
+          stack.append("</li>");
         }
       }
       response.putHeader(HttpHeaders.CONTENT_TYPE, "text/html");

diff --git a/server/src/main/java/io/kroki/server/log/Logging.java b/server/src/main/java/io/kroki/server/log/Logging.java
@@ -9,6 +9,7 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.slf4j.MDC;
+import org.slf4j.event.Level;
 
 public class Logging {
 
@@ -95,9 +96,10 @@ public void reroute(String from, String to, String source, FileFormat fileFormat
     }
   }
 
-  public void error(HttpServerRequest request, ErrorInfo errorInfo) {
+  public void log(Level level, HttpServerRequest request, ErrorInfo errorInfo) {
+    boolean error = level.toInt() >= Level.ERROR.toInt();
     try {
-      MDC.put("action", "error");
+      MDC.put("action", error ? "error" : "warning");
       MDC.put("method", request.method().toString());
       MDC.put("path", request.path());
       MDC.put("error_code", String.valueOf(errorInfo.getCode()));
@@ -113,9 +115,17 @@ public void error(HttpServerRequest request, ErrorInfo errorInfo) {
       Throwable failure = errorInfo.getFailure();
       if (failure != null) {
         MDC.put("failure_class_name", failure.getClass().getName());
-        logger.error("An error occurred", failure);
+        if (error) {
+          logger.error("Server error", failure);
+        } else {
+          logger.warn("Bad request");
+        }
       } else {
-        logger.error("An error occurred");
+        if (error) {
+          logger.error("Server error");
+        } else {
+          logger.warn("Bad request");
+        }
       }
     } finally {
       MDC.remove("action");

diff --git a/server/src/main/java/io/kroki/server/service/D2.java b/server/src/main/java/io/kroki/server/service/D2.java
@@ -71,7 +71,7 @@ public SourceDecoder getSourceDecoder() {
 
   @Override
   public String getVersion() {
-    return "undefined";
+    return "0.4.1";
   }
 
   @Override

diff --git a/server/src/main/java/io/kroki/server/service/Excalidraw.java b/server/src/main/java/io/kroki/server/service/Excalidraw.java
@@ -47,7 +47,7 @@ public SourceDecoder getSourceDecoder() {
 
   @Override
   public String getVersion() {
-    return "undefined";
+    return "0.15.2";
   }
 
   @Override

diff --git a/server/src/main/java/io/kroki/server/service/Wireviz.java b/server/src/main/java/io/kroki/server/service/Wireviz.java
@@ -49,7 +49,7 @@ public SourceDecoder getSourceDecoder() {
 
   @Override
   public String getVersion() {
-    return "undefined";
+    return "0.3.2";
   }
 
   @Override

diff --git a/server/src/main/resources/web/error.html b/server/src/main/resources/web/error.html
@@ -14,7 +14,7 @@ <h1 class="subtitle is-spaced">Convert <strong>plain text</strong> diagrams to <
     <h2 class="title">{title}</h2>
     <article class="message is-danger">
       <div class="message-body">
-        <strong><code>{errorCode}</code> {errorMessage}</strong>
+        <strong id="error"><code>{errorCode}</code> {errorMessage}</strong>
         <ul id="stacktrace">{stackTrace}</ul>
       </div>
     </article>

diff --git a/server/src/main/resources/web/root/css/main.css b/server/src/main/resources/web/root/css/main.css
@@ -75,6 +75,15 @@ ul {
   list-style: none;
 }
 
+#error {
+  vertical-align: middle;
+}
+
+#stacktrace {
+  padding-left: 4em;
+  font-size: 0.95em;
+}
+
 button,
 input,
 select,

diff --git a/server/src/test/java/io/kroki/server/action/CommanderTest.java b/server/src/test/java/io/kroki/server/action/CommanderTest.java
@@ -1,5 +1,6 @@
 package io.kroki.server.action;
 
+import io.kroki.server.error.BadRequestException;
 import io.kroki.server.unit.TimeValue;
 import io.vertx.core.json.JsonObject;
 import org.junit.jupiter.api.Test;
@@ -56,7 +57,7 @@ void should_not_fail(String source) throws IOException, InterruptedException {
         byte[] result = new Commander(new JsonObject()).execute(source.getBytes(), "dot");
         assertThat(Files.exists(Paths.get("/tmp/blns.fail"))).isFalse();
         assertThat(new String(result)).isEqualTo("");
-      } catch (IllegalStateException e) {
+      } catch (BadRequestException e) {
         assertThat(e).hasMessageStartingWith("Error: <stdin>: syntax error in line");
         assertThat(e).hasMessageEndingWith(" (exit code 1)");
       }