Repository: danielgtaylor/huma
Pull request #41 (2022-04-12 20:27:10)

feat: return more specific error codes
This PR changes the status code behavior for errors to attempt to return more specific status codes when possible while still striving for exhaustive error responses. As can be seen in the README, the new flow is:

```mermaid
flowchart TD
	Request[Request has errors?] -->|yes| Panic
	Request -->|no| Continue[Continue to handler]
	Panic[Panic?] -->|yes| 500
	Panic -->|no| RequestBody[Request body too large?]
	RequestBody -->|yes| 413
	RequestBody -->|no| RequestTimeout[Request took too long to read?]
	RequestTimeout -->|yes| 408
	RequestTimeout -->|no| ParseFailure[Cannot parse input?]
	ParseFailure -->|yes| 400
	ParseFailure -->|no| ValidationFailure[Validation failed?]
	ValidationFailure -->|yes| 422
	ValidationFailure -->|no| 400
```

The change is fairly small & unobtrusive. Several tests have been updated to reflect the changes. It should not break clients, however your API will have new documented responses when upgrading Huma.

Note: the above flowchart deliberately excludes stuff like `405 Method Not Allowed` because that returns at the low-level router before any of the above executes.


Patch:
diff --git a/README.md b/README.md
index a7d4bb3..5c2e0d7 100644
--- a/README.md
+++ b/README.md
@@ -276,11 +276,32 @@ app.Resource("/exhaustive").Get("exhaustive", "Exhastive errors example",
 })
 ```
 
+While every attempt is made to return exhaustive errors within Huma, each individual response can only contain a single HTTP status code. The following chart describes which codes get returned and when:
+
+```mermaid
+flowchart TD
+	Request[Request has errors?] -->|yes| Panic
+	Request -->|no| Continue[Continue to handler]
+	Panic[Panic?] -->|yes| 500
+	Panic -->|no| RequestBody[Request body too large?]
+	RequestBody -->|yes| 413
+	RequestBody -->|no| RequestTimeout[Request took too long to read?]
+	RequestTimeout -->|yes| 408
+	RequestTimeout -->|no| ParseFailure[Cannot parse input?]
+	ParseFailure -->|yes| 400
+	ParseFailure -->|no| ValidationFailure[Validation failed?]
+	ValidationFailure -->|yes| 422
+	ValidationFailure -->|no| 400
+```
+
+This means it is possible to, for example, get an HTTP `408 Request Timeout` response that _also_ contains an error detail with a validation error for one of the input headers. Since request timeout has higher priority, that will be the response status code that is returned.
+
 ## WriteContent
 
-Write contents allows you to write content in the provided ReadSeeker in the response.  It will handle Range, If-Match, If-Unmodified-Since, If-None-Match, If-Modified-Since, and if-Range requests for caching and large object partial content responses.
+Write contents allows you to write content in the provided ReadSeeker in the response. It will handle Range, If-Match, If-Unmodified-Since, If-None-Match, If-Modified-Since, and if-Range requests for caching and large object partial content responses.
 
 example of a partial content response sending a large video file:
+
 ```go
 package main
 
