Opened 6 years ago

Closed 6 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)

25296-test.diff (789 bytes) - added by Tim Graham 6 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 6 years ago by Rubén Durá Tarí

Component: UncategorizedDatabase layer (models, ORM)

comment:2 Changed 6 years ago by Rubén Durá Tarí

Summary: Failed Manager.create pollutes related models cacheFailed Manager.create pollutes related models cache in 1.8.4

comment:3 Changed 6 years ago by Tim Graham

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?

comment:4 Changed 6 years ago by Rubén Durá Tarí

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 Changed 6 years ago by Tim Graham

Summary: Failed Manager.create pollutes related models cache in 1.8.4Failed Manager.create() could pollute related models cache
Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

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.

Changed 6 years ago by Tim Graham

Attachment: 25296-test.diff added

comment:6 Changed 6 years ago by Raphael Merx

Owner: changed from nobody to Raphael Merx
Status: newassigned

comment:8 Changed 6 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In f5a33e48:

Fixed #25296 -- Prevented model related object cache pollution when create() fails due to an unsaved object.

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