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

Allow configure services for individual attachments #34935

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@DmitryTsepelev
Copy link
Contributor

DmitryTsepelev commented Jan 14, 2019

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.

@DmitryTsepelev DmitryTsepelev force-pushed the DmitryTsepelev:configure-store-per-attachment branch 2 times, most recently from c9ea062 to b3d16cc Jan 14, 2019

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

@palkan
Copy link
Contributor

palkan left a comment

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?

configs = Rails.configuration.active_storage.service_configurations

begin
ActiveStorage::Service.configure service_name, configs

This comment has been minimized.

@palkan

palkan Jan 14, 2019

Contributor

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

This comment has been minimized.

@DmitryTsepelev

DmitryTsepelev Jan 15, 2019

Author Contributor

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

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

def change
add_column :active_storage_blobs, :service_name, :string
end
end

This comment has been minimized.

@bogdanvlviv

bogdanvlviv Jan 17, 2019

Contributor

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 :)

Hey, we've created approach to upgrade apps of users that use Active Storage. See #33419.

Could you move this migration under activestorage/db/update_migrate/ directory so rake app:update will copy this migration to existing apps if needed. Also, you should extend activestorage/db/migrate/20170806125915_create_active_storage_tables.rb migration to add service_name column for new apps on rails active_storage:install.

This comment has been minimized.

@DmitryTsepelev

DmitryTsepelev Jan 18, 2019

Author Contributor

@bogdanvlviv Done, thanks for the suggestion!

What would happen if user won't execute rake app:update before running the app? In case he gets an error preventing him from running the app - we could also drop all these checks. Also, as far as I know, this task is not designed to be executed in point releases, so we should make this PR a part of 6.0.0 😅

This comment has been minimized.

@bogdanvlviv

bogdanvlviv Jan 18, 2019

Contributor

What would happen if user won't execute rake app:update before running the app?

It is ok when an app fails because it hasn't been upgraded well. It is users' responsibility to upgrade it right, our responsibility is to guide on how to do it.

In case he gets an error preventing him from running the app - we could also drop all these checks.

Yeah, I think we can drop these checks.

Also, as far as I know, this task is not designed to be executed in point releases, so we should make this PR a part of 6.0.0 😅

Right, rake app:update is designed only fo major and minor releases. Since this PR brings a new feature, we can release it only in any next major/minor release because we don't add features in point/patch releases.

This comment has been minimized.

@DmitryTsepelev

DmitryTsepelev Jan 18, 2019

Author Contributor

Perfect, it made things way more simple

`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`).

This comment has been minimized.

@bogdanvlviv

bogdanvlviv Jan 17, 2019

Contributor

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

This comment has been minimized.

@DmitryTsepelev

DmitryTsepelev Jan 18, 2019

Author Contributor

✔️

@DmitryTsepelev DmitryTsepelev force-pushed the DmitryTsepelev:configure-store-per-attachment branch 4 times, most recently from 717ee50 to b19d802 Jan 18, 2019

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

@@ -0,0 +1,7 @@
class AddServiceNameToActiveStorageBlobs < ActiveRecord::Migration[6.0]
def up
unless column_exitsts?(:active_storage_blobs, :service_name)

This comment has been minimized.

@bogdanvlviv

bogdanvlviv Jan 19, 2019

Contributor

should be column_exists?

This comment has been minimized.

@DmitryTsepelev

DmitryTsepelev Jan 20, 2019

Author Contributor

yah, my bad

@@ -51,14 +51,16 @@ def find_signed(id)

# Returns a new, unsaved blob instance after the +io+ has been uploaded to the service.
# When providing a content type, pass <tt>identify: false</tt> to bypass automatic content type inference.
def build_after_upload(io:, filename:, content_type: nil, metadata: nil, identify: true)
new(filename: filename, content_type: content_type, metadata: metadata).tap do |blob|
# Option <tt>:service</tt> should be provided when blob should be stored in the service different from the

This comment has been minimized.

@bogdanvlviv

bogdanvlviv Jan 19, 2019

Contributor

Shouldn't :service_name be here instead of :service and the same in the docs of create_after_upload!?

This comment has been minimized.

@DmitryTsepelev

DmitryTsepelev Jan 20, 2019

Author Contributor

✔️

rescue => e
msg = "Cannot configure service :#{service_name} for #{inspect}:"

case ActiveStorage.service_misconfiguration_behavior

This comment has been minimized.

@bogdanvlviv

bogdanvlviv Jan 19, 2019

Contributor

Not sure whether we need this since users should deal with blobs through attachments only, right?

This comment has been minimized.

@DmitryTsepelev

DmitryTsepelev Jan 20, 2019

Author Contributor

Let's imagine a following situation:

There are two services configured in the yaml - :s3_public and :s3_private, the first one is configured as a global default. App records some blobs with a service :s3_private and one day someone removes it from the config. In this case we have some blobs saved to the storage, which is not available anymore.

I think it would be helpful to notify a developer that the app is trying to access such a "broken" blob.

@@ -93,7 +94,7 @@ class Engine < Rails::Engine # :nodoc:
initializer "active_storage.services" do
ActiveSupport.on_load(:active_storage_blob) do
if config_choice = Rails.configuration.active_storage.service
configs = Rails.configuration.active_storage.service_configurations ||= begin
Rails.configuration.active_storage.service_configurations ||= begin

This comment has been minimized.

@bogdanvlviv

bogdanvlviv Jan 19, 2019

Contributor

Hm, I'm getting error:

NoMethodError (Cannot configure service :local for Post#avatar`:
undefined method `deep_symbolize_keys' for nil:NilClass):

We should ensure that this block is always executed before ActiveStorage::ServiceRegistry.fetch(service_name) call

This comment has been minimized.

@DmitryTsepelev

DmitryTsepelev Jan 20, 2019

Author Contributor

Thanks for catching that! Moved config reading to the ServiceRegistry

@DmitryTsepelev DmitryTsepelev force-pushed the DmitryTsepelev:configure-store-per-attachment branch 2 times, most recently from 40ff9b5 to 43b8441 Jan 20, 2019

@DmitryTsepelev DmitryTsepelev force-pushed the DmitryTsepelev:configure-store-per-attachment branch from 43b8441 to e40eb8f Jan 20, 2019

@DmitryTsepelev

This comment has been minimized.

Copy link
Contributor Author

DmitryTsepelev commented Jan 21, 2019

@abhchand

This comment has been minimized.

Copy link

abhchand commented Feb 18, 2019

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

This comment has been minimized.

Copy link
Contributor Author

DmitryTsepelev commented Feb 18, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment