Extracted compacting from JsonHttpLogFormatter

Fixes #361

logbook-core/pom.xml

@@ -22,7 +22,7 @@
         <dependency>
             <groupId>com.fasterxml.jackson.core</groupId>
             <artifactId>jackson-databind</artifactId>
-            <optional>true</optional>
+            <!-- TODO should be optional -->
         </dependency>
         <!-- testing -->
         <dependency>

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

@@ -23,7 +23,7 @@ private BodyFilters() {
 
     @API(status = MAINTAINED)
     public static BodyFilter defaultValue() {
-        return accessToken();
+        return BodyFilter.merge(accessToken(), compactJson(new ObjectMapper()));
     }
 
     @API(status = MAINTAINED)

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

@@ -7,33 +7,34 @@
 import java.io.IOException;
 import java.util.function.Predicate;
 
-
 @Slf4j
-class JsonCompactingBodyFilter implements BodyFilter {
+final class JsonCompactingBodyFilter implements BodyFilter {
+
+    private static final Predicate<String> JSON = MediaTypeQuery.compile("application/json", "application/*+json");
 
-    private final JsonCompactor jsonCompactor;
     private final JsonHeuristic heuristic = new JsonHeuristic();
-    private final Predicate<String> contentTypes = MediaTypeQuery.compile("application/json", "application/*+json");
+    private final JsonCompactor compactor;
 
     JsonCompactingBodyFilter(final ObjectMapper objectMapper) {
-        jsonCompactor = new JsonCompactor(objectMapper);
+        this.compactor = new JsonCompactor(objectMapper);
     }
 
     @Override
     public String filter(@Nullable final String contentType, final String body) {
-        return contentTypes.test(contentType) && shouldCompact(body) ? compact(body) : body;
+        return JSON.test(contentType) && isCompactable(body) ? compact(body) : body;
     }
 
-    private boolean shouldCompact(final String body) {
-        return heuristic.isProbablyJson(body) && !jsonCompactor.isCompacted(body);
+    private boolean isCompactable(final String body) {
+        return heuristic.isProbablyJson(body) && !compactor.isCompacted(body);
     }
 
     private String compact(final String body) {
         try {
-            return jsonCompactor.compact(body);
+            return compactor.compact(body);
         } catch (final IOException e) {
             log.trace("Unable to compact body, is it a JSON?. Keep it as-is: `{}`", e.getMessage());
             return body;
         }
     }
+
 }

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

@@ -13,6 +13,7 @@ boolean isProbablyJson(final String body) {
         // https://tools.ietf.org/html/rfc4627#section-2
         final String trimmed = body.trim();
 
+        // ordered by probability of occurrence in real world applications
         return isProbablyObject(trimmed)
                 || isProbablyArray(trimmed)
                 || isProbablyString(body)

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

@@ -5,8 +5,6 @@
 import com.fasterxml.jackson.databind.ObjectMapper;
 import lombok.AllArgsConstructor;
 import org.apiguardian.api.API;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
 
 import java.io.IOException;
 import java.util.Map;
@@ -47,11 +45,9 @@
 @API(status = STABLE)
 public final class JsonHttpLogFormatter implements PreparedHttpLogFormatter {
 
-    private static final Logger LOG = LoggerFactory.getLogger(JsonHttpLogFormatter.class);
     private static final Predicate<String> JSON = MediaTypeQuery.compile("application/json", "application/*+json");
 
     private final ObjectMapper mapper;
-    private final JsonCompactor compactor;
     private final JsonHeuristic heuristic = new JsonHeuristic();
 
     public JsonHttpLogFormatter() {
@@ -60,7 +56,6 @@ public JsonHttpLogFormatter() {
 
     public JsonHttpLogFormatter(final ObjectMapper mapper) {
         this.mapper = mapper;
-        this.compactor = new JsonCompactor(mapper);
     }
 
 
@@ -72,7 +67,7 @@ public String format(final Map<String, Object> content) throws IOException {
     @Override
     public void addBody(final HttpMessage message, final Map<String, Object> content) throws IOException {
         if (isContentTypeJson(message)) {
-            content.put("body", tryParseBodyAsJson(message.getBodyAsString()));
+            content.put("body", treatPossibleJsonAsJson(message.getBodyAsString()));
         } else {
             PreparedHttpLogFormatter.super.addBody(message, content);
         }
@@ -82,21 +77,9 @@ private boolean isContentTypeJson(final HttpMessage message) {
         return JSON.test(message.getContentType());
     }
 
-    private Object tryParseBodyAsJson(final String body) {
+    private Object treatPossibleJsonAsJson(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);
-            }
-
-            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;
-            }
+            return new JsonBody(body);
         } else {
             return body;
         }

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

@@ -24,15 +24,16 @@
 import static org.zalando.fauxpas.FauxPas.throwingSupplier;
 
 @Slf4j
-class XmlCompactingBodyFilter implements BodyFilter {
+final class XmlCompactingBodyFilter implements BodyFilter {
 
-    private final Predicate<String> contentTypes = MediaTypeQuery.compile("*/xml", "*/*+xml");
-    private final DocumentBuilderFactory documentBuilderFactory = documentBuilderFactory();
+    private static final Predicate<String> XML = MediaTypeQuery.compile("*/xml", "*/*+xml");
+
+    private final DocumentBuilderFactory factory = documentBuilderFactory();
     private final Transformer transformer = transformer();
 
     @Override
     public String filter(@Nullable final String contentType, final String body) {
-        return contentTypes.test(contentType) && shouldCompact(body) ? compact(body) : body;
+        return XML.test(contentType) && shouldCompact(body) ? compact(body) : body;
     }
 
     private boolean shouldCompact(final String body) {
@@ -52,7 +53,7 @@ private String compact(final String body) {
     }
 
     private Document parseDocument(final String body) throws Exception {
-        final DocumentBuilder documentBuilder = documentBuilderFactory.newDocumentBuilder();
+        final DocumentBuilder documentBuilder = factory.newDocumentBuilder();
         final Document document = documentBuilder.parse(new ByteArrayInputStream(body.getBytes()));
         removeEmptyTextNodes(document);
         return document;
@@ -96,4 +97,5 @@ private DocumentBuilderFactory documentBuilderFactory() {
             return factory;
         }).get();
     }
+
 }

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

@@ -1,66 +1,61 @@
 package org.zalando.logbook;
 
 import com.fasterxml.jackson.databind.ObjectMapper;
-import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.is;
 
 class JsonCompactingBodyFilterTest {
 
-    private JsonCompactingBodyFilter bodyFilter;
+    private final BodyFilter unit = new JsonCompactingBodyFilter(new ObjectMapper());
 
     /*language=JSON*/
-    private final String prettifiedJson = "{\n" +
+    private final String pretty = "{\n" +
             "  \"root\": {\n" +
             "    \"child\": \"text\"\n" +
             "  }\n" +
             "}";
 
     /*language=JSON*/
-    private final String minimisedJson = "{\"root\":{\"child\":\"text\"}}";
-
-    @BeforeEach
-    void setUp() {
-        bodyFilter = new JsonCompactingBodyFilter(new ObjectMapper());
-    }
+    private final String compacted = "{\"root\":{\"child\":\"text\"}}";
 
     @Test
     void shouldIgnoreEmptyBody() {
-        final String filtered = bodyFilter.filter("application/json", "");
+        final String filtered = unit.filter("application/json", "");
         assertThat(filtered, is(""));
     }
 
     @Test
     void shouldIgnoreInvalidContent() {
         final String invalidBody = "{\ninvalid}";
-        final String filtered = bodyFilter.filter("application/json", invalidBody);
+        final String filtered = unit.filter("application/json", invalidBody);
         assertThat(filtered, is(invalidBody));
     }
 
     @Test
     void shouldIgnoreInvalidContentType() {
-        final String filtered = bodyFilter.filter("text/plain", prettifiedJson);
-        assertThat(filtered, is(prettifiedJson));
+        final String filtered = unit.filter("text/plain", pretty);
+        assertThat(filtered, is(pretty));
     }
 
     @Test
     void shouldTransformValidJsonRequestWithSimpleContentType() {
-        final String filtered = bodyFilter.filter("application/json", prettifiedJson);
-        assertThat(filtered, is(minimisedJson));
+        final String filtered = unit.filter("application/json", pretty);
+        assertThat(filtered, is(compacted));
     }
 
     @Test
     void shouldTransformValidJsonRequestWithCompatibleContentType() {
-        final String filtered = bodyFilter.filter("application/custom+json", prettifiedJson);
-        assertThat(filtered, is(minimisedJson));
+        final String filtered = unit.filter("application/custom+json", pretty);
+        assertThat(filtered, is(compacted));
     }
 
     @Test
     void shouldSkipInvalidJsonLookingLikeAValidOne() {
         final String invalidJson = "{invalid}";
-        final String filtered = bodyFilter.filter("application/custom+json", invalidJson);
+        final String filtered = unit.filter("application/custom+json", invalidJson);
         assertThat(filtered, is(invalidJson));
     }
-}
+
+}