spring-projects / spring-framework

Spring Framework
https://spring.io/projects/spring-framework
Apache License 2.0
56.27k stars 37.98k forks source link

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

Closed spring-projects-issues closed 10 years ago

spring-projects-issues commented 12 years ago

Alexander Derenbach opened SPR-8933 and commented

When I read the reference documentation I thought the caching abstraction is on method level, but it seems it is only on the parameter value of the methods.

If I have several methods with the same parameter type it get always the same return value:

@Component public class CacheTestImpl implements CacheTest { @Cacheable("databaseCache") public Long test1() { return 1L; }

@Cacheable("databaseCache")
public Long test2() {
    return 2L;
}

@Cacheable("databaseCache")
public Long test3() {
    return 3L;
}

@Cacheable("databaseCache")
public String test4() {
    return "4";
}

}

Calling now:

System.out.println(test.test1());
System.out.println(test.test2());
System.out.println(test.test3());
System.out.println(test.test4());

results in:

1 1 1 ClassCastException: java.lang.Long cannot be cast to java.lang.String

Is this the desired behaviour? I would expect:

1 2 3 4

If I use different caches it works.

I can't access github from my place (firewall) so I have added a tar with a small maven project showing this problem.

Greets Alex


Affects: 3.1 GA

Attachments:

Issue Links:

5 votes, 9 watchers

spring-projects-issues commented 12 years ago

Alexander Derenbach commented

There is a possiblity to use only the method name:

@Cacheable(value="databaseCache", key="#root.methodName") public Long test(Long test) { return test; }

But then the parameter value is ignored.

spring-projects-issues commented 12 years ago

Alexander Derenbach commented

I found a solution (workaround) for my problem:

@Cacheable(value="databaseCache", key="{#root.methodName,#test}") public Long test(Long test) { return test; }

spring-projects-issues commented 12 years ago

Alexander Derenbach commented

If this is the intended behaviour it would be nice if either the value field of @Cacheable has a default, so that it is possible to add a annotation like this:

@Target({ElementType.METHOD, ElementType.TYPE}) @Retention(RetentionPolicy.RUNTIME) @Cacheable(key="{#root.methodName,#root.args}") public @interface MethodCacheable {

}

...

@MethodCacheable("cache") public Long test(Lont t1) { }

or to add a new annotation for that to Spring itself

Greets Alex

spring-projects-issues commented 12 years ago

Alexander Derenbach commented

I was wrong

@Cacheable(key="{#root.methodName,#root.args}") does not work because the args array is always a new one...

spring-projects-issues commented 12 years ago

Costin Leau commented

This is not a bug. All 4 methods use the same key and the same cache, thus, after you call test1, all the other methods will automatically find in the cache the same value. If you want to differentiate per method, you just need to add the method name as a key into the cache. This distinction does not happen by default since for the most part, different methods can share the same result.

spring-projects-issues commented 12 years ago

Ned Mules commented

Hi Costin

Thanks for your work on this great feature.

But I agree with Alex that its behaviour is counter-intuitive.

You said: "This distinction does not happen by default since for the most part, different methods can share the same result."

I may be missing something but why would you typically want methods with different names and the same parameters to return the same values from the cache?

For example, if I was building a music catalogue I would like to do this:

@Cacheable("musicCache") public Artist getArtist(int artistId)

@Cacheable("musicCache") public List\ getAlbums(int artistId)

As far as I can see, for this to work I would have to define different caches (with accompanying boilerplate ehcache XML configuration and associated maintenance overhead) for Artists, AlbumLists, etc, etc, and remember to use the correct one with the correct method. Or as you said, I would have to explicitly specify the method name as a key wherever I use @Cacheable. It's not clear how to do this - Alex said the examples above didn't work.

My solution to this was to write a "MethodAwareCacheKeyGenerator" which implements KeyGenerator and is manually wired into a CacheInterceptor in my @Configuration class. This works but it's about 40 lines of code that I think should be unnecessary.

Could you please reconsider changing this behaviour or at least explicitly documenting the default behaviour and how to make it cache on method name?

At the moment, the documentation says "Out of the box, the caching abstraction uses a simple hash-code based KeyGenerator that computes the key based on the hashes of all objects used for method invocation".

My assumption was that the method name was one of the "objects" used. It would help if the documentation stated that only the arguments are used by default, and not the method name.

Thanks Ned

spring-projects-issues commented 12 years ago

Alexander Derenbach commented

Hello Ned,

I am glad that I am not the only one who miss understood it. (or expected something else). My solution for my problem is now this (as mentioned above)

@Cacheable(value="databaseCache", key="{#root.methodName,#test}") public Long test(Long test) { return test; }

this works fine.

But I agree with you that at least the documentation could perhaps mention the behavior a bit better, since they are talking about "method caching" which leads me to my expecting of the cache.

Greets Alex

spring-projects-issues commented 11 years ago

Mukarram Baig commented

Hello Costin and gang - Thanks for the great work thus far! I too tend to agree with Ned that the behavior is slightly counter-intuitive. I was trying to cache calls to a third party JAR via the cache:advice route and Ned's suggestion to have a MethodAwareCacheKeyGenerator helped me out. Can we provide such a key generator from Spring rather than having each user hand-roll their own? I am attaching an implementation that I am using.

spring-projects-issues commented 11 years ago

Mukarram Baig commented

Updated definition

spring-projects-issues commented 11 years ago

Alex Wajda commented

Please do not close it as "Invalid" because it is pretty much valid. The current behaviour is too counter intuitive. The following conclusion is at least illogical.

...for the most part, different methods can share the same result The different methods are aimed to have different semantics and event though the input can be the same the result will most likely to be different. Otherwise why would 2 methods with identical semantics co-exist?

I would vote for making the (methodName,arg1,arg2,...,argN) to be the default key. So that even the key spaces for overloaded methods would not overlap. To me that is the most sensible default.

spring-projects-issues commented 10 years ago

chris marx commented

I agree, I think most people would assume that the method signature would be part of the cache key generation-