Override ObjectMapper in the DefaultDataConverter's payload coverter

[GreyTeardrop]

Overview

Expose the DataConverter.newBuilder() method to provide a way to customize e.g. ObjectMapper but not require clients to know the details of how the default DataConverter is built.

What was changed:

1. New DataConverted.Builder class is created to accept arbitrary ObjectMapper, JsonFormat.Printer and JsonFormat.Parser and build an instance of DataConverter that looks exactly like DataConverter.getDefaultInstance() except those custom mappers.
2. ProtobufJsonPayloadConverter is updated to use the printer provided via its constructor - before it used to always create a new printer via JsonFormat.printer() regardless of how it was configured.

Why?

Right now in order to use a custom ObjectMapper clients have to directly instantiate DefaultDataConverter and provide it with a proper list of PayloadConvertes. In order to keep this custom DataConverter as close to the default instance as possible clients should do

DataConverter dataConverter = new DefaultDataConverter(
    new NullPayloadConverter(),
    new ByteArrayPayloadConverter(),
    new ProtobufJsonPayloadConverter(),
    new JacksonJsonPayloadConverter(objectMapper));

which might be a bit more of knowledge of now default data converter is configured.

Instead, it would be great to only provide a custom ObjectMapper instance and otherwise build the same default DataConverter.

How has this been tested?

Existing and new unit tests.

Any docs updates needed?

Javadocs for new public methods were added.

[mfateev]

Sorry for the delay. I don't like that the JSON specific configuration leaks into DataConverter class. But I also recognize the problem you are trying to solve. Let us think about the best solution.

Keeping open for now.

[GreyTeardrop]

Thank you, @mfateev, that's definitely a fair issue.

One simple way to tackle that could be to move the builder into the DefaultDataConverter.

An alternative approach, although a bit more complex one, could be to add a "replace payload converter" method to the DefaultDataConverter, so one could start with the default instance of the DefaultDataConverter and create a copy that replaces the default JacksonJsonPayloadConverter with a provided one, so the client code could look like

DataConverter.getDefaultInstance().withPayloadConverterOverride(JacksonJsonPayloadConverter(objectMapper))

[mfateev]

I'm OK with
DataConverter.getDefaultInstance().withPayloadConverterOverride(JacksonJsonPayloadConverter(objectMapper))

[GreyTeardrop]

@mfateev
I've reworked the PR to remove the direct dependency of the DataConverter on the ObjectMapper, JsonFormat.Printer, and JsonFormat.Parser. Instead, clients can now pass a custom instance of e.g. JacksonJsonPayloadConverter and it would be used instead of the default new JacksonJsonPayloadConverter().

DataConverter converter =
    DataConverter.newBuilder()
        .setPayloadConverterOverrides(new JacksonJsonPayloadConverter(objectMapper))
        .build();

[mfateev]

Sorry, after looking at the PR I propose an alternative solution. I don't like that the DataConverter interface has an associated Builder that builds a particular implementation of this interface. To me it looks like anti-pattern.

My proposal is:

• Add an empty argument constructor to DefaultDataConverter which initializes it to the default list of converters.
• Add DefaultDataConverter.withPayloadConverterOverride method.

So the resulting code will be:

DataConverter converter = new DefaultDataConverter()
          .withPayloadConverterOverride(new JacksonJsonPayloadConverter(objectMapper));

[GreyTeardrop]

My proposal is:

• Add an empty argument constructor to DefaultDataConverter which initializes it to the default list of converters.

• Add DefaultDataConverter.withPayloadConverterOverride method.

So the resulting code will be:

DataConverter converter = new DefaultDataConverter()
          .withPayloadConverterOverride(new JacksonJsonPayloadConverter(objectMapper));

I've tried something like that. The trick is, as DefaultDataConverter's constructor accepts varargs, the empty constructor call would already resolve to it and would mean "no payload converters" instead of "default payload converters". That could easily be changed to say "if the list of converters is empty, add the default ones" but it looked a bit unpredictable to me.

I've rolled back the DataConverter changes. There's still a new DefaultDataConverter constructor that accepts a boolean and a list of payload converters, so one could directly call.

DataConverter converter = new DefaultDataConverter(true, new JacksonJsonPayloadConverter(objectMapper));

Another alternative would be to create a factory method on the DefaultDataConverter that would instantiate a new instance with default converters, and then have that withPayloadConverterOverride, but I struggle with a good name for it.

DataConverter converter = DefaultDataConverter.newInstanceWithDefaultPayloadConverters()
          .withPayloadConverterOverride(new JacksonJsonPayloadConverter(objectMapper));

[mfateev]

I see. Varags are error prone :(. And not providing a function for creation instead of a constructor was a mistake.

I would name the function newDefaultInstance:

DefaultDataConverter.newDefaultInstance()
          .withPayloadConverterOverride(new JacksonJsonPayloadConverter(objectMapper));

[GreyTeardrop]

Thanks! I've updated PR to use a factory method and a builder-like withPayloadConverterOverride. I've also changed it to replace payload converters by their encoding type instead of their class.

[mfateev]

Thanks a lot for the contribution!