refactor EnvironBuilder file handling and related code (#3101)

Author: davidism

CHANGES.rst

@@ -26,6 +26,11 @@ Unreleased
     part of the HTTP spec. :pr:`3092`
 -   ``EnvironBuilder.close`` closes all open files in ``files`` rather than only
     the first for each key. :pr:`3092`
+-   ``EnvironBuilder`` can be used as a ``with`` context manager. :pr:`3101`
+-   ``EnvironBuilder.files.add_file`` will detect the filename when passing an
+    IO object. :pr:`3101`
+-   Added the ``EnvironBuilder.files.close`` method to close all files.
+    ``EnvironBuilder.files.clear`` will call ``close``. :pr:`3101`
 -   ``Map`` takes a ``subdomain_matching`` parameter to disable subdomain
     matching. In ``bind_to_environ``, the ``server_name`` parameter is not used
     if ``host_matching`` is enabled. If ``default_subdomain`` is set, it is used
@@ -34,6 +39,10 @@ Unreleased
     validated against ``request.trusted_hosts``. An invalid host will raise a
     400 error. :issue:`3007`
 -   Watchdog reloader is more efficient at ignoring events. :issue:`3090`
+-   If multipart parsing fails after some files have already been parsed, they
+    are closed to prevent a ``ResourceWarning``. :pr:`3101`
+-   ``SpooledTemporaryFile`` is always used for multipart file parsing.
+    :pr:`3101`
 
 
 Version 3.1.5

pyproject.toml

@@ -94,8 +94,6 @@ default-groups = ["dev", "pre-commit", "tests", "typing"]
 testpaths = ["tests"]
 filterwarnings = [
     "error",
-    # TODO fix these
-    "always::pytest.PytestUnraisableExceptionWarning",
 ]
 markers = ["dev_server: tests that start the dev server"]
 

src/werkzeug/datastructures/file_storage.py

@@ -32,22 +32,7 @@ def __init__(
     ):
         self.name = name
         self.stream = stream or BytesIO()
-
-        # If no filename is provided, attempt to get the filename from
-        # the stream object. Python names special streams like
-        # ``<stderr>`` with angular brackets, skip these streams.
-        if filename is None:
-            filename = getattr(stream, "name", None)
-
-            if filename is not None:
-                filename = fsdecode(filename)
-
-            if filename and filename[0] == "<" and filename[-1] == ">":
-                filename = None
-        else:
-            filename = fsdecode(filename)
-
-        self.filename = filename
+        self.filename = _guess_filename(self.stream, filename)
 
         if headers is None:
             headers = Headers()
@@ -163,9 +148,8 @@ def __repr__(self) -> str:
 
 
 class FileMultiDict(MultiDict[str, FileStorage]):
-    """A special :class:`MultiDict` that has convenience methods to add
-    files to it.  This is used for :class:`EnvironBuilder` and generally
-    useful for unittesting.
+    """A :class:`MultiDict` for managing form data file values. Used by
+    :class:`.EnvironBuilder` for tests.
 
     .. versionadded:: 0.5
     """
@@ -177,13 +161,20 @@ def add_file(
         filename: str | None = None,
         content_type: str | None = None,
     ) -> None:
-        """Adds a new file to the dict.  `file` can be a file name or
-        a :class:`file`-like or a :class:`FileStorage` object.
-
-        :param name: the name of the field.
-        :param file: a filename or :class:`file`-like object
-        :param filename: an optional filename
-        :param content_type: an optional content type
+        """Add a file to the given key. Can be passed a filename or IO object,
+        which will construct a :class:`.FileStorage` object.
+
+        :param name: The key to add the file to.
+        :param file: The file to add. Constructs a :class:`FileStorage` object
+            if the value is not one.
+        :param filename: The filename to set for the field. Defaults to ``file``
+            if it's a filename or ``file.name`` if it's an IO object.
+        :param content_type: The content type to set for the field. Defaults to
+            guessing based on the filename, falling back to
+            ``application/octet-stream``.
+
+        .. versionchanged:: 3.2
+            The filename is detected from an IO object.
         """
         if isinstance(file, FileStorage):
             self.add(name, file)
@@ -196,8 +187,9 @@ def add_file(
             file_obj: t.IO[bytes] = open(file, "rb")
         else:
             file_obj = file  # type: ignore[assignment]
+            filename = _guess_filename(file_obj, filename)
 
-        if filename and content_type is None:
+        if filename is not None and content_type is None:
             content_type = (
                 mimetypes.guess_type(filename)[0] or "application/octet-stream"
             )
@@ -214,6 +206,30 @@ def close(self) -> None:
                 if not value.closed:
                     value.close()
 
+    def clear(self) -> None:
+        """Call :meth:`close`, then remove all items.
+
+        .. versionadded:: 3.2
+        """
+        self.close()
+        super().clear()
+
+
+def _guess_filename(stream: t.IO[t.Any], filename: str | None) -> str | None:
+    if filename is not None:
+        return fsdecode(filename)
+
+    filename = getattr(stream, "name", None)
+
+    if filename is not None:
+        filename = fsdecode(filename)
+
+        # Python names special streams like `<stderr>`, ignore these.
+        if filename[:1] == "<" and filename[-1:] == ">":
+            filename = None
+
+    return filename
+
 
 # circular dependencies
 from .. import http  # noqa: E402

src/werkzeug/formparser.py

@@ -1,7 +1,8 @@
 from __future__ import annotations
 
 import typing as t
-from io import BytesIO
+from tempfile import SpooledTemporaryFile
+from types import TracebackType
 from urllib.parse import parse_qsl
 
 from ._internal import _plain_int
@@ -19,18 +20,8 @@
 from .wsgi import get_content_length
 from .wsgi import get_input_stream
 
-# there are some platforms where SpooledTemporaryFile is not available.
-# In that case we need to provide a fallback.
-try:
-    from tempfile import SpooledTemporaryFile
-except ImportError:
-    from tempfile import TemporaryFile
-
-    SpooledTemporaryFile = None  # type: ignore
-
 if t.TYPE_CHECKING:
-    import typing as te
-
+    import typing_extensions as te
     from _typeshed.wsgi import WSGIEnvironment
 
     t_parse_result = tuple[
@@ -56,14 +47,7 @@ def default_stream_factory(
     filename: str | None,
     content_length: int | None = None,
 ) -> t.IO[bytes]:
-    max_size = 1024 * 500
-
-    if SpooledTemporaryFile is not None:
-        return t.cast(t.IO[bytes], SpooledTemporaryFile(max_size=max_size, mode="rb+"))
-    elif total_content_length is None or total_content_length > max_size:
-        return t.cast(t.IO[bytes], TemporaryFile("rb+"))
-
-    return BytesIO()
+    return SpooledTemporaryFile(max_size=1024 * 500, mode="rb+")
 
 
 def parse_form_data(
@@ -253,18 +237,19 @@ def _parse_multipart(
         content_length: int | None,
         options: dict[str, str],
     ) -> t_parse_result:
-        parser = MultiPartParser(
-            stream_factory=self.stream_factory,
-            max_form_memory_size=self.max_form_memory_size,
-            max_form_parts=self.max_form_parts,
-            cls=self.cls,
-        )
         boundary = options.get("boundary", "").encode("ascii")
 
         if not boundary:
             raise ValueError("Missing boundary")
 
-        form, files = parser.parse(stream, boundary, content_length)
+        with MultiPartParser(
+            stream_factory=self.stream_factory,
+            max_form_memory_size=self.max_form_memory_size,
+            max_form_parts=self.max_form_parts,
+            cls=self.cls,
+        ) as parser:
+            form, files = parser.parse(stream, boundary, content_length)
+
         return stream, form, files
 
     def _parse_urlencoded(
@@ -305,15 +290,26 @@ def __init__(
             stream_factory = default_stream_factory
 
         self.stream_factory = stream_factory
+        self._files: list[t.IO[bytes]] = []
 
         if cls is None:
             cls = t.cast("type[MultiDict[str, t.Any]]", MultiDict)
 
         self.cls = cls
         self.buffer_size = buffer_size
 
-    def fail(self, message: str) -> te.NoReturn:
-        raise ValueError(message)
+    def __enter__(self) -> te.Self:
+        return self
+
+    def __exit__(
+        self,
+        exc_type: type[BaseException] | None,
+        exc_val: BaseException | None,
+        exc_tb: TracebackType | None,
+    ) -> None:
+        if exc_val is not None:
+            for file in self._files:
+                file.close()
 
     def get_part_charset(self, headers: Headers) -> str:
         # Figure out input charset for current part
@@ -346,6 +342,7 @@ def start_file_streaming(
             content_type=content_type,
             content_length=content_length,
         )
+        self._files.append(container)
         return container
 
     def parse(