jazzband / django-downloadview Goto Github PK

See #57: by default, files should be opened in binary mode by default.
What about having a "mode" option which lets users change the open() mode?

Demo project is cool, but it is not as readable as the documentation.
Examples in documentation are hard to test, because they need a Django project setup.
=> In documentation, include examples from demo project.
=> In demo project, write tests
=> Synchronize documentation and demo scopes, i.e. examples in documentation are in the demo, and code in the demo is in documentation.

Scenario:

• generate a file with Django (as an example, some ZIP file)
• save it to disk
• let server stream it (X-Accel for nginx)
• remove the file when it's done

Is it possible to do it with django-downloadview?

Is it recommended to do it with django-downloadview? i.e. should Django stream file contents directly and write nothing to disk? Maybe there are 2 cases: when the file can be generated with some iterator (as a CSV file) VS when the file is generated in a single operation (as a zip archive).

Another use case: file is stored in a storage that nginx can't access. Django could copy file to a temporary location and let nginx serve it. Or maybe the best practice is: "allow nginx to access file storage directly".

=> at least add documentation
=> if a smart solution can be implemented, add it to django-downloadview

See https://github.com/johnsensible/django-sendfile
At least add a reference in docs/about/alternatives.txt
Maybe pull/request features.

=> Make sure PyPI can render README as HTML

Hi all,

I need to implement file delivery via Apache and mod_xsendfile for a customer and would be happy to use django-downloadview and contribute my code. In order to do so, I have some questions:

1. I do not have files stored in models or storage, but want to deliver them from the file path. I.e. I would need to use PathDownloadView. I assume that is correct?
2. Looking at the code of PathDownloadView, it seems like it returns the a file object directly. Correct?
3. So I am wondering, will any response from PathDownloadView by catched by the current X-Accel-Middleware? From my understanding not. So what would I need to do in general to make that happen?
4. And now regarding Apache: Would you recommend to create a new Middleware class that catches the response and returns a Apache-Xsendfile response or would you recommend to adapt the current X-Accel-Middleware to be more generic such that it can be configured to return either a X-Accel-Response or a Xsendfile-Response class?

Thanks
schacki

• Create/Edit a comparison grid with features
• Add projects to the grid (see docs/about/alternatives)
• Update docs/about/alternatives

=> Simplified imports.
import django_downloadview is enough to get most of the API.
import django_downloadview.nginx to get specific stuff for nginx.

Given there are poor chances one wants optimizations for several servers at once (i.e. return Nginx's X-Accel for some views and Lighttp's X-Sendfile for others), one "backend" should be enough. As an example, we should need only one "download middleware" instance, which gets a backend as argument.

As a comparison, django-sendfile uses a unique (and global) backend, then the sendfile() function uses the backend.

=> For most use cases, we shouldn't have to care about the backend when applying optimizations. Except in configuration where we explicitely setup the backend.

Maybe replace "global" by "default" backend, so that override is possible.

Currently, documentation is a bit obscure. It is based on generated API documentation.
As a new user, documentation does not help enough. It does not provide enough examples.
As an advanced user, reading code is faster.

=> Help new users: make the documentation follow the use case, from installation to testing via configuring views.
=> Help advanced users: make the documentation an easy-to-browse API reminder. Clean table of contents, many simple examples.

The "use case" table of contents:

• overview, concepts
• install
• configure: settings
• setup views (one page per view)
• optimize streaming
• write tests
• write healthchecks

Additional "internals" section can be really helpful:

• responses
• file wrappers (both Django's and django-downloadview's)
• middlewares
• decorators

See also #25.
This ticket focuses on multiple configurations (source to destination "rewrite" rules) capability.
As of version 1.2, django-downloadview supports only one download middleware with global configuration. It is a pain to configure.

This feature may be implemented as follow:

• "global backend" is configured in middlewares, as DOWNLOADVIEW_MIDDLEWARES items. This is not a single backend, but a set of backends. At least they are registered in a single location: settings.
• the recommended practice (in documentation) is to use the global middlewares.

=> DOWNLOADVIEW_MIDDLEWARES makes most use cases easy.
=> Custom decorators allow fine-tuning for specific needs.

DownloadMixin reads file size to respect IF_MODIFIED_SINCE header, but for VirtualFiles, this may trigger file generation and load it all in memory, which is bad.

