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

proposal: net/http/httptest: Recorder.Result() should always set Response.Body to http.NoBody when response is empty #39290

Closed
MaerF0x0 opened this issue May 28, 2020 · 10 comments

Comments

@MaerF0x0
Copy link

What version of Go are you using (go version)?

go version
go version go1.13.8 darwin/amd64

Does this issue reproduce with the latest release?

I looked at the code in master and it looks the same

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/user/Library/Caches/go-build"
GOENV="/Users/user/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/user/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.13.8/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.13.8/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/z3/089w1c5x2ngbzyfj0wb2lp440000gp/T/go-build437685849=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

https://play.golang.org/p/r3ypSy1vAIb

Attempting to use testify/assert to assert that the response is an empty body like

assert.Equal(t, http.NoBody, resp.Body)

What did you expect to see?

Return http.NoBody

What did you see instead?

ioutil.nopCloser(ioutil.nopCloser{Reader:(*bytes.Reader)(0xc000566ba0)})

@andybons andybons changed the title httptest.NewRecorder() returns non-nil Body net/http/httptest: NewRecorder() returns non-nil Body May 28, 2020
@andybons
Copy link
Member

I’m not sure we make any explicit guarantees that an empty response’s body will be equal to http.NoBody or nil in this case. Can you point to any documentation that states this?

The documentation seems to imply that this is used when constructing requests, not interpreting responses.

Is the desire that any response with an empty body be equal to http.NoBody in all cases?

@andybons
Copy link
Member

@bradfitz @neild

@andybons andybons added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label May 28, 2020
@andybons andybons added this to the Unplanned milestone May 28, 2020
@MaerF0x0
Copy link
Author

MaerF0x0 commented Jun 4, 2020

Just to pile on an additional suggestion along the same lines.

We could consider implementing json.Marshaler so that http.NoBody doesnt marshal as {} (should be empty string, no?)

@andybons
Copy link
Member

andybons commented Jun 4, 2020

@MaerF0x0 to be clear, my comments weren’t suggestions, but questions for you to gain a better understanding of your report.

@MaerF0x0
Copy link
Author

MaerF0x0 commented Jun 4, 2020

My apologies, i thought those tags for brad/damien meant the q's were for them :).

I do not see any documentation indicating http.NoBody to be returned.
However, I was under the impression it is for responses due to some cursory reads of usages in the source such as:

func (rw *ResponseRecorder) Result() *http.Response {
	//...  code ...

	if rw.Body != nil {
		res.Body = ioutil.NopCloser(bytes.NewReader(rw.Body.Bytes()))
	} else {
		res.Body = http.NoBody
	}

https://github.com/golang/go/blob/master/src/net/http/httptest/recorder.go#L181-L185

This guard seems to imply if the returned body == nil, actually make it http.NoBody . However the NewRecorder() func seems to always initialize rw.Body ...

@andybons
Copy link
Member

andybons commented Jun 5, 2020

This sounds like a proposal, as the behavior is not explicitly defined nor documented. If we decided to set a response’s Body to http.NoBody in all instances where it’s empty, then there’s a possibility it could break programs that rely on current semantics (I haven’t dug deep into the APIs so I’m not certain if that’s true).

I’ll mark as a proposal for now and see what the committee thinks. Since Brad is on proposal review he’ll be able to chime in as well (if he doesn’t get a chance in this thread).

@andybons andybons removed the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Jun 5, 2020
@andybons andybons changed the title net/http/httptest: NewRecorder() returns non-nil Body proposal: net/http/httptest: Recorder.Result() should always return http.NoBody when response is empty Jun 5, 2020
@andybons andybons changed the title proposal: net/http/httptest: Recorder.Result() should always return http.NoBody when response is empty proposal: net/http/httptest: Recorder.Result() should always set Response.Body to http.NoBody when response is empty Jun 5, 2020
@dmitshur dmitshur modified the milestones: Unplanned, Proposal Jun 5, 2020
@rsc rsc added this to Incoming in Proposals (old) Jun 10, 2020
@bradfitz
Copy link
Contributor

bradfitz commented May 5, 2021

NoBody was originally added out of necessity to make an ambiguous case not ambiguous, to know when a request was replayable (as an io.ReadCloser isn't in general comparable usefully). I'd rather not expand its meaning to guarantee when it's set in Response.Body.

I'd worry that if we did this, people would start testing Response.Body == http.NoBody and then we'd be Hyrum's-Law-locked into that forever (possible precluding optimizations in the future if there was a situation where we didn't know the Content-Length at a certain phase of the code).

@rsc
Copy link
Contributor

rsc commented May 5, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc moved this from Incoming to Active in Proposals (old) May 5, 2021
@rsc rsc moved this from Active to Likely Decline in Proposals (old) May 12, 2021
@rsc
Copy link
Contributor

rsc commented May 12, 2021

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented May 19, 2021

No change in consensus, so declined.
— rsc for the proposal review group

@rsc rsc moved this from Likely Decline to Declined in Proposals (old) May 19, 2021
@rsc rsc closed this as completed May 19, 2021
@golang golang locked and limited conversation to collaborators May 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests

6 participants