Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Chi-server wrapping middleware logic fix. #688

Conversation

juddbaguio
Copy link

Current implementation server middlewares to a handler goes like this:

var handler = func(w http.ResponseWriter, r *http.Request) {
	siw.Handler.YourRouteHandler(w, r)
}

for _, middleware := range siw.HandlerMiddlewares {
	handler = middleware(handler)
}

The snippet above tends to execute beginning middlewares at the later than the last ones.

The looping is supposed to be like this:

var handler = func(w http.ResponseWriter, r *http.Request) {
	siw.Handler.YourRouteHandler(w, r)
}

for i := len(siw.HandlerMiddlewares) - 1; i >= 0; i--  {
	h := siw.HandlerMiddlewares[i]
        if h != nil {
          handler = h(handler)
        }
}

The solution is referenced from Ardan Lab's Service repository on wrapping middlewares

@juddbaguio juddbaguio changed the title Wrapping middleware logic fix. Chi-server wrapping middleware logic fix. Jul 20, 2022
@deepmap-marcinr
Copy link
Contributor

Hmm, interesting, so we process them in reverse order. This will reverse processing order for anyone who regenerates their code.

@juddbaguio
Copy link
Author

Exactly, we need to execute the right order of middlewares, this might also cause confusion to the devs who came from the Node.js background.

@uhey22e
Copy link
Contributor

uhey22e commented Sep 3, 2022

+1

FYI,
Chi reverse the order of middlewares as like as this PR when you use (*chi.Mux).Use().
https://github.com/go-chi/chi/blob/0316d5a1df8598eceb137f5f77945be56810b564/chain.go#L42

@jamietanna
Copy link
Collaborator

Thanks for this, looks like we got it finished in #787!

@jamietanna jamietanna closed this Nov 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants