Paperclip error thrown on assign, not when attachment declared. Added…

Paperclip error thrown on assign, not when attachment declared. Added :s3_domain_url and :s3_path_url for differing S3 URL styles. Added s3_protocol for using http over https.

lib/paperclip.rb

@@ -113,12 +113,6 @@ module Paperclip
     def has_attached_file name, options = {}
       include InstanceMethods
 
-      %w(file_name).each do |field|
-        unless column_names.include?("#{name}_#{field}")
-          raise PaperclipError.new("#{self} model does not have required column '#{name}_#{field}'")
-        end
-      end
-
       write_inheritable_attribute(:attachment_definitions, {}) if attachment_definitions.nil?
       attachment_definitions[name] = {:validations => []}.merge(options)
 

lib/paperclip/attachment.rb

@@ -53,6 +53,12 @@ module Paperclip
     # In addition to form uploads, you can also assign another Paperclip attachment:
     #   new_user.avatar = old_user.avatar
     def assign uploaded_file
+      %w(file_name).each do |field|
+        unless @instance.class.column_names.include?("#{name}_#{field}")
+          raise PaperclipError.new("#{self} model does not have required column '#{name}_#{field}'")
+        end
+      end
+
       if uploaded_file.is_a?(Paperclip::Attachment)
         uploaded_file = uploaded_file.to_file(:original)
       end

lib/paperclip/storage.rb

@@ -85,9 +85,17 @@ module Paperclip
     #   policies that S3 provides (more information can be found here:
     #   http://docs.amazonwebservices.com/AmazonS3/2006-03-01/RESTAccessPolicy.html#RESTCannedAccessPolicies)
     #   The default for Paperclip is "public-read".
+    # * +s3_protocol+: The protocol for the URLs generated to your S3 assets. Can be either 
+    #   'http' or 'https'. Defaults to 'http' when your :s3_permissions are 'public-read' (the
+    #   default), and 'https' when your :s3_permissions are anything else.
     # * +bucket+: This is the name of the S3 bucket that will store your files. Remember
     #   that the bucket must be unique across all of Amazon S3. If the bucket does not exist
     #   Paperclip will attempt to create it. The bucket name will not be interpolated.
+    # * +url+: There are two options for the S3 url. You can choose to have the bucket's name
+    #   placed domain-style (bucket.s3.amazonaws.com) or path-style (s3.amazonaws.com/bucket).
+    #   Normally, this won't matter in the slightest and you can leave the default (which is
+    #   path-style, or :s3_path_url). But in some cases paths don't work and you need to use
+    #   the domain-style (:s3_domain_url). Anything else here will be treated like path-style.
     # * +path+: This is the key under the bucket in which the file will be stored. The
     #   URL will be constructed from the bucket and the path. This is what you will want
     #   to interpolate. Keys should be unique, like filenames, and despite the fact that
@@ -97,14 +105,18 @@ module Paperclip
       def self.extended base
         require 'right_aws'
         base.instance_eval do
-          @bucket             = @options[:bucket]
-          @s3_credentials     = parse_credentials(@options[:s3_credentials])
-          @s3_options         = @options[:s3_options] || {}
-          @s3_permissions     = @options[:s3_permissions] || 'public-read'
-          @url                = ":s3_url"
+          @bucket         = @options[:bucket]
+          @s3_credentials = parse_credentials(@options[:s3_credentials])
+          @s3_options     = @options[:s3_options] || {}
+          @s3_permissions = @options[:s3_permissions] || 'public-read'
+          @s3_protocol    = @options[:s3_protocol] || (@s3_permissions == 'public-read' ? 'http' : 'https')
+          @url            = ":s3_path_url" unless @url.to_s.match(/^s3.*url$/)
         end
-        base.class.interpolations[:s3_url] = lambda do |attachment, style|
-          "https://s3.amazonaws.com/#{attachment.bucket_name}/#{attachment.path(style).gsub(%r{^/}, "")}"
+        base.class.interpolations[:s3_path_url] = lambda do |attachment, style|
+          "#{attachment.s3_protocol}://s3.amazonaws.com/#{attachment.bucket_name}/#{attachment.path(style).gsub(%r{^/}, "")}"
+        end
+        base.class.interpolations[:s3_domain_url] = lambda do |attachment, style|
+          "#{attachment.s3_protocol}://#{attachment.bucket_name}.s3.amazonaws.com/#{attachment.path(style).gsub(%r{^/}, "")}"
         end
         ActiveRecord::Base.logger.info("[paperclip] S3 Storage Initalized.")
       end
@@ -132,6 +144,10 @@ module Paperclip
         s3_bucket.key(path(style)) ? true : false
       end
 
+      def s3_protocol
+        @s3_protocol
+      end
+
       # Returns representation of the data of the file assigned to the given
       # style, in the format most representative of the current storage.
       def to_file style = default_style

test/attachment_test.rb

@@ -148,40 +148,40 @@ class AttachmentTest < Test::Unit::TestCase
         :path => ":rails_root/tmp/:attachment/:class/:style/:id/:basename.:extension"
       })
       FileUtils.rm_rf("tmp")
-      @instance = stub
-      @instance.stubs(:id).returns(41)
-      @instance.stubs(:class).returns(Dummy)
-      @instance.stubs(:[]).with(:test_file_name).returns(nil)
-      @instance.stubs(:[]).with(:test_content_type).returns(nil)
-      @instance.stubs(:[]).with(:test_file_size).returns(nil)
-      @instance.stubs(:[]).with(:test_updated_at).returns(nil)
-      @instance.stubs(:logger).returns(ActiveRecord::Base.logger)
-      @attachment = Paperclip::Attachment.new(:test,
-                                              @instance)
+      rebuild_model
+      @instance = Dummy.new
+      @attachment = Paperclip::Attachment.new(:avatar, @instance)
       @file = File.new(File.join(File.dirname(__FILE__),
                                  "fixtures",
                                  "5k.png"))
     end
 
+    should "raise if there are not the correct columns when you try to assign" do
+      @other_attachment = Paperclip::Attachment.new(:not_here, @instance)
+      assert_raises(Paperclip::PaperclipError) do
+        @other_attachment.assign(@file)
+      end
+    end
+
     should "return its default_url when no file assigned" do
       assert @attachment.to_file.nil?
-      assert_equal "/tests/original/missing.png", @attachment.url
-      assert_equal "/tests/blah/missing.png", @attachment.url(:blah)
+      assert_equal "/avatars/original/missing.png", @attachment.url
+      assert_equal "/avatars/blah/missing.png", @attachment.url(:blah)
     end
     
     context "with a file assigned in the database" do
       setup do
-        @instance.stubs(:[]).with(:test_file_name).returns("5k.png")
-        @instance.stubs(:[]).with(:test_content_type).returns("image/png")
-        @instance.stubs(:[]).with(:test_file_size).returns(12345)
+        @instance.stubs(:[]).with(:avatar_file_name).returns("5k.png")
+        @instance.stubs(:[]).with(:avatar_content_type).returns("image/png")
+        @instance.stubs(:[]).with(:avatar_file_size).returns(12345)
         now = Time.now
         Time.stubs(:now).returns(now)
-        @instance.stubs(:[]).with(:test_updated_at).returns(Time.now)
+        @instance.stubs(:[]).with(:avatar_updated_at).returns(Time.now)
       end
 
       should "return a correct url even if the file does not exist" do
         assert_nil @attachment.to_file
-        assert_match %r{^/tests/41/blah/5k\.png}, @attachment.url(:blah)
+        assert_match %r{^/avatars/#{@instance.id}/blah/5k\.png}, @attachment.url(:blah)
       end
 
       should "make sure the updated_at mtime is in the url if it is defined" do
@@ -190,21 +190,21 @@ class AttachmentTest < Test::Unit::TestCase
 
       context "with the updated_at field removed" do
         setup do
-          @instance.stubs(:[]).with(:test_updated_at).returns(nil)
+          @instance.stubs(:[]).with(:avatar_updated_at).returns(nil)
         end
 
         should "only return the url without the updated_at when sent #url" do
-          assert_match "/tests/41/blah/5k.png", @attachment.url(:blah)
+          assert_match "/avatars/#{@instance.id}/blah/5k.png", @attachment.url(:blah)
         end
       end
 
       should "return the proper path when filename has a single .'s" do
-        assert_equal "./test/../tmp/tests/dummies/original/41/5k.png", @attachment.path
+        assert_equal "./test/../tmp/avatars/dummies/original/#{@instance.id}/5k.png", @attachment.path
       end
 
       should "return the proper path when filename has multiple .'s" do
-        @instance.stubs(:[]).with(:test_file_name).returns("5k.old.png")      
-        assert_equal "./test/../tmp/tests/dummies/original/41/5k.old.png", @attachment.path
+        @instance.stubs(:[]).with(:avatar_file_name).returns("5k.old.png")      
+        assert_equal "./test/../tmp/avatars/dummies/original/#{@instance.id}/5k.old.png", @attachment.path
       end
 
       context "when expecting three styles" do
@@ -212,7 +212,7 @@ class AttachmentTest < Test::Unit::TestCase
           styles = {:styles => { :large  => ["400x400", :png],
                                  :medium => ["100x100", :gif],
                                  :small => ["32x32#", :jpg]}}
-          @attachment = Paperclip::Attachment.new(:test,
+          @attachment = Paperclip::Attachment.new(:avatar,
                                                   @instance,
                                                   styles)
         end
@@ -221,15 +221,6 @@ class AttachmentTest < Test::Unit::TestCase
           setup do
             now = Time.now
             Time.stubs(:now).returns(now)
-            @instance.expects(:[]=).with(:test_file_name,
-                                         File.basename(@file.path))
-            @instance.expects(:[]=).with(:test_content_type, "image/png")
-            @instance.expects(:[]=).with(:test_file_size, @file.size)
-            @instance.expects(:[]=).with(:test_file_name, nil)
-            @instance.expects(:[]=).with(:test_content_type, nil)
-            @instance.expects(:[]=).with(:test_file_size, nil)
-            @instance.expects(:[]=).with(:test_updated_at, nil)
-            @instance.expects(:[]=).with(:test_updated_at, now)
             @attachment.assign(@file)
           end
 
@@ -244,8 +235,8 @@ class AttachmentTest < Test::Unit::TestCase
 
             should "return the real url" do
               assert @attachment.to_file
-              assert_match %r{^/tests/41/original/5k\.png}, @attachment.url
-              assert_match %r{^/tests/41/small/5k\.jpg}, @attachment.url(:small)
+              assert_match %r{^/avatars/#{@instance.id}/original/5k\.png}, @attachment.url
+              assert_match %r{^/avatars/#{@instance.id}/small/5k\.jpg}, @attachment.url(:small)
             end
 
             should "commit the files to disk" do
@@ -279,10 +270,10 @@ class AttachmentTest < Test::Unit::TestCase
                 @existing_names = @attachment.styles.keys.collect do |style|
                   @attachment.path(style)
                 end
-                @instance.expects(:[]=).with(:test_file_name, nil)
-                @instance.expects(:[]=).with(:test_content_type, nil)
-                @instance.expects(:[]=).with(:test_file_size, nil)
-                @instance.expects(:[]=).with(:test_updated_at, nil)
+                @instance.expects(:[]=).with(:avatar_file_name, nil)
+                @instance.expects(:[]=).with(:avatar_content_type, nil)
+                @instance.expects(:[]=).with(:avatar_file_size, nil)
+                @instance.expects(:[]=).with(:avatar_updated_at, nil)
                 @attachment.assign nil
                 @attachment.save
               end

test/paperclip_test.rb

@@ -7,8 +7,8 @@ class PaperclipTest < Test::Unit::TestCase
       @file = File.new(File.join(FIXTURES_DIR, "5k.png"))
     end
 
-    should "error when trying to also create a 'blah' attachment" do
-      assert_raises(Paperclip::PaperclipError) do
+    should "not error when trying to also create a 'blah' attachment" do
+      assert_nothing_raised do
         Dummy.class_eval do
           has_attached_file :blah
         end

test/storage_test.rb

@@ -71,7 +71,7 @@ class StorageTest < Test::Unit::TestCase
       should "not get a bucket to get a URL" do
         @dummy.avatar.expects(:s3).never
         @dummy.avatar.expects(:s3_bucket).never
-        assert_match %r{^https://s3\.amazonaws\.com/testing/avatars/original/5k\.png}, @dummy.avatar.url
+        assert_match %r{^http://s3\.amazonaws\.com/testing/avatars/original/5k\.png}, @dummy.avatar.url
       end
 
       context "and saved" do