Merge pull request #1275 from ZogStriP/enable-thumbnailing-on-s3

Enable thumbnailing on s3

Gemfile.lock

@@ -168,7 +168,7 @@ GEM
       handlebars-source (= 1.0.12)
     erubis (2.7.0)
     eventmachine (1.0.3)
-    excon (0.20.1)
+    excon (0.25.3)
     execjs (1.4.0)
       multi_json (~> 1.0)
     fabrication (2.6.5)
@@ -181,15 +181,15 @@ GEM
     fast_xs (0.8.0)
     fastimage (1.3.0)
     ffi (1.8.1)
-    fog (1.10.1)
+    fog (1.14.0)
       builder
-      excon (~> 0.20)
+      excon (~> 0.25.0)
       formatador (~> 0.2.0)
       mime-types
       multi_json (~> 1.0)
       net-scp (~> 1.1)
       net-ssh (>= 2.1.3)
-      nokogiri (~> 1.5.0)
+      nokogiri (~> 1.5)
       ruby-hmac
     formatador (0.2.4)
     fspath (2.0.4)
@@ -247,9 +247,9 @@ GEM
     multi_json (1.7.7)
     multipart-post (1.2.0)
     mustache (0.99.4)
-    net-scp (1.1.0)
+    net-scp (1.1.2)
       net-ssh (>= 2.6.5)
-    net-ssh (2.6.7)
+    net-ssh (2.6.8)
     nokogiri (1.5.9)
     oauth (0.4.7)
     oauth2 (0.8.1)

app/models/optimized_image.rb

@@ -8,62 +8,46 @@ class OptimizedImage < ActiveRecord::Base
 
     @image_sorcery_loaded ||= require "image_sorcery"
 
-    original_path = "#{Rails.root}/public#{upload.url}"
+    external_copy = Discourse.store.download(upload) if Discourse.store.external?
+    original_path = if Discourse.store.external?
+      external_copy.path
+    else
+      Discourse.store.path_for(upload)
+    end
+
     # create a temp file with the same extension as the original
-    temp_file = Tempfile.new(["discourse", File.extname(original_path)])
+    temp_file = Tempfile.new(["discourse-thumbnail", File.extname(original_path)])
     temp_path = temp_file.path
 
     if ImageSorcery.new(original_path).convert(temp_path, resize: "#{width}x#{height}")
-      thumbnail = OptimizedImage.new({
+      thumbnail = OptimizedImage.create!(
         upload_id: upload.id,
         sha1: Digest::SHA1.file(temp_path).hexdigest,
         extension: File.extname(temp_path),
         width: width,
-        height: height
+        height: height,
-      })
+        url: "",
-      # make sure the directory exists
+      )
-      FileUtils.mkdir_p Pathname.new(thumbnail.path).dirname
+      # store the optimized image and update its url
-      # move the temp file to the right location
+      thumbnail.url = Discourse.store.store_optimized_image(temp_file, thumbnail)
-      File.open(thumbnail.path, "wb") do |f|
+      thumbnail.save
-        f.write temp_file.read
-      end
     end
 
     # close && remove temp file
-    temp_file.close
+    temp_file.close!
-    temp_file.unlink
+    # make sure we remove the cached copy from external stores
+    external_copy.close! if Discourse.store.external?
 
     thumbnail
   end
 
   def destroy
     OptimizedImage.transaction do
-      remove_file
+      Discourse.store.remove_file(url)
       super
     end
   end
 
-  def remove_file
-    File.delete path
-  rescue Errno::ENOENT
-  end
-
-  def url
-    "#{LocalStore.base_url}/#{optimized_path}/#{filename}"
-  end
-
-  def path
-    "#{LocalStore.base_path}/#{optimized_path}/#{filename}"
-  end
-
-  def optimized_path
-    "_optimized/#{sha1[0..2]}/#{sha1[3..5]}"
-  end
-
-  def filename
-    "#{sha1[6..16]}_#{width}x#{height}#{extension}"
-  end
-
 end
 
 # == Schema Information

app/models/post.rb

@@ -1,7 +1,5 @@
 require_dependency 'jobs'
 require_dependency 'pretty_text'
-require_dependency 'local_store'
-require_dependency 's3_store'
 require_dependency 'rate_limiter'
 require_dependency 'post_revisor'
 require_dependency 'enum'

app/models/post_analyzer.rb

@@ -35,13 +35,9 @@ class PostAnalyzer
   # How many attachments are present in the post
   def attachment_count
     return 0 unless @raw.present?
-
+    attachments = cooked_document.css("a.attachment[href^=\"#{Discourse.store.absolute_base_url}\"]")
-    if SiteSetting.enable_s3_uploads?
+    attachments += cooked_document.css("a.attachment[href^=\"#{Discourse.store.relative_base_url}\"]") if Discourse.store.internal?
-      cooked_document.css("a.attachment[href^=\"#{S3Store.base_url}\"]")
+    attachments.count
-    else
-      cooked_document.css("a.attachment[href^=\"#{LocalStore.directory}\"]") +
-      cooked_document.css("a.attachment[href^=\"#{LocalStore.base_url}\"]")
-    end.count
   end
 
   def raw_mentions

app/models/topic_link.rb

@@ -103,8 +103,8 @@ class TopicLink < ActiveRecord::Base
           topic_id = nil
           post_number = nil
 
-          if Upload.has_been_uploaded?(url)
+          if Discourse.store.has_been_uploaded?(url)
-            internal = !Upload.is_on_s3?(url)
+            internal = Discourse.store.internal?
           elsif parsed.host == Discourse.current_hostname || !parsed.host
             internal = true
 

app/models/upload.rb

@@ -2,8 +2,6 @@ require 'digest/sha1'
 require 'image_sizer'
 require 'tempfile'
 require 'pathname'
-require_dependency 's3_store'
-require_dependency 'local_store'
 
 class Upload < ActiveRecord::Base
   belongs_to :user
@@ -20,17 +18,12 @@ class Upload < ActiveRecord::Base
     optimized_images.where(width: width, height: height).first
   end
 
-  def thumbnail_url
-    thumbnail.url if has_thumbnail?
-  end
-
   def has_thumbnail?
     thumbnail.present?
   end
 
   def create_thumbnail!
     return unless SiteSetting.create_thumbnails?
-    return if SiteSetting.enable_s3_uploads?
     return if has_thumbnail?
     thumbnail = OptimizedImage.create_for(self, width, height)
     optimized_images << thumbnail if thumbnail
@@ -38,7 +31,7 @@ class Upload < ActiveRecord::Base
 
   def destroy
     Upload.transaction do
-      Upload.remove_file url
+      Discourse.store.remove_file(url)
       super
     end
   end
@@ -58,7 +51,7 @@ class Upload < ActiveRecord::Base
         file.rewind
       end
       # create a db record (so we can use the id)
-      upload = Upload.create!({
+      upload = Upload.create!(
         user_id: user_id,
         original_filename: file.original_filename,
         filesize: filesize,
@@ -66,9 +59,9 @@ class Upload < ActiveRecord::Base
         url: "",
         width: width,
         height: height,
-      })
+      )
       # store the file and update its url
-      upload.url = Upload.store_file(file, sha1, upload.id)
+      upload.url = Discourse.store.store_upload(file, upload)
       # save the url
       upload.save
     end
@@ -76,36 +69,11 @@ class Upload < ActiveRecord::Base
     upload
   end
 
-  def self.store_file(file, sha1, upload_id)
-    return S3Store.store_file(file, sha1, upload_id) if SiteSetting.enable_s3_uploads?
-    return LocalStore.store_file(file, sha1, upload_id)
-  end
-
-  def self.remove_file(url)
-    return S3Store.remove_file(url) if SiteSetting.enable_s3_uploads?
-    return LocalStore.remove_file(url)
-  end
-
-  def self.has_been_uploaded?(url)
-    is_relative?(url) || is_local?(url) || is_on_s3?(url)
-  end
-
-  def self.is_relative?(url)
-    url.start_with?(LocalStore.directory)
-  end
-
-  def self.is_local?(url)
-    !SiteSetting.enable_s3_uploads? && url.start_with?(LocalStore.base_url)
-  end
-
-  def self.is_on_s3?(url)
-    SiteSetting.enable_s3_uploads? && url.start_with?(S3Store.base_url)
-  end
-
   def self.get_from_url(url)
     # we store relative urls, so we need to remove any host/cdn
-    url = url.gsub(/^#{LocalStore.asset_host}/i, "") if LocalStore.asset_host.present?
+    asset_host = Rails.configuration.action_controller.asset_host
-    Upload.where(url: url).first if has_been_uploaded?(url)
+    url = url.gsub(/^#{asset_host}/i, "") if asset_host.present?
+    Upload.where(url: url).first if Discourse.store.has_been_uploaded?(url)
   end
 
 end

db/migrate/20130728172550_add_url_to_optimized_images.rb

@@ -0,0 +1,22 @@
+class AddUrlToOptimizedImages < ActiveRecord::Migration
+  def up
+    # add a nullable url column
+    add_column :optimized_images, :url, :string
+    # compute the url for existing images
+    execute "UPDATE optimized_images
+             SET url = substring(u.url from '^\/uploads\/[^/]+\/')
+                    || '_optimized/'
+                    || substring(oi.sha1 for 3) || '/'
+                    || substring(oi.sha1 from 4 for 3) || '/'
+                    || substring(oi.sha1 from 7 for 11) || oi.extension
+            FROM optimized_images oi
+            JOIN uploads u ON u.id = oi.upload_id
+            WHERE optimized_images.id = oi.id;"
+    # change the column to be non nullable
+    change_column :optimized_images, :url, :string, null: false
+  end
+
+  def down
+    remove_column :optimized_images, :url
+  end
+end

lib/cooked_post_processor.rb

@@ -77,7 +77,7 @@ class CookedPostProcessor
   end
 
   def relative_to_absolute(src)
-    if src =~ /\A\/[^\/]/
+    if src =~ /^\/[^\/]/
       Discourse.base_url_no_prefix + src
     else
       src
@@ -98,7 +98,7 @@ class CookedPostProcessor
 
   def associate_to_post(upload)
     return if PostUpload.where(post_id: @post.id, upload_id: upload.id).count > 0
-    PostUpload.create({ post_id: @post.id, upload_id: upload.id })
+    PostUpload.create(post_id: @post.id, upload_id: upload.id)
   rescue ActiveRecord::RecordNotUnique
     # do not care if it's already associated
   end
@@ -155,7 +155,7 @@ class CookedPostProcessor
     a.add_child(img)
 
     # replace the image by its thumbnail
-    img['src'] = upload.thumbnail_url if upload && upload.has_thumbnail?
+    img['src'] = relative_to_absolute(upload.thumbnail.url) if upload && upload.has_thumbnail?
 
     # then, some overlay informations
     meta = Nokogiri::XML::Node.new("div", @doc)
@@ -206,12 +206,13 @@ class CookedPostProcessor
   end
 
   def get_size(url)
-    # make sure s3 urls have a scheme (otherwise, FastImage will fail)
+    uri = url
-    url = "http:" + url if Upload.is_on_s3?(url)
+    # make sure urls have a scheme (otherwise, FastImage will fail)
-    return unless is_valid_image_uri?(url)
+    uri = (SiteSetting.use_ssl? ? "https:" : "http:") + url if url.start_with?("//")
+    return unless is_valid_image_uri?(uri)
     # we can *always* crawl our own images
-    return unless SiteSetting.crawl_images? || Upload.has_been_uploaded?(url)
+    return unless SiteSetting.crawl_images? || Discourse.store.has_been_uploaded?(url)
-    @size_cache[url] ||= FastImage.size(url)
+    @size_cache[url] ||= FastImage.size(uri)
   rescue Zlib::BufError # FastImage.size raises BufError for some gifs
   end
 
@@ -222,14 +223,9 @@ class CookedPostProcessor
   end
 
   def attachments
-    if SiteSetting.enable_s3_uploads?
+    attachments = @doc.css("a.attachment[href^=\"#{Discourse.store.absolute_base_url}\"]")
-      @doc.css("a.attachment[href^=\"#{S3Store.base_url}\"]")
+    attachments += @doc.css("a.attachment[href^=\"#{Discourse.store.relative_base_url}\"]") if Discourse.store.internal?
-    else
+    attachments
-      # local uploads are identified using a relative uri
-      @doc.css("a.attachment[href^=\"#{LocalStore.directory}\"]") +
-      # when cdn is enabled, we have the whole url
-      @doc.css("a.attachment[href^=\"#{LocalStore.base_url}\"]")
-    end
   end
 
   def dirty?

lib/discourse.rb

@@ -105,7 +105,7 @@ module Discourse
 
   def self.git_version
     return $git_version if $git_version
-    f = Rails.root.to_s + "/config/version"
+    f = Rails.root.to_s + "/lib/version"
     require f if File.exists?("#{f}.rb")
 
     begin
@@ -122,6 +122,16 @@ module Discourse
     user
   end
 
+  def self.store
+    if SiteSetting.enable_s3_uploads?
+      @s3_store_loaded ||= require 'file_store/s3_store'
+      S3Store.new
+    else
+      @local_store_loaded ||= require 'file_store/local_store'
+      LocalStore.new
+    end
+  end
+
 private
 
   def self.maintenance_mode_key

lib/file_store/local_store.rb

@@ -0,0 +1,93 @@
+class LocalStore
+
+  def store_upload(file, upload)
+    unique_sha1 = Digest::SHA1.hexdigest("#{Time.now.to_s}#{file.original_filename}")[0,16]
+    extension = File.extname(file.original_filename)
+    clean_name = "#{unique_sha1}#{extension}"
+    path = "#{relative_base_url}/#{upload.id}/#{clean_name}"
+    # copy the file to the right location
+    copy_file(file, "#{public_dir}#{path}")
+    # url
+    Discourse.base_uri + path
+  end
+
+  def store_optimized_image(file, optimized_image)
+    # 1234567890ABCDEF_100x200.jpg
+    filename = [
+      optimized_image.sha1[6..16],
+      "_#{optimized_image.width}x#{optimized_image.height}",
+      optimized_image.extension,
+    ].join
+    # <rails>/public/uploads/site/_optimized/123/456/<filename>
+    path = File.join(
+      relative_base_url,
+      "_optimized",
+      optimized_image.sha1[0..2],
+      optimized_image.sha1[3..5],
+      filename
+    )
+    # copy the file to the right location
+    copy_file(file, "#{public_dir}#{path}")
+    # url
+    Discourse.base_uri + path
+  end
+
+  def remove_file(url)
+    File.delete("#{public_dir}#{url}") if has_been_uploaded?(url)
+  rescue Errno::ENOENT
+    # don't care if the file isn't there
+  end
+
+  def has_been_uploaded?(url)
+    is_relative?(url) || is_local?(url)
+  end
+
+  def absolute_base_url
+    url = asset_host.present? ? asset_host : Discourse.base_url_no_prefix
+    "#{url}#{relative_base_url}"
+  end
+
+  def relative_base_url
+    "/uploads/#{RailsMultisite::ConnectionManagement.current_db}"
+  end
+
+  def external?
+    !internal?
+  end
+
+  def internal?
+    true
+  end
+
+  def path_for(upload)
+    "#{public_dir}#{upload.url}"
+  end
+
+  private
+
+  def copy_file(file, path)
+    FileUtils.mkdir_p Pathname.new(path).dirname
+    # move the file to the right location
+    # not using cause mv, cause permissions are no good on move
+    File.open(path, "wb") do |f|
+      f.write(file.read)
+    end
+  end
+
+  def is_relative?(url)
+    url.start_with?(relative_base_url)
+  end
+
+  def is_local?(url)
+    url.start_with?(absolute_base_url)
+  end
+
+  def public_dir
+    "#{Rails.root}/public"
+  end
+
+  def asset_host
+    Rails.configuration.action_controller.asset_host
+  end
+
+end

lib/file_store/s3_store.rb

@@ -0,0 +1,119 @@
+require 'digest/sha1'
+require 'open-uri'
+
+class S3Store
+
+  def store_upload(file, upload)
+    extension = File.extname(file.original_filename)
+    remote_filename = "#{upload.id}#{upload.sha1}#{extension}"
+
+    # if this fails, it will throw an exception
+    upload(file.tempfile, remote_filename, file.content_type)
+
+    # returns the url of the uploaded file
+    "#{absolute_base_url}/#{remote_filename}"
+  end
+
+  def store_optimized_image(file, optimized_image)
+    extension = File.extname(file.path)
+    remote_filename = [
+      optimized_image.id,
+      optimized_image.sha1,
+      "_#{optimized_image.width}x#{optimized_image.height}",
+      extension
+    ].join
+
+    # if this fails, it will throw an exception
+    upload(file, remote_filename)
+
+    # returns the url of the uploaded file
+    "#{absolute_base_url}/#{remote_filename}"
+  end
+
+  def remove_file(url)
+    check_missing_site_settings
+    return unless has_been_uploaded?(url)
+    name = File.basename(url)
+    remove(name)
+  end
+
+  def has_been_uploaded?(url)
+    url.start_with?(absolute_base_url)
+  end
+
+  def absolute_base_url
+    "//#{s3_bucket}.s3.amazonaws.com"
+  end
+
+  def external?
+    true
+  end
+
+  def internal?
+    !external?
+  end
+
+  def download(upload)
+    temp_file = Tempfile.new(["discourse-s3", File.extname(upload.original_filename)])
+    url = (SiteSetting.use_ssl? ? "https:" : "http:") + upload.url
+
+    File.open(temp_file.path, "wb") do |f|
+      f.write open(url, "rb", read_timeout: 20).read
+    end
+
+    temp_file
+  end
+
+  private
+
+  def s3_bucket
+    SiteSetting.s3_upload_bucket.downcase
+  end
+
+  def check_missing_site_settings
+    raise Discourse::SiteSettingMissing.new("s3_upload_bucket")     if SiteSetting.s3_upload_bucket.blank?
+    raise Discourse::SiteSettingMissing.new("s3_access_key_id")     if SiteSetting.s3_access_key_id.blank?
+    raise Discourse::SiteSettingMissing.new("s3_secret_access_key") if SiteSetting.s3_secret_access_key.blank?
+  end
+
+  def get_or_create_directory(name)
+    check_missing_site_settings
+
+    @fog_loaded ||= require 'fog'
+
+    fog = Fog::Storage.new generate_options
+
+    directory = fog.directories.get(name)
+    directory = fog.directories.create(key: name) unless directory
+    directory
+  end
+
+  def generate_options
+    options = {
+      provider: 'AWS',
+      aws_access_key_id: SiteSetting.s3_access_key_id,
+      aws_secret_access_key: SiteSetting.s3_secret_access_key,
+    }
+    options[:region] = SiteSetting.s3_region unless SiteSetting.s3_region.empty?
+    options
+  end
+
+  def upload(file, name, content_type=nil)
+    args = {
+      key: name,
+      public: true,
+      body: file,
+    }
+    args[:content_type] = content_type if content_type
+    directory.files.create(args)
+  end
+
+  def remove(name)
+    directory.files.destroy(key: name)
+  end
+
+  def directory
+    get_or_create_directory(s3_bucket)
+  end
+
+end

lib/image_optimizer.rb

@@ -1,96 +0,0 @@
-#
-# This class is used to download and optimize images.
-#
-
-require 'image_sorcery'
-require 'digest/sha1'
-require 'open-uri'
-
-class ImageOptimizer
-  attr_accessor :url
-
-  # url is a url of an image ex:
-  # 'http://site.com/image.png'
-  # '/uploads/site/image.png'
-  def initialize(url)
-    @url = url
-    # make sure directories exists
-    FileUtils.mkdir_p downloads_dir
-    FileUtils.mkdir_p optimized_dir
-  end
-
-  # return the path of an optimized image,
-  #  if already cached return cached, else download and cache
-  #   at the original size.
-  # if size is specified return a resized image
-  # if height or width are nil maintain aspect ratio
-  #
-  # Optimised image is the "most efficient" storage for an image
-  #  at the basic level it runs through image_optim https://github.com/toy/image_optim
-  #  it also has a failsafe that converts jpg to png or the opposite. if jpg size is 1.5*
-  #  as efficient as png it flips formats.
-  def optimized_image_url (width = nil, height = nil)
-    begin
-      unless has_been_uploaded?
-        return @url unless SiteSetting.crawl_images?
-        # download the file if it hasn't been cached yet
-        download! unless File.exists?(cached_path)
-      end
-
-      # resize the image using Image Magick
-      result = ImageSorcery.new(cached_path).convert(optimized_path, resize: "#{width}x#{height}")
-      return optimized_url if result
-      @url
-    rescue
-      @url
-    end
-  end
-
-private
-
-  def public_dir
-    @public_dir ||= "#{Rails.root}/public"
-  end
-
-  def downloads_dir
-    @downloads_dir ||= "#{public_dir}/downloads/#{RailsMultisite::ConnectionManagement.current_db}"
-  end
-
-  def optimized_dir
-    @optimized_dir ||= "#{public_dir}/images/#{RailsMultisite::ConnectionManagement.current_db}"
-  end
-
-  def has_been_uploaded?
-    @url.start_with?(Discourse.base_url_no_prefix)
-  end
-
-  def cached_path
-    @cached_path ||= if has_been_uploaded?
-      "#{public_dir}#{@url[Discourse.base_url_no_prefix.length..-1]}"
-    else
-      "#{downloads_dir}/#{file_name(@url)}"
-    end
-  end
-
-  def optimized_path
-    @optimized_path ||= "#{optimized_dir}/#{file_name(cached_path)}"
-  end
-
-  def file_name (uri)
-    image_info = FastImage.new(uri)
-    name = Digest::SHA1.hexdigest(uri)[0,16]
-    name << ".#{image_info.type}"
-    name
-  end
-
-  def download!
-    File.open(cached_path, "wb") do |f|
-      f.write open(@url, "rb", read_timeout: 20).read
-    end
-  end
-
-  def optimized_url
-    @optimized_url ||= Discourse.base_url_no_prefix + "/images/#{RailsMultisite::ConnectionManagement.current_db}/#{file_name(cached_path)}"
-  end
-
-end

lib/local_store.rb

@@ -1,47 +0,0 @@
-module LocalStore
-
-  def self.store_file(file, sha1, upload_id)
-    unique_sha1 = Digest::SHA1.hexdigest("#{Time.now.to_s}#{file.original_filename}")[0,16]
-    extension = File.extname(file.original_filename)
-    clean_name = "#{unique_sha1}#{extension}"
-    url_root = "#{directory}/#{upload_id}"
-    path = "#{Rails.root}/public#{url_root}"
-
-    FileUtils.mkdir_p path
-
-    # not using cause mv, cause permissions are no good on move
-    File.open("#{path}/#{clean_name}", "wb") do |f|
-      f.write File.read(file.tempfile)
-    end
-
-    # url
-    Discourse::base_uri + "#{url_root}/#{clean_name}"
-  end
-
-  def self.remove_file(url)
-    File.delete("#{Rails.root}/public#{url}")
-  rescue Errno::ENOENT
-  end
-
-  def self.uploaded_regex
-    /\/uploads\/#{RailsMultisite::ConnectionManagement.current_db}\/(?<upload_id>\d+)\/[0-9a-f]{16}\.(png|jpg|jpeg|gif|tif|tiff|bmp)/
-  end
-
-  def self.base_url
-    url = asset_host.present? ? asset_host : Discourse.base_url_no_prefix
-    "#{url}#{directory}"
-  end
-
-  def self.base_path
-    "#{Rails.root}/public#{directory}"
-  end
-
-  def self.directory
-    "/uploads/#{RailsMultisite::ConnectionManagement.current_db}"
-  end
-
-  def self.asset_host
-    Rails.configuration.action_controller.asset_host
-  end
-
-end

lib/s3_store.rb

@@ -1,70 +0,0 @@
-module S3Store
-
-  def self.store_file(file, sha1, upload_id)
-    S3Store.check_missing_site_settings
-
-    directory = S3Store.get_or_create_directory(SiteSetting.s3_upload_bucket)
-    extension = File.extname(file.original_filename)
-    remote_filename = "#{upload_id}#{sha1}#{extension}"
-
-    # if this fails, it will throw an exception
-    file = S3Store.upload(file, remote_filename, directory)
-    "#{S3Store.base_url}/#{remote_filename}"
-  end
-
-  def self.base_url
-    "//#{SiteSetting.s3_upload_bucket.downcase}.s3.amazonaws.com"
-  end
-
-  def self.remove_file(url)
-    S3Store.check_missing_site_settings
-
-    directory = S3Store.get_or_create_directory(SiteSetting.s3_upload_bucket)
-
-    file = S3Store.destroy(url, directory)
-  end
-
-  def self.check_missing_site_settings
-    raise Discourse::SiteSettingMissing.new("s3_upload_bucket")     if SiteSetting.s3_upload_bucket.blank?
-    raise Discourse::SiteSettingMissing.new("s3_access_key_id")     if SiteSetting.s3_access_key_id.blank?
-    raise Discourse::SiteSettingMissing.new("s3_secret_access_key") if SiteSetting.s3_secret_access_key.blank?
-  end
-
-  def self.get_or_create_directory(name)
-    @fog_loaded = require 'fog' unless @fog_loaded
-
-    options = S3Store.generate_options
-
-    fog = Fog::Storage.new(options)
-
-    directory = fog.directories.get(name)
-    directory = fog.directories.create(key: name) unless directory
-
-    directory
-  end
-
-  def self.generate_options
-    options = {
-      provider: 'AWS',
-      aws_access_key_id: SiteSetting.s3_access_key_id,
-      aws_secret_access_key: SiteSetting.s3_secret_access_key
-    }
-    options[:region] = SiteSetting.s3_region unless SiteSetting.s3_region.empty?
-
-    options
-  end
-
-  def self.upload(file, name, directory)
-    directory.files.create(
-      key: name,
-      public: true,
-      body: file.tempfile,
-      content_type: file.content_type
-    )
-  end
-
-  def self.destroy(name, directory)
-    directory.files.destroy(key: name)
-  end
-
-end

lib/tasks/images.rake

@@ -10,27 +10,6 @@ task "images:compress" => :environment do
   end
 end
 
-desc "updates reverse index of image uploads"
-task "images:reindex" => :environment do
-	RailsMultisite::ConnectionManagement.each_connection do |db|
-		puts "Reindexing #{db}"
-    Post.select([:id, :cooked]).find_each do |p|
-			doc = Nokogiri::HTML::fragment(p.cooked)
-			doc.search("img").each do |img|
-				src = img['src']
-				if src.present? && Upload.has_been_uploaded?(src) && m = Upload.uploaded_regex.match(src)
-          begin
-            PostUpload.create({ post_id: p.id, upload_id: m[:upload_id] })
-          rescue ActiveRecord::RecordNotUnique
-          end
-				end
-			end
-      putc "."
-		end
-  end
-  puts "\ndone."
-end
-
 desc "clean orphan uploaded files"
 task "images:clean_orphans" => :environment do
   RailsMultisite::ConnectionManagement.each_connection do |db|

spec/components/cooked_post_processor_spec.rb

@@ -31,9 +31,7 @@ describe CookedPostProcessor do
         Upload.expects(:get_from_url).returns(upload)
         cpp.post_process_attachments
         # ensures absolute urls on attachment
-        cpp.html.should =~ /#{LocalStore.base_url}/
+        cpp.html.should =~ /#{Discourse.store.absolute_base_url}/
-        # ensure name is present
-        cpp.html.should =~ /archive.zip/
         # keeps the reverse index up to date
         post.uploads.reload
         post.uploads.count.should == 1
@@ -74,7 +72,7 @@ describe CookedPostProcessor do
         Upload.expects(:get_from_url).returns(upload)
         cpp.post_process_images
         # ensures absolute urls on uploaded images
-        cpp.html.should =~ /#{LocalStore.base_url}/
+        cpp.html.should =~ /#{LocalStore.new.absolute_base_url}/
         # dirty
         cpp.should be_dirty
         # keeps the reverse index up to date
@@ -227,7 +225,6 @@ describe CookedPostProcessor do
     let(:cpp) { CookedPostProcessor.new(post) }
 
     it "ensures s3 urls have a default scheme" do
-      Upload.stubs(:is_on_s3?).returns(true)
       FastImage.stubs(:size)
       cpp.expects(:is_valid_image_uri?).with("http://bucket.s3.aws.amazon.com/image.jpg")
       cpp.get_size("//bucket.s3.aws.amazon.com/image.jpg")
@@ -239,13 +236,15 @@ describe CookedPostProcessor do
 
       it "doesn't call FastImage" do
         FastImage.expects(:size).never
-        cpp.get_size("http://foo.bar/image.png").should == nil
+        cpp.get_size("http://foo.bar/image1.png").should == nil
       end
 
-      it "is always allowed to crawled our own images" do
+      it "is always allowed to crawl our own images" do
-        Upload.expects(:has_been_uploaded?).returns(true)
+        store = {}
+        Discourse.expects(:store).returns(store)
+        store.expects(:has_been_uploaded?).returns(true)
         FastImage.expects(:size).returns([100, 200])
-        cpp.get_size("http://foo.bar/image.png").should == [100, 200]
+        cpp.get_size("http://foo.bar/image2.png").should == [100, 200]
       end
 
     end
@@ -253,8 +252,8 @@ describe CookedPostProcessor do
     it "caches the results" do
       SiteSetting.stubs(:crawl_images?).returns(true)
       FastImage.expects(:size).returns([200, 400])
-      cpp.get_size("http://foo.bar/image.png")
+      cpp.get_size("http://foo.bar/image3.png")
-      cpp.get_size("http://foo.bar/image.png").should == [200, 400]
+      cpp.get_size("http://foo.bar/image3.png").should == [200, 400]
     end
 
   end

spec/components/discourse_spec.rb

@@ -65,5 +65,18 @@ describe Discourse do
 
   end
 
+  context "#store" do
+
+    it "returns LocalStore by default" do
+      Discourse.store.should be_a(LocalStore)
+    end
+
+    it "returns S3Store when S3 is enabled" do
+      SiteSetting.expects(:enable_s3_uploads?).returns(true)
+      Discourse.store.should be_a(S3Store)
+    end
+
+  end
+
 end
 

spec/components/file_store/local_store_spec.rb

@@ -0,0 +1,76 @@
+require 'spec_helper'
+require 'file_store/local_store'
+
+describe LocalStore do
+
+  let(:store) { LocalStore.new }
+
+  let(:upload) { build(:upload) }
+  let(:uploaded_file) do
+    ActionDispatch::Http::UploadedFile.new({
+      filename: 'logo.png',
+      tempfile: File.new("#{Rails.root}/spec/fixtures/images/logo.png")
+    })
+  end
+
+  let(:optimized_image) { build(:optimized_image) }
+
+  it "is internal" do
+    store.internal?.should == true
+    store.external?.should == false
+  end
+
+  describe "store_upload" do
+
+    it "returns a relative url" do
+      Time.stubs(:now).returns(Time.utc(2013, 2, 17, 12, 0, 0, 0))
+      upload.stubs(:id).returns(42)
+      store.expects(:copy_file)
+      store.store_upload(uploaded_file, upload).should == "/uploads/default/42/253dc8edf9d4ada1.png"
+    end
+
+  end
+
+  describe "store_optimized_image" do
+
+    it "returns a relative url" do
+      store.expects(:copy_file)
+      store.store_optimized_image({}, optimized_image).should == "/uploads/default/_optimized/86f/7e4/37faa5a7fce_100x200.png"
+    end
+
+  end
+
+  describe "remove_file" do
+
+    it "does not delete any file" do
+      File.expects(:delete).never
+      store.remove_file("/path/to/file")
+    end
+
+    it "deletes the file locally" do
+      File.expects(:delete)
+      store.remove_file("/uploads/default/42/253dc8edf9d4ada1.png")
+    end
+
+  end
+
+  describe "has_been_uploaded?" do
+
+    it "identifies local or relatives urls" do
+      Discourse.expects(:base_url_no_prefix).returns("http://discuss.site.com")
+      store.has_been_uploaded?("http://discuss.site.com/uploads/default/42/0123456789ABCDEF.jpg").should == true
+      store.has_been_uploaded?("/uploads/default/42/0123456789ABCDEF.jpg").should == true
+    end
+
+    it "identifies local urls when using a CDN" do
+      Rails.configuration.action_controller.stubs(:asset_host).returns("http://my.cdn.com")
+      store.has_been_uploaded?("http://my.cdn.com/uploads/default/42/0123456789ABCDEF.jpg").should == true
+    end
+
+    it "does not match dummy urls" do
+      store.has_been_uploaded?("http://domain.com/uploads/default/42/0123456789ABCDEF.jpg").should == false
+    end
+
+  end
+
+end

spec/components/file_store/s3_store_spec.rb

@@ -0,0 +1,84 @@
+require 'spec_helper'
+require 'fog'
+require 'file_store/s3_store'
+
+describe S3Store do
+
+  let(:store) { S3Store.new }
+
+  let(:upload) { build(:upload) }
+  let(:uploaded_file) do
+    ActionDispatch::Http::UploadedFile.new({
+      filename: 'logo.png',
+      tempfile: File.new("#{Rails.root}/spec/fixtures/images/logo.png")
+    })
+  end
+
+  let(:optimized_image) { build(:optimized_image) }
+  let(:optimized_image_file) do
+    ActionDispatch::Http::UploadedFile.new({
+      filename: 'logo.png',
+      tempfile: File.new("#{Rails.root}/spec/fixtures/images/logo.png")
+    })
+  end
+
+  before(:each) do
+    SiteSetting.stubs(:s3_upload_bucket).returns("S3_Upload_Bucket")
+    SiteSetting.stubs(:s3_access_key_id).returns("s3_access_key_id")
+    SiteSetting.stubs(:s3_secret_access_key).returns("s3_secret_access_key")
+    Fog.mock!
+  end
+
+  after(:each) { Fog.unmock! }
+
+  it "is internal" do
+    store.external?.should == true
+    store.internal?.should == false
+  end
+
+  describe "store_upload" do
+
+    it "returns a relative url" do
+      upload.stubs(:id).returns(42)
+      store.store_upload(uploaded_file, upload).should == "//s3_upload_bucket.s3.amazonaws.com/42e9d71f5ee7c92d6dc9e92ffdad17b8bd49418f98.png"
+    end
+
+  end
+
+  describe "store_optimized_image" do
+
+    it "returns a relative url" do
+      optimized_image.stubs(:id).returns(42)
+      store.store_optimized_image(optimized_image_file, optimized_image).should == "//s3_upload_bucket.s3.amazonaws.com/4286f7e437faa5a7fce15d1ddcb9eaeaea377667b8_100x200.png"
+    end
+
+  end
+
+  describe "remove_file" do
+
+    it "does not delete any file" do
+      store.expects(:remove).never
+      store.remove_file("//other_bucket.s3.amazonaws.com/42.png")
+    end
+
+    it "deletes the file on s3" do
+      store.expects(:remove)
+      store.remove_file("//s3_upload_bucket.s3.amazonaws.com/42.png")
+    end
+
+  end
+
+  describe "has_been_uploaded?" do
+
+    it "identifies S3 uploads" do
+      SiteSetting.stubs(:enable_s3_uploads).returns(true)
+      store.has_been_uploaded?("//s3_upload_bucket.s3.amazonaws.com/1337.png").should == true
+    end
+
+    it "does not match other s3 urls" do
+      store.has_been_uploaded?("//s3.amazonaws.com/Bucket/1337.png").should == false
+    end
+
+  end
+
+end

spec/components/local_store_spec.rb

@@ -1,30 +0,0 @@
-require 'spec_helper'
-require 'local_store'
-
-describe LocalStore do
-
-  describe "store_file" do
-
-    let(:file) do
-      ActionDispatch::Http::UploadedFile.new({
-        filename: 'logo.png',
-        content_type: 'image/png',
-        tempfile: File.new("#{Rails.root}/spec/fixtures/images/logo.png")
-      })
-    end
-
-    let(:image_info) { FastImage.new(file) }
-
-    it 'returns the url of the uploaded file if successful' do
-      # prevent the tests from creating directories & files...
-      FileUtils.stubs(:mkdir_p)
-      File.stubs(:open)
-      # The Time needs to be frozen as it is used to generate a clean & unique name
-      Time.stubs(:now).returns(Time.utc(2013, 2, 17, 12, 0, 0, 0))
-      #
-      LocalStore.store_file(file, "", 1).should == '/uploads/default/1/253dc8edf9d4ada1.png'
-    end
-
-  end
-
-end

spec/components/s3_store_spec.rb

@@ -1,36 +0,0 @@
-require 'spec_helper'
-require 'fog'
-require 's3_store'
-
-describe S3Store do
-
-  describe "store_file" do
-
-    let(:file) do
-      ActionDispatch::Http::UploadedFile.new({
-        filename: 'logo.png',
-        content_type: 'image/png',
-        tempfile: File.new("#{Rails.root}/spec/fixtures/images/logo.png")
-      })
-    end
-
-    let(:image_info) { FastImage.new(file) }
-
-    before(:each) do
-      SiteSetting.stubs(:s3_upload_bucket).returns("S3_Upload_Bucket")
-      SiteSetting.stubs(:s3_access_key_id).returns("s3_access_key_id")
-      SiteSetting.stubs(:s3_secret_access_key).returns("s3_secret_access_key")
-      Fog.mock!
-    end
-
-    it 'returns the url of the S3 upload if successful' do
-      S3Store.store_file(file, "SHA", 1).should == '//s3_upload_bucket.s3.amazonaws.com/1SHA.png'
-    end
-
-    after(:each) do
-      Fog.unmock!
-    end
-
-  end
-
-end

spec/fabricators/optimized_image_fabricator.rb

@@ -1,7 +1,8 @@
 Fabricator(:optimized_image) do
   upload
-  sha1 "abcdef"
+  sha1 "86f7e437faa5a7fce15d1ddcb9eaeaea377667b8"
   extension ".png"
   width 100
   height 200
+  url "138569_100x200.png"
 end

spec/fabricators/upload_fabricator.rb

@@ -1,5 +1,6 @@
 Fabricator(:upload) do
   user
+  sha1 "e9d71f5ee7c92d6dc9e92ffdad17b8bd49418f98"
   original_filename "uploaded.jpg"
   filesize 1234
   width 100

spec/models/optimized_image_spec.rb

@@ -4,25 +4,55 @@ describe OptimizedImage do
 
   it { should belong_to :upload }
 
-  let(:upload) { build(:upload) }
+  let(:upload) { Fabricate(:upload) }
-  let(:oi) { OptimizedImage.create_for(upload, 100, 100) }
+  let(:oi) { OptimizedImage.create_for(upload, 100, 200) }
 
   describe ".create_for" do
 
-    before(:each) do
+    before { ImageSorcery.any_instance.expects(:convert).returns(true) }
-      ImageSorcery.any_instance.expects(:convert).returns(true)
+
-      # make sure we don't hit the filesystem
+    describe "internal store" do
-      FileUtils.stubs(:mkdir_p)
-      File.stubs(:open)
-    end
 
       it "works" do
-      Tempfile.any_instance.expects(:close)
+        Tempfile.any_instance.expects(:close!)
-      Tempfile.any_instance.expects(:unlink)
         oi.sha1.should == "da39a3ee5e6b4b0d3255bfef95601890afd80709"
         oi.extension.should == ".jpg"
         oi.width.should == 100
-      oi.height.should == 100
+        oi.height.should == 200
+        oi.url.should == "/uploads/default/_optimized/da3/9a3/ee5e6b4b0d3_100x200.jpg"
+      end
+
+    end
+
+    describe "external store" do
+
+      require 'file_store/s3_store'
+      require 'fog'
+
+      let(:store) { S3Store.new }
+
+      before do
+        Discourse.stubs(:store).returns(store)
+        SiteSetting.stubs(:s3_upload_bucket).returns("S3_Upload_Bucket")
+        SiteSetting.stubs(:s3_access_key_id).returns("s3_access_key_id")
+        SiteSetting.stubs(:s3_secret_access_key).returns("s3_secret_access_key")
+        Fog.mock!
+      end
+
+      it "works" do
+        # fake downloaded file
+        downloaded_file = {}
+        downloaded_file.expects(:path).returns("/path/to/fake.png")
+        downloaded_file.expects(:close!)
+        store.expects(:download).returns(downloaded_file)
+        # assertions
+        oi.sha1.should == "da39a3ee5e6b4b0d3255bfef95601890afd80709"
+        oi.extension.should == ".png"
+        oi.width.should == 100
+        oi.height.should == 200
+        oi.url.should =~ /^\/\/s3_upload_bucket.s3.amazonaws.com\/[0-9a-f]+_100x200.png/
+      end
+
     end
 
   end

spec/models/upload_spec.rb

@@ -42,20 +42,12 @@ describe Upload do
 
     it "does not create a thumbnail when disabled" do
       SiteSetting.stubs(:create_thumbnails?).returns(false)
-      SiteSetting.expects(:enable_s3_uploads?).never
+      OptimizedImage.expects(:create_for).never
-      upload.create_thumbnail!
-    end
-
-    it "does not create a thumbnail when using S3" do
-      SiteSetting.expects(:create_thumbnails?).returns(true)
-      SiteSetting.expects(:enable_s3_uploads?).returns(true)
-      upload.expects(:has_thumbnail?).never
       upload.create_thumbnail!
     end
 
     it "does not create another thumbnail" do
       SiteSetting.expects(:create_thumbnails?).returns(true)
-      SiteSetting.expects(:enable_s3_uploads?).returns(false)
       upload.expects(:has_thumbnail?).returns(true)
       OptimizedImage.expects(:create_for).never
       upload.create_thumbnail!
@@ -65,7 +57,6 @@ describe Upload do
       upload = Fabricate(:upload)
       thumbnail = Fabricate(:optimized_image, upload: upload)
       SiteSetting.expects(:create_thumbnails?).returns(true)
-      SiteSetting.expects(:enable_s3_uploads?).returns(false)
       upload.expects(:has_thumbnail?).returns(false)
       OptimizedImage.expects(:create_for).returns(thumbnail)
       upload.create_thumbnail!
@@ -104,7 +95,9 @@ describe Upload do
     end
 
     it "saves proper information" do
-      Upload.expects(:store_file).returns(url)
+      store = {}
+      Discourse.expects(:store).returns(store)
+      store.expects(:store_upload).returns(url)
       upload = Upload.create_for(user_id, image, image_filesize)
       upload.user_id.should == user_id
       upload.original_filename.should == image.original_filename
@@ -117,66 +110,6 @@ describe Upload do
 
   end
 
-  context ".store_file" do
-
-    it "store files on s3 when enabled" do
-      SiteSetting.expects(:enable_s3_uploads?).returns(true)
-      LocalStore.expects(:store_file).never
-      S3Store.expects(:store_file)
-      Upload.store_file(image, image_sha1, 1)
-    end
-
-    it "store files locally by default" do
-      S3Store.expects(:store_file).never
-      LocalStore.expects(:store_file)
-      Upload.store_file(image, image_sha1, 1)
-    end
-
-  end
-
-  context ".remove_file" do
-
-    it "remove files on s3 when enabled" do
-      SiteSetting.expects(:enable_s3_uploads?).returns(true)
-      LocalStore.expects(:remove_file).never
-      S3Store.expects(:remove_file)
-      Upload.remove_file(upload.url)
-    end
-
-    it "remove files locally by default" do
-      S3Store.expects(:remove_file).never
-      LocalStore.expects(:remove_file)
-      Upload.remove_file(upload.url)
-    end
-
-  end
-
-  context ".has_been_uploaded?" do
-
-    it "identifies internal or relatives urls" do
-      Discourse.expects(:base_url_no_prefix).returns("http://discuss.site.com")
-      Upload.has_been_uploaded?("http://discuss.site.com/uploads/default/42/0123456789ABCDEF.jpg").should == true
-      Upload.has_been_uploaded?("/uploads/default/42/0123456789ABCDEF.jpg").should == true
-    end
-
-    it "identifies internal urls when using a CDN" do
-      Rails.configuration.action_controller.expects(:asset_host).returns("http://my.cdn.com").twice
-      Upload.has_been_uploaded?("http://my.cdn.com/uploads/default/42/0123456789ABCDEF.jpg").should == true
-    end
-
-    it "identifies S3 uploads" do
-      SiteSetting.stubs(:enable_s3_uploads).returns(true)
-      SiteSetting.stubs(:s3_upload_bucket).returns("Bucket")
-      Upload.has_been_uploaded?("//bucket.s3.amazonaws.com/1337.png").should == true
-    end
-
-    it "identifies external urls" do
-      Upload.has_been_uploaded?("http://domain.com/uploads/default/42/0123456789ABCDEF.jpg").should == false
-      Upload.has_been_uploaded?("//s3.amazonaws.com/Bucket/1337.png").should == false
-    end
-
-  end
-
   context ".get_from_url" do
 
     it "works when the file has been uploaded" do