[OJB-developers] Two exception handling discussion points

[John F. <jfr...@so...>]

I'd like to bring up two points regarding OJB exception handling for
discussion among the experts on this list.


Item 1
Perusing the source I've noticed in some places very wide exception
catching with substitution of an unrelated exception.  For example, the
RsIterator.next() method below caused me to chase a red herring
NoSuchElementException for awhile.  My root problem was a
NullPointerException in some logging stuff deep in a cache class.

My personal preference is to have things fail early and hard if they are
going to.  With that in mind, I'd volunteer to hunt down these patterns
and try to constrain them to handle only the exceptions that they expect
& can deal with.  In this example we would simply catch and map only the
PersistenceBrokerException that getObjectFromResultSet() could throw.

Thoughts?

    public synchronized Object next() throws NoSuchElementException
    {
        try
        {
            if (!hasCalledCheck)
                hasNext();
            hasCalledCheck =3D false;
            if (hasNext)
            {
                Object obj =3D getObjectFromResultSet();
                return obj;
            }
            else
                throw new NoSuchElementException();
        }
        catch (Exception ex)  // change to (PersistenceBrokerException
ex)
        {
            logger.error(ex);
            throw new NoSuchElementException();
        }
    }



Item 2
Some class methods that could fail internally with a
PersistenceBrokerException sometimes (IMHO) inappropriately mask the
exception with an unrelated one.  This could have serious ramifications.

For example PersistenceBroker.getCollectionByQuery() could simply return
a smaller collection than really exists with no other indication of a
problem to the caller because when it was using an iterator to load up
the collection, the underlying RsIterator.hasNext() method got a
PersistenceBrokerException (or a SQLException), but masked it (see the
code below).

The root of this problem seems to be an "impedence mismatch" between
wanting to re-use the standard java.util.Iterator interface, but also
trying to surface serious problems as early as possible.

One solution would be to revise the PB interface and start exposing an
RsIterator interface with methods that throw PBEs as necessary.  Gets to
the heart of the issue for me, but certainly busts a lot of existing
code.

Another solution would be to create a similiar PBE, but derived from
RuntimeException instead.  I'm not so crazy about this, but it is better
than the status quo which seems dangerous IMHO.

Thoughts?

    public synchronized boolean hasNext()
    {
        try
        {
            if (!hasCalledCheck)
            {
                hasCalledCheck =3D true;
                hasNext =3D m_rs.next();
                if (!hasNext)
                {
                    releaseDbResources();
                }
            }
        }
        catch (Exception ex)
        {
            hasNext =3D false;
        }
//#ifdef DEBUG
        logger.debug("hasNext() -> " + hasNext);
//#endif
        return hasNext;
    }


Regards,
 - John Freeborg