Code

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#15783 closed Bug (invalid)

Class FileProxyMixin has invalid attribute definition

Reported by: sebzur Owned by: nobody
Component: File uploads/storage Version: 1.3
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX:

Description

In: django/core/files/utils.py line 12, in the definition of class class FileProxyMixin used in
form fileds to handle file uploads, there is:

fileno = property(lambda self: self.file.fileno)

which proxies to self.file.fileno... *but* self.file has no fileno attribute, which
produces an error on the access try...

Attachments (0)

Change History (5)

comment:1 Changed 3 years ago by sebzur

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

I think I was not to verbose with the ticket, so some more info below:

The error occurs when I was handling uploded file, that has not been saved and is in fact InMemoryUploadedFile object. Then, InMemoryUploadedFile instance does not have fileno attribute working correctly.

Version 1, edited 3 years ago by sebzur (previous) (next) (diff)

comment:2 Changed 3 years ago by jonash

  • Easy pickings unset
  • Resolution set to invalid
  • Status changed from new to closed

From the Python documentation:

file.fileno()
[...]
File-like objects which do not have a real file descriptor should *not* provide
this method!

That's exactly what Django does: Raising an AttributeError for file-likes that have no fileno.

comment:3 Changed 3 years ago by sebzur

OK, I understand that... but the case is: FileProxyMixin defines fileno property, which proxies to self.file.fileno .. which does not exist.

The fact that self.file.fileno does not exist (when file is e.g. InMemomyUploadedFile instance) is of course valid with respect to Python docs, but.. when I'm using an object (an instance of FileProxyMixin subclass) that is reporting (i.e. with dir) that it has fileno attribute, shouldn't it have it?

I found the current FileProxyMixin definition unclear. IMHO it confuses programmer, who has to know, that despite the existence of the attribute, he can not use it.

One can of course say, that this is somehow similar case to the case of abstract method raising NonImplementedError being at the same time the existing attribute of the class, but while the Python docs states that the method should not be provided in fact it is (at least as a 'reference' to AttributeError).

This is just to comment what worries me here. :D Maybe FileProxyMixin should introspect self.file to see if it provides fileno before putting it as a proxy attr?

comment:4 Changed 3 years ago by jonash

From the runtime's point of view (hasattr/getattr), it doesn't have a fileno attribute.

    >>> class Foo(object):
    ...   @property
    ...   def fileno(self):
    ...     raise AttributeError
    ... 
    >>> hasattr(Foo(), 'fileno')
    False

Last edited 3 years ago by jonash (previous) (diff)

comment:5 Changed 3 years ago by sebzur

OKay - now I see it! This comment helps me a little. You're right the when You're looking on this with hasattr, there's no 'fileno', and everything is perfectly okay.

My 'fault' was, that I was looking on this firstly with dir, which simply listed 'fileno' as attr and than I was trying to use it. I was also looking into the code, where I also saw 'fileno' implemented.

So I thing it should be left as it is... but there's still a tiny bit of hesitation in me when I'm thinking abut it...

(fileno behaviour is like taken from the quantum mechanics: You know that it's not there only if You really try to touch it.) :D

Anyway - thanks for the discussion!

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.