Set ActiveStorage::Blob.service when ActiveStorage::Blob is loaded

[georgeclaghorn]

Fixes that ActiveStorage::Blob.service is unset when ActiveStorage::Blob is reloaded.

[rails-bot]

r? @eileencodes

(@rails-bot has picked a reviewer for you, use r? to override)

[rafaelfranca]

Can we use config_for for this?

[georgeclaghorn]

I considered that, but the YAML file doesn’t have top-level environment keys, so we can’t.

[rafaelfranca]

Good point. Maybe we could extract the file read behavior from config_for to a private API method and reuse here?

[georgeclaghorn]

I’m in favor of extracting a reusable method, but I’d like to do it in a separate PR. There are a few other places where we do roughly the same thing but in slightly different ways, and I’d like to consolidate them.

[rafaelfranca]

👍

[rafaelfranca]

Instead of reading the YAML every request in development what do you think to store the configuration in the active_storage configuration object? Something like:

initializer "active_storage.services" do
  config.active_storage.service_configuration = application.config_for("config/storage.yml")
  
  config.to_prepare do
    if config_choice = Rails.configuration.active_storage.service
      ActiveStorage::Blob.service =
             begin
               ActiveStorage::Service.configure config_choice, Rails.configuration.active_storage.service_configuration
             rescue => e
               raise e, "Cannot load `Rails.config.active_storage.service`:\n#{e.message}", e.backtrace
             end
     endddd

[georgeclaghorn]

Memoized the config in caf7e62.

[rafaelfranca]

Maybe it is better to avoid the memoization and conditionally do everything, even the to_prepare hook? We could take advantage of the scope of the blocks to even avoiding have to memoize things or create a new config.

    initializer "active_storage.services" do
      if config_choice = app.config.active_storage.service
        config_file = Pathname.new(Rails.root.join("config/storage.yml"))
        raise("Couldn't find Active Storage configuration in #{config_file}") unless config_file.exist?

        require "yaml"
        require "erb"

        configs =
          begin
            YAML.load(ERB.new(config_file.read).result) || {}
          rescue Psych::SyntaxError => e
            raise "YAML syntax error occurred while parsing #{config_file}. " \
              "Please note that YAML must be consistently indented using spaces. Tabs are not allowed. " \
              "Error: #{e.message}"
          end

        config.to_prepare do |app|
          ActiveStorage::Blob.service =
            begin
              ActiveStorage::Service.configure config_choice, configs
            rescue => e
              raise e, "Cannot load `Rails.config.active_storage.service`:\n#{e.message}", e.backtrace
            end
        end
      end
    end

[georgeclaghorn]

That would make it impossible to set Rails.configuration.active_storage.service in an initializer, no? Maybe that’s not a big deal.

[rafaelfranca]

oh, good point. I think we should keep the possibility to set that in a initializer, so let's keep your implementation for now. I'll change if I think in a way to solve that problem.