Opened 3 years ago

Last modified 14 months ago

#24539 new Bug

Attempt to create object with repeated value on a custom PK raises IntegrityError on wrong field

Reported by: Evandro Myller Owned by: nobody
Component: Database layer (models, ORM) Version: 1.7
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 (last modified by Evandro Myller)

I have this model Product, child of an abstract model which defines a created field with auto_now_add set. All the CRUD works just fine so far.

The problem happens when I add a custom primary key to Product: If I try to create a Product object through the admin giving the reference field a repeated value, expecting to see a nice validation error message saying that another object with that reference already exists, I get an IntegrityError stating that the created field cannot be NULL (traceback attached) -- nothing even about the custom primary key.

Notes:

  • I noticed the exception is raised from an UPDATE query, which is really odd since I'm posting data from the admin add view.
  • Product(reference=x).save() raises the same exception, but Product.objects.create(reference=x raises the expected exception (IntegrityError about the PK's UNIQUE constraint), which proves that it's not a problem on the admin.

Attachments (4)

traceback.txt (6.2 KB) - added by Evandro Myller 3 years ago.
full traceback
models.py (323 bytes) - added by Evandro Myller 3 years ago.
example models
24539-test.diff (1.0 KB) - added by Tim Graham 3 years ago.
admin.py (426 bytes) - added by Evandro Myller 3 years ago.
admin module

Download all attachments as: .zip

Change History (12)

Changed 3 years ago by Evandro Myller

Attachment: traceback.txt added

full traceback

Changed 3 years ago by Evandro Myller

Attachment: models.py added

example models

comment:1 Changed 3 years ago by Evandro Myller

Description: modified (diff)

comment:2 Changed 3 years ago by Tim Graham

Triage Stage: UnreviewedAccepted

I'm unable to reproduce the admin problem with the provided models.py file. I see the "Product with this Reference already exists." error in the admin as expected (tested on master and stable/1.7.x).

I could reproduce the Product(reference=x).save() problem though. Test attached.

Changed 3 years ago by Tim Graham

Attachment: 24539-test.diff added

comment:3 Changed 3 years ago by Baptiste Mispelon

I'm not sure this is actually a bug.

When you save a mode that has its primary key set, Django will look in the database to see if that key exists.
If it's found, then the corresponding record is updated (hence why you see an UPDATE query) with the values set on the instance.

What's happening here is that the UPDATE query tries to set created to NULL because the object you're saving has created = None.

comment:4 Changed 3 years ago by Tim Graham

Maybe not, I'm not sure the behavior is well defined (i.e. if should expect Field.pre_save() to be called in such a situation). Probably not very high priority at least. I'd say we can close this unless the reporter can provide more details about how to reproduce the admin bug.

comment:5 Changed 3 years ago by Evandro Myller

Hi. I tried to isolate the code necessary to reproduce the bug through the admin. Please check the attached file.

I agree it may be not high priority, but the raised exception is really odd and should have some attention IMHO. I don't know the low-level database-related modules in the Django package and currently don't have time to study them well enough, otherwise I'd be happy to send a patch.

Thanks for looking into the issue.

Last edited 3 years ago by Evandro Myller (previous) (diff)

Changed 3 years ago by Evandro Myller

Attachment: admin.py added

admin module

comment:6 Changed 3 years ago by Evandro Myller

I believe it's not an admin-related problem. The problem is gone if I remove the clean method from the model form (or call super(ProductForm, self).clean() on it). Still odd, but this might bring in some clue.

comment:7 Changed 3 years ago by fero

I debugged it for a while during the Django sprint at pycon6 Italy and it seems to be quite a Pandora's box.

I reviewed it with @apollo13 and here are the results, for a bug that can't be solved today.
In our opinion there are some bugs actually:

  1. regarding to the ORM low-level exception, the story is: when a user add a new instance from the Admin UI, he expects to INSERT a new object here is a candidate patch:
--- a/django/contrib/admin/options.py
+++ b/django/contrib/admin/options.py
@@ -969,7 +969,11 @@ class ModelAdmin(BaseModelAdmin):
         """
         Given a model instance save it to the database.
         """
-        obj.save()
+        if not change:
+            obj.save(force_insert=True)
+        else:
+            obj.save()
+
 
     def delete_model(self, request, obj):
         """

in this way the ORM performs an "INSERT" and raises the right IntegrityError exception UNIQUE constraint failed: base_product.reference (provided that you use python3 or you have applied some fixes/patches from #23643)

  1. regarding to the UX, the story is: when a user input some wrong values in a form, he expects to get a ValidationError with pink-rabbits decorations. I digged in this part with the help of @apollo13 and I noticed that the overridden primary_key field is correctly validated with the following "adjustment" ;) :
--- a/django/forms/models.py
+++ b/django/forms/models.py
@@ -433,7 +433,7 @@ class BaseModelForm(BaseForm):
         self.instance = construct_instance(self, self.instance, opts.fields, construct_instance_exclude)
 
         try:
-            self.instance.full_clean(exclude=exclude, validate_unique=False)
+            self.instance.full_clean(exclude=exclude, validate_unique=True)
         except ValidationError as e:
             self._update_errors(e)

Agreeing with @apollo13 this topic requires further discussion. So I leave it up to you dear core developers ;) This regards to ModelForm though and so has wider impact than the only admin interface (new ticket needed?)

  1. after that, the update of a primary key value has another mess to deal with: when a user update the value, he expects to just update that value ;).

The following cases could happen when value switches from xxx to yyy:

  1. one record with yyy already exists -> ValidationError should be raised and this could happen after fixing the validate_unique above
  2. one record with yyy does not exists -> the value is updated. Currently when the user update the value to yyy, the ORM performs UPDATE table SET k='yyy' WHERE k='yyy' instead of UPDATE table SET k='yyy' WHERE k='xxx'

That's my end of the sprint, it has been a good day, thanks Django team!

comment:8 Changed 14 months ago by Tim Graham

Component: UncategorizedDatabase layer (models, ORM)
Note: See TracTickets for help on using tickets.
Back to Top