Skip to content

Conversation

ziadsawalha
Copy link
Contributor

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:
6ecf5d2 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 6ecf5d2, but is more compatible with Sidekiq (and requires less serialization overhead)

The fix is included in this PR.

Sidekiq best practices prefer JSON types and
GlobalID::Locator.locate works with the string
representation of GlobalID (which is a unique URI)
@excid3 excid3 merged commit 5ef6d28 into ErwinM:master Dec 13, 2023
@excid3
Copy link
Collaborator

excid3 commented Dec 13, 2023

Thanks @ziadsawalha 👍

@excid3
Copy link
Collaborator

excid3 commented Dec 13, 2023

We should probably add a test for this.

@ziadsawalha
Copy link
Contributor Author

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
Copy link
Collaborator

excid3 commented Dec 18, 2023

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
Copy link
Contributor Author

ziadsawalha commented Dec 18, 2023

#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.

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.

2 participants