modelformset_factory and unique_together don't always validate unique fields

[Jon Dufresne]

I'm reporting this to the developers list as I feel this shows a
shortfall in (to me) expected behavior. I'm not sure this is exactly a
bug or simply a use case the unique validation wasn't designed for.

Basically, sometimes I want to create model formsets that use a
limited number of model fields. These model fields may have a unique
constraint using unique_together. The other field (or fields) of
unique together may not be included in the formset. Upon validating
the form, unique_together fields are only checked if all fields are
contained in the form. This means, my unique fields will not be
validated automatically. To achieve the validation I usually have to
copy parts of Django form validation, missing out on DRY.

I think one solution would be for model formsets to have a "static" or
"constant" fields argument. This could be a single value per field
that all forms in the formset would share for a particular set of
fields. These fields would not be a part of the rendered form, but
could be used for validating the forms and creating new models
instances. Thoughts?

An example where I might do this: suppose I have a "container" model.
This model has many "item" models created through a FK relationship.
Perhaps a field is unique together with the container. This example
could apply to any container/item relationship. I might create a
formset for all items of just one container for a bulk (unique)
rename. I have created a simple small example that illustrates my
point:

models.py

----
class Container(models.Model):
pass

class Item(models.Model):
container = models.ForeignKey(Container)
name = models.CharField(max_length=100)

class Meta:
unique_together = ('container', 'name')

ItemFormSet = modelformset_factory(model=Item, fields=['name'])
----

tests.py
----
class ItemFormSetTestCase(TestCase):
def test_unique_item_name(self):
container = Container()
container.save()
item1 = Item(container=container, name='item1')
item1.save()
item2 = Item(container=container, name='item2')
item2.save()
data = {
'form-TOTAL_FORMS': 2,
'form-INITIAL_FORMS': 2,
'form-MAX_NUM_FORMS': 2,
'form-0-id': str(item1.pk),
'form-0-name': 'newname',
'form-1-id': str(item2.pk),
'form-1-name': 'newname',
}
formset = ItemFormSet(
data,
queryset=Item.objects.filter(container=container))
self.assertFalse(formset.is_valid())
---

This test fails because the uniqueness of name is not actually
validated. If I were to go ahead an save this "valid" form, I receive
the following error:

---
Traceback (most recent call last):
File "/home/jon/djtest/djtest/myapp/tests.py", line 27, in
test_unique_item_name
formset.save()
File "/home/jon/djtest/venv/lib/python2.7/site-packages/django/forms/models.py",
line 636, in save
return self.save_existing_objects(commit) + self.save_new_objects(commit)
File "/home/jon/djtest/venv/lib/python2.7/site-packages/django/forms/models.py",
line 753, in save_existing_objects
saved_instances.append(self.save_existing(form, obj, commit=commit))
File "/home/jon/djtest/venv/lib/python2.7/site-packages/django/forms/models.py",
line 623, in save_existing
return form.save(commit=commit)
File "/home/jon/djtest/venv/lib/python2.7/site-packages/django/forms/models.py",
line 457, in save
construct=False)
File "/home/jon/djtest/venv/lib/python2.7/site-packages/django/forms/models.py",
line 103, in save_instance
instance.save()
File "/home/jon/djtest/venv/lib/python2.7/site-packages/django/db/models/base.py",
line 590, in save
force_update=force_update, update_fields=update_fields)
File "/home/jon/djtest/venv/lib/python2.7/site-packages/django/db/models/base.py",
line 618, in save_base
updated = self._save_table(raw, cls, force_insert, force_update,
using, update_fields)
File "/home/jon/djtest/venv/lib/python2.7/site-packages/django/db/models/base.py",
line 680, in _save_table
forced_update)
File "/home/jon/djtest/venv/lib/python2.7/site-packages/django/db/models/base.py",
line 724, in _do_update
return filtered._update(values) > 0
File "/home/jon/djtest/venv/lib/python2.7/site-packages/django/db/models/query.py",
line 598, in _update
return query.get_compiler(self.db).execute_sql(CURSOR)
File "/home/jon/djtest/venv/lib/python2.7/site-packages/django/db/models/sql/compiler.py",
line 1003, in execute_sql
cursor = super(SQLUpdateCompiler, self).execute_sql(result_type)
File "/home/jon/djtest/venv/lib/python2.7/site-packages/django/db/models/sql/compiler.py",
line 785, in execute_sql
cursor.execute(sql, params)
File "/home/jon/djtest/venv/lib/python2.7/site-packages/django/db/backends/utils.py",
line 65, in execute
return self.cursor.execute(sql, params)
File "/home/jon/djtest/venv/lib/python2.7/site-packages/django/db/utils.py",
line 94, in __exit__
six.reraise(dj_exc_type, dj_exc_value, traceback)
File "/home/jon/djtest/venv/lib/python2.7/site-packages/django/db/backends/utils.py",
line 65, in execute
return self.cursor.execute(sql, params)
File "/home/jon/djtest/venv/lib/python2.7/site-packages/django/db/backends/sqlite3/base.py",
line 485, in execute
return Database.Cursor.execute(self, query, params)
IntegrityError: UNIQUE constraint failed: myapp_item.container_id,
myapp_item.name
---

[Florian Apolloner]

Hi Jon,

