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.

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

request.GetBody() breaks the request object #130

Closed
bcomnes opened this issue Feb 28, 2019 · 3 comments
Closed

request.GetBody() breaks the request object #130

bcomnes opened this issue Feb 28, 2019 · 3 comments

Comments

@bcomnes
Copy link
Contributor

bcomnes commented Feb 28, 2019

I tracked down a bug in the implementation of request.GetBody I think.

I wrote a failing test to demonstrate the bug:

https://gist.github.com/bcomnes/4ffddcd270e8721267efa4b9e2ed89b8#file-http_test-go

The example is pretty contrived, but if you use a custom roundTripper and get ahold of the req object generated by go-openapi before you give it to transport.RoundTrup and call req.GetBody, it causes the client operation ClientRequestWriterFunc to write out any BodyParam to the req.Body again, doubling/tripling the req.Body etc. The length of the req.Body basically grows to n x req.ContentLength where n is the number of times you call req.GetBody before reading req.Body.

At this point the req object is toast, since the length of the Body no longer matches the ContentLengh. Passing it to transport.RoundTrip fails with this error:

'https://github.com/golang/go/blob/61170f85e62f1326d42c4dbd8aa17ab4a1305a87/src/net/http/transfer.go#L387-L389

The contents of the Body returned by req.GetBody is perfect, even if you call req.GetBody multiple times. Its only req.Body that is affected. This only applies when you have body parameters.

Bug

Calling .GetBody() should not cause the Body to be written to again by ClientRequestWriterFunc

Possible resolution

If you have any ideas of where this is happening, I would love to hear them. I would like to PR the failing test for you, and start hunting down an actual fix. I am open to any suggestions if you have them.

@casualjim
Copy link
Member

I think this is the reason: https://github.com/go-openapi/runtime/blob/master/client/request.go#L216

The first 2 options that come to mind are:

  • the buffer needs to be reset on every call to get body, or at least it should start reading after the currently captured offset
  • wrap the entire thing in a sync.Once so that it only executes once which is probably closer to intent

@bcomnes
Copy link
Contributor Author

bcomnes commented Mar 1, 2019

Very cool! Thank you for those. I am willing to try to find and PR a fix in, unless you prefer to do it.

@casualjim
Copy link
Member

Cool, looking forward to your PR

bcomnes added a commit to bcomnes/runtime that referenced this issue Mar 1, 2019
req.Body is already populated, and this, even on the first call, will write the payload again.  

Also adds a test guarding against the bug.

Closes go-openapi#130
bcomnes added a commit to bcomnes/runtime that referenced this issue Mar 1, 2019
req.Body is already populated, and this, even on the first call, will write the payload again.

Also adds a test guarding against the bug.

Closes go-openapi#130

Signed-off-by: Bret Comnes <[email protected]>
bcomnes added a commit to bcomnes/runtime that referenced this issue Mar 1, 2019
req.Body is already populated, and this, even on the first call, will write the payload again.

Also adds a test guarding against the bug.

Closes go-openapi#130

Signed-off-by: Bret Comnes <[email protected]>
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

No branches or pull requests

2 participants