Merge pull request #1155 from twisted/9655-twisted-web-tempfile-parameter

Parameterized Request content file

Author: exarkun
Reviewer: hawkowl
Fixes: ticket:9655

src/twisted/web/http.py

@@ -659,6 +659,17 @@ NO_BODY_CODES = (204, 304)
 _QUEUED_SENTINEL = object()
 
 
+
+def _getContentFile(length):
+    """
+    Get a writeable file-like object to which request content can be written.
+    """
+    if length is not None and length < 100000:
+        return StringIO()
+    return tempfile.TemporaryFile()
+
+
+
 @implementer(interfaces.IConsumer,
              _IDeprecatedHTTPChannelToRequestInterface)
 class Request:
@@ -812,10 +823,7 @@ class Request:
             request headers.  L{None} if the request headers do not indicate a
             length.
         """
-        if length is not None and length < 100000:
-            self.content = StringIO()
-        else:
-            self.content = tempfile.TemporaryFile()
+        self.content = _getContentFile(length)
 
 
     def parseCookies(self):

src/twisted/web/newsfragments/9655.feature

@@ -0,0 +1 @@
+twisted.web.server.Request will now use twisted.web.server.Site.getContentFile, if it exists, to get a file into which to write request content.  If getContentFile is not provided by the site, it will fall back to the previous behavior of using io.BytesIO for small requests and tempfile.TemporaryFile for large ones.

src/twisted/web/server.py

@@ -182,6 +182,24 @@ class Request(Copyable, http.Request, components.Componentized):
                 return name
 
 
+    def gotLength(self, length):
+        """
+        Called when HTTP channel got length of content in this request.
+
+        This method is not intended for users.
+
+        @param length: The length of the request body, as indicated by the
+            request headers.  L{None} if the request headers do not indicate a
+            length.
+        """
+        try:
+            getContentFile = self.channel.site.getContentFile
+        except AttributeError:
+            http.Request.gotLength(self, length)
+        else:
+            self.content = getContentFile(length)
+
+
     def process(self):
         """
         Process a request.

src/twisted/web/test/_util.py

@@ -81,4 +81,23 @@ class FlattenTestCase(TestCase):
         return d
 
 
-__all__ = ["_render", "FlattenTestCase"]
+
+def assertIsFilesystemTemporary(case, fileObj):
+    """
+    Assert that C{fileObj} is a temporary file on the filesystem.
+
+    @param case: A C{TestCase} instance to use to make the assertion.
+
+    @raise: C{case.failureException} if C{fileObj} is not a temporary file on
+        the filesystem.
+    """
+    # The tempfile API used to create content returns an instance of a
+    # different type depending on what platform we're running on.  The point
+    # here is to verify that the request body is in a file that's on the
+    # filesystem.  Having a fileno method that returns an int is a somewhat
+    # close approximation of this. -exarkun
+    case.assertIsInstance(fileObj.fileno(), int)
+
+
+
+__all__ = ["_render", "FlattenTestCase", "assertIsFilesystemTemporary"]

src/twisted/web/test/test_http.py

@@ -16,7 +16,11 @@ except ImportError:
 
 from io import BytesIO
 from itertools import cycle
-from zope.interface import provider
+from zope.interface import (
+    provider,
+    directlyProvides,
+    providedBy,
+)
 from zope.interface.verify import verifyObject
 
 from twisted.python.compat import (_PY3, iterbytes, long, networkString,
@@ -42,9 +46,12 @@ from twisted.web.test.requesthelper import (
     textLinearWhitespaceComponents,
 )
 
-from zope.interface import directlyProvides, providedBy
 from twisted.logger import globalLogPublisher
 
+from ._util import (
+    assertIsFilesystemTemporary,
+)
+
 
 
 class _IDeprecatedHTTPChannelToRequestInterfaceProxy(proxyForInterface(
@@ -1989,10 +1996,14 @@ Hello,
         content = []
         decoder = []
         testcase = self
+
         class MyRequest(http.Request):
             def process(self):
-                content.append(self.content.fileno())
+                content.append(self.content)
                 content.append(self.content.read())
+                # Don't let it close the original content object.  We want to
+                # inspect it later.
+                self.content = BytesIO()
                 method.append(self.method)
                 path.append(self.path)
                 decoder.append(self.channel._transferDecoder)
@@ -2000,13 +2011,12 @@ Hello,
                 self.finish()
 
         self.runRequest(httpRequest, MyRequest)
-        # The tempfile API used to create content returns an
-        # instance of a different type depending on what platform
-        # we're running on.  The point here is to verify that the
-        # request body is in a file that's on the filesystem.
-        # Having a fileno method that returns an int is a somewhat
-        # close approximation of this. -exarkun
-        self.assertIsInstance(content[0], int)
+
+        # We took responsibility for closing this when we replaced the request
+        # attribute, above.
+        self.addCleanup(content[0].close)
+
+        assertIsFilesystemTemporary(self, content[0])
         self.assertEqual(content[1], b'Hello, spam,eggs spam spam')
         self.assertEqual(method, [b'GET'])
         self.assertEqual(path, [b'/'])

src/twisted/web/test/test_web.py

@@ -7,6 +7,7 @@ Tests for various parts of L{twisted.web}.
 
 import os
 import zlib
+from io import BytesIO
 
 from zope.interface import implementer
 from zope.interface.verify import verifyObject
@@ -26,6 +27,11 @@ from twisted.web.static import Data
 from twisted.logger import globalLogPublisher, LogLevel
 from twisted.test.proto_helpers import EventLoggingObserver
 
+from ._util import (
+    assertIsFilesystemTemporary,
+)
+
+
 
 class ResourceTests(unittest.TestCase):
     def testListEntities(self):
@@ -943,6 +949,59 @@ class RequestTests(unittest.TestCase):
         self.assertNotIn(b'content-type', response.lower())
 
 
+    def test_defaultSmallContentFile(self):
+        """
+        L{http.Request} creates a L{BytesIO} if the content length is small and
+        the site doesn't offer to create one.
+        """
+        request = server.Request(DummyChannel())
+        request.gotLength(100000 - 1)
+        self.assertIsInstance(request.content, BytesIO)
+
+
+    def test_defaultLargerContentFile(self):
+        """
+        L{http.Request} creates a temporary file on the filesystem if the
+        content length is larger and the site doesn't offer to create one.
+        """
+        request = server.Request(DummyChannel())
+        request.gotLength(100000)
+        assertIsFilesystemTemporary(self, request.content)
+
+
+    def test_defaultUnknownSizeContentFile(self):
+        """
+        L{http.Request} creates a temporary file on the filesystem if the
+        content length is not known and the site doesn't offer to create one.
+        """
+        request = server.Request(DummyChannel())
+        request.gotLength(None)
+        assertIsFilesystemTemporary(self, request.content)
+
+
+    def test_siteSuppliedContentFile(self):
+        """
+        L{http.Request} uses L{Site.getContentFile}, if it exists, to get a
+        file-like object for the request content.
+        """
+        lengths = []
+        contentFile = BytesIO()
+        site = server.Site(resource.Resource())
+
+        def getContentFile(length):
+            lengths.append(length)
+            return contentFile
+        site.getContentFile = getContentFile
+
+        channel = DummyChannel()
+        channel.site = site
+
+        request = server.Request(channel)
+        request.gotLength(12345)
+        self.assertEqual([12345], lengths)
+        self.assertIs(contentFile, request.content)
+
+
 
 class GzipEncoderTests(unittest.TestCase):
     def setUp(self):