[Django] #25296: Failed Manager.create pollutes related models cache
Django
-------------------------------+--------------------
Reporter: rubendura | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.8
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------
After updating to 1.8.4 one of my tests broke. Digging down I found in my
codebase something along the lines of
{{{
try:
Token.objects.get_or_create(user=user)
except ValueError:
# The user does not yet exist
pass
}}}
The user might or might not be saved before reaching that block of code.
In case the user hasn't been created in the database get_or_create would
raise ValueError and accessing user.token would raise an exception (or in
this case, sending that user into a django-rest-framework's serializer
would output None in the token field, which is our expected behaviour).
After 1.8.4 ValueError is still raised, but an usaved token is cached in
user._token_cache, so accessing user.token now returns an object I didn't
expect, thus returning an invalid value from the serializer.
I believe this has something to do with ticket #25160.
On my side it's an easy fix (check the user before calling
Token.objects.create), but I don't know if this should be flagged as a bug
and it caught me off guard, so I thought I would be a good idea reporting
it just in case that was an unexpected side effect.
--
Ticket URL: <https://code.djangoproject.com/ticket/25296>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: rubendura | Owner: nobody
Type: Uncategorized | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by rubendura):
* needs_docs: => 0
* component: Uncategorized => Database layer (models, ORM)
* needs_tests: => 0
* needs_better_patch: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/25296#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: rubendura | Owner: nobody
Type: Uncategorized | Status: new
(models, ORM) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
--
Ticket URL: <https://code.djangoproject.com/ticket/25296#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: rubendura | Owner: nobody
Type: Uncategorized | Status: new
(models, ORM) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
Apparently we cannot please all the people all the time when it comes to
this issue. ;-)
Do you have any suggestions about what change or release notes
modification to make in Django?
--
Ticket URL: <https://code.djangoproject.com/ticket/25296#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: rubendura | Owner: nobody
Type: Uncategorized | Status: new
(models, ORM) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by rubendura):
Basically I still believe the behaviour should be considered buggy.
When I call get_or_create() (or create(), I believe it doesn't make a
difference) and exception is thrown, and as such I can't get a reference
to the object being created (technically it hasn't been created, an
exception was thrown during the process so no changes should've been
made). However, even I couldn't get a hold of that object myself, some
other objects I have roaming around my system got references to it, to
that never-created-object that threw an exception.
Say I had some code like this:
{{{
try:
token = Token.objects.create(user=user)
except ValueError:
# Can't use token variable here (no token should be created)
# user.token should raise an exception, yet it contains an unsaved
token
pass
else:
# All good, use token variable as usual
pass
}}}
If ValueError is raised, the token variable is unusable on the except
block, yet I can see some debris floating around in other objects, thus
polluting them. I can basically access a model that should've never been
created (either saved on the db or not).
--
Ticket URL: <https://code.djangoproject.com/ticket/25296#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: rubendura | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.8
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* type: Uncategorized => Bug
* stage: Unreviewed => Accepted
Comment:
Makes sense. Seems like it could be fixed if the affected `QuerySet`
methods did exception catching and then cleared the related object caches
in a `finally` block. It's possible the implementation could be more
trouble than it's worth though, and we should reclassify as a
documentation improvement. Attaching a regression test.
--
Ticket URL: <https://code.djangoproject.com/ticket/25296#comment:5>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 1.8
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* Attachment "25296-test.diff" added.
Django
-------------------------------------+-------------------------------------
Reporter: rubendura | Owner:
Type: Bug | Status: assigned
Component: Database layer | Version: 1.8
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by raphaelmerx):
* owner: nobody => raphaelmerx
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/25296#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: rubendura | Owner:
| raphaelmerx
Type: Bug | Status: assigned
Component: Database layer | Version: 1.8
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by raphaelmerx):
Pull request: https://github.com/django/django/pull/5299
--
Ticket URL: <https://code.djangoproject.com/ticket/25296#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: rubendura | Owner:
| raphaelmerx
Component: Database layer | Version: 1.8
(models, ORM) |
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"f5a33e4840d3ad4d1199e99f5a17a9af1d2176f9" f5a33e48]:
{{{
#!CommitTicketReference repository=""
revision="f5a33e4840d3ad4d1199e99f5a17a9af1d2176f9"
Fixed #25296 -- Prevented model related object cache pollution when
create() fails due to an unsaved object.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25296#comment:8>