Fix: Ensure date_as_argument option can be set from true to false in Sidekiq Cron jobs

[satyakampandya]

Summary

This pull request fixes an issue where the date_as_argument option in Sidekiq Cron jobs cannot be changed from true to false once it has been set to true. The issue occurs because the Redis key for date_as_argument is not removed when the configuration is updated.

Changes

• Added logic to delete the date_as_argument field from Redis when date_as_argument is set to false.
• This ensures that stale values in Redis are cleared, allowing configuration changes to take effect as expected

Fixes: #472

[markets]

I have a question @satyakampandya about this: Why this attribute needs special treatment compared to the others https://github.com/sidekiq-cron/sidekiq-cron?tab=readme-ov-file#job-properties?

[satyakampandya]

@markets If you take a look at this PR https://github.com/sidekiq-cron/sidekiq-cron/pull/392/files#diff-45ac47a9faa0840538f2cae00fdd31667a33fb6b4f94a7428d9c80bceaf8a87eL407, it introduces these special conditions to resolve a version compatibility issue. I’m not entirely sure of the reasoning behind that PR.

In this current PR, if date_as_argument is set to false, we make sure to delete the date_as_argument field from the redis_key.

[markets]

Ok! I'd suggest to add a test for this change.

[markets]

@satyakampandya @freemanoid I don't remember all the history behind #392 (it seems it fixes a quite relevant issue #350, but can't remember either all the reasoning behind).

My concern about this PR is that now date_as_argument seems like an special attribute (maybe it is?), in the sense that other attributes aren't never deleted from Redis.

@iwaseasahi since you reported the target issue, could you please try this branch? 🙏🏼

[satyakampandya]

I agree with @markets that, as long as PR #392 is still relevant, we need to maintain the special handling for date_as_argument.

However, if @iwaseasahi confirms that PR #392 is no longer applicable, we could revert to a simpler approach where date_as_argument is treated like other attributes,

date_as_argument: date_as_argument? ? "1" : "0"

Let's await @iwaseasahi's feedback on this.

[freemanoid]

@satyakampandya @freemanoid I don't remember all the history behind #392 (it seems it fixes a quite relevant issue #350, but can't remember either all the reasoning behind).

If that special handling is only for the backward compatibility with 1.6.0 then we can ignore it in 2.0.0, right?

[iwaseasahi]

@satyakampandya @markets Thank you for your pull request and review! It worked correctly with this branch.

[markets]

I'm a bit afraid to revert back to:

date_as_argument: date_as_argument? ? "1" : "0"

As it introduced a lot of problems in the past, I think this is due people having jobs defined in this way def perform instead of def perform(*args). That raises:

ArgumentError wrong number of arguments (given 1, expected 0)

So, probably this is finally the good fix and the one we should merge!

[freemanoid]

I think this is due people having jobs defined in this way def perform instead of def perform(*args). That raises:

ArgumentError wrong number of arguments (given 1, expected 0)

So, probably this is finally the good fix and the one we should merge!

This is unlikely, because when date_as_argument: 0 it shouldn't pass a date to the job.

[markets]

date_as_argument: "0" evaluates to a "truthy" value, see this comment: https://github.com/sidekiq-cron/sidekiq-cron/issues/350#issuecomment-1409798837.

So if we revert back that code #392, we'll probably need to fix that too (~> https://github.com/sidekiq-cron/sidekiq-cron/blob/v1.6.0/lib/sidekiq/cron/job.rb#L282).

[freemanoid]

date_as_argument: "0" evaluates to a "truthy" value, see this comment: #350 (comment).

Is it true for the latest version or only for 1.6.0?

As I see this problem, in 1.6.0 we used to check only for existance of date_as_argument, so any value, even "0" was evaluated into true

https://github.com/sidekiq-cron/sidekiq-cron/blob/5f6bd48fbcd9cfd041394035294c3e2ecd4beab4/lib/sidekiq/cron/job.rb#L282

Then in 1.7.0 it has been fixed to properly parse only truthy values into true https://github.com/sidekiq-cron/sidekiq-cron/pull/329/files#diff-45ac47a9faa0840538f2cae00fdd31667a33fb6b4f94a7428d9c80bceaf8a87eR293

And it's still how it works in current master https://github.com/sidekiq-cron/sidekiq-cron/blob/65020e5ff3ebd849516a925206aca523682dd64c/lib/sidekiq/cron/job.rb#L56

It makes me think that the problem you're mentioning is only relevant to a migration from 1.6.0 to newer versions.

[markets]

Thanks for the investigation @freemanoid! Then, it seems safe to just revert #392 🤔

@satyakampandya Could you please take a look and update this PR accordingly? After this is done, would be nice if @iwaseasahi can make a second test with this branch.

[satyakampandya]

@markets I've reverted the changes introduced in PR #392. @iwaseasahi Could you please test this branch?

[iwaseasahi]

@satyakampandya @markets It worked correctly with this branch again. Thanks！

[markets]

It worked correctly with this branch again.

Nice 👌 thanks for confirming @iwaseasahi!

So before merging, I think we just need to update a bit the current tests @satyakampandya (good point noted by @freemanoid).

[satyakampandya]

@markets I'll update the PR tomorrow.

[markets]

@satyakampandya @freemanoid I'd like to merge this one before the next release candidate. I think we all agree with the changes in lib/sidekiq/cron/job.rb, but I'm not sure if the current tests are ok. Some questions/suggestions:

• no need to write "Sidekiq Cron" in the titles/descriptions
• it seems we are just using the same "asserts" in all test cases 🤔

[satyakampandya]

• no need to write "Sidekiq Cron" in the titles/descriptions

I'll update the PR 👍🏻

• it seems we are just using the same "asserts" in all test cases 🤔

@markets Isn't it required to verify whether changing from true to false, or vice versa, is handled successfully?

[markets]

Ah! sorry didn't realize you were checking all the possible transitions.