Obviously it's quite natural to just add interceptors as you need them and no doubt there are hundreds of things like this across the Internet.
To some extent, I can't get over how much of a mess you can make by doing things like this. Because generated service definitions have a fixed schema (func (s Service) Method(context.Context, Request) (Reply, error)), you have to resort to hacks like propagating the current user through the context, instead of the easy-to-read and easy-to-test alternative of just passing in the parameters explicitly, as in func (s Service) Method(context.Context, types.Session, Request) (Reply, error). If I was going to spend time on infrastructure, that's the thing I'd fix first.
Some other frameworks do a little better here. We use GraphQL at work, and the methods are typically of the pattern:
func AutogeneratedMethod(context.Context, ...) {
foo := MustGetFoo(ctx)
bar := MustGetBar(ctx)
return ActualThingThatDoesTheWork(ctx, foo, bar, ...)
}
This makes testing the Actual Thing That Does The Work easier, and the reader of that method knows exactly what sort of state the result depends on (the most important goal in my opinion).
I assume your Must functions panic. Is panic-ing in a handler considered okay? I was under the impression that using panics as exceptions in this way was un-idiomatic and thus frowned upon.
If it’s the common practice then I’ll happily jump on board, I miss exceptions. I’m just curious because I’m starting to do more web stuff with Go and had been treating handler recovery as something I should endeavor to never touch, as a final guardrail to keep a server process from exiting.
Generally I agree with you and tend to avoid writing or calling functions that can panic. I make a slight exception in this case because it will "never" hit the error case; you are "always" calling the RPC through an interceptor that adds the necessary value to the context, so your MustGet functions will "always" work. I use "quotes" around never and always because ... sometimes someone edits the code to delete this invariant, and it is very easy to do that. But, it explodes loudly when it panics, so at least you can go in and fix the problem without much effort. (It would be really insidious to return an error and not check it -- some code three functions deep would then blow up with a nil pointer exception; or to return a subtly-not-working default value that cause subtle broken behavior that's hard to detect. Panics are preferred because the program crashes at the exact line of code that's broken.)
For any case where the error would depend on runtime input, rather than compile-time structure, you should return and check an error. If hitting the error case means "we need to edit the code and release a new version", panic is a fine tool to surface that problem.
All in all, I'd basically be happy either way. If you make your GetFoo function return an error that all the handlers check, I'd approve that PR. If you judiciously panic when something that can "never" happen happens, I'd approve that PR. The obvious preference is to update the codegen to pass those parameters in explicitly, so that if the interceptor structure changes, the program will simply not compile. But, the design of things like net/http and grpc kind of preclude easily doing that, and I'm not sure it's worth the effort to write something on top of those that fix the problem, when it's simple enough to do the unpacking manually and your integration tests test it for free anyway.
Obviously it's quite natural to just add interceptors as you need them and no doubt there are hundreds of things like this across the Internet.
To some extent, I can't get over how much of a mess you can make by doing things like this. Because generated service definitions have a fixed schema (func (s Service) Method(context.Context, Request) (Reply, error)), you have to resort to hacks like propagating the current user through the context, instead of the easy-to-read and easy-to-test alternative of just passing in the parameters explicitly, as in func (s Service) Method(context.Context, types.Session, Request) (Reply, error). If I was going to spend time on infrastructure, that's the thing I'd fix first.
Some other frameworks do a little better here. We use GraphQL at work, and the methods are typically of the pattern:
This makes testing the Actual Thing That Does The Work easier, and the reader of that method knows exactly what sort of state the result depends on (the most important goal in my opinion).