Merge branch '208403-attachment-file-not-found' into 'master'

Ensure valid path is used by AttachmentUploader

See merge request gitlab-org/gitlab!26849

app/uploaders/attachment_uploader.rb

@@ -11,4 +11,15 @@ class AttachmentUploader < GitlabUploader
   def dynamic_segment
     File.join(model.class.underscore, mounted_as.to_s, model.id.to_s)
   end
+
+  def mounted_as
+    # Geo fails to sync attachments on Note, and LegacyDiffNotes with missing mount_point.
+    #
+    # See https://gitlab.com/gitlab-org/gitlab/-/issues/209752 for more details.
+    if model.class.underscore.include?('note')
+      super || 'attachment'
+    else
+      super
+    end
+  end
 end

changelogs/unreleased/208403-attachment-file-not-found.yml

+---
+title: Ensure valid mount point is used by attachments on notes
+merge_request: 26849
+author:
+type: fixed

ee/spec/requests/api/geo_spec.rb

@@ -63,7 +63,7 @@ describe API::Geo do
       end
     end
 
-    describe 'GET /geo/transfers/attachment/1' do
+    describe 'GET /geo/transfers/attachment/:id' do
       let(:note) { create(:note, :with_attachment) }
       let(:upload) { Upload.find_by(model: note, uploader: 'AttachmentUploader') }
       let(:transfer) { Gitlab::Geo::Replication::FileTransfer.new(:attachment, upload) }
@@ -81,7 +81,7 @@ describe API::Geo do
         expect(response).to have_gitlab_http_status(:unauthorized)
       end
 
-      context 'attachment file exists' do
+      context 'when attachment file exists' do
         subject(:request) { get api("/geo/transfers/attachment/#{upload.id}"), headers: req_header }
 
         it 'responds with 200 with X-Sendfile' do
@@ -95,13 +95,25 @@ describe API::Geo do
         it_behaves_like 'with terms enforced'
       end
 
-      context 'attachment does not exist' do
+      context 'when attachment does not exist' do
         it 'responds with 404' do
           get api("/geo/transfers/attachment/100000"), headers: not_found_req_header
 
           expect(response).to have_gitlab_http_status(:not_found)
         end
       end
+
+      context 'when attachment has mount_point nil' do
+        it 'responds with 200 with X-Sendfile' do
+          upload.update(mount_point: nil)
+
+          get api("/geo/transfers/attachment/#{upload.id}"), headers: req_header
+
+          expect(response).to have_gitlab_http_status(:ok)
+          expect(response.headers['Content-Type']).to eq('application/octet-stream')
+          expect(response.headers['X-Sendfile']).to eq(note.attachment.path)
+        end
+      end
     end
 
     describe 'GET /geo/transfers/avatar/1' do

spec/lib/gitlab/background_migration/legacy_upload_mover_spec.rb

@@ -149,10 +149,12 @@ describe Gitlab::BackgroundMigration::LegacyUploadMover, schema: :latest do
 
     context 'when an upload belongs to a legacy_diff_note' do
       let!(:merge_request) { create(:merge_request, source_project: project) }
+
       let!(:note) do
         create(:legacy_diff_note_on_merge_request,
           note: 'some note', project: project, noteable: merge_request)
       end
+
       let(:legacy_upload) do
         create(:upload, :with_file, :attachment_upload,
                path: "uploads/-/system/note/attachment/#{note.id}/#{filename}", model: note)
@@ -193,6 +195,17 @@ describe Gitlab::BackgroundMigration::LegacyUploadMover, schema: :latest do
         it_behaves_like 'move error'
       end
 
+      context 'when upload has mount_point nil' do
+        let(:legacy_upload) do
+          create(:upload, :with_file, :attachment_upload,
+                 path: "uploads/-/system/note/attachment/#{note.id}/#{filename}", model: note, mount_point: nil)
+        end
+
+        it_behaves_like 'migrates the file correctly'
+        it_behaves_like 'legacy local file'
+        it_behaves_like 'legacy upload deletion'
+      end
+
       context 'when the file can be handled correctly' do
         it_behaves_like 'migrates the file correctly'
         it_behaves_like 'legacy local file'

spec/models/upload_spec.rb

@@ -127,6 +127,36 @@ describe Upload do
       expect(uploader.mounted_as).to eq(subject.send(:mount_point))
       expect(uploader.file).not_to be_nil
     end
+
+    context 'when upload has mount_point nil' do
+      context 'when an upload belongs to a note' do
+        it 'mounts it as attachment' do
+          project = create(:project, :legacy_storage)
+          merge_request = create(:merge_request, source_project: project)
+          note = create(:legacy_diff_note_on_merge_request, note: 'some note', project: project, noteable: merge_request)
+
+          subject = build(:upload, :with_file, :attachment_upload, model: note, mount_point: nil)
+          uploader = subject.retrieve_uploader
+
+          expect(uploader.upload).to eq(subject)
+          expect(uploader.path).to include('attachment')
+          expect(uploader.file).not_to be_nil
+        end
+      end
+
+      context 'when an upload does not belong to a note' do
+        it 'does not mount it as attachment' do
+          appearance = create(:appearance)
+
+          subject = build(:upload, :with_file, :attachment_upload, model: appearance, mount_point: nil)
+          uploader = subject.retrieve_uploader
+
+          expect(uploader.upload).to eq(subject)
+          expect(uploader.path).not_to include('attachment')
+          expect(uploader.file).not_to be_nil
+        end
+      end
+    end
   end
 
   describe '#needs_checksum?' do