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: strip schema from sequence name #143

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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!(schema_prefix)
end
res
end
end

require 'active_record/connection_adapters/postgresql_adapter'

class ActiveRecord::ConnectionAdapters::PostgreSQLAdapter
include Apartment::PostgreSqlAdapterPatch
end
Comment on lines +12 to +16
Copy link
Contributor Author

@fsateler fsateler Jan 6, 2021

Choose a reason for hiding this comment

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

This is of course super ugly, and will crash when pg is not available, but I didn't know how to only patch this when postgresql is in use. Guidance appreciated

Copy link
Contributor

Choose a reason for hiding this comment

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

For this i think we could require only when loading the postgresql schema adapter.
That being said, shouldn't we at this point simply make use of the active record reset_schema_name method when switching tenant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem with reset_sequence_name is twofold:

  1. It goes to the db to find it https://github.com/rails/rails/blob/ecbd92cf48c741f784c1297041d368c8eb14e9bf/activerecord/lib/active_record/model_schema.rb#L357-L360.
  2. Modifying the sequence name is not hread safe: Postgres sequence_name correction is not thread safe  #161

Thus I believe the safest fix is to actually remove the schema from the sequence name

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"
Copy link
Contributor

Choose a reason for hiding this comment

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

Was there anything particularly wrong with this test? Shouldn't these assumptions still be valid?

Copy link
Contributor

@rpbaltazar rpbaltazar Sep 12, 2021

Choose a reason for hiding this comment

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

Ok, after starting to make some tests with my codebase and looking at the changes made, I now understand.
The previous changes tried to fix this issue, but ended up introducing other bugs while fixing this problem.
So, please correct me if i'm wrong, but i think we'll need to merge these changes, that will revert the state to "broken".
Release 2.9.1.

Then we can merge the changes i'm suggesting in #169 (assuming it looks ok), document the apartment model logic and release the 3.0.0.
Agree?

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 believe this PR solves the problem better, because default_sequence_name involves a trip to the database.

Copy link
Contributor

@rpbaltazar rpbaltazar Sep 13, 2021

Choose a reason for hiding this comment

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

Got it. I did a couple of tests and as long as we have the correct search path set, it should be ok and we should be getting the correct sequence. Let's go ahead w your solution.

I still think that having the logic on a concern that we include in our models would be a simpler solution than trying to manage this changes only when using postgres

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 still think that having the logic on a concern that we include in our models would be a simpler solution than trying to manage this changes only when using postgres

I'd be willing to explore that but I'm not sure exactly what you have in mind. Do you mean something we extend into ActiveRecord::Base?

Copy link
Contributor

Choose a reason for hiding this comment

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

Essentially something along these lines: https://github.com/rails-on-services/apartment/pull/169/files#diff-19a1d0c8bf6f60ce84cd210d933378fb77dd60a56232b05b0d2b89c1979c6331R8

I've adjusted the PR to follow your approach but with what i was suggesting

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