Reverse order of middleware slice when adding orphan node. #22

mar1n3r0 · 2020-01-21T11:08:49Z

Add notes to docs that middleware is defined descending broader to specific and wildcard to static.

…to docs that middleware is defined descending broader to specific and wildcard to static

codecov · 2020-01-21T11:11:00Z

Codecov Report

Merging #22 into hotfix/middleware-by-path will increase coverage by 0.31%.
The diff coverage is 100%.

@@                      Coverage Diff                      @@
##           hotfix/middleware-by-path      #22      +/-   ##
=============================================================
+ Coverage                      71.59%   71.91%   +0.31%     
=============================================================
  Files                             10       10              
  Lines                            528      534       +6     
=============================================================
+ Hits                             378      384       +6     
  Misses                           135      135              
  Partials                          15       15

Impacted Files	Coverage Δ
mux/tree.go	`37.69% <ø> (ø)`	⬆️
middleware/middleware.go	`100% <100%> (ø)`	⬆️

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 c571661...59e48ed. Read the comment docs.

middleware/middleware_test.go

vardius · 2020-01-21T21:21:53Z

mux/tree.go

@@ -87,6 +87,7 @@ func (t Tree) Match(path string) (Node, middleware.Middleware, context.Params, s
 				if len(orphanMatches) > 0 {
 					for i := 0; i < len(orphanMatches); i++ {
 						m = orphanMatches[i].node.Middleware().Merge(m)
+						m.Reverse()


First of all adding Reverse() adds lots of complexity to a router, slowing it down. Easier is just to change loop above for i := 0; i < len(orphanMatches); i++.

Anyway I don't think there is a need to revers order at all. Overall it is working correctly, except cases where static middleware is applied to wildcard route. Given this example:

router.GET("/x/{param}", func(ctx *fasthttp.RequestCtx) {}) router.USE(http.MethodGet, "/x/{param}", mockFastHTTPMiddleware("m1")) router.USE(http.MethodGet, "/x/y", mockFastHTTPMiddleware("m2"))

You would expect m1 to be called first (in order they were created) but because this middleware was added to different tree nodes, and those nodes are ordered, m2 is called before m1.

We could change the order, but it is not a solution. Given the example:

router.GET("/x/{param}", func(ctx *fasthttp.RequestCtx) {}) router.USE(http.MethodGet, "/x/{param}", mockFastHTTPMiddleware("m1")) router.USE(http.MethodGet, "/x/y", mockFastHTTPMiddleware("m2")) router.USE(http.MethodGet, "/x/y", mockFastHTTPMiddleware("m3"))

The order here is m3->m2->m1 which is semi correct (m2 is after m3), as the order is incorrect only when merging orphan nodes with the main middleware tree (meaning static middleware is applied before wildcard one). Reversing order for the whole middleware slice, will result in completely different order for all of middleware which is incorrect.

Overall it is working correctly, except cases where static middleware is applied to wildcard route.

The issue for me is that this is my main use case.

Reversing order for the whole middleware slice, will result in completely different order for all of middleware which is incorrect.

Totally get your point. Let's elaborate further. I have added a deep nested static case to test if it will break the logic. Please note that the reversing happens only within the oprhaned match loop so it doesn't reorder items on other occasions. So far I couldn't produce incorrect results.

Benchmarks also remain stellar nevertheless:

go test -bench=. -cpu=4 test goos: linux goarch: amd64 pkg: github.com/vardius/gorouter/v4 BenchmarkStatic1-4 58706526 19.7 ns/op BenchmarkStatic2-4 45630166 26.5 ns/op BenchmarkStatic3-4 33508587 33.4 ns/op BenchmarkStatic5-4 21932134 46.0 ns/op BenchmarkStatic10-4 12442113 84.7 ns/op BenchmarkStatic20-4 7243618 154 ns/op BenchmarkWildcard1-4 6618468 181 ns/op BenchmarkWildcard2-4 5729289 205 ns/op BenchmarkWildcard3-4 5328877 219 ns/op BenchmarkWildcard5-4 4603405 255 ns/op BenchmarkWildcard10-4 3544700 340 ns/op BenchmarkWildcard20-4 2276683 540 ns/op BenchmarkRegexp1-4 4256842 273 ns/op BenchmarkRegexp2-4 3283544 369 ns/op BenchmarkRegexp3-4 2506996 485 ns/op BenchmarkRegexp5-4 1734328 655 ns/op BenchmarkRegexp10-4 1000000 1219 ns/op BenchmarkRegexp20-4 488301 2585 ns/op BenchmarkFastHTTPStatic1-4 30919155 34.5 ns/op BenchmarkFastHTTPStatic2-4 25989915 46.4 ns/op BenchmarkFastHTTPStatic3-4 20417211 60.7 ns/op BenchmarkFastHTTPStatic5-4 14565698 79.8 ns/op BenchmarkFastHTTPStatic10-4 8582150 143 ns/op BenchmarkFastHTTPStatic20-4 5158412 245 ns/op BenchmarkFastHTTPWildcard1-4 12319296 92.9 ns/op BenchmarkFastHTTPWildcard2-4 9517089 125 ns/op BenchmarkFastHTTPWildcard3-4 8331864 146 ns/op BenchmarkFastHTTPWildcard5-4 6278666 190 ns/op BenchmarkFastHTTPWildcard10-4 4149974 289 ns/op BenchmarkFastHTTPWildcard20-4 2362891 506 ns/op BenchmarkFastHTTPRegexp1-4 6121318 206 ns/op BenchmarkFastHTTPRegexp2-4 3767931 329 ns/op BenchmarkFastHTTPRegexp3-4 2734198 447 ns/op BenchmarkFastHTTPRegexp5-4 1807521 702 ns/op BenchmarkFastHTTPRegexp10-4 771120 1299 ns/op BenchmarkFastHTTPRegexp20-4 451314 2562 ns/op PASS ok github.com/vardius/gorouter/v4 52.010s

middleware/middleware.go

vardius · 2020-01-22T21:12:26Z

mux/tree.go

@@ -87,6 +87,7 @@ func (t Tree) Match(path string) (Node, middleware.Middleware, context.Params, s
 				if len(orphanMatches) > 0 {
 					for i := 0; i < len(orphanMatches); i++ {
 						m = orphanMatches[i].node.Middleware().Merge(m)
+						m.Reverse()


By doing m.Reverse() inside a loop you revers it every iteration
in the end depending of the orphanMatches slice length, odd or even resulting in reversed or not reversed order

also as I said no need to do another loop, if you need to refer which I do not think we need to. just change the logic in the for loop to integrate from end of slice.

No matter how I tweaked the for loop I could not get the result achieved with a separate reverse.
Maybe you can provide an example working as described ?

Can you please explain what is it that you are trying achieve ? Maybe I dont understand what you want to do.

For me it works fine, on the main branch (vardius:hotfix/middleware-by-path)

The example test case I suggested is passing i main branch

And if we want to fix here anything is the middleware order of nodes from the same tree level that are applied to route node.

This is the order middleware was applied

router.USE(http.MethodGet, "/", mockFastHTTPMiddleware("m1->")) router.USE(http.MethodGet, "/", mockFastHTTPMiddleware("m2->")) router.USE(http.MethodGet, "/x", mockFastHTTPMiddleware("mx1->")) router.USE(http.MethodGet, "/x", mockFastHTTPMiddleware("mx2->")) router.USE(http.MethodGet, "/x/{param}", mockFastHTTPMiddleware("mparam1->")) router.USE(http.MethodGet, "/x/{param}", mockFastHTTPMiddleware("mparam2->")) router.USE(http.MethodGet, "/x/y", mockFastHTTPMiddleware("mxy1->")) router.USE(http.MethodGet, "/x/y", mockFastHTTPMiddleware("mxy2->"))

And this would be the fix:

- m1->m2->mx1->mx2->mxy1->mxy2->mparam1->mparam2->handler + m1->m2->mx1->mx2->mparam1->mparam2->mxy1->mxy2->handler

Note the change mxy1->mxy2->mparam1->mparam2 -> mparam1->mparam2->mxy1->mxy2.

But this can not be easily achieved, and i suppose we will not do it.

Yes, exactly. This is the issue in most of the tests:

--- FAIL: TestNodeApplyMiddlewareDeepStatic (0.00s) nethttp_test.go:500: Use middleware error m1m3m2x --- FAIL: TestFastHTTPNodeApplyMiddlewareDeepWildcardStaticMix (0.00s) fasthttp_test.go:483: Use middleware error m1->m2->mx1->mx2->mxy1->mxy2->mparam1->mparam2->handler --- FAIL: TestNodeApplyMiddleware (0.00s) nethttp_test.go:432: Use middleware error m2m1y nethttp_test.go:446: Use middleware error m3m1x --- FAIL: TestFastHTTPNodeApplyMiddlewareDeepStatic (0.00s) fasthttp_test.go:458: Use middleware error m1m3m2x --- FAIL: TestFastHTTPNodeApplyMiddleware (0.00s) fasthttp_test.go:402: Use middleware error m2m1y fasthttp_test.go:412: Use middleware error m3m1x

I think we need to define what we call essential functionality for a router to be meeting basic needs. This example is more of a scientific proof but all the others not passing are basically essential to call it working as expected. In more than 50% of the cases it's a wildcard route and middleware for wildcard and static so if we have to guess the order or document that it will be reversed always that makes it unpredictable even for basic intended usage.

fasthttp_test.go

mar1n3r0 · 2020-01-23T14:14:20Z

Closing this as we figured out reverse is not an option.

Reverse order of middleware slice when adding orphan node. Add notes …

b1edc06

…to docs that middleware is defined descending broader to specific and wildcard to static

mar1n3r0 mentioned this pull request Jan 21, 2020

Static path middleware is applied to all wildcard paths #14

Closed

Move ReverseAny helper to middleware package and add to middleware tests

a1bf4d3

golangcibot reviewed Jan 21, 2020

View reviewed changes

middleware/middleware_test.go Outdated Show resolved Hide resolved

mar1n3r0 added 2 commits January 21, 2020 13:29

Change name from ReverseAny to Reverse

4de2a71

Add errcheck for write response

df97710

vardius reviewed Jan 21, 2020

View reviewed changes

Add deep static case to test reverse logic and performance

b242fb8

vardius requested changes Jan 22, 2020

View reviewed changes

mar1n3r0 added 3 commits January 23, 2020 10:35

add deep static case to nethttp tests

db65ff0

replace reflect with len for slice m

3819669

Add deep wildcardstaticmix case to tests

59e48ed

mar1n3r0 closed this Jan 23, 2020

mar1n3r0 mentioned this pull request Jan 23, 2020

General middleware execution order #23

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reverse order of middleware slice when adding orphan node. #22

Reverse order of middleware slice when adding orphan node. #22

mar1n3r0 commented Jan 21, 2020

codecov bot commented Jan 21, 2020 •

edited

Loading

vardius Jan 21, 2020 •

edited

Loading

mar1n3r0 Jan 22, 2020 •

edited

Loading

vardius Jan 22, 2020 •

edited

Loading

mar1n3r0 Jan 23, 2020

vardius Jan 23, 2020 •

edited

Loading

vardius Jan 23, 2020

mar1n3r0 Jan 23, 2020

mar1n3r0 commented Jan 23, 2020

Reverse order of middleware slice when adding orphan node. #22

Reverse order of middleware slice when adding orphan node. #22

Conversation

mar1n3r0 commented Jan 21, 2020

codecov bot commented Jan 21, 2020 • edited Loading

Codecov Report

vardius Jan 21, 2020 • edited Loading

Choose a reason for hiding this comment

mar1n3r0 Jan 22, 2020 • edited Loading

Choose a reason for hiding this comment

vardius Jan 22, 2020 • edited Loading

Choose a reason for hiding this comment

mar1n3r0 Jan 23, 2020

Choose a reason for hiding this comment

vardius Jan 23, 2020 • edited Loading

Choose a reason for hiding this comment

vardius Jan 23, 2020

Choose a reason for hiding this comment

mar1n3r0 Jan 23, 2020

Choose a reason for hiding this comment

mar1n3r0 commented Jan 23, 2020

codecov bot commented Jan 21, 2020 •

edited

Loading

vardius Jan 21, 2020 •

edited

Loading

mar1n3r0 Jan 22, 2020 •

edited

Loading

vardius Jan 22, 2020 •

edited

Loading

vardius Jan 23, 2020 •

edited

Loading