Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Please Support JavaTimeModule #109

Closed
AceHack opened this issue Apr 9, 2020 · 19 comments
Closed

Please Support JavaTimeModule #109

AceHack opened this issue Apr 9, 2020 · 19 comments
Labels
invalid This doesn't seem right

Comments

@AceHack
Copy link

AceHack commented Apr 9, 2020

It would be great if this SDK used a Jackson object mapper configured like the following, in this way I don't believe you would need any special ZonedDateTime code, this should handle it and allow for greater flexibility.

        final var objectMapper = JsonMapper.builder()
            .addModule(new ParameterNamesModule(JsonCreator.Mode.DEFAULT))
            .addModule(new Jdk8Module())
            .addModule(new JavaTimeModule())
            .addModule(new GuavaModule())
            .addModule(new HppcModule())
            .addModule(new PCollectionsModule())
            .addModule(new EclipseCollectionsModule())
            .serializationInclusion(JsonInclude.Include.NON_NULL)
            .disable(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS)
            .disable(SerializationFeature.WRITE_DURATIONS_AS_TIMESTAMPS)
            .disable(SerializationFeature.WRITE_DATE_KEYS_AS_TIMESTAMPS)
            .disable(DeserializationFeature.ADJUST_DATES_TO_CONTEXT_TIME_ZONE)
            .build();

here are the references from my gradle.build.kts if that helps

dependencies {
    implementation("io.vertx:vertx-core:3.9.0")
    implementation("software.amazon.awssdk:s3:2.11.6")
    implementation("com.google.guava:guava:28.2-jre")
    implementation("com.carrotsearch:hppc:0.8.1")
    implementation("org.pcollections:pcollections:3.1.3")
    implementation("org.eclipse.collections:eclipse-collections:10.2.0")
    implementation("io.cloudevents:cloudevents-api:1.3.0")
    implementation("org.slf4j:slf4j-log4j12:1.7.30")
    implementation("org.slf4j:slf4j-api:1.7.30")
    implementation("com.fasterxml.jackson.core:jackson-core:2.10.3")
    implementation("com.fasterxml.jackson.core:jackson-annotations:2.10.3")
    implementation("com.fasterxml.jackson.core:jackson-databind:2.10.3")
    implementation("com.fasterxml.jackson.module:jackson-module-parameter-names:2.10.3")
    implementation("com.fasterxml.jackson.datatype:jackson-datatype-jsr310:2.10.3")
    implementation("com.fasterxml.jackson.datatype:jackson-datatype-jdk8:2.10.3")
    implementation("com.fasterxml.jackson.datatype:jackson-datatype-guava:2.10.3")
    implementation("com.fasterxml.jackson.datatype:jackson-datatype-hppc:2.10.3")
    implementation("com.fasterxml.jackson.datatype:jackson-datatype-pcollections:2.10.3")
    implementation("com.fasterxml.jackson.datatype:jackson-datatype-eclipse-collections:2.10.3")
    implementation("javax.validation:validation-api:2.0.1.Final")

    testImplementation("org.junit.jupiter:junit-jupiter-api:5.6.1")

    testRuntimeOnly("org.junit.jupiter:junit-jupiter-engine:5.6.1")
}

Here is a test I wrote:

    @Test
    void zonedDateTimeDoesNotHaveTimeone()
        throws JsonProcessingException {
        final var data = "{\"zonedDateTime\":\"2020-04-09T01:14:10.698998-04:00\"}";
        final var zonedDateTimeObject = objectMapper.readValue(data, ZonedDateTimeObject.class);
        final var zonedDateTimeString = objectMapper.writeValueAsString(zonedDateTimeObject);
        assertEquals(data, zonedDateTimeString);
    }
import java.time.ZonedDateTime;

public class ZonedDateTimeObject {
    public ZonedDateTime zonedDateTime;
}
@AceHack AceHack changed the title Please Support Java 8 OffsetDateTime Please Support JavaTimeModule Apr 9, 2020
@slinkydeveloper
Copy link
Member

This one is invalid, since now the encoding/decoding is done manually in `cloudevents-json-jackson' module

@slinkydeveloper slinkydeveloper added the invalid This doesn't seem right label Apr 28, 2020
@AceHack
Copy link
Author

AceHack commented Apr 28, 2020

Can you please explain in more detail how this is invalid? I don't understand.

@AceHack
Copy link
Author

AceHack commented Apr 28, 2020

Is there a cloudevents-json-jackson module this bug needs to be moved to?

@AceHack
Copy link
Author

AceHack commented Apr 28, 2020

The json jackson module still has the same issues. It should not use a custom ZonedDateTime Ser/DeSer and should properly handle the new JavaTimeModule stuff. This bug should not be closed.

https://github.com/cloudevents/sdk-java/tree/master/formats/json-jackson/src/main/java/io/cloudevents/format/json

@slinkydeveloper
Copy link
Member

slinkydeveloper commented Apr 28, 2020

It should not use a custom ZonedDateTime Ser/DeSer and should properly handle the new JavaTimeModule stuff

Can I ask the reason why the custom serde is not valid? The serialization/deserialization is handled correctly serializing/deserializing using RFC3339 (as underlined by the spec https://github.com/cloudevents/spec/blob/v1.0/spec.md#type-system). I've tried the JavaTimeModule but i found easier to just serialize/deserialize manually

@AceHack
Copy link
Author

AceHack commented Apr 28, 2020

@slinkydeveloper I'm not sure I understand what you mean "serialize/deserialize" manually? You are using Jackson so I need to be able to extend and put my own types and types from other libraries so they will serialize correctly like.

        .addModule(new JavaTimeModule())
        .addModule(new GuavaModule())
        .addModule(new HppcModule())
        .addModule(new PCollectionsModule())
        .addModule(new EclipseCollectionsModule())

Without JavaTimeModule I can't serialize the java 8 types related to time.
Instant, OffsetDateTime, LocalDateTime, etc...

Without the ability to add other modules like guava I can't serialize types from guava. I really need to be able to extend the Jackson configuration myself or else most POJOs will not be serializable.

@slinkydeveloper
Copy link
Member

If you're referring to the event payload, in the v2 of the sdk, the CloudEvent won't parse the event payload anymore, so you won't need to configure the integrated ObjectMapper because it's used just for the CloudEvent

@AceHack
Copy link
Author

AceHack commented Apr 30, 2020

So are you saying for unstructured i.e. application/json content type cloud events, the SDK v2 does not decode the body at all?

@slinkydeveloper
Copy link
Member

The sdk v2 decodes the event, but not the "data" field of the event: https://github.com/cloudevents/sdk-java/blob/master/api/src/main/java/io/cloudevents/CloudEvent.java#L45

@slinkydeveloper
Copy link
Member

you can take the byte array fro CloudEvent#getData and use jackson object mapper, jsonb, fast-json or whatever other library you like to go from data to your pojo

@AceHack
Copy link
Author

AceHack commented Apr 30, 2020

There is no data field in in unstructured/binary application/json cloud events the ”data” is the entire HTTP request body. Cloud event stuff is just in the headers in that case not the body. You are talking about structured application/cloudevents events.

https://github.com/cloudevents/spec/blob/master/http-protocol-binding.md#13-content-modes

https://github.com/cloudevents/spec/blob/master/spec.md#message

@slinkydeveloper
Copy link
Member

slinkydeveloper commented Apr 30, 2020

There is no data field in in unstructured/binary application/json cloud events the ”data” is the entire HTTP request body.

Ok maybe we misunderstood, but when you send the event in structured mode, encoded as json, there is a data field: https://github.com/cloudevents/spec/blob/v1.0/json-format.md#32-examples

Cloud event stuff is just in the headers in that case not the body

In binary mode, the body of the http envelope maps to the cloudevent data attribute, as explained https://github.com/cloudevents/spec/blob/v1.0/http-protocol-binding.md#312-event-data-encoding

@slinkydeveloper
Copy link
Member

With latest changes, in both cases (binary/structured), the data field is handled transparently and served as binary array, that you can handle as you want

@AceHack
Copy link
Author

AceHack commented Apr 30, 2020

You can see in this code it is not, this is from the example. Your JSON decoder is used to decode the binary message into a Map.class

https://github.com/cloudevents/sdk-java/tree/master/api

/* Unmarshals as CloudEvent instance */
CloudEvent<AttributesImpl, Map> event =
  Unmarshallers.binary(Map.class)
    .withHeaders(() -> httpHeaders)
    .withPayload(() -> myPayload)
    .unmarshal();

@slinkydeveloper
Copy link
Member

I'm sorry but these examples are still not updated, look at the tests 😄

@slinkydeveloper
Copy link
Member

I reorganized a bit the READMEs, check it out!

@slinkydeveloper
Copy link
Member

Can we close this now?

@slinkydeveloper
Copy link
Member

I'm going to close it, reopen if you need it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid This doesn't seem right
Projects
None yet
Development

No branches or pull requests

2 participants