fix issue where request.args.pop removed parameters inconsistently (#2111)

Fix uvloop 1912

Makefile

@@ -71,7 +71,7 @@ black:
 	black --config ./.black.toml sanic tests
 
 fix-import: black
-	isort -rc sanic tests
+	isort sanic tests
 
 
 docs-clean:

sanic/__version__.py

@@ -1 +1 @@
-__version__ = "19.12.0"
+__version__ = "19.12.5"

sanic/app.py

@@ -194,6 +194,12 @@ class Sanic:
             strict_slashes = self.strict_slashes
 
         def response(handler):
+            if isinstance(handler, tuple):
+                # if a handler fn is already wrapped in a route, the handler
+                # variable will be a tuple of (existing routes, handler fn)
+                routes, handler = handler
+            else:
+                routes = []
             args = list(signature(handler).parameters.keys())
 
             if not args:
@@ -205,7 +211,8 @@ class Sanic:
             if stream:
                 handler.is_stream = stream
 
-            routes = self.router.add(
+            routes.extend(
+                self.router.add(
                     uri=uri,
                     methods=methods,
                     handler=handler,
@@ -214,6 +221,7 @@ class Sanic:
                     version=version,
                     name=name,
                 )
+            )
             return routes, handler
 
         return response
@@ -476,6 +484,13 @@ class Sanic:
             strict_slashes = self.strict_slashes
 
         def response(handler):
+            if isinstance(handler, tuple):
+                # if a handler fn is already wrapped in a route, the handler
+                # variable will be a tuple of (existing routes, handler fn)
+                routes, handler = handler
+            else:
+                routes = []
+
             async def websocket_handler(request, *args, **kwargs):
                 request.app = self
                 if not getattr(handler, "__blueprintname__", False):
@@ -516,7 +531,8 @@ class Sanic:
                     self.websocket_tasks.remove(fut)
                 await ws.close()
 
-            routes = self.router.add(
+            routes.extend(
+                self.router.add(
                     uri=uri,
                     handler=websocket_handler,
                     methods=frozenset({"GET"}),
@@ -524,6 +540,7 @@ class Sanic:
                     strict_slashes=strict_slashes,
                     name=name,
                 )
+            )
             return routes, handler
 
         return response
@@ -813,6 +830,14 @@ class Sanic:
                 "Endpoint with name `{}` was not found".format(view_name)
             )
 
+        # If the route has host defined, split that off
+        # TODO: Retain netloc and path separately in Route objects
+        host = uri.find("/")
+        if host > 0:
+            host, uri = uri[:host], uri[host:]
+        else:
+            host = None
+
         if view_name == "static" or view_name.endswith(".static"):
             filename = kwargs.pop("filename", None)
             # it's static folder
@@ -845,7 +870,7 @@ class Sanic:
 
         netloc = kwargs.pop("_server", None)
         if netloc is None and external:
-            netloc = self.config.get("SERVER_NAME", "")
+            netloc = host or self.config.get("SERVER_NAME", "")
 
         if external:
             if not scheme:
@@ -1449,3 +1474,5 @@ class Sanic:
         self.asgi = True
         asgi_app = await ASGIApp.create(self, scope, receive, send)
         await asgi_app()
+
+    _asgi_single_callable = True  # We conform to ASGI 3.0 single-callable

sanic/asgi.py

@@ -309,13 +309,17 @@ class ASGIApp:
         callback = None if self.ws else self.stream_callback
         await handler(self.request, None, callback)
 
+    _asgi_single_callable = True  # We conform to ASGI 3.0 single-callable
+
     async def stream_callback(self, response: HTTPResponse) -> None:
         """
         Write the response.
         """
         headers: List[Tuple[bytes, bytes]] = []
         cookies: Dict[str, str] = {}
+        content_length: List[str] = []
         try:
+            content_length = response.headers.popall("content-length", [])
             cookies = {
                 v.key: v
                 for _, v in list(
@@ -347,11 +351,23 @@ class ASGIApp:
                 if name not in (b"Set-Cookie",)
             ]
 
-        if "content-length" not in response.headers and not isinstance(
+        response.asgi = True
-            response, StreamingHTTPResponse
+        is_streaming = isinstance(response, StreamingHTTPResponse)
-        ):
+        if is_streaming and getattr(response, "chunked", False):
+            # disable sanic chunking, this is done at the ASGI-server level
+            setattr(response, "chunked", False)
+            # content-length header is removed to signal to the ASGI-server
+            # to use automatic-chunking if it supports it
+        elif len(content_length) > 0:
             headers += [
-                (b"content-length", str(len(response.body)).encode("latin-1"))
+                (b"content-length", str(content_length[0]).encode("latin-1"))
+            ]
+        elif not is_streaming:
+            headers += [
+                (
+                    b"content-length",
+                    str(len(getattr(response, "body", b""))).encode("latin-1"),
+                )
             ]
 
         if "content-type" not in response.headers:

sanic/request.py

@@ -260,9 +260,12 @@ class Request:
         :type errors: str
         :return: RequestParameters
         """
-        if not self.parsed_args[
+        if (
-            (keep_blank_values, strict_parsing, encoding, errors)
+            keep_blank_values,
-        ]:
+            strict_parsing,
+            encoding,
+            errors,
+        ) not in self.parsed_args:
             if self.query_string:
                 self.parsed_args[
                     (keep_blank_values, strict_parsing, encoding, errors)
@@ -328,9 +331,12 @@ class Request:
         :type errors: str
         :return: list
         """
-        if not self.parsed_not_grouped_args[
+        if (
-            (keep_blank_values, strict_parsing, encoding, errors)
+            keep_blank_values,
-        ]:
+            strict_parsing,
+            encoding,
+            errors,
+        ) not in self.parsed_not_grouped_args:
             if self.query_string:
                 self.parsed_not_grouped_args[
                     (keep_blank_values, strict_parsing, encoding, errors)

sanic/response.py

@@ -22,6 +22,9 @@ except ImportError:
 
 
 class BaseHTTPResponse:
+    def __init__(self):
+        self.asgi = False
+
     def _encode_body(self, data):
         try:
             # Try to encode it regularly
@@ -59,12 +62,15 @@ class StreamingHTTPResponse(BaseHTTPResponse):
         content_type="text/plain",
         chunked=True,
     ):
+        super().__init__()
+
         self.content_type = content_type
         self.streaming_fn = streaming_fn
         self.status = status
         self.headers = Header(headers or {})
         self.chunked = chunked
         self._cookies = None
+        self.protocol = None
 
     async def write(self, data):
         """Writes a chunk of data to the streaming response.
@@ -74,6 +80,8 @@ class StreamingHTTPResponse(BaseHTTPResponse):
         if type(data) != bytes:
             data = self._encode_body(data)
 
+        # `chunked` will always be False in ASGI-mode, even if the underlying
+        # ASGI Transport implements Chunked transport. That does it itself.
         if self.chunked:
             await self.protocol.push_data(b"%x\r\n%b\r\n" % (len(data), data))
         else:
@@ -93,6 +101,7 @@ class StreamingHTTPResponse(BaseHTTPResponse):
             keep_alive=keep_alive,
             keep_alive_timeout=keep_alive_timeout,
         )
+        if not getattr(self, "asgi", False):
             await self.protocol.push_data(headers)
             await self.protocol.drain()
         await self.streaming_fn(self)
@@ -144,6 +153,8 @@ class HTTPResponse(BaseHTTPResponse):
         content_type=None,
         body_bytes=b"",
     ):
+        super().__init__()
+
         self.content_type = content_type
 
         if body is not None:
@@ -202,16 +213,14 @@ class HTTPResponse(BaseHTTPResponse):
         return self._cookies
 
 
-def empty(
+def empty(status=204, headers=None):
-    status=204, headers=None,
-):
     """
     Returns an empty response to the client.
 
     :param status Response code.
     :param headers Custom Headers.
     """
-    return HTTPResponse(body_bytes=b"", status=status, headers=headers,)
+    return HTTPResponse(body_bytes=b"", status=status, headers=headers)
 
 
 def json(

sanic/server.py

@@ -731,6 +731,26 @@ class AsyncioServer:
             task = asyncio.ensure_future(coro, loop=self.loop)
             return task
 
+    def start_serving(self):
+        if self.server:
+            try:
+                return self.server.start_serving()
+            except AttributeError:
+                raise NotImplementedError(
+                    "server.start_serving not available in this version "
+                    "of asyncio or uvloop."
+                )
+
+    def serve_forever(self):
+        if self.server:
+            try:
+                return self.server.serve_forever()
+            except AttributeError:
+                raise NotImplementedError(
+                    "server.serve_forever not available in this version "
+                    "of asyncio or uvloop."
+                )
+
     def __await__(self):
         """Starts the asyncio server, returns AsyncServerCoro"""
         task = asyncio.ensure_future(self.serve_coro)

setup.py

@@ -25,6 +25,7 @@ class PyTest(TestCommand):
 
     def run_tests(self):
         import shlex
+
         import pytest
 
         errno = pytest.main(shlex.split(self.pytest_args))
@@ -75,7 +76,7 @@ env_dependency = (
     '; sys_platform != "win32" ' 'and implementation_name == "cpython"'
 )
 ujson = "ujson>=1.35" + env_dependency
-uvloop = "uvloop>=0.5.3" + env_dependency
+uvloop = "uvloop>=0.5.3,<0.15.0" + env_dependency
 
 requirements = [
     "httptools>=0.0.10",
@@ -83,13 +84,13 @@ requirements = [
     ujson,
     "aiofiles>=0.3.0",
     "websockets>=7.0,<9.0",
-    "multidict>=4.0,<5.0",
+    "multidict==5.0.0",
     "httpx==0.9.3",
 ]
 
 tests_require = [
     "pytest==5.2.1",
-    "multidict>=4.0,<5.0",
+    "multidict==5.0.0",
     "gunicorn",
     "pytest-cov",
     "httpcore==0.3.0",

tests/performance/wheezy/simple_server.py

@@ -39,6 +39,7 @@ main = WSGIApplication(
 
 if __name__ == "__main__":
     import sys
+
     from wsgiref.simple_server import make_server
 
     try:

tests/test_app.py

@@ -41,6 +41,20 @@ def test_create_asyncio_server(app):
         assert srv.is_serving() is True
 
 
+@pytest.mark.skipif(
+    sys.version_info < (3, 7), reason="requires python3.7 or higher"
+)
+def test_asyncio_server_no_start_serving(app):
+    if not uvloop_installed():
+        loop = asyncio.get_event_loop()
+        asyncio_srv_coro = app.create_server(
+            return_asyncio_server=True,
+            asyncio_server_kwargs=dict(start_serving=False),
+        )
+        srv = loop.run_until_complete(asyncio_srv_coro)
+        assert srv.is_serving() is False
+
+
 @pytest.mark.skipif(
     sys.version_info < (3, 7), reason="requires python3.7 or higher"
 )
@@ -53,6 +67,10 @@ def test_asyncio_server_start_serving(app):
         )
         srv = loop.run_until_complete(asyncio_srv_coro)
         assert srv.is_serving() is False
+        loop.run_until_complete(srv.start_serving())
+        assert srv.is_serving() is True
+        srv.close()
+        # Looks like we can't easily test `serve_forever()`
 
 
 def test_app_loop_not_running(app):

tests/test_requests.py

@@ -244,6 +244,17 @@ def test_query_string(app):
     assert request.args.getlist("test1") == ["1"]
     assert request.args.get("test3", default="My value") == "My value"
 
+def test_popped_stays_popped(app):
+    @app.route("/")
+    async def handler(request):
+        return text("OK")
+
+    request, response = app.test_client.get(
+        "/", params=[("test1", "1")]
+    )
+
+    assert request.args.pop("test1") == ["1"]
+    assert "test1" not in request.args
 
 @pytest.mark.asyncio
 async def test_query_string_asgi(app):

tests/test_response.py

@@ -15,13 +15,13 @@ from aiofiles import os as async_os
 from sanic.response import (
     HTTPResponse,
     StreamingHTTPResponse,
+    empty,
     file,
     file_stream,
     json,
     raw,
     stream,
 )
-from sanic.response import empty
 from sanic.server import HttpProtocol
 from sanic.testing import HOST, PORT
 
@@ -232,6 +232,12 @@ def test_chunked_streaming_returns_correct_content(streaming_app):
     assert response.text == "foo,bar"
 
 
+@pytest.mark.asyncio
+async def test_chunked_streaming_returns_correct_content_asgi(streaming_app):
+    request, response = await streaming_app.asgi_client.get("/")
+    assert response.text == "foo,bar"
+
+
 def test_non_chunked_streaming_adds_correct_headers(non_chunked_streaming_app):
     request, response = non_chunked_streaming_app.test_client.get("/")
     assert "Transfer-Encoding" not in response.headers
@@ -239,8 +245,18 @@ def test_non_chunked_streaming_adds_correct_headers(non_chunked_streaming_app):
     assert response.headers["Content-Length"] == "7"
 
 
+@pytest.mark.asyncio
+async def test_non_chunked_streaming_adds_correct_headers_asgi(
+    non_chunked_streaming_app,
+):
+    request, response = await non_chunked_streaming_app.asgi_client.get("/")
+    assert "Transfer-Encoding" not in response.headers
+    assert response.headers["Content-Type"] == "text/csv"
+    assert response.headers["Content-Length"] == "7"
+
+
 def test_non_chunked_streaming_returns_correct_content(
-    non_chunked_streaming_app
+    non_chunked_streaming_app,
 ):
     request, response = non_chunked_streaming_app.test_client.get("/")
     assert response.text == "foo,bar"
@@ -255,7 +271,7 @@ def test_stream_response_status_returns_correct_headers(status):
 
 @pytest.mark.parametrize("keep_alive_timeout", [10, 20, 30])
 def test_stream_response_keep_alive_returns_correct_headers(
-    keep_alive_timeout
+    keep_alive_timeout,
 ):
     response = StreamingHTTPResponse(sample_streaming_fn)
     headers = response.get_headers(
@@ -284,7 +300,7 @@ def test_stream_response_does_not_include_chunked_header_if_disabled():
 
 
 def test_stream_response_writes_correct_content_to_transport_when_chunked(
-    streaming_app
+    streaming_app,
 ):
     response = StreamingHTTPResponse(sample_streaming_fn)
     response.protocol = MagicMock(HttpProtocol)

tests/test_routes.py

@@ -551,6 +551,35 @@ def test_route_duplicate(app):
             pass
 
 
+def test_double_stack_route(app):
+    @app.route("/test/1")
+    @app.route("/test/2")
+    async def handler1(request):
+        return text("OK")
+
+    request, response = app.test_client.get("/test/1")
+    assert response.status == 200
+    request, response = app.test_client.get("/test/2")
+    assert response.status == 200
+
+
+@pytest.mark.asyncio
+async def test_websocket_route_asgi(app):
+    ev = asyncio.Event()
+
+    @app.websocket("/test/1")
+    @app.websocket("/test/2")
+    async def handler(request, ws):
+        ev.set()
+
+    request, response = await app.asgi_client.websocket("/test/1")
+    first_set = ev.is_set()
+    ev.clear()
+    request, response = await app.asgi_client.websocket("/test/1")
+    second_set = ev.is_set()
+    assert first_set and second_set
+
+
 def test_method_not_allowed(app):
     @app.route("/test", methods=["GET"])
     async def handler(request):

tox.ini

@@ -14,7 +14,7 @@ deps =
     pytest-sugar
     httpcore==0.3.0
     httpx==0.9.3
-    chardet<=2.3.0
+    multidict==5.0.0
     beautifulsoup4
     gunicorn
     pytest-benchmark