Fixes route overloading for dynamic routes

[seemethere]

Overview

Addresses #353, now dynamic routes work alongside our newly minted
overloaded routes! Also fixed an unintended side effect where methods
were still being passed in as None for Sanic.add_route.

I'll add in some tests so this shouldn't be merged until I add this in.

[seemethere]

Actually, this doesn't fully work with HTTPMethodView, due to the way it gets added. Maybe it would be better to just have an add_view function that dynamically gets what methods the view is allowed for? What do you think @AntonDnepr?

[seemethere]

Actually have a fix for HTTPMethodView, but it isn't ideal. Also for some reason calls to HEAD method endpoints don't work for me locally. 🤔

[r0fls]

@seemethere how're you testing the HEAD method? This works for me:

from sanic import Sanic
from sanic.response import text

app = Sanic()

@app.head("/")
async def test(request):
    return text("OK")

app.run()

$ curl --head localhost:8000
00
HTTP/1.1 200 OK
Content-Type: text/plain; charset=utf-8
Content-Length: 2
Connection: keep-alive
Keep-Alive: timeout=60

[seemethere]

@r0fls I figured it out, the HEAD method doesn't return a body so trying to verify the body would be useless. Instead I just switched the test to verify the headers!

[r0fls]

Can we incorporate the unit test from #353? Does this pass that?

[r0fls]

I vote we add this test case, or some variant, which passes:

from sanic import Sanic
from sanic.response import text
from sanic.utils import sanic_endpoint_test
from sanic.router import RouteExists
import pytest

@pytest.mark.parametrize("method,attr, expected", [
        ("get", "text", "OK1 test"),
        ("post", "text", "OK2 test"),
        ("put", "text", "OK2 test"),
        ("delete", "status", 405),
])
def test_overload_dynamic_routes(method, attr, expected):
    app = Sanic('test_dynamic_route')

    @app.route('/overload/<param>', methods=['GET'])
    async def handler1(request, param):
        return text('OK1 ' + param)

    @app.route('/overload/<param>', methods=['POST', 'PUT'])
    async def handler2(request, param):
        return text('OK2 ' + param)


    request, response = sanic_endpoint_test(app,
                                            method,
                                            uri='/overload/test')
    assert getattr(response, attr) == expected

Then open an issue when we close the other one, and include this test case:

def test_overload_dynamic_routes_exist():
    app = Sanic('test_dynamic_route')

    @app.route('/overload/<param>', methods=['GET'])
    async def handler1(request, param):
        return text('OK1 ' + param)

    @app.route('/overload/<param>', methods=['POST', 'PUT'])
    async def handler2(request, param):
        return text('OK2 ' + param)


    # if this doesn't raise an error, than at least the below should happen:
    # assert response.text == 'Duplicated' 
    with pytest.raises(RouteExists):
        @app.route('/overload', methods=['PUT', 'DELETE'])
        async def handler3(request):
            return text('Duplicated')

    request, response = sanic_endpoint_test(app,
                                            'PUT',
                                            uri='/overload/test')
    assert response.text == 'Duplicated'

Unless we can get this to pass this second test before we merge. It should either raise an error or update the existing PUT handler.

[youknowone]

Thanks for a nice fix. Now I learned I tried a stupid way than an easy way.

About routing handlers, I suggest:

• class-based view can be a default view handler
• Add common method handler getter to both HTTPMethodView and CompositionView
• Add a wrapper or attribute to function handlers

Then they will share same interface as router's point of view.

About the get-handler overhead, I think Router._get can cache them with uri+method input to specific handler function output.

[seemethere]

@youknowone I think we can simplify the views portion as well as the adding routes portion, for this PR though I think it's good to focus on the hot-fix for dynamic route overloading.

[youknowone]

I also definitely agree about this PR. sorry for making confusion.

[seemethere]

@r0fls I'll add those test cases tomorrow and then push up 0.3.1. Don't merge until I push up the test cases.

[seemethere]

Test cases have been put in, will merge and roll into 0.3.1 later today.