Clarification of json 'data' section docs disparity

[rdallman]

Hello. First, thank you for this project, this is a great thing for FaaS at large :)

In implementing the spec, there has been some confusion around 2 particular sections from the docs, I am hoping to have some light shed on which is the 'right' way and then to cement that in the documentation itself. I did not see any open issues or PRs at the moment that resolve this, but may have missed one.

Early in json-format.md section 2.3 https://github.com/cloudevents/spec/blob/master/json-format.md#23-mapping-object-typed-attributes notes (ATOW):

The CloudEvents data attribute is Object-typed, meaning that it either holds a String, or a Binary value, or a Map. Map entry values are also Object typed.

If an implementation determines that the actual type of an Object is a String, the value MUST be represented as JSON string expression; for Binary, the value MUST represented as JSON string expression containing the Base64 encoded binary value; for Map, the value MUST be represented as a JSON object expression, whereby the index fields become member names and the associated values become the respective member's value.

where later on it goes on in 3.1 https://github.com/cloudevents/spec/blob/master/json-format.md#31-special-handling-of-the-data-attribute

The mapping of the Object-typed data attribute follows the rules laid out in Section 2.3., with one additional rule:

If an implementation determines that the type of the data attribute is Binary or String, it MUST inspect the contentType attribute to determine whether it is indicated that the data value contains JSON data.

If the contentType value is "application/json", or any media type with a structured +json suffix, the implementation MUST translate the data attribute value into a JSON value, and set the data member of the envelope JSON object to this JSON value.

Unlike all other attributes, for which value types are restricted to strings per the type-system mapping, the resulting data member JSON value is unrestricted, and MAY also contain numeric and logical JSON types.

The disparity being that in 2.3 the spec seems to define that the "data" section can only contain a json string type (application/xml, binary, et al) or a json map type, where in 3.1 the type domain seems to broaden to any json value type (e.g. false, null, ["list"], map, etc.).

The main issue here being that, if we implement according to 2.3 this would seem to indicate that sending in {"contentType":"application/json", "data":["list"]} would be invalid (as data could only be a map or a string), and we should return an error upon receiving an event with this data section. 3.1 seems much more intuitive (and in implementation, it's quite smooth, for ahem languages without generics ;), however seems in conflict with the type system defined in 2.3. Additionally, it seems wrong to reject events that provide a valid json value in the data section (and 3.1 seems happy with this). Hopefully, this is just a matter of clarifying and this isn't just me being wrong on the internet again.

A possible solution: at present, the grammar seems somewhat ambiguous with the Map type being rigidly defined as a json map, that could also be an object. In 2.3 the wording is around String, Binary, or Map where if 3.1 is the correct way to implement things, this could easily say String, Binary or Object instead, and since an Object can have a Map production this makes complete sense -- as opposed to a Map which doesn't really have the entire Object production, as Object is defined, e.g. a Map can't have a value of false -- even though 2.3 seems to imply a Map could be an Object. To put in simpler terms, it seems like we should have things the other way around and say it's a String, Binary or Object where Object may be a Map, vs. the current other way around. Perhaps, I am way off base, too :)

Thanks again!

[duglin]

@clemensv you ok if I assign this one to you?

[duglin]

ping @clemensv

[duglin]

re-pinging @clemensv

[duglin]

ping @clemensv

[duglin]

@rdallman would it help if section 2.1 said that data's value must be valid "stand-alone" JSON? Meaning, that "data" is meant to just be a holder of the real event's data. Then we don't need to say its only a string, binary or map.

[rdallman]

@duglin something along those lines would make sense, yeah. according to the defined grammar, I think it would be of type 'Any', which produces a JSON value (per https://github.com/cloudevents/spec/blob/master/json-format.md#2-attributes). Where JSON could be of type 'String', 'Binary', or 'Any', as defined in the cloud events type mapping specification. At least, that's my interpretation as it follows from 3.1 (discussing a 'json value', or a binary or string that must be inspected if json is the content type). thanks for your reply.

[clemensv]

Goal for for data to hold any compliant JSON, including the example you cite, @rdallman. Section 3.1 spells out the exceptions from 2.3. When contenttype is known to reflect JSON (which in loose interpretation could also be when it's absent), all JSON is allowed in data.

[duglin]

@rdallman thoughts?

[rdallman]

That ended up being my intuition, that data can hold any JSON, but I maintain that it is not clear from the spec that is the case. specifically in 2.3:

The CloudEvents data attribute is Object-typed, meaning that it either holds a String, or a Binary value, or a Map. Map entry values are also Object typed.

this implies a JSON value (eg an array) is not valid (unless it's inside of a string?), as an object is a value but a value is not an object.

I'm not 100% sure, but changing this to read:

The CloudEvents data attribute is Object-typed, meaning that it either holds a String, or a Binary value, or Any JSON value.

would make this more clear, if that definition is still correct wrt intentions for non-json things (when json is not specified), in which case it should maybe specify that it can be an Object or if the content type is json, it can be any json value.

thank you for following up, I hope this feedback helps clarify the spec :)

[duglin]

@clemensv thought on the suggested edit?

[clemensv]

"JSON object" should indeed refer to JSON value in order to permit arrays. So, yes, we should change that.

[duglin]

The current doc says:

The CloudEvents `data` attribute is `Any`-typed, meaning that it holds a value
of any valid type. `Map` entry values are also `Any` typed.

Is a change still needed? Seems like this might no longer be an issue. @clemensv @rdallman

[clemensv]

if "Object" was the source of confusion (it was supposed to imply JSON's any-type: value) then we should be good now.

[duglin]

@rdallman you agree? Can we close this issue now?

[duglin]

On today's call we believed that this issue is no longer a concern given the current wording in the spec. Before we close it, @rdallman can you confirm that you're ok with the current text?

[rdallman]

spec.md LGTM now, the type is very clear. thank you!