Merge branch 'mk/enforce-not-null-external-diff-store-on-mr-diffs' into 'master'

Geo: Part 3 of Enforce not null merge_request_diffs.external_diff_store

See merge request gitlab-org/gitlab!42045

changelogs/unreleased/mk-enforce-not-null-external-diff-store-on-mr-diffs.yml

+---
+title: Validate not null external_diff_store field on merge_request_diffs to maintain
+  data integrity
+merge_request: 42045
+author:
+type: added

db/post_migrate/20200910170908_ensure_filled_external_diff_store_on_merge_request_diffs.rb

+# frozen_string_literal: true
+
+class EnsureFilledExternalDiffStoreOnMergeRequestDiffs < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  BACKGROUND_MIGRATION_CLASS = 'SetNullExternalDiffStoreToLocalValue'
+  BATCH_SIZE = 5_000
+  LOCAL_STORE = 1 # equal to ObjectStorage::Store::LOCAL
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  class MergeRequestDiff < ActiveRecord::Base
+    self.table_name = 'merge_request_diffs'
+
+    include ::EachBatch
+  end
+
+  def up
+    Gitlab::BackgroundMigration.steal(BACKGROUND_MIGRATION_CLASS)
+
+    # Do a manual update in case we lost BG jobs. The expected record count should be 0 or very low.
+    MergeRequestDiff.where(external_diff_store: nil).each_batch(of: BATCH_SIZE) do |batch, index|
+      batch.update_all(external_diff_store: LOCAL_STORE)
+    end
+  end
+
+  def down
+    # no-op
+  end
+end

db/post_migrate/20200910175553_validate_not_null_external_diff_store_on_merge_request_diffs.rb

+# frozen_string_literal: true
+
+class ValidateNotNullExternalDiffStoreOnMergeRequestDiffs < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  # Remove index which was only added to fill external_diff_store
+  INDEX_NAME = 'index_merge_request_diffs_external_diff_store_is_null'
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    validate_not_null_constraint :merge_request_diffs, :external_diff_store
+
+    remove_concurrent_index_by_name :merge_request_diffs, INDEX_NAME
+  end
+
+  def down
+    add_concurrent_index :merge_request_diffs, :id, where: 'external_diff_store IS NULL', name: INDEX_NAME
+  end
+end

db/schema_migrations/20200910170908

+de13fed042f6e3d2a6612a77acebf168c8beddab5796b96f2d77886dea431ceb
\ No newline at end of file

db/schema_migrations/20200910175553

+904e0a8623df766a1f385bbb3db8942d10c4a92354d8f5e3bc03a813337c5fa1
\ No newline at end of file

db/structure.sql

@@ -13238,7 +13238,8 @@ CREATE TABLE public.merge_request_diffs (
     external_diff character varying,
     external_diff_store integer DEFAULT 1,
     stored_externally boolean,
-    files_count smallint
+    files_count smallint,
+    CONSTRAINT check_93ee616ac9 CHECK ((external_diff_store IS NOT NULL))
 );
 
 CREATE SEQUENCE public.merge_request_diffs_id_seq
@@ -17976,9 +17977,6 @@ ALTER TABLE public.vulnerability_scanners
 ALTER TABLE public.packages_package_files
     ADD CONSTRAINT check_4c5e6bb0b3 CHECK ((file_store IS NOT NULL)) NOT VALID;
 
-ALTER TABLE public.merge_request_diffs
-    ADD CONSTRAINT check_93ee616ac9 CHECK ((external_diff_store IS NOT NULL)) NOT VALID;
-
 ALTER TABLE ONLY public.ci_build_needs
     ADD CONSTRAINT ci_build_needs_pkey PRIMARY KEY (id);
 
@@ -20301,8 +20299,6 @@ CREATE UNIQUE INDEX index_merge_request_diff_files_on_mr_diff_id_and_order ON pu
 
 CREATE INDEX index_merge_request_diffs_by_id_partial ON public.merge_request_diffs USING btree (id) WHERE ((files_count > 0) AND ((NOT stored_externally) OR (stored_externally IS NULL)));
 
-CREATE INDEX index_merge_request_diffs_external_diff_store_is_null ON public.merge_request_diffs USING btree (id) WHERE (external_diff_store IS NULL);
-
 CREATE INDEX index_merge_request_diffs_on_external_diff_store ON public.merge_request_diffs USING btree (external_diff_store);
 
 CREATE INDEX index_merge_request_diffs_on_merge_request_id_and_id ON public.merge_request_diffs USING btree (merge_request_id, id);

spec/migrations/ensure_filled_external_diff_store_on_merge_request_diffs_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20200910170908_ensure_filled_external_diff_store_on_merge_request_diffs.rb')
+
+RSpec.describe EnsureFilledExternalDiffStoreOnMergeRequestDiffs, schema: 20200908095446 do
+  let!(:merge_request_diffs) { table(:merge_request_diffs) }
+  let!(:merge_requests) { table(:merge_requests) }
+  let!(:namespaces) { table(:namespaces) }
+  let!(:projects) { table(:projects) }
+  let!(:namespace) { namespaces.create!(name: 'foo', path: 'foo') }
+  let!(:project) { projects.create!(namespace_id: namespace.id) }
+  let!(:merge_request) { merge_requests.create!(source_branch: 'x', target_branch: 'master', target_project_id: project.id) }
+
+  before do
+    constraint_name = 'check_93ee616ac9'
+
+    # In order to insert a row with a NULL to fill.
+    ActiveRecord::Base.connection.execute "ALTER TABLE merge_request_diffs DROP CONSTRAINT #{constraint_name}"
+
+    @external_diff_store_1 = merge_request_diffs.create!(external_diff_store: 1, merge_request_id: merge_request.id)
+    @external_diff_store_2 = merge_request_diffs.create!(external_diff_store: 2, merge_request_id: merge_request.id)
+    @external_diff_store_nil = merge_request_diffs.create!(external_diff_store: nil, merge_request_id: merge_request.id)
+
+    # revert DB structure
+    ActiveRecord::Base.connection.execute "ALTER TABLE merge_request_diffs ADD CONSTRAINT #{constraint_name} CHECK ((external_diff_store IS NOT NULL)) NOT VALID"
+  end
+
+  it 'correctly migrates nil external_diff_store to 1' do
+    migrate!
+
+    @external_diff_store_1.reload
+    @external_diff_store_2.reload
+    @external_diff_store_nil.reload
+
+    expect(@external_diff_store_1.external_diff_store).to eq(1)   # unchanged
+    expect(@external_diff_store_2.external_diff_store).to eq(2)   # unchanged
+    expect(@external_diff_store_nil.external_diff_store).to eq(1) # nil => 1
+  end
+end