I don't think we should call it a legacy config. It's still a valid config that most of Rails apps are going to use, as long as we don't deprecate one level config.

I find local_configurations confusing: every config is defined locally in database.yml.

Personally, I'd keep this all in configurations and turn it into an object that handles mapping between 2 or 3 level hashes:

class Configurations < Hash
  def initialize(hash)
    @configs = normalize_nesting(hash)
  end

  def [](env_or_spec_name)
    # If we couldn't normalize it properly we could handle both env and spec names here.
  end

  private
    def normalize_nesting(hash)
      # Insert Arthur Magic…
    end
end

👍 agree with Kasper, the naming here is difficult to understand; it took me several read throughs to understand the difference between local_configurations and configurations 😬

Introduces yet another name for a configuration: main. Think we need to prune the terminology :)

Active Record

But it knows lodge like no other 🤓

Don't think we need these two new lines :)

Think I finally get why "connection specification name" has bothered me so. Partly because it's so abstract, but mainly it's because Active Record just calls these configurations and that's simpler to grasp.

So while I think spec or connection specification is fine internally, I think we should try using configuration as the public terminology.

👍 Agree @kaspth ,

So the model using a readonly config could look like this:

class User < ApplicationRecord
  self.connection_configuration = :readonly
end

or:

class User < ApplicationRecord
  def self.connection_configuration
    Application.readonly_mode? ? :readonly : :primary
  end
end

Pretty much we could replace the connection_specification_name to connection_configuration in the model.

I don't want to change those in this PR tho, we can try in a next one. Also there will be some deprecation cycle for the name change, as 5.0 uses connection_specification_name

Why do we need to know the primary wording internally? Shouldn't establish connection just handle a config without a name?

New lines around here.

Think it's a smell that the database tasks have to reach for the ConnectionAdapters::ConnectionSpecification::LegacyConfigTransformer.

👍 Agree @kaspth ,

So the model using a readonly config could look like this:

class User < ApplicationRecord
  self.connection_configuration = :readonly
end

or:

class User < ApplicationRecord
  def self.connection_configuration
    Application.readonly_mode? ? :readonly : :primary
  end
end

Pretty much we could replace the connection_specification_name to connection_configuration in the model.

I don't want to change those in this PR tho, we can try in a next one. Also there will be some deprecation cycle for the name change, as 5.0 uses connection_specification_name

Maybe instead of checking for an adapter key we should check config.values.any? { |v| !v.is_a?(Hash) }?

When won't configurations respond to normalized? Think this might be too protective if it guards against people overriding ActiveRecord::Base.configurations to return whatever.

Getting rid of that check also means we can slim the diff and keep the previous easier-to-read structure of the method as a bonus 😁

Maybe we should leave out that configurations is a Hash 😉

Let's keep the previous newline below and remove the one above :)

Do these changes mean we've broken backwardscompatibility?

Looks like it indeed broke.

adding it back

Find these two lines hard to parse because of the two conditionals. The empty hash fallback seems too defensive at first sight. What case is it trying to cover? That there's a key in self[key] but with no config at all?

Could we simplify to this?

config = fetch(key, self).dup

We've just spent lines establishing "primary", surely it should have been made an empty hash by now?

Are we removing this because resolve_connection calls it?

added back

This makes me wonder how much we need to worry about people calling all the available Hash methods on ActiveRecord::Base.configurations[some_env] and if we can just normalize the configurations up front.

configuration name 😉

This API looks weird to me. I'd expect the resolver to always be initialized with the config and resolve a spec.

fixed

Looks like it indeed broke.

Why the adapter is now required?

I think it's because we only check for the adapter key at the moment. Finding a more accurate way to find an actual config could fix the need to change this.

It's already required in practice for an actual config you intend to use, and while this test was accidentally assuming it, we've never claimed to be an arbitrary hash storage bucket.

When is this called? My cmd-E on keys can't find any instances?

I still found a lot of this logic hard to read, so I took the liberty of trying to make it more concise:

class ConnectionConfigurations #:nodoc:
  def initialize(config)
    @root_config = @config = config
    @database_url_config = resolve_database_url_config || {}
  end
 
  def scope=(scope)
    @root_config, @config = @config, (@config[scope] || {})
  end
 
  def [](key)
    @database_url_config.reverse_merge(@config[key] || @root_config[key] || {})
  end

  def keys
    @config.keys
  end

  def to_h
    @config
  end

  private
    def resolve_database_url_config
      if ENV["DATABASE_URL"]
        ConnectionUrlResolver.new(ENV["DATABASE_URL"]).to_hash
      end
    end
end

I'm curious why we need the @root_config[key] fallback in []?

NOTE: scope= basically assumes that it's never assigned more than once, which it doesn't seem to be in library code, but it is in test code. Would it make more sense to pass it in at the constructor? Or have a scope method that returns a new instance of ConnectionConfigurations?

Isn't this applying the URL-supplied database to every configuration we return?

That's wrong, and I'm alarmed the tests aren't point that out. It's hard to see what's going on with them, because they're all renamed at the same time, though. 😟