Skip to content

Commit

Permalink
deprecate Relation#uniq use Relation#distinct instead.
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
senny committed May 26, 2015
1 parent b8c31fd commit adfab2d
Show file tree
Hide file tree
Showing 12 changed files with 52 additions and 31 deletions.
6 changes: 6 additions & 0 deletions activerecord/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -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.

Expand Down
1 change: 0 additions & 1 deletion activerecord/lib/active_record/associations.rb
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,6 @@ def association_instance_set(name, association)
# 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
Expand Down
3 changes: 2 additions & 1 deletion activerecord/lib/active_record/relation.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ class Relation
: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]

Expand Down Expand Up @@ -618,6 +618,7 @@ def joined_includes_values
def uniq_value
distinct_value
end
deprecate uniq_value: :distinct_value

# Compares two relations for equality.
def ==(other)
Expand Down
4 changes: 3 additions & 1 deletion activerecord/lib/active_record/relation/query_methods.rb
Original file line number Diff line number Diff line change
Expand Up @@ -587,7 +587,7 @@ def rewhere(conditions)
#
# 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'))
Expand Down Expand Up @@ -790,13 +790,15 @@ def distinct(value = true)
spawn.distinct!(value)
end
alias uniq distinct
deprecate uniq: :distinct

# Like #distinct, but modifies relation in place.
def distinct!(value = true) # :nodoc:
self.distinct_value = value
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.
Expand Down
2 changes: 1 addition & 1 deletion activerecord/test/cases/associations/eager_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1167,7 +1167,7 @@ def test_preloading_through_empty_belongs_to
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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ def test_habtm_saving_multiple_relationships
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
Expand Down Expand Up @@ -339,7 +339,7 @@ def test_creation_respects_hash_condition
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)
Expand All @@ -348,13 +348,13 @@ def test_uniq_after_the_fact
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)
Expand All @@ -365,7 +365,7 @@ def test_uniq_option_prevents_duplicate_push
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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -960,7 +960,7 @@ def test_create_has_many_through_with_default_scope_on_join_model
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
Expand Down
8 changes: 4 additions & 4 deletions activerecord/test/cases/associations/join_model_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,12 @@ def test_inherited_has_many
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 }
Expand All @@ -49,7 +49,7 @@ def test_has_many_uniq_through_count
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

Expand Down Expand Up @@ -625,7 +625,7 @@ def test_has_many_through_has_many_with_sti
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)
Expand Down
10 changes: 4 additions & 6 deletions activerecord/test/cases/base_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1411,15 +1411,13 @@ def test_touch_should_raise_error_on_a_new_object
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
Expand Down
9 changes: 6 additions & 3 deletions activerecord/test/cases/calculations_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,10 @@ def test_count_with_column_parameter

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
Expand Down Expand Up @@ -504,8 +507,8 @@ def test_pluck_type_cast
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
Expand Down
17 changes: 13 additions & 4 deletions activerecord/test/cases/relation/mutation_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ def relation
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")
Expand Down Expand Up @@ -153,13 +153,22 @@ def relation
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
11 changes: 7 additions & 4 deletions activerecord/test/cases/relations_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -908,7 +908,7 @@ def test_delete_all_loaded

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 }
Expand Down Expand Up @@ -1493,14 +1493,17 @@ def test_distinct
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
Expand Down

0 comments on commit adfab2d

Please sign in to comment.