Skip to content
Jump to bottom

Automated Test: large-image-processing #314

Closed
wants to merge 1 commit into from
+20 −12
Closed

Automated Test: large-image-processing #314

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions app/assets/javascripts/discourse/lib/utilities.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ Discourse.Utilities = {

// check file size
var fileSizeKB = file.size / 1024;
var maxSizeKB = Discourse.SiteSettings['max_' + type + '_size_kb'];
var maxSizeKB = 10 * 1024; // 10MB
if (fileSizeKB > maxSizeKB) {
bootbox.alert(I18n.t('post.errors.file_too_large', { max_size_kb: maxSizeKB }));
return false;
Expand Down Expand Up @@ -243,7 +243,7 @@ Discourse.Utilities = {

// entity too large, usually returned from the web server
case 413:
var maxSizeKB = Discourse.SiteSettings.max_image_size_kb;
var maxSizeKB = 10 * 1024; // 10 MB
bootbox.alert(I18n.t('post.errors.file_too_large', { max_size_kb: maxSizeKB }));
return;

Expand Down
11 changes: 10 additions & 1 deletion app/controllers/uploads_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,14 +52,23 @@ def create_upload(type, file, url)
begin
# API can provide a URL
if file.nil? && url.present? && is_api?
tempfile = FileHelper.download(url, SiteSetting.max_image_size_kb.kilobytes, "discourse-upload-#{type}") rescue nil
tempfile = FileHelper.download(url, 10.megabytes, "discourse-upload-#{type}") rescue nil
filename = File.basename(URI.parse(url).path)
else
tempfile = file.tempfile
filename = file.original_filename
content_type = file.content_type
end

# allow users to upload large images that will be automatically reduced to allowed size
if tempfile && tempfile.size > 0 && SiteSetting.max_image_size_kb > 0 && FileHelper.is_image?(filename)
attempt = 5
while attempt > 0 && tempfile.size > SiteSetting.max_image_size_kb.kilobytes
OptimizedImage.downsize(tempfile.path, tempfile.path, "80%", allow_animation: SiteSetting.allow_animated_thumbnails)
attempt -= 1
end
end
Comment on lines +63 to +70
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Downsizing loop ignores downsize return value; loop may spin on a failed conversion.

OptimizedImage.downsize returns false when the conversion command fails, but the return value is never checked. On failure the file is unchanged (or possibly corrupted), so subsequent iterations repeat the same failing command. Break on failure to avoid wasted work and to surface the error.

Additionally, tempfile.size on Line 66 may return a stale value after an external process (convert) rewrites the file — Ruby's IO#size uses fstat on the original file descriptor. Use File.size(tempfile.path) to stat the path directly.

🐛 Proposed fix 
       # allow users to upload large images that will be automatically reduced to allowed size
-      if tempfile && tempfile.size > 0 && SiteSetting.max_image_size_kb > 0 && FileHelper.is_image?(filename)
+      if tempfile && File.size(tempfile.path) > 0 && SiteSetting.max_image_size_kb > 0 && FileHelper.is_image?(filename)
         attempt = 5
-        while attempt > 0 && tempfile.size > SiteSetting.max_image_size_kb.kilobytes
-          OptimizedImage.downsize(tempfile.path, tempfile.path, "80%", allow_animation: SiteSetting.allow_animated_thumbnails)
+        while attempt > 0 && File.size(tempfile.path) > SiteSetting.max_image_size_kb.kilobytes
+          unless OptimizedImage.downsize(tempfile.path, tempfile.path, "80%", allow_animation: SiteSetting.allow_animated_thumbnails)
+            Rails.logger.error("Failed to downsize image: #{filename}")
+            break
+          end
           attempt -= 1
         end
       end
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# allow users to upload large images that will be automatically reduced to allowed size
if tempfile && tempfile.size > 0 && SiteSetting.max_image_size_kb > 0 && FileHelper.is_image?(filename)
attempt = 5
while attempt > 0 && tempfile.size > SiteSetting.max_image_size_kb.kilobytes
OptimizedImage.downsize(tempfile.path, tempfile.path, "80%", allow_animation: SiteSetting.allow_animated_thumbnails)
attempt -= 1
end
end
# allow users to upload large images that will be automatically reduced to allowed size
if tempfile && File.size(tempfile.path) > 0 && SiteSetting.max_image_size_kb > 0 && FileHelper.is_image?(filename)
attempt = 5
while attempt > 0 && File.size(tempfile.path) > SiteSetting.max_image_size_kb.kilobytes
unless OptimizedImage.downsize(tempfile.path, tempfile.path, "80%", allow_animation: SiteSetting.allow_animated_thumbnails)
Rails.logger.error("Failed to downsize image: #{filename}")
break
end
attempt -= 1
end
end
🤖 Prompt for AI Agents 
In `@app/controllers/uploads_controller.rb` around lines 63 - 70, The downsizing
loop ignores OptimizedImage.downsize's boolean return and uses tempfile.size
(which may be stale); update the while loop in the uploads controller to check
the return value of OptimizedImage.downsize and break on false to avoid infinite
retries, and replace usages of tempfile.size with File.size(tempfile.path) when
comparing to SiteSetting.max_image_size_kb.kilobytes so the size is re-statted
after the external conversion; keep the existing attempt retry counter but stop
looping immediately on a failed downsize to surface the error.


upload = Upload.create_for(current_user.id, tempfile, filename, tempfile.size, content_type: content_type, image_type: type)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

tempfile.size passed to Upload.create_for may be stale after downsizing.

After the downsizing loop, tempfile.size is passed as the file size to Upload.create_for. If the file was rewritten by the convert subprocess, the IO-level size may not reflect the new on-disk size. Use File.size(tempfile.path) here as well for consistency.

🐛 Proposed fix 
-      upload = Upload.create_for(current_user.id, tempfile, filename, tempfile.size, content_type: content_type, image_type: type)
+      upload = Upload.create_for(current_user.id, tempfile, filename, File.size(tempfile.path), content_type: content_type, image_type: type)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
upload = Upload.create_for(current_user.id, tempfile, filename, tempfile.size, content_type: content_type, image_type: type)
upload = Upload.create_for(current_user.id, tempfile, filename, File.size(tempfile.path), content_type: content_type, image_type: type)
🤖 Prompt for AI Agents 
In `@app/controllers/uploads_controller.rb` at line 72, The code passes
tempfile.size into Upload.create_for which can be stale if the downsizing loop
rewrote the tempfile via the convert subprocess; after the downsizing completes,
recompute the actual on-disk size and pass that to Upload.create_for (e.g. call
File.size(tempfile.path) or equivalent) instead of using tempfile.size so
Upload.create_for receives the correct file size.


if upload.errors.empty? && current_user.admin?
Expand Down
17 changes: 8 additions & 9 deletions app/models/optimized_image.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,25 +139,24 @@ def self.downsize_instructions_animated(from, to, dimensions, opts={})
end

def self.resize(from, to, width, height, opts={})
optimize("resize", from, to, width, height, opts)
optimize("resize", from, to, "#{width}x#{height}", opts)
end

def self.downsize(from, to, max_width, max_height, opts={})
optimize("downsize", from, to, max_width, max_height, opts)
optimize("downsize", from, to, "#{max_width}x#{max_height}", opts)
end

def self.optimize(operation, from, to, width, height, opts={})
dim = dimensions(width, height)
def self.downsize(from, to, dimensions, opts={})
optimize("downsize", from, to, dimensions, opts)
end
Comment on lines 145 to +151
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

cat -n app/models/optimized_image.rb | sed -n '140,160p'

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 955

🏁 Script executed:

rg -n --type=ruby 'downsize\(' -A 1 -B 1

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 1110

Duplicate downsize definition — the 4-arg overload on line 145 is dead code and will break existing callers.

Ruby does not support method overloading. The second def self.downsize (line 149) silently replaces the first (line 145), making the 4-arg version unreachable. However, this breaks the existing caller in app/jobs/regular/resize_emoji.rb:14, which uses the old signature: OptimizedImage.downsize(path, path, 100, 100, opts).

At runtime, the 4-arg call will mis-bind to the 3-arg signature: the two integers (100, 100) will land in dimensions and opts respectively, causing incorrect behavior or runtime errors when passed to optimize.

Remove the old 4-arg definition on lines 145–147. Update the resize_emoji caller to pass dimensions as a string: OptimizedImage.downsize(path, path, "100x100", opts).

🐛 Proposed fix — remove the dead overload and update the caller

app/models/optimized_image.rb:

-  def self.downsize(from, to, max_width, max_height, opts={})
-    optimize("downsize", from, to, "#{max_width}x#{max_height}", opts)
-  end
-
   def self.downsize(from, to, dimensions, opts={})
     optimize("downsize", from, to, dimensions, opts)
   end

app/jobs/regular/resize_emoji.rb:

-      OptimizedImage.downsize(path, path, 100, 100, opts)
+      OptimizedImage.downsize(path, path, "100x100", opts)
🧰 Tools
🪛 RuboCop (1.84.0)

[warning] 149-149: Method OptimizedImage.downsize is defined at both app/models/optimized_image.rb:145 and app/models/optimized_image.rb:149.

(Lint/DuplicateMethods)

🤖 Prompt for AI Agents 
In `@app/models/optimized_image.rb` around lines 145 - 151, Remove the duplicate
4-argument definition of self.downsize in the OptimizedImage class (the overload
that accepts from, to, max_width, max_height) so only the canonical signature
self.downsize(from, to, dimensions, opts={}) remains, and update the call site
in the ResizeEmoji job that calls OptimizedImage.downsize(path, path, 100, 100,
opts) to pass the dimensions as a single string like "100x100"
(OptimizedImage.downsize(path, path, "100x100", opts)) so arguments bind
correctly to dimensions and opts.


def self.optimize(operation, from, to, dimensions, opts={})
method_name = "#{operation}_instructions"
method_name += "_animated" if !!opts[:allow_animation] && from =~ /\.GIF$/i
instructions = self.send(method_name.to_sym, from, to, dim, opts)
instructions = self.send(method_name.to_sym, from, to, dimensions, opts)
convert_with(instructions, to)
end

def self.dimensions(width, height)
"#{width}x#{height}"
end

def self.convert_with(instructions, to)
`#{instructions.join(" ")} &> /dev/null`
return false if $?.exitstatus != 0
Expand Down