Add initial value support to ActiveSupport::Cache #increment and #decrement calls

[andrejbl]

Summary

ActiveSupport::Cache cache stores increment and decrement calls do not have support for setting an initial value on first call.

This change sets the initial value to the amount provided in the call (or 0 for decrement calls on MemCacheStore as it only supports unsigned integers).

Before this change, to set and increment a counter one needed to do the following:

@cache.write("bar", 1) # with :raw true for `MemCacheStore` and `RedisCacheStore`
@cache.increment("bar", 1)
=> 2
@cache.write("foo", 1)
@cache.decrement("foo", 1)
=> 0

After the change, there is no need for an explicit write to set the initial value:

@cache.increment("baz")
=> 1
@cache.increment("bar", 2)
=> 2
@cache.decrement("foo", 1)
=> -1 # or 0 for `MemCacheStore`

[asavageiv]

Thanks andrejbl!
This alleviates #45069

[byroot]

I'm afraid I don't follow, is this meant to be a fix for #45069 or does it just happens to allow a workaround?

Because by reading that issue, my reasoning is more to fix memory_store and file_store to also start from 1.

As for this specific PR, do you have a use case for setting an initial value? I guess it could be useful if you decrement until 0 ? But if we're going to add this feature, I think we should try to add it to all stores, otherwise their feature set will diverge even more.

[tgwizard]

I had the same idea in #45077, though with a more explicit opt in behaviour to not make the change potentially breaking. There's also potentially confusing behaviour around decrement, as memcached only operates on unsigned integers, so the value cannot be < 0. I think defaulting the initial value there to 0 can work fine there (if we want to add a default initial value)

[asavageiv]

Yeah, I misread this. I would prefer a solution for all stores and one set at the store level instead of the method level. E.g. MemoryStore.init_increments_to_zero = true

…

On Thu, May 12, 2022, 6:02 AM Adam Renberg Tamm ***@***.***> wrote: I had the same idea in #45077 <#45077>, though with a more explicit opt in behaviour to not make the change potentially breaking. There's also potentially confusing behaviour around decrement, as memcached only operates on unsigned integers, so the value cannot be < 0. I think defaulting the initial value there to 0 can work fine there (if we want to add a default initial value) — Reply to this email directly, view it on GitHub <#45068 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAXDJ3KPYNE2LMIDFCP4IXDVJT6PRANCNFSM5VVM22IA> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[andrejbl]

To answer @byroot 's question - this PR is not related to #45069. It just happens to touch the problem there. The initial idea was just to simplify counter initialisation in MemCacheStore.

The point about extending this to other stores makes sense (RedisCacheStore already has this behaviour). We can also ditch the :initial/:default option, and just set the amount as the default (or 0 in case of #decrement). Would this be a more suitable approach? In this case the only "odd" behaviour that deviates would be RedisCacheStore#decrement as Redis sets the counter to -1 in that case.

To @tgwizard 's and @asavageiv comments - yes this might be a breaking change, in case someone made a strong dependency on nil being returned instead, but it does seem rather unlikely as a pattern (perhaps it would be more of an anti-pattern actually). We already had breaking changes in 7.0 in this part of the code.

[byroot]

We can also ditch the :initial/:default option, and just set the amount as the default (or 0 in case of #decrement). Would this be a more suitable approach?

I think so, let's start by making the stores converge a bit more by having them all default to 0 (well 1 if you increment).

For memcached not supporting signed integers, I think it's ok to keep this as a difference, I don't want to gimp the other stores.

Then once that's done, if someone come up with reasonable use cases for an initial parameter, I'm open to consider it.

[asavageiv]

Per #45069, we want to have all the stores default to 0 for decrement and 1 for increment. @andrejbl do you want to implement that or shall I?

[byroot]

we want to have all the stores default to 0 for decrement and 1 for increment.

Just to clarify. I think the behavior that makes the most sense is to assume that if the key doesn't exist, then it's value is 0, then the increment or decrement is applied.

Of course there is what we said about Memcached not supporting negative counters, so it doesn't apply here, but for the others we should do that.

[byroot]

Not sure what to do there.

Just let Memcache be stuck at 0, it's fine.

[andrejbl]

Per #45069, we want to have all the stores default to 0 for decrement and 1 for increment. @andrejbl do you want to implement that or shall I?

I have just pushed another commit on this PR and updated the description. We can use it for further discussion.

[casperisfine]

Some minor documentation things, please squash your commits though.

[casperisfine]

Starting to look good, I'll try to find some time to do a final review. In the meantime please squash your commits, this way if there's no other concerns I can merge directly.

[byroot]

Thank you!