Skip to content

Conversation

@disilin
Copy link

@disilin disilin commented Mar 13, 2019

handle Body will return EOF when it used in middleware.

  • With pull requests:
    • Open your pull request against master
    • Your pull request should have no more than two commits, if not you should squash them.
    • It should pass all tests in the available continuous integrations systems such as TravisCI.
    • You should add/modify tests to cover your proposed code changes.
    • If your pull request contains a new feature, please document it on the README.

handle Body will return EOF when it used in middleware.
@codecov
Copy link

codecov bot commented Mar 13, 2019

Codecov Report

Merging #1810 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1810      +/-   ##
==========================================
+ Coverage   98.62%   98.62%   +<.01%     
==========================================
  Files          41       41              
  Lines        2113     2115       +2     
==========================================
+ Hits         2084     2086       +2     
  Misses         18       18              
  Partials       11       11
Impacted Files Coverage Δ
context.go 98.38% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e526148...af0276f. Read the comment docs.

Copy link
Contributor

@vkd vkd left a comment

Choose a reason for hiding this comment

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

I think it will be better if you add a test that cover your changes.

@thinkerou thinkerou added this to the 1.5 milestone Mar 15, 2019
@dmarkham
Copy link
Contributor

@disilin Thank you for this patch, I'm interested in why this is a good idea. Using any Bind will always consume c.Request.Body. I understand ShouldBindBodyWith is set up to be called more than once. After reading though (#216) to get some context on why people wanted this at all. I'm interested in why you would want to use a Bind method and still want to use c.Request.Body.

So why would we want to make this one bind method even more different that all the others?

As a side note for @thinkerou @appleboy I think we should deprecate ShouldBindBodyWith. The name is confusing, the interface it's using is also named wrong, BindBody interface is documented to do everything except bind with a body. BindBytes would have been more clear for people.

@dmarkham
Copy link
Contributor

I'm also a little concerned the Handler is pretty well documented and discourages us from changing the request. It might not matter now, but I think this would be us clearly breaking that "rule"

https://golang.org/pkg/net/http/#Handler
Except for reading the body, handlers should not modify the provided Request.

@TonyPythoneer
Copy link

@thinkerou @appleboy please review this MR.

I agree @dmarkham : handlers should not modify the provided Request.

I have the same confused about it when I invoke bind function with req, the req will be empty buffer object. I think it’s should get fiexed.

@TonyPythoneer
Copy link

@disilin Could you add testcase for this? It good to reviewer to understand your changing and ensure the framework is still stable for gin user. We will thank you.

@thinkerou thinkerou modified the milestones: 1.5, 1.6 Oct 31, 2019
@thinkerou thinkerou modified the milestones: 1.6, 1.7 Feb 28, 2020
@thinkerou thinkerou modified the milestones: 1.7, v1.8 Oct 27, 2020
Copy link
Contributor

@daheige daheige left a comment

Choose a reason for hiding this comment

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

LGTM

@appleboy
Copy link
Member

appleboy commented Feb 7, 2022

@disilin Please add some unit testing.

@thinkerou thinkerou modified the milestones: v1.8, v1.9 May 28, 2022
@thinkerou
Copy link
Member

@disilin Please add some unit testing.

still need, move to 1.10

@thinkerou thinkerou modified the milestones: v1.9, v1.10 Jan 17, 2023
@appleboy appleboy modified the milestones: v1.10, v1.11 Mar 21, 2024
@appleboy
Copy link
Member

move to 1.11

@appleboy appleboy modified the milestones: v1.11, v1.12 Jun 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants