gin icon indicating copy to clipboard operation
gin copied to clipboard

handle Body return EOF

Open disilin opened this issue 5 years ago • 8 comments

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.

disilin avatar Mar 13 '19 05:03 disilin

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%> (ø) :arrow_up:

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.

codecov[bot] avatar Mar 13 '19 05:03 codecov[bot]

@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 avatar May 12 '19 20:05 dmarkham

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.

dmarkham avatar May 12 '19 21:05 dmarkham

@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 avatar Jun 24 '19 15:06 TonyPythoneer

@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.

TonyPythoneer avatar Jun 24 '19 15:06 TonyPythoneer

@disilin Please add some unit testing.

appleboy avatar Feb 07 '22 15:02 appleboy

@disilin Please add some unit testing.

still need, move to 1.10

thinkerou avatar Jan 17 '23 04:01 thinkerou

move to 1.11

appleboy avatar Mar 21 '24 14:03 appleboy