Draft: fix: middleware ordering

[dylanhitt]

closes #98

Here's the fix. I'll get the tests up soon. Currently need this to get through the work day.

Thanks

[EwenQuim]

That's weird. We can clearly see here that it makes future uses of Use after

// Your proposition
func Use(s *Server, middlewares ...func(http.Handler) http.Handler) {
	s.middlewares = append(middlewares, s.middlewares...)
}

fuego.Use(cache)
fuego.Use(compress)

equivalent to

s.middlewares = append(cache, s.middlewares...) // [cache]
s.middlewares = append(compress, s.middlewares...) // [compress, cache], not [cache, compress] as wanted

I might need some tests to understand, I'll see this tomorrow or the day after

[dylanhitt]

Right your ordering there makes sense, in terms of when you use Use. However, register loops from the beginning of the slice. So it's in reverse. I think the code I places here isn't fully correct. And it may be simpler to just reverse the registration loop.

func withMiddlewares(controller http.Handler, middlewares ...func(http.Handler) http.Handler) http.Handler {
	for _, middleware := range middlewares {
		controller = middleware(controller)
	}
	return controller
}

I think using numbers would make it clearer

group := fuego.Group(s, "/v1"
fuego.Use(group, 1)
fuego.Use(group, 2)
fuego.Use(group, 3)

My expectation here would 1 executes before 2 and 3. So when this proposed changed we end up with a slice that looks like

[3, 2, 1]

The loop then registers 3 -> 2 -> 1 -> controller which is the equivalent of a line code like this

1middleware(2middleware(3middleware(controller)))

So in this case the above proposition is wrong

group := fuego.Group(s, "/v1"
fuego.Use(group, 1)
fuego.Use(group, 2)
fuego.Use(group, 3)
fuego.Get(group,  /test,  4, 5)

[4, 5, 3, 2, 1]

[EwenQuim]

Yes exactly ! We'll reverse the loop order of withMiddlewares

[dylanhitt]

@EwenQuim Here's my proposal for how we should test it. I will of course add more testing around groups and the other possible combinations of adding middleware i.e Group and on routes directly.

Let me know what you think or if you have a better idea or if you like this and have a thought to add another subset of tests somewhere else be my guest.

Thanks. Fun bug.

[dylanhitt]

Turns out this was still needed when adding middlewares at route level.

[EwenQuim]

Thank you for this fix and great tests!

I think a test with Group might be a final touch to this PR

I'll be merging+release this as soon as it is done!

---

EDIT: done in 0275c91 @dylanhitt

[dylanhitt]

I always forget headers.Add exists..

[dylanhitt]

I always forget headers.Add exists..