From bec5356f254dad1423bfdc17897589252aae5f44 Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Tue, 19 Jan 2010 08:20:52 -0600 Subject: [PATCH 01/29] Define named routes for other non-GET REST actions --- .../lib/action_dispatch/routing/mapper.rb | 36 ++++++++++++++++--- 1 file changed, 32 insertions(+), 4 deletions(-) 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 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: [PATCH 02/29] Do not send rack.input or any other rack information to AD listeners. --- .../middleware/notifications.rb | 19 +++++++++++++------ .../action_dispatch/railties/subscriber.rb | 4 ++-- actionpack/test/dispatch/subscriber_test.rb | 5 ++--- 3 files changed, 17 insertions(+), 11 deletions(-) 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 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: [PATCH 03/29] Fix failing test. --- actionpack/test/controller/new_base/base_test.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 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: [PATCH 04/29] Move parameters to the top on logging. --- actionpack/lib/action_controller/base.rb | 2 +- .../metal/filter_parameter_logging.rb | 5 -- .../metal/instrumentation.rb | 20 ++++--- .../action_controller/railties/subscriber.rb | 8 ++- .../activerecord/controller_runtime_test.rb | 4 +- actionpack/test/controller/subscriber_test.rb | 54 ++++++++++--------- 6 files changed, 52 insertions(+), 41 deletions(-) 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 From 88ffba232987d00717560eaac7ab4f1104e054cd Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Tue, 19 Jan 2010 09:05:34 -0600 Subject: [PATCH 05/29] Disable ShowExceptions during integration tests --- .../action_dispatch/testing/integration.rb | 4 +++- .../request/json_params_parsing_test.rb | 2 +- .../request/xml_params_parsing_test.rb | 2 +- .../test/dispatch/show_exceptions_test.rb | 22 +++++++++---------- 4 files changed, 16 insertions(+), 14 deletions(-) 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 From 73b179eb689611ac0518584b21f2304756a7e981 Mon Sep 17 00:00:00 2001 From: Pratik Naik Date: Tue, 19 Jan 2010 20:27:14 +0530 Subject: [PATCH 06/29] Delegate count to Relation --- .../lib/active_record/calculations.rb | 63 +------------------ .../relation/calculation_methods.rb | 63 ++++++++++++++++++- .../active_record/relation/finder_methods.rb | 7 ++- 3 files changed, 67 insertions(+), 66 deletions(-) diff --git a/activerecord/lib/active_record/calculations.rb b/activerecord/lib/active_record/calculations.rb index db1f332336..41acf7be4e 100644 --- a/activerecord/lib/active_record/calculations.rb +++ b/activerecord/lib/active_record/calculations.rb @@ -3,68 +3,7 @@ module ActiveRecord extend ActiveSupport::Concern module ClassMethods - # Count operates using three different approaches. - # - # * Count all: By not passing any parameters to count, it will return a count of all the rows for the model. - # * Count using column: By passing a column name to count, it will return a count of all the rows for the model with supplied column present - # * Count using options will find the row count matched by the options used. - # - # The third approach, count using options, accepts an option hash as the only parameter. The options are: - # - # * :conditions: An SQL fragment like "administrator = 1" or [ "user_name = ?", username ]. See conditions in the intro to ActiveRecord::Base. - # * :joins: Either an SQL fragment for additional joins like "LEFT JOIN comments ON comments.post_id = id" (rarely needed) - # or named associations in the same form used for the :include option, which will perform an INNER JOIN on the associated table(s). - # If the value is a string, then the records will be returned read-only since they will have attributes that do not correspond to the table's columns. - # Pass :readonly => false to override. - # * :include: Named associations that should be loaded alongside using LEFT OUTER JOINs. The symbols named refer - # to already defined associations. When using named associations, count returns the number of DISTINCT items for the model you're counting. - # See eager loading under Associations. - # * :order: An SQL fragment like "created_at DESC, name" (really only used with GROUP BY calculations). - # * :group: An attribute name by which the result should be grouped. Uses the GROUP BY SQL-clause. - # * :select: By default, this is * as in SELECT * FROM, but can be changed if you, for example, want to do a join but not - # include the joined columns. - # * :distinct: Set this to true to make this a distinct calculation, such as SELECT COUNT(DISTINCT posts.id) ... - # * :from - By default, this is the table name of the class, but can be changed to an alternate table name (or even the name - # of a database view). - # - # Examples for counting all: - # Person.count # returns the total count of all people - # - # Examples for counting by column: - # Person.count(:age) # returns the total count of all people whose age is present in database - # - # Examples for count with options: - # Person.count(:conditions => "age > 26") - # Person.count(:conditions => "age > 26 AND job.salary > 60000", :include => :job) # because of the named association, it finds the DISTINCT count using LEFT OUTER JOIN. - # Person.count(:conditions => "age > 26 AND job.salary > 60000", :joins => "LEFT JOIN jobs on jobs.person_id = person.id") # finds the number of rows matching the conditions and joins. - # Person.count('id', :conditions => "age > 26") # Performs a COUNT(id) - # Person.count(:all, :conditions => "age > 26") # Performs a COUNT(*) (:all is an alias for '*') - # - # Note: Person.count(:all) will not work because it will use :all as the condition. Use Person.count instead. - def count(*args) - case args.size - when 0 - construct_calculation_arel.count - when 1 - if args[0].is_a?(Hash) - options = args[0] - distinct = options.has_key?(:distinct) ? options.delete(:distinct) : false - construct_calculation_arel(options).count(options[:select], :distinct => distinct) - else - construct_calculation_arel.count(args[0]) - end - when 2 - column_name, options = args - distinct = options.has_key?(:distinct) ? options.delete(:distinct) : false - construct_calculation_arel(options).count(column_name, :distinct => distinct) - else - raise ArgumentError, "Unexpected parameters passed to count(): #{args.inspect}" - end - rescue ThrowResult - 0 - end - - delegate :average, :minimum, :maximum, :sum, :to => :scoped + delegate :count, :average, :minimum, :maximum, :sum, :to => :scoped # This calculates aggregate values in the given column. Methods for count, sum, average, minimum, and maximum have been added as shortcuts. # Options such as :conditions, :order, :group, :having, and :joins can be passed to customize the query. diff --git a/activerecord/lib/active_record/relation/calculation_methods.rb b/activerecord/lib/active_record/relation/calculation_methods.rb index 7dd6e04db9..b96b69a18e 100644 --- a/activerecord/lib/active_record/relation/calculation_methods.rb +++ b/activerecord/lib/active_record/relation/calculation_methods.rb @@ -1,8 +1,55 @@ module ActiveRecord module CalculationMethods + # Count operates using three different approaches. + # + # * Count all: By not passing any parameters to count, it will return a count of all the rows for the model. + # * Count using column: By passing a column name to count, it will return a count of all the rows for the model with supplied column present + # * Count using options will find the row count matched by the options used. + # + # The third approach, count using options, accepts an option hash as the only parameter. The options are: + # + # * :conditions: An SQL fragment like "administrator = 1" or [ "user_name = ?", username ]. See conditions in the intro to ActiveRecord::Base. + # * :joins: Either an SQL fragment for additional joins like "LEFT JOIN comments ON comments.post_id = id" (rarely needed) + # or named associations in the same form used for the :include option, which will perform an INNER JOIN on the associated table(s). + # If the value is a string, then the records will be returned read-only since they will have attributes that do not correspond to the table's columns. + # Pass :readonly => false to override. + # * :include: Named associations that should be loaded alongside using LEFT OUTER JOINs. The symbols named refer + # to already defined associations. When using named associations, count returns the number of DISTINCT items for the model you're counting. + # See eager loading under Associations. + # * :order: An SQL fragment like "created_at DESC, name" (really only used with GROUP BY calculations). + # * :group: An attribute name by which the result should be grouped. Uses the GROUP BY SQL-clause. + # * :select: By default, this is * as in SELECT * FROM, but can be changed if you, for example, want to do a join but not + # include the joined columns. + # * :distinct: Set this to true to make this a distinct calculation, such as SELECT COUNT(DISTINCT posts.id) ... + # * :from - By default, this is the table name of the class, but can be changed to an alternate table name (or even the name + # of a database view). + # + # Examples for counting all: + # Person.count # returns the total count of all people + # + # Examples for counting by column: + # Person.count(:age) # returns the total count of all people whose age is present in database + # + # Examples for count with options: + # Person.count(:conditions => "age > 26") + # Person.count(:conditions => "age > 26 AND job.salary > 60000", :include => :job) # because of the named association, it finds the DISTINCT count using LEFT OUTER JOIN. + # Person.count(:conditions => "age > 26 AND job.salary > 60000", :joins => "LEFT JOIN jobs on jobs.person_id = person.id") # finds the number of rows matching the conditions and joins. + # Person.count('id', :conditions => "age > 26") # Performs a COUNT(id) + # Person.count(:all, :conditions => "age > 26") # Performs a COUNT(*) (:all is an alias for '*') + # + # Note: Person.count(:all) will not work because it will use :all as the condition. Use Person.count instead. + def count(column_name = nil, options = {}) + column_name, options = nil, column_name if column_name.is_a?(Hash) + column_name = select_for_count unless column_name + distinct = options.delete(:distinct) - def count(*args) - calculate(:count, *construct_count_options_from_args(*args)) + if options.any? + calculation_relation(options).count(column_name, :distinct => distinct) + else + relation_for_association_calculations.calculate(:count, column_name || :all, :distinct => distinct) + end + rescue ThrowResult + 0 end # Calculates the average value on a given column. The value is returned as @@ -73,12 +120,16 @@ module ActiveRecord if options.present? apply_finder_options(options.except(:distinct)).calculation_relation else - (eager_loading? || includes_values.present?) ? construct_relation_for_association_calculations : self + relation_for_association_calculations end end private + def relation_for_association_calculations + (eager_loading? || includes_values.present?) ? construct_relation_for_association_calculations : self + end + def execute_simple_calculation(operation, column_name, distinct) #:nodoc: column = if @klass.column_names.include?(column_name.to_s) Arel::Attribute.new(@klass.unscoped, column_name) @@ -196,5 +247,11 @@ module ActiveRecord @select_values.join(", ") if @select_values.present? end + def select_for_count + if @select_values.present? + select = @select_values.join(", ") + select if select !~ /(,|\*)/ + end + end end end diff --git a/activerecord/lib/active_record/relation/finder_methods.rb b/activerecord/lib/active_record/relation/finder_methods.rb index c48c8fe828..85ae3d11bb 100644 --- a/activerecord/lib/active_record/relation/finder_methods.rb +++ b/activerecord/lib/active_record/relation/finder_methods.rb @@ -56,12 +56,17 @@ module ActiveRecord def construct_relation_for_association_calculations including = (@eager_load_values + @includes_values).uniq join_dependency = ActiveRecord::Associations::ClassMethods::JoinDependency.new(@klass, including, arel.joins(arel)) - construct_relation_for_association_find(join_dependency) + + relation = except(:includes, :eager_load, :preload) + apply_join_dependency(relation, join_dependency) end def construct_relation_for_association_find(join_dependency) relation = except(:includes, :eager_load, :preload, :select).select(@klass.send(:column_aliases, join_dependency)) + apply_join_dependency(relation, join_dependency) + end + def apply_join_dependency(relation, join_dependency) for association in join_dependency.join_associations relation = association.join_relation(relation) end From ec63fdcff35d9ee195e11043ba3219b1e341a3f2 Mon Sep 17 00:00:00 2001 From: Pratik Naik Date: Tue, 19 Jan 2010 20:43:03 +0530 Subject: [PATCH 07/29] Get rid of construct_count_options_from_args --- .../associations/association_collection.rb | 7 +++-- .../relation/calculation_methods.rb | 30 ------------------- 2 files changed, 4 insertions(+), 33 deletions(-) diff --git a/activerecord/lib/active_record/associations/association_collection.rb b/activerecord/lib/active_record/associations/association_collection.rb index e9402d3547..9487d16123 100644 --- a/activerecord/lib/active_record/associations/association_collection.rb +++ b/activerecord/lib/active_record/associations/association_collection.rb @@ -176,14 +176,15 @@ module ActiveRecord # be used for the query. If no +:counter_sql+ was supplied, but +:finder_sql+ was set, the # descendant's +construct_sql+ method will have set :counter_sql automatically. # Otherwise, construct options and pass them with scope to the target class's +count+. - def count(*args) + def count(column_name = nil, options = {}) if @reflection.options[:counter_sql] @reflection.klass.count_by_sql(@counter_sql) else - column_name, options = @reflection.klass.scoped.send(:construct_count_options_from_args, *args) + column_name, options = nil, column_name if column_name.is_a?(Hash) + if @reflection.options[:uniq] # This is needed because 'SELECT count(DISTINCT *)..' is not valid SQL. - column_name = "#{@reflection.quoted_table_name}.#{@reflection.klass.primary_key}" if column_name == :all + column_name = "#{@reflection.quoted_table_name}.#{@reflection.klass.primary_key}" unless column_name options.merge!(:distinct => true) end diff --git a/activerecord/lib/active_record/relation/calculation_methods.rb b/activerecord/lib/active_record/relation/calculation_methods.rb index b96b69a18e..53d8594da8 100644 --- a/activerecord/lib/active_record/relation/calculation_methods.rb +++ b/activerecord/lib/active_record/relation/calculation_methods.rb @@ -180,32 +180,6 @@ module ActiveRecord end end - def construct_count_options_from_args(*args) - options = {} - column_name = :all - - # Handles count(), count(:column), count(:distinct => true), count(:column, :distinct => true) - case args.size - when 0 - select = get_projection_name_from_chained_relations - column_name = select if select !~ /(,|\*)/ - when 1 - if args[0].is_a?(Hash) - select = get_projection_name_from_chained_relations - column_name = select if select !~ /(,|\*)/ - options = args[0] - else - column_name = args[0] - end - when 2 - column_name, options = args - else - raise ArgumentError, "Unexpected parameters passed to count(): #{args.inspect}" - end - - [column_name || :all, options] - end - # Converts the given keys to the value that the database adapter returns as # a usable column name: # @@ -243,10 +217,6 @@ module ActiveRecord column ? column.type_cast(value) : value end - def get_projection_name_from_chained_relations - @select_values.join(", ") if @select_values.present? - end - def select_for_count if @select_values.present? select = @select_values.join(", ") From e8e8da5c85c105b7a4fec904f82c5e3c7fce507a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Tue, 19 Jan 2010 16:01:53 +0100 Subject: [PATCH 08/29] Logging thread should not die on logging errors. --- railties/lib/rails/subscriber.rb | 6 +++++- railties/test/subscriber_test.rb | 14 ++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/railties/lib/rails/subscriber.rb b/railties/lib/rails/subscriber.rb index 2674bf003e..9965786d86 100644 --- a/railties/lib/rails/subscriber.rb +++ b/railties/lib/rails/subscriber.rb @@ -63,7 +63,11 @@ module Rails subscriber = subscribers[namespace.to_sym] if subscriber.respond_to?(name) && subscriber.logger - subscriber.send(name, ActiveSupport::Notifications::Event.new(*args)) + begin + subscriber.send(name, ActiveSupport::Notifications::Event.new(*args)) + rescue Exception => e + Rails.logger.error "Could not log #{args[0].inspect} event. #{e.class}: #{e.message}" + end end if args[0] == "action_dispatch.after_dispatch" && !subscribers.empty? diff --git a/railties/test/subscriber_test.rb b/railties/test/subscriber_test.rb index fa3f7bfabb..724e8a75d6 100644 --- a/railties/test/subscriber_test.rb +++ b/railties/test/subscriber_test.rb @@ -18,6 +18,10 @@ class MySubscriber < Rails::Subscriber def bar(event) info "#{color("cool", :red)}, #{color("isn't it?", :blue, true)}" end + + def puke(event) + raise "puke" + end end module SubscriberTest @@ -105,6 +109,16 @@ module SubscriberTest assert_equal 1, @logger.flush_count end + def test_logging_thread_does_not_die_on_failures + Rails::Subscriber.add :my_subscriber, @subscriber + instrument "my_subscriber.puke" + instrument "action_dispatch.after_dispatch" + wait + assert_equal 1, @logger.flush_count + assert_equal 1, @logger.logged(:error).size + assert_equal 'Could not log "my_subscriber.puke" event. RuntimeError: puke', @logger.logged(:error).last + end + def test_tails_logs_when_action_dispatch_callback_is_received log_tailer = mock() log_tailer.expects(:tail!) From fef5afa962e6c3551e8e27776e67a750799899e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Tue, 19 Jan 2010 16:12:48 +0100 Subject: [PATCH 09/29] Get rid of RAILS_ROOT deprecation on AM::TestCase. --- actionmailer/lib/action_mailer/test_case.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/actionmailer/lib/action_mailer/test_case.rb b/actionmailer/lib/action_mailer/test_case.rb index 318a1e46d1..0ca4f5494e 100644 --- a/actionmailer/lib/action_mailer/test_case.rb +++ b/actionmailer/lib/action_mailer/test_case.rb @@ -56,7 +56,7 @@ module ActionMailer end def read_fixture(action) - IO.readlines(File.join(RAILS_ROOT, 'test', 'fixtures', self.class.mailer_class.name.underscore, action)) + IO.readlines(File.join(Rails.root, 'test', 'fixtures', self.class.mailer_class.name.underscore, action)) end end end From 8f63dcb64873fe94c5d837a49f742d41589ce311 Mon Sep 17 00:00:00 2001 From: Pratik Naik Date: Tue, 19 Jan 2010 22:09:04 +0530 Subject: [PATCH 10/29] Move the only remaining calculation method calculate() to Relation --- .../lib/active_record/calculations.rb | 54 +---------- .../relation/calculation_methods.rb | 91 +++++++++++++------ 2 files changed, 62 insertions(+), 83 deletions(-) diff --git a/activerecord/lib/active_record/calculations.rb b/activerecord/lib/active_record/calculations.rb index 41acf7be4e..ea63617c52 100644 --- a/activerecord/lib/active_record/calculations.rb +++ b/activerecord/lib/active_record/calculations.rb @@ -3,59 +3,7 @@ module ActiveRecord extend ActiveSupport::Concern module ClassMethods - delegate :count, :average, :minimum, :maximum, :sum, :to => :scoped - - # This calculates aggregate values in the given column. Methods for count, sum, average, minimum, and maximum have been added as shortcuts. - # Options such as :conditions, :order, :group, :having, and :joins can be passed to customize the query. - # - # There are two basic forms of output: - # * Single aggregate value: The single value is type cast to Fixnum for COUNT, Float for AVG, and the given column's type for everything else. - # * Grouped values: This returns an ordered hash of the values and groups them by the :group option. It takes either a column name, or the name - # of a belongs_to association. - # - # values = Person.maximum(:age, :group => 'last_name') - # puts values["Drake"] - # => 43 - # - # drake = Family.find_by_last_name('Drake') - # values = Person.maximum(:age, :group => :family) # Person belongs_to :family - # puts values[drake] - # => 43 - # - # values.each do |family, max_age| - # ... - # end - # - # Options: - # * :conditions - An SQL fragment like "administrator = 1" or [ "user_name = ?", username ]. See conditions in the intro to ActiveRecord::Base. - # * :include: Eager loading, see Associations for details. Since calculations don't load anything, the purpose of this is to access fields on joined tables in your conditions, order, or group clauses. - # * :joins - An SQL fragment for additional joins like "LEFT JOIN comments ON comments.post_id = id". (Rarely needed). - # The records will be returned read-only since they will have attributes that do not correspond to the table's columns. - # * :order - An SQL fragment like "created_at DESC, name" (really only used with GROUP BY calculations). - # * :group - An attribute name by which the result should be grouped. Uses the GROUP BY SQL-clause. - # * :select - By default, this is * as in SELECT * FROM, but can be changed if you for example want to do a join, but not - # include the joined columns. - # * :distinct - Set this to true to make this a distinct calculation, such as SELECT COUNT(DISTINCT posts.id) ... - # - # Examples: - # Person.calculate(:count, :all) # The same as Person.count - # Person.average(:age) # SELECT AVG(age) FROM people... - # Person.minimum(:age, :conditions => ['last_name != ?', 'Drake']) # Selects the minimum age for everyone with a last name other than 'Drake' - # Person.minimum(:age, :having => 'min(age) > 17', :group => :last_name) # Selects the minimum age for any family without any minors - # Person.sum("2 * age") - def calculate(operation, column_name, options = {}) - construct_calculation_arel(options).calculate(operation, column_name, options.slice(:distinct)) - rescue ThrowResult - 0 - end - - private - - def construct_calculation_arel(options = {}) - relation = scoped.apply_finder_options(options.except(:distinct)) - (relation.eager_loading? || relation.includes_values.present?) ? relation.send(:construct_relation_for_association_calculations) : relation - end - + delegate :count, :average, :minimum, :maximum, :sum, :calculate, :to => :scoped end end end diff --git a/activerecord/lib/active_record/relation/calculation_methods.rb b/activerecord/lib/active_record/relation/calculation_methods.rb index 53d8594da8..155e90333c 100644 --- a/activerecord/lib/active_record/relation/calculation_methods.rb +++ b/activerecord/lib/active_record/relation/calculation_methods.rb @@ -40,16 +40,7 @@ module ActiveRecord # Note: Person.count(:all) will not work because it will use :all as the condition. Use Person.count instead. def count(column_name = nil, options = {}) column_name, options = nil, column_name if column_name.is_a?(Hash) - column_name = select_for_count unless column_name - distinct = options.delete(:distinct) - - if options.any? - calculation_relation(options).count(column_name, :distinct => distinct) - else - relation_for_association_calculations.calculate(:count, column_name || :all, :distinct => distinct) - end - rescue ThrowResult - 0 + calculate(:count, column_name, options) end # Calculates the average value on a given column. The value is returned as @@ -58,7 +49,7 @@ module ActiveRecord # # Person.average('age') # => 35.8 def average(column_name, options = {}) - calculation_relation(options).calculate(:average, column_name) + calculate(:average, column_name, options) end # Calculates the minimum value on a given column. The value is returned @@ -67,7 +58,7 @@ module ActiveRecord # # Person.minimum('age') # => 7 def minimum(column_name, options = {}) - calculation_relation(options).calculate(:minimum, column_name) + calculate(:minimum, column_name, options) end # Calculates the maximum value on a given column. The value is returned @@ -76,7 +67,7 @@ module ActiveRecord # # Person.maximum('age') # => 93 def maximum(column_name, options = {}) - calculation_relation(options).calculate(:maximum, column_name) + calculate(:maximum, column_name, options) end # Calculates the sum of values on a given column. The value is returned @@ -85,13 +76,69 @@ module ActiveRecord # # Person.sum('age') # => 4562 def sum(column_name, options = {}) - calculation_relation(options).calculate(:sum, column_name) + calculate(:sum, column_name, options) end + # This calculates aggregate values in the given column. Methods for count, sum, average, minimum, and maximum have been added as shortcuts. + # Options such as :conditions, :order, :group, :having, and :joins can be passed to customize the query. + # + # There are two basic forms of output: + # * Single aggregate value: The single value is type cast to Fixnum for COUNT, Float for AVG, and the given column's type for everything else. + # * Grouped values: This returns an ordered hash of the values and groups them by the :group option. It takes either a column name, or the name + # of a belongs_to association. + # + # values = Person.maximum(:age, :group => 'last_name') + # puts values["Drake"] + # => 43 + # + # drake = Family.find_by_last_name('Drake') + # values = Person.maximum(:age, :group => :family) # Person belongs_to :family + # puts values[drake] + # => 43 + # + # values.each do |family, max_age| + # ... + # end + # + # Options: + # * :conditions - An SQL fragment like "administrator = 1" or [ "user_name = ?", username ]. See conditions in the intro to ActiveRecord::Base. + # * :include: Eager loading, see Associations for details. Since calculations don't load anything, the purpose of this is to access fields on joined tables in your conditions, order, or group clauses. + # * :joins - An SQL fragment for additional joins like "LEFT JOIN comments ON comments.post_id = id". (Rarely needed). + # The records will be returned read-only since they will have attributes that do not correspond to the table's columns. + # * :order - An SQL fragment like "created_at DESC, name" (really only used with GROUP BY calculations). + # * :group - An attribute name by which the result should be grouped. Uses the GROUP BY SQL-clause. + # * :select - By default, this is * as in SELECT * FROM, but can be changed if you for example want to do a join, but not + # include the joined columns. + # * :distinct - Set this to true to make this a distinct calculation, such as SELECT COUNT(DISTINCT posts.id) ... + # + # Examples: + # Person.calculate(:count, :all) # The same as Person.count + # Person.average(:age) # SELECT AVG(age) FROM people... + # Person.minimum(:age, :conditions => ['last_name != ?', 'Drake']) # Selects the minimum age for everyone with a last name other than 'Drake' + # Person.minimum(:age, :having => 'min(age) > 17', :group => :last_name) # Selects the minimum age for any family without any minors + # Person.sum("2 * age") def calculate(operation, column_name, options = {}) + if options.except(:distinct).present? + apply_finder_options(options.except(:distinct)).calculate(operation, column_name, :distinct => options[:distinct]) + else + if eager_loading? || includes_values.present? + construct_relation_for_association_calculations.calculate(operation, column_name, options) + else + perform_calculation(operation, column_name, options) + end + end + rescue ThrowResult + 0 + end + + private + + def perform_calculation(operation, column_name, options = {}) operation = operation.to_s.downcase if operation == "count" + column_name ||= (select_for_count || :all) + joins = arel.joins(arel) if joins.present? && joins =~ /LEFT OUTER/i distinct = true @@ -112,22 +159,6 @@ module ActiveRecord else return execute_simple_calculation(operation, column_name, distinct) end - rescue ThrowResult - 0 - end - - def calculation_relation(options = {}) - if options.present? - apply_finder_options(options.except(:distinct)).calculation_relation - else - relation_for_association_calculations - end - end - - private - - def relation_for_association_calculations - (eager_loading? || includes_values.present?) ? construct_relation_for_association_calculations : self end def execute_simple_calculation(operation, column_name, distinct) #:nodoc: From 9465b84b543ae1ada29c0e39e34f919c724eab6d Mon Sep 17 00:00:00 2001 From: Pratik Naik Date: Tue, 19 Jan 2010 22:15:35 +0530 Subject: [PATCH 11/29] Rename CalculationMethods to Calculations and get rid of the old Calculations module --- activerecord/lib/active_record.rb | 3 +-- activerecord/lib/active_record/base.rb | 3 ++- activerecord/lib/active_record/calculations.rb | 9 --------- activerecord/lib/active_record/relation.rb | 2 +- .../relation/{calculation_methods.rb => calculations.rb} | 2 +- 5 files changed, 5 insertions(+), 14 deletions(-) delete mode 100644 activerecord/lib/active_record/calculations.rb rename activerecord/lib/active_record/relation/{calculation_methods.rb => calculations.rb} (99%) diff --git a/activerecord/lib/active_record.rb b/activerecord/lib/active_record.rb index d5b6d40514..58673ab7bd 100644 --- a/activerecord/lib/active_record.rb +++ b/activerecord/lib/active_record.rb @@ -53,14 +53,13 @@ module ActiveRecord autoload_under 'relation' do autoload :QueryMethods autoload :FinderMethods - autoload :CalculationMethods + autoload :Calculations autoload :PredicateBuilder autoload :SpawnMethods end autoload :Base autoload :Batches - autoload :Calculations autoload :Callbacks autoload :DynamicFinderMatch autoload :DynamicScopeMatch diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb index 06244d1132..e10df1abd3 100755 --- a/activerecord/lib/active_record/base.rb +++ b/activerecord/lib/active_record/base.rb @@ -654,6 +654,7 @@ module ActiveRecord #:nodoc: end delegate :select, :group, :order, :limit, :joins, :where, :preload, :eager_load, :includes, :from, :lock, :readonly, :having, :to => :scoped + delegate :count, :average, :minimum, :maximum, :sum, :calculate, :to => :scoped # A convenience wrapper for find(:first, *args). You can pass in all the # same arguments to this method as you can to find(:first). @@ -2742,7 +2743,7 @@ module ActiveRecord #:nodoc: # #save_with_autosave_associations to be wrapped inside a transaction. include AutosaveAssociation, NestedAttributes - include Aggregations, Transactions, Reflection, Batches, Calculations, Serialization + include Aggregations, Transactions, Reflection, Batches, Serialization end end diff --git a/activerecord/lib/active_record/calculations.rb b/activerecord/lib/active_record/calculations.rb deleted file mode 100644 index ea63617c52..0000000000 --- a/activerecord/lib/active_record/calculations.rb +++ /dev/null @@ -1,9 +0,0 @@ -module ActiveRecord - module Calculations #:nodoc: - extend ActiveSupport::Concern - - module ClassMethods - delegate :count, :average, :minimum, :maximum, :sum, :calculate, :to => :scoped - end - end -end diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb index 19f91f4278..6ac7137976 100644 --- a/activerecord/lib/active_record/relation.rb +++ b/activerecord/lib/active_record/relation.rb @@ -5,7 +5,7 @@ module ActiveRecord MULTI_VALUE_METHODS = [:select, :group, :order, :joins, :where, :having] SINGLE_VALUE_METHODS = [:limit, :offset, :lock, :readonly, :create_with, :from] - include FinderMethods, CalculationMethods, SpawnMethods, QueryMethods + include FinderMethods, Calculations, SpawnMethods, QueryMethods delegate :length, :collect, :map, :each, :all?, :include?, :to => :to_a diff --git a/activerecord/lib/active_record/relation/calculation_methods.rb b/activerecord/lib/active_record/relation/calculations.rb similarity index 99% rename from activerecord/lib/active_record/relation/calculation_methods.rb rename to activerecord/lib/active_record/relation/calculations.rb index 155e90333c..5645e7f031 100644 --- a/activerecord/lib/active_record/relation/calculation_methods.rb +++ b/activerecord/lib/active_record/relation/calculations.rb @@ -1,5 +1,5 @@ module ActiveRecord - module CalculationMethods + module Calculations # Count operates using three different approaches. # # * Count all: By not passing any parameters to count, it will return a count of all the rows for the model. From dbce07b81d24a991ddd29b91f9c358b1f236bda2 Mon Sep 17 00:00:00 2001 From: Pratik Naik Date: Tue, 19 Jan 2010 22:52:08 +0530 Subject: [PATCH 12/29] Give preference to to_a over arel from Relation#method_missing --- activerecord/lib/active_record/locking/optimistic.rb | 4 ++-- activerecord/lib/active_record/relation.rb | 7 ++++--- activerecord/test/cases/named_scope_test.rb | 6 ++++++ 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/activerecord/lib/active_record/locking/optimistic.rb b/activerecord/lib/active_record/locking/optimistic.rb index 9fcdabdb44..bf0683eb8f 100644 --- a/activerecord/lib/active_record/locking/optimistic.rb +++ b/activerecord/lib/active_record/locking/optimistic.rb @@ -81,8 +81,8 @@ module ActiveRecord relation = self.class.unscoped affected_rows = relation.where( - relation[self.class.primary_key].eq(quoted_id).and( - relation[self.class.locking_column].eq(quote_value(previous_value)) + relation.table[self.class.primary_key].eq(quoted_id).and( + relation.table[self.class.locking_column].eq(quote_value(previous_value)) ) ).update(arel_attributes_values(false, false, attribute_names)) diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb index 6ac7137976..8bb019f147 100644 --- a/activerecord/lib/active_record/relation.rb +++ b/activerecord/lib/active_record/relation.rb @@ -8,6 +8,7 @@ module ActiveRecord include FinderMethods, Calculations, SpawnMethods, QueryMethods delegate :length, :collect, :map, :each, :all?, :include?, :to => :to_a + delegate :insert, :update, :where_clause, :to => :arel attr_reader :table, :klass @@ -139,10 +140,10 @@ module ActiveRecord protected def method_missing(method, *args, &block) - if arel.respond_to?(method) - arel.send(method, *args, &block) - elsif Array.method_defined?(method) + if Array.method_defined?(method) to_a.send(method, *args, &block) + elsif arel.respond_to?(method) + arel.send(method, *args, &block) elsif match = DynamicFinderMatch.match(method) attributes = match.attribute_names super unless @klass.send(:all_attributes_exists?, attributes) diff --git a/activerecord/test/cases/named_scope_test.rb b/activerecord/test/cases/named_scope_test.rb index 3e2bd58f9a..2c34ab787d 100644 --- a/activerecord/test/cases/named_scope_test.rb +++ b/activerecord/test/cases/named_scope_test.rb @@ -379,6 +379,12 @@ class NamedScopeTest < ActiveRecord::TestCase def test_deprecated_named_scope_method assert_deprecated('named_scope has been deprecated') { Topic.named_scope :deprecated_named_scope } end + + def test_index_on_named_scope + approved = Topic.approved.order('id ASC') + assert_equal topics(:second), approved[0] + assert approved.loaded? + end end class DynamicScopeMatchTest < ActiveRecord::TestCase From 9acf0af544f2f5dcaf257bdc25047017c972ffce Mon Sep 17 00:00:00 2001 From: Pratik Naik Date: Tue, 19 Jan 2010 23:11:54 +0530 Subject: [PATCH 13/29] Remove Relation#where_clause --- activerecord/lib/active_record/relation.rb | 6 +----- activerecord/lib/active_record/relation/finder_methods.rb | 4 ++-- activerecord/test/cases/method_scoping_test.rb | 8 ++++---- 3 files changed, 7 insertions(+), 11 deletions(-) diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb index 8bb019f147..f86677aa51 100644 --- a/activerecord/lib/active_record/relation.rb +++ b/activerecord/lib/active_record/relation.rb @@ -8,7 +8,7 @@ module ActiveRecord include FinderMethods, Calculations, SpawnMethods, QueryMethods delegate :length, :collect, :map, :each, :all?, :include?, :to => :to_a - delegate :insert, :update, :where_clause, :to => :arel + delegate :insert, :update, :to => :arel attr_reader :table, :klass @@ -164,10 +164,6 @@ module ActiveRecord @klass.send(:with_scope, :create => scope_for_create, :find => {}) { yield } end - def where_clause(join_string = " AND ") - arel.send(:where_clauses).join(join_string) - end - def references_eager_loaded_tables? joined_tables = (tables_in_string(arel.joins(arel)) + [table.name, table.table_alias]).compact.uniq (tables_in_string(to_sql) - joined_tables).any? diff --git a/activerecord/lib/active_record/relation/finder_methods.rb b/activerecord/lib/active_record/relation/finder_methods.rb index 85ae3d11bb..90ca002c76 100644 --- a/activerecord/lib/active_record/relation/finder_methods.rb +++ b/activerecord/lib/active_record/relation/finder_methods.rb @@ -128,7 +128,7 @@ module ActiveRecord record = where(primary_key.eq(id)).first unless record - conditions = where_clause(', ') + conditions = arel.send(:where_clauses).join(', ') conditions = " [WHERE #{conditions}]" if conditions.present? raise RecordNotFound, "Couldn't find #{@klass.name} with ID=#{id}#{conditions}" end @@ -154,7 +154,7 @@ module ActiveRecord if result.size == expected_size result else - conditions = where_clause(', ') + conditions = arel.send(:where_clauses).join(', ') conditions = " [WHERE #{conditions}]" if conditions.present? error = "Couldn't find all #{@klass.name.pluralize} with IDs " diff --git a/activerecord/test/cases/method_scoping_test.rb b/activerecord/test/cases/method_scoping_test.rb index 7ca5b5a988..fbd1adf088 100644 --- a/activerecord/test/cases/method_scoping_test.rb +++ b/activerecord/test/cases/method_scoping_test.rb @@ -11,7 +11,7 @@ class MethodScopingTest < ActiveRecord::TestCase def test_set_conditions Developer.send(:with_scope, :find => { :conditions => 'just a test...' }) do - assert_equal '(just a test...)', Developer.scoped.send(:where_clause) + assert_equal '(just a test...)', Developer.scoped.arel.send(:where_clauses).join(' AND ') end end @@ -257,7 +257,7 @@ class NestedScopingTest < ActiveRecord::TestCase Developer.send(:with_scope, :find => { :conditions => 'salary = 80000' }) do Developer.send(:with_scope, :find => { :limit => 10 }) do devs = Developer.scoped - assert_equal '(salary = 80000)', devs.send(:where_clause) + assert_equal '(salary = 80000)', devs.arel.send(:where_clauses).join(' AND ') assert_equal 10, devs.taken end end @@ -285,7 +285,7 @@ class NestedScopingTest < ActiveRecord::TestCase Developer.send(:with_scope, :find => { :conditions => "name = 'David'" }) do Developer.send(:with_scope, :find => { :conditions => 'salary = 80000' }) do devs = Developer.scoped - assert_equal "(name = 'David') AND (salary = 80000)", devs.send(:where_clause) + assert_equal "(name = 'David') AND (salary = 80000)", devs.arel.send(:where_clauses).join(' AND ') assert_equal(1, Developer.count) end Developer.send(:with_scope, :find => { :conditions => "name = 'Maiha'" }) do @@ -298,7 +298,7 @@ class NestedScopingTest < ActiveRecord::TestCase Developer.send(:with_scope, :find => { :conditions => 'salary = 80000', :limit => 10 }) do Developer.send(:with_scope, :find => { :conditions => "name = 'David'" }) do devs = Developer.scoped - assert_equal "(salary = 80000) AND (name = 'David')", devs.send(:where_clause) + assert_equal "(salary = 80000) AND (name = 'David')", devs.arel.send(:where_clauses).join(' AND ') assert_equal 10, devs.taken end end From 4ca97650880a751901c4370a50c806a84fa529f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Tue, 19 Jan 2010 18:43:09 +0100 Subject: [PATCH 14/29] Allow railties to specify generators paths. --- railties/lib/rails/application.rb | 6 ++- railties/lib/rails/generators.rb | 38 ++++++++++++------- railties/lib/rails/railtie.rb | 15 ++++++++ railties/test/generators_test.rb | 7 ++++ .../test/plugins/framework_extension_test.rb | 16 ++++++++ 5 files changed, 67 insertions(+), 15 deletions(-) diff --git a/railties/lib/rails/application.rb b/railties/lib/rails/application.rb index 4d05f8115c..b92a7ff129 100644 --- a/railties/lib/rails/application.rb +++ b/railties/lib/rails/application.rb @@ -8,7 +8,7 @@ module Rails class << self attr_writer :config alias configure class_eval - delegate :initialize!, :load_tasks, :root, :to => :instance + delegate :initialize!, :load_tasks, :load_generators, :root, :to => :instance private :new def instance @@ -82,6 +82,10 @@ module Rails end end + def load_generators + plugins.each { |p| p.load_generators } + end + def initializers initializers = Bootstrap.new(self).initializers plugins.each { |p| initializers += p.initializers } diff --git a/railties/lib/rails/generators.rb b/railties/lib/rails/generators.rb index 736c36c0dc..d3175e6a9d 100644 --- a/railties/lib/rails/generators.rb +++ b/railties/lib/rails/generators.rb @@ -168,7 +168,7 @@ module Rails # Show help message with available generators. def self.help - traverse_load_paths! + lookup! namespaces = subclasses.map{ |k| k.namespace } namespaces.sort! @@ -226,22 +226,10 @@ module Rails nil end - # This will try to load any generator in the load path to show in help. - def self.traverse_load_paths! #:nodoc: - $LOAD_PATH.each do |base| - Dir[File.join(base, "{generators,rails_generators}", "**", "*_generator.rb")].each do |path| - begin - require path - rescue Exception => e - # No problem - end - end - end - end - # Receives namespaces in an array and tries to find matching generators # in the load path. def self.lookup(namespaces) #:nodoc: + load_generators_from_railties! paths = namespaces_to_paths(namespaces) paths.each do |path| @@ -261,6 +249,28 @@ module Rails end end + # This will try to load any generator in the load path to show in help. + def self.lookup! #:nodoc: + load_generators_from_railties! + + $LOAD_PATH.each do |base| + Dir[File.join(base, "{generators,rails_generators}", "**", "*_generator.rb")].each do |path| + begin + require path + rescue Exception => e + # No problem + end + end + end + end + + # Allow generators to be loaded from custom paths. + def self.load_generators_from_railties! #:nodoc: + return if defined?(@generators_from_railties) || Rails.application.nil? + @generators_from_railties = true + Rails.application.load_generators + end + # Convert namespaces to paths by replacing ":" for "/" and adding # an extra lookup. For example, "rails:model" should be searched # in both: "rails/model/model_generator" and "rails/model_generator". diff --git a/railties/lib/rails/railtie.rb b/railties/lib/rails/railtie.rb index 43a0303c5b..e3297148e5 100644 --- a/railties/lib/rails/railtie.rb +++ b/railties/lib/rails/railtie.rb @@ -35,13 +35,28 @@ module Rails @rake_tasks end + def self.generators(&blk) + @generators ||= [] + @generators << blk if blk + @generators + end + def rake_tasks self.class.rake_tasks end + def generators + self.class.generators + end + def load_tasks return unless rake_tasks rake_tasks.each { |blk| blk.call } end + + def load_generators + return unless generators + generators.each { |blk| blk.call } + end end end diff --git a/railties/test/generators_test.rb b/railties/test/generators_test.rb index 60c81a813f..664d1e5670 100644 --- a/railties/test/generators_test.rb +++ b/railties/test/generators_test.rb @@ -148,6 +148,13 @@ class GeneratorsTest < Rails::Generators::TestCase Rails::Generators.subclasses.delete(klass) end + def test_load_generators_from_railties + Rails::Generators::ModelGenerator.expects(:start).with(["Account"], {}) + Rails::Generators.send(:remove_instance_variable, :@generators_from_railties) + Rails.application.expects(:load_generators) + Rails::Generators.invoke("model", ["Account"]) + end + def test_rails_root_templates template = File.join(Rails.root, "lib", "templates", "active_record", "model", "model.rb") diff --git a/railties/test/plugins/framework_extension_test.rb b/railties/test/plugins/framework_extension_test.rb index c920db77aa..d57fd4e635 100644 --- a/railties/test/plugins/framework_extension_test.rb +++ b/railties/test/plugins/framework_extension_test.rb @@ -30,6 +30,22 @@ module PluginsTest AppTemplate::Application.load_tasks assert $ran_block end + + test "generators block is executed when MyApp.load_generators is called" do + $ran_block = false + + class MyTie < Rails::Railtie + generators do + $ran_block = true + end + end + + require "#{app_path}/config/environment" + + assert !$ran_block + AppTemplate::Application.load_generators + assert $ran_block + end end class ActiveRecordExtensionTest < Test::Unit::TestCase From d2759d125ae64e6fd64c8bffe7b15568698654bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Tue, 19 Jan 2010 20:07:50 +0100 Subject: [PATCH 15/29] Avoid load tasks from plugins recursively (so stuff in tests or vendor does not get loaded). --- railties/lib/rails/plugin.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/railties/lib/rails/plugin.rb b/railties/lib/rails/plugin.rb index 0c09730963..c3b81bcd3e 100644 --- a/railties/lib/rails/plugin.rb +++ b/railties/lib/rails/plugin.rb @@ -27,7 +27,7 @@ module Rails end def load_tasks - Dir["#{path}/**/tasks/**/*.rake"].sort.each { |ext| load ext } + Dir["#{path}/{tasks,lib/tasks,rails/tasks}/**/*.rake"].sort.each { |ext| load ext } end initializer :add_to_load_path, :after => :set_autoload_paths do |app| From 6e62e89737c991de712a13b67a282ce599710ec9 Mon Sep 17 00:00:00 2001 From: Prem Sichanugrist Date: Tue, 19 Jan 2010 01:55:48 +0700 Subject: [PATCH 16/29] Fix bug that causes TimeZone.seconds_to_utc_offset to returns wrong offset when hour < 0 and not in hundreds [#3741 status:resolved] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: José Valim --- activesupport/lib/active_support/values/time_zone.rb | 7 ++++--- activesupport/test/time_zone_test.rb | 6 ++++++ 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/activesupport/lib/active_support/values/time_zone.rb b/activesupport/lib/active_support/values/time_zone.rb index cbb8e890ae..245d3ce051 100644 --- a/activesupport/lib/active_support/values/time_zone.rb +++ b/activesupport/lib/active_support/values/time_zone.rb @@ -172,7 +172,7 @@ module ActiveSupport MAPPING.freeze end - UTC_OFFSET_WITH_COLON = '%+03d:%02d' + UTC_OFFSET_WITH_COLON = '%s%02d:%02d' UTC_OFFSET_WITHOUT_COLON = UTC_OFFSET_WITH_COLON.sub(':', '') # Assumes self represents an offset from UTC in seconds (as returned from Time#utc_offset) @@ -181,9 +181,10 @@ module ActiveSupport # TimeZone.seconds_to_utc_offset(-21_600) # => "-06:00" def self.seconds_to_utc_offset(seconds, colon = true) format = colon ? UTC_OFFSET_WITH_COLON : UTC_OFFSET_WITHOUT_COLON - hours = seconds / 3600 + sign = (seconds < 0 ? '-' : '+') + hours = seconds.abs / 3600 minutes = (seconds.abs % 3600) / 60 - format % [hours, minutes] + format % [sign, hours, minutes] end include Comparable diff --git a/activesupport/test/time_zone_test.rb b/activesupport/test/time_zone_test.rb index 99c4310854..ce43c6507d 100644 --- a/activesupport/test/time_zone_test.rb +++ b/activesupport/test/time_zone_test.rb @@ -208,6 +208,12 @@ class TimeZoneTest < Test::Unit::TestCase assert_equal "+0000", ActiveSupport::TimeZone.seconds_to_utc_offset(0, false) assert_equal "+0500", ActiveSupport::TimeZone.seconds_to_utc_offset(18_000, false) end + + def test_seconds_to_utc_offset_with_negative_offset + assert_equal "-01:00", ActiveSupport::TimeZone.seconds_to_utc_offset(-3_600) + assert_equal "-00:59", ActiveSupport::TimeZone.seconds_to_utc_offset(-3_599) + assert_equal "-05:30", ActiveSupport::TimeZone.seconds_to_utc_offset(-19_800) + end def test_formatted_offset_positive zone = ActiveSupport::TimeZone['Moscow'] From 42553a98eaa05c703de52147c870e4dd9a3d50ba Mon Sep 17 00:00:00 2001 From: Pratik Naik Date: Wed, 20 Jan 2010 01:19:53 +0530 Subject: [PATCH 17/29] Remove find_with_associations and related code from associations now that Relation handles that stuff --- .../lib/active_record/associations.rb | 67 ------------------- ...s_and_belongs_to_many_associations_test.rb | 15 ----- 2 files changed, 82 deletions(-) diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index ebf1a41e85..57785b4c93 100755 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -1463,13 +1463,6 @@ module ActiveRecord after_destroy(method_name) end - def find_with_associations(options, join_dependency) - rows = select_all_rows(options, join_dependency) - join_dependency.instantiate(rows) - rescue ThrowResult - [] - end - # Creates before_destroy callback methods that nullify, delete or destroy # has_many associated objects, according to the defined :dependent rule. # @@ -1693,66 +1686,6 @@ module ActiveRecord reflection end - def select_all_rows(options, join_dependency) - connection.select_all( - construct_finder_sql_with_included_associations(options, join_dependency), - "#{name} Load Including Associations" - ) - end - - def construct_finder_arel_with_included_associations(options, join_dependency) - relation = scoped - - for association in join_dependency.join_associations - relation = association.join_relation(relation) - end - - relation = relation.apply_finder_options(options).select(column_aliases(join_dependency)) - - if !using_limitable_reflections?(join_dependency.reflections) && relation.limit_value - relation = relation.where(construct_arel_limited_ids_condition(options, join_dependency)) - end - - relation = relation.except(:limit, :offset) unless using_limitable_reflections?(join_dependency.reflections) - - relation - end - - def construct_finder_sql_with_included_associations(options, join_dependency) - construct_finder_arel_with_included_associations(options, join_dependency).to_sql - end - - def construct_arel_limited_ids_condition(options, join_dependency) - if (ids_array = select_limited_ids_array(options, join_dependency)).empty? - raise ThrowResult - else - Arel::Predicates::In.new( - Arel::SqlLiteral.new("#{connection.quote_table_name table_name}.#{primary_key}"), - ids_array - ) - end - end - - def select_limited_ids_array(options, join_dependency) - connection.select_all( - construct_finder_sql_for_association_limiting(options, join_dependency), - "#{name} Load IDs For Limited Eager Loading" - ).collect { |row| row[primary_key] } - end - - def construct_finder_sql_for_association_limiting(options, join_dependency) - relation = scoped - - for association in join_dependency.join_associations - relation = association.join_relation(relation) - end - - relation = relation.apply_finder_options(options).except(:select) - relation = relation.select(connection.distinct("#{connection.quote_table_name table_name}.#{primary_key}", relation.order_values.join(", "))) - - relation.to_sql - end - def using_limitable_reflections?(reflections) reflections.collect(&:collection?).length.zero? end diff --git a/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb b/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb index 1bce45865f..004d0156e1 100644 --- a/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb +++ b/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb @@ -732,21 +732,6 @@ class HasAndBelongsToManyAssociationsTest < ActiveRecord::TestCase assert_equal [projects(:active_record), projects(:action_controller)].map(&:id).sort, developer.project_ids.sort end - def test_select_limited_ids_array - # Set timestamps - Developer.transaction do - Developer.find(:all, :order => 'id').each_with_index do |record, i| - record.update_attributes(:created_at => 5.years.ago + (i * 5.minutes)) - end - end - - join_base = ActiveRecord::Associations::ClassMethods::JoinDependency::JoinBase.new(Project) - join_dep = ActiveRecord::Associations::ClassMethods::JoinDependency.new(join_base, :developers, nil) - projects = Project.send(:select_limited_ids_array, {:order => 'developers.created_at'}, join_dep) - assert !projects.include?("'"), projects - assert_equal ["1", "2"], projects.sort - end - def test_scoped_find_on_through_association_doesnt_return_read_only_records tag = Post.find(1).tags.find_by_name("General") From 1b78a3f8d5417708eb7040cc00722494eaa6a2b5 Mon Sep 17 00:00:00 2001 From: Pratik Naik Date: Wed, 20 Jan 2010 01:29:18 +0530 Subject: [PATCH 18/29] with_scope no longer needs :reverse_merge --- activerecord/lib/active_record/base.rb | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb index e10df1abd3..c20a14551e 100755 --- a/activerecord/lib/active_record/base.rb +++ b/activerecord/lib/active_record/base.rb @@ -1763,11 +1763,8 @@ module ActiveRecord #:nodoc: relation = construct_finder_arel(method_scoping[:find] || {}) if current_scoped_methods && current_scoped_methods.create_with_value && method_scoping[:create] - scope_for_create = case action - when :merge + scope_for_create = if action == :merge current_scoped_methods.create_with_value.merge(method_scoping[:create]) - when :reverse_merge - method_scoping[:create].merge(current_scoped_methods.create_with_value) else method_scoping[:create] end @@ -1782,15 +1779,7 @@ module ActiveRecord #:nodoc: method_scoping = relation end - if current_scoped_methods - case action - when :merge - method_scoping = current_scoped_methods.merge(method_scoping) - when :reverse_merge - method_scoping = current_scoped_methods.except(:where).merge(method_scoping) - method_scoping = method_scoping.merge(current_scoped_methods.only(:where)) - end - end + method_scoping = current_scoped_methods.merge(method_scoping) if current_scoped_methods && action == :merge self.scoped_methods << method_scoping begin From d8c30723aad943da20fed36cceedba6225122a3a Mon Sep 17 00:00:00 2001 From: Pratik Naik Date: Wed, 20 Jan 2010 01:35:20 +0530 Subject: [PATCH 19/29] Named scopes dont need count() now that Relation#count handles all the cases --- activerecord/lib/active_record/named_scope.rb | 5 ----- 1 file changed, 5 deletions(-) diff --git a/activerecord/lib/active_record/named_scope.rb b/activerecord/lib/active_record/named_scope.rb index f1b8822892..2f3044d06b 100644 --- a/activerecord/lib/active_record/named_scope.rb +++ b/activerecord/lib/active_record/named_scope.rb @@ -176,11 +176,6 @@ module ActiveRecord end end - def count(*args) - options = args.extract_options! - options.present? ? apply_finder_options(options).count(*args) : super - end - def ==(other) other.respond_to?(:to_a) ? to_a == other.to_a : false end From 848d6cd46b93ef9b1e755449a1de7cee56e7e115 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Tue, 19 Jan 2010 21:34:42 +0100 Subject: [PATCH 20/29] Mail should log when raise_delivery_methods is false. --- actionmailer/lib/action_mailer/base.rb | 14 ++++++++------ .../lib/action_mailer/railties/subscriber.rb | 8 ++++---- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/actionmailer/lib/action_mailer/base.rb b/actionmailer/lib/action_mailer/base.rb index a8233512ab..356861b591 100644 --- a/actionmailer/lib/action_mailer/base.rb +++ b/actionmailer/lib/action_mailer/base.rb @@ -524,14 +524,16 @@ module ActionMailer #:nodoc: def deliver!(mail = @mail) raise "no mail object available for delivery!" unless mail - begin - ActiveSupport::Notifications.instrument("action_mailer.deliver", - :template => template, :mailer => self.class.name) do |payload| - self.class.set_payload_for_mail(payload, mail) + ActiveSupport::Notifications.instrument("action_mailer.deliver", + :template => template, :mailer => self.class.name) do |payload| + + self.class.set_payload_for_mail(payload, mail) + + begin self.delivery_method.perform_delivery(mail) if perform_deliveries + rescue Exception => e # Net::SMTP errors or sendmail pipe errors + raise e if raise_delivery_errors end - rescue Exception => e # Net::SMTP errors or sendmail pipe errors - raise e if raise_delivery_errors end mail diff --git a/actionmailer/lib/action_mailer/railties/subscriber.rb b/actionmailer/lib/action_mailer/railties/subscriber.rb index af9c477237..cff852055c 100644 --- a/actionmailer/lib/action_mailer/railties/subscriber.rb +++ b/actionmailer/lib/action_mailer/railties/subscriber.rb @@ -3,13 +3,13 @@ module ActionMailer class Subscriber < Rails::Subscriber def deliver(event) recipients = Array(event.payload[:to]).join(', ') - info("Sent mail to #{recipients} (%1.fms)" % event.duration) - debug("\n#{event.payload[:mail]}") + info("\nSent mail to #{recipients} (%1.fms)" % event.duration) + debug(event.payload[:mail]) end def receive(event) - info("Received mail (%.1fms)" % event.duration) - debug("\n#{event.payload[:mail]}") + info("\nReceived mail (%.1fms)" % event.duration) + debug(event.payload[:mail]) end def logger From 74e3539cda914bf3cb380e8486d316c275a5f0cf Mon Sep 17 00:00:00 2001 From: Pratik Naik Date: Wed, 20 Jan 2010 02:17:35 +0530 Subject: [PATCH 21/29] Ignore order for simple calculations to make postgresql happy --- activerecord/lib/active_record/relation/calculations.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/activerecord/lib/active_record/relation/calculations.rb b/activerecord/lib/active_record/relation/calculations.rb index 5645e7f031..e77424a64b 100644 --- a/activerecord/lib/active_record/relation/calculations.rb +++ b/activerecord/lib/active_record/relation/calculations.rb @@ -168,7 +168,8 @@ module ActiveRecord Arel::SqlLiteral.new(column_name == :all ? "*" : column_name.to_s) end - relation = select(operation == 'count' ? column.count(distinct) : column.send(operation)) + # Postgresql doesn't like ORDER BY when there are no GROUP BY + relation = except(:order).select(operation == 'count' ? column.count(distinct) : column.send(operation)) type_cast_calculated_value(@klass.connection.select_value(relation.to_sql), column_for(column_name), operation) end From 52ec4311f5bf8b596612f297da0b3be8e284b038 Mon Sep 17 00:00:00 2001 From: Pratik Naik Date: Wed, 20 Jan 2010 03:35:25 +0530 Subject: [PATCH 22/29] Delegate all finders to Relation --- activerecord/lib/active_record/base.rb | 116 +----------- activerecord/lib/active_record/relation.rb | 2 - .../active_record/relation/finder_methods.rb | 176 +++++++++++++++--- .../active_record/relation/spawn_methods.rb | 17 +- 4 files changed, 153 insertions(+), 158 deletions(-) diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb index c20a14551e..6063c9789f 100755 --- a/activerecord/lib/active_record/base.rb +++ b/activerecord/lib/active_record/base.rb @@ -556,124 +556,10 @@ module ActiveRecord #:nodoc: end alias :colorize_logging= :colorize_logging - # Find operates with four different retrieval approaches: - # - # * Find by id - This can either be a specific id (1), a list of ids (1, 5, 6), or an array of ids ([5, 6, 10]). - # If no record can be found for all of the listed ids, then RecordNotFound will be raised. - # * Find first - This will return the first record matched by the options used. These options can either be specific - # conditions or merely an order. If no record can be matched, +nil+ is returned. Use - # Model.find(:first, *args) or its shortcut Model.first(*args). - # * Find last - This will return the last record matched by the options used. These options can either be specific - # conditions or merely an order. If no record can be matched, +nil+ is returned. Use - # Model.find(:last, *args) or its shortcut Model.last(*args). - # * Find all - This will return all the records matched by the options used. - # If no records are found, an empty array is returned. Use - # Model.find(:all, *args) or its shortcut Model.all(*args). - # - # All approaches accept an options hash as their last parameter. - # - # ==== Parameters - # - # * :conditions - An SQL fragment like "administrator = 1", [ "user_name = ?", username ], or ["user_name = :user_name", { :user_name => user_name }]. See conditions in the intro. - # * :order - An SQL fragment like "created_at DESC, name". - # * :group - An attribute name by which the result should be grouped. Uses the GROUP BY SQL-clause. - # * :having - Combined with +:group+ this can be used to filter the records that a GROUP BY returns. Uses the HAVING SQL-clause. - # * :limit - An integer determining the limit on the number of rows that should be returned. - # * :offset - An integer determining the offset from where the rows should be fetched. So at 5, it would skip rows 0 through 4. - # * :joins - Either an SQL fragment for additional joins like "LEFT JOIN comments ON comments.post_id = id" (rarely needed), - # named associations in the same form used for the :include option, which will perform an INNER JOIN on the associated table(s), - # or an array containing a mixture of both strings and named associations. - # If the value is a string, then the records will be returned read-only since they will have attributes that do not correspond to the table's columns. - # Pass :readonly => false to override. - # * :include - Names associations that should be loaded alongside. The symbols named refer - # to already defined associations. See eager loading under Associations. - # * :select - By default, this is "*" as in "SELECT * FROM", but can be changed if you, for example, want to do a join but not - # include the joined columns. Takes a string with the SELECT SQL fragment (e.g. "id, name"). - # * :from - By default, this is the table name of the class, but can be changed to an alternate table name (or even the name - # of a database view). - # * :readonly - Mark the returned records read-only so they cannot be saved or updated. - # * :lock - An SQL fragment like "FOR UPDATE" or "LOCK IN SHARE MODE". - # :lock => true gives connection's default exclusive lock, usually "FOR UPDATE". - # - # ==== Examples - # - # # find by id - # Person.find(1) # returns the object for ID = 1 - # Person.find(1, 2, 6) # returns an array for objects with IDs in (1, 2, 6) - # Person.find([7, 17]) # returns an array for objects with IDs in (7, 17) - # Person.find([1]) # returns an array for the object with ID = 1 - # Person.find(1, :conditions => "administrator = 1", :order => "created_on DESC") - # - # Note that returned records may not be in the same order as the ids you - # provide since database rows are unordered. Give an explicit :order - # to ensure the results are sorted. - # - # ==== Examples - # - # # find first - # Person.find(:first) # returns the first object fetched by SELECT * FROM people - # Person.find(:first, :conditions => [ "user_name = ?", user_name]) - # Person.find(:first, :conditions => [ "user_name = :u", { :u => user_name }]) - # Person.find(:first, :order => "created_on DESC", :offset => 5) - # - # # find last - # Person.find(:last) # returns the last object fetched by SELECT * FROM people - # Person.find(:last, :conditions => [ "user_name = ?", user_name]) - # Person.find(:last, :order => "created_on DESC", :offset => 5) - # - # # find all - # Person.find(:all) # returns an array of objects for all the rows fetched by SELECT * FROM people - # Person.find(:all, :conditions => [ "category IN (?)", categories], :limit => 50) - # Person.find(:all, :conditions => { :friends => ["Bob", "Steve", "Fred"] } - # Person.find(:all, :offset => 10, :limit => 10) - # Person.find(:all, :include => [ :account, :friends ]) - # Person.find(:all, :group => "category") - # - # Example for find with a lock: Imagine two concurrent transactions: - # each will read person.visits == 2, add 1 to it, and save, resulting - # in two saves of person.visits = 3. By locking the row, the second - # transaction has to wait until the first is finished; we get the - # expected person.visits == 4. - # - # Person.transaction do - # person = Person.find(1, :lock => true) - # person.visits += 1 - # person.save! - # end - def find(*args) - options = args.extract_options! - - relation = construct_finder_arel(options, current_scoped_methods) - - case args.first - when :first, :last, :all - relation.send(args.first) - else - relation.find(*args) - end - end - + delegate :find, :first, :last, :all, :to => :scoped delegate :select, :group, :order, :limit, :joins, :where, :preload, :eager_load, :includes, :from, :lock, :readonly, :having, :to => :scoped delegate :count, :average, :minimum, :maximum, :sum, :calculate, :to => :scoped - # A convenience wrapper for find(:first, *args). You can pass in all the - # same arguments to this method as you can to find(:first). - def first(*args) - find(:first, *args) - end - - # A convenience wrapper for find(:last, *args). You can pass in all the - # same arguments to this method as you can to find(:last). - def last(*args) - find(:last, *args) - end - - # A convenience wrapper for find(:all, *args). You can pass in all the - # same arguments to this method as you can to find(:all). - def all(*args) - find(:all, *args) - end - # Executes a custom SQL query against your database and returns all the results. The results will # be returned as an array with columns requested encapsulated as attributes of the model you call # this method from. If you call Product.find_by_sql then the results will be returned in diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb index f86677aa51..decde50427 100644 --- a/activerecord/lib/active_record/relation.rb +++ b/activerecord/lib/active_record/relation.rb @@ -60,8 +60,6 @@ module ActiveRecord @records end - alias all to_a - def size loaded? ? @records.length : count end diff --git a/activerecord/lib/active_record/relation/finder_methods.rb b/activerecord/lib/active_record/relation/finder_methods.rb index 90ca002c76..999309d2bd 100644 --- a/activerecord/lib/active_record/relation/finder_methods.rb +++ b/activerecord/lib/active_record/relation/finder_methods.rb @@ -1,47 +1,130 @@ module ActiveRecord module FinderMethods - - def find(*ids, &block) + # Find operates with four different retrieval approaches: + # + # * Find by id - This can either be a specific id (1), a list of ids (1, 5, 6), or an array of ids ([5, 6, 10]). + # If no record can be found for all of the listed ids, then RecordNotFound will be raised. + # * Find first - This will return the first record matched by the options used. These options can either be specific + # conditions or merely an order. If no record can be matched, +nil+ is returned. Use + # Model.find(:first, *args) or its shortcut Model.first(*args). + # * Find last - This will return the last record matched by the options used. These options can either be specific + # conditions or merely an order. If no record can be matched, +nil+ is returned. Use + # Model.find(:last, *args) or its shortcut Model.last(*args). + # * Find all - This will return all the records matched by the options used. + # If no records are found, an empty array is returned. Use + # Model.find(:all, *args) or its shortcut Model.all(*args). + # + # All approaches accept an options hash as their last parameter. + # + # ==== Parameters + # + # * :conditions - An SQL fragment like "administrator = 1", [ "user_name = ?", username ], or ["user_name = :user_name", { :user_name => user_name }]. See conditions in the intro. + # * :order - An SQL fragment like "created_at DESC, name". + # * :group - An attribute name by which the result should be grouped. Uses the GROUP BY SQL-clause. + # * :having - Combined with +:group+ this can be used to filter the records that a GROUP BY returns. Uses the HAVING SQL-clause. + # * :limit - An integer determining the limit on the number of rows that should be returned. + # * :offset - An integer determining the offset from where the rows should be fetched. So at 5, it would skip rows 0 through 4. + # * :joins - Either an SQL fragment for additional joins like "LEFT JOIN comments ON comments.post_id = id" (rarely needed), + # named associations in the same form used for the :include option, which will perform an INNER JOIN on the associated table(s), + # or an array containing a mixture of both strings and named associations. + # If the value is a string, then the records will be returned read-only since they will have attributes that do not correspond to the table's columns. + # Pass :readonly => false to override. + # * :include - Names associations that should be loaded alongside. The symbols named refer + # to already defined associations. See eager loading under Associations. + # * :select - By default, this is "*" as in "SELECT * FROM", but can be changed if you, for example, want to do a join but not + # include the joined columns. Takes a string with the SELECT SQL fragment (e.g. "id, name"). + # * :from - By default, this is the table name of the class, but can be changed to an alternate table name (or even the name + # of a database view). + # * :readonly - Mark the returned records read-only so they cannot be saved or updated. + # * :lock - An SQL fragment like "FOR UPDATE" or "LOCK IN SHARE MODE". + # :lock => true gives connection's default exclusive lock, usually "FOR UPDATE". + # + # ==== Examples + # + # # find by id + # Person.find(1) # returns the object for ID = 1 + # Person.find(1, 2, 6) # returns an array for objects with IDs in (1, 2, 6) + # Person.find([7, 17]) # returns an array for objects with IDs in (7, 17) + # Person.find([1]) # returns an array for the object with ID = 1 + # Person.find(1, :conditions => "administrator = 1", :order => "created_on DESC") + # + # Note that returned records may not be in the same order as the ids you + # provide since database rows are unordered. Give an explicit :order + # to ensure the results are sorted. + # + # ==== Examples + # + # # find first + # Person.find(:first) # returns the first object fetched by SELECT * FROM people + # Person.find(:first, :conditions => [ "user_name = ?", user_name]) + # Person.find(:first, :conditions => [ "user_name = :u", { :u => user_name }]) + # Person.find(:first, :order => "created_on DESC", :offset => 5) + # + # # find last + # Person.find(:last) # returns the last object fetched by SELECT * FROM people + # Person.find(:last, :conditions => [ "user_name = ?", user_name]) + # Person.find(:last, :order => "created_on DESC", :offset => 5) + # + # # find all + # Person.find(:all) # returns an array of objects for all the rows fetched by SELECT * FROM people + # Person.find(:all, :conditions => [ "category IN (?)", categories], :limit => 50) + # Person.find(:all, :conditions => { :friends => ["Bob", "Steve", "Fred"] } + # Person.find(:all, :offset => 10, :limit => 10) + # Person.find(:all, :include => [ :account, :friends ]) + # Person.find(:all, :group => "category") + # + # Example for find with a lock: Imagine two concurrent transactions: + # each will read person.visits == 2, add 1 to it, and save, resulting + # in two saves of person.visits = 3. By locking the row, the second + # transaction has to wait until the first is finished; we get the + # expected person.visits == 4. + # + # Person.transaction do + # person = Person.find(1, :lock => true) + # person.visits += 1 + # person.save! + # end + def find(*args, &block) return to_a.find(&block) if block_given? - expects_array = ids.first.kind_of?(Array) - return ids.first if expects_array && ids.first.empty? + options = args.extract_options! - ids = ids.flatten.compact.uniq - - case ids.size - when 0 - raise RecordNotFound, "Couldn't find #{@klass.name} without an ID" - when 1 - result = find_one(ids.first) - expects_array ? [ result ] : result + if options.present? + apply_finder_options(options).find(*args) else - find_some(ids) + case args.first + when :first, :last, :all + send(args.first) + else + find_with_ids(*args) + end end end + # A convenience wrapper for find(:first, *args). You can pass in all the + # same arguments to this method as you can to find(:first). + def first(*args) + args.any? ? apply_finder_options(args.first).first : find_first + end + + # A convenience wrapper for find(:last, *args). You can pass in all the + # same arguments to this method as you can to find(:last). + def last(*args) + args.any? ? apply_finder_options(args.first).last : find_last + end + + # A convenience wrapper for find(:all, *args). You can pass in all the + # same arguments to this method as you can to find(:all). + def all(*args) + args.any? ? apply_finder_options(args.first).to_a : to_a + end + def exists?(id = nil) relation = select(primary_key).limit(1) relation = relation.where(primary_key.eq(id)) if id relation.first ? true : false end - def first - if loaded? - @records.first - else - @first ||= limit(1).to_a[0] - end - end - - def last - if loaded? - @records.last - else - @last ||= reverse_order.limit(1).to_a[0] - end - end - protected def find_with_associations @@ -124,6 +207,25 @@ module ActiveRecord record end + def find_with_ids(*ids, &block) + return to_a.find(&block) if block_given? + + expects_array = ids.first.kind_of?(Array) + return ids.first if expects_array && ids.first.empty? + + ids = ids.flatten.compact.uniq + + case ids.size + when 0 + raise RecordNotFound, "Couldn't find #{@klass.name} without an ID" + when 1 + result = find_one(ids.first) + expects_array ? [ result ] : result + else + find_some(ids) + end + end + def find_one(id) record = where(primary_key.eq(id)).first @@ -163,5 +265,21 @@ module ActiveRecord end end + def find_first + if loaded? + @records.first + else + @first ||= limit(1).to_a[0] + end + end + + def find_last + if loaded? + @records.last + else + @last ||= reverse_order.limit(1).to_a[0] + end + end + end end diff --git a/activerecord/lib/active_record/relation/spawn_methods.rb b/activerecord/lib/active_record/relation/spawn_methods.rb index d5b13c6100..1577a9b116 100644 --- a/activerecord/lib/active_record/relation/spawn_methods.rb +++ b/activerecord/lib/active_record/relation/spawn_methods.rb @@ -98,19 +98,12 @@ module ActiveRecord options.assert_valid_keys(VALID_FIND_OPTIONS) - relation = relation.joins(options[:joins]). - where(options[:conditions]). - select(options[:select]). - group(options[:group]). - having(options[:having]). - order(options[:order]). - limit(options[:limit]). - offset(options[:offset]). - from(options[:from]). - includes(options[:include]) + [:joins, :select, :group, :having, :order, :limit, :offset, :from, :lock, :readonly].each do |finder| + relation = relation.send(finder, options[finder]) if options.has_key?(finder) + end - relation = relation.lock(options[:lock]) if options[:lock].present? - relation = relation.readonly(options[:readonly]) if options.has_key?(:readonly) + relation = relation.where(options[:conditions]) if options.has_key?(:conditions) + relation = relation.includes(options[:include]) if options.has_key?(:include) relation end From 565b4cd3e08c148a6c8f2e80c28029c6a8a51e6e Mon Sep 17 00:00:00 2001 From: Pratik Naik Date: Wed, 20 Jan 2010 03:40:37 +0530 Subject: [PATCH 23/29] Scope#find is no longer needed now that Relation#find handles all the cases --- activerecord/lib/active_record/named_scope.rb | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/activerecord/lib/active_record/named_scope.rb b/activerecord/lib/active_record/named_scope.rb index 2f3044d06b..d606934dce 100644 --- a/activerecord/lib/active_record/named_scope.rb +++ b/activerecord/lib/active_record/named_scope.rb @@ -148,18 +148,6 @@ module ActiveRecord relation end - def find(*args) - options = args.extract_options! - relation = options.present? ? apply_finder_options(options) : self - - case args.first - when :first, :last, :all - relation.send(args.first) - else - options.present? ? relation.find(*args) : super - end - end - def first(*args) if args.first.kind_of?(Integer) || (loaded? && !args.first.kind_of?(Hash)) to_a.first(*args) From a5d06d05fbdc0b66d8ba64a7982288a54b31865b Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Tue, 19 Jan 2010 22:25:10 -0600 Subject: [PATCH 24/29] Cleanup middleware introspection output --- actionpack/lib/action_dispatch/middleware/stack.rb | 4 +--- railties/builtin/rails_info/rails/info.rb | 2 +- railties/lib/rails/tasks/middleware.rake | 2 +- 3 files changed, 3 insertions(+), 5 deletions(-) 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) diff --git a/railties/builtin/rails_info/rails/info.rb b/railties/builtin/rails_info/rails/info.rb index 269fe488b0..90c9015fcf 100644 --- a/railties/builtin/rails_info/rails/info.rb +++ b/railties/builtin/rails_info/rails/info.rb @@ -114,7 +114,7 @@ module Rails end property 'Middleware' do - Rails.configuration.middleware.active.map { |middle| middle.inspect } + Rails.configuration.middleware.active.map(&:inspect) end # The Rails Git revision, if it's checked out into vendor/rails. diff --git a/railties/lib/rails/tasks/middleware.rake b/railties/lib/rails/tasks/middleware.rake index e1ab309157..5a5bd7a7e9 100644 --- a/railties/lib/rails/tasks/middleware.rake +++ b/railties/lib/rails/tasks/middleware.rake @@ -3,5 +3,5 @@ task :middleware => :environment do Rails.configuration.middleware.active.each do |middleware| puts "use #{middleware.inspect}" end - puts "run ActionController::Routing::Routes" + puts "run #{Rails.application.class.name}" end From 5ebfa6242726bd186452640ed5704f2adc1a5007 Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Tue, 19 Jan 2010 22:51:41 -0600 Subject: [PATCH 25/29] 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(-) 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 From 1a50d2e66a80c910fe1e2203eb2c993e5dbc4e5b Mon Sep 17 00:00:00 2001 From: Yehuda Katz Date: Tue, 19 Jan 2010 22:35:09 -0800 Subject: [PATCH 26/29] Stop overriding LoadError.new to return a MissingSourceError (and sometimes nil!) --- .../lib/action_controller/metal/helpers.rb | 2 +- .../test/controller/new_base/base_test.rb | 2 +- activemodel/lib/active_model/lint.rb | 2 + .../lib/active_support/core_ext/load_error.rb | 50 +++++++------------ .../lib/active_support/dependencies.rb | 2 +- .../test/core_ext/load_error_test.rb | 15 ++++++ 6 files changed, 38 insertions(+), 35 deletions(-) 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 diff --git a/activemodel/lib/active_model/lint.rb b/activemodel/lib/active_model/lint.rb index 0be82aa180..1330bf7042 100644 --- a/activemodel/lib/active_model/lint.rb +++ b/activemodel/lib/active_model/lint.rb @@ -53,6 +53,8 @@ module ActiveModel assert_kind_of String, model_name assert_kind_of String, model_name.human assert_kind_of String, model_name.partial_path + assert_kind_of String, model_name.singular + assert_kind_of String, model_name.plural end # errors diff --git a/activesupport/lib/active_support/core_ext/load_error.rb b/activesupport/lib/active_support/core_ext/load_error.rb index cc6287b100..615ebe9588 100644 --- a/activesupport/lib/active_support/core_ext/load_error.rb +++ b/activesupport/lib/active_support/core_ext/load_error.rb @@ -1,36 +1,22 @@ -class MissingSourceFile < LoadError #:nodoc: - attr_reader :path - def initialize(message, path) - super(message) - @path = path - end - - def is_missing?(path) - path.gsub(/\.rb$/, '') == self.path.gsub(/\.rb$/, '') - end - - def self.from_message(message) - REGEXPS.each do |regexp, capture| - match = regexp.match(message) - return MissingSourceFile.new(message, match[capture]) unless match.nil? - end - nil - end - - REGEXPS = [ - [/^no such file to load -- (.+)$/i, 1], - [/^Missing \w+ (file\s*)?([^\s]+.rb)$/i, 2], - [/^Missing API definition file in (.+)$/i, 1], - [/win32/, 0] - ] unless defined?(REGEXPS) -end - class LoadError - def self.new(*args) - if self == LoadError - MissingSourceFile.from_message(args.first) - else - super + REGEXPS = [ + /^no such file to load -- (.+)$/i, + /^Missing \w+ (?:file\s*)?([^\s]+.rb)$/i, + /^Missing API definition file in (.+)$/i, + ] + + def path + @path ||= begin + REGEXPS.find do |regex| + message =~ regex + end + $1 end end + + def is_missing?(location) + location.sub(/\.rb$/, '') == path.sub(/\.rb$/, '') + end end + +MissingSourceFile = LoadError \ No newline at end of file diff --git a/activesupport/lib/active_support/dependencies.rb b/activesupport/lib/active_support/dependencies.rb index e858bcdc80..8ded9f8b2d 100644 --- a/activesupport/lib/active_support/dependencies.rb +++ b/activesupport/lib/active_support/dependencies.rb @@ -236,7 +236,7 @@ module ActiveSupport #:nodoc: rescue LoadError => load_error unless swallow_load_errors if file_name = load_error.message[/ -- (.*?)(\.rb)?$/, 1] - raise MissingSourceFile.new(message % file_name, load_error.path).copy_blame!(load_error) + raise LoadError.new(message % file_name).copy_blame!(load_error) end raise end diff --git a/activesupport/test/core_ext/load_error_test.rb b/activesupport/test/core_ext/load_error_test.rb index b775b65f9f..d7b8f602ca 100644 --- a/activesupport/test/core_ext/load_error_test.rb +++ b/activesupport/test/core_ext/load_error_test.rb @@ -15,3 +15,18 @@ class TestMissingSourceFile < Test::Unit::TestCase end end end + +class TestLoadError < Test::Unit::TestCase + def test_with_require + assert_raise(LoadError) { require 'no_this_file_don\'t_exist' } + end + def test_with_load + assert_raise(LoadError) { load 'nor_does_this_one' } + end + def test_path + begin load 'nor/this/one.rb' + rescue LoadError => e + assert_equal 'nor/this/one.rb', e.path + end + end +end \ No newline at end of file From 394c05ed82c1fbfc1c5d27f223b49975f439729c Mon Sep 17 00:00:00 2001 From: Pratik Naik Date: Wed, 20 Jan 2010 13:24:37 +0530 Subject: [PATCH 27/29] Remove stale methods constructing joins --- activerecord/lib/active_record/base.rb | 28 -------------------------- 1 file changed, 28 deletions(-) diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb index 6063c9789f..8fcc872113 100755 --- a/activerecord/lib/active_record/base.rb +++ b/activerecord/lib/active_record/base.rb @@ -1455,34 +1455,6 @@ module ActiveRecord #:nodoc: relation end - def construct_join(joins) - case joins - when Symbol, Hash, Array - if array_of_strings?(joins) - joins.join(' ') + " " - else - build_association_joins(joins) - end - when String - " #{joins} " - else - "" - end - end - - def build_association_joins(joins) - join_dependency = ActiveRecord::Associations::ClassMethods::JoinDependency.new(self, joins, nil) - relation = unscoped.table - join_dependency.join_associations.map { |association| - if (association_relation = association.relation).is_a?(Array) - [Arel::InnerJoin.new(relation, association_relation.first, *association.association_join.first).joins(relation), - Arel::InnerJoin.new(relation, association_relation.last, *association.association_join.last).joins(relation)].join() - else - Arel::InnerJoin.new(relation, association_relation, *association.association_join).joins(relation) - end - }.join(" ") - end - def array_of_strings?(o) o.is_a?(Array) && o.all?{|obj| obj.is_a?(String)} end From 5502780c6910fbf6825efa58601d868fca2f1cc1 Mon Sep 17 00:00:00 2001 From: Pratik Naik Date: Wed, 20 Jan 2010 14:01:42 +0530 Subject: [PATCH 28/29] Move array_of_strings? to Relation --- activerecord/lib/active_record/base.rb | 4 ---- activerecord/lib/active_record/relation/query_methods.rb | 8 ++++++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb index 8fcc872113..8f2ea10206 100755 --- a/activerecord/lib/active_record/base.rb +++ b/activerecord/lib/active_record/base.rb @@ -1455,10 +1455,6 @@ module ActiveRecord #:nodoc: relation end - def array_of_strings?(o) - o.is_a?(Array) && o.all?{|obj| obj.is_a?(String)} - end - def type_condition sti_column = arel_table[inheritance_column] condition = sti_column.eq(sti_name) diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb index a3ac58bc81..163d698b5c 100644 --- a/activerecord/lib/active_record/relation/query_methods.rb +++ b/activerecord/lib/active_record/relation/query_methods.rb @@ -77,7 +77,7 @@ module ActiveRecord # Build association joins first joins.each do |join| - association_joins << join if [Hash, Array, Symbol].include?(join.class) && !@klass.send(:array_of_strings?, join) + association_joins << join if [Hash, Array, Symbol].include?(join.class) && !array_of_strings?(join) end if association_joins.any? @@ -110,7 +110,7 @@ module ActiveRecord when Relation::JoinOperation arel = arel.join(join.relation, join.join_class).on(*join.on) when Hash, Array, Symbol - if @klass.send(:array_of_strings?, join) + if array_of_strings?(join) join_string = join.join(' ') arel = arel.join(join_string) end @@ -193,5 +193,9 @@ module ActiveRecord }.join(',') end + def array_of_strings?(o) + o.is_a?(Array) && o.all?{|obj| obj.is_a?(String)} + end + end end From 8a1be228491f433fa8d20be4f485e2159f5ebe59 Mon Sep 17 00:00:00 2001 From: Pratik Naik Date: Wed, 20 Jan 2010 16:11:14 +0530 Subject: [PATCH 29/29] Use unscoped instead of with_exclusive_scope for preloading --- .../lib/active_record/association_preload.rb | 27 +++++++++---------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/activerecord/lib/active_record/association_preload.rb b/activerecord/lib/active_record/association_preload.rb index a43c4d09d6..a5b06460fe 100644 --- a/activerecord/lib/active_record/association_preload.rb +++ b/activerecord/lib/active_record/association_preload.rb @@ -187,13 +187,12 @@ module ActiveRecord conditions = "t0.#{reflection.primary_key_name} #{in_or_equals_for_ids(ids)}" conditions << append_conditions(reflection, preload_options) - associated_records = reflection.klass.with_exclusive_scope do - reflection.klass.where([conditions, ids]). + associated_records = reflection.klass.unscoped.where([conditions, ids]). includes(options[:include]). joins("INNER JOIN #{connection.quote_table_name options[:join_table]} t0 ON #{reflection.klass.quoted_table_name}.#{reflection.klass.primary_key} = t0.#{reflection.association_foreign_key}"). select("#{options[:select] || table_name+'.*'}, t0.#{reflection.primary_key_name} as the_parent_record_id"). order(options[:order]).to_a - end + set_association_collection_records(id_to_record_map, reflection.name, associated_records, 'the_parent_record_id') end @@ -341,9 +340,7 @@ module ActiveRecord conditions = "#{table_name}.#{connection.quote_column_name(primary_key)} #{in_or_equals_for_ids(ids)}" conditions << append_conditions(reflection, preload_options) - associated_records = klass.with_exclusive_scope do - klass.where([conditions, ids]).apply_finder_options(options.slice(:include, :select, :joins, :order)).to_a - end + associated_records = klass.unscoped.where([conditions, ids]).apply_finder_options(options.slice(:include, :select, :joins, :order)).to_a set_association_single_records(id_map, reflection.name, associated_records, primary_key) end @@ -362,14 +359,16 @@ module ActiveRecord conditions << append_conditions(reflection, preload_options) - reflection.klass.with_exclusive_scope do - reflection.klass.select(preload_options[:select] || options[:select] || "#{table_name}.*"). - includes(preload_options[:include] || options[:include]). - where([conditions, ids]). - joins(options[:joins]). - group(preload_options[:group] || options[:group]). - order(preload_options[:order] || options[:order]) - end + find_options = { + :select => preload_options[:select] || options[:select] || "#{table_name}.*", + :include => preload_options[:include] || options[:include], + :conditions => [conditions, ids], + :joins => options[:joins], + :group => preload_options[:group] || options[:group], + :order => preload_options[:order] || options[:order] + } + + reflection.klass.unscoped.apply_finder_options(find_options).to_a end