another round of spec fixing

app/models/group.rb

@@ -34,6 +34,7 @@ class Group < Namespace
 
   has_many :ldap_group_links, foreign_key: 'group_id', dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
   has_many :hooks, dependent: :destroy, class_name: 'GroupHook' # rubocop:disable Cop/ActiveRecordDependent
+  has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
 
   # We cannot simply set `has_many :audit_events, as: :entity, dependent: :destroy`
   # here since Group inherits from Namespace, the entity_type would be set to `Namespace`.

app/models/upload.rb

@@ -12,10 +12,6 @@ class Upload < ActiveRecord::Base
   before_save  :calculate_checksum!, if: :foreground_checksummable?
   after_commit :schedule_checksum,   if: :checksummable?
 
-  def self.remove_path(path)
-    where(path: path).destroy_all
-  end
-
   def self.hexdigest(path)
     Digest::SHA256.file(path).hexdigest
   end

app/models/user.rb

@@ -139,6 +139,7 @@ class User < ActiveRecord::Base
   has_many :assigned_merge_requests,  dependent: :nullify, foreign_key: :assignee_id, class_name: "MergeRequest" # rubocop:disable Cop/ActiveRecordDependent
 
   has_many :custom_attributes, class_name: 'UserCustomAttribute'
+  has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
 
   #
   # Validations

app/uploaders/records_uploads.rb

@@ -5,7 +5,6 @@ module RecordsUploads
     attr_accessor :upload
 
     included do
-      before :store,  :destroy_previous_upload
       after  :store,  :record_upload
       before :remove, :destroy_upload
     end
@@ -21,12 +20,13 @@ module RecordsUploads
       return unless model
       return unless file && file.exists?
 
-      Upload.create(
-        size: file.size,
-        path: upload_path,
-        model: model,
-        uploader: self.class.to_s
-      )
+      Upload.transaction do
+        uploads.where(path: upload_path).delete_all
+        upload.destroy! if upload
+
+        self.upload = build_upload_from_uploader(self)
+        upload.save!
+      end
     end
 
     def upload_path
@@ -35,10 +35,17 @@ module RecordsUploads
 
     private
 
-    def destroy_previous_upload(*args)
-      return unless upload
+    def uploads
+      Upload.order(id: :desc).where(uploader: self.class.to_s)
+    end
 
-      upload.destroy!
+    def build_upload_from_uploader(uploader)
+      Upload.new(
+        size: uploader.file.size,
+        path: uploader.upload_path,
+        model: uploader.model,
+        uploader: uploader.class.to_s
+      )
     end
 
     # Before removing an attachment, destroy any Upload records at the same path
@@ -47,8 +54,8 @@ module RecordsUploads
     def destroy_upload(*args)
       return unless file && file.exists?
 
-      # that should be the old path?
-      Upload.remove_path(upload_path)
+      self.upload = nil
+      uploads.where(path: upload_path).delete_all
     end
   end
 end

ee/app/uploaders/object_storage.rb

@@ -29,30 +29,16 @@ module ObjectStorage
       def retrieve_from_store!(identifier)
         paths = store_dirs.map { |store, path| File.join(path, identifier) }
 
-        unless paths.include?(upload&.path)
+        unless current_upload_satisfies?(paths, model)
           # we already have the right upload, don't fetch
-          self.upload = Upload.find_by(uploader: self.class.to_s, model: model, path: paths)
+          self.upload = uploads.find_by(model: model, path: paths)
         end
 
         super
       end
 
-      def record_upload(_tempfile = nil)
-        return unless model
-        return unless file && file.exists?
-
-        self.upload = Upload.create(
-          size: file.size,
-          path: upload_path,
-          model: model,
-          uploader: self.class.to_s,
-          store: object_store
-        )
-      end
-
-      def destroy_upload(_tempfile = nil)
-        super
-        self.upload = nil
+      def build_upload_from_uploader(uploader)
+        super.tap { |upload| upload.store = object_store }
       end
 
       def upload=(upload)
@@ -61,6 +47,17 @@ module ObjectStorage
         self.object_store = upload.store
         super
       end
+
+      private
+
+      def current_upload_satisfies?(paths, model)
+        return false unless upload
+        return false unless model
+
+        paths.include?(upload.path) &&
+          upload.model_id == model.id &&
+          upload.model_type == model.class.to_s
+      end
     end
   end
 

spec/uploaders/object_storage_spec.rb

@@ -110,26 +110,16 @@ describe ObjectStorage do
   # this means the model shall include
   #   include RecordsUpload::Concern
   #   prepend ObjectStorage::Extension::RecordsUploads
