Code

Opened 5 years ago

Closed 10 months ago

Last modified 10 months ago

#11857 closed Bug (fixed)

TemporaryFile class is missing the closed - attribute

Reported by: andi100 Owned by: Christopher Adams <christopher.r.adams@…>
Component: File uploads/storage Version:
Severity: Normal Keywords: TemporaryFile closed windows
Cc: petar.maric@…, jdunck@…, pombredanne@…, christopher.r.adams@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

Hi,

the TemporaryFile-class (core/files/temp.py), is missing the 'closed'-attribute. It inherits from FileProxyMixin, but this class also doesn't have a closed-attribute.

If I want to access the 'width' attribute of an ImageFieldFile, I get an "'TemporaryFile' object has no attribute 'closed'" - error.

Traceback:

File "D:\websites\picality\src\picalityproject\picality\models\image.py" in save
  98.         print self.image.width
File "D:\websites\django-trunk\django\core\files\images.py" in _get_width
  15.         return self._get_image_dimensions()[0]
File "D:\websites\django-trunk\django\core\files\images.py" in _get_image_dimensions
  24.             close = self.closed
File "D:\websites\django-trunk\django\db\models\fields\files.py" in _get_closed
  126.         return file is None or file.closed
File "D:\websites\django-trunk\django\core\files\base.py" in _get_closed
  51.         return not self.file or self.file.closed

Exception Type: AttributeError at /upload/
Exception Value: 'TemporaryFile' object has no attribute 'closed'

But somehow this error occurs only if I want to upload 2 image files in the same form -> because if I upload only one image, the self.image.file.file is a StringIO object, which has a 'closed' attribute.

Attachments (1)

quickfix patch for 11857 against Django 1.2 pre-alpha SVN-11477.diff (497 bytes) - added by petar 5 years ago.

Download all attachments as: .zip

Change History (36)

comment:1 Changed 5 years ago by petar

  • Has patch set
  • Needs documentation unset
  • Needs tests set
  • Owner changed from nobody to petar
  • Patch needs improvement set
  • Status changed from new to assigned

Run into the same error, under different conditions.

I made a quick fix (works for me!) as I'm a bit busy these days to do things properly. When I catch a breath I'll improve the patch with comments and tests.

comment:2 Changed 5 years ago by petar

  • Cc petar.maric@… added

comment:3 Changed 4 years ago by russellm

  • Triage Stage changed from Unreviewed to Accepted

comment:4 follow-up: Changed 4 years ago by danostrowski

This issue is still present, as I've had the same problem.

However, the patch attached will not work if inserted as presented. 'temp.py' has had a lot of it's contents moved into 'tempfile.py' and there's apparently different classes now for temporary files based on whether you're using 'nt' or not. I am developing on Windows so this is of concern to me.

comment:5 Changed 3 years ago by baumer1122

  • Severity set to Normal
  • Type set to Bug

comment:6 Changed 3 years ago by homeip127@…

  • Easy pickings unset
  • UI/UX unset
  • Version SVN deleted

Bug still present! Django 1.3

File "C:\Python26\lib\site-packages\django\forms\forms.py" in full_clean
  269.         self._post_clean()
File "C:\Python26\lib\site-packages\django\forms\models.py" in _post_clean
  308.         self.instance = construct_instance(self, self.instance, opts.fields, opts.exclude)
File "C:\Python26\lib\site-packages\django\forms\models.py" in construct_instance
  53.         f.save_form_data(instance, cleaned_data[f.name])
File "C:\Python26\lib\site-packages\django\db\models\fields\files.py" in save_form_data
  281.             setattr(instance, self.name, data)
File "C:\Python26\lib\site-packages\django\db\models\fields\files.py" in __set__
  314.             self.field.update_dimension_fields(instance, force=True)
File "C:\Python26\lib\site-packages\django\db\models\fields\files.py" in update_dimension_fields
  382.             width = file.width
File "C:\Python26\lib\site-packages\django\core\files\images.py" in _get_width
  15.         return self._get_image_dimensions()[0]
File "C:\Python26\lib\site-packages\django\core\files\images.py" in _get_image_dimensions
  24.             close = self.closed
File "C:\Python26\lib\site-packages\django\db\models\fields\files.py" in _get_closed
  127.         return file is None or file.closed
File "C:\Python26\lib\site-packages\django\core\files\base.py" in _get_closed
  51.         return not self.file or self.file.closed

Exception Type: AttributeError at /photo/upload/
Exception Value: 'TemporaryFile' object has no attribute 'closed'

comment:7 Changed 3 years ago by kmtracey

Note it's is not necessary or or particularly helpful to post that still-open bugs are still recreatable. This bug is open, there is no indication anywhere in it that I can see that it has been fixed, so it's expected that it's still present. If you find an open bug that can no longer be recreated, that would be something to post (and possibly close the ticket), but posting that open bugs are still there is just noise.

comment:8 in reply to: ↑ 4 Changed 3 years ago by rush340

  • Easy pickings set
  • Patch needs improvement unset

Replying to danostrowski:

This issue is still present, as I've had the same problem.

However, the patch attached will not work if inserted as presented. 'temp.py' has had a lot of it's contents moved into 'tempfile.py' and there's apparently different classes now for temporary files based on whether you're using 'nt' or not. I am developing on Windows so this is of concern to me.

The patch works fine for me, and I don't see why it wouldn't for you. What issues are you having with it? Also, tempfile is not part of Django, it is imported from the Python Standard Library.

temp.py just tells Django what class to use for temporary files. If you are using a Windows NT OS, temp.py will define a custom TemporaryFile class for Django to use. If you are not using a Windows NT OS, temp.py will tell Django to just use the tempfile.NamedTemporaryFile class. The reason for this is that NamedTemporaryFile cannot be reopened with a Windows NT OS.

This bug is caused because the custom TemporaryFile object Django defines for NT does not have a closed property, but self.closed is accessed when using the inherited open method. If you are not using NT, this bug won't affect you.

The patch seems to add the property with the expected behavior. In addition to the changes made by the patch, it seems like it would be cleaner to only use the custom TemporaryFile object regardless of OS. It's a pretty simple class and I don't see any platform specific notes for the Standard Library functionality used by it.

Either way, just applying the patch should work. It just needs a test.

I'll try to get to it if I have time, but I'm pretty busy at the moment.

comment:9 Changed 3 years ago by anonymous

The patch works fine for me.

Environment - Windows 2008 server, Apache + mod_wsgi, Django 1.3

comment:10 Changed 3 years ago by anentropic

Patch worked for me: Windows 7 64 bit, Django 1.3 dev server

comment:11 Changed 2 years ago by claudep

  • Keywords windows added

comment:12 Changed 2 years ago by anonymous

Patch doesn't work on Win 8

comment:13 Changed 2 years ago by anonymous

Sorry it does work on win8 django 1.3.1 devserver

comment:14 Changed 16 months ago by AnkitBagaria

  • Owner petar deleted
  • Status changed from assigned to new

comment:15 Changed 16 months ago by susan

  • Owner set to susan
  • Status changed from new to assigned

comment:16 Changed 15 months ago by jdunck

  • Cc jdunck@… added

comment:17 Changed 15 months ago by pombredanne

I experienced the same issue on win7 and django 1.5.1.
Susan, do you want a patch?

comment:18 Changed 15 months ago by pombredanne@…

  • Cc pombredanne@… added

comment:19 Changed 15 months ago by susan

Sure, I'll appreciate. Since Im running macOS, and the bug appears only on windowsOS, I think I may have difficulty replicating this bug.

comment:20 Changed 15 months ago by pombredanne@…

susan, let me craft that and reference a pull request here.

comment:21 Changed 15 months ago by pombredanne

  • Owner changed from susan to pombredanne

comment:22 Changed 15 months ago by pombredanne

Since Python 2.6, tempfile.NamedTemporaryFile has a new attribute: delete=True/False which likely removes the needs entirely to have a django.core.temp.NamedTemporaryFile wrapper specifically for 'nt'. I will investigate this since 2.6 is a requirement for 1.5.x. The lack of a closed attribute is something that happens only in some contexts and is something that is injected elsewhere and I am still trying to understand where/when exactly.

Version 0, edited 15 months ago by pombredanne (next)

comment:23 Changed 13 months ago by pombredanne

  • Owner pombredanne deleted
  • Status changed from assigned to new

comment:24 Changed 13 months ago by pombredanne

I am unassigning this ticket from me, as I am not able to work on it in the short term ... but this is still on my radar

comment:25 Changed 10 months ago by adamsc64

  • Owner set to anonymous
  • Status changed from new to assigned

comment:26 Changed 10 months ago by anonymous

  • Needs tests unset

comment:27 Changed 10 months ago by adamsc64

Submitted a pull request that should solve this at https://github.com/django/django/pull/1566. This could probably easily be made into a bug fix for previous versions of Django, as it's just a few lines of code and the issue seems to be very old.

comment:28 Changed 10 months ago by Christopher Adams <christopher.r.adams@…>

  • TemporaryFile now minimally mocks the API of the Python standard library class tempfile.NamedTemporaryFile to avoid AttributeError exceptions.
  • The symbol django.core.files.NamedTemporaryFile is actually assigned as a different class on different operating systems.
  • The bug only occurred if Django is running on Windows, hence why it was hard to diagnose.
  • All tests pass with ./runtests.py --settings=test_sqlite

comment:29 Changed 10 months ago by Christopher Adams <christopher.r.adams@…>

  • Owner anonymous deleted
  • Status changed from assigned to new

comment:30 Changed 10 months ago by Christopher Adams <christopher.r.adams@…>

  • Resolution set to fixed
  • Status changed from new to closed
  • Triage Stage changed from Accepted to Unreviewed

comment:31 Changed 10 months ago by timo

  • Resolution fixed deleted
  • Status changed from closed to new
  • Triage Stage changed from Unreviewed to Accepted

Please don't close a ticket until the fix has been committed to master.

comment:32 Changed 10 months ago by Christopher Adams <christopher.r.adams@…>

  • Cc christopher.r.adams@… added

Gotcha. Sorry about that - I'm a n00b!

comment:33 Changed 10 months ago by Christopher Adams <christopher.r.adams@…>

  • Owner set to Christopher Adams <christopher.r.adams@…>
  • Resolution set to fixed
  • Status changed from new to closed

In b2f5ac16565605f20a0c4e90acc6beed5a5ac1ce:

Fixed #11857 -- Added missing 'closed' property on TemporaryFile class.

  • TemporaryFile now minimally mocks the API of the Python standard library class tempfile.NamedTemporaryFile to avoid AttributeError exceptions.
  • The symbol django.core.files.NamedTemporaryFile is actually assigned as a different class on different operating systems.
  • The bug only occurred if Django is running on Windows, hence why it was hard to diagnose.

comment:34 Changed 10 months ago by Russell Keith-Magee <russell@…>

In 926bc421d9bcf04a79f0026a60d3d4b0570b7fe2:

Merge pull request #1566 from adamsc64/ticket_11857

Fixed #11857 -- Added missing 'closed' property on TemporaryFile class.

comment:35 Changed 10 months ago by Russell Keith-Magee <russell@…>

In 2a2ac5c1400c67f25388621a39749c918a4efe98:

Merge pull request #1566 from adamsc64/ticket_11857

Fixed #11857 -- Added missing 'closed' property on TemporaryFile class.

Backport of 926bc42 from trunk.

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.