speps / go-hashids

Go (golang) implementation of http://www.hashids.org
MIT License
1.32k stars 109 forks source link

Decode shouldn't panic #17

Closed kjk closed 8 years ago

kjk commented 8 years ago

Hashed ids are often used as part of urls. They are outside of program's control (e.g. a user on the website might manually modify url).

If such modified value is passed to Decode(), it'll panic, which is against Go conventions, see http://blog.golang.org/defer-panic-and-recover:

The convention in the Go libraries is that even when a package uses panic internally, its external API still presents explicit error return values.

It would be better if it was Decode(hash string) ([]int, error).

This would require breaking the API, which might not be a good idea, so an alternative would be to provide additional DecodeSafe (not a great name) or IsValid(hash string) bool so that the string can be checked for correctness before calling Decode().

kjk commented 8 years ago

panic()-ing is not just annoyance but might be a serious issue. If there's a sync.Mutex.Lock() that is unlocked without using defer anywhere in the request handling go-routine, panic will cause permanent deadlock of the whole web app. This happened in my app.

speps commented 8 years ago

Very good point, will take a look when I get home.

speps commented 8 years ago

Fixed by 9aaed95555a16b68e19ea27f9a70ad72ee2531c8. There are now versions of Decrypt and DecryptInt64 that return an error instead of panicking.