Opened 7 years ago

Closed 7 years ago

#7812 closed (invalid)

read() in an InMemoryUploadedFile returns an empty string

Reported by: kratorius Owned by: nobody
Component: Core (Other) Version: master
Severity: Keywords: upload
Cc: Triage Stage: Design decision needed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: UI/UX:

Description

If you try to access to .read() in an InMemoryUploadedFile object, you'll get an empty string unless you don't call .seek(0) so it gets to the beginning of the file.

To test, just write a model like this:

class TestModel(models.Model):
    photo = models.ImageField(upload_to='test/', validator_list=[validators.CustomValidator()])

    class Admin:
        pass

And this validator:

class CustomValidator(object):
    def __call__(self, field_data, all_data):
        print "Size: " + str(field_data.size)
        print "Len: " + str(len(field_data.read()))

We get:

Size: 378054
Len: 0

Attached patch solves this issue.

Attachments (5)

inmemoryupload_fileseek.patch (415 bytes) - added by kratorius 7 years ago.
inmemoryupload_fileseek-r7977.patch (1.0 KB) - added by kratorius 7 years ago.
Fixes .read() behaviour for InMemoryUploadedFile and TemporaryUploadedFile
inmemoryupload_fileseek_testcases-r7977.patch (2.2 KB) - added by kratorius 7 years ago.
Test cases for this ticket
r8007-seek-testcase.patch (2.5 KB) - added by kratorius 7 years ago.
Test for the right behaviour of .read()
r8161-seek-testcase-fixed.patch (2.5 KB) - added by kratorius 7 years ago.
Fixed testcase

Download all attachments as: .zip

Change History (11)

Changed 7 years ago by kratorius

comment:1 Changed 7 years ago by Simon Greenhill

  • Needs documentation unset
  • Needs tests set
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Ready for checkin

It'd be quite nice if we could get a test for this.

comment:2 Changed 7 years ago by kratorius

The same behaviour applies to TemporaryUploadedFile too, so I'm wondering if this is a wanted thing (in this case shouldn't this be properly documented?).
Anyway next patch fixes both InMemoryUploadedFile and TemporaryUploadedFile, and I provided test cases for both.

Changed 7 years ago by kratorius

Fixes .read() behaviour for InMemoryUploadedFile and TemporaryUploadedFile

Changed 7 years ago by kratorius

Test cases for this ticket

comment:3 follow-up: Changed 7 years ago by kratorius

  • Needs tests unset
  • Triage Stage changed from Ready for checkin to Design decision needed

After a bit of thinking (and a short discussion yesterday on IRC), I came to conclusion that the previous patch breaks the .read() behaviour since with that patch, .read() always read from the beginning of the file, but someone could still want to read a bunch of bytes at times.

The right behaviour should be:

  1. .read() called first time, returns the whole file
  2. the second time (and third, fourth and so on) .read() returns the empty string

(I didn't talked about the case when you call .read(size), but should work more or less in the same way, except that you'll have to call .read() multiple times before to get the empty string).

I'm still trying to figure out what's happening since both at InMemoryUploadedFile and TemporaryUploadedFile there's a file.seek(0) in the __init__, but looks like if the file pointer gets changed somewhere before I can call .read().
Anyway, I'm going to attach a fixed testcase that covers this issue.

Changed 7 years ago by kratorius

Test for the right behaviour of .read()

comment:4 in reply to: ↑ 3 ; follow-up: Changed 7 years ago by Jan Rademaker <j.rademaker@…>

Replying to kratorius:

I'm still trying to figure out what's happening since both at InMemoryUploadedFile and TemporaryUploadedFile there's a file.seek(0) in the __init__, but looks like if the file pointer gets changed somewhere before I can call .read().

Isn't this because the call to seek() missing from source:/django/trunk/django/core/files/uploadhandler.py#8046 (the file_complete method)?

comment:5 in reply to: ↑ 4 Changed 7 years ago by Giuliani Vito Ivan <giuliani.v@…>

  • Patch needs improvement set

Replying to Jan Rademaker <j.rademaker@gmail.com>:

Replying to kratorius:

I'm still trying to figure out what's happening since both at InMemoryUploadedFile and TemporaryUploadedFile there's a file.seek(0) in the __init__, but looks like if the file pointer gets changed somewhere before I can call .read().

Isn't this because the call to seek() missing from source:/django/trunk/django/core/files/uploadhandler.py#8046 (the file_complete method)?

No, it isn't. The .seek() call is made in InMemoryUploadedFile.__init__ instead of file_complete (anyway should this be moved for consistency sake?). Actually I'm also experiencing problems with my internet connection and accidentally lost my SVN copy of django, so I have to stop for a while on investigating on this issue.

Changed 7 years ago by kratorius

Fixed testcase

comment:6 Changed 7 years ago by kratorius

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

It turns out that this was an issue with the old admin (more precisely, validator_list was read()ing the whole file before passing the object to the validators), as with newforms-admin everything works as expected. I also fixed the testcase that was missing two calls to .seek() (and fooled me).

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