Merge branch 'ops-ff-iid-atomback' into 'master'

Add IID to Feature Flags and Backfill with Param in AtomicInternalId

See merge request gitlab-org/gitlab!22175

app/models/concerns/atomic_internal_id.rb

@@ -27,7 +27,7 @@ module AtomicInternalId
   extend ActiveSupport::Concern
 
   class_methods do
-    def has_internal_id(column, scope:, init:, ensure_if: nil, track_if: nil, presence: true) # rubocop:disable Naming/PredicateName
+    def has_internal_id(column, scope:, init:, ensure_if: nil, track_if: nil, presence: true, backfill: false) # rubocop:disable Naming/PredicateName
       # We require init here to retain the ability to recalculate in the absence of a
       # InternalId record (we may delete records in `internal_ids` for example).
       raise "has_internal_id requires a init block, none given." unless init
@@ -38,6 +38,8 @@ module AtomicInternalId
       validates column, presence: presence
 
       define_method("ensure_#{scope}_#{column}!") do
+        return if backfill && self.class.where(column => nil).exists?
+
         scope_value = internal_id_read_scope(scope)
         value = read_attribute(column)
         return value unless scope_value

app/models/internal_id.rb

@@ -21,7 +21,7 @@ class InternalId < ApplicationRecord
   belongs_to :project
   belongs_to :namespace
 
-  enum usage: { issues: 0, merge_requests: 1, deployments: 2, milestones: 3, epics: 4, ci_pipelines: 5 }
+  enum usage: { issues: 0, merge_requests: 1, deployments: 2, milestones: 3, epics: 4, ci_pipelines: 5, operations_feature_flags: 6 }
 
   validates :usage, presence: true
 

changelogs/unreleased/ops-ff-iid.yml

+---
+title: Add iid to operations_feature_flags and backfill
+merge_request: 22175
+author:
+type: added

db/migrate/20200117194830_add_iid_to_operations_feature_flags.rb

+# frozen_string_literal: true
+
+class AddIidToOperationsFeatureFlags < ActiveRecord::Migration[5.2]
+  DOWNTIME = false
+
+  def up
+    add_column :operations_feature_flags, :iid, :integer
+  end
+
+  def down
+    remove_column :operations_feature_flags, :iid
+  end
+end

db/migrate/20200117194840_add_index_on_operations_feature_flags_iid.rb

+# frozen_string_literal: true
+
+class AddIndexOnOperationsFeatureFlagsIid < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :operations_feature_flags, [:project_id, :iid], unique: true
+  end
+
+  def down
+    remove_concurrent_index :operations_feature_flags, [:project_id, :iid]
+  end
+end

db/post_migrate/20200117194850_backfill_operations_feature_flags_iid.rb

+# frozen_string_literal: true
+
+class BackfillOperationsFeatureFlagsIid < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  ###
+  # This should update about 700 rows on gitlab.com
+  # Execution time is predicted to take less than a second based on #database-lab results
+  # https://gitlab.com/gitlab-org/gitlab/merge_requests/22175#migration-performance
+  ###
+  def up
+    execute('LOCK operations_feature_flags IN ACCESS EXCLUSIVE MODE')
+
+    backfill_iids('operations_feature_flags')
+
+    change_column_null :operations_feature_flags, :iid, false
+  end
+
+  def down
+    change_column_null :operations_feature_flags, :iid, true
+  end
+end

db/post_migrate/20200117194900_delete_internal_ids_where_feature_flags_usage.rb

+# frozen_string_literal: true
+
+class DeleteInternalIdsWhereFeatureFlagsUsage < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def up
+    sql = <<~SQL
+      DELETE FROM internal_ids WHERE usage = 6
+    SQL
+
+    execute(sql)
+  end
+
+  def down
+    # no-op
+  end
+end

db/schema.rb

@@ -2881,6 +2881,8 @@ ActiveRecord::Schema.define(version: 2020_01_27_090233) do
     t.datetime_with_timezone "updated_at", null: false
     t.string "name", null: false
     t.text "description"
+    t.integer "iid", null: false
+    t.index ["project_id", "iid"], name: "index_operations_feature_flags_on_project_id_and_iid", unique: true
     t.index ["project_id", "name"], name: "index_operations_feature_flags_on_project_id_and_name", unique: true
   end
 

ee/app/models/operations/feature_flag.rb

@@ -2,10 +2,14 @@
 
 module Operations
   class FeatureFlag < ApplicationRecord
+    include AtomicInternalId
+
     self.table_name = 'operations_feature_flags'
 
     belongs_to :project
 
+    has_internal_id :iid, scope: :project, init: ->(s) { s&.project&.operations_feature_flags&.maximum(:iid) }, backfill: true, presence: false
+
     default_value_for :active, true
 
     has_many :scopes, class_name: 'Operations::FeatureFlagScope'

ee/spec/migrations/backfill_operations_feature_flags_iid_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20200117194850_backfill_operations_feature_flags_iid.rb')
+
+describe BackfillOperationsFeatureFlagsIid, :migration do
+  let(:namespaces)     { table(:namespaces) }
+  let(:projects)       { table(:projects) }
+  let(:flags)          { table(:operations_feature_flags) }
+  let(:issues)         { table(:issues) }
+  let(:merge_requests) { table(:merge_requests) }
+  let(:internal_ids)   { table(:internal_ids) }
+
+  def setup
+    namespace = namespaces.create!(name: 'foo', path: 'foo')
+    project = projects.create!(namespace_id: namespace.id)
+
+    project
+  end
+
+  it 'backfills the iid for a flag' do
+    project = setup
+    flag = flags.create!(project_id: project.id, active: true, name: 'test_flag')
+
+    expect(flag.iid).to be_nil
+
+    disable_migrations_output { migrate! }
+
+    expect(flag.reload.iid).to eq(1)
+  end
+
+  it 'backfills the iid for multiple flags' do
+    project = setup
+    flag_a = flags.create!(project_id: project.id, active: true, name: 'test_flag')
+    flag_b = flags.create!(project_id: project.id, active: false, name: 'other_flag')
+    flag_c = flags.create!(project_id: project.id, active: false, name: 'last_flag', created_at: '2019-10-11T08:00:11Z')
+
+    expect(flag_a.iid).to be_nil
+    expect(flag_b.iid).to be_nil
+
+    disable_migrations_output { migrate! }
+
+    expect(flag_a.reload.iid).to eq(1)
+    expect(flag_b.reload.iid).to eq(2)
+    expect(flag_c.reload.iid).to eq(3)
+  end
+
+  it 'backfills the iid for multiple flags across projects' do
+    project_a = setup
+    project_b = setup
+    flag_a = flags.create!(project_id: project_a.id, active: true, name: 'test_flag')
+    flag_b = flags.create!(project_id: project_b.id, active: false, name: 'other_flag')
+
+    expect(flag_a.iid).to be_nil
+    expect(flag_b.iid).to be_nil
+
+    disable_migrations_output { migrate! }
+
+    expect(flag_a.reload.iid).to eq(1)
+    expect(flag_b.reload.iid).to eq(1)
+  end
+
+  it 'does not change an iid for an issue' do
+    project = setup
+    flag = flags.create!(project_id: project.id, active: true, name: 'test_flag')
+    issue = issues.create!(project_id: project.id, iid: 8)
+    internal_id = internal_ids.create!(project_id: project.id, usage: 0, last_value: issue.iid)
+
+    disable_migrations_output { migrate! }
+
+    expect(flag.reload.iid).to eq(1)
+    expect(issue.reload.iid).to eq(8)
+    expect(internal_id.reload.usage).to eq(0)
+    expect(internal_id.last_value).to eq(8)
+  end
+
+  it 'does not change an iid for a merge request' do
+    project_a = setup
+    project_b = setup
+    flag = flags.create!(project_id: project_a.id, active: true, name: 'test_flag')
+    merge_request_a = merge_requests.create!(target_project_id: project_b.id, target_branch: 'master', source_branch: 'feature-1', title: 'merge request', iid: 1)
+    merge_request_b = merge_requests.create!(target_project_id: project_b.id, target_branch: 'master', source_branch: 'feature-2', title: 'merge request', iid: 2)
+    internal_id = internal_ids.create!(project_id: project_b.id, usage: 1, last_value: merge_request_b.iid)
+
+    disable_migrations_output { migrate! }
+
+    expect(flag.reload.iid).to eq(1)
+    expect(merge_request_a.reload.iid).to eq(1)
+    expect(merge_request_b.reload.iid).to eq(2)
+    expect(internal_id.reload.usage).to eq(1)
+    expect(internal_id.last_value).to eq(2)
+  end
+end

ee/spec/models/operations/feature_flag_spec.rb

@@ -16,6 +16,14 @@ describe Operations::FeatureFlag do
     it { is_expected.to validate_presence_of(:project) }
     it { is_expected.to validate_presence_of(:name) }
     it { is_expected.to validate_uniqueness_of(:name).scoped_to(:project_id) }
+
+    it_behaves_like 'AtomicInternalId', validate_presence: false do
+      let(:internal_id_attribute) { :iid }
+      let(:instance) { build(:operations_feature_flag) }
+      let(:scope) { :project }
+      let(:scope_attrs) { { project: instance.project } }
+      let(:usage) { :operations_feature_flags }
+    end
   end
 
   describe 'Scope creation' do

lib/gitlab/database/migration_helpers.rb

@@ -1119,6 +1119,20 @@ into similar problems in the future (e.g. when new tables are created).
         SQL
       end
 
+      # Note this should only be used with very small tables
+      def backfill_iids(table)
+        sql = <<-END
+          UPDATE #{table}
+          SET iid = #{table}_with_calculated_iid.iid_num
+          FROM (
+            SELECT id, ROW_NUMBER() OVER (PARTITION BY project_id ORDER BY id ASC) AS iid_num FROM #{table}
+          ) AS #{table}_with_calculated_iid
+          WHERE #{table}.id = #{table}_with_calculated_iid.id
+        END
+
+        execute(sql)
+      end
+
       private
 
       def tables_match?(target_table, foreign_key_table)

spec/migrations/backfill_operations_feature_flags_iid_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20200117194850_backfill_operations_feature_flags_iid.rb')
+
+describe BackfillOperationsFeatureFlagsIid, :migration do
+  let(:namespaces)   { table(:namespaces) }
+  let(:projects)     { table(:projects) }
+  let(:flags)        { table(:operations_feature_flags) }
+
+  def setup
+    namespace = namespaces.create!(name: 'foo', path: 'foo')
+    project = projects.create!(namespace_id: namespace.id)
+
+    project
+  end
+
+  it 'migrates successfully when there are no flags in the database' do
+    setup
+
+    disable_migrations_output { migrate! }
+
+    expect(flags.count).to eq(0)
+  end
+
+  it 'migrates successfully with a row in the table in both FOSS and EE' do
+    project = setup
+    flags.create!(project_id: project.id, active: true, name: 'test_flag')
+
+    disable_migrations_output { migrate! }
+
+    expect(flags.count).to eq(1)
+  end
+end

spec/migrations/delete_internal_ids_where_feature_flags_usage_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20200117194900_delete_internal_ids_where_feature_flags_usage')
+
+describe DeleteInternalIdsWhereFeatureFlagsUsage, :migration do
+  let(:namespaces)   { table(:namespaces) }
+  let(:projects)     { table(:projects) }
+  let(:internal_ids) { table(:internal_ids) }
+
+  def setup
+    namespace = namespaces.create!(name: 'foo', path: 'foo')
+    project = projects.create!(namespace_id: namespace.id)
+
+    project
+  end
+
+  it 'deletes feature flag rows from the internal_ids table' do
+    project = setup
+    internal_ids.create!(project_id: project.id, usage: 6, last_value: 1)
+
+    disable_migrations_output { migrate! }
+
+    expect(internal_ids.count).to eq(0)
+  end
+
+  it 'does not delete issue rows from the internal_ids table' do
+    project = setup
+    internal_ids.create!(project_id: project.id, usage: 0, last_value: 1)
+
+    disable_migrations_output { migrate! }
+
+    expect(internal_ids.count).to eq(1)
+  end
+
+  it 'does not delete merge request rows from the internal_ids table' do
+    project = setup
+    internal_ids.create!(project_id: project.id, usage: 1, last_value: 1)
+
+    disable_migrations_output { migrate! }
+
+    expect(internal_ids.count).to eq(1)
+  end
+end