AssertionError raised when bulk creation model with primary key set via pre_save (e.g. auto_now_add) on databases with can_return_columns_from_insert

[Will-Ruddick]

I have a model that looks something like this

class CompositeModel(models.Model):
    pk = models.CompositePrimaryKey("other_model", "the_time", "created_at")

    other_model = models.ForeignKey("test.OtherModel", on_delete=models.CASCADE)
    the_time = models.DateTimeField()
    created_at = models.DateTimeField(auto_now_add=True)

And when I try and bulk create it in the code I get the following exception.

    CompositeModel.objects.bulk_create(new_items)
  File "django/db/models/manager.py", line 87, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "django/db/models/query.py", line 836, in bulk_create
    assert len(returned_columns) == len(objs_without_pk)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError

Where returned_columns is an empty list and objs_without_pk is a list of the objects I'm trying to create.

If I use ignore_conflicts=True then everything works fine and the objects are created properly in the DB. I think this is a bug, but it could be user error.

This is happening on version 5.2.5 with a postgres DB

[Sarah Boyce]

tests/composite_pk/test_create.py

@@ -1 +1 @@
 from django.db import IntegrityError
 from django.test import TestCase, skipUnlessDBFeature
 
-from .models import Post, Tenant, User
+from .models import Post, Tenant, User, TimeStamped
 
 
 class CompositePKCreateTests(TestCase):
@@ -78 +78 @@
         self.assertEqual(obj_3.pk, (obj_3.tenant_id, obj_3.id))
         self.assertEqual(obj_3.email, "user8214@example.com")
 
+    def test_bulk_create_auto_now_add_primary_key(self):
+        objs = [TimeStamped(id=1), TimeStamped(id=2), TimeStamped(id=3)]
+        self.assertEqual(len(TimeStamped.objects.bulk_create(objs)), 3)
+
     @skipUnlessDBFeature(
         "supports_update_conflicts",
         "supports_update_conflicts_with_target",

Thank you, replicated the error with the above test

[Jason Hall]

Attached 36550.patch. The existing regression test for bulk_create() with a composite PK containing an auto_now_add field now passes with this change. This seems to fix the issue, but I’m not sure if this is the preferred approach.

[Sarah Boyce]

Been looking into this more today

This actually doesn't have anything to do with CompositePrimaryKey and we can have the same error doing TimeStamped.objects.bulk_create([TimeStamped()]) when TimeStamped is:

class TimeStamped(models.Model):
    created = models.DateTimeField(auto_now_add=True, primary_key=True)

I believe the route of this is documented (​https://docs.djangoproject.com/en/5.2/ref/models/querysets/#bulk-create):

The model’s save() method will not be called, and the pre_save and post_save signals will not be sent.

And the value for created is defined on the field's pre_save method

To support bulk_create to support primary_keys that are set of pre_save would be a new feature. We _might_ be able to detect this better and have a nicer error message

But in short, I do think this is roughly expected behavior with some documentation and so will close

[Jacob Walls]

Wanted to mention that there's a difference between the model pre_save signal and the Field.pre_save() method. Looks like bulk_create() has always used the pre_save() value, so that part is not a new feature. It might be worth evaluating this a little further, as from the docs I don't see an admonition about this not working (happy to be corrected on this).

[Sarah Boyce]

Looked into it more and found this is only an issue for databases with can_return_columns_from_insert set to True (so is allowed on MySQL for example)

[Jacob Walls]

Thanks. Jason, given Sarah's investigation, it looks like inserting logic to set the value is not necessary, since this already works on some databases.

[Jason Hall]

Then would we want to modify the test logic and add @skipUnlessDBFeature("can_return_columns_from_insert")?

As far as a fix I was thinking we could ensure Field.pre_save() is applied before partitioning objects into with_pk and without_pk, adjust the classification to check completeness of all PK fields (including composite keys), and only request returning_fields when at least one object is missing a pk (skip requesting PKs when they're all known locally). Would that be acceptable?

[Sarah Boyce]

Then would we want to modify the test logic and add @skipUnlessDBFeature("can_return_columns_from_insert")?

Adding the skipUnlessDBFeature shouldn't be necessary as we expect it to work for all backends, regardless of whether can_return_columns_from_insert is True or False
I don't know what the fix should be but I recently reviewed #36490 and Simon had a few ideas around improving bulk_create, so I would be interested to hear his thoughts here

[Jason Hall]

Here is what I came up with this morning...

This patch updates QuerySet._prepare_for_bulk_create() to correctly classify objects when using composite primary keys and auto_now_add fields. Instead of relying only on _is_pk_set(), it checks that all PK fields are populated (non-None and not DatabaseDefault) before treating an object as "with PK".

Also, I modifed the original regression test from previous comments. This is now backend agnostic and it ensures that objects bulk-created with a composite PK and auto_now_add values still have their primary key set correctly both in memory and in the database.