• Bug
  • Status: Open
  • 2 Major
  • Resolution:
  • ehcache-core
  • cdennis
  • Reporter: ppurang
  • February 10, 2012
  • 0
  • Watchers: 3
  • February 17, 2012

Description

The behaviour is misleading because:

  1. A guard method should only return true if the method it is guarding will return a non null value.
  2. The guard method in question covers two methods but the naming suggests it is guarding {{#getCache}} (which isn’t true as {{#getCache}} does an additional {{#instanceof}} check)
  3. No documentation or deprecation hint that might warn consumers of the API of this fact.

The usual usage pattern (perhaps) never runs foul because devs use the {{#addCacheIfAbsent}}, {{#getCache}} and {{#cacheExists}}. However, the following usage pattern combined with some existing code bombs.

     //do some wrapping
     final String cacheName = "myCache"
     final Ehcache original = cacheManager.addCacheIfAbsent(cacheName);
     final Ehcache wrapper = new EhCacheWrapper(cacheName, original);     //a nested class that extends EhcacheDecoratorAdapter
     cacheManager.replaceCacheWithDecoratedCache(original, wrapper);

     //existing code 
     if (cacheManager.cacheExists(cacheName)) {
       final Cache cache = cacheManager.getCache(cacheName);
       cache.remove(key);                                                 //NPE...
     }

Another bit that fails:

     final String[] cacheNames = cacheManager.getCacheNames();
     for (final String cacheName : cacheNames) {
       cacheManager.getCache(cacheName).removeAll();                      //NPE...
     }

Suggestions:

  1. Deprecate {{#cacheExists}} and {{#getCache}}
  2. Introduce {{#ehcacheExists}}.
  3. Document the deprecation and point to the newer methods {{#ehcacheExists}} and {{#getEhcache}} as alternatives
  4. Stop using them internally within the ehcache code base and documentation
  5. Remove them in the next major release (?)

Comments

Alexander Snaps 2012-02-10

As the javadoc on mentions, cacheExists will “Checks whether a cache of type ehcache exists.” The important bit being “of type ehcache”. I agree that this is confusing, especially in conjunction with getCache(). We can’t really deprecated getCache I think, but adding ehcacheExists() ? And deprecate the former ? We should investigate this…

Alexander Snaps 2012-02-10

Now how much we want to invest in clearing this mess before an API overhaul is unclear…

Fiona OShea 2012-02-13

In the short term do we need to update docs/javadocs?

Chris Dennis 2012-02-13

This name is confusing but the javadoc is correct (albeit badly written/formed). Right now this is low priority and is something that will get fixed naturally in 3.x. Discussing with James, he suggests we let this stay in 2.5.x (not promote to 2.5.2) and see how things go on 3.0 timeframe wise.