Show Routes while Debugging RoutingError #6696

Merged
merged 3 commits into from Jul 8, 2012

Conversation

Projects
None yet
@schneems
Member

schneems commented Jun 10, 2012

If someone receives a routing error, they likely need to view the routes. Rather than making them visit '/rails/info/routes' or run rake routes we can give them that information on the page.

Screenshot:
Screeshot

ActionDispatch Tests Pass

@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Jun 10, 2012

Member

The only real question was where to put the code to format the routes. In a more traditional architecture it would belong in the controller 100%. However, in other exceptions there are code blocks in the views , and while it doesn't make for clean view code, it does keep the middleware lean.

Member

schneems commented Jun 10, 2012

The only real question was where to put the code to format the routes. In a more traditional architecture it would belong in the controller 100%. However, in other exceptions there are code blocks in the views , and while it doesn't make for clean view code, it does keep the middleware lean.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Jun 10, 2012

Member

Very cool. I would leave the formatter code in the middleware.

Member

rafaelfranca commented Jun 10, 2012

Very cool. I would leave the formatter code in the middleware.

@rafaelfranca

View changes

actionpack/lib/action_dispatch/middleware/debug_exceptions.rb
@@ -1,5 +1,7 @@
require 'action_dispatch/http/request'
require 'action_dispatch/middleware/exception_wrapper'
+require 'rails/application/route_inspector'

This comment has been minimized.

@rafaelfranca

rafaelfranca Jun 11, 2012

Member

I just realized that this will only work if railties is loaded in your application. As railties is not a actionpack dependency this can fail with a LoadError

@rafaelfranca

rafaelfranca Jun 11, 2012

Member

I just realized that this will only work if railties is loaded in your application. As railties is not a actionpack dependency this can fail with a LoadError

This comment has been minimized.

@rafaelfranca

rafaelfranca Jun 11, 2012

Member

@josevalim thoughts about this?

@rafaelfranca

rafaelfranca Jun 11, 2012

Member

@josevalim thoughts about this?

@dmathieu

This comment has been minimized.

Show comment
Hide comment
@dmathieu

dmathieu Jun 11, 2012

Contributor

👍 love the idea.

Contributor

dmathieu commented Jun 11, 2012

👍 love the idea.

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Jun 11, 2012

Contributor

As @rafaelfranca, this is backwards, actionpack cannot depend on railties. In fact, every time you see a reference to the Rails constant in actionpack, it is something that needs to be fixed by providing a proper configuration or abstract (except in railties files).

That said, it seems the best way would be to move the router inspector to actionpack.

Contributor

josevalim commented Jun 11, 2012

As @rafaelfranca, this is backwards, actionpack cannot depend on railties. In fact, every time you see a reference to the Rails constant in actionpack, it is something that needs to be fixed by providing a proper configuration or abstract (except in railties files).

That said, it seems the best way would be to move the router inspector to actionpack.

@tenderlove

This comment has been minimized.

Show comment
Hide comment
@tenderlove

tenderlove Jun 11, 2012

Member

Agree with @josevalim, we need to move the inspector to AP. I do like this idea, but we should be careful about performance of the 500 pages.

Member

tenderlove commented Jun 11, 2012

Agree with @josevalim, we need to move the inspector to AP. I do like this idea, but we should be careful about performance of the 500 pages.

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Jun 11, 2012

Contributor

This is the debugger middleware, only available in development. Perf should
not be a problem?

Contributor

josevalim commented Jun 11, 2012

This is the debugger middleware, only available in development. Perf should
not be a problem?

@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Jun 11, 2012

Member

ActionDispatch already knows quite a bit about routes, formatting them to be human readable doesn't seem like too far of a leap.

On the perf side it looks like DebugExceptions is in the middleware in production, but the custom error pages are ignored unless env['action_dispatch.show_detailed_exceptions'] is set. Restrict any route formatting/rendering code to within that section and there won't be any performance degradation.

Member

schneems commented Jun 11, 2012

ActionDispatch already knows quite a bit about routes, formatting them to be human readable doesn't seem like too far of a leap.

