Restricting character set for attribute names #321

clemensv · 2018-10-02T10:03:25Z

At least week's meeting I was asked to create PR as a follow-up to my oral comment on #317

I appreciate the work done on that PR and the attempt to preserve case-sensitivity through an HTTP hop where the actors might perform case-folding for any reason, including semantically pointless aesthetics, due to HTTP being case-insensitive. That PR having to introduce complex rules is an issue with the main spec, and not something the HTTP mapping ought to fix, IMO.

This PR puts a constraint on the attribute name character set, only allowing lower-case letters and digits, with a leading lower-case letter. I am also including the rationale in the spec.

In effect, we are not taking a stance on case-sensitive or case-insensitive handling, but we rather eliminate the problem by only permitting lower-case letters for normatively named attributes.

While we're at it, I'm also adding a wire-footprint-greedy SHOULD clause regarding length.

To make up for the loss of the word separation by case, it would be great to have some kind of word separator available in addition to lower-case letters and digits, but dots and dashes and underscores clash with identifier rules in various languages/protocols. I don't think the result is all that terrible and the wire doesn't care about aesthetics.

If an SDK wants to "right case" identifiers for its clients, it's free to do so. A C# SDK ought to use PascalCase for strongly typed identifiers in order to comply with language conventions, and therefore "eventtime" ought to surface as "EventTime". The rules we make here must avoid that an implementer gets confused about what comes across and goes on the wire.

I have only modified the core spec for the purpose of the initial discussion. Specs that explicitly refer to attribute names will also have to change accordingly, of course, and I will amend the PR if we decide to go this route.

duglin · 2018-10-03T12:24:33Z

spec.md

+languages. Some of these treat metadata elements as case-sensitive while others
+do not, and a single CloudEvent might be routed via multiple hops that involve
+a mix of protocols, encodings, and runtimes. Therefore, this specification
+limits the available character set of all normatively named attributes such that


I wonder if we should drop "normatively named" because these rules would apply to extensions too, right?

@clemensv thoughts on this one?

Tapppi · 2018-10-03T13:12:34Z

I understand the need for clarification, and like the simplicity of this approach. However, I think this approach simplifies the spec by shifting the cost to usability. Practically speaking this will force keys to be lowercased everywhere, which will not play well with many programming languages and will feel seriously out of place. I worry about the inertia on adoption this approach might cause.

If an SDK wants to "right case" identifiers for its clients, it's free to do so. A C# SDK ought to use PascalCase for strongly typed identifiers in order to comply with language conventions, and therefore "eventtime" ought to surface as "EventTime". The rules we make here must avoid that an implementer gets confused about what comes across and goes on the wire.

I'm doubtful about the practicality of this approach and think it's a false promise. If the spec mandates all lowercase charactes with no word separator, each possible word would have to be known to the SDK for it to correctly apply word separation. That is impossible, because:

extensions
Imagine an extension property bucket companySpecificLabels with key goodTeamLabel. The SDK would need not only the extension spec but also any possible words present in keys before surfacing the event attributes. EventTime next to companyspecificlabels.goodteamlabel does not sound fun.
attributes from new versions of the spec would be lowercased but would then change casing when upgrading SDK, Duglin put it well in this comment:

Keep in mind that I don't think this is just about extensions, I think all CE properties will need to be encoded the same way as extensions. This is because to a v1.0 consumer, any new v1.1 properties will be viewed as extensions.

tenczar · 2018-10-04T15:45:06Z

I'm doubtful about the practicality of this approach and think it's a false promise. If the spec mandates all lowercase charactes with no word separator, each possible word would have to be known to the SDK for it to correctly apply word separation. That is impossible, because:

extensions
Imagine an extension property bucket companySpecificLabels with key goodTeamLabel. The SDK would need not only the extension spec but also any possible words present in keys before surfacing the event attributes. EventTime next to companyspecificlabels.goodteamlabel does not sound fun.

attributes from new versions of the spec would be lowercased but would then change casing when upgrading SDK, Duglin put it well in this comment:

Keep in mind that I don't think this is just about extensions, I think all CE properties will need to be encoded the same way as extensions. This is because to a v1.0 consumer, any new v1.1 properties will be viewed as extensions.

The point of this change is that on the wire the attributes are always lower case. Period.
For extensions the SDK will not even attempt to apply word separation, you can retrieve extensions by their all lower case name from whatever map-like struct the SDK holds them in.
We also don't need to care when promoting an extension because the wire format is still all lowercase. A new SDK version can provide a helper that can retrieve the extension with a name that matches the language's conventions, but that wrapper is going to grab the name from the all lower case representation.

Tapppi · 2018-10-04T17:39:17Z

The call was very informative. I realise #321 (comment) might not be a problem, other than losing any possibility of casing all identifiers, which is kind of the point anyway but affects some languages.

A new SDK version can provide a helper that can retrieve the extension with a name that matches the language's conventions, but that wrapper is going to grab the name from the all lower case representation.

As I said in the call, my concern is with dynamically typed languages, where an idiomatic way to access the event (dynamic property access, e.g. event.myEventExtension) wouldn't necessarily distinguish between core spec fields and extensions.

For extensions the SDK will not even attempt to apply word separation

To me extensions and core fields being idiomatically cased and not separated would've seemed a natural progression after extensions were moved out of a bag to the top-level, but I I jumped to conclusions 👍

cneijenhuis

I opened #317 (mainly to get a discussion started), but I also support this PR. Both have their tradeoffs, but they solve the same issue (in different ways).

cneijenhuis · 2018-10-08T14:48:38Z

spec.md

-Words that are acronyms are written in all-caps, e.g. "ID" and "URL".
+The CloudEvents specifications define mappings to various protocols and
+encodings, and the accompanying CloudEvents SDK targets various runtimes and
+languages. Some of these treat metadata elements as case-sensitive while others


I like that a rationale is provided. I think it could be extended to include a rationale for ASCII/alpha-numeric characters, and the length limit.

cneijenhuis · 2018-10-08T14:48:59Z

spec.md

+case-sensitivity issues or clashes with the permissible character set for
+identifiers in common languages are prevented.
+
+CloudEvents attribute names MUST consist of lower-case letter ('a' to 'z')


s/letter/letters

@clemensv please fix this one during your rebase - it should be non-controversial.

cneijenhuis · 2018-10-08T14:56:33Z

spec.md

-        "otherValue": 5
+    "eventid" : "A234-1234-1234",
+    "eventtime" : "2018-04-05T17:31:00Z",
+    "comexampleextension1" : "value",


Exactly 20 characters 😉

I'm wondering if, with the 20 character limit, we should "examplify" the use of a reverse URL. Even for this rather short TLD, 10 characters are taken. "commicrosoft" is 12 chars, "comserverless" is 13 chars...

I don't have a good alternative suggestion, though.

duglin · 2018-10-18T17:38:05Z

On today's call we discussed this topic and narrowed our choices down to this PR and #317. We then decided to start a vote between the two. Please add a comment to this PR to vote regardless of which PR you would like to vote for (if you did not vote already during today's call). The vote ends at the start of next week's call.

Voting members are: Adobe, Alibaba, Confluent, ControlBEAM, Fujitsu, Google, Huawei, IBM, Itau Unibanco, Microsoft, NAIC, NATS/Synadia, Nordstrom, Oracle, PayPal, Red Hat, SAP, Serverless, Solace, VMWare, Vlad Ionescu, Tapani Moilanen, Renato Gallis (Renato: do you work for Itau or are you independent?)

On today's call, the following companies voted:
MSFT: 321
PayPal: 321
Nordstrom: 321
Google: 321
Itau: 317

rperelma · 2018-10-18T17:49:53Z

Adobe: 321

Vlaaaaaaad · 2018-10-18T18:22:49Z

I'm voting for 321 too.

ColinSullivan1 · 2018-10-18T18:39:50Z

Synadia: 321

deissnerk · 2018-10-23T14:08:54Z

SAP: 321

duglin · 2018-10-24T19:56:14Z

IBM: 321

NaohiroTamura · 2018-10-25T09:09:08Z

Fujitsu: 321

duglin · 2018-10-29T13:25:21Z

Per the 10/25 call, finally tally is:
321: MSFT, PayPal, Nordstrom, Google, Vlad, Synadia, SAP, IBM, Fujitsu, Adobe
317: Itau,

So, #317 will be closed and @clemensv will rebase this PR - then we'll merge.

Signed-off-by: Clemens Vasters <clemensv@microsoft.com>

duglin · 2018-10-31T10:32:01Z

