Currently Errors in Detectors, e.g. an un-intentional IndexError, KeyError, or ValueError, should raise a traceback.

Currently the error is formatted like an Error message raised by a Reject exception.

I *think* this is Legacy code that allowed detectors to raise these errors for some backward compatibility but this is one or two decades ago.

The offending code is in cgi/actions.py in some of the edit/create actions:

Line 843 in EditItemAction:
    except (ValueError, KeyError, IndexError, Reject) as message:
Line 890 in NewItemAction:
    except (ValueError, KeyError, IndexError, Reject) as message:
Line 1106 in RegisterAction:
    except (ValueError, KeyError, IndexError, Reject) as message:

this is raised in all cases when the called _editnodes raises an exception (which in turn calls a database action which in turn calls detectors).
In my opinion (and experience) this can only happen when a detector raises an exception. Detectors should never raise one of those exceptions (apart from Reject) in the error-free case. I think we should fix this and put it into the documentation for upgrading.

Note that somewhere else in the framework ValueError is also handled synonymous to Reject, it has to be checked if that is also legacy.

Let me know what you think, an in particular if you're using those errors for raising an error message to the user instead of using Reject for that purpose.

I'd also appreciate your experience when writing detectors that it's *very* hard to really find out what went wrong and where.

Note also that there may also be similar errors in the rest and xmlrpc APIs which should probably raise an internal server error in these cases. I would make a separate issue for those once we've decided on a course.

On Thu, Apr 08, 2021 at 04:22:26PM +0000, Ralf Schlatterbeck wrote:
> 
> Currently Errors in Detectors, e.g. an un-intentional IndexError,
> KeyError, or ValueError, should raise a traceback.
> 
> Currently the error is formatted like an Error message raised by a
> Reject exception.

If you're developing detectors and have seen this, I would be glad if I
can get feedback on this.

Ralf
-- 
Dr. Ralf Schlatterbeck                  Tel:   +43/2243/26465-16
Open Source Consulting                  www:   www.runtux.com
Reichergasse 131, A-3411 Weidling       email: office@runtux.com

Since the userauditor.py provided in all the templates shipped with 
Roundup uses ValueError like this (for incorrect data provided by a 
tracker user rather than a bug in the tracker), I expect that example is 
very widely followed by people's own detectors.

Hi Ralf and Joseph:

In message <alpine.DEB.2.22.394.2104081709090.924398@digraph.polyomino.org.uk>,
Joseph Myers writes:
>Since the userauditor.py provided in all the templates shipped with 
>Roundup uses ValueError like this (for incorrect data provided by a 
>tracker user rather than a bug in the tracker), I expect that example is 
>very widely followed by people's own detectors.

My detectors also use ValueError(21 times) for cases where the user
supplied value is incorrect. E.G.

   raise ValueError('Timezone "%s" does not exist' % tz)

From the exception message, it looks like it is intended for the user.
I use this when the user supplied an invalid value.

They also use:

   roundupdb.DetectorError, mailgw.Unauthorized and MessageSendError

which are intended for the administrator.

Along with:

   Reject, RejectRaw

meant for the users. These are used when the values are valid but
something else (e.g. trying to close an issue with open dependencies,
setting up a loop of issues etc.)

The template detectors use:

  ValueError (lots)
  roundupdb.DetectorError
  InvalidOptionError

So I claim we should keep ValueError at least. I am not sure about the
KeyError or IndexError. From a user level perspective, I would expect
KeyError to be mapped to a ValueError assuming the user supplied the
key value. IndexError, I am not sure why that is being reformatted.

I don't remeber having any issues with a rogue ValueError or
IndexError causing issue or had issues debugging the cause of the
error.

That said, I assume this is the issue Ralf was trying to debug.  I can
see a couple of ways of making this easier.  Adding an option to
config.ini:

   detector_debug = Yes/No
or
   detector_debug = InvalidOptionError, DetectorError, ValueError

(maybe auditor_debug would be better) which would:

   enable tracebacks for all detector/auditor exceptions except
   Reject/RejectRaw
or
   enable tracebacks for only specified exceptions

If we can determine where the exception is thrown another possibility
presents itself. If it's not raised in a detector file you get a
traceback.

So if in detectors/foo.py I have:

    raise ValueError('this is an allowed error')

or if I have:
    try:
       somestuff_that_can_raise_value_error()
    except ValueError:

      check some stuff
      raise

they get formatted like Reject.

However if in detectors/foo.py I call:

    somestuff_that_can_raise_value_error()

and that raises a ValueError I get a traceback.

The ideas is if the user raised/re-raised the exception by writing
code in a detector, format it like Reject. If the exception originates
from code called by the detector and not caught you get the traceback
to help debug the issue.

Thoughts?

Have a great weekend all.

I don't think it's a good idea to depend on whether the exception is 
raised directly in a detectors file.  (I prefer the style where all the 
substantive code used by detectors etc. is in external modules and as 
little as possible is in the .py files in the instance.)

On Thu, Apr 08, 2021 at 06:46:34PM +0000, Joseph Myers wrote:
> 
> I don't think it's a good idea to depend on whether the exception is 
> raised directly in a detectors file.  (I prefer the style where all the 
> substantive code used by detectors etc. is in external modules and as 
> little as possible is in the .py files in the instance.)

Me too

So I think we could
- Remove the special handling for KeyError, IndexError now
- Deprecate the use of ValueError for uses where Reject should be used
- In a future version remove the special handling for ValueError, too

I'm not very happy with additional options for these.
And I think Johns Use-Case (invalid value entered by the user) can be
met by using Reject for these cases, too (the user does *not* see any
difference between Reject and ValueError). And all too often a
ValueError is unintentional in a detector. It could even disclose
information to the user that she should not see at this point.

What do you think?

Ralf
-- 
Dr. Ralf Schlatterbeck                  Tel:   +43/2243/26465-16
Open Source Consulting                  www:   www.runtux.com
Reichergasse 131, A-3411 Weidling       email: office@runtux.com

Hmpf:
I've just discovered that the framework also raises some of the
exceptions that I'd like to get rid of:
-> Look into backends/rdbms_common.py:
   This raises KeyError for several conditions:
   - trying to set reserved properties like creation or id
   - trying to change a retired item
   ...
   Or ValueError:
   - Trying to set a value that already exists
   ...
   Or IndexError for some consistency-checks with Link properties

I'm not sure if digging into this is really worth the effort.
On the other hand these are database consistency checks that should get
to the user (I've verified that for example trying to change a retired
item *does* hit the user, it would probably be better to check for this
in the html framework and not offer an editable field in that case :-)

But I'd really like to reserve the generic errors to python programming
errors that raise a traceback. And raise some "DatabaseConsistencyError"
or similar in case on of the other conditions fails.

Thoughts?
Ralf
-- 
Dr. Ralf Schlatterbeck                  Tel:   +43/2243/26465-16
Open Source Consulting                  www:   www.runtux.com
Reichergasse 131, A-3411 Weidling       email: office@runtux.com