[Django] #23386: Unexpected result when incrimenting field with F expression
Django
-------------------------------+--------------------
Reporter: codefisher | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.6
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23386>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: codefisher | Owner: nobody
Type: Uncategorized | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by codefisher):
* needs_better_patch: => 0
* component: Uncategorized => Database layer (models, ORM)
* needs_tests: => 0
* needs_docs: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/23386#comment:1>
Django
--------------------------------------+------------------------------------
Reporter: codefisher | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: 1.6
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* type: Uncategorized => Cleanup/optimization
* component: Database layer (models, ORM) => Documentation
* stage: Unreviewed => Accepted
Comment:
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()`.
--
Ticket URL: <https://code.djangoproject.com/ticket/23386#comment:2>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: 1.6
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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:
https://docs.djangoproject.com/en/dev/ref/models/queries/#f-expressions
But the effect of calling save more then once is no where to be seen, and
really not that expected.
--
Ticket URL: <https://code.djangoproject.com/ticket/23386#comment:3>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: 1.6
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23386#comment:4>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: 1.6
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by jarshwah):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23386#comment:5>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: 1.6
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"fc4b4fd5850989458d6e54de12a29b2e40e94ce8" fc4b4fd]:
{{{
#!CommitTicketReference repository=""
revision="fc4b4fd5850989458d6e54de12a29b2e40e94ce8"
Refs #23386 -- Documented that F() expressions are applied on each
model.save()
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23386#comment:6>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: 1.6
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"be03ce25c4bdf3fc5b413e6ca83edb4661cf608d" be03ce2]:
{{{
#!CommitTicketReference repository=""
revision="be03ce25c4bdf3fc5b413e6ca83edb4661cf608d"
[1.10.x] Refs #23386 -- Documented that F() expressions are applied on
each model.save()
Backport of fc4b4fd5850989458d6e54de12a29b2e40e94ce8 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23386#comment:7>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: 1.6
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"67c60cce7073762106bca186669af0fe1f588d15" 67c60cce]:
{{{
#!CommitTicketReference repository=""
revision="67c60cce7073762106bca186669af0fe1f588d15"
[1.9.x] Refs #23386 -- Documented that F() expressions are applied on each
model.save()
Backport of fc4b4fd5850989458d6e54de12a29b2e40e94ce8 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23386#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: codefisher | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 1.6
(models, ORM) |
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* component: Documentation => Database layer (models, ORM)
Comment:
Reclassifying the ticket to evaluate changing the current behavior.
--
Ticket URL: <https://code.djangoproject.com/ticket/23386#comment:9>
Django
-------------------------------------+-------------------------------------
Type: | Status: closed
Cleanup/optimization |
Component: Database layer | Version: 1.6
(models, ORM) |
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* status: new => closed
* resolution: => duplicate
Comment:
This will be fixed by #27222.
--
Ticket URL: <https://code.djangoproject.com/ticket/23386#comment:10>