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 test_tenant leaking into current_tenant #278

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

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