Added JSON heuristic

logbook-core/src/main/java/org/zalando/logbook/JsonCompactor.java

@@ -0,0 +1,42 @@
+package org.zalando.logbook;
+
+import com.fasterxml.jackson.core.JsonFactory;
+import com.fasterxml.jackson.core.JsonGenerator;
+import com.fasterxml.jackson.core.JsonParser;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import lombok.AllArgsConstructor;
+
+import java.io.IOException;
+import java.io.StringWriter;
+
+@AllArgsConstructor
+final class JsonCompactor {
+
+    private final ObjectMapper mapper;
+
+    // this wouldn't catch spaces in json, but that's ok for our use case here
+    boolean isCompacted(final String json) {
+        return json.indexOf('\n') == -1;
+    }
+
+    String compact(final String json) throws IOException {
+        final StringWriter output = new StringWriter(json.length());
+        final JsonFactory factory = mapper.getFactory();
+        final JsonParser parser = factory.createParser(json);
+
+        final JsonGenerator generator = factory.createGenerator(output);
+
+        // https://github.com/jacoco/jacoco/wiki/FilteringOptions
+        //noinspection TryFinallyCanBeTryWithResources - jacoco can't handle try-with correctly
+        try {
+            while (parser.nextToken() != null) {
+                generator.copyCurrentEvent(parser);
+            }
+        } finally {
+            generator.close();
+        }
+
+        return output.toString();
+    }
+
+}

logbook-core/src/main/java/org/zalando/logbook/JsonHeuristic.java

@@ -0,0 +1,44 @@
+package org.zalando.logbook;
+
+import java.util.regex.Pattern;
+
+import static java.util.regex.Pattern.compile;
+
+final class JsonHeuristic {
+
+    private final Pattern number = compile("^-?(0|[1-9][0-9]*)(\\.[0-9]+)?([eE][+-]?[0-9]+)?$");
+
+    boolean isProbablyJson(final String body) {
+        return isNull(body)
+                || isBoolean(body)
+                || isNumber(body)
+                || isProbablyString(body)
+                || isProbablyArray(body)
+                || isProbablyObject(body);
+    }
+
+    private boolean isNull(final String body) {
+        return "null".equals(body);
+    }
+
+    private boolean isBoolean(final String body) {
+        return "true".equals(body) || "false".equals(body);
+    }
+
+    private boolean isNumber(final String body) {
+        return number.matcher(body).matches();
+    }
+
+    private boolean isProbablyString(final String body) {
+        return body.startsWith("\"") && body.endsWith("\"") && body.length() > 1;
+    }
+
+    private boolean isProbablyArray(final String body) {
+        return body.startsWith("[") && body.endsWith("]");
+    }
+
+    private boolean isProbablyObject(final String body) {
+        return body.startsWith("{") && body.endsWith("}");
+    }
+
+}

logbook-core/src/main/java/org/zalando/logbook/JsonHttpLogFormatter.java

@@ -2,17 +2,13 @@
 
 import com.fasterxml.jackson.annotation.JsonRawValue;
 import com.fasterxml.jackson.annotation.JsonValue;
-import com.fasterxml.jackson.core.JsonFactory;
-import com.fasterxml.jackson.core.JsonGenerator;
-import com.fasterxml.jackson.core.JsonParser;
 import com.fasterxml.jackson.databind.ObjectMapper;
+import lombok.AllArgsConstructor;
 import org.apiguardian.api.API;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-import javax.annotation.Nullable;
 import java.io.IOException;
-import java.io.StringWriter;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
@@ -58,13 +54,16 @@ public final class JsonHttpLogFormatter implements HttpLogFormatter {
     private static final Predicate<String> JSON = MediaTypeQuery.compile("application/json", "application/*+json");
 
     private final ObjectMapper mapper;
+    private final JsonHeuristic heuristic = new JsonHeuristic();
+    private final JsonCompactor compactor;
 
     public JsonHttpLogFormatter() {
         this(new ObjectMapper());
     }
 
     public JsonHttpLogFormatter(final ObjectMapper mapper) {
         this.mapper = mapper;
+        this.compactor = new JsonCompactor(mapper);
     }
 
     @Override
@@ -148,75 +147,46 @@ private static <T> void addUnless(final Map<String, Object> target, final String
     private void addBody(final HttpMessage message, final Map<String, Object> map) throws IOException {
         final String body = message.getBodyAsString();
 
-        if (isJson(message.getContentType())) {
+        if (isContentTypeJson(message)) {
             map.put("body", tryParseBodyAsJson(body));
         } else {
             addUnless(map, "body", body, String::isEmpty);
         }
     }
 
-    private Object tryParseBodyAsJson(final String body) {
-        if (body.isEmpty()) {
-            return JsonBody.EMPTY;
-        }
-
-        try {
-            return new JsonBody(compactJson(body));
-        } catch (final IOException e) {
-            LOG.trace("Unable to parse body as JSON; embedding it as-is: [{}]", e.getMessage());
-            return body;
-        }
-    }
-
-    private boolean isJson(@Nullable final String type) {
-        return JSON.test(type);
+    private boolean isContentTypeJson(final HttpMessage message) {
+        return JSON.test(message.getContentType());
     }
 
-    private String compactJson(final String json) throws IOException {
-        if (isAlreadyCompacted(json)) {
-            return json;
-        }
-
-        final StringWriter output = new StringWriter();
-        final JsonFactory factory = mapper.getFactory();
-        final JsonParser parser = factory.createParser(json);
-
-        final JsonGenerator generator = factory.createGenerator(output);
+    private Object tryParseBodyAsJson(final String body) {
+        if (heuristic.isProbablyJson(body)) {
+            if (compactor.isCompacted(body)) {
+                // any body that looks like JSON (according to our heuristic) and has no newlines would be
+                // incorrectly treated as JSON here which results in an invalid JSON output
+                // see https://github.com/zalando/logbook/issues/279
+                return new JsonBody(body);
+            }
 
-        // https://github.com/jacoco/jacoco/wiki/FilteringOptions
-        //noinspection TryFinallyCanBeTryWithResources - jacoco can't handle try-with correctly
-        try {
-            while (parser.nextToken() != null) {
-                generator.copyCurrentEvent(parser);
+            try {
+                return new JsonBody(compactor.compact(body));
+            } catch (final IOException e) {
+                LOG.trace("Unable to compact body, probably because it's not JSON. Embedding it as-is: [{}]", e.getMessage());
+                return body;
             }
-        } finally {
-            generator.close();
+        } else {
+            return body;
         }
-
-        return output.toString();
-    }
-
-    // this wouldn't catch spaces in json, but that's ok for our use case here
-    private boolean isAlreadyCompacted(final String json) {
-        return json.indexOf('\n') == -1;
     }
 
+    @AllArgsConstructor
     private static final class JsonBody {
-
-        static final String EMPTY = "";
-
-        private final String json;
-
-        private JsonBody(final String json) {
-            this.json = json;
-        }
+        String json;
 
         @JsonRawValue
         @JsonValue
         public String getJson() {
             return json;
         }
-
     }
 
     /**

logbook-core/src/test/java/org/zalando/logbook/JsonHeuristicTest.java

@@ -0,0 +1,54 @@
+package org.zalando.logbook;
+
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+class JsonHeuristicTest {
+
+    private final JsonHeuristic unit = new JsonHeuristic();
+
+    @ParameterizedTest
+    @ValueSource(strings = {
+            "",
+            " ",
+            "\t",
+            "\n",
+            "void",
+            "True",
+            "<skipped>",
+            "\"",
+            "\"missing end quote",
+            "123.4.5",
+            "{},",
+            "[],"
+    })
+    void notJson(final String value) {
+        assertFalse(unit.isProbablyJson(value));
+    }
+
+    @ParameterizedTest
+    @ValueSource(strings = {
+            "null",
+            "true",
+            "false",
+            "\"string\"",
+            "\"technically\"not a valid string\"", // acceptable false positive
+            "123",
+            "123.45",
+            "{}",
+            "{\"key\",\"value\"}",
+            "{key:value}", // acceptable false positive
+            "{\"key\",{}", // acceptable false positive
+            "[]",
+            "[\"value\"]",
+            "[]]", // acceptable false positive
+            "[value]", // acceptable false positive
+    })
+    void probablyJson(final String value) {
+        assertTrue(unit.isProbablyJson(value));
+    }
+
+}