Error messages more than one from validate_attachment are ignored

[Scott Wang <scott_wang@…>]

We wrote an IAttachmentManipulator plugin to validate a user attachment. It can be more than one error that fails the validation, and we want to tell the user about all of them.

In the description of the validate_attachment API, it says that it "Must return a list of (field, message) tuples, one for each problem detected. "

However, in where the plugin get called, it will raise an exception when the first error is found and ignore the rest of them:

for field, message in manipulator.validate_attachment(req, attachment):
    if field:
        raise InvalidAttachment(
            _('Attachment field %(field)s is invalid: %(message)s',
              field=field, message=message))
    else:
        raise InvalidAttachment(
            _('Invalid attachment: %(message)s', message=message))

If it cannot show multiple errors, then the description of the API should be modified.

[Remy Blank]

Yes, it would be better to collect and display all messages. Would you like to try and write a patch?

[Scott Wang <scott_wang@…>]

Of course, I will submit the patch soon.

[Scott Wang <scott_wang@…>]

Proposed patch is attached. Use add_warning() to show all errors, which is similar to what is done in ticket validation.

[Remy Blank]

That's a good start. Instead of having separate error and warning boxes, I would prefer placing everything into the error box. You can generate an HTML fragment with tag (from genshi.builder), and pass that fragment as the exception message. So you could have the error message and a <ul> list below with the individual messages.

[Scott Wang <scott_wang@…>]

Replying to rblank:

That's a good start. Instead of having separate error and warning boxes, I would prefer placing everything into the error box. You can generate an HTML fragment with tag (from genshi.builder), and pass that fragment as the exception message. So you could have the error message and a <ul> list below with the individual messages.

Got it. I was originally going to write in the error message but just could not figure out how to display more then one item.

I will try it and submit a new one.

[Scott Wang <scott_wang@…>]

An alternative patch is attached, along with the screenshot of its output. Now it gathers all error reasons generated from all attachment manipulators and put it in a single error message box.

[osimons]

Thanks for finding this bug, and obviously all warnings should be collected before we abort the save.

I've got a few comments though, mostly general and not patch-specific:

1. I don't like collecting and exposing the plugin name to the user. We don't do this anywhere else in Trac, and no need to do it here.

2. Why not use add_warning() like we do for Ticket manipulators and similar? Here is the ticket code, just modify slightly to redirect back to the same page and re-populate the form with description, checkbox and filename (if possible):

           valid = True
           # Custom validation rules
           for manipulator in self.ticket_manipulators:
               for field, message in manipulator.validate_ticket(req, ticket):
                   valid = False
                   if field:
                       add_warning(req, _("The ticket field '%(field)s' is "
                                          "invalid: %(message)s",
                                          field=field, message=message))
                   else:
                       add_warning(req, message)
           return valid
   

3. I need to completely re-implement this validation and other checks for the RPC plugin - preferably without web-formatted errors… Perhaps trac.attachment would be a good first place to try to split the 2 levels into 3 - for a total of 4 components:
   • Attachment for model/db much as today.
   • AttachmentCore for exposing the public attachment api to be used by internal and external modules for all attachment manipulation, and that handles permissions, manipulators, listeners and so on.
   • AttachmentModule that exposes attachments to the web and handles web-based request manipulation presentation and feedback.
   • AttachmentAdmin that similarly provides reading and manipulation via the command line, that similarly would benefit from additions such as support for listeners.

4. Various places in Trac where we check for 'warnings' or similarly collect potential issues, and I have the same in a number of my own plugins. These are then situations where you can either call a single TracError (or any of its derivatives), or collect a list of warnings to pass around. What I have personally ended up doing is making a new TracWarnings error class that I use internally in my code. I think it works well to signal collected warnings up the hierarchy, particularly useful when the number of layers increase from 2 to 3 by adding a core level that really should be output-agnostic. Simplified, it just looks like this:

   class TracWarnings(Exception):
       """
       A class used to signal up any warnings that may occur for
       operations at lower levels in the code. This exception types
       contain a ``.warnings`` array with any messages for user.
       """
   
       def __init__(self, warnings=None):
           self.warnings = warnings or []
   
   

[Christian Boos]

Replying to osimons:

• I'd rather have AttachmentSystem instead of AttachmentCore, for symmetry with other sub-systems; also do you think we should split attachment.py to attachment/{model,api,web_ui,admin}.py?
• about TracWarnings: is it really worth having another Exception (not even inheriting from TracError) instead of extending TracError with this warnings property? If it's just used for warnings and not for a "real" error (i.e. the operation succeeds in the end), then is using an Exception really appropriate?

[osimons]

Replying to cboos:

• I'd rather have AttachmentSystem instead of AttachmentCore, for symmetry with other sub-systems;

Sure, it is much like the other *System classes, so the name is of less importance to me.

also do you think we should split attachment.py to attachment/{model,api,web_ui,admin}.py?

Not so important now, but I suppose that makes sense too at some stage going forward.

• about TracWarnings: is it really worth having another Exception (not even inheriting from TracError) instead of extending TracError with this warnings property? If it's just used for warnings and not for a "real" error (i.e. the operation succeeds in the end), then is using an Exception really appropriate?

