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)
Change History (19)
comment:1 by , 14 years ago
Resolution: | → invalid |
---|---|
Status: | new → closed |
comment:2 by , 14 years ago
Resolution: | invalid |
---|---|
Status: | closed → reopened |
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()
follow-up: 4 comment:3 by , 14 years ago
Component: | File uploads/storage → Documentation |
---|---|
Triage Stage: | Unreviewed → Accepted |
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.
comment:4 by , 14 years ago
Component: | Documentation → File 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 , 14 years ago
Severity: | → Normal |
---|---|
Type: | → Bug |
comment:6 by , 13 years ago
Cc: | added |
---|---|
Easy pickings: | unset |
Needs tests: | set |
UI/UX: | unset |
Version: | 1.2 → SVN |
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.
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.
FieldFile.open()
should not implicitly open the file in "rb" mode when a mode is explicitly set.FieldFile.open()
.
Cheers.
comment:7 by , 13 years ago
Cc: | 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.
by , 12 years ago
Attachment: | 13809-1.diff added |
---|
Allow FielField instance to be open in writable mode
comment:8 by , 12 years ago
Has patch: | set |
---|---|
Needs tests: | unset |
Owner: | removed |
Status: | reopened → new |
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:12 by , 10 years ago
Patch needs improvement: | set |
---|
comment:13 by , 10 years ago
Still present in 1.7 :(
My workaround is always close the file before opening it again
comment:14 by , 9 years ago
Resolution: | → duplicate |
---|---|
Status: | new → closed |
I think #26398 may have at least partially addressed this but I haven't verified completely.
comment:15 by , 9 years ago
Resolution: | duplicate |
---|---|
Status: | closed → new |
comment:16 by , 8 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:17 by , 8 years ago
Patch needs improvement: | unset |
---|---|
Triage Stage: | Accepted → Ready for checkin |
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