On the perf side it looks like DebugExceptions is in the middleware in production, but the custom error pages are ignored unless env['action_dispatch.show_detailed_exceptions'] is set. Restrict any route formatting/rendering code to within that section and there won't be any performance degradation.

@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Jul 5, 2012

Member

@josevalim, @rafaelfranca, & @tenderlove I paired with @mattt to move the Route Inspector out of Railties, and into Actionpack (action_dispatch). Re-ran all railties tests (https://gist.github.com/b298e74a2d20863c9486) and AR with sqlite3 (https://gist.github.com/b298e74a2d20863c9486). ATP.

Now action_dispatch can render routes when routing errors are received independently of railties.

Member

schneems commented Jul 5, 2012

@josevalim, @rafaelfranca, & @tenderlove I paired with @mattt to move the Route Inspector out of Railties, and into Actionpack (action_dispatch). Re-ran all railties tests (https://gist.github.com/b298e74a2d20863c9486) and AR with sqlite3 (https://gist.github.com/b298e74a2d20863c9486). ATP.

Now action_dispatch can render routes when routing errors are received independently of railties.

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Jul 6, 2012

Contributor

Thanks @schneems ! One thing left: could we move the route inspector tests to inside action dispatch too? Thanks!

Contributor

josevalim commented Jul 6, 2012

Thanks @schneems ! One thing left: could we move the route inspector tests to inside action dispatch too? Thanks!

@schneems

View changes

actionpack/lib/action_dispatch/middleware/debug_exceptions.rb
+ def formatted_routes(exception)
+ if exception.is_a?(ActionController::RoutingError) || exception.is_a?(ActionView::Template::Error)
+ inspector = ActionDispatch::Routing::RouteInspector.new
+ inspector.format(Rails.application.routes.routes).join("\n")

This comment has been minimized.

@schneems

schneems Jul 6, 2012

Member

We missed this reference Rails.application.routes.routes. Moving the inspector was the easy part, getting rid of that line will require more code changes. Right now railties builds routes using ActionDispatch, but ActionDispatch doesn't actually know what routes it has. This is the method in railties for Rails.application.routes.

def routes
  @routes ||= ActionDispatch::Routing::RouteSet.new.tap do |routes|
    routes.draw_paths.concat paths["config/routes"].paths
  end

  @routes.append(&Proc.new) if block_given?
  @routes
end

My first reaction is to keep ActionDispatch agnostic about where the routes come from, but to store the results, so that Railties can inject the path where it wants the routes to be drawn from. ActionDispatch, rather than just spitting out routes can hold onto them.

def routes
  @routes ||= ActionDispatch::Routing.set_from_paths(paths["config/routes"].paths)

  @routes.append(&Proc.new) if block_given?
  @routes
end

Then we could have a getter in ActionDispatch along the lines of ActionDispatch::Routing.routes, how does that sound?

(on a side note we should also make Journey an injectable dependency rather than hard coding it into ActionDispatch, though this isn't the place or time for that)

@schneems

schneems Jul 6, 2012

Member

We missed this reference Rails.application.routes.routes. Moving the inspector was the easy part, getting rid of that line will require more code changes. Right now railties builds routes using ActionDispatch, but ActionDispatch doesn't actually know what routes it has. This is the method in railties for Rails.application.routes.

def routes
  @routes ||= ActionDispatch::Routing::RouteSet.new.tap do |routes|
    routes.draw_paths.concat paths["config/routes"].paths
  end

  @routes.append(&Proc.new) if block_given?
  @routes
end

My first reaction is to keep ActionDispatch agnostic about where the routes come from, but to store the results, so that Railties can inject the path where it wants the routes to be drawn from. ActionDispatch, rather than just spitting out routes can hold onto them.

def routes
  @routes ||= ActionDispatch::Routing.set_from_paths(paths["config/routes"].paths)

  @routes.append(&Proc.new) if block_given?
  @routes
end

Then we could have a getter in ActionDispatch along the lines of ActionDispatch::Routing.routes, how does that sound?

(on a side note we should also make Journey an injectable dependency rather than hard coding it into ActionDispatch, though this isn't the place or time for that)

This comment has been minimized.

@josevalim

josevalim Jul 6, 2012

Contributor

You can simply pass the route set or Rails.application as argument to the middleware initialization.

@josevalim

josevalim Jul 6, 2012

Contributor

You can simply pass the route set or Rails.application as argument to the middleware initialization.

@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Jul 6, 2012

Member

Updated the middleware to take an optional routes_app that should respond to :routes. Also moved the route inspector test to action_dispatch, which you'll be happy to know shaves off 2 seconds from the railties test suite!!

ATP on ActionPack https://gist.github.com/56da1c69f269900bb51c, ActiveRecord, and Railties

Member

schneems commented Jul 6, 2012

Updated the middleware to take an optional routes_app that should respond to :routes. Also moved the route inspector test to action_dispatch, which you'll be happy to know shaves off 2 seconds from the railties test suite!!

ATP on ActionPack https://gist.github.com/56da1c69f269900bb51c, ActiveRecord, and Railties

@exviva

View changes

railties/lib/rails/application.rb
@@ -266,7 +266,7 @@ def default_middleware_stack
middleware.use ::ActionDispatch::RequestId
middleware.use ::Rails::Rack::Logger, config.log_tags # must come after Rack::MethodOverride to properly log overridden methods
middleware.use ::ActionDispatch::ShowExceptions, config.exceptions_app || ActionDispatch::PublicExceptions.new(Rails.public_path)
- middleware.use ::ActionDispatch::DebugExceptions
+ middleware.use ::ActionDispatch::DebugExceptions, Rails.application

This comment has been minimized.

@exviva

exviva Jul 6, 2012

Contributor

Couldn't this simply be self instead of Rails.application?

@exviva

exviva Jul 6, 2012

Contributor

Couldn't this simply be self instead of Rails.application?

This comment has been minimized.

@schneems

schneems Jul 6, 2012

Member

yes, and there is a line a little lower that sets app = self, it might be more clear to move that line up and use the app var rather than relying on the hard coded

@schneems

schneems Jul 6, 2012

Member

yes, and there is a line a little lower that sets app = self, it might be more clear to move that line up and use the app var rather than relying on the hard coded

@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Jul 6, 2012

Member

updated

Member

schneems commented Jul 6, 2012

updated

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Jul 7, 2012

Contributor

This looks good to be merged in my opinion. It needs to be rebased though and we need a changelog entry. :) Thanks!

Contributor

josevalim commented Jul 7, 2012

This looks good to be merged in my opinion. It needs to be rebased though and we need a changelog entry. :) Thanks!

schneems and others added some commits Jun 9, 2012

show routes while debugging RoutingError
If someone receives a routing error, they likely need to view the routes. Rather than making them visit '/rails/info/routes' or run `rake routes` we can give them that information on the page.
move route_inspector to actionpack
this is so we can show route output in the development when we get a routing error. Railties can use features of ActionDispatch, but ActionDispatch should not depend on Railties.
@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Jul 7, 2012

Member

Rebased against upstream master, added changelog entry, let me know if there is anything else. Thanks for your help!

Member

schneems commented Jul 7, 2012

Rebased against upstream master, added changelog entry, let me know if there is anything else. Thanks for your help!

josevalim added a commit that referenced this pull request Jul 8, 2012

@josevalim josevalim merged commit 7404cda into rails:master Jul 8, 2012

@mike-burns

This comment has been minimized.

Show comment
Hide comment
@mike-burns

mike-burns Jul 27, 2012

Fantastic!

Fantastic!

@klevo

This comment has been minimized.

Show comment
Hide comment
@klevo

klevo Sep 20, 2012

So cool.

klevo commented Sep 20, 2012

So cool.

@rodrigoalvesvieira

This comment has been minimized.

Show comment
Hide comment
@rodrigoalvesvieira

rodrigoalvesvieira Sep 20, 2012

Great idea!

Great idea!

@daniloassis

This comment has been minimized.

Show comment
Hide comment
@daniloassis

daniloassis Sep 20, 2012

Simple but GREAT!

Simple but GREAT!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment