-
Notifications
You must be signed in to change notification settings - Fork 107
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 WithConditionalHandlerOptions for conditional options #538
Conversation
Allows servers to set specific configuration based on the methods Spec.
error_writer.go
Outdated
@@ -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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
- Add a
WithStreamType
handler option and document that it only affectsErrorWriter
. - Add an
ErrorWriter.SetStreamType
method, which alters the default and reconfigures the error writer. - 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?
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 toStreamTypeUnary
.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
option.go
Outdated
// } | ||
// return nil | ||
// }) | ||
type WithHandlerOptional func(Spec) HandlerOption |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
}
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. 👍
5163a46
to
427242d
Compare
427242d
to
b9ed035
Compare
error_writer.go
Outdated
@@ -40,8 +40,9 @@ type ErrorWriter struct { | |||
// NewErrorWriter constructs an ErrorWriter. To properly recognize supported | |||
// RPC Content-Types in net/http middleware, you must pass the same | |||
// HandlerOptions to NewErrorWriter and any wrapped Connect handlers. | |||
// Conditional options are ignored. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The conditional function is applied but the options the conditional has are skipped by:
if spec.Procedure == "" {
return // ignore empty specs
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, you're right. I totally missed that 🤦🏽♂️ That's ok with me, then.
d23ca1a
to
11df7c4
Compare
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: ```go connect.WithConditionalHandlerOptions(func(spec connect.Spec) (options []connect.HandlerOption) { if spec.Procedure == pingv1connect.PingServicePingProcedure { options = append(options, connect.WithReadMaxBytes(1024)) } return options }) ``` --------- Co-authored-by: Akshay Shah <akshayjshah@users.noreply.github.com>
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: