Merge branch 'ab/ignorable-columns' into 'master'

Programmatically document column ignores

See merge request gitlab-org/gitlab!19963

app/models/application_setting.rb

@@ -5,12 +5,9 @@ class ApplicationSetting < ApplicationRecord
   include CacheMarkdownField
   include TokenAuthenticatable
   include ChronicDurationAttribute
+  include IgnorableColumns
 
-  # Only remove this >= %12.6 and >= 2019-12-01
-  self.ignored_columns += %i[
-      pendo_enabled
-      pendo_url
-    ]
+  ignore_columns :pendo_enabled, :pendo_url, remove_after: '2019-12-01', remove_with: '12.6'
 
   add_authentication_token_field :runners_registration_token, encrypted: -> { Feature.enabled?(:application_settings_tokens_optional_encryption, default_enabled: true) ? :optional : :required }
   add_authentication_token_field :health_check_access_token

app/models/ci/build.rb

@@ -13,17 +13,11 @@ module Ci
     include Importable
     include Gitlab::Utils::StrongMemoize
     include HasRef
+    include IgnorableColumns
 
     BuildArchivedError = Class.new(StandardError)
 
-    self.ignored_columns += %i[
-      artifacts_file
-      artifacts_file_store
-      artifacts_metadata
-      artifacts_metadata_store
-      artifacts_size
-      commands
-    ]
+    ignore_columns :artifacts_file, :artifacts_file_store, :artifacts_metadata, :artifacts_metadata_store, :artifacts_size, :commands, remove_after: '2019-12-15', remove_with: '12.7'
 
     belongs_to :project, inverse_of: :builds
     belongs_to :runner

app/models/ci/runner.rb

@@ -8,6 +8,7 @@ module Ci
     include ChronicDurationAttribute
     include FromUnion
     include TokenAuthenticatable
+    include IgnorableColumns
 
     add_authentication_token_field :token, encrypted: -> { Feature.enabled?(:ci_runners_tokens_optional_encryption, default_enabled: true) ? :optional : :required }
 
@@ -35,7 +36,7 @@ module Ci
 
     FORM_EDITABLE = %i[description tag_list active run_untagged locked access_level maximum_timeout_human_readable].freeze
 
-    self.ignored_columns += %i[is_shared]
+    ignore_column :is_shared, remove_after: '2019-12-15', remove_with: '12.6'
 
     has_many :builds
     has_many :runner_projects, inverse_of: :runner, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent

app/models/concerns/ignorable_columns.rb

+# frozen_string_literal: true
+
+module IgnorableColumns
+  extend ActiveSupport::Concern
+
+  ColumnIgnore = Struct.new(:remove_after, :remove_with) do
+    def safe_to_remove?
+      Date.today > remove_after
+    end
+
+    def to_s
+      "(#{remove_after}, #{remove_with})"
+    end
+  end
+
+  class_methods do
+    # Ignore database columns in a model
+    #
+    # Indicate the earliest date and release we can stop ignoring the column with +remove_after+ (a date string) and +remove_with+ (a release)
+    def ignore_columns(*columns, remove_after:, remove_with:)
+      raise ArgumentError, 'Please indicate when we can stop ignoring columns with remove_after (date string YYYY-MM-DD), example: ignore_columns(:name, remove_after: \'2019-12-01\', remove_with: \'12.6\')' unless remove_after =~ Gitlab::Regex.utc_date_regex
+      raise ArgumentError, 'Please indicate in which release we can stop ignoring columns with remove_with, example: ignore_columns(:name, remove_after: \'2019-12-01\', remove_with: \'12.6\')' unless remove_with
+
+      self.ignored_columns += columns.flatten # rubocop:disable Cop/IgnoredColumns
+
+      columns.flatten.each do |column|
+        self.ignored_columns_details[column.to_sym] = ColumnIgnore.new(Date.parse(remove_after), remove_with)
+      end
+    end
+
+    alias_method :ignore_column, :ignore_columns
+
+    def ignored_columns_details
+      unless defined?(@ignored_columns_details)
+        IGNORE_COLUMN_MUTEX.synchronize do
+          @ignored_columns_details ||= superclass.try(:ignored_columns_details)&.dup || {}
+        end
+      end
+
+      @ignored_columns_details
+    end
+
+    IGNORE_COLUMN_MUTEX = Mutex.new
+  end
+end

app/models/deploy_key.rb

