Merge branch 'revert-13382-use-not-valid-to-immediately' into 'master'

Reverts !28946 because of 400 errors uploading CI artifacts

See merge request gitlab-org/gitlab!29589

app/models/ci/job_artifact.rb

@@ -73,14 +73,12 @@ 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?
-    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
 
+    after_save :update_file_store, if: :saved_change_to_file?
+
     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 }) }
@@ -228,15 +226,6 @@ 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,8 +17,6 @@ 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)
@@ -57,17 +55,6 @@ 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,31 +56,10 @@ module RecordsUploads
         size: file.size,
         path: upload_path,
         model: model,
-        mount_point: mounted_as,
-        store: initial_store
+        mount_point: mounted_as
       )
     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/20200406172135_add_not_null_constraint_on_file_store_to_uploads.rb → db/migrate/20200415160722_remove_not_null_lfs_objects_constraint.rb

 # frozen_string_literal: true
 
-class AddNotNullConstraintOnFileStoreToUploads < ActiveRecord::Migration[6.0]
+class RemoveNotNullLfsObjectsConstraint < 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;
+        ALTER TABLE lfs_objects DROP CONSTRAINT IF EXISTS lfs_objects_file_store_not_null;
       SQL
     end
   end
 
   def down
-    with_lock_retries do
-      execute <<~SQL
-        ALTER TABLE uploads DROP CONSTRAINT IF EXISTS #{CONSTRAINT_NAME};
-      SQL
-    end
+    # No-op
   end
 end

db/migrate/20200406165950_add_not_null_constraint_on_file_store_to_lfs_objects.rb → db/migrate/20200415161021_remove_not_null_ci_job_artifacts_constraint.rb

 # frozen_string_literal: true
 
-class AddNotNullConstraintOnFileStoreToLfsObjects < ActiveRecord::Migration[6.0]
+class RemoveNotNullCiJobArtifactsConstraint < 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;
+        ALTER TABLE ci_job_artifacts DROP CONSTRAINT IF EXISTS ci_job_artifacts_file_store_not_null;
       SQL
     end
   end
 
   def down
-    with_lock_retries do
-      execute <<~SQL
-        ALTER TABLE lfs_objects DROP CONSTRAINT IF EXISTS #{CONSTRAINT_NAME};
-      SQL
-    end
+    # No-op
   end
 end

db/migrate/20200406171857_add_not_null_constraint_on_file_store_to_ci_job_artifacts.rb → db/migrate/20200415161206_remove_not_null_uploads_constraint.rb

 # frozen_string_literal: true
 
-class AddNotNullConstraintOnFileStoreToCiJobArtifacts < ActiveRecord::Migration[6.0]
+class RemoveNotNullUploadsConstraint < 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;
+        ALTER TABLE uploads DROP CONSTRAINT IF EXISTS uploads_store_not_null;
       SQL
     end
   end
 
   def down
-    with_lock_retries do
-      execute <<~SQL
-        ALTER TABLE ci_job_artifacts DROP CONSTRAINT IF EXISTS #{CONSTRAINT_NAME};
-      SQL
-    end
+    # No-op
   end
 end

db/structure.sql

@@ -7699,9 +7699,6 @@ 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);
 
@@ -8059,9 +8056,6 @@ 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);
 
@@ -8455,9 +8449,6 @@ 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);
 
@@ -13162,9 +13153,6 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200406102111
 20200406102120
 20200406135648
-20200406165950
-20200406171857
-20200406172135
 20200406192059
 20200406193427
 20200407094005
@@ -13173,5 +13161,8 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200408153842
 20200408175424
 20200409211607
+20200415160722
+20200415161021
+20200415161206
 \.
 

ee/spec/services/security/store_scans_service_spec.rb

@@ -51,7 +51,6 @@ 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,13 +349,16 @@ describe Ci::JobArtifact do
   end
 
   describe 'file is being stored' do
-    context 'when object has nil store' do
-      it 'is stored locally' do
-        subject = build(:ci_job_artifact, :archive, file_store: nil)
+    subject { create(:ci_job_artifact, :archive) }
 
-        subject.save
+    context 'when object has nil store' do
+      before do
+        subject.update_column(:file_store, nil)
+        subject.reload
+      end
 
-        expect(subject.file_store).to be(ObjectStorage::Store::LOCAL)
+      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
@@ -363,10 +366,6 @@ 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)
@@ -380,10 +379,6 @@ 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,8 +78,7 @@ describe RecordsUploads do
         path: File.join('uploads', 'rails_sample.jpg'),
         size: 512.kilobytes,
         model: build_stubbed(:user),
-        uploader: uploader.class.to_s,
-        store: ::ObjectStorage::Store::LOCAL
+        uploader: uploader.class.to_s
       )
 
       uploader.upload = existing
@@ -99,8 +98,7 @@ describe RecordsUploads do
         path: File.join('uploads', 'rails_sample.jpg'),
         size: 512.kilobytes,
         model: project,
-        uploader: uploader.class.to_s,
-        store: ::ObjectStorage::Store::LOCAL
+        uploader: uploader.class.to_s
       )
 
       uploader.store!(upload_fixture('rails_sample.jpg'))