Deprecated send_file etag support and mimetype guessing for file-like objects. This fixes pallets#104

mitsuhiko · mitsuhiko · commit fda14678c07d · 2010-08-07T13:36:39.000+02:00
diff --git a/CHANGES b/CHANGES
@@ -13,6 +13,10 @@ Release date to be announced, codename to be selected
   behaviour for `OPTIONS` responses.
 - Unbound locals now raise a proper :exc:`RuntimeError` instead
   of an :exc:`AttributeError`.
+- Mimetype guessing and etag support based on file objects is now
+  deprecated for :func:`flask.send_file` because it was unreliable.
+  Pass filenames instead or attach your own etags and provide a
+  proper mimetype by hand.
 
 Version 0.6.1
 -------------
diff --git a/docs/upgrading.rst b/docs/upgrading.rst
@@ -27,6 +27,23 @@ raise a :exc:`RuntimeError` instead of an :exc:`AttributeError` when they
 are unbound.  If you cought these exceptions with :exc:`AttributeError`
 before, you should catch them with :exc:`RuntimeError` now.
 
+Additionally the :func:`~flask.send_file` function is now issuing
+deprecation warnings if you depend on functionality that will be removed
+in Flask 1.0.  Previously it was possible to use etags and mimetypes
+when file objects were passed.  This was unreliable and caused issues
+for a few setups.  If you get a deprecation warning, make sure to
+update your application to work with either filenames there or disable
+etag attaching and attach them yourself.
+
+Old code::
+
+    return send_file(my_file_object)
+    return send_file(my_file_object)
+
+New code::
+
+    return send_file(my_file_object, add_etags=False)
+
 Version 0.6
 -----------
 
diff --git a/flask/helpers.py b/flask/helpers.py
@@ -259,7 +259,9 @@ def send_file(filename_or_fp, mimetype=None, as_attachment=False,
 
     By default it will try to guess the mimetype for you, but you can
     also explicitly provide one.  For extra security you probably want
-    to sent certain files as attachment (HTML for instance).
+    to sent certain files as attachment (HTML for instance).  The mimetype
+    guessing requires a `filename` or an `attachment_filename` to be
+    provided.
 
     Please never pass filenames to this function from user sources without
     checking them first.  Something like this is usually sufficient to
@@ -274,6 +276,12 @@ def send_file(filename_or_fp, mimetype=None, as_attachment=False,
        The `add_etags`, `cache_timeout` and `conditional` parameters were
        added.  The default behaviour is now to attach etags.
 
+    .. versionchanged:: 0.7
+       mimetype guessing and etag support for file objects was
+       deprecated because it was unreliable.  Pass a filename if you are
+       able to, otherwise attach an etag yourself.  This functionality
+       will be removed in Flask 1.0
+
     :param filename_or_fp: the filename of the file to send.  This is
                            relative to the :attr:`~Flask.root_path` if a
                            relative path is specified.
@@ -295,8 +303,25 @@ def send_file(filename_or_fp, mimetype=None, as_attachment=False,
         filename = filename_or_fp
         file = None
     else:
+        from warnings import warn
         file = filename_or_fp
         filename = getattr(file, 'name', None)
+
+        # XXX: this behaviour is now deprecated because it was unreliable.
+        # removed in Flask 1.0
+        if not attachment_filename and not mimetype \
+           and isinstance(filename, basestring):
+            warn(DeprecationWarning('The filename support for file objects '
+                'passed to send_file is not deprecated.  Pass an '
+                'attach_filename if you want mimetypes to be guessed.'),
+                stacklevel=2)
+        if add_etags:
+            warn(DeprecationWarning('In future flask releases etags will no '
+                'longer be generated for file objects passed to the send_file '
+                'function because this behaviour was unreliable.  Pass '
+                'filenames instead if possible, otherwise attach an etag '
+                'yourself based on another value'), stacklevel=2)
+
     if filename is not None:
         if not os.path.isabs(filename):
             filename = os.path.join(current_app.root_path, filename)
diff --git a/tests/flask_tests.py b/tests/flask_tests.py
@@ -33,8 +33,27 @@
 SECRET_KEY = 'devkey'
 
 
+@contextmanager
+def catch_warnings():
+    """Catch warnings in a with block in a list"""
+    import warnings
+    filters = warnings.filters
+    warnings.filters = filters[:]
+    old_showwarning = warnings.showwarning
+    log = []
+    def showwarning(message, category, filename, lineno, file=None, line=None):
+        log.append(locals())
+    try:
+        warnings.showwarning = showwarning
+        yield log
+    finally:
+        warnings.filters = filters
+        warnings.showwarning = old_showwarning
+
+
 @contextmanager
 def catch_stderr():
+    """Catch stderr in a StringIO"""
     old_stderr = sys.stderr
     sys.stderr = rv = StringIO()
     try:
@@ -834,46 +853,64 @@ def test_send_file_xsendfile(self):
 
     def test_send_file_object(self):
         app = flask.Flask(__name__)
-        with app.test_request_context():
-            f = open(os.path.join(app.root_path, 'static/index.html'))
-            rv = flask.send_file(f)
-            with app.open_resource('static/index.html') as f:
-                assert rv.data == f.read()
-            assert rv.mimetype == 'text/html'
+        with catch_warnings() as captured:
+            with app.test_request_context():
+                f = open(os.path.join(app.root_path, 'static/index.html'))
+                rv = flask.send_file(f)
+                with app.open_resource('static/index.html') as f:
+                    assert rv.data == f.read()
+                assert rv.mimetype == 'text/html'
+            # mimetypes + etag
+            assert len(captured) == 2
 
         app.use_x_sendfile = True
-        with app.test_request_context():
-            f = open(os.path.join(app.root_path, 'static/index.html'))
-            rv = flask.send_file(f)
-            assert rv.mimetype == 'text/html'
-            assert 'x-sendfile' in rv.headers
-            assert rv.headers['x-sendfile'] == \
-                os.path.join(app.root_path, 'static/index.html')
+        with catch_warnings() as captured:
+            with app.test_request_context():
+                f = open(os.path.join(app.root_path, 'static/index.html'))
+                rv = flask.send_file(f)
+                assert rv.mimetype == 'text/html'
+                assert 'x-sendfile' in rv.headers
+                assert rv.headers['x-sendfile'] == \
+                    os.path.join(app.root_path, 'static/index.html')
+            # mimetypes + etag
+            assert len(captured) == 2
 
         app.use_x_sendfile = False
         with app.test_request_context():
-            f = StringIO('Test')
-            rv = flask.send_file(f)
-            assert rv.data == 'Test'
-            assert rv.mimetype == 'application/octet-stream'
-            f = StringIO('Test')
-            rv = flask.send_file(f, mimetype='text/plain')
-            assert rv.data == 'Test'
-            assert rv.mimetype == 'text/plain'
+            with catch_warnings() as captured:
+                f = StringIO('Test')
+                rv = flask.send_file(f)
+                assert rv.data == 'Test'
+                assert rv.mimetype == 'application/octet-stream'
+            # etags
+            assert len(captured) == 1
+            with catch_warnings() as captured:
+                f = StringIO('Test')
+                rv = flask.send_file(f, mimetype='text/plain')
+                assert rv.data == 'Test'
+                assert rv.mimetype == 'text/plain'
+            # etags
+            assert len(captured) == 1
 
         app.use_x_sendfile = True
-        with app.test_request_context():
-            f = StringIO('Test')
-            rv = flask.send_file(f)
-            assert 'x-sendfile' not in rv.headers
+        with catch_warnings() as captured:
+            with app.test_request_context():
+                f = StringIO('Test')
+                rv = flask.send_file(f)
+                assert 'x-sendfile' not in rv.headers
+            # etags
+            assert len(captured) == 1
 
     def test_attachment(self):
         app = flask.Flask(__name__)
-        with app.test_request_context():
-            f = open(os.path.join(app.root_path, 'static/index.html'))
-            rv = flask.send_file(f, as_attachment=True)
-            value, options = parse_options_header(rv.headers['Content-Disposition'])
-            assert value == 'attachment'
+        with catch_warnings() as captured:
+            with app.test_request_context():
+                f = open(os.path.join(app.root_path, 'static/index.html'))
+                rv = flask.send_file(f, as_attachment=True)
+                value, options = parse_options_header(rv.headers['Content-Disposition'])
+                assert value == 'attachment'
+            # mimetypes + etag
+            assert len(captured) == 2
 
         with app.test_request_context():
             assert options['filename'] == 'index.html'
@@ -884,7 +921,8 @@ def test_attachment(self):
 
         with app.test_request_context():
             rv = flask.send_file(StringIO('Test'), as_attachment=True,
-                                 attachment_filename='index.txt')
+                                 attachment_filename='index.txt',
+                                 add_etags=False)
             assert rv.mimetype == 'text/plain'
             value, options = parse_options_header(rv.headers['Content-Disposition'])
             assert value == 'attachment'
