proposal: net/http: export name for "request body too large" error returned by MaxBytesReader

[jerome-laforge]

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

Go 1.15.2

Does this issue reproduce with the latest release?

yes

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

All

What did you do?

With http.MaxByteReader, I want easily check (for example with switch case statement) the error http: request body too large.

What did you expect to see?

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

func TestMaxBytesReader(t *testing.T) {
	req := ioutil.NopCloser(bytes.NewBufferString("my too long request"))
	_, err := http.MaxBytesReader(httptest.NewRecorder(), req, 2).Read([]byte{10: 0})
	if err != http.ErrBodyTooLarge {
		t.FailNow()
	}
}

What did you see instead?

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

func TestMaxBytesReader(t *testing.T) {
	req := ioutil.NopCloser(bytes.NewBufferString("my too long request"))
	_, err := http.MaxBytesReader(httptest.NewRecorder(), req, 2).Read([]byte{10: 0})
	if err == nil || err.Error() != "http: request body too large" {
		t.FailNow()
	}
}

[ianlancetaylor]

When do you need this? When does this need come up in practice? Thanks.

CC @bradfitz

[jerome-laforge]

Hello @ianlancetaylor,
In practice, in our REST API, we want easily check error with switch case statement and reply by an error accordingly to our specification.

for example:

func TestDecode(t *testing.T) {
	type errorBody struct {
		Code    int
		Message string
	}

	testCases := []struct {
		name               string
		body               string
		expectedStatusCode int
		expectedCode       int
	}{
		{
			name:               "too long request",
			body:               `{"Message":"my too long request"}`,
			expectedStatusCode: http.StatusRequestEntityTooLarge,
			expectedCode:       12,
		},
		{
			name:               "unexpected EOF",
			body:               `{"M`,
			expectedStatusCode: http.StatusBadRequest,
			expectedCode:       34,
		},
	}

	for _, tc := range testCases {
		t.Run(tc.name, func(t *testing.T) {
			w := httptest.NewRecorder()
			reqBody := struct{ Message string }{}
			req := ioutil.NopCloser(bytes.NewBufferString(tc.body))
			r := http.MaxBytesReader(w, req, 5)

			err := json.NewDecoder(r).Decode(&reqBody)
			if err != nil {
				var (
					errResBody errorBody
					statusCode int
				)

				switch err {
				case http.ErrBodyTooLarge:
					statusCode = http.StatusRequestEntityTooLarge
					errResBody = errorBody{
						Code:    12,
						Message: "blah blah",
					}

				case io.ErrUnexpectedEOF:
					statusCode = http.StatusBadRequest
					errResBody = errorBody{
						Code:    34,
						Message: "blah blah",
					}

				default:
					// unspecified error case
					statusCode = http.StatusBadRequest
					errResBody = errorBody{
						Code:    56,
						Message: "blah blah",
					}
				}

				w.WriteHeader(statusCode)
				json.NewEncoder(w).Encode(errResBody)
			}

			resp := w.Result()
			actualBody := errorBody{}
			json.NewDecoder(resp.Body).Decode(&actualBody)

			if resp.StatusCode != tc.expectedStatusCode {
				t.Error("unexpected status code")
			}

			if actualBody.Code != tc.expectedCode {
				t.Error("unexpected code")
			}
		})
	}
}

[ianlancetaylor]

I'm not clear: is this just for testing purposes?

For testing purposes I tend to think it's usually best to check for the presence of an error, rather than for an exact error. Users won't usually care about an exact error.

I think that the best way to decide whether we should export this error is whether ordinary non-test code needs to distinguish this case.

[psampaz]

@ianlancetaylor if you want to check for this error in non test code, in order to reply explicitly with http.StatusRequestEntityTooLarge, what would be the best approach to do it?

This is requirement from a real project and I suppose not something very uncommon.

[ianlancetaylor]

@psampaz Can you show us the kind of code you want to write? This proposal would be stronger with an example of where the information is needed by non-test code. Thanks.

[psampaz]

@ianlancetaylor something similar to this:

			body = http.MaxBytesReader(w, r.Body, bodySizeLimit)

			if _, err := ioutil.ReadAll(body); err != nil {
				if err.Error() == "http: request body too large" {
					http.Error(w, err.Error(), http.StatusRequestEntityTooLarge)
					return
				}

				http.Error(w, err.Error(), http.StatusBadRequest)
				return
			}

The following

if err.Error() == "http: request body too large" {

could be turned to:

errors.Is(err, http.ErrBodyTooLarge)

[JShorthouse]

I use a MaxBytesReader on multipart forms with file uploads. As far as I see it there are two groups of errors that can occur from ParseMultipartForm with a MaxBytesReader:

• The user has accidentally uploaded a file that is too large
• A malicious (or badly implemented) client has sent a malformed body

These are completely different errors in my opinion and I want to handle them as such.

[floriantz]

I currently stumbled on the same issue, and I do believe it is much friendlier to the user to return an exported error which allows the use of errors.Is() and add a reference to it in the MaxBytesReader doc, so that a user can see right away what error to look for to handle this case.

[earthboundkid]

This is a dupe of #30715.

[neild]

Duplicate of #30715. Will comment on that one.