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

Middleware does not work because of subrouter order #393

Closed
zhyq0826 opened this Issue Jul 16, 2018 · 7 comments

Comments

Projects
None yet
3 participants
@zhyq0826
Copy link

zhyq0826 commented Jul 16, 2018

What version of Go are you running? (Paste the output of go version)

go version go1.10.3 darwin/amd64

What version of gorilla/mux are you at? (Paste the output of git rev-parse HEAD inside $GOPATH/src/github.com/gorilla/mux)

ded0c29

Describe your problem (and what you have tried so far)

The problem is that middleware does not work because of the subrouter order。

I have some subrouter like this:

func InitRoutes(router *mux.Router) *mux.Router {
	appRouter := router.PathPrefix("/v1/gitlab").Subrouter()
	appRouter.Use(FormatResponseMiddleware)
	appRouter.HandleFunc("/projects", getProjects).Methods("GET")
	return router
}

These subrouter will be invoked like this:

	router := mux.NewRouter()
	router = domain.InitRoutes(router)
	router = computer.InitRoutes(router)
	router = service.InitRoutes(router)
	router = consul.InitRoutes(router)
	router = app.InitRoutes(router)
	router = gitlab.InitRoutes(router) 
	router = home.InitRoutes(router)  
	return router

The home subrouter is special like this:

func InitRoutes(router *mux.Router) *mux.Router {
	appRouter := router.PathPrefix("/").Subrouter()
	appRouter.HandleFunc("/index", home)
	appRouter.HandleFunc("/", home)
	return router
}

func home(w http.ResponseWriter, r *http.Request) {
	t := template.Must(template.ParseFiles("templates/index.html"))
	t.Execute(w, "hello world")
}

If you place gitlab subrouter invoking after home subrouter like this:

	router = home.InitRoutes(router)  
	router = gitlab.InitRoutes(router) 

then middleware in gitlab does not work, it seems that the subrouter order have impact on the middleware.

Paste a minimal, runnable, reproduction of your issue below (use backticks to format it)

This is demo https://github.com/zhyq0826/muxdemo

@elithrar elithrar added the bug label Jul 16, 2018

@elithrar

This comment has been minimized.

Copy link
Member

elithrar commented Jul 16, 2018

Thanks for the report. I don't have time to look at this in-depth until later this week (at best), so if you're interested in helping fix it, writing:

  1. A failing test case (cases)
  2. A PR to address that test case

... would be super helpful.

@zhyq0826

This comment has been minimized.

Copy link
Author

zhyq0826 commented Jul 17, 2018

I will have a try. It may take more time to fix it.

@zhyq0826

This comment has been minimized.

Copy link
Author

zhyq0826 commented Jul 18, 2018

@elithrar I have tried to trace this problem today and found that it seems that #378 has fix it. The reason for this problem is that when in subRouter has not found matched route, then match.MatchErr = ErrNotFound,then later in other subRouter,the check :

if match.MatchErr == nil {
    for i := len(r.middlewares) - 1; i >= 0; i-- {
        match.Handler = r.middlewares[i].Middleware(match.Handler)
    }
}

will fail.

@bestform

This comment has been minimized.

Copy link

bestform commented Sep 14, 2018

I am experiencing the same problem. I tried the proposed changes in #378 and they did indeed fix it for me.

@stale

This comment has been minimized.

Copy link

stale bot commented Dec 9, 2018

This issue has been automatically marked as stale because it hasn't seen a recent update. It'll be automatically closed in a few days.

@stale stale bot added the stale label Dec 9, 2018

@bestform

This comment has been minimized.

Copy link

bestform commented Dec 12, 2018

There is a merged PR in the context of this whole problem. I will try with the changes of #422 and see if this fixes my issue and close it if it does.

@stale stale bot removed the stale label Dec 12, 2018

@stale

This comment has been minimized.

Copy link

stale bot commented Feb 10, 2019

This issue has been automatically marked as stale because it hasn't seen a recent update. It'll be automatically closed in a few days.

@stale stale bot added the stale label Feb 10, 2019

@stale stale bot closed this Feb 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment