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

[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)

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

[trybik]

PR with a fix: ​https://github.com/django/django/pull/13975

[Simon Charette]

Thanks for the detailed report and the PR.

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

[Mariusz Felisiak <felisiak.mariusz@…>]

In 8e90560a:

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

[Mariusz Felisiak <felisiak.mariusz@…>]

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