WIP: Add ability to modify matched router parameters in middleware

[ahopkins]

Currently, there is no way to modify the matched parameter values on the router in the middleware.

Part of the problem is that the router is called potentially multiple times in the same request. This PR does not attempt to fix that problem. But, instead it tries to allow both the getting and setting of matched parameter values using the existing request.match_info.

[codecov-io]

Codecov Report

Merging #1373 into master will decrease coverage by 0.08%.
The diff coverage is 81.81%.

@@            Coverage Diff             @@
##           master    #1373      +/-   ##
==========================================
- Coverage    81.5%   81.41%   -0.09%     
==========================================
  Files          17       17              
  Lines        1692     1700       +8     
  Branches      321      322       +1     
==========================================
+ Hits         1379     1384       +5     
- Misses        246      247       +1     
- Partials       67       69       +2

Impacted Files	Coverage Δ
sanic/router.py	92.34% <100%> (-0.36%)	⬇️
sanic/request.py	75.25% <60%> (-0.41%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d06ea9b...7ad7b5b. Read the comment docs.

[vltr]

Oh boy this is awesome! 😎

[sjsadowski]

I like it. Let's get it in to 19.03

[yunstanford]

If not return directly, it'll try _get twice here ?

[vltr]

@yunstanford there's a lot of router.get or router._get calls inside Sanic (in the same request lifecycle), from the Router itself, Request or Protocol ... This is something that annoys me and should be tackled in the future - I was thinking on making a common interface (perhaps using ABC) for these objects.

[ahopkins]

Let's leave this one on hold until after the next release and see what happens (if anything) with the router.

[em92]

Using your branch of sanic (latest commit 4d22b6c).
Example app.

#!/usr/bin/env python3
# -*- coding: utf-8 -*-

from sanic import Sanic

from sanic.response import text

app = Sanic()

@app.middleware
def add_underscore(request):
    if request.match_info and 'username' in request.match_info:
        request.match_info['username'] = "_" + request.match_info['username']


@app.route("/test/<username>")
def http_test(request, username):
    return text(username + "\n")

app.run( host = "0.0.0.0", port = 8080 )

Assuming /test/hey will always return _hey, but:

$ curl http://vm1:8080/test/hey
_hey
$ curl http://vm1:8080/test/hey
__hey
$ curl http://vm1:8080/test/hey
___hey
$ curl http://vm1:8080/test/hey
____hey
$ curl http://vm1:8080/test/hey
_____hey
$ curl http://vm1:8080/test/hey
______hey
$ curl http://vm1:8080/test/hey
_______hey

[em92]

Disabling routing cache fixes bug above, but it does not seem to be appliable solution:

diff --git a/sanic/router.py b/sanic/router.py
index 6110a55..a9bf94c 100644
--- a/sanic/router.py
+++ b/sanic/router.py
@@ -416,7 +416,7 @@ class Router:
         # if methods are None then this logic will prevent an error
         return getattr(route, "methods", None) or frozenset()
 
-    @lru_cache(maxsize=ROUTER_CACHE_SIZE)
     def _get(self, url, method, host):
         """Get a request handler based on the URL of the request, or raises an
         error.  Internal method for caching.

[ahopkins]

Thanks for catching this. We certainly do not want to bypass the route cache.

TODO

• Add test that hits the endpoint multiple times without unintended consequences from caching the value of match_info

[vltr]

I really have a small hunch that the cache may not be the real issue here (although disabling it seems to fix the problem). I'll take a closer look.

[vltr]

The problem here seems to be that the Router.get method is called twice in this request lifecycle, since match_info is a property on the Request object (that calls the router again). I kind of described this request workflow problem on #1317. IMO, the match_info should be a object (dict, list of namedtuple, immutables, Idk, to be discussed) inside the request instance - but that would require a lot of work on the protocol, router, request and I don't know what else (described in the referenced issue) ...

[ahopkins]

@vltr I agree. This is something that belongs on the Request. So, while this is a worthwhile and needed feature, the hacky hoops to jump thru are probably not worth it right now.

We need to spend some effort as a community thinking thru the router. I am putting this on hold until we decide.

[sjsadowski]

Moving to 19.6

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this is incorrect, please respond with an update. Thank you for your contributions.

[ahopkins]

I think we can bump this one over to 19.9.

I have some ideas that I want to tackle regarding the router and have been playing around with some implementation ideas. I think I found a better way to accomplish this and solve some of the repetitive calls to the router in Sanic.

Regardless, I am closing this PR now because I do not think this is the right approach.