[Django] #32420: build_instance in (de)serializers uses model's primary key field name instead of attname to detect if data contains a primary key
Django
instead of attname to detect if data contains a primary key
------------------------------------------------+--------------------------
Reporter: trybik | Owner: trybik
Type: Bug | Status: assigned
Component: Core (Serialization) | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------------+--------------------------
The bug was introduced in #28385 fix, in
[https://github.com/django/django/commit/dcd1025f4c03faa4a9915a1d47d07008280dd3cf
commit dcd1025]. Currently in master, the
{{{django.core.serializers.base.build_instance}}} function uses {{{pk =
data.get(Model._meta.pk.name)}}} instead of {{{pk =
data.get(Model._meta.pk.attname)}}}:
{{{
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.
"""
default_manager = Model._meta.default_manager
pk = data.get(Model._meta.pk.name)
if (pk is None and hasattr(default_manager, 'get_by_natural_key') and
hasattr(Model, 'natural_key')):
natural_key = Model(**data).natural_key()
try:
data[Model._meta.pk.attname] = Model._meta.pk.to_python(
default_manager.db_manager(db).get_by_natural_key(*natural_key).pk
)
except Model.DoesNotExist:
pass
return Model(**data)
}}}
([https://github.com/django/django/blob/738e9e615dc81b561c9fb01439119f4475c2e25b/django/core/serializers/base.py#L260
Source on GitHub])
Why it should use {{{attname}}}? Because it may be different than
{{{name}}} and the following corresponding sets use
{{{data[Model._meta.pk.attname] = ...}}}:
1. proceeding {{{django.core.serializers.python.Deserializer}}} which
calls {{{build_instance}}}:
{{{
def Deserializer(...):
...
for d in object_list:
...
data = {}
if 'pk' in d:
try:
data[Model._meta.pk.attname] =
Model._meta.pk.to_python(d.get('pk'))
except Exception as e:
...
obj = base.build_instance(Model, data, using)
yield ...
}}}
([https://github.com/django/django/blob/738e9e615dc81b561c9fb01439119f4475c2e25b/django/core/serializers/python.py#L100
Source on GitHub])
2. following 5 lines further assignment {{{ data[Model._meta.pk.attname] =
...}}} uses {{{attname}}}.
The marginal error case is when primary key is also a foreign key (then
{{{attname}}} has {{{"_id"}}} suffix). Moreover, to actually make it an
error you have to create a bit pathological situation where you have to
have natural key but it does not work, e.g.:
{{{
class Author(models.Model):
name = models.CharField(max_length=20)
class GhostWriterManager(models.Manager):
def get_by_natural_key(self, *args, **kwargs):
raise NotImplementedError("Don't get by natural key")
class GhostWriter(models.Model):
author_ptr = models.ForeignKey(Author, on_delete=models.CASCADE,
primary_key=True)
objects = GhostWriterManager()
def natural_key(self):
raise NotImplementedError("There is no natural key")
}}}
This rare situation actually can come up in an arguably normal situation,
when using django-polymorphic and loading the subclass part of JSON list
and the subclass uses a natural key that refers to fields from base class.
The natural key will work perfectly fine, just not when loading the
subclass part of JSON.
--
Ticket URL: <https://code.djangoproject.com/ticket/32420>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
instead of attname to detect if data contains a primary key
Reporter: trybik | Owner: trybik
Type: Bug | Status: assigned
(Serialization) |
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
Comment (by trybik):
PR with a fix: https://github.com/django/django/pull/13975
--
Ticket URL: <https://code.djangoproject.com/ticket/32420#comment:1>
Django
instead of attname to detect if data contains a primary key
Reporter: trybik | Owner: trybik
Type: Bug | Status: assigned
(Serialization) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Description changed by trybik:
Old description:
New description:
{{{name}}} and the following corresponding assignments use
class GhostWriterManager(models.Manager):
objects = GhostWriterManager()
--
--
Ticket URL: <https://code.djangoproject.com/ticket/32420#comment:2>
Django
instead of attname to detect if data contains a primary key
Reporter: trybik | Owner: trybik
Type: Bug | Status: assigned
Component: Core (Serialization) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* has_patch: 0 => 1
* stage: Unreviewed => Accepted
Comment:
Thanks for the detailed report and the PR.
https://github.com/django/django/pull/13975
--
Ticket URL: <https://code.djangoproject.com/ticket/32420#comment:3>
Django
instead of attname to detect if data contains a primary key
Reporter: trybik | Owner: trybik
Type: Bug | Status: assigned
Component: Core (Serialization) | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Description changed by trybik:
Old description:
> The bug was introduced in #28385 fix, in
> {{{name}}} and the following corresponding assignments use
New description:
{{{name}}} and the following corresponding assignments use
class GhostWriterManager(models.Manager):
objects = GhostWriterManager()
when using ~~django-polymorphic~~ non-abstract subclasses - when loading
the subclass part of JSON list, and when the subclass uses a natural key
that refers to fields from base class. The natural key will work perfectly
fine, just not when loading the subclass part of JSON.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/32420#comment:4>
Django
instead of attname to detect if data contains a primary key
Reporter: trybik | Owner: trybik
Type: Bug | Status: closed
Component: Core (Serialization) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"8e90560aa8868a42bb8eda6273595bf0932a6090" 8e90560a]:
{{{
#!CommitTicketReference repository=""
revision="8e90560aa8868a42bb8eda6273595bf0932a6090"
Fixed #32420 -- Fixed detecting primary key values in deserialization when
PK is also a FK.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32420#comment:5>
Django
instead of attname to detect if data contains a primary key
Reporter: trybik | Owner: trybik
Type: Bug | Status: closed
Component: Core (Serialization) | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"d881a0ea3b733a64cfb5fe19561bb01479669ed6" d881a0ea]:
{{{
#!CommitTicketReference repository=""
revision="d881a0ea3b733a64cfb5fe19561bb01479669ed6"
[3.2.x] Fixed #32420 -- Fixed detecting primary key values in
deserialization when PK is also a FK.
Backport of 8e90560aa8868a42bb8eda6273595bf0932a6090 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32420#comment:6>