search

contribsys / faktory_worker_ruby

Faktory worker for Ruby
GNU Lesser General Public License v3.0
214 stars 31 forks source link

Fix Faktory::Job.set ignores faktory_options #63

Closed ahacop closed 3 years ago

ahacop commented 3 years ago

Fixes #62.

mperham commented 3 years ago

Does Job.set(queue: "foo").set(queue: "bar").perform_async work as expected? Seems like Setter needs to implement set too.

ahacop commented 3 years ago

Does Job.set(queue: "foo").set(queue: "bar").perform_async work as expected? Seems like Setter needs to implement set too.

Didn't realize that was a thing. How's this?

ahacop commented 3 years ago

One caveat: no deep merge on custom.

mperham commented 3 years ago

This looks wonderful but I think you rightfully point out that the lack of deep merge on the custom attribute is a footgun that will hurt someone eventually. I think we need to have it.

ahacop commented 3 years ago

Yeah, I agree. I can write something.

ahacop commented 3 years ago

Wasn't really sure where to put the deep_merge method, since the Util module seemed more Faktory related.

Also, while writing the deep_merge, discovered that the actual merge in the prior commits was happening in the shallow stringification of the keys in client_push as in { 'foo' => 1, foo: 2 }.stringify #=> { 'foo' => 2 }.

The keys inside of custom are not stringified, however (until, I guess, they are converted to JSON). So, I'm stringifying during deep merge to deal with the case where faktory_options custom: { track: 1 } and SomeJob.set(custom: { 'track' => 2 }).perform_async both occur; otherwise I would just deep_stringify the options passed into set.

ahacop commented 3 years ago

Thanks!

mperham commented 3 years ago

Thank you for not only opening the issue but contributing the fix!