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

Changed datacontentencoding to dataencoding #491

Closed
wants to merge 1 commit into from

Conversation

clemensv
Copy link
Contributor

Per discussion on the call last week.

This PR renames datacontentencoding to dataencoding and tightens the rules around it since there have been several misunderstandings about what the attribute is meant to help with, see #486 #481 #477

The attribute is ONLY for structured mode and ONLY for when a value cannot be correctly expressed in the chosen event format's type system and needs further encoding. The specific concern is making binary fit into JSON.

The attribute does not map to transport-level content encoding concerns like compression or signatures or encryption or the like, specifically not to Content-Transfer-Encoding fields of SMTP or the Content-Encoding or Transfer-Encoding fields of HTTP. This is a CloudEvents specific construct for the aforementioned purpose. That does not at all preclude that one uses message-level compression in HTTP for transferring a CloudEvent; it's simply an orthogonal concern.

Signed-off-by: Clemens Vasters clemensv@microsoft.com

Signed-off-by: Clemens Vasters <clemensv@microsoft.com>
@duglin
Copy link
Collaborator

duglin commented Aug 27, 2019

@clemensv thanks!

@evankanderson I think this overlaps a bit with your PR: #488 and, if adopted, might mean a few of your edits could go away, no?

@duglin
Copy link
Collaborator

duglin commented Aug 27, 2019

While I think it could be a bit more "in your face" about only using it for structured mode, we can tweak the wording later if needed.

LGTM

@duglin duglin added the v1.0 label Aug 27, 2019
@evankanderson
Copy link
Contributor

evankanderson commented Aug 27, 2019

This might have overlap with #488.

Framing question -- is there any reason for dataencoding to be a CloudEvents attribute, rather than a field in the JSON event format envelope, like we defined data to be?

@evankanderson
Copy link
Contributor

Rationale for moving this to the json-format.md file:

AMQP, AVRO, and protobuf all support binary data encoding without needing the dataencoding attribute. This issue only occurs for JSON because JSON attempts to serialize arbitrary data within a text-format envelope, and so runs into escaping, etc issues.

We would have the same problem with a hypothetical XML or YAML encoding, but we might choose different mechanisms to deal with the encoding issues; for example:

<data encoding="Base64">...</data>

(rather than <encoding>Base64</encoding> or <encoding value="Base64"/> as we might normally encode an attribute.)

  data: !binary |
    ....

@clemensv
Copy link
Contributor Author

@evankanderson I agree on the motivation to push that off into the JSON spec based on what we have right now. Your argument that XML and YAML might chose different hints also makes sense.

The lack of such annotations in JSON makes it a little iffy to express the encoding mode as an "encoding-private" attribute, though.

An alternate proposal for JSON I can think of is to simply call, in JSON encoding, a field that holds regular string data or inline JSON (which is factually the same thing) ´data´ and call the alternate field that holds base64-encoded data ´base64data´ - and kill dataencoding altogether.

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.

3 participants