Merge branch '213382-use-not-valid-to-immediately-enforce-a-not-null-constraint' into 'master'

Use NOT VALID to enforce a not null constraint on file store columns

See merge request gitlab-org/gitlab!31261

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 }