Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix decoding empty request body #527

Merged
merged 1 commit into from
Jan 21, 2018
Merged

Fix decoding empty request body #527

merged 1 commit into from
Jan 21, 2018

Conversation

syhpoon
Copy link
Contributor

@syhpoon syhpoon commented Jan 17, 2018

Fix for this issue: #464

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.
  • If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot. The email used to register you as an authorized contributor must be the email used for the Git commit.
  • In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

@syhpoon
Copy link
Contributor Author

syhpoon commented Jan 17, 2018

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@achew22 achew22 requested a review from tmc January 17, 2018 19:08
achew22
achew22 approved these changes Jan 17, 2018
View reviewed changes
Copy link
Collaborator

@achew22 achew22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Giving @tmc a chance to look at it.

@achew22
Copy link
Collaborator

achew22 commented Jan 21, 2018

Looks like @tmc doesn't want a view. Merging.

@achew22 achew22 merged commit 4529f7f into grpc-ecosystem:master Jan 21, 2018
jacksontj added a commit to jacksontj/grpc-gateway that referenced this pull request Apr 12, 2018
@jacksontj
Support cases where the request is done with transfer-encoding chunked
1a0cf45 
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).
@jacksontj jacksontj mentioned this pull request Apr 12, 2018
Merged
jacksontj added a commit to jacksontj/grpc-gateway that referenced this pull request Apr 14, 2018
@jacksontj
Support cases where the request is done with transfer-encoding chunked
8bb505b 
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).
jacksontj added a commit to jacksontj/grpc-gateway that referenced this pull request Apr 16, 2018
@jacksontj
Support cases where the request is done with transfer-encoding chunked
526e240 
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).
jacksontj added a commit to jacksontj/grpc-gateway that referenced this pull request Apr 18, 2018
@jacksontj
Support cases where the request is done with transfer-encoding chunked
2443650 
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).
achew22 pushed a commit that referenced this pull request Apr 18, 2018
@jacksontj @achew22
Support cases where the request is done with transfer-encoding chunked (
a5bcd11 
#589)

* Support cases where the request is done with transfer-encoding chunked

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).
adasari pushed a commit to adasari/grpc-gateway that referenced this pull request Apr 9, 2020
@syhpoon @achew22
Fix decoding empty request body (grpc-ecosystem#527)
730ddb5
adasari pushed a commit to adasari/grpc-gateway that referenced this pull request Apr 9, 2020
@jacksontj @achew22
Support cases where the request is done with transfer-encoding chunked (
bb00f27 
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).
@johanbrandhorst johanbrandhorst mentioned this pull request Apr 5, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@achew22 achew22 achew22 approved these changes

@tmc tmc Awaiting requested review from tmc

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@syhpoon @googlebot @achew22