Add WithConditionalHandlerOptions for conditional options

[emcfarlane]

Allows servers to set specific configuration based on the methods Spec.
This can be used to configure a specific route with different limits, such as:

connect.WithConditionalHandlerOptions(func(spec connect.Spec) (options []connect.HandlerOption) {
	if spec.Procedure == pingv1connect.PingServicePingProcedure {
		options = append(options, connect.WithReadMaxBytes(1024))
	}
	return options
})

[jhump]

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?

[akshayjshah]

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?

[emcfarlane]

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.

[emcfarlane]

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.

[jhump]

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.

[emcfarlane]

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?

[jhump]

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.

[emcfarlane]

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.

[jhump]

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.

[emcfarlane]

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).

[jhump]

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?

[akshayjshah]

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)
}

[emcfarlane]

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.

[jhump]

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. 👍

[akshayjshah]

Small nit: rather than assuming that the reader remembers all the available options, let's refer to the option directly:

Options supplied via [WithConditionalHandlerOptions] are ignored.

Also, this isn't accurate. We're not ignoring conditional options - we're applying them as though the Spec is unary. To actually ignore conditional options, we'd need to recursively flatten options (expanding out WithOptions and friends), then strip out the conditional options. Alternatively, we could flatten and apply all options (ignoring the conditional func), so we're sure that we get the superset of all codecs.

@jhump and @emcfarlane - what do you want to do here?

[emcfarlane]

The conditional function is applied but the options the conditional has are skipped by:

	if spec.Procedure == "" {
		return // ignore empty specs
	}

[akshayjshah]

Ah, you're right. I totally missed that 🤦🏽‍♂️ That's ok with me, then.