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

Proper error handling (eliminate unwrap calls) #24

Closed
awakecoding opened this issue Dec 6, 2019 · 4 comments
Closed

Proper error handling (eliminate unwrap calls) #24

awakecoding opened this issue Dec 6, 2019 · 4 comments
Assignees
Labels
enhancement New feature or request

Comments

@awakecoding
Copy link

We (Devolutions) are looking at switching to the new casbin-rs project, leaving our initial port of casbin to Rust from a year ago behind in favour of this one. Overall, the new project looks good, but our primary concern is the lack of proper error handling.

A quick search in the code base reveals there are 92 unwrap() calls. Some of them are benign in nature, like unwrap calls in unit tests, but there are a bunch of them that could cause issues in production. We intend to run this code inside a server process, and unwrap() would cause the entire thread to panic, which is obviously not desirable.

Our old code had a simple error type with an enumeration of a few errors (evaluation error, parsing error, invalid value, etc.). The new code has a simple string error type, but it should be improved with a real enumeration of errors to facilitate error processing.

The idea would be to replace all unwrap() calls with errors, which would affect the function signatures (it would return a Result, which is either a success value or an error value). In addition to this, the error type would be improved with an enumeration of error types. Once functions are modified to return Results with the same error type, it should become easy to use the ? operator to reduce the amount of boilerplate error handling.

We would like to know what would work best to make these kind of improvements. Should we do it ourselves according to the way we have described it and submit a PR, or would you prefer doing it? The side effect of adding proper error handling is that changes will affect a lot of files.

Please let us know, and we'll begin putting our efforts on the new casbin-rs. Thanks!

@hsluoyz
Copy link
Member

hsluoyz commented Dec 6, 2019

@xcaptain @GopherJ

@xcaptain
Copy link
Contributor

xcaptain commented Dec 8, 2019

@awakecoding Sorry for the late reply. You are right, the current error handling is still very simple, only a CasbinError, would be great if we have a finer error handling (I was too lazy when doing #19). I believe this project hasn't been widely used (maybe only I and GopherJ), It would be awesome if you can help, PRs are welcomed, Thanks.

@hsluoyz
Copy link
Member

hsluoyz commented Jan 10, 2020

@GopherJ is this issue resolved?

@GopherJ
Copy link
Member

GopherJ commented Jan 22, 2020

@hsluoyz Yes, it has been resolved and sorry for the late reply.

@hsluoyz hsluoyz closed this as completed Jan 22, 2020
@hsluoyz hsluoyz assigned GopherJ and unassigned awakecoding Jan 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants