Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @georgeclaghorn (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

Can you add tests for it?

Here is the actual library which requires this change: https://github.com/ioquatix/activerecord-configurations

I will submit a basic test case. It should be relatively trivial.

Just as a heads-up, it sounds a bit like you're reinventing (the currently not-so-documented) #28095. (With less YAML, perhaps -- but in an overlapping "multiple connections configured per environment" solution-space.)

Or if not, could you elaborate on the use case for multiple configuration sets? I'm a little hesitant to endorse overriding configurations. 😕

@rafaelfranca Please see the additional test case.

@matthewd Thanks for your input. I'd be happy to discuss with you the issues I see with how configurations currently work in active record but I feel such a discussion might derail this PR. However, I'd be happy to have a chat on Gitter.im or even a Hangout on Google if that's something that works for you. I hope that this PR can be seen as a simplification of the logic as implemented (i.e. using self rather than tightly coupling to Base.) If we can implement this small change, which I feel is non-invasive, it should provide benefits to all who are willing to explore improvements to how AR configurations work :)

It looks like the travis build failed, but due to some issue with travis, not the PR.

Wondering if we can get an update here? I believe this is a good change.

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

I still think this is a good idea.

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

I still think this is a good idea.

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

I still think this is a good idea.

I reviewed the changes between Rails 6 and Rails 7.

The main entry point for connection configuration changed from establish_connection to resolve_config_for_connection which is private. There is no easy way for external libraries to intercept configuration of connections reliably now. This was possible in Rails 5 and 6 by monkey patching establish_connection.

This proposal seeks to make more isolated connection state per class. However Rails 7 has moved in a different direction, with more shared state on ActiveRecord::Base. This makes it hard to isolate state and behaviour. I see this was done to decouple model classes from connection state, which in lots of ways is an admirable goal, but one which is at odds with this proposal.

I would suggest that ideally, ActiveRecord::Base has nothing to do with connections, and this is instead delegated out to a separate connection handling instance. Right now, there appears to be a mismatch between old world "connection per class" and new world "connection depending on current per-thread configuration".

As such, and specifically due to the expansion of global state stored on ActiveRecord::Base, I don't think this PR is going to be particularly useful, so I'll close it.

I would suggest that ideally, ActiveRecord::Base has nothing to do with connections, and this is instead delegated out to a separate connection handling instance. Right now, there appears to be a mismatch between old world "connection per class" and new world "connection depending on current per-thread configuration".

I am planning on a project with connection management aimed at achieving the following:

1. Stop using Base as a default connection which fundamentally doesn't work well with a sharding strategy
2. Improve current sharding behavior which is incomplete at the moment due to how Base.connection behaves.
3. Make connection management safer for Rails apps and database gems to avoid accidentally overwriting of connections based on class (this happens especially often on Base and can have severe consequences if done without understanding it clobbers the existing connection)
4. Combine/remove/rewrite ConnectionHandler and friends so there are basically 2 classes that almost do the same thing but not quite. The majority of the API in ConnectionHandler is used by external gems even though it is technically private. I'd like to make it clearer what is public and what is private ultimately making an API that is safe and clear.

I don't have an exact plan yet but figured it was worth sharing what I'm currently thinking about with connections.

Thanks @eileencodes.

When I described "new world" / "old world" style, I was considering the number of users who want sharding/replicas vs those who prefer the simplicity of the previous design. I wouldn't say the new way is better (or worse), it just depends on what dimension you measure it by and the relative weights of what features you care about.

My initial feeling was, ActiveRecord is pretty good, how many people care about sharding? (I don't know). Is the internal design churn and refactoring worth the cost? Because despite being mostly internal refactoring, this does represent quite a big shift in the semantics, as you described above.

What you are really talking about is separating the schema from the connection. This makes total sense to me, but it's also quite a big step from AR 6 where model and connection is intimately connected through the class hierarchy.

No matter how you choose to store the connection state, the idea is similar to the db-model gem I created:

# This is our database schema:
class TestSchema
  include DB::Model::Schema
  
  class User
    include DB::Model::Record
    
    property :id
    property :name
    
    def posts
      scope(Post, user_id: self.id)
    end
  end
  
  class Post
    include DB::Model::Record
    
    property :id
    property :user_id
    
    def user
      scope(User, id: self.user_id)
    end
  end
  
  def users
    table(User)
  end
  
  def posts
    table(Post)
  end
end

# Create an event loop:
Sync do
  # Connect to the database:
  client.transaction do |context|
    schema = TestSchema.new(context)
    
    # Create a user:
    user = schema.users.create(name: "Posty McPostface")
    
    # Insert several posts:
    user.posts.insert([:body], [
      ["Hello World"],
      ["Goodbye World"]
    ])
  end
end

Essentially, a schema is bound to a connection. By doing this, any number of connections/schemas can be combined without it being joined by the static class data. You can even attach multiple schemas to the same connection.

The problem with AR is it never made the assumption this was possible, and that's the hard lift in AR7 and by extension your proposal. However, you could largely implement something similar just by duplicating full class hierarchies and connecting different hierarchies to different backends. Some could even argue this is a better design because it's easier for most users to understand... anyway.

I think moving more of the connection handling into the connection handling class would make sense. However, it also seems to me that it lacks some of the elegance, simplicity of the old world. Trying to focus on some of that and providing hooks for extensions (e.g. dynamically loading credentials for connections) would go a long way to solving the pain points I personally had with AR7. Right now, instead of that, we are looking to modify ENV['...DATABASE_URL'] to try and inject connection configuration from the AWS secrets manager. It's definitely feels wrong for us to inject these credentials into database.yml using ERB templates.

So, as a general idea, I'd like to see a little bit improved separation of concerns between the different layers, and interfaces with slightly more extensible design. This original PR was motivated by the former (reduce interdependency on ActiveRecord::Base). Therefore, I basically agree with the direction of your proposal above.

Thanks for your continued effort.