FAQ
I know this has come up before and some debate goes on and then no
decision is really made. Do we want to improve exception reporting?

I've always found this to be a weak point in Cayenne's game.
Fortunately, I've become more comfortable with loading up the source
code and stepping through things to see what's wrong, but it's normally
debugging time that could largely be saved. The root problem, I think,
is that where a failure actually occurs and where a failure are
introduced are normally far apart.

As a minimal example, I tried to create likeIgnoreCaseExp(), but passed
in null for the value. I received the following NPE:

Caused by: java.lang.NullPointerException
at
org.apache.cayenne.exp.parser.SimpleNode.connectChildren(SimpleNode.java
:276)
at
org.apache.cayenne.exp.parser.ASTLikeIgnoreCase.<init>(ASTLikeIgnoreCase
.java:43)
at
org.apache.cayenne.exp.ExpressionFactory.likeIgnoreCaseExp(ExpressionFac
tory.java:529)
at
com.servprise.www.pages.store.CheckOut.addCoupon(CheckOut.java:291)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at
sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.jav
a:39)
at
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessor
Impl.java:25)
at java.lang.reflect.Method.invoke(Method.java:597)
at
org.apache.tapestry.listener.ListenerMethodInvokerImpl.invokeTargetMetho
d(ListenerMethodInvokerImpl.java:214)
at
org.apache.tapestry.listener.ListenerMethodInvokerImpl.invokeListenerMet
hod(ListenerMethodInvokerImpl.java:155)
... 51 more

Now, had I not had the source loaded in Eclipse already, it would have
been awfully difficult to track down. Tapestry loading on its own
exception stuff doesn't help much.

On the other hand, if an IllegalArgumentException had been thrown in
likeIgnoreCaseExp indicating that value must not be null, my debugging
time would have been cut down considerably. I have run across many
similar situations in the past couple years.

If anyone has any thoughts on the matter, please chime in. It is
something I would like to see move forward with the 3.0 release.

--
Kevin Menard
Servprise International, Inc.
800.832.3823 x308

