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.

Sign up for GitHub

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

Jump to bottom

Add WithConditionalHandlerOptions for conditional options #538

Merged
merged 4 commits into from
Jul 13, 2023
Merged

Add WithConditionalHandlerOptions for conditional options #538

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cec2a2a
Add WithHandlerOptional for method specific option
emcfarlane Jun 28, 2023
b9ed035
Fix naming and error handler sentinel type
emcfarlane Jul 3, 2023
11df7c4
Add example for conditional option
emcfarlane Jul 12, 2023
4ef063d
Merge branch 'main' into emcfarlane/handler-optional
akshayjshah Jul 13, 2023
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
10 changes: 5 additions & 5 deletions compression_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,35 +128,35 @@ func TestHandlerCompressionOptionTest(t *testing.T) {

t.Run("defaults", func(t *testing.T) {
t.Parallel()
config := newHandlerConfig(testProc, nil)
config := newHandlerConfig(testProc, StreamTypeUnary, nil)
assert.Equal(t, config.CompressionNames, []string{compressionGzip})
checkPools(t, config)
})
t.Run("WithCompression", func(t *testing.T) {
t.Parallel()
opts := []HandlerOption{WithCompression("foo", dummyDecompressCtor, dummyCompressCtor)}
config := newHandlerConfig(testProc, opts)
config := newHandlerConfig(testProc, StreamTypeUnary, opts)
assert.Equal(t, config.CompressionNames, []string{compressionGzip, "foo"})
checkPools(t, config)
})
t.Run("WithCompression-empty-name-noop", func(t *testing.T) {
t.Parallel()
opts := []HandlerOption{WithCompression("", dummyDecompressCtor, dummyCompressCtor)}
config := newHandlerConfig(testProc, opts)
config := newHandlerConfig(testProc, StreamTypeUnary, opts)
assert.Equal(t, config.CompressionNames, []string{compressionGzip})
checkPools(t, config)
})
t.Run("WithCompression-nil-ctors-noop", func(t *testing.T) {
t.Parallel()
opts := []HandlerOption{WithCompression("foo", nil, nil)}
config := newHandlerConfig(testProc, opts)
config := newHandlerConfig(testProc, StreamTypeUnary, opts)
assert.Equal(t, config.CompressionNames, []string{compressionGzip})
checkPools(t, config)
})
t.Run("WithCompression-nil-ctors-unregisters", func(t *testing.T) {
t.Parallel()
opts := []HandlerOption{WithCompression("gzip", nil, nil)}
config := newHandlerConfig(testProc, opts)
config := newHandlerConfig(testProc, StreamTypeUnary, opts)
assert.Equal(t, config.CompressionNames, nil)
checkPools(t, config)
})
Expand Down
7 changes: 6 additions & 1 deletion connect_ext_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1001,7 +1001,12 @@ func TestHandlerWithReadMaxBytes(t *testing.T) {
readMaxBytes := 1024
mux.Handle(pingv1connect.NewPingServiceHandler(
pingServer{},
connect.WithReadMaxBytes(readMaxBytes),
connect.WithHandlerOptional(func(spec connect.Spec) connect.HandlerOption {
if spec.Procedure == pingv1connect.PingServicePingProcedure {
return connect.WithReadMaxBytes(readMaxBytes)
}
return nil
}),
))
readMaxBytesMatrix := func(t *testing.T, client pingv1connect.PingServiceClient, compressed bool) {
t.Helper()
Expand Down
2 changes: 1 addition & 1 deletion error_writer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ type ErrorWriter struct {
// RPC Content-Types in net/http middleware, you must pass the same
// HandlerOptions to NewErrorWriter and any wrapped Connect handlers.
func NewErrorWriter(opts ...HandlerOption) *ErrorWriter {
config := newHandlerConfig("", opts)
config := newHandlerConfig("", ^StreamType(0), opts)
Copy link
Member

Choose a reason for hiding this comment

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

Is StreamType a bit mask? Why are all bits set here? Would it work just as well to pass an actual valid StreamType (i.e. StreamTypeUnary) and put a comment that it's really just a sentinel value here?

Copy link
Member

@akshayjshah akshayjshah Jun 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This seems like a problematic corner of the design. We're using the options here to know the codecs registered, so that we can understand the content-types that the handler supports. If we're now allowing codecs to be registered conditionally depending on the stream type, we'll have to make sure that the stream type we supply here is correct. I'm not sure how to do that.

The best options I can come up with are:

  1. Add a WithStreamType handler option and document that it only affects ErrorWriter.
  2. Add an ErrorWriter.SetStreamType method, which alters the default and reconfigures the error writer.
  3. Document that conditionally-applied codecs are applied as though the procedure is unary (or bidi, or whatever we decide the sentinel value should be).

Neither of those feel great to me. If I had to choose, I'd probably pick the last option. Can anyone think of a nicer, backward-compatible alternative?

Copy link
Contributor Author

@emcfarlane emcfarlane Jun 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

StreamType is a bit mask with the zero type equal to StreamTypeUnary. Maybe we could document an unknown sentinel type: type StreamTypeUnknown StreamType = ^StreamType(0)? I wanted the zero value not to match real ones to avoid confusing cases where applying options with a spec.StreamType == StreamTypeUnary could be applied to a StreamTypeBidi under error conditions. Probably not an issue though.

A non backward-compatible alternative is to move http handler middlerware onto the handler with an option like:

type HandlerFunc func(w http.ResponseWriter, r *http.Request) error

func WithHandlerMiddleware(middleware func(HandlerFunc) HandlerFunc) HandlerOption

So each error handler is aware of the protocol and can deduce the StreamType. This maybe has too many sharp edges, but it would integrate nicely on the level of Handler.

Copy link
Contributor Author

@emcfarlane emcfarlane Jun 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Would header middleware be fine for most applications?

type HeaderFunc func(responseHeader http.Header, requestHeader http.Header) error

Then the auth example here could be written as:

// NewAuthenticatedHandler middleware to authenticate a RPC request.
func NewAuthenticatedHandler(handler connect.HeaderFunc) connect.HeaderFunc {
	return func(responseHeader http.Header, requestHeader http.Header) error {
		// Dummy authentication logic.
		if requestHeader.Get("Token") == "super-secret" {
			return handler(responseHeader,  requestHeader)
		}
		// Send a protocol-appropriate error to RPC clients, so that they receive
		// the right code, message, and any metadata or error details.
		return connect.NewError(connect.CodeUnauthenticated, errors.New("invalid token"))
	})
}

And going further with the Optional types we can implement per RPC checks based on the Spec. I guess this is too close to interceptors, the only part that is different is not having the messages read.

Copy link
Member

Choose a reason for hiding this comment

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

StreamType is a bit mask with the zero type equal to StreamTypeUnary.

Is that intentional? It's also a simple, sequential enum where the zero value is "unary", so it's unclear if it was meant to be used as a bitmask. Do we actually expect code to use the client and server stream constants as masks to decode whether the stream type supports client and server streaming?

FWIW, this still doesn't make sense to me to use all bits set. That implies that we want bit-wise tests of whether this is a client or server stream to treat it as a bidi stream.

A non backward-compatible alternative

We are past v1.0, so I think incompatible changes are non-starters.

Although TBH, I don't see how that quite solves the issue at hand.

As far as @akshayjshah's proposals, I can think of a fourth one if the shape of WithConditionalHandlerOptions was a predicated and associated options instead of a function that produces options:

  • NewErrorWriter could actually crawl all of the options, using type assertions to find the codec options. For conditional options, it could recursively crawl the set of associated options (no need to apply the predicate). At that point, it has a union of all supported codecs, which should be sufficient to correctly configure the writer.

Copy link
Contributor Author

@emcfarlane emcfarlane Jun 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

unclear if it was meant to be used as a bitmask

I see what you mean, it looks like it would be checked for a streaming client or streaming response which then wouldn't make sense to have either bits set.

crawl the set of associated options (no need to apply the predicate)

I don't understand how this works. You would have all the non conditional options and union them with the conditional?

Copy link
Member

Choose a reason for hiding this comment

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

You would have all the non conditional options and union them with the conditional?

Correct. The error writer really just needs to know what content types are supported. Thinking about this more, I guess this would be problematic if the conditional options were used to override a codec, such that the union of all codecs was ambiguous regarding which codec to use for a particular content type.

I think the only way we could handle that sort of setup would be to provide the Spec somehow to the error writer, similar to what Akshay suggested above. Though I'm now a little confused why the suggestions above focus solely on stream type, and not the whole spec, since the conditional options might have logic based on procedure name, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can easily get the procedure name from the path, given a valid request, but stream type would be harder. I don't think that information is available.

Copy link
Member

Choose a reason for hiding this comment

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

easily get the procedure name from the path

That's only true when using protos (and gRPC conventions). But that's not actually a requirement in the Connect protocol spec. So, used with some other service description convention, the Procedure-Name production in the spec could map to something with fewer or more than two path components, in which case this assumption means we'd compute the wrong string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Set stream type to unary and added a check in applying conditionals to ignore options when the Spec is missing (procedure is an empty string).

writer := &ErrorWriter{
bufferPool: config.BufferPool,
protobuf: newReadOnlyCodecs(config.Codecs).Protobuf(),
Expand Down
24 changes: 13 additions & 11 deletions handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ func NewUnaryHandler[Req, Res any](
}
return res, err
})
config := newHandlerConfig(procedure, options)
config := newHandlerConfig(procedure, StreamTypeUnary, options)
if interceptor := config.Interceptor; interceptor != nil {
untyped = interceptor.WrapUnary(untyped)
}
Expand Down Expand Up @@ -87,9 +87,9 @@ func NewUnaryHandler[Req, Res any](
return conn.Send(response.Any())
}

protocolHandlers := config.newProtocolHandlers(StreamTypeUnary)
protocolHandlers := config.newProtocolHandlers()
return &Handler{
spec: config.newSpec(StreamTypeUnary),
spec: config.newSpec(),
implementation: implementation,
protocolHandlers: mappedMethodHandlers(protocolHandlers),
allowMethod: sortedAllowMethodValue(protocolHandlers),
Expand Down Expand Up @@ -253,9 +253,10 @@ type handlerConfig struct {
BufferPool *bufferPool
ReadMaxBytes int
SendMaxBytes int
StreamType StreamType
}

func newHandlerConfig(procedure string, options []HandlerOption) *handlerConfig {
func newHandlerConfig(procedure string, streamType StreamType, options []HandlerOption) *handlerConfig {
protoPath := extractProtoPath(procedure)
config := handlerConfig{
Procedure: protoPath,
Expand All @@ -264,6 +265,7 @@ func newHandlerConfig(procedure string, options []HandlerOption) *handlerConfig
HandleGRPC: true,
HandleGRPCWeb: true,
BufferPool: newBufferPool(),
StreamType: streamType,
}
withProtoBinaryCodec().applyToHandler(&config)
withProtoJSONCodecs().applyToHandler(&config)
Expand All @@ -274,15 +276,15 @@ func newHandlerConfig(procedure string, options []HandlerOption) *handlerConfig
return &config
}

func (c *handlerConfig) newSpec(streamType StreamType) Spec {
func (c *handlerConfig) newSpec() Spec {
return Spec{
Procedure: c.Procedure,
StreamType: streamType,
StreamType: c.StreamType,
IdempotencyLevel: c.IdempotencyLevel,
}
}

func (c *handlerConfig) newProtocolHandlers(streamType StreamType) []protocolHandler {
func (c *handlerConfig) newProtocolHandlers() []protocolHandler {
protocols := []protocol{&protocolConnect{}}
if c.HandleGRPC {
protocols = append(protocols, &protocolGRPC{web: false})
Expand All @@ -298,7 +300,7 @@ func (c *handlerConfig) newProtocolHandlers(streamType StreamType) []protocolHan
)
for _, protocol := range protocols {
handlers = append(handlers, protocol.NewHandler(&protocolHandlerParams{
Spec: c.newSpec(streamType),
Spec: c.newSpec(),
Codecs: codecs,
CompressionPools: compressors,
CompressMinBytes: c.CompressMinBytes,
Expand All @@ -318,13 +320,13 @@ func newStreamHandler(
implementation StreamingHandlerFunc,
options ...HandlerOption,
) *Handler {
config := newHandlerConfig(procedure, options)
config := newHandlerConfig(procedure, streamType, options)
if ic := config.Interceptor; ic != nil {
implementation = ic.WrapStreamingHandler(implementation)
}
protocolHandlers := config.newProtocolHandlers(streamType)
protocolHandlers := config.newProtocolHandlers()
return &Handler{
spec: config.newSpec(streamType),
spec: config.newSpec(),
implementation: implementation,
protocolHandlers: mappedMethodHandlers(protocolHandlers),
allowMethod: sortedAllowMethodValue(protocolHandlers),
Expand Down
19 changes: 19 additions & 0 deletions option.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,25 @@ func WithRequireConnectProtocolHeader() HandlerOption {
return &requireConnectProtocolHeaderOption{}
}

// WithHandlerOptional is a function that returns a HandlerOption. It's used
// to conditionally apply HandlerOption to a Handler based on the Spec.
// For example, to apply a HandlerOption only to a specific procedure:
//
// connect.WithHandlerOptional(func(spec connect.Spec) connect.HandlerOption {
// if spec.Procedure == pingv1connect.PingServicePingProcedure {
// return connect.WithReadMaxBytes(1024)
// }
// return nil
// })
emcfarlane marked this conversation as resolved.
Show resolved Hide resolved
type WithHandlerOptional func(Spec) HandlerOption
Copy link
Member

Choose a reason for hiding this comment

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

This strikes me a little odd that this one is a type and not a function that returns an option. For consistency and "path of least surprise", maybe this should be a function that returns an unexported type handlerOptionalOption func(Spec) HandlerOption?

Also, the name is not particularly intuitive or self-describing, and the way this works feels a little clunky. I can see why you may not want to just have the user provide a procedure name since they may want to do something more clever in the predicate, such as including matching on stream type instead of procedure name, or matching multiple procedure names. But I think then the predicate may be an explicit parameter, separate from the options. Something like so:

func WithHandlerOptions(predicate func(Spec) bool, opts ... HandlerOption) HandlerOption

To apply this to a single procedure, the code snippet then looks something like so:

connect.WithHandlerOptions(
    func(spec connect.Spec) bool {
        return spec.Procedure == pingv1connect.PingServicePingProcedure
    },
    connect.WithReadMaxBytes(1024),
)

I find this a little more intuitive than a function that can optionally return a non-nil option. It also makes it easier to configure multiple options for the same procedure(s).

WDYT?

Copy link
Member

@akshayjshah akshayjshah Jun 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Unfortunately, we've already got a WithHandlerOptions - it turns a slice of handler options into a single option (which is useful with Ed's design). I don't mind Josh's version either, though - up to you guys which you prefer.

What do you think of the name WithConditionalHandlerOptions? Might be worth shipping WithConditionalOptions and WithConditionalClientOptions too.

If we keep the current approach, I agree with Josh that this is surprisingly different from the other option constructor type signatures. Because of that, it won't get grouped nicely in the GoDoc. If we stick with this approach, I'd go for something like this:

func WithHandlerOptional(f func(Spec) HandlerOption) HandlerOption {
  return handlerOptionalFunc(f)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the single HandlerOption function with a single return allows for better configuration. For example if you have a config and want to optionally apply handler options this can be done in a single option:

connect.WithHandlerOption(func(spec connect.Spec) connect.HandlerOption {
  var options []connect.HandlerOption
  if cfg, ok := config[spec.Procedure]; ok {
    if cfg.ReadMaxLimit > 0 {
      options = append(options, connect.WithReadMaxBytes(cfg.ReadMaxBytes)
    }
    // ...
  }
  return connect.WithHandlerOptions(options...)
})

To avoid the nil case maybe the signature should be:

func WithHandlerOptional(f func(Spec) []HandlerOption) HandlerOption

Which helps point out that there can be zero or more options returned.

Copy link
Member

Choose a reason for hiding this comment

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

That signature feels a little overly tailored to the above use case (where you want to produce per-method options from some separate config source), and might be a little less intuitive for simpler usages. But if we think that's a likely usage pattern, then it is indeed simpler to configure with that signature.

So having the function return a slice, along with a name like WithConditionalHandlerOptions, is a marked improvement IMO. 👍


func (f WithHandlerOptional) applyToHandler(h *handlerConfig) {
spec := h.newSpec()
if option := f(spec); option != nil {
option.applyToHandler(h)
}
}

// Option implements both [ClientOption] and [HandlerOption], so it can be
// applied both client-side and server-side.
type Option interface {
Expand Down