gocardless / coach

Alternative controllers with middleware
MIT License
165 stars 14 forks source link

Not compliant with `Rails.application.routes.recognize_path` #44

Open tscolari opened 6 years ago

tscolari commented 6 years ago

Apparently routes registered with coach do not show as true by the Rails.application.routes.recognize_path method. As a side effect, some gems that might use this method will not work for those endpoints. e.g. https://github.com/openzipkin/zipkin-ruby -> https://github.com/openzipkin/zipkin-ruby/blob/master/lib/zipkin-tracer/rack/zipkin-tracer.rb#L43.

I've being trying different instrumentation tools and they all failed to report, this one (zipkin) was the first I dug in why, but I suspect this might be affecting others too.

tscolari commented 6 years ago

More digging for future investigation.

Debugging the method points that the route is registered to the application but, deep inside actionpack-5.1.6/lib/action_dispatch/routing/route_set.rb @ line 869 ActionDispatch::Routing::RouteSet#recognize_path: :

    868:           app = route.app
 => 869:           if app.matches?(req) && app.dispatcher?
    870:             begin
    871:               req.controller_class

when using a normal rails route, app.dispatcher? will return true. It's a ActionDispatch::Routing::RouteSet::Dispatcher object, and this is the dispatch? definition:

# actionpack-5.1.6/lib/action_dispatch/routing/endpoint.rb @ line 6 ActionDispatch::Routing::Endpoint#matches?:
 => 6: def matches?(req); true;  end

But when using coach, instead of a Dispatcher we have a Mapper ActionDispatch::Routing::Mapper::Constraints. And this is the dispatch? definition there:

# actionpack-5.1.6/lib/action_dispatch/routing/mapper.rb @ line 34 ActionDispatch::Routing::Mapper::Constraints#dispatcher?:
 => 34: def dispatcher?; @strategy == SERVE; end

And @strategy is a Proc, so is SERVE, but they don't match:

2.5.0 (#<ActionDispatch::Routing::Mapper::Constraints:0x00007fc7638b3f58>):0 > show-source @strategy

From: .../.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/actionpack-5.1.6/lib/action_dispatch/routing/mapper.rb
Number of lines: 1

CALL  = ->(app, req) { app.call req.env }
2.5.0 (#<ActionDispatch::Routing::Mapper::Constraints:0x00007fc7638b3f58>):0 > show-source SERVE

From: .../.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/actionpack-5.1.6/lib/action_dispatch/routing/mapper.rb
Number of lines: 1

SERVE = ->(app, req) { app.serve req }

The 2 are defined in here: https://github.com/rails/rails/blob/v5.1.6/actionpack/lib/action_dispatch/routing/mapper.rb#L16-L17

Not sure why we have CALL instead of SERVE, yet.

tscolari commented 6 years ago

Cause of why, I think, it's not a SERVE:

https://github.com/rails/rails/blob/v5.1.6/actionpack/lib/action_dispatch/routing/mapper.rb#L294-L304

          def app(blocks)
            if to.respond_to?(:action)
              Routing::RouteSet::StaticDispatcher.new to
            elsif to.respond_to?(:call)
              Constraints.new(to, blocks, Constraints::CALL)
            elsif blocks.any?
              Constraints.new(dispatcher(defaults.key?(:controller)), blocks, Constraints::SERVE)
            else
              dispatcher(defaults.key?(:controller))
            end
          end

Probably our middlewares/routes need to quack more like a controller.