Only send a 304 when the request was a GET (#957)

The 304 branch in `connectUnaryHandlerConn.Close` works out whether it's
answering a GET by counting query parameters, and `Peer.Query` gets
filled in for every method. So any POST that happens to carry a query
string gets the conditional-GET treatment. Same handler returning the
same `NewNotModifiedError`, one extra param on the URL:

```
POST /svc/Method          ->  500  application/json  {"code":"unknown","message":"not modified"}
POST /svc/Method?foo=bar  ->  304  no body
```

The message and any error details go with the body, and a connect-go
client turns the second one into `unknown: 304 Not Modified`. Thats not
hard to land on - a base URL with a param baked into it, or a proxy
tacking on cache-busting ones.

`mergeResponseHeader` a few lines down already checks `request.Method`,
and the client side only honours a 304 for GETs, so Im fairly confident
the method is what the query count was standing in for.

The test bolts the query string on in a wrapper handler rather than
through the client, beacuse a connect-go client won't put one on a POST
by itself. `TestGetNotModified` still covers the GET side.

Signed-off-by: Iain <iain1970@protonmail.com>

Author: NotAFlightRisk

client_ext_test.go

@@ -202,6 +202,25 @@ func TestGetNotModified(t *testing.T) {
 	assert.Equal(t, http.MethodGet, unaryReq.HTTPMethod())
 }
 
+func TestNotModifiedOnlyForGet(t *testing.T) {
+	t.Parallel()
+
+	mux := http.NewServeMux()
+	mux.Handle(pingv1connect.NewPingServiceHandler(&alwaysNotModifiedPingServer{}))
+	server := memhttptest.NewServer(t, http.HandlerFunc(func(respWriter http.ResponseWriter, req *http.Request) {
+		req.URL.RawQuery = "cache-buster=1"
+		mux.ServeHTTP(respWriter, req)
+	}))
+	client := pingv1connect.NewPingServiceClient(server.Client(), server.URL())
+
+	_, err := client.Ping(t.Context(), connect.NewRequest(&pingv1.PingRequest{}))
+	assert.NotNil(t, err)
+	assert.Equal(t, connect.CodeOf(err), connect.CodeUnknown)
+	var connectErr *connect.Error
+	assert.True(t, errors.As(err, &connectErr))
+	assert.Equal(t, connectErr.Message(), "not modified")
+}
+
 func TestGetNoContentHeaders(t *testing.T) {
 	t.Parallel()
 
@@ -863,6 +882,17 @@ func (s *notModifiedPingServer) Ping(
 	return resp, nil
 }
 
+type alwaysNotModifiedPingServer struct {
+	pingv1connect.UnimplementedPingServiceHandler
+}
+
+func (*alwaysNotModifiedPingServer) Ping(
+	_ context.Context,
+	_ *connect.Request[pingv1.PingRequest],
+) (*connect.Response[pingv1.PingResponse], error) {
+	return nil, connect.NewNotModifiedError(nil)
+}
+
 type assertPeerInterceptor struct {
 	tb testing.TB
 }

protocol_connect.go

@@ -728,7 +728,7 @@ func (hc *connectUnaryHandlerConn) Close(err error) error {
 		hc.mergeResponseHeader(err)
 		// If the handler received a GET request and the resource hasn't changed,
 		// return a 304.
-		if len(hc.peer.Query) > 0 && IsNotModifiedError(err) {
+		if hc.request.Method == http.MethodGet && IsNotModifiedError(err) {
 			hc.responseWriter.WriteHeader(http.StatusNotModified)
 			return hc.request.Body.Close()
 		}