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.
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
Define several tricky cases for encoding based on spec. #456
Define several tricky cases for encoding based on spec. #456
Changes from 2 commits
bfabde3
151dc4b
87b92a3
cb99096
71637f5
d118419
bd0bf97
900a2a5
b13ca64
cc5a368
219ddb9
234510c
8af2339
ef0712a
85fbd92
ce1b180
9cd4c3a
e85ccc2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
remove trailing
,
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.
done.
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.
Done.
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.
@clemensv is there an MQTT equivalent property for datacontentencoding?
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.
Searching for "encoding" in https://docs.oasis-open.org/mqtt/mqtt/v5.0/os/mqtt-v5.0-os.html produces 25 hits, all of which cover UTF-8 or property encoding, rather than payload encoding.
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.
@duglin No, there is no equivalent in MQTT. For MQTT 3.1.x, you even need to know the datacontenttype apriori as a convention on the topic. This particular field missing is not an issue, though, because the body is always a byte sequence.
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.
you show
17
as a string here but as a number in testcases/extensions-map.http - should they match?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.
Integers and Strings have this problem for unknown extension schemas, I believe.
I.e. given an HTTP header:
Does this render as:
or
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.
yes that's true - I think it'll show up as a string though since I think unknown attributes are kept as string. But, was that the point of the test? I only commented on it because I thought you were trying to be consistent - if not then never mind.
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.
remove trailing
,
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.
If #478 goes thru then this will need to be changed
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.
remove trailing
,
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.
Do we need to say something in our spec about doing a percent-decoding?
I want to say 'no', but some kind of "heads-up" might be good, not from a
strcmp
perspective, but rather to remind people that per RFC3986 someone might URL-encoding them even if they're not meant to be deference-able. Perhaps something in the primer?what do people think?
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.
This is a tricky case, because it is a valid URL, but if you percent-decode it, it is not a valid URL, but instead a
hier-part
, which requires ascheme ":"
prefix to be a validURI
production.Clarified the comment.
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.
Hmm, it looks like I didn't look at this quite closely enough (but may perhaps need to clarify the HTTP spec as well):
https://github.com/cloudevents/spec/blob/master/http-transport-binding.md#3132-http-header-values
However, the characters I encoded are not in the set (non-printable ASCII characters and non-ASCII characters), so it's not clear whether it is allowed for them to be percent-encoded or not.
Thinking about this from a software-implementer point of view, it would be a nightmare to scan through each percent-encoded string and determine whether or not the character was from the (non-printable ASCII or non-ASCII) character set before deciding whether or not to process the escape sequence.
I'll clarify the HTTP transport bindings to make it clear that all HTTP Header Values should be decoded through a single round of percent-decoding, and update this PR when that lands.