Re: [Django] #36550: 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

[Django]

#36550: 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
-------------------------------------+-------------------------------------
Reporter: Will-Ruddick | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/36550#comment:8>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36550: 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
-------------------------------------+-------------------------------------
Reporter: Will-Ruddick | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by 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?
--
Ticket URL: <https://code.djangoproject.com/ticket/36550#comment:9>

[Django]

#36550: 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
-------------------------------------+-------------------------------------
Reporter: Will-Ruddick | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce):

* cc: Simon Charette (added)

Comment:

> 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
--
Ticket URL: <https://code.djangoproject.com/ticket/36550#comment:10>

[Django]

#36550: 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
-------------------------------------+-------------------------------------

Reporter: Will-Ruddick | Owner: Jason
| Hall
Type: Bug | Status: assigned

Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jason Hall):

* owner: (none) => Jason Hall
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/36550#comment:11>

[Django]

#36550: 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
-------------------------------------+-------------------------------------
Reporter: Will-Ruddick | Owner: Jason
| Hall
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Jason Hall):

* Attachment "ticket_36550_bulk_create_composite_auto_now_add.patch"
added.

--
Ticket URL: <https://code.djangoproject.com/ticket/36550>

[Django]

#36550: 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
-------------------------------------+-------------------------------------
Reporter: Will-Ruddick | Owner: Jason
| Hall
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Jason Hall):

* Attachment "36550.patch" removed.

[Django]

#36550: 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
-------------------------------------+-------------------------------------
Reporter: Will-Ruddick | Owner: Jason
| Hall
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/36550#comment:12>

[Django]

#36550: 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
-------------------------------------+-------------------------------------
Reporter: Will-Ruddick | Owner: Jason
| Hall
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jason Hall):

* has_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/36550#comment:13>

[Django]

#36550: 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
-------------------------------------+-------------------------------------

Reporter: Will-Ruddick | Owner: (none)
Type: Bug | Status: new

Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Jason Hall):

* owner: Jason Hall => (none)
* status: assigned => new

--
Ticket URL: <https://code.djangoproject.com/ticket/36550#comment:14>

[Django]

#36550: 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
-------------------------------------+-------------------------------------
Reporter: Will-Ruddick | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce):

* has_patch: 1 => 0

--
Ticket URL: <https://code.djangoproject.com/ticket/36550#comment:15>

[Django]

#36550: 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
-------------------------------------+-------------------------------------
Reporter: Will-Ruddick | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Sarah Boyce):

* has_patch: 0 => 1
* needs_better_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/36550#comment:16>

[Django]

#36550: 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
-------------------------------------+-------------------------------------
Reporter: Will-Ruddick | Owner:

| JaeHyuckSa
Type: Bug | Status: assigned

Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by JaeHyuckSa):

* owner: (none) => JaeHyuckSa

* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/36550#comment:17>

[Django]

#36550: 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
-------------------------------------+-------------------------------------
Reporter: Will-Ruddick | Owner:
| JaeHyuckSa
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Abhimanyu Singh Negi):

Replying to [comment:16 Sarah Boyce]:
Hi Sarah, I see the last patch was more thatn 5 months earlier, and the
latest owner hasnt made any progress on it, so i would like to take up the
issue.

I plan to update the bulk_create preparation step so that primary key
values which can be determined locally are populated before objects are
partitioned, and so that classification checks whether all primary key
fields are actually filled rather than relying only on the existing check.
I’ll also extend or adjust the tests to ensure that bulk_create()
correctly handles primary keys set via pre_save(), both in memory and in
the database, and that the behavior remains consistent across supported
backends.
**
if this sounds okay assign me the issue or give permission to assign
myself.**
--
Ticket URL: <https://code.djangoproject.com/ticket/36550#comment:18>