From ed8501ef16fb2f5e4bd4d987740f5e5f62978400 Mon Sep 17 00:00:00 2001 From: Pratik Naik Date: Tue, 19 Jan 2010 15:23:56 +0530 Subject: Fix DoubleRenderError error message --- actionpack/lib/abstract_controller/rendering.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'actionpack') diff --git a/actionpack/lib/abstract_controller/rendering.rb b/actionpack/lib/abstract_controller/rendering.rb index d57136dbb8..0dab4a3cc0 100644 --- a/actionpack/lib/abstract_controller/rendering.rb +++ b/actionpack/lib/abstract_controller/rendering.rb @@ -42,7 +42,7 @@ module AbstractController # Delegates render_to_body and sticks the result in self.response_body. def render(*args) if response_body - raise AbstractController::DoubleRenderError, "OMG" + raise AbstractController::DoubleRenderError, "Can only render or redirect once per action" end self.response_body = render_to_body(*args) -- cgit v1.2.3 From bec5356f254dad1423bfdc17897589252aae5f44 Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Tue, 19 Jan 2010 08:20:52 -0600 Subject: Define named routes for other non-GET REST actions --- actionpack/lib/action_dispatch/routing/mapper.rb | 36 +++++++++++++++++++++--- 1 file changed, 32 insertions(+), 4 deletions(-) (limited to 'actionpack') diff --git a/actionpack/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb index 811c287355..fcbb70749f 100644 --- a/actionpack/lib/action_dispatch/routing/mapper.rb +++ b/actionpack/lib/action_dispatch/routing/mapper.rb @@ -375,6 +375,15 @@ module ActionDispatch end end + def action_type(action) + case action + when :index, :create + :collection + when :show, :update, :destroy + :member + end + end + def name options[:as] || plural end @@ -391,6 +400,15 @@ module ActionDispatch plural end + def name_for_action(action) + case action_type(action) + when :collection + collection_name + when :member + member_name + end + end + def id_segment ":#{singular}_id" end @@ -405,6 +423,13 @@ module ActionDispatch super end + def action_type(action) + case action + when :show, :create, :update, :destroy + :member + end + end + def name options[:as] || singular end @@ -428,7 +453,7 @@ module ActionDispatch with_scope_level(:resource, resource) do yield if block_given? - get :show, :as => resource.member_name if resource.actions.include?(:show) + get :show if resource.actions.include?(:show) post :create if resource.actions.include?(:create) put :update if resource.actions.include?(:update) delete :destroy if resource.actions.include?(:destroy) @@ -454,14 +479,14 @@ module ActionDispatch yield if block_given? with_scope_level(:collection) do - get :index, :as => resource.collection_name if resource.actions.include?(:index) + get :index if resource.actions.include?(:index) post :create if resource.actions.include?(:create) get :new, :as => resource.singular if resource.actions.include?(:new) end with_scope_level(:member) do scope(':id') do - get :show, :as => resource.member_name if resource.actions.include?(:show) + get :show if resource.actions.include?(:show) put :update if resource.actions.include?(:update) delete :destroy if resource.actions.include?(:destroy) get :edit, :as => resource.singular if resource.actions.include?(:edit) @@ -525,7 +550,10 @@ module ActionDispatch begin old_path = @scope[:path] @scope[:path] = "#{@scope[:path]}(.:format)" - return match(options.reverse_merge(:to => action)) + return match(options.reverse_merge( + :to => action, + :as => parent_resource.name_for_action(action) + )) ensure @scope[:path] = old_path end -- cgit v1.2.3 From 4e2852a4870677cab12e61795a1c500058f8f111 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Tue, 19 Jan 2010 12:17:52 +0100 Subject: Do not send rack.input or any other rack information to AD listeners. --- .../lib/action_dispatch/middleware/notifications.rb | 19 +++++++++++++------ actionpack/lib/action_dispatch/railties/subscriber.rb | 4 ++-- actionpack/test/dispatch/subscriber_test.rb | 5 ++--- 3 files changed, 17 insertions(+), 11 deletions(-) (limited to 'actionpack') diff --git a/actionpack/lib/action_dispatch/middleware/notifications.rb b/actionpack/lib/action_dispatch/middleware/notifications.rb index 01d2cbb435..c3776d53a8 100644 --- a/actionpack/lib/action_dispatch/middleware/notifications.rb +++ b/actionpack/lib/action_dispatch/middleware/notifications.rb @@ -8,17 +8,24 @@ module ActionDispatch @app = app end - def call(stack_env) - env = stack_env.dup - ActiveSupport::Notifications.instrument("action_dispatch.before_dispatch", :env => env) + def call(env) + payload = retrieve_payload_from_env(env) + ActiveSupport::Notifications.instrument("action_dispatch.before_dispatch", payload) - ActiveSupport::Notifications.instrument!("action_dispatch.after_dispatch", :env => env) do - @app.call(stack_env) + ActiveSupport::Notifications.instrument!("action_dispatch.after_dispatch", payload) do + @app.call(env) end rescue Exception => exception ActiveSupport::Notifications.instrument('action_dispatch.exception', - :env => stack_env, :exception => exception) + :env => env, :exception => exception) raise exception end + + protected + + # Remove any rack related constants from the env, like rack.input. + def retrieve_payload_from_env(env) + Hash[:env => env.except(*env.keys.select { |k| k.to_s.index("rack.") == 0 })] + end end end \ No newline at end of file diff --git a/actionpack/lib/action_dispatch/railties/subscriber.rb b/actionpack/lib/action_dispatch/railties/subscriber.rb index c08a844c6a..cdb1162eac 100644 --- a/actionpack/lib/action_dispatch/railties/subscriber.rb +++ b/actionpack/lib/action_dispatch/railties/subscriber.rb @@ -5,8 +5,8 @@ module ActionDispatch request = Request.new(event.payload[:env]) path = request.request_uri.inspect rescue "unknown" - info "\n\nProcessing #{path} to #{request.formats.join(', ')} " << - "(for #{request.remote_ip} at #{event.time.to_s(:db)}) [#{request.method.to_s.upcase}]" + info "\n\nStarted #{request.method.to_s.upcase} #{path} " << + "for #{request.remote_ip} at #{event.time.to_s(:db)}" end def logger diff --git a/actionpack/test/dispatch/subscriber_test.rb b/actionpack/test/dispatch/subscriber_test.rb index a7f1a2659a..3096c132e7 100644 --- a/actionpack/test/dispatch/subscriber_test.rb +++ b/actionpack/test/dispatch/subscriber_test.rb @@ -78,9 +78,8 @@ module DispatcherSubscriberTest log = @logger.logged(:info).first assert_equal 1, @logger.logged(:info).size - assert_match %r{^Processing "/" to text/html}, log - assert_match %r{\(for 127\.0\.0\.1}, log - assert_match %r{\[GET\]}, log + assert_match %r{^Started GET "/"}, log + assert_match %r{for 127\.0\.0\.1}, log end def test_subscriber_has_its_logged_flushed_after_request -- cgit v1.2.3 From 5a81dbf4894f112b73da160611c8be28b44c261f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Tue, 19 Jan 2010 12:22:22 +0100 Subject: Fix failing test. --- actionpack/test/controller/new_base/base_test.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'actionpack') diff --git a/actionpack/test/controller/new_base/base_test.rb b/actionpack/test/controller/new_base/base_test.rb index 964780eaf2..579f9f349f 100644 --- a/actionpack/test/controller/new_base/base_test.rb +++ b/actionpack/test/controller/new_base/base_test.rb @@ -77,9 +77,9 @@ module Dispatching test "action methods" do assert_equal Set.new(%w( + index modify_response_headers modify_response_body_twice - index modify_response_body show_actions )), SimpleController.action_methods @@ -88,7 +88,7 @@ module Dispatching assert_equal Set.new, Submodule::ContainedEmptyController.action_methods get "/dispatching/simple/show_actions" - assert_body "actions: modify_response_headers, modify_response_body_twice, index, modify_response_body, show_actions" + assert_body "actions: index, modify_response_body, modify_response_body_twice, modify_response_headers, show_actions" end end end -- cgit v1.2.3 From a8e25a518ae8df1682c84affa3b986ca3627da12 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Tue, 19 Jan 2010 12:52:10 +0100 Subject: Move parameters to the top on logging. --- actionpack/lib/action_controller/base.rb | 2 +- .../metal/filter_parameter_logging.rb | 5 -- .../lib/action_controller/metal/instrumentation.rb | 20 +++++--- .../lib/action_controller/railties/subscriber.rb | 8 +++- .../test/activerecord/controller_runtime_test.rb | 4 +- actionpack/test/controller/subscriber_test.rb | 54 ++++++++++++---------- 6 files changed, 52 insertions(+), 41 deletions(-) (limited to 'actionpack') diff --git a/actionpack/lib/action_controller/base.rb b/actionpack/lib/action_controller/base.rb index 260e5da336..f86a61d791 100644 --- a/actionpack/lib/action_controller/base.rb +++ b/actionpack/lib/action_controller/base.rb @@ -26,6 +26,7 @@ module ActionController include ActionController::Compatibility include ActionController::Cookies + include ActionController::FilterParameterLogging include ActionController::Flash include ActionController::Verification include ActionController::RequestForgeryProtection @@ -37,7 +38,6 @@ module ActionController # Add instrumentations hooks at the bottom, to ensure they instrument # all the methods properly. include ActionController::Instrumentation - include ActionController::FilterParameterLogging # TODO: Extract into its own module # This should be moved together with other normalizing behavior diff --git a/actionpack/lib/action_controller/metal/filter_parameter_logging.rb b/actionpack/lib/action_controller/metal/filter_parameter_logging.rb index 0b1e1ee6ab..9e03f50759 100644 --- a/actionpack/lib/action_controller/metal/filter_parameter_logging.rb +++ b/actionpack/lib/action_controller/metal/filter_parameter_logging.rb @@ -58,11 +58,6 @@ module ActionController protected - def append_info_to_payload(payload) - super - payload[:params] = filter_parameters(request.params) - end - def filter_parameters(params) params.dup.except!(*INTERNAL_PARAMS) end diff --git a/actionpack/lib/action_controller/metal/instrumentation.rb b/actionpack/lib/action_controller/metal/instrumentation.rb index 876f778751..7b2b037c67 100644 --- a/actionpack/lib/action_controller/metal/instrumentation.rb +++ b/actionpack/lib/action_controller/metal/instrumentation.rb @@ -9,18 +9,24 @@ module ActionController module Instrumentation extend ActiveSupport::Concern - included do - include AbstractController::Logger - end + include AbstractController::Logger + include ActionController::FilterParameterLogging attr_internal :view_runtime def process_action(action, *args) - ActiveSupport::Notifications.instrument("action_controller.process_action") do |payload| + raw_payload = { + :controller => self.class.name, + :action => self.action_name, + :params => filter_parameters(params), + :formats => request.formats.map(&:to_sym) + } + + ActiveSupport::Notifications.instrument("action_controller.start_processing", raw_payload.dup) + + ActiveSupport::Notifications.instrument("action_controller.process_action", raw_payload) do |payload| result = super - payload[:controller] = self.class.name - payload[:action] = self.action_name - payload[:status] = response.status + payload[:status] = response.status append_info_to_payload(payload) result end diff --git a/actionpack/lib/action_controller/railties/subscriber.rb b/actionpack/lib/action_controller/railties/subscriber.rb index 6659e5df47..d257d6ac2c 100644 --- a/actionpack/lib/action_controller/railties/subscriber.rb +++ b/actionpack/lib/action_controller/railties/subscriber.rb @@ -1,15 +1,19 @@ module ActionController module Railties class Subscriber < Rails::Subscriber - def process_action(event) + def start_processing(event) payload = event.payload + info " Processing by #{payload[:controller]}##{payload[:action]} as #{payload[:formats].first.to_s.upcase}" info " Parameters: #{payload[:params].inspect}" unless payload[:params].blank? + end + def process_action(event) + payload = event.payload additions = ActionController::Base.log_process_action(payload) message = "Completed in %.0fms" % event.duration message << " (#{additions.join(" | ")})" unless additions.blank? - message << " by #{payload[:controller]}##{payload[:action]} [#{payload[:status]}]" + message << " with #{payload[:status]}" info(message) end diff --git a/actionpack/test/activerecord/controller_runtime_test.rb b/actionpack/test/activerecord/controller_runtime_test.rb index d6f7cd80ab..37c7738301 100644 --- a/actionpack/test/activerecord/controller_runtime_test.rb +++ b/actionpack/test/activerecord/controller_runtime_test.rb @@ -37,8 +37,8 @@ module ControllerRuntimeSubscriberTest get :show wait - assert_equal 1, @logger.logged(:info).size - assert_match /\(Views: [\d\.]+ms | ActiveRecord: [\d\.]+ms\)/, @logger.logged(:info)[0] + assert_equal 2, @logger.logged(:info).size + assert_match /\(Views: [\d\.]+ms | ActiveRecord: [\d\.]+ms\)/, @logger.logged(:info)[1] end class SyncSubscriberTest < ActionController::TestCase diff --git a/actionpack/test/controller/subscriber_test.rb b/actionpack/test/controller/subscriber_test.rb index 24132ee928..950eecaf6f 100644 --- a/actionpack/test/controller/subscriber_test.rb +++ b/actionpack/test/controller/subscriber_test.rb @@ -63,13 +63,19 @@ module ActionControllerSubscriberTest ActionController::Base.logger = logger end + def test_start_processing + get :show + wait + assert_equal 2, logs.size + assert_equal "Processing by Another::SubscribersController#show as HTML", logs.first + end + def test_process_action get :show wait - assert_equal 1, logs.size - assert_match /Completed/, logs.first - assert_match /\[200\]/, logs.first - assert_match /Another::SubscribersController#show/, logs.first + assert_equal 2, logs.size + assert_match /Completed/, logs.last + assert_match /with 200/, logs.last end def test_process_action_without_parameters @@ -82,14 +88,14 @@ module ActionControllerSubscriberTest get :show, :id => '10' wait - assert_equal 2, logs.size - assert_equal 'Parameters: {"id"=>"10"}', logs[0] + assert_equal 3, logs.size + assert_equal 'Parameters: {"id"=>"10"}', logs[1] end def test_process_action_with_view_runtime get :show wait - assert_match /\(Views: [\d\.]+ms\)/, logs[0] + assert_match /\(Views: [\d\.]+ms\)/, logs[1] end def test_process_action_with_filter_parameters @@ -98,7 +104,7 @@ module ActionControllerSubscriberTest get :show, :lifo => 'Pratik', :amount => '420', :step => '1' wait - params = logs[0] + params = logs[1] assert_match /"amount"=>"\[FILTERED\]"/, params assert_match /"lifo"=>"\[FILTERED\]"/, params assert_match /"step"=>"1"/, params @@ -108,34 +114,34 @@ module ActionControllerSubscriberTest get :redirector wait - assert_equal 2, logs.size - assert_equal "Redirected to http://foo.bar/", logs[0] + assert_equal 3, logs.size + assert_equal "Redirected to http://foo.bar/", logs[1] end def test_send_data get :data_sender wait - assert_equal 2, logs.size - assert_match /Sent data omg\.txt/, logs[0] + assert_equal 3, logs.size + assert_match /Sent data omg\.txt/, logs[1] end def test_send_file get :file_sender wait - assert_equal 2, logs.size - assert_match /Sent file/, logs[0] - assert_match /test\/fixtures\/company\.rb/, logs[0] + assert_equal 3, logs.size + assert_match /Sent file/, logs[1] + assert_match /test\/fixtures\/company\.rb/, logs[1] end def test_send_xfile get :xfile_sender wait - assert_equal 2, logs.size - assert_match /Sent X\-Sendfile header/, logs[0] - assert_match /test\/fixtures\/company\.rb/, logs[0] + assert_equal 3, logs.size + assert_match /Sent X\-Sendfile header/, logs[1] + assert_match /test\/fixtures\/company\.rb/, logs[1] end def test_with_fragment_cache @@ -143,9 +149,9 @@ module ActionControllerSubscriberTest get :with_fragment_cache wait - assert_equal 3, logs.size - assert_match /Exist fragment\? views\/foo/, logs[0] - assert_match /Write fragment views\/foo/, logs[1] + assert_equal 4, logs.size + assert_match /Exist fragment\? views\/foo/, logs[1] + assert_match /Write fragment views\/foo/, logs[2] ensure ActionController::Base.perform_caching = true end @@ -155,9 +161,9 @@ module ActionControllerSubscriberTest get :with_page_cache wait - assert_equal 2, logs.size - assert_match /Write page/, logs[0] - assert_match /\/index\.html/, logs[0] + assert_equal 3, logs.size + assert_match /Write page/, logs[1] + assert_match /\/index\.html/, logs[1] ensure ActionController::Base.perform_caching = true end -- cgit v1.2.3 From 88ffba232987d00717560eaac7ab4f1104e054cd Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Tue, 19 Jan 2010 09:05:34 -0600 Subject: Disable ShowExceptions during integration tests --- .../lib/action_dispatch/testing/integration.rb | 4 +++- .../dispatch/request/json_params_parsing_test.rb | 2 +- .../dispatch/request/xml_params_parsing_test.rb | 2 +- actionpack/test/dispatch/show_exceptions_test.rb | 22 +++++++++++----------- 4 files changed, 16 insertions(+), 14 deletions(-) (limited to 'actionpack') diff --git a/actionpack/lib/action_dispatch/testing/integration.rb b/actionpack/lib/action_dispatch/testing/integration.rb index 4ec47d146c..d4c9df7ebd 100644 --- a/actionpack/lib/action_dispatch/testing/integration.rb +++ b/actionpack/lib/action_dispatch/testing/integration.rb @@ -259,7 +259,9 @@ module ActionDispatch "HTTP_HOST" => host, "REMOTE_ADDR" => remote_addr, "CONTENT_TYPE" => "application/x-www-form-urlencoded", - "HTTP_ACCEPT" => accept + "HTTP_ACCEPT" => accept, + + "action_dispatch.show_exceptions" => false } (rack_environment || {}).each do |key, value| diff --git a/actionpack/test/dispatch/request/json_params_parsing_test.rb b/actionpack/test/dispatch/request/json_params_parsing_test.rb index d3308f73cc..0faa99a912 100644 --- a/actionpack/test/dispatch/request/json_params_parsing_test.rb +++ b/actionpack/test/dispatch/request/json_params_parsing_test.rb @@ -35,7 +35,7 @@ class JsonParamsParsingTest < ActionController::IntegrationTest begin $stderr = StringIO.new json = "[\"person]\": {\"name\": \"David\"}}" - post "/parse", json, {'CONTENT_TYPE' => 'application/json'} + post "/parse", json, {'CONTENT_TYPE' => 'application/json', 'action_dispatch.show_exceptions' => true} assert_response :error $stderr.rewind && err = $stderr.read assert err =~ /Error occurred while parsing request parameters/ diff --git a/actionpack/test/dispatch/request/xml_params_parsing_test.rb b/actionpack/test/dispatch/request/xml_params_parsing_test.rb index 96189e4ca2..488799ac2a 100644 --- a/actionpack/test/dispatch/request/xml_params_parsing_test.rb +++ b/actionpack/test/dispatch/request/xml_params_parsing_test.rb @@ -43,7 +43,7 @@ class XmlParamsParsingTest < ActionController::IntegrationTest begin $stderr = StringIO.new xml = "David#{ActiveSupport::Base64.encode64('ABC')}" - post "/parse", xml, default_headers + post "/parse", xml, default_headers.merge('action_dispatch.show_exceptions' => true) assert_response :error $stderr.rewind && err = $stderr.read assert err =~ /Error occurred while parsing request parameters/ diff --git a/actionpack/test/dispatch/show_exceptions_test.rb b/actionpack/test/dispatch/show_exceptions_test.rb index def86c8323..97da680f17 100644 --- a/actionpack/test/dispatch/show_exceptions_test.rb +++ b/actionpack/test/dispatch/show_exceptions_test.rb @@ -38,15 +38,15 @@ class ShowExceptionsTest < ActionController::IntegrationTest @app = ProductionApp self.remote_addr = '208.77.188.166' - get "/" + get "/", {}, {'action_dispatch.show_exceptions' => true} assert_response 500 assert_equal "500 error fixture\n", body - get "/not_found" + get "/not_found", {}, {'action_dispatch.show_exceptions' => true} assert_response 404 assert_equal "404 error fixture\n", body - get "/method_not_allowed" + get "/method_not_allowed", {}, {'action_dispatch.show_exceptions' => true} assert_response 405 assert_equal "", body end @@ -56,15 +56,15 @@ class ShowExceptionsTest < ActionController::IntegrationTest ['127.0.0.1', '::1'].each do |ip_address| self.remote_addr = ip_address - get "/" + get "/", {}, {'action_dispatch.show_exceptions' => true} assert_response 500 assert_match /puke/, body - get "/not_found" + get "/not_found", {}, {'action_dispatch.show_exceptions' => true} assert_response 404 assert_match /#{ActionController::UnknownAction.name}/, body - get "/method_not_allowed" + get "/method_not_allowed", {}, {'action_dispatch.show_exceptions' => true} assert_response 405 assert_match /ActionController::MethodNotAllowed/, body end @@ -78,11 +78,11 @@ class ShowExceptionsTest < ActionController::IntegrationTest @app = ProductionApp self.remote_addr = '208.77.188.166' - get "/" + get "/", {}, {'action_dispatch.show_exceptions' => true} assert_response 500 assert_equal "500 localized error fixture\n", body - get "/not_found" + get "/not_found", {}, {'action_dispatch.show_exceptions' => true} assert_response 404 assert_equal "404 error fixture\n", body ensure @@ -94,15 +94,15 @@ class ShowExceptionsTest < ActionController::IntegrationTest @app = DevelopmentApp self.remote_addr = '208.77.188.166' - get "/" + get "/", {}, {'action_dispatch.show_exceptions' => true} assert_response 500 assert_match /puke/, body - get "/not_found" + get "/not_found", {}, {'action_dispatch.show_exceptions' => true} assert_response 404 assert_match /#{ActionController::UnknownAction.name}/, body - get "/method_not_allowed" + get "/method_not_allowed", {}, {'action_dispatch.show_exceptions' => true} assert_response 405 assert_match /ActionController::MethodNotAllowed/, body end -- cgit v1.2.3 From a5d06d05fbdc0b66d8ba64a7982288a54b31865b Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Tue, 19 Jan 2010 22:25:10 -0600 Subject: Cleanup middleware introspection output --- actionpack/lib/action_dispatch/middleware/stack.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) (limited to 'actionpack') diff --git a/actionpack/lib/action_dispatch/middleware/stack.rb b/actionpack/lib/action_dispatch/middleware/stack.rb index 24be4fee55..0dc1d70e37 100644 --- a/actionpack/lib/action_dispatch/middleware/stack.rb +++ b/actionpack/lib/action_dispatch/middleware/stack.rb @@ -60,9 +60,7 @@ module ActionDispatch end def inspect - str = klass.to_s - args.each { |arg| str += ", #{build_args.inspect}" } - str + klass.to_s end def build(app) -- cgit v1.2.3 From 5ebfa6242726bd186452640ed5704f2adc1a5007 Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Tue, 19 Jan 2010 22:51:41 -0600 Subject: Revert streaming params parser support. AS Xml and Json parsers expect the request body to be a real IO object supporting methods like getc or ungetc which are not specified by the Rack spec and aren't supported by Passenger or the Rewindable input wrapper. We can restore functionality if the AS parsers are rewritten to support Racks subset of supported IO methods. --- actionpack/lib/action_dispatch/middleware/params_parser.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'actionpack') diff --git a/actionpack/lib/action_dispatch/middleware/params_parser.rb b/actionpack/lib/action_dispatch/middleware/params_parser.rb index 534390d4aa..522982e202 100644 --- a/actionpack/lib/action_dispatch/middleware/params_parser.rb +++ b/actionpack/lib/action_dispatch/middleware/params_parser.rb @@ -35,14 +35,14 @@ module ActionDispatch when Proc strategy.call(request.raw_post) when :xml_simple, :xml_node - request.body.size == 0 ? {} : Hash.from_xml(request.body).with_indifferent_access + request.body.size == 0 ? {} : Hash.from_xml(request.raw_post).with_indifferent_access when :yaml - YAML.load(request.body) + YAML.load(request.raw_post) when :json if request.body.size == 0 {} else - data = ActiveSupport::JSON.decode(request.body) + data = ActiveSupport::JSON.decode(request.raw_post) data = {:_json => data} unless data.is_a?(Hash) data.with_indifferent_access end -- cgit v1.2.3 From 1a50d2e66a80c910fe1e2203eb2c993e5dbc4e5b Mon Sep 17 00:00:00 2001 From: Yehuda Katz Date: Tue, 19 Jan 2010 22:35:09 -0800 Subject: Stop overriding LoadError.new to return a MissingSourceError (and sometimes nil!) --- actionpack/lib/action_controller/metal/helpers.rb | 2 +- actionpack/test/controller/new_base/base_test.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) (limited to 'actionpack') diff --git a/actionpack/lib/action_controller/metal/helpers.rb b/actionpack/lib/action_controller/metal/helpers.rb index d0402e5bad..cdd14560e1 100644 --- a/actionpack/lib/action_controller/metal/helpers.rb +++ b/actionpack/lib/action_controller/metal/helpers.rb @@ -100,7 +100,7 @@ module ActionController module_path = module_name.underscore helper module_path rescue MissingSourceFile => e - raise e unless e.is_missing? "#{module_path}_helper" + raise e unless e.is_missing? "helpers/#{module_path}_helper" rescue NameError => e raise e unless e.missing_name? "#{module_name}Helper" end diff --git a/actionpack/test/controller/new_base/base_test.rb b/actionpack/test/controller/new_base/base_test.rb index 579f9f349f..0b40f8ce95 100644 --- a/actionpack/test/controller/new_base/base_test.rb +++ b/actionpack/test/controller/new_base/base_test.rb @@ -22,7 +22,7 @@ module Dispatching end def show_actions - render :text => "actions: #{action_methods.to_a.join(', ')}" + render :text => "actions: #{action_methods.to_a.sort.join(', ')}" end protected -- cgit v1.2.3 From 8e2fd54b19656a6edbd94f8707927d09e167e7fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Wed, 20 Jan 2010 14:21:27 +0100 Subject: Bring normalize behavior to AbstractController::Rendering --- .../lib/abstract_controller/localized_cache.rb | 2 +- actionpack/lib/abstract_controller/rendering.rb | 22 +++++++++++++--- actionpack/lib/action_controller/base.rb | 29 ++++++---------------- .../lib/action_controller/metal/instrumentation.rb | 16 ++++-------- .../lib/action_controller/metal/rendering.rb | 7 +++--- 5 files changed, 37 insertions(+), 39 deletions(-) (limited to 'actionpack') diff --git a/actionpack/lib/abstract_controller/localized_cache.rb b/actionpack/lib/abstract_controller/localized_cache.rb index bf648af60a..5e3efa002c 100644 --- a/actionpack/lib/abstract_controller/localized_cache.rb +++ b/actionpack/lib/abstract_controller/localized_cache.rb @@ -34,7 +34,7 @@ module AbstractController end end - def render(options) + def render(*args) Thread.current[:format_locale_key] = HashKey.get(self.class, formats, I18n.locale) super end diff --git a/actionpack/lib/abstract_controller/rendering.rb b/actionpack/lib/abstract_controller/rendering.rb index 0dab4a3cc0..644419a585 100644 --- a/actionpack/lib/abstract_controller/rendering.rb +++ b/actionpack/lib/abstract_controller/rendering.rb @@ -40,12 +40,13 @@ module AbstractController # Mostly abstracts the fact that calling render twice is a DoubleRenderError. # Delegates render_to_body and sticks the result in self.response_body. - def render(*args) + def render(*args, &block) if response_body raise AbstractController::DoubleRenderError, "Can only render or redirect once per action" end - self.response_body = render_to_body(*args) + options = _normalize_options(*args, &block) + self.response_body = render_to_body(options) end # Raw rendering of a template to a Rack-compatible body. @@ -69,7 +70,8 @@ module AbstractController # render_to_body into a String. # # :api: plugin - def render_to_string(options = {}) + def render_to_string(*args) + options = _normalize_options(*args) AbstractController::Rendering.body_to_s(render_to_body(options)) end @@ -96,6 +98,20 @@ module AbstractController _view_paths end + # Normalize options, by converting render "foo" to render :template => "foo" + # and render "/foo" to render :file => "/foo". + def _normalize_options(action=nil, options={}) + case action + when Hash + options, action = action, nil + when String + key = (action.index("/") == 0 ? :file : :template) + options.merge!(key => action) + end + + options + end + # Return a string representation of a Rack-compatible response body. def self.body_to_s(body) if body.respond_to?(:to_str) diff --git a/actionpack/lib/action_controller/base.rb b/actionpack/lib/action_controller/base.rb index f86a61d791..4f928e37ea 100644 --- a/actionpack/lib/action_controller/base.rb +++ b/actionpack/lib/action_controller/base.rb @@ -74,17 +74,14 @@ module ActionController @subclasses ||= [] end - def _normalize_options(action = nil, options = {}, &blk) - if action.is_a?(Hash) - options, action = action, nil - elsif action.is_a?(String) || action.is_a?(Symbol) - key = case action = action.to_s - when %r{^/} then :file - when %r{/} then :template - else :action - end - options.merge! key => action - elsif action + def _normalize_options(action=nil, options={}, &blk) + case action + when NilClass + when Hash, String + options = super + when Symbol + options.merge! :action => action + else options.merge! :partial => action end @@ -99,15 +96,5 @@ module ActionController options[:update] = blk if block_given? options end - - def render(action = nil, options = {}, &blk) - options = _normalize_options(action, options, &blk) - super(options) - end - - def render_to_string(action = nil, options = {}, &blk) - options = _normalize_options(action, options, &blk) - super(options) - end end end diff --git a/actionpack/lib/action_controller/metal/instrumentation.rb b/actionpack/lib/action_controller/metal/instrumentation.rb index 7b2b037c67..19c962bafa 100644 --- a/actionpack/lib/action_controller/metal/instrumentation.rb +++ b/actionpack/lib/action_controller/metal/instrumentation.rb @@ -32,18 +32,12 @@ module ActionController end end - def render(*args, &block) - if logger - render_output = nil - - self.view_runtime = cleanup_view_runtime do - Benchmark.ms { render_output = super } - end - - render_output - else - super + def render(*args) + render_output = nil + self.view_runtime = cleanup_view_runtime do + Benchmark.ms { render_output = super } end + render_output end def send_file(path, options={}) diff --git a/actionpack/lib/action_controller/metal/rendering.rb b/actionpack/lib/action_controller/metal/rendering.rb index 74e50bb032..475ed54167 100644 --- a/actionpack/lib/action_controller/metal/rendering.rb +++ b/actionpack/lib/action_controller/metal/rendering.rb @@ -12,9 +12,10 @@ module ActionController super end - def render(options) - super - self.content_type ||= options[:_template].mime_type.to_s + def render(*args) + args << {} unless args.last.is_a?(Hash) + super(*args) + self.content_type ||= args.last[:_template].mime_type.to_s response_body end -- cgit v1.2.3 From 6e26be69606c52dbccfad366661b455157c35be4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Wed, 20 Jan 2010 14:41:23 +0100 Subject: Move ActionController::Translation to AbstractController::Translation. --- actionpack/lib/abstract_controller.rb | 1 + actionpack/lib/abstract_controller/rendering.rb | 2 +- actionpack/lib/abstract_controller/translation.rb | 13 ++++++++++++ actionpack/lib/action_controller.rb | 1 - actionpack/lib/action_controller/base.rb | 2 +- actionpack/lib/action_controller/translation.rb | 13 ------------ actionpack/test/abstract/translation_test.rb | 26 +++++++++++++++++++++++ actionpack/test/controller/translation_test.rb | 26 ----------------------- 8 files changed, 42 insertions(+), 42 deletions(-) create mode 100644 actionpack/lib/abstract_controller/translation.rb delete mode 100644 actionpack/lib/action_controller/translation.rb create mode 100644 actionpack/test/abstract/translation_test.rb delete mode 100644 actionpack/test/controller/translation_test.rb (limited to 'actionpack') diff --git a/actionpack/lib/abstract_controller.rb b/actionpack/lib/abstract_controller.rb index efc35a7e56..725d8fb8fc 100644 --- a/actionpack/lib/abstract_controller.rb +++ b/actionpack/lib/abstract_controller.rb @@ -15,5 +15,6 @@ module AbstractController autoload :LocalizedCache autoload :Logger autoload :Rendering + autoload :Translation autoload :UrlFor end diff --git a/actionpack/lib/abstract_controller/rendering.rb b/actionpack/lib/abstract_controller/rendering.rb index 644419a585..826e82c8c6 100644 --- a/actionpack/lib/abstract_controller/rendering.rb +++ b/actionpack/lib/abstract_controller/rendering.rb @@ -42,7 +42,7 @@ module AbstractController # Delegates render_to_body and sticks the result in self.response_body. def render(*args, &block) if response_body - raise AbstractController::DoubleRenderError, "Can only render or redirect once per action" + raise AbstractController::DoubleRenderError end options = _normalize_options(*args, &block) diff --git a/actionpack/lib/abstract_controller/translation.rb b/actionpack/lib/abstract_controller/translation.rb new file mode 100644 index 0000000000..6d68cf4944 --- /dev/null +++ b/actionpack/lib/abstract_controller/translation.rb @@ -0,0 +1,13 @@ +module AbstractController + module Translation + def translate(*args) + I18n.translate(*args) + end + alias :t :translate + + def localize(*args) + I18n.localize(*args) + end + alias :l :localize + end +end \ No newline at end of file diff --git a/actionpack/lib/action_controller.rb b/actionpack/lib/action_controller.rb index 8bc2cc79d2..fa4a253ec1 100644 --- a/actionpack/lib/action_controller.rb +++ b/actionpack/lib/action_controller.rb @@ -8,7 +8,6 @@ module ActionController autoload :Base autoload :Caching autoload :PolymorphicRoutes - autoload :Translation autoload :Metal autoload :Middleware diff --git a/actionpack/lib/action_controller/base.rb b/actionpack/lib/action_controller/base.rb index 4f928e37ea..21a811c004 100644 --- a/actionpack/lib/action_controller/base.rb +++ b/actionpack/lib/action_controller/base.rb @@ -4,6 +4,7 @@ module ActionController include AbstractController::Callbacks include AbstractController::Layouts + include AbstractController::Translation include ActionController::Helpers helper :all # By default, all helpers should be included @@ -33,7 +34,6 @@ module ActionController include ActionController::Streaming include ActionController::HttpAuthentication::Basic::ControllerMethods include ActionController::HttpAuthentication::Digest::ControllerMethods - include ActionController::Translation # Add instrumentations hooks at the bottom, to ensure they instrument # all the methods properly. diff --git a/actionpack/lib/action_controller/translation.rb b/actionpack/lib/action_controller/translation.rb deleted file mode 100644 index 65e9eddb0a..0000000000 --- a/actionpack/lib/action_controller/translation.rb +++ /dev/null @@ -1,13 +0,0 @@ -module ActionController - module Translation - def translate(*args) - I18n.translate(*args) - end - alias :t :translate - - def localize(*args) - I18n.localize(*args) - end - alias :l :localize - end -end \ No newline at end of file diff --git a/actionpack/test/abstract/translation_test.rb b/actionpack/test/abstract/translation_test.rb new file mode 100644 index 0000000000..0bf61a6556 --- /dev/null +++ b/actionpack/test/abstract/translation_test.rb @@ -0,0 +1,26 @@ +require 'abstract_unit' + +# class TranslatingController < ActionController::Base +# end + +class TranslationControllerTest < Test::Unit::TestCase + def setup + @controller = ActionController::Base.new + end + + def test_action_controller_base_responds_to_translate + assert @controller.respond_to?(:translate) + end + + def test_action_controller_base_responds_to_t + assert @controller.respond_to?(:t) + end + + def test_action_controller_base_responds_to_localize + assert @controller.respond_to?(:localize) + end + + def test_action_controller_base_responds_to_l + assert @controller.respond_to?(:l) + end +end \ No newline at end of file diff --git a/actionpack/test/controller/translation_test.rb b/actionpack/test/controller/translation_test.rb deleted file mode 100644 index 0bf61a6556..0000000000 --- a/actionpack/test/controller/translation_test.rb +++ /dev/null @@ -1,26 +0,0 @@ -require 'abstract_unit' - -# class TranslatingController < ActionController::Base -# end - -class TranslationControllerTest < Test::Unit::TestCase - def setup - @controller = ActionController::Base.new - end - - def test_action_controller_base_responds_to_translate - assert @controller.respond_to?(:translate) - end - - def test_action_controller_base_responds_to_t - assert @controller.respond_to?(:t) - end - - def test_action_controller_base_responds_to_localize - assert @controller.respond_to?(:localize) - end - - def test_action_controller_base_responds_to_l - assert @controller.respond_to?(:l) - end -end \ No newline at end of file -- cgit v1.2.3 From 909443eab67c4f07aeb6a294e3858792f075b3ab Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Wed, 20 Jan 2010 08:59:26 -0600 Subject: Expose last controller in rack env["action_controller.instance"] --- actionpack/lib/action_controller/metal.rb | 1 + .../lib/action_dispatch/testing/integration.rb | 38 ++++------------------ 2 files changed, 7 insertions(+), 32 deletions(-) (limited to 'actionpack') diff --git a/actionpack/lib/action_controller/metal.rb b/actionpack/lib/action_controller/metal.rb index 1819c0f886..57627a6f0b 100644 --- a/actionpack/lib/action_controller/metal.rb +++ b/actionpack/lib/action_controller/metal.rb @@ -60,6 +60,7 @@ module ActionController # :api: private def dispatch(name, env) @_env = env + @_env['action_controller.instance'] = self process(name) to_a end diff --git a/actionpack/lib/action_dispatch/testing/integration.rb b/actionpack/lib/action_dispatch/testing/integration.rb index d4c9df7ebd..2093bb3a0e 100644 --- a/actionpack/lib/action_dispatch/testing/integration.rb +++ b/actionpack/lib/action_dispatch/testing/integration.rb @@ -240,9 +240,9 @@ module ActionDispatch path = location.query ? "#{location.path}?#{location.query}" : location.path end - [ControllerCapture, ActionController::Testing].each do |mod| - unless ActionController::Base < mod - ActionController::Base.class_eval { include mod } + unless ActionController::Base < ActionController::Testing + ActionController::Base.class_eval do + include ActionController::Testing end end @@ -269,16 +269,15 @@ module ActionDispatch end session = Rack::Test::Session.new(@mock_session) - - @controller = ActionController::Base.capture_instantiation do - session.request(path, env) - end + session.request(path, env) @request_count += 1 @request = ActionDispatch::Request.new(session.last_request.env) @response = ActionDispatch::TestResponse.from_response(@mock_session.last_response) @html_document = nil + @controller = session.last_request.env['action_controller.instance'] + return response.status end @@ -296,31 +295,6 @@ module ActionDispatch end end - # A module used to extend ActionController::Base, so that integration tests - # can capture the controller used to satisfy a request. - module ControllerCapture #:nodoc: - extend ActiveSupport::Concern - - included do - alias_method_chain :initialize, :capture - end - - def initialize_with_capture(*args) - initialize_without_capture - self.class.last_instantiation ||= self - end - - module ClassMethods #:nodoc: - mattr_accessor :last_instantiation - - def capture_instantiation - self.last_instantiation = nil - yield - return last_instantiation - end - end - end - module Runner def app @app -- cgit v1.2.3 From 87bcf1aa15f8edb4287e1916b65a2c523f765e86 Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Wed, 20 Jan 2010 09:55:33 -0600 Subject: Request#filter_parameters and filter_env --- actionpack/lib/action_dispatch/http/parameters.rb | 26 +++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) (limited to 'actionpack') diff --git a/actionpack/lib/action_dispatch/http/parameters.rb b/actionpack/lib/action_dispatch/http/parameters.rb index 97546d5f93..68ba3637bf 100644 --- a/actionpack/lib/action_dispatch/http/parameters.rb +++ b/actionpack/lib/action_dispatch/http/parameters.rb @@ -29,9 +29,31 @@ module ActionDispatch def path_parameters @env["action_dispatch.request.path_parameters"] ||= {} end - - private + def filter_parameters + # TODO: Remove dependency on controller + if controller = @env['action_controller.instance'] + controller.send(:filter_parameters, params) + else + params + end + end + + def filter_env + if controller = @env['action_controller.instance'] + @env.map do |key, value| + if (key =~ /RAW_POST_DATA/i) + '[FILTERED]' + else + controller.send(:filter_parameters, {key => value}).values[0] + end + end + else + env + end + end + + private # Convert nested Hashs to HashWithIndifferentAccess def normalize_parameters(value) case value -- cgit v1.2.3 From 93956a18e45b6bc2127da6b71dfab53516da4593 Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Wed, 20 Jan 2010 10:07:23 -0600 Subject: Only send filtered_env for notifications --- .../lib/action_dispatch/middleware/notifications.rb | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) (limited to 'actionpack') diff --git a/actionpack/lib/action_dispatch/middleware/notifications.rb b/actionpack/lib/action_dispatch/middleware/notifications.rb index c3776d53a8..ce3732b740 100644 --- a/actionpack/lib/action_dispatch/middleware/notifications.rb +++ b/actionpack/lib/action_dispatch/middleware/notifications.rb @@ -9,7 +9,9 @@ module ActionDispatch end def call(env) - payload = retrieve_payload_from_env(env) + request = Request.new(env) + payload = retrieve_payload_from_env(request.filter_env) + ActiveSupport::Notifications.instrument("action_dispatch.before_dispatch", payload) ActiveSupport::Notifications.instrument!("action_dispatch.after_dispatch", payload) do @@ -21,11 +23,10 @@ module ActionDispatch raise exception end - protected - - # Remove any rack related constants from the env, like rack.input. - def retrieve_payload_from_env(env) - Hash[:env => env.except(*env.keys.select { |k| k.to_s.index("rack.") == 0 })] - end + protected + # Remove any rack related constants from the env, like rack.input. + def retrieve_payload_from_env(env) + Hash[:env => env.except(*env.keys.select { |k| k.to_s.index("rack.") == 0 })] + end end -end \ No newline at end of file +end -- cgit v1.2.3 From c8cba7db76d7128dfd8fd03de0e085c81bbed92a Mon Sep 17 00:00:00 2001 From: Sam Ruby Date: Wed, 20 Jan 2010 10:56:06 -0600 Subject: Add AD::Route#to_s Signed-off-by: Joshua Peek --- actionpack/lib/action_dispatch/routing/route.rb | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 'actionpack') diff --git a/actionpack/lib/action_dispatch/routing/route.rb b/actionpack/lib/action_dispatch/routing/route.rb index f1431e7a37..e6e44d3546 100644 --- a/actionpack/lib/action_dispatch/routing/route.rb +++ b/actionpack/lib/action_dispatch/routing/route.rb @@ -44,6 +44,12 @@ module ActionDispatch def to_a [@app, @conditions, @defaults, @name] end + + def to_s + @to_s ||= begin + "%-6s %-40s %s" % [(verb || :any).to_s.upcase, path, requirements.inspect] + end + end end end end -- cgit v1.2.3 From bd4f21fbac495fb28b6be993be808509e567239e Mon Sep 17 00:00:00 2001 From: Prem Sichanugrist Date: Thu, 21 Jan 2010 04:37:10 +0700 Subject: Move filter_parameter_logging logic out of the controller and create ActionDispatch::ParametersFilter to handle parameter filteration instead. This will make filteration not depending on controller anymore. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: José Valim --- .../metal/filter_parameter_logging.rb | 36 +-------- .../lib/action_controller/metal/instrumentation.rb | 2 +- actionpack/lib/action_dispatch.rb | 1 + actionpack/lib/action_dispatch/http/parameters.rb | 23 ------ .../lib/action_dispatch/http/parameters_filter.rb | 88 ++++++++++++++++++++++ actionpack/lib/action_dispatch/http/request.rb | 1 + .../action_dispatch/middleware/notifications.rb | 2 +- actionpack/test/dispatch/request_test.rb | 34 +++++++++ 8 files changed, 130 insertions(+), 57 deletions(-) create mode 100644 actionpack/lib/action_dispatch/http/parameters_filter.rb (limited to 'actionpack') diff --git a/actionpack/lib/action_controller/metal/filter_parameter_logging.rb b/actionpack/lib/action_controller/metal/filter_parameter_logging.rb index 9e03f50759..befb4a58cc 100644 --- a/actionpack/lib/action_controller/metal/filter_parameter_logging.rb +++ b/actionpack/lib/action_controller/metal/filter_parameter_logging.rb @@ -2,8 +2,6 @@ module ActionController module FilterParameterLogging extend ActiveSupport::Concern - INTERNAL_PARAMS = %w(controller action format _method only_path) - module ClassMethods # Replace sensitive parameter data from the request log. # Filters parameters that have any of the arguments as a substring. @@ -27,40 +25,14 @@ module ActionController # => reverses the value to all keys matching /secret/i, and # replaces the value to all keys matching /foo|bar/i with "[FILTERED]" def filter_parameter_logging(*filter_words, &block) - raise "You must filter at least one word from logging" if filter_words.empty? - - parameter_filter = Regexp.new(filter_words.join('|'), true) - - define_method(:filter_parameters) do |original_params| - filtered_params = {} - - original_params.each do |key, value| - if key =~ parameter_filter - value = '[FILTERED]' - elsif value.is_a?(Hash) - value = filter_parameters(value) - elsif value.is_a?(Array) - value = value.map { |item| filter_parameters(item) } - elsif block_given? - key = key.dup - value = value.dup if value.duplicable? - yield key, value - end - - filtered_params[key] = value - end - - filtered_params.except!(*INTERNAL_PARAMS) - end - protected :filter_parameters + ActionDispatch::Http::ParametersFilter.filter_parameters(*filter_words, &block) end end - + protected - + def filter_parameters(params) - params.dup.except!(*INTERNAL_PARAMS) + request.send(:process_parameter_filter, params) end - end end diff --git a/actionpack/lib/action_controller/metal/instrumentation.rb b/actionpack/lib/action_controller/metal/instrumentation.rb index 19c962bafa..0f22bf96cf 100644 --- a/actionpack/lib/action_controller/metal/instrumentation.rb +++ b/actionpack/lib/action_controller/metal/instrumentation.rb @@ -18,7 +18,7 @@ module ActionController raw_payload = { :controller => self.class.name, :action => self.action_name, - :params => filter_parameters(params), + :params => request.filtered_parameters, :formats => request.formats.map(&:to_sym) } diff --git a/actionpack/lib/action_dispatch.rb b/actionpack/lib/action_dispatch.rb index 082562d921..e2d64fcd53 100644 --- a/actionpack/lib/action_dispatch.rb +++ b/actionpack/lib/action_dispatch.rb @@ -63,6 +63,7 @@ module ActionDispatch autoload :Headers autoload :MimeNegotiation autoload :Parameters + autoload :ParametersFilter autoload :Upload autoload :UploadedFile, 'action_dispatch/http/upload' autoload :URL diff --git a/actionpack/lib/action_dispatch/http/parameters.rb b/actionpack/lib/action_dispatch/http/parameters.rb index 68ba3637bf..40b40ea94e 100644 --- a/actionpack/lib/action_dispatch/http/parameters.rb +++ b/actionpack/lib/action_dispatch/http/parameters.rb @@ -30,29 +30,6 @@ module ActionDispatch @env["action_dispatch.request.path_parameters"] ||= {} end - def filter_parameters - # TODO: Remove dependency on controller - if controller = @env['action_controller.instance'] - controller.send(:filter_parameters, params) - else - params - end - end - - def filter_env - if controller = @env['action_controller.instance'] - @env.map do |key, value| - if (key =~ /RAW_POST_DATA/i) - '[FILTERED]' - else - controller.send(:filter_parameters, {key => value}).values[0] - end - end - else - env - end - end - private # Convert nested Hashs to HashWithIndifferentAccess def normalize_parameters(value) diff --git a/actionpack/lib/action_dispatch/http/parameters_filter.rb b/actionpack/lib/action_dispatch/http/parameters_filter.rb new file mode 100644 index 0000000000..bec5e7427d --- /dev/null +++ b/actionpack/lib/action_dispatch/http/parameters_filter.rb @@ -0,0 +1,88 @@ +require 'active_support/core_ext/hash/keys' + +module ActionDispatch + module Http + module ParametersFilter + INTERNAL_PARAMS = %w(controller action format _method only_path) + + @@filter_parameters = nil + @@filter_parameters_block = nil + + # Specify sensitive parameters which will be replaced from the request log. + # Filters parameters that have any of the arguments as a substring. + # Looks in all subhashes of the param hash for keys to filter. + # If a block is given, each key and value of the parameter hash and all + # subhashes is passed to it, the value or key + # can be replaced using String#replace or similar method. + # + # Examples: + # + # ActionDispatch::Http::ParametersFilter.filter_parameters :password + # => replaces the value to all keys matching /password/i with "[FILTERED]" + # + # ActionDispatch::Http::ParametersFilter.filter_parameters :foo, "bar" + # => replaces the value to all keys matching /foo|bar/i with "[FILTERED]" + # + # ActionDispatch::Http::ParametersFilter.filter_parameters do |k,v| + # v.reverse! if k =~ /secret/i + # end + # => reverses the value to all keys matching /secret/i + # + # ActionDispatch::Http::ParametersFilter.filter_parameters(:foo, "bar") do |k,v| + # v.reverse! if k =~ /secret/i + # end + # => reverses the value to all keys matching /secret/i, and + # replaces the value to all keys matching /foo|bar/i with "[FILTERED]" + def self.filter_parameters(*filter_words, &block) + raise "You must filter at least one word" if filter_words.empty? and !block_given? + + @@filter_parameters = filter_words.empty? ? nil : Regexp.new(filter_words.join('|'), true) + @@filter_parameters_block = block + end + + # Return a hash of parameters with all sensitive data replaced. + def filtered_parameters + @filtered_parameters ||= process_parameter_filter(parameters) + end + alias_method :fitered_params, :filtered_parameters + + # Return a hash of request.env with all sensitive data replaced. + def filtered_env + @env.merge(@env) do |key, value| + if (key =~ /RAW_POST_DATA/i) + '[FILTERED]' + else + process_parameter_filter({key => value}, false).values[0] + end + end + end + + protected + + def process_parameter_filter(original_parameters, validate_block = true) + if @@filter_parameters or @@filter_parameters_block + filtered_params = {} + + original_parameters.each do |key, value| + if key =~ @@filter_parameters + value = '[FILTERED]' + elsif value.is_a?(Hash) + value = process_parameter_filter(value) + elsif value.is_a?(Array) + value = value.map { |item| process_parameter_filter({key => item}, validate_block).values[0] } + elsif validate_block and @@filter_parameters_block + key = key.dup + value = value.dup if value.duplicable? + value = @@filter_parameters_block.call(key, value) || value + end + + filtered_params[key] = value + end + filtered_params.except!(*INTERNAL_PARAMS) + else + original_parameters.except(*INTERNAL_PARAMS) + end + end + end + end +end \ No newline at end of file diff --git a/actionpack/lib/action_dispatch/http/request.rb b/actionpack/lib/action_dispatch/http/request.rb index 187ce7c15d..7c3a228149 100755 --- a/actionpack/lib/action_dispatch/http/request.rb +++ b/actionpack/lib/action_dispatch/http/request.rb @@ -11,6 +11,7 @@ module ActionDispatch include ActionDispatch::Http::Cache::Request include ActionDispatch::Http::MimeNegotiation include ActionDispatch::Http::Parameters + include ActionDispatch::Http::ParametersFilter include ActionDispatch::Http::Upload include ActionDispatch::Http::URL diff --git a/actionpack/lib/action_dispatch/middleware/notifications.rb b/actionpack/lib/action_dispatch/middleware/notifications.rb index ce3732b740..65409f57fd 100644 --- a/actionpack/lib/action_dispatch/middleware/notifications.rb +++ b/actionpack/lib/action_dispatch/middleware/notifications.rb @@ -10,7 +10,7 @@ module ActionDispatch def call(env) request = Request.new(env) - payload = retrieve_payload_from_env(request.filter_env) + payload = retrieve_payload_from_env(request.filtered_env) ActiveSupport::Notifications.instrument("action_dispatch.before_dispatch", payload) diff --git a/actionpack/test/dispatch/request_test.rb b/actionpack/test/dispatch/request_test.rb index cb95ecea50..0a6180959e 100644 --- a/actionpack/test/dispatch/request_test.rb +++ b/actionpack/test/dispatch/request_test.rb @@ -453,6 +453,40 @@ class RequestTest < ActiveSupport::TestCase request.expects(:parameters).at_least_once.returns({}) assert_equal Mime::XML, request.negotiate_mime([Mime::XML, Mime::CSV]) end + + test "filter_parameters" do + request = stub_request + request.stubs(:request_parameters).returns({ "foo" => 1 }) + request.stubs(:query_parameters).returns({ "bar" => 2 }) + + assert_raises RuntimeError do + ActionDispatch::Http::ParametersFilter.filter_parameters + end + + test_hashes = [ + [{'foo'=>'bar'},{'foo'=>'bar'},%w'food'], + [{'foo'=>'bar'},{'foo'=>'[FILTERED]'},%w'foo'], + [{'foo'=>'bar', 'bar'=>'foo'},{'foo'=>'[FILTERED]', 'bar'=>'foo'},%w'foo baz'], + [{'foo'=>'bar', 'baz'=>'foo'},{'foo'=>'[FILTERED]', 'baz'=>'[FILTERED]'},%w'foo baz'], + [{'bar'=>{'foo'=>'bar','bar'=>'foo'}},{'bar'=>{'foo'=>'[FILTERED]','bar'=>'foo'}},%w'fo'], + [{'foo'=>{'foo'=>'bar','bar'=>'foo'}},{'foo'=>'[FILTERED]'},%w'f banana'], + [{'baz'=>[{'foo'=>'baz'}]}, {'baz'=>[{'foo'=>'[FILTERED]'}]}, %w(foo)]] + + test_hashes.each do |before_filter, after_filter, filter_words| + ActionDispatch::Http::ParametersFilter.filter_parameters(*filter_words) + assert_equal after_filter, request.__send__(:process_parameter_filter, before_filter) + + filter_words.push('blah') + ActionDispatch::Http::ParametersFilter.filter_parameters(*filter_words) do |key, value| + value.reverse! if key =~ /bargain/ + end + + before_filter['barg'] = {'bargain'=>'gain', 'blah'=>'bar', 'bar'=>{'bargain'=>{'blah'=>'foo'}}} + after_filter['barg'] = {'bargain'=>'niag', 'blah'=>'[FILTERED]', 'bar'=>{'bargain'=>{'blah'=>'[FILTERED]'}}} + + assert_equal after_filter, request.__send__(:process_parameter_filter, before_filter) + end + end protected -- cgit v1.2.3 From b1bc3b3cd352f68d79d7e232e9520eacb56ca41e Mon Sep 17 00:00:00 2001 From: Prem Sichanugrist Date: Thu, 21 Jan 2010 11:48:27 +0700 Subject: Add deprecation warning for calling filter_parameter_logging ActionController::Base, and allow it to be configured in config.filter_parameters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: José Valim --- .../metal/filter_parameter_logging.rb | 29 +----------- actionpack/test/controller/filter_params_test.rb | 51 ---------------------- 2 files changed, 2 insertions(+), 78 deletions(-) delete mode 100644 actionpack/test/controller/filter_params_test.rb (limited to 'actionpack') diff --git a/actionpack/lib/action_controller/metal/filter_parameter_logging.rb b/actionpack/lib/action_controller/metal/filter_parameter_logging.rb index befb4a58cc..b59f6df244 100644 --- a/actionpack/lib/action_controller/metal/filter_parameter_logging.rb +++ b/actionpack/lib/action_controller/metal/filter_parameter_logging.rb @@ -3,36 +3,11 @@ module ActionController extend ActiveSupport::Concern module ClassMethods - # Replace sensitive parameter data from the request log. - # Filters parameters that have any of the arguments as a substring. - # Looks in all subhashes of the param hash for keys to filter. - # If a block is given, each key and value of the parameter hash and all - # subhashes is passed to it, the value or key - # can be replaced using String#replace or similar method. - # - # Examples: - # - # filter_parameter_logging :password - # => replaces the value to all keys matching /password/i with "[FILTERED]" - # - # filter_parameter_logging :foo, "bar" - # => replaces the value to all keys matching /foo|bar/i with "[FILTERED]" - # - # filter_parameter_logging { |k,v| v.reverse! if k =~ /secret/i } - # => reverses the value to all keys matching /secret/i - # - # filter_parameter_logging(:foo, "bar") { |k,v| v.reverse! if k =~ /secret/i } - # => reverses the value to all keys matching /secret/i, and - # replaces the value to all keys matching /foo|bar/i with "[FILTERED]" + # This method has been moved to ActionDispatch::Http::ParametersFilter.filter_parameters def filter_parameter_logging(*filter_words, &block) + ActiveSupport::Deprecation.warn("Setting filter_parameter_logging in ActionController is deprecated, please set 'config.filter_parameters' in application.rb or environments/[environment_name].rb instead.", caller) ActionDispatch::Http::ParametersFilter.filter_parameters(*filter_words, &block) end end - - protected - - def filter_parameters(params) - request.send(:process_parameter_filter, params) - end end end diff --git a/actionpack/test/controller/filter_params_test.rb b/actionpack/test/controller/filter_params_test.rb deleted file mode 100644 index 45949636c3..0000000000 --- a/actionpack/test/controller/filter_params_test.rb +++ /dev/null @@ -1,51 +0,0 @@ -require 'abstract_unit' - -class FilterParamController < ActionController::Base - def payment - head :ok - end -end - -class FilterParamTest < ActionController::TestCase - tests FilterParamController - - def test_filter_parameters_must_have_one_word - assert_raises RuntimeError do - FilterParamController.filter_parameter_logging - end - end - - def test_filter_parameters - assert FilterParamController.respond_to?(:filter_parameter_logging) - - test_hashes = [ - [{'foo'=>'bar'},{'foo'=>'bar'},%w'food'], - [{'foo'=>'bar'},{'foo'=>'[FILTERED]'},%w'foo'], - [{'foo'=>'bar', 'bar'=>'foo'},{'foo'=>'[FILTERED]', 'bar'=>'foo'},%w'foo baz'], - [{'foo'=>'bar', 'baz'=>'foo'},{'foo'=>'[FILTERED]', 'baz'=>'[FILTERED]'},%w'foo baz'], - [{'bar'=>{'foo'=>'bar','bar'=>'foo'}},{'bar'=>{'foo'=>'[FILTERED]','bar'=>'foo'}},%w'fo'], - [{'foo'=>{'foo'=>'bar','bar'=>'foo'}},{'foo'=>'[FILTERED]'},%w'f banana'], - [{'baz'=>[{'foo'=>'baz'}]}, {'baz'=>[{'foo'=>'[FILTERED]'}]}, %w(foo)]] - - test_hashes.each do |before_filter, after_filter, filter_words| - FilterParamController.filter_parameter_logging(*filter_words) - assert_equal after_filter, @controller.__send__(:filter_parameters, before_filter) - - filter_words.push('blah') - FilterParamController.filter_parameter_logging(*filter_words) do |key, value| - value.reverse! if key =~ /bargain/ - end - - before_filter['barg'] = {'bargain'=>'gain', 'blah'=>'bar', 'bar'=>{'bargain'=>{'blah'=>'foo'}}} - after_filter['barg'] = {'bargain'=>'niag', 'blah'=>'[FILTERED]', 'bar'=>{'bargain'=>{'blah'=>'[FILTERED]'}}} - - assert_equal after_filter, @controller.__send__(:filter_parameters, before_filter) - end - end - - def test_filter_parameters_is_protected - FilterParamController.filter_parameter_logging(:foo) - assert !FilterParamController.action_methods.include?('filter_parameters') - assert_raise(NoMethodError) { @controller.filter_parameters([{'password' => '[FILTERED]'}]) } - end -end -- cgit v1.2.3 From 31fddf2ace29518399f15f718ff408737e0031a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Thu, 21 Jan 2010 11:39:57 +0100 Subject: Tidy up new filter_parameters implementation. --- actionpack/lib/action_controller.rb | 1 - actionpack/lib/action_controller/base.rb | 6 +- .../metal/filter_parameter_logging.rb | 13 --- .../lib/action_controller/metal/instrumentation.rb | 1 - actionpack/lib/action_dispatch.rb | 2 +- .../lib/action_dispatch/http/filter_parameters.rb | 94 ++++++++++++++++++++++ .../lib/action_dispatch/http/parameters_filter.rb | 88 -------------------- actionpack/lib/action_dispatch/http/request.rb | 2 +- actionpack/test/controller/subscriber_test.rb | 5 +- actionpack/test/dispatch/request_test.rb | 55 ++++++++++--- 10 files changed, 147 insertions(+), 120 deletions(-) delete mode 100644 actionpack/lib/action_controller/metal/filter_parameter_logging.rb create mode 100644 actionpack/lib/action_dispatch/http/filter_parameters.rb delete mode 100644 actionpack/lib/action_dispatch/http/parameters_filter.rb (limited to 'actionpack') diff --git a/actionpack/lib/action_controller.rb b/actionpack/lib/action_controller.rb index fa4a253ec1..33e107d216 100644 --- a/actionpack/lib/action_controller.rb +++ b/actionpack/lib/action_controller.rb @@ -16,7 +16,6 @@ module ActionController autoload :ConditionalGet autoload :Configuration autoload :Cookies - autoload :FilterParameterLogging autoload :Flash autoload :Head autoload :Helpers diff --git a/actionpack/lib/action_controller/base.rb b/actionpack/lib/action_controller/base.rb index 21a811c004..cf66e99fa5 100644 --- a/actionpack/lib/action_controller/base.rb +++ b/actionpack/lib/action_controller/base.rb @@ -27,7 +27,6 @@ module ActionController include ActionController::Compatibility include ActionController::Cookies - include ActionController::FilterParameterLogging include ActionController::Flash include ActionController::Verification include ActionController::RequestForgeryProtection @@ -74,6 +73,11 @@ module ActionController @subclasses ||= [] end + # This method has been moved to ActionDispatch::Request.filter_parameters + def self.filter_parameter_logging(*) + ActiveSupport::Deprecation.warn("Setting filter_parameter_logging in ActionController is deprecated and has no longer effect, please set 'config.filter_parameters' in config/application.rb instead", caller) + end + def _normalize_options(action=nil, options={}, &blk) case action when NilClass diff --git a/actionpack/lib/action_controller/metal/filter_parameter_logging.rb b/actionpack/lib/action_controller/metal/filter_parameter_logging.rb deleted file mode 100644 index b59f6df244..0000000000 --- a/actionpack/lib/action_controller/metal/filter_parameter_logging.rb +++ /dev/null @@ -1,13 +0,0 @@ -module ActionController - module FilterParameterLogging - extend ActiveSupport::Concern - - module ClassMethods - # This method has been moved to ActionDispatch::Http::ParametersFilter.filter_parameters - def filter_parameter_logging(*filter_words, &block) - ActiveSupport::Deprecation.warn("Setting filter_parameter_logging in ActionController is deprecated, please set 'config.filter_parameters' in application.rb or environments/[environment_name].rb instead.", caller) - ActionDispatch::Http::ParametersFilter.filter_parameters(*filter_words, &block) - end - end - end -end diff --git a/actionpack/lib/action_controller/metal/instrumentation.rb b/actionpack/lib/action_controller/metal/instrumentation.rb index 0f22bf96cf..7f9a7c068b 100644 --- a/actionpack/lib/action_controller/metal/instrumentation.rb +++ b/actionpack/lib/action_controller/metal/instrumentation.rb @@ -10,7 +10,6 @@ module ActionController extend ActiveSupport::Concern include AbstractController::Logger - include ActionController::FilterParameterLogging attr_internal :view_runtime diff --git a/actionpack/lib/action_dispatch.rb b/actionpack/lib/action_dispatch.rb index e2d64fcd53..42ff9ca2e4 100644 --- a/actionpack/lib/action_dispatch.rb +++ b/actionpack/lib/action_dispatch.rb @@ -63,7 +63,7 @@ module ActionDispatch autoload :Headers autoload :MimeNegotiation autoload :Parameters - autoload :ParametersFilter + autoload :FilterParameters autoload :Upload autoload :UploadedFile, 'action_dispatch/http/upload' autoload :URL diff --git a/actionpack/lib/action_dispatch/http/filter_parameters.rb b/actionpack/lib/action_dispatch/http/filter_parameters.rb new file mode 100644 index 0000000000..0f4afb01d9 --- /dev/null +++ b/actionpack/lib/action_dispatch/http/filter_parameters.rb @@ -0,0 +1,94 @@ +require 'active_support/core_ext/hash/keys' + +module ActionDispatch + module Http + module FilterParameters + extend ActiveSupport::Concern + + INTERNAL_PARAMS = %w(controller action format _method only_path) + + module ClassMethods + # Specify sensitive parameters which will be replaced from the request log. + # Filters parameters that have any of the arguments as a substring. + # Looks in all subhashes of the param hash for keys to filter. + # If a block is given, each key and value of the parameter hash and all + # subhashes is passed to it, the value or key + # can be replaced using String#replace or similar method. + # + # Examples: + # + # ActionDispatch::Request.filter_parameters :password + # => replaces the value to all keys matching /password/i with "[FILTERED]" + # + # ActionDispatch::Request.filter_parameters :foo, "bar" + # => replaces the value to all keys matching /foo|bar/i with "[FILTERED]" + # + # ActionDispatch::Request.filter_parameters do |k,v| + # v.reverse! if k =~ /secret/i + # end + # => reverses the value to all keys matching /secret/i + # + # ActionDispatch::Request.filter_parameters(:foo, "bar") do |k,v| + # v.reverse! if k =~ /secret/i + # end + # => reverses the value to all keys matching /secret/i, and + # replaces the value to all keys matching /foo|bar/i with "[FILTERED]" + def filter_parameters(*filter_words, &block) + raise "You must filter at least one word" if filter_words.empty? + + parameter_filter = Regexp.new(filter_words.join('|'), true) + + define_method(:process_parameter_filter) do |original_params| + filtered_params = {} + + original_params.each do |key, value| + if key =~ parameter_filter + value = '[FILTERED]' + elsif value.is_a?(Hash) + value = process_parameter_filter(value) + elsif value.is_a?(Array) + value = value.map { |i| process_parameter_filter(i) } + elsif block_given? + key = key.dup + value = value.dup if value.duplicable? + yield key, value + end + + filtered_params[key] = value + end + + filtered_params.except!(*INTERNAL_PARAMS) + end + + protected :process_parameter_filter + end + end + + # Return a hash of parameters with all sensitive data replaced. + def filtered_parameters + @filtered_parameters ||= process_parameter_filter(parameters) + end + alias :fitered_params :filtered_parameters + + # Return a hash of request.env with all sensitive data replaced. + # TODO Josh should white list env to remove stuff like rack.input and rack.errors + def filtered_env + filtered_env = @env.dup + filtered_env.each do |key, value| + if (key =~ /RAW_POST_DATA/i) + filtered_env[key] = '[FILTERED]' + elsif value.is_a?(Hash) + filtered_env[key] = process_parameter_filter(value) + end + end + filtered_env + end + + protected + + def process_parameter_filter(original_parameters) + original_parameters.except(*INTERNAL_PARAMS) + end + end + end +end \ No newline at end of file diff --git a/actionpack/lib/action_dispatch/http/parameters_filter.rb b/actionpack/lib/action_dispatch/http/parameters_filter.rb deleted file mode 100644 index bec5e7427d..0000000000 --- a/actionpack/lib/action_dispatch/http/parameters_filter.rb +++ /dev/null @@ -1,88 +0,0 @@ -require 'active_support/core_ext/hash/keys' - -module ActionDispatch - module Http - module ParametersFilter - INTERNAL_PARAMS = %w(controller action format _method only_path) - - @@filter_parameters = nil - @@filter_parameters_block = nil - - # Specify sensitive parameters which will be replaced from the request log. - # Filters parameters that have any of the arguments as a substring. - # Looks in all subhashes of the param hash for keys to filter. - # If a block is given, each key and value of the parameter hash and all - # subhashes is passed to it, the value or key - # can be replaced using String#replace or similar method. - # - # Examples: - # - # ActionDispatch::Http::ParametersFilter.filter_parameters :password - # => replaces the value to all keys matching /password/i with "[FILTERED]" - # - # ActionDispatch::Http::ParametersFilter.filter_parameters :foo, "bar" - # => replaces the value to all keys matching /foo|bar/i with "[FILTERED]" - # - # ActionDispatch::Http::ParametersFilter.filter_parameters do |k,v| - # v.reverse! if k =~ /secret/i - # end - # => reverses the value to all keys matching /secret/i - # - # ActionDispatch::Http::ParametersFilter.filter_parameters(:foo, "bar") do |k,v| - # v.reverse! if k =~ /secret/i - # end - # => reverses the value to all keys matching /secret/i, and - # replaces the value to all keys matching /foo|bar/i with "[FILTERED]" - def self.filter_parameters(*filter_words, &block) - raise "You must filter at least one word" if filter_words.empty? and !block_given? - - @@filter_parameters = filter_words.empty? ? nil : Regexp.new(filter_words.join('|'), true) - @@filter_parameters_block = block - end - - # Return a hash of parameters with all sensitive data replaced. - def filtered_parameters - @filtered_parameters ||= process_parameter_filter(parameters) - end - alias_method :fitered_params, :filtered_parameters - - # Return a hash of request.env with all sensitive data replaced. - def filtered_env - @env.merge(@env) do |key, value| - if (key =~ /RAW_POST_DATA/i) - '[FILTERED]' - else - process_parameter_filter({key => value}, false).values[0] - end - end - end - - protected - - def process_parameter_filter(original_parameters, validate_block = true) - if @@filter_parameters or @@filter_parameters_block - filtered_params = {} - - original_parameters.each do |key, value| - if key =~ @@filter_parameters - value = '[FILTERED]' - elsif value.is_a?(Hash) - value = process_parameter_filter(value) - elsif value.is_a?(Array) - value = value.map { |item| process_parameter_filter({key => item}, validate_block).values[0] } - elsif validate_block and @@filter_parameters_block - key = key.dup - value = value.dup if value.duplicable? - value = @@filter_parameters_block.call(key, value) || value - end - - filtered_params[key] = value - end - filtered_params.except!(*INTERNAL_PARAMS) - else - original_parameters.except(*INTERNAL_PARAMS) - end - end - end - end -end \ No newline at end of file diff --git a/actionpack/lib/action_dispatch/http/request.rb b/actionpack/lib/action_dispatch/http/request.rb index 7c3a228149..7a17023ed2 100755 --- a/actionpack/lib/action_dispatch/http/request.rb +++ b/actionpack/lib/action_dispatch/http/request.rb @@ -11,7 +11,7 @@ module ActionDispatch include ActionDispatch::Http::Cache::Request include ActionDispatch::Http::MimeNegotiation include ActionDispatch::Http::Parameters - include ActionDispatch::Http::ParametersFilter + include ActionDispatch::Http::FilterParameters include ActionDispatch::Http::Upload include ActionDispatch::Http::URL diff --git a/actionpack/test/controller/subscriber_test.rb b/actionpack/test/controller/subscriber_test.rb index 950eecaf6f..52434426f7 100644 --- a/actionpack/test/controller/subscriber_test.rb +++ b/actionpack/test/controller/subscriber_test.rb @@ -99,7 +99,8 @@ module ActionControllerSubscriberTest end def test_process_action_with_filter_parameters - Another::SubscribersController.filter_parameter_logging(:lifo, :amount) + ActionDispatch::Request.class_eval "alias :safe_process_parameter_filter :process_parameter_filter" + ActionDispatch::Request.filter_parameters(:lifo, :amount) get :show, :lifo => 'Pratik', :amount => '420', :step => '1' wait @@ -108,6 +109,8 @@ module ActionControllerSubscriberTest assert_match /"amount"=>"\[FILTERED\]"/, params assert_match /"lifo"=>"\[FILTERED\]"/, params assert_match /"step"=>"1"/, params + ensure + ActionDispatch::Request.class_eval "alias :process_parameter_filter :safe_process_parameter_filter" end def test_redirect_to diff --git a/actionpack/test/dispatch/request_test.rb b/actionpack/test/dispatch/request_test.rb index 0a6180959e..c5a75f5d21 100644 --- a/actionpack/test/dispatch/request_test.rb +++ b/actionpack/test/dispatch/request_test.rb @@ -453,16 +453,19 @@ class RequestTest < ActiveSupport::TestCase request.expects(:parameters).at_least_once.returns({}) assert_equal Mime::XML, request.negotiate_mime([Mime::XML, Mime::CSV]) end - - test "filter_parameters" do - request = stub_request - request.stubs(:request_parameters).returns({ "foo" => 1 }) - request.stubs(:query_parameters).returns({ "bar" => 2 }) - + + class FilterRequest < ActionDispatch::Request + end + + test "filter_parameters raises error without arguments" do assert_raises RuntimeError do - ActionDispatch::Http::ParametersFilter.filter_parameters + FilterRequest.filter_parameters end - + end + + test "process parameter filter" do + request = FilterRequest.new({}) + test_hashes = [ [{'foo'=>'bar'},{'foo'=>'bar'},%w'food'], [{'foo'=>'bar'},{'foo'=>'[FILTERED]'},%w'foo'], @@ -473,21 +476,47 @@ class RequestTest < ActiveSupport::TestCase [{'baz'=>[{'foo'=>'baz'}]}, {'baz'=>[{'foo'=>'[FILTERED]'}]}, %w(foo)]] test_hashes.each do |before_filter, after_filter, filter_words| - ActionDispatch::Http::ParametersFilter.filter_parameters(*filter_words) - assert_equal after_filter, request.__send__(:process_parameter_filter, before_filter) + FilterRequest.filter_parameters(*filter_words) + assert_equal after_filter, request.send(:process_parameter_filter, before_filter) filter_words.push('blah') - ActionDispatch::Http::ParametersFilter.filter_parameters(*filter_words) do |key, value| + + FilterRequest.filter_parameters(*filter_words) do |key, value| value.reverse! if key =~ /bargain/ end before_filter['barg'] = {'bargain'=>'gain', 'blah'=>'bar', 'bar'=>{'bargain'=>{'blah'=>'foo'}}} - after_filter['barg'] = {'bargain'=>'niag', 'blah'=>'[FILTERED]', 'bar'=>{'bargain'=>{'blah'=>'[FILTERED]'}}} + after_filter['barg'] = {'bargain'=>'niag', 'blah'=>'[FILTERED]', 'bar'=>{'bargain'=>{'blah'=>'[FILTERED]'}}} - assert_equal after_filter, request.__send__(:process_parameter_filter, before_filter) + assert_equal after_filter, request.send(:process_parameter_filter, before_filter) end end + test "filtered_parameters returns params filtered" do + FilterRequest.filter_parameters(:lifo, :amount) + + request = FilterRequest.new('action_dispatch.request.parameters' => + { 'lifo' => 'Pratik', 'amount' => '420', 'step' => '1' }) + + params = request.filtered_parameters + assert_equal "[FILTERED]", params["lifo"] + assert_equal "[FILTERED]", params["amount"] + assert_equal "1", params["step"] + end + + test "filtered_env filters env as a whole" do + FilterRequest.filter_parameters(:lifo, :amount) + + request = FilterRequest.new('action_dispatch.request.parameters' => + { 'amount' => '420', 'step' => '1' }, "RAW_POST_DATA" => "yada yada") + + request = FilterRequest.new(request.filtered_env) + + assert_equal "[FILTERED]", request.raw_post + assert_equal "[FILTERED]", request.params["amount"] + assert_equal "1", request.params["step"] + end + protected def stub_request(env={}) -- cgit v1.2.3 From dcb8b64975832ac75d92104da3c95876e56eec66 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Thu, 21 Jan 2010 12:12:10 +0100 Subject: Add shortcut test to abstract/render_test.rb --- actionpack/test/abstract/render_test.rb | 9 +++++++++ 1 file changed, 9 insertions(+) (limited to 'actionpack') diff --git a/actionpack/test/abstract/render_test.rb b/actionpack/test/abstract/render_test.rb index be0478b638..4bec44c9ae 100644 --- a/actionpack/test/abstract/render_test.rb +++ b/actionpack/test/abstract/render_test.rb @@ -33,6 +33,10 @@ module AbstractController render end + def shortcut + render "template" + end + def template_name render :_template_name => :template_name end @@ -73,6 +77,11 @@ module AbstractController assert_equal "With Default", @controller.response_body end + def test_render_template_through_shortcut + @controller.process(:shortcut) + assert_equal "With Template", @controller.response_body + end + def test_render_template_name @controller.process(:template_name) assert_equal "With Template Name", @controller.response_body -- cgit v1.2.3 From 378464a2e47bb849f3351cb8c87366554b7ce74d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Thu, 21 Jan 2010 13:05:30 +0100 Subject: Default to sync instrumentation. --- actionpack/lib/action_dispatch.rb | 1 - .../action_dispatch/middleware/notifications.rb | 32 ------ actionpack/lib/action_dispatch/railtie.rb | 3 - .../lib/action_dispatch/railties/subscriber.rb | 17 ---- .../test/activerecord/controller_runtime_test.rb | 19 +--- actionpack/test/controller/subscriber_test.rb | 18 +--- actionpack/test/dispatch/subscriber_test.rb | 111 --------------------- actionpack/test/template/subscriber_test.rb | 13 +-- 8 files changed, 9 insertions(+), 205 deletions(-) delete mode 100644 actionpack/lib/action_dispatch/middleware/notifications.rb delete mode 100644 actionpack/lib/action_dispatch/railties/subscriber.rb delete mode 100644 actionpack/test/dispatch/subscriber_test.rb (limited to 'actionpack') diff --git a/actionpack/lib/action_dispatch.rb b/actionpack/lib/action_dispatch.rb index 42ff9ca2e4..f0490a5619 100644 --- a/actionpack/lib/action_dispatch.rb +++ b/actionpack/lib/action_dispatch.rb @@ -46,7 +46,6 @@ module ActionDispatch autoload :Cookies autoload :Flash autoload :Head - autoload :Notifications autoload :ParamsParser autoload :Rescue autoload :ShowExceptions diff --git a/actionpack/lib/action_dispatch/middleware/notifications.rb b/actionpack/lib/action_dispatch/middleware/notifications.rb deleted file mode 100644 index 65409f57fd..0000000000 --- a/actionpack/lib/action_dispatch/middleware/notifications.rb +++ /dev/null @@ -1,32 +0,0 @@ -module ActionDispatch - # Provide notifications in the middleware stack. Notice that for the before_dispatch - # and after_dispatch notifications, we just send the original env, so we don't pile - # up large env hashes in the queue. However, in exception cases, the whole env hash - # is actually useful, so we send it all. - class Notifications - def initialize(app) - @app = app - end - - def call(env) - request = Request.new(env) - payload = retrieve_payload_from_env(request.filtered_env) - - ActiveSupport::Notifications.instrument("action_dispatch.before_dispatch", payload) - - ActiveSupport::Notifications.instrument!("action_dispatch.after_dispatch", payload) do - @app.call(env) - end - rescue Exception => exception - ActiveSupport::Notifications.instrument('action_dispatch.exception', - :env => env, :exception => exception) - raise exception - end - - protected - # Remove any rack related constants from the env, like rack.input. - def retrieve_payload_from_env(env) - Hash[:env => env.except(*env.keys.select { |k| k.to_s.index("rack.") == 0 })] - end - end -end diff --git a/actionpack/lib/action_dispatch/railtie.rb b/actionpack/lib/action_dispatch/railtie.rb index 18978bfb39..e4bd143e78 100644 --- a/actionpack/lib/action_dispatch/railtie.rb +++ b/actionpack/lib/action_dispatch/railtie.rb @@ -5,9 +5,6 @@ module ActionDispatch class Railtie < Rails::Railtie plugin_name :action_dispatch - require "action_dispatch/railties/subscriber" - subscriber ActionDispatch::Railties::Subscriber.new - # Prepare dispatcher callbacks and run 'prepare' callbacks initializer "action_dispatch.prepare_dispatcher" do |app| # TODO: This used to say unless defined?(Dispatcher). Find out why and fix. diff --git a/actionpack/lib/action_dispatch/railties/subscriber.rb b/actionpack/lib/action_dispatch/railties/subscriber.rb deleted file mode 100644 index cdb1162eac..0000000000 --- a/actionpack/lib/action_dispatch/railties/subscriber.rb +++ /dev/null @@ -1,17 +0,0 @@ -module ActionDispatch - module Railties - class Subscriber < Rails::Subscriber - def before_dispatch(event) - request = Request.new(event.payload[:env]) - path = request.request_uri.inspect rescue "unknown" - - info "\n\nStarted #{request.method.to_s.upcase} #{path} " << - "for #{request.remote_ip} at #{event.time.to_s(:db)}" - end - - def logger - ActionController::Base.logger - end - end - end -end \ No newline at end of file diff --git a/actionpack/test/activerecord/controller_runtime_test.rb b/actionpack/test/activerecord/controller_runtime_test.rb index 37c7738301..ed8e324938 100644 --- a/actionpack/test/activerecord/controller_runtime_test.rb +++ b/actionpack/test/activerecord/controller_runtime_test.rb @@ -6,16 +6,15 @@ require 'action_controller/railties/subscriber' ActionController::Base.send :include, ActiveRecord::Railties::ControllerRuntime -module ControllerRuntimeSubscriberTest +class ControllerRuntimeSubscriberTest < ActionController::TestCase class SubscriberController < ActionController::Base def show render :inline => "<%= Project.all %>" end end - - def self.included(base) - base.tests SubscriberController - end + + include Rails::Subscriber::TestHelper + tests SubscriberController def setup @old_logger = ActionController::Base.logger @@ -40,14 +39,4 @@ module ControllerRuntimeSubscriberTest assert_equal 2, @logger.logged(:info).size assert_match /\(Views: [\d\.]+ms | ActiveRecord: [\d\.]+ms\)/, @logger.logged(:info)[1] end - - class SyncSubscriberTest < ActionController::TestCase - include Rails::Subscriber::SyncTestHelper - include ControllerRuntimeSubscriberTest - end - - class AsyncSubscriberTest < ActionController::TestCase - include Rails::Subscriber::AsyncTestHelper - include ControllerRuntimeSubscriberTest - end end \ No newline at end of file diff --git a/actionpack/test/controller/subscriber_test.rb b/actionpack/test/controller/subscriber_test.rb index 52434426f7..bd0c83413c 100644 --- a/actionpack/test/controller/subscriber_test.rb +++ b/actionpack/test/controller/subscriber_test.rb @@ -35,11 +35,9 @@ module Another end end -module ActionControllerSubscriberTest - - def self.included(base) - base.tests Another::SubscribersController - end +class ACSubscriberTest < ActionController::TestCase + tests Another::SubscribersController + include Rails::Subscriber::TestHelper def setup @old_logger = ActionController::Base.logger @@ -174,14 +172,4 @@ module ActionControllerSubscriberTest def logs @logs ||= @logger.logged(:info) end - - class SyncSubscriberTest < ActionController::TestCase - include Rails::Subscriber::SyncTestHelper - include ActionControllerSubscriberTest - end - - class AsyncSubscriberTest < ActionController::TestCase - include Rails::Subscriber::AsyncTestHelper - include ActionControllerSubscriberTest - end end diff --git a/actionpack/test/dispatch/subscriber_test.rb b/actionpack/test/dispatch/subscriber_test.rb deleted file mode 100644 index 3096c132e7..0000000000 --- a/actionpack/test/dispatch/subscriber_test.rb +++ /dev/null @@ -1,111 +0,0 @@ -require "abstract_unit" -require "rails/subscriber/test_helper" -require "action_dispatch/railties/subscriber" - -module DispatcherSubscriberTest - Boomer = lambda do |env| - req = ActionDispatch::Request.new(env) - case req.path - when "/" - [200, {}, []] - else - raise "puke!" - end - end - - App = ActionDispatch::Notifications.new(Boomer) - - def setup - Rails::Subscriber.add(:action_dispatch, ActionDispatch::Railties::Subscriber.new) - @app = App - super - - @events = [] - ActiveSupport::Notifications.subscribe do |*args| - @events << args - end - end - - def set_logger(logger) - ActionController::Base.logger = logger - end - - def test_publishes_notifications - get "/" - wait - - assert_equal 2, @events.size - before, after = @events - - assert_equal 'action_dispatch.before_dispatch', before[0] - assert_kind_of Hash, before[4][:env] - assert_equal 'GET', before[4][:env]["REQUEST_METHOD"] - - assert_equal 'action_dispatch.after_dispatch', after[0] - assert_kind_of Hash, after[4][:env] - assert_equal 'GET', after[4][:env]["REQUEST_METHOD"] - end - - def test_publishes_notifications_even_on_failures - begin - get "/puke" - rescue - end - - wait - - assert_equal 3, @events.size - before, after, exception = @events - - assert_equal 'action_dispatch.before_dispatch', before[0] - assert_kind_of Hash, before[4][:env] - assert_equal 'GET', before[4][:env]["REQUEST_METHOD"] - - assert_equal 'action_dispatch.after_dispatch', after[0] - assert_kind_of Hash, after[4][:env] - assert_equal 'GET', after[4][:env]["REQUEST_METHOD"] - - assert_equal 'action_dispatch.exception', exception[0] - assert_kind_of Hash, exception[4][:env] - assert_equal 'GET', exception[4][:env]["REQUEST_METHOD"] - assert_kind_of RuntimeError, exception[4][:exception] - end - - def test_subscriber_logs_notifications - get "/" - wait - - log = @logger.logged(:info).first - assert_equal 1, @logger.logged(:info).size - - assert_match %r{^Started GET "/"}, log - assert_match %r{for 127\.0\.0\.1}, log - end - - def test_subscriber_has_its_logged_flushed_after_request - assert_equal 0, @logger.flush_count - get "/" - wait - assert_equal 1, @logger.flush_count - end - - def test_subscriber_has_its_logged_flushed_even_after_busted_requests - assert_equal 0, @logger.flush_count - begin - get "/puke" - rescue - end - wait - assert_equal 1, @logger.flush_count - end - - class SyncSubscriberTest < ActionController::IntegrationTest - include Rails::Subscriber::SyncTestHelper - include DispatcherSubscriberTest - end - - class AsyncSubscriberTest < ActionController::IntegrationTest - include Rails::Subscriber::AsyncTestHelper - include DispatcherSubscriberTest - end -end \ No newline at end of file diff --git a/actionpack/test/template/subscriber_test.rb b/actionpack/test/template/subscriber_test.rb index af0b3102cf..5db2b16ac1 100644 --- a/actionpack/test/template/subscriber_test.rb +++ b/actionpack/test/template/subscriber_test.rb @@ -3,7 +3,8 @@ require "rails/subscriber/test_helper" require "action_view/railties/subscriber" require "controller/fake_models" -module ActionViewSubscriberTest +class AVSubscriberTest < ActiveSupport::TestCase + include Rails::Subscriber::TestHelper def setup @old_logger = ActionController::Base.logger @@ -89,14 +90,4 @@ module ActionViewSubscriberTest assert_equal 1, @logger.logged(:info).size assert_match /Rendered collection/, @logger.logged(:info).last end - - class SyncSubscriberTest < ActiveSupport::TestCase - include Rails::Subscriber::SyncTestHelper - include ActionViewSubscriberTest - end - - class AsyncSubscriberTest < ActiveSupport::TestCase - include Rails::Subscriber::AsyncTestHelper - include ActionViewSubscriberTest - end end \ No newline at end of file -- cgit v1.2.3 From fc4f237864541f5012f9b8cc8e0ec81960377e55 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Thu, 21 Jan 2010 16:50:11 +0100 Subject: Make filter parameters based on request, so they can be modified for anything in the middleware stack. --- actionpack/lib/action_controller/base.rb | 6 +- .../lib/action_controller/railties/subscriber.rb | 6 +- .../lib/action_dispatch/http/filter_parameters.rb | 128 +++++++++++---------- actionpack/test/controller/base_test.rb | 21 +++- actionpack/test/controller/subscriber_test.rb | 5 +- actionpack/test/dispatch/request_test.rb | 39 +++---- 6 files changed, 109 insertions(+), 96 deletions(-) (limited to 'actionpack') diff --git a/actionpack/lib/action_controller/base.rb b/actionpack/lib/action_controller/base.rb index cf66e99fa5..a66aafd80e 100644 --- a/actionpack/lib/action_controller/base.rb +++ b/actionpack/lib/action_controller/base.rb @@ -74,8 +74,12 @@ module ActionController end # This method has been moved to ActionDispatch::Request.filter_parameters - def self.filter_parameter_logging(*) + def self.filter_parameter_logging(*args, &block) ActiveSupport::Deprecation.warn("Setting filter_parameter_logging in ActionController is deprecated and has no longer effect, please set 'config.filter_parameters' in config/application.rb instead", caller) + filter = Rails.application.config.filter_parameters + filter.concat(args) + filter << block if block + filter end def _normalize_options(action=nil, options={}, &blk) diff --git a/actionpack/lib/action_controller/railties/subscriber.rb b/actionpack/lib/action_controller/railties/subscriber.rb index d257d6ac2c..1f0e6bf51a 100644 --- a/actionpack/lib/action_controller/railties/subscriber.rb +++ b/actionpack/lib/action_controller/railties/subscriber.rb @@ -1,10 +1,14 @@ module ActionController module Railties class Subscriber < Rails::Subscriber + INTERNAL_PARAMS = %w(controller action format _method only_path) + def start_processing(event) payload = event.payload + params = payload[:params].except(*INTERNAL_PARAMS) + info " Processing by #{payload[:controller]}##{payload[:action]} as #{payload[:formats].first.to_s.upcase}" - info " Parameters: #{payload[:params].inspect}" unless payload[:params].blank? + info " Parameters: #{params.inspect}" unless params.empty? end def process_action(event) diff --git a/actionpack/lib/action_dispatch/http/filter_parameters.rb b/actionpack/lib/action_dispatch/http/filter_parameters.rb index 0f4afb01d9..1958e1668d 100644 --- a/actionpack/lib/action_dispatch/http/filter_parameters.rb +++ b/actionpack/lib/action_dispatch/http/filter_parameters.rb @@ -1,69 +1,30 @@ +require 'active_support/core_ext/object/blank' require 'active_support/core_ext/hash/keys' module ActionDispatch module Http + # Allows you to specify sensitive parameters which will be replaced from + # the request log by looking in all subhashes of the param hash for keys + # to filter. If a block is given, each key and value of the parameter + # hash and all subhashes is passed to it, the value or key can be replaced + # using String#replace or similar method. + # + # Examples: + # + # env["action_dispatch.parameter_filter"] = [:password] + # => replaces the value to all keys matching /password/i with "[FILTERED]" + # + # env["action_dispatch.parameter_filter"] = [:foo, "bar"] + # => replaces the value to all keys matching /foo|bar/i with "[FILTERED]" + # + # env["action_dispatch.parameter_filter"] = lambda do |k,v| + # v.reverse! if k =~ /secret/i + # end + # => reverses the value to all keys matching /secret/i + # module FilterParameters extend ActiveSupport::Concern - INTERNAL_PARAMS = %w(controller action format _method only_path) - - module ClassMethods - # Specify sensitive parameters which will be replaced from the request log. - # Filters parameters that have any of the arguments as a substring. - # Looks in all subhashes of the param hash for keys to filter. - # If a block is given, each key and value of the parameter hash and all - # subhashes is passed to it, the value or key - # can be replaced using String#replace or similar method. - # - # Examples: - # - # ActionDispatch::Request.filter_parameters :password - # => replaces the value to all keys matching /password/i with "[FILTERED]" - # - # ActionDispatch::Request.filter_parameters :foo, "bar" - # => replaces the value to all keys matching /foo|bar/i with "[FILTERED]" - # - # ActionDispatch::Request.filter_parameters do |k,v| - # v.reverse! if k =~ /secret/i - # end - # => reverses the value to all keys matching /secret/i - # - # ActionDispatch::Request.filter_parameters(:foo, "bar") do |k,v| - # v.reverse! if k =~ /secret/i - # end - # => reverses the value to all keys matching /secret/i, and - # replaces the value to all keys matching /foo|bar/i with "[FILTERED]" - def filter_parameters(*filter_words, &block) - raise "You must filter at least one word" if filter_words.empty? - - parameter_filter = Regexp.new(filter_words.join('|'), true) - - define_method(:process_parameter_filter) do |original_params| - filtered_params = {} - - original_params.each do |key, value| - if key =~ parameter_filter - value = '[FILTERED]' - elsif value.is_a?(Hash) - value = process_parameter_filter(value) - elsif value.is_a?(Array) - value = value.map { |i| process_parameter_filter(i) } - elsif block_given? - key = key.dup - value = value.dup if value.duplicable? - yield key, value - end - - filtered_params[key] = value - end - - filtered_params.except!(*INTERNAL_PARAMS) - end - - protected :process_parameter_filter - end - end - # Return a hash of parameters with all sensitive data replaced. def filtered_parameters @filtered_parameters ||= process_parameter_filter(parameters) @@ -71,7 +32,6 @@ module ActionDispatch alias :fitered_params :filtered_parameters # Return a hash of request.env with all sensitive data replaced. - # TODO Josh should white list env to remove stuff like rack.input and rack.errors def filtered_env filtered_env = @env.dup filtered_env.each do |key, value| @@ -86,8 +46,52 @@ module ActionDispatch protected - def process_parameter_filter(original_parameters) - original_parameters.except(*INTERNAL_PARAMS) + def compile_parameter_filter #:nodoc: + strings, regexps, blocks = [], [], [] + + Array(@env["action_dispatch.parameter_filter"]).each do |item| + case item + when NilClass + when Proc + blocks << item + when Regexp + regexps << item + else + strings << item.to_s + end + end + + regexps << Regexp.new(strings.join('|'), true) unless strings.empty? + [regexps, blocks] + end + + def filtering_parameters? #:nodoc: + @env["action_dispatch.parameter_filter"].present? + end + + def process_parameter_filter(original_params) #:nodoc: + return original_params.dup unless filtering_parameters? + + filtered_params = {} + regexps, blocks = compile_parameter_filter + + original_params.each do |key, value| + if regexps.find { |r| key =~ r } + value = '[FILTERED]' + elsif value.is_a?(Hash) + value = process_parameter_filter(value) + elsif value.is_a?(Array) + value = value.map { |i| process_parameter_filter(i) } + elsif blocks.present? + key = key.dup + value = value.dup if value.duplicable? + blocks.each { |b| b.call(key, value) } + end + + filtered_params[key] = value + end + + filtered_params end end end diff --git a/actionpack/test/controller/base_test.rb b/actionpack/test/controller/base_test.rb index 1510a6a7e0..4fcfbacf4e 100644 --- a/actionpack/test/controller/base_test.rb +++ b/actionpack/test/controller/base_test.rb @@ -2,6 +2,9 @@ require 'abstract_unit' require 'logger' require 'pp' # require 'pp' early to prevent hidden_methods from not picking up the pretty-print methods until too late +module Rails +end + # Provide some controller to run the tests on. module Submodule class ContainedEmptyController < ActionController::Base @@ -63,7 +66,7 @@ class DefaultUrlOptionsController < ActionController::Base end end -class ControllerClassTests < Test::Unit::TestCase +class ControllerClassTests < ActiveSupport::TestCase def test_controller_path assert_equal 'empty', EmptyController.controller_path assert_equal EmptyController.controller_path, EmptyController.new.controller_path @@ -74,7 +77,21 @@ class ControllerClassTests < Test::Unit::TestCase def test_controller_name assert_equal 'empty', EmptyController.controller_name assert_equal 'contained_empty', Submodule::ContainedEmptyController.controller_name - end + end + + def test_filter_parameter_logging + parameters = [] + config = mock(:config => mock(:filter_parameters => parameters)) + Rails.expects(:application).returns(config) + + assert_deprecated do + Class.new(ActionController::Base) do + filter_parameter_logging :password + end + end + + assert_equal [:password], parameters + end end class ControllerInstanceTests < Test::Unit::TestCase diff --git a/actionpack/test/controller/subscriber_test.rb b/actionpack/test/controller/subscriber_test.rb index bd0c83413c..152a0d0c04 100644 --- a/actionpack/test/controller/subscriber_test.rb +++ b/actionpack/test/controller/subscriber_test.rb @@ -97,8 +97,7 @@ class ACSubscriberTest < ActionController::TestCase end def test_process_action_with_filter_parameters - ActionDispatch::Request.class_eval "alias :safe_process_parameter_filter :process_parameter_filter" - ActionDispatch::Request.filter_parameters(:lifo, :amount) + @request.env["action_dispatch.parameter_filter"] = [:lifo, :amount] get :show, :lifo => 'Pratik', :amount => '420', :step => '1' wait @@ -107,8 +106,6 @@ class ACSubscriberTest < ActionController::TestCase assert_match /"amount"=>"\[FILTERED\]"/, params assert_match /"lifo"=>"\[FILTERED\]"/, params assert_match /"step"=>"1"/, params - ensure - ActionDispatch::Request.class_eval "alias :process_parameter_filter :safe_process_parameter_filter" end def test_redirect_to diff --git a/actionpack/test/dispatch/request_test.rb b/actionpack/test/dispatch/request_test.rb index c5a75f5d21..2b5c19361a 100644 --- a/actionpack/test/dispatch/request_test.rb +++ b/actionpack/test/dispatch/request_test.rb @@ -454,18 +454,7 @@ class RequestTest < ActiveSupport::TestCase assert_equal Mime::XML, request.negotiate_mime([Mime::XML, Mime::CSV]) end - class FilterRequest < ActionDispatch::Request - end - - test "filter_parameters raises error without arguments" do - assert_raises RuntimeError do - FilterRequest.filter_parameters - end - end - test "process parameter filter" do - request = FilterRequest.new({}) - test_hashes = [ [{'foo'=>'bar'},{'foo'=>'bar'},%w'food'], [{'foo'=>'bar'},{'foo'=>'[FILTERED]'},%w'foo'], @@ -473,18 +462,18 @@ class RequestTest < ActiveSupport::TestCase [{'foo'=>'bar', 'baz'=>'foo'},{'foo'=>'[FILTERED]', 'baz'=>'[FILTERED]'},%w'foo baz'], [{'bar'=>{'foo'=>'bar','bar'=>'foo'}},{'bar'=>{'foo'=>'[FILTERED]','bar'=>'foo'}},%w'fo'], [{'foo'=>{'foo'=>'bar','bar'=>'foo'}},{'foo'=>'[FILTERED]'},%w'f banana'], - [{'baz'=>[{'foo'=>'baz'}]}, {'baz'=>[{'foo'=>'[FILTERED]'}]}, %w(foo)]] + [{'baz'=>[{'foo'=>'baz'}]}, {'baz'=>[{'foo'=>'[FILTERED]'}]}, [/foo/]]] test_hashes.each do |before_filter, after_filter, filter_words| - FilterRequest.filter_parameters(*filter_words) + request = stub_request('action_dispatch.parameter_filter' => filter_words) assert_equal after_filter, request.send(:process_parameter_filter, before_filter) - filter_words.push('blah') - - FilterRequest.filter_parameters(*filter_words) do |key, value| + filter_words << 'blah' + filter_words << lambda { |key, value| value.reverse! if key =~ /bargain/ - end + } + request = stub_request('action_dispatch.parameter_filter' => filter_words) before_filter['barg'] = {'bargain'=>'gain', 'blah'=>'bar', 'bar'=>{'bargain'=>{'blah'=>'foo'}}} after_filter['barg'] = {'bargain'=>'niag', 'blah'=>'[FILTERED]', 'bar'=>{'bargain'=>{'blah'=>'[FILTERED]'}}} @@ -493,10 +482,9 @@ class RequestTest < ActiveSupport::TestCase end test "filtered_parameters returns params filtered" do - FilterRequest.filter_parameters(:lifo, :amount) - - request = FilterRequest.new('action_dispatch.request.parameters' => - { 'lifo' => 'Pratik', 'amount' => '420', 'step' => '1' }) + request = stub_request('action_dispatch.request.parameters' => + { 'lifo' => 'Pratik', 'amount' => '420', 'step' => '1' }, + 'action_dispatch.parameter_filter' => [:lifo, :amount]) params = request.filtered_parameters assert_equal "[FILTERED]", params["lifo"] @@ -505,12 +493,11 @@ class RequestTest < ActiveSupport::TestCase end test "filtered_env filters env as a whole" do - FilterRequest.filter_parameters(:lifo, :amount) - - request = FilterRequest.new('action_dispatch.request.parameters' => - { 'amount' => '420', 'step' => '1' }, "RAW_POST_DATA" => "yada yada") + request = stub_request('action_dispatch.request.parameters' => + { 'amount' => '420', 'step' => '1' }, "RAW_POST_DATA" => "yada yada", + 'action_dispatch.parameter_filter' => [:lifo, :amount]) - request = FilterRequest.new(request.filtered_env) + request = stub_request(request.filtered_env) assert_equal "[FILTERED]", request.raw_post assert_equal "[FILTERED]", request.params["amount"] -- cgit v1.2.3 From 1a750da130223867195503d284cf4c73a345eee5 Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Fri, 22 Jan 2010 10:18:19 -0600 Subject: Make @controller an internal ivar in the view --- actionpack/lib/action_view/base.rb | 14 ++++++-------- actionpack/lib/action_view/helpers/asset_tag_helper.rb | 8 ++++---- actionpack/lib/action_view/helpers/cache_helper.rb | 2 +- actionpack/lib/action_view/helpers/url_helper.rb | 12 ++++++------ 4 files changed, 17 insertions(+), 19 deletions(-) (limited to 'actionpack') diff --git a/actionpack/lib/action_view/base.rb b/actionpack/lib/action_view/base.rb index 4970c768e8..c4b0455c2a 100644 --- a/actionpack/lib/action_view/base.rb +++ b/actionpack/lib/action_view/base.rb @@ -181,7 +181,6 @@ module ActionView #:nodoc: extend ActiveSupport::Memoizable attr_accessor :base_path, :assigns, :template_extension, :formats - attr_accessor :controller attr_internal :captures def reset_formats(formats) @@ -277,13 +276,13 @@ module ActionView #:nodoc: @config = nil @formats = formats @assigns = assigns_for_first_render.each { |key, value| instance_variable_set("@#{key}", value) } - @controller = controller + @_controller = controller @helpers = self.class.helpers || Module.new @_content_for = Hash.new {|h,k| h[k] = ActionView::SafeBuffer.new } self.view_paths = view_paths end - attr_internal :template + attr_internal :controller, :template attr_reader :view_paths def view_paths=(paths) @@ -298,12 +297,11 @@ module ActionView #:nodoc: # Evaluates the local assigns and controller ivars, pushes them to the view. def _evaluate_assigns_and_ivars #:nodoc: - if @controller - variables = @controller.instance_variable_names - variables -= @controller.protected_instance_variables if @controller.respond_to?(:protected_instance_variables) - variables.each { |name| instance_variable_set(name, @controller.instance_variable_get(name)) } + if controller + variables = controller.instance_variable_names + variables -= controller.protected_instance_variables if controller.respond_to?(:protected_instance_variables) + variables.each { |name| instance_variable_set(name, controller.instance_variable_get(name)) } end end - end end diff --git a/actionpack/lib/action_view/helpers/asset_tag_helper.rb b/actionpack/lib/action_view/helpers/asset_tag_helper.rb index 15b70ecff5..83357dd76f 100644 --- a/actionpack/lib/action_view/helpers/asset_tag_helper.rb +++ b/actionpack/lib/action_view/helpers/asset_tag_helper.rb @@ -634,8 +634,8 @@ module ActionView # Prefix with /dir/ if lacking a leading +/+. Account for relative URL # roots. Rewrite the asset path for cache-busting asset ids. Include # asset host, if configured, with the correct request protocol. - def compute_public_path(source, dir, ext = nil, include_host = true) - has_request = @controller.respond_to?(:request) + def compute_public_path(source, dir, ext = nil, include_host = true) + has_request = controller.respond_to?(:request) source_ext = File.extname(source)[1..-1] if ext && !is_uri?(source) && (source_ext.blank? || (ext != source_ext && File.exist?(File.join(config.assets_dir, dir, "#{source}.#{ext}")))) @@ -658,7 +658,7 @@ module ActionView host = compute_asset_host(source) if has_request && !host.blank? && !is_uri?(host) - host = "#{@controller.request.protocol}#{host}" + host = "#{controller.request.protocol}#{host}" end "#{host}#{source}" @@ -681,7 +681,7 @@ module ActionView if host.is_a?(Proc) || host.respond_to?(:call) case host.is_a?(Proc) ? host.arity : host.method(:call).arity when 2 - request = @controller.respond_to?(:request) && @controller.request + request = controller.respond_to?(:request) && controller.request host.call(source, request) else host.call(source) diff --git a/actionpack/lib/action_view/helpers/cache_helper.rb b/actionpack/lib/action_view/helpers/cache_helper.rb index 64d1ad2715..d5cc14b29a 100644 --- a/actionpack/lib/action_view/helpers/cache_helper.rb +++ b/actionpack/lib/action_view/helpers/cache_helper.rb @@ -32,7 +32,7 @@ module ActionView # Topics listed alphabetically # <% end %> def cache(name = {}, options = nil, &block) - @controller.fragment_for(output_buffer, name, options, &block) + controller.fragment_for(output_buffer, name, options, &block) end end end diff --git a/actionpack/lib/action_view/helpers/url_helper.rb b/actionpack/lib/action_view/helpers/url_helper.rb index 14628c5404..511386fede 100644 --- a/actionpack/lib/action_view/helpers/url_helper.rb +++ b/actionpack/lib/action_view/helpers/url_helper.rb @@ -13,7 +13,7 @@ module ActionView # Need to map default url options to controller one. def default_url_options(*args) #:nodoc: - @controller.send(:default_url_options, *args) + controller.send(:default_url_options, *args) end # Returns the URL for the set of +options+ provided. This takes the @@ -89,10 +89,10 @@ module ActionView when Hash options = { :only_path => options[:host].nil? }.update(options.symbolize_keys) escape = options.key?(:escape) ? options.delete(:escape) : false - @controller.send(:url_for, options) + controller.send(:url_for, options) when :back escape = false - @controller.request.env["HTTP_REFERER"] || 'javascript:history.back()' + controller.request.env["HTTP_REFERER"] || 'javascript:history.back()' else escape = false polymorphic_path(options) @@ -546,10 +546,10 @@ module ActionView # # => false def current_page?(options) url_string = CGI.unescapeHTML(url_for(options)) - request = @controller.request - # We ignore any extra parameters in the request_uri if the + request = controller.request + # We ignore any extra parameters in the request_uri if the # submitted url doesn't have any either. This lets the function - # work with things like ?order=asc + # work with things like ?order=asc if url_string.index("?") request_uri = request.request_uri else -- cgit v1.2.3 From 5c53ffe1dbb3ece45ed58ed5b005e9c104f61842 Mon Sep 17 00:00:00 2001 From: Xavier Noria Date: Fri, 22 Jan 2010 16:46:45 +0100 Subject: Add missing require for Object#try [#3771 state:resolved] Signed-off-by: Pratik Naik --- actionpack/lib/action_view/render/rendering.rb | 2 ++ 1 file changed, 2 insertions(+) (limited to 'actionpack') diff --git a/actionpack/lib/action_view/render/rendering.rb b/actionpack/lib/action_view/render/rendering.rb index ec278ca783..7c33f1334a 100644 --- a/actionpack/lib/action_view/render/rendering.rb +++ b/actionpack/lib/action_view/render/rendering.rb @@ -1,3 +1,5 @@ +require 'active_support/core_ext/object/try' + module ActionView module Rendering # Returns the result of a render that's dictated by the options hash. The primary options are: -- cgit v1.2.3 From d618b7e3dcf2fe6040f025c02bf29691aefc8a71 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Fri, 22 Jan 2010 17:57:36 +0100 Subject: Ensure strings given to render with slash are rendered relative to the configured _prefix. --- actionpack/lib/abstract_controller/rendering.rb | 37 +++++++++++++++------- actionpack/lib/action_controller/base.rb | 23 -------------- .../lib/action_controller/metal/rendering.rb | 36 ++++++++++++++------- actionpack/test/abstract/render_test.rb | 35 +++++++++++++++++--- 4 files changed, 79 insertions(+), 52 deletions(-) (limited to 'actionpack') diff --git a/actionpack/lib/abstract_controller/rendering.rb b/actionpack/lib/abstract_controller/rendering.rb index 826e82c8c6..a168b1b4c5 100644 --- a/actionpack/lib/abstract_controller/rendering.rb +++ b/actionpack/lib/abstract_controller/rendering.rb @@ -98,18 +98,9 @@ module AbstractController _view_paths end - # Normalize options, by converting render "foo" to render :template => "foo" - # and render "/foo" to render :file => "/foo". - def _normalize_options(action=nil, options={}) - case action - when Hash - options, action = action, nil - when String - key = (action.index("/") == 0 ? :file : :template) - options.merge!(key => action) - end - - options + # The prefix used in render "foo" shortcuts. + def _prefix + controller_path end # Return a string representation of a Rack-compatible response body. @@ -126,6 +117,28 @@ module AbstractController private + # Normalize options, by converting render "foo" to render :template => "prefix/foo" + # and render "/foo" to render :file => "/foo". + def _normalize_options(action=nil, options={}) + case action + when Hash + options, action = action, nil + when String, Symbol + action = action.to_s + case action.index("/") + when NilClass + options[:_prefix] = _prefix + options[:_template_name] = action + when 0 + options[:file] = action + else + options[:template] = action + end + end + + options + end + # Take in a set of options and determine the template to render # # ==== Options diff --git a/actionpack/lib/action_controller/base.rb b/actionpack/lib/action_controller/base.rb index a66aafd80e..f46231d72b 100644 --- a/actionpack/lib/action_controller/base.rb +++ b/actionpack/lib/action_controller/base.rb @@ -81,28 +81,5 @@ module ActionController filter << block if block filter end - - def _normalize_options(action=nil, options={}, &blk) - case action - when NilClass - when Hash, String - options = super - when Symbol - options.merge! :action => action - else - options.merge! :partial => action - end - - if options.key?(:action) && options[:action].to_s.index("/") - options[:template] = options.delete(:action) - end - - if options[:status] - options[:status] = Rack::Utils.status_code(options[:status]) - end - - options[:update] = blk if block_given? - options - end end end diff --git a/actionpack/lib/action_controller/metal/rendering.rb b/actionpack/lib/action_controller/metal/rendering.rb index 475ed54167..72e2bbd00e 100644 --- a/actionpack/lib/action_controller/metal/rendering.rb +++ b/actionpack/lib/action_controller/metal/rendering.rb @@ -25,18 +25,6 @@ module ActionController end private - def _prefix - controller_path - end - - def _determine_template(options) - if (options.keys & [:partial, :file, :template, :text, :inline]).empty? - options[:_template_name] ||= options[:action] - options[:_prefix] = _prefix - end - - super - end def _render_partial(options) options[:partial] = action_name if options[:partial] == true @@ -54,5 +42,29 @@ module ActionController self.content_type = content_type if content_type self.headers["Location"] = url_for(location) if location end + + def _normalize_options(action=nil, options={}, &blk) + case action + when NilClass + when Hash + options = super(action.delete(:action), action) + when String, Symbol + options = super + else + options.merge! :partial => action + end + + if (options.keys & [:partial, :file, :template, :text, :inline]).empty? + options[:_template_name] ||= options[:action] + options[:_prefix] = _prefix + end + + if options[:status] + options[:status] = Rack::Utils.status_code(options[:status]) + end + + options[:update] = blk if block_given? + options + end end end diff --git a/actionpack/test/abstract/render_test.rb b/actionpack/test/abstract/render_test.rb index 4bec44c9ae..ffd430fa86 100644 --- a/actionpack/test/abstract/render_test.rb +++ b/actionpack/test/abstract/render_test.rb @@ -6,9 +6,16 @@ module AbstractController class ControllerRenderer < AbstractController::Base include AbstractController::Rendering + def _prefix + "renderer" + end + self.view_paths = [ActionView::FixtureResolver.new( "default.erb" => "With Default", "template.erb" => "With Template", + "renderer/string.erb" => "With String", + "renderer/symbol.erb" => "With Symbol", + "string/with_path.erb" => "With String With Path", "some/file.erb" => "With File", "template_name.erb" => "With Template Name" )] @@ -33,8 +40,16 @@ module AbstractController render end - def shortcut - render "template" + def string + render "string" + end + + def string_with_path + render "string/with_path" + end + + def symbol + render :symbol end def template_name @@ -77,9 +92,19 @@ module AbstractController assert_equal "With Default", @controller.response_body end - def test_render_template_through_shortcut - @controller.process(:shortcut) - assert_equal "With Template", @controller.response_body + def test_render_string + @controller.process(:string) + assert_equal "With String", @controller.response_body + end + + def test_render_symbol + @controller.process(:symbol) + assert_equal "With Symbol", @controller.response_body + end + + def test_render_string_with_path + @controller.process(:string_with_path) + assert_equal "With String With Path", @controller.response_body end def test_render_template_name -- cgit v1.2.3