Roundup Tracker - Issues

Issue 2551083

classification
Title: Derive Roundup exceptions from common base class other than BaseException
Type: behavior Severity: normal
Components: Web interface, Mail interface, Command-line interface Versions: 2.0.0alpha
process
Status: fixed Resolution: fixed
Dependencies: Superseder:
Assigned To: rouilj Nosy List: rouilj, schlatterbeck, tttech-klonner
Priority: high Keywords: Blocker

Created on 2020-03-25 07:27 by tttech-klonner, last changed 2020-04-03 22:48 by rouilj.

Messages
msg6898 Author: [hidden] (tttech-klonner) Date: 2020-03-25 07:27
In roundup/exceptions.py there are some exceptions defined which are derived 
from BaseException. I have a use case where I have to handle those exceptions 
but I can't because they are using BaseException directly. I would have to 
catch it with "except BaseException:" which would result in also catching 
signals like keyboard interrupts or system exits).

I would propose to make a common exception class where all other custom Roundup 
exceptions are derived from. It is also done like this in the 
roundup/cgi/exceptions.py exceptions.

In addition I think we can derive the Roundup base exception class from 
"Exception" and not "BaseException" as the latter one should be used for built-
in exceptions and not user-defined exceptions.


roundup/exceptions.py

class RoundupException(Exception):
    pass

class Unauthorised(RoundupException):
    pass

class Reject(RoundupException):
    pass

...


If we align on this we could also go though the exceptions in 
roundup/cgi/exceptions.py and check for those concepts. We can import the 
RoundupException from roundup/exceptions.py and derive the exceptions for cgi.
msg6899 Author: [hidden] (rouilj) Date: 2020-03-25 20:21
Hi Robert:

In message <1585121254.8.0.412971837801.issue2551083@roundup.psfhosted.org>,
Robert Klonner writes:
>In roundup/exceptions.py there are some exceptions defined which are derived 
>from BaseException. I have a use case where I have to handle those exceptions 
>but I can't because they are using BaseException directly. I would have to 
>catch it with "except BaseException:" which would result in also catching 
>signals like keyboard interrupts or system exits).

IIRC BaseException was the way it was done in earlier 2.2 pythons. The
code base is showing its age.

I am interested in your use case. AFAIK you can always:

  from exceptions import *

then use

  try:
     ...
  except (exception1, exception2, exception3 ...):

