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)
Change History (20)
by , 6 years ago
Attachment: | bug.tar.gz added |
---|
comment:1 by , 6 years ago
comment:2 by , 6 years ago
Resolution: | → invalid |
---|---|
Status: | new → closed |
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 , 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 , 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.
follow-up: 6 comment:5 by , 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.
Does setting both self.uid
and self.pk
to None
works?
Thanks!
comment:6 by , 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
).
follow-up: 8 comment:7 by , 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.
comment:8 by , 6 years ago
Replying to Simon Charette:
Could you confirm that the following patch work for you when setting
self.pk = None
inreset()
.
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'...
follow-up: 10 comment:9 by , 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)
follow-up: 11 comment:10 by , 6 years ago
comment:11 by , 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 , 6 years ago
Resolution: | invalid |
---|---|
Status: | closed → new |
Summary: | Resetting primary key for a derived object does not work → Resetting primary key for a child model doesn't work |
Triage Stage: | Unreviewed → Accepted |
comment:13 by , 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 , 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 , 5 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
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 , 5 years ago
Patch needs improvement: | unset |
---|
I've updated the PR, the test is now passing.
comment:17 by , 5 years ago
Patch needs improvement: | set |
---|---|
Summary: | Resetting primary key for a child model doesn't work → Resetting primary key for a child model doesn't work. |
Version: | 2.1 → master |
comment:18 by , 5 years ago
Patch needs improvement: | unset |
---|---|
Triage Stage: | Accepted → Ready for checkin |
I'm not sure if this is a bug. The test passes after adding
self.item_ptr = None
toItem.reset()
. Is that the behavior you're looking for?