mirror of
https://github.com/rails/rails.git
synced 2022-11-09 12:12:34 -05:00
Fix pluck
to correctly type cast same column names and association columns
That issues are caused by using only the model's cast types on the relation. To fix that issues, use the attribute's type caster takes precedence over the model's cast types on the relation. Fixes #35232. Fixes #36042. Fixes #37484.
This commit is contained in:
parent
ed29afcc03
commit
71f0df943d
9 changed files with 57 additions and 24 deletions
|
@ -190,8 +190,9 @@ module ActiveRecord
|
||||||
relation.pluck(*column_names)
|
relation.pluck(*column_names)
|
||||||
else
|
else
|
||||||
klass.disallow_raw_sql!(column_names)
|
klass.disallow_raw_sql!(column_names)
|
||||||
|
columns = arel_columns(column_names)
|
||||||
relation = spawn
|
relation = spawn
|
||||||
relation.select_values = column_names
|
relation.select_values = columns
|
||||||
result = skip_query_cache_if_necessary do
|
result = skip_query_cache_if_necessary do
|
||||||
if where_clause.contradiction?
|
if where_clause.contradiction?
|
||||||
ActiveRecord::Result.new([], [])
|
ActiveRecord::Result.new([], [])
|
||||||
|
@ -199,7 +200,7 @@ module ActiveRecord
|
||||||
klass.connection.select_all(relation.arel, nil)
|
klass.connection.select_all(relation.arel, nil)
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
result.cast_values(klass.attribute_types)
|
type_cast_pluck_values(result, columns)
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
@ -277,14 +278,9 @@ module ActiveRecord
|
||||||
return column_name if Arel::Expressions === column_name
|
return column_name if Arel::Expressions === column_name
|
||||||
|
|
||||||
arel_column(column_name.to_s) do |name|
|
arel_column(column_name.to_s) do |name|
|
||||||
if name.match?(/\A\w+\.\w+\z/)
|
|
||||||
table, column = name.split(".")
|
|
||||||
predicate_builder.resolve_arel_attribute(table, column)
|
|
||||||
else
|
|
||||||
Arel.sql(column_name == :all ? "*" : name)
|
Arel.sql(column_name == :all ? "*" : name)
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
end
|
|
||||||
|
|
||||||
def operation_over_aggregate_column(column, operation, distinct)
|
def operation_over_aggregate_column(column, operation, distinct)
|
||||||
operation == "count" ? column.count(distinct) : column.send(operation)
|
operation == "count" ? column.count(distinct) : column.send(operation)
|
||||||
|
@ -414,6 +410,20 @@ module ActiveRecord
|
||||||
@klass.type_for_attribute(field_name, &block)
|
@klass.type_for_attribute(field_name, &block)
|
||||||
end
|
end
|
||||||
|
|
||||||
|
def type_cast_pluck_values(result, columns)
|
||||||
|
cast_types = if result.columns.size != columns.size
|
||||||
|
klass.attribute_types
|
||||||
|
else
|
||||||
|
columns.map.with_index do |column, i|
|
||||||
|
column.try(:type_caster) ||
|
||||||
|
klass.attribute_types.fetch(name = result.columns[i]) do
|
||||||
|
result.column_types.fetch(name, Type.default_value)
|
||||||
|
end
|
||||||
|
end
|
||||||
|
end
|
||||||
|
result.cast_values(cast_types)
|
||||||
|
end
|
||||||
|
|
||||||
def type_cast_calculated_value(value, operation)
|
def type_cast_calculated_value(value, operation)
|
||||||
case operation
|
case operation
|
||||||
when "count", "sum"
|
when "count", "sum"
|
||||||
|
|
|
@ -1272,6 +1272,9 @@ module ActiveRecord
|
||||||
|
|
||||||
if klass.columns_hash.key?(field) && (!from || table_name_matches?(from))
|
if klass.columns_hash.key?(field) && (!from || table_name_matches?(from))
|
||||||
arel_attribute(field)
|
arel_attribute(field)
|
||||||
|
elsif field.match?(/\A\w+\.\w+\z/)
|
||||||
|
table, column = field.split(".")
|
||||||
|
predicate_builder.resolve_arel_attribute(table, column)
|
||||||
else
|
else
|
||||||
yield field
|
yield field
|
||||||
end
|
end
|
||||||
|
|
|
@ -110,13 +110,21 @@ module ActiveRecord
|
||||||
if columns.one?
|
if columns.one?
|
||||||
# Separated to avoid allocating an array per row
|
# Separated to avoid allocating an array per row
|
||||||
|
|
||||||
type = column_type(columns.first, type_overrides)
|
type = if type_overrides.is_a?(Array)
|
||||||
|
type_overrides.first
|
||||||
|
else
|
||||||
|
column_type(columns.first, type_overrides)
|
||||||
|
end
|
||||||
|
|
||||||
rows.map do |(value)|
|
rows.map do |(value)|
|
||||||
type.deserialize(value)
|
type.deserialize(value)
|
||||||
end
|
end
|
||||||
else
|
else
|
||||||
types = columns.map { |name| column_type(name, type_overrides) }
|
types = if type_overrides.is_a?(Array)
|
||||||
|
type_overrides
|
||||||
|
else
|
||||||
|
columns.map { |name| column_type(name, type_overrides) }
|
||||||
|
end
|
||||||
|
|
||||||
rows.map do |values|
|
rows.map do |values|
|
||||||
Array.new(values.size) { |i| types[i].deserialize(values[i]) }
|
Array.new(values.size) { |i| types[i].deserialize(values[i]) }
|
||||||
|
|
|
@ -437,16 +437,16 @@ class HasManyAssociationsTest < ActiveRecord::TestCase
|
||||||
self.table_name = "books"
|
self.table_name = "books"
|
||||||
|
|
||||||
belongs_to :author
|
belongs_to :author
|
||||||
enum read_status: { unread: 0, reading: 2, read: 3, forgotten: nil }
|
enum last_read: { unread: 0, reading: 2, read: 3, forgotten: nil }
|
||||||
end
|
end
|
||||||
|
|
||||||
def test_association_enum_works_properly
|
def test_association_enum_works_properly
|
||||||
author = SpecialAuthor.create!(name: "Test")
|
author = SpecialAuthor.create!(name: "Test")
|
||||||
book = SpecialBook.create!(read_status: "reading")
|
book = SpecialBook.create!(last_read: "reading")
|
||||||
author.books << book
|
author.books << book
|
||||||
|
|
||||||
assert_equal "reading", book.read_status
|
assert_equal "reading", book.last_read
|
||||||
assert_not_equal 0, SpecialAuthor.joins(:books).where(books: { read_status: "reading" }).count
|
assert_not_equal 0, SpecialAuthor.joins(:books).where(books: { last_read: "reading" }).count
|
||||||
end
|
end
|
||||||
|
|
||||||
# When creating objects on the association, we must not do it within a scope (even though it
|
# When creating objects on the association, we must not do it within a scope (even though it
|
||||||
|
|
|
@ -729,6 +729,18 @@ class CalculationsTest < ActiveRecord::TestCase
|
||||||
assert_equal [ topic.approved ], relation.pluck(:approved)
|
assert_equal [ topic.approved ], relation.pluck(:approved)
|
||||||
assert_equal [ topic.last_read ], relation.pluck(:last_read)
|
assert_equal [ topic.last_read ], relation.pluck(:last_read)
|
||||||
assert_equal [ topic.written_on ], relation.pluck(:written_on)
|
assert_equal [ topic.written_on ], relation.pluck(:written_on)
|
||||||
|
|
||||||
|
expected = [
|
||||||
|
[Date.new(2004, 4, 15), "unread"],
|
||||||
|
[Date.new(2004, 4, 15), "reading"],
|
||||||
|
[Date.new(2004, 4, 15), "read"],
|
||||||
|
]
|
||||||
|
actual =
|
||||||
|
Author.joins(:topics, :books).order(:"books.last_read")
|
||||||
|
.where.not("books.last_read": nil)
|
||||||
|
.pluck(:"topics.last_read", :"books.last_read")
|
||||||
|
|
||||||
|
assert_equal expected, actual
|
||||||
end
|
end
|
||||||
|
|
||||||
def test_pluck_with_type_cast_does_not_corrupt_the_query_cache
|
def test_pluck_with_type_cast_does_not_corrupt_the_query_cache
|
||||||
|
|
|
@ -27,7 +27,7 @@ class EnumTest < ActiveRecord::TestCase
|
||||||
|
|
||||||
test "query state with strings" do
|
test "query state with strings" do
|
||||||
assert_equal "published", @book.status
|
assert_equal "published", @book.status
|
||||||
assert_equal "read", @book.read_status
|
assert_equal "read", @book.last_read
|
||||||
assert_equal "english", @book.language
|
assert_equal "english", @book.language
|
||||||
assert_equal "visible", @book.author_visibility
|
assert_equal "visible", @book.author_visibility
|
||||||
assert_equal "visible", @book.illustrator_visibility
|
assert_equal "visible", @book.illustrator_visibility
|
||||||
|
@ -68,7 +68,7 @@ class EnumTest < ActiveRecord::TestCase
|
||||||
assert_not_equal @book, Book.where(status: [:written]).first
|
assert_not_equal @book, Book.where(status: [:written]).first
|
||||||
assert_not_equal @book, Book.where.not(status: :published).first
|
assert_not_equal @book, Book.where.not(status: :published).first
|
||||||
assert_equal @book, Book.where.not(status: :written).first
|
assert_equal @book, Book.where.not(status: :written).first
|
||||||
assert_equal books(:ddd), Book.where(read_status: :forgotten).first
|
assert_equal books(:ddd), Book.where(last_read: :forgotten).first
|
||||||
end
|
end
|
||||||
|
|
||||||
test "find via where with strings" do
|
test "find via where with strings" do
|
||||||
|
@ -78,7 +78,7 @@ class EnumTest < ActiveRecord::TestCase
|
||||||
assert_not_equal @book, Book.where(status: ["written"]).first
|
assert_not_equal @book, Book.where(status: ["written"]).first
|
||||||
assert_not_equal @book, Book.where.not(status: "published").first
|
assert_not_equal @book, Book.where.not(status: "published").first
|
||||||
assert_equal @book, Book.where.not(status: "written").first
|
assert_equal @book, Book.where.not(status: "written").first
|
||||||
assert_equal books(:ddd), Book.where(read_status: "forgotten").first
|
assert_equal books(:ddd), Book.where(last_read: "forgotten").first
|
||||||
end
|
end
|
||||||
|
|
||||||
test "build from scope" do
|
test "build from scope" do
|
||||||
|
@ -242,8 +242,8 @@ class EnumTest < ActiveRecord::TestCase
|
||||||
end
|
end
|
||||||
|
|
||||||
test "assign nil value to enum which defines nil value to hash" do
|
test "assign nil value to enum which defines nil value to hash" do
|
||||||
@book.read_status = nil
|
@book.last_read = nil
|
||||||
assert_equal "forgotten", @book.read_status
|
assert_equal "forgotten", @book.last_read
|
||||||
end
|
end
|
||||||
|
|
||||||
test "assign empty string value" do
|
test "assign empty string value" do
|
||||||
|
|
6
activerecord/test/fixtures/books.yml
vendored
6
activerecord/test/fixtures/books.yml
vendored
|
@ -4,7 +4,7 @@ awdr:
|
||||||
name: "Agile Web Development with Rails"
|
name: "Agile Web Development with Rails"
|
||||||
format: "paperback"
|
format: "paperback"
|
||||||
status: :published
|
status: :published
|
||||||
read_status: :read
|
last_read: :read
|
||||||
language: :english
|
language: :english
|
||||||
author_visibility: :visible
|
author_visibility: :visible
|
||||||
illustrator_visibility: :visible
|
illustrator_visibility: :visible
|
||||||
|
@ -18,7 +18,7 @@ rfr:
|
||||||
name: "Ruby for Rails"
|
name: "Ruby for Rails"
|
||||||
format: "ebook"
|
format: "ebook"
|
||||||
status: "proposed"
|
status: "proposed"
|
||||||
read_status: "reading"
|
last_read: "reading"
|
||||||
|
|
||||||
ddd:
|
ddd:
|
||||||
author_id: 1
|
author_id: 1
|
||||||
|
@ -26,7 +26,7 @@ ddd:
|
||||||
name: "Domain-Driven Design"
|
name: "Domain-Driven Design"
|
||||||
format: "hardcover"
|
format: "hardcover"
|
||||||
status: 2
|
status: 2
|
||||||
read_status: "forgotten"
|
last_read: "forgotten"
|
||||||
|
|
||||||
tlg:
|
tlg:
|
||||||
author_id: 1
|
author_id: 1
|
||||||
|
|
|
@ -10,7 +10,7 @@ class Book < ActiveRecord::Base
|
||||||
has_many :subscribers, through: :subscriptions
|
has_many :subscribers, through: :subscriptions
|
||||||
|
|
||||||
enum status: [:proposed, :written, :published]
|
enum status: [:proposed, :written, :published]
|
||||||
enum read_status: { unread: 0, reading: 2, read: 3, forgotten: nil }
|
enum last_read: { unread: 0, reading: 2, read: 3, forgotten: nil }
|
||||||
enum nullable_status: [:single, :married]
|
enum nullable_status: [:single, :married]
|
||||||
enum language: [:english, :spanish, :french], _prefix: :in
|
enum language: [:english, :spanish, :french], _prefix: :in
|
||||||
enum author_visibility: [:visible, :invisible], _prefix: true
|
enum author_visibility: [:visible, :invisible], _prefix: true
|
||||||
|
|
|
@ -107,7 +107,7 @@ ActiveRecord::Schema.define do
|
||||||
t.string :format
|
t.string :format
|
||||||
t.column :name, :string
|
t.column :name, :string
|
||||||
t.column :status, :integer, **default_zero
|
t.column :status, :integer, **default_zero
|
||||||
t.column :read_status, :integer, **default_zero
|
t.column :last_read, :integer, **default_zero
|
||||||
t.column :nullable_status, :integer
|
t.column :nullable_status, :integer
|
||||||
t.column :language, :integer, **default_zero
|
t.column :language, :integer, **default_zero
|
||||||
t.column :author_visibility, :integer, **default_zero
|
t.column :author_visibility, :integer, **default_zero
|
||||||
|
|
Loading…
Reference in a new issue