@@ -2,6 +2,7 @@
 
 class DeployKey < Key
   include FromUnion
+  include IgnorableColumns
 
   has_many :deploy_keys_projects, inverse_of: :deploy_key, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
   has_many :projects, through: :deploy_keys_projects
@@ -10,7 +11,7 @@ class DeployKey < Key
   scope :are_public,  -> { where(public: true) }
   scope :with_projects, -> { includes(deploy_keys_projects: { project: [:route, :namespace] }) }
 
-  self.ignored_columns += %i[can_push]
+  ignore_column :can_push, remove_after: '2019-12-15', remove_with: '12.6'
 
   accepts_nested_attributes_for :deploy_keys_projects
 

app/models/epic.rb

@@ -3,7 +3,9 @@
 # Placeholder class for model that is implemented in EE
 # It reserves '&' as a reference prefix, but the table does not exists in CE
 class Epic < ApplicationRecord
-  self.ignored_columns += %i[milestone_id]
+  include IgnorableColumns
+
+  ignore_column :milestone_id, remove_after: '2019-12-15', remove_with: '12.7'
 
   def self.link_reference_pattern
     nil

app/models/issue.rb

@@ -14,6 +14,7 @@ class Issue < ApplicationRecord
   include TimeTrackable
   include ThrottledTouch
   include LabelEventable
+  include IgnorableColumns
 
   DueDateStruct                   = Struct.new(:title, :name).freeze
   NoDueDate                       = DueDateStruct.new('No Due Date', '0').freeze
@@ -68,8 +69,7 @@ class Issue < ApplicationRecord
 
   scope :counts_by_state, -> { reorder(nil).group(:state_id).count }
 
-  # Only remove after 2019-12-22 and with %12.7
-  self.ignored_columns += %i[state]
+  ignore_column :state, remove_with: '12.7', remove_after: '2019-12-22'
 
   after_commit :expire_etag_cache
   after_save :ensure_metrics, unless: :imported?

app/models/merge_request.rb

@@ -17,6 +17,7 @@ class MergeRequest < ApplicationRecord
   include FromUnion
   include DeprecatedAssignee
   include ShaAttribute
+  include IgnorableColumns
 
   sha_attribute :squash_commit_sha
 
@@ -239,8 +240,7 @@ class MergeRequest < ApplicationRecord
     with_state(:opened).where(auto_merge_enabled: true)
   end
 
-  # Only remove after 2019-12-22 and with %12.7
-  self.ignored_columns += %i[state]
+  ignore_column :state, remove_with: '12.7', remove_after: '2019-12-22'
 
   after_save :keep_around_commit
 

app/models/project.rb

@@ -31,6 +31,7 @@ class Project < ApplicationRecord
   include FeatureGate
   include OptionallySearch
   include FromUnion
+  include IgnorableColumns
   extend Gitlab::Cache::RequestCache
 
   extend Gitlab::ConfigHelper
@@ -64,7 +65,7 @@ class Project < ApplicationRecord
 
   # TODO: remove once GitLab 12.5 is released
   # https://gitlab.com/gitlab-org/gitlab/issues/34638
-  self.ignored_columns += %i[merge_requests_require_code_owner_approval]
+  ignore_column :merge_requests_require_code_owner_approval, remove_after: '2019-12-01', remove_with: '12.6'
 
   default_value_for :archived, false
   default_value_for :resolve_outdated_diff_discussions, false

app/models/project_ci_cd_setting.rb

 # frozen_string_literal: true
 
 class ProjectCiCdSetting < ApplicationRecord
-  # TODO: remove once GitLab 12.7 is released
+  include IgnorableColumns
   # https://gitlab.com/gitlab-org/gitlab/issues/36651
-  self.ignored_columns += %i[merge_trains_enabled]
+  ignore_column :merge_trains_enabled, remove_with: '12.7', remove_after: '2019-12-22'
+
   belongs_to :project, inverse_of: :ci_cd_settings
 
   # The version of the schema that first introduced this model/table.

doc/development/database_review.md

@@ -101,6 +101,7 @@ and details for a database reviewer:
   - Check consistency with `db/schema.rb` and that migrations are [reversible](migration_style_guide.md#reversibility)
   - Check queries timing (If any): Queries executed in a migration
     need to fit comfortably within `15s` - preferably much less than that - on GitLab.com.
+  - For column removals, make sure the column has been [ignored in a previous release](what_requires_downtime.md#dropping-columns)
 - Check [background migrations](background_migrations.md):
   - Establish a time estimate for execution on GitLab.com.
   - They should only be used when migrating data in larger tables.

doc/development/what_requires_downtime.md

@@ -37,11 +37,19 @@ information on how to use this method.
 ## Dropping Columns
 
 Removing columns is tricky because running GitLab processes may still be using
-the columns. To work around this you will need two separate merge requests and
-releases: one to ignore and then remove the column, and one to remove the ignore
-rule.
+the columns. To work around this safely, you will need three steps in three releases:
 
-### Step 1: Ignoring The Column
+1. Ignoring the column (release M)
+1. Dropping the column (release M+1)
+1. Removing the ignore rule (release M+2)
+
+The reason we spread this out across three releases is that dropping a column is
+a destructive operation that can't be rolled back easily.
+
+Following this procedure helps us to make sure there are no deployments to GitLab.com
+and upgrade processes for self-hosted installations that lump together any of these steps.
+
+### Step 1: Ignoring the column (release M)
 
 The first step is to ignore the column in the application code. This is
 necessary because Rails caches the columns and re-uses this cache in various
@@ -50,18 +58,46 @@ places. This can be done by defining the columns to ignore. For example, to igno
 
 ```ruby
 class User < ApplicationRecord
-  self.ignored_columns += %i[updated_at]
+  include IgnorableColumns
+  ignore_column :updated_at, remove_with: '12.7', remove_after: '2019-12-22'
 end
 ```
 
-Once added you should create a _post-deployment_ migration that removes the
-column. Both these changes should be submitted in the same merge request.
+Multiple columns can be ignored, too:
+
+```ruby
+ignore_columns %i[updated_at created_at], remove_with: '12.7', remove_after: '2019-12-22'
+```
+
+We require indication of when it is safe to remove the column ignore with:
+
+- `remove_with`: set to a GitLab release typically two releases (M+2) after adding the
+  column ignore.
+- `remove_after`: set to a date after which we consider it safe to remove the column
+  ignore, typically within the development cycle of release M+2.
+
+This information allows us to reason better about column ignores and makes sure we
+don't remove column ignores too early for both regular releases and deployments to GitLab.com. For
+example, this avoids a situation where we deploy a bulk of changes that include both changes
+to ignore the column and subsequently remove the column ignore (which would result in a downtime).
+
+In this example, the change to ignore the column went into release 12.5.
+
+### Step 2: Dropping the column (release M+1)
+
+Continuing our example, dropping the column goes into a _post-deployment_ migration in release 12.6:
+
+```ruby
+ remove_column :user, :updated_at
+```
+
+### Step 3: Removing the ignore rule (release M+2)
 
-### Step 2: Removing The Ignore Rule
+With the next release, in this example 12.7, we set up another merge request to remove the ignore rule.
+This removes the `ignore_column` line and - if not needed anymore - also the inclusion of `IgnoreableColumns`.
 
-Once the changes from step 1 have been released & deployed you can set up a
-separate merge request that removes the ignore rule. This merge request can
-simply remove the `self.ignored_columns` line.
+This should only get merged with the release indicated with `remove_with` and once
+the `remove_after` date has passed.
 
 ## Renaming Columns
 

ee/app/models/ee/project.rb

@@ -11,6 +11,7 @@ module EE
     extend ::Gitlab::Cache::RequestCache
     include ::Gitlab::Utils::StrongMemoize
     include ::EE::GitlabRoutingHelper # rubocop: disable Cop/InjectEnterpriseEditionModule
+    include IgnorableColumns
 
     GIT_LFS_DOWNLOAD_OPERATION = 'download'.freeze
 
@@ -23,10 +24,7 @@ module EE
       include DeprecatedApprovalsBeforeMerge
       include UsageStatistics
 
-      self.ignored_columns += %i[
-        mirror_last_update_at
-        mirror_last_successful_update_at
-      ]
+      ignore_columns :mirror_last_update_at, :mirror_last_successful_update_at, remove_after: '2019-12-15', remove_with: '12.6'
 
       before_save :set_override_pull_mirror_available, unless: -> { ::Gitlab::CurrentSettings.mirror_available }
       before_save :set_next_execution_timestamp_to_now, if: ->(project) { project.mirror? && project.mirror_changed? && project.import_state }

ee/app/models/operations/feature_flags_client.rb

@@ -3,9 +3,10 @@
 module Operations
   class FeatureFlagsClient < ApplicationRecord
     include TokenAuthenticatable
+    include IgnorableColumns
 
     self.table_name = 'operations_feature_flags_clients'
-    self.ignored_columns += %i[token]
+    ignore_column :token, remove_after: '2019-12-15', remove_with: '12.6'
 
     belongs_to :project
 

ee/app/models/plan.rb

 # frozen_string_literal: true
 
 class Plan < ApplicationRecord
-  # Remove these in version >= 12.6
-  self.ignored_columns += %i[active_pipelines_limit pipeline_size_limit active_jobs_limit]
+  include IgnorableColumns
+
+  ignore_columns %i[active_pipelines_limit pipeline_size_limit active_jobs_limit], remove_after: '2019-12-01', remove_with: '12.6'
 
   DEFAULT = 'default'.freeze
   FREE = 'free'.freeze

lib/gitlab/database/obsolete_ignored_columns.rb

@@ -23,8 +23,15 @@ module Gitlab
       private
 
       def ignored_columns_safe_to_remove_for(klass)
-        ignored = klass.ignored_columns.map(&:to_s)
+        ignores = ignored_and_not_present(klass).each_with_object({}) do |col, h|
+          h[col] = klass.ignored_columns_details[col.to_sym]
+        end
 
+        ignores.select { |_, i| i&.safe_to_remove? }
+      end
+
+      def ignored_and_not_present(klass)
+        ignored = klass.ignored_columns.map(&:to_s)
         return [] if ignored.empty?
 
         schema = klass.connection.schema_cache.columns_hash(klass.table_name)

lib/tasks/db_obsolete_ignored_columns.rake

@@ -8,7 +8,10 @@ task 'db:obsolete_ignored_columns' => :environment do
     puts 'The following `ignored_columns` are obsolete and can be removed:'
 
     list.each do |name, ignored_columns|
-      puts "- #{name}: #{ignored_columns.join(', ')}"
+      puts "#{name}:"
+      ignored_columns.each do |column, removal|
+        puts " - #{column.ljust(30)} Remove after #{removal.remove_after} with #{removal.remove_with}"
+      end
     end
 
     puts <<~TEXT

rubocop/cop/ignored_columns.rb

+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    # Cop that blacklists the usage of Group.public_or_visible_to_user
+    class IgnoredColumns < RuboCop::Cop::Cop
+      MSG = 'Use `IgnoredColumns` concern instead of adding to `self.ignored_columns`.'
+
+      def_node_matcher :ignored_columns?, <<~PATTERN
+        (send (self) :ignored_columns)
+      PATTERN
+
+      def on_send(node)
+        return unless ignored_columns?(node)
+
+        add_offense(node, location: :expression)
+      end
+    end
+  end
+end

rubocop/rubocop.rb

@@ -54,3 +54,4 @@ require_relative 'cop/group_public_or_visible_to_user'
 require_relative 'cop/inject_enterprise_edition_module'
 require_relative 'cop/graphql/authorize_types'
 require_relative 'cop/graphql/descriptions'
+require_relative 'cop/ignored_columns'

spec/lib/gitlab/database/obsolete_ignored_columns_spec.rb

@@ -8,21 +8,27 @@ describe Gitlab::Database::ObsoleteIgnoredColumns do
     end
 
     class SomeAbstract < MyBase
+      include IgnorableColumns
+
       self.abstract_class = true
 
       self.table_name = 'projects'
 
-      self.ignored_columns += %i[unused]
+      ignore_column :unused, remove_after: '2019-01-01', remove_with: '12.0'
     end
 
     class B < MyBase
+      include IgnorableColumns
+
       self.table_name = 'issues'
 
-      self.ignored_columns += %i[id other]
+      ignore_column :id, :other, remove_after: '2019-01-01', remove_with: '12.0'
+      ignore_column :not_used_but_still_ignored, remove_after: Date.today.to_s, remove_with: '12.1'
     end
 
     class A < SomeAbstract
-      self.ignored_columns += %i[id also_unused]
+      ignore_column :also_unused, remove_after: '2019-02-01', remove_with: '12.1'
+      ignore_column :not_used_but_still_ignored, remove_after: Date.today.to_s, remove_with: '12.1'
     end
 
     class C < MyBase
@@ -35,8 +41,13 @@ describe Gitlab::Database::ObsoleteIgnoredColumns do
   describe '#execute' do
     it 'returns a list of class names and columns pairs' do
       expect(subject.execute).to eq([
-        ['Testing::A', %w(unused also_unused)],
-        ['Testing::B', %w(other)]
+        ['Testing::A', {
+          'unused' => IgnorableColumns::ColumnIgnore.new(Date.parse('2019-01-01'), '12.0'),
+          'also_unused' => IgnorableColumns::ColumnIgnore.new(Date.parse('2019-02-01'), '12.1')
+        }],
+        ['Testing::B', {
+          'other' => IgnorableColumns::ColumnIgnore.new(Date.parse('2019-01-01'), '12.0')
+        }]
       ])
     end
   end

spec/models/concerns/ignorable_columns_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe IgnorableColumns do
+  let(:record_class) do
+    Class.new(ApplicationRecord) do
+      include IgnorableColumns
+    end
+  end
+
+  subject { record_class }
+
+  it 'adds columns to ignored_columns' do
+    expect do
+      subject.ignore_columns(:name, :created_at, remove_after: '2019-12-01', remove_with: '12.6')
+    end.to change { subject.ignored_columns }.from([]).to(%w(name created_at))
+  end
+
+  it 'adds columns to ignored_columns (array version)' do
+    expect do
+      subject.ignore_columns(%i[name created_at], remove_after: '2019-12-01', remove_with: '12.6')
+    end.to change { subject.ignored_columns }.from([]).to(%w(name created_at))
+  end
+
+  it 'requires remove_after attribute to be set' do
+    expect { subject.ignore_columns(:name, remove_after: nil, remove_with: 12.6) }.to raise_error(ArgumentError, /Please indicate/)
+  end
+
+  it 'requires remove_after attribute to be set' do
+    expect { subject.ignore_columns(:name, remove_after: "not a date", remove_with: 12.6) }.to raise_error(ArgumentError, /Please indicate/)
+  end
+
+  it 'requires remove_with attribute to be set' do
+    expect { subject.ignore_columns(:name, remove_after: '2019-12-01', remove_with: nil) }.to raise_error(ArgumentError, /Please indicate/)
+  end
+
+  describe '.ignored_columns_details' do
+    shared_examples_for 'storing removal information' do
+      it 'storing removal information' do
+        subject.ignore_column(columns, remove_after: '2019-12-01', remove_with: '12.6')
+
+        [columns].flatten.each do |column|
+          expect(subject.ignored_columns_details[column].remove_after).to eq(Date.parse('2019-12-01'))
+          expect(subject.ignored_columns_details[column].remove_with).to eq('12.6')
+        end
+      end
+    end
+
+    context 'with single column' do
+      let(:columns) { :name }
+      it_behaves_like 'storing removal information'
+    end
+
+    context 'with array column' do
+      let(:columns) { %i[name created_at] }
+      it_behaves_like 'storing removal information'
+    end
+
+    it 'defaults to empty Hash' do
+      expect(subject.ignored_columns_details).to eq({})
+    end
+  end
+
+  describe IgnorableColumns::ColumnIgnore do
+    subject { described_class.new(remove_after, remove_with) }
+
+    let(:remove_with) { double }
+
+    describe '#safe_to_remove?' do
+      context 'after remove_after date has passed' do
+        let(:remove_after) { Date.parse('2019-01-10') }
+
+        it 'returns true (safe to remove)' do
+          expect(subject.safe_to_remove?).to be_truthy
+        end
+      end
+
+      context 'before remove_after date has passed' do
+        let(:remove_after) { Date.today }
+
+        it 'returns false (not safe to remove)' do
+          expect(subject.safe_to_remove?).to be_falsey
+        end
+      end
+    end
+  end
+end

spec/rubocop/cop/ignored_columns_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require 'rubocop'
+require 'rubocop/rspec/support'
+require_relative '../../../rubocop/cop/ignored_columns'
+
+describe RuboCop::Cop::IgnoredColumns do
+  include CopHelper
+
+  subject(:cop) { described_class.new }
+
+  it 'flags the use of destroy_all with a local variable receiver' do
+    inspect_source(<<~RUBY)
+      class Foo < ApplicationRecord
+        self.ignored_columns += %i[id]
+      end
+    RUBY
+
+    expect(cop.offenses.size).to eq(1)
+  end
+end