[FEATURE] Document how to use Java 8 Time (JSR 310) objects

[dbwiddis]

Is your feature request related to a problem?

As part of extensibility work in opensearch-sdk-java we are leveraging the OpenSearch Java Client to make REST calls to the OpenSearch cluster from extensions. Our initial proof-of-concept feature is creating a detector as defined in the Anomaly Detection plugin, with work in this draft PR.

The class being modeled, AnomalyDetector, includes classes from the JSR-310 Time API, introduced with JDK-8, namely Instant and Duration.

The Java Client depends on Jackson's ObjectMapper class. Because Jackson 2.x versions still support Java 7 (and earlier) time functions, they do not include the JSR-310 API by default, and Java 8 support must be added by one or more external modules. Without this module included, attempts to serialize classes which use the JSR 310 API fail with this message:

com.fasterxml.jackson.databind.exc.InvalidDefinitionException:
Java 8 date/time type java.time.Instant not supported by default: add Module "com.fasterxml.jackson.datatype:jackson-datatype-jsr310" to enable

What solution would you like?

1. Add a dependency to the specified module in the above exception message, e.g.,

implementation "com.fasterxml.jackson.datatype:jackson-datatype-jsr310:2.13.4"

2. Add the module in the instantiation of ObjectMapper in the JacksonJsonpMapper constructor here, by adding an additional call to .registerModule(new JavaTimeModule()):
   

   opensearch-java/java-client/src/main/java/org/opensearch/client/json/jackson/JacksonJsonpMapper.java

   Lines 60 to 65 in 0ef1246

   	public JacksonJsonpMapper(ObjectMapper objectMapper, JsonFactory jsonFactory) {
   	this.provider = new JacksonJsonProvider(jsonFactory);
   	this.objectMapper = objectMapper
   	.configure(SerializationFeature.INDENT_OUTPUT, false)
   	.setSerializationInclusion(JsonInclude.Include.NON_NULL);
   	}

This is the easiest path forward from the existing code with the minimal amount of effort, to support just the Java 8 Time classes requested. However, the Jackson JSR-310 module also provides alternatives that may be considered to expand support:

To load all modules (letting users simply include the appropriate datatype implementation):

// Jackson 2.10 and later
ObjectMapper mapper = JsonMapper.builder()
    .findAndAddModules()
    .build();
// or, 2.x before 2.9
ObjectMapper mapper = new ObjectMapper();
mapper.findAndRegisterModules();

This comes with a caution about performance.

To selectively load just the Java 8 Time module (the request of this Issue):

// Jackson 2.10 and later:
ObjectMapper mapper = JsonMapper.builder()
    .addModule(new JavaTimeModule())
    .build();
// or, 2.x before 2.9
ObjectMapper mapper = new ObjectMapper();
mapper.registerModule(new JavaTimeModule());

What alternatives have you considered?

1. We are presently using a temporary workaround to down-cast our JsonpMapper to the JacksonJsonpMapper implementation, retrieving the object mapper and adding the module ourselves. While this unblocks our immediate work, it is not a scalable long term solution as it would have to be repeated for every REST call involving these classes.

((JacksonJsonpMapper) mapper).objectMapper().registerModule(new JavaTimeModule());

2. Jackson 3.x will support these classes by default, but there is no clear timeline of a 3.x release.

3. We could backport existing JDK-8 Time and Date API functions to JDK7 using the ThreeTen dependency. Forbidding extension developers from using JDK8 API seems an absurd proposal, however. Similarly, using Joda Time dependency (pre-dating even JSR-310) is not a realistic option.

Do you have any additional context?

This is discussed on multiple Stack Overflow posts such as this one.

The OpenSearch data-prepper project has already done this for its ObjectMapper implementations.

[wbeckler]

If it's a REST service, is there ever going to be a non-java client? If so, maybe the API interface is too specifically java-centric, and simpler time types would be better, such as long for microseconds.

…

