Apartment is not thread-safe with postgres schemas and transactional system tests

maerch commented 4 years ago

First of all thanks for this great gem. A real time saver in our project.

We have some issues with the Apartment gem after integrating use_transactional_fixtures (resp. use_transactional_tests) in our System Specs (RSpec and probably MiniTest as well). It seemed pretty non-deterministic at first, i.e. records disappeared to just reappear at some point. Apartment::Tenant.current always pointed to the correct tenant all the time, though.

After some digging I guess I have found the root cause.

In https://github.com/rails/rails/pull/28083 @eileencodes introduces a feature to enable transactional test for system specs. Basically it ensures that two different threads (in our case the RSpec thread and the Puma thread) share the same connection to enable a rollback of all changes after the test.

Apartment stores its database adapter instance which holds the tenant name inside a thread, but if you use Postgres the schema is stored right inside the connection and since two threads share the same connection we are not thread-safe anymore.

What is happening? In our RSpec thread we switch! to a tenant (e.g. wat). After we make a call to rails Apartment creates a new PostgresqlAdapter instance for the rails thread. It is not aware of the wat tenant in the RSpec thread, thus the previous tenant is public. The request is wrapped in a switch block and at the end it will reset the schema_search_path of the (shared) connection to public – the default tenant name. This will change the search path for the RSpec thread as well which cannot find any record anymore and will have a mismatch between Apartment::Tenant.current and ActiveRecord::Base.connection.schema_search_path.

Also, with more than one Puma thread this will yield random ActiveRecord::RecordNotFound.

I am not sure how to solve or fix this. If you could point me in a direction, I would be willing to work on a PR for this. Or do you think this cannot be solved, thus transactional tests are not compatible with Apartment?

Thanks again!

Steps to reproduce

Create a RSpec system spec (I guess this is true for MiniTest as well, but I haven't tested this)
Configure and switch to a tenant wat in a before block
puts “#{Apartment::Tenant.current}/#{ActiveRecord::Base.connection.schema_search_path}”
Make a request to rails (visit a page which uses an Apartment elevator)
puts “#{Apartment::Tenant.current}/#{ActiveRecord::Base.connection.schema_search_path}”

Expected behavior

It should return wat/wat twice

Actual behavior

It returns wat/wat and then wat/public

System configuration

Database: PostgreSQL 9.6.13
Apartment version: Apartment 2.2.0
Apartment config:

Apartment.configure do |config|
  config.use_schemas = true
  config.excluded_models = %w{ GlobalUser Company CompanyAssignment Tool UserAgent }
  config.tenant_names = lambda { Company.pluck :tenant_name }
  config.seed_after_create = true
  config.tld_length = 0 if Rails.env == 'development'
end

Rails (or ActiveRecord) version: 5.2.2.1 with Puma
Ruby version: 2.6.2

fschwahn commented 4 years ago

@maerch I'm seeing this issue in our test suite after recently upgrading to rails 5.2.3 - do you have a workaround in place?

masterkain commented 4 years ago

I'm interested in this too.

maerch commented 4 years ago

@fschwahn We refactored some specs to avoid tenant switching within a single spec.

In addition we've added a little hack after which the problems disappeared. In the Rspec config we have something like this:

  config.around(:each, :tenant) do |example|
    tenant = example.metadata[:tenant].to_s

    thread_tenant_switch(tenant) do
      example.run
    end
  end

So in a example or example group we could specify the tenant (e.g. "bike") for which we want the test to run:

RSpec.describe "BoM", type: :system, tenant: :bike do
   ...
end

thread_tenant_switch is an additional helper which is used in the configuration to ensure that the default tenant is reset correctly.

  def thread_tenant_switch(tenant)
    tenant = tenant.to_s

    default_tenant = Apartment.default_tenant
    Apartment.default_tenant = tenant

    Apartment::Tenant.switch tenant do
      yield
    end

    Apartment.default_tenant = default_tenant
  end

That's the gist as far as I remember.

JeremiahChurch commented 4 years ago

following up on @maerch 's EXCELLENT note - this belongs in the wiki as the setup that is there is very dated now.

I wasn't able to fully get rid of DB cleaner - but it's only needed at the beginning of the suite - otherwise, out of the box rspec using rails system specs - here's the entire support file for the next person that ends up down this rabbit hole:

RSpec.configure do |config|
  # configuration to get us setup and testing in the right apartment
  # https://github.com/influitive/apartment/wiki/Testing-Your-Application

  config.before(:suite) do
    # Clean all tables to start - wasn't able to get around this with system tests & transactions - only used in the beginning
    DatabaseCleaner.clean_with :truncation

    # will need to add any other tenants added as part of tests here
    Apartment::Tenant.drop('test-app') rescue nil
    # Create the default tenant for our tests
    Company.create!(name: 'Test Company!', subdomain: 'test-app')
  end

  # https://github.com/influitive/apartment/issues/615
  # without this we always ended up outside of the tenant - inside of rspec - the screenshots were good, rspec was in public
  config.around(:each, type: :system) do |example|
    tenant = tenant.presence ? example.metadata[:tenant].to_s : 'test-app'

    thread_tenant_switch(tenant) do
      example.run
    end
  end
end

def thread_tenant_switch(tenant)
  tenant = tenant.to_s

  default_tenant = Apartment.default_tenant
  Apartment.default_tenant = tenant

  Apartment::Tenant.switch tenant do
    yield
  end

  Apartment.default_tenant = default_tenant
end

influitive / apartment