Improve ignored_columns handling

- Actually enumerate columns when any are ignored to avoid loading unknown attributes
- Remove old ignored_columns so we don't unnecessary bloat queries when not ignoring
- Various minor fixes for places we do unusual AR things to ensure they work with explicit columns
- Tweak some migrations to clear column information so future migrations are happy

refs AE-747

Change-Id: I60b1c3eae73f4fa9f0b6b6ab4d2b00abd8f8395f
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/339971
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Cody Cutrer <cody@instructure.com>
Migration-Review: Cody Cutrer <cody@instructure.com>
QA-Review: Jacob Burroughs <jburroughs@instructure.com>
Product-Review: Jacob Burroughs <jburroughs@instructure.com>

app/graphql/loaders/outcome_alignment_loader.rb

@@ -37,6 +37,7 @@ class Loaders::OutcomeAlignmentLoader < GraphQL::Batch::Loader
     active_os_alignments = outcome_alignment_summary_with_new_quizzes_enabled?(@context) ? get_active_os_alignments(@context) : {}
 
     outcomes.each do |outcome|
+      all_fields = %w[id alignment_type content_id content_type context_id context_type title learning_outcome_id created_at updated_at assignment_id assignment_submission_types assignment_workflow_state discussion_id quiz_id module_id module_name module_workflow_state]
       # direct outcome alignments to rubric, assignment, quiz, and graded discussions
       # map assignment id to quiz/discussion id
       assignments_sub = Assignment
@@ -137,9 +138,9 @@ class Loaders::OutcomeAlignmentLoader < GraphQL::Batch::Loader
                               ")
                               .distinct
 
-        all_alignments = ContentTag.from("(#{direct_alignments.to_sql} UNION #{indirect_alignments.to_sql} UNION #{external_alignments.to_sql}) AS content_tags")
+        all_alignments = ContentTag.select(all_fields).from("(#{direct_alignments.to_sql} UNION #{indirect_alignments.to_sql} UNION #{external_alignments.to_sql}) AS content_tags")
       else
-        all_alignments = ContentTag.from("(#{direct_alignments.to_sql} UNION #{indirect_alignments.to_sql}) AS content_tags")
+        all_alignments = ContentTag.select(all_fields).from("(#{direct_alignments.to_sql} UNION #{indirect_alignments.to_sql}) AS content_tags")
       end
 
       # deduplicate and sort alignments

app/models/abstract_assignment.rb

@@ -39,8 +39,6 @@ class AbstractAssignment < ActiveRecord::Base
   include LockedFor
   include Lti::Migratable
 
-  self.ignored_columns += %i[context_code checkpointed checkpoint_label]
-
   GRADING_TYPES = OpenStruct.new(
     {
       points: "points",
@@ -3162,7 +3160,7 @@ class AbstractAssignment < ActiveRecord::Base
     from("(SELECT s.cached_due_date AS user_due_date, a.*
           FROM #{Assignment.quoted_table_name} a
           INNER JOIN #{Submission.quoted_table_name} AS s ON s.assignment_id = a.id
-          WHERE s.user_id = #{User.connection.quote(user.id_for_database)} AND s.workflow_state <> 'deleted') AS assignments")
+          WHERE s.user_id = #{User.connection.quote(user.id_for_database)} AND s.workflow_state <> 'deleted') AS assignments").select(arel.projections, "user_due_date")
   }
 
   scope :with_latest_due_date, lambda {
@@ -3171,7 +3169,7 @@ class AbstractAssignment < ActiveRecord::Base
           LEFT JOIN #{AssignmentOverride.quoted_table_name} ao
           ON ao.assignment_id = a.id
           AND ao.due_at_overridden
-          GROUP BY a.id) AS assignments")
+          GROUP BY a.id) AS assignments").select(arel.projections, "latest_due_date")
   }
 
   scope :updated_after, lambda { |*args|

app/models/attachment.rb

@@ -25,21 +25,6 @@ require "crocodoc"
 class Attachment < ActiveRecord::Base
   class UniqueRenameFailure < StandardError; end
 
-  self.ignored_columns += %i[last_lock_at
-                             last_unlock_at
-                             enrollment_id
-                             cached_s3_url
-                             s3_url_cached_at
-                             scribd_account_id
-                             scribd_user
-                             scribd_mime_type_id
-                             submitted_to_scribd_at
-                             scribd_doc
-                             scribd_attempts
-                             cached_scribd_thumbnail
-                             last_inline_view
-                             local_filename]
-
   def self.display_name_order_by_clause(table = nil)
     col = table ? "#{table}.display_name" : "display_name"
     best_unicode_collation_key(col)
@@ -369,7 +354,7 @@ class Attachment < ActiveRecord::Base
           ) AS media_tracks_all
         SQL
       :media_tracks
