Opened 6 years ago

Closed 5 years ago

#29871 closed Bug (fixed)

Resetting primary key for a child model doesn't work.

Reported by: Victor Porton Owned by: Chetan Khanna
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

In the attached example code setting the primary key to None does not work (so that the existing object is overwritten on save()).

The most important code fragments of the bug example:

from django.db import models

class Item(models.Model):
    # uid = models.UUIDField(primary_key=True, default=uuid.uuid4, editable=False)
    uid = models.AutoField(primary_key=True, editable=False)

    f = models.BooleanField(default=False)

    def reset(self):
        self.uid = None
        self.f = False

class Derived(Item):
    pass
class SaveTestCase(TestCase):
    def setUp(self):
        self.derived = Derived.objects.create(f=True)  # create the first object
        item = Item.objects.get(pk=self.derived.pk)
        obj1 = item.derived
        obj1.reset()
        obj1.save()  # the first object is overwritten

    def test_f_true(self):
        obj = Item.objects.get(pk=self.derived.pk)
        self.assertTrue(obj.f)

Django 2.1.2

Attachments (1)

bug.tar.gz (3.0 KB ) - added by Victor Porton 6 years ago.

Download all attachments as: .zip

Change History (20)

by Victor Porton, 6 years ago

Attachment: bug.tar.gz added

comment:1 by Tim Graham, 6 years ago

I'm not sure if this is a bug. The test passes after adding self.item_ptr = None to Item.reset(). Is that the behavior you're looking for?

comment:2 by Simon Charette, 6 years ago

Resolution: invalid
Status: newclosed

I agree with Tim here.

It feels like what you're after is self.pk = None as it will be alias for self.item_ptr for Derived instances and self.uid for Item instances.

comment:3 by Victor Porton, 6 years ago

@Simon Charette

No self.pk = None does not work too.

It seems that there is no reliable (not error-prone, as depending on the usage of base or derived class) way to do this :-(

comment:4 by Victor Porton, 6 years ago

Can we consider that self.pk = None does not work too, as a bug?

At least it is a counterintuitive (and dangerous for the data!) behavior.

comment:5 by Simon Charette, 6 years ago

Hello Victor, could you provide more details about what exactly you are trying to achieve here?

So far you've only provided a test case that fails. Are you trying to create a copy of an existing objects using MTI?

Providing more details about what you are trying to achieve and why you're expecting the test to pass would help us to determining if this is actually a bug.

Thanks!

Version 0, edited 6 years ago by Simon Charette (next)

in reply to:  5 comment:6 by Victor Porton, 6 years ago

Replying to Simon Charette:

Hello Victor, could you provide more details about what exactly you are trying to achieve here?

So far you've only provided a test case that fails. Are you trying to create a copy of an existing objects using MTI?

Yes. I am trying to create a copy of an existing object using MTI.

Providing more details about what you are trying to achieve and why you're expecting the test to pass would help us to determining if this is actually a bug.

I am trying to create a copy of a Derived object which was in the DB long before. The copy should contain all fields of the Derived model and all fields of its base models.

As for now, I do not know a reliable and not error-prone (such as depending on usage of base of derived class) way to do this. If there is no such way, it is a missing feature in Django and this should be considered at least as a feature suggestion.

In my real code I may have several levels of inheritance (not just Item and Derived).

comment:7 by Simon Charette, 6 years ago

Thanks for the extra details Victor.

Could you confirm that the following patch work for you when setting self.pk = None in reset().

diff --git a/django/db/models/base.py b/django/db/models/base.py
index 751f42bb9b..535928ce05 100644
--- a/django/db/models/base.py
+++ b/django/db/models/base.py
@@ -553,7 +553,11 @@ class Model(metaclass=ModelBase):
         return getattr(self, meta.pk.attname)

     def _set_pk_val(self, value):
-        return setattr(self, self._meta.pk.attname, value)
+        field = self._meta.pk
+        setattr(self, field.attname, value)
+        while getattr(field, 'parent_link', False):
+            field = field.target_field
+            setattr(self, field.attname, value)

     pk = property(_get_pk_val, _set_pk_val)

This code should make sure that setting self.pk = None does self.item_ptr_id = self.id = None for any level of concrete model inheritance. That should be enough for save() to create new objects from my local testing.

FWIW this changes passes the full test suite on SQLite so it could be a tentative patch but it kind of break the symmetry with _get_pk.

Something to keep in mind though is that right now pk = None assignment trick for object copying is neither documented or embraced by the Django documentation AFAIK.

Last edited 6 years ago by Simon Charette (previous) (diff)

in reply to:  7 comment:8 by Victor Porton, 6 years ago

Replying to Simon Charette:

Could you confirm that the following patch work for you when setting self.pk = None in reset().

No, the patch does not make self.pk = None to work!

pip install django
...
patch -p1 < ~/t/patch.diff
cd /home/porton/Projects/test/testsave
(env) testsave,0$ ./manage.py test
Creating test database for alias 'default'...
System check identified no issues (0 silenced).
F
======================================================================
FAIL: test_f_true (test1.tests.SaveTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/porton/Projects/test/testsave/test1/tests.py", line 19, in test_f_true
    self.assertTrue(obj.f)
AssertionError: False is not true

----------------------------------------------------------------------
Ran 1 test in 0.005s

FAILED (failures=1)
Destroying test database for alias 'default'...

comment:9 by Simon Charette, 6 years ago

The following should do

diff --git a/django/db/models/base.py b/django/db/models/base.py
index 751f42bb9b..d3141d6180 100644
--- a/django/db/models/base.py
+++ b/django/db/models/base.py
@@ -553,6 +553,8 @@ class Model(metaclass=ModelBase):
         return getattr(self, meta.pk.attname)

     def _set_pk_val(self, value):
+        for parent_link in self._meta.parents.values():
+            setattr(self, parent_link.target_field.attname, value)
         return setattr(self, self._meta.pk.attname, value)

     pk = property(_get_pk_val, _set_pk_val)

in reply to:  9 ; comment:10 by Victor Porton, 6 years ago

Replying to Simon Charette:

The following should do

My test with this patch passed.

in reply to:  10 comment:11 by Victor Porton, 6 years ago

Replying to Victor Porton:

My test with this patch passed.

When to expect it to be included in Django distribution?

comment:12 by Tim Graham, 6 years ago

Resolution: invalid
Status: closednew
Summary: Resetting primary key for a derived object does not workResetting primary key for a child model doesn't work
Triage Stage: UnreviewedAccepted

comment:13 by Tim Graham, 6 years ago

Has patch: set
Patch needs improvement: set

The patch doesn't seem to work for child models that inherit from multiple models. It also created some other test failures. See my PR.

comment:14 by Simon Charette, 6 years ago

Weird that didn't get these failures locally. Anyway this clearly need more work as it was an idea designed on the back of an envelope.

comment:15 by Chetan Khanna, 5 years ago

Owner: changed from nobody to Chetan Khanna
Status: newassigned

https://github.com/django/django/pull/12273

I was able to start this PR thanks to the work Simon and Tim did. Unfortunately the regression test fail on mysql (I didn't run tests over mysql before creating the PR). Posting here for reference. I'll update on further progress in a few days.

comment:16 by Chetan Khanna, 5 years ago

Patch needs improvement: unset

I've updated the PR, the test is now passing.

comment:17 by Mariusz Felisiak, 5 years ago

Patch needs improvement: set
Summary: Resetting primary key for a child model doesn't workResetting primary key for a child model doesn't work.
Version: 2.1master

comment:18 by Mariusz Felisiak, 5 years ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:19 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

Resolution: fixed
Status: assignedclosed

In 63e6ee1f:

Fixed #29871 -- Allowed setting pk=None on a child model to create a copy.

Thanks Simon Charette and Tim Graham for the initial patch.

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