From f4bfb98d958fb64d7a5add66717de9a2bdc09e40 Mon Sep 17 00:00:00 2001 From: Mykola Rybak Date: Tue, 30 Mar 2021 23:49:45 +0300 Subject: [PATCH 1/6] Builder for DataConverter Expose DataConverter.newBuilder() method to provide a way to customize e.g. ObjectMapper but not require clients to know the details how the default DataConverter is built. --- .../common/converter/DataConverter.java | 50 ++++++++ .../converter/DefaultDataConverter.java | 20 +++- .../JacksonJsonPayloadConverter.java | 14 ++- .../ProtobufJsonPayloadConverter.java | 9 +- .../converter/ProtoPayloadConverterTest.java | 108 ++++++++++++++++++ 5 files changed, 188 insertions(+), 13 deletions(-) diff --git a/temporal-sdk/src/main/java/io/temporal/common/converter/DataConverter.java b/temporal-sdk/src/main/java/io/temporal/common/converter/DataConverter.java index 44e4ffd050..b495903dba 100644 --- a/temporal-sdk/src/main/java/io/temporal/common/converter/DataConverter.java +++ b/temporal-sdk/src/main/java/io/temporal/common/converter/DataConverter.java @@ -19,7 +19,9 @@ package io.temporal.common.converter; +import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.base.Defaults; +import com.google.protobuf.util.JsonFormat; import io.temporal.api.common.v1.Payload; import io.temporal.api.common.v1.Payloads; import java.lang.reflect.Type; @@ -38,6 +40,14 @@ static DataConverter getDefaultInstance() { return DefaultDataConverter.getDefaultInstance(); } + /** + * Creates a builder for customized {@code DataConverter} that behaves similar to the {@link + * #getDefaultInstance() default instance}. + */ + static Builder newBuilder() { + return new Builder(); + } + Optional toPayload(T value); T fromPayload(Payload payload, Class valueClass, Type valueType); @@ -106,4 +116,44 @@ static Object[] arrayFromPayloads( } return result; } + + class Builder { + + private ObjectMapper jacksonObjectMapper; + private JsonFormat.Printer protobufJsonPrinter; + private JsonFormat.Parser protobufJsonParser; + + private Builder() {} + + /** + * Set custom Jackson {@link ObjectMapper} used by the data converter to serialize and + * deserialize arbitrary payload types. + */ + public Builder setJacksonObjectMapper(ObjectMapper jacksonObjectMapper) { + this.jacksonObjectMapper = jacksonObjectMapper; + return this; + } + + /** + * Set custom Protobuf {@link JsonFormat.Printer} used by the data converter to serialize + * Protobuf payload types. + */ + public Builder setProtobufJsonPrinter(JsonFormat.Printer protobufJsonPrinter) { + this.protobufJsonPrinter = protobufJsonPrinter; + return this; + } + + /** + * Set custom Protobuf {@link JsonFormat.Parser} used by the data converter to deserialize + * Protobuf payload types. + */ + public Builder setProtobufJsonParser(JsonFormat.Parser protobufJsonParser) { + this.protobufJsonParser = protobufJsonParser; + return this; + } + + public DataConverter build() { + return new DefaultDataConverter(jacksonObjectMapper, protobufJsonPrinter, protobufJsonParser); + } + } } diff --git a/temporal-sdk/src/main/java/io/temporal/common/converter/DefaultDataConverter.java b/temporal-sdk/src/main/java/io/temporal/common/converter/DefaultDataConverter.java index d558dd46dd..7f92670318 100644 --- a/temporal-sdk/src/main/java/io/temporal/common/converter/DefaultDataConverter.java +++ b/temporal-sdk/src/main/java/io/temporal/common/converter/DefaultDataConverter.java @@ -21,7 +21,9 @@ import static java.nio.charset.StandardCharsets.UTF_8; +import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.base.Defaults; +import com.google.protobuf.util.JsonFormat; import io.temporal.api.common.v1.Payload; import io.temporal.api.common.v1.Payloads; import java.lang.reflect.Type; @@ -39,12 +41,15 @@ */ public class DefaultDataConverter implements DataConverter { + private static final PayloadConverter NULL_CONVERTER = new NullPayloadConverter(); + private static final PayloadConverter BYTE_ARRAY_CONVERTER = new ByteArrayPayloadConverter(); + private static final AtomicReference defaultDataConverterInstance = new AtomicReference<>( // Order is important as the first converter that can convert the payload is used new DefaultDataConverter( - new NullPayloadConverter(), - new ByteArrayPayloadConverter(), + NULL_CONVERTER, + BYTE_ARRAY_CONVERTER, new ProtobufJsonPayloadConverter(), new JacksonJsonPayloadConverter())); @@ -82,6 +87,17 @@ public DefaultDataConverter(PayloadConverter... converters) { } } + DefaultDataConverter( + ObjectMapper jacksonObjectMapper, + JsonFormat.Printer protobufJsonPrinter, + JsonFormat.Parser protobufJsonParser) { + this( + NULL_CONVERTER, + BYTE_ARRAY_CONVERTER, + new ProtobufJsonPayloadConverter(protobufJsonPrinter, protobufJsonParser), + new JacksonJsonPayloadConverter(jacksonObjectMapper)); + } + @Override public Optional toPayload(T value) { for (PayloadConverter converter : converters) { diff --git a/temporal-sdk/src/main/java/io/temporal/common/converter/JacksonJsonPayloadConverter.java b/temporal-sdk/src/main/java/io/temporal/common/converter/JacksonJsonPayloadConverter.java index b949a8fe20..b198ef7bcb 100644 --- a/temporal-sdk/src/main/java/io/temporal/common/converter/JacksonJsonPayloadConverter.java +++ b/temporal-sdk/src/main/java/io/temporal/common/converter/JacksonJsonPayloadConverter.java @@ -37,14 +37,11 @@ public class JacksonJsonPayloadConverter implements PayloadConverter { private final ObjectMapper mapper; public JacksonJsonPayloadConverter() { - mapper = new ObjectMapper(); - mapper.configure(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS, false); - mapper.registerModule(new JavaTimeModule()); - mapper.setVisibility(PropertyAccessor.FIELD, JsonAutoDetect.Visibility.ANY); + this(null); } public JacksonJsonPayloadConverter(ObjectMapper mapper) { - this.mapper = mapper; + this.mapper = mapper != null ? mapper : newDefaultObjectMapper(); } @Override @@ -82,4 +79,11 @@ public T fromData(Payload content, Class valueClass, Type valueType) throw new DataConverterException(e); } } + + private ObjectMapper newDefaultObjectMapper() { + return new ObjectMapper() + .configure(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS, false) + .registerModule(new JavaTimeModule()) + .setVisibility(PropertyAccessor.FIELD, JsonAutoDetect.Visibility.ANY); + } } diff --git a/temporal-sdk/src/main/java/io/temporal/common/converter/ProtobufJsonPayloadConverter.java b/temporal-sdk/src/main/java/io/temporal/common/converter/ProtobufJsonPayloadConverter.java index e14bf885ec..e0e53f6638 100644 --- a/temporal-sdk/src/main/java/io/temporal/common/converter/ProtobufJsonPayloadConverter.java +++ b/temporal-sdk/src/main/java/io/temporal/common/converter/ProtobufJsonPayloadConverter.java @@ -29,7 +29,6 @@ import io.temporal.api.common.v1.Payload; import java.lang.reflect.Method; import java.lang.reflect.Type; -import java.util.Objects; import java.util.Optional; public final class ProtobufJsonPayloadConverter implements PayloadConverter { @@ -38,13 +37,12 @@ public final class ProtobufJsonPayloadConverter implements PayloadConverter { private final JsonFormat.Parser parser; public ProtobufJsonPayloadConverter() { - printer = JsonFormat.printer().preservingProtoFieldNames(); - parser = JsonFormat.parser().ignoringUnknownFields(); + this(null, null); } public ProtobufJsonPayloadConverter(JsonFormat.Printer printer, JsonFormat.Parser parser) { - this.printer = Objects.requireNonNull(printer); - this.parser = Objects.requireNonNull(parser); + this.printer = printer != null ? printer : JsonFormat.printer().preservingProtoFieldNames(); + this.parser = parser != null ? parser : JsonFormat.parser().ignoringUnknownFields(); } @Override @@ -57,7 +55,6 @@ public Optional toData(Object value) throws DataConverterException { if (!(value instanceof MessageOrBuilder)) { return Optional.empty(); } - JsonFormat.Printer printer = JsonFormat.printer(); try { String data = printer.print((MessageOrBuilder) value); return Optional.of( diff --git a/temporal-sdk/src/test/java/io/temporal/common/converter/ProtoPayloadConverterTest.java b/temporal-sdk/src/test/java/io/temporal/common/converter/ProtoPayloadConverterTest.java index e3269c7792..6797b57079 100644 --- a/temporal-sdk/src/test/java/io/temporal/common/converter/ProtoPayloadConverterTest.java +++ b/temporal-sdk/src/test/java/io/temporal/common/converter/ProtoPayloadConverterTest.java @@ -21,8 +21,14 @@ import static org.junit.Assert.assertEquals; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.SerializationFeature; +import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule; +import com.google.protobuf.util.JsonFormat; import io.temporal.api.common.v1.Payloads; import io.temporal.api.common.v1.WorkflowExecution; +import java.time.Instant; +import java.util.Objects; import java.util.Optional; import java.util.UUID; import org.junit.Test; @@ -43,6 +49,20 @@ public void testProtoJson() { assertEquals(execution, converted); } + @Test + public void testCustomJson() { + ObjectMapper objectMapper = + new ObjectMapper() + .configure(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS, true) + .registerModule(new JavaTimeModule()); + DataConverter converter = + DataConverter.newBuilder().setJacksonObjectMapper(objectMapper).build(); + TestPayload payload = new TestPayload(1L, Instant.now(), "myPayload"); + Optional data = converter.toPayloads(payload); + TestPayload converted = converter.fromPayloads(0, data, TestPayload.class, TestPayload.class); + assertEquals(payload, converted); + } + @Test public void testProto() { DataConverter converter = new DefaultDataConverter(new ProtobufPayloadConverter()); @@ -56,4 +76,92 @@ public void testProto() { converter.fromPayloads(0, data, WorkflowExecution.class, WorkflowExecution.class); assertEquals(execution, converted); } + + @Test + public void testCustomProto() { + DataConverter converter = + DataConverter.newBuilder() + .setProtobufJsonPrinter(JsonFormat.printer().printingEnumsAsInts()) + .setProtobufJsonParser(JsonFormat.parser()) + .build(); + WorkflowExecution execution = + WorkflowExecution.newBuilder() + .setWorkflowId(UUID.randomUUID().toString()) + .setRunId(UUID.randomUUID().toString()) + .build(); + Optional data = converter.toPayloads(execution); + WorkflowExecution converted = + converter.fromPayloads(0, data, WorkflowExecution.class, WorkflowExecution.class); + assertEquals(execution, converted); + } + + static class TestPayload { + private long id; + private Instant timestamp; + private String name; + + public TestPayload() {} + + TestPayload(long id, Instant timestamp, String name) { + this.id = id; + this.timestamp = timestamp; + this.name = name; + } + + public long getId() { + return id; + } + + public void setId(long id) { + this.id = id; + } + + public Instant getTimestamp() { + return timestamp; + } + + public void setTimestamp(Instant timestamp) { + this.timestamp = timestamp; + } + + public String getName() { + return name; + } + + public void setName(String name) { + this.name = name; + } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + TestPayload that = (TestPayload) o; + return id == that.id + && Objects.equals(timestamp, that.timestamp) + && Objects.equals(name, that.name); + } + + @Override + public int hashCode() { + return Objects.hash(id, timestamp, name); + } + + @Override + public String toString() { + return "TestPayload{" + + "id=" + + id + + ", timestamp=" + + timestamp + + ", name='" + + name + + '\'' + + '}'; + } + } } From 1510f8c5c7d1d48f38184ada1e8d179c819d1d7a Mon Sep 17 00:00:00 2001 From: Mykola Rybak Date: Thu, 8 Apr 2021 00:00:43 +0300 Subject: [PATCH 2/6] No direct dependency on PayloadConverter internals Avoid direct dependency on DataConverter on specific PayloadConverter internals like ObjectMapper by supporting an option of merging custom PayloadConverters into the default PayloadConverters list in the DefaultDataConverter class. --- .../common/converter/DataConverter.java | 34 ++------ .../converter/DefaultDataConverter.java | 79 ++++++++++++------- .../converter/ProtoPayloadConverterTest.java | 9 ++- 3 files changed, 64 insertions(+), 58 deletions(-) diff --git a/temporal-sdk/src/main/java/io/temporal/common/converter/DataConverter.java b/temporal-sdk/src/main/java/io/temporal/common/converter/DataConverter.java index b495903dba..b8880579d3 100644 --- a/temporal-sdk/src/main/java/io/temporal/common/converter/DataConverter.java +++ b/temporal-sdk/src/main/java/io/temporal/common/converter/DataConverter.java @@ -19,9 +19,7 @@ package io.temporal.common.converter; -import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.base.Defaults; -import com.google.protobuf.util.JsonFormat; import io.temporal.api.common.v1.Payload; import io.temporal.api.common.v1.Payloads; import java.lang.reflect.Type; @@ -119,41 +117,21 @@ static Object[] arrayFromPayloads( class Builder { - private ObjectMapper jacksonObjectMapper; - private JsonFormat.Printer protobufJsonPrinter; - private JsonFormat.Parser protobufJsonParser; + private PayloadConverter[] payloadConverterOverrides = {}; private Builder() {} /** - * Set custom Jackson {@link ObjectMapper} used by the data converter to serialize and - * deserialize arbitrary payload types. + * Provide {@link PayloadConverter}s that would be merged with the default list of payload + * converters. */ - public Builder setJacksonObjectMapper(ObjectMapper jacksonObjectMapper) { - this.jacksonObjectMapper = jacksonObjectMapper; - return this; - } - - /** - * Set custom Protobuf {@link JsonFormat.Printer} used by the data converter to serialize - * Protobuf payload types. - */ - public Builder setProtobufJsonPrinter(JsonFormat.Printer protobufJsonPrinter) { - this.protobufJsonPrinter = protobufJsonPrinter; - return this; - } - - /** - * Set custom Protobuf {@link JsonFormat.Parser} used by the data converter to deserialize - * Protobuf payload types. - */ - public Builder setProtobufJsonParser(JsonFormat.Parser protobufJsonParser) { - this.protobufJsonParser = protobufJsonParser; + public Builder setPayloadConverterOverrides(PayloadConverter... payloadConverterOverrides) { + this.payloadConverterOverrides = payloadConverterOverrides; return this; } public DataConverter build() { - return new DefaultDataConverter(jacksonObjectMapper, protobufJsonPrinter, protobufJsonParser); + return new DefaultDataConverter(true, payloadConverterOverrides); } } } diff --git a/temporal-sdk/src/main/java/io/temporal/common/converter/DefaultDataConverter.java b/temporal-sdk/src/main/java/io/temporal/common/converter/DefaultDataConverter.java index 7f92670318..be7479d810 100644 --- a/temporal-sdk/src/main/java/io/temporal/common/converter/DefaultDataConverter.java +++ b/temporal-sdk/src/main/java/io/temporal/common/converter/DefaultDataConverter.java @@ -21,13 +21,12 @@ import static java.nio.charset.StandardCharsets.UTF_8; -import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.base.Defaults; -import com.google.protobuf.util.JsonFormat; import io.temporal.api.common.v1.Payload; import io.temporal.api.common.v1.Payloads; import java.lang.reflect.Type; import java.util.ArrayList; +import java.util.Collections; import java.util.List; import java.util.Map; import java.util.Optional; @@ -41,19 +40,16 @@ */ public class DefaultDataConverter implements DataConverter { - private static final PayloadConverter NULL_CONVERTER = new NullPayloadConverter(); - private static final PayloadConverter BYTE_ARRAY_CONVERTER = new ByteArrayPayloadConverter(); + // Order is important as the first converter that can convert the payload is used + private static final PayloadConverter[] DEFAULT_PAYLOAD_CONVERTERS = { + new NullPayloadConverter(), + new ByteArrayPayloadConverter(), + new ProtobufJsonPayloadConverter(), + new JacksonJsonPayloadConverter() + }; private static final AtomicReference defaultDataConverterInstance = - new AtomicReference<>( - // Order is important as the first converter that can convert the payload is used - new DefaultDataConverter( - NULL_CONVERTER, - BYTE_ARRAY_CONVERTER, - new ProtobufJsonPayloadConverter(), - new JacksonJsonPayloadConverter())); - - private static final Object[] EMPTY_OBJECT_ARRAY = new Object[0]; + new AtomicReference<>(new DefaultDataConverter(DEFAULT_PAYLOAD_CONVERTERS)); private final Map converterMap = new ConcurrentHashMap<>(); @@ -77,25 +73,47 @@ public static void setDefaultDataConverter(DataConverter converter) { /** * Creates instance from ordered array of converters. When converting an object to payload the - * array of converters is iterated from the beginning until one of the converters succesfully + * array of converters is iterated from the beginning until one of the converters successfully * converts the value. */ public DefaultDataConverter(PayloadConverter... converters) { - for (PayloadConverter converter : converters) { - this.converters.add(converter); - this.converterMap.put(converter.getEncodingType(), converter); - } + setPayloadConverters(converters); } - DefaultDataConverter( - ObjectMapper jacksonObjectMapper, - JsonFormat.Printer protobufJsonPrinter, - JsonFormat.Parser protobufJsonParser) { - this( - NULL_CONVERTER, - BYTE_ARRAY_CONVERTER, - new ProtobufJsonPayloadConverter(protobufJsonPrinter, protobufJsonParser), - new JacksonJsonPayloadConverter(jacksonObjectMapper)); + /** + * Creates instance from ordered array of converters. Provided converters may be merged with the + * default list of converters if {@code mergeDefaultConverters} is {@code true}. + * + *

When converting an object to payload the array of converters is iterated from the beginning + * until one of the converters successfully converts the value. + */ + public DefaultDataConverter(boolean mergeDefaultConverters, PayloadConverter... converters) { + if (!mergeDefaultConverters) { + setPayloadConverters(converters); + } else if (converters.length == 0) { + setPayloadConverters(DEFAULT_PAYLOAD_CONVERTERS); + } else { + List mergedConverters = new ArrayList<>(DEFAULT_PAYLOAD_CONVERTERS.length); + Collections.addAll(mergedConverters, DEFAULT_PAYLOAD_CONVERTERS); + + for (PayloadConverter newConverter : converters) { + Class newConverterClass = newConverter.getClass(); + boolean merged = false; + for (int i = 0; i < mergedConverters.size(); i++) { + PayloadConverter existingConverter = mergedConverters.get(i); + if (existingConverter.getClass().isAssignableFrom(newConverterClass)) { + mergedConverters.set(i, newConverter); + merged = true; + break; + } + } + if (!merged) { + mergedConverters.add(newConverter); + } + } + + setPayloadConverters(mergedConverters.toArray(new PayloadConverter[0])); + } } @Override @@ -170,4 +188,11 @@ public T fromPayloads( } return fromPayload(content.get().getPayloads(index), parameterType, genericParameterType); } + + private void setPayloadConverters(PayloadConverter... converters) { + for (PayloadConverter converter : converters) { + this.converters.add(converter); + this.converterMap.put(converter.getEncodingType(), converter); + } + } } diff --git a/temporal-sdk/src/test/java/io/temporal/common/converter/ProtoPayloadConverterTest.java b/temporal-sdk/src/test/java/io/temporal/common/converter/ProtoPayloadConverterTest.java index 6797b57079..e1fab34b21 100644 --- a/temporal-sdk/src/test/java/io/temporal/common/converter/ProtoPayloadConverterTest.java +++ b/temporal-sdk/src/test/java/io/temporal/common/converter/ProtoPayloadConverterTest.java @@ -56,7 +56,9 @@ public void testCustomJson() { .configure(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS, true) .registerModule(new JavaTimeModule()); DataConverter converter = - DataConverter.newBuilder().setJacksonObjectMapper(objectMapper).build(); + DataConverter.newBuilder() + .setPayloadConverterOverrides(new JacksonJsonPayloadConverter(objectMapper)) + .build(); TestPayload payload = new TestPayload(1L, Instant.now(), "myPayload"); Optional data = converter.toPayloads(payload); TestPayload converted = converter.fromPayloads(0, data, TestPayload.class, TestPayload.class); @@ -81,8 +83,9 @@ public void testProto() { public void testCustomProto() { DataConverter converter = DataConverter.newBuilder() - .setProtobufJsonPrinter(JsonFormat.printer().printingEnumsAsInts()) - .setProtobufJsonParser(JsonFormat.parser()) + .setPayloadConverterOverrides( + new ProtobufJsonPayloadConverter( + JsonFormat.printer().printingEnumsAsInts(), JsonFormat.parser())) .build(); WorkflowExecution execution = WorkflowExecution.newBuilder() From 0fda8e4463cb1e00e5d782814db0f7b03ded1d4e Mon Sep 17 00:00:00 2001 From: Mykola Rybak Date: Fri, 9 Apr 2021 12:21:48 +0300 Subject: [PATCH 3/6] Revert PayloadConverters changes --- .../converter/JacksonJsonPayloadConverter.java | 14 +++++--------- .../converter/ProtobufJsonPayloadConverter.java | 9 ++++++--- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/temporal-sdk/src/main/java/io/temporal/common/converter/JacksonJsonPayloadConverter.java b/temporal-sdk/src/main/java/io/temporal/common/converter/JacksonJsonPayloadConverter.java index b198ef7bcb..b949a8fe20 100644 --- a/temporal-sdk/src/main/java/io/temporal/common/converter/JacksonJsonPayloadConverter.java +++ b/temporal-sdk/src/main/java/io/temporal/common/converter/JacksonJsonPayloadConverter.java @@ -37,11 +37,14 @@ public class JacksonJsonPayloadConverter implements PayloadConverter { private final ObjectMapper mapper; public JacksonJsonPayloadConverter() { - this(null); + mapper = new ObjectMapper(); + mapper.configure(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS, false); + mapper.registerModule(new JavaTimeModule()); + mapper.setVisibility(PropertyAccessor.FIELD, JsonAutoDetect.Visibility.ANY); } public JacksonJsonPayloadConverter(ObjectMapper mapper) { - this.mapper = mapper != null ? mapper : newDefaultObjectMapper(); + this.mapper = mapper; } @Override @@ -79,11 +82,4 @@ public T fromData(Payload content, Class valueClass, Type valueType) throw new DataConverterException(e); } } - - private ObjectMapper newDefaultObjectMapper() { - return new ObjectMapper() - .configure(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS, false) - .registerModule(new JavaTimeModule()) - .setVisibility(PropertyAccessor.FIELD, JsonAutoDetect.Visibility.ANY); - } } diff --git a/temporal-sdk/src/main/java/io/temporal/common/converter/ProtobufJsonPayloadConverter.java b/temporal-sdk/src/main/java/io/temporal/common/converter/ProtobufJsonPayloadConverter.java index e0e53f6638..e14bf885ec 100644 --- a/temporal-sdk/src/main/java/io/temporal/common/converter/ProtobufJsonPayloadConverter.java +++ b/temporal-sdk/src/main/java/io/temporal/common/converter/ProtobufJsonPayloadConverter.java @@ -29,6 +29,7 @@ import io.temporal.api.common.v1.Payload; import java.lang.reflect.Method; import java.lang.reflect.Type; +import java.util.Objects; import java.util.Optional; public final class ProtobufJsonPayloadConverter implements PayloadConverter { @@ -37,12 +38,13 @@ public final class ProtobufJsonPayloadConverter implements PayloadConverter { private final JsonFormat.Parser parser; public ProtobufJsonPayloadConverter() { - this(null, null); + printer = JsonFormat.printer().preservingProtoFieldNames(); + parser = JsonFormat.parser().ignoringUnknownFields(); } public ProtobufJsonPayloadConverter(JsonFormat.Printer printer, JsonFormat.Parser parser) { - this.printer = printer != null ? printer : JsonFormat.printer().preservingProtoFieldNames(); - this.parser = parser != null ? parser : JsonFormat.parser().ignoringUnknownFields(); + this.printer = Objects.requireNonNull(printer); + this.parser = Objects.requireNonNull(parser); } @Override @@ -55,6 +57,7 @@ public Optional toData(Object value) throws DataConverterException { if (!(value instanceof MessageOrBuilder)) { return Optional.empty(); } + JsonFormat.Printer printer = JsonFormat.printer(); try { String data = printer.print((MessageOrBuilder) value); return Optional.of( From 122126e2abb61cb3588048ec9f18baf66c6a65d0 Mon Sep 17 00:00:00 2001 From: Mykola Rybak Date: Fri, 9 Apr 2021 12:28:45 +0300 Subject: [PATCH 4/6] Revert DataConverter changes --- .../common/converter/DataConverter.java | 28 ------------------- 1 file changed, 28 deletions(-) diff --git a/temporal-sdk/src/main/java/io/temporal/common/converter/DataConverter.java b/temporal-sdk/src/main/java/io/temporal/common/converter/DataConverter.java index b8880579d3..44e4ffd050 100644 --- a/temporal-sdk/src/main/java/io/temporal/common/converter/DataConverter.java +++ b/temporal-sdk/src/main/java/io/temporal/common/converter/DataConverter.java @@ -38,14 +38,6 @@ static DataConverter getDefaultInstance() { return DefaultDataConverter.getDefaultInstance(); } - /** - * Creates a builder for customized {@code DataConverter} that behaves similar to the {@link - * #getDefaultInstance() default instance}. - */ - static Builder newBuilder() { - return new Builder(); - } - Optional toPayload(T value); T fromPayload(Payload payload, Class valueClass, Type valueType); @@ -114,24 +106,4 @@ static Object[] arrayFromPayloads( } return result; } - - class Builder { - - private PayloadConverter[] payloadConverterOverrides = {}; - - private Builder() {} - - /** - * Provide {@link PayloadConverter}s that would be merged with the default list of payload - * converters. - */ - public Builder setPayloadConverterOverrides(PayloadConverter... payloadConverterOverrides) { - this.payloadConverterOverrides = payloadConverterOverrides; - return this; - } - - public DataConverter build() { - return new DefaultDataConverter(true, payloadConverterOverrides); - } - } } From 979d0b08d5ab3441565971fee39ac4cd16d327f5 Mon Sep 17 00:00:00 2001 From: Mykola Rybak Date: Fri, 9 Apr 2021 12:32:13 +0300 Subject: [PATCH 5/6] Call DefaultDataConverter constructor from tests --- .../common/converter/ProtoPayloadConverterTest.java | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/temporal-sdk/src/test/java/io/temporal/common/converter/ProtoPayloadConverterTest.java b/temporal-sdk/src/test/java/io/temporal/common/converter/ProtoPayloadConverterTest.java index e1fab34b21..e427c2fffc 100644 --- a/temporal-sdk/src/test/java/io/temporal/common/converter/ProtoPayloadConverterTest.java +++ b/temporal-sdk/src/test/java/io/temporal/common/converter/ProtoPayloadConverterTest.java @@ -56,9 +56,7 @@ public void testCustomJson() { .configure(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS, true) .registerModule(new JavaTimeModule()); DataConverter converter = - DataConverter.newBuilder() - .setPayloadConverterOverrides(new JacksonJsonPayloadConverter(objectMapper)) - .build(); + new DefaultDataConverter(true, new JacksonJsonPayloadConverter(objectMapper)); TestPayload payload = new TestPayload(1L, Instant.now(), "myPayload"); Optional data = converter.toPayloads(payload); TestPayload converted = converter.fromPayloads(0, data, TestPayload.class, TestPayload.class); @@ -82,11 +80,10 @@ public void testProto() { @Test public void testCustomProto() { DataConverter converter = - DataConverter.newBuilder() - .setPayloadConverterOverrides( - new ProtobufJsonPayloadConverter( - JsonFormat.printer().printingEnumsAsInts(), JsonFormat.parser())) - .build(); + new DefaultDataConverter( + true, + new ProtobufJsonPayloadConverter( + JsonFormat.printer().printingEnumsAsInts(), JsonFormat.parser())); WorkflowExecution execution = WorkflowExecution.newBuilder() .setWorkflowId(UUID.randomUUID().toString()) From 663388da6c94bd7d4e12bc48bdae7d90ae948e82 Mon Sep 17 00:00:00 2001 From: Mykola Rybak Date: Sat, 10 Apr 2021 12:34:04 +0300 Subject: [PATCH 6/6] Switch to factory & builder-like method Also, replace by PayloadConverter.getEncodingType() instead of class. --- .../converter/DefaultDataConverter.java | 65 +++++++++---------- .../converter/ProtoPayloadConverterTest.java | 11 ++-- 2 files changed, 36 insertions(+), 40 deletions(-) diff --git a/temporal-sdk/src/main/java/io/temporal/common/converter/DefaultDataConverter.java b/temporal-sdk/src/main/java/io/temporal/common/converter/DefaultDataConverter.java index be7479d810..630f86712e 100644 --- a/temporal-sdk/src/main/java/io/temporal/common/converter/DefaultDataConverter.java +++ b/temporal-sdk/src/main/java/io/temporal/common/converter/DefaultDataConverter.java @@ -49,7 +49,7 @@ public class DefaultDataConverter implements DataConverter { }; private static final AtomicReference defaultDataConverterInstance = - new AtomicReference<>(new DefaultDataConverter(DEFAULT_PAYLOAD_CONVERTERS)); + new AtomicReference<>(newDefaultInstance()); private final Map converterMap = new ConcurrentHashMap<>(); @@ -71,49 +71,44 @@ public static void setDefaultDataConverter(DataConverter converter) { defaultDataConverterInstance.set(converter); } + /** + * Creates a new instance of {@code DefaultDataConverter} populated with the default list of + * payload converters. + */ + public static DefaultDataConverter newDefaultInstance() { + return new DefaultDataConverter(DEFAULT_PAYLOAD_CONVERTERS); + } + /** * Creates instance from ordered array of converters. When converting an object to payload the * array of converters is iterated from the beginning until one of the converters successfully * converts the value. */ public DefaultDataConverter(PayloadConverter... converters) { - setPayloadConverters(converters); + Collections.addAll(this.converters, converters); + updateConverterMap(); } /** - * Creates instance from ordered array of converters. Provided converters may be merged with the - * default list of converters if {@code mergeDefaultConverters} is {@code true}. - * - *

When converting an object to payload the array of converters is iterated from the beginning - * until one of the converters successfully converts the value. + * Modifies this {@code DefaultDataConverter} by overriding some of its {@link PayloadConverter}s. + * Every payload converter from {@code overrideConverters} either replaces existing payload + * converter with the same encoding type, or is added to the end of payload converters list. */ - public DefaultDataConverter(boolean mergeDefaultConverters, PayloadConverter... converters) { - if (!mergeDefaultConverters) { - setPayloadConverters(converters); - } else if (converters.length == 0) { - setPayloadConverters(DEFAULT_PAYLOAD_CONVERTERS); - } else { - List mergedConverters = new ArrayList<>(DEFAULT_PAYLOAD_CONVERTERS.length); - Collections.addAll(mergedConverters, DEFAULT_PAYLOAD_CONVERTERS); - - for (PayloadConverter newConverter : converters) { - Class newConverterClass = newConverter.getClass(); - boolean merged = false; - for (int i = 0; i < mergedConverters.size(); i++) { - PayloadConverter existingConverter = mergedConverters.get(i); - if (existingConverter.getClass().isAssignableFrom(newConverterClass)) { - mergedConverters.set(i, newConverter); - merged = true; - break; - } - } - if (!merged) { - mergedConverters.add(newConverter); - } + public DefaultDataConverter withPayloadConverterOverrides( + PayloadConverter... overrideConverters) { + for (PayloadConverter overrideConverter : overrideConverters) { + PayloadConverter existingConverter = converterMap.get(overrideConverter.getEncodingType()); + if (existingConverter != null) { + int existingConverterIndex = converters.indexOf(existingConverter); + converters.set(existingConverterIndex, overrideConverter); + } else { + converters.add(overrideConverter); } - - setPayloadConverters(mergedConverters.toArray(new PayloadConverter[0])); } + + updateConverterMap(); + + return this; } @Override @@ -189,10 +184,10 @@ public T fromPayloads( return fromPayload(content.get().getPayloads(index), parameterType, genericParameterType); } - private void setPayloadConverters(PayloadConverter... converters) { + private void updateConverterMap() { + converterMap.clear(); for (PayloadConverter converter : converters) { - this.converters.add(converter); - this.converterMap.put(converter.getEncodingType(), converter); + converterMap.put(converter.getEncodingType(), converter); } } } diff --git a/temporal-sdk/src/test/java/io/temporal/common/converter/ProtoPayloadConverterTest.java b/temporal-sdk/src/test/java/io/temporal/common/converter/ProtoPayloadConverterTest.java index e427c2fffc..f2cefc9e7d 100644 --- a/temporal-sdk/src/test/java/io/temporal/common/converter/ProtoPayloadConverterTest.java +++ b/temporal-sdk/src/test/java/io/temporal/common/converter/ProtoPayloadConverterTest.java @@ -56,7 +56,8 @@ public void testCustomJson() { .configure(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS, true) .registerModule(new JavaTimeModule()); DataConverter converter = - new DefaultDataConverter(true, new JacksonJsonPayloadConverter(objectMapper)); + DefaultDataConverter.newDefaultInstance() + .withPayloadConverterOverrides(new JacksonJsonPayloadConverter(objectMapper)); TestPayload payload = new TestPayload(1L, Instant.now(), "myPayload"); Optional data = converter.toPayloads(payload); TestPayload converted = converter.fromPayloads(0, data, TestPayload.class, TestPayload.class); @@ -80,10 +81,10 @@ public void testProto() { @Test public void testCustomProto() { DataConverter converter = - new DefaultDataConverter( - true, - new ProtobufJsonPayloadConverter( - JsonFormat.printer().printingEnumsAsInts(), JsonFormat.parser())); + DefaultDataConverter.newDefaultInstance() + .withPayloadConverterOverrides( + new ProtobufJsonPayloadConverter( + JsonFormat.printer().printingEnumsAsInts(), JsonFormat.parser())); WorkflowExecution execution = WorkflowExecution.newBuilder() .setWorkflowId(UUID.randomUUID().toString())