ActiveStorage support #2974

elia · 2018-11-23T18:23:28Z

📝 This is an attempt to resurrect the great work started by @swcraig (here and here) and building on the gallery API introduced in #2337

The old approach

After attempting to stay within the bounds of the old attempt I realized there was a problem with the fact that the gallery implementation wasn't really completed (several places still access images directly instead of going through the new API).

The new approach

The new approach is not to create an extension but rather extract all the Paperclip stuff to a concern and provide an alternative implementation based on ActiveStorage. That turned out to be way more clean and required way less code.

Migrating the assets and Paperclip/AS bridge

The nice thing is that it's also possible to offer a better migration path thanks to ActiveStorage mirroring. The idea is to create an ActiveStorage service based on the existing Paperclip storage that will serve the images from their current locations (e.g. S3 with the Paperclip defined folder structure) and have at the same time an AS mirror that points to the AS-S3 service. At this point new uploads would go to both service and there's plenty of time for a progressive migration of the assets base until the old service can be dismissed.

This will be part of a different PR, so this one won't be blocked and new installation can start with AS as soon as possible

🛣 Roadmap

move image/paperclip stuff to a concern
implement an alternative image/activestorage concern
allow to switch image concern via an app-configuration
move taxon/paperclip stuff to a concern
implement an alternative taxon/activestorage concern
allow to switch taxon concern via an app-configuration
evaluate extracting common code between the concerns / refactoring

Old roadmap (discarded)

~~setup the extension inside the repo making it a dependency of the main gem~~
~~move paperclip-related code piece by piece to the extension~~
~~add an alternative ActiveStorage-based gallery implementation~~
~~(maybe) add a specialized ActiveStorage service that can work with paperclip assets, to be as mirror service~~
~~extract the extension to a different repo~~

Reference links

solidus_paperclip/lib/solidus_paperclip/gallery.rb

solidus_paperclip/lib/solidus_paperclip/factories.rb

solidus_paperclip/lib/solidus_paperclip/version.rb

solidus_paperclip/spec/spec_helper.rb

solidus_paperclip/lib/solidus_paperclip.rb

solidus_paperclip/lib/generators/solidus_paperclip/install/install_generator.rb

solidus_paperclip/config/routes.rb

solidus_paperclip/Rakefile

solidus_paperclip/Gemfile

swcraig · 2018-11-23T19:05:11Z

Hey @elia, thank you for picking this up! I don't have the bandwidth right now to get this over the finish line; it's much appreciated to have you take a go at it. Feel free to send me an email or @ me if you have any questions, I'll try my best to help out.

solidus_paperclip/app/models/solidus_paperclip/gallery.rb

solidus_paperclip/lib/solidus_paperclip/engine.rb

core/lib/active_storage/service/paperclip_storage.rb

core/app/models/spree/image/active_storage_attachment.rb

frontend/app/controllers/spree/store_controller.rb

backend/app/controllers/spree/admin/base_controller.rb

core/app/models/spree/image/active_storage_attachment.rb

elia · 2019-05-13T23:39:53Z

Be great to rebase this branch with Master so people could use it. I'll fork it in the meantime

@dankmitchell sorry for the delay, I rebased the branch, I hope to be back working on it by next Friday

elia · 2019-05-27T16:01:38Z

What is outstanding on this PR?
I’ve got a new project that is built and ready for products. I would love to not use Paperclip :)

My bad for not posting this here in addition to the solidus slack channel

@dankmitchell there's a rather serious problem with how ActiveStorage works, that basically makes it almost unusable for any serious ecommerce work, alas.

We uncovered a couple weeks ago about using AS for public-facing sites, i.e. that the generated image URLs are temporary and have a 5min expiration time by default. That's something that may be ok for a very small store, but once the RPM start to rocket and caching+performance becomes necessary it's clearly a no-go.

There's an open PR that's sitting in the Rails repo that missed the Rails 5.2 window, which would solve this issue by adding an internal method that will produce public links for assets. But, as said, is not yet merged.

rails/rails#34581

We evaluated the option of monkey-patching AS to basically get the PR changes in, but that seems bad, and the change has already been approved by rails-core, so it's just a matter of time.

Meanwhile, the only other thing we can do is to lobby for the PR to be merged as soon as possible, ideally before Rails 6 gets to the release-candidate stage.

Update: we're going to put back paperclip as the default configuration for new apps, after that this PR will most likely be merged! 🎉

dankmitchell · 2019-05-27T16:04:08Z

That’s a great decision @elia 💯

It's good practice not to define methods on Object, even if the method name was unique enough not to be in conflict with anything. Also changed the constant to reflect the actual content (Rails 5.2+) rather than a consequence of it, that allowed reusing it in more places.

ActiveStorage::Current.host must be set before every action dealing with AS URLs.

Only extracted the common stuff, leaving specific implementations in their specific modules.

Given the unresolved issues in the ActiveStorage implementation we keep Paperclip as the default strategy for new apps.

E.g. `.attachment(:mini)`.

This is necessary for `url_for` and other helpers to work.

kennyadsl · 2019-06-18T13:01:46Z

I think that we cannot ship ActiveStorage support with the Rails issue mentioned above but I'd leave the possibility to use it for anyone that wants to switch at this stage.

My suggestion is to:

leave the adapter logic and paperclip as an adapter in core
move ActiveStorage code into an extension
merge this PR and release it with v2.9 so we can have some feedback about ActiveStorage real-world usage via the new extension.

What do you think?

tvdeyen · 2019-06-18T16:43:18Z

This sounds like a good plan 👍

pedantic-git · 2019-11-25T16:39:54Z

Hi @elia and everyone working on this - thanks for your help!

Question: If AS support isn't coming any time soon, is anyone working on bumping Paperclip to the latest version? The version of Paperclip that Solidus depends on is still one that needs aws-sdk 2.x for S3 support which is very out of date and I fear may not be patched if there are ever security advisories against it.

kennyadsl · 2019-11-26T09:59:29Z

@pedantic-git Hey there, thanks for pointing out this thing.

Honestly, I'm not sure it's worth updating paperclip to a major version, now that ActiveStorage has support for public URLs merged, and will be released with Rails 6.1. We "just" have to create the new adapter.

Also, aws-sdk 2.x looks actually well maintained on RubyGems, while the 3.x versions are not having new releases in a while. Or am I missing something?

tvdeyen · 2019-11-26T11:46:05Z

Question: If AS support isn't coming any time soon, is anyone working on bumping Paperclip to the latest version? The version of Paperclip that Solidus depends on is still one that needs aws-sdk 2.x for S3 support which is very out of date and I fear may not be patched if there are ever security advisories against it.

@pedantic-git this should be resolved by #3438

kennyadsl · 2019-11-26T12:42:44Z

Ah sorry, I think we were kinda locked on some old Paperclip version, if it's just about relaxing dependencies, I think it's something we should definitely do!

pedantic-git · 2019-11-27T08:59:19Z

@kennyadsl Looking forward to the merged version! I hadn't noticed that about 2.x still receiving updates, which is great news and puts my mind at ease for now.

elia · 2020-03-06T09:34:56Z

Closing, the work will keep going in #3501, I'll leave the branch there in case anyone is relying on it, thanks for picking up the slack @filippoliverani!

houndci-bot reviewed Nov 23, 2018

View reviewed changes

elia force-pushed the elia/paperclip-extraction branch from 7a0b440 to c93822c Compare November 23, 2018 18:27

kennyadsl added this to the 3.0 milestone Dec 6, 2018

ericsaupe mentioned this pull request Dec 19, 2018

Move Paperclip into an extension #2328

Closed

elia force-pushed the elia/paperclip-extraction branch from 0eb9e0d to fe0e616 Compare January 11, 2019 16:14

houndci-bot reviewed Jan 11, 2019

View reviewed changes

solidus_paperclip/app/models/solidus_paperclip/gallery.rb Outdated Show resolved Hide resolved

solidus_paperclip/lib/solidus_paperclip/engine.rb Outdated Show resolved Hide resolved

elia force-pushed the elia/paperclip-extraction branch from fe0e616 to b710149 Compare January 24, 2019 21:38

houndci-bot reviewed Jan 24, 2019

View reviewed changes

elia force-pushed the elia/paperclip-extraction branch 2 times, most recently from ff90227 to 7a375c8 Compare January 24, 2019 21:52

houndci-bot reviewed Jan 24, 2019

View reviewed changes

core/app/models/spree/image/active_storage_attachment.rb Outdated Show resolved Hide resolved

core/app/models/spree/image/active_storage_attachment.rb Outdated Show resolved Hide resolved

elia force-pushed the elia/paperclip-extraction branch 2 times, most recently from b17e405 to 3928f90 Compare January 24, 2019 22:14

houndci-bot reviewed Jan 24, 2019

View reviewed changes

frontend/app/controllers/spree/store_controller.rb Outdated Show resolved Hide resolved

backend/app/controllers/spree/admin/base_controller.rb Outdated Show resolved Hide resolved

elia force-pushed the elia/paperclip-extraction branch 3 times, most recently from 0183b63 to ba75f7f Compare January 24, 2019 22:50

swcraig mentioned this pull request Jan 29, 2019

Paperclip closing, time to move to ActiveStorage? #2725

Closed

elia changed the title ~~Paperclip extraction~~ ActiveStorage support Feb 1, 2019

elia force-pushed the elia/paperclip-extraction branch from ba75f7f to 2b3bfc5 Compare February 1, 2019 14:05

houndci-bot reviewed Feb 1, 2019

View reviewed changes

core/app/models/spree/image/active_storage_attachment.rb Outdated Show resolved Hide resolved

elia force-pushed the elia/paperclip-extraction branch 4 times, most recently from 5eccbe5 to cce8798 Compare February 1, 2019 22:46

ericsaupe mentioned this pull request Feb 5, 2019

RFC: Support Rails 6 #3053

Closed

elia force-pushed the elia/paperclip-extraction branch 3 times, most recently from d214d9b to b131da3 Compare February 8, 2019 15:48

elia added 10 commits May 27, 2019 22:14

Extract Spree::Image Paperclip implementation

1153395

Provide Spree::Image ActiveStorage implementation

808d93b

ActiveStorage::Current.host must be set before every action dealing with AS URLs.

Extract Spree::Taxon Paperclip implementation

1789677

Provide Spree::Taxon ActiveStorage implementation

0b4d540

Extract common ActiveStorageAttachment code

fe995c4

Only extracted the common stuff, leaving specific implementations in their specific modules.

Set Paperclip as the default storage strategy

f5db2d1

Given the unresolved issues in the ActiveStorage implementation we keep Paperclip as the default strategy for new apps.

Add a list of possible gotchas for ActiveStorage

2435b84

Support deprecated paperclip API

47b377e

E.g. `.attachment(:mini)`.

Add route resolvers for ActiveStorage models

3032e4a

This is necessary for `url_for` and other helpers to work.

elia force-pushed the elia/paperclip-extraction branch from 8fb9ae5 to 3032e4a Compare May 27, 2019 21:20

elia mentioned this pull request Jun 21, 2019

Attachment adapters #3237

Merged

4 tasks

kennyadsl mentioned this pull request Nov 26, 2019

Create an ActiveStorage adapter nebulab/solidus#32

Closed

6 tasks

swcraig mentioned this pull request Nov 28, 2019

[RFC] Move Paperclip into an extension #2324

Closed

filippoliverani mentioned this pull request Jan 31, 2020

Add ActiveStorage adapter #3501

Merged

3 tasks

elia closed this Mar 6, 2020

kennyadsl mentioned this pull request Jan 8, 2021

Enable ActiveStorage specs against Rails 6.1 #3886

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ActiveStorage support #2974

ActiveStorage support #2974

elia commented Nov 23, 2018 •

edited

Loading

swcraig commented Nov 23, 2018

elia commented May 13, 2019

elia commented May 27, 2019

dankmitchell commented May 27, 2019

kennyadsl commented Jun 18, 2019

tvdeyen commented Jun 18, 2019 via email

pedantic-git commented Nov 25, 2019

kennyadsl commented Nov 26, 2019

tvdeyen commented Nov 26, 2019

kennyadsl commented Nov 26, 2019

pedantic-git commented Nov 27, 2019

elia commented Mar 6, 2020

ActiveStorage support #2974

ActiveStorage support #2974

Conversation

elia commented Nov 23, 2018 • edited Loading

The old approach

The new approach

Migrating the assets and Paperclip/AS bridge

🛣 Roadmap

Old roadmap (discarded)

Reference links

swcraig commented Nov 23, 2018

elia commented May 13, 2019

elia commented May 27, 2019

dankmitchell commented May 27, 2019

kennyadsl commented Jun 18, 2019

tvdeyen commented Jun 18, 2019 via email

pedantic-git commented Nov 25, 2019

kennyadsl commented Nov 26, 2019

tvdeyen commented Nov 26, 2019

kennyadsl commented Nov 26, 2019

pedantic-git commented Nov 27, 2019

elia commented Mar 6, 2020

elia commented Nov 23, 2018 •

edited

Loading