Merge pull request rails#46503 from joshuay03/update-calculations-ids

Fix: `ActiveRecord::Calculations#ids` returns duplicate ids

Author: byroot

activerecord/CHANGELOG.md

@@ -1,3 +1,19 @@
+*   Return only unique ids from ActiveRecord::Calculations#ids
+
+    Updated ActiveRecord::Calculations#ids to only return the unique ids of the base model
+    when using eager_load, preload and includes.
+
+    ```ruby
+    Post.find_by(id: 1).comments.count
+    # => 5
+    Post.includes(:comments).where(id: 1).pluck(:id)
+    # => [1, 1, 1, 1, 1]
+    Post.includes(:comments).where(id: 1).ids
+    # => [1]
+    ```
+
+    *Joshua Young*
+
 *   Stop using `LOWER()` for case-insensitive queries on `citext` columns
 
     Previously, `LOWER()` was added for e.g. uniqueness validations with

activerecord/lib/active_record/querying.rb

@@ -17,7 +17,7 @@ module Querying
       :and, :or, :annotate, :optimizer_hints, :extending,
       :having, :create_with, :distinct, :references, :none, :unscope, :merge, :except, :only,
       :count, :average, :minimum, :maximum, :sum, :calculate,
-      :pluck, :pick, :ids, :strict_loading, :excluding, :without, :with,
+      :pluck, :pick, :ids, :async_ids, :strict_loading, :excluding, :without, :with,
       :async_count, :async_average, :async_minimum, :async_maximum, :async_sum, :async_pluck, :async_pick,
     ].freeze # :nodoc:
     delegate(*QUERYING_METHODS, to: :all)

activerecord/lib/active_record/relation/calculations.rb

@@ -317,12 +317,33 @@ def async_pick(*column_names)
       async.pick(*column_names)
     end
 
-    # Pluck all the ID's for the relation using the table's primary key
+    # Returns the base model's ID's for the relation using the table's primary key
     #
     #   Person.ids # SELECT people.id FROM people
     #   Person.joins(:companies).ids # SELECT people.id FROM people INNER JOIN companies ON companies.person_id = people.id
     def ids
-      pluck primary_key
+      if loaded?
+        result = records.pluck(primary_key)
+        return @async ? Promise::Complete.new(result) : result
+      end
+
+      columns = arel_columns([primary_key])
+      relation = spawn
+      relation.select_values = columns
+      result = if relation.where_clause.contradiction?
+        ActiveRecord::Result.empty
+      else
+        skip_query_cache_if_necessary do
+          klass.connection.select_all(relation, "#{klass.name} Ids", async: @async)
+        end
+      end
+
+      result.then { |result| type_cast_pluck_values(result, columns) }
+    end
+
+    # Same as <tt>#ids</tt> but perform the query asynchronously and returns an <tt>ActiveRecord::Promise</tt>
+    def async_ids
+      async.ids
     end
 
     private

activerecord/test/cases/calculations_test.rb

@@ -928,8 +928,69 @@ def test_pluck_with_selection_clause
     assert_equal [50 + 53 + 55 + 60], Account.pluck(Arel.sql("SUM(DISTINCT(credit_limit))"))
   end
 
-  def test_plucks_with_ids
-    assert_equal Company.all.map(&:id).sort, Company.ids.sort
+  def test_ids
+    assert_equal Company.all.map(&:id).sort, Company.all.ids.sort
+  end
+
+  def test_ids_with_scope
+    scoped_ids = [1, 2]
+    assert_equal Company.where(id: scoped_ids).map(&:id).sort, Company.where(id: scoped_ids).ids.sort
+  end
+
+  def test_ids_on_loaded_relation
+    loaded_companies = Company.all.load
+    company_ids = Company.all.map(&:id)
+    assert_queries(0) do
+      assert_equal company_ids.sort, loaded_companies.ids.sort
+    end
+  end
+
+  def test_ids_on_loaded_relation_with_scope
+    scoped_ids = [1, 2]
+    loaded_companies = Company.where(id: scoped_ids).load
+    company_ids = Company.where(id: scoped_ids).map(&:id)
+    assert_queries(0) do
+      assert_equal company_ids.sort, loaded_companies.ids.sort
+    end
+  end
+
+  def test_ids_async_on_loaded_relation
+    loaded_companies = Company.all.order(:id).load
+    assert_async_equal loaded_companies.ids, loaded_companies.async_ids
+  end
+
+  def test_ids_with_contradicting_scope
+    empty_scope_ids = []
+    company_ids = Company.where(id: empty_scope_ids).map(&:id)
+    assert_predicate company_ids, :empty?
+    assert_queries(0) do
+      assert_equal company_ids, Company.where(id: empty_scope_ids).ids
+    end
+  end
+
+  def test_ids_with_eager_load
+    company = Company.first
+    5.times { company.contracts.create! }
+    assert_equal Company.all.map(&:id).sort, Company.all.eager_load(:contracts).ids.sort
+  end
+
+  def test_ids_with_preload
+    company = Company.first
+    5.times { company.contracts.create! }
+    assert_equal Company.all.map(&:id).sort, Company.all.preload(:contracts).ids.sort
+  end
+
+  def test_ids_with_includes
+    company = Company.first
+    5.times { company.contracts.create! }
+    assert_equal Company.all.map(&:id).sort, Company.all.includes(:contracts).ids.sort
+  end
+
+  def test_ids_with_scope_and_includes
+    scoped_ids = [1, 2]
+    company = Company.where(id: scoped_ids).first
+    5.times { company.contracts.create! }
+    assert_equal Company.where(id: scoped_ids).map(&:id).sort, Company.where(id: scoped_ids).includes(:contracts).ids.sort
   end
 
   def test_pluck_with_includes_limit_and_empty_result

activerecord/test/cases/relation_test.rb

@@ -256,9 +256,9 @@ def test_relation_with_merged_joins_aliased_works
       posts_with_joins_and_merges = Post.joins(:author, :categorizations)
                                         .merge(Author.select(:id)).merge(categorizations_with_authors)
 
-      author_with_posts = Author.joins(:posts).ids
-      categorizations_with_author = Categorization.joins(:author).ids
-      posts_with_author_and_categorizations = Post.joins(:categorizations).where(author_id: author_with_posts, categorizations: { id: categorizations_with_author }).ids
+      author_with_posts = Author.joins(:posts).pluck(:id)
+      categorizations_with_author = Categorization.joins(:author).pluck(:id)
+      posts_with_author_and_categorizations = Post.joins(:categorizations).where(author_id: author_with_posts, categorizations: { id: categorizations_with_author }).pluck(:id)
 
       assert_equal posts_with_author_and_categorizations.size, posts_with_joins_and_merges.count
       assert_equal posts_with_author_and_categorizations.size, posts_with_joins_and_merges.to_a.size