From c59142986844ce029803bb4a22cc5c8e4053f7c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Mon, 1 Aug 2016 18:35:57 +0200 Subject: [PATCH] FIX: don't destroy uploads in queued posts and drafts --- app/jobs/scheduled/clean_up_uploads.rb | 36 +++++++++---------- spec/jobs/clean_up_uploads_spec.rb | 49 +++++++++++++++++++++----- 2 files changed, 58 insertions(+), 27 deletions(-) diff --git a/app/jobs/scheduled/clean_up_uploads.rb b/app/jobs/scheduled/clean_up_uploads.rb index 0ec3c96b5..b4b55ae5c 100644 --- a/app/jobs/scheduled/clean_up_uploads.rb +++ b/app/jobs/scheduled/clean_up_uploads.rb @@ -5,31 +5,29 @@ module Jobs 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 diff --git a/spec/jobs/clean_up_uploads_spec.rb b/spec/jobs/clean_up_uploads_spec.rb index ca0fd7390..9b7f22d99 100644 --- a/spec/jobs/clean_up_uploads_spec.rb +++ b/spec/jobs/clean_up_uploads_spec.rb @@ -3,11 +3,16 @@ require 'rails_helper' 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 @@ describe Jobs::CleanUpUploads do 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 @@ describe Jobs::CleanUpUploads do 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 @@ describe Jobs::CleanUpUploads do 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 @@ describe Jobs::CleanUpUploads do 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 @@ describe Jobs::CleanUpUploads do 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 @@ describe Jobs::CleanUpUploads do 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,7 +84,7 @@ describe Jobs::CleanUpUploads do 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) @@ -87,4 +92,32 @@ describe Jobs::CleanUpUploads do 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