Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from Mar 27, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
38 changes: 7 additions & 31 deletions .circleci/config.yml
View file
Open in desktop
Expand Up @@ -3,11 +3,9 @@ version: 2.1
jobs:
build:
docker:
- image: << parameters.ruby_version >>
- image: postgres:11.19-alpine
environment:
POSTGRES_HOST_AUTH_METHOD: "trust"
- image: mysql:5.7
- image: cimg/<< parameters.ruby_version >>
- image: cimg/postgres:12.13
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

- image: cimg/mysql:8.0
environment:
MYSQL_ALLOW_EMPTY_PASSWORD: "yes"
parameters:
Expand All @@ -19,31 +17,16 @@ jobs:
BUNDLE_GEMFILE: << parameters.gemfile >>
steps:
- checkout
# Restore Cached Dependencies
# - restore_cache:
# keys:
# - gem-cache-v1-{{ arch }}-{{ .Branch }}-{{ checksum "<< parameters.gemfile >>.lock" }}
# - gem-cache-v1-{{ arch }}-{{ .Branch }}
# - gem-cache-v1

- run: bundle install --path vendor/bundle

# - save_cache:
# key: gem-cache-v1-{{ arch }}-{{ .Branch }}-{{ checksum "<< parameters.gemfile >>.lock" }}
# paths:
# - vendor/bundle

- run:
name: Update apt
command: apt update -y
command: sudo apt update -y

- run:
name: Install dependencies
command: apt install -y curl postgresql-client default-mysql-client

- run:
name: Install dockerize
command: curl -sfL $(curl -s https://api.github.com/repos/powerman/dockerize/releases/latest | grep -i /dockerize-$(uname -s)-$(uname -m)\" | cut -d\" -f4) | install /dev/stdin /usr/local/bin/dockerize
command: sudo apt install -y curl postgresql-client default-mysql-client

- run:
name: Configure config database.yml
Expand Down Expand Up @@ -75,12 +58,5 @@ workflows:
- build:
matrix:
parameters:
ruby_version: ["ruby:2.7-buster", "ruby:3.0-buster", "ruby:3.1-buster", "ruby:3.2-buster"]
gemfile: ["gemfiles/rails_5_2.gemfile", "gemfiles/rails_6_0.gemfile", "gemfiles/rails_6_1.gemfile", "gemfiles/rails_7_0.gemfile"]
exclude:
- ruby_version: "ruby:3.0-buster"
gemfile: "gemfiles/rails_5_2.gemfile"
- ruby_version: "ruby:3.1-buster"
gemfile: "gemfiles/rails_5_2.gemfile"
- ruby_version: "ruby:3.2-buster"
gemfile: "gemfiles/rails_5_2.gemfile"
ruby_version: ["ruby:3.1.4", "ruby:3.2.2"]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

gemfile: ["gemfiles/rails_6_1.gemfile", "gemfiles/rails_7_0.gemfile", "gemfiles/rails_7_1.gemfile"]
2 changes: 1 addition & 1 deletion .ruby-version
View file
Open in desktop
@@ -1 +1 @@
2.7.7
3.1.4
29 changes: 10 additions & 19 deletions Appraisals
View file
Open in desktop
@@ -1,28 +1,19 @@
# frozen_string_literal: true

appraise 'rails-5-2' do
gem 'rails', '~> 5.2.0'
platforms :jruby do
gem 'activerecord-jdbc-adapter', '~> 52.0'
gem 'activerecord-jdbcpostgresql-adapter', '~> 52.0'
gem 'activerecord-jdbcmysql-adapter', '~> 52.0'
end
end

appraise 'rails-6-0' do
gem 'rails', '~> 6.0.0'
appraise 'rails-6-1' do
gem 'rails', '~> 6.1.0'
platforms :ruby do
gem 'sqlite3', '~> 1.4'
end
platforms :jruby do
gem 'activerecord-jdbc-adapter', '~> 60.0'
gem 'activerecord-jdbcpostgresql-adapter', '~> 60.0'
gem 'activerecord-jdbcmysql-adapter', '~> 60.0'
gem 'activerecord-jdbc-adapter', '~> 61.0'
gem 'activerecord-jdbcpostgresql-adapter', '~> 61.0'
gem 'activerecord-jdbcmysql-adapter', '~> 61.0'
end
end

appraise 'rails-6-1' do
gem 'rails', '~> 6.1.0'
appraise 'rails-7-0' do
gem 'rails', '~> 7.0.0'
platforms :ruby do
gem 'sqlite3', '~> 1.4'
end
Expand All @@ -33,10 +24,10 @@ appraise 'rails-6-1' do
end
end

appraise 'rails-7-0' do
gem 'rails', '~> 7.0.0'
appraise 'rails-7-1' do
gem 'rails', '~> 7.1.0'
platforms :ruby do
gem 'sqlite3', '~> 1.4'
gem 'sqlite3', '~> 1.6'
end
platforms :jruby do
gem 'activerecord-jdbc-adapter', '~> 61.0'
Expand Down
20 changes: 3 additions & 17 deletions Rakefile
View file
Open in desktop
Expand Up @@ -132,22 +132,8 @@ def my_config
config['mysql']
end

def activerecord_below_5_2?
ActiveRecord.version.release < Gem::Version.new('5.2.0')
end

def activerecord_below_6_0?
ActiveRecord.version.release < Gem::Version.new('6.0.0')
end

def migrate
if activerecord_below_5_2?
ActiveRecord::Migrator.migrate('spec/dummy/db/migrate')
elsif activerecord_below_6_0?
ActiveRecord::MigrationContext.new('spec/dummy/db/migrate').migrate
else
# TODO: Figure out if there is any other possibility that can/should be
# passed here as the second argument for the migration context
ActiveRecord::MigrationContext.new('spec/dummy/db/migrate', ActiveRecord::SchemaMigration).migrate
end
# TODO: Figure out if there is any other possibility that can/should be
# passed here as the second argument for the migration context
ActiveRecord::MigrationContext.new('spec/dummy/db/migrate', ActiveRecord::SchemaMigration).migrate
end
13 changes: 0 additions & 13 deletions gemfiles/rails_5_2.gemfile
View file
Open in desktop

This file was deleted.

17 changes: 0 additions & 17 deletions gemfiles/rails_6_0.gemfile
View file
Open in desktop

This file was deleted.

17 changes: 17 additions & 0 deletions gemfiles/rails_7_1.gemfile
View file
Open in desktop
@@ -0,0 +1,17 @@
# This file was generated by Appraisal

source "http://rubygems.org"

gem "rails", "~> 7.1.0"

platforms :ruby do
gem "sqlite3", "~> 1.6"
end

platforms :jruby do
gem "activerecord-jdbc-adapter", "~> 61.0"
gem "activerecord-jdbcpostgresql-adapter", "~> 61.0"
gem "activerecord-jdbcmysql-adapter", "~> 61.0"
end

gemspec path: "../"
22 changes: 6 additions & 16 deletions lib/apartment.rb
View file
Open in desktop
Expand Up @@ -7,15 +7,9 @@
require 'apartment/tenant'

require_relative 'apartment/log_subscriber'

if ActiveRecord.version.release >= Gem::Version.new('6.0')
require_relative 'apartment/active_record/connection_handling'
end

if ActiveRecord.version.release >= Gem::Version.new('6.1')
require_relative 'apartment/active_record/schema_migration'
require_relative 'apartment/active_record/internal_metadata'
end
require_relative 'apartment/active_record/connection_handling'
require_relative 'apartment/active_record/schema_migration'
require_relative 'apartment/active_record/internal_metadata'

# Apartment main definitions
module Apartment
Expand All @@ -33,14 +27,10 @@ class << self
attr_accessor(*ACCESSOR_METHODS)
attr_writer(*WRITER_METHODS)

if ActiveRecord.version.release >= Gem::Version.new('6.1')
def_delegators :connection_class, :connection, :connection_db_config, :establish_connection
def_delegators :connection_class, :connection, :connection_db_config, :establish_connection

def connection_config
connection_db_config.configuration_hash
end
else
def_delegators :connection_class, :connection, :connection_config, :establish_connection
def connection_config
connection_db_config.configuration_hash
end

# configure apartment with available options
Expand Down
26 changes: 12 additions & 14 deletions lib/apartment/active_record/connection_handling.rb
View file
Open in desktop
@@ -1,22 +1,20 @@
# frozen_string_literal: true

module ActiveRecord # :nodoc:
if ActiveRecord::VERSION::MAJOR >= 6
# This is monkeypatching Active Record to ensure that whenever a new connection is established it
# switches to the same tenant as before the connection switching. This problem is more evident when
# using read replica in Rails 6
module ConnectionHandling
def connected_to_with_tenant(role: nil, prevent_writes: false, &blk)
current_tenant = Apartment::Tenant.current
# This is monkeypatching Active Record to ensure that whenever a new connection is established it
# switches to the same tenant as before the connection switching. This problem is more evident when
# using read replica in Rails 6
module ConnectionHandling
def connected_to_with_tenant(role: nil, prevent_writes: false, &blk)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

current_tenant = Apartment::Tenant.current

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

alias connected_to_without_tenant connected_to
alias connected_to connected_to_with_tenant
end

alias connected_to_without_tenant connected_to
alias connected_to connected_to_with_tenant
end
end
2 changes: 1 addition & 1 deletion lib/apartment/active_record/schema_migration.rb
View file
Open in desktop
@@ -1,7 +1,7 @@
# frozen_string_literal: true

module ActiveRecord
class SchemaMigration < ActiveRecord::Base # :nodoc:
class SchemaMigration # :nodoc:
class << self
def table_exists?
connection.table_exists?(table_name)
Comment on lines +4 to 7
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a failing test I can add then?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Expand Down
24 changes: 3 additions & 21 deletions lib/apartment/migrator.rb
View file
Open in desktop
Expand Up @@ -13,40 +13,22 @@ def migrate(database)

migration_scope_block = ->(migration) { ENV['SCOPE'].blank? || (ENV['SCOPE'] == migration.scope) }

if activerecord_below_5_2?
ActiveRecord::Migrator.migrate(ActiveRecord::Migrator.migrations_paths, version, &migration_scope_block)
else
ActiveRecord::Base.connection.migration_context.migrate(version, &migration_scope_block)
end
ActiveRecord::Base.connection.migration_context.migrate(version, &migration_scope_block)
end
end

# Migrate up/down to a specific version
def run(direction, database, version)
Tenant.switch(database) do
if activerecord_below_5_2?
ActiveRecord::Migrator.run(direction, ActiveRecord::Migrator.migrations_paths, version)
else
ActiveRecord::Base.connection.migration_context.run(direction, version)
end
ActiveRecord::Base.connection.migration_context.run(direction, version)
end
end

# rollback latest migration `step` number of times
def rollback(database, step = 1)
Tenant.switch(database) do
if activerecord_below_5_2?
ActiveRecord::Migrator.rollback(ActiveRecord::Migrator.migrations_paths, step)
else
ActiveRecord::Base.connection.migration_context.rollback(step)
end
ActiveRecord::Base.connection.migration_context.rollback(step)
end
end

private

def activerecord_below_5_2?
ActiveRecord.version.release < Gem::Version.new('5.2.0')
end
end
end
4 changes: 2 additions & 2 deletions ros-apartment.gemspec
View file
Open in desktop
Expand Up @@ -27,7 +27,7 @@ Gem::Specification.new do |s|
s.homepage = 'https://github.com/rails-on-services/apartment'
s.licenses = ['MIT']

s.add_dependency 'activerecord', '>= 5.0.0', '< 7.1'
s.add_dependency 'activerecord', '>= 6.1.0', '< 7.2'
s.add_dependency 'parallel', '< 2.0'
s.add_dependency 'public_suffix', '>= 2.0.5', '< 5.0'
s.add_dependency 'rack', '>= 1.3.6', '< 3.0'
Expand All @@ -39,7 +39,7 @@ Gem::Specification.new do |s|
s.add_development_dependency 'rake', '~> 13.0'
s.add_development_dependency 'rspec', '~> 3.4'
s.add_development_dependency 'rspec_junit_formatter'
s.add_development_dependency 'rspec-rails', '~> 3.4'
s.add_development_dependency 'rspec-rails', '~> 6.1'
s.add_development_dependency 'rubocop', '~> 0.93'
s.add_development_dependency 'rubocop-performance', '~> 1.10'
s.add_development_dependency 'rubocop-rails', '~> 2.1'
Expand Down
3 changes: 1 addition & 2 deletions spec/dummy/db/migrate/20110613152810_create_dummy_models.rb
View file
Open in desktop
@@ -1,7 +1,6 @@
# frozen_string_literal: true

migration_class = ActiveRecord::VERSION::MAJOR >= 5 ? ActiveRecord::Migration[4.2] : ActiveRecord::Migration
class CreateDummyModels < migration_class
class CreateDummyModels < ActiveRecord::Migration[4.2]
def self.up
create_table :companies do |t|
t.boolean :dummy
Expand Down
3 changes: 1 addition & 2 deletions spec/dummy/db/migrate/20111202022214_create_table_books.rb
View file
Open in desktop
@@ -1,7 +1,6 @@
# frozen_string_literal: true

migration_class = ActiveRecord::VERSION::MAJOR >= 5 ? ActiveRecord::Migration[4.2] : ActiveRecord::Migration
class CreateTableBooks < migration_class
class CreateTableBooks < ActiveRecord::Migration[4.2]
def up
create_table :books do |t|
t.string :name
Expand Down
3 changes: 1 addition & 2 deletions spec/dummy/db/migrate/20180415260934_create_public_tokens.rb
View file
Open in desktop
@@ -1,7 +1,6 @@
# frozen_string_literal: true

migration_class = ActiveRecord::VERSION::MAJOR >= 5 ? ActiveRecord::Migration[4.2] : ActiveRecord::Migration
class CreatePublicTokens < migration_class
class CreatePublicTokens < ActiveRecord::Migration[4.2]
def up
create_table :public_tokens do |t|
t.string :token
Expand Down
2 changes: 1 addition & 1 deletion spec/examples/schema_adapter_examples.rb
View file
Open in desktop
Expand Up @@ -213,7 +213,7 @@
it 'should not raise any errors' do
expect do
subject.switch! 'unknown_schema'
end.not_to raise_error(Apartment::TenantNotFound)
end.not_to raise_error
end
end

Expand Down