amqp-transport-binding.md

-All [CloudEvents][CE] attributes with exception of `contentType` and `data`
-MUST be individually mapped to and from the AMQP
+All [CloudEvents][CE] attributes with exception of `contenttype` and `data`
+are individually mapped to and from the AMQP


Why did you drop the "MUST"?

duglin · 2018-10-31T10:32:39Z

http-transport-binding.md

-All [CloudEvents][CE] attributes with exception of `contentType` and `data`
-MUST be individually mapped to and from distinct HTTP message headers,
+All [CloudEvents][CE] attributes with exception of `contenttype` and `data`
+are individually mapped to and from distinct HTTP message headers,


Signed-off-by: Clemens Vasters <clemensv@microsoft.com>

duglin · 2018-10-31T16:43:53Z

LGTM

I'd like to get at least one more set of eyes on this to make sure we didn't miss something before we merge it.

cneijenhuis

LGTM

(I still think we should provide better guidance/examples on how to provide descriptive extension names with a limit of 20 characters. The reverse url "wastes" too much characters quickly. But we can do that in a follow-on PR)

duglin · 2018-11-01T18:07:20Z

Approved on 11/1 call.

ottomata · 2019-02-06T20:30:55Z

I've just heard of CloudEvents, so am very new to this. At the Wikimedia Foundation, we are undergoing a similar (internal) process to standardize our event schemas. I'd like it if we could conform to some larger standard rather than one we make up for ourselves.

I appreciate the restriction of lower case here; often our events are imported into case-insensitive SQL based systems, and we also have issues with case insensitive HTTP headers too. Lowercasing everything helps. However, @clemensv, you wrote

To make up for the loss of the word separation by case, it would be great to have some kind of word separator available in addition to lower-case letters and digits, but dots and dashes and underscores clash with identifier rules in various languages/protocols.

Where do underscores clash? I'm sure this spec has to be compatible with many more languages and protocols than I'm familiar with, but off the top of my head I don't know of one. I understand that you are trying to just standardize 'wire' protocols here, but I doubt event data will only be used for async service communication. They are often also used for analytics purposes where they are imported into databases and need to be readable by humans. Underscores would make this much saner.

Signed-off-by: Christoph Neijenhuis <christoph.neijenhuis@commercetools.de>

Extensions follow attribute naming scheme introduced in #321

cloudevents attributes should be handled. See cloudevents/spec#321 for context.

* Fix issue with broker dropping messages with "No TTL Seen" * Treat the broker TTL attribute as case insensitive, in line with how cloudevents attributes should be handled. See cloudevents/spec#321 for context. * Remove unnecessary test change.

) * Fix issue with broker dropping messages with "No TTL Seen" * Treat the broker TTL attribute as case insensitive, in line with how cloudevents attributes should be handled. See cloudevents/spec#321 for context. * Remove unnecessary test change.

* Fix issue with broker dropping messages with "No TTL Seen" * Treat the broker TTL attribute as case insensitive, in line with how cloudevents attributes should be handled. See cloudevents/spec#321 for context. * Remove unnecessary test change.

clemensv force-pushed the attribute-names branch from ef2fafe to 54873d6 Compare October 2, 2018 10:08

clemensv changed the title ~~Attribute names~~ Restricting character set for attribute names Oct 2, 2018

clemensv mentioned this pull request Oct 2, 2018

Restrict character set (keep case-sensitivity/camelCase), dashes for HTTP case-insensitivity #317

Closed

rperelma mentioned this pull request Oct 2, 2018

Context attribute naming consistency #323

Closed

duglin reviewed Oct 3, 2018

View reviewed changes

cneijenhuis reviewed Oct 8, 2018

View reviewed changes

clemensv mentioned this pull request Oct 9, 2018

PR #321 but with underscores #327

Closed

This was referenced Oct 30, 2018

snake_case instead of camelCase #324

Closed

Extension names become case insensitive in binary encoding #177

Closed

Issue with "-" in extension names #265

Closed

clemensv force-pushed the attribute-names branch 2 times, most recently from 76ad9d1 to af03f0a Compare October 31, 2018 09:34

attribute names

678e215

Signed-off-by: Clemens Vasters <clemensv@microsoft.com>

clemensv force-pushed the attribute-names branch from af03f0a to 678e215 Compare October 31, 2018 09:44

