Sanic Updates (#251)

* Fix sanic response header parsing.

* Call twice since underlying function mutates the headers.

* Fix request middleware duplication counts.

* Add app argument to request.

* Fix return count change.

* Convert middleware patches to deque.

* Fix custom router.

* Fix binding.

* Expand sanic testing in toxfile.

Co-authored-by: Katherine Kelly <[email protected]>
Co-authored-by: Uma Annamalai <[email protected]>
Co-authored-by: Kevin K. Yang <[email protected]>

* Update fixture signature for attribute checking

* Fix all sanic tests

Fix header tests

Working on sanic testing

Converting sanic to AsgiTest

Asgi Testing

Co-authored-by: Uma Annamalai <[email protected]>

Patching for latest sanic versions

Co-authored-by: Uma Annamalai <[email protected]>

Reverting ill conceived changes

Finish fixing sanic tests

* Fix forgone param fixture

* Fix tox.ini typo

* Remove unused package

Co-authored-by: Allan Feldman <[email protected]>
Co-authored-by: Katherine Kelly <[email protected]>
Co-authored-by: Uma Annamalai <[email protected]>
Co-authored-by: Kevin K. Yang <[email protected]>

Author: 5 people

newrelic/hooks/framework_sanic.py

@@ -13,6 +13,7 @@
 # limitations under the License.
 
 import sys
+from inspect import isawaitable, iscoroutinefunction
 
 from newrelic.api.web_transaction import web_transaction
 from newrelic.api.transaction import current_transaction
@@ -157,6 +158,44 @@ def _sanic_app_init(wrapped, instance, args, kwargs):
     return result
 
 
+def _nr_sanic_response_get_headers(wrapped, instance, args, kwargs):
+    # We call the wrapped function twice to allow the function to mutate the headers
+    result = wrapped(*args, **kwargs)
+    transaction = current_transaction()
+
+    if transaction is None:
+        return result
+
+    # instance is the response object
+    cat_headers = transaction.process_response(str(instance.status),
+            instance.headers.items())
+
+    for header_name, header_value in cat_headers:
+        if header_name not in instance.headers:
+            instance.headers[header_name] = header_value
+
+    return wrapped(*args, **kwargs)
+
+
+async def _nr_sanic_response_send(wrapped, instance, args, kwargs):
+    transaction = current_transaction()
+    result = wrapped(*args, **kwargs)
+    if isawaitable(result):
+        await result
+
+    if transaction is None:
+        return wrapped(*args, **kwargs)
+
+    # instance is the response object
+    cat_headers = transaction.process_response(str(instance.status),
+            instance.headers.items())
+
+    for header_name, header_value in cat_headers:
+        if header_name not in instance.headers:
+            instance.headers[header_name] = header_value
+
+    return result
+
 def _nr_sanic_response_parse_headers(wrapped, instance, args, kwargs):
     transaction = current_transaction()
 
@@ -235,5 +274,13 @@ def instrument_sanic_app(module):
 
 
 def instrument_sanic_response(module):
-    wrap_function_wrapper(module, 'BaseHTTPResponse._parse_headers',
-        _nr_sanic_response_parse_headers)
+    if hasattr(module.BaseHTTPResponse, 'send'):
+        wrap_function_wrapper(module, 'BaseHTTPResponse.send',
+            _nr_sanic_response_send)
+    else:
+        if hasattr(module.BaseHTTPResponse, 'get_headers'):
+            wrap_function_wrapper(module, 'BaseHTTPResponse.get_headers',
+                _nr_sanic_response_get_headers)
+        if hasattr(module.BaseHTTPResponse, '_parse_headers'):
+            wrap_function_wrapper(module, 'BaseHTTPResponse._parse_headers',
+                _nr_sanic_response_parse_headers)

tests/framework_sanic/_target_application.py

@@ -50,24 +50,39 @@ def add(self, exception, handler, *args, **kwargs):
 
 
 class CustomRouter(Router):
+    def __init__(self):
+        try:
+            super().__init__(app=None)
+        except TypeError:
+            super().__init__()
+
     def add(self, *args, **kwargs):
         base_add = Router.add
         if hasattr(base_add, '__wrapped__'):
             base_add = base_add.__wrapped__
-        return base_add(self, *args, **kwargs)
+        return base_add.__get__(self, Router)(*args, **kwargs)
 
-    def get(self, request):
+    def get(self, *args):
         base_get = Router.get
         if hasattr(base_get, '__wrapped__'):
             base_get = base_get.__wrapped__
 
-        handler, args, kwargs, uri = base_get(self, request)
-        if request.path == '/server-error':
-            handler = None
-        return handler, args, kwargs, uri
+        if len(args) == 1:
+            path = args[0].path
+        else:
+            path = args[0]
+
+        bound_get = base_get.__get__(self, Router)
+        get_results = list(bound_get(*args))
+        if path == '/server-error':
+            from sanic.exceptions import ServerError
+            raise ServerError("Server Error")
+        return get_results
 
 
-app = Sanic(error_handler=CustomErrorHandler(), router=CustomRouter())
+router = CustomRouter()
+app = Sanic(name="test app", error_handler=CustomErrorHandler(), router=router)
+router.app = app
 
 
 @app.route('/')
@@ -165,5 +180,9 @@ async def async_error(request):
 
 
 app.add_route(MethodView.as_view(), '/method_view')
+
+if not getattr(router, "finalized", True):
+    router.finalize()
+
 if __name__ == '__main__':
     app.run(host='127.0.0.1', port=8000)

tests/framework_sanic/conftest.py

@@ -14,11 +14,12 @@
 
 import pytest
 
+from newrelic.common.object_wrapper import transient_function_wrapper
+
 from testing_support.fixtures import (code_coverage_fixture,
         collector_agent_registration_fixture, collector_available_fixture)
 
 import asyncio
-from sanic.request import Request
 
 _coverage_source = [
     'newrelic.hooks.framework_sanic',
@@ -39,47 +40,82 @@
         default_settings=_default_settings)
 
 
-def create_request_class(method, url, headers=None):
-    _request = Request(
-        method=method.upper(),
-        url_bytes=url.encode('utf-8'),
-        headers=headers,
-        version='1.0',
-        transport=None,
-    )
-    return _request
-
-
-def create_request_coroutine(app, method, url, headers=None, responses=None):
-    if responses is None:
-        responses = []
-
-    def write_callback(response):
-        response.raw_headers = response.output()
-        if b'write_response_error' in response.raw_headers:
-            raise ValueError()
+RESPONSES = []
+
+def create_request_class(app, method, url, headers=None, loop=None):
+    from sanic.request import Request
+    try:
+        _request = Request(
+            method=method.upper(),
+            url_bytes=url.encode('utf-8'),
+            headers=headers,
+            version='1.0',
+            transport=None,
+        )
+    except TypeError:
+        _request = Request(
+            app=app,
+            method=method.upper(),
+            url_bytes=url.encode('utf-8'),
+            headers=headers,
+            version='1.0',
+            transport=None,
+        )
+
+    try:
+        # Manually initialize HTTP protocol
+        from sanic.http import Http, Stage
+        from sanic.server import HttpProtocol
+
+        class MockProtocol(HttpProtocol):
+            async def send(*args, **kwargs):
+                return
+
+        proto = MockProtocol(loop=loop, app=app)
+        proto.recv_buffer = bytearray()
+        http = Http(proto)
+        http.stage = Stage.HANDLER
+        http.response_func = http.http1_response_header
+        _request.stream = http
+        pass
+    except ImportError:
+        pass
 
-        responses.append(response)
+    return _request
 
-    async def stream_callback(response):
-        response.raw_headers = response.get_headers()
-        responses.append(response)
 
+def create_request_coroutine(app, method, url, headers=None, loop=None):
     headers = headers or {}
-    coro = app.handle_request(
-        create_request_class(method, url, headers),
-        write_callback,
-        stream_callback,
-    )
+    try:
+        coro = app.handle_request(create_request_class(app, method, url, headers, loop=loop))
+    except TypeError:
+        def write_callback(response):
+            response.raw_headers = response.output()
+            if b'write_response_error' in response.raw_headers:
+                raise ValueError("write_response_error")
+
+            if response not in RESPONSES:
+                RESPONSES.append(response)
+
+        async def stream_callback(response):
+            response.raw_headers = response.get_headers()
+            if response not in RESPONSES:
+                RESPONSES.append(response)
+
+        coro = app.handle_request(
+            create_request_class(app, method, url, headers, loop=loop),
+            write_callback,
+            stream_callback,
+        )
+
     return coro
 
 
 def request(app, method, url, headers=None):
-    responses = []
-    coro = create_request_coroutine(app, method, url, headers, responses)
     loop = asyncio.get_event_loop()
+    coro = create_request_coroutine(app, method, url, headers, loop)
     loop.run_until_complete(coro)
-    return responses[0]
+    return RESPONSES.pop()
 
 
 class TestApplication(object):
@@ -94,3 +130,19 @@ def fetch(self, method, url, headers=None):
 def app():
     from _target_application import app
     return TestApplication(app)
+
+@pytest.fixture(autouse=True)
+def capture_requests(monkeypatch):
+    from sanic.response import BaseHTTPResponse
+    original = BaseHTTPResponse.__init__
+
+    def capture(*args, **kwargs):
+        original(*args, **kwargs)
+        response = args[0]
+        if getattr(response, "status", None) is None:
+            response.status = 200
+
+        if response not in RESPONSES:
+            RESPONSES.append(response)
+
+    monkeypatch.setattr(BaseHTTPResponse, "__init__", capture)

tests/framework_sanic/test_application.py

@@ -29,9 +29,12 @@
     function_not_called)
 
 
+sanic_21 = int(sanic.__version__.split('.', 1)[0]) >= 21
+
+
 BASE_METRICS = [
     ('Function/_target_application:index', 1),
-    ('Function/_target_application:request_middleware', 2),
+    ('Function/_target_application:request_middleware', 1 if int(sanic.__version__.split('.', 1)[0]) > 18 else 2),
 ]
 FRAMEWORK_METRICS = [
     ('Python/Framework/Sanic/%s' % sanic.__version__, 1),
@@ -108,8 +111,10 @@ def test_inbound_distributed_trace(app):
     response = app.fetch('get', '/', headers=dict(dt_headers))
     assert response.status == 200
 
-
-@pytest.mark.parametrize('endpoint', ('error', 'write_response_error'))
+_params = ["error"]
+if not sanic_21:
+    _params.append('write_response_error')
+@pytest.mark.parametrize('endpoint', _params)
 def test_recorded_error(app, endpoint):
     ERROR_METRICS = [
         ('Function/_target_application:%s' % endpoint, 1),
@@ -304,45 +309,56 @@ def _test():
         app.app.response_middleware = original_response_middleware
 
 
-ERROR_HANDLER_METRICS = [
-    ('Function/_target_application:handle_server_error', 1),
-]
+def error_middleware(*args, **kwargs):
+    raise ValueError("1 != 0")
 
 
-@validate_transaction_metrics(
-        '_target_application:handle_server_error',
-        scoped_metrics=ERROR_HANDLER_METRICS,
-        rollup_metrics=ERROR_HANDLER_METRICS,
-)
-@validate_base_transaction_event_attr
-@validate_transaction_errors(errors=['sanic.exceptions:ServerError'])
-def test_error_handler_transaction_naming(app):
+def test_errors_in_middleware(app):
+    metrics = [('Function/test_application:error_middleware', 1)]
+
+    @validate_transaction_metrics(
+            'test_application:error_middleware',
+            scoped_metrics=metrics,
+            rollup_metrics=metrics,
+    )
+    @validate_base_transaction_event_attr
+    @validate_transaction_errors(errors=['builtins:ValueError'])
+    def _test():
+        response = app.fetch('get', '/')
+        assert response.status == 500
+
     original_request_middleware = deque(app.app.request_middleware)
     original_response_middleware = deque(app.app.response_middleware)
-    app.app.request_middleware = []
-    app.app.response_middleware = []
+    app.app.register_middleware(error_middleware, "request")
 
     try:
-        response = app.fetch('get', '/server-error')
-        assert response.status == 500
+        _test()
     finally:
         app.app.request_middleware = original_request_middleware
         app.app.response_middleware = original_response_middleware
 
 
-@validate_transaction_metrics(
-        '_target_application:CustomRouter.get'
-)
 def test_unknown_route(app):
-    response = app.fetch('get', '/what-route')
-    assert response.status == 404
-
+    import sanic
+    sanic_version = [int(x) for x in sanic.__version__.split(".")]
+    _tx_name = "_target_application:CustomRouter.get" if sanic_version[0] < 21 else "_target_application:request_middleware"
+    
+    @validate_transaction_metrics(_tx_name)
+    def _test():
+        response = app.fetch('get', '/what-route')
+        assert response.status == 404
+    
+    _test()    
 
-@validate_transaction_metrics(
-        '_target_application:CustomRouter.get'
-)
-@override_ignore_status_codes([405])
-@validate_transaction_errors(errors=[])
 def test_bad_method(app):
-    response = app.fetch('post', '/')
-    assert response.status == 405
+    import sanic
+    sanic_version = [int(x) for x in sanic.__version__.split(".")]
+    _tx_name = "_target_application:CustomRouter.get" if sanic_version[0] < 21 else "_target_application:request_middleware"
+
+    @validate_transaction_metrics(_tx_name)
+    @override_ignore_status_codes([405])
+    @validate_transaction_errors(errors=[])
+    def _test():
+        response = app.fetch('post', '/')
+        assert response.status == 405
+    _test()