Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Duplicate
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: Core
    • Last commented by a User:
      true

      Description

      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

        Issue Links

          Activity

          Hide
          Hans-Peter Störr added a comment - - edited

          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

          Show
          Hans-Peter Störr added a comment - - edited 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
          Hide
          Bozhidar Bozhanov added a comment - - edited

          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();
          	}
          
          }
          
          Show
          Bozhidar Bozhanov added a comment - - edited 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(); } }
          Hide
          Connor Barry added a comment -

          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-

          Show
          Connor Barry added a comment - 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-
          Hide
          Connor Barry added a comment -

          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).

          Show
          Connor Barry added a comment - 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).
          Hide
          Elryk added a comment -

          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.

          Show
          Elryk added a comment - 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.

            People

            • Assignee:
              Phil Webb
              Reporter:
              Bozhidar Bozhanov
              Last updater:
              Phil Webb
            • Votes:
              9 Vote for this issue
              Watchers:
              11 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:
                Days since last comment:
                1 year, 18 weeks, 1 day ago