Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#32420 closed Bug (fixed)

build_instance in (de)serializers uses model's primary key field name instead of attname to detect if data contains a primary key

Reported by: trybik Owned by: trybik
Component: Core (Serialization) Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by trybik)

The bug was introduced in #28385 fix, in 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)

(Source on GitHub)

Why it should use attname? Because it may be different than name and the following corresponding assignments 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 ...
    

(Source on GitHub)

  1. 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 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.

Change History (6)

comment:2 by trybik, 4 years ago

Description: modified (diff)

comment:3 by Simon Charette, 4 years ago

Has patch: set
Triage Stage: UnreviewedAccepted

Thanks for the detailed report and the PR.

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

comment:4 by trybik, 4 years ago

Description: modified (diff)

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

Resolution: fixed
Status: assignedclosed

In 8e90560a:

Fixed #32420 -- Fixed detecting primary key values in deserialization when PK is also a FK.

comment:6 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

In d881a0ea:

[3.2.x] Fixed #32420 -- Fixed detecting primary key values in deserialization when PK is also a FK.

Backport of 8e90560aa8868a42bb8eda6273595bf0932a6090 from master

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