Repository: pallets/werkzeug (python)

Issue:
MapAdapter.build() should not flatten a value passed as single-item-list
<!--
This issue tracker is a tool to address bugs in Werkzeug itself. Please
use Pallets Discord or Stack Overflow for questions about your own code.

Replace this comment with a clear outline of what the bug is.
-->
Let's consider this code:

```
from flask import Flask, url_for
import logging

app = Flask(__name__)

@app.route('/test/<int:x>')
def urltest(x):
    return f"{x}"

@app.route("/")
def main():
    BUILT_URL = url_for('urltest', x=[1337])
    logging.warning(BUILT_URL)
    return ""
```

since Werkzeug v2 (apparently since b45b508c48cfca5ccbb582fdc39bbf486bed4e1a), `BUILT_URL` obtains a value `/test/1337`. But, as I understand, the call should have raised an error, because the type of parameter `x` is obviously wrong and should be `int`, not `List[int]`. If the value is a list containing single item, it is transformed to just that item. If the value contains many items, it is passed to [converter](https://github.com/pallets/werkzeug/blob/fb7ddd89ae3072e4f4002701a643eb247a402b64/src/werkzeug/routing.py#L1189) as is.

This behavior breaks my application, because I use the custom converter for lists, similar to this one: https://uniwebsidad.com/libros/explore-flask/chapter-6/url-converters#custom-converters

I don't know, maybe it is a bad idea to build URLs from a list, but this behavior feels just wrong.


<!--
Describe how to replicate the bug.

Include a minimal reproducible example that demonstrates the bug.
Include the full traceback if there was an exception.
-->

<!--
Describe the expected behavior that should have happened but didn't.
-->

Environment:

- Python version: 3.8.5
- Werkzeug version: 2.0.1


Pull request description:
`MapAdapter.build` does not flatten list value from plain dict
It should be possible to pass a list value to a custom URL converter when building. However, after some other changes to the code to support `MultiDict`, a regression was introduced that made that difficult.

This changes the behavior so flattening single-item lists only happens when passing `MultiDict`, where values will always be lists, and are mainly coming from `request.args` where this behavior makes sense. For plain dicts, neither of these things are done, it is assumed that the user will intentionally pass a list or single item depending on what's appropriate.

Additionally, `build` no longer filters `None` items from list values. Either the list will be passed to a URL converter, where it's the converter's job to handle the value passed to it, or it's a query arg, and `url_encode` already filters out `None`.

fixes #2249 
continues #2261 

Patch:
diff --git a/CHANGES.rst b/CHANGES.rst
index 1d1f6a14..e7c85ee0 100644
--- a/CHANGES.rst
+++ b/CHANGES.rst
@@ -56,6 +56,11 @@ Unreleased
     convert the value to an int. :issue:`2230`
 -   Always use ``socket.fromfd`` when restarting the dev server.
     :pr:`2287`
+-   When passing a dict of URL values to ``Map.build``, list values do
+    not filter out ``None`` or collapse to a single value. Passing a
+    ``MultiDict`` does collapse single items. This undoes a previous
+    change that made it difficult to pass a list, or ``None`` values in
+    a list, to custom URL converters. :issue:`2249`
 
 
 Version 2.0.3
diff --git a/src/werkzeug/routing.py b/src/werkzeug/routing.py
index 809308c6..0a66c41f 100644
--- a/src/werkzeug/routing.py
+++ b/src/werkzeug/routing.py
@@ -545,10 +545,10 @@ def _prefix_names(src: str) -> ast.stmt:
 _CALL_CONVERTER_CODE_FMT = "self._converters[{elem!r}].to_url()"
 _IF_KWARGS_URL_ENCODE_CODE = """\
 if kwargs:
-    q = '?'
     params = self._encode_query_vars(kwargs)
+    q = "?" if params else ""
 else:
-    q = params = ''
+    q = params = ""
 """
 _IF_KWARGS_URL_ENCODE_AST = _prefix_names(_IF_KWARGS_URL_ENCODE_CODE)
 _URL_ENCODE_AST_NAMES = (_prefix_names("q"), _prefix_names("params"))
@@ -2283,29 +2283,14 @@ class MapAdapter:
         self.map.update()
 
         if values:
-            temp_values: t.Dict[str, t.Union[t.List[t.Any], t.Any]] = {}
-            always_list = isinstance(values, MultiDict)
-            key: str
-            value: t.Optional[t.Union[t.List[t.Any], t.Any]]
-
-            # For MultiDict, dict.items(values) is like values.lists()
-            # without the call or list coercion overhead.
-            for key, value in dict.items(values):  # type: ignore
-                if value is None:
-                    continue
-
-                if always_list or isinstance(value, (list, tuple)):
-                    value = [v for v in value if v is not None]
-
-                    if not value:
-                        continue
-
-                    if len(value) == 1:
-                        value = value[0]
-
-                temp_values[key] = value
-
-            values = temp_values
+            if isinstance(values, MultiDict):
+                values = {
+                    k: (v[0] if len(v) == 1 else v)
+                    for k, v in dict.items(values)
+                    if len(v) != 0
+                }
+            else:  # plain dict
+                values = {k: v for k, v in values.items() if v is not None}
         else:
             values = {}
 


Test patch:
diff --git a/tests/test_routing.py b/tests/test_routing.py
index be3767d2..e8c7a429 100644
--- a/tests/test_routing.py
+++ b/tests/test_routing.py
@@ -1,4 +1,5 @@
 import gc
+import typing as t
 import uuid
 
 import pytest
@@ -754,48 +755,108 @@ def test_anyconverter():
 
 
 @pytest.mark.parametrize(
-    ("value", "expect"),
+    ("endpoint", "value", "expect"),
     [
-        (None, ""),
-        ([None], ""),
-        ([None, None], ""),
-        ("", "?extra="),
-        ([""], "?extra="),
-        (1.0, "?extra=1.0"),
-        ([1, 2], "?extra=1&extra=2"),
-        ([1, None, 2], "?extra=1&extra=2"),
-        ([1, "", 2], "?extra=1&extra=&extra=2"),
+        ("int", 1, "/1"),
+        ("int", None, r.BuildError),
+        ("int", [1], TypeError),
+        ("list", [1], "/1"),
+        ("list", [1, None, 2], "/1.None.2"),
+        ("list", 1, TypeError),
     ],
 )
-def test_build_append_unknown(value, expect):
-    map = r.Map([r.Rule("/foo/<name>", endpoint="foo")])
-    adapter = map.bind("example.org", "/", subdomain="test")
-    assert (
-        adapter.build("foo", {"name": "bar", "extra": value})
-        == f"http://example.org/foo/bar{expect}"
+def test_build_values_dict(endpoint, value, expect):
+    class ListConverter(r.BaseConverter):
+        def to_url(self, value: t.Any) -> str:
+            return super().to_url(".".join(map(str, value)))
+
+    url_map = r.Map(
+        [r.Rule("/<int:v>", endpoint="int"), r.Rule("/<list:v>", endpoint="list")],
+        converters={"list": ListConverter},
     )
-    assert (
-        adapter.build("foo", {"name": "bar", "extra": value}, append_unknown=False)
-        == "http://example.org/foo/bar"
+    adapter = url_map.bind("localhost")
+
+    if isinstance(expect, str):
+        assert adapter.build(endpoint, {"v": value}) == expect
+    else:
+        with pytest.raises(expect):
+            adapter.build(endpoint, {"v": value})
+
+
+@pytest.mark.parametrize(
+    ("endpoint", "value", "expect"),
+    [
+        ("int", 1, "/1"),
+        ("int", [1], "/1"),
+        ("int", [], r.BuildError),
+        ("int", None, TypeError),
+        ("int", [None], TypeError),
+        ("list", 1, TypeError),
+        ("list", [1], TypeError),
+        ("list", [[1]], "/1"),
+        ("list", [1, None, 2], "/1.None.2"),
+    ],
+)
+def test_build_values_multidict(endpoint, value, expect):
+    class ListConverter(r.BaseConverter):
+        def to_url(self, value: t.Any) -> str:
+            return super().to_url(".".join(map(str, value)))
+
+    url_map = r.Map(
+        [r.Rule("/<int:v>", endpoint="int"), r.Rule("/<list:v>", endpoint="list")],
+        converters={"list": ListConverter},
     )
+    adapter = url_map.bind("localhost")
 
+    if isinstance(expect, str):
+        assert adapter.build(endpoint, MultiDict({"v": value})) == expect
+    else:
+        with pytest.raises(expect):
+            adapter.build(endpoint, MultiDict({"v": value}))
 
-def test_build_append_multiple():
-    map = r.Map([r.Rule("/bar/<float:foo>", endpoint="endp")])
-    adapter = map.bind("example.org", "/", subdomain="subd")
-    params = {"foo": 0.815, "x": [1.0, 3.0], "y": 2.0}
-    a, b = adapter.build("endp", params).split("?")
-    assert a == "http://example.org/bar/0.815"
-    assert set(b.split("&")) == set("y=2.0&x=1.0&x=3.0".split("&"))
+
+@pytest.mark.parametrize(
+    ("value", "expect"),
+    [
+        (None, ""),
+        ([None], ""),
+        ([None, None], ""),
+        ("", "?v="),
+        ([""], "?v="),
+        (0, "?v=0"),
+        (1.0, "?v=1.0"),
+        ([1, 2], "?v=1&v=2"),
+        ([1, None, 2], "?v=1&v=2"),
+        ([1, "", 2], "?v=1&v=&v=2"),
+    ],
+)
+def test_build_append_unknown_dict(value, expect):
+    map = r.Map([r.Rule("/", endpoint="a")])
+    adapter = map.bind("localhost")
+    assert adapter.build("a", {"v": value}) == f"/{expect}"
+    assert adapter.build("a", {"v": value}, append_unknown=False) == "/"
 
 
-def test_build_append_multidict():
-    map = r.Map([r.Rule("/bar/<float:foo>", endpoint="endp")])
-    adapter = map.bind("example.org", "/", subdomain="subd")
-    params = MultiDict((("foo", 0.815), ("x", 1.0), ("x", 3.0), ("y", 2.0)))
-    a, b = adapter.build("endp", params).split("?")
-    assert a == "http://example.org/bar/0.815"
-    assert set(b.split("&")) == set("y=2.0&x=1.0&x=3.0".split("&"))
+@pytest.mark.parametrize(
+    ("value", "expect"),
+    [
+        (None, ""),
+        ([None], ""),
+        ([None, None], ""),
+        ("", "?v="),
+        ([""], "?v="),
+        (0, "?v=0"),
+        (1.0, "?v=1.0"),
+        ([1, 2], "?v=1&v=2"),
+        ([1, None, 2], "?v=1&v=2"),
+        ([1, "", 2], "?v=1&v=&v=2"),
+    ],
+)
+def test_build_append_unknown_multidict(value, expect):
+    map = r.Map([r.Rule("/", endpoint="a")])
+    adapter = map.bind("localhost")
+    assert adapter.build("a", MultiDict({"v": value})) == f"/{expect}"
+    assert adapter.build("a", MultiDict({"v": value}), append_unknown=False) == "/"
 
 
 def test_build_drop_none():