Don’t recreate deleted uploads

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)

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

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!