allow empty payloads in post,put,patch, issue #580

[emicklei]

No description provided.

[dashpole]

cc @liggitt

[liggitt]

shouldn't this expand to include explicit POST/PUT/PATCH requests even if they have no content length (essentially relocating the error from below to here)?

That preserves the 415 error correctly if we get a 0 content-length POST/PUT/PATCH advertising a content-type that doesn't match a route

		available := []string{}
		for _, candidate := range previous {
			available = append(available, candidate.Produces...)
		}
		if httpRequest.ContentLength > 0 || method == http.MethodPost || method == http.MethodPut || method == http.MethodPatch {
			return nil, NewError(
				http.StatusUnsupportedMediaType,
				fmt.Sprintf("415: Unsupported Media Type\n\nAvailable representations: %s", strings.Join(available, ", ")),
			)
		}

[liggitt]

that makes the block that looks at Content-Type of the incoming request responsible for returning 415 errors, and the block that looks at Accept of the incoming request responsible for returning 406 errors, which seems better organized and more correct

[liggitt]

The problem with #543 was that it made it possible to get a 415 when the Content-Type was actually fine and matched a route perfectly, when the thing that filtered us down to zero candidate routes was actually the Accept header that should have returned a 406

[emicklei]

A section in HTTP RFC states:

A user agent SHOULD send Content-Length in a request when the method defines a meaning for enclosed content

This means it is not required, so I am inclined to decide that the package should not care about it. The actual handler of the request, the application, may decide to create a Bad Request.

see https://httpwg.org/specs/rfc9110.html#field.content-length

[liggitt]

By making this unconditional, existing GET requests that spuriously set a content-type header for some reason, but no content-length, would now get 415 errors if the content-type happened to not match a route.

Using a non-zero content-length from the client OR the set of known content-sending methods (as suggested in #581 (comment)) as a signal that a mismatched content-type should 415 error still seems reasonable to me.

I'm mostly trying to avoid introducing new 415 error responses here to requests for which a 415 error would not really make sense.

[liggitt]

Reading the matchesContentType impl more, I'm coming around to this version of the PR that just unconditionally returned a 415 here and dropped all the 415 handling from the later if block.

I guess for a library consumer to be impacted by this in a way that would return a new error to a user, they would have to

1. register a service for a method that typically does not send content-type (like GET / HEAD / OPTIONS / DELETE / TRACE)
2. AND indicate the content-type the service consumes as something other than */*

If they do that (which would be pretty strange, actually), it's reasonable for the content-type filtering to work as expected and 415. If the consumer doesn't want that, they can leave their route consuming unspecified content or explicitly indicate they want to consume */*. That's probably worth a release note though.

[liggitt]

The if block above (where candidates is filtered to 0-length based on the content-type header) definitely seems like a clearer place to return 415 errors

Since this block is about filtering based on the Accept header, which should return 406 errors, I liked how the previous iteration of this PR moved the 415 errors out of this block.

[liggitt]

Reading the matchesContentType impl more, I'm coming around to this version of the PR that just unconditionally returned a 415 here and dropped all the 415 handling from the later if block.

I guess for a library consumer to be impacted by this in a way that would return a new error to a user, they would have to

1. register a service for a method that typically does not send content-type (like GET / HEAD / OPTIONS / DELETE / TRACE)
2. AND indicate the content-type the service consumes as something other than */*

If they do that (which would be pretty strange, actually), it's reasonable for the content-type filtering to work as expected and 415. If the consumer doesn't want that, they can leave their route consuming unspecified content or explicitly indicate they want to consume */*. That's probably worth a release note though.

[emicklei]

@liggitt thank you commenting on this issue and my changes. To be honest, I find it difficult to find the right solution now. I need some more time (or tests or suggestions) on how to proceed

[liggitt]

I need some more time (or tests or suggestions) on how to proceed

As a diff to the current state of this PR, I would suggest the following:

1. Drop 415 error generation from the Accept header route filtering entirely:

diff --git a/jsr311.go b/jsr311.go
index 42758d0..7f04bd9 100644
--- a/jsr311.go
+++ b/jsr311.go
@@ -149,13 +149,6 @@ func (r RouterJSR311) detectRoute(routes []Route, httpRequest *http.Request) (*R
 		for _, candidate := range previous {
 			available = append(available, candidate.Produces...)
 		}
-		method := httpRequest.Method
-		if httpRequest.ContentLength > 0 || method == http.MethodPost || method == http.MethodPut || method == http.MethodPatch {
-			return nil, NewError(
-				http.StatusUnsupportedMediaType,
-				fmt.Sprintf("415: Unsupported Media Type\n\nAvailable representations: %s", strings.Join(available, ", ")),
-			)
-		}
 		return nil, NewError(
 			http.StatusNotAcceptable,
 			fmt.Sprintf("406: Not Acceptable\n\nAvailable representations: %s", strings.Join(available, ", ")))

2. Switch the test exercising Accept header route filtering to expect a 406 response, not a 415 response:

diff --git a/web_service_test.go b/web_service_test.go
index 0a2ee72..aeed87f 100644
--- a/web_service_test.go
+++ b/web_service_test.go
@@ -198,8 +198,8 @@ func TestPostWithNonMatchingAccept(t *testing.T) {
 	httpRequest.Header.Set("Accept", "application/yaml")
 	httpWriter := httptest.NewRecorder()
 	DefaultContainer.dispatch(httpWriter, httpRequest)
-	if httpWriter.Code != http.StatusUnsupportedMediaType {
-		t.Errorf("[%s] 415 expected got %d", "POST", httpWriter.Code)
+	if httpWriter.Code != http.StatusNotAcceptable {
+		t.Errorf("[%s] 406 expected got %d", "POST", httpWriter.Code)
 	}
 }