Fix up JSON type mappings#496
Conversation
Signed-off-by: Evan Anderson <evan.k.anderson@gmail.com>
|
LGTM since this is more like a typo, can I get one more sanity check LGTM and then I'll merge it? |
|
I'm afraid we'll have to change it in the other formats as well... @evankanderson Do you want to do it in this PR as well? The change for JSON itself LGTM. |
|
I can look at the other formats later today. Do you want it in this PR or subsequent PRs. |
|
if it's today go ahead and do it in here. Otherwise I'll merge this one and we do roll a 2nd PR |
Signed-off-by: Evan Anderson <evan.k.anderson@gmail.com>
Signed-off-by: Evan Anderson <evan.k.anderson@gmail.com>
Signed-off-by: Evan Anderson <evan.k.anderson@gmail.com>
|
I added AMQP, but that one was a bigger change. I opened a separate PR for AVRO, and I can take the AMQP changes to a separate PR if needed. I'll probably have a PR for protobuf on Monday, I'm double-checking whether we have any internal use we're worried about breaking. |
| [amqp-describedtype]: | ||
| http://docs.oasis-open.org/amqp/core/v1.0/os/amqp-core-types-v1.0-os.html#doc-idp38080 | ||
| [amqp-data]: | ||
| http://docs.oasis-open.org/amqp/core/v1.0/os/amqp-core-messaging-v1.0-os.html#type-data |
There was a problem hiding this comment.
Seems right, but @clemensv do you agree with this reference?
|
I agree with the change, but given where we ended up with the general encoding strategy, I'd like for AMQP to track JSON even more closely and have binary The text for that can steal from #492 I was hesitant about doing that initially, but it's more symmetric now. |
| http://docs.oasis-open.org/amqp/core/v1.0/os/amqp-core-types-v1.0-os.html#section-encodings | ||
| [amqp-string]: | ||
| [amqp-boolean]: | ||
| http://docs.oasis-open.org/amqp/core/v1.0/os/amqp-core-types-v1.0-os.html#type-string |
There was a problem hiding this comment.
| or a `Map`. `Map` entry values are also `Any` typed. AMQP's type system natively | ||
| represents dynamic typing in its [type system encoding][type-system-encoding], | ||
| and therefore immediately allows for the required variant type representation. | ||
| The CloudEvents `data` payload SHALL be mapped to a single [AMQP |
There was a problem hiding this comment.
I propose this text for section 3:
Before encoding, the AMQP serializer MUST first determine the runtime data type of the data content.
If the implementation determines that the type of data is Binary, the value MUST be stored in the payload as an [amqp-data][amqp-data] section.
For any other type, the implementation MUST translate the data value into a AMQP type system value, and the value MUST be stored in an [amqp-value][amqp-value] section.
http://docs.oasis-open.org/amqp/core/v1.0/os/amqp-core-messaging-v1.0-os.html#type-amqp-value
There was a problem hiding this comment.
What happens, if a content type for a text-based format like application/json is used? How does the implementation determine, if the type of data is binary? Shouldn't every event with a specified content-type just go into the data section and the AMQP property content-type be used?
There was a problem hiding this comment.
I'm working on a better AMQP transport for the cloudevents/go-sdk now so this is timely! I propose a slight modification:
For structured mode messages: AMQP content-type == event content-type, AMQP payload is a single data section contain the formatted event payload.
For binary mode messages:
- If the event datacontenttype is non-empty and event data is binary then: AMQP content-type == event content-type, AMQP payload is single data section containing the unmodified binary event data. (This is all I have implemented so far)
- If event content-type is empty/missing then payload is an AMQP value mapped from the event value type (which might be binary!!) and AMQP content-type is empty.
This differs from Clemens for events with binary data and no datacontentype. I would map these into AMQP with a binary value body.
I think from the AMQP view-point this is more consistent:
- content-type not empty => data section encoded according to content-type.
- content-type empty => AMQP value section
There was a problem hiding this comment.
The above doesn't address events with non-empty datacontenttype and a non-binary data. I hope this is already illegal :)
There was a problem hiding this comment.
@alanconway I'm kind of lost here. I think that datacontenttype application/json will probably be the most common case. I just want to be sure that a piece of JSON can be transported across multiple hops without being touched by any type mapping or transcoding magic. 😉
Perhaps this is just a misunderstanding and application/json is regarded as binary in this context.
By the way, I'm looking forward to the improved AMQP support in the go sdk. AFAIK the AMQP client it currently uses, does not support AMQP values, though. 🤔
There was a problem hiding this comment.
I think Clemens explicitly wants AMQP application/json to be transcoded to AMQP types (and possibly converted back at the other end), but I'm not sure. I tend to agree with @deissnerk, so I'm writing the spec that way.
This entire area feels a bit weak as well (including the JSON source) -- it says "MUST determine the runtime data type of the data content" and talks about "Binary" type, but there's no introduction of what the other types are, since we banished the Any type for attributes and decided that our type system only covers attributes.
If my content is:
iVBORw0KGgoAAAANSUhEUgAAACAAAAAgCAYAAABzenr0AAAABGdBTUEAALGPC/xhBQAAACBjSFJNAAB6JgAAgIQAAPoAAACA6AAAdTAAAOpgAAA6mAAAF3CculE8AAAABmJLR0QAAAAAAAD5Q7t/AAAACXBIWXMAAC4jAAAuIwF4pT92AAAAB3RJTUUH4wkKETMqDJ0kOAAAA65JREFUWMPt1luIlWUUBuBnZraVye4waY2pUV4YWWjszsaQRTI37qCiG2ssIrAyK0uisovqIjCqmwg6GUFSSReSuwNaGp1IpPYMFlMUHahGi47OnkkddaaLb+3mnz0HZyDqZhZs5vvn/w7v/653vetjIibif4668UwulivV4XE4C/PQiB50YDt2or9UyP+7AOLww3ElbsV8HJGZcgBf4hk8hy44FJAxAYjDj8Rq3B7jrjjwFxyFOZgWQF7CnfFuVBC5cdB+I1bFmo14DO3Yg8MCwHJcjVZUcAf2jbZ//RgzMB8r46AXcB3exe5SId9bKuS7UQ4ADwcL16Kl5iPGByCz8CrMxGe4H3/UUhvjvXgUb2MKrpF0MyKIQ6YAk3FBjDfgu9rDa2K3pIEWLMJ67EK5WK5sxTcyVTJIhBmUuaD7APpioyvwIt6JOQ3xdX1SnvtLhXx1jwK24JjM9v34Fk/gaXSXCvkEIHPwybgczZKiu9GGdfg8AJ+Ay3AxTozDO/AqPozn43Ez8pHmmTgPs7Afj+M+7KmLw+uwGA/h9BpmuiTRbcBCrMHZhuqngufxIH6tedeAU3E3lgSzy7G2qoFL8RSm4we8IdV4E36Uym4B1mJ2gNoW+Tw2AM3GLZJH3Iaeap6L5crBYGlFgFkSDG2qK5Yr0/AKLsJHsUlb5KwajTHnEnwtmcwmSfX1kbrVwdR+3BBpkwFR3etMvCnZ+bL6ENj5+D0oKhvq5YtCF3/h3sj33lIhr1TI9wUTq/BWCLM1mPgnMvt14GNMwoIcLoxFW6VmMlyJNceCT7C5+s9iuXIalgYLfVLlwBmYga8MjV50xrgph6nxsCsorY0GSfmkTpd1lDkG7DkbU3C04aM+8647h9/iYXowUevdB0VnC7CTpfIkqf29AFmNupj/5wgAZkk+ATtyUu1ej3Pj936xXKlNQ1v8nYdzDJjRNql8s1EnCXgQm5l2viKY68TmnCSc7VKe10j12V4sV7JVsFkS2mw8gJ8kYzpYKuT3GCUy6p8hVc9N8bwe7VUjapFMpAnf43WDfeBJLMMj0iXkC7yMT6Wyq71X7MMHka6F0u1pMeaGBrZI3bIz64RFyQnnGuqESyVzWom7pBomKb+/5vD6YKg5GCsF9VVgG3GP5CdDesEpBveCHunSsS6+tkFyxNbQS6PBAqzu+XPscxKelTrkDrwmteoKqdxH6oaTDHTDXhljijkNUqfLDwNArNsplefUOLALfWO9rE7ERPxn8TfC2R2lypC+yAAAACV0RVh0ZGF0ZTpjcmVhdGUAMjAxOS0wOS0xMFQxNzo1MTo0Mi0wNDowMLnwAFoAAAAldEVYdGRhdGU6bW9kaWZ5ADIwMTktMDktMTBUMTc6NTE6NDItMDQ6MDDIrbjmAAAAAElFTkSuQmCC
(the CloudEvents PNG logo, base64 encoded), does that count as a String or Binary? Does that change if datacontenttype is set?
Signed-off-by: Evan Anderson <evan.k.anderson@gmail.com>
|
IMO an event with datacontenttype=application/json is encoded as a binary mode AMQP messages like this: The spec defines an AMQP event format, a peer to the JSON event format - don't confuse event format with datacontentype. If an event has no datacontenttype, the AMQP binding maps the event data to an AMQP value (the default format for this binding): The binding does not spontaneously re-format data: a datacontenttype=json event sent over AMQP remains datacontenttype=json. application/json is not special as a datacontenttype, it is just like application/xml or any other. For AMQP-value encoded messages the client library will map to whatever is it's representation for a datacontenttype="" event - probably a programming language object of suitable type. This is no different from any other binding. The AMQP event format is an independent peer of the JSON format - we don't explicitly name it "application/cloudevents+amqp" because it's only ever used with the AMQP binding, but it is an independent format nonetheless. |
|
Approved in the 9/12 call. |
I noticed while reviewing #492 that the attribute types in the JSON mapping don't line up with the types in spec.md anymore.