search

ErwinM / acts_as_tenant

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

Serialize tenant to string instead of GlobalID #326

Closed ziadsawalha closed 6 months ago

ziadsawalha commented 6 months ago

PROBLEM: Upgrading to acts_as_tenant 1.0 started raising this error:

ArgumentError: Job arguments to Turbo::Streams::ActionBroadcastJob must be native JSON types, but #<GlobalID:0x000000010f555540 @uri=#<URI::GID gid://equipped/Account/1>> is a GlobalID. (ArgumentError)
See https://github.com/sidekiq/sidekiq/wiki/Best-Practices
To disable this error, add `Sidekiq.strict_args!(false)` to your initializer.

            raise(ArgumentError, msg)

POSSIBLE CAUSE: 6ecf5d2d2e74338362142f5a52464a65ee6dae7c introduced the breaking change.

SUGGESTED FIX: Sidekiq best practices prefer JSON types and GlobalID::Locator.locate works with the string representation of GlobalID (which is a unique URI). This keeps the desired change of commit 6ecf5d2d2e74338362142f5a52464a65ee6dae7c, but is more compatible with Sidekiq (and requires less serialization overhead)

The fix is included in this PR.

excid3 commented 6 months ago

Thanks @ziadsawalha 👍

excid3 commented 6 months ago

We should probably add a test for this.

ziadsawalha commented 6 months ago

Saw this was released in 1.0.1. Thanks!

Do you still want a test for this? What would we test... that we always cast job args to JSON types or just that we cast GID?

excid3 commented 6 months ago

I expected the Sidekiq tests to already cover this case, so we'd probably want a test that replicates the error (before this fix) and that would be good enough.

ziadsawalha commented 6 months ago

327 submitted. Some of the CI failures don't seem to be related to the change, though (errors while installing gems?) Touched the last commit and CI passing now, so CI issues were transient.