aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
Diffstat
-rw-r--r--actionview/test/template/text_test.rb2
-rw-r--r--activerecord/CHANGELOG.md7
-rw-r--r--activerecord/lib/active_record/core.rb3
-rw-r--r--activerecord/lib/active_record/relation/calculations.rb11
-rw-r--r--activerecord/test/cases/bind_parameter_test.rb21
-rw-r--r--activerecord/test/cases/calculations_test.rb18
-rw-r--r--activerecord/test/cases/date_time_precision_test.rb22
-rw-r--r--activerecord/test/cases/inheritance_test.rb3
-rw-r--r--activerecord/test/cases/relations_test.rb17
-rw-r--r--activerecord/test/cases/time_precision_test.rb20
-rw-r--r--activestorage/lib/active_storage/attached/changes/create_one.rb1
-rw-r--r--activestorage/test/models/attached/many_test.rb3
-rw-r--r--activestorage/test/models/attached/one_test.rb3
13 files changed, 116 insertions, 15 deletions
diff --git a/actionview/test/template/text_test.rb b/actionview/test/template/text_test.rb
index e7e3b6aae8..c837c53587 100644
--- a/actionview/test/template/text_test.rb
+++ b/actionview/test/template/text_test.rb
@@ -3,7 +3,7 @@
require "abstract_unit"
class TextTest < ActiveSupport::TestCase
- test "formats always return :text" do
+ test "format always return :text" do
assert_equal :text, ActionView::Template::Text.new("").format
end
diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 77cb167e7c..4323c57b9e 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,10 @@
+* Fix different `count` calculation when using `size` with manual `select` with DISTINCT.
+
+ Fixes #35214.
+
+ *Juani Villarejo*
+
+
## Rails 6.0.0.beta2 (February 25, 2019) ##
* Fix prepared statements caching to be enabled even when query caching is enabled.
diff --git a/activerecord/lib/active_record/core.rb b/activerecord/lib/active_record/core.rb
index c67980173f..18cfac1f2f 100644
--- a/activerecord/lib/active_record/core.rb
+++ b/activerecord/lib/active_record/core.rb
@@ -182,7 +182,8 @@ module ActiveRecord
end
def find_by(*args) # :nodoc:
- return super if scope_attributes? || reflect_on_all_aggregations.any?
+ return super if scope_attributes? || reflect_on_all_aggregations.any? ||
+ columns_hash.key?(inheritance_column) && !base_class?
hash = args.first
diff --git a/activerecord/lib/active_record/relation/calculations.rb b/activerecord/lib/active_record/relation/calculations.rb
index bdd3c540bb..4f9ddf302e 100644
--- a/activerecord/lib/active_record/relation/calculations.rb
+++ b/activerecord/lib/active_record/relation/calculations.rb
@@ -221,7 +221,6 @@ module ActiveRecord
end
private
-
def has_include?(column_name)
eager_loading? || (includes_values.present? && column_name && column_name != :all)
end
@@ -236,10 +235,12 @@ module ActiveRecord
if operation == "count"
column_name ||= select_for_count
if column_name == :all
- if distinct && (group_values.any? || select_values.empty? && order_values.empty?)
+ if !distinct
+ distinct = distinct_select?(select_for_count) if group_values.empty?
+ elsif group_values.any? || select_values.empty? && order_values.empty?
column_name = primary_key
end
- elsif column_name.is_a?(::String) && /\bDISTINCT[\s(]/i.match?(column_name)
+ elsif distinct_select?(column_name)
distinct = nil
end
end
@@ -251,6 +252,10 @@ module ActiveRecord
end
end
+ def distinct_select?(column_name)
+ column_name.is_a?(::String) && /\bDISTINCT[\s(]/i.match?(column_name)
+ end
+
def aggregate_column(column_name)
return column_name if Arel::Expressions === column_name
diff --git a/activerecord/test/cases/bind_parameter_test.rb b/activerecord/test/cases/bind_parameter_test.rb
index 8c3fe0437c..b89f054821 100644
--- a/activerecord/test/cases/bind_parameter_test.rb
+++ b/activerecord/test/cases/bind_parameter_test.rb
@@ -2,6 +2,7 @@
require "cases/helper"
require "models/topic"
+require "models/reply"
require "models/author"
require "models/post"
@@ -53,12 +54,17 @@ if ActiveRecord::Base.connection.prepared_statements
@connection.disable_query_cache!
end
- def test_statement_cache_with_find
+ def test_statement_cache_with_find_by
@connection.clear_cache!
- topics = Topic.where(id: 1).limit(1)
- assert_equal 1, Topic.find(1).id
- assert_includes statement_cache, to_sql_key(topics.arel)
+ assert_equal 1, Topic.find_by!(id: 1).id
+ assert_equal 2, Reply.find_by!(id: 2).id
+
+ topic_sql = cached_statement(Topic, [:id])
+ assert_includes statement_cache, to_sql_key(topic_sql)
+
+ e = assert_raise { cached_statement(Reply, [:id]) }
+ assert_equal "Reply has no cached statement by [:id]", e.message
end
def test_statement_cache_with_in_clause
@@ -139,6 +145,13 @@ if ActiveRecord::Base.connection.prepared_statements
@connection.respond_to?(:sql_key, true) ? @connection.send(:sql_key, sql) : sql
end
+ def cached_statement(klass, key)
+ cache = klass.send(:cached_find_by_statement, key) do
+ raise "#{klass} has no cached statement by #{key.inspect}"
+ end
+ cache.send(:query_builder).instance_variable_get(:@sql)
+ end
+
def statement_cache
@connection.instance_variable_get(:@statements).send(:cache)
end
diff --git a/activerecord/test/cases/calculations_test.rb b/activerecord/test/cases/calculations_test.rb
index 8ac2d55218..f1e35d6ab9 100644
--- a/activerecord/test/cases/calculations_test.rb
+++ b/activerecord/test/cases/calculations_test.rb
@@ -473,6 +473,24 @@ class CalculationsTest < ActiveRecord::TestCase
assert_equal 6, Account.select("DISTINCT accounts.id").includes(:firm).count
end
+ def test_should_count_manual_select_with_count_all
+ assert_equal 5, Account.select("DISTINCT accounts.firm_id").count(:all)
+ end
+
+ def test_should_count_with_manual_distinct_select_and_distinct
+ assert_equal 4, Account.select("DISTINCT accounts.firm_id").distinct(true).count
+ end
+
+ def test_should_count_manual_select_with_group_with_count_all
+ expected = { nil => 1, 1 => 1, 2 => 1, 6 => 2, 9 => 1 }
+ actual = Account.select("DISTINCT accounts.firm_id").group("accounts.firm_id").count(:all)
+ assert_equal expected, actual
+ end
+
+ def test_should_count_manual_with_count_all
+ assert_equal 6, Account.count(:all)
+ end
+
def test_count_selected_arel_attribute
assert_equal 5, Account.select(Account.arel_table[:firm_id]).count
assert_equal 4, Account.distinct.select(Account.arel_table[:firm_id]).count
diff --git a/activerecord/test/cases/date_time_precision_test.rb b/activerecord/test/cases/date_time_precision_test.rb
index e64a8372d0..9d1af9362d 100644
--- a/activerecord/test/cases/date_time_precision_test.rb
+++ b/activerecord/test/cases/date_time_precision_test.rb
@@ -29,7 +29,7 @@ if subsecond_precision_supported?
def test_datetime_precision_is_truncated_on_assignment
@connection.create_table(:foos, force: true)
- @connection.add_column :foos, :created_at, :datetime, precision: 0
+ @connection.add_column :foos, :created_at, :datetime, precision: 0
@connection.add_column :foos, :updated_at, :datetime, precision: 6
time = ::Time.now.change(nsec: 123456789)
@@ -45,6 +45,26 @@ if subsecond_precision_supported?
assert_equal 123456000, foo.updated_at.nsec
end
+ unless current_adapter?(:Mysql2Adapter)
+ def test_no_datetime_precision_isnt_truncated_on_assignment
+ @connection.create_table(:foos, force: true)
+ @connection.add_column :foos, :created_at, :datetime
+ @connection.add_column :foos, :updated_at, :datetime, precision: 6
+
+ time = ::Time.now.change(nsec: 123)
+ foo = Foo.new(created_at: time, updated_at: time)
+
+ assert_equal 123, foo.created_at.nsec
+ assert_equal 0, foo.updated_at.nsec
+
+ foo.save!
+ foo.reload
+
+ assert_equal 0, foo.created_at.nsec
+ assert_equal 0, foo.updated_at.nsec
+ end
+ end
+
def test_timestamps_helper_with_custom_precision
@connection.create_table(:foos, force: true) do |t|
t.timestamps precision: 4
diff --git a/activerecord/test/cases/inheritance_test.rb b/activerecord/test/cases/inheritance_test.rb
index 19655a2d38..629167e9ed 100644
--- a/activerecord/test/cases/inheritance_test.rb
+++ b/activerecord/test/cases/inheritance_test.rb
@@ -514,10 +514,12 @@ class InheritanceComputeTypeTest < ActiveRecord::TestCase
# Should fail without FirmOnTheFly in the type condition.
assert_raise(ActiveRecord::RecordNotFound) { Firm.find(foo.id) }
+ assert_raise(ActiveRecord::RecordNotFound) { Firm.find_by!(id: foo.id) }
# Nest FirmOnTheFly in the test case where Dependencies won't see it.
self.class.const_set :FirmOnTheFly, Class.new(Firm)
assert_raise(ActiveRecord::SubclassNotFound) { Firm.find(foo.id) }
+ assert_raise(ActiveRecord::SubclassNotFound) { Firm.find_by!(id: foo.id) }
# Nest FirmOnTheFly in Firm where Dependencies will see it.
# This is analogous to nesting models in a migration.
@@ -526,6 +528,7 @@ class InheritanceComputeTypeTest < ActiveRecord::TestCase
# And instantiate will find the existing constant rather than trying
# to require firm_on_the_fly.
assert_nothing_raised { assert_kind_of Firm::FirmOnTheFly, Firm.find(foo.id) }
+ assert_nothing_raised { assert_kind_of Firm::FirmOnTheFly, Firm.find_by!(id: foo.id) }
end
end
diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb
index 2de0a81c99..a7f09e6de0 100644
--- a/activerecord/test/cases/relations_test.rb
+++ b/activerecord/test/cases/relations_test.rb
@@ -972,6 +972,13 @@ class RelationTest < ActiveRecord::TestCase
assert_queries(1) { assert_equal 11, posts.load.size }
end
+ def test_size_with_eager_loading_and_manual_distinct_select_and_custom_order
+ accounts = Account.select("DISTINCT accounts.firm_id").order("accounts.firm_id")
+
+ assert_queries(1) { assert_equal 5, accounts.size }
+ assert_queries(1) { assert_equal 5, accounts.load.size }
+ end
+
def test_count_explicit_columns
Post.update_all(comments_count: nil)
posts = Post.all
@@ -1483,10 +1490,10 @@ class RelationTest < ActiveRecord::TestCase
assert_equal [posts(:welcome)], relation.to_a
author_posts = relation.except(:order, :limit)
- assert_equal Post.where(author_id: 1).to_a, author_posts.to_a
+ assert_equal Post.where(author_id: 1).sort_by(&:id), author_posts.sort_by(&:id)
all_posts = relation.except(:where, :order, :limit)
- assert_equal Post.all, all_posts
+ assert_equal Post.all.sort_by(&:id), all_posts.sort_by(&:id)
end
def test_only
@@ -1494,10 +1501,10 @@ class RelationTest < ActiveRecord::TestCase
assert_equal [posts(:welcome)], relation.to_a
author_posts = relation.only(:where)
- assert_equal Post.where(author_id: 1).to_a, author_posts.to_a
+ assert_equal Post.where(author_id: 1).sort_by(&:id), author_posts.sort_by(&:id)
- all_posts = relation.only(:limit)
- assert_equal Post.limit(1).to_a, all_posts.to_a
+ all_posts = relation.only(:order)
+ assert_equal Post.order("id ASC").to_a, all_posts.to_a
end
def test_anonymous_extension
diff --git a/activerecord/test/cases/time_precision_test.rb b/activerecord/test/cases/time_precision_test.rb
index 086500de38..2f534ea110 100644
--- a/activerecord/test/cases/time_precision_test.rb
+++ b/activerecord/test/cases/time_precision_test.rb
@@ -45,6 +45,26 @@ if subsecond_precision_supported?
assert_equal 123456000, foo.finish.nsec
end
+ unless current_adapter?(:Mysql2Adapter)
+ def test_no_time_precision_isnt_truncated_on_assignment
+ @connection.create_table(:foos, force: true)
+ @connection.add_column :foos, :start, :time
+ @connection.add_column :foos, :finish, :time, precision: 6
+
+ time = ::Time.now.change(nsec: 123)
+ foo = Foo.new(start: time, finish: time)
+
+ assert_equal 123, foo.start.nsec
+ assert_equal 0, foo.finish.nsec
+
+ foo.save!
+ foo.reload
+
+ assert_equal 0, foo.start.nsec
+ assert_equal 0, foo.finish.nsec
+ end
+ end
+
def test_passing_precision_to_time_does_not_set_limit
@connection.create_table(:foos, force: true) do |t|
t.time :start, precision: 3
diff --git a/activestorage/lib/active_storage/attached/changes/create_one.rb b/activestorage/lib/active_storage/attached/changes/create_one.rb
index 5812fd2b08..89cccfb58a 100644
--- a/activestorage/lib/active_storage/attached/changes/create_one.rb
+++ b/activestorage/lib/active_storage/attached/changes/create_one.rb
@@ -30,6 +30,7 @@ module ActiveStorage
def save
record.public_send("#{name}_attachment=", attachment)
+ record.public_send("#{name}_blob=", blob)
end
private
diff --git a/activestorage/test/models/attached/many_test.rb b/activestorage/test/models/attached/many_test.rb
index 8fede0e682..e826109874 100644
--- a/activestorage/test/models/attached/many_test.rb
+++ b/activestorage/test/models/attached/many_test.rb
@@ -16,6 +16,9 @@ class ActiveStorage::ManyAttachedTest < ActiveSupport::TestCase
@user.highlights.attach create_blob(filename: "funky.jpg"), create_blob(filename: "town.jpg")
assert_equal "funky.jpg", @user.highlights.first.filename.to_s
assert_equal "town.jpg", @user.highlights.second.filename.to_s
+
+ assert_not_empty @user.highlights_attachments
+ assert_equal @user.highlights_blobs.count, 2
end
test "attaching existing blobs from signed IDs to an existing record" do
diff --git a/activestorage/test/models/attached/one_test.rb b/activestorage/test/models/attached/one_test.rb
index 7fb3262781..ac08d324bb 100644
--- a/activestorage/test/models/attached/one_test.rb
+++ b/activestorage/test/models/attached/one_test.rb
@@ -15,6 +15,9 @@ class ActiveStorage::OneAttachedTest < ActiveSupport::TestCase
test "attaching an existing blob to an existing record" do
@user.avatar.attach create_blob(filename: "funky.jpg")
assert_equal "funky.jpg", @user.avatar.filename.to_s
+
+ assert_not_nil @user.avatar_attachment
+ assert_not_nil @user.avatar_blob
end
test "attaching an existing blob from a signed ID to an existing record" do
generated by cgit v1.2.3 (git 2.25.1) at 2024-06-29 18:46:47 +0000