Skip to content

Commit

Permalink
fix: strip schema from sequence name
Browse files Browse the repository at this point in the history
sequence_name is shared across threads because it is stored in a class
ivar. This means that on threaded servers, the sequence_name might point
to a different tenant, if another tenant made the switch at the right
time. This race is likely the cause of
#81 as well.

This reverts commit f8eefc4.
  • Loading branch information
fsateler committed Jan 6, 2021
1 parent 30f08c4 commit 7eda570
Show file tree
Hide file tree
Showing 4 changed files with 23 additions and 22 deletions.
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'
if ActiveRecord.version.release >= Gem::Version.new('6.0')
require_relative 'apartment/active_record/connection_handling'
end
Expand Down
16 changes: 16 additions & 0 deletions lib/apartment/active_record/postgresql_adapter.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
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!("#{Apartment::Tenant.current}.")
end
res
end
end

require 'active_record/connection_adapters/postgresql_adapter'

class ActiveRecord::ConnectionAdapters::PostgreSQLAdapter
include Apartment::PostgreSqlAdapterPatch
end
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 @@ -83,7 +82,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
raise TenantNotFound, "One of the following schema(s) is invalid: \"#{tenant}\" #{full_search_path}"
end
Expand Down Expand Up @@ -131,24 +129,6 @@ def postgresql_version
# public from Rails 5.0.
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
end

# Another Adapter for Postgresql when using schemas and SQL
Expand Down
8 changes: 6 additions & 2 deletions 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(User.table_name).to eq('users')
expect(Company.sequence_name).to eq('public.companies_id_seq')
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(User.table_name).to eq('users')
expect(Company.sequence_name).to eq("#{default_tenant}.companies_id_seq")
expect(User.sequence_name).to eq('users_id_seq')
end

it 'sets the search_path correctly' do
Expand Down Expand Up @@ -121,11 +127,9 @@
it 'connects and resets' do
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"
end

expect(connection.schema_search_path).to start_with %("#{public_schema}")
expect(User.sequence_name).to eq "#{public_schema}.#{User.table_name}_id_seq"
end
end

Expand Down

0 comments on commit 7eda570

Please sign in to comment.