GeneratedField(primary_key=True) crashes on create(), and other issues

[David Sanders]

Related to: #36466

Given the following model:

class Foo(Model):
    foo = IntegerField()
    bar = GeneratedField(
        expression=F("foo") + 5,
        output_field=IntegerField(),
        db_persist=True,
        primary_key=True,
    )

Django will crash on the following:

create()

>>> Foo.objects.create(foo=100)
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "/path/to/django/db/models/manager.py", line 87, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/path/to/django/db/models/query.py", line 665, in create
    obj.save(force_insert=True, using=self.db)
  File "/path/to/django/db/models/base.py", line 873, in save
    self.save_base(
  File "/path/to/django/db/models/base.py", line 965, in save_base
    updated = self._save_table(
              ^^^^^^^^^^^^^^^^^
  File "/path/to/django/db/models/base.py", line 1066, in _save_table
    if not self._is_pk_set(meta):
           ^^^^^^^^^^^^^^^^^^^^^
  File "/path/to/django/db/models/base.py", line 688, in _is_pk_set
    pk_val = self._get_pk_val(meta)
             ^^^^^^^^^^^^^^^^^^^^^^
  File "/path/to/django/db/models/base.py", line 677, in _get_pk_val
    return getattr(self, meta.pk.attname)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/path/to/django/db/models/query_utils.py", line 221, in __get__
    val = self._check_parent_chain(instance)
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/path/to/django/db/models/query_utils.py", line 242, in _check_parent_chain
    return getattr(instance, link_field.attname)
                             ^^^^^^^^^^^^^^^^^^
AttributeError: 'NoneType' object has no attribute 'attname'

if we patch DeferredAttribute to account for None with

--- a/django/db/models/query_utils.py
+++ b/django/db/models/query_utils.py
@@ -238,7 +238,7 @@ class DeferredAttribute:
         """
         opts = instance._meta
         link_field = opts.get_ancestor_link(self.field.model)
-        if self.field.primary_key and self.field != link_field:
+        if link_field and self.field.primary_key and self.field != link_field:
             return getattr(instance, link_field.attname)
         return None

then we get infinite recursion:

>>> Foo.objects.create(foo=100)
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "/path/to/django/db/models/manager.py", line 87, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/path/to/django/db/models/query.py", line 665, in create
    obj.save(force_insert=True, using=self.db)
  File "/path/to/django/db/models/base.py", line 873, in save
    self.save_base(
  File "/path/to/django/db/models/base.py", line 965, in save_base
    updated = self._save_table(
              ^^^^^^^^^^^^^^^^^
  File "/path/to/django/db/models/base.py", line 1066, in _save_table
    if not self._is_pk_set(meta):
           ^^^^^^^^^^^^^^^^^^^^^
  File "/path/to/django/db/models/base.py", line 688, in _is_pk_set
    pk_val = self._get_pk_val(meta)
             ^^^^^^^^^^^^^^^^^^^^^^
  File "/path/to/django/db/models/base.py", line 677, in _get_pk_val
    return getattr(self, meta.pk.attname)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/path/to/django/db/models/query_utils.py", line 223, in __get__
    if not instance._is_pk_set():
           ^^^^^^^^^^^^^^^^^^^^^

    >>>> etc etc etc <<<

RecursionError: maximum recursion depth exceeded

Patching DeferredAttribute seems to address the infinite recursion and allow creation (but this will block the update of the generated field's value):

--- a/django/db/models/query_utils.py
+++ b/django/db/models/query_utils.py
@@ -219,7 +219,7 @@ class DeferredAttribute:
             # Let's see if the field is part of the parent chain. If so we
             # might be able to reuse the already loaded value. Refs #18343.
             val = self._check_parent_chain(instance)
-            if val is None:
+            if val is None and not self.field.generated:
                 if not instance._is_pk_set():
                     raise AttributeError(
                         f"Cannot retrieve deferred field {field_name!r} "

Then there are issues around fetching, eg consider the following:

>>> foo = Foo.objects.get(...)
>>> foo.foo = <some other value>
>>> foo.save()
>>> foo.refresh_from_db() 
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "/path/to/django/db/models/base.py", line 757, in refresh_from_db
    db_instance = db_instance_qs.get()
                  ^^^^^^^^^^^^^^^^^^^^
  File "/path/to/django/db/models/query.py", line 635, in get
    raise self.model.DoesNotExist(
generated_as_pk.models.Foo.DoesNotExist: Foo matching query does not exist.

These are just some basic tests, there may be other issues not yet noticed.

I don't think it's as bad as banning primary_key=True though. Simon has a branch for #27222 which seems to fix the refresh_from_db() issue. Perhaps the infinite recursion can be solved simply/elegantly as well?

[David Sanders]

Also Sarah first noticed this same recursion when testing with CompositePrimaryKey: ​https://github.com/django/django/pull/18056#discussion_r1673832270

[Simon Charette]

I haven't played with it a lot so take this with a grain of salt but I think the crux of the issue here is that we mark generated field as deferred on instance that are meant for adding.

We don't do that for any other fields even the one that are db generated (think of Field.db_default usage) and it makes little sense knowing there's nothing to fetch until persistence is performed.

If we're speaking code here I meant that for a model of the form

from django.db import models

class Foo(models.Model):
    field = models.IntegerField()
    field_db_default = models.IntegerField(
        db_default=models.Value(1)
    )
    field_generated = models.GeneratedField(
        expression=models.F("field"),
        db_persist=True,
        output_field=models.IntegerField()
    )

def run():
    instance = Foo()
    print(repr(instance.field))
    print(repr(instance.field_db_default))
    try:
        print(repr(instance.field_generated))
    except Exception as exc:
        print(f"Failed to access instance.field_generated: {exc}")

​You'd get the following output

None
<django.db.models.expressions.DatabaseDefault object at 0x7f9ba68a27b0>
Failed to access instance.field_generated: Cannot read a generated field from an unsaved model.

As David pointed out on the forum this has a bit of overlap with #27222.

[David Sanders]

Note that MariaDB does not support generated columns as PK: ​https://mariadb.com/docs/server/reference/sql-statements/data-definition/create/generated-columns#index-support

[David Sanders]

Draft PR with some ideas: ​https://github.com/django/django/pull/19580

[David Sanders]

Simon/others: I think the PR should be ready to take a look.