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.

Looking forward to reading what you all think of this. Also, this test was failing before the code updates, and is now passing. I can prepare a PR for this, but wanted to wait for some expert input first.