(I may be off on the syntax but you get the idea). Then list each
exception you want to catch. Clumsy but workable.  However if a new
exception is added you will need to change your code 8-(.

I see Ralf is already on this ticket. Ralf are you working it?

I think (as recommended) a:

  class RoundupException(Exception):
     pass

and substituting BaseException/Exception in all class definitions with
RoundupException makes sense and follows recommended exception
hierarchy guidelines.

Does anybody see a problem with this? Any cases where this change may
break something?

I would like to get this in the 2.0 beta release (was planning on
starting the release sequence this weekend). Robert I assume you can
test it for your use case?
msg6900 Author: [hidden] (rouilj) Date: 2020-04-01 05:01
Hi Robert,

Ralf and I are discussing what need to change off list.

Can you describe your use case so we can be sure that all exceptions
(base roundup, email, cgi, tal, detector) that we are modifying will
meet your needs.

Thanks

-- rouilj
msg6901 Author: [hidden] (rouilj) Date: 2020-04-02 21:13
I think this is the new exception hierarchy that Ralf and I worked out.

+ indicates new exception class
* indicates exception class needs to be changed

Inheritence by indentation:

RoundupException+
   RejectBase+
      Reject*
         RejectRaw
   RoundupCGIException+
      HTTPException*
         Redirect
         NotFound
         NotModified
         PreconditionFailed
   DetectorError*
   SendFile*
   SendStaticFile*
   SeriousError*
   
   NoTemplate*
   Unauthorised* (2 instances)

   ConfigurationError*

   LoginError*
   Reject*

   TrackerError*

   MailUsageHelp*
   Unauthorized*
   IgnoreMessage*
   
Exception
   SysCallError - defined in except block on ssl import failure
   PoSyntaxError
   
BaseException
   SvcShutdown - server should exit on this
   
Exceptions defined under:

 roundup/cgi/TAL/...
 roundup/cgi/PageTemplates/...

stay as they are. These are inherited files into roundup.

Richard will this work for your use case?

Ralf does this look right? If so I'll try to do them this weekend.

-- rouilj
msg6902 Author: [hidden] (schlatterbeck) Date: 2020-04-03 09:32
On Thu, Apr 02, 2020 at 09:13:48PM +0000, John Rouillard wrote:
> 
> John Rouillard added the comment:
> 
> I think this is the new exception hierarchy that Ralf and I worked out.
> 
> + indicates new exception class
> * indicates exception class needs to be changed
> 
> Inheritence by indentation:

Shouldn't RoundupException inherit from Exception?

Otherwise everything looks fine to me, thanks for your effort!

Ralf

> RoundupException+
>    RejectBase+
>       Reject*
>          RejectRaw
>    RoundupCGIException+
>       HTTPException*
>          Redirect
>          NotFound
>          NotModified
>          PreconditionFailed
>    DetectorError*
>    SendFile*
>    SendStaticFile*
>    SeriousError*
>    
>    NoTemplate*
>    Unauthorised* (2 instances)
> 
>    ConfigurationError*
> 
>    LoginError*
>    Reject*
> 
>    TrackerError*
> 
>    MailUsageHelp*
>    Unauthorized*
>    IgnoreMessage*
>    
> Exception
>    SysCallError - defined in except block on ssl import failure
>    PoSyntaxError
>    
> BaseException
>    SvcShutdown - server should exit on this
>    
> Exceptions defined under:
> 
>  roundup/cgi/TAL/...
>  roundup/cgi/PageTemplates/...
> 
> stay as they are. These are inherited files into roundup.
msg6903 Author: [hidden] (rouilj) Date: 2020-04-03 20:11
Ralf Said:

> Shouldn't RoundupException inherit from Exception?

Yes.

I have the code changes done. Running tests locally to see if
I broke anything. Sadly exception handling is not well covered
by the test suite, so I could break something silently.

Haven't heard back from Robert, hopefully this meets his needs.
msg6904 Author: [hidden] (rouilj) Date: 2020-04-03 20:25
Committed as rev:6121:c177e7128dc9 lets see what CI has to say.
msg6905 Author: [hidden] (rouilj) Date: 2020-04-03 22:47
Sigh, fixed in: rev6125:38fbfbb24cbd and  rev 6126:15c68580578f
and rev 6127:80915fd9ad24
msg6906 Author: [hidden] (rouilj) Date: 2020-04-03 22:48
Closing since we haven't gotten any feedback.
History
Date User Action Args
2020-04-03 22:48:31rouiljsetstatus: open -> fixed
resolution: fixed
messages: + msg6906
2020-04-03 22:47:57rouiljsetmessages: + msg6905
2020-04-03 20:27:39rouiljsettype: behavior
components: + Web interface, Mail interface, Command-line interface
versions: + 2.0.0alpha
2020-04-03 20:25:51rouiljsetmessages: + msg6904
2020-04-03 20:11:51rouiljsetmessages: + msg6903
2020-04-03 09:32:14schlatterbecksetmessages: + msg6902
2020-04-02 21:13:48rouiljsetassignee: rouilj
messages: + msg6901
2020-04-01 12:22:05rouiljsetstatus: new -> open
priority: high
keywords: + Blocker
2020-04-01 05:01:43rouiljsetmessages: + msg6900
2020-03-25 20:21:36rouiljsetnosy: + rouilj
messages: + msg6899
2020-03-25 07:27:34tttech-klonnercreate