db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "David Van Couvering (JIRA)" <derby-...@db.apache.org>
Subject [jira] Commented: (DERBY-336) The wrong overload of StandardException::newException() is used in some cases
Date Thu, 23 Jun 2005 21:06:10 GMT
    [ http://issues.apache.org/jira/browse/DERBY-336?page=comments#action_12314354 ] 

David Van Couvering commented on DERBY-336:
-------------------------------------------

Hi, Dyre. I'm kicking off a build and will let you know.  In the meantime, here are my comments
on the changes.

For the most part this looks good, it's nice to see these cleaned up.  A couple of comments:

---

In RAFContainer.java, you modified one message to say "(see nested exception)".

It seems to me the last argument of FILE_CANNOT_REMOVE_FILE should not be
there if all we're going to say is "see nested exception".  We should either
remove this argument, or provide the message string.  Note that the writer
of this message may have intended for the exception message to be printed
out in the exception string and that the user could find the details of
the exception (e.g. the stack trace) by looking in the error log.  My
personal preference would be to include the exception message, better to
provide more detail rather than less when it comes to errors.

--

The FILE_CREATE_NO_CLEANUP is a strange beast.  It is only used in *one*
single place in the code with both exceptions and is creating some real
complexity in the StandardException.newException() methods.  It seems to
me that if all you want to show is the message of an exception, you
should pass the exception string as an argument, e.g.


StandardException.newException(SQLState.FILE_CREATE_NO_CLEANUP,
  ioe, file, se.getMessage());

If you modified the once instance where two exceptions are passed to use
this approach, then you wouldn't need this specialized version of
newException() two exception objects. You could just use the one that takes
(messageID, Throwable, Object, Object).

---

I understand what you're trying to accomplish with the "dummy" overloads,
but it makes me a bit nervous.  It's an odd use of the checked exception
model.

That said, I think it does work to catch improper usage of the
StandardException.newException().  I just wish there was another way to
do this...

One approach would be to have a setContainedException() method on
StandardException and have a policy that newException *only* takes
arguments to the message string.  I think this is a better model than
the "fake" constructor methods, but the drawback is that it would
likely require a significant rewrite of existing code that pass
exceptions into StandardException.

Thanks,

David

P.S. I may have other comments on your comment above, but I wanted to focus on your initial
patch first.

> The wrong overload of StandardException::newException() is used in some cases
> -----------------------------------------------------------------------------
>
>          Key: DERBY-336
>          URL: http://issues.apache.org/jira/browse/DERBY-336
>      Project: Derby
>         Type: Bug
>  Environment: Any
>     Reporter: Dyre Tjeldvoll
>     Assignee: Dyre Tjeldvoll
>     Priority: Trivial
>  Attachments: derby-336.diff, derby-336.stat, derbyall_report.txt
>
> When looking at DERBY-128 it became clear that the wrong overload of StandardException::newException()
was used when reporting
> SQLState.SERVICE_DIRECTORY_CREATE_ERROR. The message string only takes one parameter
so only one additional parameter (other than Throwable) should be used:
> PersistentServiceImpl.java:676
>                             throw StandardException.newException(SQLState.SERVICE_DIRECTORY_CREATE_ERROR,
>                                                                  serviceDirectory, null);
> // Calls StandardException.newException(String, Object, Object)
> // Should call StandardException.newException(String, Object)? Or StandardException.newException(String,
Throwable, Object)? With the IOException as  
> // Throwable?
> PersistentServiceImpl.java:692
>         throw StandardException.newException(SQLState.SERVICE_DIRECTORY_CREATE_ERROR,
name, t);
> // Calls StandardException.newException(String, Object, Object)
> // Should call StandardException.newException(String, Throwable, Object)?
> BaseDataFileFactory.java:279
>                 throw StandardException.newException( SQLState.SERVICE_DIRECTORY_CREATE_ERROR,
dataDirectory, ioe);
> // Calls StandardException.newException(String, Object, Object)
> // Should call StandardException.newException(String, Throwable, Object)?

-- 
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators:
   http://issues.apache.org/jira/secure/Administrators.jspa
-
For more information on JIRA, see:
   http://www.atlassian.com/software/jira


Mime
View raw message