It is very much 'real' IMHO. It is checks that halts a processing, and very much an signal for out-of-the-ordinary handling. The idea with keeping it distinct from a TracError is that you can distinguish them from each other and handle them in different except clauses. And our other internal errors don't usually inherit from TracError? Extending a common error object with all kinds of facets and custom handling will instead litter the code with if else checks for various properties and circumstances.

Anyway, it is something I've found more intuitive for my own code. But, return warnings works too. It is just that raising some general InvalidAttachment and packing the message with formatted information is not ideal for me.

[Christian Boos]

Replying to osimons:

Replying to cboos:

• about TracWarnings: is it really worth having another Exception (not even inheriting from TracError) instead of extending TracError with this warnings property? If it's just used for warnings and not for a "real" error (i.e. the operation succeeds in the end), then is using an Exception really appropriate?

It is very much 'real' IMHO. It is checks that halts a processing, and very much an signal for out-of-the-ordinary handling.

If you mean to use that exception "locally", in a try: ... except TracWarnings, e: ..., but you don't want that exception to actually terminate a request the way TracError does, then my concerns about this class don't hold.

Otherwise, if you want to use it as an alternate way to terminate a request like a TracError can do, but with "structured" extra informations instead of markup, then I think it's probably easier to simply extend TracError in some ways (so that we can just go through the normal HTTPException.__init__, _send_user_error sequence).

In the example of the show_attachment_manipulator_error.2.diff​ this could translate to:

    raise InvalidAttachment(title=_("Invalid attachment"), reasons=all_errs)

And in TracError, the message property would derive from both _message and the new reasons attribute.

[Scott Wang <scott_wang@…>]

Replying to osimons:

Thanks for finding this bug, and obviously all warnings should be collected before we abort the save.

I've got a few comments though, mostly general and not patch-specific:

1. I don't like collecting and exposing the plugin name to the user. We don't do this anywhere else in Trac, and no need to do it here.

It is useful in my use case, but if it does not make sense in general cases, it can be removed as well.

2. Why not use add_warning() like we do for Ticket manipulators and similar? Here is the ticket code, just modify slightly to redirect back to the same page and re-populate the form with description, checkbox and filename (if possible):

           valid = True
           # Custom validation rules
           for manipulator in self.ticket_manipulators:
               for field, message in manipulator.validate_ticket(req, ticket):
                   valid = False
                   if field:
                       add_warning(req, _("The ticket field '%(field)s' is "
                                          "invalid: %(message)s",
                                          field=field, message=message))
                   else:
                       add_warning(req, message)
           return valid
   

Actually the first version of the patch uses the add_warning method just like the one in the ticket manipulator does. How are we going to solve this problem?

[Remy Blank]

Replying to Scott Wang <scott_wang@…>:

Actually the first version of the patch uses the add_warning method just like the one in the ticket manipulator does. How are we going to solve this problem?

I should have been clearer in comment:4, sorry. What I didn't like was displaying an error page, and having the errors in a separate warning box. An error page should have the error in the error box, so that's what I suggested.

What Simon suggests above is not to display an error page, but to go back to the attachment page, with all fields unchanged, and the warnings displayed on top. It's much more user-friendly, as the user doesn't have to go back to the attachment page. We do the same e.g. when submitting tickets, and we should really do it everywhere (e.g. invalid milestone times, …), because it's so much nicer.

[scott_wang@…]

Has been busy for a while before fixing this issue.

New patch and screenshot attached. Now it will stay on the same page and show the warning message as suggested.

[Remy Blank]

This looks much better. It should be possible to avoid the redirect by simply re-rendering the "Add Attachment" page, already pre-filled with the field values passed in the request.

[scott_wang@…]

Replying to rblank:

This looks much better. It should be possible to avoid the redirect by simply re-rendering the "Add Attachment" page, already pre-filled with the field values passed in the request.

Well, I'm not familiar with the code and does not know how to do that efficiently without adding extra stuff. Maybe someone knows the attachment module better can do this?

[osimons]

Patch against current trunk, updated to handle re-populating fields.

[osimons]

Replying to scott_wang@…:

Well, I'm not familiar with the code and does not know how to do that efficiently without adding extra stuff. Maybe someone knows the attachment module better can do this?

See t10353-show_attachment_manipulator_error_4-r10893.diff​. Seeing the browser only forwards the filename and not the full path, there is no way that I can think of to actually repopulate the filename field too. Should we add another warning to make sure the user remember to re-select a file?

Note: The patch is now updated against trunk, and not 0.12. I doubt it will make stable at this point.

[Remy Blank]

Replying to osimons:

See t10353-show_attachment_manipulator_error_4-r10893.diff​. Seeing the browser only forwards the filename and not the full path, there is no way that I can think of to actually repopulate the filename field too. Should we add another warning to make sure the user remember to re-select a file?

That's probably a good idea, yes.

[osimons]

Added extra warning for user to select the file to upload again.

[osimons]

Replying to rblank:

That's probably a good idea, yes.

Agree. Added to t10353-show_attachment_manipulator_error_5-r10893.diff​.

[Remy Blank]

Preparing for 1.0.

[Christian Boos]

Simon, any chance this could come to a conclusion for 1.0?

[osimons]

The patch applies cleanly in latest version, and I have nothing further to add at this point. Shall I commit?

[Remy Blank]

Yes, please do :)

[osimons]

Committed as [11106].