Fix `pluck` and `select` with custom attributes

Currently custom attributes are always qualified by the table name in
the generated SQL wrongly even if the table doesn't have the named
column, it would cause an invalid SQL error.

Custom attributes should only be qualified if the table has the same
named column.

10 files changed, 50 insertions, 16 deletions

diff --git a/activerecord/lib/active_record/relation/calculations.rb b/activerecord/lib/active_record/relation/calculations.rb
index cef31bea94..bdd3c540bb 100644
--- a/activerecord/lib/active_record/relation/calculations.rb
+++ b/activerecord/lib/active_record/relation/calculations.rb
@@ -186,11 +186,9 @@ module ActiveRecord
         relation = apply_join_dependency
         relation.pluck(*column_names)
       else
-        disallow_raw_sql!(column_names)
+        klass.disallow_raw_sql!(column_names)
         relation = spawn
-        relation.select_values = column_names.map { |cn|
-          @klass.has_attribute?(cn) || @klass.attribute_alias?(cn) ? arel_attribute(cn) : cn
-        }
+        relation.select_values = column_names
         result = skip_query_cache_if_necessary { klass.connection.select_all(relation.arel, nil) }
         result.cast_values(klass.attribute_types)
       end
diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb
index eb80aab701..75976aa8fc 100644
--- a/activerecord/lib/active_record/relation/query_methods.rb
+++ b/activerecord/lib/active_record/relation/query_methods.rb
@@ -1052,11 +1052,13 @@ module ActiveRecord
 
       def arel_columns(columns)
         columns.flat_map do |field|
-          if (Symbol === field || String === field) && (klass.has_attribute?(field) || klass.attribute_alias?(field)) && !from_clause.value
-            arel_attribute(field)
-          elsif Symbol === field
-            connection.quote_table_name(field.to_s)
-          elsif Proc === field
+          case field
+          when Symbol
+            field = field.to_s
+            arel_column(field) { connection.quote_table_name(field) }
+          when String
+            arel_column(field) { field }
+          when Proc
             field.call
           else
             field
@@ -1064,6 +1066,16 @@ module ActiveRecord
         end
       end
 
+      def arel_column(field)
+        field = klass.attribute_alias(field) if klass.attribute_alias?(field)
+
+        if klass.columns_hash.key?(field) && !from_clause.value
+          arel_attribute(field)
+        else
+          yield
+        end
+      end
+
       def reverse_sql_order(order_query)
         if order_query.empty?
           return [arel_attribute(primary_key).desc] if primary_key
diff --git a/activerecord/test/cases/associations/belongs_to_associations_test.rb b/activerecord/test/cases/associations/belongs_to_associations_test.rb
index c01138c059..3525fa2ab8 100644
--- a/activerecord/test/cases/associations/belongs_to_associations_test.rb
+++ b/activerecord/test/cases/associations/belongs_to_associations_test.rb
@@ -448,8 +448,13 @@ class BelongsToAssociationsTest < ActiveRecord::TestCase
   end
 
   def test_with_select
-    assert_equal 1, Company.find(2).firm_with_select.attributes.size
-    assert_equal 1, Company.all.merge!(includes: :firm_with_select).find(2).firm_with_select.attributes.size
+    assert_equal 1, Post.find(2).author_with_select.attributes.size
+    assert_equal 1, Post.includes(:author_with_select).find(2).author_with_select.attributes.size
+  end
+
+  def test_custom_attribute_with_select
+    assert_equal 2, Company.find(2).firm_with_select.attributes.size
+    assert_equal 2, Company.includes(:firm_with_select).find(2).firm_with_select.attributes.size
   end
 
   def test_belongs_to_without_counter_cache_option
diff --git a/activerecord/test/cases/base_test.rb b/activerecord/test/cases/base_test.rb
index 363593ca19..1b8f748bad 100644
--- a/activerecord/test/cases/base_test.rb
+++ b/activerecord/test/cases/base_test.rb
@@ -1226,14 +1226,15 @@ class BasicsTest < ActiveRecord::TestCase
   end
 
   def test_attribute_names
-    assert_equal ["id", "type", "firm_id", "firm_name", "name", "client_of", "rating", "account_id", "description"],
-                 Company.attribute_names
+    expected = ["id", "type", "firm_id", "firm_name", "name", "client_of", "rating", "account_id", "description", "metadata"]
+    assert_equal expected, Company.attribute_names
   end
 
   def test_has_attribute
     assert Company.has_attribute?("id")
     assert Company.has_attribute?("type")
     assert Company.has_attribute?("name")
+    assert Company.has_attribute?("metadata")
     assert_not Company.has_attribute?("lastname")
     assert_not Company.has_attribute?("age")
   end
diff --git a/activerecord/test/cases/calculations_test.rb b/activerecord/test/cases/calculations_test.rb
index 850bc49676..b001667ac9 100644
--- a/activerecord/test/cases/calculations_test.rb
+++ b/activerecord/test/cases/calculations_test.rb
@@ -690,8 +690,9 @@ class CalculationsTest < ActiveRecord::TestCase
   end
 
   def test_pluck_not_auto_table_name_prefix_if_column_joined
-    Company.create!(name: "test", contracts: [Contract.new(developer_id: 7)])
-    assert_equal [7], Company.joins(:contracts).pluck(:developer_id)
+    company = Company.create!(name: "test", contracts: [Contract.new(developer_id: 7)])
+    metadata = company.contracts.first.metadata
+    assert_equal [metadata], Company.joins(:contracts).pluck(:metadata)
   end
 
   def test_pluck_with_selection_clause
diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb
index 0ab0459c38..857d743605 100644
--- a/activerecord/test/cases/relations_test.rb
+++ b/activerecord/test/cases/relations_test.rb
@@ -14,6 +14,7 @@ require "models/person"
 require "models/computer"
 require "models/reply"
 require "models/company"
+require "models/contract"
 require "models/bird"
 require "models/car"
 require "models/engine"
@@ -1815,6 +1816,12 @@ class RelationTest < ActiveRecord::TestCase
     assert_equal [1, 1, 1], posts.map(&:author_address_id)
   end
 
+  test "joins with select custom attribute" do
+    contract = Company.create!(name: "test").contracts.create!
+    company = Company.joins(:contracts).select(:id, :metadata).find(contract.company_id)
+    assert_equal contract.metadata, company.metadata
+  end
+
   test "delegations do not leak to other classes" do
     Topic.all.by_lifo
     assert Topic.all.class.method_defined?(:by_lifo)
diff --git a/activerecord/test/models/company.rb b/activerecord/test/models/company.rb
index 838f515aad..a0f48d23f1 100644
--- a/activerecord/test/models/company.rb
+++ b/activerecord/test/models/company.rb
@@ -13,6 +13,8 @@ class Company < AbstractCompany
   has_many :contracts
   has_many :developers, through: :contracts
 
+  attribute :metadata, :json
+
   scope :of_first_firm, lambda {
     joins(account: :firm).
     where("firms.id" => 1)
diff --git a/activerecord/test/models/contract.rb b/activerecord/test/models/contract.rb
index 3f663375c4..89719775c4 100644
--- a/activerecord/test/models/contract.rb
+++ b/activerecord/test/models/contract.rb
@@ -5,7 +5,9 @@ class Contract < ActiveRecord::Base
   belongs_to :developer, primary_key: :id
   belongs_to :firm, foreign_key: "company_id"
 
-  before_save :hi
+  attribute :metadata, :json
+
+  before_save :hi, :update_metadata
   after_save :bye
 
   attr_accessor :hi_count, :bye_count
@@ -19,6 +21,10 @@ class Contract < ActiveRecord::Base
     @bye_count ||= 0
     @bye_count += 1
   end
+
+  def update_metadata
+    self.metadata = { company_id: company_id, developer_id: developer_id }
+  end
 end
 
 class NewContract < Contract
diff --git a/activerecord/test/models/post.rb b/activerecord/test/models/post.rb
index e32cc59399..65d1fce66d 100644
--- a/activerecord/test/models/post.rb
+++ b/activerecord/test/models/post.rb
@@ -31,6 +31,7 @@ class Post < ActiveRecord::Base
 
   belongs_to :author_with_posts, -> { includes(:posts) }, class_name: "Author", foreign_key: :author_id
   belongs_to :author_with_address, -> { includes(:author_address) }, class_name: "Author", foreign_key: :author_id
+  belongs_to :author_with_select, -> { select(:id) }, class_name: "Author", foreign_key: :author_id
 
   def first_comment
     super.body
diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb
index 86d5a67a13..349b8afc48 100644
--- a/activerecord/test/schema/schema.rb
+++ b/activerecord/test/schema/schema.rb
@@ -247,6 +247,7 @@ ActiveRecord::Schema.define do
   create_table :contracts, force: true do |t|
     t.references :developer, index: false
     t.references :company, index: false
+    t.string :metadata
   end
 
   create_table :customers, force: true do |t|