deprecate `Relation#uniq` use `Relation#distinct` instead.

See #9683 for the reasons we switched to `distinct`.

Here is the discussion that triggered the actual deprecation #20198.

`uniq`, `uniq!` and `uniq_value` are still around.
They will be removed in the next minor release after Rails 5.

12 files changed, 52 insertions, 31 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 5b09101bca..a3e2d5bd8e 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,9 @@
+*   Deprecate `Relation#uniq` use `Relation#distinct` instead.
+
+    See #9683.
+
+    *Yves Senn*
+
 *   Allow single table inheritance instantiation to work when storing
     demodulized class names.
 
diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb
index a3c30642d0..779a3fbbbd 100644
--- a/activerecord/lib/active_record/associations.rb
+++ b/activerecord/lib/active_record/associations.rb
@@ -266,7 +266,6 @@ module ActiveRecord
     #   others.find(*args)                |   X   |    X     |    X
     #   others.exists?                    |   X   |    X     |    X
     #   others.distinct                   |   X   |    X     |    X
-    #   others.uniq                       |   X   |    X     |    X
     #   others.reset                      |   X   |    X     |    X
     #
     # === Overriding generated methods
diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb
index 85648a7f8f..7d37313058 100644
--- a/activerecord/lib/active_record/relation.rb
+++ b/activerecord/lib/active_record/relation.rb
@@ -9,7 +9,7 @@ module ActiveRecord
                             :extending, :unscope]
 
     SINGLE_VALUE_METHODS = [:limit, :offset, :lock, :readonly, :reordering,
-                            :reverse_order, :distinct, :create_with, :uniq]
+                            :reverse_order, :distinct, :create_with]
     CLAUSE_METHODS = [:where, :having, :from]
     INVALID_METHODS_FOR_DELETE_ALL = [:limit, :distinct, :offset, :group, :having]
 
@@ -618,6 +618,7 @@ module ActiveRecord
     def uniq_value
       distinct_value
     end
+    deprecate uniq_value: :distinct_value
 
     # Compares two relations for equality.
     def ==(other)
diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb
index 69ce5cdc2a..fd78db2e95 100644
--- a/activerecord/lib/active_record/relation/query_methods.rb
+++ b/activerecord/lib/active_record/relation/query_methods.rb
@@ -587,7 +587,7 @@ module ActiveRecord
     #
     # The two relations must be structurally compatible: they must be scoping the same model, and
     # they must differ only by +where+ (if no +group+ has been defined) or +having+ (if a +group+ is
-    # present). Neither relation may have a +limit+, +offset+, or +uniq+ set.
+    # present). Neither relation may have a +limit+, +offset+, or +distinct+ set.
     #
     #    Post.where("id = 1").or(Post.where("id = 2"))
     #    # SELECT `posts`.* FROM `posts`  WHERE (('id = 1' OR 'id = 2'))
@@ -790,6 +790,7 @@ module ActiveRecord
       spawn.distinct!(value)
     end
     alias uniq distinct
+    deprecate uniq: :distinct
 
     # Like #distinct, but modifies relation in place.
     def distinct!(value = true) # :nodoc:
@@ -797,6 +798,7 @@ module ActiveRecord
       self
     end
     alias uniq! distinct!
+    deprecate uniq!: :distinct!
 
     # Used to extend a scope with additional methods, either through
     # a module or through a block provided.
diff --git a/activerecord/test/cases/associations/eager_test.rb b/activerecord/test/cases/associations/eager_test.rb
index 0ecf2ddfd1..ffbf60e390 100644
--- a/activerecord/test/cases/associations/eager_test.rb
+++ b/activerecord/test/cases/associations/eager_test.rb
@@ -1167,7 +1167,7 @@ class EagerAssociationTest < ActiveRecord::TestCase
     assert_no_queries { assert client.accounts.empty? }
   end
 
-  def test_preloading_has_many_through_with_uniq
+  def test_preloading_has_many_through_with_distinct
     mary = Author.includes(:unique_categorized_posts).where(:id => authors(:mary).id).first
     assert_equal 1, mary.unique_categorized_posts.length
     assert_equal 1, mary.unique_categorized_post_ids.length
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 aea9207bfe..2ec2bb1754 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
@@ -234,7 +234,7 @@ class HasAndBelongsToManyAssociationsTest < ActiveRecord::TestCase
     assert_equal developers, new_project.developers
   end
 
-  def test_habtm_unique_order_preserved
+  def test_habtm_distinct_order_preserved
     assert_equal developers(:poor_jamis, :jamis, :david), projects(:active_record).non_unique_developers
     assert_equal developers(:poor_jamis, :jamis, :david), projects(:active_record).developers
   end
@@ -339,7 +339,7 @@ class HasAndBelongsToManyAssociationsTest < ActiveRecord::TestCase
     assert_equal  'Yet Another Testing Title', another_post.title
   end
 
-  def test_uniq_after_the_fact
+  def test_distinct_after_the_fact
     dev = developers(:jamis)
     dev.projects << projects(:active_record)
     dev.projects << projects(:active_record)
@@ -348,13 +348,13 @@ class HasAndBelongsToManyAssociationsTest < ActiveRecord::TestCase
     assert_equal 1, dev.projects.distinct.size
   end
 
-  def test_uniq_before_the_fact
+  def test_distinct_before_the_fact
     projects(:active_record).developers << developers(:jamis)
     projects(:active_record).developers << developers(:david)
     assert_equal 3, projects(:active_record, :reload).developers.size
   end
 
-  def test_uniq_option_prevents_duplicate_push
+  def test_distinct_option_prevents_duplicate_push
     project = projects(:active_record)
     project.developers << developers(:jamis)
     project.developers << developers(:david)
@@ -365,7 +365,7 @@ class HasAndBelongsToManyAssociationsTest < ActiveRecord::TestCase
     assert_equal 3, project.developers.size
   end
 
-  def test_uniq_when_association_already_loaded
+  def test_distinct_when_association_already_loaded
     project = projects(:active_record)
     project.developers << [ developers(:jamis), developers(:david), developers(:jamis), developers(:david) ]
     assert_equal 3, Project.includes(:developers).find(project.id).developers.size
diff --git a/activerecord/test/cases/associations/has_many_through_associations_test.rb b/activerecord/test/cases/associations/has_many_through_associations_test.rb
index 190cef55c4..1d545af5a5 100644
--- a/activerecord/test/cases/associations/has_many_through_associations_test.rb
+++ b/activerecord/test/cases/associations/has_many_through_associations_test.rb
@@ -960,7 +960,7 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase
     assert_equal 1, category.categorizations.where(:special => true).count
   end
 
-  def test_joining_has_many_through_with_uniq
+  def test_joining_has_many_through_with_distinct
     mary = Author.joins(:unique_categorized_posts).where(:id => authors(:mary).id).first
     assert_equal 1, mary.unique_categorized_posts.length
     assert_equal 1, mary.unique_categorized_post_ids.length
diff --git a/activerecord/test/cases/associations/join_model_test.rb b/activerecord/test/cases/associations/join_model_test.rb
index 213be50e67..5575419c35 100644
--- a/activerecord/test/cases/associations/join_model_test.rb
+++ b/activerecord/test/cases/associations/join_model_test.rb
@@ -35,12 +35,12 @@ class AssociationsJoinModelTest < ActiveRecord::TestCase
     assert categories(:sti_test).authors.include?(authors(:mary))
   end
 
-  def test_has_many_uniq_through_join_model
+  def test_has_many_distinct_through_join_model
     assert_equal 2, authors(:mary).categorized_posts.size
     assert_equal 1, authors(:mary).unique_categorized_posts.size
   end
 
-  def test_has_many_uniq_through_count
+  def test_has_many_distinct_through_count
     author = authors(:mary)
     assert !authors(:mary).unique_categorized_posts.loaded?
     assert_queries(1) { assert_equal 1, author.unique_categorized_posts.count }
@@ -49,7 +49,7 @@ class AssociationsJoinModelTest < ActiveRecord::TestCase
     assert !authors(:mary).unique_categorized_posts.loaded?
   end
 
-  def test_has_many_uniq_through_find
+  def test_has_many_distinct_through_find
     assert_equal 1, authors(:mary).unique_categorized_posts.to_a.size
   end
 
@@ -625,7 +625,7 @@ class AssociationsJoinModelTest < ActiveRecord::TestCase
     assert_equal [comments(:does_it_hurt)], authors(:david).special_post_comments
   end
 
-  def test_uniq_has_many_through_should_retain_order
+  def test_distinct_has_many_through_should_retain_order
     comment_ids = authors(:david).comments.map(&:id)
     assert_equal comment_ids.sort, authors(:david).ordered_uniq_comments.map(&:id)
     assert_equal comment_ids.sort.reverse, authors(:david).ordered_uniq_comments_desc.map(&:id)
diff --git a/activerecord/test/cases/base_test.rb b/activerecord/test/cases/base_test.rb
index 4306738670..31c31e4329 100644
--- a/activerecord/test/cases/base_test.rb
+++ b/activerecord/test/cases/base_test.rb
@@ -1411,15 +1411,13 @@ class BasicsTest < ActiveRecord::TestCase
   end
 
   def test_uniq_delegates_to_scoped
-    scope = stub
-    Bird.stubs(:all).returns(mock(:uniq => scope))
-    assert_equal scope, Bird.uniq
+    assert_deprecated do
+      assert_equal Bird.all.distinct, Bird.uniq
+    end
   end
 
   def test_distinct_delegates_to_scoped
-    scope = stub
-    Bird.stubs(:all).returns(mock(:distinct => scope))
-    assert_equal scope, Bird.distinct
+    assert_equal Bird.all.distinct, Bird.distinct
   end
 
   def test_table_name_with_2_abstract_subclasses
