Opened 14 years ago

Closed 8 years ago

#13809 closed Bug (fixed)

FileField open method is only accepting 'rb' modes

Reported by: Cesar Canassa Owned by: Chris Sinchok
Component: File uploads/storage Version: dev
Severity: Normal Keywords:
Cc: real.human@…, David Foster 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

I have the following model in my application:

class OrdemServico(models.Model):
    carrinho = models.OneToOneField('Carrinho')
    criacao = models.DateTimeField(u'data de criação', default=datetime.now)
    pdf = models.FileField(upload_to="ordem_pdf")

Then I have the following code in my application:

>>> ordem, created = OrdemServico.objects.get_or_create(carrinho=self)
>>> ordem.pdf.open(mode='wb')
>>> f = ordem.pdf.file
>>> print f
c:\users\cesar\documents\projetos\rm2\mysite\media\ordem_pdf\157_5.pdf
>>> print f.tell()
0
>>> print f.write('abc')
Traceback (most recent call last):
  File "C:\Python26\lib\site-packages\django\core\servers\basehttp.py", line 280, in run
    self.result = application(self.environ, self.start_response)
  File "C:\Python26\lib\site-packages\django\core\servers\basehttp.py", line 674, in __call__
    return self.application(environ, start_response)
  File "C:\Python26\lib\site-packages\django\core\handlers\wsgi.py", line 241, in __call__
    response = self.get_response(request)
  File "C:\Python26\lib\site-packages\django\core\handlers\base.py", line 142, in get_response
    return self.handle_uncaught_exception(request, resolver, exc_info)
  File "C:\Python26\lib\site-packages\django\core\handlers\base.py", line 100, in get_response
    response = callback(request, *callback_args, **callback_kwargs)
  File "C:\Python26\lib\site-packages\django\contrib\admin\options.py", line 239, in wrapper
    return self.admin_site.admin_view(view)(*args, **kwargs)
  File "C:\Python26\lib\site-packages\django\utils\decorators.py", line 76, in _wrapped_view
    response = view_func(request, *args, **kwargs)
  File "C:\Python26\lib\site-packages\django\views\decorators\cache.py", line 69, in _wrapped_view_func
    response = view_func(request, *args, **kwargs)
  File "C:\Python26\lib\site-packages\django\contrib\admin\sites.py", line 190, in inner
    return view(request, *args, **kwargs)
  File "C:\Python26\lib\site-packages\django\utils\decorators.py", line 21, in _wrapper
    return decorator(bound_func)(*args, **kwargs)
  File "C:\Python26\lib\site-packages\django\utils\decorators.py", line 76, in _wrapped_view
    response = view_func(request, *args, **kwargs)
  File "C:\Python26\lib\site-packages\django\utils\decorators.py", line 17, in bound_func
    return func(self, *args2, **kwargs2)
  File "C:\Python26\lib\site-packages\django\contrib\admin\options.py", line 991, in changelist_view
    response = self.response_action(request, queryset=cl.get_query_set())
  File "C:\Python26\lib\site-packages\django\contrib\admin\options.py", line 749, in response_action
    response = func(self, request, queryset)
  File "C:\Users\Cesar\Documents\Projetos\RM2\mysite\..\mysite\moveit\admin.py", line 41, in gerar_pdf
    carrinho.gera_ordem()
  File "C:\Users\Cesar\Documents\Projetos\RM2\mysite\..\mysite\moveit\models.py", line 204, in gera_ordem
    print f.write('abc')
IOError: [Errno 9] Bad file descriptor

I did some debugging and I find out that the open() method was opening the file in read-only mode. This was causing the IOError during the write call.

I checked the django.db.models.fields.file.py file:

    def _get_file(self):
        self._require_file()
        if not hasattr(self, '_file') or self._file is None:
            self._file = self.storage.open(self.name, 'rb')
        return self._file

    def open(self, mode='rb'):
        self._require_file()
        self.file.open(mode)
    # open() doesn't alter the file's contents, but it does reset the pointer
    open.alters_data = True

We I called the open() method it tried to access the file property which itself also tries to open the file, but in read-only mode.

I kinda solved the problem by calling the storage method directly:

    def open(self, mode='rb'):
        self._require_file()
        #self.file.open(mode)
        self._file = self.storage.open(self.name, mode)

Thanks,
Cesar Canassa

Attachments (1)

13809-1.diff (1.9 KB ) - added by Claude Paroz 12 years ago.
Allow FielField instance to be open in writable mode

Download all attachments as: .zip

Change History (19)

comment:1 by Daniel F Moisset, 14 years ago

Resolution: invalid
Status: newclosed

The file field is not documented (so it isn't doing anything wrong), and the documented method (open()) work as expected, so I think this is not a bug

comment:2 by jesh, 14 years ago

Resolution: invalid
Status: closedreopened

This still does not work for me. Documented open() does not honor mode (it's not *reopening* r/o file). That has a annoying side effect, so that FieldFile.file.open("w") does not work. This is because _get_file has already opened it as read only. As a workaround, every open could be preceded by lonely close(), but IMHO that's just plain ugly. My app code looks like

class MyModel(models.Model:
    file = models.FileField(...)

this does not work:
f=mymodel.file.open("rw")
mymodel.file.write(content) # -> invalid fd
mymodel.file.close()

this will work:
f=mymodel.file # file gets opened here, see [1]
f.file.close() # following open [2] uses "rb" without this -> invalid fd when writing
f.file.open("wb")
mymodel.file.write(content)
mymodel.file.close()
  1. http://code.djangoproject.com/browser/django/trunk/django/db/models/fields/files.py#L49
  2. http://code.djangoproject.com/browser/django/trunk/django/core/files/base.py#L110

comment:3 by Russell Keith-Magee, 14 years ago

Component: File uploads/storageDocumentation
Triage Stage: UnreviewedAccepted

I don't think it's unreasonable to expect that when you access a file through a model field, it will be open ready for reading. The best solution I can see here is documenting the fact that files are opened "rb" when accessed, and if you want to perform write operations, you'll need to close the file first.

I'm open to any other suggestions, but I'll accept this as a documentation issue for now.

in reply to:  3 comment:4 by yishai@…, 14 years ago

Component: DocumentationFile uploads/storage

Replying to russellm:

I don't think it's unreasonable to expect that when you access a file through a model field, it will be open ready for reading. The best solution I can see here is documenting the fact that files are opened "rb" when accessed, and if you want to perform write operations, you'll need to close the file first.

I think this is a little more subtle than that; since the underlying file object is "automatically" opened (with 'rb' mode) upon direct access, the main reason one would call FieldFile.open is to open the file in another mode. Having FieldFile.open(mode='w') return a bad file descriptor (for writing) without complaining is hardly optimal.

I'm open to any other suggestions, but I'll accept this as a documentation issue for now.

Even if deciding that one should always close first before opening the file for writing, calling FieldFile.close() is not enough as it will skip the underlying file object if it was not opened yet. This code will not work:

  obj = MyModel.objects.get(pk=1)
  obj.myFileField.close() # would expect this to close the underlying file object, so it's ready to be reopened for writing
  obj.myFileField.open('w')
  obj.myFileField.write('something')

If we rewrite FieldFile.close() from:

    def close(self):
        file = getattr(self, '_file', None)
        if file is not None:
            file.close()

to:

    def close(self):
        self.file.close()

then the above snippet will work as advertised - the underlying file will be opened (rb) and then closed again. Currently the snippet needs to be:

  obj = MyModel.objects.get(pk=1)
  obj.myFileField.file.close() # works, but hardly nice 
  obj.myFileField.open('w')
  obj.myFileField.write('something')

which works, but accessing the underlying (undocumented?) file object directly is probably not what we really want here.

comment:5 by Julien Phalip, 13 years ago

Severity: Normal
Type: Bug

comment:6 by Tai Lee, 13 years ago

Cc: real.human@… added
Easy pickings: unset
Needs tests: set
UI/UX: unset
Version: 1.2SVN

The docs say that File.open() will "re-open" the file (and as a side effect, do File.seek(0). In reality, File.open() will only do File.seek(0) if the file is already open, and will ignore the newly specified mode.

A separate but related issue is that even if the file is explicitly closed, and File.open() is called with a new mode, File.mode will still return the original mode. I'm not sure if the intent of the docs was that re-opening a file would use the original mode if not specified, or the current mode. Either way, I think that File.mode should mirror the current file mode.

The problem with FieldFile.open() (a subclass of File) is that it calls FieldFile.file.open(), when FieldFile.file will open the file implicitly with a mode of "rb", and then .open() will just do .seek(0), regardless of the mode that was passed in.

I'm fine with the idea that FieldFile should be open in "rb" mode when accessed by default, but users shouldn't have to explicitly access it first so that they can insert an explicit call to .close() before calling .open() with a different mode.

I think the fix here involves a couple of steps.

  1. File.open() should call .close() if the file is already open (instead of .seek(0)), and then actually re-open it with the new mode (or the mode that was originally used, if not specified), as per the docs.
  1. FieldFile.open() should not implicitly open the file in "rb" mode when a mode is explicitly set. FieldFile.open().

Cheers.

comment:7 by David Foster, 13 years ago

Cc: David Foster added

I confirm this issue as still present in Django 1.3.

In short, if you call FieldFile.open(mode=NOT_RB), it will always open in mode rb. This is due to FieldFile.open calling the self.file property, which auto-opens in 'rb' mode. FieldFile.open attempts to subsequently call open on this new file, but that does not reopen the file again.

My suggested workaround for:

  • field_file.open(mode='w+b')

is:

  • field_file.file = field_file.storage.open(field_file.name, 'w+b')

Naturally this is just a workaround, not a real fix.

Last edited 13 years ago by David Foster (previous) (diff)

by Claude Paroz, 12 years ago

Attachment: 13809-1.diff added

Allow FielField instance to be open in writable mode

comment:8 by Claude Paroz, 12 years ago

Has patch: set
Needs tests: unset
Owner: nobody removed
Status: reopenednew

The patch is still not optimal, because the file is opened in 'rb' mode upon access, then closed and reopened in the specified mode. Seems to work, though.

comment:9 by Claude Paroz, 12 years ago

#16964 was a duplicate.

comment:10 by anonymous, 11 years ago

I confirm this issue as still present in Django 1.5.

comment:11 by jetfix, 11 years ago

I confirm this that this issue is still present in 1.5 =(

comment:12 by Tim Graham, 10 years ago

Patch needs improvement: set

comment:13 by synotna, 10 years ago

Still present in 1.7 :(

My workaround is .read(), .close () then open it again in another mode

Last edited 10 years ago by synotna (previous) (diff)

comment:14 by Tim Graham, 9 years ago

Resolution: duplicate
Status: newclosed

I think #26398 may have at least partially addressed this but I haven't verified completely.

comment:15 by Tim Graham, 9 years ago

Resolution: duplicate
Status: closednew

comment:16 by Chris Sinchok, 8 years ago

Owner: set to Chris Sinchok
Status: newassigned

comment:17 by Tim Graham, 8 years ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

PR. I marked #26398 as a duplicate and will include the test from the PR for that ticket here.

comment:18 by Tim Graham <timograham@…>, 8 years ago

Resolution: fixed
Status: assignedclosed

In ac1975b1:

Fixed #13809 -- Made FieldFile.open() respect its mode argument.

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