-    ).where(row: 1)
+    ).select(MediaTrack.all.arel.projections, "for_att_id", "inherited").where(row: 1)
   end
 
   # this is here becase attachment_fu looks to make sure that parent_id is nil before it will create a thumbnail of something.

app/models/calendar_event.rb

@@ -31,8 +31,6 @@ class CalendarEvent < ActiveRecord::Base
 
   include MasterCourses::Restrictor
 
-  self.ignored_columns += %i[series_id]
-
   restrict_columns :content, [:title, :description]
   restrict_columns :settings, %i[location_name location_address start_at end_at all_day all_day_date series_uuid rrule]
 

app/models/conversation.rb

@@ -19,8 +19,6 @@
 #
 
 class Conversation < ActiveRecord::Base
-  self.ignored_columns += %i[root_account_id]
-
   include SimpleTags
   include ModelCache
   include SendToStream

app/models/conversation_message.rb

@@ -21,8 +21,6 @@
 require "atom"
 
 class ConversationMessage < ActiveRecord::Base
-  self.ignored_columns += %i[root_account_id]
-
   include HtmlTextHelper
   include ConversationHelper
   include Rails.application.routes.url_helpers

app/models/conversation_message_participant.rb

@@ -19,8 +19,6 @@
 #
 
 class ConversationMessageParticipant < ActiveRecord::Base
-  self.ignored_columns += %i[root_account_id]
-
   include SimpleTags
   include Workflow
   include ConversationHelper

app/models/conversation_participant.rb

@@ -19,8 +19,6 @@
 #
 
 class ConversationParticipant < ActiveRecord::Base
-  self.ignored_columns += %i[root_account_id]
-
   include Workflow
   include TextHelper
   include SimpleTags

app/models/course_pace.rb

@@ -68,8 +68,6 @@ class CoursePace < ActiveRecord::Base
     can :read
   end
 
-  self.ignored_columns += %i[start_date]
-
   def asset_name
     I18n.t("Course Pace")
   end

app/models/developer_key.rb

@@ -96,8 +96,6 @@ class DeveloperKey < ActiveRecord::Base
     state :deleted
   end
 
-  self.ignored_columns += %i[oidc_login_uri tool_id]
-
   # https://stackoverflow.com/a/2500819
   alias_method :referenced_tool_configuration, :tool_configuration
 

app/models/feature_flag.rb

@@ -27,8 +27,6 @@ class FeatureFlag < ActiveRecord::Base
 
   belongs_to :context, polymorphic: %i[account course user]
 
-  self.ignored_columns += %i[visibility manipulate]
-
   validate :valid_state, :feature_applies
   before_save :check_cache
   after_create :audit_log_create # to make sure we have an ID, must be after

app/models/folder.rb

@@ -19,8 +19,6 @@
 #
 
 class Folder < ActiveRecord::Base
-  self.ignored_columns += %i[last_lock_at last_unlock_at]
-
   def self.name_order_by_clause(table = nil)
     col = table ? "#{table}.name" : "name"
     best_unicode_collation_key(col)

app/models/learning_outcome.rb

@@ -23,7 +23,6 @@ class LearningOutcome < ActiveRecord::Base
   include Workflow
   include MasterCourses::Restrictor
   restrict_columns :state, [:workflow_state]
-  self.ignored_columns += %i[migration_id_2 vendor_guid_2 root_account_id]
 
   belongs_to :context, polymorphic: [:account, :course]
   has_many :learning_outcome_results

app/models/learning_outcome_group.rb

@@ -24,7 +24,6 @@ class LearningOutcomeGroup < ActiveRecord::Base
   extend RootAccountResolver
 
   restrict_columns :state, [:workflow_state]
-  self.ignored_columns += %i[migration_id_2 vendor_guid_2]
 
   belongs_to :learning_outcome_group
   belongs_to :source_outcome_group, class_name: "LearningOutcomeGroup", inverse_of: :destination_outcome_groups

app/models/lti/resource_link.rb

@@ -21,8 +21,6 @@
 class Lti::ResourceLink < ApplicationRecord
   include Canvas::SoftDeletable
 
-  self.ignored_columns += %i[lookup_id resource_link_id]
-
   validates :context_external_tool_id,
             :context_id,
             :context_type,

app/models/notification.rb

@@ -21,8 +21,6 @@
 class Notification < Switchman::UnshardedRecord
   include TextHelper
 
-  self.ignored_columns += %i[workflow_state]
-
   TYPES_TO_SHOW_IN_FEED = [
     # Assignment
     "Assignment Created",

app/models/outcome_proficiency.rb

@@ -21,7 +21,6 @@
 class OutcomeProficiency < ApplicationRecord
   extend RootAccountResolver
   include Canvas::SoftDeletable
-  self.ignored_columns += %i[account_id]
 
   def self.emit_live_events_on_any_update?
     true

app/models/quizzes/quiz.rb

@@ -1246,7 +1246,7 @@ class Quizzes::Quiz < ActiveRecord::Base
         SELECT CASE WHEN overrides.due_at_overridden THEN overrides.due_at ELSE q.due_at END as user_due_date, q.*
         FROM #{Quizzes::Quiz.quoted_table_name} q
         INNER JOIN overrides ON overrides.quiz_id = q.id) as quizzes")
-      .not_for_assignment
+      .select(arel.projections, "user_due_date").not_for_assignment
   }
 
   scope :ungraded_due_between_for_user, lambda { |start, ending, user|

app/models/rubric_assessment.rb

@@ -36,8 +36,6 @@ class RubricAssessment < ActiveRecord::Base
   has_many :learning_outcome_results, as: :artifact, dependent: :destroy
   serialize :data
 
-  self.ignored_columns += [:comments]
-
   simply_versioned
 
   validates :assessment_type, :rubric_id, :artifact_id, :artifact_type, :assessor_id, presence: true

app/models/submission.rb

@@ -22,8 +22,6 @@ require "atom"
 require "anonymity"
 
 class Submission < ActiveRecord::Base
-  self.ignored_columns += %w[has_admin_comment has_rubric_assessment process_attempts context_code]
-
   include Canvas::GradeValidations
   include CustomValidations
   include SendToStream

app/models/user.rb

@@ -54,19 +54,6 @@ class User < ActiveRecord::Base
     best_unicode_collation_key(col)
   end
 
-  self.ignored_columns += %i[type
-                             creation_unique_id
-                             creation_sis_batch_id
-                             creation_email
-                             sis_name
-                             bio
-                             merge_to
-                             unread_inbox_items_count
-                             visibility
-                             account_pronoun_id
-                             gender
-                             birthdate]
-
   include Context
   include ModelCache
   include UserLearningObjectScopes

app/models/wiki_page.rb

@@ -102,8 +102,6 @@ class WikiPage < ActiveRecord::Base
   TITLE_LENGTH = 255
   SIMPLY_VERSIONED_EXCLUDE_FIELDS = %i[workflow_state editing_roles notify_of_update].freeze
 
-  self.ignored_columns += %i[view_count]
-
   def ensure_wiki_and_context
     self.wiki_id ||= context.wiki_id || context.wiki.id
   end

config/initializers/active_record.rb

@@ -1074,6 +1074,11 @@ module UsefulFindInBatches
 
         base_class = klass.base_class
         base_class.unscoped do
+          # Ensure we don't enumerate columns on the temp table, because the temp table may not have the same columns as the base class
+          ignored_columns_was = base_class.ignored_columns
+          enumerate_columns_was = base_class.enumerate_columns_in_select_statements
+          base_class.enumerate_columns_in_select_statements = false
+          base_class.ignored_columns = [] # rubocop:disable Rails/IgnoredColumnsAssignment
           batch_relation = base_class.from("#{connection.quote_column_name(table)} as #{connection.quote_column_name(base_class.table_name)}").limit(of).preload(includes_values + preload_values)
           batch_relation = batch_relation.order(Arel.sql(connection.quote_column_name(index))) if index
           yielded_relation = batch_relation
@@ -1086,12 +1091,15 @@ module UsefulFindInBatches
             last_value = if yielded_relation.loaded?
                            yielded_relation.last[index]
                          else
-                           yielded_relation.offset(of - 1).limit(1).pluck(index).first
+                           yielded_relation.offset(of - 1).limit(1).pick(index)
                          end
             break if last_value.nil?
 
             yielded_relation = batch_relation.where("#{connection.quote_column_name(index)} > ?", last_value)
           end
+        ensure
+          base_class.ignored_columns = ignored_columns_was # rubocop:disable Rails/IgnoredColumnsAssignment
+          base_class.enumerate_columns_in_select_statements = enumerate_columns_was
         end
       ensure
         if !$!.is_a?(ActiveRecord::StatementInvalid) || connection.open_transactions == 0
@@ -2025,20 +2033,6 @@ ActiveRecord::Base.prepend(ActiveRecord::CacheRegister::Base)
 ActiveRecord::Base.singleton_class.prepend(ActiveRecord::CacheRegister::Base::ClassMethods)
 ActiveRecord::Relation.prepend(ActiveRecord::CacheRegister::Relation)
 
-# see https://github.com/rails/rails/issues/37745
-module DontExplicitlyNameColumnsBecauseOfIgnores
-  def build_select(arel)
-    if select_values.any?
-      arel.project(*arel_columns(select_values.uniq))
-    elsif !from_clause.value && klass.ignored_columns.any? && !!klass.ignored_columns.intersect?(klass.column_names)
-      arel.project(*klass.column_names.map { |field| arel_attribute(field) })
-    else
-      arel.project(table[Arel.star])
-    end
-  end
-end
-ActiveRecord::Relation.prepend(DontExplicitlyNameColumnsBecauseOfIgnores)
-
 module PreserveShardAfterTransaction
   def after_transaction_commit(&)
     shards = Shard.send(:active_shards)

db/migrate/20170224175329_remove_computed_current_score_and_computed_final_score_from_enrollments.rb

@@ -23,6 +23,7 @@ class RemoveComputedCurrentScoreAndComputedFinalScoreFromEnrollments < ActiveRec
   def up
     remove_column :enrollments, :computed_current_score
     remove_column :enrollments, :computed_final_score
+    Enrollment.reset_column_information
   end
 
   def down

db/migrate/20180813194457_drop_last_un_lock_at.rb

@@ -26,5 +26,7 @@ class DropLastUnLockAt < ActiveRecord::Migration[5.1]
     remove_column :folders, :last_unlock_at, :datetime
     remove_column :attachments, :last_lock_at, :datetime
     remove_column :attachments, :last_unlock_at, :datetime
+    Folder.reset_column_information
+    Attachment.reset_column_information
   end
 end

db/migrate/20191030231711_drop_account_pronouns.rb

@@ -23,5 +23,6 @@ class DropAccountPronouns < ActiveRecord::Migration[5.2]
   def change
     drop_table :account_pronouns
     remove_column :users, :account_pronoun_id, :int, limit: 8
+    User.reset_column_information
   end
 end

db/migrate/20200707155842_drop_unused_user_columns.rb

@@ -23,5 +23,6 @@ class DropUnusedUserColumns < ActiveRecord::Migration[5.2]
   def change
     remove_column :users, :gender, :string, limit: 255
     remove_column :users, :birthdate, :datetime
+    User.reset_column_information
   end
 end

db/migrate/20210331164124_drop_feature_flag_visibility_and_manipulate.rb

@@ -23,5 +23,6 @@ class DropFeatureFlagVisibilityAndManipulate < ActiveRecord::Migration[6.0]
   def change
     remove_column :feature_flags, :visibility, :string, limit: 255
     remove_column :feature_flags, :manipulate, :string, limit: 255
+    FeatureFlag.reset_column_information
   end
 end

spec/conditional_release_spec_helper.rb

@@ -23,12 +23,12 @@ RSpec.shared_examples "a soft-deletable model" do
   it { is_expected.to have_db_column(:deleted_at) }
 
   it "adds a deleted_at where clause when requested" do
-    expect(described_class.active.all.to_sql).to include('"deleted_at" IS NULL')
+    expect(described_class.active.all.where_clause.ast.to_sql).to include('"deleted_at" IS NULL')
   end
 
   it "skips adding the deleted_at where clause normally" do
     # sorry - no default scopes
-    expect(described_class.all.to_sql).not_to include("deleted_at")
+    expect(described_class.all.where_clause.ast.to_sql).not_to include("deleted_at")
   end
 
   it "soft deletes" do

spec/lib/acts_as_list_spec.rb

@@ -104,7 +104,7 @@ describe "acts_as_list" do
   describe "base scope" do
     it "scopes by the base class rather then the STI class" do
       scope = AuthenticationProvider::CAS.new.list_scope_base
-      expect(scope.to_sql).not_to(match(/auth_type/))
+      expect(scope.where_clause.ast.to_sql).not_to(match(/auth_type/))
     end
   end
 end

spec/lib/sentry_extensions/tracing/active_record_subscriber_spec.rb

@@ -71,7 +71,7 @@ describe SentryExtensions::Tracing::ActiveRecordSubscriber do
 
       span = transaction[:spans][0]
       expect(span[:op]).to eq("db.sql.active_record")
-      expect(span[:description]).to eq('SELECT "users".* FROM "public"."users"')
+      expect(span[:description]).to eq(User.all.to_sql)
       expect(span[:trace_id]).to eq(transaction.dig(:contexts, :trace, :trace_id))
     end
 
@@ -81,7 +81,7 @@ describe SentryExtensions::Tracing::ActiveRecordSubscriber do
       expect(transport.events.count).to eq(1)
 
       span = transport.events.first.to_hash[:spans][0]
-      expect(span[:description]).to eq('SELECT "users".* FROM "public"."users" WHERE "users"."name" = $1')
+      expect(span[:description]).to eq(User.where(name: "Taylor Swift").to_sql.gsub("'Taylor Swift'", "$1"))
     end
 
     it "provides fallback description when normalization fails" do

spec/models/message_spec.rb

@@ -848,15 +848,15 @@ describe Message do
     let(:partition) { { "created_at" => DateTime.new(2020, 8, 25) } }
 
     it "uses the specific partition table" do
-      expect(Message.in_partition(partition).to_sql).to match(/^SELECT "messages_2020_35"\.\* FROM .*"messages_2020_35"$/)
+      expect(Message.in_partition(partition).to_sql).to match(/^SELECT "messages_2020_35".* FROM .*"messages_2020_35"$/)
     end
 
     it "can be chained" do
-      expect(Message.in_partition(partition).where(id: 3).to_sql).to match(/^SELECT "messages_2020_35"\.\* FROM .*"messages_2020_35" WHERE "messages_2020_35"."id" = 3$/)
+      expect(Message.in_partition(partition).where(id: 3).to_sql).to match(/^SELECT "messages_2020_35".* FROM .*"messages_2020_35" WHERE "messages_2020_35"."id" = 3$/)
     end
 
     it "has no side-effects on other scopes" do
-      expect(Message.in_partition(partition).unscoped.to_sql).to match(/^SELECT "messages"\.\* FROM .*"messages"$/)
+      expect(Message.in_partition(partition).unscoped.to_sql).to match(/^SELECT "messages".* FROM .*"messages"$/)
     end
   end