Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

Merge pull request #9683 from senny/deprecate_count_distinct_option

rename `Relation#uniq` to `Relation#distinct`
  • Loading branch information...
commit 0721d3b37062eca73da3efd669142d7e381e4d80 2 parents df21d1e + cd87c85
@jonleighton jonleighton authored
Showing with 129 additions and 59 deletions.
  1. +20 −0 activerecord/CHANGELOG.md
  2. +1 −0  activerecord/lib/active_record/associations.rb
  3. +9 −7 activerecord/lib/active_record/associations/collection_association.rb
  4. +4 −3 activerecord/lib/active_record/associations/collection_proxy.rb
  5. +1 −1  activerecord/lib/active_record/associations/preloader/has_many_through.rb
  6. +1 −1  activerecord/lib/active_record/querying.rb
  7. +7 −1 activerecord/lib/active_record/relation.rb
  8. +8 −3 activerecord/lib/active_record/relation/calculations.rb
  9. +11 −9 activerecord/lib/active_record/relation/query_methods.rb
  10. +1 −1  activerecord/test/cases/associations/cascaded_eager_loading_test.rb
  11. +1 −1  activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb
  12. +1 −1  activerecord/test/cases/associations/inner_join_association_test.rb
  13. +2 −2 activerecord/test/cases/associations/join_model_test.rb
  14. +1 −1  activerecord/test/cases/associations/nested_through_associations_test.rb
  15. +7 −1 activerecord/test/cases/base_test.rb
  16. +16 −5 activerecord/test/cases/calculations_test.rb
  17. +1 −1  activerecord/test/cases/finder_test.rb
  18. +12 −0 activerecord/test/cases/relation_test.rb
  19. +9 −5 activerecord/test/cases/relations_test.rb
  20. +4 −4 activerecord/test/models/author.rb
  21. +1 −1  activerecord/test/models/book.rb
  22. +1 −2  activerecord/test/models/liquid.rb
  23. +4 −4 activerecord/test/models/project.rb
  24. +6 −5 guides/source/active_record_querying.md
