From 45e6f19925f23c3db257c15371d8f512cca843cd Mon Sep 17 00:00:00 2001 From: Pratik Naik Date: Tue, 30 Jun 2009 23:55:09 +0100 Subject: Revert "Revert "Generate proper :counter_sql from :finder_sql when there is a newline character immediately following 'SELECT' [#2118 state:resolved]"" This reverts commit 80f1f863cd0f9cba89079511282de5710a2e1832. The feature doesn't work on Postgres, so don't test it on Postgres. Also, Postgres compatibility is irrelevant to the ticket/patch in question. --- .../active_record/associations/association_collection.rb | 14 +++++++++++++- .../associations/has_and_belongs_to_many_association.rb | 10 +--------- .../lib/active_record/associations/has_many_association.rb | 10 +--------- .../associations/has_many_through_association.rb | 10 +--------- .../has_and_belongs_to_many_associations_test.rb | 7 +++++++ .../test/cases/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, 57 insertions(+), 39 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/associations/association_collection.rb b/activerecord/lib/active_record/associations/association_collection.rb index e12f6be35d..84edaec15e 100644 --- a/activerecord/lib/active_record/associations/association_collection.rb +++ b/activerecord/lib/active_record/associations/association_collection.rb @@ -351,7 +351,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 af9ce3dfb2..fd23e59e82 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 73dd50dd07..e4b631bc54 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 67631f0120..e21ef90391 100644 --- a/activerecord/lib/active_record/associations/has_many_through_association.rb +++ b/activerecord/lib/active_record/associations/has_many_through_association.rb @@ -90,15 +90,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 has_cached_counter? 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 8dc95806b9..14b96caaae 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 @@ -803,6 +803,13 @@ class HasAndBelongsToManyAssociationsTest < ActiveRecord::TestCase assert_equal 1, developer.projects.count end + unless current_adapter?(:PostgreSQLAdapter) + 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 + 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 d99424f9cd..15919e2289 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 96270118d8..0f3f6e41a5 100755 --- a/activerecord/test/cases/base_test.rb +++ b/activerecord/test/cases/base_test.rb @@ -599,9 +599,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 fd455e0864..75f52dfa4a 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 037b67ec84..d8f5695a0f 100644 --- a/activerecord/test/cases/finder_test.rb +++ b/activerecord/test/cases/finder_test.rb @@ -1044,8 +1044,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 eae5a60829..167d3abad9 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 d861b5bb24..194d5e9dff 100644 --- a/activerecord/test/cases/reflection_test.rb +++ b/activerecord/test/cases/reflection_test.rb @@ -176,8 +176,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 e7691fde46..9ad68fbe11 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 4c2514b0b3..840527ddeb 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 f25b2ddf77..422b12dc83 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 || ''}"}, -- cgit v1.2.3