Support cases where the request is done with transfer-encoding chunked #589
Conversation
This file contains 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
|
Looking at the CI failure it seems unrelated? |
|
@jacksontj, yep. I fixed the CI just now. Could you rebase please? |
|
@achew22 rebased |
|
This looks like a reasonable way to approach it. Do you think you could add some tests to that would have failed in the old system that pass in the new one? |
|
Added a test case for it, and rebased again (someone changed the template file ;) ) |
|
@achew22 test added, CI is passing |
|
@achew22 ping (in case the last one got lost in the inbox :) ) |
|
I'm terribly sorry, would you be willing to rebase this change to master? I made some significant changes to the Travis CI config I want to make sure this passes with them. |
PR grpc-ecosystem#527 was put in place to fix an issue where an empty request body would cause an empty message to be sent down to the GRPC service (instead of failing). The fix at the time was to check that the ContentLength was >0, but this doesn't take into consideration transfer-encoding chunked POSTs. Since this patch all chunked POSTs no longer unmarshal the message (as the content-length was 0). My proposed fix is instead to always call Decode and simply ignore EOF errors (as we still want to pass the un-filled struct down). I have tested that things such as partial json blobs (something like '{') don' t return EOF (they have their own json error).
|
It most definitely got lost in my inbox. Thanks so much for rebasing |
adasari
pushed a commit
to adasari/grpc-gateway
that referenced
this pull request
Apr 9, 2020
grpc-ecosystem#589) * Support cases where the request is done with transfer-encoding chunked PR grpc-ecosystem#527 was put in place to fix an issue where an empty request body would cause an empty message to be sent down to the GRPC service (instead of failing). The fix at the time was to check that the ContentLength was >0, but this doesn't take into consideration transfer-encoding chunked POSTs. Since this patch all chunked POSTs no longer unmarshal the message (as the content-length was 0). My proposed fix is instead to always call Decode and simply ignore EOF errors (as we still want to pass the un-filled struct down). I have tested that things such as partial json blobs (something like '{') don' t return EOF (they have their own json error).
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.
PR #527 was put in place to fix an issue where an empty request body
would cause an empty message to be sent down to the GRPC service
(instead of failing). The fix at the time was to check that the
ContentLength was >0, but this doesn't take into consideration
transfer-encoding chunked POSTs. Since this patch all chunked POSTs
no longer unmarshal the message (as the content-length was 0).
My proposed fix is instead to always call Decode and simply ignore EOF
errors (as we still want to pass the un-filled struct down). I have
tested that things such as partial json blobs (something like '{') don'
t return EOF (they have their own json error).