New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve DefaultKeyGenerator [SPR-9036] #13675

Closed
spring-issuemaster opened this Issue Jan 18, 2012 · 5 comments

Comments

Projects
None yet
2 participants
@spring-issuemaster
Copy link
Collaborator

spring-issuemaster commented Jan 18, 2012

Bozhidar Bozhanov opened SPR-9036 and commented

org.springframework.cache.interceptor.DefaultKeyGenerator is implemented by calculating the hashcode of all arguments. This is problematic in many cases:

  • methods with similar signature - foo(int, String), foo(int, List) - if the 2nd argument is null they will get mixed
  • methods with the same signature in different classes, using the same cache - they will get mixed
  • is there a collision resolution mechanism? If not, things will randomly fail. Rarely, but they will. Imagine that the arguments of two distinct methods (that use the same cache region) evaluate to the same hashcode (not unlikely) - they will override each other's values, and possibly a ClassCastException will occur on retrieval

What is in the first place the reason to get the hashcodes? The underlying cache mechanism is a hashtable of some sort, so it will calculate the hash of the key anyway. But it will have a collision-resolution strategy, unlike the KeyGenerator.

I suggest:

  • concatenating the string representations of primitives, and the hashcode of non-primitives.
  • adding the class name and method name to the key
  • optionally, where non-primitive values are used as keys, log a warning - this is usually wrong, as the hashcode will change even though the values inside the object are the same

Attachments:

Issue Links:

  • #14870 Cacheable key collision with DefaultKeyGenerator ("duplicates")
  • #14870 Cacheable key collision with DefaultKeyGenerator
  • #14013 org.springframework.cache.interceptor.DefaultKeyGenerator has too weak hashing functionality

9 votes, 11 watchers

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator

spring-issuemaster commented May 8, 2012

Hans-Peter Störr commented

I'd say the DefaultKeyGenerator is completely broken. If you cache, say, webservice calls with customer information, it would be absolutely inacceptable to accidentially return the customer information about one customer to another customer. Thus, you can never ever use hashcodes as keys in the cache - no matter how sophisticated the hashing function is. If you even use the hashcodes of the arguments you have already lost - String.hashCode for instance returns the same hashcode for many strings - you get collisions as short as "Ca" and "DB".

The problem is even worse - if you put the results of several methods into one cache, the cache confuses the two methods if you use DefaultKeyGenerator. There should at least be a warning about that in the documentation.

Being a little of the "better safe than sorry" persuasion, I'd sugest something like that:

public Object generate(Object target, Method method, Object... params) {
  Class<?> objectclass = AopProxyUtils.ultimateTargetClass(target);
  List<Object> cacheKey = new ArrayList<Object>();
  cacheKey.add(objectclass.getName().intern());
  cacheKey.add(System.identityHashCode(target));
  cacheKey.add(method.toString().intern());
  cacheKey.addAll(Arrays.asList(params));
  return cacheKey;
}

Of course it is debatable and dependent of the application whether the target's identity and class should play a role here, and if so, the identity should be taken on the proxied object if it is a proxy. If you aim for a widely usable implementation that should probably be configurable - to include identity might not make sense for e.g. distributed caches. Also, this works only for key types that implement their own equals and hashCode methods, and they should be serializable if you want to use a distributed cache. But if the keys don't, you are probably lost, anyway.

If you still want to provide a hashcode based implementation for applications that don't care about this, you might at least use another prime like 92821 instead of 31: http://stackoverflow.com/a/2816747/21499

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator

spring-issuemaster commented May 8, 2012

Bozhidar Bozhanov commented

Here's my implementation of a cache key generator:

 
public class CacheKeyGenerator implements KeyGenerator {
	private static final Logger logger = LoggerFactory.getLogger(ARTCacheKeyGenerator.class);
	
	public static final int NO_PARAM_KEY = 0;
	public static final int NULL_PARAM_KEY = 53;
	
	@Override
	public Object generate(Object target, Method method, Object... params) {
		StringBuilder key = new StringBuilder();
		key.append(target.getClass().getSimpleName()).append(".").append(method.getName()).append(":");
		
		if (params.length == 0) {
			return key.append(NO_PARAM_KEY).toString();
		}
		
		for (Object param : params) {
			if (param == null) {
				key.append(NULL_PARAM_KEY);
			} else if (ClassUtils.isPrimitiveOrWrapper(param.getClass()) || param instanceof String) {
				key.append(param);
			} else if (param instanceof CacheKey) {
				key.append(((CacheKey) param).getCacheKey());
			} else {
				logger.warn("Using an object as a cache key may lead to unexpected results. " +
						"Either use @Cacheable(key=..) or implement CacheKey. Method is " + target.getClass() + "#" + method.getName());
				key.append(param.hashCode());
			}
		}
		
		return  key.toString();
	}

}
@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator

spring-issuemaster commented Nov 8, 2012

Connor Barry commented

Hi Bozhidar, there are several holes in your CacheKeyGenerator. getSimpleName() may resolve to the same thing for two classes with the same name but in different packages. Also someone may pass in "53" as the value of the first param (either a String or a primitive), which would get a cache hit on your null param entry. Also the fact that you use hashCode() at all is insecure, although I do like that you put a logger.warn out there.

I am working on some fixes to these issues, will post shortly-

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator

spring-issuemaster commented Nov 8, 2012

Connor Barry commented

Bonehead move, I didn't realize EHCache could accept objects as keys instead of Strings. I like Hans-Peter Störr's solution, and it works for my app since my cached methods are within singletons (no need to worry about object identity).

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator

spring-issuemaster commented Dec 13, 2012

Elryk commented

No unit test exists on GitHub :
https://github.com/SpringSource/spring-framework/tree/master/spring-context/src/test/java/org/springframework/cache/interceptor

So I upload the TestDefaultKeyGenerator.java that covers 2 cases not supported by the current DefaultKeyGenerator implementation.

The Hans-Peter Störr's implementation passed them with success.

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