From 6b6a0b8f6ff39c2508c70570200d9e852c7e7205 Mon Sep 17 00:00:00 2001 From: Akshay Shah Date: Tue, 18 Apr 2023 11:28:15 -0700 Subject: [PATCH] Add APIs to make and handle conditional GETs (#494) If servers want clients to cache responses to HTTP GETs, they'll also need a mechanism to return HTTP 304s. Unfortunately, we can't introduce a new status code without breaking our promise that clients can switch protocols with just a configuration flag, so we need a sentinel error instead. --- .golangci.yml | 7 ++++ client_ext_test.go | 56 ++++++++++++++++++++++++++ error.go | 34 ++++++++++++++++ error_example_test.go | 35 +++++++++++++++++ error_not_modified_example_test.go | 63 ++++++++++++++++++++++++++++++ protocol_connect.go | 13 +++++- 6 files changed, 207 insertions(+), 1 deletion(-) create mode 100644 error_not_modified_example_test.go diff --git a/.golangci.yml b/.golangci.yml index e70e6773..d2735f98 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -91,3 +91,10 @@ issues: # No output assertions needed for these examples. - linters: [testableexamples] path: error_writer_example_test.go + - linters: [testableexamples] + path: error_not_modified_example_test.go + - linters: [testableexamples] + path: error_example_test.go + # In examples, it's okay to use http.ListenAndServe. + - linters: [gosec] + path: error_not_modified_example_test.go diff --git a/client_ext_test.go b/client_ext_test.go index e448bd59..6c18b38e 100644 --- a/client_ext_test.go +++ b/client_ext_test.go @@ -145,6 +145,62 @@ func TestClientPeer(t *testing.T) { }) } +func TestGetNotModified(t *testing.T) { + t.Parallel() + + const etag = "some-etag" + // Handlers should automatically set Vary to include request headers that are + // part of the RPC protocol. + expectVary := []string{"Accept-Encoding"} + + mux := http.NewServeMux() + mux.Handle(pingv1connect.NewPingServiceHandler(¬ModifiedPingServer{etag: etag})) + server := httptest.NewUnstartedServer(mux) + server.EnableHTTP2 = true + server.StartTLS() + t.Cleanup(server.Close) + + client := pingv1connect.NewPingServiceClient( + server.Client(), + server.URL, + connect.WithHTTPGet(), + ) + ctx := context.Background() + // unconditional request + res, err := client.Ping(ctx, connect.NewRequest(&pingv1.PingRequest{})) + assert.Nil(t, err) + assert.Equal(t, res.Header().Get("Etag"), etag) + assert.Equal(t, res.Header().Values("Vary"), expectVary) + + conditional := connect.NewRequest(&pingv1.PingRequest{}) + conditional.Header().Set("If-None-Match", etag) + _, err = client.Ping(ctx, conditional) + assert.NotNil(t, err) + assert.Equal(t, connect.CodeOf(err), connect.CodeUnknown) + assert.True(t, connect.IsNotModifiedError(err)) + var connectErr *connect.Error + assert.True(t, errors.As(err, &connectErr)) + assert.Equal(t, connectErr.Meta().Get("Etag"), etag) + assert.Equal(t, connectErr.Meta().Values("Vary"), expectVary) +} + +type notModifiedPingServer struct { + pingv1connect.UnimplementedPingServiceHandler + + etag string +} + +func (s *notModifiedPingServer) Ping( + _ context.Context, + req *connect.Request[pingv1.PingRequest]) (*connect.Response[pingv1.PingResponse], error) { + if len(req.Peer().Query) > 0 && req.Header().Get("If-None-Match") == s.etag { + return nil, connect.NewNotModifiedError(http.Header{"Etag": []string{s.etag}}) + } + resp := connect.NewResponse(&pingv1.PingResponse{}) + resp.Header().Set("Etag", s.etag) + return resp, nil +} + type assertPeerInterceptor struct { tb testing.TB } diff --git a/error.go b/error.go index 16fb11c3..aac26909 100644 --- a/error.go +++ b/error.go @@ -31,6 +31,14 @@ const ( defaultAnyResolverPrefix = "type.googleapis.com/" ) +var ( + // errNotModified signals Connect-protocol responses to GET requests to use the + // 304 Not Modified HTTP error code. + errNotModified = errors.New("not modified") + // errNotModifiedClient wraps ErrNotModified for use client-side. + errNotModifiedClient = fmt.Errorf("HTTP 304: %w", errNotModified) +) + // An ErrorDetail is a self-describing Protobuf message attached to an [*Error]. // Error details are sent over the network to clients, which can then work with // strongly-typed data rather than trying to parse a complex error message. For @@ -151,6 +159,24 @@ func IsWireError(err error) bool { return se.wireErr } +// NewNotModifiedError indicates that the requested resource hasn't changed. It +// should be used only when handlers wish to respond to conditional HTTP GET +// requests with a 304 Not Modified. In all other circumstances, including all +// RPCs using the gRPC or gRPC-Web protocols, it's equivalent to sending an +// error with [CodeUnknown]. The supplied headers should include Etag, +// Cache-Control, or any other headers required by [RFC 9110 § 15.4.5]. +// +// Clients should check for this error using [IsNotModifiedError]. +// +// [RFC 9110 § 15.4.5]: https://httpwg.org/specs/rfc9110.html#status.304 +func NewNotModifiedError(headers http.Header) *Error { + err := NewError(CodeUnknown, errNotModified) + if headers != nil { + err.meta = headers + } + return err +} + func (e *Error) Error() string { message := e.Message() if message == "" { @@ -214,6 +240,14 @@ func (e *Error) detailsAsAny() []*anypb.Any { return anys } +// IsNotModifiedError checks whether the supplied error indicates that the +// requested resource hasn't changed. It only returns true if the server used +// [NewNotModifiedError] in response to a Connect-protocol RPC made with an +// HTTP GET. +func IsNotModifiedError(err error) bool { + return errors.Is(err, errNotModified) +} + // errorf calls fmt.Errorf with the supplied template and arguments, then wraps // the resulting error. func errorf(c Code, template string, args ...any) *Error { diff --git a/error_example_test.go b/error_example_test.go index d79fe7ff..f4d5fa26 100644 --- a/error_example_test.go +++ b/error_example_test.go @@ -15,10 +15,14 @@ package connect_test import ( + "context" "errors" "fmt" + "net/http" "github.com/bufbuild/connect-go" + pingv1 "github.com/bufbuild/connect-go/internal/gen/connect/ping/v1" + "github.com/bufbuild/connect-go/internal/gen/connect/ping/v1/pingv1connect" ) func ExampleError_Message() { @@ -33,3 +37,34 @@ func ExampleError_Message() { // Output: // underlying error message: failed to foo } + +func ExampleIsNotModifiedError() { + // Assume that the server from NewNotModifiedError's example is running on + // localhost:8080. + client := pingv1connect.NewPingServiceClient( + http.DefaultClient, + "http://localhost:8080", + // Enable client-side support for HTTP GETs. + connect.WithHTTPGet(), + ) + req := connect.NewRequest(&pingv1.PingRequest{Number: 42}) + first, err := client.Ping(context.Background(), req) + if err != nil { + fmt.Println(err) + return + } + // If the server set an Etag, we can use it to cache the response. + etag := first.Header().Get("Etag") + if etag == "" { + fmt.Println("no Etag in response headers") + return + } + fmt.Println("cached response with Etag", etag) + // Now we'd like to make the same request again, but avoid re-fetching the + // response if possible. + req.Header().Set("If-None-Match", etag) + _, err = client.Ping(context.Background(), req) + if connect.IsNotModifiedError(err) { + fmt.Println("can reuse cached response") + } +} diff --git a/error_not_modified_example_test.go b/error_not_modified_example_test.go new file mode 100644 index 00000000..c6f6e0d9 --- /dev/null +++ b/error_not_modified_example_test.go @@ -0,0 +1,63 @@ +// Copyright 2021-2023 Buf Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package connect_test + +import ( + "context" + "fmt" + "net/http" + + "github.com/bufbuild/connect-go" + pingv1 "github.com/bufbuild/connect-go/internal/gen/connect/ping/v1" + "github.com/bufbuild/connect-go/internal/gen/connect/ping/v1/pingv1connect" +) + +// ExampleCachingServer is an example of how servers can take advantage the +// Connect protocol's support for HTTP-level caching. The Protobuf +// definition for this API is in proto/connect/ping/v1/ping.proto. +type ExampleCachingPingServer struct { + pingv1connect.UnimplementedPingServiceHandler +} + +// Ping is idempotent and free of side effects (and the Protobuf schema +// indicates this), so clients using the Connect protocol may call it with HTTP +// GET requests. This implementation uses Etags to manage client-side caching. +func (*ExampleCachingPingServer) Ping( + _ context.Context, + req *connect.Request[pingv1.PingRequest], +) (*connect.Response[pingv1.PingResponse], error) { + resp := connect.NewResponse(&pingv1.PingResponse{ + Number: req.Msg.Number, + }) + // Our hashing logic is simple: we use the number in the PingResponse. + hash := fmt.Sprint(resp.Msg.Number) + // If the request was an HTTP GET (which always has URL query parameters), + // we'll need to check if the client already has the response cached. + if len(req.Peer().Query) > 0 { + if req.Header().Get("If-None-Match") == hash { + return nil, connect.NewNotModifiedError(http.Header{ + "Etag": []string{hash}, + }) + } + resp.Header().Set("Etag", hash) + } + return resp, nil +} + +func ExampleNewNotModifiedError() { + mux := http.NewServeMux() + mux.Handle(pingv1connect.NewPingServiceHandler(&ExampleCachingPingServer{})) + _ = http.ListenAndServe("localhost:8080", mux) +} diff --git a/protocol_connect.go b/protocol_connect.go index 6b935c51..0232b1cd 100644 --- a/protocol_connect.go +++ b/protocol_connect.go @@ -518,7 +518,12 @@ func (cc *connectUnaryClientConn) validateResponse(response *http.Response) *Err cc.compressionPools.CommaSeparatedNames(), ) } - if response.StatusCode != http.StatusOK { + if response.StatusCode == http.StatusNotModified && cc.Spec().IdempotencyLevel == IdempotencyNoSideEffects { + serverErr := NewWireError(CodeUnknown, errNotModifiedClient) + // RFC 9110 doesn't allow trailers on 304s, so we only need to include headers. + serverErr.meta = cc.responseHeader.Clone() + return serverErr + } else if response.StatusCode != http.StatusOK { unmarshaler := connectUnaryUnmarshaler{ reader: response.Body, compressionPool: cc.compressionPools.Get(compression), @@ -689,6 +694,12 @@ func (hc *connectUnaryHandlerConn) ResponseTrailer() http.Header { func (hc *connectUnaryHandlerConn) Close(err error) error { if !hc.wroteBody { hc.writeResponseHeader(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) { + hc.responseWriter.WriteHeader(http.StatusNotModified) + return hc.request.Body.Close() + } } if err == nil { return hc.request.Body.Close()