Refs #43 - ObjectDownloadView.get_file() raises FileNotFound if file field does not exists or is empty. Introduced DownloadMixin.file_not_found_response().

django_downloadview/exceptions.py

@@ -0,0 +1,11 @@
+# -*- coding: utf-8 -*-
+"""Custom exceptions."""
+
+
+class FileNotFound(IOError):
+    """Requested file does not exist.
+
+    This exception is to be raised when operations (such as read) fail because
+    file does not exist (whatever the storage or location).
+
+    """

django_downloadview/tests/views.py

@@ -6,11 +6,13 @@ try:
 except ImportError:
     import mock
 
+from django.http import Http404
 from django.http.response import HttpResponseNotModified
 import django.test
 
+from django_downloadview import exceptions
 from django_downloadview.test import setup_view
-from django_downloadview.views import base
+from django_downloadview import views
 
 
 class DownloadMixinTestCase(unittest.TestCase):
@@ -21,13 +23,13 @@ class DownloadMixinTestCase(unittest.TestCase):
         Subclasses must implement it!
 
         """
-        mixin = base.DownloadMixin()
+        mixin = views.DownloadMixin()
         with self.assertRaises(NotImplementedError):
             mixin.get_file()
 
     def test_get_basename(self):
         """DownloadMixin.get_basename() returns basename attribute."""
-        mixin = base.DownloadMixin()
+        mixin = views.DownloadMixin()
         self.assertEqual(mixin.get_basename(), None)
         mixin.basename = 'fake'
         self.assertEqual(mixin.get_basename(), 'fake')
@@ -42,7 +44,7 @@ class DownloadMixinTestCase(unittest.TestCase):
         file_wrapper = mock.Mock()
         file_wrapper.was_modified_since = mock.Mock(
             return_value=mock.sentinel.was_modified)
-        mixin = base.DownloadMixin()
+        mixin = views.DownloadMixin()
         self.assertIs(
             mixin.was_modified_since(file_wrapper, mock.sentinel.since),
             mock.sentinel.was_modified)
@@ -66,7 +68,7 @@ class DownloadMixinTestCase(unittest.TestCase):
         file_wrapper.modified_time = mock.sentinel.modified_time
         was_modified_since_mock = mock.Mock(
             return_value=mock.sentinel.was_modified)
-        mixin = base.DownloadMixin()
+        mixin = views.DownloadMixin()
         with mock.patch('django_downloadview.views.base.was_modified_since',
                         new=was_modified_since_mock):
             self.assertIs(
@@ -97,20 +99,20 @@ class DownloadMixinTestCase(unittest.TestCase):
             side_effect=NotImplementedError)
         type(file_wrapper).modified_time = mock.PropertyMock(
             side_effect=NotImplementedError)
-        mixin = base.DownloadMixin()
+        mixin = views.DownloadMixin()
         self.assertIs(
             mixin.was_modified_since(file_wrapper, 'fake since'),
             True)
 
     def test_not_modified_response(self):
         "DownloadMixin.not_modified_response returns HttpResponseNotModified."
-        mixin = base.DownloadMixin()
+        mixin = views.DownloadMixin()
         response = mixin.not_modified_response()
         self.assertTrue(isinstance(response, HttpResponseNotModified))
 
     def test_download_response(self):
         "DownloadMixin.download_response() returns download response instance."
-        mixin = base.DownloadMixin()
+        mixin = views.DownloadMixin()
         mixin.file_instance = mock.sentinel.file_wrapper
         response_factory = mock.Mock(return_value=mock.sentinel.response)
         mixin.response_class = response_factory
@@ -126,7 +128,7 @@ class DownloadMixinTestCase(unittest.TestCase):
         """DownloadMixin.render_to_response() respects HTTP_IF_MODIFIED_SINCE
         header (calls ``not_modified_response()``)."""
         # Setup.
-        mixin = base.DownloadMixin()
+        mixin = views.DownloadMixin()
         mixin.request = django.test.RequestFactory().get(
             '/dummy-url',
             HTTP_IF_MODIFIED_SINCE=mock.sentinel.http_if_modified_since)
@@ -147,7 +149,7 @@ class DownloadMixinTestCase(unittest.TestCase):
     def test_render_to_response_modified(self):
         """DownloadMixin.render_to_response() calls download_response()."""
         # Setup.
-        mixin = base.DownloadMixin()
+        mixin = views.DownloadMixin()
         mixin.request = django.test.RequestFactory().get(
             '/dummy-url',
             HTTP_IF_MODIFIED_SINCE=None)
@@ -163,6 +165,24 @@ class DownloadMixinTestCase(unittest.TestCase):
         self.assertEqual(mixin.was_modified_since.call_count, 0)
         mixin.download_response.assert_called_once_with()
 
+    def test_render_to_response_file_not_found(self):
+        "DownloadMixin.render_to_response() calls file_not_found_response()."
+        # Setup.
+        mixin = views.DownloadMixin()
+        mixin.request = django.test.RequestFactory().get('/dummy-url')
+        mixin.get_file = mock.Mock(side_effect=exceptions.FileNotFound)
+        mixin.file_not_found_response = mock.Mock()
+        # Run.
+        mixin.render_to_response()
+        # Check.
+        mixin.file_not_found_response.assert_called_once_with()
+
+    def test_file_not_found_response(self):
+        """DownloadMixin.file_not_found_response() raises Http404."""
+        mixin = views.DownloadMixin()
+        with self.assertRaises(Http404):
+            mixin.file_not_found_response()
+
 
 class BaseDownloadViewTestCase(unittest.TestCase):
     "Tests around :class:`django_downloadviews.views.base.BaseDownloadView`."
@@ -171,9 +191,37 @@ class BaseDownloadViewTestCase(unittest.TestCase):
         request = django.test.RequestFactory().get('/dummy-url')
         args = ['dummy-arg']
         kwargs = {'dummy': 'kwarg'}
-        view = setup_view(base.BaseDownloadView(), request, *args, **kwargs)
+        view = setup_view(views.BaseDownloadView(), request, *args, **kwargs)
         view.render_to_response = mock.Mock(
             return_value=mock.sentinel.response)
         response = view.get(request, *args, **kwargs)
         self.assertIs(response, mock.sentinel.response)
         view.render_to_response.assert_called_once_with()
+
+
+class ObjectDownloadViewTestCase(unittest.TestCase):
+    "Tests for :class:`django_downloadviews.views.object.ObjectDownloadView`."
+    def test_get_file_ok(self):
+        "ObjectDownloadView.get_file() returns ``file`` field by default."
+        view = setup_view(views.ObjectDownloadView(), 'fake request')
+        view.object = mock.Mock(spec=['file'])
+        view.get_file()
+
+    def test_get_file_wrong_field(self):
+        """ObjectDownloadView.get_file() raises FileNotFound if field does not
+        exist."""
+        view = setup_view(views.ObjectDownloadView(file_field='other_field'),
+                          'fake request')
+        view.object = mock.Mock(spec=['file'])
+        with self.assertRaises(exceptions.FileNotFound):
+            view.get_file()
+
+    def test_get_file_empty_field(self):
+        """ObjectDownloadView.get_file() raises FileNotFound if field does not
+        exist."""
+        view = setup_view(views.ObjectDownloadView(file_field='other_field'),
+                          'fake request')
+        view.object = mock.Mock()
+        view.object.other_field = None
+        with self.assertRaises(exceptions.FileNotFound):
+            view.get_file()

django_downloadview/views/base.py

@@ -1,10 +1,11 @@
 # -*- coding: utf-8 -*-
 """Base material for download views: :class:`DownloadMixin` and
 :class:`BaseDownloadView`"""
-from django.http import HttpResponseNotModified
+from django.http import HttpResponseNotModified, Http404
 from django.views.generic.base import View
 from django.views.static import was_modified_since
 
+from django_downloadview import exceptions
 from django_downloadview.response import DownloadResponse
 
 
@@ -33,7 +34,12 @@ class DownloadMixin(object):
     basename = None
 
     def get_file(self):
-        """Return a file wrapper instance."""
+        """Return a file wrapper instance.
+
+        Raises :class:`~django_downloadview.exceptions.FileNotFound` if file
+        does not exist.
+
+        """
         raise NotImplementedError()
 
     def get_basename(self):
@@ -78,8 +84,14 @@ class DownloadMixin(object):
         response = self.response_class(*response_args, **response_kwargs)
         return response
 
+    def file_not_found_response(self):
+        """Raise Http404."""
+        raise Http404()
+
     def render_to_response(self, *response_args, **response_kwargs):
-        """Return "download" response.
+        """Return "download" response (if everything is ok).
+
+        Return :meth:`file_not_found_response` if file does not exist.
 
         Respects the "HTTP_IF_MODIFIED_SINCE" header if any. In that case, uses
         :py:meth:`was_modified_since` and :py:meth:`not_modified_response`.
@@ -87,7 +99,10 @@ class DownloadMixin(object):
         Else, uses :py:meth:`download_response` to return a download response.
 
         """
