Opened 7 years ago

Closed 21 months ago

#28405 closed Bug (duplicate)

CICharField on a ModelFormSet doesn't catch unique constraint violations with different capitalization

Reported by: Jon Dufresne Owned by:
Component: contrib.postgres Version: 1.11
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

When CICharField (new in 1.11) with unique=True is used on a ModelFormSet, values differing only by case are not reported as a validation error. As the form appears to be valid, saving the form will result in a database IntegrityError.

Will attach example test.

For example:

class MyModel(models.Model):
    name = CICharField(max_length=100, unique=True)

MyModelFormset = modelformset_factory(MyModel, fields=['name'])

data = {
    'form-TOTAL_FORMS': 2,
    'form-INITIAL_FORMS': 0,
    'form-0-name': 'frank',
    'form-1-name': 'FRANK',
}
formset = MyModelFormSet(data)

Trying to save the formset, which reports no validation errors, results in an exception:

Traceback (most recent call last):
  File "/home/jon/devel/django/tests/postgres_tests/test_citext.py", line 59, in test_duplicate_formset_mixed_case
    formset.save()
  File "/home/jon/devel/django/django/forms/models.py", line 671, in save
    return self.save_existing_objects(commit) + self.save_new_objects(commit)
  File "/home/jon/devel/django/django/forms/models.py", line 806, in save_new_objects
    self.new_objects.append(self.save_new(form, commit=commit))
  File "/home/jon/devel/django/django/forms/models.py", line 647, in save_new
    return form.save(commit=commit)
  File "/home/jon/devel/django/django/forms/models.py", line 457, in save
    self.instance.save()
  File "/home/jon/devel/django/django/db/models/base.py", line 711, in save
    force_update=force_update, update_fields=update_fields)
  File "/home/jon/devel/django/django/db/models/base.py", line 741, in save_base
    updated = self._save_table(raw, cls, force_insert, force_update, using, update_fields)
  File "/home/jon/devel/django/django/db/models/base.py", line 825, in _save_table
    result = self._do_insert(cls._base_manager, using, fields, update_pk, raw)
  File "/home/jon/devel/django/django/db/models/base.py", line 863, in _do_insert
    using=using, raw=raw)
  File "/home/jon/devel/django/django/db/models/manager.py", line 82, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
  File "/home/jon/devel/django/django/db/models/query.py", line 1047, in _insert
    return query.get_compiler(using=using).execute_sql(return_id)
  File "/home/jon/devel/django/django/db/models/sql/compiler.py", line 1202, in execute_sql
    cursor.execute(sql, params)
  File "/home/jon/devel/django/django/db/backends/utils.py", line 62, in execute
    return self.cursor.execute(sql, params)
  File "/home/jon/devel/django/django/db/utils.py", line 89, in __exit__
    raise dj_exc_value.with_traceback(traceback) from exc_value
  File "/home/jon/devel/django/django/db/backends/utils.py", line 62, in execute
    return self.cursor.execute(sql, params)
django.db.utils.IntegrityError: duplicate key value violates unique constraint "postgres_tests_citestmodel2_name_key"
DETAIL:  Key (name)=(FRANK) already exists.

Attachments (1)

test.diff (3.0 KB ) - added by Jon Dufresne 7 years ago.
An example unit test

Download all attachments as: .zip

Change History (6)

by Jon Dufresne, 7 years ago

Attachment: test.diff added

An example unit test

comment:1 by Tim Graham, 7 years ago

Triage Stage: UnreviewedAccepted

A related issue (case-insensitive collations on MySQL causing this problem) is #23964.

comment:2 by Jon Dufresne, 7 years ago

As a potential solution, what do others think about catching the database's IntegrityError during ModelForm.save() to report unique constraint violations in model forms? The database is already optimized for robust checking of unique constraints, why repeat that code at the application layer? It produces subtle differences and edge cases. This solution could potentially deprecate the large amounts of Django code used to check if a submitted form violates unique constraints and rely on the basic database feature instead. All builtin datbase backends support unique constraints. If we rely on the database, we wouldn't need to figure out how the database is comparing rows (case sensitive vs case insensitive), it'll tell us.

Something to note, this would mean that not all validation occurs before the ModelForm.save() and that the ModelForm.save() can be unsuccessful and could add errors to the form.

(Usual backward compatibility concerns apply.)

comment:3 by Tim Graham, 7 years ago

I think it'd be difficult to translate a database's exception into the corresponding (translated) Django message.

comment:4 by Simon Charette, 6 years ago

I feel like the underlying issue here is that the formset logic is not relying on the database to determine unicity of values like a model form does through Model.validate_unique.

Having CICharField and friends return a subclass of str that implements __eq__(self, other): self.casefold() == other.casefold() as suggested in #28405 would solve the issue for these fields but that not for case-insensitive collations (#23964) and any other kind of non-injective functional unique index. The same thing can be said about database level constraints.

comment:5 by Mariusz Felisiak, 21 months ago

Resolution: duplicate
Status: newclosed

Marking as a duplicate of #23964 (see comment).

Note: See TracTickets for help on using tickets.
Back to Top