Fix a bug in conversion to metrics packets

- NPE was thrown when there is no 'node' json.
- Add testing for this case.

3 files changed, 70 insertions, 9 deletions

diff --git a/metrics-proxy/src/main/java/ai/vespa/metricsproxy/metric/model/json/GenericJsonUtil.java b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/metric/model/json/GenericJsonUtil.java
index 948fb73cb16..c20ded30361 100644
--- a/metrics-proxy/src/main/java/ai/vespa/metricsproxy/metric/model/json/GenericJsonUtil.java
+++ b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/metric/model/json/GenericJsonUtil.java
@@ -106,6 +106,8 @@ public class GenericJsonUtil {
 
     private static List<MetricsPacket.Builder> toNodePackets(GenericNode node) {
         List<MetricsPacket.Builder> packets = new ArrayList<>();
+        if (node == null) return packets;
+
         if (node.metrics == null || node.metrics.isEmpty()) {
             return singletonList(new MetricsPacket.Builder(VESPA_NODE_SERVICE_ID)
                                          .statusCode(StatusCode.UP.ordinal())
diff --git a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/metric/model/json/GenericJsonModelTest.java b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/metric/model/json/GenericJsonModelTest.java
index ad11a82e547..f56f3531982 100644
--- a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/metric/model/json/GenericJsonModelTest.java
+++ b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/metric/model/json/GenericJsonModelTest.java
@@ -27,22 +27,31 @@ import static org.junit.Assert.assertNotNull;
  */
 public class GenericJsonModelTest {
     private static final String TEST_FILE = "generic-sample.json";
+    private static final String TEST_FILE_WITHOUT_NODE = "generic-without-node.json";
 
     @Test
     public void deserialize_serialize_roundtrip() throws IOException {
-        GenericJsonModel jsonModel = genericJsonModelFromTestFile();
+        GenericJsonModel jsonModel = genericJsonModelFromTestFile(TEST_FILE);
 
         // Do some sanity checking
         assertEquals(2, jsonModel.services.size());
         assertEquals(2, jsonModel.node.metrics.size());
         assertEquals(16.222, jsonModel.node.metrics.get(0).values.get("cpu.util"), 0.01d);
 
-        assertThatSerializedModelEqualsTestFile(jsonModel);
+        assertThatSerializedModelEqualsTestFile(jsonModel, TEST_FILE);
+    }
+
+    @Test
+    public void deserialize_serialize_roundtrip_without_node_json() throws IOException {
+        GenericJsonModel jsonModel = genericJsonModelFromTestFile(TEST_FILE_WITHOUT_NODE);
+        assertEquals(2, jsonModel.services.size());
+
+        assertThatSerializedModelEqualsTestFile(jsonModel, TEST_FILE_WITHOUT_NODE);
     }
 
     @Test
     public void deserialize_serialize_roundtrip_with_metrics_packets() throws IOException {
-        GenericJsonModel modelFromFile = genericJsonModelFromTestFile();
+        GenericJsonModel modelFromFile = genericJsonModelFromTestFile(TEST_FILE);
         List<MetricsPacket> metricsPackets = GenericJsonUtil.toMetricsPackets(modelFromFile).stream()
                 .map(MetricsPacket.Builder::build)
                 .collect(toList());
@@ -56,7 +65,20 @@ public class GenericJsonModelTest {
         assertEquals(2, modelFromFile.node.metrics.size());
         assertEquals(16.222, modelFromFile.node.metrics.get(0).values.get("cpu.util"), 0.01d);
 
-        assertThatSerializedModelEqualsTestFile(modelFromPackets);
+        assertThatSerializedModelEqualsTestFile(modelFromPackets, TEST_FILE);
+    }
+
+    @Test
+    public void deserialize_serialize_roundtrip_without_node_json_with_metrics_packets() throws IOException {
+        GenericJsonModel modelFromFile = genericJsonModelFromTestFile(TEST_FILE_WITHOUT_NODE);
+        List<MetricsPacket> metricsPackets = GenericJsonUtil.toMetricsPackets(modelFromFile).stream()
+                .map(MetricsPacket.Builder::build)
+                .collect(toList());
+
+        assertEquals(2, metricsPackets.size());
+
+        GenericJsonModel modelFromPackets = GenericJsonUtil.toGenericJsonModel(metricsPackets);
+        assertThatSerializedModelEqualsTestFile(modelFromPackets, TEST_FILE_WITHOUT_NODE);
     }
 
     @Test
@@ -108,20 +130,20 @@ public class GenericJsonModelTest {
         assertEquals(4, metricsPackets.size());
         GenericJsonModel modelFromPackets = GenericJsonUtil.toGenericJsonModel(metricsPackets);
 
-        assertThatSerializedModelEqualsTestFile(modelFromPackets);
+        assertThatSerializedModelEqualsTestFile(modelFromPackets, TEST_FILE);
     }
 
-    private void assertThatSerializedModelEqualsTestFile(GenericJsonModel modelFromPackets) {
+    private void assertThatSerializedModelEqualsTestFile(GenericJsonModel modelFromPackets, String testFile) {
         String serialized = modelFromPackets.serialize();
         String trimmed = serialized.trim().replaceAll("\\s+", "");
 
-        String expected = getFileContents(TEST_FILE).trim().replaceAll("\\s+", "");
+        String expected = getFileContents(testFile).trim().replaceAll("\\s+", "");
         assertEquals(expected, trimmed);
     }
 
-    private GenericJsonModel genericJsonModelFromTestFile() throws IOException {
+    private GenericJsonModel genericJsonModelFromTestFile(String filename) throws IOException {
         ObjectMapper mapper = createObjectMapper();
-        return mapper.readValue(getFileContents(TEST_FILE), GenericJsonModel.class);
+        return mapper.readValue(getFileContents(filename), GenericJsonModel.class);
     }
 
 }
diff --git a/metrics-proxy/src/test/resources/generic-without-node.json b/metrics-proxy/src/test/resources/generic-without-node.json
new file mode 100644
index 00000000000..f4914a86129
--- /dev/null
+++ b/metrics-proxy/src/test/resources/generic-without-node.json
@@ -0,0 +1,37 @@
+{
+  "services": [
+    {
+      "name": "searchnode",
+      "timestamp": 1234,
+      "status": {
+        "code": "up"
+      },
+      "metrics": [
+        {
+          "values": {
+            "queries.count": 4
+          },
+          "dimensions": {
+            "documentType": "music"
+          }
+        }
+      ]
+    },
+    {
+      "name": "slobrok",
+      "timestamp": 1234,
+      "status": {
+        "code": "unknown",
+        "description": "Unable to fetch metrics from service 'slobrok'"
+      },
+      "metrics": [
+        {
+          "values": {},
+          "dimensions": {
+            "instance": "slobrok0"
+          }
+        }
+      ]
+    }
+  ]
+}
\ No newline at end of file