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 to ci_job_artifacts, lsf_objects and uploads tables

See merge request gitlab-org/gitlab!28946

app/models/ci/job_artifact.rb

@@ -73,12 +73,14 @@ module Ci
 
     validates :file_format, presence: true, unless: :trace?, on: :create
     validate :valid_file_format?, unless: :trace?, on: :create
-    before_save :set_size, if: :file_changed?
 
-    update_project_statistics project_statistics_name: :build_artifacts_size
+    before_save :set_size, if: :file_changed?
+    before_save :set_file_store, if: ->(job_artifact) { job_artifact.file_store.nil? }
 
     after_save :update_file_store, if: :saved_change_to_file?
 
+    update_project_statistics project_statistics_name: :build_artifacts_size
+
     scope :with_files_stored_locally, -> { where(file_store: [nil, ::JobArtifactUploader::Store::LOCAL]) }
     scope :with_files_stored_remotely, -> { where(file_store: ::JobArtifactUploader::Store::REMOTE) }
     scope :for_sha, ->(sha, project_id) { joins(job: :pipeline).where(ci_pipelines: { sha: sha, project_id: project_id }) }
@@ -226,6 +228,15 @@ module Ci
       self.size = file.size
     end
 
+    def set_file_store
+      self.file_store =
+        if JobArtifactUploader.object_store_enabled? && JobArtifactUploader.direct_upload_enabled?
+          JobArtifactUploader::Store::REMOTE
+        else
+          file.object_store
+        end
+    end
+
     def project_destroyed?
       # Use job.project to avoid extra DB query for project
       job.project.pending_delete?

app/models/lfs_object.rb

@@ -17,6 +17,8 @@ class LfsObject < ApplicationRecord
 
   mount_uploader :file, LfsObjectUploader
 
+  before_save :set_file_store, if: ->(lfs_object) { lfs_object.file_store.nil? }
+
   after_save :update_file_store, if: :saved_change_to_file?
 
   def self.not_linked_to_project(project)
@@ -55,6 +57,17 @@ class LfsObject < ApplicationRecord
   def self.calculate_oid(path)
     self.hexdigest(path)
   end
+
+  private
+
+  def set_file_store
+    self.file_store =
+      if LfsObjectUploader.object_store_enabled? && LfsObjectUploader.direct_upload_enabled?
+        LfsObjectUploader::Store::REMOTE
+      else
+        file.object_store
+      end
+  end
 end
 
 LfsObject.prepend_if_ee('EE::LfsObject')

app/uploaders/records_uploads.rb

@@ -56,10 +56,31 @@ module RecordsUploads
         size: file.size,
         path: upload_path,
         model: model,
-        mount_point: mounted_as
+        mount_point: mounted_as,
+        store: initial_store
       )
     end
 
+    def initial_store
+      if immediately_remote_stored?
+        ::ObjectStorage::Store::REMOTE
+      else
+        ::ObjectStorage::Store::LOCAL
+      end
+    end
+
+    def immediately_remote_stored?
+      object_storage_available? && direct_upload_enabled?
+    end
+
+    def object_storage_available?
+      self.class.ancestors.include?(ObjectStorage::Concern)
+    end
+
+    def direct_upload_enabled?
+      self.class.object_store_enabled? && self.class.direct_upload_enabled?
+    end
+
     # Before removing an attachment, destroy any Upload records at the same path
     #
     # Called `before :remove`

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 to ci_job_artifacts,
+  lfs_objects and uploads tables
+merge_request: 28946
+author:
+type: fixed

db/migrate/20200406165950_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
+
+  CONSTRAINT_NAME = 'lfs_objects_file_store_not_null'
+  DOWNTIME = false
+
+  def up
+    with_lock_retries do
+      execute <<~SQL
+        ALTER TABLE lfs_objects ADD CONSTRAINT #{CONSTRAINT_NAME} CHECK (file_store IS NOT NULL) NOT VALID;
+      SQL
+    end
+  end
+
+  def down
+    with_lock_retries do
+      execute <<~SQL
+        ALTER TABLE lfs_objects DROP CONSTRAINT IF EXISTS #{CONSTRAINT_NAME};
+      SQL
+    end
+  end
+end

db/migrate/20200406171857_add_not_null_constraint_on_file_store_to_ci_job_artifacts.rb

+# frozen_string_literal: true
+
+class AddNotNullConstraintOnFileStoreToCiJobArtifacts < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  CONSTRAINT_NAME = 'ci_job_artifacts_file_store_not_null'
+  DOWNTIME = false
+
+  def up
+    with_lock_retries do
+      execute <<~SQL
+        ALTER TABLE ci_job_artifacts ADD CONSTRAINT #{CONSTRAINT_NAME} CHECK (file_store IS NOT NULL) NOT VALID;
+      SQL
+    end
+  end
+
+  def down
+    with_lock_retries do
+      execute <<~SQL
+        ALTER TABLE ci_job_artifacts DROP CONSTRAINT IF EXISTS #{CONSTRAINT_NAME};
+      SQL
+    end
+  end
+end

db/migrate/20200406172135_add_not_null_constraint_on_file_store_to_uploads.rb

