Allow configure services for individual attachments #34935

DmitryTsepelev · 2019-01-14T20:07:28Z

Idea is taken from this comment

This change allows to configure custom services for individual attachments in the following way:

class User < ActiveRecord::Base
  has_one_attached :avatar, service: :s3
end

If specified service is not properly configured - has_one_attached would throw an error. Since service_name is stored inside active_storage_blobs table it's also possible that misconfiguration will appear in the realtime - for such cases, there is a special configuration option Rails.application.configuration.active_storage.service_misconfiguration_behavior, which can take the following values: :log (default), :fallback_to_default and :raise.

In order to migrate existing apps users would have to re-run rails active_storage:install and rails db:migrate to add a new migration, suggestions are welcome :)

On the side note, I've moved require "database/setup" to the beginning of the test helper to make it look more like a real boot process - migrations are executed before app starts.

palkan

In order to migrate existing apps users would have to re-run rails active_storage:install and rails db:migrate to add a new migration, suggestions are welcome :)

Can we make the presence of service_name column optional?

Right now we check for it in the macroses (https://github.com/rails/rails/pull/34935/files#diff-fc054319f93d887d4427a7a728670ba9R159), but we still rely on service_name column in Blob#service.

What about extracting this feature check (say, ActiveStorage.per_blob_service_supported?) and re-use in Blob#service to avoid calling #service_name?

palkan · 2019-01-14T21:36:36Z

activestorage/app/models/active_storage/blob.rb

+        configs = Rails.configuration.active_storage.service_configurations
+
+        begin
+          ActiveStorage::Service.configure service_name, configs


This would build a new service instance for each blob–we need some kind of memoization here (ServiceRegistry.fetch?). That could also allow us to move exception handling logic there and make #service method much more readable:

def service ActiveStorage::ServiceRegistry.fetch(service_name, self.class.service) end

I've added memoization, but it looks hard to move exception handling to the same place cause error messages are different enough

activestorage/db/migrate/20190112182829_add_service_name_to_active_storage_blobs.rb

bogdanvlviv · 2019-01-17T16:45:55Z

guides/source/active_storage_overview.md

+`RuntimeError`.
+
+NOTE: if you've initialized ActiveStorage before this feature was shipped, Rails would
+ask you to generate and execute one more migration (`rails active_storage:install && rails db:migrate`).


We can remove this NOTE when following the approach described in #34935 (comment)

activestorage/db/update_migrate/20190112182829_add_service_name_to_active_storage_blobs.rb

activestorage/app/models/active_storage/blob.rb

activestorage/lib/active_storage/engine.rb

DmitryTsepelev · 2019-01-21T20:36:32Z

r? @georgeclaghorn

abhchand · 2019-02-18T19:23:09Z

Hey all, just came across this PR while browsing other activestorage PRs.

I just recently opened #35290 that adds the ability to configure attachments.

I don't want to get ahead of myself and assume that change will be accepted. But if it is accepted I wanted to suggest that we could use that same mechanism here when configuring the service.

class User < ApplicationRecord
  has_one_attached :avatar do |attachment|
    attachment.service :s3
  end
end

Just a thought. Thanks!

DmitryTsepelev · 2019-02-18T20:11:37Z

Hi @abhchand, thanks for letting me know! I would add it to my PR if yours will be accepted earlier

iggant · 2019-04-02T20:36:18Z

There is no activity on this PR, but will be nice to have this feature in Rails 6.0
@DmitryTsepelev can you please resolve conflicts and maybe there will be any chance to review this again?

DmitryTsepelev · 2019-04-02T20:45:22Z

Hi @iggant, thanks for the support, I'd like to have it merged too :) The only conflict is in the changelog so it shouldn't be a blocker for a review. I believe core team members are busy polishing release candidates

iggant · 2019-04-04T04:22:29Z

@bogdanvlviv Can you please review this PR again, will be very nice to have this feature in new major Rails release

lordjavac · 2019-04-05T19:13:21Z

Lack of this feature is the one thing preventing me from moving to ActiveStorage. I'd love to see this get implemented soon. Thanks for the hard work!

DmitryTsepelev · 2019-04-11T14:50:27Z

Hi @georgeclaghorn, is there a chance this PR can fit into 6.0.0? 🙂

georgeclaghorn · 2019-04-11T14:52:29Z

I’m afraid it’s not going to make 6.0. Sorry.

activestorage/test/models/blob_test.rb

Hareramrai · 2020-03-12T14:32:41Z

Hi @DmitryTsepelev, From which version of rails, we can use this?

DmitryTsepelev · 2020-03-12T14:40:16Z

Hi @Hareramrai, it's still in the main CHANGELOG so I guess it will be released as part of 6.1.

dnalbach · 2020-04-28T16:37:00Z

@DmitryTsepelev does your merged PR allow for different S3 bucket names to be usable per attachment, or just the toggle of storage types? Is a service "s3" or is it granular down to the bucket name?

DmitryTsepelev · 2020-04-28T19:13:29Z

@dnalbach The line has_one_attached :avatar, service: :s3 forces ActiveStorage to use the service called s3 from storage.yml, so you can use any options (bucket, region, etc.) that are available for the specified adapter. For instance, if you have two buckets avatars and cover_photos you can do the following:

# storage.yml
s3_avatars:
  service: S3
  bucket: "photos"
  access_key_id: "..."
  secret_access_key: "..."

s3_cover_photos:
  service: S3
  bucket: "cover_photos"
  access_key_id: "..."
  secret_access_key: "..."

class User < ApplicationRecord
  has_one_attached :avatar, service: :s3_avatars
  has_one_attached :cover_photo, service: :s3_cover_photos
end

dnalbach · 2020-04-29T21:29:33Z

Thats awsome @DmitryTsepelev , exactly what I was hoping for. This is a BigDeal in the cloud world I live in. Desperately needed. Look forward to seeing your contribution released!

jules2689 · 2020-08-24T03:28:35Z

Hi! This is a great feature and I really appreciate the work done on it! I've opted to start using master on my app to take advantage of this new feature.

One thing I did notice is that the ergonomics of the system becomes a bit awkward with testing environments. Originally I had:

class Model < ApplicationModel
 has_one_attached :thing, service: :s3
end

But my tests started trying to upload to S3 😅 It was a simple fix for now:

class Model < ApplicationModel
 has_one_attached :thing, service: (Rails.env.test? ? :local : :s3)
end

But I was wondering if you had thought of some better ergonomics for this given that a ternary statement for every override could become quite awkward. Other things I've considered are:

Override the :s3 storage config in the YAML file with if/else ERB blocks (This feels awkward too)
Service class that sets constants based on environment (e.g. Service::THING_SERVICE could point to :s3 in dev/prod and :local in test). This seems ok, but it's not a pattern that I see a ton in rails

Again, great feature - would love to hear if anyone had any thoughts on a better solution here 🙇

DmitryTsepelev · 2020-08-24T06:23:51Z

Hi @jules2689, I'm going to recommend you a trick that will work for your specific case only (s3/disk pair), but it will probably will save a lot of time. Have you heard about minio? It's an S3–compatible storage, you can run it locally and keep using "S3" both in prod and dev

palkan · 2020-08-24T08:51:54Z

Override the :s3 storage config in the YAML file with if/else ERB blocks (This feels awkward too)

A better option would be to support per-environment configuration in storage.yml and move from service-oriented to context-oriented semantics, e.g.:

# storage.yml
production:
  images:
    service: s3
  documents:
    service: gcloud

test:
  images:
    service: disk
  documents:
    service: disk

And then you just have:

# Note that name "storage" would match better than "service" here
has_one_attached :avatar, storage: :images

IMO, that's the Rails Way of dealing with this problem.

swissonelabs · 2020-10-12T13:50:36Z

I am upgrading an existing app to 6.1.0.alpha

Why doesn't rails active_storage:install rails db:migrate update my tables?

because of that I don't have service_name in my storage_blob and facing errors.

swissonelabs · 2020-10-12T14:05:41Z

rails active_storage:update
worked!

ghiculescu · 2020-12-10T18:40:27Z

rails active_storage:update
worked!

Should this be in the upgrade guide? See https://discuss.rubyonrails.org/t/problem-with-activestorage-blob-on-upgrade-from-6-0-3-4-to-6-1

gokninski · 2021-06-23T00:25:48Z

Hi! This is a great feature and I really appreciate the work done on it! I've opted to start using master on my app to take advantage of this new feature.

One thing I did notice is that the ergonomics of the system becomes a bit awkward with testing environments. Originally I had:
class Model < ApplicationModel
 has_one_attached :thing, service: :s3
end
But my tests started trying to upload to S3 😅 It was a simple fix for now:
class Model < ApplicationModel
 has_one_attached :thing, service: (Rails.env.test? ? :local : :s3)
end
But I was wondering if you had thought of some better ergonomics for this given that a ternary statement for every override could become quite awkward. Other things I've considered are:

Override the :s3 storage config in the YAML file with if/else ERB blocks (This feels awkward too)

Service class that sets constants based on environment (e.g. Service::THING_SERVICE could point to :s3 in dev/prod and :local in test). This seems ok, but it's not a pattern that I see a ton in rails

Again, great feature - would love to hear if anyone had any thoughts on a better solution here 🙇

I've done this using custom configuration in the environment config file (https://guides.rubyonrails.org/configuring.html#custom-configuration)

…igration In rails/rails#33419, we added this migration to properly upgrade Active Storage from 5.2 to 6.0 On Rails 6.1 `rails app:update` shouldn't add this migration to users' app. Note that, I've left implementation that makes `rails app:update` to generate migrations for users' app that are in `activestorage/db/update_migrate/` because we are likely to need it e.g.: rails/rails#34935, rails/rails#36835.

rails-bot bot added activestorage docs labels Jan 14, 2019

DmitryTsepelev force-pushed the configure-store-per-attachment branch 2 times, most recently from c9ea062 to b3d16cc Compare January 14, 2019 21:28

rails-bot bot added the actionmailbox label Jan 14, 2019

palkan reviewed Jan 14, 2019

View reviewed changes

DmitryTsepelev force-pushed the configure-store-per-attachment branch 2 times, most recently from 398c000 to f5d5fa6 Compare January 15, 2019 15:14

bogdanvlviv reviewed Jan 17, 2019

View reviewed changes

activestorage/db/migrate/20190112182829_add_service_name_to_active_storage_blobs.rb Outdated Show resolved Hide resolved

bogdanvlviv suggested changes Jan 17, 2019

View reviewed changes

DmitryTsepelev force-pushed the configure-store-per-attachment branch 4 times, most recently from 717ee50 to b19d802 Compare January 18, 2019 10:49

rails-bot bot added the actiontext label Jan 18, 2019

bogdanvlviv reviewed Jan 19, 2019

View reviewed changes

DmitryTsepelev force-pushed the configure-store-per-attachment branch 3 times, most recently from 43b8441 to e40eb8f Compare January 20, 2019 09:59

georgeclaghorn self-assigned this Jan 22, 2019

allomov mentioned this pull request Jan 31, 2019

ActiveStorage : support multiple storages/buckets in the same app #34509

Closed

gmcgibbon reviewed Oct 1, 2019

View reviewed changes

activestorage/test/models/blob_test.rb Outdated Show resolved Hide resolved

Allow configure services for individual attachments

e7f798c

DmitryTsepelev force-pushed the configure-store-per-attachment branch from e3471c0 to e7f798c Compare October 1, 2019 18:24

gmcgibbon merged commit 84d236d into rails:master Oct 1, 2019

DmitryTsepelev mentioned this pull request Oct 2, 2019

Adds support for defined variants and generic support for attachment configuration #35290

Closed

5 tasks

gmcgibbon mentioned this pull request Oct 2, 2019

Make Active Storage services aware of names #37346

Merged

erluti mentioned this pull request Apr 3, 2020

Evaluate feasibility of moving from carrierwave to ActiveStorage department-of-veterans-affairs/va.gov-team#6768

Closed

scottjg mentioned this pull request Apr 13, 2020

ActiveStorage direct uploads can't specify a service_name? #38940

Open

DmitryTsepelev mentioned this pull request Apr 15, 2020

Pass service_name param to DirectUploadsController #38957

Merged

composerinteralia mentioned this pull request Sep 17, 2020

Active Storage 6.1 doesn't use the local disk service in development when overriding the default service #40248

Closed

michelson mentioned this pull request Dec 10, 2020

ActiveStorage upgrade command #40791

Merged

dorianmariecom mentioned this pull request Dec 29, 2020

undefined method 'service_name' for ActiveStorage::Blob after upgrading to rails 6.1.0 #40970

Closed

clwy-cn mentioned this pull request Jan 26, 2021

direct_upload在rails 6.1下有问题 huacnlee/activestorage-aliyun#70

Closed

This was referenced Mar 27, 2021

Set rails limit version to less than 6.1.0 comfy/comfortable-mexican-sofa#928

Open

Rails 6.1: support ActiveStorage::Blob#service_name comfy/comfortable-mexican-sofa#929

Open

drewbaumann mentioned this pull request Aug 18, 2021

fix(activestorage): Allows configurable service on attachment motor-admin/motor-admin-rails#38

Merged

gbp mentioned this pull request Nov 24, 2021

Install and configure Active Storage mysociety/alaveteli#6530

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow configure services for individual attachments #34935

Allow configure services for individual attachments #34935

DmitryTsepelev commented Jan 14, 2019 •

edited

Loading

palkan left a comment

palkan Jan 14, 2019

DmitryTsepelev Jan 15, 2019

bogdanvlviv Jan 17, 2019 •

edited

Loading

DmitryTsepelev Jan 18, 2019

DmitryTsepelev commented Jan 21, 2019

abhchand commented Feb 18, 2019

DmitryTsepelev commented Feb 18, 2019

iggant commented Apr 2, 2019

DmitryTsepelev commented Apr 2, 2019

iggant commented Apr 4, 2019

lordjavac commented Apr 5, 2019

DmitryTsepelev commented Apr 11, 2019

georgeclaghorn commented Apr 11, 2019

Hareramrai commented Mar 12, 2020

DmitryTsepelev commented Mar 12, 2020

dnalbach commented Apr 28, 2020 •

edited

Loading

DmitryTsepelev commented Apr 28, 2020

dnalbach commented Apr 29, 2020

jules2689 commented Aug 24, 2020

DmitryTsepelev commented Aug 24, 2020

palkan commented Aug 24, 2020

swissonelabs commented Oct 12, 2020

swissonelabs commented Oct 12, 2020

ghiculescu commented Dec 10, 2020

gokninski commented Jun 23, 2021

Allow configure services for individual attachments #34935

Allow configure services for individual attachments #34935

Conversation

DmitryTsepelev commented Jan 14, 2019 • edited Loading

palkan left a comment

Choose a reason for hiding this comment

palkan Jan 14, 2019

Choose a reason for hiding this comment

DmitryTsepelev Jan 15, 2019

Choose a reason for hiding this comment

bogdanvlviv Jan 17, 2019 • edited Loading

Choose a reason for hiding this comment

DmitryTsepelev Jan 18, 2019

Choose a reason for hiding this comment

DmitryTsepelev commented Jan 21, 2019

abhchand commented Feb 18, 2019

DmitryTsepelev commented Feb 18, 2019

iggant commented Apr 2, 2019

DmitryTsepelev commented Apr 2, 2019

iggant commented Apr 4, 2019

lordjavac commented Apr 5, 2019

DmitryTsepelev commented Apr 11, 2019

georgeclaghorn commented Apr 11, 2019

Hareramrai commented Mar 12, 2020

DmitryTsepelev commented Mar 12, 2020

dnalbach commented Apr 28, 2020 • edited Loading

DmitryTsepelev commented Apr 28, 2020

dnalbach commented Apr 29, 2020

jules2689 commented Aug 24, 2020

DmitryTsepelev commented Aug 24, 2020

palkan commented Aug 24, 2020

swissonelabs commented Oct 12, 2020

swissonelabs commented Oct 12, 2020

ghiculescu commented Dec 10, 2020

gokninski commented Jun 23, 2021

DmitryTsepelev commented Jan 14, 2019 •

edited

Loading

bogdanvlviv Jan 17, 2019 •

edited

Loading

dnalbach commented Apr 28, 2020 •

edited

Loading