From 2f02f70cab2d08a073b7107d6c060f7971d64ba5 Mon Sep 17 00:00:00 2001 From: Patrick Joyce Date: Tue, 3 Mar 2009 10:19:31 -0500 Subject: [PATCH] Make AssociationCollections correctly generate :counter_sql from :finder_sql when there is a newline character immediately following 'SELECT' --- .../associations/association_collection.rb | 14 +++++++++++++- .../has_and_belongs_to_many_association.rb | 10 +--------- .../associations/has_many_association.rb | 10 +--------- .../associations/has_many_through_association.rb | 10 +--------- .../has_and_belongs_to_many_associations_test.rb | 5 +++++ .../associations/has_many_associations_test.rb | 5 +++++ activerecord/test/cases/base_test.rb | 6 +++--- activerecord/test/cases/calculations_test.rb | 4 ++-- activerecord/test/cases/finder_test.rb | 4 ++-- activerecord/test/cases/inheritance_test.rb | 4 ++-- activerecord/test/cases/reflection_test.rb | 4 ++-- activerecord/test/fixtures/companies.yml | 8 ++++++++ activerecord/test/models/company.rb | 4 ++++ activerecord/test/models/project.rb | 6 ++++++ 14 files changed, 55 insertions(+), 39 deletions(-) diff --git a/activerecord/lib/active_record/associations/association_collection.rb b/activerecord/lib/active_record/associations/association_collection.rb index 0fefec1..f9be356 100644 --- a/activerecord/lib/active_record/associations/association_collection.rb +++ b/activerecord/lib/active_record/associations/association_collection.rb @@ -337,7 +337,19 @@ module ActiveRecord protected def construct_find_options!(options) end - + + def construct_counter_sql + if @reflection.options[:counter_sql] + @counter_sql = interpolate_sql(@reflection.options[:counter_sql]) + elsif @reflection.options[:finder_sql] + # replace the SELECT clause with COUNT(*), preserving any hints within /* ... */ + @reflection.options[:counter_sql] = @reflection.options[:finder_sql].sub(/SELECT\b(\/\*.*?\*\/ )?(.*)\bFROM\b/im) { "SELECT #{$1}COUNT(*) FROM" } + @counter_sql = interpolate_sql(@reflection.options[:counter_sql]) + else + @counter_sql = @finder_sql + end + end + def load_target if !@owner.new_record? || foreign_key_present begin diff --git a/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb b/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb index af9ce3d..fd23e59 100644 --- a/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb +++ b/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb @@ -85,15 +85,7 @@ module ActiveRecord @join_sql = "INNER JOIN #{@owner.connection.quote_table_name @reflection.options[:join_table]} ON #{@reflection.quoted_table_name}.#{@reflection.klass.primary_key} = #{@owner.connection.quote_table_name @reflection.options[:join_table]}.#{@reflection.association_foreign_key}" - if @reflection.options[:counter_sql] - @counter_sql = interpolate_sql(@reflection.options[:counter_sql]) - elsif @reflection.options[:finder_sql] - # replace the SELECT clause with COUNT(*), preserving any hints within /* ... */ - @reflection.options[:counter_sql] = @reflection.options[:finder_sql].sub(/SELECT (\/\*.*?\*\/ )?(.*)\bFROM\b/im) { "SELECT #{$1}COUNT(*) FROM" } - @counter_sql = interpolate_sql(@reflection.options[:counter_sql]) - else - @counter_sql = @finder_sql - end + construct_counter_sql end def construct_scope diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb index a2cbabf..d5ff9f7 100644 --- a/activerecord/lib/active_record/associations/has_many_association.rb +++ b/activerecord/lib/active_record/associations/has_many_association.rb @@ -97,15 +97,7 @@ module ActiveRecord @finder_sql << " AND (#{conditions})" if conditions end - if @reflection.options[:counter_sql] - @counter_sql = interpolate_sql(@reflection.options[:counter_sql]) - elsif @reflection.options[:finder_sql] - # replace the SELECT clause with COUNT(*), preserving any hints within /* ... */ - @reflection.options[:counter_sql] = @reflection.options[:finder_sql].sub(/SELECT (\/\*.*?\*\/ )?(.*)\bFROM\b/im) { "SELECT #{$1}COUNT(*) FROM" } - @counter_sql = interpolate_sql(@reflection.options[:counter_sql]) - else - @counter_sql = @finder_sql - end + construct_counter_sql end def construct_scope diff --git a/activerecord/lib/active_record/associations/has_many_through_association.rb b/activerecord/lib/active_record/associations/has_many_through_association.rb index 1c091e7..0614fa2 100644 --- a/activerecord/lib/active_record/associations/has_many_through_association.rb +++ b/activerecord/lib/active_record/associations/has_many_through_association.rb @@ -191,15 +191,7 @@ module ActiveRecord @finder_sql = construct_conditions end - if @reflection.options[:counter_sql] - @counter_sql = interpolate_sql(@reflection.options[:counter_sql]) - elsif @reflection.options[:finder_sql] - # replace the SELECT clause with COUNT(*), preserving any hints within /* ... */ - @reflection.options[:counter_sql] = @reflection.options[:finder_sql].sub(/SELECT (\/\*.*?\*\/ )?(.*)\bFROM\b/im) { "SELECT #{$1}COUNT(*) FROM" } - @counter_sql = interpolate_sql(@reflection.options[:counter_sql]) - else - @counter_sql = @finder_sql - end + construct_counter_sql end def conditions 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 ca1772d..df57e11 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 @@ -775,6 +775,11 @@ class HasAndBelongsToManyAssociationsTest < ActiveRecord::TestCase assert_equal 1, developer.projects.count end + def test_count_with_finder_sql + assert_equal 3, projects(:active_record).developers_with_finder_sql.count + assert_equal 3, projects(:active_record).developers_with_multiline_finder_sql.count + end + def test_association_proxy_transaction_method_starts_transaction_in_association_class Post.expects(:transaction) Category.find(:first).posts.transaction do diff --git a/activerecord/test/cases/associations/has_many_associations_test.rb b/activerecord/test/cases/associations/has_many_associations_test.rb index b7fa9d9..fa1d0f5 100644 --- a/activerecord/test/cases/associations/has_many_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_associations_test.rb @@ -163,6 +163,11 @@ class HasManyAssociationsTest < ActiveRecord::TestCase assert_equal 0, Firm.find(:first).no_clients_using_counter_sql.size end + def test_counting_using_finder_sql + assert_equal 2, Firm.find(4).clients_using_sql.count + assert_equal 2, Firm.find(4).clients_using_multiline_sql.count + end + def test_belongs_to_sanity c = Client.new assert_nil c.firm diff --git a/activerecord/test/cases/base_test.rb b/activerecord/test/cases/base_test.rb index 99d7796..aa570e0 100755 --- a/activerecord/test/cases/base_test.rb +++ b/activerecord/test/cases/base_test.rb @@ -593,9 +593,9 @@ class BasicsTest < ActiveRecord::TestCase end def test_destroy_many - assert_equal 3, Client.count - Client.destroy([2, 3]) - assert_equal 1, Client.count + assert_difference('Client.count', -2) do + Client.destroy([2, 3]) + end end def test_delete_many diff --git a/activerecord/test/cases/calculations_test.rb b/activerecord/test/cases/calculations_test.rb index 56dcdea..a17a79e 100644 --- a/activerecord/test/cases/calculations_test.rb +++ b/activerecord/test/cases/calculations_test.rb @@ -203,7 +203,7 @@ class CalculationsTest < ActiveRecord::TestCase c = Company.count(:all, :group => "UPPER(#{QUOTED_TYPE})") assert_equal 2, c[nil] assert_equal 1, c['DEPENDENTFIRM'] - assert_equal 3, c['CLIENT'] + assert_equal 4, c['CLIENT'] assert_equal 2, c['FIRM'] end @@ -211,7 +211,7 @@ class CalculationsTest < ActiveRecord::TestCase c = Company.count(:all, :group => "UPPER(companies.#{QUOTED_TYPE})") assert_equal 2, c[nil] assert_equal 1, c['DEPENDENTFIRM'] - assert_equal 3, c['CLIENT'] + assert_equal 4, c['CLIENT'] assert_equal 2, c['FIRM'] end diff --git a/activerecord/test/cases/finder_test.rb b/activerecord/test/cases/finder_test.rb index d877895..014efec 100644 --- a/activerecord/test/cases/finder_test.rb +++ b/activerecord/test/cases/finder_test.rb @@ -1037,8 +1037,8 @@ class FinderTest < ActiveRecord::TestCase end def test_select_values - assert_equal ["1","2","3","4","5","6","7","8","9"], Company.connection.select_values("SELECT id FROM companies ORDER BY id").map! { |i| i.to_s } - assert_equal ["37signals","Summit","Microsoft", "Flamboyant Software", "Ex Nihilo", "RailsCore", "Leetsoft", "Jadedpixel", "Odegy"], Company.connection.select_values("SELECT name FROM companies ORDER BY id") + assert_equal ["1","2","3","4","5","6","7","8","9", "10"], Company.connection.select_values("SELECT id FROM companies ORDER BY id").map! { |i| i.to_s } + assert_equal ["37signals","Summit","Microsoft", "Flamboyant Software", "Ex Nihilo", "RailsCore", "Leetsoft", "Jadedpixel", "Odegy", "Ex Nihilo Part Deux"], Company.connection.select_values("SELECT name FROM companies ORDER BY id") end def test_select_rows diff --git a/activerecord/test/cases/inheritance_test.rb b/activerecord/test/cases/inheritance_test.rb index eae5a60..167d3ab 100644 --- a/activerecord/test/cases/inheritance_test.rb +++ b/activerecord/test/cases/inheritance_test.rb @@ -112,9 +112,9 @@ class InheritanceTest < ActiveRecord::TestCase end def test_inheritance_condition - assert_equal 9, Company.count + assert_equal 10, Company.count assert_equal 2, Firm.count - assert_equal 3, Client.count + assert_equal 4, Client.count end def test_alt_inheritance_condition diff --git a/activerecord/test/cases/reflection_test.rb b/activerecord/test/cases/reflection_test.rb index db64bbb..9275c9a 100644 --- a/activerecord/test/cases/reflection_test.rb +++ b/activerecord/test/cases/reflection_test.rb @@ -170,8 +170,8 @@ class ReflectionTest < ActiveRecord::TestCase def test_reflection_of_all_associations # FIXME these assertions bust a lot - assert_equal 28, Firm.reflect_on_all_associations.size - assert_equal 21, Firm.reflect_on_all_associations(:has_many).size + assert_equal 29, Firm.reflect_on_all_associations.size + assert_equal 22, Firm.reflect_on_all_associations(:has_many).size assert_equal 7, Firm.reflect_on_all_associations(:has_one).size assert_equal 0, Firm.reflect_on_all_associations(:belongs_to).size end diff --git a/activerecord/test/fixtures/companies.yml b/activerecord/test/fixtures/companies.yml index e7691fd..9ad68fb 100644 --- a/activerecord/test/fixtures/companies.yml +++ b/activerecord/test/fixtures/companies.yml @@ -35,6 +35,14 @@ another_client: name: Ex Nihilo ruby_type: Client +a_third_client: + id: 10 + type: Client + firm_id: 4 + client_of: 4 + name: Ex Nihilo Part Deux + ruby_type: Client + rails_core: id: 6 name: RailsCore diff --git a/activerecord/test/models/company.rb b/activerecord/test/models/company.rb index 02a775f..9b73095 100644 --- a/activerecord/test/models/company.rb +++ b/activerecord/test/models/company.rb @@ -48,6 +48,10 @@ class Firm < Company has_many :clients_with_interpolated_conditions, :class_name => "Client", :conditions => 'rating > #{rating}' has_many :clients_like_ms_with_hash_conditions, :conditions => { :name => 'Microsoft' }, :class_name => "Client", :order => "id" has_many :clients_using_sql, :class_name => "Client", :finder_sql => 'SELECT * FROM companies WHERE client_of = #{id}' + has_many :clients_using_multiline_sql, :class_name => "Client", :finder_sql => ' + SELECT + companies.* + FROM companies WHERE companies.client_of = #{id}' has_many :clients_using_counter_sql, :class_name => "Client", :finder_sql => 'SELECT * FROM companies WHERE client_of = #{id}', :counter_sql => 'SELECT COUNT(*) FROM companies WHERE client_of = #{id}' diff --git a/activerecord/test/models/project.rb b/activerecord/test/models/project.rb index 550d4ae..07fee01 100644 --- a/activerecord/test/models/project.rb +++ b/activerecord/test/models/project.rb @@ -8,6 +8,12 @@ class Project < ActiveRecord::Base has_and_belongs_to_many :developers_named_david_with_hash_conditions, :class_name => "Developer", :conditions => { :name => 'David' }, :uniq => true has_and_belongs_to_many :salaried_developers, :class_name => "Developer", :conditions => "salary > 0" has_and_belongs_to_many :developers_with_finder_sql, :class_name => "Developer", :finder_sql => 'SELECT t.*, j.* FROM developers_projects j, developers t WHERE t.id = j.developer_id AND j.project_id = #{id} ORDER BY t.id' + has_and_belongs_to_many :developers_with_multiline_finder_sql, :class_name => "Developer", :finder_sql => ' + SELECT + t.*, j.* + FROM + developers_projects j, + developers t WHERE t.id = j.developer_id AND j.project_id = #{id} ORDER BY t.id' has_and_belongs_to_many :developers_by_sql, :class_name => "Developer", :delete_sql => "DELETE FROM developers_projects WHERE project_id = \#{id} AND developer_id = \#{record.id}" has_and_belongs_to_many :developers_with_callbacks, :class_name => "Developer", :before_add => Proc.new {|o, r| o.developers_log << "before_adding#{r.id || ''}"}, :after_add => Proc.new {|o, r| o.developers_log << "after_adding#{r.id || ''}"}, -- 1.5.6.5