Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add APIs to make and handle conditional GETs #494

Merged
merged 2 commits into from
Apr 18, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .golangci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
56 changes: 56 additions & 0 deletions client_ext_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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(&notModifiedPingServer{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
}
Expand Down
34 changes: 34 additions & 0 deletions error.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 == "" {
Expand Down Expand Up @@ -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 {
Expand Down
35 changes: 35 additions & 0 deletions error_example_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand All @@ -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")
}
}
63 changes: 63 additions & 0 deletions error_not_modified_example_test.go
Original file line number Diff line number Diff line change
@@ -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)
}
13 changes: 12 additions & 1 deletion protocol_connect.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible for the handler to detect whether the request was a GET vs a POST? Would that possibly be a better check here than just looking if the method is side-effect-free?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jchadwick-buf, do you know the answer to above? I think either way, we can probably go ahead and merge this PR. If the answer is "no, there isn't yet a way for a handler to detect if current request was GET vs POST", then that is something that can be tacked on later.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, no - currently, there's no way for clients to easily detect whether the request was a GET or a POST. (The GET/POST decision happens after interceptors run, so there wasn't any need for such an API before.) We could add a way to detect GETs directly in a later PR - it's feasible, but would be a bit gross.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just talking about handlers, not clients.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's not a great way to tell with handlers, though as a hack it is possible to inspect the Query value on Peer.

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),
Expand Down Expand Up @@ -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()
Expand Down