To me this is no shortfall but expected and good behaviour, including other fields in the validation (i.e. fields not on the form) would be very confusing. Where would errors show up? Also, even if we find a place to show the errors, the user is (usually) in no position to correct them (after all, there is no field he could change to fix it). I think in your case you should call model.full_clean() after form validation and handle any unique errors there…

Cheers,
Florian

[Jon Dufresne]

On Thu, Jul 3, 2014 at 1:12 AM, Florian Apolloner <f.apo...@gmail.com> wrote:
> To me this is no shortfall but expected and good behaviour, including other
> fields in the validation (i.e. fields not on the form) would be very
> confusing. Where would errors show up?

Right now unique violations show up as non-field errors on the forms
with duplicate values. So I my proposal would do the same.

> Also, even if we find a place to show
> the errors, the user is (usually) in no position to correct them (after all,
> there is no field he could change to fix it).

I don't follow. In my specific example the user is able to change the
"name" field. In my opinion, the form should fail to validate because
the _user_ entered "newname" twice, for two different names when they
should be unique. The user is in the position to 1) make these
conflict and 2) correct them.

> I think in your case you
> should call model.full_clean() after form validation and handle any unique
> errors there…

I will certainly experiment with this thanks.

Cheers,
Jon

[Tim Graham]

Another suggestion: could you include the field on the form but use a HiddenInput widget?

[Jon Dufresne]

On Thu, Jul 3, 2014 at 7:02 AM, Tim Graham <timog...@gmail.com> wrote:
> Another suggestion: could you include the field on the form but use a
> HiddenInput widget?

I could, but originally I wanted to avoid this. The container_id
should not be editable by the user. Including this in the form seems
to make a statement against this. I could experiment making this
readonly so the form validates that the value is unchanged. But in the
end, this seems to be a workaround to implement my original
suggestion. That is, when instantiating the formset, pass a dictionary
of "static" fields and their data, that are not rendered as part of
the form HTML, but could be used in:

* Default queryset (could always be overridden)
* Creating new instances
* Validating form data

I suppose readonly hidden inputs would achieve the same effect, but it
seems there is no need for the data to be a part of the rendered HTML
form.

I'll play around with this, thanks for the suggestion.

> I think in your case you
> should call model.full_clean() after form validation and handle any unique
> errors there…

I experimented with this, but this will not work. The method
model.full_clean() validates against the database. But the form needs
to validate against unsaved data. I imagine this is why
BaseModelFormSet.validate_unique() was written in the first place.

[gavi...@gmail.com]

I, too, struggle with this frequently.

[Florian Apolloner]

On Thursday, July 3, 2014 3:03:25 PM UTC+2, Jon Dufresne wrote:

> Also, even if we find a place to show
> the errors, the user is (usually) in no position to correct them (after all,
> there is no field he could change to fix it).

I don't follow. In my specific example the user is able to change the
"name" field. In my opinion, the form should fail to validate because
the _user_ entered "newname" twice, for two different names when they
should be unique. The user is in the position to 1) make these
conflict and 2) correct them.

I don't follow; if the user is able to change the "name" field; it's included in the form and uniqueness is checked, I thought we are talking about fields __not__ included in a form.

Cheers,
florian

[Gavin Wahl]

The problem is when a subset of the fields in a unique_together constraint are in the form.

--
You received this message because you are subscribed to a topic in the Google Groups "Django developers" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/django-developers/FmllO4t53bE/unsubscribe.
To unsubscribe from this group and all its topics, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/53cd7280-38ba-40cb-aff1-0f9558673b39%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

[Jon Dufresne]

The "name" field is included in the form, but the "container" field is
not. The name and container fields are unique_together. The formset
only validates unique fields when all fields are in the form. Here is
the example again.

[Carl Meyer]

Hi Jon,

On 07/02/2014 09:06 PM, Jon Dufresne wrote:
> I'm reporting this to the developers list as I feel this shows a
> shortfall in (to me) expected behavior. I'm not sure this is exactly a
> bug or simply a use case the unique validation wasn't designed for.
>
> Basically, sometimes I want to create model formsets that use a
> limited number of model fields. These model fields may have a unique
> constraint using unique_together. The other field (or fields) of
> unique together may not be included in the formset. Upon validating
> the form, unique_together fields are only checked if all fields are
> contained in the form. This means, my unique fields will not be
> validated automatically. To achieve the validation I usually have to
> copy parts of Django form validation, missing out on DRY.
>
> I think one solution would be for model formsets to have a "static" or
> "constant" fields argument. This could be a single value per field
> that all forms in the formset would share for a particular set of
> fields. These fields would not be a part of the rendered form, but
> could be used for validating the forms and creating new models
> instances. Thoughts?

I agree this is a problem that should be fixed; it is tracked in
https://code.djangoproject.com/ticket/13091 and there is already a
history of proposals for fixing it.

I think your proposed fix is a new one, though. It's less flexible than
the context-manager API I had proposed at one time [1], but it may cover
the 80% case adequately, and has the major advantage of not requiring
any changes in the code that checks the validity of the form.

There is already a way to get "default" values for excluded fields into
a ModelForm: pass it an unsaved "instance" at form instantiation, with
those fields set on it. So it feels odd to me to add a new way to pass
in such values. But passing in an instance doesn't trigger full-model
validation, and making it do that could break existing code, so we'd
still need some way to flag that you want that behavior.

Hmm...

Carl

[1]
https://groups.google.com/d/topic/django-developers/PT17cgDSOw8/discussion