diff --git a/activerecord/test/cases/calculations_test.rb b/activerecord/test/cases/calculations_test.rb
index 8fc996ee74..b246eae5f5 100644
--- a/activerecord/test/cases/calculations_test.rb
+++ b/activerecord/test/cases/calculations_test.rb
@@ -359,7 +359,10 @@ class CalculationsTest < ActiveRecord::TestCase
 
   def test_count_with_distinct
     assert_equal 4, Account.select(:credit_limit).distinct.count
-    assert_equal 4, Account.select(:credit_limit).uniq.count
+
+    assert_deprecated do
+      assert_equal 4, Account.select(:credit_limit).uniq.count
+    end
   end
 
   def test_count_with_aliased_attribute
@@ -504,8 +507,8 @@ class CalculationsTest < ActiveRecord::TestCase
     assert_equal [ topic.written_on ], relation.pluck(:written_on)
   end
 
-  def test_pluck_and_uniq
-    assert_equal [50, 53, 55, 60], Account.order(:credit_limit).uniq.pluck(:credit_limit)
+  def test_pluck_and_distinct
+    assert_equal [50, 53, 55, 60], Account.order(:credit_limit).distinct.pluck(:credit_limit)
   end
 
   def test_pluck_in_relation
diff --git a/activerecord/test/cases/relation/mutation_test.rb b/activerecord/test/cases/relation/mutation_test.rb
index 45ead08bd5..2f2987e42d 100644
--- a/activerecord/test/cases/relation/mutation_test.rb
+++ b/activerecord/test/cases/relation/mutation_test.rb
@@ -81,7 +81,7 @@ module ActiveRecord
       assert_equal [], relation.extending_values
     end
 
-    (Relation::SINGLE_VALUE_METHODS - [:lock, :reordering, :reverse_order, :create_with]).each do |method|
+    (Relation::SINGLE_VALUE_METHODS - [:lock, :reordering, :reverse_order, :create_with, :uniq]).each do |method|
       test "##{method}!" do
         assert relation.public_send("#{method}!", :foo).equal?(relation)
         assert_equal :foo, relation.public_send("#{method}_value")
@@ -153,13 +153,22 @@ module ActiveRecord
     test 'distinct!' do
       relation.distinct! :foo
       assert_equal :foo, relation.distinct_value
-      assert_equal :foo, relation.uniq_value # deprecated access
+
+      assert_deprecated do
+        assert_equal :foo, relation.uniq_value # deprecated access
+      end
     end
 
     test 'uniq! was replaced by distinct!' do
-      relation.uniq! :foo
+      assert_deprecated(/use distinct! instead/) do
+        relation.uniq! :foo
+      end
+
+      e = assert_deprecated(/use distinct_value instead/) do
+        assert_equal :foo, relation.uniq_value # deprecated access
+      end
+
       assert_equal :foo, relation.distinct_value
-      assert_equal :foo, relation.uniq_value # deprecated access
     end
   end
 end
diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb
index b8e2041b6d..a4ca3ab637 100644
--- a/activerecord/test/cases/relations_test.rb
+++ b/activerecord/test/cases/relations_test.rb
@@ -908,7 +908,7 @@ class RelationTest < ActiveRecord::TestCase
 
   def test_delete_all_with_unpermitted_relation_raises_error
     assert_raises(ActiveRecord::ActiveRecordError) { Author.limit(10).delete_all }
-    assert_raises(ActiveRecord::ActiveRecordError) { Author.uniq.delete_all }
+    assert_raises(ActiveRecord::ActiveRecordError) { Author.distinct.delete_all }
     assert_raises(ActiveRecord::ActiveRecordError) { Author.group(:name).delete_all }
     assert_raises(ActiveRecord::ActiveRecordError) { Author.having('SUM(id) < 3').delete_all }
     assert_raises(ActiveRecord::ActiveRecordError) { Author.offset(10).delete_all }
@@ -1493,14 +1493,17 @@ class RelationTest < ActiveRecord::TestCase
     assert_equal ['Foo', 'Foo'], query.map(&:name)
     assert_sql(/DISTINCT/) do
       assert_equal ['Foo'], query.distinct.map(&:name)
-      assert_equal ['Foo'], query.uniq.map(&:name)
+      assert_deprecated { assert_equal ['Foo'], query.uniq.map(&:name) }
     end
     assert_sql(/DISTINCT/) do
       assert_equal ['Foo'], query.distinct(true).map(&:name)
-      assert_equal ['Foo'], query.uniq(true).map(&:name)
+      assert_deprecated { assert_equal ['Foo'], query.uniq(true).map(&:name) }
     end
     assert_equal ['Foo', 'Foo'], query.distinct(true).distinct(false).map(&:name)
-    assert_equal ['Foo', 'Foo'], query.uniq(true).uniq(false).map(&:name)
+
+    assert_deprecated do
+      assert_equal ['Foo', 'Foo'], query.uniq(true).uniq(false).map(&:name)
+    end
   end
 
   def test_doesnt_add_having_values_if_options_are_blank