Merge branch 'mk/enforce-not-null-file-store-on-package-files' into 'master'

Geo: Part 2 of Enforce not null packages_package_files.file_store

Closes #235516

See merge request gitlab-org/gitlab!42400

changelogs/unreleased/mk-enforce-not-null-file-store-on-package-files.yml

+---
+title: Validate not null file_store field on packages_package_files to maintain data
+  integrity
+merge_request: 42400
+author:
+type: added

db/post_migrate/20200915185707_ensure_filled_file_store_on_package_files.rb

+# frozen_string_literal: true
+
+class EnsureFilledFileStoreOnPackageFiles < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  BACKGROUND_MIGRATION_CLASS = 'SetNullPackageFilesFileStoreToLocalValue'
+  BATCH_SIZE = 5_000
+  LOCAL_STORE = 1 # equal to ObjectStorage::Store::LOCAL
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  module Packages
+    class PackageFile < ActiveRecord::Base
+      self.table_name = 'packages_package_files'
+
+      include ::EachBatch
+    end
+  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.
+    Packages::PackageFile.where(file_store: nil).each_batch(of: BATCH_SIZE) do |batch, index|
+      batch.update_all(file_store: LOCAL_STORE)
+    end
+  end
+
+  def down
+    # no-op
+  end
+end

db/post_migrate/20200915191156_validate_not_null_file_store_on_package_files.rb

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

db/schema_migrations/20200915185707

+e1ae80d6f0a6372bb329d45257d9a0a8ca5b6a83718d2a10ee295b8c4c97f60e
\ No newline at end of file

db/schema_migrations/20200915191156

+d8ddec6b234d59b3b85705dfa7b724d3be4974bfa57fae70aa5c2dbdd2e73cfa
\ No newline at end of file

db/structure.sql

@@ -14123,7 +14123,8 @@ CREATE TABLE packages_package_files (
     verified_at timestamp with time zone,
     verification_failure character varying(255),
     verification_retry_count integer,
-    verification_checksum bytea
+    verification_checksum bytea,
+    CONSTRAINT check_4c5e6bb0b3 CHECK ((file_store IS NOT NULL))
 );
 
 CREATE SEQUENCE packages_package_files_id_seq
@@ -18084,9 +18085,6 @@ ALTER TABLE design_management_designs
 ALTER TABLE vulnerability_scanners
     ADD CONSTRAINT check_37608c9db5 CHECK ((char_length(vendor) <= 255)) NOT VALID;
 
-ALTER TABLE packages_package_files
-    ADD CONSTRAINT check_4c5e6bb0b3 CHECK ((file_store IS NOT NULL)) NOT VALID;
-
 ALTER TABLE group_import_states
     ADD CONSTRAINT check_cda75c7c3f CHECK ((user_id IS NOT NULL)) NOT VALID;
 
@@ -20684,8 +20682,6 @@ CREATE INDEX index_packages_nuget_dl_metadata_on_dependency_link_id ON packages_
 
 CREATE UNIQUE INDEX index_packages_on_project_id_name_version_unique_when_generic ON packages_packages USING btree (project_id, name, version) WHERE (package_type = 7);
 
-CREATE INDEX index_packages_package_files_file_store_is_null ON packages_package_files USING btree (id) WHERE (file_store IS NULL);
-
 CREATE INDEX index_packages_package_files_on_file_store ON packages_package_files USING btree (file_store);
 
 CREATE INDEX index_packages_package_files_on_package_id_and_file_name ON packages_package_files USING btree (package_id, file_name);

spec/migrations/ensure_filled_file_store_on_package_files_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20200915185707_ensure_filled_file_store_on_package_files.rb')
+
+RSpec.describe EnsureFilledFileStoreOnPackageFiles, schema: 20200910175553 do
+  let!(:packages_package_files) { table(:packages_package_files) }
+  let!(:packages_packages) { table(:packages_packages) }
+  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!(:package) { packages_packages.create!(project_id: project.id, name: 'bar', package_type: 1) }
+
+  before do
+    constraint_name = 'check_4c5e6bb0b3'
+
+    # In order to insert a row with a NULL to fill.
+    ActiveRecord::Base.connection.execute "ALTER TABLE packages_package_files DROP CONSTRAINT #{constraint_name}"
+
+    @file_store_1 = packages_package_files.create!(file_store: 1, file_name: 'foo_1', file: 'foo_1', package_id: package.id)
+    @file_store_2 = packages_package_files.create!(file_store: 2, file_name: 'foo_2', file: 'foo_2', package_id: package.id)
+    @file_store_nil = packages_package_files.create!(file_store: nil, file_name: 'foo_nil', file: 'foo_nil', package_id: package.id)
+
+    # revert DB structure
+    ActiveRecord::Base.connection.execute "ALTER TABLE packages_package_files ADD CONSTRAINT #{constraint_name} CHECK ((file_store IS NOT NULL)) NOT VALID"
+  end
+
+  it 'correctly migrates nil file_store to 1' do
+    migrate!
+
+    @file_store_1.reload
+    @file_store_2.reload
+    @file_store_nil.reload
+
+    expect(@file_store_1.file_store).to eq(1)   # unchanged
+    expect(@file_store_2.file_store).to eq(2)   # unchanged
+    expect(@file_store_nil.file_store).to eq(1) # nil => 1
+  end
+end