Skip to content

ContainsOrAdd functions#22

Open
Tehhs wants to merge 6 commits intogo-pkgz:masterfrom
Tehhs:master
Open

ContainsOrAdd functions#22
Tehhs wants to merge 6 commits intogo-pkgz:masterfrom
Tehhs:master

Conversation

@Tehhs
Copy link
Copy Markdown

@Tehhs Tehhs commented Oct 11, 2025

PR should hopefully address #17

Changes

  • Adds ContainsOrAdd(key, value) (contains, evicted) in an attempt to match hashicorps ContainsOrAdd function
  • Adds similar function ContainsOrSet(key, value, ttl) (contains) to V1 & V2 as they don't really have a concept of "Add" which returns a boolean if evicted like V3. Not sure about this part, let me know.
  • Unit tests

Testing

All unit tests pass in V1, V2 & V3.

@Tehhs Tehhs marked this pull request as ready for review October 11, 2025 01:27
@Tehhs Tehhs requested a review from umputun as a code owner October 11, 2025 01:27
Copy link
Copy Markdown
Collaborator

@paskal paskal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution! The overall approach is good — atomic check-and-add without external locking is a useful feature. A few things need to be addressed:

  1. Expired item check missing — In all versions, ContainsOrSet/ContainsOrAdd checks if _, ok := c.items[key]; ok but doesn't verify whether the item is expired. This means calling the method on an expired key would incorrectly return "contains=true" and skip the add, even though a subsequent Get() would return not-found. The check should also verify expiration, e.g.:

    if ent, ok := c.items[key]; ok {
        if !time.Now().After(ent.Value.(*cacheItem[K, V]).expiresAt) {
            return true, false
        }
    }
  2. Test naming inconsistency — The test is named TestCacheContainsOrAdd in all versions, but v1 and v2 test ContainsOrSet. The test names should match the method names being tested (TestCacheContainsOrSet for v1/v2).

  3. Missing test cases:

    • No test for expired key behaviour (calling the method on a key that exists but is expired — should treat it as not-present and add the new value)
    • No test for TTL override in ContainsOrSet (v1/v2)
    • No concurrency test for the new method
  4. v1 interface change — Adding ContainsOrSet to the Cache interface in v1 is a breaking change for anyone who implements the interface externally. I would rather avoid it, maybe let's just add it to v3 if there is no way around it.

The naming split (v1/v2 use ContainsOrSet with TTL param, v3 uses ContainsOrAdd matching hashicorp's signature) makes sense given the API differences between versions — v3 has Add() while v1/v2 only have Set().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants