Skip to content
This repository has been archived by the owner on Jul 13, 2023. It is now read-only.

Commit

Permalink
Converted to AWS::S3, all tests passing.
Browse files Browse the repository at this point in the history
  • Loading branch information
Jon Yurek committed Jul 30, 2009
1 parent 80d699c commit ac86bb9
Show file tree
Hide file tree
Showing 5 changed files with 93 additions and 80 deletions.
2 changes: 1 addition & 1 deletion lib/paperclip.rb
Expand Up @@ -93,7 +93,7 @@ def interpolates key, &block
# Paperclip.options[:log_command] is set to true (defaults to false). This
# will only log if logging in general is set to true as well.
def run cmd, params = "", expected_outcodes = 0
command = %Q<#{%Q[#{path_for_command(cmd)} #{params}].gsub(/\s+/, " ")}>
command = %Q[#{path_for_command(cmd)} #{params}].gsub(/\s+/, " ")
command = "#{command} 2>#{bit_bucket}" if Paperclip.options[:swallow_stderr]
Paperclip.log(command) if Paperclip.options[:log_command]
output = `#{command}`
Expand Down
38 changes: 18 additions & 20 deletions lib/paperclip/storage.rb
Expand Up @@ -33,7 +33,6 @@ def exists?(style = default_style)
def to_file style = default_style
@queued_for_write[style] || (File.new(path(style), 'rb') if exists?(style))
end
alias_method :to_io, :to_file

def flush_writes #:nodoc:
@queued_for_write.each do |style, file|
Expand Down Expand Up @@ -128,7 +127,7 @@ def flush_deletes #:nodoc:
# separate parts of your file name.
module S3
def self.extended base
require 'right_aws'
require 'aws/s3'
base.instance_eval do
@s3_credentials = parse_credentials(@options[:s3_credentials])
@bucket = @options[:bucket] || @s3_credentials[:bucket]
Expand All @@ -152,13 +151,10 @@ def self.extended base
end

def s3
@s3 ||= RightAws::S3.new(@s3_credentials[:access_key_id],
@s3_credentials[:secret_access_key],
@s3_options)
end

def s3_bucket
@s3_bucket ||= s3.bucket(@bucket, true, @s3_permissions)
AWS::S3::S3Object.establish_connection( @s3_options.merge(
:access_key_id => @s3_credentials[:access_key_id],
:secret_access_key => @s3_credentials[:secret_access_key]
))
end

def bucket_name
Expand All @@ -175,7 +171,7 @@ def parse_credentials creds
end

def exists?(style = default_style)
s3_bucket.key(path(style)) ? true : false
AWS::S3::S3Object.exists?(path(style), bucket_name)
end

def s3_protocol
Expand All @@ -185,18 +181,22 @@ def s3_protocol
# 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
@queued_for_write[style] || s3_bucket.key(path(style))
return @queued_for_write[style] if @queued_for_write[style]
file = Tempfile.new(path(style))
file.write(AWS::S3::S3Object.value(path(style), bucket_name))
file.rewind
return file
end
alias_method :to_io, :to_file

def flush_writes #:nodoc:
@queued_for_write.each do |style, file|
begin
log("saving #{path(style)}")
key = s3_bucket.key(path(style))
key.data = file
key.put(nil, @s3_permissions, {'Content-type' => instance_read(:content_type)}.merge(@s3_headers))
rescue RightAws::AwsError => e
AWS::S3::S3Object.store(path(style),
file,
bucket_name,
{:content_type => instance_read(:content_type)}.merge(@s3_headers))
rescue AWS::S3::ResponseError => e
raise
end
end
Expand All @@ -207,10 +207,8 @@ def flush_deletes #:nodoc:
@queued_for_delete.each do |path|
begin
log("deleting #{path}")
if file = s3_bucket.key(path)
file.delete
end
rescue RightAws::AwsError
AWS::S3::S3Object.delete(path, bucket_name)
rescue AWS::S3::ResponseError
# Ignore this.
end
end
Expand Down
14 changes: 13 additions & 1 deletion test/attachment_test.rb
Expand Up @@ -13,6 +13,18 @@ class AttachmentTest < Test::Unit::TestCase
assert_equal "#{RAILS_ROOT}/public/fake_models/1234/fake", @attachment.path
end

should "call a proc sent to check_guard" do
@dummy = Dummy.new
@dummy.expects(:one).returns(:one)
assert_equal :one, @dummy.avatar.send(:check_guard, lambda{|x| x.one })
end

should "call a method name sent to check_guard" do
@dummy = Dummy.new
@dummy.expects(:one).returns(:one)
assert_equal :one, @dummy.avatar.send(:check_guard, :one)
end

context "Attachment default_options" do
setup do
rebuild_model
Expand Down Expand Up @@ -593,7 +605,7 @@ def do_after_all; end

should "commit the files to disk" do
[:large, :medium, :small].each do |style|
io = @attachment.to_io(style)
io = @attachment.to_file(style)
assert File.exists?(io)
assert ! io.is_a?(::Tempfile)
io.close
Expand Down
85 changes: 53 additions & 32 deletions test/paperclip_test.rb
@@ -1,14 +1,18 @@
require 'test/helper'

class PaperclipTest < Test::Unit::TestCase
[:image_magick_path, :convert_path].each do |path|
context "Calling Paperclip.run with an #{path} specified" do
[:image_magick_path, :command_path].each do |path|
context "Calling Paperclip.run with #{path} specified" do
setup do
Paperclip.options[:image_magick_path] = nil
Paperclip.options[:convert_path] = nil
Paperclip.options[:command_path] = nil
Paperclip.options[path] = "/usr/bin"
end

should "return the expected path for path_for_command" do
assert_equal "/usr/bin/convert", Paperclip.path_for_command("convert")
end

should "execute the right command" do
Paperclip.expects(:path_for_command).with("convert").returns("/usr/bin/convert")
Paperclip.expects(:bit_bucket).returns("/dev/null")
Expand All @@ -21,7 +25,11 @@ class PaperclipTest < Test::Unit::TestCase
context "Calling Paperclip.run with no path specified" do
setup do
Paperclip.options[:image_magick_path] = nil
Paperclip.options[:convert_path] = nil
Paperclip.options[:command_path] = nil
end

should "return the expected path fro path_for_command" do
assert_equal "convert", Paperclip.path_for_command("convert")
end

should "execute the right command" do
Expand All @@ -30,38 +38,38 @@ class PaperclipTest < Test::Unit::TestCase
Paperclip.expects(:"`").with("convert one.jpg two.jpg 2>/dev/null")
Paperclip.run("convert", "one.jpg two.jpg")
end
end

should "log the command when :log_command is set" do
context "Calling Paperclip.run and logging" do
setup do
Paperclip.options[:image_magick_path] = nil
Paperclip.options[:command_path] = nil
Paperclip.stubs(:bit_bucket).returns("/dev/null")
Paperclip.stubs(:log)
Paperclip.stubs(:"`").with("this is the command 2>/dev/null")
end

should "log the command when :log_command is true" do
Paperclip.options[:log_command] = true
Paperclip.expects(:bit_bucket).returns("/dev/null")
Paperclip.expects(:log).with("this is the command 2>/dev/null")
Paperclip.expects(:"`").with("this is the command 2>/dev/null")
Paperclip.run("this","is the command")
assert_received(Paperclip, :log) do |p|
p.with("this is the command 2>/dev/null")
end
assert_received(Paperclip, :`) do |p|
p.with("this is the command 2>/dev/null")
end
end
end

should "raise when sent #processor and the name of a class that exists but isn't a subclass of Processor" do
assert_raises(Paperclip::PaperclipError){ Paperclip.processor(:attachment) }
end

should "raise when sent #processor and the name of a class that doesn't exist" do
assert_raises(NameError){ Paperclip.processor(:boogey_man) }
end

should "return a class when sent #processor and the name of a class under Paperclip" do
assert_equal ::Paperclip::Thumbnail, Paperclip.processor(:thumbnail)
end

should "call a proc sent to check_guard" do
@dummy = Dummy.new
@dummy.expects(:one).returns(:one)
assert_equal :one, @dummy.avatar.send(:check_guard, lambda{|x| x.one })
end

should "call a method name sent to check_guard" do
@dummy = Dummy.new
@dummy.expects(:one).returns(:one)
assert_equal :one, @dummy.avatar.send(:check_guard, :one)
should "not log the command when :log_command is false" do
Paperclip.options[:log_command] = false
Paperclip.run("this","is the command")
assert_received(Paperclip, :log) do |p|
p.with("this is the command 2>/dev/null").never
end
assert_received(Paperclip, :`) do |p|
p.with("this is the command 2>/dev/null")
end
end
end

context "Paperclip.bit_bucket" do
Expand All @@ -86,6 +94,18 @@ class PaperclipTest < Test::Unit::TestCase
end
end

should "raise when sent #processor and the name of a class that exists but isn't a subclass of Processor" do
assert_raises(Paperclip::PaperclipError){ Paperclip.processor(:attachment) }
end

should "raise when sent #processor and the name of a class that doesn't exist" do
assert_raises(NameError){ Paperclip.processor(:boogey_man) }
end

should "return a class when sent #processor and the name of a class under Paperclip" do
assert_equal ::Paperclip::Thumbnail, Paperclip.processor(:thumbnail)
end

context "An ActiveRecord model with an 'avatar' attachment" do
setup do
rebuild_model :path => "tmp/:class/omg/:style.:extension"
Expand Down Expand Up @@ -139,7 +159,8 @@ class ::SubDummy < Dummy; end
end

should "be able to see the attachment definition from the subclass's class" do
assert_equal "tmp/:class/omg/:style.:extension", SubDummy.attachment_definitions[:avatar][:path]
assert_equal "tmp/:class/omg/:style.:extension",
SubDummy.attachment_definitions[:avatar][:path]
end

teardown do
Expand Down
34 changes: 8 additions & 26 deletions test/storage_test.rb
Expand Up @@ -146,14 +146,7 @@ class StorageTest < Test::Unit::TestCase

context "and saved" do
setup do
@s3_mock = stub
@bucket_mock = stub
RightAws::S3.expects(:new).with("12345", "54321", {}).returns(@s3_mock)
@s3_mock.expects(:bucket).with("testing", true, "public-read").returns(@bucket_mock)
@key_mock = stub
@bucket_mock.expects(:key).returns(@key_mock)
@key_mock.expects(:data=)
@key_mock.expects(:put).with(nil, 'public-read', 'Content-type' => 'image/png')
AWS::S3::S3Object.stubs(:store).with(@dummy.avatar.path, anything, 'testing', :content_type => 'image/png')
@dummy.save
end

Expand All @@ -164,13 +157,8 @@ class StorageTest < Test::Unit::TestCase

context "and remove" do
setup do
@s3_mock = stub
@bucket_mock = stub
RightAws::S3.expects(:new).with("12345", "54321", {}).returns(@s3_mock)
@s3_mock.expects(:bucket).with("testing", true, "public-read").returns(@bucket_mock)
@key_mock = stub
@bucket_mock.expects(:key).at_least(2).returns(@key_mock)
@key_mock.expects(:delete)
AWS::S3::S3Object.stubs(:exists?).returns(true)
AWS::S3::S3Object.stubs(:delete)
@dummy.destroy_attached_files
end

Expand Down Expand Up @@ -217,17 +205,11 @@ class StorageTest < Test::Unit::TestCase

context "and saved" do
setup do
@s3_mock = stub
@bucket_mock = stub
RightAws::S3.expects(:new).with("12345", "54321", {}).returns(@s3_mock)
@s3_mock.expects(:bucket).with("testing", true, "public-read").returns(@bucket_mock)
@key_mock = stub
@bucket_mock.expects(:key).returns(@key_mock)
@key_mock.expects(:data=)
@key_mock.expects(:put).with(nil,
'public-read',
'Content-type' => 'image/png',
'Cache-Control' => 'max-age=31557600')
AWS::S3::S3Object.stubs(:store).with(@dummy.avatar.path,
anything,
'testing',
:content_type => 'image/png',
'Cache-Control' => 'max-age=31557600')
@dummy.save
end

Expand Down

25 comments on commit ac86bb9

@michaeldwan
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was the reason for switching from RightAWS to AWS::S3?

@colin
Copy link

@colin colin commented on ac86bb9 Aug 4, 2009

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be interested in the reason for switching from RightAWS to AWS::S3, too, especially as AWS::S3 doesn't support European S3 buckets a far as I know.

@StanBright
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's interesting to me too. I think that RightAWS has good support and good implementation. Moreover in our project we are using European S3 buckets too and it'll be a problem if it's not supported.

@cpytel
Copy link
Member

@cpytel cpytel commented on ac86bb9 Aug 5, 2009

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michaeldwan
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chad, thanks for the link to the discussion.

Jonathan, Thanks for this change. I agree that RightAWS's slowness and complexity was more trouble than it was worth. Great job!

@khelal
Copy link

@khelal khelal commented on ac86bb9 Aug 6, 2009

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is bad news. We were just updating our app and migrating from attachment_fu / AWS::S3 to paperclip because we were getting sick and tired of the problems with european buckets :(

@rob-at-thewebfellas
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've managed to get around the European problem by monkey patching using these changes and by patching the Paperclip S3 storage module to pass :server => "#{@bucket}.s3.amazonaws.com" when it establishes a connection. It's not pretty but it'll do till I get chance to work on a nicer solution :)

@luislavena
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my case we use lot of buckets located in EU, so this could be a problem.

Anyhow, I believe this can be easily solve as pointed by Rob from TheWebFellas. The exact same issue we have with s3sync and such, we need to use subdomains instead of subfolders.

Putting that change in place would sort out these issues.

@khelal
Copy link

@khelal khelal commented on ac86bb9 Aug 8, 2009

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well this commit (http://github.com/vladr/aws-s3/commit/a13504ba581496eab253583f38d26aa475949b53) actually fixes the problem with S3 buckets, but it hasn't been rolled into the main branch (yet) as it has no associated tests....

@khelal
Copy link

@khelal khelal commented on ac86bb9 Aug 8, 2009

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to clarify my last post, we have been using this patched version of aws-s3 along with attachment_fu in our production system (vladr is actually our local guru) and it's been working flawlessly. We never got around to actually writing test cases to it, which is why the original author of aws-s3 refused to merge it in the main branch (with reason).

@chrisanderton
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's the same commit as posted to earlier..

@donaldpiret
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this ever going to get fixed? Not working with european buckets is kinda killing paperclip for us..

@masterkain
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still no fix, I tried the patch suggested above but I get the same endpoint error.

@khelal
Copy link

@khelal khelal commented on ac86bb9 Nov 17, 2009

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are actually also having some issues with European buckets when working on OSX, even tho it's working fine on Ubuntu 8.04. Looking into it.. Sure would help if the official aws:s3 gem would get updated once in a while, especially now that it's pretty much the only lib that can be used...

@fesplugas
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We made a fork of Marcel's aws-s3 and we have it on production. It's available here. http://github.com/fesplugas/aws-s3

@khelal
Copy link

@khelal khelal commented on ac86bb9 Nov 17, 2009

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick reply! unfortunately, it doesn't work 100%. We specify our buckets in the server field (xxx.yyy.zzz.s3.amazonaws.com) and while we can indeed connect, download and upload files that way, some commands don't seem to work. At least Bucket.list doesn't seem to work as I get an error:

AWS::S3::SignatureDoesNotMatch: The request signature we calculated does not match the signature you provided. Check your key and signing method.
from /Users/khelal/Sites/monaqasat.com/1.7/vendor/plugins/aws-s3/lib/aws/s3/error.rb:38:in raise' from /Users/khelal/Sites/monaqasat.com/1.7/vendor/plugins/aws-s3/lib/aws/s3/base.rb:75:inrequest'
from /Users/khelal/Sites/monaqasat.com/1.7/vendor/plugins/aws-s3/lib/aws/s3/base.rb:101:in get' from /Users/khelal/Sites/monaqasat.com/1.7/vendor/plugins/aws-s3/lib/aws/s3/service.rb:21:inunmemoized_buckets_1258454875'
from /Users/khelal/Sites/monaqasat.com/1.7/vendor/plugins/aws-s3/lib/aws/s3/extensions.rb:177:in send' from /Users/khelal/Sites/monaqasat.com/1.7/vendor/plugins/aws-s3/lib/aws/s3/extensions.rb:177:inbuckets'
from /Users/khelal/Sites/monaqasat.com/1.7/vendor/plugins/aws-s3/lib/aws/s3/extensions.rb:146:in expirable_memoize' from /Users/khelal/Sites/monaqasat.com/1.7/vendor/plugins/aws-s3/lib/aws/s3/extensions.rb:176:inbuckets'
from /Users/khelal/Sites/monaqasat.com/1.7/vendor/plugins/aws-s3/lib/aws/s3/bucket.rb:196:in `list'
from (irb):11

@masterkain
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@khelal
Copy link

@khelal khelal commented on ac86bb9 Nov 23, 2009

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AWS is based on right_aws, which is not supported anymore by paperclip. This is thus not a viable solution. Can't believe this is still ongoing months after the original announcement by thoughbot :)

@alxgsv
Copy link
Contributor

@alxgsv alxgsv commented on ac86bb9 Dec 21, 2009

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey! This problem prevents all European folks from usage of paperclip... I am sad about it :-(

@jimneath
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still no fix for this?

@fesplugas
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend you using the S3 gem from qoobaa. It provides an S3 backend for paperclip and works perfect for all bucket regions.

http://github.com/qoobaa/s3/blob/master/extra/s3_paperclip.rb

@donaldpiret
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seconded on the S3 gem, very simple to get up and running and works great in production on both European and American buckets

@rob-at-thewebfellas
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a patch with updated tests to make the change to the s3 gem, i'll upload it later so you can give it a try.

@rob-at-thewebfellas
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've created a new issue with patch here for your enjoyment :)

@steveh
Copy link

@steveh steveh commented on ac86bb9 Jan 10, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's how I solved this:

require "aws/s3"

AWS::S3::DEFAULT_HOST = "s3-us-west-1.amazonaws.com"

Please sign in to comment.