FIX: raise exception when getting dimensions of missing image

FIX: raise exception when getting dimensions of missing image

  • follow-up on 0eacd45ab15cbd20ed9f444fd447886a7fc6dccb
From f8e6a378584c36ed668d5e57453a3d1662d0242f Mon Sep 17 00:00:00 2001
From: Penar Musaraj <pmusaraj@gmail.com>
Date: Mon, 3 Dec 2018 10:19:49 -0500
Subject: [PATCH] FIX: raise exception when getting dimensions of missing image

- follow-up on 0eacd45ab15cbd20ed9f444fd447886a7fc6dccb

diff --git a/app/models/upload.rb b/app/models/upload.rb
index bf3d415..3505379 100644
--- a/app/models/upload.rb
+++ b/app/models/upload.rb
@@ -127,8 +127,12 @@ class Upload < ActiveRecord::Base
         Discourse.store.download(self).path
       end
 
-    self.width, self.height = size = FastImage.new(path).size
-    self.thumbnail_width, self.thumbnail_height = ImageSizer.resize(*size)
+    begin
+      self.width, self.height = size = FastImage.new(path, raise_on_failure: true).size
+      self.thumbnail_width, self.thumbnail_height = ImageSizer.resize(*size)
+    rescue => e
+      Discourse.warn_exception(e, message: "Error getting image dimensions")
+    end
     nil
   end
 
diff --git a/lib/image_sizer.rb b/lib/image_sizer.rb
index 6e16464..ee368ce 100644
--- a/lib/image_sizer.rb
+++ b/lib/image_sizer.rb
@@ -1,7 +1,7 @@
 module ImageSizer
 
   # Resize an image to the aspect ratio we want
-  def self.resize(width = nil, height = nil, opts = {})
+  def self.resize(width, height, opts = {})
     return if width.blank? || height.blank?
 
     max_width = (opts[:max_width] || SiteSetting.max_image_width).to_f
diff --git a/spec/components/image_sizer_spec.rb b/spec/components/image_sizer_spec.rb
index f3c0960..b013267 100644
--- a/spec/components/image_sizer_spec.rb
+++ b/spec/components/image_sizer_spec.rb
@@ -24,10 +24,6 @@ describe ImageSizer do
     expect(ImageSizer.resize('100', '101')).to eq([100, 101])
   end
 
-  it 'returns nil if no width or height are provided' do
-    expect(ImageSizer.resize).to eq(nil)
-  end
-
   describe 'when larger than the maximum width' do
 
     before do
diff --git a/spec/models/upload_spec.rb b/spec/models/upload_spec.rb
index f3b88c01..6e64ee8 100644
--- a/spec/models/upload_spec.rb
+++ b/spec/models/upload_spec.rb
@@ -62,6 +62,16 @@ describe Upload do
     expect(upload.thumbnail_height).to eq(500)
   end
 
+  it "dimension calculation returns nil on missing image" do
+    upload = UploadCreator.new(huge_image, "image.png").create_for(user_id)
+    upload.update_columns(width: nil, height: nil, thumbnail_width: nil, thumbnail_height: nil)
+
+    missing_url = "wrong_folder#{upload.url}"
+    upload.update_columns(url: missing_url)
+    expect(upload.thumbnail_height).to eq(nil)
+    expect(upload.thumbnail_width).to eq(nil)
+  end
+
   it "extracts file extension" do
     created_upload = UploadCreator.new(image, image_filename).create_for(user_id)
     expect(created_upload.extension).to eq("png")

GitHub

1 Like

This is definitely an improvement cause at least we are tracking the problem.

But… Why create the upload at all in this case? Can the exception just leak out and the upload not be created? Essentially we are creating an invalid upload here.

1 Like

AFAICT, we are not creating an upload here, FastImage is looking up the image and getting its size. But when image is missing, it was failing without raising an exception.

2 Likes

I see, this is just a safety guard for cases where we are missing sizes. No probs.

Note in general @tgxworld is not a fan of blanket rescues, but in this case I think it is OK cause we are reporting it right away. The edge cases of out of memory etc are just too rare to worry about.

2 Likes

Echoing what you said, if we log the exception when we do a blanket rescue I am OK with that because it isn’t being silently ignored. I’m not a fan when we do rescue nil or any similar pattern.

3 Likes