FasterXML / jackson-databind

General data-binding package for Jackson (2.x): works on streaming API (core) implementation(s)
Apache License 2.0
3.52k stars 1.38k forks source link

ObjectMapper doesn't call my PrettyPrinter overrides #2203

Closed cakoose closed 5 years ago

cakoose commented 5 years ago

I'm subclassing DefaultPrettyPrinter to override the writeEndArray/writeEndObject behavior.

The overrides seem to work when using JsonGenerator directly, but not when using ObjectMapper.

import com.fasterxml.jackson.core.JsonGenerator;
import com.fasterxml.jackson.core.JsonFactory;
import com.fasterxml.jackson.core.util.DefaultIndenter;
import com.fasterxml.jackson.core.util.DefaultPrettyPrinter;
import com.fasterxml.jackson.databind.MapperFeature;
import com.fasterxml.jackson.databind.ObjectMapper;

import java.util.HashMap;
import java.io.IOException;

public final class Test {
    private static final JsonFactory jsonFactory = new JsonFactory()
            .disable(JsonGenerator.Feature.AUTO_CLOSE_TARGET);

    private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper(jsonFactory);

    private static final DefaultPrettyPrinter PRETTY_PRINTER = new DefaultPrettyPrinter() {
        {
            DefaultPrettyPrinter.Indenter indenter = new DefaultIndenter("    ", "\n");
            this._objectFieldValueSeparatorWithSpaces = ": ";
            this.indentArraysWith(indenter);
            this.indentObjectsWith(indenter);
        }

        @Override
        public void writeObjectFieldValueSeparator(JsonGenerator g) throws IOException {
            System.err.println("CALLED: writeObjectFieldValueSeparator");
            super.writeObjectFieldValueSeparator(g);
        }
        @Override
        public void writeStartObject(JsonGenerator g) throws IOException {
            System.err.println("CALLED: writeStartObject");
            super.writeStartObject(g);
        }
        @Override
        public void writeStartArray(JsonGenerator g) throws IOException {
            System.err.println("CALLED: writeStartArray");
            super.writeStartArray(g);
        }

        // HACK: DefaultPrettyPrinter adds a space before the closing brace/bracket for empty
        // arrays/objects.  This override is a copy/paste of the relevant code, without that
        // one behavior.
        @Override
        public void writeEndObject(JsonGenerator g, int nrOfEntries) throws IOException {
            System.err.println("CALLED: writeEndObject");
            if (!_objectIndenter.isInline()) {
                --_nesting;
            }
            if (nrOfEntries > 0) {
                _objectIndenter.writeIndentation(g, _nesting);
            }
            g.writeRaw('}');
        }
        @Override
        public void writeEndArray(JsonGenerator g, int nrOfEntries) throws IOException {
            System.err.println("CALLED: writeEndArray");
            if (!_arrayIndenter.isInline()) {
                --_nesting;
            }
            if (nrOfEntries > 0) {
                _arrayIndenter.writeIndentation(g, _nesting);
            }
            g.writeRaw(']');
        }
    };

    public static void main(String[] args) throws IOException {
        if (args.length != 0) {
            System.err.println("No args allowed.");
            System.exit(1); throw new AssertionError();
        }

        System.out.println("Using JsonGenerator directly.");
        JsonGenerator generator = jsonFactory.createGenerator(System.out);
        generator.setPrettyPrinter(PRETTY_PRINTER);
        generator.writeStartObject();
        generator.writeFieldName("stuff");
        generator.writeStartArray();
        generator.writeEndArray();
        generator.writeEndObject();
        generator.close();
        System.out.println();

        HashMap<String, Object> data = new HashMap<>();
        data.put("stuff", new String[0]);

        System.out.println("Using ObjectMapper with default PrettyPrinter.");
        OBJECT_MAPPER.writerWithDefaultPrettyPrinter().writeValue(System.out, data);
        System.out.println();

        System.out.println("Using ObjectMapper with custom PrettyPrinter.");
        OBJECT_MAPPER.writer(PRETTY_PRINTER).writeValue(System.out, data);
        System.out.println();
    }
}

Output:

Using JsonGenerator directly.
CALLED: writeStartObject
CALLED: writeObjectFieldValueSeparator
CALLED: writeStartArray
CALLED: writeEndArray
CALLED: writeEndObject
{
    "stuff": []
}
Using ObjectMapper with default PrettyPrinter.
{
  "stuff" : [ ]
}
Using ObjectMapper with custom PrettyPrinter.
{
    "stuff": [ ]
}

The "CALLED" lines appear when using JsonGenerator directly, but not when using ObjectMapper.

Additional weirdness: the "with custom PrettyPrinter" has my customized field separator, but not my customized writeEndObject/writeEndArray behavior.

My environment:

cakoose commented 5 years ago

One more thing: I just tried implementing my own PrettyPrinter from scratch. Passing in an instance of that yields the formatting I want.

However, if I change my implementation from implements PrettyPrinter to extends DefaultPrettyPrinter, the bug reappears. Maybe Jackson checks for instanceof DefaultPrettyPrinter somewhere?

Updated test case:

import com.fasterxml.jackson.core.JsonGenerator;
import com.fasterxml.jackson.core.JsonFactory;
import com.fasterxml.jackson.core.PrettyPrinter;
import com.fasterxml.jackson.core.util.DefaultPrettyPrinter;
import com.fasterxml.jackson.databind.ObjectMapper;

import java.util.HashMap;
import java.io.IOException;

public final class Test {
    private static final JsonFactory jsonFactory = new JsonFactory()
            .disable(JsonGenerator.Feature.AUTO_CLOSE_TARGET);

    private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper(jsonFactory);

    // changing "new DefaultPrettyPrinter" to "new PrettyPrinter" fixes the problem!
    private static final PrettyPrinter PRETTY_PRINTER = new DefaultPrettyPrinter() {
        private int indentationLevel = 0;

        @Override
        public void writeRootValueSeparator(JsonGenerator g) {}

        @Override
        public void writeStartObject(JsonGenerator g) throws IOException {
            g.writeRaw('{');
            indentationLevel++;
        }

        @Override
        public void beforeObjectEntries(JsonGenerator g) throws IOException {
            writeIndentation(g);
        }

        private void writeIndentation(JsonGenerator g) throws IOException {
            g.writeRaw('\n');
            for (int i = 0; i < indentationLevel; i++) {
                g.writeRaw("    ");
            }
        }

        @Override
        public void writeObjectFieldValueSeparator(JsonGenerator g) throws IOException
        {
            g.writeRaw(": ");
        }

        @Override
        public void writeObjectEntrySeparator(JsonGenerator g) throws IOException
        {
            g.writeRaw(',');
            writeIndentation(g);
        }

        @Override
        public void writeEndObject(JsonGenerator g, int nrOfEntries) throws IOException
        {
            indentationLevel--;
            if (nrOfEntries > 0) {
                writeIndentation(g);
            }
            g.writeRaw('}');
        }

        @Override
        public void writeStartArray(JsonGenerator g) throws IOException
        {
            indentationLevel++;
            g.writeRaw('[');
        }

        @Override
        public void beforeArrayValues(JsonGenerator g) throws IOException {
            writeIndentation(g);
        }

        @Override
        public void writeArrayValueSeparator(JsonGenerator g) throws IOException
        {
            g.writeRaw(',');
            writeIndentation(g);
        }

        @Override
        public void writeEndArray(JsonGenerator g, int nrOfValues) throws IOException
        {
            indentationLevel--;
            if (nrOfValues > 0) {
                writeIndentation(g);
            }
            g.writeRaw(']');
        }
    };

    public static void main(String[] args) throws IOException {
        if (args.length != 0) {
            System.err.println("No args allowed.");
            System.exit(1); throw new AssertionError();
        }

        System.out.println("1. JsonGenerator directly");
        {
            JsonGenerator generator = jsonFactory.createGenerator(System.out).setPrettyPrinter(PRETTY_PRINTER);
            generator.writeStartObject();
            generator.writeFieldName("stuff");
            generator.writeStartArray();
            generator.writeEndArray();
            generator.writeEndObject();
            generator.close();
            System.out.println();
        }

        HashMap<String, Object> data = new HashMap<>();
        data.put("stuff", new String[0]);

        System.out.println("2. ObjectMapper.writerWithDefaultPrettyPrinter().writeValue(out, data)");
        OBJECT_MAPPER.writerWithDefaultPrettyPrinter().writeValue(System.out, data);
        System.out.println();

        System.out.println("3. ObjectMapper.writer(PRETTY_PRINTER).writeValue(out, data)");
        OBJECT_MAPPER.writer(PRETTY_PRINTER).writeValue(System.out, data);
        System.out.println();
    }
}

Output with new DefaultPrettyPrinter() { ... } (bad result):

1. JsonGenerator directly
{
    "stuff": []
}
2. ObjectMapper.writerWithDefaultPrettyPrinter().writeValue(out, data)
{
  "stuff" : [ ]
}
3. ObjectMapper.writer(PRETTY_PRINTER).writeValue(out, data)
{
  "stuff" : [ ]
}

Output with new PrettyPrinter() { ... } (expected result):

1. JsonGenerator directly
{
    "stuff": []
}
2. ObjectMapper.writerWithDefaultPrettyPrinter().writeValue(out, data)
{
  "stuff" : [ ]
}
3. ObjectMapper.writer(PRETTY_PRINTER).writeValue(out, data)
{
  "stuff" : [ ]
}
cowtowncoder commented 5 years ago

Actually I think I know what the problem is: you do need to override:

public DefaultPrettyPrinter createInstance() {
}

method, as anything that implements Instantiatable<?> will get this method called. So basically you are registering a "blueprint" instance (~= factory) -- instances are not thread-safe, so new instance needs to be created.

But I think I should change implementation in DefaultPrettyPrinter to throw an exception in case it is sub-classed; behavior is otherwise quite unintuitive.

cakoose commented 5 years ago

Ah, I see.

Maybe methods that need an instance can take PrettyPrinter but methods that need a factory take something like Instantiable<PrettyPrinter>?

cowtowncoder commented 5 years ago

If I was adding an API or extension point, absolutely. The reason it is this way is that it was a retrofit, having to solve statefulness problem after PrettyPrinter was already in place.

This actually might be a good thing to do for Jackson 3.0, separating out instance, supplier. With Java 8 baseline that would be much more natural. Although could then consider question of context to pass etc... I'll file a new issue for this one.