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.

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

Fix sequence name #187

Merged
merged 14 commits into from
Feb 3, 2022
Merged
1 change: 1 addition & 0 deletions lib/apartment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

require_relative 'apartment/log_subscriber'

require_relative 'apartment/active_record/postgresql_adapter'
Copy link
Contributor

Choose a reason for hiding this comment

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

This would force the loading of the pg adapter. Are other dbs no longer supported? This was my comment on #143 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I have not looked at how can we fix this and test it on a non postgresql db. I currently run only postgres so it's also hard for me to test these details on all dbs. I've been tempted on starting refactoring the gem to have one gem per adapter and a core, but that's discussion for another day.

Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps we can go the "easy" route:

if defined?(PG)
  require_relative 'apartment/active_record/postgresql_adapter'
end

Perhaps in an initializer so that this happens after all gems are loaded?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think i have a clearer working solution. We only require the adapter once we eval the config. I'm requiring this patch after requiring the postgres adapter

if ActiveRecord.version.release >= Gem::Version.new('6.0')
require_relative 'apartment/active_record/connection_handling'
end
Expand Down
25 changes: 25 additions & 0 deletions lib/apartment/active_record/postgresql_adapter.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
# frozen_string_literal: true

# rubocop:disable Style/ClassAndModuleChildren

# NOTE: This patch is meant to remove any schema_prefix appart from the ones for
# excluded models. The schema_prefix would be resolved by apartment's setting
# of search path
module Apartment::PostgreSqlAdapterPatch
def default_sequence_name(table, _column)
res = super
schema_prefix = "#{Apartment::Tenant.current}."
if res&.starts_with?(schema_prefix) && Apartment.excluded_models.none? { |m| m.constantize.table_name == table }
res.delete_prefix!(schema_prefix)
end
res
end
end

require 'active_record/connection_adapters/postgresql_adapter'

# NOTE: inject this into postgresql adapters
class ActiveRecord::ConnectionAdapters::PostgreSQLAdapter
include Apartment::PostgreSqlAdapterPatch
end
# rubocop:enable Style/ClassAndModuleChildren
20 changes: 0 additions & 20 deletions lib/apartment/adapters/postgresql_adapter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ def default_tenant
def reset
@current = default_tenant
Apartment.connection.schema_search_path = full_search_path
reset_sequence_names
end

def init
Expand Down Expand Up @@ -81,7 +80,6 @@ def connect_to_new(tenant = nil)
# there is a issue for prepared statement with changing search_path.
# https://www.postgresql.org/docs/9.3/static/sql-prepare.html
Apartment.connection.clear_cache! if postgresql_version < 90_300
reset_sequence_names
rescue *rescuable_exceptions => e
raise_schema_connect_to_new(tenant, e)
end
Expand Down Expand Up @@ -130,24 +128,6 @@ def postgresql_version
Apartment.connection.send(:postgresql_version)
end

def reset_sequence_names
# sequence_name contains the schema, so it must be reset after switch
# There is `reset_sequence_name`, but that method actually goes to the database
# to find out the new name. Therefore, we do this hack to only unset the name,
# and it will be dynamically found the next time it is needed
descendants_to_unset = ActiveRecord::Base.descendants
.select { |c| c.instance_variable_defined?(:@sequence_name) }
.reject do |c|
c.instance_variable_defined?(:@explicit_sequence_name) &&
c.instance_variable_get(:@explicit_sequence_name)
end
descendants_to_unset.each do |c|
# NOTE: due to this https://github.com/rails-on-services/apartment/issues/81
# unreproduceable error we're checking before trying to remove it
c.remove_instance_variable :@sequence_name if c.instance_variable_defined?(:@sequence_name)
end
end

def schema_exists?(schemas)
return true unless Apartment.tenant_presence_check

Expand Down
10 changes: 9 additions & 1 deletion spec/examples/schema_adapter_examples.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,19 @@
Apartment::Tenant.init

expect(Company.table_name).to eq('public.companies')
expect(Company.sequence_name).to eq('public.companies_id_seq')
expect(User.table_name).to eq('users')
expect(User.sequence_name).to eq('users_id_seq')
end

context 'with a default_tenant', default_tenant: true do
it 'should set the proper table_name on excluded_models' do
Apartment::Tenant.init

expect(Company.table_name).to eq("#{default_tenant}.companies")
expect(Company.sequence_name).to eq("#{default_tenant}.companies_id_seq")
expect(User.table_name).to eq('users')
expect(User.sequence_name).to eq('users_id_seq')
end

it 'sets the search_path correctly' do
Expand Down Expand Up @@ -120,10 +126,12 @@
subject.switch(schema1) do
expect(connection.schema_search_path).to start_with %("#{schema1}")
expect(User.sequence_name).to eq "#{schema1}.#{User.table_name}_id_seq"
rpbaltazar marked this conversation as resolved.
Show resolved Hide resolved
expect(Company.sequence_name).to eq "#{public_schema}.#{Company.table_name}_id_seq"
end

expect(connection.schema_search_path).to start_with %("#{public_schema}")
expect(User.sequence_name).to eq "#{public_schema}.#{User.table_name}_id_seq"
expect(User.sequence_name).to eq "#{User.table_name}_id_seq"
rpbaltazar marked this conversation as resolved.
Show resolved Hide resolved
expect(Company.sequence_name).to eq "#{public_schema}.#{Company.table_name}_id_seq"
end

it 'allows a list of schemas' do
Expand Down