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

Error handler interface is too restricted #52

Closed
aaokunev opened this issue Jun 20, 2018 · 5 comments
Closed

Error handler interface is too restricted #52

aaokunev opened this issue Jun 20, 2018 · 5 comments
Milestone

Comments

@aaokunev
Copy link

Hello! Right now you have type errorHandler:
type errorHandler func(w http.ResponseWriter, r *http.Request, err string)
i think, it seems too restricted, because type of err is string, instead of error. In case of type error, interface will be more Go-ideomatic. And in the client error handler it will be possible to operate with types, not strings:

err, ok := error.(MySuperError)
if ok == true {
	//do something with this error
}

instead:

if err == "Hey man! I'am Your Super Error! How are you? Are you missing?" {
	//do something
}

What do you think?

@dbyington
Copy link

It kinda looks like string was used to satisfy OnError() when it calls http.Error(), which expects a string.
It may be better, probably more idomatic, to use an error type all the way through and let OnError() call err.error().
I'll take a crack at refactoring for that and see what I come up with.

@dbyington
Copy link

Oh man. When was the last time the tests were run?

> go test
# github.com/dbyington/go-jwt-middleware
./jwtmiddleware_test.go:107:16: cannot use c (type map[string]interface {}) as type jwt.Claims in assignment:
        map[string]interface {} does not implement jwt.Claims (missing Valid method)
./jwtmiddleware_test.go:202:25: invalid operation: user.Claims["foo"] (type jwt.Claims does not support indexing)
FAIL    github.com/dbyington/go-jwt-middleware [build failed]

jwt-go Claims only implements Valid() and has no map slice. At least against the current version. I have no idea what version this test was written against as there's no vendor info.

Going to take a bit just to get the tests functioning, before I can even dig into making changes. Buckle up, this is going to be a bumpy ride.

@mujtaba1747
Copy link

Yeah, I also agree, the Error Handler is way too restricted !

@grounded042
Copy link
Contributor

I apologize for the silence on this issue and the PR. We are ramping up work on v2 of this package and will keep this issue in mind for that release.

@grounded042 grounded042 added this to the v2 milestone Jan 29, 2021
@grounded042 grounded042 mentioned this issue Apr 23, 2021
21 tasks
@sergiught
Copy link
Contributor

We just released the v2.0.0-beta 🥳 !

You can start testing it by running go get github.com/auth0/go-jwt-middleware/v2@v2.0.0-beta.

In case of issues fetching the v2 you might want to try go clean --modcache first before doing go get.

I'm closing this issue as now this is part of v2, but feel free to reopen if needed.

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 a pull request may close this issue.

5 participants