Deprecate :finder_sql, :counter_sql, :insert_sql, :delete_sql.

activerecord/CHANGELOG.md

@@ -1,5 +1,16 @@
 ## Rails 4.0.0 (unreleased) ##
 
+*   `:finder_sql` and `:counter_sql` options on collection associations
+    are deprecated. Please transition to using scopes.
+
+    *Jon Leighton*
+
+*   `:insert_sql` and `:delete_sql` options on `has_and_belongs_to_many`
+    associations are deprecated. Please transition to using `has_many
+    :through`
+
+    *Jon Leighton*
+
 *   The migration generator now creates a join table with (commented) indexes every time
     the migration name contains the word `join_table`:
 

activerecord/lib/active_record/associations/builder/collection_association.rb

@@ -1,3 +1,5 @@
+require 'active_support/deprecation'
+
 module ActiveRecord::Associations::Builder
   class CollectionAssociation < Association #:nodoc:
     CALLBACKS = [:before_add, :after_add, :before_remove, :after_remove]
@@ -14,6 +16,7 @@ def initialize(*args, &extension)
     end
 
     def build
+      show_deprecation_warnings
       wrap_block_extension
       reflection = super
       CALLBACKS.each { |callback_name| define_callback(callback_name) }
@@ -24,6 +27,14 @@ def writable?
       true
     end
 
+    def show_deprecation_warnings
+      [:finder_sql, :counter_sql].each do |name|
+        if options.include? name
+          ActiveSupport::Deprecation.warn("The :#{name} association option is deprecated. Please find an alternative (such as using scopes).")
+        end
+      end
+    end
+
     private
 
       def wrap_block_extension

activerecord/lib/active_record/associations/builder/has_and_belongs_to_many.rb

@@ -14,6 +14,16 @@ def build
       reflection
     end
 
+    def show_deprecation_warnings
+      super
+
+      [:delete_sql, :insert_sql].each do |name|
+        if options.include? name
+          ActiveSupport::Deprecation.warn("The :#{name} association option is deprecated. Please find an alternative (such as using has_many :through).")
+        end
+      end
+    end
+
     private
 
       def define_destroy_hook

activerecord/lib/active_record/associations/collection_association.rb

@@ -380,7 +380,7 @@ def find_target
             if options[:finder_sql]
               reflection.klass.find_by_sql(custom_finder_sql)
             else
-              scoped.all
+              scoped.to_a
             end
 
           records.each { |record| set_inverse_instance(record) }

activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb

@@ -67,12 +67,15 @@ class DeveloperWithSymbolsForKeys < ActiveRecord::Base
 
 class DeveloperWithCounterSQL < ActiveRecord::Base
   self.table_name = 'developers'
-  has_and_belongs_to_many :projects,
-    :class_name => "DeveloperWithCounterSQL",
-    :join_table => "developers_projects",
-    :association_foreign_key => "project_id",
-    :foreign_key => "developer_id",
-    :counter_sql => proc { "SELECT COUNT(*) AS count_all FROM projects INNER JOIN developers_projects ON projects.id = developers_projects.project_id WHERE developers_projects.developer_id =#{id}" }
+
+  ActiveSupport::Deprecation.silence do
+    has_and_belongs_to_many :projects,
+      :class_name => "DeveloperWithCounterSQL",
+      :join_table => "developers_projects",
+      :association_foreign_key => "project_id",
+      :foreign_key => "developer_id",
+      :counter_sql => proc { "SELECT COUNT(*) AS count_all FROM projects INNER JOIN developers_projects ON projects.id = developers_projects.project_id WHERE developers_projects.developer_id =#{id}" }
+  end
 end
 
 class HasAndBelongsToManyAssociationsTest < ActiveRecord::TestCase
@@ -841,4 +844,16 @@ def test_include_method_in_has_and_belongs_to_many_association_should_return_tru
     developer = project.developers.build
     assert project.developers.include?(developer)
   end
