Open jskeet opened 2 years ago
Largely lovely and appreciated observations. Some nits...
should "not omitted" actually be "omitted"?
>> o = { "a": "b", "c": null, "d": undefined }
<- Object { a: "b", c: null, d: undefined }
>> JSON.stringify(o)
<- "{\"a\":\"b\",\"c\":null}"
I don't recall the intention but I expect the existing spec language is meant to specify that behavior. As written, it could potentially leave a choice to implementers about how to handle attributes explicitly sent to undefined
(i.e. omitting them as per the stringify
behavior or including them with value null
).
4."document or object" - ❤️
Placed: yes, it's about the format, but I don't think it has to be about serialization. For example, it would be reasonable to have a JSON file that's expected to represent a CloudEvent, and talk about its meaning, without any SDK being involved. I wonder if "represented" would be unambiguous?
Omitted/not-omitted: thanks for the clarification. It's probably worth clarifying that :)
RE Placed - it's a good point. Perhaps "...extensions MUST be top-level..."
RE Placed - it's a good point. Perhaps "...extensions MUST be top-level..."
Works for me.
3.1.2 Note the spec also says "Unset attributes MAY be encoded to the JSON value of null.". So present w/null is valid.
rest LGTM
@jskeet status of this one?
@duglin: Waiting for me to have more time :)
This issue is stale because it has been open for 30 days with no
activity. Mark as fresh by updating e.g., adding the comment /remove-lifecycle stale
.
/remove-lifecycle stale
I should get back to this at some point...
This issue is stale because it has been open for 30 days with no
activity. Mark as fresh by updating e.g., adding the comment /remove-lifecycle stale
.
/remove-lifecycle stale
I do still want to do this - the recent discussions in #1186 demonstrate the importance of it.
This issue is stale because it has been open for 30 days with no
activity. Mark as fresh by updating e.g., adding the comment /remove-lifecycle stale
.
/remove-lifecycle stale
This issue is stale because it has been open for 30 days with no
activity. Mark as fresh by updating e.g., adding the comment /remove-lifecycle stale
.
/remove-lifecycle stale
This issue is stale because it has been open for 30 days with no
activity. Mark as fresh by updating e.g., adding the comment /remove-lifecycle stale
.
As promised, this is the result of trawling through the JSON format spec, and seeing which aspects should possibly be specified elsewhere, or generally clarified. There was actually less scope creep than I expected to find - partly as one example ("all events in the batch must have the same specversion") is bizarrely in the HTTP spec.
2. Attributes - duplication
Current text:
It's not clear how these two sentences are expected to say anything other than exactly the same thing. Proposal: move "MUST" into the first sentence, and delete the second sentence (which has singular/plural issues anyway).
2.2 Type system mapping - "primary vs secondary mappings"
Current text:
This was raised as a point of confusion in a discussion.
Proposal: delete. The intention is already covered elsewhere, as Doug demonstrated in the discussion. If we feel there is something JSON-specific that we want to include, we should make it much clearer.
2.2 Type system mapping - ambiguity in extension types
Current text:
Firstly, I suspect that "like when decoding Maps" harks back to a pre-1.0 era where "map" was a CloudEvent attribute type. It should probably be removed.
Secondly, it's noted that an extension attribute with a JSON type of "string" is ambiguous, but no information is provided about how to resolve this. This is primarily a practical issue for SDKs parsing JSON-format events, but it's a conceptual one as well. If a JSON object represents a CloudEvent, what is the type of an extension specified using a string value?
I can give the answer from a C# SDK perspective: it's inferred to be
string
unless there is pre-existing information to the contrary. (A user can provide a known set of extension attributes to the parsing operation.) I've no idea whether other SDKs do the same thing though. While I know we try to avoid providing too much SDK-specific guidance in formats (and I'd argue we should probably give more), in this particular case I think we could do so without going too deep. I'm happy to create a PR if that would be useful.3. Envelope - clarity/accuracy and typo
Current text:
Clearly "represeted" should be "represented" - but should "not omitted" actually be "omitted"? I thought we had agreed before that "null" wasn't really a "present" value anyway, conceptually. In other words, there should be no such thing as a "not omitted attribute with a value of null".
3.1.2 Payload deserialization - handling of non-string values
This was what #934 was about. I knew I'd got the impression from somewhere that strings were important...
Current text:
Given that this is about deserialization, it's not really clear what it means to say that the data member SHOULD be treated as an encoded string. When we're deserializing, we don't get to decide its type - the JSON does! The equivalent part of serialization is more reasonable, basically saying we shouldn't create JSON representations with non-JSON datacontenttype and non-string values.
4. JSON batch format: document or object
Current text:
I'd argue that really only a JSON object is specified. Indeed, RFC 7159 doesn't even describe what a JSON document is... but either way, it would be entirely reasonable to create a JSON object which had a JSON batch object as the value of one property.
And yes, this is very much like my objection to the use of document in the XML format.
4. JSON batch format: separate formats?
Current text:
We've got this in every format that supports batches. Rather than repeating this text everywhere, I think it would be better to specify it in a single place, for "event formats that support batch". A new document about event formats in general would be useful - that could also give more details of structured vs binary mode and the relationship between them and event formats.
4. JSON batch format: context for an empty batch
Current text:
It's useful to state that a batch may be empty, and to give an example, but we should probably remove any expectations as to when it'll be used.