Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
fix(json-format): Clarify data encoding in JSON format with a JSON datacontenttype #861
fix(json-format): Clarify data encoding in JSON format with a JSON datacontenttype #861
Changes from 6 commits
5ee78f7
59e395e
0cc5248
1bc294a
bc1b4a1
c7f366c
bd50bac
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means that for at least the C# implementation, the result of deserializing a JSON-formatted event with no
datacontenttype
to aCloudEvent
object will result in an object with theDataContentType
property set to"application/json"
exactly as if it had been set explicitly. There's no easy way of saying "Well, it wasn't really set, but it was implicitly application/json, so use that if it's reserialized." My guess is that other SDKs may be in the same position. Note that this will happen even if it's reserialized with a JSON event formatter - the act of "deserialize/reserialize" won't be a no-op here. (There are other cases where it might not be a no-op too, so I don't think this is fatal.)I'm okay with that - I just want to check that we're all on the same page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's how the Ruby SDK would behave as well. It's possible an SDK could keep the "original datacontenttype" and "effective datacontenttype" independently in the event object, but I don't think that's strictly necessary. It is important that the event semantics (i.e. the effective datacontenttype) do not change when the event is reserialized, even to a different format. It is not important that deserialize-reserialize to the same format is always a noop (although an SDK is free to try to do so anyway.) Do you think it's worth trying to state this for clarity? It seems like if we wanted to say something like that, it belongs in the main spec rather than in a format-specific document.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be quite useful to be specified somewhere, but I don't mind where. I was mostly checking that I wouldn't end up with a completely different implementation to everyone else.