Support Rails 7.1, drop Ruby 2.x and Rails < 6.1

[eraffel-MDSol]

No description provided.

[JakubF]

I think this method should be implemented as such:

    def connected_to_with_tenant(role: nil, shard: nil, prevent_writes: false, &blk)
      current_tenant = Apartment::Tenant.current

      connected_to_without_tenant(role: role, shard: shard, prevent_writes: prevent_writes) do
        Apartment::Tenant.switch!(current_tenant)
        yield(blk)
      end
    end

    alias connected_to_without_tenant connected_to
    alias connected_to connected_to_with_tenant

Potentially current_tenant = Apartment::Tenant.current is redundant because adapter is stored on thread and current is delegated to the adapter so this could potentially be further simplified but needs testing (I only tested with pg).
I’m pretty new to the source code of this gem and am still struggling to understand some of the code here.
From my initial research into this monkey-patch, current rails’ connected_to is defined as def connected_to(role: nil, shard: nil, prevent_writes: false, &blk) - so this monkeypatch removes the ability to pass shard and forces connected_to to always default to “nil”
I tested this with my local app with sharding and seems to work, this code doesn't seem to be tested very well, removing this monkey-patch doesn't really break tests - I believe this is because there is only 1 shard setup in test and rails caches connections and looks them up by role/shard.

[eraffel-MDSol]

Changing this method would be out of the scope of my PR, which is just to get Rails 7.1 supported. The only reason there's changes in this file is I got rid of the if condition which affected the indentation of the rest of the file

[JakubF]

I believe this monkey-patch needs to be removed as it breaks rails 7.1 support
This is basically an implementation of table_exists? from rails 5.2 and has worked for rails < 7.1 because SchemaMigration was inheriting from ActiveRecord::Base that was implementing connection method, after your PR, this method will start raising error "undefined local variable or method `connection' for ActiveRecord::SchemaMigration:Class"
Current implementation of table_exists is here:

    def table_exists?
      @pool.with_connection do |connection|
        connection.data_source_exists?(table_name)
      end
    end

it's almost identical - the main difference is that it uses data_source_exists? on connection while this monkey patch is forcing it to use table_exists?
The difference between these 2 methods is here:

      # Checks to see if the data source +name+ exists on the database.
      #
      #   data_source_exists?(:ebooks)
      #
      def data_source_exists?(name)
        query_values(data_source_sql(name), "SCHEMA").any? if name.present?
      rescue NotImplementedError
        data_sources.include?(name.to_s)
      end

      # Checks to see if the table +table_name+ exists on the database.
      #
      #   table_exists?(:developers)
      #
      def table_exists?(table_name)
        query_values(data_source_sql(table_name, type: "BASE TABLE"), "SCHEMA").any? if table_name.present?
      rescue NotImplementedError
        tables.include?(table_name.to_s)
      end

so basically just data_source_sql(name) VS data_source_sql(table_name, type: "BASE TABLE"), the difference that this type: part does, is that on table_exists it only looks up partitioned tables and relational tables, and on data_source_exists it also looks up views and materialised views, so this difference is irrelevant for schema migrations.

I believe this monkey-patch needs to be removed, this will 100% cause errors on rails 7.1

[eraffel-MDSol]

Do you have a failing test I can add then?

[JakubF]

Calling ActiveRecord::SchemaMigration.table_exists? should be enough,
In rails 7.1 it should raise undefined local variable or method 'connection' for ActiveRecord::SchemaMigration:Class (NameError)
In rails 7.0 it should be perfectly fine and return true

[eraffel-MDSol]

It looks like this was added because table_exists? was removed from SchemaMigration in rails 6.1, but it looks like it was added back in 7.0. Also, you're looking at main in Rails. you need to look at the 7.1 branch. As far as I can tell, this monkey patch is fine, as it is identical to the method in 7.0 and 7.1, and fixes the issue in 6.1. You're right that when 7.2 comes out that this will need to be changed again or just dropped entirely since 6.0 will go out of support at that point, but it seems fine now

[Amnesthesia]

We had issues with Rails 7.1 and the postgresql adapter, causing a lot of very heavy internal queries, which we reported to rails/rails but which was closed as an Apartment issue (rails/rails#49976)

Curious, does this PR resolve this?

[brosintoski]

@JakubF any idea of when this can get merged? This is currently a blocker for upgrading my app to 7.1.3, and really would like to avoid forking.

[Amnesthesia]

@riggleg maybe?

[eraffel-MDSol]

@Amnesthesia I haven't had a chance to look at the issue you linked to yet, just FYI

[Amnesthesia]

@Amnesthesia I haven't had a chance to look at the issue you linked to yet, just FYI

Alright, thanks for the update!

[Amnesthesia]

Any progress on this @riggleg @JakubF or any other maintainer here?

[mnovelo]

@Amnesthesia Hi, I'm a new maintainer!

Taking a look at the issue you reported to Rails, my understanding is that Rails 7.1 adds the ability to set the isolation level using Fibers to be Fiber-safe, whereas before, the focus was on Threads. Threads are still the default for Rails 7.1.

I don't yet see why you experienced a behavior change when upgrading to Rails 7.1 or why the solution that worked for you was changing to using Fibers instead of Threads. As far as I know, the Apartment gem has not yet done anything to use Fibers over Threads. This may well be outside of the scope of this PR.

[mnovelo]

Suggested change

	- image: cimg/postgres:12.13
	- image: cimg/postgres:12.18

[mnovelo]

Suggested change

	ruby_version: ["ruby:3.1.4", "ruby:3.2.2"]
	ruby_version: ["ruby:3.1.4", "ruby:3.2.3"]

[mnovelo]

UPDATE: I missed that Rails 7.1 just adds a warning about the change, so this isn't a blocker on this PR. We can address the warning in another PR.

@eraffel-MDSol from PR #236 it looks like there was an API change to ActiveSupport::LobSubscriber that we should also account for in lib/apartment/log_subscriber.rb for Rails 7.1

In < 7.1, https://github.com/rails/rails/blob/506462ab13755d9f024e1ddbfc8c58d73e7a1bce/activesupport/lib/active_support/log_subscriber.rb#L139

In 7.1, https://github.com/rails/rails/blob/6f0d1ad14b92b9f5906e44740fce8b4f1c7075dc/activesupport/lib/active_support/log_subscriber.rb#L175

[mnovelo]

@Amnesthesia I wonder if what you were experiencing might be related to this issue and have been aggravated by this change in Rails 7.1. This fix was proposed, but I wonder if it'd be better to have something like what Rails is doing for ActiveSupport::IsolatedExecutionState is better that allows use of Fibers or Threads.

This could be related to the Rails 7.1 upgrade and could warrant including (unless the change is too big), but it'd be helpful if you could also test in your environment

[brosintoski]

@mnovelo is this pr not stable enough to merge? Currently there is No 7.1 support and it appears all critical issues have been addressed.

Some co-workers of mine are also looking to open a PR for Trilogy db adapter and waiting on this to merge before doing so.

[mnovelo]

@brosintoski you're right. I just realized the logger API change is handled in Rails 7.1 with a warning, which we can address in another PR.

The issue with threads vs fibers requires more investigation, so I'll merge this in for now to unblock other PRs

[brosintoski]

@mnovelo @eraffel-MDSol thanks!!!

[Amnesthesia]

@Amnesthesia I wonder if what you were experiencing might be related to this issue and have been aggravated by this change in Rails 7.1. This fix was proposed, but I wonder if it'd be better to have something like what Rails is doing for ActiveSupport::IsolatedExecutionState is better that allows use of Fibers or Threads.

This could be related to the Rails 7.1 upgrade and could warrant including (unless the change is too big), but it'd be helpful if you could also test in your environment

It sounds very likely that it would be related to this indeed. We're in the process of getting rid of Apartment instead because of the amount of headache it causes us, so I'm not sure how much time I'll be able to spend on testing this

[brosintoski]

@Amnesthesia out of curiosity what are you replacing Apartment with?

[eraffel-MDSol]

seems like this was merged before I had a chance to read comments from the last few days

[Amnesthesia]

@Amnesthesia out of curiosity what are you replacing Apartment with?

Looking at replacing it with acts_as_tenant, or custom column based multitenancy

[mnovelo]

@Amnesthesia let us know how it goes! Because of how our security documentation is written, column-based multitenancy isn't an option for us, but having an "escape hatch" would be nice to document since I've seen it come up in a few issues