-
Notifications
You must be signed in to change notification settings - Fork 584
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
remove dataencoding and intro data_base64 in JSON #492
Conversation
Signed-off-by: Clemens Vasters <clemensv@microsoft.com>
Signed-off-by: Clemens Vasters <clemensv@microsoft.com>
json-format.md
Outdated
member name inside the JSON object MUST be `data`. For `Binary`, the value MUST | ||
be represented as a [JSON string][json-string] expression containing the | ||
[Base64][base64] encoded binary value, and the member name inside the JSON | ||
object MUST be `data_base64`. |
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.
While it's implied, it doesn't actually ban someone from having both data
and data_base64
in the JSON. Can we add something like:
Note, `data` and `data_base64` MUST NOT both appear at the same time within a CloudEvent serialization.
Someone may try to be "smart" and fill in both (when possible, like when it's a string) and that's a situation I think we need to avoid.
Also, could someone misinterpret the Binary
usage here? I believe you mean that the data type of data
is binary and you're not referring to the binary serialization of the CE. Perhaps:
For cases where data
is a Binary
value, it MUST be represented ....
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.
json-format.md
Outdated
@@ -92,9 +92,12 @@ respective CloudEvents type when the mapping rules are fulfilled. | |||
### 2.3. Mapping Data |
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.
Terrible nit, but I think this should be in section 3 now, because we've removed data
from the attributes, and section 2 is "Attributes".
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.
That would also put this text closer to the text in section 3.1 'Special Handling of "data"'.
Does this also need to update |
I'll address @duglin and @evankanderson concerns tomorrow. I agree with all comments made. |
Signed-off-by: Clemens Vasters <clemensv@microsoft.com>
I consolidated the |
LGTM |
Approved on the Sept 5 call |
Signed-off-by: Evan Anderson <evan.k.anderson@gmail.com>
* Fix up JSON type mappings Signed-off-by: Evan Anderson <evan.k.anderson@gmail.com> * Fix type mapping for AMQP Signed-off-by: Evan Anderson <evan.k.anderson@gmail.com> * Add `data` to AMQP. Signed-off-by: Evan Anderson <evan.k.anderson@gmail.com> * Add payload data to amqp-format.md Signed-off-by: Evan Anderson <evan.k.anderson@gmail.com> * Update AMQP data encoding per discussion in #492 Signed-off-by: Evan Anderson <evan.k.anderson@gmail.com>
Alternate to #491