[Django] #29871: Resetting primary key for a derived object does not work
Django
-------------------------------------+-------------------------------------
Reporter: Victor | Owner: nobody
Porton |
Type: Bug | Status: new
Component: Database | Version: 2.1
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
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
--
Ticket URL: <https://code.djangoproject.com/ticket/29871>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 2.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* Attachment "bug.tar.gz" added.
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 2.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham):
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/29871#comment:1>
Django
-------------------------------------+-------------------------------------
Component: Database layer | Version: 2.1
(models, ORM) |
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => invalid
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/29871#comment:2>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: closed
Component: Database layer | Version: 2.1
(models, ORM) |
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Victor Porton):
@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 :-(
--
Ticket URL: <https://code.djangoproject.com/ticket/29871#comment:3>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: closed
Component: Database layer | Version: 2.1
(models, ORM) |
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Victor Porton):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/29871#comment:4>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: closed
Component: Database layer | Version: 2.1
(models, ORM) |
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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?
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!
--
Ticket URL: <https://code.djangoproject.com/ticket/29871#comment:5>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: closed
Component: Database layer | Version: 2.1
(models, ORM) |
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Victor Porton):
Replying to [comment:5 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`).
--
Ticket URL: <https://code.djangoproject.com/ticket/29871#comment:6>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: closed
Component: Database layer | Version: 2.1
(models, ORM) |
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/29871#comment:7>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: closed
Component: Database layer | Version: 2.1
(models, ORM) |
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Victor Porton):
Replying to [comment:7 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'...
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29871#comment:8>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: closed
Component: Database layer | Version: 2.1
(models, ORM) |
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
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)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29871#comment:9>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: closed
Component: Database layer | Version: 2.1
(models, ORM) |
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Victor Porton):
Replying to [comment:9 Simon Charette]:
> The following should do
My test with this patch passed.
--
Ticket URL: <https://code.djangoproject.com/ticket/29871#comment:10>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: closed
Component: Database layer | Version: 2.1
(models, ORM) |
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Victor Porton):
Replying to [comment:10 Victor Porton]:
> My test with this patch passed.
When to expect it to be included in Django distribution?
--
Ticket URL: <https://code.djangoproject.com/ticket/29871#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: Victor Porton | Owner: nobody
Component: Database layer | Version: 2.1
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: closed => new
* resolution: invalid =>
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/29871#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: Victor Porton | Owner: nobody
Component: Database layer | Version: 2.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
Comment:
The patch doesn't seem to work for child models that inherit from multiple
models. It also created some other test failures. See my
[https://github.com/django/django/pull/10549 PR].
--
Ticket URL: <https://code.djangoproject.com/ticket/29871#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: Victor Porton | Owner: nobody
Component: Database layer | Version: 2.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/29871#comment:14>
Django
-------------------------------------+-------------------------------------
| Khanna
Type: Bug | Status: assigned
Component: Database layer | Version: 2.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
Comment:
I've updated the PR, the test is now passing.
--
Ticket URL: <https://code.djangoproject.com/ticket/29871#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: Victor Porton | Owner: Chetan
| Khanna
Type: Bug | Status: assigned
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
* version: 2.1 => master
--
Ticket URL: <https://code.djangoproject.com/ticket/29871#comment:17>
Django
-------------------------------------+-------------------------------------
Reporter: Victor Porton | Owner: Chetan
| Khanna
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/29871#comment:18>
Django
-------------------------------------+-------------------------------------
Reporter: Victor Porton | Owner: Chetan
| Khanna
Component: Database layer | Version: master
(models, ORM) |
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"63e6ee1f996e16a1a6238fed16fdb28bce156bc6" 63e6ee1f]:
{{{
#!CommitTicketReference repository=""
revision="63e6ee1f996e16a1a6238fed16fdb28bce156bc6"
Fixed #29871 -- Allowed setting pk=None on a child model to create a copy.
Thanks Simon Charette and Tim Graham for the initial patch.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29871#comment:19>