Skip to content
Jump to bottom

Automated Test: large-image-processing #322

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

Automated Test: large-image-processing #322

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
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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check other download size limits and site setting usage
rg -n "FileHelper\.download\(" -C2 app lib
echo "---"
rg -n "max_image_size_kb|max_attachment_size_kb" -C2 app lib

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 8172

Use site settings for the initial download limit instead of hardcoded 10 MB.

Line 55 uses a hardcoded 10.megabytes limit, but the rest of the codebase consistently uses SiteSetting.max_image_size_kb.kilobytes for download size constraints (and the resize loop on lines 66-68 also respects the site setting). This creates a mismatch: if max_image_size_kb is lower, the initial download overshoots the limit; if higher, the download is unnecessarily capped. Use [SiteSetting.max_image_size_kb, SiteSetting.max_attachment_size_kb].max.kilobytes to align with the pattern elsewhere.

🧰 Tools
🪛 RuboCop (1.84.0)

[convention] 55-55: Avoid using rescue in its modifier form.

(Style/RescueModifier)

🤖 Prompt for AI Agents 
In `@app/controllers/uploads_controller.rb` at line 55, Replace the hardcoded
10.megabytes used when downloading into tempfile with the site-setting-based
limit to match the rest of the code: use [SiteSetting.max_image_size_kb,
SiteSetting.max_attachment_size_kb].max.kilobytes for the size argument in the
FileHelper.download call in uploads_controller.rb (the line that assigns
tempfile) while preserving the existing rescue nil behavior.

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

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

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

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)
Comment on lines 141 to +156
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

🧩 Analysis chain

🏁 Script executed:

# First, verify the file and the code at the specified lines
cat -n app/models/optimized_image.rb | sed -n '141,156p'

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 835

🏁 Script executed:

# Search for all downsize call sites in the codebase
rg -n "downsize\(" -C2 app lib

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 1427

🏁 Script executed:

# Also check for OptimizedImage.downsize specifically
rg -n "OptimizedImage\.downsize\(" -C2

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 861

🏁 Script executed:

# Let me verify the resize_emoji.rb context and opts usage
cat -n app/jobs/regular/resize_emoji.rb | head -20

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 578

🏁 Script executed:

# Also check if there are any other downsize calls I might have missed
rg -n "\.downsize\(" --type rb

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 114

🏁 Script executed:

# Search for all downsize calls without type filter
rg "downsize\(" --color=never

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 516

Duplicate downsize definitions break existing callers (Ruby has no overloads).

The second downsize definition at line 149 completely overrides the first at line 145. The resize_emoji job calls downsize with the old signature (path, path, 100, 100, opts) (5 positional args), which will fail with the current second definition that expects only 4 parameters: (from, to, dimensions, opts={}). This causes an ArgumentError: wrong number of arguments (given 5, expected 2..4) at runtime.

The uploads_controller call uses the new signature (tempfile.path, tempfile.path, "80%", allow_animation: ...) and will continue to work.

To fix this, either:

  • Keep only the second definition and update the resize_emoji job to pass dimensions as a string: OptimizedImage.downsize(path, path, "100x100", opts)
  • Implement a single method that supports both signatures (see original suggestion)
🧰 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 141 - 156, The duplicate downsize
definitions must be consolidated so callers like resize_emoji (which passes
three positional args for width/height) and uploads_controller (which passes a
dimensions string) both work: remove the duplicated method and implement a
single OptimizedImage.downsize that accepts both signatures (e.g., parameters
from, to, then either (max_width, max_height, opts={}) or (dimensions_string,
opts={}), detect whether the third arg is a String or a Numeric, normalize to a
dimensions string like "WxH", merge opts, and then call optimize(operation,
from, to, dimensions, opts); update references to method names used in optimize
(e.g., optimize and any *_instructions methods remain unchanged).

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