DEV: Improve script/downsize_uploads.rb (#13508)

* Only shrink images that are used in Posts and no other models
* Don't save the upload if the size is the same

app/jobs/scheduled/clean_up_uploads.rb

@@ -29,36 +29,9 @@ module Jobs
         .where("uploads.retain_hours IS NULL OR uploads.created_at < current_timestamp - interval '1 hour' * uploads.retain_hours")
         .where("uploads.created_at < ?", grace_period.hour.ago)
         .where("uploads.access_control_post_id IS NULL")
-        .joins(<<~SQL)
-          LEFT JOIN site_settings ss
-          ON NULLIF(ss.value, '')::integer = uploads.id
-          AND ss.data_type = #{SiteSettings::TypeSupervisor.types[:upload].to_i}
-        SQL
         .joins("LEFT JOIN post_uploads pu ON pu.upload_id = uploads.id")
-        .joins("LEFT JOIN users u ON u.uploaded_avatar_id = uploads.id")
-        .joins("LEFT JOIN user_avatars ua ON ua.gravatar_upload_id = uploads.id OR ua.custom_upload_id = uploads.id")
-        .joins("LEFT JOIN user_profiles up ON up.profile_background_upload_id = uploads.id OR up.card_background_upload_id = uploads.id")
-        .joins("LEFT JOIN categories c ON c.uploaded_logo_id = uploads.id OR c.uploaded_background_id = uploads.id")
-        .joins("LEFT JOIN custom_emojis ce ON ce.upload_id = uploads.id")
-        .joins("LEFT JOIN theme_fields tf ON tf.upload_id = uploads.id")
-        .joins("LEFT JOIN user_exports ue ON ue.upload_id = uploads.id")
-        .joins("LEFT JOIN groups g ON g.flair_upload_id = uploads.id")
-        .joins("LEFT JOIN badges b ON b.image_upload_id = uploads.id")
         .where("pu.upload_id IS NULL")
-        .where("u.uploaded_avatar_id IS NULL")
+        .with_no_non_post_relations
-        .where("ua.gravatar_upload_id IS NULL AND ua.custom_upload_id IS NULL")
-        .where("up.profile_background_upload_id IS NULL AND up.card_background_upload_id IS NULL")
-        .where("c.uploaded_logo_id IS NULL AND c.uploaded_background_id IS NULL")
-        .where("ce.upload_id IS NULL")
-        .where("tf.upload_id IS NULL")
-        .where("ue.upload_id IS NULL")
-        .where("g.flair_upload_id IS NULL")
-        .where("b.image_upload_id IS NULL")
-        .where("ss.value IS NULL")
-
-      if SiteSetting.selectable_avatars.present?
-        result = result.where.not(id: SiteSetting.selectable_avatars.map(&:id))
-      end
 
       result.find_each do |upload|
         if upload.sha1.present?

app/models/upload.rb

@@ -64,6 +64,40 @@ class Upload < ActiveRecord::Base
     )
   end
 
+  def self.with_no_non_post_relations
+    scope = self
+      .joins(<<~SQL)
+        LEFT JOIN site_settings ss
+        ON NULLIF(ss.value, '')::integer = uploads.id
+        AND ss.data_type = #{SiteSettings::TypeSupervisor.types[:upload].to_i}
+      SQL
+      .where("ss.value IS NULL")
+      .joins("LEFT JOIN users u ON u.uploaded_avatar_id = uploads.id")
+      .where("u.uploaded_avatar_id IS NULL")
+      .joins("LEFT JOIN user_avatars ua ON ua.gravatar_upload_id = uploads.id OR ua.custom_upload_id = uploads.id")
+      .where("ua.gravatar_upload_id IS NULL AND ua.custom_upload_id IS NULL")
+      .joins("LEFT JOIN user_profiles up ON up.profile_background_upload_id = uploads.id OR up.card_background_upload_id = uploads.id")
+      .where("up.profile_background_upload_id IS NULL AND up.card_background_upload_id IS NULL")
+      .joins("LEFT JOIN categories c ON c.uploaded_logo_id = uploads.id OR c.uploaded_background_id = uploads.id")
+      .where("c.uploaded_logo_id IS NULL AND c.uploaded_background_id IS NULL")
+      .joins("LEFT JOIN custom_emojis ce ON ce.upload_id = uploads.id")
+      .where("ce.upload_id IS NULL")
+      .joins("LEFT JOIN theme_fields tf ON tf.upload_id = uploads.id")
+      .where("tf.upload_id IS NULL")
+      .joins("LEFT JOIN user_exports ue ON ue.upload_id = uploads.id")
+      .where("ue.upload_id IS NULL")
+      .joins("LEFT JOIN groups g ON g.flair_upload_id = uploads.id")
+      .where("g.flair_upload_id IS NULL")
+      .joins("LEFT JOIN badges b ON b.image_upload_id = uploads.id")
+      .where("b.image_upload_id IS NULL")
+
+    if SiteSetting.selectable_avatars.present?
+      scope = scope.where.not(id: SiteSetting.selectable_avatars.map(&:id))
+    end
+
+    scope
+  end
+
   def to_s
     self.url
   end

