Decoupling from Gorilla Mux

[rickb777]

In recent months, gothic.go has become coupled to gorilla/mux. See lines 176 onward

func getProviderName(req *http.Request) (string, error) {
	provider := req.URL.Query().Get("provider")
	if provider == "" {
		if p, ok := mux.Vars(req)["provider"]; ok {
			return p, nil
		}
	}
	if provider == "" {
		provider = req.URL.Query().Get(":provider")
	}
	if provider == "" {
		return provider, errors.New("you must select a provider")
	}
	return provider, nil
}

Firstly, note that req.URL.Query().Get(":provider") is possibly called twice (this may be a mistake).

Secondly, mux.Vars(req)["provider"] is specific to gorilla/mux. I happen to use julienschmidt/httprouter and I'd quite like to decouple from gorilla/mux dependency, which I don't need in my codebase.

So, perhaps the package gothic is really two things: (a) it provides the main API for goth and (b) it provides gorilla/mux lookup of the provider variable. Combining these is unfortunate because it reduces the flexibility of the package. [There is a third concern: storage of state in the session; however I'm content to leave this unchanged.]

There are several possible options to reduce this coupling. The simplest I can think of is to do away with the GetProviderName method (and getProviderName) and change the signature of CompleteUserAuth to take an additional providerName string parameter.

It would then be quite easy to have another function that wraps CompleteUserAuth as a standard http.HandlerFunc, along with the gorilla/mux providerName expression - this would need to be in a different package so that gorilla/mux drops out of the imports list (note that gorilla/mux is in the list of imports and this is the key difficulty).

It would also be easy to implement different integrations with other routing libraries, which Is what I would need.

My workaround for this is to make a copy of gothic.go in my own codebase and with the necessary alteration I've described. But I'd prefer not to have to duplicate the code.

[acornejo]

+1

[willemvd]

@rickb777 there is a minor difference in the first code what on first looks like the same:
the second req.URL.Query().Get(":provider") is with a semicolon before the provider
So think it has to do with resolving the provider value in side the different http engines

@markbates binding against mux is indeed a problem , but I know when we would change this it would break applications using this default behavior . Any thoughts on how to decouple this?

I also changed the GetProviderName function to a custom function to do my own implementation when using Macaron , but this does indeed not remove the gorilla/mux dependency on the project.

[markbates]

This coupling has been there for a few years, and to remove it would break existing apps. If someone can figure out a way to do this would breaking backward compatibility I'm all ears.

[IngCr3at1on]

@markbates what about a v2 sub-package in-line with Go module design?

Some common functionality could be moved into an internal package and exported by both while the v2 package could be built without being coupled to gorilla/mux and the old package could remain as is.

[xeoncross]

As for the backwards compatible approach Both gorilla/mux and httperouter can use context.Content to find route parameters. I've done a simple context.Values check in my "mid" middleware.

[lrstanley]

Has there been any follow-up on this? Would be nice to remove the dependency on gorilla packages (both Store and url param functionality), and make it a bit more pluggable. Could also have sub-packages to still allow the use of gorilla packages in a seamless way. I don't think there is an easy way to be backwards compatible, but might be worth a v2 version, with an interface/context approach.

Would @markbates or other maintainers be open to supporting something like this? I could potentially help with PR's and whatnot.

EDIT: /cc @techknowlogick

[pinpox]

See #552