FEATURE: correctly store width and height on uploads

Previously we used width and height for thumbnails, new code ensures

1. We auto correct width and height
2. We added extra columns for thumbnail_width and height, this is determined
 by actual upload and no longer passed in as a side effect
3. Optimized Image now stores filesize which can be used for analysis, decisions

Also

- fixes Android image manifest as a side effect
- fixes issue where a thumbnail generated that is smaller than the upload is no longer used

app/assets/javascripts/discourse/lib/utilities.js.es6

@@ -441,8 +441,9 @@ export function uploadLocation(url) {
 export function getUploadMarkdown(upload) {
   if (isAnImage(upload.original_filename)) {
     const name = imageNameFromFileName(upload.original_filename);
-    return `![${name}|${upload.width}x${upload.height}](${upload.short_url ||
+    return `![${name}|${upload.thumbnail_width}x${
-      upload.url})`;
+      upload.thumbnail_height
+    }](${upload.short_url || upload.url})`;
   } else if (
     !Discourse.SiteSettings.prevent_anons_from_downloading_files &&
     /\.(mov|mp4|webm|ogv|mp3|ogg|wav|m4a)$/i.test(upload.original_filename)

app/jobs/onceoff/clear_width_and_height.rb

@@ -0,0 +1,15 @@
+module Jobs
+  class ClearWidthAndHeight < Jobs::Onceoff
+    def execute_onceoff(args)
+      # we have to clear all old uploads cause
+      # we could have old versions of height / width
+      # this column used to store thumbnail size instead of
+      # actual size
+      DB.exec(<<~SQL)
+        UPDATE uploads
+        SET width = null, height = null
+        WHERE width IS NOT NULL OR height IS NOT NULL
+      SQL
+    end
+  end
+end

app/models/optimized_image.rb

@@ -81,6 +81,7 @@ class OptimizedImage < ActiveRecord::Base
         end
 
         if resized
+
           thumbnail = OptimizedImage.create!(
             upload_id: upload.id,
             sha1: Upload.generate_digest(temp_path),
@@ -88,6 +89,7 @@ class OptimizedImage < ActiveRecord::Base
             width: width,
             height: height,
             url: "",
+            filesize: File.size(temp_path)
           )
           # store the optimized image and update its url
           File.open(temp_path) do |file|
@@ -123,6 +125,32 @@ class OptimizedImage < ActiveRecord::Base
     !(url =~ /^(https?:)?\/\//)
   end
 
+  def calculate_filesize
+    path =
+      if local?
+        Discourse.store.path_for(self)
+      else
+        Discourse.store.download(self).path
+      end
+    File.size(path)
+  end
+
+  def filesize
+    if size = read_attribute(:filesize)
+      size
+    else
+      # we may have a bad optimized image so just skip for now
+      # and do not break here
+      size = calculate_filesize rescue nil
+
+      write_attribute(:filesize, size)
+      if !new_record?
+        update_columns(filesize: size)
+      end
+      size
+    end
+  end
+
   def self.safe_path?(path)
     # this matches instructions which call #to_s
     path = path.to_s

app/models/upload.rb

@@ -24,7 +24,7 @@ class Upload < ActiveRecord::Base
 
   validates_with ::Validators::UploadValidator
 
-  def thumbnail(width = self.width, height = self.height)
+  def thumbnail(width = self.thumbnail_width, height = self.thumbnail_height)
     optimized_images.find_by(width: width, height: height)
   end
 
@@ -41,10 +41,6 @@ class Upload < ActiveRecord::Base
     }
 
     if get_optimized_image(width, height, opts)
-      # TODO: this code is not right, we may have multiple
-      # thumbs
-      self.width = width
-      self.height = height
       save(validate: false)
     end
   end
@@ -103,6 +99,51 @@ class Upload < ActiveRecord::Base
     "upload://#{Base62.encode(sha1.hex)}.#{extension}"
   end
 
+  def local?
+    !(url =~ /^(https?:)?\/\//)
+  end
+
+  def fix_dimensions!
+    return if !FileHelper.is_image?("image.#{extension}")
+
+    path =
+      if local?
+        Discourse.store.path_for(self)
+      else
+        Discourse.store.download(self).path
+      end
+
+    self.width, self.height = size = FastImage.new(path).size
+    self.thumbnail_width, self.thumbnail_height = ImageSizer.resize(*size)
+    nil
+  end
+
+  # on demand image size calculation, this allows us to null out image sizes
+  # and still handle as needed
+  def get_dimension(key)
+    if v = read_attribute(key)
+      return v
+    end
+    fix_dimensions!
+    read_attribute(key)
+  end
+
+  def width
+    get_dimension(:width)
+  end
+
+  def height
+    get_dimension(:height)
+  end
+
+  def thumbnail_width
+    get_dimension(:thumbnail_width)
+  end
+
+  def thumbnail_height
+    get_dimension(:thumbnail_height)
+  end
+
   def self.sha1_from_short_url(url)
     if url =~ /(upload:\/\/)?([a-zA-Z0-9]+)(\..*)?/
       sha1 = Base62.decode($2).to_s(16)

app/serializers/upload_serializer.rb

@@ -5,6 +5,8 @@ class UploadSerializer < ApplicationSerializer
              :filesize,
              :width,
              :height,
+             :thumbnail_width,
+             :thumbnail_height,
              :extension,
              :short_url,
              :retain_hours

db/migrate/20180827053514_add_filesize_to_optimized_images.rb

@@ -0,0 +1,7 @@
+class AddFilesizeToOptimizedImages < ActiveRecord::Migration[5.2]
+  def change
+    add_column :optimized_images, :filesize, :integer
+    add_column :uploads, :thumbnail_width, :integer
+    add_column :uploads, :thumbnail_height, :integer
+  end
+end

lib/cooked_post_processor.rb

@@ -327,7 +327,17 @@ class CookedPostProcessor
 
     # replace the image by its thumbnail
     w, h = img["width"].to_i, img["height"].to_i
-    img["src"] = upload.thumbnail(w, h).url if upload && upload.has_thumbnail?(w, h)
+
+    if upload
+      thumbnail = upload.thumbnail(w, h)
+
+      img["src"] =
+        if thumbnail && thumbnail.filesize.to_i < upload.filesize
+          upload.thumbnail(w, h).url
+        else
+          upload.url
+        end
+    end
 
     # then, some overlay informations
     meta = create_node("div", "meta")

lib/upload_creator.rb

@@ -106,7 +106,8 @@ class UploadCreator
       @upload.extension         = image_type || File.extname(@filename)[1..10]
 
       if is_image
-        @upload.width, @upload.height = ImageSizer.resize(*@image_info.size)
+        @upload.thumbnail_width, @upload.thumbnail_height = ImageSizer.resize(*@image_info.size)
+        @upload.width, @upload.height = @image_info.size
       end
 
       @upload.for_private_message = true if @opts[:for_private_message]

spec/models/optimized_image_spec.rb

@@ -54,22 +54,35 @@ describe OptimizedImage do
       end
 
       it 'should work correctly' do
-        tmp_path = "/tmp/resized.png"
 
-        begin
+        file = File.open("#{Rails.root}/spec/fixtures/images/resized.png")
-          OptimizedImage.resize(
+        upload = UploadCreator.new(file, "test.bin").create_for(-1)
-            "#{Rails.root}/spec/fixtures/images/logo.png",
-            tmp_path,
-            5,
-            5
-          )
 
-          expect(File.read(tmp_path)).to eq(
+        expect(upload.filesize).to eq(199)
-            File.read("#{Rails.root}/spec/fixtures/images/resized.png")
+
-          )
+        expect(upload.width).to eq(5)
-        ensure
+        expect(upload.height).to eq(5)
-          File.delete(tmp_path) if File.exists?(tmp_path)
+
-        end
+        upload.create_thumbnail!(10, 10)
+        thumb = upload.thumbnail(10, 10)
+
+        expect(thumb.width).to eq(10)
+        expect(thumb.height).to eq(10)
+
+        # very image magic specific so fudge here
+        expect(thumb.filesize).to be > 200
+
+        # this size is based off original upload
+        # it is the size we render, by default, in the post
+        expect(upload.thumbnail_width).to eq(5)
+        expect(upload.thumbnail_height).to eq(5)
+
+        # lets ensure we can rebuild the filesize
+        thumb.update_columns(filesize: nil)
+        thumb = OptimizedImage.find(thumb.id)
+
+        # attempts to auto correct
+        expect(thumb.filesize).to be > 200
       end
 
       describe 'when an svg with a href is masked as a png' do

spec/models/upload_spec.rb

@@ -46,6 +46,22 @@ describe Upload do
 
   end
 
+  it "can reconstruct dimensions on demand" 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)
+
+    upload = Upload.find(upload.id)
+
+    expect(upload.width).to eq(64250)
+    expect(upload.height).to eq(64250)
+
+    upload.update_columns(width: nil, height: nil, thumbnail_width: nil, thumbnail_height: nil)
+
+    expect(upload.thumbnail_width).to eq(500)
+    expect(upload.thumbnail_height).to eq(500)
+  end
+
   it "extracts file extension" do
     created_upload = UploadCreator.new(image, image_filename).create_for(user_id)
     expect(created_upload.extension).to eq("png")

test/javascripts/lib/utilities-test.js.es6

@@ -161,8 +161,8 @@ var testUploadMarkdown = function(filename) {
   return getUploadMarkdown({
     original_filename: filename,
     filesize: 42,
-    width: 100,
+    thumbnail_width: 100,
-    height: 200,
+    thumbnail_height: 200,
     url: "/uploads/123/abcdef.ext"
   });
 };