-
-
Notifications
You must be signed in to change notification settings - Fork 199
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
Ensure Concurrency Keys are string-like and return a better error when they cannot be cast to a string #791
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Earlopain Thinking about this, it's too much of a breaking change: objects that previously could be coerced to a string will start raising exceptions.
Simply doing the coercion would avoid the issue you had of the value failing to cast (though still leaves open it not being understood that a complex object is being stringified)
raise ArgumentError, "Concurrency key must be a String, was #{key.class}" unless key.is_a?(String) | ||
|
||
key |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
raise ArgumentError, "Concurrency key must be a String, was #{key.class}" unless key.is_a?(String) | |
key | |
key.to_s |
I don't think doing class ExampleJob < ApplicationJob
include GoodJob::ActiveJobExtensions::Concurrency
good_job_control_concurrency_with(perform_limit: 1, key: -> { arguments.first })
def perform(arg)
sleep 60
end
end
ExampleJob.perform_later(ExampleModel.first)
# key: "#<ExampleModel:0x00007f892b76bea0>"
ExampleJob.perform_later(ExampleModel.first)
# key: "#<ExampleModel:0x00007f892d66ee38>" Both jobs would execute in parallel. I agree on it being a breaking change. I have no issues with this being put on the backlog for v4, whenever that might be. |
Hmmm. I don't want to require it to be ...though maybe that isn't as infinite as I feared and it would be fine to typecheck those. I also wonder if it would be possible to catch the bad cast exception and raise an easier to understand error. |
I like the approach of just catching the error. I changed the commit to do just that. |
…is always validated early in enqueue callback
Closes #784
If you pass something like a Hash or Array as the concurrency key, the resulting stack trace doesn't make it very clear what you did wrong:
This made me believe that I'm not allowed to pass a Hash to the job itself. Now the error is much clearer:
This is technically a breaking change. Some types like Number, Time and Symbol were being cast to a String by rails automatically, this now raises an error. The readme says that the concurrency key must be a String, but it would be best for this to go in the next major version if accepted.