When dealing with generated files, if the file has a modified_time, use it ; else suppose that the file is new, no matter its size.

Let's consider the following code:

class CategoryIconDisplay(ObjectDownloadView):
    model = Category
    file_field = 'icon'

ObjectDownloadView return a "404" when the model is missing. Fine.

But what happens when the "icon" field is empty?

if category.icon returns <FieldFile: None>

At the moment, it fails miserably and throws an exception:

raise ValueError("The '%s' attribute has no file associated with it." % self.field.name)
   ValueError: The 'icon' attribute has no file associated with it.

Maybe we could:

• throw a 404 error,
• throw a specific exception that developers could catch and handle by themselves.

See http://pypi.python.org/pypi/django-private-files
At least add a reference in docs/about/alternatives.txt
Maybe pull/request features.

In https://code.djangoproject.com/ticket/21321, @claudep said:

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.

=> In https://github.com/benoitbryon/django-downloadview/blob/c32313ca8e1e008dc8b3758724d4da19cb38358a/django_downloadview/views/path.py#L33:

- return File(open(self.get_path()))
+ return File(open(self.get_path(), 'rb'))

See https://docs.djangoproject.com/en/1.5/ref/files/file/#the-contentfile-class
Looks like django-downloadview's files.VirtualFile could be replaced by Django's ContentFile.

See also #46.

• Do not run tests on Python 2.6
• Remove Python 2.6 from system requirements (INSTALL, docs/dev.txt, ...)

See also #36 and #2.
All views/file wrappers are not compatible with all optimizations:

• Nginx, Apache and Lighttpd cannot handle generated files (they do not live on disk) (and there is currently no "write a temporary file" feature)
• Apache and Lighttpd can only handle files on local filesystem (absolute path), whereas nginx uses URLs for internal redirect.

As of version 1.2, ObjectDownloadView inherits from DownloadMixin and BaseDetailView.
But BaseDownloadView.get() implementation fits better than BaseDetailView.get().
As an example, in download views, we do not need to run get_context_data().

In current implementation, views populate the response attributes.
But, if some optimizations are configured, the response may be captured and altered.
As the file content, response attributes shouldn't be loaded in the view, whenever possible.

response['Content-Disposition'] = 'attachment; filename=*=UTF-8\'\'%s' % urllib.quote(filename.encode('utf-8')) ?

Provide utilities to create models instances with files for tests.

Maybe provide a set of test fixtures (pdf, zip, text...).

Wonder if it could be based on factory_boy.

See http://wiki.nginx.org/X-accel and http://wiki.nginx.org/XSendfile

Here is a raw implementation, to give an idea...

def XAccelRedirectResponse(url, basename=None):
    """Return a HttpResponse with header for Nginx X-Accel-Redirect."""
    response = HttpResponse()
    basename = basename or url.split('/')[-1]
    response['X-Accel-Redirect'] = url
    response['Content-Type'] = ""
    response['Content-Disposition'] = 'attachment; filename=%s' % basename
    response['Expires'] = datetime.today() + timedelta(days=1)
    return response

The response class may support all headers supported by Nginx.

https://github.com/benoitbryon/django-downloadview/blob/master/django_downloadview/response.py#L46

This line take a file_instance but if you use response = self.client.post and try to read the response.content you get a ValueError: I/O cannot read a closed file.

We need to change that so that even if we close the file we can still read it in our test.

http://redmine.lighttpd.net/projects/1/wiki/Docs_ModFastCGI#X-Sendfile

If the HTTP client needs to resume a download it will send a HTTP header called "Range"
In case of a download resume (for instance when using wget -c), the header will be like "bytes=2375680-"
See RFC2616 section 14.35.2 http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.35.2

And

send the "X-Sendfile2: filename range" header (available since 1.4.24)
The filename has to be urlencoded (including encoding ',' as '%2c': str_replace(',', '%2c', urlencode($path));)
Range has the form "start-end" (inclusive end) or "start-" (from start until file end); you must always specify the range, use "0-" for the complete file.
Example: X-Sendfile2: /tmp/test.txt 0-499 sends the first 500 bytes from /tmp/test.txt.
You can send this header more than once with different parameters (or you can combine single values with ", " between them).

As Django's generic views return TemplateResponse, DownloadView (or DownloadMixin) could return some DownloadResponse:

• DownloadResponse stores minimal information about the file to download
• Middlewares are given the opportunity to handle the response. Could be useful to implement #1 (nginx) and #2 (lighttpd).
• A fallback middleware serves the file with Python/Django, i.e. most of current DownloadMixin.render_to_response() logic would be moved to a middleware.

In download views, we want to instanciate a download response.
We currently compute file attributes (size, name, ...) and pass it to the response constructor.
The more attributes (see #21), the more the response class gets complicated.
Moreover we would like these attributes (see #22) to be lazy.
We could implement some methods that support storages (and thus, FileField and ImageField).
But then, what if someone wants to change the behaviour? He would have to override the response class.

The response class should have some "file" (or whatever the name) attribute, and handle it via some API. Could sound like "the response's file attribute is an object with url, filename, basename, size... attributes".
Then it would make it possible to have various implementations for this "file wrapper". One would use Django storages.

Could look like django.db.models.fields.files.FieldFile, but not tied to models.

Could help #5, #21 and #22.

List of attributes for the file wrapper:

• name: absolute filename in filesystem
• url: URL where file contents live
• size: in bytes
• mime_type
• encoding
• charset
• modification_time
• content: iterator over file contents

Are these attributes needed?

• media_root: typically storage's location
• relative_filename: filename relative to media_root
• is_virtual: True if the file isn't on some disk, i.e. Django/Python is to compute file contents. Should be True if filename (and maybe URL too) is empty. Could also be "is_persistent", i.e. will the file live after Django processed it.

https://github.com/benoitbryon/django-downloadview/blob/b7f660c5e3f37d918b106b02c5af7a887acc0111/django_downloadview/test.py#L87
Checking response.streaming_content should be enough.

Moreover, when files are generated, we cannot read the file contents twice, since generated files does not support seek() operation.

In INSTALL, show the versions used in test environment, i.e. known good set of versions.

Add base classes to django_downloadview.middlewares which make it easier to create backend specific middlewares.

Implement sendfile function that mimics django-sendfile's signature.
Use a StorageDownloadView with FileSystemStorage and settings.SENDFILE_ROOT.
Write a "migration guide" in documentation

Optionally, add some middleware which reads settings.SENDFILE_BACKEND, settings.SENDFILE_ROOT and settings.SENDFILE_URL and behave like the corresponding django-downloadview's middleware. Should also raise some "DeprecationWarning".

It checks response.basename instead, twice: https://github.com/benoitbryon/django-downloadview/blob/b7f660c5e3f37d918b106b02c5af7a887acc0111/django_downloadview/test.py#L75-78

Related to #44:

• #57 should fix PathDownloadView
• this ticket should fix VirtualDownloadView

As I understood @claudep notes about File: File is not intended to support text-mode files (only binary ones). So move the text-mode support to VirtualFile. ContentFile does something similar.

Use case: Django is to fetch the file via an URL. In current implementation, Django would have to fetch and read the file. But proxy servers may have direct access to the resource, i.e. they could fetch the file themselves => improved resources usage.

As a developer, in order to test download views, I want to use utility functions that implement common patterns.

As an example, the django_downloadview.tests.temporary_media_root function could be made generic: use a temporary directory and optionally override some setting (not only MEDIA_ROOT)

Given storage and filename (or FileField/FieldFile), use storage's size() method instead of os.path.get_size() or os.stat.

See https://github.com/lincolnloop/django-protected-files
At least add a reference in docs/about/alternatives.txt
Maybe pull/request features.

As a developer, in order to test views I configure with django-downloadview, I want to use short assertions in tests.

Example:

• for URL "/download/", I registered some download_view
• and I enabled nginx X-Accel optimisations
• I want to make sure it works as expected
• so I write a (short) test.

Test may look like this:

from django.test import TestCase
from django_downloadview.nginx import assert_download_response

class DownloadTestCase(TestCase):
    def test_get_download(self):
        response = self.client.get('/download')
        assert_download_response(self, response,basename=u"hello-world.txt", redirect_url="/proxied-download/example/hello-world.txt")

=> Add some "assert_download_response(test_case, **assertions)" function.

New in Django 1.5: https://docs.djangoproject.com/en/dev/ref/request-response/#django.http.StreamingHttpResponse

• StreamingHttpResponses are to be used when file content is generated! Not when full file content lives somewhere (in memory or on disk).
• Prepare arrival of StreamingHttpResponse (kind of backport).
• When django-1.5 is out, use remove StreamingHttpResponse implementation from django-downloadview and use the builtin.

As a relatively new django/python programmer, I am attempting to use downloadview in my project. I set up a test case from the demo, which works in python2.7, but not in python3.3. The tests are with django 1.5.4 (had similar results with 1.6 and 1.7). The results seem to suggest a problem with django, rather than downloadview, but I wanted to check here first.

In attempting to use download_hello_world=views.PathDownloadView.as_view(path=hello_world_path)

I got the following error

Traceback (most recent call last): File "/usr/lib64/python3.3/wsgiref/handlers.py", line 138, in run self.finish_response() File "/usr/lib64/python3.3/wsgiref/handlers.py", line 178, in finish_response for data in self.result: File "/home/jeremiah/.virtualenvs/auto_p3_d15/lib/python3.3/site-packages/django/http/response.py", line 223, in __next__ return self.make_bytes(next(self._iterator)) File "/home/jeremiah/.virtualenvs/auto_p3_d15/lib/python3.3/site-packages/django/core/files/base.py", line 97, in __iter__ chunk_buffer = BytesIO(chunk) TypeError: 'str' does not support the buffer interface [23/Oct/2013 22:34:13] "GET /download/hello-world.txt HTTP/1.1" 500 59

So I added a number of print() statements to downloadview to determine the point of failure, but it seems the problem is not in downloadview, but in django. So in dango/core/files/base.py::Files::__iter__() I converted
chunk_buffer = BytesIO(chunk)
to
chunk_buffer = BytesIO(str.encode(chunk))

And now PathDownloadView works with python 3 and 2.

Before filing a bug in django, I wanted to check if this would be the proper fix. It seems unlikely I've discovered such a bug in django.

Thanks.

https://pypi.python.org/pypi/django-transfer

Currently (version 1.2), StorageDownloadView inherits PathDownloadView.
Should be: StorageDownloadView makes it easy to PathDownloadView.

• Add get_storage(), get_storage_class and get_storage_kwargs() methods to StorageDownloadView, i.e. make the storage overridable.
• By default, get_storage() uses Django's get_storage_class(settings.DEFAULT_STORAGE).
• In documentation, explain that (deprecated) PathDownloadView is a StorageDownloadView with storage=FileSystemStorage(location=CUSTOM_LOCATION)
• Deprecate PathDownloadView.
• Optionally setup a backward-compatible PathDownloadView which inherits StorageDownloadView and uses FileSystemStorage as a default.

Requires an example!

By the way, make sure it works as expected :)

Major feature request: Python 3 support!

• Setup development environment to support concurrent Python versions, i.e. make it easy for developers to run code with python 2.7 or Python 3.3
• Optionally drop support of Python 2.6 (setup.py, INSTALL, .travis.yml)
• Update setup.py's classifiers
• Update TravisCI configuration
• Update code of course! Make it Python 3 compatible.
• Update documentation: mostly INSTALL.

See http://redmine.lighttpd.net/projects/lighttpd/wiki/X-LIGHTTPD-send-file
Similar feature as #1, but for Lighttpd.

Use case:

• We have some remote file Django can download (perhaps the client cannot)
• Without optimizations, Django is a proxy to download/stream the file
• With optimizations, nginx/lighttpd/apache... proxies the file directly

The URLDownloadView may:

• have a get_url() method
• use some virtual file wrapper which has an url property
• use requests to stream the file

Note: only simple GET requests are supported there. Other methods/options could be supported in another (more complex) generic view.

django_downloadview.response.DownloadResponse has some attachment attribute.
Download views have one too.
Without nginx, download views can stream files "inline", not as attachment.
But XAccelRedirectResponse does not take care of it and nginx always streams files as attachments.

There may be tickets on djangoproject.com about static files and server-specific optimizations (x-sendfile and co).

=> search for related tickets on djangoproject.com
=> create a new one to ask whether django-downloadview features could be merged in django.contrib.staticfiles

A common use case is to download multiple files as an archive (zip, gzip, tar.gz...).

Like for single file download, server-side features exist. See http://wiki.nginx.org/NgxZip for example.

=> if it is in django-downloadview scope, implement it.