Closed lorepozo closed 7 years ago
I'm not sure what problem this commit tries to solve? It would be a breaking change (as in "would break a lot of code which uses this package"), so I'm reluctant to accept that without clear understanding of the practical benefits. So: can you describe the issue?
Thanks for your quick response.
The problem is for middleware of the form typeA
:
func(http.Handler) http.Handler // typeA
Functions like (*DigestAuth).JustCheck()
are incompatible, because they have the following form, typeB
:
func(http.HandlerFunc) http.HandlerFunc // typeB
where http.HandlerFunc
is unnecessarily specific.
You're right that this is a breaking change for people using the returned value from typeB
as a http.HandlerFunc
rather than a http.Handler
. It should also be noted that using typeA
only broadens the kind of arguments that can be taken.
The breaking behavior is realized when the value of (*DigestAccess).JustCheck(wrapped)
is used directly for a function call, rather than using the (*http.HandlerFunc).ServeHTTP
method on it (or just using it as an http.Handler
).
To be more specific on the issue, here's an example use-case that works with these changes:
package main
import (
"net/http"
"github.com/abbot/go-http-auth"
"goji.io"
"goji.io/pat"
)
func JohnSecret(user, _ string) string {
if user == "john" {
// password is hello
return "b98e16cbc3d01734b264adba7baa3bf9"
}
return ""
}
func hello(w http.ResponseWriter, r *http.Request) {
w.Write([]byte("hello from the inside"))
}
func main() {
mux := goji.NewMux()
johnAuth := auth.NewDigestAuthenticator("example.com", JohnSecret)
mux.Use(johnAuth.JustCheck) // use as middleware with typeA
mux.Handle(pat.Get("/*"), http.HandlerFunc(hello))
http.Handle("/john", mux)
http.ListenAndServe(":8080", nil)
}
currently, to use go-http-auth's digest access as such a middleware, it must be wrapped like so:
// ideally, with typeA: mux.Use(digestAuth.JustCheck)
// instead, with typeB:
mux.Use(func(next http.Handler) http.Handler) {
return digestAuth.JustCheck(http.HandlerFunc(next.ServeHTTP))
})
this sacrifice is due only to a lack of free abstraction you could get with the http.Handler
interface. The concrete use of http.HandlerFunc
causes this unnecessary boilerplate.
A non-breaking change would make the functions in question look like typeC
:
func(http.Handler) http.HandlerFunc // typeC
but unfortunately the Go compiler doesn't think typeC
fits as a typeA
and many benefits (like my demonstrated use as middleware) are lost; so using typeC
would serve little purpose.
Thanks for the explanation. I understand that from the design perspective it would be better to use http.Handler
in this package, however I think it might be a bit late for this change to happen. I think there are two reasonable alternatives:
JustCheckHandler
, with a signature func(http.Handler) http.Handler
func convert(f func(w http.HandlerFunc) http.HandlerFunc) func(http.Handler) http.Handler {
return func(h http.Handler) http.Handler {
return f(h.ServeHTTP)
}
}
With that your example code becomes:
// ...
mux.Use(convert(johnAuth.JustCheck))
// ...
I'm leaning towards the latter solution, as it doesn't increase the interface surface of the package (which adds to the maintenance cost, and which I would happily cut by half if I were to re-do this from scratch). Would this work for your use case?
I suppose, to prevent the breaking change or package bloat, we'll have to suffice with converting to the more compatible interface outside of this package. If only semantic versioning were more of a thing with Go packages..
This helps the package fit better with a lot of middleware (e.g. goji).