From 0ade0beec8ce8f1c5c97b389880a3759cadced86 Mon Sep 17 00:00:00 2001 From: Ryan Tomayko Date: Sat, 24 Jan 2009 23:41:09 -0800 Subject: [PATCH] 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. --- lib/sinatra/base.rb | 62 +++++++++++++++++++++++++++---------------- lib/sinatra/compat.rb | 2 +- test/filter_test.rb | 13 +++++++++ test/routing_test.rb | 41 +++++++++++++++++++++++++++- 4 files changed, 93 insertions(+), 25 deletions(-) diff --git a/lib/sinatra/base.rb b/lib/sinatra/base.rb index 61035db1..c7e1e7c2 100644 --- a/lib/sinatra/base.rb +++ b/lib/sinatra/base.rb @@ -320,7 +320,11 @@ module Sinatra @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 @@ -329,7 +333,8 @@ module Sinatra end def halt(*response) - throw :halt, *response + response = response.first if response.length == 1 + throw :halt, response end def pass @@ -337,18 +342,19 @@ module Sinatra 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| + routes.each do |pattern, keys, conditions, block| if match = pattern.match(path) values = match.captures.map{|val| val && unescape(val) } params = @@ -368,14 +374,15 @@ module Sinatra 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 @@ -397,7 +404,8 @@ module Sinatra 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? @@ -429,15 +437,15 @@ module Sinatra 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 = ['

Not Found

'] - handler = errmap[boom.class] || errmap[NotFound] - invoke handler unless handler.nil? + error_block! boom.class, NotFound + rescue ::Exception => boom @env['sinatra.error'] = boom @@ -449,11 +457,19 @@ module Sinatra 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 def clean_backtrace(trace) @@ -592,7 +608,7 @@ module Sinatra 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 diff --git a/lib/sinatra/compat.rb b/lib/sinatra/compat.rb index 0eef8131..13d3bed7 100644 --- a/lib/sinatra/compat.rb +++ b/lib/sinatra/compat.rb @@ -107,7 +107,7 @@ module Sinatra # 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) diff --git a/test/filter_test.rb b/test/filter_test.rb index 5e556cdd..61885c71 100644 --- a/test/filter_test.rb +++ b/test/filter_test.rb @@ -73,6 +73,19 @@ describe "Filters" do 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'] } diff --git a/test/routing_test.rb b/test/routing_test.rb index e15e4902..faf54553 100644 --- a/test/routing_test.rb +++ b/test/routing_test.rb @@ -6,7 +6,7 @@ def route_def(pattern) end 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 @@ -21,6 +21,21 @@ describe "Routing" do 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') { } @@ -296,6 +311,30 @@ describe "Routing" do 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