arthurnn / twirp-ruby

Twirp services in Ruby
Apache License 2.0
152 stars 59 forks source link

Equivalent service hook to Go's RequestReceived? #31

Closed iancanderson closed 5 years ago

iancanderson commented 5 years ago

Hello, I was wondering if there were plans to expose a service hook in twirp-ruby that's the equivalent of RequestReceived in the go library: https://github.com/twitchtv/twirp/blob/ad6d075411e462f08e971d7efc18f4a770ac8b40/hooks.go#L23-L27

We're looking to hook in to calculate a signature on the encoded payload, so we're currently re-encoding in a before hook for the purposes of calculating the signature.

It seems like the twirp-ruby before hook is equivalent to the RequestRouted hook in Go, so it feels like there should be an equivalent to RequestReceived as well.

marioizquierdo commented 5 years ago

There's no equivalent to RequestRouted in twirp-ruby, because the service handler is a Rack.app already (see service handlers in the docs).

Resources for Rack middleware:

In a Rack application, you can easily write your own middleware. For example:

class CalculateSignature
  def initialize(app)
    @app = app       
  end                

  def call(env)      
    // do stuff before, you can return with status codes and such
    @app.call(env)   
    // do stuff after is also possible
  end                
end

Then mount that middleware in your rack stack before the Twirp handler (depends if you are using Rails or something else).

Rack responses are in the form [status, headers, response_body]. If you need to return a Twirp error from the Rack middleware, you can use some helpers in the library:

twerr = Twirp::Error.invalid_argument("invalid encoded signature", foo: "bar-meta")

status = Twirp::ERROR_CODES_TO_HTTP_STATUS[twerr.code]
headers = {'Content-Type' => 'application/json'}
return [status, headers, [JSON.generate(twerr.to_h)]]

There are other implications like triggering error hooks from middleware and such. Adding a before_request_routed hook would simplify some of this, but it would also add other problems. For example, right now the on_error hooks always have information about the twirp route, which is useful for reporting. If we halt the request before, then that would not work as expected (could cause nil method_missing exceptions).

mtodd commented 5 years ago

@marioizquierdo ☝️ makes sense! We were thinking we wanted to hook into the Twirp handlers in order to get access to the route information (e.g. rpc_method et al) but all of that can be parsed out from the Rack env in the middleware as well (and is also more consistent with our client middleware approach).

Our current approach with the before hook needs the raw request body, but the before hook is only given the decoded input so we end up re-encoding that (instead of reading from rack_env["rack.input"] to avoid rewinding the input stream repeatedly). Using the middleware forces us to use the Rack input stream but we'll be early enough that we can read and rewind without issue.

Thanks for the pointer on reconstructing the Twirp error response, that's something that's not great but entirely workable.

marioizquierdo commented 5 years ago

The Twirp::Service.error_response(twerr) helper was added in release v1.3.0.

marioizquierdo commented 5 years ago

Added Rack Middleware example in the wiki: https://github.com/twitchtv/twirp-ruby/wiki/Service-Hooks

iancanderson commented 5 years ago

Thanks @marioizquierdo !