From 71f0df943de60aeefa186e6197126c9d0cb6dee8 Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Wed, 13 May 2020 19:58:12 +0900 Subject: [PATCH] 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. --- .../active_record/relation/calculations.rb | 26 +++++++++++++------ .../active_record/relation/query_methods.rb | 3 +++ activerecord/lib/active_record/result.rb | 12 +++++++-- .../has_many_associations_test.rb | 8 +++--- activerecord/test/cases/calculations_test.rb | 12 +++++++++ activerecord/test/cases/enum_test.rb | 10 +++---- activerecord/test/fixtures/books.yml | 6 ++--- activerecord/test/models/book.rb | 2 +- activerecord/test/schema/schema.rb | 2 +- 9 files changed, 57 insertions(+), 24 deletions(-) diff --git a/activerecord/lib/active_record/relation/calculations.rb b/activerecord/lib/active_record/relation/calculations.rb index 6c1106f103..3ee56fb53a 100644 --- a/activerecord/lib/active_record/relation/calculations.rb +++ b/activerecord/lib/active_record/relation/calculations.rb @@ -190,8 +190,9 @@ module ActiveRecord relation.pluck(*column_names) else klass.disallow_raw_sql!(column_names) + columns = arel_columns(column_names) relation = spawn - relation.select_values = column_names + relation.select_values = columns result = skip_query_cache_if_necessary do if where_clause.contradiction? ActiveRecord::Result.new([], []) @@ -199,7 +200,7 @@ module ActiveRecord klass.connection.select_all(relation.arel, nil) end end - result.cast_values(klass.attribute_types) + type_cast_pluck_values(result, columns) end end @@ -277,12 +278,7 @@ module ActiveRecord return column_name if Arel::Expressions === column_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) - end + Arel.sql(column_name == :all ? "*" : name) end end @@ -414,6 +410,20 @@ module ActiveRecord @klass.type_for_attribute(field_name, &block) 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) case operation when "count", "sum" diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb index e89b174d1e..822c3cba88 100644 --- a/activerecord/lib/active_record/relation/query_methods.rb +++ b/activerecord/lib/active_record/relation/query_methods.rb @@ -1272,6 +1272,9 @@ module ActiveRecord if klass.columns_hash.key?(field) && (!from || table_name_matches?(from)) arel_attribute(field) + elsif field.match?(/\A\w+\.\w+\z/) + table, column = field.split(".") + predicate_builder.resolve_arel_attribute(table, column) else yield field end diff --git a/activerecord/lib/active_record/result.rb b/activerecord/lib/active_record/result.rb index 57118c52bc..516a8881ab 100644 --- a/activerecord/lib/active_record/result.rb +++ b/activerecord/lib/active_record/result.rb @@ -110,13 +110,21 @@ module ActiveRecord if columns.one? # 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)| type.deserialize(value) end 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| Array.new(values.size) { |i| types[i].deserialize(values[i]) } diff --git a/activerecord/test/cases/associations/has_many_associations_test.rb b/activerecord/test/cases/associations/has_many_associations_test.rb index daccd95729..4e7219cef3 100644 --- a/activerecord/test/cases/associations/has_many_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_associations_test.rb @@ -437,16 +437,16 @@ class HasManyAssociationsTest < ActiveRecord::TestCase self.table_name = "books" 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 def test_association_enum_works_properly author = SpecialAuthor.create!(name: "Test") - book = SpecialBook.create!(read_status: "reading") + book = SpecialBook.create!(last_read: "reading") author.books << book - assert_equal "reading", book.read_status - assert_not_equal 0, SpecialAuthor.joins(:books).where(books: { read_status: "reading" }).count + assert_equal "reading", book.last_read + assert_not_equal 0, SpecialAuthor.joins(:books).where(books: { last_read: "reading" }).count end # When creating objects on the association, we must not do it within a scope (even though it diff --git a/activerecord/test/cases/calculations_test.rb b/activerecord/test/cases/calculations_test.rb index 7aaeba34ab..f78f0e228f 100644 --- a/activerecord/test/cases/calculations_test.rb +++ b/activerecord/test/cases/calculations_test.rb @@ -729,6 +729,18 @@ class CalculationsTest < ActiveRecord::TestCase assert_equal [ topic.approved ], relation.pluck(:approved) assert_equal [ topic.last_read ], relation.pluck(:last_read) 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 def test_pluck_with_type_cast_does_not_corrupt_the_query_cache diff --git a/activerecord/test/cases/enum_test.rb b/activerecord/test/cases/enum_test.rb index b545a406f6..e35500ee0f 100644 --- a/activerecord/test/cases/enum_test.rb +++ b/activerecord/test/cases/enum_test.rb @@ -27,7 +27,7 @@ class EnumTest < ActiveRecord::TestCase test "query state with strings" do assert_equal "published", @book.status - assert_equal "read", @book.read_status + assert_equal "read", @book.last_read assert_equal "english", @book.language assert_equal "visible", @book.author_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.not(status: :published).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 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.not(status: "published").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 test "build from scope" do @@ -242,8 +242,8 @@ class EnumTest < ActiveRecord::TestCase end test "assign nil value to enum which defines nil value to hash" do - @book.read_status = nil - assert_equal "forgotten", @book.read_status + @book.last_read = nil + assert_equal "forgotten", @book.last_read end test "assign empty string value" do diff --git a/activerecord/test/fixtures/books.yml b/activerecord/test/fixtures/books.yml index ed4d673fe6..824f6c3dc1 100644 --- a/activerecord/test/fixtures/books.yml +++ b/activerecord/test/fixtures/books.yml @@ -4,7 +4,7 @@ awdr: name: "Agile Web Development with Rails" format: "paperback" status: :published - read_status: :read + last_read: :read language: :english author_visibility: :visible illustrator_visibility: :visible @@ -18,7 +18,7 @@ rfr: name: "Ruby for Rails" format: "ebook" status: "proposed" - read_status: "reading" + last_read: "reading" ddd: author_id: 1 @@ -26,7 +26,7 @@ ddd: name: "Domain-Driven Design" format: "hardcover" status: 2 - read_status: "forgotten" + last_read: "forgotten" tlg: author_id: 1 diff --git a/activerecord/test/models/book.rb b/activerecord/test/models/book.rb index de51267093..78ea179f01 100644 --- a/activerecord/test/models/book.rb +++ b/activerecord/test/models/book.rb @@ -10,7 +10,7 @@ class Book < ActiveRecord::Base has_many :subscribers, through: :subscriptions 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 language: [:english, :spanish, :french], _prefix: :in enum author_visibility: [:visible, :invisible], _prefix: true diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb index 901fae8dd0..f919f109ae 100644 --- a/activerecord/test/schema/schema.rb +++ b/activerecord/test/schema/schema.rb @@ -107,7 +107,7 @@ ActiveRecord::Schema.define do t.string :format t.column :name, :string 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 :language, :integer, **default_zero t.column :author_visibility, :integer, **default_zero