From f407e3cf8a2f8c860c2402458a431f6fb186fa57 Mon Sep 17 00:00:00 2001
From: David Lord <davidism@gmail.com>
Date: Fri, 25 Oct 2024 15:46:58 -0700
Subject: [PATCH] pop raises type conversion failure

---
 src/werkzeug/datastructures/structures.py | 55 ++++++++++++-----------
 tests/test_datastructures.py              | 29 +++++++-----
 2 files changed, 45 insertions(+), 39 deletions(-)

diff --git a/src/werkzeug/datastructures/structures.py b/src/werkzeug/datastructures/structures.py
index 90479091c..90ed95bc0 100644
--- a/src/werkzeug/datastructures/structures.py
+++ b/src/werkzeug/datastructures/structures.py
@@ -45,8 +45,8 @@ def __repr__(self):
 
 
 class TypeConversionDict(dict):
-    """Works like a regular dict but the :meth:`get` method can perform
-    type conversions.  :class:`MultiDict` and :class:`CombinedMultiDict`
+    """Works like a regular dict but the :meth:`get` and :meth:`pop` methods can
+    perform type conversions. :class:`MultiDict` and :class:`CombinedMultiDict`
     are subclasses of this class and provide the same feature.
 
     .. versionadded:: 0.5
@@ -89,51 +89,52 @@ def get(self, key, default=None, type=None):
         return rv
 
     def pop(self, key, default=_missing, type=None):
-        """Like :meth:`get` but removes the key/value pair.
+        """Similar to :meth:`get` but removes the item, and does not default to
+        ``None`` if the key doesn't exist. If type conversion fails and no
+        default is given, the item is not removed and the error is re-raised.
 
-        >>> d = TypeConversionDict(foo='42', bar='blub')
-        >>> d.pop('foo', type=int)
+        >>> d = TypeConversionDict(a="42", b="abc", c="def")
+        >>> d.pop("a", type=int)
         42
-        >>> 'foo' in d
+        >>> "a" in d
         False
-        >>> d.pop('bar', -1, type=int)
+        >>> d.pop("b", -1, type=int)
         -1
-        >>> 'bar' in d
+        >>> "b" in d
         False
+        >>> d.pop("c", type=int)
+        ValueError: ...
+        >>> "c" in d
+        True
 
         :param key: The key to be looked up.
-        :param default: The default value to be returned if the key is not
-                        in the dictionary. If not further specified it's
-                        an :exc:`KeyError`.
-        :param type: A callable that is used to cast the value in the dict.
-                        If a :exc:`ValueError` or a :exc:`TypeError` is raised
-                        by this callable the default value is returned.
-
-        .. admonition:: note
-
-           If the type conversion fails, the key is **not** removed from the
-           dictionary.
-
+        :param default: The value to be returned if the key doesn't exist. If
+            not given, a ``KeyError`` is raised.
+        :param type: A callable that is used to convert the value.
+            If a ``ValueError`` or ``TypeError`` is raised, the default value is
+            returned if given, otherwise the error is raised.
         """
+
         try:
+            # Don't remove the item yet, type conversion might fail.
             rv = self[key]
         except KeyError:
             if default is _missing:
                 raise
+
             return default
+
         if type is not None:
             try:
                 rv = type(rv)
             except (ValueError, TypeError):
                 if default is _missing:
-                    return None
+                    raise
+
                 return default
-        try:
-            # This method is not meant to be thread-safe, but at least lets not
-            # fall over if the dict was mutated between the get and the delete. -MK
-            del self[key]
-        except KeyError:
-            pass
+
+        # Remove the item after type conversion succeeds.
+        del self[key]
         return rv
 
 
diff --git a/tests/test_datastructures.py b/tests/test_datastructures.py
index 0ece3a4e7..f3fc2ba1b 100644
--- a/tests/test_datastructures.py
+++ b/tests/test_datastructures.py
@@ -585,19 +585,24 @@ def test_pop_value_conversion(self):
         assert d.pop("foo", type=int) == 1
         assert "foo" not in d
 
-    def test_pop_return_when_conversion_is_not_possible(self):
-        d = self.storage_class(foo="bar", baz=None)
-        assert d.pop("foo", type=int) is None
-        assert "foo" in d  # key is still in the dict, because the conversion failed
-        assert d.pop("baz", type=int) is None
-        assert "baz" in d  # key is still in the dict, because the conversion failed
+    @pytest.mark.parametrize(
+        ("value", "exc_type"), [("a", ValueError), (None, TypeError)]
+    )
+    def test_pop_conversion_fails(self, value, exc_type):
+        d = self.storage_class(a=value)
 
-    def test_pop_return_default_when_conversion_is_not_possible(self):
-        d = self.storage_class(foo="bar", baz=None)
-        assert d.pop("foo", default=-1, type=int) == -1
-        assert "foo" in d  # key is still in the dict, because the conversion failed
-        assert d.pop("baz", default=-1, type=int) == -1
-        assert "baz" in d  # key is still in the dict, because the conversion failed
+        with pytest.raises(exc_type):
+            d.pop("a", type=int)
+
+        assert "a" in d
+
+    @pytest.mark.parametrize(
+        ("value", "exc_type"), [("a", ValueError), (None, TypeError)]
+    )
+    def test_pop_conversion_fails_default(self, value, exc_type):
+        d = self.storage_class(a=value)
+        assert d.pop("a", default=None, type=int) is None
+        assert "a" in d
 
     def test_pop_propagate_exceptions_in_conversion(self):
         d = self.storage_class(foo="bar")