-
Notifications
You must be signed in to change notification settings - Fork 612
Resolve quote issue for HTTP Binary Transport Encoding. #413
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
25cecd8
Atempting to resolve quote issue.
3c34416
Attempting to resolve quote issue.
305fb16
Merge branch 'quotes' of github.com:n3wscott/spec into quotes
cc305ab
Merge branch 'master' into quotes
a0bb8fe
try no quotes.
0538aa3
Quote json ints.:
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An encoding that isn't using quotes is no longer using JSON. "Unquoted JSON value" doesn't make sense if the only JSON value type for which quotes are used are strings and the quotes are significant for type inference. The convenient thing about using JSON here was that we get to borrow all of the type system and encoding and character set rules for when we need to distinguish between numbers and strings and complex types and we get automatic type inference so that generic frameworks can surface up the right types in spite of being unaware of specific rules of extensions.
If we choose to revert to "it's all just strings" and we would then still care about being able to represent numbers and complex types, we need to have a common model for how to restrict those strings, or it would be up to any individual extension to define such constraints locally within their scope.
For instance, we might keep "integer" and mandate that attributes of that type MUST be encoded, for instance, per https://tools.ietf.org/html/rfc7159.html#section-6 - or we abandon the type model altogether and have that encoding rule as a constraint next to the attribute(s) that need it.
For URI-reference and Timestamp we already have such constraints in place by referring to RFC3986 and RFC 3339 as a type-level constraint and it seems to make sense to do that just once and do the same for Integer.
What we're definitely losing with dropping the quotes is type inference for values of extensions that a generic CloudEvents framework doesn't know the specs for, because you can't easily tell whether 41751 is a number or a legitimate string (solution: it's my postal code and therefore I wanted it to be a string). I think that's a little sad, because we're sacrificing that capability for optics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is apples to oranges in regards to JSON encoding. JSON needs the
"because JSON is bytes. HTTP headers are strings. We are currently doing a hack to put bytes as sting to be interpreted as a string because it has quotes.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what the apples and the oranges are. We currently define the header value as to be JSON encoded:
"The value for each HTTP header is constructed from the respective attribute's JSON value representation"
The JSON encoding of a string is a string in quotes, again rendered as a string. The JSON encoding of an integer is a stringified integer, rendered as a string. You're adding "unquoted" into the definition above, but you can't just change the rules of JSON from the outside.
The consequence of dropping the quotes is simply that we're no longer encoding strings in compliance with JSON, i.e. we need to drop the JSON reference altogether and just say that everything is a string and then constrain the string content either at the type level (if we want to keep those) or at the attribute level.