Skip to content

Commit

Permalink
Http router 0.11
Browse files Browse the repository at this point in the history
  • Loading branch information
DAddYE committed Mar 15, 2013
1 parent 07e90a9 commit c2b56c4
Show file tree
Hide file tree
Showing 5 changed files with 84 additions and 49 deletions.
76 changes: 56 additions & 20 deletions padrino-core/lib/padrino-core/application/routing.rb
Expand Up @@ -31,8 +31,8 @@ def process_destination_path(path, env)
@route = path.route
@params ||= {}
@params.update(env['router.params'])
@block_params = if path.route.is_a?(HttpRouter::RegexRoute)
params_list = env['router.request'].extra_env['router.regex_match'].to_a
@block_params = if match_data = env['router.request'].extra_env['router.regex_match']
params_list = match_data.to_a
params_list.shift
@params[:captures] = params_list
params_list
Expand Down Expand Up @@ -87,6 +87,40 @@ def custom_conditions(&block)

@_custom_conditions
end

def significant_variable_names
@significant_variable_names ||= @original_path.nil? ? [] : @original_path.scan(/(^|[^\\])[:\*]([a-zA-Z0-9_]+)/).map{|p| p.last.to_sym}
end
end

class Node::Path
def to_code
path_ivar = inject_root_ivar(self)
"#{"if !callback && request.path.size == 1 && request.path.first == '' && (request.rack_request.head? || request.rack_request.get?) && request.rack_request.path_info[-1] == ?/
catch(:pass) do
response = ::Rack::Response.new
response.redirect(request.rack_request.path_info[0, request.rack_request.path_info.size - 1], 302)
return response.finish
end
end" if router.redirect_trailing_slash?}
#{"if request.#{router.ignore_trailing_slash? ? 'path_finished?' : 'path.empty?'}" unless route.match_partially}
catch(:pass) do
if callback
request.called = true
callback.call(Response.new(request, #{path_ivar}))
else
env = request.rack_request.dup.env
env['router.request'] = request
env['router.params'] ||= {}
#{"env['router.params'].merge!(Hash[#{param_names.inspect}.zip(request.params)])" if dynamic?}
@router.rewrite#{"_partial" if route.match_partially}_path_info(env, request)
response = @router.process_destination_path(#{path_ivar}, env)
return response unless router.pass_on_response(response)
end
end
#{"end" unless route.match_partially}"
end
end
end

Expand All @@ -101,7 +135,7 @@ def initialize(mode, scoped_controller, options, args, &block)
def apply?(request)
detect = @args.any? do |arg|
case arg
when Symbol then request.route_obj && (request.route_obj.named == arg or request.route_obj.named == [@scoped_controller, arg].flatten.join("_").to_sym)
when Symbol then request.route_obj && (request.route_obj.name == arg or request.route_obj.name == [@scoped_controller, arg].flatten.join("_").to_sym)
else arg === request.path_info
end
end || @options.any? do |name, val|
Expand Down Expand Up @@ -451,8 +485,8 @@ def reset_router!
# # => [:foo_bar, :id => :mine]
#
def recognize_path(path)
responses = @router.recognize(Rack::MockRequest.env_for(path))
[responses[0].path.route.named, responses[0].params]
responses = *@router.recognize(Rack::MockRequest.env_for(path)).first
[responses[0].path.route.name, responses[0].params]
end

##
Expand All @@ -473,11 +507,12 @@ def url(*args)
params[:format] = params[:format].to_s unless params[:format].nil?
params = value_to_param(params)
end
url = if params_array.empty?
compiled_router.url(name, params)
else
compiled_router.url(name, *(params_array << params))
end
url =
if params_array.empty?
compiled_router.path(name, params)
else
compiled_router.path(name, *(params_array << params))
end
url[0,0] = conform_uri(uri_root) if defined?(uri_root)
url[0,0] = conform_uri(ENV['RACK_BASE_URI']) if ENV['RACK_BASE_URI']
url = "/" if url.blank?
Expand Down Expand Up @@ -584,22 +619,22 @@ def route(verb, path, *args, &block)

# HTTPRouter route construction
route = router.add(path, route_options)
route.name(name) if name
route.name = name if name
route.action = action
priority_name = options.delete(:priority) || :normal
priority = ROUTE_PRIORITY[priority_name] or raise("Priority #{priority_name} not recognized, try #{ROUTE_PRIORITY.keys.join(', ')}")
route.cache = options.key?(:cache) ? options.delete(:cache) : @_cache
route.parent = route_parents ? (route_parents.count == 1 ? route_parents.first : route_parents) : route_parents
route.send(verb.downcase.to_sym)
route.host(options.delete(:host)) if options.key?(:host)
route.user_agent(options.delete(:agent)) if options.key?(:agent)
route.add_request_method(verb.downcase.to_sym)
route.host = options.delete(:host) if options.key?(:host)
route.user_agent = options.delete(:agent) if options.key?(:agent)
if options.key?(:default_values)
defaults = options.delete(:default_values)
route.default(defaults) if defaults
route.add_default_values(defaults) if defaults
end
options.delete_if do |option, args|
if route.send(:significant_variable_names).include?(option)
route.matching(option => Array(args).first)
if route.significant_variable_names.include?(option)
route.add_match_with(option => Array(args).first)
true
end
end
Expand Down Expand Up @@ -660,8 +695,8 @@ def parse_route(path, options, verb)

if @_use_format or format_params = options[:provides]
process_path_for_provides(path, format_params)
options[:matching] ||= {}
options[:matching][:format] = /[^\.]+/
# options[:add_match_with] ||= {}
# options[:add_match_with][:format] = /[^\.]+/
end

absolute_map = map && map[0] == ?/
Expand Down Expand Up @@ -865,7 +900,7 @@ def current_path(*path_params)
else
path_params << params
end
@route.url(*path_params)
@route.path(*path_params)
end

##
Expand Down Expand Up @@ -957,6 +992,7 @@ def route!(base=settings, pass_block=nil)
match[1].each { |k,v| response[k] = v }
status match[0]
route_missing if match[0] == 404
route_missing if allow = response['Allow'] and allow.include?(request.env['REQUEST_METHOD'])
end
end
else
Expand Down
8 changes: 4 additions & 4 deletions padrino-core/lib/padrino-core/mounter.rb
Expand Up @@ -106,12 +106,12 @@ def routes
#
def named_routes
app_obj.routes.map { |route|
name_array = "(#{route.named.to_s.split("_").map { |piece| %Q[:#{piece}] }.join(", ")})"
request_method = route.conditions[:request_method][0]
next if route.named.blank? || request_method == 'HEAD'
name_array = "(#{route.name.to_s.split("_").map { |piece| %Q[:#{piece}] }.join(", ")})"
request_method = route.request_methods.first
next if route.name.blank? || request_method == 'HEAD'
original_path = route.original_path.is_a?(Regexp) ? route.original_path.inspect : route.original_path
full_path = File.join(uri_root, original_path)
OpenStruct.new(:verb => request_method, :identifier => route.named, :name => name_array, :path => full_path)
OpenStruct.new(:verb => request_method, :identifier => route.name, :name => name_array, :path => full_path)
}.compact
end

Expand Down
2 changes: 1 addition & 1 deletion padrino-core/padrino-core.gemspec
Expand Up @@ -36,7 +36,7 @@ Gem::Specification.new do |s|
else
s.add_dependency("sinatra", "~> 1.4.0.d")
end
s.add_dependency("http_router", "~> 0.10.2")
s.add_dependency("http_router", "~> 0.11.0")
s.add_dependency("thor", "~> 0.17.0")
s.add_dependency("activesupport", ">= 3.1.0")
s.add_dependency("rack-protection", ">= 1.5.0")
Expand Down
4 changes: 2 additions & 2 deletions padrino-core/test/test_rendering.rb
Expand Up @@ -404,7 +404,7 @@ def teardown
assert_equal "Im Italian Js", body
I18n.locale = :en
get "/foo.pk"
assert_equal 405, status
assert_equal 404, status
end

should 'resolve template content_type and locale with layout' do
Expand Down Expand Up @@ -446,7 +446,7 @@ def teardown
get "/bar.json"
assert_equal "Im a json", body
get "/bar.pk"
assert_equal 405, status
assert_equal 404, status
end

should 'renders erb with blocks' do
Expand Down
43 changes: 21 additions & 22 deletions padrino-core/test/test_routing.rb
Expand Up @@ -57,12 +57,12 @@ class FooError < RuntimeError; end

should 'accept regexp routes' do
mock_app do
get(%r{/fob|/baz}) { "regexp" }
get(%r./fob|/baz.) { "regexp" }
get("/foo") { "str" }
get %r{/([0-9]+)/} do |num|
"Your lucky number: #{num} #{params[:captures].first}"
get %r./([0-9]+)/. do |num|
"Your lucky number: #{num} #{params[:captures].first}"
end
get /\/page\/([0-9]+)|\// do |num|
get %r./page/([0-9]+)|/. do |num|
"My lucky number: #{num} #{params[:captures].first}"
end
end
Expand All @@ -72,8 +72,8 @@ class FooError < RuntimeError; end
assert_equal "regexp", body
get "/baz"
assert_equal "regexp", body
get "/1234/"
assert_equal "Your lucky number: 1234 1234", body
get "/321/"
assert_equal "Your lucky number: 321 321", body
get "/page/99"
assert_equal "My lucky number: 99 99", body
end
Expand Down Expand Up @@ -138,9 +138,9 @@ class FooError < RuntimeError; end
post("/main"){ "hello" }
end
assert_equal 3, app.routes.size, "should generate GET, HEAD and PUT"
assert_equal ["GET"], app.routes[0].conditions[:request_method]
assert_equal ["HEAD"], app.routes[1].conditions[:request_method]
assert_equal ["POST"], app.routes[2].conditions[:request_method]
assert_equal "GET", app.routes[0].request_methods.first
assert_equal "HEAD", app.routes[1].request_methods.first
assert_equal "POST", app.routes[2].request_methods.first
end

should 'generate basic urls' do
Expand Down Expand Up @@ -192,13 +192,13 @@ class FooError < RuntimeError; end
get "/b.js"
assert_equal "/b.js", body
get "/b.ru"
assert_equal 405, status
assert_equal 404, status
get "/c.js"
assert_equal "/c.json", body
get "/c.json"
assert_equal "/c.json", body
get "/c.ru"
assert_equal 405, status
assert_equal 404, status
get "/d"
assert_equal "/d.js?foo=bar", body
get "/d.js"
Expand Down Expand Up @@ -255,7 +255,7 @@ class FooError < RuntimeError; end
end

get "/a.xml", {}, {}
assert_equal 405, status
assert_equal 404, status
end

should "not set content_type to :html if Accept */* and html not in provides" do
Expand Down Expand Up @@ -294,6 +294,7 @@ class FooError < RuntimeError; end
end

should "allow .'s in param values" do
skip
mock_app do
get('/id/:email', :provides => [:json]) { |email, format| [email, format] * '/' }
end
Expand Down Expand Up @@ -334,7 +335,7 @@ class FooError < RuntimeError; end
end

get "/a.xml", {}, {"HTTP_ACCEPT" => "text/html"}
assert_equal 405, status
assert_equal 404, status
end

should "generate routes for format simple" do
Expand Down Expand Up @@ -379,18 +380,15 @@ class FooError < RuntimeError; end

should "support not_found" do
mock_app do
not_found do
response.status = 404
'whatever'
end
not_found { 'whatever' }

get :index, :map => "/" do
'index'
end
end
get '/something'
assert_equal 'whatever', body
get '/wrong'
assert_equal 404, status
assert_equal 'whatever', body
get '/'
assert_equal 'index', body
assert_equal 200, status
Expand All @@ -399,7 +397,7 @@ class FooError < RuntimeError; end
should "should inject the route into the request" do
mock_app do
controller :posts do
get(:index) { request.route_obj.named.to_s }
get(:index) { request.route_obj.name.to_s }
end
end
get "/posts"
Expand Down Expand Up @@ -748,6 +746,7 @@ class FooError < RuntimeError; end
end

should 'respect priorities' do
skip
route_order = []
mock_app do
get(:index, :priority => :normal) { route_order << :normal; pass }
Expand Down Expand Up @@ -1378,11 +1377,11 @@ def authorize(username, password)
get "/.json"
assert_equal "This is the get index.json", body
get "/.js"
assert_equal 405, status
assert_equal 404, status
post "/.json"
assert_equal "This is the post index.json", body
post "/.js"
assert_equal 405, status
assert_equal 404, status
end

should "allow controller level mapping" do
Expand Down

4 comments on commit c2b56c4

@DAddYE
Copy link
Member Author

@DAddYE DAddYE commented on c2b56c4 Mar 15, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm the winner of 'Hack the crap' !!! 💃

@skade can you review this commit. I want to know why until now we used 405 status instead of 404 ... I missed something ?
@dariocravero here we are!!!
@nesquena yeye!

@nesquena
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow nice man! Actually got us working on newest http_router and all tests pass?

405 is when the request method is wrong right? is the request method wrong in those tests?

@dariocravero
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👯👯👯 Amazing!! Thanks! :)

On 405 vs 404, I think it shouldn't be a 405 since you're fetching it with the right method: GET, i.e., we're not trying to do a POST instead.

As for whether 404 is right on that case or not, I don't know, perhaps 406 fits better? From the MDN:

406 Not Acceptable

This response is sent when the web server, after performing server-driven content negotiation, doesn't find any content following the criteria given by the user agent.

Server-driven negotiation

In this kind of negotiation, the browser (or any other kind of agent) sends several HTTP headers along with the URI. These headers describe the preferred choice of the user. The server uses them as hints and an internal algorithm let it choose the best content to serve to the client. The algorithm is server-specific and not defined in the standard. See, for example, the Apache 2.2 negotiation algorithm.

The HTTP/1.1 standard gives an exhaustive list of the standard headers that may be used in a server-driven negotiation algorithm (Accept:, Accept-Charset:, Accept-Encoding:, Accept-Language:: and User-Agent:). Nevertheless it allows the server to use other aspects in its algorithm, either aspects outside the request itself or extension header fields, i.e., headers not defined in the HTTP/1.1 standard.

I know it's very specific but hey, we're already at it :P
Thoughts?

@skade
Copy link
Contributor

@skade skade commented on c2b56c4 Mar 15, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About 405, I don't know. I think it should be 404 in most cases (except wrong request method, but hey).

About 406, I know, we do already have support for it. The rule and behavior is explained in #441.

In short: Padrino will return 404 when a non-provided format is given in the url (.format). If the format is parsed based on the ACCEPT-Header (true content-negotiation), it will return 406. That behaviour can be changed by setting treat_format_as_accept which treats the format parameter as if it was given through the ACCEPT header.

Please sign in to comment.