[Django] #28385: deserialisers ignore natural keys when primary key has a default value
Django
------------------------------------------------+------------------------
Reporter: Daniel Knell | Owner: nobody
Type: Bug | Status: new
Component: Core (Serialization) | Version: 1.11
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 1
UI/UX: 0 |
------------------------------------------------+------------------------
https://github.com/django/django/blob/master/django/core/serializers/base.py#L223
{{{
def build_instance(Model, data, db):
"""
Build a model instance.
If the model instance doesn't have a primary key and the model
supports
natural keys, try to retrieve it from the database.
"""
obj = Model(**data)
if (obj.pk is None and hasattr(Model, 'natural_key') and
hasattr(Model._default_manager, 'get_by_natural_key')):
natural_key = obj.natural_key()
try:
obj.pk =
Model._default_manager.db_manager(db).get_by_natural_key(*natural_key).pk
except Model.DoesNotExist:
pass
return obj
}}}
this causes loaddata to fail on second attempt when using natural keys and
uuid primary keys as pk is set.
{{{
class FooManager(models.Manager):
def get_by_natural_key(self, name):
return self.get(name=name)
class Foo(DateTimeMixin, models.Model):
id = models.UUIDField(primary_key=True, default=uuid.uuid4,
editable=False)
name = models.CharField(max_length=100, unique=True)
objects = FooManager()
def natural_key(self):
return (self.name,)
}}}
checking the primary key was not actually set seems to fix things:
{{{
def build_instance(Model, data, db):
obj = Model(**data)
pk = data.get(Model._meta.pk.name, None)
if (pk is None and hasattr(Model, 'natural_key') and
hasattr(Model._default_manager, 'get_by_natural_key')):
natural_key = obj.natural_key()
try:
obj.pk =
Model._default_manager.db_manager(db).get_by_natural_key(*natural_key).pk
except Model.DoesNotExist:
pass
return obj
import django.core.serializers.base
django.core.serializers.base.build_instance = build_instance
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28385>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
--------------------------------------+------------------------------------
Reporter: Daniel Knell | Owner: nobody
Type: Bug | Status: new
Component: Core (Serialization) | Version: 1.11
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Tim Graham):
* stage: Unreviewed => Accepted
Old description:
> https://github.com/django/django/blob/master/django/core/serializers/base.py#L223
>
New description:
{{{
def build_instance(Model, data, db):
"""
Build a model instance.
If the model instance doesn't have a primary key and the model
supports
natural keys, try to retrieve it from the database.
"""
obj = Model(**data)
if (obj.pk is None and hasattr(Model, 'natural_key') and
hasattr(Model._default_manager, 'get_by_natural_key')):
natural_key = obj.natural_key()
try:
obj.pk =
Model._default_manager.db_manager(db).get_by_natural_key(*natural_key).pk
except Model.DoesNotExist:
pass
return obj
}}}
([https://github.com/django/django/blob/169c3b3e07829d9ffa409b6eb5c1094d8ef918a8/django/core/serializers/base.py#L223
source on GitHub])
objects = FooManager()
def natural_key(self):
return (self.name,)
}}}
import django.core.serializers.base
django.core.serializers.base.build_instance = build_instance
}}}
--
--
Ticket URL: <https://code.djangoproject.com/ticket/28385#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: Daniel Knell | Owner: Irindu
| Indeera
Type: Bug | Status: assigned
Component: Core | Version: 1.11
(Serialization) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Irindu Indeera ):
* owner: nobody => Irindu Indeera
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/28385#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: Daniel Knell | Owner: Irindu
| Indeera
Type: Bug | Status: assigned
Component: Core | Version: 1.11
(Serialization) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by Dmytryi Striletskyi):
Hello,
I cannot reproduce this bug, i tried few ways, but got no error during
`loaddata`.
May my understanding is not clear, but how do we want to get `natural_key`
if `Foo` model's `primary key` exists?
From `build_instance` docstring:
> If the model instance doesn't have a primary key and the model supports
natural keys, try to retrieve it from the database.
Clarify bug's space for me, please. I wanna resolve it. Thanks.
--
Ticket URL: <https://code.djangoproject.com/ticket/28385#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: Daniel Knell | Owner: Irindu
| Indeera
Type: Bug | Status: assigned
Component: Core | Version: 1.11
(Serialization) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by Daniel Knell):
When a models primary key has default set, and a fixture is loaded that
does not have a pk attribute, then the serialisation code will refuse to
update the existing instance and instead try to create a new record in the
db.
When the primary key attribute has a default value - such as models using
UUID primary keys - the model will fill the pk attribute with that at
instantiation time, and the check in build_instance will see that default
value and refuse to goto the database to fetch the existing instance.
This is against the expectations set in the docs, that when the
model/manager implement the natural keys interfaces and the primary key is
omitted, the framework will update the existing instance when
deserialising.
--
Ticket URL: <https://code.djangoproject.com/ticket/28385#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Daniel Knell | Owner: Irindu
| Indeera
Type: Bug | Status: assigned
Component: Core | Version: 1.11
(Serialization) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by Dmytryi Striletskyi):
{{{
def build_instance(Model, data, db):
obj = Model(**data)
if obj.pk is None or (hasattr(Model, 'natural_key') and
hasattr(Model._default_manager,
'get_by_natural_key')):
natural_key = obj.natural_key()
try:
obj.pk =
Model._default_manager.db_manager(db).get_by_natural_key(*natural_key).pk
except Model.DoesNotExist:
pass
return obj
}}}
I can provide this solution:
> if obj.pk is None or (hasattr(Model, 'natural_key') and
hasattr(Model._default_manager, 'get_by_natural_key')):
If `pk` is set - it is update model via `return obj` without `updating
pk` logic.
If `pk` isn't set, but `natural key` implementation exists - `if False or
True` (return `True`) - go to `updating pk` logic.
I did test manually, it seems to fix bug.
Am I on right way, Daniel Knell?
--
Ticket URL: <https://code.djangoproject.com/ticket/28385#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Daniel Knell | Owner: Irindu
| Indeera
Type: Bug | Status: assigned
Component: Core | Version: 1.11
(Serialization) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by Irindu Indeera ):
Can you please list the correct steps to reproduce this bug ?
--
Ticket URL: <https://code.djangoproject.com/ticket/28385#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Daniel Knell | Owner: Irindu
| Indeera
Type: Bug | Status: assigned
Component: Core | Version: 1.11
(Serialization) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by Daniel Knell):
define the above models then try and import the following fixture using
loaddata.
{{{
- model: myapp.Foo
fields:
name: foobar
}}}
if you try and import a second time it will create a duplicate instance
instead of using the existing one.
--
Ticket URL: <https://code.djangoproject.com/ticket/28385#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Daniel Knell | Owner: Irindu
| Indeera
Type: Bug | Status: assigned
Component: Core | Version: 1.11
(Serialization) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Dmytryi Striletskyi):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/28385#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Daniel Knell | Owner: Irindu
| Indeera
Type: Bug | Status: assigned
Component: Core | Version: 1.11
(Serialization) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 0
Changes (by Markus Holtermann):
* needs_better_patch: 0 => 1
Comment:
PR: https://github.com/django/django/pull/8801
--
Ticket URL: <https://code.djangoproject.com/ticket/28385#comment:9>
Django
-------------------------------------+-------------------------------------
| Ramezani
Type: Bug | Status: assigned
Component: Core | Version: 1.11
(Serialization) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Hasan Ramezani):
* owner: Irindu Indeera => Hasan Ramezani
* needs_better_patch: 1 => 0
Comment:
[https://github.com/django/django/pull/10681 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/28385#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Daniel Knell | Owner: Hasan
| Ramezani
Component: Core | Version: 1.11
(Serialization) |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"dcd1025f4c03faa4a9915a1d47d07008280dd3cf" dcd1025]:
{{{
#!CommitTicketReference repository=""
revision="dcd1025f4c03faa4a9915a1d47d07008280dd3cf"
Fixed #28385 -- Fixed deserializing natural keys when primary key has a
default value.
Co-Authored-By: Hasan Ramezani <hasa...@gmail.com>
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28385#comment:11>