+
+  test ":insert_sql is deprecated" do
+    klass = Class.new(ActiveRecord::Base)
+    def klass.name; 'Foo'; end
+    assert_deprecated { klass.has_and_belongs_to_many :posts, :insert_sql => 'lol' }
+  end
+
+  test ":delete_sql is deprecated" do
+    klass = Class.new(ActiveRecord::Base)
+    def klass.name; 'Foo'; end
+    assert_deprecated { klass.has_and_belongs_to_many :posts, :delete_sql => 'lol' }
+  end
 end

activerecord/test/cases/associations/has_many_associations_test.rb

@@ -22,7 +22,9 @@
 
 class HasManyAssociationsTestForCountWithFinderSql < ActiveRecord::TestCase
   class Invoice < ActiveRecord::Base
-    has_many :custom_line_items, :class_name => 'LineItem', :finder_sql => "SELECT line_items.* from line_items"
+    ActiveSupport::Deprecation.silence do
+      has_many :custom_line_items, :class_name => 'LineItem', :finder_sql => "SELECT line_items.* from line_items"
+    end
   end
   def test_should_fail
     assert_raise(ArgumentError) do
@@ -33,7 +35,9 @@ def test_should_fail
 
 class HasManyAssociationsTestForCountWithCountSql < ActiveRecord::TestCase
   class Invoice < ActiveRecord::Base
-    has_many :custom_line_items, :class_name => 'LineItem', :counter_sql => "SELECT COUNT(*) line_items.* from line_items"
+    ActiveSupport::Deprecation.silence do
+      has_many :custom_line_items, :class_name => 'LineItem', :counter_sql => "SELECT COUNT(*) line_items.* from line_items"
+    end
   end
   def test_should_fail
     assert_raise(ArgumentError) do
@@ -44,7 +48,9 @@ def test_should_fail
 
 class HasManyAssociationsTestForCountDistinctWithFinderSql < ActiveRecord::TestCase
   class Invoice < ActiveRecord::Base
-    has_many :custom_line_items, :class_name => 'LineItem', :finder_sql => "SELECT DISTINCT line_items.amount from line_items"
+    ActiveSupport::Deprecation.silence do
+      has_many :custom_line_items, :class_name => 'LineItem', :finder_sql => "SELECT DISTINCT line_items.amount from line_items"
+    end
   end
 
   def test_should_count_distinct_results
@@ -308,30 +314,30 @@ def test_finding_using_primary_key
   end
 
   def test_finding_using_sql
-    firm = Firm.scoped(:order => "id").first
+    firm = Firm.order("id").first
     first_client = firm.clients_using_sql.first
     assert_not_nil first_client
     assert_equal "Microsoft", first_client.name
     assert_equal 1, firm.clients_using_sql.size
-    assert_equal 1, Firm.scoped(:order => "id").first.clients_using_sql.size
+    assert_equal 1, Firm.order("id").first.clients_using_sql.size
   end
 
   def test_finding_using_sql_take_into_account_only_uniq_ids
-    firm = Firm.scoped(:order => "id").first
+    firm = Firm.order("id").first
     client = firm.clients_using_sql.first
     assert_equal client, firm.clients_using_sql.find(client.id, client.id)
     assert_equal client, firm.clients_using_sql.find(client.id, client.id.to_s)
   end
 
   def test_counting_using_sql
-    assert_equal 1, Firm.scoped(:order => "id").first.clients_using_counter_sql.size
-    assert Firm.scoped(:order => "id").first.clients_using_counter_sql.any?
-    assert_equal 0, Firm.scoped(:order => "id").first.clients_using_zero_counter_sql.size
-    assert !Firm.scoped(:order => "id").first.clients_using_zero_counter_sql.any?
+    assert_equal 1, Firm.order("id").first.clients_using_counter_sql.size
+    assert Firm.order("id").first.clients_using_counter_sql.any?
+    assert_equal 0, Firm.order("id").first.clients_using_zero_counter_sql.size
+    assert !Firm.order("id").first.clients_using_zero_counter_sql.any?
   end
 
   def test_counting_non_existant_items_using_sql
-    assert_equal 0, Firm.scoped(:order => "id").first.no_clients_using_counter_sql.size
+    assert_equal 0, Firm.order("id").first.no_clients_using_counter_sql.size
   end
 
   def test_counting_using_finder_sql
@@ -366,7 +372,7 @@ def test_find_ids
   end
 
   def test_find_string_ids_when_using_finder_sql
-    firm = Firm.scoped(:order => "id").first
+    firm = Firm.order("id").first
 
     client = firm.clients_using_finder_sql.find("2")
     assert_kind_of Client, client
@@ -1638,4 +1644,14 @@ def test_collection_association_with_private_kernel_method
       post.taggings_with_delete_all.delete_all
     end
   end
+
+  test ":finder_sql is deprecated" do
+    klass = Class.new(ActiveRecord::Base)
+    assert_deprecated { klass.has_many :foo, :finder_sql => 'lol' }
+  end
+
+  test ":counter_sql is deprecated" do
+    klass = Class.new(ActiveRecord::Base)
+    assert_deprecated { klass.has_many :foo, :counter_sql => 'lol' }
+  end
 end

activerecord/test/models/company.rb

@@ -36,11 +36,13 @@ class Client < ::Company
 end
 
 class Firm < Company
-  has_many :clients, -> { order "id" }, :dependent => :destroy, :counter_sql =>
-      "SELECT COUNT(*) FROM companies WHERE firm_id = 1 " +
-      "AND (#{QUOTED_TYPE} = 'Client' OR #{QUOTED_TYPE} = 'SpecialClient' OR #{QUOTED_TYPE} = 'VerySpecialClient' )",
-      :before_remove => :log_before_remove,
-      :after_remove  => :log_after_remove
+  ActiveSupport::Deprecation.silence do
+    has_many :clients, -> { order "id" }, :dependent => :destroy, :counter_sql =>
+        "SELECT COUNT(*) FROM companies WHERE firm_id = 1 " +
+        "AND (#{QUOTED_TYPE} = 'Client' OR #{QUOTED_TYPE} = 'SpecialClient' OR #{QUOTED_TYPE} = 'VerySpecialClient' )",
+        :before_remove => :log_before_remove,
+        :after_remove  => :log_after_remove
+  end
   has_many :unsorted_clients, :class_name => "Client"
   has_many :unsorted_clients_with_symbol, :class_name => :Client
   has_many :clients_sorted_desc, -> { order "id DESC" }, :class_name => "Client"
@@ -53,17 +55,19 @@ class Firm < Company
   has_many :clients_with_interpolated_conditions, ->(firm) { where "rating > #{firm.rating}" }, :class_name => "Client"
   has_many :clients_like_ms, -> { where("name = 'Microsoft'").order("id") }, :class_name => "Client"
   has_many :clients_like_ms_with_hash_conditions, -> { where(:name => 'Microsoft').order("id") }, :class_name => "Client"
