Merge pull request #1075 from ZogStriP/fix-lightboxing-when-using-s3

FIX: lightbox wasn't working when using s3 upload

app/models/upload.rb

@@ -30,6 +30,7 @@ class Upload < ActiveRecord::Base
 
   def create_thumbnail!
     return unless SiteSetting.create_thumbnails?
+    return if SiteSetting.enable_s3_uploads?
     return unless width > SiteSetting.auto_link_images_wider_than
     return if has_thumbnail?
     thumbnail = OptimizedImage.create_for(self, width, height)
@@ -86,12 +87,20 @@ class Upload < ActiveRecord::Base
     LocalStore.remove_file(url)
   end
 
-  def self.uploaded_regex
-    /\/uploads\/#{RailsMultisite::ConnectionManagement.current_db}\/(?<upload_id>\d+)\/[0-9a-f]{16}\.(png|jpg|jpeg|gif|tif|tiff|bmp)/
+  def self.has_been_uploaded?(url)
+    is_relative?(url) || is_local?(url) || is_on_s3?(url)
   end
 
-  def self.has_been_uploaded?(url)
-    (url =~ /^\/[^\/]/) == 0 || url.start_with?(base_url)
+  def self.is_relative?(url)
+    (url =~ /^\/[^\/]/) == 0
+  end
+
+  def self.is_local?(url)
+    url.start_with?(base_url)
+  end
+
+  def self.is_on_s3?(url)
+    SiteSetting.enable_s3_uploads? && url.start_with?(S3.base_url)
   end
 
   def self.base_url
@@ -122,6 +131,7 @@ end
 # Indexes
 #
 #  index_uploads_on_sha1     (sha1) UNIQUE
+#  index_uploads_on_url      (url)
 #  index_uploads_on_user_id  (user_id)
 #
 

db/migrate/20130622110348_add_url_index_to_uploads.rb

@@ -0,0 +1,5 @@
+class AddUrlIndexToUploads < ActiveRecord::Migration
+  def change
+    add_index :uploads, :url
+  end
+end

lib/cooked_post_processor.rb

@@ -85,8 +85,12 @@ class CookedPostProcessor
   end
 
   def get_upload_from_url(url)
-    if Upload.has_been_uploaded?(url) && m = Upload.uploaded_regex.match(url)
-      Upload.where("id = ?", m[:upload_id]).first
+    if Upload.has_been_uploaded?(url)
+      if m = LocalStore.uploaded_regex.match(url)
+        Upload.where(id: m[:upload_id]).first
+      elsif Upload.is_on_s3?(url)
+        Upload.where(url: url).first
+      end
     end
   end
 
@@ -160,22 +164,23 @@ class CookedPostProcessor
 
   # Retrieve the image dimensions for a url
   def image_dimensions(url)
-    uri = get_image_uri(url)
-    return unless uri
     w, h = get_size(url)
     ImageSizer.resize(w, h) if w && h
   end
 
   def get_size(url)
-    # we can always crawl our own images
+    # make sure s3 urls have a scheme (otherwise, FastImage will fail)
+    url = "http:" + url if Upload.is_on_s3? (url)
+    return unless is_valid_image_uri? url
+    # we can *always* crawl our own images
     return unless SiteSetting.crawl_images? || Upload.has_been_uploaded?(url)
     @size_cache[url] ||= FastImage.size(url)
   rescue Zlib::BufError # FastImage.size raises BufError for some gifs
   end
 
-  def get_image_uri(url)
+  def is_valid_image_uri?(url)
     uri = URI.parse(url)
-    uri if %w(http https).include?(uri.scheme)
+    %w(http https).include? uri.scheme
   end
 
   def dirty?

lib/local_store.rb

@@ -20,4 +20,8 @@ module LocalStore
   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
+
 end

lib/s3.rb

@@ -9,7 +9,11 @@ module S3
 
     # if this fails, it will throw an exception
     file = S3.upload(file, remote_filename, directory)
-    return "//#{SiteSetting.s3_upload_bucket}.s3.amazonaws.com/#{remote_filename}"
+    "#{S3.base_url}/#{remote_filename}"
+  end
+
+  def self.base_url
+    "//#{SiteSetting.s3_upload_bucket}.s3.amazonaws.com"
   end
 
   def self.remove_file(url)

spec/components/cooked_post_processor_spec.rb

@@ -179,15 +179,15 @@ describe CookedPostProcessor do
     end
   end
 
-  context 'get_image_uri' do
+  context 'is_valid_image_uri?' do
 
-    it "returns nil unless the scheme is either http or https" do
-      cpp.get_image_uri("http://domain.com").should   == URI.parse("http://domain.com")
-      cpp.get_image_uri("https://domain.com").should  == URI.parse("https://domain.com")
-      cpp.get_image_uri("ftp://domain.com").should    == nil
-      cpp.get_image_uri("ftps://domain.com").should   == nil
-      cpp.get_image_uri("//domain.com").should        == nil
-      cpp.get_image_uri("/tmp/image.png").should      == nil
+    it "needs the scheme to be either http or https" do
+      cpp.is_valid_image_uri?("http://domain.com").should   == true
+      cpp.is_valid_image_uri?("https://domain.com").should  == true
+      cpp.is_valid_image_uri?("ftp://domain.com").should    == false
+      cpp.is_valid_image_uri?("ftps://domain.com").should   == false
+      cpp.is_valid_image_uri?("//domain.com").should        == false
+      cpp.is_valid_image_uri?("/tmp/image.png").should      == false
     end
 
   end

spec/models/upload_spec.rb

@@ -71,8 +71,15 @@ describe Upload do
       Upload.has_been_uploaded?("http://my.cdn.com/upload/1234/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/upload/1234/42/0123456789ABCDEF.jpg").should == false
+      Upload.has_been_uploaded?("//bucket.s3.amazonaws.com/1337.png").should == false
     end
 
   end