search

vardius / gorouter

Go Server/API micro framework, HTTP request router, multiplexer, mux
https://rafallorenz.com/gorouter
MIT License
154 stars 17 forks source link

Fix middleware for wildcard routes #15

Closed vardius closed 4 years ago

codecov[bot] commented 4 years ago

Codecov Report

Merging #15 into master will decrease coverage by 4.23%. The diff coverage is 67.27%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #15      +/-   ##
==========================================
- Coverage   71.26%   67.02%   -4.24%     
==========================================
  Files          10       11       +1     
  Lines         515      552      +37     
==========================================
+ Hits          367      370       +3     
- Misses        134      171      +37     
+ Partials       14       11       -3
Impacted Files Coverage Δ
middleware/collection.go 100% <100%> (ø)
tree.go 90.9% <100%> (+23.05%) :arrow_up:
mux/node.go 45.53% <26.08%> (-3.95%) :arrow_down:
mux/tree.go 33.06% <33.33%> (-1.13%) :arrow_down:
middleware/middleware.go 80% <80%> (-20%) :arrow_down:
fasthttp.go 84.4% <91.66%> (-7.98%) :arrow_down:
nethttp.go 87.61% <97.14%> (-10.56%) :arrow_down:
... and 2 more

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 e898a77...7eb5db7. Read the comment docs.

vardius commented 4 years ago

If you want to know if you should look for other node, because current one was created with a middleware only, I believe assigning custom route is not a solution. Route is a route, and should only be a route, if you can't use route as a handler then it is invalid implementation. Everything should be decoupled, and have only one responsibility.

Lets summarise the state before your commit. There are 2 issues:

  1. Adding middleware might create a node without route that takes priority while matching request path. (Priority due to the fact that tree slice is sorted in order subrouter/static/regexp/wildcard).
  2. Method route middleware is not appended to computed one.

For 2 it is an easy fix, for 1 I believe we should panic and let user handle this in the handler or with use of subrouter. Maybe we could do something like you did. But with proper implementation. Instead of using Route to flag nodes that are empty middleware nodes, we could just create a proper struct boolean property. Simply a flag. And then in the Tree.Match loop instead of checking if route is not nil, we would check node.ourFlag. This could work, but we need to think what happens when user first adds middleware, we flag that route and then he adds a handler. We need to make sure flag is removed.

vardius commented 4 years ago

I reverted your commit, and pushed fix for mentioned issue number 2. Issue number 2 still needs to be fixed. Might be done following your idea, with correct implementation which would be just adding flag to node as I mentioned earlier.

vardius commented 4 years ago

@mar1n3r0 please review this pull request, let me know your thoughts and I believe we can merge it.

Also if there is anything that you did not know before working on this router, and you had to learn/understand how it works yourself, could you summarise it and maybe we could update docs ? Maybe there still is something that you would like me to explain ? That we could also include in the docs.

Benchmark comparison:

master

➜  gorouter git:(master) ✗ go test -bench=. -cpu=4 -benchmem
test
goos: darwin
goarch: amd64
pkg: github.com/vardius/gorouter/v4
BenchmarkStatic1-4                  35997880            27.9 ns/op         0 B/op          0 allocs/op
BenchmarkStatic2-4                  33372828            37.7 ns/op         0 B/op          0 allocs/op
BenchmarkStatic3-4                  28278296            43.6 ns/op         0 B/op          0 allocs/op
BenchmarkStatic5-4                  18393667            57.0 ns/op         0 B/op          0 allocs/op
BenchmarkStatic10-4                 13458920            95.3 ns/op         0 B/op          0 allocs/op
BenchmarkStatic20-4                  7444556           158 ns/op           0 B/op          0 allocs/op
BenchmarkWildcard1-4                 5017465           225 ns/op         496 B/op          5 allocs/op
BenchmarkWildcard2-4                 4917728           300 ns/op         528 B/op          5 allocs/op
BenchmarkWildcard3-4                 4460277           279 ns/op         560 B/op          5 allocs/op
BenchmarkWildcard5-4                 4117964           361 ns/op         624 B/op          5 allocs/op
BenchmarkWildcard10-4                2757624           405 ns/op         784 B/op          5 allocs/op
BenchmarkWildcard20-4                1877768           562 ns/op        1104 B/op          5 allocs/op
BenchmarkRegexp1-4                   3372494           353 ns/op         500 B/op          5 allocs/op
BenchmarkRegexp2-4                   2474242           501 ns/op         532 B/op          5 allocs/op
BenchmarkRegexp3-4                   1929957           663 ns/op         564 B/op          5 allocs/op
BenchmarkRegexp5-4                   1332510           933 ns/op         629 B/op          5 allocs/op
BenchmarkRegexp10-4                   617763          2248 ns/op         791 B/op          5 allocs/op
BenchmarkRegexp20-4                   366548          3350 ns/op        1113 B/op          5 allocs/op
BenchmarkFastHTTPStatic1-4          29598078            35.3 ns/op         2 B/op          1 allocs/op
BenchmarkFastHTTPStatic2-4          27048343            45.7 ns/op         4 B/op          1 allocs/op
BenchmarkFastHTTPStatic3-4          20608680            76.4 ns/op         8 B/op          1 allocs/op
BenchmarkFastHTTPStatic5-4          15883792            72.0 ns/op        16 B/op          1 allocs/op
BenchmarkFastHTTPStatic10-4         11401267           124 ns/op          32 B/op          1 allocs/op
BenchmarkFastHTTPStatic20-4          5862892           217 ns/op          48 B/op          1 allocs/op
BenchmarkFastHTTPWildcard1-4        11145861            98.8 ns/op        66 B/op          3 allocs/op
BenchmarkFastHTTPWildcard2-4         9085635           178 ns/op         100 B/op          3 allocs/op
BenchmarkFastHTTPWildcard3-4         6847353           172 ns/op         136 B/op          3 allocs/op
BenchmarkFastHTTPWildcard5-4         6322854           185 ns/op         208 B/op          3 allocs/op
BenchmarkFastHTTPWildcard10-4        4685559           308 ns/op         384 B/op          3 allocs/op
BenchmarkFastHTTPWildcard20-4        2442094           786 ns/op         720 B/op          3 allocs/op
BenchmarkFastHTTPRegexp1-4           4234552           254 ns/op          69 B/op          3 allocs/op
BenchmarkFastHTTPRegexp2-4           2538032           486 ns/op         113 B/op          3 allocs/op
BenchmarkFastHTTPRegexp3-4           1806182           589 ns/op         145 B/op          3 allocs/op
BenchmarkFastHTTPRegexp5-4           1000000          1078 ns/op         226 B/op          3 allocs/op
BenchmarkFastHTTPRegexp10-4           740905          2075 ns/op         419 B/op          3 allocs/op
BenchmarkFastHTTPRegexp20-4           335192          3066 ns/op         791 B/op          3 allocs/op
PASS
ok      github.com/vardius/gorouter/v4  56.036s

hotfix/middleware-by-path

