[Django] #36888: acreate method doesn't call asave

[Django]

#36888: acreate method doesn't call asave
-------------------------------------+-------------------------------------
Reporter: Materson | Owner: @…
Type: Bug | Status: assigned
Component: Database | Version: 6.0
layer (models, ORM) |
Severity: Normal | Keywords: acreate asave
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 1
UI/UX: 0 |
-------------------------------------+-------------------------------------
acreate doesn't call asave method.

When you use async Django methods and you want to add custom logic in
asave method, when you create your object with acreate, it doesn't call
your asave method.

{{{
class SimpleModel(models.Model):
field = models.IntegerField()

async def asave(self, *args, **kwargs):
self.field += 1
await super().asave(*args, **kwargs)

obj = await SimpleModel.objects.acreate(field=4)
obj.field # returns 4, should be 5
}}}
When you run create, it calls save().
--
Ticket URL: <https://code.djangoproject.com/ticket/36888>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36888: acreate method doesn't call asave
-------------------------------------+-------------------------------------

Reporter: Mateusz Szymanowski | Owner: @…
Type: Bug | Status: assigned
Component: Database layer | Version: 6.0
(models, ORM) |
Severity: Normal | Resolution:

Keywords: acreate asave | Triage Stage:

| Unreviewed
Has patch: 1 | Needs documentation: 0

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

Changes (by Mateusz Szymanowski):

* has_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/36888#comment:1>

[Django]

#36888: acreate method doesn't call asave
-------------------------------------+-------------------------------------
Reporter: Mateusz Szymanowski | Owner: @…
Type: Bug | Status: assigned
Component: Database layer | Version: 6.0
(models, ORM) |
Severity: Normal | Resolution:

Keywords: acreate asave async | Triage Stage: Accepted

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

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Natalia Bidart):

* easy: 1 => 0
* keywords: acreate asave => acreate asave async
* stage: Unreviewed => Accepted

Comment:

Thank you Mateusz for your report! Great catch.
--
Ticket URL: <https://code.djangoproject.com/ticket/36888#comment:2>

[Django]

#36888: acreate method doesn't call asave
-------------------------------------+-------------------------------------
Reporter: Mateusz Szymanowski | Owner: @…
Type: Bug | Status: assigned
Component: Database layer | Version: 6.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: acreate asave async | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Mariusz Felisiak):

* needs_better_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/36888#comment:3>

[Django]

#36888: acreate method doesn't call asave
-------------------------------------+-------------------------------------

Reporter: Mateusz Szymanowski | Owner: Mateusz
| Szymanowski

Type: Bug | Status: assigned
Component: Database layer | Version: 6.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: acreate asave async | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Mariusz Felisiak):

* owner: @… => Mateusz Szymanowski

--
Ticket URL: <https://code.djangoproject.com/ticket/36888#comment:4>

[Django]

#36888: acreate method doesn't call asave
-------------------------------------+-------------------------------------

Reporter: Mateusz Szymanowski | Owner: Jericho
| Serrano

Type: Bug | Status: assigned
Component: Database layer | Version: 6.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: acreate asave async | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jericho Serrano):

* owner: Mateusz Szymanowski => Jericho Serrano

--
Ticket URL: <https://code.djangoproject.com/ticket/36888#comment:5>

[Django]

#36888: acreate method doesn't call asave
-------------------------------------+-------------------------------------
Reporter: Mateusz Szymanowski | Owner: Jericho
| Serrano
Type: Bug | Status: assigned
Component: Database layer | Version: 6.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: acreate asave async | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 1 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Jericho Serrano):

* needs_better_patch: 1 => 0
* needs_tests: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/36888#comment:6>

[Django]

#36888: acreate method doesn't call asave
-------------------------------------+-------------------------------------
Reporter: Mateusz Szymanowski | Owner: Jericho
| Serrano
Type: Bug | Status: assigned
Component: Database layer | Version: 6.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: acreate asave async | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Jericho Serrano):

I created a pr [https://github.com/django/django/pull/20896] here
--
Ticket URL: <https://code.djangoproject.com/ticket/36888#comment:7>

[Django]

#36888: acreate method doesn't call asave
-------------------------------------+-------------------------------------

Reporter: Mateusz Szymanowski | Owner: Mateusz
| Szymanowski

Type: Bug | Status: assigned
Component: Database layer | Version: 6.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: acreate asave async | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Natalia Bidart):

* needs_tests: 1 => 0
* owner: Jericho Serrano => Mateusz Szymanowski

Comment:

Restoring the previous owner since the PR is needing a re-review, but
owner did not update the ticket flags I think.
--
Ticket URL: <https://code.djangoproject.com/ticket/36888#comment:8>

[Django]

#36888: acreate method doesn't call asave
-------------------------------------+-------------------------------------
Reporter: Mateusz Szymanowski | Owner: Mateusz
| Szymanowski
Type: Bug | Status: assigned
Component: Database layer | Version: 6.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: acreate asave async | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Jericho Serrano):

Hello Natalia, my bad; I thought this ticket was inactive. I lately
realized the owner's PR is still ongoing, and there was no activity here
in the track ticket.
--
Ticket URL: <https://code.djangoproject.com/ticket/36888#comment:9>

[Django]

#36888: acreate method doesn't call asave
-------------------------------------+-------------------------------------
Reporter: Mateusz Szymanowski | Owner: Mateusz
| Szymanowski
Type: Bug | Status: assigned
Component: Database layer | Version: 6.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: acreate asave async | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jacob Walls):

* needs_better_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/36888#comment:10>

[Django]

#36888: acreate method doesn't call asave
-------------------------------------+-------------------------------------
Reporter: Mateusz Szymanowski | Owner: Mateusz
| Szymanowski
Type: Bug | Status: assigned
Component: Database layer | Version: 6.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: acreate asave async | Triage Stage:

| Someday/Maybe

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Jacob Walls):

* stage: Accepted => Someday/Maybe

Comment:

Before, `acreate()` had a `sync_to_async()` right off the bat:

{{{#!py
async def acreate(self, **kwargs):
return await sync_to_async(self.create)(**kwargs)
}}}

Attempts in the PRs to remove that `sync_to_async()` stalled on the fact
that `GenericForeignKeyDescriptor.__set__` accesses the database (see
`generic_relations.tests.GenericRelationsTests.test_aadd`).

If we're just shuffling around *where* the `sync_to_async` lies, then
we're not accomplishing very much, as pointed out in the reviews. I
[https://github.com/django/django/pull/20602#pullrequestreview-4391796527
posted] a sketch of an async interface for Model instantiation.

I think we need both that ''as well as'' an async implementation of
`ContentType` methods (`aget_content_type()` etc.) before we can advance
this.

If someone can confirm the general direction, we can open separate tickets
for those.
--
Ticket URL: <https://code.djangoproject.com/ticket/36888#comment:11>

[Django]

#36888: acreate method doesn't call asave
-------------------------------------+-------------------------------------
Reporter: Mateusz Szymanowski | Owner: Mateusz
| Szymanowski
Type: Bug | Status: assigned
Component: Database layer | Version: 6.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: acreate asave async | Triage Stage:
| Someday/Maybe
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Carlton Gibson):

I worry slightly about habits or patterns we might be encouraging with
this change (beyond the issue GFK's).

Take this model (from the description):

{{{
class SimpleModel(models.Model):
field = models.IntegerField()

async def asave(self, *args, **kwargs):
self.field += 1
await super().asave(*args, **kwargs)
}}}

Here `self.field += 1` is what we might call ''business logic''. But
presumably we also are going to have the sync API for our model, and so
would need to duplicate this logic in the sync `save` method. But then
we're introducing a double entry bookkeeping requirement, which is
guaranteed to mean folks forget to update one or the other, and introduce
bugs into their application.

As it stands now, the ORM's async methods proxy directly/immediately to
their sync counterparts, which means that the one correct place for the
business logic is in `save`. Diluting that guarantee strikes me as a
concern.
--
Ticket URL: <https://code.djangoproject.com/ticket/36888#comment:12>

[Django]

#36888: acreate method doesn't call asave
-------------------------------------+-------------------------------------
Reporter: Mateusz Szymanowski | Owner: Mateusz
| Szymanowski
Type: Bug | Status: assigned
Component: Database layer | Version: 6.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: acreate asave async | Triage Stage:
| Someday/Maybe
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Jacob Walls):

Yes, Mykhailo also made that point in
[https://github.com/django/django/pull/20602#issuecomment-4379334329 a
review].
--
Ticket URL: <https://code.djangoproject.com/ticket/36888#comment:13>

[Django]

#36888: acreate method doesn't call asave
-------------------------------------+-------------------------------------
Reporter: Mateusz Szymanowski | Owner: Mateusz
| Szymanowski

Type: Bug | Status: closed

Component: Database layer | Version: 6.0
(models, ORM) |

Severity: Normal | Resolution: wontfix

Keywords: acreate asave async | Triage Stage:

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

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jacob Walls):

* has_patch: 1 => 0

* needs_better_patch: 1 => 0

* resolution: => wontfix
* stage: Someday/Maybe => Unreviewed
* status: assigned => closed

Comment:

In the cache framework, async methods delegate to other async methods if
available, but given that folks mix business logic into `save()`, that
makes less sense for models. Reimplementing `acreate()` without a plan for
doing the same in `get_or_create()` or `update_or_create()` doesn't seem
prudent.

In my opinion placing business logic in `save()` has been a footgun since
Django 4.2, but that might be a minority opinion. I
[https://forum.djangoproject.com/t/hook-for-extending-update-fields-in-
save-overrides/26040 didn't get anyone interested] in shipping a util to
express the
[https://docs.djangoproject.com/en/dev/topics/db/models/#:~:text=if%20(%0A%20%20%20%20%20%20%20%20%20%20%20%20update_fields%20%3A%3D%20kwargs.get(%22update_fields%22)%0A%20%20%20%20%20%20%20%20)%20is%20not%20None%20and%20%22name%22%20in%20update_fields%3A%0A%20%20%20%20%20%20%20%20%20%20%20%20kwargs%5B%22update_fields%22%5D%20%3D%20%7B%22slug%22%7D.union(update_fields)
convoluted handling] of `update_fields` that is necessary now.

Quoting Simon from that forum thread:
> I’m not convinced that a core function is warranted here. I think it
might be better to invest efforts in coming up with a generic solution to
the problem of derived / generated fields and their interactions with
update_fields.

Maybe if we had that derived state feature in hand, and people had a nice
path to moving their business logic out of `save()`, we could take a fresh
look. Until then I don't think we can really advance this.
--
Ticket URL: <https://code.djangoproject.com/ticket/36888#comment:14>