Cacheable javadoc wrongly explains how cache keys are generated [SPR-11736]

[spring-projects-issues]

Lari Hotari opened SPR-11736 and commented

In Cacheable's javadoc , it says:

Annotation indicating that a method (or all the methods on a class) can be cached.
The method arguments and signature are used for computing the key while the returned instance is used as the cache value.

Method signature is ignored in SimpleKeyGenerator .

Perhaps the target instance's identityHashCode should be part of the key besides the method signature in SimpleKeyGenerator by default.

I'll send a PR which now contains the broken unit test for this problem.

---

Affects: 4.0.3

Issue Links:

• Caching Abstraction ignores method name? [SPR-8933] #13573 Caching Abstraction ignores method name?

Referenced from: commits 90081f7

[spring-projects-issues]

Craig commented

I don't think that using System.identityHashCode as part of the cache key is a viable solution because then the key would vary by JVM which would break distributed caching.

[spring-projects-issues]

Lari Hotari commented

failing test case
lhotari@2ca8cdd

[spring-projects-issues]

Lari Hotari commented

@Craig, good point.

[spring-projects-issues]

Lari Hotari commented

Pull request sent for the failing test #531

[spring-projects-issues]

Juergen Hoeller commented

This is maybe not ideally worded in the javadoc, since a KeyGenerator implementation may consider the signature but doesn't have to.

In any case, we should consider a more extensive key generation variant which does consider the signature - possibly as an optional KeyGenerator variant next to SimpleKeyGenerator.

Juergen

[spring-projects-issues]

Stéphane Nicoll commented

This is actually a documentation issue, @Cacheable Javadoc definitely needs to be rephrased. I'll take care of that. Thanks for the report!

[spring-projects-issues]

Craig commented

I think that the current implementation and defaults in Spring violates the principle of least surprise. I think that including target signature information in the cache key is the way users would expect caching to work, so I think that in addition to providing the KeyGenerator that does so, that KeyGenerator should be the default.

I've submitted a pull request with a new key generator, updates to make that the default key generator (including updating the documentation), and cherry-picked Lari's test commit.

Note that there are test failures (49 to be precise). Those tests rely on knowing the cache key that a cacheable method invocation will use - so I think the solution is to either:

1. Make those tests use SimpleKeyGenerator so their assumption about the key remains valid
2. Change the tests so they don't rely on knowing what key will be used

#532

[spring-projects-issues]

Stéphane Nicoll commented

I actually still don't understand what the problem might be. Can you give a concrete example of why you believe the current implementation is wrong and how your proposal will fix that? Thanks.

[spring-projects-issues]

Craig commented

I think Lari's test demonstrates the problem pretty well. The problem is that if two methods of totally unrelated instances of unrelated classes are marked as @Cache and the first is called with the same argument as the second, then when the second method is called it will return (unexpectedly and incorrectly) the return value of the first method.

[spring-projects-issues]

Lari Hotari commented

I'm just wondering how could you use @Cacheable (backed by SimpleKeyGenerator) at the class level (type annotation) if the cache key doesn't include the method name as part of it?

Perhaps the title of this issue is misleading? Perhaps we have a different understanding of "method signature" that's in the title?
In this case it isn't about the types of the method parameters. The class name and method name is more important. "method signature" was mentioned in the Cacheable javadoc and that's why I called the identity of the method as "method signature".

Does this clarify anything?

[spring-projects-issues]

Stéphane Nicoll commented

Well. I am sorry to insist guys, but to me this looks like a wrong use of the API which is why I asked for a concrete example (and not a cacheand cacheAnother kind of methods).

What seems to be lost here is that the cache to impact is part of the identity. Let me try to phrase that with an example which I believe do make sense. Let's say you want to cache books and users. These are totally unrelated things but they are both identified by a Long id. Let's say that 120L returns Harry Potter and the "John Smith" user. You have something like this

@Cacheable("container")
public class Foo {

   public class Book getBook(Long id} { ... }
   public class User getUser(Long id) { ... }

This seems really a weird use of caching IMO because your users and your books should be in different regions. Even the class Foo is weird as it returns totally unrelated things that are identified in the same way.

What you'll get usually is a BookRepository and a UserRepository and each would have a @Cacheable annotation with different caches. That's how I would expect the vast majority of use cases to work.

Craig is summarizing it quite well actually. Unless your use case is to put "anything" in a unique cache, unrelated stuff should work with unrelated caches. If you remove this abstraction altogether, I would expect some API like "give me the user cache and on it do something like userCache.get(120L)". I am not expecting to pass a method signature in there.

To answer Lari, the purpose of that annotation is to mention that a service is working for a certain cache. So, on the book repository you mention you are working with the "books" cache. In a less naive option and because you may need to put different things in the same cache, I believe that you are responsible to provide the necessary definition of a unique key and the SPeL support is exactly meant for that. If you ever decided to cache books and users in the same cache, so be it but you should end up with something like this IMO

@Cacheable("container")
public class Foo {

   @Cacheable(key = "'book.' + #id")
   public class Book getBook(Long id} { ... }
   @Cacheable(key = "'user.' + #id")
   public class User getUser(Long id) { ... }

And you still get a valid contract, i.e. cache.get(user.120) would return "John Smith".

You may disagree but the abstraction does not free you to understand how your model is held within the cache: if you have a cache for a certain type, you can just use the natural id of your choice and you'll be good. If you decide to hold unrelated things in the same cache, I believe that you are responsible to define the proper identifiers for each object type.

On a final note do not forget that cache implementations can use a persistent store and have a lifespan longer than the one of the application. Caching the "service/bean" identity or even the method in case of refactoring wouldn't crash the application, but it wouldn't give the expected result either (compared to the SPeL expression I wrote above).

[spring-projects-issues]

Stéphane Nicoll commented

Fixed the javadoc of Cacheable so that it explicitly mentions what happens and how the default behaviour can be overridden.