diff options
25 files changed, 578 insertions, 88 deletions
diff --git a/actionpack/CHANGELOG b/actionpack/CHANGELOG index 7376f5ea0a..5ef2283838 100644 --- a/actionpack/CHANGELOG +++ b/actionpack/CHANGELOG @@ -1,5 +1,11 @@ *SVN* +* Fix problems with pagination and :include. [Kevin Clark] + +* Add ActiveRecordTestCase for testing AR integration. [Kevin Clark] + +* Add Unit Tests for pagination [Kevin Clark] + * Add :html option for specifying form tag options in form_for. [Sam Stephenson] * Replace dubious controller parent class in filter docs. #3655, #3722 [info@rhalff.com, eigentone@gmail.com] diff --git a/actionpack/Rakefile b/actionpack/Rakefile index 8e9ed0893b..a1fd6a6064 100755 --- a/actionpack/Rakefile +++ b/actionpack/Rakefile @@ -31,6 +31,13 @@ Rake::TestTask.new { |t| t.verbose = true } +desc 'ActiveRecord Integration Tests' +Rake::TestTask.new(:test_active_record_integration) do |t| + t.libs << "test" + t.test_files = Dir.glob("test/activerecord/*_test.rb") + t.verbose = true +end + # Genereate the RDoc documentation diff --git a/actionpack/lib/action_controller/pagination.rb b/actionpack/lib/action_controller/pagination.rb index 7e4b4754f2..2f1a1d985c 100644 --- a/actionpack/lib/action_controller/pagination.rb +++ b/actionpack/lib/action_controller/pagination.rb @@ -163,10 +163,11 @@ module ActionController # Returns the total number of items in the collection to be paginated for # the +model+ and given +conditions+. Override this method to implement a # custom counter. - def count_collection_for_pagination(model, conditions, joins) - model.count(conditions,joins) + def count_collection_for_pagination(model, options) + model.count(:conditions => options[:conditions], + :joins => options[:join] || options[:joins], :include => options[:include]) end - + # Returns a collection of items for the given +model+ and +options[conditions]+, # ordered by +options[order]+, for the current page in the given +paginator+. # Override this method to implement a custom finder. @@ -185,12 +186,10 @@ module ActionController def paginator_and_collection_for(collection_id, options) #:nodoc: klass = options[:class_name].constantize page = @params[options[:parameter]] - count = count_collection_for_pagination(klass, options[:conditions], - options[:join] || options[:joins]) - + count = count_collection_for_pagination(klass, options) paginator = Paginator.new(self, count, options[:per_page], page) collection = find_collection_for_pagination(klass, options, paginator) - + return paginator, collection end diff --git a/actionpack/test/active_record_unit.rb b/actionpack/test/active_record_unit.rb new file mode 100644 index 0000000000..016f331d77 --- /dev/null +++ b/actionpack/test/active_record_unit.rb @@ -0,0 +1,88 @@ +require File.dirname(__FILE__) + '/abstract_unit' + +# Define the essentials +class ActiveRecordTestConnector + cattr_accessor :able_to_connect + cattr_accessor :connected + + # Set our defaults + self.connected = false + self.able_to_connect = true +end + +# Try to grab AR +begin + PATH_TO_AR = File.dirname(__FILE__) + '/../../activerecord' + require "#{PATH_TO_AR}/lib/active_record" unless Object.const_defined?(:ActiveRecord) + require "#{PATH_TO_AR}/lib/active_record/fixtures" unless Object.const_defined?(:Fixtures) +rescue Object => e + $stderr.puts "\nSkipping ActiveRecord assertion tests: #{e}" + ActiveRecordTestConnector.able_to_connect = false +end + +# Define the rest of the connector +class ActiveRecordTestConnector + def self.setup + unless self.connected || !self.able_to_connect + setup_connection + load_schema + self.connected = true + end + rescue Object => e + $stderr.puts "\nSkipping ActiveRecord assertion tests: #{e}" + #$stderr.puts " #{e.backtrace.join("\n ")}\n" + self.able_to_connect = false + end + + private + + def self.setup_connection + if Object.const_defined?(:ActiveRecord) + + begin + ActiveRecord::Base.establish_connection(:adapter => 'sqlite3', :dbfile => ':memory:') + ActiveRecord::Base.connection + rescue Object + $stderr.puts 'SQLite 3 unavailable; falling to SQLite 2.' + ActiveRecord::Base.establish_connection(:adapter => 'sqlite', :dbfile => ':memory:') + ActiveRecord::Base.connection + end + + Object.send(:const_set, :QUOTED_TYPE, ActiveRecord::Base.connection.quote_column_name('type')) unless Object.const_defined?(:QUOTED_TYPE) + else + raise "Couldn't locate ActiveRecord." + end + end + + # Load actionpack sqlite tables + def self.load_schema + File.read(File.dirname(__FILE__) + "/fixtures/db_definitions/sqlite.sql").split(';').each do |sql| + ActiveRecord::Base.connection.execute(sql) unless sql.blank? + end + end +end + +# Test case for inheiritance +class ActiveRecordTestCase < Test::Unit::TestCase + # Set our fixture path + self.fixture_path = "#{File.dirname(__FILE__)}/fixtures/" + + def setup + abort_tests unless ActiveRecordTestConnector.connected = true + end + + # Default so Test::Unit::TestCase doesn't complain + def test_truth + end + + private + + # If things go wrong, we don't want to run our test cases. We'll just define them to test nothing. + def abort_tests + self.class.public_instance_methods.grep(/^test./).each do |method| + self.class.class_eval { define_method(method.to_sym){} } + end + end +end + +ActiveRecordTestConnector.setup
\ No newline at end of file diff --git a/actionpack/test/controller/active_record_assertions_test.rb b/actionpack/test/activerecord/active_record_assertions_test.rb index b8b6309295..f5661d19cc 100644 --- a/actionpack/test/controller/active_record_assertions_test.rb +++ b/actionpack/test/activerecord/active_record_assertions_test.rb @@ -1,45 +1,6 @@ -require "#{File.dirname(__FILE__)}/../abstract_unit" - -# Unfurl the safety net. -path_to_ar = File.dirname(__FILE__) + '/../../../activerecord' -if Object.const_defined?(:ActiveRecord) || File.exist?(path_to_ar) - begin - -# These tests require Active Record, so you're going to need AR in a -# sibling directory to AP and have SQLite installed. - -unless Object.const_defined?(:ActiveRecord) - require "#{path_to_ar}/lib/active_record" -end - -begin - ActiveRecord::Base.establish_connection(:adapter => 'sqlite3', :dbfile => ':memory:') - ActiveRecord::Base.connection -rescue Object - $stderr.puts 'SQLite 3 unavailable; falling to SQLite 2.' - ActiveRecord::Base.establish_connection(:adapter => 'sqlite', :dbfile => ':memory:') - ActiveRecord::Base.connection -end - -# Set up company fixtures. -$LOAD_PATH << "#{path_to_ar}/test" -QUOTED_TYPE = ActiveRecord::Base.connection.quote_column_name('type') unless Object.const_defined?(:QUOTED_TYPE) +require "#{File.dirname(__FILE__)}/../active_record_unit" require 'fixtures/company' -File.read("#{path_to_ar}/test/fixtures/db_definitions/sqlite.sql").split(';').each do |sql| - ActiveRecord::Base.connection.execute(sql) unless sql.blank? -end - -# Add some validation rules to trip up the assertions. -class Company - protected - def validate - errors.add_on_empty('name') - errors.add('rating', 'rating should not be 2') if rating == 2 - errors.add_to_base('oh oh') if rating == 3 - end -end -# A controller to host the assertions. class ActiveRecordAssertionsController < ActionController::Base self.template_root = "#{File.dirname(__FILE__)}/../fixtures/" @@ -76,12 +37,15 @@ class ActiveRecordAssertionsController < ActionController::Base # the safety dance...... def rescue_action(e) raise; end end - -class ActiveRecordAssertionsControllerTest < Test::Unit::TestCase + +class ActiveRecordAssertionsControllerTest < ActiveRecordTestCase + fixtures :companies + def setup @request = ActionController::TestRequest.new @response = ActionController::TestResponse.new @controller = ActiveRecordAssertionsController.new + super end # test for 1 bad apple column @@ -117,11 +81,4 @@ class ActiveRecordAssertionsControllerTest < Test::Unit::TestCase assert_success assert_invalid_record 'company' end -end - -# End of safety net. - rescue Object => e - $stderr.puts "Skipping Active Record assertion tests: #{e}" - #$stderr.puts " #{e.backtrace.join("\n ")}" - end -end +end
\ No newline at end of file diff --git a/actionpack/test/activerecord/pagination_test.rb b/actionpack/test/activerecord/pagination_test.rb new file mode 100644 index 0000000000..00ae78614f --- /dev/null +++ b/actionpack/test/activerecord/pagination_test.rb @@ -0,0 +1,146 @@ +require File.dirname(__FILE__) + '/../active_record_unit' + +require 'fixtures/topic' +require 'fixtures/reply' +require 'fixtures/developer' +require 'fixtures/project' + +class PaginationTest < ActiveRecordTestCase + fixtures :topics, :replies, :developers, :projects, :developers_projects + + class PaginationController < ActionController::Base + self.template_root = "#{File.dirname(__FILE__)}/../fixtures/" + + def simple_paginate + @topic_pages, @topics = paginate(:topics) + render :nothing => true + end + + def paginate_with_per_page + @topic_pages, @topics = paginate(:topics, :per_page => 1) + render :nothing => true + end + + def paginate_with_order + @topic_pages, @topics = paginate(:topics, :order => 'created_at asc') + render :nothing => true + end + + def paginate_with_order_by + @topic_pages, @topics = paginate(:topics, :order_by => 'created_at asc') + render :nothing => true + end + + def paginate_with_include_and_order + @topic_pages, @topics = paginate(:topics, :include => :replies, :order => 'replies.created_at asc, topics.created_at asc') + render :nothing => true + end + + def paginate_with_conditions + @topic_pages, @topics = paginate(:topics, :conditions => ["created_at > ?", 30.minutes.ago]) + render :nothing => true + end + + def paginate_with_class_name + @developer_pages, @developers = paginate(:developers, :class_name => "DeVeLoPeR") + render :nothing => true + end + + def paginate_with_singular_name + @developer_pages, @developers = paginate() + render :nothing => true + end + + def paginate_with_joins + @developer_pages, @developers = paginate(:developers, + :joins => 'LEFT JOIN developers_projects ON developers.id = developers_projects.developer_id', + :conditions => 'project_id=1') + render :nothing => true + end + + def paginate_with_join + @developer_pages, @developers = paginate(:developers, + :join => 'LEFT JOIN developers_projects ON developers.id = developers_projects.developer_id', + :conditions => 'project_id=1') + render :nothing => true + end + + def rescue_errors(e) raise e end + + def rescue_action(e) raise end + + end + + def setup + @controller = PaginationController.new + @request = ActionController::TestRequest.new + @response = ActionController::TestResponse.new + super + end + + # Single Action Pagination Tests + + def test_simple_paginate + get :simple_paginate + assert_equal 1, assigns(:topic_pages).page_count + assert_equal 3, assigns(:topics).size + end + + def test_paginate_with_per_page + get :paginate_with_per_page + assert_equal 1, assigns(:topics).size + assert_equal 3, assigns(:topic_pages).page_count + end + + def test_paginate_with_order + get :paginate_with_order + expected = [topics(:futurama), + topics(:harvey_birdman), + topics(:rails)] + assert_equal expected, assigns(:topics) + assert_equal 1, assigns(:topic_pages).page_count + end + + def test_paginate_with_order_by + get :paginate_with_order + expected = assigns(:topics) + get :paginate_with_order_by + assert_equal expected, assigns(:topics) + assert_equal 1, assigns(:topic_pages).page_count + end + + def test_paginate_with_conditions + get :paginate_with_conditions + expected = [topics(:rails)] + assert_equal expected, assigns(:topics) + assert_equal 1, assigns(:topic_pages).page_count + end + + def test_paginate_with_class_name + get :paginate_with_class_name + + assert assigns(:developers).size > 0 + assert_equal DeVeLoPeR, assigns(:developers).first.class + end + + def test_paginate_with_joins + get :paginate_with_joins + assert_equal 2, assigns(:developers).size + developer_names = assigns(:developers).map { |d| d.name } + assert developer_names.include?('David') + assert developer_names.include?('Jamis') + end + + def test_paginate_with_join_and_conditions + get :paginate_with_joins + expected = assigns(:topics) + get :paginate_with_join + assert_equal expected, assigns(:topics) + end + + def test_paginate_with_include_and_order + get :paginate_with_include_and_order + expected = Topic.find(:all, :include => 'replies', :order => 'replies.created_at asc, topics.created_at asc', :limit => 10) + assert_equal expected, assigns(:topics) + end +end diff --git a/actionpack/test/fixtures/companies.yml b/actionpack/test/fixtures/companies.yml new file mode 100644 index 0000000000..707f72abc6 --- /dev/null +++ b/actionpack/test/fixtures/companies.yml @@ -0,0 +1,24 @@ +thirty_seven_signals: + id: 1 + name: 37Signals + rating: 4 + +TextDrive: + id: 2 + name: TextDrive + rating: 4 + +PlanetArgon: + id: 3 + name: Planet Argon + rating: 4 + +Google: + id: 4 + name: Google + rating: 4 + +Ionist: + id: 5 + name: Ioni.st + rating: 4
\ No newline at end of file diff --git a/actionpack/test/fixtures/company.rb b/actionpack/test/fixtures/company.rb new file mode 100644 index 0000000000..0d1c29b906 --- /dev/null +++ b/actionpack/test/fixtures/company.rb @@ -0,0 +1,9 @@ +class Company < ActiveRecord::Base + attr_protected :rating + set_sequence_name :companies_nonstd_seq + + validates_presence_of :name + def validate + errors.add('rating', 'rating should not be 2') if rating == 2 + end +end
\ No newline at end of file diff --git a/actionpack/test/fixtures/db_definitions/sqlite.sql b/actionpack/test/fixtures/db_definitions/sqlite.sql new file mode 100644 index 0000000000..b4e7539d16 --- /dev/null +++ b/actionpack/test/fixtures/db_definitions/sqlite.sql @@ -0,0 +1,42 @@ +CREATE TABLE 'companies' ( + 'id' INTEGER PRIMARY KEY NOT NULL, + 'name' TEXT DEFAULT NULL, + 'rating' INTEGER DEFAULT 1 +); + +CREATE TABLE 'replies' ( + 'id' INTEGER PRIMARY KEY NOT NULL, + 'content' text, + 'created_at' datetime, + 'updated_at' datetime, + 'topic_id' integer +); + +CREATE TABLE 'topics' ( + 'id' INTEGER PRIMARY KEY NOT NULL, + 'title' varchar(255), + 'subtitle' varchar(255), + 'content' text, + 'created_at' datetime, + 'updated_at' datetime +); + +CREATE TABLE 'developers' ( + 'id' INTEGER PRIMARY KEY NOT NULL, + 'name' TEXT DEFAULT NULL, + 'salary' INTEGER DEFAULT 70000, + 'created_at' DATETIME DEFAULT NULL, + 'updated_at' DATETIME DEFAULT NULL +); + +CREATE TABLE 'projects' ( + 'id' INTEGER PRIMARY KEY NOT NULL, + 'name' TEXT DEFAULT NULL +); + +CREATE TABLE 'developers_projects' ( + 'developer_id' INTEGER NOT NULL, + 'project_id' INTEGER NOT NULL, + 'joined_on' DATE DEFAULT NULL, + 'access_level' INTEGER DEFAULT 1 +); diff --git a/actionpack/test/fixtures/developer.rb b/actionpack/test/fixtures/developer.rb new file mode 100644 index 0000000000..f5e5b901fd --- /dev/null +++ b/actionpack/test/fixtures/developer.rb @@ -0,0 +1,7 @@ +class Developer < ActiveRecord::Base + has_and_belongs_to_many :projects +end + +class DeVeLoPeR < ActiveRecord::Base + set_table_name "developers" +end diff --git a/actionpack/test/fixtures/developers.yml b/actionpack/test/fixtures/developers.yml new file mode 100644 index 0000000000..308bf75de2 --- /dev/null +++ b/actionpack/test/fixtures/developers.yml @@ -0,0 +1,21 @@ +david: + id: 1 + name: David + salary: 80000 + +jamis: + id: 2 + name: Jamis + salary: 150000 + +<% for digit in 3..10 %> +dev_<%= digit %>: + id: <%= digit %> + name: fixture_<%= digit %> + salary: 100000 +<% end %> + +poor_jamis: + id: 11 + name: Jamis + salary: 9000
\ No newline at end of file diff --git a/actionpack/test/fixtures/developers_projects.yml b/actionpack/test/fixtures/developers_projects.yml new file mode 100644 index 0000000000..cee359c7cf --- /dev/null +++ b/actionpack/test/fixtures/developers_projects.yml @@ -0,0 +1,13 @@ +david_action_controller: + developer_id: 1 + project_id: 2 + joined_on: 2004-10-10 + +david_active_record: + developer_id: 1 + project_id: 1 + joined_on: 2004-10-10 + +jamis_active_record: + developer_id: 2 + project_id: 1
\ No newline at end of file diff --git a/actionpack/test/fixtures/project.rb b/actionpack/test/fixtures/project.rb new file mode 100644 index 0000000000..2b53d39ed5 --- /dev/null +++ b/actionpack/test/fixtures/project.rb @@ -0,0 +1,3 @@ +class Project < ActiveRecord::Base + has_and_belongs_to_many :developers, :uniq => true +end diff --git a/actionpack/test/fixtures/projects.yml b/actionpack/test/fixtures/projects.yml new file mode 100644 index 0000000000..02800c7824 --- /dev/null +++ b/actionpack/test/fixtures/projects.yml @@ -0,0 +1,7 @@ +action_controller: + id: 2 + name: Active Controller + +active_record: + id: 1 + name: Active Record diff --git a/actionpack/test/fixtures/replies.yml b/actionpack/test/fixtures/replies.yml new file mode 100644 index 0000000000..284c9c0796 --- /dev/null +++ b/actionpack/test/fixtures/replies.yml @@ -0,0 +1,13 @@ +witty_retort: + id: 1 + topic_id: 1 + content: Birdman is better! + created_at: <%= 6.hours.ago.to_s(:db) %> + updated_at: nil + +another: + id: 2 + topic_id: 2 + content: Nuh uh! + created_at: <%= 1.hour.ago.to_s(:db) %> + updated_at: nil
\ No newline at end of file diff --git a/actionpack/test/fixtures/reply.rb b/actionpack/test/fixtures/reply.rb new file mode 100644 index 0000000000..ea84042b9a --- /dev/null +++ b/actionpack/test/fixtures/reply.rb @@ -0,0 +1,5 @@ +class Reply < ActiveRecord::Base + belongs_to :topic, :include => [:replies] + + validates_presence_of :content +end diff --git a/actionpack/test/fixtures/topic.rb b/actionpack/test/fixtures/topic.rb new file mode 100644 index 0000000000..0929de7e08 --- /dev/null +++ b/actionpack/test/fixtures/topic.rb @@ -0,0 +1,3 @@ +class Topic < ActiveRecord::Base + has_many :replies, :include => [:user], :dependent => true +end
\ No newline at end of file diff --git a/actionpack/test/fixtures/topics.yml b/actionpack/test/fixtures/topics.yml new file mode 100644 index 0000000000..61ea02d76f --- /dev/null +++ b/actionpack/test/fixtures/topics.yml @@ -0,0 +1,22 @@ +futurama: + id: 1 + title: Isnt futurama awesome? + subtitle: It really is, isnt it. + content: I like futurama + created_at: <%= 1.day.ago.to_s(:db) %> + updated_at: + +harvey_birdman: + id: 2 + title: Harvey Birdman is the king of all men + subtitle: yup + content: It really is + created_at: <%= 2.hours.ago.to_s(:db) %> + updated_at: + +rails: + id: 3 + title: Rails is nice + subtitle: It makes me happy + content: except when I have to hack internals to fix pagination. even then really. + created_at: <%= 20.minutes.ago.to_s(:db) %> diff --git a/activerecord/CHANGELOG b/activerecord/CHANGELOG index 32cb4562a0..03a9f25325 100644 --- a/activerecord/CHANGELOG +++ b/activerecord/CHANGELOG @@ -1,5 +1,7 @@ *SVN* +* Fix problems with count when used with :include [Jeremy Hopple and Kevin Clark] + * ActiveRecord::RecordInvalid now states which validations failed in its default error message [Tobias Luetke] * Using AssociationCollection#build with arrays of hashes should call build, not create [DHH] diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index ea442021c5..6b461b5514 100755 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -779,6 +779,11 @@ module ActiveRecord end end end + + def count_with_associations(options = {}) + reflections = reflect_on_included_associations(options[:include]) + return count_by_sql(construct_counter_sql_with_included_associations(options, reflections)) + end def find_with_associations(options = {}) reflections = reflect_on_included_associations(options[:include]) @@ -996,40 +1001,72 @@ module ActiveRecord "#{name} Load Including Associations" ) end + + def construct_counter_sql_with_included_associations(options, reflections) + sql = "SELECT COUNT(DISTINCT #{table_name}.#{primary_key})" + + # A (slower) workaround if we're using a backend, like sqlite, that doesn't support COUNT DISTINCT. + if !Base.connection.supports_count_distinct? + sql = "SELECT COUNT(*) FROM (SELECT DISTINCT #{table_name}.#{primary_key}" + end + + sql << " FROM #{table_name} " + sql << reflections.collect { |reflection| association_join(reflection) }.to_s + sql << "#{options[:joins]} " if options[:joins] + + add_conditions!(sql, options[:conditions]) + add_sti_conditions!(sql, reflections) + add_limited_ids_condition!(sql, options, reflections) if !using_limitable_reflections?(reflections) && options[:limit] + + add_limit!(sql, options) if using_limitable_reflections?(reflections) + + if !Base.connection.supports_count_distinct? + sql << ")" + end + + return sanitize_sql(sql) + end def construct_finder_sql_with_included_associations(options, schema_abbreviations, reflections) sql = "SELECT #{column_aliases(schema_abbreviations)} FROM #{table_name} " sql << reflections.collect { |reflection| association_join(reflection) }.to_s sql << "#{options[:joins]} " if options[:joins] - + add_conditions!(sql, options[:conditions]) add_sti_conditions!(sql, reflections) - add_limited_ids_condition!(sql, options) if !using_limitable_reflections?(reflections) && options[:limit] + add_limited_ids_condition!(sql, options, reflections) if !using_limitable_reflections?(reflections) && options[:limit] sql << "ORDER BY #{options[:order]} " if options[:order] - + add_limit!(sql, options) if using_limitable_reflections?(reflections) - + return sanitize_sql(sql) end - - def add_limited_ids_condition!(sql, options) - unless (id_list = select_limited_ids_list(options)).empty? + + def add_limited_ids_condition!(sql, options, reflections) + unless (id_list = select_limited_ids_list(options, reflections)).empty? sql << "#{condition_word(sql)} #{table_name}.#{primary_key} IN (#{id_list}) " end end - - def select_limited_ids_list(options) + + def select_limited_ids_list(options, reflections) connection.select_values( - construct_finder_sql_for_association_limiting(options), + construct_finder_sql_for_association_limiting(options, reflections), "#{name} Load IDs For Limited Eager Loading" ).collect { |id| connection.quote(id) }.join(", ") end - - def construct_finder_sql_for_association_limiting(options) - raise(ArgumentError, "Limited eager loads and conditions on the eager tables is incompatible") if include_eager_conditions?(options) + + def construct_finder_sql_for_association_limiting(options, reflections) + #sql = "SELECT DISTINCT #{table_name}.#{primary_key} FROM #{table_name} " + sql = "SELECT " + sql << "DISTINCT #{table_name}." if include_eager_conditions?(options) || include_eager_order?(options) + sql << "#{primary_key} FROM #{table_name} " + + if include_eager_conditions?(options) || include_eager_order?(options) + sql << reflections.collect { |reflection| association_join(reflection) }.to_s + sql << "#{options[:joins]} " if options[:joins] + end - sql = "SELECT #{primary_key} FROM #{table_name} " add_conditions!(sql, options[:conditions]) sql << "ORDER BY #{options[:order]} " if options[:order] add_limit!(sql, options) @@ -1044,6 +1081,14 @@ module ActiveRecord condition_table_name != table_name end end + + def include_eager_order?(options) + order = options[:order] + return false unless order + order.scan(/(\w+)\.\w+/).flatten.any? do |order_table_name| + order_table_name != table_name + end + end def using_limitable_reflections?(reflections) reflections.reject { |r| [ :belongs_to, :has_one ].include?(r.macro) }.length.zero? diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb index dd0c3e5c79..deb3c5fc1f 100755 --- a/activerecord/lib/active_record/base.rb +++ b/activerecord/lib/active_record/base.rb @@ -495,13 +495,59 @@ module ActiveRecord #:nodoc: connection.delete(sql, "#{name} Delete all") end - # Returns the number of records that meet the +conditions+. Zero is returned if no records match. Example: - # Product.count "sales > 1" - def count(conditions = nil, joins = nil) - sql = "SELECT COUNT(*) FROM #{table_name} " - sql << " #{joins} " if joins - add_conditions!(sql, conditions) - count_by_sql(sql) + # 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 by conditions or joins: For backwards compatibility, you can pass in +conditions+ and +joins+ as individual parameters. + # * Count using options will find the row count matched by the options used. + # + # The last approach, count using options, accepts an option hash as the only parameter. The options are: + # + # * <tt>:conditions</tt>: An SQL fragment like "administrator = 1" or [ "user_name = ?", username ]. See conditions in the intro. + # * <tt>:joins</tt>: 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. + # * <tt>:include</tt>: 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 DISTINCT items for the model you're counting. + # See eager loading under Associations. + # + # Examples for counting all: + # Person.count # returns the total count of all people + # + # Examples for count by +conditions+ and +joins+ (for backwards compatibility): + # Person.count("age > 26") # returns the number of people older than 26 + # Person.find("age > 26 AND job.salary > 60000", "LEFT JOIN jobs on jobs.person_id = person.id") # returns the total number of rows matching the conditions and joins fetched by SELECT COUNT(*). + # + # 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. + def count(*args) + options = {} + + #For backwards compatibility, we need to handle both count(conditions=nil, joins=nil) or count(options={}). + if args.size >= 0 and args.size <= 2 + if args.first.is_a?(Hash) + options = args.first + #should we verify the options hash??? + else + #Handle legacy paramter options: def count(conditions=nil, joins=nil) + options.merge!(:conditions => args[0]) if args.length > 0 + options.merge!(:joins => args[1]) if args.length > 1 + end + else + raise(ArgumentError, "Unexpected parameters passed to count(*args): expected either count(conditions=nil, joins=nil) or count(options={})") + end + + options[:include] ? count_with_associations(options) : count_by_sql(construct_counter_sql(options)) + end + + def construct_counter_sql(options) + sql = "SELECT COUNT(" + sql << "DISTINCT " if options[:distinct] + sql << "#{table_name}.#{primary_key}) FROM #{table_name} " + sql << " #{options[:joins]} " if options[:joins] + add_conditions!(sql, options[:conditions]) + sql end # Returns the result of an SQL statement that should only include a COUNT(*) in the SELECT part. diff --git a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb index f120a50f8f..b01ef89dfc 100755 --- a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb @@ -38,6 +38,12 @@ module ActiveRecord def supports_migrations? false end + + # Does this adapter support using DISTINCT within COUNT? This is +true+ + # for all adapters except sqlite. + def supports_count_distinct? + true + end # Should primary key values be selected from their corresponding # sequence before the insert statement? If true, next_sequence_value diff --git a/activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb b/activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb index 44011a09fd..579bc471d2 100644 --- a/activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb @@ -98,6 +98,10 @@ module ActiveRecord def supports_migrations? #:nodoc: true end + + def supports_count_distinct? #:nodoc: + false + end def native_database_types #:nodoc: { diff --git a/activerecord/test/associations_go_eager_test.rb b/activerecord/test/associations_go_eager_test.rb index f345656cc2..a03c201d76 100644 --- a/activerecord/test/associations_go_eager_test.rb +++ b/activerecord/test/associations_go_eager_test.rb @@ -120,9 +120,11 @@ class EagerAssociationTest < Test::Unit::TestCase end def test_eager_with_has_many_and_limit_and_conditions_array_on_the_eagers - assert_raises(ArgumentError) do - posts = Post.find(:all, :include => [ :author, :comments ], :limit => 2, :conditions => [ "authors.name = ?", 'David' ]) - end + posts = Post.find(:all, :include => [ :author, :comments ], :limit => 2, :conditions => [ "authors.name = ?", 'David' ]) + assert_equal 2, posts.size + + count = Post.count(:include => [ :author, :comments ], :limit => 2, :conditions => [ "authors.name = ?", 'David' ]) + assert_equal count, posts.size end def test_eager_with_has_many_and_limit_with_no_results @@ -141,13 +143,19 @@ class EagerAssociationTest < Test::Unit::TestCase end def test_eager_with_has_many_and_limit_and_conditions_on_the_eagers - assert_raises(ArgumentError) do - posts = authors(:david).posts.find(:all, - :include => :comments, - :conditions => "comments.body like 'Normal%' OR comments.#{QUOTED_TYPE}= 'SpecialComment'", - :limit => 2 - ) - end + posts = authors(:david).posts.find(:all, + :include => :comments, + :conditions => "comments.body like 'Normal%' OR comments.#{QUOTED_TYPE}= 'SpecialComment'", + :limit => 2 + ) + assert_equal 2, posts.size + + count = Post.count( + :include => [ :comments, :author ], + :conditions => "authors.name = 'David' AND (comments.body like 'Normal%' OR comments.#{QUOTED_TYPE}= 'SpecialComment')", + :limit => 2 + ) + assert_equal count, posts.size end def test_eager_association_loading_with_habtm diff --git a/activerecord/test/base_test.rb b/activerecord/test/base_test.rb index d2f347e0cb..475e31028a 100755 --- a/activerecord/test/base_test.rb +++ b/activerecord/test/base_test.rb @@ -1056,6 +1056,13 @@ class BasicsTest < Test::Unit::TestCase "LEFT JOIN comments ON posts.id=comments.post_id") end assert_equal res, res2 + + res3 = res + 1 + assert_nothing_raised do + res3 = Post.count(:conditions => "posts.#{QUOTED_TYPE} = 'Post'", + :joins => "LEFT JOIN comments ON posts.id=comments.post_id") + end + assert_equal res, res3 end def test_clear_association_cache_stored |