From 869d08b581495161352a661ac29b20b3925deaf0 Mon Sep 17 00:00:00 2001
From: Michael Kozono <mkozono@gmail.com>
Date: Tue, 5 Dec 2017 12:26:20 -0800
Subject: [PATCH] Process normal paths in batch containing bad paths

---
 .../populate_untracked_uploads.rb             | 29 ++++++++++++++++---
 .../populate_untracked_uploads_spec.rb        | 20 +++++++++++++
 2 files changed, 45 insertions(+), 4 deletions(-)

diff --git a/lib/gitlab/background_migration/populate_untracked_uploads.rb b/lib/gitlab/background_migration/populate_untracked_uploads.rb
index ebb483c3cff..81e95e5832d 100644
--- a/lib/gitlab/background_migration/populate_untracked_uploads.rb
+++ b/lib/gitlab/background_migration/populate_untracked_uploads.rb
@@ -57,7 +57,7 @@ module Gitlab
         ].freeze
 
         def to_h
-          {
+          @upload_hash ||= {
             path: upload_path,
             uploader: uploader,
             model_type: model_type,
@@ -156,8 +156,8 @@ module Gitlab
         return unless migrate?
 
         files = UntrackedFile.where(id: start_id..end_id)
-        insert_uploads_if_needed(files)
-        files.delete_all
+        processed_files = insert_uploads_if_needed(files)
+        processed_files.delete_all
 
         drop_temp_table_if_finished
       end
@@ -169,9 +169,30 @@ module Gitlab
       end
 
       def insert_uploads_if_needed(files)
-        filtered_files = filter_existing_uploads(files)
+        filtered_files, error_files = filter_error_files(files)
+        filtered_files = filter_existing_uploads(filtered_files)
         filtered_files = filter_deleted_models(filtered_files)
         insert(filtered_files)
+
+        processed_files = files.where.not(id: error_files.map(&:id))
+        processed_files
+      end
+
+      def filter_error_files(files)
+        files.partition do |file|
+          begin
+            file.to_h
+            true
+          rescue => e
+            msg = <<~MSG
+              Error parsing path "#{file.path}":
+                #{e.message}
+                #{e.backtrace.join("\n  ")}
+            MSG
+            Rails.logger.error(msg)
+            false
+          end
+        end
       end
 
       def filter_existing_uploads(files)
diff --git a/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb b/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb
index 35ea8059510..e1a5a17a60c 100644
--- a/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb
+++ b/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb
@@ -119,6 +119,26 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :migration, :sid
 
       expect(table_exists?(:untracked_files_for_uploads)).to be_falsey
     end
+
+    it 'does not block a whole batch because of one bad path' do
+      untracked_files_for_uploads.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/#{project2.full_path}/._7d37bf4c747916390e596744117d5d1a")
+      expect(untracked_files_for_uploads.count).to eq(9)
+      expect(uploads.count).to eq(4)
+
+      subject.perform(1, untracked_files_for_uploads.last.id)
+
+      expect(untracked_files_for_uploads.count).to eq(1)
+      expect(uploads.count).to eq(8)
+    end
+
+    it 'an unparseable path is shown in error output' do
+      bad_path = "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/#{project2.full_path}/._7d37bf4c747916390e596744117d5d1a"
+      untracked_files_for_uploads.create!(path: bad_path)
+
+      expect(Rails.logger).to receive(:error).with(/Error parsing path "#{bad_path}":/)
+
+      subject.perform(1, untracked_files_for_uploads.last.id)
+    end
   end
 
   context 'with no untracked files' do