Skip to content

Mapping rules for attributes/headers#255

Merged
duglin merged 2 commits into
cloudevents:masterfrom
clemensv:patch-2
Jul 5, 2018
Merged

Mapping rules for attributes/headers#255
duglin merged 2 commits into
cloudevents:masterfrom
clemensv:patch-2

Conversation

@clemensv
Copy link
Copy Markdown
Contributor

@clemensv clemensv commented Jun 26, 2018

Mapping rules for HTTP headers, including stating that extensions can do their own thing. I also added a rule for all 'Map' typed attributes, which will catch whatever we call 'application-properties' once they land.

If this meets everyone's expectations, I will do the equivalent edits for the other protocols.

On prefixing, I am landing on keeping CE- for everything from CloudEvents (including extensions) for HTTP so that we don't get in the way of similar HTTP constructs as HTTP headers aren't structured. For MQTT and AMQP it's not going to hurt to align to the same convention even though they have richer structure.

#Signed-off-by: clemensv@microsoft.com

Signed-off-by: Clemens Vasters <clemensv@microsoft.com>
Signed-off-by: clemensv@microsoft.com
@duglin
Copy link
Copy Markdown
Collaborator

duglin commented Jun 27, 2018

LGTM

@clemensv were you going to add similar text for the other protocols too?

@cneijenhuis
Copy link
Copy Markdown
Contributor

Since Extensions are a Map, they can contain further Maps. Slightly off-topic, I've been wondering if this is intentional, or if it would be better to lock them down.

If it is intentional, it may make sense to specify that nested sets should be fully flattened, e.g.

- key1
  - key2: foo
  - key3: bar

should be mapped to:

CE-attrib-key1-key2
CE-attrib-key1-key3

Otherwise, it is not immediately clear to me how I am supposed to map that, and for a larger structure it is easier to run into commonly enforced restrictions on HTTP Headers or attribute sizes.

@duglin
Copy link
Copy Markdown
Collaborator

duglin commented Jun 27, 2018

@cneijenhuis once #225 is merged extensions won't be a map

@clemensv
Copy link
Copy Markdown
Contributor Author

@duglin yes, the changes will be equivalent but I wanted to get a precedent agreed on before I go change all.

Comment thread http-transport-binding.md
("-"), and the name of the map entry key, e.g. "CE-attrib-key".

CloudEvents extensions that define their own attributes MAY define a
diverging mapping to HTTP headers for those attributes, especially if
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens in a situation where producer and consumer understand the extension but middleware does not?

producer -> middleware -> consumer

Does this PR allow the extension to define its HTTP header to be something other than a CE- prefixed extension header? I'm trying to understand how middleware would know to transmit the data the consumer. If we just send all the original headers to consumer this might work, but how do we disambiguate between some other headers that are not related to CloudEvents? What if the consumer expects the JSON form of CloudEvents?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@clemensv ^^

My 2 cents... I think the middleware would have to forward all headers on to the next hop.

re: what if the next hop expects JSON.... this is why the spec says everyone SHOULD support both so its less of an issue. However, I've been thinking more about @inlined's suggestion that we drop the Structured version and just support Binary HTTP - it's sounding more appealing each time I think about it. While I do still value in defining a JSON serialization of CloudEvents (if nothing else so we have interop for how they look when written in JSON), by forcing all CE properties into HTTP headers I think people would be less inclined to treat them as extensions of data and put a ton of non-routing specific info into them. It might help ensure we keep things simple. But still thinking about it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HTTP has clear rules about this for its intermediaries. Proxies must forward all RFC7231 headers and that includes all extensions. The same is true for MQTT and AMQP. For the transport bindings that makes the raised point implicitly clear for within a transport, which what they subject of this PR is.

I am not in favor of dropping the structured format, but I am sympathetic to making the header mapping a MUST rule for the structured format rather than a SHOULD, i.e. all metadata headers and extensions MUST be visible to middleware at all times.

The case where the structured format has a clear advantage is for multi-hop routing, when an event gets forwarded HTTP->AMQP->AMQP->[Avro file on disk]->HTTP. The structured format keeps everything together.

In the CloudEvents main spec we should indeed discuss routing/forwarding explicitly and make a hard rule that middleware MUST forward all elements of an event, including those that have been mapped onto transport constructs.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this can be done in a follow-on PR since that would be additive and not a change to this PR - plus that change is independent of the purpose of this PR too.

@zpencer
Copy link
Copy Markdown
Contributor

zpencer commented Jun 29, 2018

Can - characters appear as map keys?

@duglin
Copy link
Copy Markdown
Collaborator

duglin commented Jul 5, 2018

Approved on 7/5 call

@duglin duglin merged commit f31c3b3 into cloudevents:master Jul 5, 2018
@cneijenhuis
Copy link
Copy Markdown
Contributor

@duglin Based on the call yesterday, I'm not sure I got your reply #255 (comment) to #255 (comment)

We discussed this example of (roughly):

"extensions": {
  "ibm": {
    "sendSMS": true
  }
}

If I'm not mistaken, this is still valid after #225 is merged? Maybe the top-level "extensions" bag becomes unnecessary anymore, but the extension can still be a Map.

It is not clear to me how I should turn this into HTTP headers.

CE-extensions-ibm-sendSMS: true

Is that correct?

If yes, what happens if I turn "ibm": {...} into "ibm-extensions": {...}?

CE-extensions-ibm-extensions-sendSMS: true

Going back to JSON, I'll probably end up with "ibm": { "extensions": {...} } then.

@duglin
Copy link
Copy Markdown
Collaborator

duglin commented Jul 6, 2018

@cneijenhuis I just meant that if we remove the "bag" for extensions then we no longer have a special map for them. You are correct that even if extensions are top-level properties they are still part of a map, just the same map as spec-defined properties - so the naming rules that add a prefix like "extensions-" would go away. Yes there may still be another prefix, like "CE-" but its then the same thing as all other properties - extensions aren't special at that point. And, yes an extension itself could be of type "map" whether we have an "extensions" bag or not.

I think you and @zpencer are both raising good points about the use of "-" in names. If "-" will be used as a separator that denotes setting into an object/struct, then we may need to ban the use of "-" in property names.

@clemensv I'll open an issue to track this - want to do a PR to clarify?

@inlined
Copy link
Copy Markdown
Contributor

inlined commented Jul 25, 2018

@duglin I'm on a quest to remind you every time you mention something like

just meant that if we remove the "bag" for extensions then we no longer have a special map for them

That you should add the words "in JSON" where appropriate. Client libraries will still need separate string-based accessors for fields they don't understand (and thus can't have type definitions for). Strongly typed transports like proto will also need a separate escape hatch. I'm honestly tempted to push for re-adding the extensions bag to JSON just to stop this common misunderstanding that simpler JSON means this problem has gone away in any other case.

@duglin
Copy link
Copy Markdown
Collaborator

duglin commented Jul 25, 2018

Oh man, that's one heck of a threat! :-) ok, I'll try to remind myself to say "in JSON"

@inlined
Copy link
Copy Markdown
Contributor

inlined commented Jul 25, 2018 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants