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

refactor: return custom error from utils when bcrypt fails #1426

Merged
merged 2 commits into from
Mar 23, 2021

Conversation

mantariksh
Copy link
Contributor

Problem

  1. In src/app/utils/hash, we have utility functions which wrap calls to bcrypt.hash and bcrypt.compare in ResultAsync. However, we return a generic ApplicationError when these functions fail. This means that downstream functionality has to deal with the error as a generic ApplicationError without context of where it came from, e.g. when deciding what status code and message to return in mapRouteError. Developers have to search through all the service functions called in controllers one by one until they find where the ApplicationError was returned.
  2. We have a generateOtp utility, but all the times when we need to call it, we have to create not just the OTP but a hash of it as well. Since these two bits of functionality always come together, it makes sense to have a utility function which does both.

Part of #941

Solution

  1. Return a new HashingError when bcrypt fails. This makes the origin of the error much clearer.
  2. Create a generateOtpWithHash utility function, so we can call generateOtpWithHash().andThen... instead of const otp = generateOtp(); hashData(otp).andThen.... Note that verification.service was left out (it still calls generateOtp instead of generateOtpWithHash) as it will be addressed together with the rest of Convert verification service to use neverthrow #941.

@liangyuanruo liangyuanruo merged commit 3bf1010 into develop Mar 23, 2021
@liangyuanruo liangyuanruo deleted the ref/hash-errors branch March 23, 2021 01:52
@karrui karrui mentioned this pull request Mar 23, 2021
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 this pull request may close these issues.

None yet

2 participants