[Django] #21321: File.__iter__().chunk python3 compatibility
Django
----------------------------------+---------------------
Reporter: jeremiahsavage@… | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: 1.5
Severity: Normal | Keywords: python3
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------+---------------------
In testing django-downloadview for a project, I found one of it's classes,
`PathDownloadView` worked with python2.7, but not with python3.3. I've
made the following change to `django/core/files/base.py` in
`Files.__iter()` and now I am able to use `PathDownloadView` and
`StorageDownloadView`.
from
{{{
chunk_buffer = BytesIO(chunk)
}}}
to
{{{
if type(chunk) is str:
chunk_buffer = BytesIO(str.encode(chunk))
elif type(chunk) is bytes:
chunk_buffer = BytesIO(chunk)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21321>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
----------------------------------+------------------------------------
Reporter: jeremiahsavage@… | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: 1.5
Keywords: python3 | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timo):
* needs_better_patch: => 0
* needs_docs: => 0
* needs_tests: => 0
* stage: Unreviewed => Accepted
Comment:
If you're able to offer a patch along with a test, that will expedite us
being able to fix this, thanks!
--
Ticket URL: <https://code.djangoproject.com/ticket/21321#comment:1>
Django
----------------------------------+------------------------------------
Type: Bug | Status: new
Component: Core (Other) | Version: 1.5
Keywords: python3 | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by benoitbryon):
In Python 2.7, if "chunk" is unicode, we get the same kind of error.
What about the following changes?
{{{
- chunk_buffer = BytesIO(chunk)
+ chunk_buffer = BytesIO(force_bytes(chunk))
}}}
... where force_bytes is already imported from django.utils.encoding and
used in ContentFile.
--
Ticket URL: <https://code.djangoproject.com/ticket/21321#comment:2>
Django
----------------------------------+------------------------------------
Type: Bug | Status: new
Component: Core (Other) | Version: 1.5
Keywords: python3 | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by daftshady):
Using `force_bytes` looks good.
I will attach a test code on it.
--
Ticket URL: <https://code.djangoproject.com/ticket/21321#comment:3>
Django
----------------------------------+------------------------------------
Type: Bug | Status: new
Component: Core (Other) | Version: 1.5
Keywords: python3 | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by claudep):
Until now, the `File` wrapper has always assumed the underlying file
object was byte-oriented (in the sense of `io.FileIO` of the standard
lib).
It must not absolutely remain so, but then isn't this report more about
the new feature "Support text-oriented I/O in Django File wrapper"?
--
Ticket URL: <https://code.djangoproject.com/ticket/21321#comment:4>
Django
----------------------------------+------------------------------------
Type: Bug | Status: new
Component: Core (Other) | Version: 1.5
Keywords: python3 | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by daftshady):
Attached pull request https://github.com/django/django/pull/1808
temporally fixing that errors.
--
Ticket URL: <https://code.djangoproject.com/ticket/21321#comment:5>
Django
----------------------------------+------------------------------------
Type: Bug | Status: new
Component: Core (Other) | Version: 1.5
Keywords: python3 | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by claudep):
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
Comment:
I think you missed the point. The issue is not about getting automagically
decoded text by iterating a file, but supporting a text-oriented
underlying file object.
--
Ticket URL: <https://code.djangoproject.com/ticket/21321#comment:6>
Django
----------------------------------+------------------------------------
Type: Bug | Status: new
Component: Core (Other) | Version: 1.5
Keywords: python3 | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by vajrasky):
Attached the PR: https://github.com/django/django/pull/1814 fixing (I
hope) this issue. Tested with Python 3.3, 3.4, 2.7.
Test with Python2.7 may need to be improved. I update the PR later while
waiting for the confirmation about whether this PR is the correct way to
solve the problem.
--
Ticket URL: <https://code.djangoproject.com/ticket/21321#comment:7>
Django
----------------------------------+------------------------------------
Type: Bug | Status: new
Component: Core (Other) | Version: 1.5
Keywords: python3 | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by vajrasky):
Okay, fixed the test with Python2.7 (Same PR).
https://github.com/django/django/pull/1814
It is so cumbersome to make both python2 and python3 happy. >.<
--
Ticket URL: <https://code.djangoproject.com/ticket/21321#comment:8>
Django
----------------------------------+------------------------------------
Type: New feature | Status: new
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: python3 | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by claudep):
* version: 1.5 => master
* type: Bug => New feature
Comment:
Note that about the OP use case, it's actually a bug in django-
downloadview. As the file type is not known in `PathDownloadView`, binary
mode should be explicitly specified in the `open()` call.
--
Ticket URL: <https://code.djangoproject.com/ticket/21321#comment:9>
Django
----------------------------------+------------------------------------
Type: New feature | Status: new
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: python3 | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by benoitbryon):
> Note that about the OP use case, it's actually a bug in django-
downloadview. As the file type is not known in PathDownloadView, binary
mode should be explicitly specified in the open() call.
Created https://github.com/benoitbryon/django-downloadview/issues/57 about
binary VS text mode.
--
Ticket URL: <https://code.djangoproject.com/ticket/21321#comment:10>
Django
----------------------------------+------------------------------------
Type: New feature | Status: new
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: python3 | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by benoitbryon):
> In Python 2.7, if "chunk" is unicode, we get the same kind of error.
Here is a simple example to reproduce the story...
With "str" it works fine:
{{{
>>> from StringIO import StringIO
>>> from django.core.files import File
>>> str_file = StringIO('Hello world')
>>> str_file_wrapper = File(str_file)
>>> [chunk for chunk in str_file_wrapper]
['Hello world']
}}}
But with unicode ValueError is raised:
{{{
>>> unicode_file = StringIO(u'Hello world')
>>> unicode_file_wrapper = File(unicode_file)
>>> [chunk for chunk in unicode_file_wrapper]
Traceback (most recent call last):
...
File "/mnt/data/web/django-
downloadview/lib/buildout/eggs/Django-1.5-py2.7.egg/django/core/files/base.py",
line 97, in __iter__
chunk_buffer = BytesIO(chunk)
TypeError: 'unicode' does not have the buffer interface
}}}
Is the use case valid? Is File intended to support such file objects?
In django-downloadview, I use something similar to support generators
(files that are generated via "yield" statement). Having "yield u'Hello
world'" is valid, isn't it?
Should I use another file wrapper that inherits File? Something similar to
ContentFile, but for generators?
--
Ticket URL: <https://code.djangoproject.com/ticket/21321#comment:11>
Django
----------------------------------+------------------------------------
Type: New feature | Status: new
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: python3 | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by benoitbryon):
Note: I just released an update of django-downloadview where:
* files on local filesystem are opened using binary mode by default
* VirtualFile (generated files, StringIO) overrides `__iter__()`
implementation to use `force_bytes()`
=> It looks like django-downloadview works even if Django's File is byte-
oriented.
So, I cannot tell if the feature request / bug report is still valid.
Any idea jeremiahsavage?
--
Ticket URL: <https://code.djangoproject.com/ticket/21321#comment:12>
Django
----------------------------------+------------------------------------
Type: New feature | Status: closed
Component: Core (Other) | Version: master
Keywords: python3 | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by claudep):
* status: new => closed
* resolution: => wontfix
Comment:
I would say that we will keep the Django File interface byte-oriented for
now. If we find more use cases which are harder to workaround, we might
reconsider this.
--
Ticket URL: <https://code.djangoproject.com/ticket/21321#comment:13>