TreeCache.getKeys() returns null sometimes
See JBCACHE-535
Manik asked me:
Which corresponding unit test contradicts the TreeCache.getKeys() contract?[sp]
The test org/jboss/cache/optimistic/NodeInterceptorGetKeysTest.java contradicts the documentation in two places:
//assert we can see this with a key value get in the transaction
assertEquals(0, cache.getKeys("/").size());
also later
assertEquals(0, cache.getKeys("/one/two").size());
This is from 1.143, before I got my hands on it:
/**
* @param fqn
* @return A Set<String> of keys. This is a copy of the key set, modifications will not be written to the original.
* Returns null if the node is not found, or the node has no attributes
*/
public Set getKeys(Fqn fqn) throws CacheException
{
MethodCall m = new MethodCall(getKeysMethodLocal, new Object[]{fqn});
return (Set) invokeMethod(m);
}
public Set _getKeys(Fqn fqn) throws CacheException
{
Set retval = null;
DataNode n = findNode(fqn);
if (n == null)
return null;
retval = n.getDataKeys();
// return retval != null? new LinkedHashSet(retval) : null;
return retval != null ? new HashSet(retval) : null;
}
It now looks like this:
/**
* Returns a set of attribute keys for the Fqn.
* Returns null if the node is not found, or the node has no attributes
* @param fqn
* @return a copy of the key set, modifications will not be written to the original.
*/
public Set getKeys(Fqn fqn) throws CacheException
{
MethodCall m = new MethodCall(getKeysMethodLocal, new Object[]{fqn});
Set s = (Set) invokeMethod(m);
if (s == null || s.isEmpty())
return null; // sort of dumb IMHO
return s;
}
public Set _getKeys(Fqn fqn) throws CacheException
{
DataNode n = findNode(fqn);
if (n == null)
return null;
Set keys = n.getDataKeys();
if (keys == null)
return null;
return new HashSet(keys);
}
The tests currently (as if right now) don't pass. I'm thinking since the API wasn't very clean to begin with, I want to change it to be more useful and consistent:
1. If the node exists, return the keys. If there are no keys, return 0.
2. If the node does not exist, return null.
3. Possibly change DataNode's docs and behavior to match it
It used to be that:
1. If the node exists, return the keys. If there are no keys, return 0.
2. Also, if DataNode.getDataKeys() returned null because no keys were loaded (lazy created), return null
3. The documentation was not correct
My changes to optimize the loading behavior of CacheLoader caused certain tests to break that relied on some state that DataNode was in.
I think returning an empty set in all cases where the node exists is less likely to break people's code.
There are also a couple of other APIs which either return null for the empty set or an empty set or both. These should probably be documented or fixed or at least logged as issues to fix in the future.
