Code

Opened 4 years ago

Closed 3 years ago

#13249 closed Bug (duplicate)

Feature request: ModelForm validation error handling after commit=False

Reported by: orokusaki Owned by: nobody
Component: Forms Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Design decision needed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX:

Description

For lack of a good explanation, I'll include an example first:

#First, the models and forms used in example:

class Account(models.Model):
    subdomain = SubdomainField(max_length=25, unique=True)


class Administrator(models.Model):
    account = models.ForeignKey(Account, related_name='administrators')
    username = LowerAlphanumericField(max_length=50)
    email = models.EmailField()
    
    class Meta:
        unique_together = (('account', 'username'), ('account', 'email'))


class AdministratorForm(ModelForm):
    class Meta:
        model = Administrator
        exclude = ('account',)

And, here's the example usage that breaks:

#views.py

def create_administrator(request):
    from django.db.utils import IntegrityError
    from myproject.apps.account.forms import AdministratorForm
    if request.method == 'POST':
        form = AdministratorForm(request.POST)
        if form.is_valid():
            instance = form.save(commit=False)
            instance.account = request.account  # ``account`` is attached to ``request`` in custom middleware.
            instance.save()
            # What happens if there is an error here such as IntegrityError with one of the 2 unique_together fields ('username', and 'email').
            # You cannot tell which field failed the ``unique_together`` test. validate_unique skips over the excluded fields in the form.
            #
            # You could run instance.full_clean() first and catch ValidationErrors and handle them manually, but this would not be very DRY after about 3 views.
            # I don't know what the new version would be like, but I can imagine a way to validate the instance and then add the errors into the form again or something.

I probably made this very confusing, but basically there is no way to use commit=False and still use ModelForm's built-in validation conveniences.

Attachments (0)

Change History (6)

comment:1 Changed 4 years ago by orokusaki

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design decision needed

Here is one case during the model validation talks in January where somebody talks about precisely what I'm dealing with, although he puts it better:

http://osdir.com/ml/django-developers/2010-01/msg00169.html

comment:2 Changed 4 years ago by orokusaki

One solution I could propose as an abstract idea is to allow for the inclusion of extra data during the creation of a ModelForm instance:

form = MySpamModelForm(request.POST, data_override={'account': request.account})
if form.is_valid():
    instance = form.save()

This doesn't allow for the use of instance.pk, but it at lease allows you to add request related data to the instance before creation.

comment:3 Changed 4 years ago by russellm

  • Resolution set to wontfix
  • Status changed from new to closed
  • Triage Stage changed from Design decision needed to Unreviewed

Please don't triage your own tickets. The DDN marker indicates that the idea has been accepted prima facie, but a final decision is required.

Firstly, the assertion in the problem statement ("there is no way to use commit=False and still use ModelForm?'s built-in validation conveniences") is false. When you call 'form.save(clean=False)', you're saying that you're taking control of making sure the instance is saved properly. If you're using model validation, this means taking control of the validation process, including calls to full_clean(), and the handling of the resultant errors. The docs added in r12206 pretty much say this explicitly, and give examples of how it should be handled.

As for potential fixes - the "data override" idea you suggest isn't required- it's already possible to do this by specifying an instance.

    if request.method == 'POST':
        new_obj = Administrator()
        new_obj.account = request.account
        form = AdministratorForm(data=request.POST, instance=new_obj)
        if form.is_valid():
            form.save()
        ...

comment:4 Changed 4 years ago by orokusaki

@Russellm

There's still one problem with doing what you did above.

Thanks for the code snippet BTW first of all. I ended up using form.instance.account = request.account before my call to form.is_valid() , but in the case of adding data to an instance or a form during all of this is usually due to the fact that the field is not included in the form because of editable=False on the model or exclude=('field',) in the form. When this is the case the fields that are excluded still don't go through validation with form.is_valid() . Is it reasonable to request the addition of either (A) If a field's value is set via form.instance.account = request.account , then it is included in validation again, or (B) A way to add the field back into validation again manually such as form.include_in_validation = 'account' .

If you'd like, I can create a new ticket, or is it relevant enough to this ticket, or do you think it's even worth the time for what I mentioned?

comment:5 Changed 3 years ago by legutierr

  • Easy pickings unset
  • Resolution wontfix deleted
  • Severity set to Normal
  • Status changed from closed to reopened
  • Triage Stage changed from Unreviewed to Design decision needed
  • Type set to Bug

I agree with orokusaki that although there is a better approach to that described in the original ticket (i.e. the use of form.save(commit=False)), there is, nonetheless, a problem in the way that form.is_valid() and model validation validates unique_together. Currently, model validation ignores all unique_together tuples where *any* of the referenced fields are excluded; instead, model validation should only ignore those unique_together tuples where *all* of the referenced fields are excluded.

I am reopening this ticket with the purpose of marking it as a duplicate of #13091.

comment:6 Changed 3 years ago by legutierr

  • Resolution set to duplicate
  • Status changed from reopened to closed

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.