Skip to content

Conversation

artemave
Copy link

@artemave artemave commented Mar 15, 2022

Using test_tenant in conjunction with with_tenant {} and without_tenant {} in some cases leads to test_tenant being assign to current_tenant which leads to subtle bugs, that are difficult to pin down.

This happens because the value of test_tenant is restored as current_tenant in the ensure part of those methods. The test cases in this pr illustrate the issue.

@excid3
Copy link
Collaborator

excid3 commented Mar 15, 2022

I'm pretty sure that's the expected behavior: https://github.com/ErwinM/acts_as_tenant#testing

@refractalize
Copy link

Hi @excid3

Just to give a bit more explanation to what's going on here. We start with test_tenant set to test_account, this behaves like this:

# start (set test_tenant = test_account)
RequestStore.store[:current_tenant] == nil
ActsAsTenant.test_tenant == test_account
ActsAsTenant.current_tenant == test_account

Then we might do a request, using ActsAsTenant::TestTenantMiddleware, which sets test_tenant to nil during the request. So far, everything is still fine.

# start request with ActsAsTenant::TestTenantMiddleware (set test_tenant = nil)
RequestStore.store[:current_tenant] == nil
ActsAsTenant.test_tenant == nil
ActsAsTenant.current_tenant == nil
# finish request with ActsAsTenant::TestTenantMiddleware (reset test_tenant = test_account)

Now we do a with_tenant block. This is where things start to go wrong.

ActsAsTenant.with_tenant(account1) do
  RequestStore.store[:current_tenant] == account1
  ActsAsTenant.test_tenant == test_account
  ActsAsTenant.current_tenant == account1
end

At the end of with_tenant, the RequestStore.store[:current_tenant] is set to the test_tenant... but, it still behaves correctly, for now.

RequestStore.store[:current_tenant] == test_account # <--- not correct!
ActsAsTenant.test_tenant == test_account
ActsAsTenant.current_tenant == test_account # <--- but still ok...

We do another request, again test_tenant is set to nil, BUT this time RequestStore.store[:current_tenant] is set, so current_tenant looks like it's set!

# start request with ActsAsTenant::TestTenantMiddleware (set test_tenant = nil)
RequestStore.store[:current_tenant] == test_account
ActsAsTenant.test_tenant == nil
ActsAsTenant.current_tenant == test_account # wrong!
# finish request with ActsAsTenant::TestTenantMiddleware (reset test_tenant = test_account)

In short, what we're seeing is that with_tenant sets the RequestStore.store[:current_tenant] and doesn't reset it afterwards, which affects any further requests that rely on ActsAsTenant::TestTenantMiddleware. In particular, requesting routes that don't expect a tenant (i.e. routes that show multiple tenants) now have a tenant set.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants