Code

Opened 3 years ago

Closed 2 years ago

Last modified 19 months ago

#15644 closed Bug (fixed)

django.core.files.base.File enhancement / fix

Reported by: nickname123 Owned by: nobody
Component: File uploads/storage Version: master
Severity: Normal Keywords: file
Cc: michael.palumbo87@… 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,

I think that django.core.files.base.File should be expanded to handle a wider range of file objects. In my specific case, StringIO/cStringIO and tempfile.SpooledTemporaryFile objects.

Here is a simple demonstration of where the File class breaks:

from tempfile import SpooledTemporaryFile
from django.core.files import File
f = SpooledTemporaryFile(
		max_size    = 1024,# 1kb
		mode        = 'w+b',# must be open in binary mode
		bufsize     = -1,
		suffix      = '',
		prefix      = 'tmp',
		dir         = None
)
f.write("hello")
print len(File(f))

Here is the result (on Windows using Python2.6):

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\...\django\core\files\base.py", line 33, in __len__
    return self.size
  File "C:\...\django\core\files\base.py", line 39, in _get_size
    elif os.path.exists(self.file.name):
  File "C:\...\lib\tempfile.py", line 559, in name
    return self._file.name
AttributeError: 'cStringIO.StringO' object has no attribute 'name'

It should be noted that not only does the current implementation fail, but it breaks in the wrong code block because it doesn't verify that the name attribute is available.

I propose that the file objects seek and tell method be used as an additional fallback before throwing the attribute error as follows:

def _get_size(self):
	if not hasattr(self, '_size'):
		if hasattr(self.file, 'size'):
			self._size = self.file.size
		elif hasattr(self.file, 'name') and os.path.exists(self.file.name):
			self._size = os.path.getsize(self.file.name)
		elif hasattr(self.file, 'tell') and hasattr(self.file, 'seek'):
			pos = self.file.tell()
			self.file.seek(0,os.SEEK_END)
			self._size = self.file.tell()
			self.file.seek(pos)
		else:
			raise AttributeError("Unable to determine the file's size.")
	return self._size

My proposed patch fixes the problems mentioned above.

Attachments (3)

django.core.files.base.File.diff (911 bytes) - added by nickname123 3 years ago.
patch
15644-2.diff (2.3 KB) - added by claudep 2 years ago.
Patch with tests
15644-3.diff (3.7 KB) - added by Michael Palumbo <michael.palumbo87@…> 2 years ago.

Download all attachments as: .zip

Change History (16)

Changed 3 years ago by nickname123

patch

comment:1 Changed 3 years ago by nickname123

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Version changed from 1.2 to SVN

comment:2 Changed 3 years ago by lukeplant

  • Type set to New feature

comment:3 Changed 3 years ago by lukeplant

  • Severity set to Normal

comment:4 Changed 3 years ago by julien

  • Component changed from Uncategorized to File uploads/storage
  • Needs documentation set
  • Needs tests set
  • Triage Stage changed from Unreviewed to Accepted

comment:5 Changed 3 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

comment:11 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:12 Changed 2 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

Changed 2 years ago by claudep

Patch with tests

comment:13 Changed 2 years ago by claudep

  • Needs documentation unset
  • Needs tests unset
  • Type changed from New feature to Bug

I've reviewed the File documentation (https://docs.djangoproject.com/en/dev/ref/files/file/), but didn't find any complement to add related to this fix.

Changed Type to bug because of the error raised by _get_size when the underlying file object has no name (see also #16946 which is a duplicate).

comment:14 Changed 2 years ago by Michael Palumbo <michael.palumbo87@…>

That would be nice to make the File object more file-like object so that we could use the File Storage API with any file-like objects (e.g. with what urllib2.urlopen returns)
Because as for now, this does not work for the main reason said previously (elif os.path.exists(self.file.name):

import urllib2
from django.core.files.base import File
from django.core.files.storage import FileSystemStorage
url = 'http://www.google.fr'
fs = FileSystemStorage(location='./here/')
f = File(urllib2.urlopen(urllib2.Request(url)))
fs.save("downloaded_file.html", f)

Is it planned to be changed ? Maybe for django 1.4 ? How can we help ?

Thanks.

Changed 2 years ago by Michael Palumbo <michael.palumbo87@…>

comment:15 Changed 2 years ago by Michael Palumbo <michael.palumbo87@…>

  • Cc michael.palumbo87@… added

I added a new patch because the chunks method of the File object did not work with urllib2.urlopen().
Indeed, this method was using the size which is unknown in this case.
I completed claudep's patch.

comment:16 Changed 2 years ago by anonymous

  • Triage Stage changed from Accepted to Ready for checkin

I think this is good to go.

comment:17 Changed 2 years ago by claudep

  • Resolution set to fixed
  • Status changed from new to closed

In [17871]:

Fixed #15644 -- Improved Django File wrapper to support more file-like objects. Thanks nickname123 and Michael Palumbo for working on the patch.

comment:18 Changed 19 months ago by CollinAnderson

(this was actually a dup of #8501)

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.