utils.path_with_added_args() improvements

* Now covered by unit tests
* Preserves original order
* Can handle multiple args of the same name, e.g. ?bar=1&bar=2

datasette/utils.py

@@ -150,17 +150,17 @@ def path_with_added_args(request, args, path=None):
     if isinstance(args, dict):
         args = args.items()
     arg_keys = set(a[0] for a in args)
-    current = [
-        (key, value)
-        for key, value in request.raw_args.items()
-        if key not in arg_keys
-    ]
+    current = []
+    for key, values in request.args.items():
+        current.extend(
+            [(key, value) for value in values if key not in arg_keys]
+        )
     current.extend([
         (key, value)
         for key, value in args
         if value is not None
     ])
-    query_string = urllib.parse.urlencode(sorted(current))
+    query_string = urllib.parse.urlencode(current)
     if query_string:
         query_string = '?{}'.format(query_string)
     return path + query_string

tests/test_html.py

@@ -65,7 +65,7 @@ def test_add_filter_redirects(app_client):
     path = path_base + '?foo=bar&' + filter_args
     response = app_client.get(path, allow_redirects=False, gather_request=False)
     assert response.status == 302
-    assert response.headers['Location'].endswith('?content__startswith=x&foo=bar')
+    assert response.headers['Location'].endswith('?foo=bar&content__startswith=x')
 
     # Test that op with a __x suffix overrides the filter value
     path = path_base + '?' + urllib.parse.urlencode({
@@ -100,7 +100,7 @@ def test_existing_filter_redirects(app_client):
     response = app_client.get(path, allow_redirects=False, gather_request=False)
     assert response.status == 302
     assert response.headers['Location'].endswith(
-        '?age__gte=22&age__lt=30&name__contains=hello&name__contains=world'
+        '?name__contains=hello&age__gte=22&age__lt=30&name__contains=world'
     )
 
     # Setting _filter_column_3 to empty string should remove *_3 entirely
@@ -109,7 +109,7 @@ def test_existing_filter_redirects(app_client):
     response = app_client.get(path, allow_redirects=False, gather_request=False)
     assert response.status == 302
     assert response.headers['Location'].endswith(
-        '?age__gte=22&name__contains=hello&name__contains=world'
+        '?name__contains=hello&age__gte=22&name__contains=world'
     )
 
     # ?_filter_op=exact should be removed if unaccompanied by _fiter_column

tests/test_utils.py

@@ -6,6 +6,7 @@ from datasette import utils
 import json
 import os
 import pytest
+from sanic.request import Request
 import sqlite3
 import tempfile
 from unittest.mock import patch
@@ -22,6 +23,25 @@ def test_urlsafe_components(path, expected):
     assert expected == utils.urlsafe_components(path)
 
 
+@pytest.mark.parametrize('path,added_args,expected', [
+    ('/foo', {'bar': 1}, '/foo?bar=1'),
+    ('/foo?bar=1', {'baz': 2}, '/foo?bar=1&baz=2'),
+    ('/foo?bar=1&bar=2', {'baz': 3}, '/foo?bar=1&bar=2&baz=3'),
+    ('/foo?bar=1', {'bar': None}, '/foo'),
+    # Test order is preserved
+    ('/?_facet=prim_state&_facet=area_name', {
+        'prim_state': 'GA'
+    }, '/?_facet=prim_state&_facet=area_name&prim_state=GA'),
+])
+def test_path_with_added_args(path, added_args, expected):
+    request = Request(
+        path.encode('utf8'),
+        {}, '1.1', 'GET', None
+    )
+    actual = utils.path_with_added_args(request, added_args)
+    assert expected == actual
+
+
 @pytest.mark.parametrize('row,pks,expected_path', [
     ({'A': 'foo', 'B': 'bar'}, ['A', 'B'], 'foo,bar'),
     ({'A': 'f,o', 'B': 'bar'}, ['A', 'B'], 'f%2Co,bar'),