From 473ddfb453d820f1a32fb48477e17ba45bdbd2f0 Mon Sep 17 00:00:00 2001
From: Michael Kozono <mkozono@gmail.com>
Date: Fri, 24 Nov 2017 00:49:04 -0800
Subject: =?UTF-8?q?Don=E2=80=99t=20recreate=20deleted=20uploads?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

---
 .../populate_untracked_uploads.rb                  | 40 +++++++++++++++++++---
 .../populate_untracked_uploads_spec.rb             | 14 ++++----
 spec/migrations/track_untracked_uploads_spec.rb    |  9 +++++
 3 files changed, 51 insertions(+), 12 deletions(-)

diff --git a/lib/gitlab/background_migration/populate_untracked_uploads.rb b/lib/gitlab/background_migration/populate_untracked_uploads.rb
index 9fb5b3f77a2..50ec2260c60 100644
--- a/lib/gitlab/background_migration/populate_untracked_uploads.rb
+++ b/lib/gitlab/background_migration/populate_untracked_uploads.rb
@@ -81,13 +81,13 @@ module Gitlab
         end
 
         def model_id
+          return @model_id if defined?(@model_id)
+
           matchd = path_relative_to_upload_dir.match(matching_pattern_map[:pattern])
 
           # If something is captured (matchd[1] is not nil), it is a model_id
-          return matchd[1] if matchd[1]
-
           # Only the FileUploader pattern will not match an ID
-          file_uploader_model_id
+          @model_id = matchd[1] ? matchd[1].to_i : file_uploader_model_id
         end
 
         def file_size
@@ -122,7 +122,9 @@ module Gitlab
 
           full_path = matchd[1]
           project = Project.find_by_full_path(full_path)
-          project.id.to_s
+          return nil unless project
+
+          project.id
         end
 
         def absolute_path
@@ -165,8 +167,36 @@ module Gitlab
         end
       end
 
+      # There are files on disk that are not in the uploads table because their
+      # model was deleted, and we don't delete the files on disk.
       def filter_deleted_models(files)
-        files # TODO
+        ids = deleted_model_ids(files)
+
+        files.reject do |file|
+          ids[file.model_type].include?(file.model_id)
+        end
+      end
+
+      def deleted_model_ids(files)
+        ids = {
+          'Appearance' => [],
+          'Namespace' => [],
+          'Note' => [],
+          'Project' => [],
+          'User' => []
+        }
+
+        # group model IDs by model type
+        files.each do |file|
+          ids[file.model_type] << file.model_id
+        end
+
+        ids.each do |model_type, model_ids|
+          found_ids = Object.const_get(model_type).where(id: model_ids.uniq).pluck(:id)
+          ids[model_type] = ids[model_type] - found_ids # replace with deleted ids
+        end
+
+        ids
       end
 
       def insert(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 623725bffca..85da8ac5b1e 100644
--- a/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb
+++ b/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb
@@ -392,37 +392,37 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do
 
     context 'for an appearance logo file path' do
       it 'returns the ID as a string' do
-        assert_model_id('/-/system/appearance/logo/1/some_logo.jpg', '1')
+        assert_model_id('/-/system/appearance/logo/1/some_logo.jpg', 1)
       end
     end
 
     context 'for an appearance header_logo file path' do
       it 'returns the ID as a string' do
-        assert_model_id('/-/system/appearance/header_logo/1/some_logo.jpg', '1')
+        assert_model_id('/-/system/appearance/header_logo/1/some_logo.jpg', 1)
       end
     end
 
     context 'for a pre-Markdown Note attachment file path' do
       it 'returns the ID as a string' do
-        assert_model_id('/-/system/note/attachment/1234/some_attachment.pdf', '1234')
+        assert_model_id('/-/system/note/attachment/1234/some_attachment.pdf', 1234)
       end
     end
 
     context 'for a user avatar file path' do
       it 'returns the ID as a string' do
-        assert_model_id('/-/system/user/avatar/1234/avatar.jpg', '1234')
+        assert_model_id('/-/system/user/avatar/1234/avatar.jpg', 1234)
       end
     end
 
     context 'for a group avatar file path' do
       it 'returns the ID as a string' do
-        assert_model_id('/-/system/group/avatar/1234/avatar.jpg', '1234')
+        assert_model_id('/-/system/group/avatar/1234/avatar.jpg', 1234)
       end
     end
 
     context 'for a project avatar file path' do
       it 'returns the ID as a string' do
-        assert_model_id('/-/system/project/avatar/1234/avatar.jpg', '1234')
+        assert_model_id('/-/system/project/avatar/1234/avatar.jpg', 1234)
       end
     end
 
@@ -430,7 +430,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do
       it 'returns the ID as a string' do
         project = create(:project)
 
-        assert_model_id("/#{project.full_path}/#{SecureRandom.hex}/Some file.jpg", project.id.to_s)
+        assert_model_id("/#{project.full_path}/#{SecureRandom.hex}/Some file.jpg", project.id)
       end
     end
   end
diff --git a/spec/migrations/track_untracked_uploads_spec.rb b/spec/migrations/track_untracked_uploads_spec.rb
index 01bfe26744f..9fa586ff177 100644
--- a/spec/migrations/track_untracked_uploads_spec.rb
+++ b/spec/migrations/track_untracked_uploads_spec.rb
@@ -75,6 +75,15 @@ describe TrackUntrackedUploads, :migration, :sidekiq do
       expect(project1.uploads.where(uploader: 'FileUploader').first.attributes).to include(@project1_markdown_attributes)
     end
 
+    it 'ignores uploads for deleted models' do
+      user2.destroy
+      project2.destroy
+
+      expect do
+        migrate!
+      end.to change { uploads.count }.from(4).to(5)
+    end
+
     it 'the temporary table untracked_files_for_uploads no longer exists' do
       migrate!
 
-- 
cgit v1.2.1