fix: ensure binary events can handle no content-type header #134
Conversation
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
The fix provided in cloudevents#118 only included tests for `receiver.check()`, and the change in that case was to add the `application/json` content type to the cleansed headers if to type was specified. However, `receiver.parse()` did not receive the benefit of this change. It calls `this.check()` but then sanitizes the original headers again, and the missing content-type was not re-inserted into the newly sanitized headers. This commit, modifies the code so that `receiver.check()` does not insert the content-type, but does allow the validation check to pass if no content-type header exists. When `receiver.parse()` is called, and the headers are sanitized again - and this time used to look up parser implementation, the default `application/json` content-is applied if no content-type header exists. I've also removed a redundant call to `receiver.check()` in receiver_binary_1.js and simplified the usage of `Constants` in the test. Signed-off-by: Lance Ball <lball@redhat.com>
|
@fabiojose @grant @helio-frota I was hoping to get this into the upcoming 2.0.0 release. Would you mind giving this a look today? I was aiming to release this week, but it's Friday and there are still some things that should be documented, so probably Monday for the release. |
helio-frota
reviewed
May 8, 2020
helio-frota
suggested changes
May 8, 2020
Signed-off-by: Lance Ball <lball@redhat.com>
helio-frota
approved these changes
May 8, 2020
lance
added a commit
to lance/sdk-javascript
that referenced
this pull request
May 9, 2020
…nts#134) * fix: ensure binary events can handle no content-type header The fix provided in cloudevents#118 only included tests for `receiver.check()`, and the change in that case was to add the `application/json` content type to the cleansed headers if to type was specified. However, `receiver.parse()` did not receive the benefit of this change. It calls `this.check()` but then sanitizes the original headers again, and the missing content-type was not re-inserted into the newly sanitized headers. This commit, modifies the code so that `receiver.check()` does not insert the content-type, but does allow the validation check to pass if no content-type header exists. When `receiver.parse()` is called, and the headers are sanitized again - and this time used to look up parser implementation, the default `application/json` content-is applied if no content-type header exists. I've also removed a redundant call to `receiver.check()` in receiver_binary_1.js and simplified the usage of `Constants` in the test. Signed-off-by: Lance Ball <lball@redhat.com> * chore: clean up header sniffing Signed-off-by: Lance Ball <lball@redhat.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The fix provided in #118
only included tests for
receiver.check(), and the change in thatcase was to add the
application/jsoncontent type to the cleansedheaders if no type was specified.
However,
receiver.parse()did not receive the benefit of this change. Itcalls
this.check()but then sanitizes the original headers again, and themissing content-type was not re-inserted into the newly sanitized headers.
This commit, modifies the code so that
receiver.check()does not insertthe content-type, but does allow the validation check to pass if no
content-type header exists. When
receiver.parse()is called, and theheaders are sanitized again - and this time used to look up parser implementation,
the default
application/jsoncontent-is applied if no content-type headerexists.
I've also removed a redundant call to
receiver.check()in receiver_binary_1.jsand simplified the usage of
Constantsin the test.Fixes: #133
Signed-off-by: Lance Ball lball@redhat.com