+# frozen_string_literal: true
+
+class AddNotNullConstraintOnFileStoreToUploads < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  CONSTRAINT_NAME = 'uploads_store_not_null'
+  DOWNTIME = false
+
+  def up
+    with_lock_retries do
+      execute <<~SQL
+        ALTER TABLE uploads ADD CONSTRAINT #{CONSTRAINT_NAME} CHECK (store IS NOT NULL) NOT VALID;
+      SQL
+    end
+  end
+
+  def down
+    with_lock_retries do
+      execute <<~SQL
+        ALTER TABLE uploads DROP CONSTRAINT IF EXISTS #{CONSTRAINT_NAME};
+      SQL
+    end
+  end
+end

db/structure.sql

@@ -7697,6 +7697,9 @@ ALTER TABLE ONLY public.ci_daily_report_results
 ALTER TABLE ONLY public.ci_group_variables
     ADD CONSTRAINT ci_group_variables_pkey PRIMARY KEY (id);
 
+ALTER TABLE public.ci_job_artifacts
+    ADD CONSTRAINT ci_job_artifacts_file_store_not_null CHECK ((file_store IS NOT NULL)) NOT VALID;
+
 ALTER TABLE ONLY public.ci_job_artifacts
     ADD CONSTRAINT ci_job_artifacts_pkey PRIMARY KEY (id);
 
@@ -8054,6 +8057,9 @@ ALTER TABLE ONLY public.ldap_group_links
 ALTER TABLE ONLY public.lfs_file_locks
     ADD CONSTRAINT lfs_file_locks_pkey PRIMARY KEY (id);
 
+ALTER TABLE public.lfs_objects
+    ADD CONSTRAINT lfs_objects_file_store_not_null CHECK ((file_store IS NOT NULL)) NOT VALID;
+
 ALTER TABLE ONLY public.lfs_objects
     ADD CONSTRAINT lfs_objects_pkey PRIMARY KEY (id);
 
@@ -8447,6 +8453,9 @@ ALTER TABLE ONLY public.u2f_registrations
 ALTER TABLE ONLY public.uploads
     ADD CONSTRAINT uploads_pkey PRIMARY KEY (id);
 
+ALTER TABLE public.uploads
+    ADD CONSTRAINT uploads_store_not_null CHECK ((store IS NOT NULL)) NOT VALID;
+
 ALTER TABLE ONLY public.user_agent_details
     ADD CONSTRAINT user_agent_details_pkey PRIMARY KEY (id);
 
@@ -13129,6 +13138,9 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200403185127
 20200403185422
 20200406135648
+20200406165950
+20200406171857
+20200406172135
 20200406192059
 20200407094005
 20200407094923

ee/spec/services/security/store_scans_service_spec.rb

@@ -51,6 +51,7 @@ describe Security::StoreScansService do
     before do
       create(:ee_ci_job_artifact, :dast_with_missing_file, job: build)
     end
+
     it 'stores 0 scanned resources on the scan' do
       subject
 

spec/factories/ci/job_artifacts.rb

@@ -13,7 +13,7 @@ FactoryBot.define do
     end
 
     trait :remote_store do
-      file_store { JobArtifactUploader::Store::REMOTE}
+      file_store { JobArtifactUploader::Store::REMOTE }
     end
 
     after :build do |artifact|

spec/models/ci/job_artifact_spec.rb

@@ -349,16 +349,13 @@ describe Ci::JobArtifact do
   end
 
   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)
+        subject = build(:ci_job_artifact, :archive, file_store: nil)
+
+        subject.save
+
+        expect(subject.file_store).to be(ObjectStorage::Store::LOCAL)
         expect(subject.file).to be_file_storage
         expect(subject.file.object_store).to eq(ObjectStorage::Store::LOCAL)
       end
@@ -366,6 +363,10 @@ describe Ci::JobArtifact do
 
     context 'when existing object has local store' do
       it 'is stored locally' do
+        subject = build(:ci_job_artifact, :archive)
+
+        subject.save
+
         expect(subject.file_store).to be(ObjectStorage::Store::LOCAL)
         expect(subject.file).to be_file_storage
         expect(subject.file.object_store).to eq(ObjectStorage::Store::LOCAL)
@@ -379,6 +380,10 @@ describe Ci::JobArtifact do
 
       context 'when file is stored' do
         it 'is stored remotely' do
+          subject = build(:ci_job_artifact, :archive)
+
+          subject.save
+
           expect(subject.file_store).to eq(ObjectStorage::Store::REMOTE)
           expect(subject.file).not_to be_file_storage
           expect(subject.file.object_store).to eq(ObjectStorage::Store::REMOTE)

spec/uploaders/records_uploads_spec.rb

@@ -78,7 +78,8 @@ describe RecordsUploads do
         path: File.join('uploads', 'rails_sample.jpg'),
         size: 512.kilobytes,
         model: build_stubbed(:user),
-        uploader: uploader.class.to_s
+        uploader: uploader.class.to_s,
+        store: ::ObjectStorage::Store::LOCAL
       )
 
       uploader.upload = existing
@@ -98,7 +99,8 @@ describe RecordsUploads do
         path: File.join('uploads', 'rails_sample.jpg'),
         size: 512.kilobytes,
         model: project,
-        uploader: uploader.class.to_s
+        uploader: uploader.class.to_s,
+        store: ::ObjectStorage::Store::LOCAL
       )
 
       uploader.store!(upload_fixture('rails_sample.jpg'))