Ticket #131 (closed enhancement: fixed)

Opened 1 year ago

Last modified 1 year ago

[patch] Regular Expression Routes, Late-bound Routes, and Non-path-only routes

Reported by: duane.johns..@gmail.com Assigned to: e.@brainspl.at
Priority: major Milestone: 0.4
Component: Routing Keywords:
Cc:

Description

I'm adding this patch here first before committing so that you can try it out, comment and/or improve it before it becomes the built-in routing system. I had several goals for this patch, as documented in a previous email.

  • Routes are implemented as a basic regular expression matching system, with the "simple_route" and "resource_route" tacked on top. Some new features include:

-- Use any Merb::Request method in the router. For example, you can now check the route against a domain:

r.route :host => %r[^www\.], :path => %r[/admin/([^/]+)], :controller => "admin/:path[1]"

-- or an HTTP method:

r.route :method => /put/, :path => %r[/^\d+$/], :controller => "updater", :action => "guid"

-- or the user agent:

r.route :user_agent => /MSIE/, :controller => "download", :action => "firefox"

-- or the referer, the encoding, or the language, etc.

  • Any regular expression grouping can be used in the right-hand-side of the params. This is fantastic news for namespaces and other more complicated routes:

r.route :path => %r[/([^/]+)/([^/]+)/([^/]+)/(\d+)], :controller => ":path[1]/:path[2]", :action => ":path[3]", :id => ":path[4]"

or the common case of an admin module becomes:

r.route :path => %r[/admin/([^/]+)/([^/]+)/(\d+)], :controller => "admin/:path[2]", :action => ":path[3]", :id => ":path[4]"

  • I also implemented the late-bound routes (type-agnostic) as I previously described. This essentially means that you can pass a Proc object to any route and let the proc return either 'false' or a hash. If false, the route is not matched. If a hash, the route is matched and the hash becomes the params, including the :controller and :action keys. Example: 
    r.route do |url|
  if url.path.include? "fun"
    {:controller => "has_fun", :action => "index"}
  end
end
  • With all of this power, it's now too complicated (as you said in our #merb chat) for easy usage. As a remedy, the old routes are (mostly) supported as a layer on top of the regular expression matching system. For example:

r.simple_route '/admin/:controller/:action', :controller => 'admin/:controller'

This makes for some interesting possibilities:

r.simple_route '/:controller/:action/:id/:prefix', :action => ':prefix_:action'

(this would route a request such as "/users/show/1/mini" to the Users controller, and the "mini_show" action.)

  • Resource routes are partially supported. I'm not quite as familiar as you are in this area, so I did what made sense to me, but I think we will need some more work here. For example, I did not implement a route generator (required for resources?).
  • Now for some of the gut internal changes:

-- I took some of the code from the Merb::Controller class and put it in the dispatcher because in order to dispatch based on any Merb::Request information (basically the env variables) it made more sense to create the Request object sooner.

-- As a consequence, the Controller's "build" method is now called with six parameters:

status, headers, response, request, params, cookies

-- This change was necessary so that there was no duplication (slowdown) of dispatching.

  • The compiled_statement in the Router is now a series of "if" statements rather than a "case/when" statement.
  • Specs still in progress

Attachments

regexp_routes.diff (28.9 kB) - added by duane.johns..@gmail.com on 08/17/07 08:04:28.
regexp_routes_with_specs.diff (81.0 kB) - added by duane.johns..@gmail.com on 08/25/07 20:46:30.

Change History

08/17/07 08:04:28 changed by duane.johns..@gmail.com

  • attachment regexp_routes.diff added.

(follow-up: ↓ 2 ) 08/17/07 10:25:40 changed by v..@exdolo.com

personally, I'd like to see some specs for this. It's a huge change in an important underlying system and there's zero testing.

(in reply to: ↑ 1 ) 08/17/07 10:31:01 changed by duane.johns..@gmail.com

Replying to v..@exdolo.com:

personally, I'd like to see some specs for this. It's a huge change in an important underlying system and there's zero testing.

Yes, they will be forthcoming. I am leaving on vacation for a week.

08/25/07 20:46:30 changed by duane.johns..@gmail.com

  • attachment regexp_routes_with_specs.diff added.

08/25/07 22:22:01 changed by e.@brainspl.at

  • owner set to e.@brainspl.at.
  • status changed from new to assigned.

Ok so I really like the functionality and flexibility this system has. But I'm having a hard time swallowing the performance hit :( Using the routes defined at the top of merb_dispatch_spec.rb I ran some benchmarks with the old route system and the new route system:

user system total real

new routes 16.090000 0.040000 16.130000 ( 16.195940)

user system total real

old routes 3.890000 0.020000 5.910000 ( 3.934794)

The new system is 4 times slower to match the same routes without any of the new request routing features and even slower when the extra new features are used.

I'm going to have to think about this one. I know you spent a lot of time working on this and I *really* like the features. But I'm concerned about the slow down in something that is such a big part of the dispatch.

08/26/07 10:49:15 changed by duane.johns..@gmail.com

I think we can get this up to speed, so I'm not too worried about it being indefinitely 4 times slower. This is a first go at it, and I didn't do any benchmarking to see which approaches would be fastest. With a little work, I'm confident I can get the compiled "if" statements to be nearly as fast as the former "case" statement (even if I have to return to using a case statement).

With regard to functionality, I think the benefits outweigh the costs, even with the speed hit. Rails never did get the Routing system right and so I think this could be a "killer feature" of merb, in addition to its enticing multi-threadedness. I can't tell you how many times I've heard people ask how to make a route that uses subdomains, or distinguishes between http and https. There are a lot of apps that had to be crammed into the limitations of Rails' routes, but we don't have to do that here. With this patch, Merb can be the alternative that people have been looking for.

Lastly, I'm curious about the actual cost of this change. When you did the benchmarking, did you consider that the query string parsing that used to be done in the controller class is now being done in the dispatcher? I moved the following methods into the dispatcher: normalize_params, parse_request, parse_multipart, and query_parse. Are we really getting a performance hit, or is it just moving some of the processing around?

08/26/07 22:35:47 changed by duane.johns..@gmail.com

  • status changed from assigned to closed.
  • resolution set to fixed.

Applied as changeset 429