Skip to content

Commit

Permalink
Rework the Rails integration
Browse files Browse the repository at this point in the history
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 honeycombio#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 honeycombio#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 honeycombio#39, but
  ultimately is a problem that should be fixed upstream.

* Fixes honeycombio#49, since the parameter encoding won't matter if we never try
  to parse them. Renders honeycombio#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.
  • Loading branch information
ajvondrak committed Feb 22, 2020
1 parent 69a02e7 commit ec737fb
Showing 1 changed file with 70 additions and 25 deletions.
95 changes: 70 additions & 25 deletions lib/honeycomb/integrations/rails.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit ec737fb

Please sign in to comment.