HTTP transport and JSON mapping#148
Conversation
|
|
||
| #### 3.2.4 Examples | ||
|
|
||
| This example shows a JSON event format encoded event, sent with a PUT request: |
There was a problem hiding this comment.
I agree the wording comes out a bit awkward, but it's indeed encoded using the JSON event format.
http-transport-binding.md
Outdated
| @@ -0,0 +1,338 @@ | |||
| # HTTP Transport Binding for Cloud Events | |||
|
|
|||
| Editor: Clemens Vasters (clemensv@microsoft.com) | |||
There was a problem hiding this comment.
I'd prefer to remove this section. Or add the same (c) for all of our docs, but IMO its unnecessary.
We should, at some point, discuss the versioning scheme we're going to use for these secondary documents. I'm leaning towards having all docs be at the same level, otherwise mixing-n-matching will be kind of awkward.
http-transport-binding.md
Outdated
|
|
||
| ### 1.3. Content Modes | ||
|
|
||
| The specification defines two content modes for transferring events: |
There was a problem hiding this comment.
"The" or "This"? I assume you mean "This http mapping doc", so then "This" would be appropriate. With "The" I wasn't sure if you were referring to this doc or the CloudEvents spec.
|
|
||
| ### 2.1. contentType Attribute | ||
|
|
||
| The `contentType` attribute is assumed to contain a [RFC2046][RFC2046] |
There was a problem hiding this comment.
While technically correct, this section feels a bit odd. Its so short and doesn't add anything beyond what the CE spec itself says. In fact, I don't think we should be repeating what the spec says since it could lead to things being out of sync at some point. For now I think we can remove this section
There was a problem hiding this comment.
I'm using the section as a reference point in this spec, and it's also putting a hard reference on the contentType attribute in the main spec. If there were a debate breaking out about contentType in the main spec potentially also accommodating some oder kind of reference, I'm saying here that this won't work for HTTP. So that reinforcement of the string REALLY being RFC2046 is deliberate, because I prescribe how to evaluate it.
There was a problem hiding this comment.
I'd like to keep the "contentType" section because it discusses one of the two properties that we do special handling for and with, even through I admit that it doesn't say anything substantially new for the property itself aside from aforementioned reinforcement of RFC2046.
There was a problem hiding this comment.
I'd prefer not to duplicate RFC references from the spec. Propose simply linking to https://github.com/cloudevents/spec/blob/master/spec.md#contenttype
| defaults to *binary* mode. | ||
|
|
||
| If a receiver detects the Cloud Events media type, but with an event format that | ||
| it cannot handle, for instance `application/cloudevents+avro`, it MAY still |
There was a problem hiding this comment.
I wonder what "handle" means in this case? Do we need to get into the processing model at all?
http-transport-binding.md
Outdated
|
|
||
| #### 3.1.3. Metadata Headers | ||
|
|
||
| All [Cloud Events][CE] attributes with exception of `contentType` and `data` |
There was a problem hiding this comment.
Didn't we already say this? Perhaps we need to reorder things so we only say it once.
There was a problem hiding this comment.
This is specifically saying that all attributes except those two become HTTP headers in this mode. So, it's not about contentType/data (which section 2 is largely about), but about the rest.
http-transport-binding.md
Outdated
|
|
||
| The naming convention for the HTTP header mapping of attributes is: | ||
|
|
||
| * Prefixed with "CE-" |
http-transport-binding.md
Outdated
| Examples: | ||
|
|
||
| * `eventTime` maps to `CE-EventTime` | ||
| * `eventId` maps to `CE-EventId` |
http-transport-binding.md
Outdated
| [JSON value][JSON-value] representation, compliant with the [JSON event | ||
| format][JSON-format] specification. | ||
|
|
||
| The JSON value MUST NOT contain any unencoded LF (ASCII 0x10) or CR (ASCII |
There was a problem hiding this comment.
Perhaps it would be better to point to the HTTP spec and let it define what value http header values can/should be - and how to encode/escape them?
There was a problem hiding this comment.
I do this here specifically to bring this spec into compliance with HTTP. We can't have CRLF inside of a value (and neither of them singly), inside an HTTP header.
There was a problem hiding this comment.
I've been doing a substantial rework of that rule now to cover UTF-8 content as well.
http-transport-binding.md
Outdated
| CE-CloudEventsVersion: "0.1" | ||
| CE-EventType: "com.example.someevent" | ||
| CE-EventTime: "2018-04-05T03:56:24Z" | ||
| CE-EventId: "1234-1234-1234" |
| Content-Type: application/json; charset=utf-8 | ||
| Content-Length: nnnn | ||
|
|
||
| { |
There was a problem hiding this comment.
I wonder if we should remove the {}'s otherwise people might thing that {} isn't part of the app data, and I believe in this example it is.
There was a problem hiding this comment.
That's why I broke those across multiple lines. I find the notation here somewhat better than adding fake data that people will then argue about.
|
|
||
| #### 3.2.3. Metadata Headers | ||
|
|
||
| Implementations MAY include the same HTTP headers as defined for the [binary |
There was a problem hiding this comment.
While I think its ok for people to do this, we should make it clear that doing so doesn't remove their requirement to include the same properties in the JSON/body.
There was a problem hiding this comment.
I'm clarifying that.
json-format.md
Outdated
|
|
||
| Such a representation uses the media type `application/cloudevents+json` | ||
|
|
||
| > The media type must be registered with IANA |
There was a problem hiding this comment.
We should open an issue to track this - then we can remove it from this doc.
json-format.md
Outdated
| "eventType" : "com.example.someevent", | ||
| "eventTypeVersion" : "1.0", | ||
| "source" : "/mycontext", | ||
| "eventId" : "A234-1234-1234", |
json-format.md
Outdated
| "eventType" : "com.example.someevent", | ||
| "eventTypeVersion" : "1.0", | ||
| "source" : "/mycontext", | ||
| "eventId" : "B234-1234-1234", |
|
Overall I like what's in here. I think the 2nd doc and the existing serialization.md docs should probably be merged - don't see much value in splitting the json serialization details across two docs. But, that merging and other syntax restructuring can be done later. LGTM to getting this in there so people can submit follow-on PR to tweak, but it gives us a good baseline to work with for now. |
|
DCO checker failing - need to sign all commits |
|
What makes the DCO checker required? |
|
We need some kind of legal check in-place, usually either CLAs or DCOs. Not sure we want to deal with CLAs |
|
DCO should be enabled
On Mon, Apr 9, 2018 at 9:20 AM Doug Davis ***@***.***> wrote:
We need some kind of legal check in-place, usually either CLAs or DCOs.
Not sure we want to deal with CLAs
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#148 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAD5IaYo944fLr28uuzwp9w59NUHZbQcks5tm24ngaJpZM4TJLZO>
.
--
Cheers,
Chris Aniszczyk
http://aniszczyk.org
+1 512 961 6719
|
|
@duglin I will also have an AMQP serialization doc to go along with the AMQP transport binding because I need a type system mapping for AMQP metadata (this will only define elements and not a body format). I also think we'll want to have BSON or MessagePack event formats eventually, so I think there ought to be format specific docs. |
|
|
||
| ##### 3.1.3.1 HTTP Header Names | ||
|
|
||
| The naming convention for the HTTP header mapping of attributes is: |
There was a problem hiding this comment.
Where do the set of "extension" attributes go? I assuming as HTTP headers, but we probably need to prefix them with something like CE-X-...
There was a problem hiding this comment.
Yes, it'll make sense to promote each extension to being its own field. CE-X- seems ok. I'l add that.
We don't need the TBD since each mapping/serialization doc we produce should state how extensions should appear. E.g. cloudevents#148 Closes: cloudevents#125 Signed-off-by: Doug Davis <dug@us.ibm.com>
|
might want to "s/Cloud Event/CloudEvent/g" |
|
Should this PR delete the serialization.md file? Or if not, it seems like it should reference the doc. |
Signed-off-by: Clemens Vasters <clemensv@microsoft.com>
Signed-off-by: Clemens Vasters <clemensv@microsoft.com>
Signed-off-by: Clemens Vasters <clemensv@microsoft.com>
Signed-off-by: Clemens Vasters <clemensv@microsoft.com>
Signed-off-by: Clemens Vasters <clemensv@microsoft.com>
Signed-off-by: Clemens Vasters <clemensv@microsoft.com>
Signed-off-by: Clemens Vasters <clemensv@microsoft.com>
Signed-off-by: Clemens Vasters <clemensv@microsoft.com>
Signed-off-by: Clemens Vasters <clemensv@microsoft.com>
Signed-off-by: Clemens Vasters <clemensv@microsoft.com>
http-transport-binding.md
Outdated
|
|
||
| #### 3.2.1. HTTP Content-Type | ||
|
|
||
| The [HTTP `Content-Type`][Content-Type] header is set to the media type of an |
There was a problem hiding this comment.
yes. will correct.
json-format.md
Outdated
| Such a representation uses the media type `application/cloudevents+json` | ||
|
|
||
| All REQUIRED and all not omitted OPTIONAL attributes in the given event | ||
| become members of the JSON object, with the respective JSON object member |
There was a problem hiding this comment.
We should probably mention that the order of the members in the resulting JSON is not relevant.
And, at some point (not as part of this PR) we should say whether its ok for people to include additional json properties in there outside of the extensions property. Normally I've seen people say its ok but then receivers just ignore unknown ones but when you have a well-defined location for extensions it becomes less clear if that's ok since you now have two spots for them. This could be a rat-hole discussion which is why I think a follow-on PR would be better. :-)
There was a problem hiding this comment.
agree on the "MUST become". will change.
Order is already addressed in https://tools.ietf.org/html/rfc7159#section-1 by stating that an object is an unordered collection, but I will add the hint.
The additional properties thing is probably a topic for the F2F. I'm in the "be lenient" camp and we do have versioning that helps with deconflicting collisions, but let's hear what others say.
|
None of my suggested edit/comments are blocking, so LGTM - and thanks! |
| The naming convention for the HTTP header mapping of attributes is: | ||
|
|
||
| * MUST be prefixed with "CE-" | ||
| * Each attribute name's first character MUST be capitalized |
There was a problem hiding this comment.
Since headers are case insensitive it seems strange to dictate their capitalization.
|
|
||
| The receiver of the event can distinguish between the two modes by inspecting | ||
| the `Content-Type` header value. If the value is prefixed with the CloudEvents | ||
| media type `application/cloudevents`, indicating the use of a known [event |
There was a problem hiding this comment.
I'm not happy with us inventing a new "application/cloudevents" content type. I strongly believe this should be an encoding format alone. E.g. application/json, text/xml, application/protobuf.
Failing to do so will make processing CloudEvents harder with off-the-shelf HTTP frameworks. For example, expressJS's built-in parsers will convert bodies to buffer instead of JS objects when the content-type is "application/cloudevents+json"
There was a problem hiding this comment.
I don't buy that argument. By that logic, we could not have anything but text and octet-stream, ever. We are factually defining a media type here and that media type has different renderings. if ExpressJS doesn't know +json, someone should teach it that.
There was a problem hiding this comment.
AI: Thomas - will open an issue to track this
| ### 1.3. Content Modes | ||
|
|
||
| This specification defines two content modes for transferring events: | ||
| *structured* and *binary*. Every compliant implementation SHOULD support both |
There was a problem hiding this comment.
Based on section 3, it sounds like every compliant receiver of events SHOULD support both. Unless I'm misinterpreting and anything that sends CloudEvents should be configurable to send either format?
There was a problem hiding this comment.
I wrote "implementation" specifically so that producers and consumers have a choice when they use said implementation. Senders will typically pick one of the two and Consumers should be prepared to understand both. Do you think that needs rephrasing?
There was a problem hiding this comment.
@inlined should we open an issue to revisit this? If you have some text in mind perhaps you could PR it? If you have some questions about the wording then others might as well
| The naming convention for the HTTP header mapping of attributes is: | ||
|
|
||
| * Each attribute name MUST be prefixed with "CE-" | ||
| * Each attribute name's first character MUST be capitalized |
There was a problem hiding this comment.
It doesn't make sense to dictate the capitalization of headers when HTTP defines header names as case insensitive.
There was a problem hiding this comment.
We can drop that. I just think it helps with consistency.
| Examples: | ||
|
|
||
| * `example` maps to `CE-X-Example` | ||
| * `testExtension` maps to `CE-X-TestExtension` |
There was a problem hiding this comment.
Many extensions will not be scalars. It may be worthwhile to define how extensions should be encoded into headers.
FWIW, with the labels proposal I was going to consider
CE-Label: key: value
CE-Label: secondkey: secondvalue
Though this will only work for a single map -> scalar
There was a problem hiding this comment.
The spec actually says that precisely https://github.com/cloudevents/spec/pull/148/files#diff-a60455371e295444f7369e2d9329c95bR188
The header value is actually a JSON value. You may not have caught that.
| CE-EventType: "com.example.someevent" | ||
| CE-EventTime: "2018-04-05T03:56:24Z" | ||
| CE-EventID: "1234-1234-1234" | ||
| CE-Source: "/mycontext/subcontext" |
There was a problem hiding this comment.
I know I've been confused in the past, but are we now allowing Source to be a URI-reference or still a full URI
There was a problem hiding this comment.
I believe URI references ought to be permissible.
There was a problem hiding this comment.
4/19 - Clemens will make it a full URI
There was a problem hiding this comment.
AI: Thomas - Consider a new PR to change spec to change this property from URI to URI-Reference
There was a problem hiding this comment.
Thomas will open a PR later today to change URI to URI-Reference before we create v0.1 release
| names in URIs [RFC3986 3.2.2][RFC3986], the JSON value MUST be encoded as | ||
| follows: | ||
|
|
||
| Non-printable ASCII characters and non-ASCII characters MUST first be encoded |
There was a problem hiding this comment.
Nit: to make this reversible we must also require percents to be re-encoded. Then we require percent decoding for all headers
As an example, Stackdriver logging unfortunately uses semantically different "/" (log repository structure) and "%2F" (client's resource structure).
There was a problem hiding this comment.
I likely need to point clearer to the percent encoding rules in RFC3986 here; I actually don't want to define that here at all but fully lean on prior art. This here is intended to be a summary.
There was a problem hiding this comment.
4/19 call - Clemens will tweak
|
|
||
| #### 3.2.2. Event Data Encoding | ||
|
|
||
| The chosen [event format](#14-event-formats) defines how all attributes, |
There was a problem hiding this comment.
I vehemently agree with this statement and wish it could be hoisted up even higher (e.g. this is not just a nuance of HTTP, but that CloudEvents will not support mixed-encoding).
This is the meat behind my argument that "content-type" is a requirement of every transport which we define in this spec, but is not itself a feature of a CloudEvent.
There was a problem hiding this comment.
Content-Type is required in the event, because it designates the event payload and not the event.
There was a problem hiding this comment.
It really is about encoding. Maybe it should be a separate part of the CloudEvent spec, since it is about how the event is formatted, rather than the semantic of the event itself. (This was why I originally thought it didn't belong in the spec, but can see that people really want to put some encoding semantics in the spec itself. I'm ok with leaving this interpretation for now, and then seeing how it plays out in implementations.)
|
@clemensv This is incredible work. Thank you for thinking through all of this. |
ultrasaurus
left a comment
There was a problem hiding this comment.
Please remove duplications from spec (and instead just reference the spec)
|
|
||
| ### 2.1. contentType Attribute | ||
|
|
||
| The `contentType` attribute is assumed to contain a [RFC2046][RFC2046] |
There was a problem hiding this comment.
I'd prefer not to duplicate RFC references from the spec. Propose simply linking to https://github.com/cloudevents/spec/blob/master/spec.md#contenttype
|
|
||
| ## 3. HTTP Message Mapping | ||
|
|
||
| The event binding is identical for both HTTP request and response messages. |
There was a problem hiding this comment.
Is this implying that the system that receives an event is expected to return a response in CloudEvents format? Interesting, yet feels a bit outside of the scope of what we've been talking about.
There was a problem hiding this comment.
I believe the intention is to say that this serialization is good for request and responses, not that both MUST be in our serialization formats at the same time.
| The event binding is identical for both HTTP request and response messages. | ||
|
|
||
| The content mode is chosen by the sender of the event, which is either the | ||
| requesting or the responding party. Gestures that might allow solicitation of |
There was a problem hiding this comment.
I paused on that one too :-) I think he meant "there may be data elsewhere" - but that's just a guess.
There was a problem hiding this comment.
We can reword that. With "gesture" I generally refer to an protocol interaction pattern. "Using a GET request to obtain an event held in a buffer". That's broader than "method".
| ### 2.2. data Attribute | ||
|
|
||
| The `data` attribute is assumed to contain opaque application data that is | ||
| encoded as declared by the `contentType` attribute. |
There was a problem hiding this comment.
This is confusing. How is the event meta-data encoded then? this seems to invite multiple kinds of encoding. contentType is pretty ambiguous in the specification -- there was prior discussion that it applied to meta-data attributes as well. The spec says "Describe the data encoding format" which it seems @clemensv interprets as the encoding of the data attribute (and others previously discussed as including all of the event data). Whatever is decided, please clarify in the spec.
There was a problem hiding this comment.
I agree we should clarify that in the core spec. The contentType field only makes sense to describe the data field, but for that it is required.
There was a problem hiding this comment.
@clemensv can you open an issue (or PR if you have text in mind) for this so we don't lose track of it?
|
|
||
| #### 3.2.2. Event Data Encoding | ||
|
|
||
| The chosen [event format](#14-event-formats) defines how all attributes, |
There was a problem hiding this comment.
It really is about encoding. Maybe it should be a separate part of the CloudEvent spec, since it is about how the event is formatted, rather than the semantic of the event itself. (This was why I originally thought it didn't belong in the spec, but can see that people really want to put some encoding semantics in the spec itself. I'm ok with leaving this interpretation for now, and then seeing how it plays out in implementations.)
Signed-off-by: Clemens Vasters <clemensv@microsoft.com>
Signed-off-by: Clemens Vasters <clemensv@microsoft.com>
We don't need the TBD since each mapping/serialization doc we produce should state how extensions should appear. E.g. cloudevents#148 Closes: cloudevents#125 Signed-off-by: Doug Davis <dug@us.ibm.com>
* HTTP binding and JSON event format Signed-off-by: Clemens Vasters <clemensv@microsoft.com> * HTTP header encoding changes and feedback fixes Signed-off-by: Clemens Vasters <clemensv@microsoft.com> * Cloud Events to CloudEvents Signed-off-by: Clemens Vasters <clemensv@microsoft.com> * Deleting serialization.md Signed-off-by: Clemens Vasters <clemensv@microsoft.com> * Adding extension header mapping Signed-off-by: Clemens Vasters <clemensv@microsoft.com> * TOC reference fix Signed-off-by: Clemens Vasters <clemensv@microsoft.com> * refer to JSON format Signed-off-by: Clemens Vasters <clemensv@microsoft.com> * change Makefile to remove serialization.md and add new specs Signed-off-by: Clemens Vasters <clemensv@microsoft.com> * fix RFC issues and branding Signed-off-by: Clemens Vasters <clemensv@microsoft.com> * fix RFC issues and branding Signed-off-by: Clemens Vasters <clemensv@microsoft.com> * added MUST clauses per Doug's comments on cloudevents#93 Signed-off-by: Clemens Vasters <clemensv@microsoft.com> * clarification for percent encoding with percent sign Signed-off-by: Clemens Vasters <clemensv@microsoft.com>
Refers to #93
These two documents are proposals for the HTTP transport mapping and for the JSON event format.
Expect some unprompted churn for the next couple of days while I add a further clarifications and examples and do fixups for links, etc.
Please ask each and all questions you may have here in the form of comments; all questions will help to make the spec more legible.
Closes: #93
Signed-off-by: clemensv clemensv@microsoft.com