Add middleware/interceptors #41
Conversation
nexus/operation.go
Outdated
|
|
||
| type OperationInvoker[I, O any] interface { |
There was a problem hiding this comment.
Maybe we should call this OperationHandler since it similar to the Handler interface but for a single Operation?
There was a problem hiding this comment.
Oh yeah we have the name OperationHandler available in Go, that is a better name
nexus/operation.go
Outdated
| // CancelOperation implements Handler. | ||
| func (r *registryHandler) CancelOperation(ctx context.Context, service, operation string, token string, options CancelOperationOptions) error { | ||
| s, ok := r.services[service] | ||
| func (r *registryHandler) getOperation(options OperationOptions) (OperationInvoker[any, any], error) { |
There was a problem hiding this comment.
nit: Go doesn't like the get prefix for getters.
| func (r *registryHandler) getOperation(options OperationOptions) (OperationInvoker[any, any], error) { | |
| func (r *registryHandler) operationHandler(options OperationOptions) (OperationHandler[any, any], error) { |
nexus/operation.go
Outdated
| // OperationOptions contains the general options for an operation, across different handlers. | ||
| // | ||
| // NOTE: Experimental | ||
| type OperationOptions struct { |
There was a problem hiding this comment.
Not loving the name Options but I can see that it's consistent across all methods and every method has its own Options struct.
Maybe we should call this MiddlewareOptions or MiddlewareInfo?
There was a problem hiding this comment.
Alternatively we could call this something like CommonOperationOptions and embedded it in every other operation option struct
There was a problem hiding this comment.
No need to embed it, the options are used both in the client and the server and this information would be redundant.
There was a problem hiding this comment.
Another option is HandlerInfo, does that seem better to you?
There was a problem hiding this comment.
hmm it is OK
nexus/operation.go
Outdated
| // ServiceName is the name of the service that contains the operation. | ||
| ServiceName string | ||
| // OperationName is the name of the operation. | ||
| OperationName string |
There was a problem hiding this comment.
I think we can drop the Name suffix here but don't have a strong opinion.
| return h.Cancel(ctx, operationID, options) | ||
| } | ||
|
|
||
| // operationHandlerInfo implements Handler. |
| return h.GetInfo(ctx, operationID, options) | ||
| } | ||
|
|
||
| // operationHandlerResult implements Handler. |
nexus/operation.go
Outdated
| // If the middleware wants to stop the chain before any handler is called, it can return an error. | ||
| // | ||
| // NOTE: Experimental | ||
| type MiddlewareFunc func(HandlerInfo, OperationHandler[any, any]) (OperationHandler[any, any], error) |
There was a problem hiding this comment.
I'm wondering if we should just put HandlerInfo on the Go context, it could be useful outside of middleware context.
You also will want to expose the Go context here to allow middleware to call functions that require a context.
There was a problem hiding this comment.
Hmm, so the HandlerInfo contains the headers that the respective handler inputs already have. Would we be OK moving them out of the handler input into the HandlerInfo ?
There was a problem hiding this comment.
I'm on the fence here. I kept going back and forth between headers in the context and explicit argument and that's where I landed. We should discuss this offline before we stabilize the API.
nexus/operation.go
Outdated
| return nil, HandlerErrorf(HandlerErrorTypeNotFound, "operation %q not found", operation) | ||
| } | ||
|
|
||
| func (r *rootOperationHandler) GetInfo(ctx context.Context, token string, options GetOperationInfoOptions) (*OperationInfo, error) { | ||
| // NOTE: We could avoid reflection here if we put the Cancel method on RegisterableOperation but it doesn't seem |
There was a problem hiding this comment.
Noticed my own copy pasta.
| // NOTE: We could avoid reflection here if we put the Cancel method on RegisterableOperation but it doesn't seem | |
| // NOTE: We could avoid reflection here if we put the GetInfo method on RegisterableOperation but it doesn't seem |
bergundy
force-pushed
the
interceptors
branch
2 times, most recently
from
ef1bfcb to
62c5ffc
Compare
bergundy
force-pushed
the
interceptors
branch
from
62c5ffc to
0104867
Compare
|
Verified with the Temporal SDK, good to merge now. |
Taken over by @bergundy (thanks @Quinn-With-Two-Ns for kicking this off).
💥 BREAKING CHANGE 💥 The experimental
WithHandlerContextmethod signature was changed to accept the associatedHandlerInfo.Do not merge until the Temporal SDK work that depends on this is ready for review.