small upload code refactor

app/controllers/categories_controller.rb

@@ -36,7 +36,7 @@ class CategoriesController < ApplicationController
     guardian.ensure_can_create!(Category)
 
     file = params[:file] || params[:files].first
-    upload = Upload.create_for(current_user.id, file.tempfile, file.original_filename, File.size(file.tempfile))
+    upload = Upload.create_for(current_user.id, file.tempfile, file.original_filename, file.tempfile.size)
     if upload.errors.blank?
       render json: { url: upload.url, width: upload.width, height: upload.height }
     else

app/controllers/uploads_controller.rb

@@ -4,7 +4,7 @@ class UploadsController < ApplicationController
 
   def create
     file = params[:file] || params[:files].first
-    filesize = File.size(file.tempfile)
+    filesize = file.tempfile.size
     upload = Upload.create_for(current_user.id, file.tempfile, file.original_filename, filesize, { content_type: file.content_type })
 
     if upload.errors.empty? && current_user.admin?

app/controllers/user_avatars_controller.rb

@@ -13,7 +13,7 @@ class UserAvatarsController < ApplicationController
       user.create_user_avatar(user_id: user.id) unless user.user_avatar
       user.user_avatar.update_gravatar!
 
-      render json: {upload_id: user.user_avatar.gravatar_upload_id}
+      render json: { upload_id: user.user_avatar.gravatar_upload_id }
     else
       raise Discourse::NotFound
     end

app/models/discourse_single_sign_on.rb

@@ -130,12 +130,12 @@ class DiscourseSingleSignOn < SingleSignOn
       sso_record.external_avatar_url != avatar_url)
 
       begin
-        tempfile = FileHelper.download(avatar_url, 1.megabyte, "sso-avatar", true)
+        tempfile = FileHelper.download(avatar_url, SiteSetting.max_image_size_kb.kilobytes, "sso-avatar", true)
 
         ext = FastImage.type(tempfile).to_s
         tempfile.rewind
 
-        upload = Upload.create_for(user.id, tempfile, "external-avatar." + ext, File.size(tempfile.path), { origin: avatar_url })
+        upload = Upload.create_for(user.id, tempfile, "external-avatar." + ext, tempfile.size, { origin: avatar_url })
         user.uploaded_avatar_id = upload.id
 
         unless user.user_avatar

app/models/optimized_image.rb

@@ -15,59 +15,60 @@ class OptimizedImage < ActiveRecord::Base
       thumbnail = nil
     end
 
+    # return the previous thumbnail if any
+    return thumbnail unless thumbnail.nil?
+
     # create the thumbnail otherwise
-    unless thumbnail
+    external_copy = Discourse.store.download(upload) if Discourse.store.external?
-      external_copy = Discourse.store.download(upload) if Discourse.store.external?
+    original_path = if Discourse.store.external?
-      original_path = if Discourse.store.external?
+      external_copy.try(:path)
-        external_copy.try(:path)
+    else
-      else
+      Discourse.store.path_for(upload)
-        Discourse.store.path_for(upload)
-      end
-
-      if original_path.blank?
-        Rails.logger.error("Could not find file in the store located at url: #{upload.url}")
-      else
-        # create a temp file with the same extension as the original
-        extension = File.extname(original_path)
-        temp_file = Tempfile.new(["discourse-thumbnail", extension])
-        temp_path = temp_file.path
-
-        if extension =~ /\.svg$/i
-          FileUtils.cp(original_path, temp_path)
-          resized = true
-        else
-          resized = resize(original_path, temp_path, width, height, opts[:allow_animation])
-        end
-
-        if resized
-          thumbnail = OptimizedImage.create!(
-            upload_id: upload.id,
-            sha1: Digest::SHA1.file(temp_path).hexdigest,
-            extension: File.extname(temp_path),
-            width: width,
-            height: height,
-            url: "",
-          )
-          # store the optimized image and update its url
-          url = Discourse.store.store_optimized_image(temp_file, thumbnail)
-          if url.present?
-            thumbnail.url = url
-            thumbnail.save
-          else
-            Rails.logger.error("Failed to store avatar #{size} for #{upload.url} from #{source}")
-          end
-        else
-          Rails.logger.error("Failed to create optimized image #{width}x#{height} for #{upload.url}")
-        end
-
-        # close && remove temp file
-        temp_file.close!
-      end
-
-      # make sure we remove the cached copy from external stores
-      external_copy.close! if Discourse.store.external?
     end
 
+    if original_path.blank?
+      Rails.logger.error("Could not find file in the store located at url: #{upload.url}")
+    else
+      # create a temp file with the same extension as the original
+      extension = File.extname(original_path)
+      temp_file = Tempfile.new(["discourse-thumbnail", extension])
+      temp_path = temp_file.path
+
+      if extension =~ /\.svg$/i
+        FileUtils.cp(original_path, temp_path)
+        resized = true
+      else
+        resized = resize(original_path, temp_path, width, height, opts[:allow_animation])
+      end
+
+      if resized
+        thumbnail = OptimizedImage.create!(
+          upload_id: upload.id,
+          sha1: Digest::SHA1.file(temp_path).hexdigest,
+          extension: extension,
+          width: width,
+          height: height,
+          url: "",
+        )
+        # store the optimized image and update its url
+        url = Discourse.store.store_optimized_image(temp_file, thumbnail)
+        if url.present?
+          thumbnail.url = url
+          thumbnail.save
+        else
+          Rails.logger.error("Failed to store avatar #{size} for #{upload.url} from #{source}")
+        end
+      else
+        Rails.logger.error("Failed to create optimized image #{width}x#{height} for #{upload.url}")
+      end
+
+      # close && remove temp file
+      temp_file.close!
+    end
+
+    # make sure we remove the cached copy from external stores
+    external_copy.close! if Discourse.store.external?
+
     thumbnail
   end
 

app/models/upload.rb

@@ -50,56 +50,41 @@ class Upload < ActiveRecord::Base
   #   - content_type
   #   - origin
   def self.create_for(user_id, file, filename, filesize, options = {})
-    # compute the sha
     sha1 = Digest::SHA1.file(file).hexdigest
-    # check if the file has already been uploaded
+
-    upload = Upload.find_by(sha1: sha1)
+    # do we already have that upload?
-    # delete the previously uploaded file if there's been an error
+    upload = find_by(sha1: sha1)
+
+    # make sure the previous upload has not failed
     if upload && upload.url.blank?
       upload.destroy
       upload = nil
     end
-    # create the upload
-    unless upload
-      # initialize a new upload
-      upload = Upload.new(
-        user_id: user_id,
-        original_filename: filename,
-        filesize: filesize,
-        sha1: sha1,
-        url: ""
-      )
-      # trim the origin if any
-      upload.origin = options[:origin][0...1000] if options[:origin]
 
-      # check the size of the upload
+    # return the previous upload if any
-      if FileHelper.is_image?(filename)
+    return upload unless upload.nil?
-        if SiteSetting.max_image_size_kb > 0 && filesize >= SiteSetting.max_image_size_kb.kilobytes
-          upload.errors.add(:base, I18n.t("upload.images.too_large", max_size_kb: SiteSetting.max_image_size_kb))
-        else
-          # deal with width & height for images
-          upload = Upload.resize_image(filename, file, upload)
-        end
-      else
-        if SiteSetting.max_attachment_size_kb > 0 && filesize >= SiteSetting.max_attachment_size_kb.kilobytes
-          upload.errors.add(:base, I18n.t("upload.attachments.too_large", max_size_kb: SiteSetting.max_attachment_size_kb))
-        end
-      end
 
