ErwinM / acts_as_tenant

Easy multi-tenancy for Rails in a shared database setup.
MIT License
1.56k stars 264 forks source link

Replace RequestStore dependency with CurrentAttributes #313

Closed excid3 closed 10 months ago

excid3 commented 1 year ago

This simplifies ActsAsTenant by dropping the RequestStore dependency. We can use Rails' built-in CurrentAttributes feature that accomplishes the same thing.

CurrentAttributes was introduced in Rails 5.2 and we already require 5.2 or higher, so this should not be a breaking change. While this should be a drop-in replacement, it'd be nice to have this tested in several applications before releasing the change. If you have a chance to test this in your application(s), please do!

The only minor gripe I know of is that CurrentAttributes doesn't play well with the web-console gem because those requests are not in the same thread.

tmaier commented 1 year ago

Great idea. Is it possible to go farther?

I am considering to get rid of the classic “current_user” and “current_tenant” methods in my code and to introduce my own Current class instead. I thinks such an would reduce even more complexities at this gem.

However, everyone would be forced to set this up themselves, when they use the gem and acts_as_tenant would need to know the name of the Current class the individual app uses and how the attributes are called…

excid3 commented 1 year ago

However, everyone would be forced to set this up themselves, when they use the gem and acts_as_tenant would need to know the name of the Current class the individual app uses and how the attributes are called…

Users of acts_as_tenant would still use ActsAsTenant.current_tenant. This only refactors the internal storage.

pond commented 10 months ago

I just wrote my own version of this several months later for the same reason (we finally got around to moving our Rails app over from RequestStore). In particular, if you're using AAT & CurrentAttributes, the reset behaviour in test mode isn't the same and this was actually giving us compatibility headaches.

I'd really like to see this merged ASAP! In the mean time I'm monkey patching :-/

@excid3 One note - you don't need your inner Current class to include attribute test_tenant. It's not used.

pond commented 10 months ago

🎉 Thank you! 😄