Opened 11 years ago

Closed 5 years ago

#21238 closed Bug (fixed)

`ImageFieldFile.url` raises an AttributeError exception after retrieving it from cache

Reported by: Pierre Dulac Owned by: Hasan Ramezani
Component: Core (Cache system) Version: dev
Severity: Normal Keywords: imagefield, cache
Cc: sky.kok@…, lucmult@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Hi everyone,

To reproduce the exception, just cache an instance of ImageFieldFile then retrieve it and call the url property on this retrieved instance.

Here is a minimal code to reproduce :

from django.core.cache import cache
from django.db import models

class A(models.Model):
    pictogram = models.ImageField(upload_to='images')
    class Meta:
        app_label = 'test'

a = A()
a.pictogram = 'fake/test/url.png'
original_picto = a.pictogram

cache_key = 'picto'
cache.set(cache_key, original_picto, 60)
cached_picto = cache.get(cache_key)

print original_picto.url
print cached_picto.url

and you get

AttributeError: 'ImageFieldFile' object has no attribute 'storage'

Didn't tried to fix it yet...

Have a nice day,

Pierre

Attachments (2)

alternative_patch_ticket_21238.patch (2.0 KB ) - added by Vajrasky Kok 11 years ago.
fieldfile_pickle_test.diff (1.5 KB ) - added by Tim Graham 10 years ago.

Download all attachments as: .zip

Change History (20)

comment:1 by Pierre Dulac, 11 years ago

Summary: `ImageFieldFile.url` raise an AttributeError after retrieving it from cache`ImageFieldFile.url` raises an AttributeError after retrieving it from cache

comment:2 by Pierre Dulac, 11 years ago

Summary: `ImageFieldFile.url` raises an AttributeError after retrieving it from cache`ImageFieldFile.url` raises an AttributeError exception after retrieving it from cache

comment:3 by Tim Graham, 11 years ago

Triage Stage: UnreviewedAccepted

ImageFieldFile inherits from ImageFile and FieldFile. It looks like the problem is due to the fact that FieldFile.__init__ isn't called when the object is retrieved from the cache. I think the inheritance chain is broken because ImageFile.__init__ (inheriting from django.core.files.base.File) doesn't call super.

comment:4 by Vajrasky Kok, 11 years ago

Here is the pull request (based on master/1.7) that can fix this problem: https://github.com/django/django/pull/1781

comment:5 by Claude Paroz, 11 years ago

Has patch: set

comment:6 by Tim Graham, 11 years ago

I thought fixing the inheritance chain by having django.core.files.base.File.__init__ call super() would be the way to fix this. Thoughts?

comment:7 by Vajrasky Kok, 11 years ago

I don't understand. django.core.files.base.File has only one parent, which is django.core.files.utils.FileProxyMixin which does not have __init__ method.

I try to do this in django/core/files/base.py:

    def __init__(self, file, name=None):
+       super(File, self).__init__()
        self.file = file
        if name is None:
            name = getattr(file, 'name', None)
        self.name = name
        if hasattr(file, 'mode'):
            self.mode = file.mode
+       # or even this, but it does not work
+       # super(File, self).__init__()

Before resorting to __reduce__ way, I did try to solve the problem by playing with super and __init__ in some classes related with ImageFieldFile, either I missed something or it did not work that way.

comment:8 by Tim Graham, 11 years ago

Ok, thanks for the response -- I guess my initial hunch wasn't correct. To be honest, I'm not super familiar with the internals of how pickle and __reduce__ work so I have some reading to do. If you could include a quick explanation of your understanding of the problem and why this solution works, that would be quite helpful for me.

comment:9 by Vajrasky Kok, 11 years ago

Cc: sky.kok@… added
Owner: changed from nobody to Vajrasky Kok
Status: newassigned
Version: 1.5master

Sorry, the original PR does not work with Python 2.7. Also, __reduce__ is prone to error. It's better to use __getstate__. I'll already updated the PR. https://github.com/django/django/pull/1781

But first, let me give you the lesson of pickling. :) Naturally, without human intervention, the pickling (pickle.dumps) will copy the instance's dict. So why in this case the all ImageFieldFile instance's dict members (notably the storage attribute) do not get copied. The ImageFieldFile class does not define any method that can intervene the pickling process (__getinitargs__, __getnewargs__, __getstate__, __setstate__, __reduce__, __reduce_ex__). But the ancestor of ImageFieldFile, which is FieldFile, does have method that intervenes pickling process.

    def __getstate__(self):
        # FieldFile needs access to its associated model field and an instance
        # it's attached to in order to work properly, but the only necessary
        # data to be pickled is the file's name itself. Everything else will
        # be restored later, by FileDescriptor below.
        return {'name': self.name, 'closed': False, '_committed': True, '_file'

That's why the storage attribute of ImageFieldFile instance is missing because only name, closed, _committed attributes are copied in the pickling process.

There is an alternative solution for this problem beside my PR, which is removing the __getstate__ method from FieldFile. The downside is the dump result of FieldFile will be unnecessarily bigger (based on my understanding of the comment). I am not sure whether there is another ill effects or not. Attached the patch to do that.

by Vajrasky Kok, 11 years ago

comment:10 by lucmult@…, 11 years ago

Cc: lucmult@… added

comment:11 by Tim Graham, 10 years ago

Cc: Tim Graham added
Patch needs improvement: set

Won't ImageFieldFile be bigger if we override its __getstate__? I'm attaching a patch with a test that shows FieldFile has the same problem (AttributeError: 'FieldFile' object has no attribute 'storage' when unpickling) so I think we need a solution that covers both classes.

by Tim Graham, 10 years ago

Attachment: fieldfile_pickle_test.diff added

comment:12 by Hasan Ramezani, 5 years ago

Owner: changed from Vajrasky Kok to Hasan Ramezani
Patch needs improvement: unset
Version 0, edited 5 years ago by Hasan Ramezani (next)

comment:13 by Mariusz Felisiak, 5 years ago

Patch needs improvement: set

comment:14 by Tim Graham, 5 years ago

Cc: Tim Graham removed

comment:15 by Hasan Ramezani, 5 years ago

Patch needs improvement: unset

comment:16 by Mariusz Felisiak, 5 years ago

Triage Stage: AcceptedReady for checkin

comment:17 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

In aaea9de:

Refs #21238 -- Added more tests for pickling FileField and ImageField.

comment:18 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

Resolution: fixed
Status: assignedclosed

In f600e3f:

Fixed #21238 -- Fixed restoring attributes when pickling FileField and ImageField.

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