Opened 8 years ago

Closed 18 months ago

#8149 closed Cleanup/optimization (fixed)

UploadedFile doesn't iterate over lines with \r line endings

Reported by: mrmachine Owned by: nobody
Component: File uploads/storage Version: master
Severity: Normal Keywords: UploadedFile
Cc: real.human@…, timograham@…, jon.dufresne@… 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

UploadedFile.__iter__ loops over chunks and converts each chunk to a StringIO object and then iterates over that. Iterating over StringIO objects only splits on \n, and I'm not sure why we need StringIO at all. Couldn't we just do for line in chunk.splitlines() (which does split on \r and \n).

Attachments (2)

UploadedFile-r8225.diff (593 bytes) - added by mrmachine 8 years ago.
fix iteration of UploadedFile objects on newlines.
UploadedFile-r8226.diff (711 bytes) - added by mrmachine 8 years ago.
Updated for r8654 and fixed regular expression to not match empty strings.

Download all attachments as: .zip

Change History (20)

Changed 8 years ago by mrmachine

fix iteration of UploadedFile objects on newlines.

comment:1 Changed 8 years ago by mrmachine

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 8 years ago by mrmachine

  • Cc real.human@… added

comment:3 Changed 8 years ago by mrmachine

Looks like using splitlines will strip the line endings and break binary uploads. Here's a new patch that iterates through a call to re.findall with a simple regular expression instead of a StringIO object.

comment:4 Changed 8 years ago by mrmachine

Here's a use-case for this change. Upload a CSV file (which can have \r or \r\n line endings), and pass the UploadedFile object to csv.reader and you get Error: newline inside string when you try to iterate over it. When working with real files on the local file system you can open them with universal newline support, which is not available with StringIO or cStringIO objects.

comment:5 Changed 8 years ago by ericholscher

  • Triage Stage changed from Unreviewed to Accepted

Changed 8 years ago by mrmachine

Updated for r8654 and fixed regular expression to not match empty strings.

comment:6 Changed 8 years ago by mtredinnick

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

Universal newline handling only makes sense for files generated/stored on the local system, where the newline convention is known. The UploadedFile class, pretty much by definition, is for data that has come from, and been generated on, a remote system that we know nothing about. So trying to use the appropriate line-ending handling is an unattainable goal for that situation. #8656 is the ticket to make sure this is documented.

comment:7 Changed 5 years ago by jacob

  • milestone 1.0 deleted

Milestone 1.0 deleted

comment:8 Changed 4 years ago by mrmachine

  • Component changed from Forms to File uploads/storage
  • Easy pickings unset
  • Resolution wontfix deleted
  • Severity set to Normal
  • Status changed from closed to reopened
  • Type set to Uncategorized
  • UI/UX unset

I'm re-opening this ticket because I have found a much simpler implementation, and this is still a problem for anyone who needs to parse uploaded text files line-by-line (e.g. CSV) when they can't know what line terminators end-users are using in their files. Excel 2011 on OS X is at least one commonly used program that still saves CSV data with \r line terminators by default.

Originally, there was a discussion between Malcolm and Jacob on IRC and the strongest objection at the time to fixing this was the ugly regular expression in my previous patch. This was also during a busy time, and it was decided that this should be a documentation issue.

I don't think the explanation given above when this ticket was wontfixed actually makes much sense. Uploaded file data which has come from and been generated on a remote system that we know nothing about is precisely the reason why we *need* universal newline support when iterating those files line-by-line.

I'm not sure how I missed it before, but when I said that we couldn't just use splitlines() because it consumed the line terminators, it seems I was wrong. splitlines() has a keepends argument that will actually do exactly what we need without any ugly regular expressions. Perhaps this wasn't the case back then, when we still had to support Python 2.3 and 2.4. I couldn't find confirmation in the Python 2.3 and 2.4 docs, but I did confirm that splitlines(True) works in Python 2.5 which is now our minimum version number.

I've fixed this in a branch on GitHub, and opened a pull request. All tests pass under OS X Lion with Python 2.7.2 and SQLite.

https://github.com/thirstydigital/django/tree/tickets/8149-universal-newline-uploads

https://github.com/django/django/pull/320

comment:9 Changed 4 years ago by claudep

  • Needs tests set

This __iter__ method appears to be untested.

Last edited 4 years ago by claudep (previous) (diff)

comment:10 Changed 4 years ago by mrmachine

Sorry. Don't know why I thought this didn't need tests. New pull request with test.

https://github.com/django/django/pull/323

comment:11 Changed 3 years ago by mrmachine

Updated versionchanged note to 1.6. Any feedback?

comment:12 Changed 3 years ago by aaugustin

  • Type changed from Uncategorized to Cleanup/optimization

comment:13 Changed 3 years ago by aaugustin

  • Status changed from reopened to new

comment:14 Changed 3 years ago by timo

  • Cc timograham@… added
  • Needs tests unset
  • Patch needs improvement set

The test doesn't pass on Python 3.

comment:15 Changed 19 months ago by jdufresne

  • Cc jon.dufresne@… added

I've been running into this issue as well. Most notably when parsing uploaded files as CSV from Mac.

I added a submitted a pull request that works with Python 2 and 3: https://github.com/django/django/pull/3291

comment:16 Changed 19 months ago by jdufresne

  • Patch needs improvement unset

Sorry in advance if this is incorrect usage of this flag.

I am removing "Patch needs improvement" as my latest pull request does work with Python 3 and there are no outstanding requests from a review.

comment:17 Changed 18 months ago by timgraham

  • Triage Stage changed from Accepted to Ready for checkin

@jdufresne - that's correct usage. In fact, since tchaumeny reviewed it, he could mark the ticket as "ready for checkin".

comment:18 Changed 18 months ago by Tim Graham <timograham@…>

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

In eb4f6de980c5148ba48d4ed67f31cca27dd132a8:

Fixed #8149 -- Made File.iter() support universal newlines.

The following are recognized as ending a line: the Unix end-of-line
convention '\n', the Windows convention '\r\n', and the old
Macintosh convention '\r'.

http://www.python.org/dev/peps/pep-0278

Thanks tchaumeny for review.

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