JCache Cache.putAll(...) with immutable map can cause exception

[kdombeck]

Added test case showing that calling javax.cache.Cache.putAll(Map) with an immutable map causes a UnsupportedOperationException "if" the CacheWriter throws an exception. The problem with this is that you loose the original exception from the CacheWriter and thus do not know what the root cause was.

The issue appears to be with CacheProxy in that it tries to remove entries from the original map. Which in this case is immutable.

Seems like similar issue in CacheProxy.deleteAllToCacheWriter

I can change my code to be a mutable map but wanted to see what your thoughts were on this issue.

[CLAassistant]

All committers have signed the CLA.

[ben-manes]

This is one of those JCache design surprises that is poorly documented and explained. In CacheWriter it states,

* @param entries a mutable collection to write. Upon invocation, it contains
*                the entries to write for write-through. Upon return the
*                collection must only contain entries that were not
*                successfully written. (see partial success above)

This is not stated on the Cache api, so I wasn't sure what the expectation is for the vendor (perform a copy or pass it in directly so that the caller can also inspect these results?). It looks like I thought this was required (either due to the TCK or forum discussions) and wrote a unit test to assert what I thought was required.

For the removeAll(keys) I did the defensive copy as that passes the latest TCK. When I add similar to putAll(map) it seems to pass now too, so I think it must be okay now to change this. When I run older TCKs a few tests fail (since it asserted buggy behavior of the spec lead's implementation divergences), but I can't tell if this was entirely my goof or what I felt forced into doing. Either way a mutable copy has to be made somewhere due to how their API was defined.

caffeine/jcache/src/main/java/com/github/benmanes/caffeine/jcache/CacheProxy.java

Lines 728 to 729 in 412e0d0

	Set<K> keysToRemove = new HashSet<>(keys);
	CacheWriterException e = deleteAllToCacheWriter(keysToRemove);

[kdombeck]

Thanks for the response. I completely missed that documentation.

I was looking at Cache.putAll(...) since that is the public interface most people will be using.

I must say I am a bit surprised to see a parameter being mutated thou since it seems fairly common for things to be immutable for caching. Thanks for pointing me to that documentation!!!

[ben-manes]

It is very convoluted. If the writer does not throw an exception then it is successful and we ignore any mutations done to entries (so all are put into the cache). If it is thrown, then the implementer must also mutate the mapping to communicate correctly for partial success. You would more typically see an exception carry this information, not the argument. Unfortunately another quirk is that CacheWriter is incompatible with the annotation-style AOP caching, since the synthetic key is a private data class of the spec's implementation. Also note that since it was rejected for inclusion by JEE and Jakarta, it seems to be a dead (jsr107/jsr107spec#415).

[ben-manes]

Released in 3.1.3