lib/shrink_uploaded_image.rb

@@ -12,6 +12,20 @@ class ShrinkUploadedImage
   end
 
   def perform
+    # Neither #dup or #clone provide a complete copy
+    original_upload = Upload.find_by(id: upload.id)
+    unless original_upload
+      log "Upload is missing"
+      return false
+    end
+
+    posts = Post.unscoped.joins(:post_uploads).where(post_uploads: { upload_id: original_upload.id }).uniq.sort_by(&:created_at)
+
+    if posts.empty?
+      log "Upload not used in any posts"
+      return false
+    end
+
     OptimizedImage.downsize(path, path, "#{@max_pixels}@", filename: upload.original_filename)
     sha1 = Upload.generate_digest(path)
 
@@ -27,13 +41,6 @@ class ShrinkUploadedImage
       return false
     end
 
-    # Neither #dup or #clone provide a complete copy
-    original_upload = Upload.find_by(id: upload.id)
-    unless original_upload
-      log "Upload is missing"
-      return false
-    end
-
     ww, hh = ImageSizer.resize(w, h)
 
     # A different upload record that matches the sha1 of the downsized image
@@ -49,7 +56,7 @@ class ShrinkUploadedImage
       filesize: File.size(path)
     }
 
-    if upload.filesize > upload.filesize_was
+    if upload.filesize >= upload.filesize_was
       log "No filesize reduction"
       return false
     end
@@ -70,7 +77,6 @@ class ShrinkUploadedImage
     log "(an existing upload)" if existing_upload
 
     success = true
-    posts = Post.unscoped.joins(:post_uploads).where(post_uploads: { upload_id: original_upload.id }).uniq.sort_by(&:created_at)
 
     posts.each do |post|
       transform_post(post, original_upload, upload)
@@ -99,32 +105,6 @@ class ShrinkUploadedImage
       log "#{Discourse.base_url}/p/#{post.id}"
     end
 
-    if posts.empty?
-      log "Upload not used in any posts"
-
-      if User.where(uploaded_avatar_id: original_upload.id).exists?
-        log "Used as a User avatar"
-      elsif UserAvatar.where(gravatar_upload_id: original_upload.id).exists?
-        log "Used as a UserAvatar gravatar"
-      elsif UserAvatar.where(custom_upload_id: original_upload.id).exists?
-        log "Used as a UserAvatar custom upload"
-      elsif UserProfile.where(profile_background_upload_id: original_upload.id).exists?
-        log "Used as a UserProfile profile background"
-      elsif UserProfile.where(card_background_upload_id: original_upload.id).exists?
-        log "Used as a UserProfile card background"
-      elsif Category.where(uploaded_logo_id: original_upload.id).exists?
-        log "Used as a Category logo"
-      elsif Category.where(uploaded_background_id: original_upload.id).exists?
-        log "Used as a Category background"
-      elsif CustomEmoji.where(upload_id: original_upload.id).exists?
-        log "Used as a CustomEmoji"
-      elsif ThemeField.where(upload_id: original_upload.id).exists?
-        log "Used as a ThemeField"
-      else
-        success = false
-      end
-    end
-
     unless success
       if @interactive
         print "Press any key to continue with the upload"
@@ -157,16 +137,6 @@ class ShrinkUploadedImage
         PostUpload.where(upload_id: original_upload.id).update_all(upload_id: upload.id)
       rescue ActiveRecord::RecordNotUnique, PG::UniqueViolation
       end
-
-      User.where(uploaded_avatar_id: original_upload.id).update_all(uploaded_avatar_id: upload.id)
-      UserAvatar.where(gravatar_upload_id: original_upload.id).update_all(gravatar_upload_id: upload.id)
-      UserAvatar.where(custom_upload_id: original_upload.id).update_all(custom_upload_id: upload.id)
-      UserProfile.where(profile_background_upload_id: original_upload.id).update_all(profile_background_upload_id: upload.id)
-      UserProfile.where(card_background_upload_id: original_upload.id).update_all(card_background_upload_id: upload.id)
-      Category.where(uploaded_logo_id: original_upload.id).update_all(uploaded_logo_id: upload.id)
-      Category.where(uploaded_background_id: original_upload.id).update_all(uploaded_background_id: upload.id)
-      CustomEmoji.where(upload_id: original_upload.id).update_all(upload_id: upload.id)
-      ThemeField.where(upload_id: original_upload.id).update_all(upload_id: upload.id)
     else
       upload.optimized_images.each(&:destroy!)
     end

script/downsize_uploads.rb

@@ -35,7 +35,11 @@ def process_uploads
   dimensions_count = 0
   downsized_count = 0
 
-  scope = Upload.by_users.where("LOWER(extension) IN ('jpg', 'jpeg', 'gif', 'png')")
+  scope = Upload
+    .by_users
+    .with_no_non_post_relations
+    .where("LOWER(extension) IN ('jpg', 'jpeg', 'gif', 'png')")
+
   scope = scope.where(<<-SQL, MAX_IMAGE_PIXELS)
     COALESCE(width, 0) = 0 OR
     COALESCE(height, 0) = 0 OR

spec/lib/shrink_uploaded_image_spec.rb

@@ -66,6 +66,18 @@ describe ShrinkUploadedImage do
 
       expect(result).to be(false)
     end
+
+    it "returns false when the upload is not used in any posts" do
+      Fabricate(:user, uploaded_avatar: upload)
+
+      result = ShrinkUploadedImage.new(
+        upload: upload,
+        path: Discourse.store.path_for(upload),
+        max_pixels: 10_000
+      ).perform
+
+      expect(result).to be(false)
+    end
   end
 
   context "when S3 uploads are enabled" do

spec/models/upload_spec.rb

@@ -3,9 +3,7 @@
 require 'rails_helper'
 
 describe Upload do
-
   let(:upload) { build(:upload) }
-
   let(:user_id) { 1 }
 
   let(:image_filename) { "logo.png" }
@@ -20,8 +18,34 @@ describe Upload do
   let(:attachment_path) { __FILE__ }
   let(:attachment) { File.new(attachment_path) }
 
-  context ".create_thumbnail!" do
+  describe '.with_no_non_post_relations' do
+    it "does not find non-post related uploads" do
+      post_upload = Fabricate(:upload)
+      post = Fabricate(:post, raw: "<img src='#{post_upload.url}'>")
+      post.link_post_uploads
 
+      badge_upload = Fabricate(:upload)
+      Fabricate(:badge, image_upload: badge_upload)
+
+      avatar_upload = Fabricate(:upload)
+      Fabricate(:user, uploaded_avatar: avatar_upload)
+
+      site_setting_upload = Fabricate(:upload)
+      SiteSetting.create!(
+        name: "logo",
+        data_type: SiteSettings::TypeSupervisor.types[:upload],
+        value: site_setting_upload.id
+      )
+
+      upload_ids = Upload
+        .by_users
+        .with_no_non_post_relations
+        .pluck(:id)
+      expect(upload_ids).to eq([post_upload.id])
+    end
+  end
+
+  context ".create_thumbnail!" do
     it "does not create a thumbnail when disabled" do
       SiteSetting.create_thumbnails = false
       OptimizedImage.expects(:create_for).never