You are viewing a plain text version of this content. The canonical link for it is here.
Posted to derby-dev@db.apache.org by "Dyre Tjeldvoll (JIRA)" <de...@db.apache.org> on 2005/06/05 14:54:40 UTC
[jira] Created: (DERBY-336) The wrong overload of StandardException::newException() is used in some cases
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
Assigned to: Dyre Tjeldvoll
Priority: Trivial
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
[jira] Commented: (DERBY-336) The wrong overload of StandardException::newException() is used in some cases
Posted by "Dyre Tjeldvoll (JIRA)" <de...@db.apache.org>.
[ http://issues.apache.org/jira/browse/DERBY-336?page=comments#action_12312870 ]
Dyre Tjeldvoll commented on DERBY-336:
--------------------------------------
This was a little more complicated than I first thought: It looks like there are three different cases of incorrect, or at least unconventional, use of StandardException.newException(...):
1) newException() is called with and explicit "null" argument.
Error codes:
SQLState.SERVICE_DIRECTORY_CREATE_ERROR XBM0H.D=Directory {0} cannot be created.
SQLState.LOG_FULL XSLA4.D=Cannot write to the log, most likely the log is full. Please delete unnecessary files. It is also possible that the file system is read only, or the disk has failed, or some other problems with the media.
SQLState.RAWSTORE_ERROR_RENAMING_FILE XSRS4.S=Error renaming file (during backup) from {0} to {1}.
SQLState.RAWSTORE_ERROR_COPYING_FILE XSRS5.S=Error copying file (during backup) from {0} to {1}.
In all of these cases it should be safe to remove the "null" as it is not used in the message text.
2) newException(...) is called with Throwable as the LAST argument, and not the SECOND as it should be.
Error codes:
SQLState.AUTH_INVALID_AUTHORIZATION_PROPERTY 28501=Invalid database authorization property ''{0}={1}''.
SQLState.SERVICE_DIRECTORY_CREATE_ERROR XBM0H.D=Directory {0} cannot be created.
These two cases are simply a matter of changing the order of the arguments, i.e. move the Throwable argument.
SQLState.LANG_FILE_ERROR X0X63.S=Got IOException ''{0}''.
This is a bit more tricky. Clearly the author of the message text intended that a textual representation of the original exception should be inserted into the message text. But this error code is typically used like this:
StandardException.newException(SQLState.LANG_FILE_ERROR, ioe.toString(), ioe)
So even if we fix the order of the arguments (making "ioe" the 2nd argument), we have a situation where "ioe" is both the next exception, and its string representation gets inserted into the message string. This seems a bit redundant to me, but I'll let the experts decide what to do... :)
3) newException(...) is called with multiple arguments of type Throwable.
Error codes:
SQLState.FILE_CONTAINER_EXCEPTION XSDG3.D=Meta-data for Container {0} could not be accessed
This error code is used as follows:
StandardException.newException(SQLState.FILE_CONTAINER_EXCEPTION, this, ioe)
So the last "ioe" argument appears to be redundant, and should be removed.
SQLState.FILE_CREATE_NO_CLEANUP XSDF2.S=Exception during creation of file {0} for container, file could not be removed. The exeception was: {1}.
Here it seems that the intention has been to associate two different exceptions with the same StandardExecption:
StandardException.newException(SQLState.FILE_CREATE_NO_CLEANUP, ioe, file, se)
where "ioe" is an io exception, and "se" is a security exception. "ioe" will be the next exception, and "se" will be inserted (as string) at {1}. This error code is also used with an explicit "null" instead of "se". In this case I guess the explicit "null" is warranted, although not very elegant...
SQLState.FILE_CANNOT_REMOVE_FILE XSDF4.S=Exception during remove of file {0} for dropped container, file could not be removed {1}.
This error code is used in much the same way as the previous one, except that in the case where there is only one execption this is used both as the next exception and as the message argument, rather than using an explicit null.
StandardException.newException(SQLState.FILE_CANNOT_REMOVE_FILE, se, file, se);
When used with two excptions this error reports two io exceptions:
StandardException.newException(SQLState.FILE_CANNOT_REMOVE_FILE, ioe2, file, ioe);
Comments? Please... ;)
> 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
>
> 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
[jira] Commented: (DERBY-336) The wrong overload of StandardException::newException() is used in some cases
Posted by "Dyre Tjeldvoll (JIRA)" <de...@db.apache.org>.
[ http://issues.apache.org/jira/browse/DERBY-336?page=comments#action_12314532 ]
Dyre Tjeldvoll commented on DERBY-336:
--------------------------------------
Thanks for your comments, David!
I see what you are saying about FILE_CANNOT_REMOVE_FILE, but look at how it is used other places in the code:
./java/engine/org/apache/derby/impl/store/raw/data/RFResource.java:261:
SQLState.FILE_CANNOT_REMOVE_FILE, fileToGo);
./java/engine/org/apache/derby/impl/store/raw/data/RAFContainer.java:1077:
newException(SQLState.FILE_CANNOT_REMOVE_FILE, se, file,
./java/engine/org/apache/derby/impl/store/raw/data/RAFContainer.java:1102:
SQLState.FILE_CANNOT_REMOVE_FILE, ioe2, file, ioe);
./java/engine/org/apache/derby/impl/store/raw/data/RAFContainer.java:1107:
SQLState.FILE_CANNOT_REMOVE_FILE, se, file, stub);
./java/engine/org/apache/derby/iapi/reference/SQLState.java:489: String FILE_CANNOT_REMOVE_FILE = "XSDF4.S";
As you can see, this error message is used both with one and TWO nested exceptions (in much the same way as FILE_CREATE_NO_CLEANUP),
so you cannot remove the additional parameter from the message string. I agree that you can choose to supply the same exception both as the nested exception, and as a parameter, but I doubt that this is why the error message is written the way it is... Also, notice the first use of this message, where NO nested exception is supplied!
---
I think you are correct about using a string argument. However, I was cautioned about converting arguments to string before it is necessary, as this could lead to performance problems, (if the exception is caught, the conversion to string is unnecessary). But in this particular case I doubt that performance is relevant, so I'll consider changing it...
---
I understand your concerns about the dummy overloads, - it is a non-standard and non-intuitive construct. I was not sure if I should include them in the patch or just use them locally to find the StandardException usages that needed to be fixed. In the end, I decided to include them since it is easy to make another such mistake later, (and you will not be warned about it).
> 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
[jira] Closed: (DERBY-336) The wrong overload of
StandardException::newException() is used in some cases
Posted by "Dyre Tjeldvoll (JIRA)" <de...@db.apache.org>.
[ http://issues.apache.org/jira/browse/DERBY-336?page=all ]
Dyre Tjeldvoll closed DERBY-336:
--------------------------------
Resolution: Fixed
> 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
> Components: Newcomer
> Versions: 10.1.2.0
> Environment: Any
> Reporter: Dyre Tjeldvoll
> Assignee: Dyre Tjeldvoll
> Priority: Trivial
> Fix For: 10.2.0.0
> 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
[jira] Commented: (DERBY-336) The wrong overload of StandardException::newException() is used in some cases
Posted by "David Van Couvering (JIRA)" <de...@db.apache.org>.
[ http://issues.apache.org/jira/browse/DERBY-336?page=comments#action_12315768 ]
David Van Couvering commented on DERBY-336:
-------------------------------------------
Build runs, derbyall passes with no failures.
+1
> 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
[jira] Updated: (DERBY-336) The wrong overload of StandardException::newException() is used in some cases
Posted by "Dyre Tjeldvoll (JIRA)" <de...@db.apache.org>.
[ http://issues.apache.org/jira/browse/DERBY-336?page=all ]
Dyre Tjeldvoll updated DERBY-336:
---------------------------------
Attachment: derby-336.diff
derby-336.stat
derbyall_report.txt
The patch fixes the cases where an incorrect overload of StandardException.newException() is used. I used the better matching illegal overload trick to find these cases. Only the two illegal overloads that detected errors have been included in the patch.
> 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
[jira] Closed: (DERBY-336) The wrong overload of StandardException::newException() is used in some cases
Posted by "Dyre Tjeldvoll (JIRA)" <de...@db.apache.org>.
[ http://issues.apache.org/jira/browse/DERBY-336?page=all ]
Dyre Tjeldvoll closed DERBY-336:
--------------------------------
David Van Couvering Committed revision 230900.
> 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
[jira] Commented: (DERBY-336) The wrong overload of StandardException::newException() is used in some cases
Posted by "David Van Couvering (JIRA)" <de...@db.apache.org>.
[ http://issues.apache.org/jira/browse/DERBY-336?page=comments#action_12315524 ]
David Van Couvering commented on DERBY-336:
-------------------------------------------
Diffs look good. I'll try applying the patch and running the tests. I'll log a bug about fixing the StandardException semantics.
> 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
Re: [jira] Updated: (DERBY-336) The wrong overload of
StandardException::newException() is used in some cases
Posted by David Van Couvering <Da...@Sun.COM>.
Dyre, are you on hold with this, or is it moving forward?
Thanks,
David
Dyre Tjeldvoll (JIRA) wrote:
> [ http://issues.apache.org/jira/browse/DERBY-336?page=all ]
>
>Dyre Tjeldvoll updated DERBY-336:
>---------------------------------
>
> Attachment: derby-336.diff
> derby-336.stat
> derbyall_report.txt
>
>New patch, stat output and derbyall_report based on comments from David Van Couvering.
>
>
>
>>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)?
>>
>>
>
>
>
[jira] Updated: (DERBY-336) The wrong overload of StandardException::newException() is used in some cases
Posted by "Dyre Tjeldvoll (JIRA)" <de...@db.apache.org>.
[ http://issues.apache.org/jira/browse/DERBY-336?page=all ]
Dyre Tjeldvoll updated DERBY-336:
---------------------------------
Attachment: derby-336.diff
derby-336.stat
derbyall_report.txt
New patch, stat output and derbyall_report based on comments from David Van Couvering.
> 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
[jira] Commented: (DERBY-336) The wrong overload of StandardException::newException() is used in some cases
Posted by "Dyre Tjeldvoll (JIRA)" <de...@db.apache.org>.
[ http://issues.apache.org/jira/browse/DERBY-336?page=comments#action_12313180 ]
Dyre Tjeldvoll commented on DERBY-336:
--------------------------------------
Kathey Marsden <km...@sbcglobal.net> wrote in Message-id: <42...@sbcglobal.net>:
>File a bug for the XBM0H exception being generated with null
>arguments. File a patch plus your NSinSameJVM test.
>The NSinSameJVM test will be a good test of a hard to test
>exception. I think you need security manager to reproduce and only the
>network server tests run with security manager. Embedded gets tested
>because network server just makes embedded calls.
I've tried to modify NSinSameJVM.java to test this, but it seems impossible! As long as the test i run in Network mode it is impossible to verify that the Exception is correctly constucted, because the client only gets an SqlException from the driver. The SqlException only contains the message constructed on the server so unless the null argument is actually inserted into the message, the error will not be observable in the client.
In embedded mode it is straight-forward to inspect the EmbedSQLException to verify that it is correctly constructed.
I am starting to think that this type of error should be detected by the StandardException class itself. It should not allow adding more arguments than its message string supports, and not allow null arguments. And perhaps no Throwable args unless it already has a nested exception. These constraints should be enforced when the StandardException is created, (or modified).
Case 1 and 2 in my first comment can be detected at compile time by adding dummy versions of StandardException.newException(...) that match the incorrect usage, but trigger a compilation error (by throwing an unhandled exception). Unfortunately this will also flag Case 3 as an error, making things more complicated. Maybe another (legal) overload can fix this...
> 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
>
> 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
[jira] Commented: (DERBY-336) The wrong overload of StandardException::newException() is used in some cases
Posted by "David Van Couvering (JIRA)" <de...@db.apache.org>.
[ http://issues.apache.org/jira/browse/DERBY-336?page=comments#action_12314591 ]
David Van Couvering commented on DERBY-336:
-------------------------------------------
Hi, Dyre, your responses seem good.
There is still something funny about FILE_CANNOT_REMOVE_FILE. If there is a case where the second parameter is not even used, I would prefer that that usage be split into a separate message, rather than inserting a dummy string.
Regarding the dummy overloads: the reason mistakes have been made and you have had to do this is because the semantics of the newException are not clear: when you want to chain an exception, where does it go? How does one distinguish a chained exception from a parameter to the message string? It's particularly confusing because in some cases you want an exception in both places (sometimes the same exception, sometimes different exceptions).
I think in the name of convenience (e.g. only writing one line to throw the exception) we have created semantic muddiness, and it is now forcing us to do unnatural acts to get the compiler to catch improper usage. I
I would like to suggest opening a JIRA item to fix this so the semantics are clear and the compiler can catch mistakes the way it was intended to do so -- with strong typing.
Perhaps this could be done as part of the larger task of migrating to J2SE 1.4 chained exceptions... I can envision only allow message string arguments to newException(), and chaining exceptions be allowed only by calling a separate method on the exception class, e.g.:
catch ( IOException ioe ) {
try { somethingProtected() }
catch ( SecurityException se ) {
StandardException stde = StandardException.newException(msgid, arg1, arg2, ioe);
se.setCause(ioe)
stde.setCause(se);
throw stde;
}
}
In terms of this patch, however, I think what you have works pretty well and prevents further misuse, as long as it's commented extremely well why we're doing this, because it is very odd.
> 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
[jira] Updated: (DERBY-336) The wrong overload of
StandardException::newException() is used in some cases
Posted by "Dyre Tjeldvoll (JIRA)" <de...@db.apache.org>.
[ http://issues.apache.org/jira/browse/DERBY-336?page=all ]
Dyre Tjeldvoll updated DERBY-336:
---------------------------------
Component: Newcomer
Fix Version: 10.2.0.0
Version: 10.1.2.0
Setting fixin to 10.2 since this does not seem important enough to warrant a back-port to 10.1.3.
> 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
> Components: Newcomer
> Versions: 10.1.2.0
> Environment: Any
> Reporter: Dyre Tjeldvoll
> Assignee: Dyre Tjeldvoll
> Priority: Trivial
> Fix For: 10.2.0.0
> 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
[jira] Commented: (DERBY-336) The wrong overload of StandardException::newException() is used in some cases
Posted by "David Van Couvering (JIRA)" <de...@db.apache.org>.
[ 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
[jira] Reopened: (DERBY-336) The wrong overload of
StandardException::newException() is used in some cases
Posted by "Dyre Tjeldvoll (JIRA)" <de...@db.apache.org>.
[ http://issues.apache.org/jira/browse/DERBY-336?page=all ]
Dyre Tjeldvoll reopened DERBY-336:
----------------------------------
Need to set fixin
> 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