-  # the object_store persistence is delegated to the `Upload` model
-  # this also implies a <mounted_as>_uploader method can be implemented to
-  # correctly fetch the upload.
+  # the object_store persistence is delegated to the `Upload` model.
   #
   context 'when persist_object_store? is false' do
     let(:object) { create(:project, :with_avatar) }
-    let(:uploader_class) { AvatarUploader }
-    let(:uploader) { uploader_class.new(object, :avatar) }
-    # let(:upload) { create(:upload, model: project) }
+    let(:uploader) { object.avatar }
 
     it { expect(object).to be_a(Avatarable) }
     it { expect(uploader.persist_object_store?).to be_falsey }
 
     describe 'delegates the object_store logic to the `Upload` model' do
-      it 'call the <mounted_as>_uploader hook' do
-        expect(object).to receive(:avatar_upload)
-
-        expect(uploader).to be
-      end
-
       it 'sets @upload to the found `upload`' do
         expect(uploader.upload).to eq(uploader.upload)
       end
@@ -140,7 +130,6 @@ describe ObjectStorage do
     end
   end
 
-  # TODO: persist_object_store? is true
   # this means the model holds an <mounted_as>_store attribute directly
   # and do not delegate the object_store persistence to the `Upload` model.
   #

spec/uploaders/records_uploads_spec.rb

@@ -33,12 +33,6 @@ describe RecordsUploads do
       uploader.store!(upload_fixture('doc_sample.txt'))
     end
 
-    it '#destroy_upload before `store`' do
-      expect(uploader).to receive(:destroy_upload).once
-
-      uploader.store!(upload_fixture('doc_sample.txt'))
-    end
-
     it '#destroy_upload after `remove`' do
       uploader.store!(upload_fixture('doc_sample.txt'))
 
@@ -48,25 +42,34 @@ describe RecordsUploads do
   end
 
   describe '#record_upload callback' do
-    it "returns early when the file doesn't exist" do
-      allow(uploader).to receive(:file).and_return(double(exists?: false))
-      expect(Upload).not_to receive(:record)
+    it 'creates an Upload record after store' do
+      expect { uploader.store!(upload_fixture('rails_sample.jpg')) }.to change { Upload.count }.by(1)
+    end
 
+    it 'creates a new record and assigns size, path, model, and uploader' do
       uploader.store!(upload_fixture('rails_sample.jpg'))
+
+      upload = uploader.upload
+      aggregate_failures do
+        expect(upload).to be_persisted
+        expect(upload.size).to eq uploader.file.size
+        expect(upload.path).to eq uploader.upload_path
+        expect(upload.model_id).to eq uploader.model.id
+        expect(upload.model_type).to eq uploader.model.class.to_s
+        expect(upload.uploader).to eq uploader.class
+      end
     end
 
-    it 'creates an Upload record after store' do
-      expect(Upload).to receive(:record)
-        .with(uploader)
+    it "does not create an Upload record when the file doesn't exist" do
+      allow(uploader).to receive(:file).and_return(double(exists?: false))
 
-      uploader.store!(upload_fixture('rails_sample.jpg'))
+      expect { uploader.store!(upload_fixture('rails_sample.jpg')) }.to_not change { Upload.count }
     end
 
     it 'does not create an Upload record if model is missing' do
-      expect_any_instance_of(RecordsUploadsExampleUploader).to receive(:model).and_return(nil)
-      expect(Upload).not_to receive(:record).with(uploader)
+      allow_any_instance_of(RecordsUploadsExampleUploader).to receive(:model).and_return(nil)
 
-      uploader.store!(upload_fixture('rails_sample.jpg'))
+      expect { uploader.store!(upload_fixture('rails_sample.jpg')) }.to_not change { Upload.count }
     end
 
     it 'it destroys Upload records at the same path before recording' do
@@ -77,6 +80,7 @@ describe RecordsUploads do
         uploader: uploader.class.to_s
       )
 
+      uploader.upload = existing
       uploader.store!(upload_fixture('rails_sample.jpg'))
 
       expect { existing.reload }.to raise_error(ActiveRecord::RecordNotFound)
@@ -85,12 +89,6 @@ describe RecordsUploads do
   end
 
   describe '#destroy_upload callback' do
-    it 'returns early when file is nil' do
-      expect(Upload).not_to receive(:remove_path)
-
-      uploader.remove!
-    end
-
     it 'it destroys Upload records at the same path after removal' do
       uploader.store!(upload_fixture('rails_sample.jpg'))