gorilla/rpc

consider making `request` field in json2.CodecRequest public

mediocregopher opened this issue · 6 comments

I'm using the github.com/gorilla/rpc/v2/json2 package for implementing a json rpc 2 server. I'd like to write a middleware-like wrapper around it to do some logging automatically, since we want that functionality for all of our projects that use an rpc server. There's two ways I could go about doing this, that I can see:

  1. Write a proper http.Handler wrapper. The problem with this is that I'd have to decode the json at this point, and then within the rpc handler it would be decoding it again, duplicating work. I'd also need a struct to decode into, serverRequest is private so I'd have to copy-paste that out so my wrapper could see, something I don't want to do. Additionally I'd like to be able to log any rpc errors the server is returning to the client. an http.Handler wrapper would have a lot of difficulty doing this, since it can't get the response's output afaik, and serverResponse is private besides.

  2. Write a wrapper around json2.Codec and json2.CodecRequest which passes everything back to those, but only after logging about it. The problem with this approach is that json2.CodecRequest doesn't expose any information about the request, except for the method name via the Method() method.

My solution would be to make the serverRequest type public, make its instance in CodecRequest (called request) public, and add a field onto CodecRequest which can house the original *http.Request and is public as well. Doing these things would make writing wrappers around the interface much more approachable.

I'm happy to do the work for this and submit a PR, if the maintainers of this project would be open to the idea. Thanks!

actually, on further review, I don't think it's necessary to add *http.Request to CodecRequest, my wrapper around Codec has access to that so I can pass it along from that point.

It sounds like a reasonable addition. Would be good to see an example of it working before deciding whether it's a good way to do it or not. The wrapper idea sounds promising though.

Sweet! I made a PR with the changes I was envisioning, let me know what you think :)

Thanks. Do you have an example of it being used somewhere? Just in a gist or something would be fine.

Good thing you had me do that, in doing it I realized that it's not actually necessary to change anything on gorilla's side, because the reply argument to ReadRequest has the json unmarshalled into it. So All the necessary information is available anyway. I'll go ahead and close this and my PR.

For the future generations who are curious how a wrapper might work, here's the wrapper code, and some example code which uses it: https://gist.github.com/mediocregopher/f35d61ffdec752a90ef4

Cool, thanks for documenting it. Might be good to have in the package docs if I get around to it...