Exception handling for flow control is EVIL!!!
time to read 4 min | 792 words

I just spent over half a day trying to fix a problem in NHibernate. To be short one of the updates that I made caused a backward compatibility error, and I really wanted to fix it. The actual error condition happen only when you use triple nested detached criteria with reference to the root from the inner most child. To make things fun, that change was close to 9 months ago, and over a 1,000 revisions.

I finally found the revision that introduced the error, and started looking at the changes. It was a big change, and a while ago, so it took some time. Just to give you some idea, here is where the failure happened:

public string GetEntityName(ICriteria criteria)
{
  ICriteriaInfoProvider result;
  if(criteriaInfoMap.TryGetValue(criteria, out result)==false)
    throw new ArgumentException("Could not find a matching criteria info provider to: " + criteria);
  return result.Name;
}

For some reason, the old version could find it, and the new version couldn’t. I traced how we got the values to criteriaInfoMap every each way, and I couldn’t see where the behavior was different between the two revisions.

Finally, I resorted to a line by line revert of the revision, trying to see when the test will break. Oh, I forgot to mention, here is the old version of this function:

public string GetEntityName(ICriteria criteria)
{
  string result;
  criteriaEntityNames.TryGetValue(criteria, out result);
  return result;
}

The calling method (several layers up) looks like this:

private string[] GetColumns(string propertyName, ICriteria subcriteria)
{
  string entName = GetEntityName(subcriteria, propertyName);
  if (entName == null)
  {
    throw new QueryException("Could not find property " + propertyName);
  }
  return GetPropertyMapping(entName).ToColumns(GetSQLAlias(subcriteria, propertyName), GetPropertyName(propertyName));
}

But even when I did a line by line change, it still kept failing. Eventually I got fed up and change the GetEntityName to return null if it doesn’t find something, instead of throw.

The test passed!

But I knew that returning null wasn’t valid, so what the hell(!) was going on?

Here is the method that calls the calling method;

public string[] GetColumnsUsingProjection(ICriteria subcriteria, string propertyName)
{
  // NH Different behavior: we don't use the projection alias for NH-1023
  try
  {
    return GetColumns(subcriteria, propertyName);
  }
  catch (HibernateException)
  {
    //not found in inner query , try the outer query
    if (outerQueryTranslator != null)
    {
      return outerQueryTranslator.GetColumnsUsingProjection(subcriteria, propertyName);
    }
    else
    {
      throw;
    }
  }
}

I actually tried to track down who exactly wrote this mantrap, (this nasty came from the Hibernate codebase). But I got lost in migrations, reformatting, etc.

All in all, given how I feel right now, probably a good thing.

Tweet Share Share 12 comments
Tags: