Static path middleware is applied to all wildcard paths

[vardius]

    router := gorouter.New()
    router.GET("/hello/{name}", http.HandlerFunc(Hello))

    router.USE("GET", "/hello/Joe", someMiddleware)

    log.Fatal(http.ListenAndServe(":8080", router))

Given the example above middleware for path /hello/Joe will be applied to wildcard route /hello/{name} as the path of middleware matches node's path, resulting in middleware being invoked for all requests matching node's pattern.

• /hello/Joe
• /hello/Jerry

etc.

[vardius]

https://github.com/vardius/gorouter/pull/15

[vardius]

Temporary fix would be to extract static route that we want to apply middleware for:

    router := gorouter.New()
    helloHandler := http.HandlerFunc(Hello)

    router.GET("/hello/Joe", helloHandler)
    router.USE("GET", "/hello/Joe", someMiddleware)

    router.GET("/hello/{name}", helloHandler)

    log.Fatal(http.ListenAndServe(":8080", router))

Anyone having this issue right now ? How urgent it is ?

[mar1n3r0]

Yes that explains it.

[mar1n3r0]

Doing quite a lot of testing.

From what I can see so far by the time the request reaches the router handler the middleware is already appended. This is easily demonstrated by the fact that the decision whether to add middleware or not is taken at the time of calling router.USE() when there is no info yet as to what the request path would be rather than at the time of processing the request.

Not sure how hard it is to change the core logic but to me it seems that router.USE() should only keep a map and return it as a reference to the router handler so that the actual evaluation whether to add middleware happens at execution time by comparing request path and the path value from the map returned by addMiddleware().

In that case maybe we can add a function called mapMiddleware() to be called at router.USE() execution time and call addMiddleware() later from within HandleFastHTTP().

Edit: I think there is even a more elegant solution since AppendMiddleware is a two-phase commit so to speak. If we move the compose action in a separate method say ComposeMiddleware and keep track of path reference at router.USE execution we can compare middleware path and request path at Handler execution.

[vardius]

https://github.com/vardius/gorouter/blob/9233b0485a17c45a4f5e443830a2c6fc27b0a5c9/tree.go#L15

What happens now while adding middleware is:

• we try to find routes that match that path
• we add middleware to all routes from that position in a mux tree

Ideal solution would be To update Middleware type containing path as well as you suggested Store middleware in the same tree ? Similar to nodes And append it to routes while traversing tree

The reason why it is computed before is to improve performance.

This issue needs much more thinking.

For now maybe something like this ?

1. .USE() adds middleware to the tree (nodes tree)
2. when request comes we traverse tree to find matching node, on the way collecting all the middleware that applies to all routes below
3. once we find our node, we compute the handler (same thing we do before)

The question for me is how to do it, not loosing performance or maybe there is a way to do it and improve it ?

[mar1n3r0]

I think that's the price to pay when using wildcards in general. In exchange for flexibility you get a bit of extra heap load because there is no way to predict a match and compute it beforehand when the possibilities are endless. Maybe it can be partially computed for reg exp but that would be again at the expense of extra computational resources to run through the cached data before making a decision whether to apply the middleware.

Anyways let's run the benchmarks and see if the performance loss is actually significant ?

[vardius]

@mar1n3r0 what do you think about possible solution like this one: https://github.com/vardius/gorouter/pull/15/commits/cfe703a18303ea97b61e9d9d5d0b83cdf01539b1

basically we keep creating orphan nodes, but once we match agains one we store them in a tmp slice to append middleware to a node with a route later one. this has advantages:

• we are sure middleware should be applied as it matched agains request path
• we do not need more then one node tree if there is no node with route we return first matched orphan node
• this allows us to keep router logic the same potentially avoid new bugs

this solution most likely will need deeper thinking and fixing eventual edge cases, but as an idea should work, what do you think ?

[mar1n3r0]

It does look like a very elegant solution and it passes the node reference test.

Brief testing reveals two minor things:

• TestNodeApplyMiddleware order of middleware reversed
• static route and middleware produce a result omitting the request path: 1853262ea0bcff88033c604a040dba4fe44dd5d4

[vardius]

I removed the panic test, we will not panic if middleware was applied to tree that technically will never be executed.

Also i think we will just explain how order for orphan nodes middleware will work and will leave it like that.

[mar1n3r0]

We can still reverse it the same place where it's added. Seems to be working as long as middleware is defined descending broad to specific and wildcard to static. #22

[vardius]

Please read my comment https://github.com/vardius/gorouter/pull/22#pullrequestreview-346199845

[mar1n3r0]

I guess this one has been solved so we can close it and open a new open specifically about ordering not only about orphaned nodes but in general.