-      # make sure there is no error
+    # create the upload otherwise
-      return upload unless upload.errors.empty?
+    upload = Upload.new
+    upload.user_id           = user_id
+    upload.original_filename = filename
+    upload.filesize          = filesize
+    upload.sha1              = sha1
+    upload.url               = ""
+    upload.origin            = options[:origin][0...1000] if options[:origin]
 
-      # create a db record (so we can use the id)
+    # deal with width & height for images
-      return upload unless upload.save
+    upload = resize_image(filename, file, upload) if FileHelper.is_image?(filename)
 
-      # store the file and update its url
+    return upload unless upload.save
-      url = Discourse.store.store_upload(file, upload, options[:content_type])
+
-      if url.present?
+    # store the file and update its url
-        upload.url = url
+    url = Discourse.store.store_upload(file, upload, options[:content_type])
-        upload.save
+    if url.present?
-      else
+      upload.url = url
-        upload.errors.add(:url, I18n.t("upload.store_failure", { upload_id: upload.id, user_id: user_id }))
+      upload.save
-      end
+    else
+      upload.errors.add(:url, I18n.t("upload.store_failure", { upload_id: upload.id, user_id: user_id }))
     end
 
     # return the uploaded file

app/models/user_avatar.rb

@@ -15,22 +15,20 @@ class UserAvatar < ActiveRecord::Base
 
     self.last_gravatar_download_attempt = Time.new
     gravatar_url = "http://www.gravatar.com/avatar/#{email_hash}.png?s=500&d=404"
-    tempfile = FileHelper.download(gravatar_url, 1.megabyte, "gravatar")
+    tempfile = FileHelper.download(gravatar_url, SiteSetting.max_image_size_kb.kilobytes, "gravatar")
 
-    upload = Upload.create_for(user.id, tempfile, 'gravatar.png', File.size(tempfile.path))
+    upload = Upload.create_for(user.id, tempfile, 'gravatar.png', tempfile.size, { origin: gravatar_url })
 
     if gravatar_upload_id != upload.id
       gravatar_upload.try(:destroy!)
       self.gravatar_upload = upload
       save!
-    else
-      gravatar_upload
     end
   rescue OpenURI::HTTPError
     save!
   rescue SocketError
     # skip saving, we are not connected to the net
-    Rails.logger.warn "Failed to download gravatar, socket error - user id #{ user.id }"
+    Rails.logger.warn "Failed to download gravatar, socket error - user id #{user.id}"
   ensure
     tempfile.close! if tempfile && tempfile.respond_to?(:close!)
   end

lib/avatar_upload_service.rb

@@ -14,9 +14,9 @@ class AvatarUploadService
     case source
     when :url
       tmp = FileHelper.download(file, SiteSetting.max_image_size_kb.kilobytes, "discourse-avatar")
-      [tmp, File.basename(URI.parse(file).path), File.size(tmp)]
+      [tmp, File.basename(URI.parse(file).path), tmp.size]
     when :image
-      [file.tempfile, file.original_filename, File.size(file.tempfile)]
+      [file.tempfile, file.original_filename, file.tempfile.size]
     end
   end
 

lib/email/receiver.rb

@@ -224,7 +224,7 @@ module Email
           # read attachment
           File.open(tmp.path, "w+b") { |f| f.write attachment.body.decoded }
           # create the upload for the user
-          upload = Upload.create_for(user.id, tmp, attachment.filename, File.size(tmp))
+          upload = Upload.create_for(user.id, tmp, attachment.filename, tmp.size)
           if upload && upload.errors.empty?
             # TODO: should use the same code as the client to insert attachments
             raw << "\n#{attachment_markdown(upload)}\n"

script/import_scripts/base.rb

@@ -462,7 +462,7 @@ class ImportScripts::Base
     src.close
     tmp.rewind
 
-    Upload.create_for(user_id, tmp, source_filename, File.size(tmp))
+    Upload.create_for(user_id, tmp, source_filename, tmp.size)
   ensure
     tmp.close rescue nil
     tmp.unlink rescue nil