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

ZonedDateTime field limits interoperability #29

Closed
dturanski opened this issue Jan 3, 2019 · 13 comments
Closed

ZonedDateTime field limits interoperability #29

dturanski opened this issue Jan 3, 2019 · 13 comments
Labels
invalid This doesn't seem right

Comments

@dturanski
Copy link

Currently the time field is a ZonedDateTime. This requires a custom deserializer ZonedDateTimeDeserializer implemented with jackson. Sending a CloudEvent via http then causes problems on the receiving end. IMHO It would be better to represent time with a more portable type, e.g., long.

@tenczar
Copy link

tenczar commented Jan 5, 2019

The CloudEvent spec defines the time field as a RFC 3339 timestamp. All of the sdks read/write this format because it contains enough information to fully identify what time is being represented. Using a long strips the timestamp of necessary timezone information. Why not use the sdk on both the producer and consumer?

@dturanski
Copy link
Author

dturanski commented Jan 5, 2019

I'm using the SDK on both sides, but the JSON serialization happens at another level, based on spring MVC, but uses GSON in this case. GSON does not know how to deserialize ZonedDateTime which results in an error. Is there a way to satisfy the spec without relying on Jackson? @dsyer - FYI

@matzew
Copy link
Member

matzew commented Jan 7, 2019

Hi, just returning from the holiday break. It seems to be a limitation on GSON itself:

google/gson#1059 (comment)

Looks like a custom TypeAdapter is needed ? 😞

@dturanski
Copy link
Author

dturanski commented Jan 7, 2019

I think it's more a limitation of JSON itself. Any Java deserializer will need custom configuration if, e.g., a type does not have a default constructor. Jackson requires ZonedDateTimeDeserializer and GSON requires the corresponding thing. This makes DefaultCloudEventImp less usable. Would it be possible to represent time as a more JSON friendly value object that validates RFC 3339? Another potential work-around is to receive the data as a Map and manually convert that to a CloudEvent.

@matzew
Copy link
Member

matzew commented Jan 7, 2019 via email

@fabiojose
Copy link
Contributor

@dturanski Any insights on that?

@dturanski
Copy link
Author

I would go with a String representation of the ISO-8601 standard. ZonedDateTime.now( ZoneOffset.UTC ).format( DateTimeFormatter.ISO_INSTANT ) This will make the date-time portable. It requires the application to parse it, e.g., ZonedDateTime.parse(zdtAsString).

@fabiojose
Copy link
Contributor

@dturanski Could you create a PR with this solution?

@dturanski
Copy link
Author

@fabiojose - I'll see what I can do

@dturanski
Copy link
Author

The PR addresses the date issue but more is needed for GSON compatibility in general. There are numerous Jackson annotations now which make CloudEventImpl less compatible with other json libraries. A fully portable object will need a zero arg constructor and flattened attributes to begin with.

@AceHack
Copy link

AceHack commented Apr 9, 2020

Any updates on this?

@dturanski
Copy link
Author

@fabiojose - Any thoughts on merging the PR? @AceHack - Is there a specific issue you are dealing with?

@slinkydeveloper
Copy link
Member

Hi @dturanski, I think i'm going to mark this issue as invalid since in sdk v2 we don't use anymore jackson annotations and the implementation of json ser/de is hugely simplified thanks to the Message api. You can easily implement a json serializer/deserializer for GSON similar to what is done for https://github.com/cloudevents/sdk-java/tree/master/formats/json-jackson.
We're also happy to accept a contribution for a json format using GSON!

@slinkydeveloper slinkydeveloper added the invalid This doesn't seem right label Apr 28, 2020
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

Successfully merging a pull request may close this issue.

6 participants