-
Notifications
You must be signed in to change notification settings - Fork 23
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
fix: Wallet Management Error Handling #1248
Conversation
Signed-off-by: Bassam Riman <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGMT but it would be good to have @patlo-iog's review as well IMO 💪
case class PermissionNotAvailable(userId: UUID, cause: String) | ||
extends PermissionManagementServiceError(StatusCode.BadRequest, cause) | ||
|
||
case class ServiceError(cause: String) extends PermissionManagementServiceError(StatusCode.InternalServerError, cause) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this one represents the catch-all "unexpected error" case, right? From the caller's perspective, there is no way to recover from this type of failure, so I would remove it and throw a defect in the service implementation instead.
) | ||
.asSome | ||
.catchSome { case e: RuntimeException => | ||
if (e.getMessage().contains("Could not find resource")) ZIO.none | ||
if (e.getMessage.contains("Could not find resource")) ZIO.none |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@patlo-iog Just a side comment, but this looks rather fragile... 🧐
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. It's a shame library we use don't expose information about resource not found 🥲, but I can recheck.
Late to the party, but LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
# [1.38.0](cloud-agent-v1.37.0...cloud-agent-v1.38.0) (2024-07-15) ### Bug Fixes * Move InMemory classes to the test moduels ([#1240](#1240)) ([823057a](823057a)) * move mocks into the test modules ([#1236](#1236)) ([df83026](df83026)) * use Put and Get for DID in doobie statement ([#1250](#1250)) ([fc1cf51](fc1cf51)) * Wallet Management Error Handling ([#1248](#1248)) ([cfd5101](cfd5101)) ### Features * upgrade docusaurus and semantic-release packages ([de53f1d](de53f1d)) Signed-off-by: Allain Magyar <[email protected]>
Signed-off-by: Bassam Riman <[email protected]> Signed-off-by: Shota Jolbordi <[email protected]>
# [1.38.0](cloud-agent-v1.37.0...cloud-agent-v1.38.0) (2024-07-15) ### Bug Fixes * Move InMemory classes to the test moduels ([#1240](#1240)) ([823057a](823057a)) * move mocks into the test modules ([#1236](#1236)) ([df83026](df83026)) * use Put and Get for DID in doobie statement ([#1250](#1250)) ([fc1cf51](fc1cf51)) * Wallet Management Error Handling ([#1248](#1248)) ([cfd5101](cfd5101)) ### Features * upgrade docusaurus and semantic-release packages ([de53f1d](de53f1d)) Signed-off-by: Allain Magyar <[email protected]> Signed-off-by: Shota Jolbordi <[email protected]>
https://input-output.atlassian.net/browse/ATL-6838