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

Open
MaerF0x0 opened this issue May 28, 2020 · 6 comments
Labels
Projects
Milestone

Comments

@MaerF0x0
Copy link

@MaerF0x0 MaerF0x0 commented May 28, 2020

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

@andybons andybons commented May 28, 2020

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

@andybons andybons commented May 28, 2020

@andybons andybons added this to the Unplanned milestone May 28, 2020
@MaerF0x0
Copy link
Author

@MaerF0x0 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 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 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 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 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
@dmitshur dmitshur added the Proposal label Jun 5, 2020
@rsc rsc added this to Incoming in Proposals Jun 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Incoming
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.