Inconsistent Error and Exception Handling: Difference between revisions

From OpenPetra Wiki
Jump to navigation Jump to search
 
(9 intermediate revisions by 3 users not shown)
Line 1: Line 1:
==Overview==
==Overview==
'''We do not yet have a consistent way of handling errors in all situations. This is strongly desired.'''
This page served as a 'blackboard' for the Architecture Team to discuss Error and Exception handling in OpenPetra. As such, personal opinions are recorded.
 
'''TODO''': Extend the [[Error and Exception Handling Policy]] - especially in the areas of Client and Server error handling.
 
== Input from Wolfgang U ==
 
Section Practices That Need to be Adhered to:
*** Another part of re throwing an exception is a proper reaction of an error message (ref. [[Comments to Revision 1089#A new common Exception: CancelSaveException]]). In this case the error handling has been done inside of the exception handler joined with a appropriate error message and then a large part of manual code and automatic generated code shall not to be done. One example of this is the CancelSaveException which is defined in PetraEditForms.cs. This exception is thrown in GLAccountHierarchy.ManualCode.cs. In the try block in specific situations a System.Data.ConstraintException is thrown.


''Information found on this page is mostly 'historic' - much of the suggested has been implemented, and where this is so it is noted.''


== Input from ThiasG ==
== Input from ThiasG ==
* An exception shall never be caught and re-thrown with one exception: Resources need to be freed in error case, e.g. SQL transaction rollbacked, handles freed, wait cursor reseted, what-ever.
Expected System-Exceptions shall be caught and be put into an OpenPetra-specific Exception. This help to differentiate between programmed errors and expected Exception conditions.
* Expected System-Exceptions shall be caught and be put into a OpenPetra specific exception. This help to differentiate between programmed errors and expected error conditions.
''--> Use <code>EOPAppException</code> or one of the OpenPetra Exceptions that derive from it (or create another OpenPetra Exception, deriving from <code>EOPAppException</code>, it if that is warranted). --[[User:Christiankatict|Christiankatict]] 07:35, 10 April 2014 (UTC)''
* Exceptions, which are a result of a programming error shall never be caught, e.g. NullPointerException. This would mask the original error making it hard to find it.
* If there is additional information for an exception when it is thrown, then it should be added in extra attributes to the exception. This help in handling the exception, because it is possible to process the additional data in the code. Also it is possible to show a i18n error including the additional data. (Is this possible in C# like in C++/Python?)
* All OpenPetra specific Exception shall derive from the base class XXX. The base class adds support for i18n of the Exception for showing an error message to the user. Exceptions should always logged in English to the log file (without i18n).
* Specialized base class for exception for database, IO, server-communication, ... are provided. This helps to catch specific areas of expected exception, which could be handled in this place.
* Exceptions should not be used to change the flow control. Exception shall only be used for error conditions.
 
I would remove the part of specialization. Often you throw a specialized exception like System.IO.Exception because e.g. a file is not found, but from the point of view of caller of the function/method, e.g. cached data is not accessible and is a more appropriate error then the file  is not found message. Internally it is important to have the original exception, which needs to be logged, too.


Until now, we have not added anything about handling the exception, e.g. handling the exception in the client, aborting the current function, logging the exception and  showing the error to the user.
Until now, we have not added anything about handling the exception, e.g. handling the exception in the client, aborting the current function, logging the exception and  showing the error to the user.


We need to add something about handling the extreme cases of losing connection to DB or errors in the Client-Server/Server-Client connection. Do we stop the client session? In which cases the server needs to stop? Not all error condition are initiated by a function call from the client.
All our exceptions should be RemotedExceptions. If possible, our base class should handle most stuff of the GetObjectData method. Why is it needed, that the Exception are in the namespace 'Ict.Petra.Shared.RemotedExceptions'?
I do not understand the use case, where an exception is thrown in a loop many times, because the first exception would abort the loop and the exception should not be caught without been re-thrown...


I would avoid to make the remote boundary a special case in the policy, even if it cost some time. We are talking about error condition, not the good case...
We need to add something about ''handling extreme cases'', such as
* Losing connection to DB;
* Client-Server/Server-Client connection;
* other cases?
What to do in such situations: Do we stop the client session? In which cases the server needs to stop? - Not all error conditions are initiated by a function call from the client.


--[[User:Thiasg|Thiasg]] 09:27, 31 January 2011 (UTC)
--[[User:Thiasg|Thiasg]] 09:27, 31 January 2011 (UTC)


== Nested Exceptions (Wolfgang U) ==
== 2.Input from ThiasG ==
 
Let me point out another important part of the error handling policy and this are nested and parametrizde exceptions.
 
Let us see the following code:
 
    public class InvalidLedgerInfoException: SystemException
    {
    public InvalidLedgerInfoException(string message, Exception innerException)
    : base(message, innerException) {}
    }
   
    public class GetLedgerInfo
    {
    int ledgerNumber;
    private ALedgerTable ledger=null;
   
    public GetLedgerInfo(int ALedgerNumber)
    {
    ledgerNumber = ALedgerNumber;
    ledger = ALedgerAccess.LoadByPrimaryKey(ALedgerNumber, null);
    }
   
    public string RevaluationAccount
    {
   
    get {
    try {
    ALedgerRow row = (ALedgerRow)ledger[0];
    return row.ForexGainsLossesAccount;
    } catch (Exception catchedException)
    {
    string message =  
    "The RevaluationAccount of leger {0} has been unsuccessfully required!";
    throw new InvalidLedgerInfoException(
    String.Format(message, ledgerNumber) ,catchedException);
    }
    }
    }
   
    }
 


A data table is read and only the content of a field is used. If we use the test code line
I would throw an Exception for verification errors. Otherwise this would be a extra condition to think of. There should be a special Exception Class for verification errors, which include the TVerificationResult and TVerificationResultCollection objects for information about the verification error.
''--> This has been addressed: The <code>EVerificationResultsException</code> serves that purpose.'' --[[User:Christiankatict|Christiankatict]] 07:31, 10 April 2014 (UTC)


  Assert.AreEqual("5003", new GetLedgerInfo(44).RevaluationAccount, "...");


we get the following result.  
I would not allow to "translate" an exception with one exception: Putting a database error into a OpenPetra specific exception. But as general rule, translation should not be done and not be allowed.


[[File:NestedExceptions.JPG]]
All Exception we are throwing should be custom exceptions deriving from OPDBExcpetion and OPAppException (perhaps some more, eg on for the verification errors). By this it is possible to catch exceptions by catching only some types of exception we are interested in.
''--> This has been addressed: <code>EOPException</code> and <code>EOPAppException</code> Exception Classes have been introduced. Only <code>EOPAppException</code> and <code>EOPDBException</code> derive from <code>EOPException</code>, all other OpenPetra-specific Exceptions derive from one of those two (e.g. <code>EVerificationResultsException</code> derives from <code>EOPAppException</code>). --[[User:Christiankatict|Christiankatict]] 07:31, 10 April 2014 (UTC)


This means that we have the info:
Example of use case does not make sense. The exception should not catched, because it is not handled at this point of code! The information, that it is the ledger 44, is an internal information, a end user is not interested in. This information is available in the debugger through the stacktrace anyway. Writing a log and rethrowing the original exception would be sufficient. The problem with catching and rethrowing is, that in most cases it is harder to debug, because the exception is handled and the original cause is hard to find.


: Tests.MFinance.GL.TRevaluationTests.T03_GetLedgerInfo:
<code>CancelSaveException</code> sounds weird: The exception is mis-used as flow control. In which cases the save operation is canceled, after a verification was already done? Why is it not possible to throw a custom exception with the specific information, which is shown later on after rollback to the user?
:Ict.Petra.Server.MFinance.GL.InvalidLedgerInfoException : The RevaluationAccount of leger 44 has been unsuccessfully required!
''--> This has been addressed in [[https://tracker.openpetra.org/view.php?id=1503 Bug 1503]] - CancelSaveException has been removed; the Data Validation Framework is used instead.'' --[[User:Christiankatict|Christiankatict]] 07:31, 10 April 2014 (UTC)
:  ----> System.IndexOutOfRangeException : There is no row at position 0.


In normal cases we only have the information of the System.IndexOutOfRangeException in specific file and line but now we have the additional information that the ledger number 44 has been requested. Even if programm is delivered without any debug info, we now new a user level reason for the error (wrong ledger number) and the programm level consequences for the software (There is no row at position 0).


An interesting article about this problem(s) you can find here: [http://blog.gurock.com/articles/creating-custom-exceptions-in-dotnet/ Creating Custom Exceptions in .NET]
--[[User:Thiasg|Thiasg]] 07:51, 9 May 2011 (UTC)

Latest revision as of 14:24, 24 April 2014

Overview

This page served as a 'blackboard' for the Architecture Team to discuss Error and Exception handling in OpenPetra. As such, personal opinions are recorded.

Information found on this page is mostly 'historic' - much of the suggested has been implemented, and where this is so it is noted.

Input from ThiasG

Expected System-Exceptions shall be caught and be put into an OpenPetra-specific Exception. This help to differentiate between programmed errors and expected Exception conditions. --> Use EOPAppException or one of the OpenPetra Exceptions that derive from it (or create another OpenPetra Exception, deriving from EOPAppException, it if that is warranted). --Christiankatict 07:35, 10 April 2014 (UTC)

Until now, we have not added anything about handling the exception, e.g. handling the exception in the client, aborting the current function, logging the exception and showing the error to the user.


We need to add something about handling extreme cases, such as

  • Losing connection to DB;
  • Client-Server/Server-Client connection;
  • other cases?

What to do in such situations: Do we stop the client session? In which cases the server needs to stop? - Not all error conditions are initiated by a function call from the client.

--Thiasg 09:27, 31 January 2011 (UTC)

2.Input from ThiasG

I would throw an Exception for verification errors. Otherwise this would be a extra condition to think of. There should be a special Exception Class for verification errors, which include the TVerificationResult and TVerificationResultCollection objects for information about the verification error. --> This has been addressed: The EVerificationResultsException serves that purpose. --Christiankatict 07:31, 10 April 2014 (UTC)


I would not allow to "translate" an exception with one exception: Putting a database error into a OpenPetra specific exception. But as general rule, translation should not be done and not be allowed.

All Exception we are throwing should be custom exceptions deriving from OPDBExcpetion and OPAppException (perhaps some more, eg on for the verification errors). By this it is possible to catch exceptions by catching only some types of exception we are interested in. --> This has been addressed: EOPException and EOPAppException Exception Classes have been introduced. Only EOPAppException and EOPDBException derive from EOPException, all other OpenPetra-specific Exceptions derive from one of those two (e.g. EVerificationResultsException derives from EOPAppException). --Christiankatict 07:31, 10 April 2014 (UTC)

Example of use case does not make sense. The exception should not catched, because it is not handled at this point of code! The information, that it is the ledger 44, is an internal information, a end user is not interested in. This information is available in the debugger through the stacktrace anyway. Writing a log and rethrowing the original exception would be sufficient. The problem with catching and rethrowing is, that in most cases it is harder to debug, because the exception is handled and the original cause is hard to find.

CancelSaveException sounds weird: The exception is mis-used as flow control. In which cases the save operation is canceled, after a verification was already done? Why is it not possible to throw a custom exception with the specific information, which is shown later on after rollback to the user? --> This has been addressed in [Bug 1503] - CancelSaveException has been removed; the Data Validation Framework is used instead. --Christiankatict 07:31, 10 April 2014 (UTC)


--Thiasg 07:51, 9 May 2011 (UTC)

Retrieved from ‘https://wiki.openpetra.org/index.php?title=Inconsistent_Error_and_Exception_Handling&oldid=3996’