Remove ActiveRecord::QueryMethods#build_select monkey patch

Removing the build_select monkey patch removes surprising behavior when
using ignored_columns, and helps to mitigate uneven selects in unions
when columns are removed.

Relates to https://gitlab.com/gitlab-org/gitlab/-/issues/336286Co-authored-by: Simon Tomlinson <stomlinson@gitlab.com>

app/models/application_record.rb

@@ -100,6 +100,14 @@ class ApplicationRecord < ActiveRecord::Base
     self.column_names.map { |column_name| self.arel_table[column_name] }
   end
 
+  def self.default_select_columns
+    if ignored_columns.any?
+      cached_column_list
+    else
+      arel_table[Arel.star]
+    end
+  end
+
   def readable_by?(user)
     Ability.allowed?(user, "read_#{to_ability_name}".to_sym, self)
   end

app/models/integration.rb

@@ -274,7 +274,7 @@ class Integration < ApplicationRecord
   end
 
   def self.closest_group_integration(type, scope)
-    group_ids = scope.ancestors(hierarchy_order: :asc).select(:id)
+    group_ids = scope.ancestors(hierarchy_order: :asc).reselect(:id)
     array = group_ids.to_sql.present? ? "array(#{group_ids.to_sql})" : 'ARRAY[]'
 
     where(type: type, group_id: group_ids, inherit_from_id: nil)

app/models/namespaces/traversal/linear.rb

@@ -176,13 +176,14 @@ module Namespaces
         # if you are walking up the ancestors or down the descendants.
         if hierarchy_order
           depth_sql = "ABS(#{traversal_ids.count} - array_length(traversal_ids, 1))"
-          skope = skope.select(skope.arel_table[Arel.star], "#{depth_sql} as depth")
+          skope = skope.select(skope.default_select_columns, "#{depth_sql} as depth")
           # The SELECT includes an extra depth attribute. We wrap the SQL in a
           # standard SELECT to avoid mismatched attribute errors when trying to
           # chain future ActiveRelation commands, and retain the ordering.
           skope = self.class
             .without_sti_condition
             .from(skope, self.class.table_name)
+            .select(skope.arel_table[Arel.star])
             .order(depth: hierarchy_order)
         end
 

config/initializers/active_record_build_select.rb

-# frozen_string_literal: true
-
-# rubocop:disable Gitlab/ModuleWithInstanceVariables
-
-# build_select only selects the required fields if the model has ignored_columns.
-# This is incompatible with some migrations or background migration specs because
-# rails keeps a statement cache in memory. So if a model with ignored_columns in a
-# migration is used, the query with select table.col1, table.col2 is stored in the
-# statement cache. If a different migration is then run and one of these columns is
-# removed in the meantime, the query is invalid.
-
-module ActiveRecord
-  module QueryMethods
-    private
-
-    def build_select(arel)
-      if select_values.any?
-        arel.project(*arel_columns(select_values.uniq))
-      else
-        arel.project(@klass.arel_table[Arel.star])
-      end
-    end
-  end
-end

ee/app/finders/epics_finder.rb

@@ -212,7 +212,7 @@ class EpicsFinder < IssuableFinder
   def by_child(items)
     return items unless child_id?
 
-    ancestor_ids = Epic.find(params[:child_id]).ancestors.select(:id)
+    ancestor_ids = Epic.find(params[:child_id]).ancestors.reselect(:id)
     items.where(id: ancestor_ids)
   end
   # rubocop: enable CodeReuse/ActiveRecord

ee/app/finders/geo/repository_verification_finder.rb

@@ -11,7 +11,7 @@ module Geo
       query = build_query_to_find_failed_projects(type: :repository, batch_size: batch_size)
       cte   = Gitlab::SQL::CTE.new(:failed_repositories, query)
 
-      Project.with(cte.to_arel)
+      project_with_select_all.with(cte.to_arel)
              .from(cte.alias_to(projects_table))
              .order("projects.repository_retry_at ASC")
     end
@@ -22,7 +22,7 @@ module Geo
       query = build_query_to_find_failed_projects(type: :wiki, batch_size: batch_size)
       cte   = Gitlab::SQL::CTE.new(:failed_wikis, query)
 
-      Project.with(cte.to_arel)
+      project_with_select_all.with(cte.to_arel)
              .from(cte.alias_to(projects_table))
              .order("projects.wiki_retry_at ASC")
     end
@@ -33,7 +33,7 @@ module Geo
       query = build_query_to_find_recently_updated_projects(batch_size: batch_size)
       cte   = Gitlab::SQL::CTE.new(:recently_updated_projects, query)
 
-      Project.with(cte.to_arel)
+      project_with_select_all.with(cte.to_arel)
              .from(cte.alias_to(projects_table))
              .order(last_repository_updated_at_asc)
     end
@@ -138,12 +138,16 @@ module Geo
 
       # We should prioritize less active projects first because high active
       # projects have their repositories verified more frequently.
-      Project.with(cte.to_arel)
+      project_with_select_all.with(cte.to_arel)
              .from(cte.alias_to(projects_table))
              .order(last_repository_updated_at_asc)
     end
     # rubocop: enable CodeReuse/ActiveRecord
 
+    def project_with_select_all
+      Project.select(projects_table[Arel.star])
+    end
+
     def projects_table
       Project.arel_table
     end

lib/gitlab/object_hierarchy.rb

@@ -65,8 +65,15 @@ module Gitlab
     # Note: By default the order is breadth-first
     # rubocop: disable CodeReuse/ActiveRecord
     def base_and_ancestors(upto: nil, hierarchy_order: nil)
-      recursive_query = base_and_ancestors_cte(upto, hierarchy_order).apply_to(unscoped_model.all)
-      recursive_query = recursive_query.order(depth: hierarchy_order) if hierarchy_order
+      cte = base_and_ancestors_cte(upto, hierarchy_order)
+
+      recursive_query = if hierarchy_order
+                          # othewise depth won't be available for outer query
+                          cte.apply_to(unscoped_model.all.select(objects_table[Arel.star])).order(depth: hierarchy_order)
+                        else
+                          cte.apply_to(unscoped_model.all)
+                        end
+
       read_only(recursive_query)
     end
     # rubocop: enable CodeReuse/ActiveRecord
@@ -78,7 +85,10 @@ module Gitlab
     # and incremented as we go down the descendant tree
     # rubocop: disable CodeReuse/ActiveRecord
     def base_and_descendants(with_depth: false)
-      read_only(base_and_descendants_cte(with_depth: with_depth).apply_to(unscoped_model.all))
+      outer_select_relation = unscoped_model.all
+      outer_select_relation = outer_select_relation.select(objects_table[Arel.star]) if with_depth # Otherwise Active Record will not select `depth` as it's not a table column
+
+      read_only(base_and_descendants_cte(with_depth: with_depth).apply_to(outer_select_relation))
     end
     # rubocop: enable CodeReuse/ActiveRecord
 
@@ -145,7 +155,7 @@ module Gitlab
       cte = SQL::RecursiveCTE.new(:base_and_ancestors)
 
       base_query = ancestors_base.except(:order)
-      base_query = base_query.select("1 as #{DEPTH_COLUMN}", "ARRAY[#{objects_table.name}.id] AS tree_path", "false AS tree_cycle", objects_table[Arel.star]) if hierarchy_order
+      base_query = base_query.select("1 as #{DEPTH_COLUMN}", "ARRAY[#{objects_table.name}.id] AS tree_path", "false AS tree_cycle", base_query.default_select_columns) if hierarchy_order
 
       cte << base_query
 
@@ -162,7 +172,7 @@ module Gitlab
           cte.table[DEPTH_COLUMN] + 1,
           "tree_path || #{quoted_objects_table_name}.id",
           "#{quoted_objects_table_name}.id = ANY(tree_path)",
-          objects_table[Arel.star]
+          parent_query.default_select_columns
         ).where(cte.table[:tree_cycle].eq(false))
       end
 
@@ -178,7 +188,7 @@ module Gitlab
       cte = SQL::RecursiveCTE.new(:base_and_descendants)
 
       base_query = descendants_base.except(:order)
-      base_query = base_query.select("1 AS #{DEPTH_COLUMN}", "ARRAY[#{objects_table.name}.id] AS tree_path", "false AS tree_cycle", objects_table[Arel.star]) if with_depth
+      base_query = base_query.select("1 AS #{DEPTH_COLUMN}", "ARRAY[#{objects_table.name}.id] AS tree_path", "false AS tree_cycle", base_query.default_select_columns) if with_depth
 
       cte << base_query
 
@@ -195,7 +205,7 @@ module Gitlab
           cte.table[DEPTH_COLUMN] + 1,
           "tree_path || #{quoted_objects_table_name}.id",
           "#{quoted_objects_table_name}.id = ANY(tree_path)",
-          objects_table[Arel.star]
+          descendants_query.default_select_columns
         ).where(cte.table[:tree_cycle].eq(false))
       end
 

spec/models/application_record_spec.rb

@@ -235,4 +235,46 @@ RSpec.describe ApplicationRecord do
       end
     end
   end
+
+  describe '.default_select_columns' do
+    shared_examples_for 'selects identically to the default' do
+      it 'generates the same sql as the default' do
+        expected_sql = test_model.all.to_sql
+        generated_sql = test_model.all.select(test_model.default_select_columns).to_sql
+
+        expect(expected_sql).to eq(generated_sql)
+      end
+    end
+
+    before do
+      ApplicationRecord.connection.execute(<<~SQL)
+        create table tests (
+          id bigserial primary key not null,
+          ignore_me text
+        )
+      SQL
+    end
+    context 'without an ignored column' do
+      let(:test_model) do
+        Class.new(ApplicationRecord) do
+          self.table_name = 'tests'
+        end
+      end
+
+      it_behaves_like 'selects identically to the default'
+    end
+
+    context 'with an ignored column' do
+      let(:test_model) do
+        Class.new(ApplicationRecord) do
+          include IgnorableColumns
+          self.table_name = 'tests'
+
+          ignore_columns :ignore_me, remove_after: '2100-01-01', remove_with: '99.12'
+        end
+      end
+
+      it_behaves_like 'selects identically to the default'
+    end
+  end
 end

spec/requests/api/project_attributes.yml

@@ -55,6 +55,7 @@ itself: # project
     - can_create_merge_request_in
     - compliance_frameworks
     - container_expiration_policy
+    - container_registry_enabled
     - container_registry_image_prefix
     - default_branch
     - empty_repo