On Sat, Oct 29, 2022, 4:36 PM Daniel Widdis ***@***.***> wrote: Is your feature request related to a problem? As part of extensibility work in opensearch-sdk-java <https://github.com/opensearch-project/opensearch-sdk-java> we are leveraging the OpenSearch Java Client to make REST calls to the OpenSearch cluster from extensions. Our initial proof-of-concept feature is creating a detector as defined in the Anomaly Detection plugin, with work in this draft PR <opensearch-project/anomaly-detection#692>. The class being modeled, AnomalyDetector <https://github.com/opensearch-project/anomaly-detection/blob/main/src/main/java/org/opensearch/ad/model/AnomalyDetector.java>, includes classes from the JSR-310 Time API <https://jcp.org/aboutJava/communityprocess/pfd/jsr310/JSR-310-guide.html>, introduced with JDK-8, namely Instant and Duration. The Java Client depends on Jackson's ObjectMapper <https://fasterxml.github.io/jackson-databind/javadoc/2.7/com/fasterxml/jackson/databind/ObjectMapper.html> class. Because Jackson 2.x versions still support Java 7 (and earlier) time functions, they do not include the JSR-310 API by default, and Java 8 support must be added by one or more external modules <https://github.com/FasterXML/jackson-modules-java8>. Without this module included, attempts to serialize classes which use the JSR 310 API fail with this message: com.fasterxml.jackson.databind.exc.InvalidDefinitionException: Java 8 date/time type java.time.Instant not supported by default: add Module "com.fasterxml.jackson.datatype:jackson-datatype-jsr310" to enable What solution would you like? 1. Add a dependency to the specified module in the above exception message, e.g., implementation "com.fasterxml.jackson.datatype:jackson-datatype-jsr310:2.13.4" 1. Add the module in the instantiation of ObjectMapper in the JacksonJsonpMapper constructor here, by adding an additional call to .registerModule(new JavaTimeModule()): https://github.com/opensearch-project/opensearch-java/blob/0ef1246d716482209b3cbb175b675c4dc16e86ae/java-client/src/main/java/org/opensearch/client/json/jackson/JacksonJsonpMapper.java#L60-L65 This is the easiest path forward from the existing code with the minimal amount of effort, to support just the Java 8 Time classes requested. However, the Jackson JSR-310 module <https://github.com/FasterXML/jackson-modules-java8/tree/2.14/datetime> also provides alternatives that may be considered to expand support: To load all modules (letting users simply include the appropriate datatype implementation): // Jackson 2.10 and later ObjectMapper mapper = JsonMapper.builder() .findAndAddModules() .build(); // or, 2.x before 2.9 ObjectMapper mapper = new ObjectMapper(); mapper.findAndRegisterModules(); This comes with a caution about performance. To selectively load just the Java 8 Time module (the request of this Issue): // Jackson 2.10 and later: ObjectMapper mapper = JsonMapper.builder() .addModule(new JavaTimeModule()) .build(); // or, 2.x before 2.9 ObjectMapper mapper = new ObjectMapper(); mapper.registerModule(new JavaTimeModule()); What alternatives have you considered? 1. We are presently using a temporary workaround to down-cast our JsonpMapper to the JacksonJsonpMapper implementation, retrieving the object mapper and adding the module ourselves. While this unblocks our immediate work, it is not a scalable long term solution as it would have to be repeated for every REST call involving these classes. ((JacksonJsonpMapper) mapper).objectMapper().registerModule(new JavaTimeModule()); 1. Jackson 3.x will support these classes by default, but there is no clear timeline of a 3.x release. 2. We could backport existing JDK-8 Time and Date API functions to JDK7 using the ThreeTen <https://www.threeten.org/> dependency. Forbidding extension developers from using JDK8 API seems an absurd proposal, however. Similarly, using Joda Time dependency (pre-dating even JSR-310) is not a realistic option. Do you have any additional context? This is discussed on multiple Stack Overflow posts such as this one <https://stackoverflow.com/questions/21384820/is-there-a-jackson-datatype-module-for-jdk8-java-time> . The OpenSearch data-prepper project has already done this for its ObjectMapper implementations <https://github.com/opensearch-project/data-prepper/search?q=JavaTimeModule> . — Reply to this email directly, view it on GitHub <#250>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA5PRLWDTRKLGILTAXPNOBDWFWDFBANCNFSM6AAAAAARR6INFE> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[dbwiddis]

If it's a REST service, is there ever going to be a non-java client? If so, maybe the API interface is too specifically java-centric, and simpler time types would be better, such as long for microseconds.

Good question, but the serialization turns it from Java into standard cross-platform representations. Jackson, by default, serializes Instant into ISO 8601 standard time, as can be seen in the testcases in #251 with the string "1970-01-01T00:00:00Z".

The bigger question is what Java class we want Java extension developers to use when deserializing ISO 8601 date/time values into an object. I don't see a compelling reason not to allow them to use classes from java.time.

Authors of extensions using other language SDKs will likely have their own deserialization methods.

[wbeckler]

Why wouldn't the dependency be added to the code that calls the client? I think when you use opensearch-java, you instantiate the deserializer outside the client. I'm super rusty on java, but I thought I'd ask.

Transport transport = new RestClientTransport(restClient, new JacksonJsonpMapper( /* put stuff here */ )); OpenSearchClient client = new OpenSearchClient(transport);

[dbwiddis]

Good point, @wbeckler and we could in fact pass a new ObjectMapper there.

This then falls back to the larger question of "why does Java client restrict users to the Java 7 classes?".

The above example would be great to include in the USER_GUIDE, if you'd prefer to keep the code as-is I can switch my PR to make that change.