@@ -328,7 +349,7 @@ func main() {
 }
 ```
 
-Note that `WriteContent` does not automatically set the mime type.  You should set the `Content-Type` response header directly.  Also in order for `WriteContent` to respect the `Modified` headers you must call `SetContentLastModified`.  This is optional and if not set `WriteContent` will simply not respect the `Modified` request headers.
+Note that `WriteContent` does not automatically set the mime type. You should set the `Content-Type` response header directly. Also in order for `WriteContent` to respect the `Modified` headers you must call `SetContentLastModified`. This is optional and if not set `WriteContent` will simply not respect the `Modified` request headers.
 
 ## Request Inputs
 
diff --git a/context.go b/context.go
index b524c47..203ded7 100644
--- a/context.go
+++ b/context.go
@@ -78,6 +78,7 @@ type hcontext struct {
 	http.ResponseWriter
 	r                     *http.Request
 	errors                []error
+	errorCode             int
 	op                    *Operation
 	closed                bool
 	docsPrefix            string
diff --git a/operation.go b/operation.go
index 146b0ab..eaeff65 100644
--- a/operation.go
+++ b/operation.go
@@ -220,9 +220,20 @@ func (o *Operation) Run(handler interface{}) {
 			}
 		}
 
+		possible := []int{http.StatusBadRequest}
+
+		if _, ok := input.FieldByName("Body"); ok || len(o.params) > 0 {
+			// Invalid parameter values or body values can cause a 422.
+			possible = append(possible, http.StatusUnprocessableEntity)
+		}
+
 		// Get body if present.
 		if body, ok := input.FieldByName("Body"); ok {
 			o.requestModel = body.Type
+			possible = append(possible,
+				http.StatusRequestEntityTooLarge,
+				http.StatusRequestTimeout,
+			)
 
 			if o.requestSchema == nil {
 				o.requestSchema, err = schema.GenerateWithMode(body.Type, schema.ModeWrite, nil)
@@ -232,18 +243,17 @@ func (o *Operation) Run(handler interface{}) {
 			}
 		}
 
-		// It's possible for the inputs to generate a 400, so add it if it wasn't
-		// explicitly defined.
-		found400 := false
+		// It's possible for the inputs to generate a few different errors, so
+		// generate them if not already present.
+		found := map[int]bool{}
 		for _, r := range o.responses {
-			if r.status == http.StatusBadRequest {
-				found400 = true
-				break
-			}
+			found[r.status] = true
 		}
 
-		if !found400 {
-			o.responses = append(o.responses, NewResponse(http.StatusBadRequest, http.StatusText(http.StatusBadRequest)).ContentType("application/problem+json").Model(&ErrorModel{}))
+		for _, s := range possible {
+			if !found[s] {
+				o.responses = append(o.responses, NewResponse(s, http.StatusText(s)).ContentType("application/problem+json").Model(&ErrorModel{}))
+			}
 		}
 	}
 
@@ -266,6 +276,7 @@ func (o *Operation) Run(handler interface{}) {
 			docsPrefix:            o.resource.router.docsPrefix,
 			urlPrefix:             o.resource.router.urlPrefix,
 			disableSchemaProperty: o.resource.router.disableSchemaProperty,
+			errorCode:             http.StatusBadRequest,
 		}
 
 		// If there is no input struct (just a context), then the call is simple.
@@ -296,9 +307,14 @@ func (o *Operation) Run(handler interface{}) {
 		}
 
 		setFields(ctx, ctx.r, input, inputType)
+		if !ctx.HasError() {
+			// No errors yet, so any errors that come after should be treated as a
+			// semantic rather than structural error.
+			ctx.errorCode = http.StatusUnprocessableEntity
+		}
 		resolveFields(ctx, "", input)
 		if ctx.HasError() {
-			ctx.WriteError(http.StatusBadRequest, "Error while parsing input parameters")
+			ctx.WriteError(ctx.errorCode, "Error while processing input parameters")
 			return
 		}
 
diff --git a/resolver.go b/resolver.go
index 8270e37..2cb37e6 100644
--- a/resolver.go
+++ b/resolver.go
@@ -70,6 +70,10 @@ func validAgainstSchema(ctx *hcontext, label string, schema *schema.Schema, data
 		for _, desc := range result.Errors() {
 			// Note: some descriptions start with the context location so we trim
 			// those off to prevent duplicating data. (e.g. see the enum error)
+			if ctx.errorCode <= 400 {
+				// Set if a more specific code hasn't been set yet.
+				ctx.errorCode = http.StatusUnprocessableEntity
+			}
 			ctx.AddError(&ErrorDetail{
 				Message:  strings.TrimPrefix(desc.Description(), desc.Context().String()+" "),
 				Location: strings.TrimSuffix(label+strings.TrimPrefix(desc.Field(), "(root)"), "."),
@@ -201,6 +205,7 @@ func setFields(ctx *hcontext, req *http.Request, input reflect.Value, t reflect.
 			if length := req.Header.Get("Content-Length"); length != "" {
 				if l, err := strconv.ParseInt(length, 10, 64); err == nil {
 					if l > ctx.op.maxBodyBytes {
+						ctx.errorCode = http.StatusRequestEntityTooLarge
 						ctx.AddError(&ErrorDetail{
 							Message:  fmt.Sprintf("Request body too large, limit = %d bytes", ctx.op.maxBodyBytes),
 							Location: locationBody,
@@ -215,11 +220,13 @@ func setFields(ctx *hcontext, req *http.Request, input reflect.Value, t reflect.
 			data, err := ioutil.ReadAll(req.Body)
 			if err != nil {
 				if strings.Contains(err.Error(), "request body too large") {
+					ctx.errorCode = http.StatusRequestEntityTooLarge
 					ctx.AddError(&ErrorDetail{
 						Message:  fmt.Sprintf("Request body too large, limit = %d bytes", ctx.op.maxBodyBytes),
 						Location: locationBody,
 					})
 				} else if e, ok := err.(net.Error); ok && e.Timeout() {
+					ctx.errorCode = http.StatusRequestTimeout
 					ctx.AddError(&ErrorDetail{
 						Message:  fmt.Sprintf("Request body took too long to read: timed out after %v", ctx.op.bodyReadTimeout),
 						Location: locationBody,
@@ -238,7 +245,11 @@ func setFields(ctx *hcontext, req *http.Request, input reflect.Value, t reflect.
 
 			err = json.Unmarshal(data, inField.Addr().Interface())
 			if err != nil {
-				panic(err)
+				ctx.AddError(&ErrorDetail{
+					Message:  "Cannot unmarshal JSON request body",
+					Location: locationBody,
+					Value:    string(data),
+				})
 			}
 			continue
 		}


Test patch:
diff --git a/resolver_test.go b/resolver_test.go
index 88b7700..fc63d86 100644
--- a/resolver_test.go
+++ b/resolver_test.go
@@ -34,7 +34,7 @@ func TestExhaustiveErrors(t *testing.T) {
 	r.Host = "example.com"
 	app.ServeHTTP(w, r)
 
-	assert.JSONEq(t, `{"$schema": "https://example.com/schemas/ErrorModel.json", "title":"Bad Request","status":400,"detail":"Error while parsing input parameters","errors":[{"message":"cannot parse boolean","location":"query.bool","value":"bad"},{"message":"cannot parse integer","location":"query.int","value":"bad"},{"message":"cannot parse float","location":"query.float32","value":"bad"},{"message":"cannot parse float","location":"query.float64","value":"bad"},{"message":"cannot parse integer","location":"query[2].tags","value":"bad"},{"message":"unable to validate against schema: invalid character 'b' looking for beginning of value","location":"query.tags","value":"[1,2,bad]"},{"message":"cannot parse time","location":"query.time","value":"bad"},{"message":"Must be greater than or equal to 5","location":"body.test","value":1}]}`, w.Body.String())
+	assert.JSONEq(t, `{"$schema": "https://example.com/schemas/ErrorModel.json", "title":"Unprocessable Entity","status":422,"detail":"Error while processing input parameters","errors":[{"message":"cannot parse boolean","location":"query.bool","value":"bad"},{"message":"cannot parse integer","location":"query.int","value":"bad"},{"message":"cannot parse float","location":"query.float32","value":"bad"},{"message":"cannot parse float","location":"query.float64","value":"bad"},{"message":"cannot parse integer","location":"query[2].tags","value":"bad"},{"message":"unable to validate against schema: invalid character 'b' looking for beginning of value","location":"query.tags","value":"[1,2,bad]"},{"message":"cannot parse time","location":"query.time","value":"bad"},{"message":"Must be greater than or equal to 5","location":"body.test","value":1}]}`, w.Body.String())
 }
 
 type Dep1 struct {
@@ -110,9 +110,9 @@ func TestNestedResolverError(t *testing.T) {
 
 	assert.JSONEq(t, `{
 		"$schema": "https://example.com/schemas/ErrorModel.json",
-		"status": 400,
-		"title": "Bad Request",
-		"detail": "Error while parsing input parameters",
+		"status": 422,
+		"title": "Unprocessable Entity",
+		"detail": "Error while processing input parameters",
 		"errors": [
 			{
 				"message": "Only one of ['one', 'two'] is allowed.",
@@ -122,3 +122,22 @@ func TestNestedResolverError(t *testing.T) {
 		]
 	}`, w.Body.String())
 }
+
+func TestInvalidJSON(t *testing.T) {
+	app := newTestRouter()
+
+	app.Resource("/").Post("test", "Test",
+		NewResponse(http.StatusNoContent, "desc"),
+	).Run(func(ctx Context, input struct {
+		Body string
+	}) {
+		ctx.WriteHeader(http.StatusNoContent)
+	})
+
+	// Test happy case just sending ONE of the two possible fields in each struct.
+	w := httptest.NewRecorder()
+	r, _ := http.NewRequest(http.MethodPost, "/", strings.NewReader(`{.2asdf2`))
+	app.ServeHTTP(w, r)
+
+	assert.Equal(t, http.StatusBadRequest, w.Result().StatusCode)
+}
diff --git a/router_test.go b/router_test.go
index 199b502..d69fbbf 100644
--- a/router_test.go
+++ b/router_test.go
@@ -198,7 +198,7 @@ func TestTooBigBody(t *testing.T) {
 	w := httptest.NewRecorder()
 	req, _ := http.NewRequest(http.MethodPut, "/test", strings.NewReader(`{"id": "foo"}`))
 	app.ServeHTTP(w, req)
-	assert.Equal(t, http.StatusBadRequest, w.Code)
+	assert.Equal(t, http.StatusRequestEntityTooLarge, w.Code)
 	assert.Contains(t, w.Body.String(), "Request body too large")
 
 	// With content length
@@ -206,7 +206,7 @@ func TestTooBigBody(t *testing.T) {
 	req, _ = http.NewRequest(http.MethodPut, "/test", strings.NewReader(`{"id": "foo"}`))
 	req.Header.Set("Content-Length", "13")
 	app.ServeHTTP(w, req)
-	assert.Equal(t, http.StatusBadRequest, w.Code)
+	assert.Equal(t, http.StatusRequestEntityTooLarge, w.Code)
 	assert.Contains(t, w.Body.String(), "Request body too large")
 }
 
@@ -250,7 +250,7 @@ func TestBodySlow(t *testing.T) {
 	w := httptest.NewRecorder()
 	req, _ := http.NewRequest(http.MethodPut, "/test", &slowReader{})
 	app.ServeHTTP(w, req)
-	assert.Equal(t, http.StatusBadRequest, w.Code)
+	assert.Equal(t, http.StatusRequestTimeout, w.Code)
 	assert.Contains(t, w.Body.String(), "timed out")
 }
 
@@ -414,7 +414,7 @@ func TestCustomRequestSchema(t *testing.T) {
 	req, _ := http.NewRequest(http.MethodPost, "/foo", strings.NewReader("1234"))
 	app.ServeHTTP(w, req)
 
-	assert.Equal(t, http.StatusBadRequest, w.Result().StatusCode)
+	assert.Equal(t, http.StatusUnprocessableEntity, w.Result().StatusCode)
 }
 
 func TestGetOperationName(t *testing.T) {