Ensure always valid json from JsonMetricsRenderingException

4 files changed, 75 insertions, 18 deletions

diff --git a/metrics-proxy/src/main/java/ai/vespa/metricsproxy/http/GenericMetricsHandler.java b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/http/GenericMetricsHandler.java
index 488f26b7af7..cadfc053b94 100644
--- a/metrics-proxy/src/main/java/ai/vespa/metricsproxy/http/GenericMetricsHandler.java
+++ b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/http/GenericMetricsHandler.java
@@ -6,14 +6,12 @@ package ai.vespa.metricsproxy.http;
 
 import ai.vespa.metricsproxy.core.MetricsManager;
 import ai.vespa.metricsproxy.metric.model.MetricsPacket;
-import ai.vespa.metricsproxy.metric.model.json.GenericJsonModel;
-import ai.vespa.metricsproxy.metric.model.json.GenericJsonUtil;
+import ai.vespa.metricsproxy.metric.model.json.JsonRenderingException;
 import ai.vespa.metricsproxy.service.VespaServices;
 import com.google.inject.Inject;
 import com.yahoo.container.jdisc.HttpRequest;
 import com.yahoo.container.jdisc.HttpResponse;
 import com.yahoo.container.jdisc.ThreadedHttpRequestHandler;
-import com.yahoo.yolean.Exceptions;
 
 import java.io.IOException;
 import java.io.OutputStream;
@@ -22,6 +20,8 @@ import java.time.Instant;
 import java.util.List;
 import java.util.concurrent.Executor;
 
+import static ai.vespa.metricsproxy.metric.model.json.GenericJsonUtil.toGenericJsonModel;
+
 /**
  * Handler exposing the generic metrics format via http.
  *
@@ -43,9 +43,9 @@ public class GenericMetricsHandler extends ThreadedHttpRequestHandler {
     public HttpResponse handle(HttpRequest request) {
         try {
             List<MetricsPacket> metrics = metricsManager.getMetrics(vespaServices.getVespaServices(), Instant.now());
-            return new Response(200, GenericJsonUtil.toGenericJsonModel(metrics).serialize());
-        } catch (GenericJsonModel.JsonMetricsRenderingException e) {
-            return new ErrorResponse(500, Exceptions.toMessageString(e));
+            return new Response(200, toGenericJsonModel(metrics).serialize());
+        } catch (JsonRenderingException e) {
+            return new Response(500, e.getMessageAsJson());
         }
     }
 
@@ -68,10 +68,4 @@ public class GenericMetricsHandler extends ThreadedHttpRequestHandler {
         }
     }
 
-    private static class ErrorResponse extends Response {
-        ErrorResponse(int code, String data) {
-            super(code, "{\"error\":\"" + data + "\"}");
-        }
-    }
-
 }
diff --git a/metrics-proxy/src/main/java/ai/vespa/metricsproxy/metric/model/json/GenericJsonModel.java b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/metric/model/json/GenericJsonModel.java
index 14f085ff388..03e353cab22 100644
--- a/metrics-proxy/src/main/java/ai/vespa/metricsproxy/metric/model/json/GenericJsonModel.java
+++ b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/metric/model/json/GenericJsonModel.java
@@ -38,13 +38,8 @@ public class GenericJsonModel {
             return mapper.writeValueAsString(this);
         } catch (IOException e) {
             log.log(Level.WARNING, "Got exception when rendering metrics:", e);
-            throw new JsonMetricsRenderingException("Could not render metrics. Check the log for details.");
+            throw new JsonRenderingException("Could not render metrics. Check the log for details.");
         }
     }
 
-    public static class JsonMetricsRenderingException extends RuntimeException {
-        JsonMetricsRenderingException(String message) {
-            super(message);
-        }
-    }
 }
diff --git a/metrics-proxy/src/main/java/ai/vespa/metricsproxy/metric/model/json/JsonRenderingException.java b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/metric/model/json/JsonRenderingException.java
new file mode 100644
index 00000000000..05df3bb12c9
--- /dev/null
+++ b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/metric/model/json/JsonRenderingException.java
@@ -0,0 +1,41 @@
+/*
+ * Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+ */
+
+package ai.vespa.metricsproxy.metric.model.json;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+
+import java.io.IOException;
+import java.util.Map;
+import java.util.logging.Logger;
+
+import static java.util.logging.Level.WARNING;
+
+/**
+ * An exception whose message is always valid json or, if all fails, a plain text string.
+ *
+ * @author gjoranv
+ */
+public class JsonRenderingException extends RuntimeException {
+
+    private static Logger log = Logger.getLogger(JsonRenderingException.class.getName());
+
+    JsonRenderingException(String message) {
+        super(message);
+    }
+
+    public String getMessageAsJson() {
+        return wrap(getMessage());
+    }
+
+    private static String wrap(String message) {
+        try {
+            return new ObjectMapper().writeValueAsString(Map.of("error", message));
+        } catch (IOException e) {
+            log.log(WARNING, "Could not encode error message to json:", e);
+            return "Could not encode error message to json, check the log for details.";
+        }
+    }
+
+}
diff --git a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/metric/model/json/JsonRenderingExceptionTest.java b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/metric/model/json/JsonRenderingExceptionTest.java
new file mode 100644
index 00000000000..9b502d3f67a
--- /dev/null
+++ b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/metric/model/json/JsonRenderingExceptionTest.java
@@ -0,0 +1,27 @@
+/*
+ * Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+ */
+
+package ai.vespa.metricsproxy.metric.model.json;
+
+import org.junit.Test;
+
+import static org.junit.Assert.assertEquals;
+
+/**
+ * @author gjoranv
+ */
+public class JsonRenderingExceptionTest {
+
+    @Test
+    public void error_message_is_wrapped_in_json_object() {
+        var exception =  new JsonRenderingException("bad");
+        assertEquals("{\"error\":\"bad\"}", exception.getMessageAsJson());
+    }
+
+    @Test
+    public void quotes_are_escaped() {
+        var exception =  new JsonRenderingException("Message \" with \" embedded quotes.");
+        assertEquals("{\"error\":\"Message \\\" with \\\" embedded quotes.\"}", exception.getMessageAsJson());
+    }
+}