acreate method doesn't call asave

[Mateusz Szymanowski]

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().

[Natalia Bidart]

Thank you Mateusz for your report! Great catch.

[Jericho Serrano]

I created a pr ​https://github.com/django/django/pull/20896 here

[Natalia Bidart]

Restoring the previous owner since the PR is needing a re-review, but owner did not update the ticket flags I think.

[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.

[Jacob Walls]

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

     async def acreate(self, **kwargs):
        return await sync_to_async(self.create)(**kwargs)

Attempts in the PRs to delegate to asave() and push that sync_to_async() boundary into asave() stalled on the fact that GenericForeignKeyDescriptor.__set__ accesses the database (see generic_relations.tests.GenericRelationsTests.test_aadd). The PRs added another call to sync_to_async, which isn't pointing in the right direction, as pointed out in the reviews. (edits: precision)

I ​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.

[Carlton Gibson]

I worry slightly about habits or patterns we might be encouraging with this change (beyond the issue with 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.

[Jacob Walls]

Yes, Mykhailo also made that point in ​a review.

[Jacob Walls]

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 ​didn't get anyone interested in shipping a util to express the ​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.