duglin reviewed Oct 31, 2018

View reviewed changes

duglin added the v0.2 label Oct 31, 2018

Clemens Vasters added 2 commits October 31, 2018 17:31

dropping 'normatively named' restriction

ca8eed8

Signed-off-by: Clemens Vasters <clemensv@microsoft.com>

fixing merge bugs

19950a1

Signed-off-by: Clemens Vasters <clemensv@microsoft.com>

cneijenhuis reviewed Nov 1, 2018

View reviewed changes

duglin merged commit 044d6aa into cloudevents:master Nov 1, 2018

duglin mentioned this pull request Nov 30, 2018

v0.2 #358

Merged

cneijenhuis mentioned this pull request Jan 29, 2019

HTTP binary mode header mapping and header count limits #367

Closed

cneijenhuis added a commit to cneijenhuis/spec that referenced this pull request Mar 20, 2019

Extensions follow attribute naming scheme introduced in cloudevents#321

4401c6d

Signed-off-by: Christoph Neijenhuis <christoph.neijenhuis@commercetools.de>

cneijenhuis mentioned this pull request Mar 20, 2019

Extensions follow attribute naming scheme introduced in #321 #408

Merged

duglin pushed a commit that referenced this pull request Mar 21, 2019

Merge pull request #408 from cneijenhuis/fix-extension-attr-names

0ac2e2f

Extensions follow attribute naming scheme introduced in #321

mikehelmick added a commit to mikehelmick/eventing that referenced this pull request Jun 24, 2019

Treat the broker TTL attribute as case insensitive, in line with how

20b46cb

cloudevents attributes should be handled. See cloudevents/spec#321 for context.

mikehelmick added a commit to mikehelmick/eventing that referenced this pull request Jun 24, 2019

Treat the broker TTL attribute as case insensitive, in line with how

48bb3d3

cloudevents attributes should be handled. See cloudevents/spec#321 for context.

grantr mentioned this pull request Jun 25, 2019

Backport #1452 knative/eventing#1466

Merged

cneijenhuis mentioned this pull request Jul 19, 2019

Remove Map from allowed Context Attribute types #467

Merged

garyedwards mentioned this pull request Feb 15, 2021

Add a new RFC for cloud event data mapping drogue-iot/rfcs#3

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restricting character set for attribute names #321

Restricting character set for attribute names #321

clemensv commented Oct 2, 2018

duglin Oct 3, 2018

duglin Oct 30, 2018

Tapppi commented Oct 3, 2018

tenczar commented Oct 4, 2018

Tapppi commented Oct 4, 2018 •

edited

Loading

cneijenhuis left a comment

cneijenhuis Oct 8, 2018

cneijenhuis Oct 8, 2018

duglin Oct 30, 2018

cneijenhuis Oct 8, 2018

duglin commented Oct 18, 2018 •

edited

Loading

rperelma commented Oct 18, 2018

Vlaaaaaaad commented Oct 18, 2018

ColinSullivan1 commented Oct 18, 2018

deissnerk commented Oct 23, 2018

duglin commented Oct 24, 2018

NaohiroTamura commented Oct 25, 2018

duglin commented Oct 29, 2018

duglin Oct 31, 2018

duglin Oct 31, 2018

duglin commented Oct 31, 2018

cneijenhuis left a comment

duglin commented Nov 1, 2018

ottomata commented Feb 6, 2019

Restricting character set for attribute names #321

Restricting character set for attribute names #321

Conversation

clemensv commented Oct 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tapppi commented Oct 3, 2018

tenczar commented Oct 4, 2018

Tapppi commented Oct 4, 2018 • edited Loading

cneijenhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duglin commented Oct 18, 2018 • edited Loading

rperelma commented Oct 18, 2018

Vlaaaaaaad commented Oct 18, 2018

ColinSullivan1 commented Oct 18, 2018

deissnerk commented Oct 23, 2018

duglin commented Oct 24, 2018

NaohiroTamura commented Oct 25, 2018

duglin commented Oct 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duglin commented Oct 31, 2018

cneijenhuis left a comment

Choose a reason for hiding this comment

duglin commented Nov 1, 2018

ottomata commented Feb 6, 2019

Tapppi commented Oct 4, 2018 •

edited

Loading

duglin commented Oct 18, 2018 •

edited

Loading