Search Discussions

  • Mike Kienenberger at Aug 3, 2007 at 3:58 pm
    In my opinion, it's best to trap the error as soon as possible. As
    far as I remember, that's our standard practice, and as we come across
    items like this, we correct them.

    On 8/3/07, Kevin Menard wrote:
    I know this has come up before and some debate goes on and then no
    decision is really made. Do we want to improve exception reporting?

    I've always found this to be a weak point in Cayenne's game.
    Fortunately, I've become more comfortable with loading up the source
    code and stepping through things to see what's wrong, but it's normally
    debugging time that could largely be saved. The root problem, I think,
    is that where a failure actually occurs and where a failure are
    introduced are normally far apart.

    As a minimal example, I tried to create likeIgnoreCaseExp(), but passed
    in null for the value. I received the following NPE:

    Caused by: java.lang.NullPointerException
    at
    org.apache.cayenne.exp.parser.SimpleNode.connectChildren(SimpleNode.java
    :276)
    at
    org.apache.cayenne.exp.parser.ASTLikeIgnoreCase.<init>(ASTLikeIgnoreCase
    .java:43)
    at
    org.apache.cayenne.exp.ExpressionFactory.likeIgnoreCaseExp(ExpressionFac
    tory.java:529)
    at
    com.servprise.www.pages.store.CheckOut.addCoupon(CheckOut.java:291)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at
    sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.jav
    a:39)
    at
    sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessor
    Impl.java:25)
    at java.lang.reflect.Method.invoke(Method.java:597)
    at
    org.apache.tapestry.listener.ListenerMethodInvokerImpl.invokeTargetMetho
    d(ListenerMethodInvokerImpl.java:214)
    at
    org.apache.tapestry.listener.ListenerMethodInvokerImpl.invokeListenerMet
    hod(ListenerMethodInvokerImpl.java:155)
    ... 51 more

    Now, had I not had the source loaded in Eclipse already, it would have
    been awfully difficult to track down. Tapestry loading on its own
    exception stuff doesn't help much.

    On the other hand, if an IllegalArgumentException had been thrown in
    likeIgnoreCaseExp indicating that value must not be null, my debugging
    time would have been cut down considerably. I have run across many
    similar situations in the past couple years.

    If anyone has any thoughts on the matter, please chime in. It is
    something I would like to see move forward with the 3.0 release.

    --
    Kevin Menard
    Servprise International, Inc.
    800.832.3823 x308
  • Kevin Menard at Aug 3, 2007 at 4:04 pm

    -----Original Message-----
    From: Mike Kienenberger
    Sent: Friday, August 03, 2007 11:59 AM
    To: dev@cayenne.apache.org
    Subject: Re: Exceptions . . .

    In my opinion, it's best to trap the error as soon as possible. As
    far as I remember, that's our standard practice, and as we come across
    items like this, we correct them.
    I may be mistaken, but my experience tracking down some problems in the
    past couple days indicates that we rarely check that invariants hold
    true. Instead, things go as far as they can, the exception is caught,
    wrapped, and sent back up the pipeline. I imagine it's been done this
    way as a performance benefit, but I'm not sure the overhead would be
    that great anyway. Comparisons are pretty cheap.

    --
    Kevin
  • Mike Kienenberger at Aug 3, 2007 at 4:07 pm
    Yes, I'm not saying we do it right. I'm just saying we correct things
    as we find them. I know of at least one instance where I was hit by
    something like this and the fix used was to detect the error at the
    initial point of failure.
    On 8/3/07, Kevin Menard wrote:
    -----Original Message-----
    From: Mike Kienenberger
    Sent: Friday, August 03, 2007 11:59 AM
    To: dev@cayenne.apache.org
    Subject: Re: Exceptions . . .

    In my opinion, it's best to trap the error as soon as possible. As
    far as I remember, that's our standard practice, and as we come across
    items like this, we correct them.
    I may be mistaken, but my experience tracking down some problems in the
    past couple days indicates that we rarely check that invariants hold
    true. Instead, things go as far as they can, the exception is caught,
    wrapped, and sent back up the pipeline. I imagine it's been done this
    way as a performance benefit, but I'm not sure the overhead would be
    that great anyway. Comparisons are pretty cheap.

    --
    Kevin
  • Kevin Menard at Aug 3, 2007 at 4:21 pm

    -----Original Message-----
    From: Mike Kienenberger
    Sent: Friday, August 03, 2007 12:07 PM
    To: dev@cayenne.apache.org
    Subject: Re: Exceptions . . .

    Yes, I'm not saying we do it right. I'm just saying we correct things
    as we find them. I know of at least one instance where I was hit by
    something like this and the fix used was to detect the error at the
    initial point of failure.
    Gotcha. Are there any guidelines on this? I imagine throwing
    CayenneException is preferred, but given that it's a checked exception,
    that could mean rewriting interfaces. I'm a fan of throwing unchecked
    exceptions if we can't do anything about the problem anyway.

    Also, should we be adding tests to check the exceptions are actually
    thrown and pair them up with fail() calls? Or, has the approach been to
    just introduce the exception and just make sure the existing test suite
    passes?

    --
    Kevin
  • Mike Kienenberger at Aug 3, 2007 at 4:29 pm
    If it's a code error rather than a data error, I don't see a problem
    with throwing an NPE. I'd save CayenneException for things that are
    environment-related.

    Unit tests would be great.
    On 8/3/07, Kevin Menard wrote:
    -----Original Message-----
    From: Mike Kienenberger
    Sent: Friday, August 03, 2007 12:07 PM
    To: dev@cayenne.apache.org
    Subject: Re: Exceptions . . .

    Yes, I'm not saying we do it right. I'm just saying we correct things
    as we find them. I know of at least one instance where I was hit by
    something like this and the fix used was to detect the error at the
    initial point of failure.
    Gotcha. Are there any guidelines on this? I imagine throwing
    CayenneException is preferred, but given that it's a checked exception,
    that could mean rewriting interfaces. I'm a fan of throwing unchecked
    exceptions if we can't do anything about the problem anyway.

    Also, should we be adding tests to check the exceptions are actually
    thrown and pair them up with fail() calls? Or, has the approach been to
    just introduce the exception and just make sure the existing test suite
    passes?

    --
    Kevin
  • Andrus Adamchik at Aug 3, 2007 at 4:37 pm

    On Aug 3, 2007, at 7:18 PM, Kevin Menard wrote:

    I imagine throwing
    CayenneException is preferred, but given that it's a checked
    exception,
    that could mean rewriting interfaces.
    We certainly DO NOT want every method in Cayenne to throw checked
    exceptions. Then it will be as "usable" as JDBC :-)
    CayenneRuntimeException (and its two specialized subclasses -
    ExpressionException and EJBQLException) and IllegalArgumentException
    are two of my favorites.

    For things like argument checking in the user-facing API,
    IllegalArgumentException seems more appropriate than
    CayenneRuntimeException.

    Also I don't think we have too many methods in Cayenne (if any at
    all) that throw checked CayenneException, requiring a try/catch. It
    is always CayenneRuntimeException.
    I'm a fan of throwing unchecked exceptions if we can't do anything
    about the problem anyway. Yep.
    Also, should we be adding tests to check the exceptions are actually
    thrown and pair them up with fail() calls? Or, has the approach
    been to
    just introduce the exception and just make sure the existing test
    suite
    passes?
    It's been the later. But also because everybody wants to cut down
    time writing unit tests, not because of some principle :-)

    Andrus
  • Mike Kienenberger at Aug 3, 2007 at 4:41 pm
    For what it's worth, when I write my own code these days, I use either
    NullPointerException("variable-name") or IllegalArgumentException("why
    it's illegal")

    On 8/3/07, Andrus Adamchik wrote:
    On Aug 3, 2007, at 7:18 PM, Kevin Menard wrote:

    I imagine throwing
    CayenneException is preferred, but given that it's a checked
    exception,
    that could mean rewriting interfaces.
    We certainly DO NOT want every method in Cayenne to throw checked
    exceptions. Then it will be as "usable" as JDBC :-)
    CayenneRuntimeException (and its two specialized subclasses -
    ExpressionException and EJBQLException) and IllegalArgumentException
    are two of my favorites.

    For things like argument checking in the user-facing API,
    IllegalArgumentException seems more appropriate than
    CayenneRuntimeException.

    Also I don't think we have too many methods in Cayenne (if any at
    all) that throw checked CayenneException, requiring a try/catch. It
    is always CayenneRuntimeException.
    I'm a fan of throwing unchecked exceptions if we can't do anything
    about the problem anyway. Yep.
    Also, should we be adding tests to check the exceptions are actually
    thrown and pair them up with fail() calls? Or, has the approach
    been to
    just introduce the exception and just make sure the existing test
    suite
    passes?
    It's been the later. But also because everybody wants to cut down
    time writing unit tests, not because of some principle :-)

    Andrus

  • Aristedes Maniatis at Aug 4, 2007 at 12:41 am

    On 04/08/2007, at 2:41 AM, Mike Kienenberger wrote:

    For what it's worth, when I write my own code these days, I use either
    NullPointerException("variable-name") or IllegalArgumentException("why
    it's illegal")
    I also use IllegalArgumentException quite a bit in my own code. And I
    think it makes more sense to a user than a generic
    CayenneRuntimeException. Plus you can use it to show some sort of
    error dialog with a useful "you typed invalid search criteria" or
    whatever.

    Ari


    -------------------------->
    Aristedes Maniatis
    phone +61 2 9660 9700
    PGP fingerprint 08 57 20 4B 80 69 59 E2 A9 BF 2D 48 C2 20 0C C8
  • Malcolm Edgar at Aug 4, 2007 at 9:29 am
    I much prefer IllegalArguementException if it is an invalid method
    parameter or IllegalStateException if the object is in an invalid
    state for an method call.

    The reason I dont throw NPE is because the JRE will do that. When I
    see a NPE I generally think there may be a problem with the underlying
    code rather than the way I am using it.

    regards Malcolm Edgar
    On 8/4/07, Aristedes Maniatis wrote:
    On 04/08/2007, at 2:41 AM, Mike Kienenberger wrote:

    For what it's worth, when I write my own code these days, I use either
    NullPointerException("variable-name") or IllegalArgumentException("why
    it's illegal")
    I also use IllegalArgumentException quite a bit in my own code. And I
    think it makes more sense to a user than a generic
    CayenneRuntimeException. Plus you can use it to show some sort of
    error dialog with a useful "you typed invalid search criteria" or
    whatever.

    Ari


    -------------------------->
    Aristedes Maniatis
    phone +61 2 9660 9700
    PGP fingerprint 08 57 20 4B 80 69 59 E2 A9 BF 2D 48 C2 20 0C C8


  • Kevin Menard at Aug 3, 2007 at 4:55 pm

    -----Original Message-----
    From: Andrus Adamchik
    Sent: Friday, August 03, 2007 12:38 PM
    To: dev@cayenne.apache.org
    Subject: Re: Exceptions . . .
    For things like argument checking in the user-facing API,
    IllegalArgumentException seems more appropriate than
    CayenneRuntimeException. Agreed.
    Also I don't think we have too many methods in Cayenne (if any at
    all) that throw checked CayenneException, requiring a try/catch. It
    is always CayenneRuntimeException.
    Yeap. My mistake.
    It's been the later. But also because everybody wants to cut down
    time writing unit tests, not because of some principle :-)
    Then I suppose this would raise another topic. JUnit 3 is a bit of a
    boar for testing exception. You basically have to try, catch, fail.
    TestNG has a standard facility for testing for expected exceptions. My
    guess is that JUnit4 does as well, but I have nothing to back that up
    with. There was some discussion some time ago about moving the testing
    framework. Have we hit that crossroad yet, or is that still for a rainy
    day?

    --
    Kevin
  • Andrus Adamchik at Aug 3, 2007 at 5:04 pm

    On Aug 3, 2007, at 7:55 PM, Kevin Menard wrote:

    Then I suppose this would raise another topic. JUnit 3 is a bit of a
    boar for testing exception. You basically have to try, catch, fail.
    TestNG has a standard facility for testing for expected
    exceptions. My
    guess is that JUnit4 does as well, but I have nothing to back that up
    with. There was some discussion some time ago about moving the
    testing
    framework. Have we hit that crossroad yet, or is that still for a
    rainy
    day?
    This is certainly close to zero priority on my personal list. But I
    have no objections to somebody else working on the switch to a newer
    JUnit. I will only be against it if it turns out to be as disruptive
    as the recent Maven migration.

    Andrus
  • Andrus Adamchik at Aug 3, 2007 at 4:09 pm

    imagine it's been done this
    way as a performance benefit,
    No, probably just laziness :-)

    Andrus

    On Aug 3, 2007, at 7:04 PM, Kevin Menard wrote:

    -----Original Message-----
    From: Mike Kienenberger
    Sent: Friday, August 03, 2007 11:59 AM
    To: dev@cayenne.apache.org
    Subject: Re: Exceptions . . .

    In my opinion, it's best to trap the error as soon as possible. As
    far as I remember, that's our standard practice, and as we come
    across
    items like this, we correct them.
    I may be mistaken, but my experience tracking down some problems in
    the
    past couple days indicates that we rarely check that invariants hold
    true. Instead, things go as far as they can, the exception is caught,
    wrapped, and sent back up the pipeline. I imagine it's been done this
    way as a performance benefit, but I'm not sure the overhead would be
    that great anyway. Comparisons are pretty cheap.

    --
    Kevin
  • Andrus Adamchik at Aug 3, 2007 at 4:06 pm
    +1

    I don't think there's any disagreement on that.

    Andrus
    On Aug 3, 2007, at 6:58 PM, Mike Kienenberger wrote:
    In my opinion, it's best to trap the error as soon as possible. As
    far as I remember, that's our standard practice, and as we come across
    items like this, we correct them.

    On 8/3/07, Kevin Menard wrote:
    I know this has come up before and some debate goes on and then no
    decision is really made. Do we want to improve exception reporting?

    I've always found this to be a weak point in Cayenne's game.
    Fortunately, I've become more comfortable with loading up the source
    code and stepping through things to see what's wrong, but it's
    normally
    debugging time that could largely be saved. The root problem, I
    think,
    is that where a failure actually occurs and where a failure are
    introduced are normally far apart.

    As a minimal example, I tried to create likeIgnoreCaseExp(), but
    passed
    in null for the value. I received the following NPE:

    Caused by: java.lang.NullPointerException
    at
    org.apache.cayenne.exp.parser.SimpleNode.connectChildren
    (SimpleNode.java
    :276)
    at
    org.apache.cayenne.exp.parser.ASTLikeIgnoreCase.<init>
    (ASTLikeIgnoreCase
    .java:43)
    at
    org.apache.cayenne.exp.ExpressionFactory.likeIgnoreCaseExp
    (ExpressionFac
    tory.java:529)
    at
    com.servprise.www.pages.store.CheckOut.addCoupon(CheckOut.java:291)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native
    Method)
    at
    sun.reflect.NativeMethodAccessorImpl.invoke
    (NativeMethodAccessorImpl.jav
    a:39)
    at
    sun.reflect.DelegatingMethodAccessorImpl.invoke
    (DelegatingMethodAccessor
    Impl.java:25)
    at java.lang.reflect.Method.invoke(Method.java:597)
    at
    org.apache.tapestry.listener.ListenerMethodInvokerImpl.invokeTargetMe
    tho
    d(ListenerMethodInvokerImpl.java:214)
    at
    org.apache.tapestry.listener.ListenerMethodInvokerImpl.invokeListener
    Met
    hod(ListenerMethodInvokerImpl.java:155)
    ... 51 more

    Now, had I not had the source loaded in Eclipse already, it would
    have
    been awfully difficult to track down. Tapestry loading on its own
    exception stuff doesn't help much.

    On the other hand, if an IllegalArgumentException had been thrown in
    likeIgnoreCaseExp indicating that value must not be null, my
    debugging
    time would have been cut down considerably. I have run across many
    similar situations in the past couple years.

    If anyone has any thoughts on the matter, please chime in. It is
    something I would like to see move forward with the 3.0 release.

    --
    Kevin Menard
    Servprise International, Inc.
    800.832.3823 x308

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupdev @
categoriescayenne
postedAug 3, '07 at 3:52p
activeAug 4, '07 at 9:29a
posts14
users5
websitecayenne.apache.org

People

Translate

site design / logo © 2022 Grokbase