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

tweak how we talk about data and datacontentencoding #484

Merged
merged 1 commit into from
Aug 16, 2019

Conversation

duglin
Copy link
Collaborator

@duglin duglin commented Aug 13, 2019

This might address #482 #471 #470

Signed-off-by: Doug Davis dug@us.ibm.com

@clemensv
Copy link
Contributor

I've closed #471 since this is ultimately a cleaner solution to the set of problems we're having.

This preserves the simplest case where data is just another bit of JSON inside the outer envelope for a structured JSON event without having to add a declaration.

Even though this PR abandons the abstract Map type altogether and we're effectively leaving structured data inside data without a more specific definition, we can probably take that risk until implementations turn up real issues, with a little extra tweak.

Generally, serializers are able to (de-)serialize a generic object graph for the respective runtime. An intermediary can therefore transcode the event if it reads it in one format and writes it in another and uses the respective serialization format's mapping to the runtime that is being used. Some of that might however be lossy.

If I build an intermediary in C# and read a structure based on an event format that knows about timestamps, my resulting graph will contain DateTime. If I serialize that back out as JSON, which doesn't have a timestamp concept, the respective field will be a string, meaning that I lose the type information.

I'm okay with a date turning into a string via intermediaries if I can be sure that the string has a predictable format. I want to avoid the situation where some implementations choose RFC1123 and others choose RFC3339 for dates, specifically in the chaotic world of JSON/JavaScript where RFC1123 dates are still common.

Therefore, we should augment the JSON event format with a statement that any date and time expressions, anywhere and even inside data, MUST follow RFC3339. That's the one simple type I'm really worried about with JSON because the format itself doesn't take a stance.

@duglin
Copy link
Collaborator Author

duglin commented Aug 14, 2019

For CloudEvents defined metadata (e.g. our time attribute) I agree with you that being very clear about the syntax of the time string, to ensure interop, is very important. Then, as you said, even if it's converted into a string people should still be able to reason about it and get interop even w/o a strongly typed protocol.

I'm less clear on the idea of mandating what's inside of data since that's not within our domain to control. I agree with you that if an event is transmitted across multiple transports and/or multiple formats that things can be lossy, but I view that as a problem for the implementation to deal with. I say this because anyone who wants to create one of those converters (e.g. gateways) will have this problem even without CloudEvents in the picture - meaning, we didn't introduce the issue - the converter did by coming into existence. So, it's not our problem to solve.

@clemensv
Copy link
Contributor

clemensv commented Aug 14, 2019

It may not be our problem to solve, but we could make interop a little easier by taking a stance. How about a SHOULD? :)

@duglin
Copy link
Collaborator Author

duglin commented Aug 14, 2019

To be clear... would you want to say that about just JSON or any format that uses structured?

@rperelma
Copy link
Contributor

I think this is a great PR, @duglin, thanks for putting it together!

@cneijenhuis
Copy link
Contributor

💯 👍

Probably should have posted this comment here: #481 (comment)

I still think we should give @jroper some credit, the idea is more-or-less the same as in #457

@duglin
Copy link
Collaborator Author

duglin commented Aug 15, 2019

I think all credit goes to @jroper and @clemensv :-) I'm just trying to repeat the words I've heard from them in a way that's acceptable to people.

Copy link
Contributor

@evankanderson evankanderson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Two small nits/questions.

@@ -237,7 +237,7 @@ content-type: application/cloudevents+json; charset=UTF-8
#### 3.2.2. Event Data Encoding

The chosen [event format](#14-event-formats) defines how all attributes,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove "," with a 2-clause "and"

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

language/runtime types at the producer and consumer ends, and never materialize
as a string.

- `Boolean` - a boolean value of "true" or "false".
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to add Boolean to the type system in this PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes I added it because someone may want to define an extension of type boolean. I pointed it out on the call and there was no objection.

Signed-off-by: Doug Davis <dug@us.ibm.com>
@duglin
Copy link
Collaborator Author

duglin commented Aug 15, 2019

fixed comma issue per @evankanderson - ready for another review/check

@cneijenhuis
Copy link
Contributor

I still think we can clarify what conditional attributes mean, but it isn't the main point of this PR. I'll submit a follow-on PR for #481 .

In general, LGTM 👍

@duglin
Copy link
Collaborator Author

duglin commented Aug 16, 2019

thanks @cneijenhuis merging.

Approved on the 8/15 call.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants