Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A migration guide from Paperclip to ActiveStorage #2568

Merged
merged 3 commits into from Mar 9, 2018

Conversation

@mike-burns
Copy link
Member

@mike-burns mike-burns commented Mar 2, 2018

A start at an introductory migration path for moving from Paperclip to
ActiveStorage.

ActiveStorage is coming in Rails 5.2 (April, perhaps) and we want to
help people move from Paperclip to it. Having a file upload strategy
baked into Rails itself will allow us all to focus our efforts and
follow a common goal.

This migration guide is rudimentary at best: it works for a User
object with one avatar attachment. It is provided as a framework to
start the documentation.

I have tried this in a dummy app.

@mike-burns
Copy link
Member Author

@mike-burns mike-burns commented Mar 2, 2018

Ping @geoffharcourt -- thanks for your support when I started this document!

Copy link
Contributor

@derekprior derekprior left a comment

Very thorough. Nice work!

# remote
# url = attachment.url
Digest::MD5.base64digest(open(url).read)

This comment has been minimized.

@derekprior

derekprior Mar 2, 2018
Contributor

This is probably ok since theoretically you had good control over the URL that was written to your database, but Kernel.open makes me really nervous. So I just wanted to share my nerves so you could probably assuage them. I'm not sure of any specific attack vector here, but OpenURI can really do damage.

This comment has been minimized.

@mike-burns

mike-burns Mar 2, 2018
Author Member

No, you're right. Maybe this method would be safer either explicitly expecting a file path or explicitly expected a HTTP URL.

If it's a file path, use File.read instead of open(url).read.

If it's a URL use Net::HTTP.get(URI(url).host, URI(url).path).

Does that seem right, or should the Net::HTTP code to be fancier?

This comment has been minimized.

@derekprior

derekprior Mar 2, 2018
Contributor

I never know how to do anything in Net:HTTP, but yeah. I think that approach is good.

In ActiveStorage it looks like this:

```ruby
image_tag @user.avatar.variant(resize: "250x250")

This comment has been minimized.

@derekprior

derekprior Mar 2, 2018
Contributor

is it worth calling out that this may require benefit from preloading avatar (if, for example, you're rendering many user avatars)? My understanding is that paperclip has the necessary columns for avatar on its User model, while the major difference with ActiveStorage is that it's a polymorphic relationship? Is this correct? Out of scope for this doc, maybe?

This comment has been minimized.

@mike-burns

mike-burns Mar 2, 2018
Author Member

Both a fair point and out of scope for this doc, but maybe it's worth mentioning in this document because we will need to watch for that when converting.

So @user = User.find(params[:id]) becomes @user = User.find(params[:id]).include(:funny_pictures), for a has_many_attached :funny_pictures. That's where we'll run into the most problems, right -- a has-many relationship?

This comment has been minimized.

@derekprior

derekprior Mar 2, 2018
Contributor

I was also thinking about this particular case (for example, registering the avatar of each commenter in a list of comments on a post). Maybe a heads up on this could go in the currently-empty controllers section?

file = record.send(name)
file.respond_to?(:variant) && file.respond_to?(:attach)
end
end

This comment has been minimized.

@geoffharcourt

geoffharcourt Mar 3, 2018
Contributor

You went the extra mile with this matcher.

mike-burns added 3 commits Mar 2, 2018
A start at an introductory migration path for moving from Paperclip to
[ActiveStorage].

ActiveStorage is coming in Rails 5.2 (April, perhaps) and we want to
help people move from Paperclip to it. Having a file upload strategy
baked into Rails itself will allow us all to focus our efforts and
follow a common goal.

This migration guide is rudimentary at best: it works for a `User`
object with one `avatar` attachment. It is provided as a framework to
start the documentation.

I have tried this in a dummy app.

[ActiveStorage]: http://edgeguides.rubyonrails.org/active_storage_overview.html
- open-uri is a security concern, so let's not recommend it. Separate
one-liners based on whether they're doing local or remote storage.
- Mention the performance impact of the separate table. While migrating
they will want to consider each use case for n+1 loads.

Thanks, Derek!
@mike-burns mike-burns force-pushed the migrate-to-active-storage branch from 3e50d0d to be23fbf Mar 9, 2018
@mike-burns mike-burns merged commit be23fbf into master Mar 9, 2018
1 of 3 checks passed
1 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
hound No violations found. Woof!
@mike-burns mike-burns deleted the migrate-to-active-storage branch Mar 9, 2018
@sankage
Copy link

@sankage sankage commented Apr 16, 2018

I just tried running the migration using pg (1.0.0) and psql (PostgreSQL) 10.3 and get a bunch of errors from prepared statements.

  1. First error:

    ArgumentError: wrong number of arguments (given 1, expected 2..3)
    [...]_convert_to_active_storage.rb:14:in `prepare'    
    

    I added names to the prepared statements:

    ActiveRecord::Base.connection.raw_connection.prepare('active_storage_blob_statement', <<-SQL)
    
    ActiveRecord::Base.connection.raw_connection.prepare('active_storage_attachment_statement', <<-SQL)
    
  2. And now the error is:

    ERROR:  syntax error at or near ","
    LINE 4:       ) VALUES (?, ?, ?, '{}', ?, ?, ?)
                             ^
    [...]_convert_to_active_storage.rb:14:in `prepare'
    

    I added numbered placeholders:

    ) VALUES ($1, $2, $3, '{}', $4, $5, $6)
    
    ) VALUES ($1, $2, $3, #{get_blob_id}, $4)
    
  3. I then get the error:

    NoMethodError: undefined method `execute' for #<PG::Result>                                                                                
    [...]_convert_to_active_storage.rb:39:in `block (4 levels) in up'   
    

    I switch the execute command with exec_prepared:

    ActiveRecord::Base.connection.raw_connection.
      exec_prepared("active_storage_blob_statement", [
        key(instance, attachment),
        instance.send("#{attachment}_file_name"),
        instance.send("#{attachment}_content_type"),
        instance.send("#{attachment}_file_size"),
        checksum(instance.send(attachment)),
        instance.updated_at.iso8601
      ])
    
    ActiveRecord::Base.connection.raw_connection.
      exec_prepared("active_storage_attachment_statement", [
        attachment, model.name, instance.id, instance.updated_at.iso8601
      ])
  4. Now the error:

    NoMethodError: undefined method `close' for #<PG::Result>
    [...]_convert_to_active_storage.rb:60:in `up'  
    

    Fixed by removing the 2 close calls:

    active_storage_attachment_statement.close
    active_storage_blob_statement.close

After making these changes I was able to get the migration to complete.

@mike-burns
Copy link
Member Author

@mike-burns mike-burns commented Apr 16, 2018

Wonderful report, thank you @sankage ! I'll look into this on Friday.

@mike-burns
Copy link
Member Author

@mike-burns mike-burns commented May 1, 2018

@sidraval - I won't have time this coming Friday, but if you do: consider the above when looking at #2596.

@GBH
Copy link

@GBH GBH commented May 2, 2018

Any reason that Github issues got removed? I'm sure that there are many users that still need access there to debug their existing installs. Not all Rails apps are going to be upgraded to Rails 5.2 anytime soon (or ever).

@mike-burns
Copy link
Member Author

@mike-burns mike-burns commented Sep 28, 2018

Thanks @sankage , I've opened a PR: #2613 .

mike-burns added a commit that referenced this pull request Oct 26, 2018
This is a combination of [a comment from @sankage] and [a gist by
@colinpetruno]. Many thanks to them for sharing their discoveries.

[a comment from @sankage]: #2568 (comment)
[a gist by @colinpetruno]: https://gist.github.com/colinpetruno/037de4fafa4cff695b1d7905cd6fd7c2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants