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

Serialize tenant to string instead of GlobalID #326

Merged
merged 1 commit into from
Dec 13, 2023

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
27 checks passed
@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