Add migration to fill file_store from NULL to 1

we already performed a background migration in to fill file
stores for these tables, but at that time we not added the
NOT NULL constraint.

Since the record count is low for these tables, we are now
filling the file stores using a regular migration.

changelogs/unreleased/213382-use-not-valid-to-immediately-enforce-a-not-null-constraint.yml

+---
+title: Use NOT VALID to enforce a not null constraint on file store columns
+merge_request: 31261
+author:
+type: performance

db/migrate/20200513234502_fill_file_store_lfs_objects.rb

+# frozen_string_literal: true
+
+class FillFileStoreLfsObjects < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    update_column_in_batches(:lfs_objects, :file_store, 1) do |table, query|
+      query.where(table[:file_store].eq(nil))
+    end
+  end
+
+  def down
+    # no-op
+  end
+end

db/migrate/20200513235347_fill_store_uploads.rb

+# frozen_string_literal: true
+
+class FillStoreUploads < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    update_column_in_batches(:uploads, :store, 1) do |table, query|
+      query.where(table[:store].eq(nil))
+    end
+  end
+
+  def down
+    # no-op
+  end
+end

db/migrate/20200513235532_fill_file_store_ci_job_artifacts.rb

+# frozen_string_literal: true
+
+class FillFileStoreCiJobArtifacts < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    # rubocop:disable Migration/UpdateLargeTable
+    update_column_in_batches(:ci_job_artifacts, :file_store, 1) do |table, query|
+      query.where(table[:file_store].eq(nil))
+    end
+    # rubocop:enable Migration/UpdateLargeTable
+  end
+
+  def down
+    # no-op
+  end
+end

db/post_migrate/20200514000009_add_not_null_constraint_on_file_store_to_lfs_objects.rb

+# frozen_string_literal: true
+
+class AddNotNullConstraintOnFileStoreToLfsObjects < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_not_null_constraint(:lfs_objects, :file_store, validate: false)
+  end
+
+  def down
+    remove_not_null_constraint(:lfs_objects, :file_store)
+  end
+end

db/post_migrate/20200514000132_add_not_null_constraint_on_store_to_uploads.rb

+# frozen_string_literal: true
+
+class AddNotNullConstraintOnStoreToUploads < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_not_null_constraint(:uploads, :store, validate: false)
+  end
+
+  def down
+    remove_not_null_constraint(:uploads, :store)
+  end
+end

db/post_migrate/20200514000340_add_not_null_constraint_on_file_store_to_ci_jobs_artifacts.rb

+# frozen_string_literal: true
+
+class AddNotNullConstraintOnFileStoreToCiJobsArtifacts < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_not_null_constraint(:ci_job_artifacts, :file_store, validate: false)
+  end
+
+  def down
+    remove_not_null_constraint(:ci_job_artifacts, :file_store)
+  end
+end

db/structure.sql

@@ -7995,6 +7995,15 @@ ALTER TABLE ONLY public.chat_names
 ALTER TABLE ONLY public.chat_teams
     ADD CONSTRAINT chat_teams_pkey PRIMARY KEY (id);
 
+ALTER TABLE public.ci_job_artifacts
+    ADD CONSTRAINT check_27f0f6dbab CHECK ((file_store IS NOT NULL)) NOT VALID;
+
+ALTER TABLE public.uploads
+    ADD CONSTRAINT check_5e9547379c CHECK ((store IS NOT NULL)) NOT VALID;
+
+ALTER TABLE public.lfs_objects
+    ADD CONSTRAINT check_eecfc5717d CHECK ((file_store IS NOT NULL)) NOT VALID;
+
 ALTER TABLE ONLY public.ci_build_needs
     ADD CONSTRAINT ci_build_needs_pkey PRIMARY KEY (id);
 
@@ -13795,5 +13804,11 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200511162057
 20200511162115
 20200512085150
+20200513234502
+20200513235347
+20200513235532
+20200514000009
+20200514000132
+20200514000340
 \.
 

spec/migrations/fill_file_store_ci_job_artifacts_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'migrate', '20200513235532_fill_file_store_ci_job_artifacts.rb')
+
+describe FillFileStoreCiJobArtifacts do
+  let(:namespaces) { table(:namespaces) }
+  let(:projects) { table(:projects) }
+  let(:builds) { table(:ci_builds) }
+  let(:job_artifacts) { table(:ci_job_artifacts) }
+
+  before do
+    namespaces.create!(id: 123, name: 'sample', path: 'sample')
+    projects.create!(id: 123, name: 'sample', path: 'sample', namespace_id: 123)
+    builds.create!(id: 1)
+  end
+
+  context 'when file_store is nil' do
+    it 'updates file_store to local' do
+      job_artifacts.create!(project_id: 123, job_id: 1, file_type: 1, file_store: nil)
+      job_artifact = job_artifacts.find_by(project_id: 123, job_id: 1)
+
+      expect { migrate! }.to change { job_artifact.reload.file_store }.from(nil).to(1)
+    end
+  end
+
+  context 'when file_store is set to local' do
+    it 'does not update file_store' do
+      job_artifacts.create!(project_id: 123, job_id: 1, file_type: 1, file_store: 1)
+      job_artifact = job_artifacts.find_by(project_id: 123, job_id: 1)
+
+      expect { migrate! }.not_to change { job_artifact.reload.file_store }
+    end
+  end
+
+  context 'when file_store is set to object storage' do
+    it 'does not update file_store' do
+      job_artifacts.create!(project_id: 123, job_id: 1, file_type: 1, file_store: 2)
+      job_artifact = job_artifacts.find_by(project_id: 123, job_id: 1)
+
+      expect { migrate! }.not_to change { job_artifact.reload.file_store }
+    end
+  end
+end

spec/migrations/fill_file_store_lfs_objects_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'migrate', '20200513234502_fill_file_store_lfs_objects.rb')
+
+describe FillFileStoreLfsObjects do
+  let(:lfs_objects) { table(:lfs_objects) }
+  let(:oid) { 'b804383982bb89b00e828e3f44c038cc991d3d1768009fc39ba8e2c081b9fb75' }
+
+  context 'when file_store is nil' do
+    it 'updates file_store to local' do
+      lfs_objects.create(oid: oid, size: 1062, file_store: nil)
+      lfs_object = lfs_objects.find_by(oid: oid)
+
+      expect { migrate! }.to change { lfs_object.reload.file_store }.from(nil).to(1)
+    end
+  end
+
+  context 'when file_store is set to local' do
+    it 'does not update file_store' do
+      lfs_objects.create(oid: oid, size: 1062, file_store: 1)
+      lfs_object = lfs_objects.find_by(oid: oid)
+
+      expect { migrate! }.not_to change { lfs_object.reload.file_store }
+    end
+  end
+
+  context 'when file_store is set to object storage' do
+    it 'does not update file_store' do
+      lfs_objects.create(oid: oid, size: 1062, file_store: 2)
+      lfs_object = lfs_objects.find_by(oid: oid)
+
+      expect { migrate! }.not_to change { lfs_object.reload.file_store }
+    end
+  end
+end

spec/migrations/fill_store_uploads_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'migrate', '20200513235347_fill_store_uploads.rb')
+
+describe FillStoreUploads do
+  let(:uploads) { table(:uploads) }
+  let(:path) { 'uploads/-/system/avatar.jpg' }
+
+  context 'when store is nil' do
+    it 'updates store to local' do
+      uploads.create(size: 100.kilobytes,
+                     uploader: 'AvatarUploader',
+                     path: path,
+                     store: nil)
+
+      upload = uploads.find_by(path: path)
+
+      expect { migrate! }.to change { upload.reload.store }.from(nil).to(1)
+    end
+  end
+
+  context 'when store is set to local' do
+    it 'does not update store' do
+      uploads.create(size: 100.kilobytes,
+                     uploader: 'AvatarUploader',
+                     path: path,
+                     store: 1)
+
+      upload = uploads.find_by(path: path)
+
+      expect { migrate! }.not_to change { upload.reload.store }
+    end
+  end
+
+  context 'when store is set to object storage' do
+    it 'does not update store' do
+      uploads.create(size: 100.kilobytes,
+                     uploader: 'AvatarUploader',
+                     path: path,
+                     store: 2)
+
+      upload = uploads.find_by(path: path)
+
+      expect { migrate! }.not_to change { upload.reload.store }
+    end
+  end
+end

spec/models/ci/job_artifact_spec.rb

@@ -378,19 +378,6 @@ describe Ci::JobArtifact do
   describe 'file is being stored' do
     subject { create(:ci_job_artifact, :archive) }
 
-    context 'when object has nil store' do
-      before do
-        subject.update_column(:file_store, nil)
-        subject.reload
-      end
-
-      it 'is stored locally' do
-        expect(subject.file_store).to be(nil)
-        expect(subject.file).to be_file_storage
-        expect(subject.file.object_store).to eq(ObjectStorage::Store::LOCAL)
-      end
-    end
-
     context 'when existing object has local store' do
       it 'is stored locally' do
         expect(subject.file_store).to be(ObjectStorage::Store::LOCAL)

spec/tasks/gitlab/artifacts/migrate_rake_spec.rb

@@ -22,18 +22,6 @@ describe 'gitlab:artifacts namespace rake task' do
     context 'when local storage is used' do
       let(:store) { ObjectStorage::Store::LOCAL }
 
-      context 'and job does not have file store defined' do
-        let(:object_storage_enabled) { true }
-        let(:store) { nil }
-
-        it "migrates file to remote storage" do
-          subject
-
-          expect(artifact.reload.file_store).to eq(ObjectStorage::Store::REMOTE)
-          expect(job_trace.reload.file_store).to eq(ObjectStorage::Store::REMOTE)
-        end
-      end
-
       context 'and remote storage is defined' do
         let(:object_storage_enabled) { true }