Handle map representation of cache in SafeCaches

[kokosing]

Handle map representation of cache in SafeCaches

[findepi]

@kokosing the CI is unhappy

[findepi]

add assertion that cache gets emptied by each of these

[kokosing]

#14653 (comment)

I don't think so. SafeCaches provides forwarding implementation of cache, so it is up to a delegate cache to clear things. So testing if methods are properly implemented should be on a different level, where a delegate cache implementation is.

I would like to implement this anyway, but the challenge is that it is not clear how to put/load data into the cache. Without it I cannot verify if clear clears.

[findepi]

So testing if methods are properly implemented should be on a different level

we don't have tests for SafeCaches's wrappers, which was a mistake. You're adding them now.

where a delegate cache implementation is.

We could want a "smoke test" to verify things generally work. I.e. what if SafeCaches wrapper missed a call to delegate?

Other than that, I agree there is no point in fine grained tests, we don't want to test underlying implementation here, nor concurrency, etc.

[findepi]

but the challenge is that it is not clear how to put/load data into the cache.

maybe we can have something like

private static void verifyClearIsPossible(Cache<Object, Object> cache)
        throws Exception
{
    Object key = new Object();
    Object firstValue = new Object();
    cache.get(key, () -> firstValue);
    assertSame(cache.getIfPresent(key), firstValue);

    cache.invalidateAll();
    assertNull(cache.getIfPresent(key));

    Object secondValue = new Object();
    cache.get(key, () -> secondValue);
    assertSame(cache.getIfPresent(key), secondValue);
    cache.asMap().clear();
    assertNull(cache.getIfPresent(key));
}

[kokosing]

Your example assumes that loading is possible which is no always a case.

Anyway I follow you suggestion, and I will change it later if needed (NonLoadableCache, however it is a different story)

[findepi]

This one may actually be safe? 🤔

[kokosing]

I am not sure.

[findepi]

If we forbid without being sure, we will appear as if being sure.

I am fine with forbidding this, but let's have a comment that we don't know what we're doing.

[kokosing]

AC

Please see Expose NonKeyEvictableLoadingCache::unsafeInvalidate which I had to add as there was actually map used to by bypass forbidden invalidation.

[kokosing]

#14653 (comment)

I don't think so. SafeCaches provides forwarding implementation of cache, so it is up to a delegate cache to clear things. So testing if methods are properly implemented should be on a different level, where a delegate cache implementation is.

I would like to implement this anyway, but the challenge is that it is not clear how to put/load data into the cache. Without it I cannot verify if clear clears.

[kokosing]

I am not sure.

[findepi]

assert on the message