Skip to content
Browse files

Much needed refactoring in dispatching code [#131]/[#127]

This moves the :halt catch out so that all routing code runs
within one giant catch block instead of running each type
of handler in its own catch block. This required some cleanup
in the error handling code, which cleaned things up quite a bit.

This corrects two issues:

1. halt with > 1 args causes ArgumentError
   http://sinatra.lighthouseapp.com/projects/9779/tickets/131

2. halting in a before filter doesn't modify response
   http://sinatra.lighthouseapp.com/projects/9779/tickets/127

We still need to split up the more epic methods (#route!, #invoke)
but the logic is pretty sound at this point.
  • Loading branch information...
1 parent b709c8a commit b923e0906e781bd45ab1eb81d06ec77e515c3fbf @rtomayko rtomayko committed Jan 24, 2009
Showing with 95 additions and 27 deletions.
  1. +41 −25 lib/sinatra/base.rb
  2. +1 −1 lib/sinatra/compat.rb
  3. +13 −0 test/filter_test.rb
  4. +40 −1 test/routing_test.rb
View
66 lib/sinatra/base.rb
@@ -311,7 +311,11 @@ def call!(env)
@request = Request.new(env)
@response = Response.new
@params = nil
- error_detection { dispatch! }
+
+ invoke { dispatch! }
+ invoke { error_block!(response.status) }
+
+ @response.body = [] if @env['REQUEST_METHOD'] == 'HEAD'
@response.finish
end
@@ -320,28 +324,30 @@ def options
end
def halt(*response)
- throw :halt, *response
+ response = response.first if response.length == 1
+ throw :halt, response
end
def pass
throw :pass
end
private
- def dispatch!
- @params = original_params = nested_params(@request.params)
+ # Run before filters and then locate and run a matching route.
+ def route!
+ @params = nested_params(@request.params)
- self.class.filters.each do |block|
- res = catch(:halt) { instance_eval(&block) ; :continue }
- return unless res == :continue
- end
+ # before filters
+ self.class.filters.each { |block| instance_eval(&block) }
+ # routes
if routes = self.class.routes[@request.request_method]
+ original_params = @params
path = @request.path_info
- routes.each do |pattern, keys, conditions, method_name|
- if pattern =~ path
- values = $~.captures.map{|val| val && unescape(val) }
+ routes.each do |pattern, keys, conditions, block|
+ if match = pattern.match(path)
+ values = match.captures.map{|val| val && unescape(val) }
params =
if keys.any?
keys.zip(values).inject({}) do |hash,(k,v)|
@@ -359,14 +365,15 @@ def dispatch!
end
@params = original_params.merge(params)
- catch(:pass) {
+ catch(:pass) do
conditions.each { |cond|
throw :pass if instance_eval(&cond) == false }
- return invoke(method_name)
- }
+ throw :halt, instance_eval(&block)
+ end
end
end
end
+
raise NotFound
end
@@ -388,7 +395,8 @@ def indifferent_hash
Hash.new {|hash,key| hash[key.to_s] if Symbol === key }
end
- def invoke(block)
+ # Run the block with 'throw :halt' support and apply result to the response.
+ def invoke(&block)
res = catch(:halt) { instance_eval(&block) }
return if res.nil?
@@ -420,15 +428,15 @@ def invoke(block)
res
end
- def error_detection
- errmap = self.class.errors
- yield
+ # Dispatch a request with error handling.
+ def dispatch!
+ route!
rescue NotFound => boom
@env['sinatra.error'] = boom
@response.status = 404
@response.body = ['<h1>Not Found</h1>']
- handler = errmap[boom.class] || errmap[NotFound]
- invoke handler unless handler.nil?
+ error_block! boom.class, NotFound
+
rescue ::Exception => boom
@env['sinatra.error'] = boom
@@ -439,11 +447,19 @@ def error_detection
raise boom if options.raise_errors?
@response.status = 500
- invoke errmap[boom.class] || errmap[Exception]
- ensure
- if @response.status >= 400 && errmap.key?(response.status)
- invoke errmap[response.status]
+ error_block! boom.class, Exception
+ end
+
+ # Find an custom error block for the key(s) specified.
+ def error_block!(*keys)
+ errmap = self.class.errors
+ keys.each do |key|
+ if block = errmap[key]
+ res = instance_eval(&block)
+ return res
+ end
end
+ nil
end
@routes = {}
@@ -573,7 +589,7 @@ def get(path, opts={}, &block)
route('GET', path, opts, &block)
@conditions = conditions
- head(path, opts) { invoke(block) ; [] }
+ route('HEAD', path, opts, &block)
end
def put(path, opts={}, &bk); route 'PUT', path, opts, &bk; end
View
2 lib/sinatra/compat.rb
@@ -118,7 +118,7 @@ def send_file(path, opts={})
# Throwing halt with a Symbol and the to_result convention are
# deprecated. Override the invoke method to detect those types of return
# values.
- def invoke(handler)
+ def invoke(&block)
res = super
case
when res.kind_of?(Symbol)
View
13 test/filter_test.rb
@@ -73,6 +73,19 @@
assert_equal 'cool', body
end
+ it "does modify the response with halt" do
+ mock_app {
+ before { halt 302, 'Hi' }
+ get '/foo' do
+ "should not happen"
+ end
+ }
+
+ get '/foo'
+ assert_equal 302, response.status
+ assert_equal 'Hi', body
+ end
+
it "gives you access to params" do
mock_app {
before { @foo = params['foo'] }
View
41 test/routing_test.rb
@@ -1,7 +1,7 @@
require File.dirname(__FILE__) + '/helper'
describe "Routing" do
- %w[get put post delete head].each do |verb|
+ %w[get put post delete].each do |verb|
it "defines #{verb.upcase} request handlers with #{verb}" do
mock_app {
send verb, '/hello' do
@@ -16,6 +16,21 @@
end
end
+ it "defines HEAD request handlers with HEAD" do
+ mock_app {
+ head '/hello' do
+ response['X-Hello'] = 'World!'
+ 'remove me'
+ end
+ }
+
+ request = Rack::MockRequest.new(@app)
+ response = request.request('HEAD', '/hello', {})
+ assert response.ok?
+ assert_equal 'World!', response['X-Hello']
+ assert_equal '', response.body
+ end
+
it "404s when no route satisfies the request" do
mock_app {
get('/foo') { }
@@ -254,6 +269,30 @@
assert_equal 'Hello World', body
end
+ it "halts with a response tuple" do
+ mock_app {
+ get '/' do
+ halt 295, {'Content-Type' => 'text/plain'}, 'Hello World'
+ end
+ }
+
+ get '/'
+ assert_equal 295, status
+ assert_equal 'text/plain', response['Content-Type']
+ assert_equal 'Hello World', body
+ end
+
+ it "halts with an array of strings" do
+ mock_app {
+ get '/' do
+ halt %w[Hello World How Are You]
+ end
+ }
+
+ get '/'
+ assert_equal 'HelloWorldHowAreYou', body
+ end
+
it "transitions to the next matching route on pass" do
mock_app {
get '/:foo' do

0 comments on commit b923e09

Please sign in to comment.
Something went wrong with that request. Please try again.