➜  gorouter git:(hotfix/middleware-by-path) go test -bench=. -cpu=4 -benchmem
test
goos: darwin
goarch: amd64
pkg: github.com/vardius/gorouter/v4
BenchmarkStatic1-4                  33909061            34.3 ns/op         0 B/op          0 allocs/op
BenchmarkStatic2-4                  25284765            47.5 ns/op         0 B/op          0 allocs/op
BenchmarkStatic3-4                  21225134            54.6 ns/op         0 B/op          0 allocs/op
BenchmarkStatic5-4                  14245224            72.2 ns/op         0 B/op          0 allocs/op
BenchmarkStatic10-4                  9515647           127 ns/op           0 B/op          0 allocs/op
BenchmarkStatic20-4                  5496370           218 ns/op           0 B/op          0 allocs/op
BenchmarkWildcard1-4                 4730401           227 ns/op         496 B/op          5 allocs/op
BenchmarkWildcard2-4                 4525063           261 ns/op         528 B/op          5 allocs/op
BenchmarkWildcard3-4                 4346812           283 ns/op         560 B/op          5 allocs/op
BenchmarkWildcard5-4                 3658440           318 ns/op         624 B/op          5 allocs/op
BenchmarkWildcard10-4                2758533           411 ns/op         784 B/op          5 allocs/op
BenchmarkWildcard20-4                1871103           639 ns/op        1104 B/op          5 allocs/op
BenchmarkRegexp1-4                   3286542           362 ns/op         501 B/op          5 allocs/op
BenchmarkRegexp2-4                   2416214           508 ns/op         533 B/op          5 allocs/op
BenchmarkRegexp3-4                   1846933           687 ns/op         565 B/op          5 allocs/op
BenchmarkRegexp5-4                   1342232           911 ns/op         630 B/op          5 allocs/op
BenchmarkRegexp10-4                   673621          1559 ns/op         790 B/op          5 allocs/op
BenchmarkRegexp20-4                   377835          2923 ns/op        1113 B/op          5 allocs/op
BenchmarkFastHTTPStatic1-4          25817391            47.7 ns/op         2 B/op          1 allocs/op
BenchmarkFastHTTPStatic2-4          21749991            55.9 ns/op         4 B/op          1 allocs/op
BenchmarkFastHTTPStatic3-4          17201960            69.5 ns/op         8 B/op          1 allocs/op
BenchmarkFastHTTPStatic5-4          13123863            88.9 ns/op        16 B/op          1 allocs/op
BenchmarkFastHTTPStatic10-4          8274832           147 ns/op          32 B/op          1 allocs/op
BenchmarkFastHTTPStatic20-4          4877264           249 ns/op          48 B/op          1 allocs/op
BenchmarkFastHTTPWildcard1-4        10780489           106 ns/op          66 B/op          3 allocs/op
BenchmarkFastHTTPWildcard2-4         8331529           145 ns/op         100 B/op          3 allocs/op
BenchmarkFastHTTPWildcard3-4         7211712           245 ns/op         136 B/op          3 allocs/op
BenchmarkFastHTTPWildcard5-4         3829846           289 ns/op         208 B/op          3 allocs/op
BenchmarkFastHTTPWildcard10-4        3918559           360 ns/op         384 B/op          3 allocs/op
BenchmarkFastHTTPWildcard20-4        1796622           583 ns/op         720 B/op          3 allocs/op
BenchmarkFastHTTPRegexp1-4           4201428           267 ns/op          70 B/op          3 allocs/op
BenchmarkFastHTTPRegexp2-4           3115568           429 ns/op         113 B/op          3 allocs/op
BenchmarkFastHTTPRegexp3-4           2258680           521 ns/op         145 B/op          3 allocs/op
BenchmarkFastHTTPRegexp5-4           1577082           785 ns/op         225 B/op          3 allocs/op
BenchmarkFastHTTPRegexp10-4           726134          1620 ns/op         419 B/op          3 allocs/op
BenchmarkFastHTTPRegexp20-4           343843          2915 ns/op         791 B/op          3 allocs/op
PASS
ok      github.com/vardius/gorouter/v4  56.069s
mar1n3r0 commented 4 years ago

I reckon this is definitely a step in the right direction and I also realized something that you mentioned before and we didn't account for yet.

To be honest I am not sure that this issue, can be solved with the current tree implementation.

Basically it all comes down to this:

// try to find node by matching name against nodes
if node == nil {
    node, _, _, _ = t.Match(name)
}

What happens here is when we attempt to match it we essentially reference the wildcard one found and matched - x/{param}.

This reference is undesirable and has side effects in other matches after because the tree remains [name: x/{param}] instead of becoming [0][name: x/{param}], [1][name: x/x]. Our aim though is only to verify the match so that we can justify creating our new node with the static route. This is different in nature because we don't want to reference x/{param} on match but to merely get a confirmation that we can create a new node with the static route and add it to the tree.

Two approaches stem from that to solve this:

Scenario one - reuse the same tree and add the new node on match:

// try to find node by matching name against nodes
if node == nil {
    if node, _, _, _ = t.Match(name); node == nil {
                panic("Could not find node for given path")
        } else {
               newNode := NewNode(parts[0], 0)
               newTree = t.withNode(newNode)
        }
}

The main difference seems to be that when the request comes in without that change it loops only through x/{param} because we didn't create a new node we just referenced the matching one while with the above change the request will be matched against both x/{param} and x/x because we created and added a new node to the tree once we verified that x/x is matching x/{param}. The same is valid for the middleware part without the above change the middleware will be attached to x/{param} because we referenced it instead of creating a new node. Thus it will incorrectly map the middleware to x/{param} instead of x/x as intended.

Scenario two - create two separate independent trees and match the request to each of them sequentially:

Step one: Create accessTree with access routes defined by router.GET: accessTree := [name: x/{param}]

// WithRoute returns new Tree with Route set to Node // Route is set to Node under the give path, if Node does not exist it is created func (t Tree) WithRoute(path string, route Route, maxParamsSize uint8) Tree { path = pathutils.TrimSlash(path) if path == "" { return t }

parts := strings.Split(path, "/")
name, _ := pathutils.GetNameFromPart(parts[0])
node := t.Find(name)
accessTree := t

if node == nil {
    node = NewNode(parts[0], maxParamsSize)
    accessTree = t.withNode(node)
}

if len(parts) == 1 {
    node.WithRoute(route)
} else {
    node.WithChildren(node.Tree().WithRoute(strings.Join(parts[1:], "/"), route, node.MaxParamsSize()))
}

return accessTree

}

Step two: Create middlewareTree with middleware routes defined by router.USE which are first matched to the corresponding same depth nodes in the accessTree to be verified matching but not referencing them: 

// WithMiddleware returns new Tree with Middleware appended to given Node
// Middleware is appended to Node under the give path, if Node does not exist it will panic
func (t Tree) WithMiddleware(path string, m middleware.Middleware) Tree {
    path = pathutils.TrimSlash(path)
    if path == "" {
        return t
    }

    parts := strings.Split(path, "/")
    name, _ := pathutils.GetNameFromPart(parts[0])
    node := t.Find(name)
    middlewareTree := t

    // try to find node by matching name against nodes
        if node == nil {
             if node, _, _, _ = t.Match(name); node == nil {
                         panic("Could not find node for given path")
                 } else {
                         middlewareTree = NewNode(parts[0], maxParamsSize)
                         middlewareTree = t.withNode(newNode)
                 }
        }

    if len(parts) == 1 {
        node.AppendMiddleware(m)
    } else {
        node.WithChildren(node.Tree().WithMiddleware(strings.Join(parts[1:], "/"), m))
    }

    return middlewareTree
}

That way we end up with two trees:

accessTree: [name: x/{param}] - no middleware attached

middlewareTree: [name: x/x] - middleware attached

When the request comes in:

    router.GET("/x/{param}", func(ctx *fasthttp.RequestCtx) {
        params := ctx.UserValue("params").(context.Params)
        if _, err := fmt.Fprintf(ctx, "%s", params.Value("param")); err != nil {
            t.Fatal(err)
        }
    })

    router.USE(http.MethodGet, "/x/x", mockFastHTTPMiddleware("m1"))

    ctx := buildFastHTTPRequestContext(http.MethodGet, "/x/y")

First loop router.GET("/x/{param}": 

If root := r.routes.Find(method); root != nil {
        if node, treeMiddleware, params, subPath := root.accessTree().Match("x/{param}"); node != nil && node.Route() != nil {
              // accessTree now contains one node with name x/{param} and no middleware

Second loop router.USE("x/x"):

If root := r.routes.Find(method); root != nil {
        if node, treeMiddleware, params, subPath := root.middlewareTree().Match("x/x"); node != nil && node.Route() != nil {
              // middlewareTree now contains one node with name x/x and a middleware attached

Third loop ctx := buildFastHTTPRequestContext(http.MethodGet, "/x/y"):

Option one - match accessTree first if successful try matching middlewareTree: 

               if node, treeMiddleware, params, subPath := root.accessTree().Match("x/y"); node != nil && node.Route() != nil {
                       if node, treeMiddleware, params, subPath := root.middlewareTree().Match("x/y"); node != nil && node.Route() != nil {

Option two:

Change structure of type Tree from []Node to:

type accessTree []Node

type middlewareTree []Node

type Tree struct {
        *accessTree
        *middlewareTree
}

Create two more Match methods one for each type like the Match method for the nodes:

// Match path to accessTree Node
func (t *accessTree) Match(path string) (Node, middleware.Middleware, context.Params, string) {
    for _, child := range t {
        if node, m, params, subPath := child.Match(path); node != nil {
            return node, m, params, subPath
        }
    }

    return nil, nil, nil, ""
}
// Match path to middlewareTree Node
func (t *middlewareTree) Match(path string) (Node, middleware.Middleware, context.Params, string) {
    for _, child := range t {
        if node, m, params, subPath := child.Match(path); node != nil {
            return node, m, params, subPath
        }
    }

    return nil, nil, nil, ""
}

I think scenario two is very tedious and creates quite a lot of overhead and complexity so if scenario one works we can avoid resorting to that although as the router matures with more tests and functionality it might become inevitable as a new way to structure the Tree with sub-trees to separate nodes by concerns - access nodes checking only if the request shall pass and independent middleware nodes which are in a separate tree for checking if they match access node routes.

What do you think about all this, are we missing something, do you see any major flaws ?

Everything above is just my grasp so far and I am not yet 100% sure about it because I still don't understand all aspects of tree creation in the original code for example why:

node := t.Find(name)
newTree := t

and it is still working while I was expecting that the tree will be empty before calling t.WithNode(node):

node := t.Find(name)
newTree := t.withNode(node)

A hint about that would be more than welcome.

In the meantime I will write some granular tests to try to cover all of the above theory.

Edit: After doing some tests I think that the only viable option is scenario 2. Basically no matter how we tweak it we will always end up with one tree. At the same time the right way for the router to traverse the tree when matching the request is to go first through the tree with router.GET generated nodes to ensure access to proceed to the router.USE generated tree. After this match succeeds we start traversing and matching the middleware generated nodes which are held in a separate tree as discussed above.

vardius commented 4 years ago

Benchmark comparison:

master

➜  gorouter git:(master) ✗ go test -bench=. -cpu=4 -benchmem
test
goos: darwin
goarch: amd64
pkg: github.com/vardius/gorouter/v4
BenchmarkStatic1-4                  35997880            27.9 ns/op         0 B/op          0 allocs/op
BenchmarkStatic2-4                  33372828            37.7 ns/op         0 B/op          0 allocs/op
BenchmarkStatic3-4                  28278296            43.6 ns/op         0 B/op          0 allocs/op
BenchmarkStatic5-4                  18393667            57.0 ns/op         0 B/op          0 allocs/op
BenchmarkStatic10-4                 13458920            95.3 ns/op         0 B/op          0 allocs/op
BenchmarkStatic20-4                  7444556           158 ns/op           0 B/op          0 allocs/op
BenchmarkWildcard1-4                 5017465           225 ns/op         496 B/op          5 allocs/op
BenchmarkWildcard2-4                 4917728           300 ns/op         528 B/op          5 allocs/op
BenchmarkWildcard3-4                 4460277           279 ns/op         560 B/op          5 allocs/op
BenchmarkWildcard5-4                 4117964           361 ns/op         624 B/op          5 allocs/op
BenchmarkWildcard10-4                2757624           405 ns/op         784 B/op          5 allocs/op
BenchmarkWildcard20-4                1877768           562 ns/op        1104 B/op          5 allocs/op
BenchmarkRegexp1-4                   3372494           353 ns/op         500 B/op          5 allocs/op
BenchmarkRegexp2-4                   2474242           501 ns/op         532 B/op          5 allocs/op
BenchmarkRegexp3-4                   1929957           663 ns/op         564 B/op          5 allocs/op
BenchmarkRegexp5-4                   1332510           933 ns/op         629 B/op          5 allocs/op
BenchmarkRegexp10-4                   617763          2248 ns/op         791 B/op          5 allocs/op
BenchmarkRegexp20-4                   366548          3350 ns/op        1113 B/op          5 allocs/op
BenchmarkFastHTTPStatic1-4          29598078            35.3 ns/op         2 B/op          1 allocs/op
BenchmarkFastHTTPStatic2-4          27048343            45.7 ns/op         4 B/op          1 allocs/op
BenchmarkFastHTTPStatic3-4          20608680            76.4 ns/op         8 B/op          1 allocs/op
BenchmarkFastHTTPStatic5-4          15883792            72.0 ns/op        16 B/op          1 allocs/op
BenchmarkFastHTTPStatic10-4         11401267           124 ns/op          32 B/op          1 allocs/op
BenchmarkFastHTTPStatic20-4          5862892           217 ns/op          48 B/op          1 allocs/op
BenchmarkFastHTTPWildcard1-4        11145861            98.8 ns/op        66 B/op          3 allocs/op
BenchmarkFastHTTPWildcard2-4         9085635           178 ns/op         100 B/op          3 allocs/op
BenchmarkFastHTTPWildcard3-4         6847353           172 ns/op         136 B/op          3 allocs/op
BenchmarkFastHTTPWildcard5-4         6322854           185 ns/op         208 B/op          3 allocs/op
BenchmarkFastHTTPWildcard10-4        4685559           308 ns/op         384 B/op          3 allocs/op
BenchmarkFastHTTPWildcard20-4        2442094           786 ns/op         720 B/op          3 allocs/op
BenchmarkFastHTTPRegexp1-4           4234552           254 ns/op          69 B/op          3 allocs/op
BenchmarkFastHTTPRegexp2-4           2538032           486 ns/op         113 B/op          3 allocs/op
BenchmarkFastHTTPRegexp3-4           1806182           589 ns/op         145 B/op          3 allocs/op
BenchmarkFastHTTPRegexp5-4           1000000          1078 ns/op         226 B/op          3 allocs/op
BenchmarkFastHTTPRegexp10-4           740905          2075 ns/op         419 B/op          3 allocs/op
BenchmarkFastHTTPRegexp20-4           335192          3066 ns/op         791 B/op          3 allocs/op
PASS
ok      github.com/vardius/gorouter/v4  56.036s

hotfix/middleware-by-path

➜  gorouter git:(hotfix/middleware-by-path) ✗ go test -bench=. -cpu=4 -benchmem
test
goos: darwin
goarch: amd64
pkg: github.com/vardius/gorouter/v4
BenchmarkStatic1-4                  28802757            46.3 ns/op         0 B/op          0 allocs/op
BenchmarkStatic2-4                  20745105            51.6 ns/op         0 B/op          0 allocs/op
BenchmarkStatic3-4                  17873828            63.0 ns/op         0 B/op          0 allocs/op
BenchmarkStatic5-4                  12449367            89.2 ns/op         0 B/op          0 allocs/op
BenchmarkStatic10-4                  7169173           156 ns/op           0 B/op          0 allocs/op
BenchmarkStatic20-4                  3915980           298 ns/op           0 B/op          0 allocs/op
BenchmarkWildcard1-4                 4210562           259 ns/op         496 B/op          5 allocs/op
BenchmarkWildcard2-4                 3726679           304 ns/op         528 B/op          5 allocs/op
BenchmarkWildcard3-4                 3704440           321 ns/op         560 B/op          5 allocs/op
BenchmarkWildcard5-4                 2829639           367 ns/op         624 B/op          5 allocs/op
BenchmarkWildcard10-4                2339979           530 ns/op         784 B/op          5 allocs/op
BenchmarkWildcard20-4                1485880           946 ns/op        1104 B/op          5 allocs/op
BenchmarkRegexp1-4                   2667024           410 ns/op         500 B/op          5 allocs/op
BenchmarkRegexp2-4                   2204582           533 ns/op         532 B/op          5 allocs/op
BenchmarkRegexp3-4                   1413793           959 ns/op         566 B/op          5 allocs/op
BenchmarkRegexp5-4                   1000000          1020 ns/op         630 B/op          5 allocs/op
BenchmarkRegexp10-4                   621724          1808 ns/op         791 B/op          5 allocs/op
BenchmarkRegexp20-4                   337516          3303 ns/op        1115 B/op          5 allocs/op
BenchmarkFastHTTPStatic1-4          22720663            51.7 ns/op         2 B/op          1 allocs/op
BenchmarkFastHTTPStatic2-4          14602131            69.3 ns/op         4 B/op          1 allocs/op
BenchmarkFastHTTPStatic3-4          13987215            87.2 ns/op         8 B/op          1 allocs/op
BenchmarkFastHTTPStatic5-4          10888626           112 ns/op          16 B/op          1 allocs/op
BenchmarkFastHTTPStatic10-4          6836913           181 ns/op          32 B/op          1 allocs/op
BenchmarkFastHTTPStatic20-4          3270436           350 ns/op          48 B/op          1 allocs/op
BenchmarkFastHTTPWildcard1-4         8443278           125 ns/op          66 B/op          3 allocs/op
BenchmarkFastHTTPWildcard2-4         7232194           155 ns/op         100 B/op          3 allocs/op
BenchmarkFastHTTPWildcard3-4         6635278           179 ns/op         136 B/op          3 allocs/op
BenchmarkFastHTTPWildcard5-4         4677546           242 ns/op         208 B/op          3 allocs/op
BenchmarkFastHTTPWildcard10-4        3079581           362 ns/op         384 B/op          3 allocs/op
BenchmarkFastHTTPWildcard20-4        1788163           698 ns/op         720 B/op          3 allocs/op
BenchmarkFastHTTPRegexp1-4           5048209           247 ns/op          70 B/op          3 allocs/op
BenchmarkFastHTTPRegexp2-4           2915995           439 ns/op         112 B/op          3 allocs/op
BenchmarkFastHTTPRegexp3-4           2035629           569 ns/op         145 B/op          3 allocs/op
BenchmarkFastHTTPRegexp5-4           1375347           839 ns/op         225 B/op          3 allocs/op
BenchmarkFastHTTPRegexp10-4           619560          1649 ns/op         419 B/op          3 allocs/op
BenchmarkFastHTTPRegexp20-4           422613          3054 ns/op         790 B/op          3 allocs/op
PASS
ok      github.com/vardius/gorouter/v4  53.734s
vardius commented 4 years ago

Would be good to add some tests to hit 100% coverage or at least close to that.

mar1n3r0 commented 4 years ago

It will happen slowly over time as I discover more cases to test.

mar1n3r0 commented 4 years ago

It was meant to be a quick demo not really intended to be merged.

vardius commented 4 years ago

@mar1n3r0 can you have a look and let me know if this changes are easy to understand. we want code to be readable and not complex

mar1n3r0 commented 4 years ago

Works like a charm! The solution seems easy to comprehend. You avoided creating a new tree by managing middleware order per node instead. If I got that right then it's understandable :)

vardius commented 4 years ago

@mar1n3r0 so you feel like adding some test cases to increase the coverage so all check are passing ? https://github.com/vardius/gorouter/pull/15#issuecomment-570126051

I think with some more test we can merge and release a new version.

mar1n3r0 commented 4 years ago

I think codecov is only relative and should not be taken as an absolute criteria to achieve numbers - the coverage percentage depends on the architecture of the code . In our case a lot of the functionality can not be tested outside of the context of fasthttp/nethttp. For example MatchRoute and MatchMiddleware require a node with route. This is only available within a full request cycle where there is a request handler. To the same extend this is valid for the dependency between the tree and the nodes. Because Match is recursively calling children.Match and referencing the tree from within the node we can not isolate this logic in node tests.