FIX: don't destroy uploads in queued posts and drafts

app/jobs/scheduled/clean_up_uploads.rb

@@ -5,31 +5,29 @@ class CleanUpUploads < Jobs::Scheduled
     def execute(args)
       return unless SiteSetting.clean_up_uploads?
 
-      ignore_urls = []
-      ignore_urls |= UserProfile.uniq.select(:profile_background).where("profile_background IS NOT NULL AND profile_background != ''").pluck(:profile_background)
-      ignore_urls |= UserProfile.uniq.select(:card_background).where("card_background IS NOT NULL AND card_background != ''").pluck(:card_background)
-      ignore_urls |= Category.uniq.select(:logo_url).where("logo_url IS NOT NULL AND logo_url != ''").pluck(:logo_url)
-      ignore_urls |= Category.uniq.select(:background_url).where("background_url IS NOT NULL AND background_url != ''").pluck(:background_url)
+      ignore_urls  = []
+      ignore_urls |= UserProfile.uniq.where("profile_background IS NOT NULL AND profile_background != ''").pluck(:profile_background)
+      ignore_urls |= UserProfile.uniq.where("card_background IS NOT NULL AND card_background != ''").pluck(:card_background)
+      ignore_urls |= Category.uniq.where("logo_url IS NOT NULL AND logo_url != ''").pluck(:logo_url)
+      ignore_urls |= Category.uniq.where("background_url IS NOT NULL AND background_url != ''").pluck(:background_url)
 
-      ids = []
-      ids |= PostUpload.uniq.select(:upload_id).pluck(:upload_id)
-      ids |= User.uniq.select(:uploaded_avatar_id).where("uploaded_avatar_id IS NOT NULL").pluck(:uploaded_avatar_id)
-      ids |= UserAvatar.uniq.select(:gravatar_upload_id).where("gravatar_upload_id IS NOT NULL").pluck(:gravatar_upload_id)
+      ids  = []
+      ids |= PostUpload.uniq.pluck(:upload_id)
+      ids |= User.uniq.where("uploaded_avatar_id IS NOT NULL").pluck(:uploaded_avatar_id)
+      ids |= UserAvatar.uniq.where("gravatar_upload_id IS NOT NULL").pluck(:gravatar_upload_id)
 
       grace_period = [SiteSetting.clean_orphan_uploads_grace_period_hours, 1].max
 
-      result = Upload.where("created_at < ?", grace_period.hour.ago)
-                     .where("retain_hours IS NULL OR created_at < current_timestamp - interval '1 hour' * retain_hours")
+      result = Upload.where("retain_hours IS NULL OR created_at < current_timestamp - interval '1 hour' * retain_hours")
+      result = result.where("created_at < ?", grace_period.hour.ago)
+      result = result.where("id NOT IN (?)", ids) if !ids.empty?
+      result = result.where("url NOT IN (?)", ignore_urls) if !ignore_urls.empty?
 
-      if !ids.empty?
-        result = result.where("id NOT IN (?)", ids)
+      result.find_each do |upload|
+        next if QueuedPost.where("raw LIKE '%#{upload.sha1}%'").exists?
+        next if Draft.where("data LIKE '%#{upload.sha1}%'").exists?
+        upload.destroy
       end
-
-      if !ignore_urls.empty?
-        result = result.where("url NOT IN (?)", ignore_urls)
-      end
-
-      result.find_each { |upload| upload.destroy }
     end
   end
 end

spec/jobs/clean_up_uploads_spec.rb

@@ -3,11 +3,16 @@
 require_dependency 'jobs/scheduled/clean_up_uploads'
 
 describe Jobs::CleanUpUploads do
+
+  def fabricate_upload
+    Fabricate(:upload, created_at: 2.hours.ago)
+  end
+
   before do
     Upload.destroy_all
     SiteSetting.clean_up_uploads = true
     SiteSetting.clean_orphan_uploads_grace_period_hours = 1
-    @upload = Fabricate(:upload, created_at: 2.hours.ago)
+    @upload = fabricate_upload
   end
 
   it "deletes orphan uploads" do
@@ -19,7 +24,7 @@
   end
 
   it "does not delete profile background uploads" do
-    profile_background_upload = Fabricate(:upload, created_at: 2.hours.ago)
+    profile_background_upload = fabricate_upload
     UserProfile.last.update_attributes!(profile_background: profile_background_upload.url)
 
     Jobs::CleanUpUploads.new.execute(nil)
@@ -29,7 +34,7 @@
   end
 
   it "does not delete card background uploads" do
-    card_background_upload = Fabricate(:upload, created_at: 2.hours.ago)
+    card_background_upload = fabricate_upload
     UserProfile.last.update_attributes!(card_background: card_background_upload.url)
 
     Jobs::CleanUpUploads.new.execute(nil)
@@ -39,7 +44,7 @@
   end
 
   it "does not delete category logo uploads" do
-    category_logo_upload = Fabricate(:upload, created_at: 2.hours.ago)
+    category_logo_upload = fabricate_upload
     category = Fabricate(:category, logo_url: category_logo_upload.url)
 
     Jobs::CleanUpUploads.new.execute(nil)
@@ -49,7 +54,7 @@
   end
 
   it "does not delete category background url uploads" do
-    category_background_url = Fabricate(:upload, created_at: 2.hours.ago)
+    category_background_url = fabricate_upload
     category = Fabricate(:category, background_url: category_background_url.url)
 
     Jobs::CleanUpUploads.new.execute(nil)
@@ -59,7 +64,7 @@
   end
 
   it "does not delete post uploads" do
-    upload = Fabricate(:upload, created_at: 2.hours.ago)
+    upload = fabricate_upload
     post = Fabricate(:post, uploads: [upload])
 
     Jobs::CleanUpUploads.new.execute(nil)
@@ -69,7 +74,7 @@
   end
 
   it "does not delete user uploaded avatar" do
-    upload = Fabricate(:upload, created_at: 2.hours.ago)
+    upload = fabricate_upload
     user = Fabricate(:user, uploaded_avatar: upload)
 
     Jobs::CleanUpUploads.new.execute(nil)
@@ -79,12 +84,40 @@
   end
 
   it "does not delete user gravatar" do
-    upload = Fabricate(:upload, created_at: 2.hours.ago)
+    upload = fabricate_upload
     user = Fabricate(:user, user_avatar: Fabricate(:user_avatar, gravatar_upload: upload))
 
     Jobs::CleanUpUploads.new.execute(nil)
 
     expect(Upload.find_by(id: @upload.id)).to eq(nil)
     expect(Upload.find_by(id: upload.id)).to eq(upload)
   end
+
+  it "does not delete uploads in a queued post" do
+    upload = fabricate_upload
+
+    QueuedPost.create(
+      queue: "uploads",
+      state: QueuedPost.states[:new],
+      user_id: Fabricate(:user).id,
+      raw: upload.sha1,
+      post_options: {}
+    )
+
+    Jobs::CleanUpUploads.new.execute(nil)
+
+    expect(Upload.find_by(id: @upload.id)).to eq(nil)
+    expect(Upload.find_by(id: upload.id)).to eq(upload)
+  end
+
+  it "does not delete uploads in a draft" do
+    upload = fabricate_upload
+    Draft.set(Fabricate(:user), "test", 0, upload.sha1)
+
+    Jobs::CleanUpUploads.new.execute(nil)
+
+    expect(Upload.find_by(id: @upload.id)).to eq(nil)
+    expect(Upload.find_by(id: upload.id)).to eq(upload)
+  end
+
 end