Code

Opened 5 years ago

Closed 5 years ago

#9605 closed (wontfix)

form.save() exceptions should be more informative

Reported by: to.roma.from.djbug@… Owned by: nobody
Component: Uncategorized Version: 1.0
Severity: Keywords:
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Currently form.save() raises a ValueError if !form.is_valid(). I suggest it should throw some exception from which form.errors could be extracted.

I have this workaround:

def enforce_valid(form):
    if not form.is_valid():
        raise invalid_form(form.errors)
    return form

...
enforce_valid(some_form(...)).save()
...

and also there’s code that catches the custom exception and puts the errors into a session variable and redirects back to the originating page and shows the errors there.

Why does form.save() only provide a message that something went wrong and not the actual errors? It could raise an exception derived from ValueError that would contain the list of errors.

Attachments (0)

Change History (3)

comment:1 Changed 5 years ago by kmtracey

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Why are you trying to save a form before first verifying that it is valid? The normal pattern (http://docs.djangoproject.com/en/dev/topics/forms/#using-a-form-in-a-view) is to call is_valid yourself first and not proceed with an attempt to save if the form hasn't validated. You seem to have come up with a different pattern (somewhat more complicated, involving storing errors in a session variable instead of just re-displaying the posted form with errors in place) and I don't understand why.

comment:2 Changed 5 years ago by to.roma.from.djbug@…

Okay, let me explain.

First of all, it’s common practice not to return a page in response to a POST, but rather always redirect to GET. That’s to allow bookmarking of URLs and sensible behavior of the reload button. Following this principle, when a POST fails, I redirect back to the original form and also pass the variable with the error list to that view. In fact, all my POSTs are to the same URL (/submission). A typical POST handler looks like this:

@post_handler(action="property:delete") # <input type="hidden" name="action" value="property:delete" />
@access_restricted # require request.user.is_authenticated, handle access_denied exception
@autoload_require_editable(models.property, pk="property_id") # fetches the object, can raise access_denied if not owned by request.user
def do_delete_property(request, property):
    property.delete()
    return url_from_view(console_list) # this calls urlresolvers.reverse and returns a string

where the function can’t even return anything except a URL that will be then wrapped in an HttpResponseRedirect.


What I ask for is merely:

  1. Create an exception class that can store form.errors;
  2. Raise it rather than ValueError;
  3. Not to break code that depends on current behavior.

I mean, rather than

if form.errors:
    raise ValueError("The %s could not be %s because the data didn't"
                     " validate." % (opts.object_name, fail_message))

have

class InvalidFormError(ValueError):
    ...

if form.errors:
    raise InvalidFormError("..." % (...), form.errors)

That would help for use cases such as mine, and would not break anything for users that do not need such a functionality.

comment:3 Changed 5 years ago by mtredinnick

  • Resolution set to wontfix
  • Status changed from new to closed

This isn't worth it -- it provides more than one way to do something. The errors are already available on the form instance at that moment (via self.errors) and if you want to throw away the form before checking the exception, you have to handle it yourself. Subclassing ModelForm and providing a wrapper that raises the exception you want seems like one solution there.

All your explanation of how POST and GET, etc, should be done is very normal and not relevant here. The only piece that seems relevant is that you don't access the form instance (it's difficult to tell from your example where the form handling is done, since it's all in the decorators). Since it's easy enough to do so (whatever one of your decorators is handling the form can take care of that), Django doesn't need a change to accommodate this.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.