-        self.file_instance = self.get_file()
+        try:
+            self.file_instance = self.get_file()
+        except exceptions.FileNotFound:
+            return self.file_not_found_response()
         # Respect the If-Modified-Since header.
         since = self.request.META.get('HTTP_IF_MODIFIED_SINCE', None)
         if since is not None:

django_downloadview/views/object.py

@@ -2,25 +2,33 @@
 """Stream files that live in models."""
 from django.views.generic.detail import SingleObjectMixin
 
+from django_downloadview.exceptions import FileNotFound
 from django_downloadview.views.base import BaseDownloadView
 
 
 class ObjectDownloadView(SingleObjectMixin, BaseDownloadView):
     """Serve file fields from models.
 
-    This class extends BaseDetailView, so you can use its arguments to target
-    the instance to operate on: slug, slug_kwarg, model, queryset...
-    See Django's DetailView reference for details.
+    This class extends :class:`~django.views.generic.detail.SingleObjectMixin`,
+    so you can use its arguments to target the instance to operate on:
+    ``slug``, ``slug_kwarg``, ``model``, ``queryset``...
 
-    In addition to BaseDetailView arguments, you can set arguments related to
-    the file to be downloaded.
+    In addition to :class:`~django.views.generic.detail.SingleObjectMixin`
+    arguments, you can set arguments related to the file to be downloaded:
 
-    The main one is ``file_field``.
+    * :attr:`file_field`;
+    * :attr:`basename_field`;
+    * :attr:`encoding_field`;
+    * :attr:`mime_type_field`;
+    * :attr:`charset_field`;
+    * :attr:`modification_time_field`;
+    * :attr:`size_field`.
 
-    The other arguments are provided for convenience, in case your model holds
-    some (deserialized) metadata about the file, such as its basename, its
-    modification time, its MIME type... These fields may be particularly handy
-    if your file storage is not the local filesystem.
+    :attr:`file_field` is the main one. Other arguments are provided for
+    convenience, in case your model holds some (deserialized) metadata about
+    the file, such as its basename, its modification time, its MIME type...
+    These fields may be particularly handy if your file storage is not the
+    local filesystem.
 
     """
     #: Name of the model's attribute which contains the file to be streamed.
@@ -58,7 +66,18 @@ class ObjectDownloadView(SingleObjectMixin, BaseDownloadView):
         :attr:`size` are configured.
 
         """
-        file_instance = getattr(self.object, self.file_field)
+        try:
+            file_instance = getattr(self.object, self.file_field)
+        except AttributeError:
+            raise FileNotFound('Failed to retrieve file from field="{field}" '
+                               'on object="{object}"'.format(
+                                   field=self.file_field,
+                                   object=self.object))
+        if not file_instance:
+            raise FileNotFound('Field="{field}" on object="{object}" is '
+                               'empty'.format(
+                                   field=self.file_field,
+                                   object=self.object))
         for field in ('encoding', 'mime_type', 'charset', 'modification_time',
                       'size'):
             model_field = getattr(self, '%s_field' % field, False)