Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
tscolari opened this issue Apr 6, 2018 · 2 comments
Open

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

tscolari opened this issue Apr 6, 2018 · 2 comments

Comments

@tscolari
Copy link

tscolari commented Apr 6, 2018

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
Copy link
Author

tscolari commented Apr 6, 2018

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
Copy link
Author

tscolari commented Apr 6, 2018

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant