Opened 2 years ago

Last modified 3 months ago

#23386 new Cleanup/optimization

Clear F expressions after a model instance is saved

Reported by: codefisher Owned by: nobody
Component: Database layer (models, ORM) Version: 1.6
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I would have thought these two expresses would be exactly the same, except the second would be more efficient.

A: some_model.some_field += 1
B: some_model.some_field = F('some_field') + 1

I hit some unexpected behaviour when, I did this

some_model.save()
# and somewhere far far away
some_model.save()

With A, the field is incremented once, as I expected. But with B, it gets incremented twice. I would have expected the F query to effect the next database call, and nothing more. It might be intended, but then it is not documented.

I could write some come to demonstrate this. I only noticed it because I was using an import script that was creating lots of objects, and I had A and B in the model's overridden save method. They were causing vastly different results.

Attachments (1)

diff.txt (840 bytes) - added by codefisher 2 years ago.
Adding the needed note into the documentation

Download all attachments as: .zip

Change History (10)

comment:1 Changed 2 years ago by codefisher

Component: UncategorizedDatabase layer (models, ORM)
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

comment:2 Changed 2 years ago by Tim Graham

Component: Database layer (models, ORM)Documentation
Summary: Unexpected result when incrimenting field with F expressionDocument that F expressions are reapplied each time an object is saved
Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization

It doesn't seem intuitive and should definitely be documented. Probably a good idea to recommend fetching the object from the database after saving in order to get the new value if you are still using the object after save().

comment:3 Changed 2 years ago by codefisher

You say "Probably a good idea to recommend fetching the object from the database after saving in order to get the new value if you are still using the object after save()."

That is already done, at least on the latest dev docs (see the note): https://docs.djangoproject.com/en/dev/ref/models/queries/#f-expressions

But the effect of calling save more than once is no where to be seen, and really not that expected.

Last edited 2 years ago by codefisher (previous) (diff)

Changed 2 years ago by codefisher

Attachment: diff.txt added

Adding the needed note into the documentation

comment:4 Changed 2 years ago by codefisher

Just thinking about this, I am really more inclined to say how it works should be changed. I would suggest that after save() is called, it would be like the object had been called with defer() on that field. I don't think it would be hard to implement, I don't know the internals of the ORM, but defer() is already there, so what ever it does could be used again.

I can't see it causing backwards compatibility changes, this is undocumented, and not very intuitive. I would even suggest that after saving, that having the field affected by the F() expression not giving a sensible value is a bug in itself, and should be fixed - by treating as though marked by defer().

But there is always a change of causing backwards compatibility problems, that would be very hard to track down. Which needs to be weighed against how many problems the current counter intuitive behaviour would cause.

comment:5 Changed 2 years ago by Josh Smeaton

I would be inclined to call this a bug - so I don't think backwards compatibility would be much of an issue. It's totally unexpected (for me at least) that the F() "persists" across a model.save() call. When the model is saved, the attribute should be converted to the relevant field/descriptor, rather than maintaining the proxy of the F() expression. I have no idea how hard or easy this would be without looking into it though.

comment:6 Changed 3 months ago by Tim Graham <timograham@…>

In fc4b4fd:

Refs #23386 -- Documented that F() expressions are applied on each model.save()

comment:7 Changed 3 months ago by Tim Graham <timograham@…>

In be03ce2:

[1.10.x] Refs #23386 -- Documented that F() expressions are applied on each model.save()

Backport of fc4b4fd5850989458d6e54de12a29b2e40e94ce8 from master

comment:8 Changed 3 months ago by Tim Graham <timograham@…>

In 67c60cce:

[1.9.x] Refs #23386 -- Documented that F() expressions are applied on each model.save()

Backport of fc4b4fd5850989458d6e54de12a29b2e40e94ce8 from master

comment:9 Changed 3 months ago by Tim Graham

Component: DocumentationDatabase layer (models, ORM)
Summary: Document that F expressions are reapplied each time an object is savedClear F expressions after a model instance is saved

Reclassifying the ticket to evaluate changing the current behavior.

Note: See TracTickets for help on using tickets.
Back to Top