-  has_many :clients_using_sql, :class_name => "Client", :finder_sql => proc { "SELECT * FROM companies WHERE client_of = #{id}" }
-  has_many :clients_using_counter_sql, :class_name => "Client",
-           :finder_sql  => proc { "SELECT * FROM companies WHERE client_of = #{id} " },
-           :counter_sql => proc { "SELECT COUNT(*) FROM companies WHERE client_of = #{id}" }
-  has_many :clients_using_zero_counter_sql, :class_name => "Client",
-           :finder_sql  => proc { "SELECT * FROM companies WHERE client_of = #{id}" },
-           :counter_sql => proc { "SELECT 0 FROM companies WHERE client_of = #{id}" }
-  has_many :no_clients_using_counter_sql, :class_name => "Client",
-           :finder_sql  => 'SELECT * FROM companies WHERE client_of = 1000',
-           :counter_sql => 'SELECT COUNT(*) FROM companies WHERE client_of = 1000'
-  has_many :clients_using_finder_sql, :class_name => "Client", :finder_sql => 'SELECT * FROM companies WHERE 1=1'
+  ActiveSupport::Deprecation.silence do
+    has_many :clients_using_sql, :class_name => "Client", :finder_sql => proc { "SELECT * FROM companies WHERE client_of = #{id}" }
+    has_many :clients_using_counter_sql, :class_name => "Client",
+             :finder_sql  => proc { "SELECT * FROM companies WHERE client_of = #{id} " },
+             :counter_sql => proc { "SELECT COUNT(*) FROM companies WHERE client_of = #{id}" }
+    has_many :clients_using_zero_counter_sql, :class_name => "Client",
+             :finder_sql  => proc { "SELECT * FROM companies WHERE client_of = #{id}" },
+             :counter_sql => proc { "SELECT 0 FROM companies WHERE client_of = #{id}" }
+    has_many :no_clients_using_counter_sql, :class_name => "Client",
+             :finder_sql  => 'SELECT * FROM companies WHERE client_of = 1000',
+             :counter_sql => 'SELECT COUNT(*) FROM companies WHERE client_of = 1000'
+    has_many :clients_using_finder_sql, :class_name => "Client", :finder_sql => 'SELECT * FROM companies WHERE 1=1'
+  end
   has_many :plain_clients, :class_name => 'Client'
   has_many :readonly_clients, -> { readonly }, :class_name => 'Client'
   has_many :clients_using_primary_key, :class_name => 'Client',

activerecord/test/models/company_in_module.rb

@@ -11,7 +11,9 @@ class Firm < Company
       has_many :clients_sorted_desc, -> { order("id DESC") }, :class_name => "Client"
       has_many :clients_of_firm, -> { order "id" }, :foreign_key => "client_of", :class_name => "Client"
       has_many :clients_like_ms, -> { where("name = 'Microsoft'").order("id") }, :class_name => "Client"
-      has_many :clients_using_sql, :class_name => "Client", :finder_sql => 'SELECT * FROM companies WHERE client_of = #{id}'
+      ActiveSupport::Deprecation.silence do
+        has_many :clients_using_sql, :class_name => "Client", :finder_sql => 'SELECT * FROM companies WHERE client_of = #{id}'
+      end
 
       has_one :account, :class_name => 'MyApplication::Billing::Account', :dependent => :destroy
     end

activerecord/test/models/project.rb

@@ -7,15 +7,19 @@ class Project < ActiveRecord::Base
   has_and_belongs_to_many :developers_named_david, -> { where("name = 'David'").uniq }, :class_name => "Developer"
   has_and_belongs_to_many :developers_named_david_with_hash_conditions, -> { where(:name => 'David').uniq }, :class_name => "Developer"
   has_and_belongs_to_many :salaried_developers, -> { where "salary > 0" }, :class_name => "Developer"
-  has_and_belongs_to_many :developers_with_finder_sql, :class_name => "Developer", :finder_sql => proc { "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 => proc {
-    "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 => proc { |record| "DELETE FROM developers_projects WHERE project_id = #{id} AND developer_id = #{record.id}" }
+
+  ActiveSupport::Deprecation.silence do
+    has_and_belongs_to_many :developers_with_finder_sql, :class_name => "Developer", :finder_sql => proc { "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 => proc {
+      "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 => proc { |record| "DELETE FROM developers_projects WHERE project_id = #{id} AND developer_id = #{record.id}" }
+  end
+
   has_and_belongs_to_many :developers_with_callbacks, :class_name => "Developer", :before_add => Proc.new {|o, r| o.developers_log << "before_adding#{r.id || '<new>'}"},
                             :after_add => Proc.new {|o, r| o.developers_log << "after_adding#{r.id || '<new>'}"},
                             :before_remove => Proc.new {|o, r| o.developers_log << "before_removing#{r.id}"},