View
20 activerecord/CHANGELOG.md
@@ -1,5 +1,25 @@
## Rails 4.0.0 (unreleased) ##
+* The `:distinct` option for `Relation#count` is deprecated. You
+ should use `Relation#distinct` instead.
+
+ Example:
+
+ # Before
+ Post.select(:author_name).count(distinct: true)
+
+ # After
+ Post.select(:author_name).distinct.count
+
+ *Yves Senn*
+
+* Rename `Relation#uniq` to `Relation#distinct`. `#uniq` is still
+ available as an alias but we encourage to use `#distinct` instead.
+ Also `Relation#uniq_value` is aliased to `Relation#distinct_value`,
+ this is a temporary solution and you should migrate to `distinct_value`.
+
+ *Yves Senn*
+
* Fix quoting for sqlite migrations using copy_table_contents() with binary
columns.
View
1  activerecord/lib/active_record/associations.rb
@@ -241,6 +241,7 @@ def association_instance_set(name, association)
# others.destroy_all | X | X | X
# 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
#
View
16 activerecord/lib/active_record/associations/collection_association.rb
@@ -174,13 +174,14 @@ def count(column_name = nil, count_options = {})
reflection.klass.count_by_sql(custom_counter_sql)
else
- if association_scope.uniq_value
+ relation = scope
+ if association_scope.distinct_value
# This is needed because 'SELECT count(DISTINCT *)..' is not valid SQL.
column_name ||= reflection.klass.primary_key
- count_options[:distinct] = true
+ relation = relation.distinct
end
- value = scope.count(column_name, count_options)
+ value = relation.count(column_name)
limit = options[:limit]
offset = options[:offset]
@@ -246,14 +247,14 @@ def destroy(*records)
# +count_records+, which is a method descendants have to provide.
def size
if !find_target? || loaded?
- if association_scope.uniq_value
+ if association_scope.distinct_value
target.uniq.size
else
target.size
end
elsif !loaded? && !association_scope.group_values.empty?
load_target.size
- elsif !loaded? && !association_scope.uniq_value && target.is_a?(Array)
+ elsif !loaded? && !association_scope.distinct_value && target.is_a?(Array)
unsaved_records = target.select { |r| r.new_record? }
unsaved_records.size + count_records
else
@@ -306,12 +307,13 @@ def many?
end
end
- def uniq
+ def distinct
seen = {}
load_target.find_all do |record|
seen[record.id] = true unless seen.key?(record.id)
end
end
+ alias uniq distinct
# Replace this collection with +other_array+. This will perform a diff
# and delete/add only records that have changed.
@@ -352,7 +354,7 @@ def add_to_target(record)
callback(:before_add, record)
yield(record) if block_given?
- if association_scope.uniq_value && index = @target.index(record)
+ if association_scope.distinct_value && index = @target.index(record)
@target[index] = record
else
@target << record
View
7 activerecord/lib/active_record/associations/collection_proxy.rb
@@ -649,11 +649,12 @@ def destroy(*records)
# # #<Pet name: "Fancy-Fancy">
# # ]
#
- # person.pets.select(:name).uniq
+ # person.pets.select(:name).distinct
# # => [#<Pet name: "Fancy-Fancy">]
- def uniq
- @association.uniq
+ def distinct
+ @association.distinct
end
+ alias uniq distinct
# Count all records using SQL.
#
View
2  activerecord/lib/active_record/associations/preloader/has_many_through.rb
@@ -6,7 +6,7 @@ class HasManyThrough < CollectionAssociation #:nodoc:
def associated_records_by_owner
super.each do |owner, records|
- records.uniq! if reflection_scope.uniq_value
+ records.uniq! if reflection_scope.distinct_value
end
end
end
View
2  activerecord/lib/active_record/querying.rb
@@ -8,7 +8,7 @@ module Querying
delegate :find_each, :find_in_batches, :to => :all
delegate :select, :group, :order, :except, :reorder, :limit, :offset, :joins,
:where, :preload, :eager_load, :includes, :from, :lock, :readonly,
- :having, :create_with, :uniq, :references, :none, :to => :all
+ :having, :create_with, :uniq, :distinct, :references, :none, :to => :all
delegate :count, :average, :minimum, :maximum, :sum, :calculate, :pluck, :ids, :to => :all
# Executes a custom SQL query against your database and returns all the results. The results will
View
8 activerecord/lib/active_record/relation.rb
@@ -10,7 +10,7 @@ class Relation
:extending]
SINGLE_VALUE_METHODS = [:limit, :offset, :lock, :readonly, :from, :reordering,
- :reverse_order, :uniq, :create_with]
+ :reverse_order, :distinct, :create_with]
VALUE_METHODS = MULTI_VALUE_METHODS + SINGLE_VALUE_METHODS
@@ -506,6 +506,12 @@ def joined_includes_values
includes_values & joins_values
end
+ # +uniq+ and +uniq!+ are silently deprecated. +uniq_value+ delegates to +distinct_value+
+ # to maintain backwards compatibility. Use +distinct_value+ instead.
+ def uniq_value
+ distinct_value
+ end
+
# Compares two relations for equality.
def ==(other)
case other
View
11 activerecord/lib/active_record/relation/calculations.rb
@@ -11,7 +11,7 @@ module Calculations
# Person.count(:all)
# # => performs a COUNT(*) (:all is an alias for '*')
#
- # Person.count(:age, distinct: true)
+ # Person.distinct.count(:age)
# # => counts the number of different age values
#
# If +count+ is used with +group+, it returns a Hash whose keys represent the aggregated column,
@@ -198,8 +198,13 @@ def has_include?(column_name)
def perform_calculation(operation, column_name, options = {})
operation = operation.to_s.downcase
- # If #count is used in conjuction with #uniq it is considered distinct. (eg. relation.uniq.count)
- distinct = options[:distinct] || self.uniq_value
+ # If #count is used with #distinct / #uniq it is considered distinct. (eg. relation.distinct.count)
+ distinct = self.distinct_value
+ if options.has_key?(:distinct)
+ ActiveSupport::Deprecation.warn "The :distinct option for `Relation#count` is deprecated. " \
+ "Please use `Relation#distinct` instead. (eg. `relation.distinct.count`)"
+ distinct = options[:distinct]
+ end
if operation == "count"
column_name ||= (select_for_count || :all)
View
20 activerecord/lib/active_record/relation/query_methods.rb
@@ -710,20 +710,22 @@ def from!(value, subquery_name = nil) # :nodoc:
# User.select(:name)
# # => Might return two records with the same name
#
- # User.select(:name).uniq
- # # => Returns 1 record per unique name
+ # User.select(:name).distinct
+ # # => Returns 1 record per distinct name
#
- # User.select(:name).uniq.uniq(false)
+ # User.select(:name).distinct.distinct(false)
# # => You can also remove the uniqueness
- def uniq(value = true)
- spawn.uniq!(value)
+ def distinct(value = true)
+ spawn.distinct!(value)
end
+ alias uniq distinct
- # Like #uniq, but modifies relation in place.
- def uniq!(value = true) # :nodoc:
- self.uniq_value = value
+ # Like #distinct, but modifies relation in place.
+ def distinct!(value = true) # :nodoc:
+ self.distinct_value = value
self
end
+ alias uniq! distinct!
# Used to extend a scope with additional methods, either through
# a module or through a block provided.
@@ -814,7 +816,7 @@ def build_arel
build_select(arel, select_values.uniq)
- arel.distinct(uniq_value)
+ arel.distinct(distinct_value)
arel.from(build_from) if from_value
arel.lock(lock_value) if lock_value
View
2  activerecord/test/cases/associations/cascaded_eager_loading_test.rb
@@ -55,7 +55,7 @@ def test_cascaded_eager_association_loading_with_join_for_count
assert_nothing_raised do
assert_equal 4, categories.count
assert_equal 4, categories.to_a.count
- assert_equal 3, categories.count(:distinct => true)
+ assert_equal 3, categories.distinct.count
assert_equal 3, categories.to_a.uniq.size # Must uniq since instantiating with inner joins will get dupes
end
end
View
2  activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb
@@ -316,7 +316,7 @@ def test_uniq_after_the_fact
dev.projects << projects(:active_record)
assert_equal 3, dev.projects.size
- assert_equal 1, dev.projects.uniq.size
+ assert_equal 1, dev.projects.distinct.size
end
def test_uniq_before_the_fact
View
2  activerecord/test/cases/associations/inner_join_association_test.rb
@@ -82,7 +82,7 @@ def test_calculate_honors_implicit_inner_joins
def test_calculate_honors_implicit_inner_joins_and_distinct_and_conditions
real_count = Author.all.to_a.select {|a| a.posts.any? {|p| p.title =~ /^Welcome/} }.length
- authors_with_welcoming_post_titles = Author.all.merge!(:joins => :posts, :where => "posts.title like 'Welcome%'").calculate(:count, 'authors.id', :distinct => true)
+ authors_with_welcoming_post_titles = Author.all.merge!(joins: :posts, where: "posts.title like 'Welcome%'").distinct.calculate(:count, 'authors.id')
assert_equal real_count, authors_with_welcoming_post_titles, "inner join and conditions should have only returned authors posting titles starting with 'Welcome'"
end
View
4 activerecord/test/cases/associations/join_model_test.rb
@@ -397,14 +397,14 @@ def test_has_many_through_polymorphic_has_one
end
def test_has_many_through_polymorphic_has_many
- assert_equal taggings(:welcome_general, :thinking_general), authors(:david).taggings.uniq.sort_by { |t| t.id }
+ assert_equal taggings(:welcome_general, :thinking_general), authors(:david).taggings.distinct.sort_by { |t| t.id }
end
def test_include_has_many_through_polymorphic_has_many
author = Author.includes(:taggings).find authors(:david).id
expected_taggings = taggings(:welcome_general, :thinking_general)
assert_no_queries do
- assert_equal expected_taggings, author.taggings.uniq.sort_by { |t| t.id }
+ assert_equal expected_taggings, author.taggings.distinct.sort_by { |t| t.id }
end
end
View
2  activerecord/test/cases/associations/nested_through_associations_test.rb
@@ -410,7 +410,7 @@ def test_nested_has_many_through_with_a_table_referenced_multiple_times
# Mary and Bob both have posts in misc, but they are the only ones.
authors = Author.joins(:similar_posts).where('posts.id' => posts(:misc_by_bob).id)
- assert_equal [authors(:mary), authors(:bob)], authors.uniq.sort_by(&:id)
+ assert_equal [authors(:mary), authors(:bob)], authors.distinct.sort_by(&:id)
# Check the polymorphism of taggings is being observed correctly (in both joins)
authors = Author.joins(:similar_posts).where('taggings.taggable_type' => 'FakeModel')
View
8 activerecord/test/cases/base_test.rb
@@ -1105,7 +1105,7 @@ def test_count_with_join
res6 = Post.count_by_sql "SELECT COUNT(DISTINCT p.id) FROM posts p, comments co WHERE p.#{QUOTED_TYPE} = 'Post' AND p.id=co.post_id"
res7 = nil
assert_nothing_raised do
- res7 = Post.where("p.#{QUOTED_TYPE} = 'Post' AND p.id=co.post_id").joins("p, comments co").select("p.id").count(distinct: true)
+ res7 = Post.where("p.#{QUOTED_TYPE} = 'Post' AND p.id=co.post_id").joins("p, comments co").select("p.id").distinct.count
end
assert_equal res6, res7
end
@@ -1499,6 +1499,12 @@ def test_uniq_delegates_to_scoped
assert_equal scope, Bird.uniq
end
+ def test_distinct_delegates_to_scoped
+ scope = stub
+ Bird.stubs(:all).returns(mock(:distinct => scope))
+ assert_equal scope, Bird.distinct
+ end
+
def test_table_name_with_2_abstract_subclasses
assert_equal "photos", Photo.table_name
end
View
21 activerecord/test/cases/calculations_test.rb
@@ -305,8 +305,8 @@ def test_should_group_by_summed_field_through_association_and_having
end
def test_should_count_selected_field_with_include
- assert_equal 6, Account.includes(:firm).count(:distinct => true)
- assert_equal 4, Account.includes(:firm).select(:credit_limit).count(:distinct => true)
+ assert_equal 6, Account.includes(:firm).distinct.count
+ assert_equal 4, Account.includes(:firm).distinct.select(:credit_limit).count
end
def test_should_not_perform_joined_include_by_default
@@ -341,7 +341,18 @@ def test_count_with_column_parameter
assert_equal 5, Account.count(:firm_id)
end
- def test_count_with_uniq
+ def test_count_distinct_option_is_deprecated
+ assert_deprecated do
+ assert_equal 4, Account.select(:credit_limit).count(distinct: true)
+ end
+
+ assert_deprecated do
+ assert_equal 6, Account.select(:credit_limit).count(distinct: false)
+ end
+ end
+
+ def test_count_with_distinct
+ assert_equal 4, Account.select(:credit_limit).distinct.count
assert_equal 4, Account.select(:credit_limit).uniq.count
end
@@ -351,7 +362,7 @@ def test_count_with_column_and_options_parameter
def test_should_count_field_in_joined_table
assert_equal 5, Account.joins(:firm).count('companies.id')
- assert_equal 4, Account.joins(:firm).count('companies.id', :distinct => true)
+ assert_equal 4, Account.joins(:firm).distinct.count('companies.id')
end
def test_should_count_field_in_joined_table_with_group_by
@@ -455,7 +466,7 @@ def test_distinct_is_honored_when_used_with_count_operation_after_group
approved_topics_count = Topic.group(:approved).count(:author_name)[true]
assert_equal approved_topics_count, 3
# Count the number of distinct authors for approved Topics
- distinct_authors_for_approved_count = Topic.group(:approved).count(:author_name, :distinct => true)[true]
+ distinct_authors_for_approved_count = Topic.group(:approved).distinct.count(:author_name)[true]
assert_equal distinct_authors_for_approved_count, 2
end
View
2  activerecord/test/cases/finder_test.rb
@@ -82,7 +82,7 @@ def test_exists_with_nil_arg
# ensures +exists?+ runs valid SQL by excluding order value
def test_exists_with_order
- assert Topic.order(:id).uniq.exists?
+ assert Topic.order(:id).distinct.exists?
end
def test_exists_with_includes_limit_and_empty_result
View
12 activerecord/test/cases/relation_test.rb
@@ -278,5 +278,17 @@ def relation
assert_equal [NullRelation], relation.extending_values
assert relation.is_a?(NullRelation)
end
+
+ test "distinct!" do
+ relation.distinct! :foo
+ assert_equal :foo, relation.distinct_value
+ assert_equal :foo, relation.uniq_value # deprecated access
+ end
+
+ test "uniq! was replaced by distinct!" do
+ relation.uniq! :foo
+ assert_equal :foo, relation.distinct_value
+ assert_equal :foo, relation.uniq_value # deprecated access
+ end
end
end
View
14 activerecord/test/cases/relations_test.rb
@@ -492,6 +492,7 @@ def test_dynamic_find_by_attributes
expected_taggings = taggings(:welcome_general, :thinking_general)
assert_no_queries do
+ assert_equal expected_taggings, author.taggings.distinct.sort_by { |t| t.id }
assert_equal expected_taggings, author.taggings.uniq.sort_by { |t| t.id }
end
@@ -789,11 +790,11 @@ def test_count
def test_count_with_distinct
posts = Post.all
- assert_equal 3, posts.count(:comments_count, :distinct => true)
- assert_equal 11, posts.count(:comments_count, :distinct => false)
+ assert_equal 3, posts.distinct(true).count(:comments_count)
+ assert_equal 11, posts.distinct(false).count(:comments_count)
- assert_equal 3, posts.select(:comments_count).count(:distinct => true)
- assert_equal 11, posts.select(:comments_count).count(:distinct => false)
+ assert_equal 3, posts.distinct(true).select(:comments_count).count
+ assert_equal 11, posts.distinct(false).select(:comments_count).count
end
def test_count_explicit_columns
@@ -1269,7 +1270,7 @@ def test_update_all_with_joins_and_offset_and_order
assert_equal posts(:welcome), comments(:greetings).post
end
- def test_uniq
+ def test_distinct
tag1 = Tag.create(:name => 'Foo')
tag2 = Tag.create(:name => 'Foo')
@@ -1277,11 +1278,14 @@ def test_uniq
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)
end
assert_sql(/DISTINCT/) do
+ assert_equal ['Foo'], query.distinct(true).map(&:name)
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)
end
View
8 activerecord/test/models/author.rb
@@ -30,8 +30,8 @@ class Author < ActiveRecord::Base
has_many :comments_desc, -> { order('comments.id DESC') }, :through => :posts, :source => :comments
has_many :limited_comments, -> { limit(1) }, :through => :posts, :source => :comments
has_many :funky_comments, :through => :posts, :source => :comments
- has_many :ordered_uniq_comments, -> { uniq.order('comments.id') }, :through => :posts, :source => :comments
- has_many :ordered_uniq_comments_desc, -> { uniq.order('comments.id DESC') }, :through => :posts, :source => :comments
+ has_many :ordered_uniq_comments, -> { distinct.order('comments.id') }, :through => :posts, :source => :comments
+ has_many :ordered_uniq_comments_desc, -> { distinct.order('comments.id DESC') }, :through => :posts, :source => :comments
has_many :readonly_comments, -> { readonly }, :through => :posts, :source => :comments
has_many :special_posts
@@ -78,7 +78,7 @@ class Author < ActiveRecord::Base
has_many :categories_like_general, -> { where(:name => 'General') }, :through => :categorizations, :source => :category, :class_name => 'Category'
has_many :categorized_posts, :through => :categorizations, :source => :post
- has_many :unique_categorized_posts, -> { uniq }, :through => :categorizations, :source => :post
+ has_many :unique_categorized_posts, -> { distinct }, :through => :categorizations, :source => :post
has_many :nothings, :through => :kateggorisatons, :class_name => 'Category'
@@ -91,7 +91,7 @@ class Author < ActiveRecord::Base
has_many :post_categories, :through => :posts, :source => :categories
has_many :tagging_tags, :through => :taggings, :source => :tag
- has_many :similar_posts, -> { uniq }, :through => :tags, :source => :tagged_posts
+ has_many :similar_posts, -> { distinct }, :through => :tags, :source => :tagged_posts
has_many :distinct_tags, -> { select("DISTINCT tags.*").order("tags.name") }, :through => :posts, :source => :tags
has_many :tags_with_primary_key, :through => :posts
View
2  activerecord/test/models/book.rb
@@ -2,7 +2,7 @@ class Book < ActiveRecord::Base
has_many :authors
has_many :citations, :foreign_key => 'book1_id'
- has_many :references, -> { uniq }, :through => :citations, :source => :reference_of
+ has_many :references, -> { distinct }, :through => :citations, :source => :reference_of
has_many :subscriptions
has_many :subscribers, :through => :subscriptions
View
3  activerecord/test/models/liquid.rb
@@ -1,5 +1,4 @@
class Liquid < ActiveRecord::Base
self.table_name = :liquid
- has_many :molecules, -> { uniq }
+ has_many :molecules, -> { distinct }
end
-
View
8 activerecord/test/models/project.rb
@@ -1,11 +1,11 @@
class Project < ActiveRecord::Base
- has_and_belongs_to_many :developers, -> { uniq.order 'developers.name desc, developers.id desc' }
+ has_and_belongs_to_many :developers, -> { distinct.order 'developers.name desc, developers.id desc' }
has_and_belongs_to_many :readonly_developers, -> { readonly }, :class_name => "Developer"
- has_and_belongs_to_many :selected_developers, -> { uniq.select "developers.*" }, :class_name => "Developer"
+ has_and_belongs_to_many :selected_developers, -> { distinct.select "developers.*" }, :class_name => "Developer"
has_and_belongs_to_many :non_unique_developers, -> { order 'developers.name desc, developers.id desc' }, :class_name => 'Developer'
has_and_belongs_to_many :limited_developers, -> { limit 1 }, :class_name => "Developer"
- 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 :developers_named_david, -> { where("name = 'David'").distinct }, :class_name => "Developer"
+ has_and_belongs_to_many :developers_named_david_with_hash_conditions, -> { where(:name => 'David').distinct }, :class_name => "Developer"
has_and_belongs_to_many :salaried_developers, -> { where "salary > 0" }, :class_name => "Developer"
ActiveSupport::Deprecation.silence do
View
11 guides/source/active_record_querying.md
@@ -76,6 +76,7 @@ The methods are:
* `reorder`
* `reverse_order`
* `select`
+* `distinct`
* `uniq`
* `where`
@@ -580,10 +581,10 @@ ActiveModel::MissingAttributeError: missing attribute: <attribute>
Where `<attribute>` is the attribute you asked for. The `id` method will not raise the `ActiveRecord::MissingAttributeError`, so just be careful when working with associations because they need the `id` method to function properly.
-If you would like to only grab a single record per unique value in a certain field, you can use `uniq`:
+If you would like to only grab a single record per unique value in a certain field, you can use `distinct`:
```ruby
-Client.select(:name).uniq
+Client.select(:name).distinct
```
This would generate SQL like:
@@ -595,10 +596,10 @@ SELECT DISTINCT name FROM clients
You can also remove the uniqueness constraint:
```ruby
-query = Client.select(:name).uniq
+query = Client.select(:name).distinct
# => Returns unique names
-query.uniq(false)
+query.distinct(false)
# => Returns all names, even if there are duplicates
```
@@ -1438,7 +1439,7 @@ Client.where(active: true).pluck(:id)
# SELECT id FROM clients WHERE active = 1
# => [1, 2, 3]
-Client.uniq.pluck(:role)
+Client.distinct.pluck(:role)
# SELECT DISTINCT role FROM clients
# => ['admin', 'member', 'guest']
Please sign in to comment.
Something went wrong with that request. Please try again.