Opened 9 years ago
Closed 9 years ago
#25296 closed Bug (fixed)
Failed Manager.create() could pollute related models cache
Reported by: | Rubén Durá Tarí | Owned by: | Raphael Merx |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | 1.8 |
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
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.
Attachments (1)
Change History (9)
comment:1 by , 9 years ago
Component: | Uncategorized → Database layer (models, ORM) |
---|
comment:2 by , 9 years ago
Summary: | Failed Manager.create pollutes related models cache → Failed Manager.create pollutes related models cache in 1.8.4 |
---|
comment:3 by , 9 years ago
comment:4 by , 9 years ago
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).
comment:5 by , 9 years ago
Summary: | Failed Manager.create pollutes related models cache in 1.8.4 → Failed Manager.create() could pollute related models cache |
---|---|
Triage Stage: | Unreviewed → Accepted |
Type: | Uncategorized → Bug |
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.
by , 9 years ago
Attachment: | 25296-test.diff added |
---|
comment:6 by , 9 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
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?