From ec737fb965de7ae77af6097eafb8074472e74728 Mon Sep 17 00:00:00 2001 From: Alex Vondrak Date: Fri, 21 Feb 2020 23:15:09 -0800 Subject: [PATCH] Rework the Rails integration This refactoring avoids ever parsing GET/POST parameters to obtain the Rails routing information required for the `request.action`, `request.controller`, and `request.route` fields. This is accomplished in two ways: 1. Use `ActionDispatch::Request#path_parameters` instead of `ActionDispatch::Request#params`. The former is the subset of data we need for `request.action` + `request.controller`, and in fact are the values merged into the latter hash after `#params` parses the normal GET/POST parameters. So we can avoid the extraneous parsing by just getting the path parameters straight from the horse's mouth. 2. Call `ActionDispatch::Journey::Router#recognize` on a simplified request that *only* contains the necessary routing information. The `#recognize` method will yield the `ActionDispatch::Request#params` when it matches a route, which again triggers the GET/POST parsing. But we don't care about the HTTP parameters. So instead I copied the approach of `ActionDispatch::Routing::RouteSet#recognize_path`, which reconstructs a simplified request with just the information that it needs. (Unfortunately we can't use `#recognize_path` directly because instead of returning the route it returns the path parameters, which we can already get directly from `#path_parameters`.) This accomplishes several things: * Fixes #62, since we don't get a chance to erroneously fall back to GET/POST parameters in the event that the routing information is missing (due to requests to unrecognized routes). * Fixes #31, since the bug ultimately comes from the twirp gem causing POST parameter parsing to fail. But if we never parse the HTTP parameters, no problem. This is a more direct workaround than #39, but ultimately is a problem that should be fixed upstream. * Fixes #49, since the parameter encoding won't matter if we never try to parse them. Renders #50 unnecessary, since we won't have a chance to trigger the exceptions in question. * Gives us more data in more situations. The new implementation of the `request.route` field now comes up non-nil in the event of errors because we use the `ActionDispatch::Request#original_fullpath` and not the `#path_info` that gets [rewritten by `ActionDispatch::ShowExceptions`](https://github.com/rails/rails/blob/758e4f8406e680a6cbf21b170749202c537a2576/actionpack/lib/action_dispatch/middleware/show_exceptions.rb#L49). * Gives us more data in more Rails versions. With the addition of actual tests for the field values, it became clear that `request.route` was missing in Rails 4, simply because of an interface change introduced in Rails 5. --- lib/honeycomb/integrations/rails.rb | 95 +++++++++++++++++++++-------- 1 file changed, 70 insertions(+), 25 deletions(-) diff --git a/lib/honeycomb/integrations/rails.rb b/lib/honeycomb/integrations/rails.rb index 26a614ad..c32b1eca 100644 --- a/lib/honeycomb/integrations/rails.rb +++ b/lib/honeycomb/integrations/rails.rb @@ -11,34 +11,79 @@ def add_package_information(env) yield "meta.package", "rails" yield "meta.package_version", ::Rails::VERSION::STRING - ::ActionDispatch::Request.new(env).tap do |request| - # calling request.params will blow up if raw_post is nil - # the only known cause of this is when using the - # [twirp](https://github.com/twitchtv/twirp-ruby) rack app mounted in - # the rails app - if request.raw_post - begin - yield "request.controller", request.params[:controller] - yield "request.action", request.params[:action] - rescue ::ActionController::BadRequest => e - yield "request.controller", e.message - yield "request.action", e.message - end - end - - break unless request.respond_to? :routes - break unless request.routes.respond_to? :router - - found_route = false - request.routes.router.recognize(request) do |route, _| - break if found_route - - found_route = true - yield "request.route", "#{env['REQUEST_METHOD']} #{route.path.spec}" - end + request = ::ActionDispatch::Request.new(env) + + yield "request.controller", request.path_parameters[:controller] + yield "request.action", request.path_parameters[:action] + yield "request.route", route_for(request) + end + + private + + def route_for(request) + router = router_for(request) + routing = routing_for(request) + + return unless router && routing + + router.recognize(routing) do |route, _| + return "#{request.method} #{route.path.spec}" end end + # Broadly compatible way of getting the ActionDispatch::Routing::RouteSet. + # + # While we'd like to just use ActionDispatch::Request#routes, that method + # was only added circa Rails 5. To support Rails 4, we have to use direct + # Rack env access. + # + # @see https://github.com/rails/rails/commit/87a75910640b83a677099198ccb3317d9850c204 + def router_for(request) + routes = request.env["action_dispatch.routes"] + routes.router if routes.respond_to?(:router) + end + + # Constructs a simplified ActionDispatch::Request with the original route. + # + # This is based on ActionDispatch::Routing::RouteSet#recognize_path, which + # reconstructs an ActionDispatch::Request using a given HTTP method + path + # by making a mock Rack environment. Here, instead of taking the method + + # path from input parameters, we use the original values from the actual + # incoming request (prior to any mangling that may have been done by + # middleware). + # + # The resulting ActionDispatch::Request instance is suitable for passing to + # ActionDispatch::Journey::Router#recognize to get the original Rails + # routing information corresponding to the incoming request. + # + # @param request [ActionDispatch::Request] + # the actual incoming Rails request + # + # @return [ActionDispatch::Request] + # a simplified version of the incoming request that retains the original + # routing information, but nothing else (e.g., no HTTP parameters) + # + # @return [nil] + # if the original request's path is invalid + # + # @see https://api.rubyonrails.org/classes/ActionDispatch/Request.html#method-i-method + # @see https://api.rubyonrails.org/classes/ActionDispatch/Request.html#method-i-original_fullpath + # @see https://github.com/rails/rails/blob/2a44ff12c858d296797963f7aa97abfa0c840a15/actionpack/lib/action_dispatch/journey/router/utils.rb#L7-L27 + # @see https://github.com/rails/rails/blob/2a44ff12c858d296797963f7aa97abfa0c840a15/actionpack/lib/action_dispatch/routing/route_set.rb#L846-L859 + def routing_for(request) + verb = request.method + path = request.original_fullpath + path = normalize(path) unless path =~ %r{://} + env = ::Rack::MockRequest.env_for(path, method: verb) + ::ActionDispatch::Request.new(env) + rescue URI::InvalidURIError + nil + end + + def normalize(path) + ::ActionDispatch::Journey::Router::Utils.normalize_path(path) + end + # Rails middleware class Middleware include Rack