feat(disable_overwrite_on_set): Add ability to disable overwrites when setting an item in the cache

[patpicos]

Add ability to disable overwrites when setting an item in the cache

• Added new option & associated test
• Added functionality in cache.set() to return the item if overwrite is disabled. Could be done as an else.. but wanted to make explicit up front at top of func.

Not sure if more tests need additional testing. I didn't a chance to review all existing cases / structure of testing framework

Addresses issue / enhancement request https://github.com/jellydator/ttlcache/issues/78

[patpicos]

local tests

go test .
ok      github.com/jellydator/ttlcache/v3       4.030s

[swithek]

@patpicos Thanks for the PR and sorry for the delay.

I can see how this addition is useful, however, it might raise a few issues as well:

• The Set() method acts as a constructor and returns only the item instance it creates. If it were to return existing cache instances, its purpose would change which might lead to more confusion later (e.g., what if the cache with this new option is created in one package and used in another, it might not be clear why the Set() method acts differently without seeing how it's created).
• With this change, the Set() method also acts similarly to the Get() method, however, it doesn't touch/extend expiration timestamps like the Get method. Again, the purpose of the Set() method is unclear.

I understand that you need to check if a specific item exists before setting a new one in a non-racy way. This is a perfect situation for a transaction of some kind. We have an issue for this as well: #72. Any help with that would be appreciated.

[patpicos]

The transaction sounds interesting, but I think overkill from a cache perspective. When I think of a cache it is meant to be atomic on the key. A transaction would imply I want to perform actions on multiple keys and ensure they are modified as a group. This moves away from an atomic key and starts to look like a database. I'm not discounting the idea; but seems to blow up a bit the cache concept.

What I am proposing is a new behavior of first Set() wins and the value within that key can be be updated accordingly, but the key's value is not replaceable, only updatable.

Regarding the updating the TTL on cache entries. on a Set() operation if not overwriting the value; Am I resetting the clock or not...not sure where to take that piece. I primarily use the TTL to maintain the size a "bounded" size or roll off some cache entries faster than others. I am open on approaches to TLL management for this added option. IMO this is an option you subscribe to, not a default.

[swithek]

Yes, the transaction idea needs to be polished or at least limited in scope.

As for your proposed changes and their use cases, I think what you have in mind resembles sync.Map's LoadOrStore() method. A dedicated method like that would have a much clearer purpose.

The signature of such a method in this package could be:

func (c *Cache[K, V]) GetOrSet(key K, value V, ttl time.Duration, opts ...Option[K, V]) *Item[K, V])

Though I have to note that merging Get() and Set() makes the func look slightly cluttered.

[patpicos]

The LoadOrStore() does perform the intent that I have. But instead of a combine function, I define the behavior as an option you subscribe.

Based on the feedback above, I updated the PR to reflect that a key can be overwritten (default behavior) or never overwritten if using this new option. I don't think this additional behavior pollutes the intent of a ttlcache and is a useful enhancement should the user need that capability.

Also, since the Set() does a fetch, it made sense to update the expiration like the existing overwrite

How do you propose we move forward? Curious of Rene's feedback

[swithek]

I'm still not entirely convinced that the Set() method should contain this extra logic, especially if the method's purpose is changed.

Another possible problem: most of the Get() options can be passed into Get() or used as global options when creating a new cache and as a user I would expect Set() to work the same way. However, this problem cannot be fixed by adding another parameter to Set() as that would be a breaking change.

As I see it, the only way forward is to have a new method that would wrap both Set() and Get() or start a transaction/lock a mutex of some kind.

[patpicos]

fair enough. I will proceed with keeping this change in my fork for my uses.