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.

Sign up for GitHub

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

report better error messages after Diesel queries #907

Merged
merged 3 commits into from
Apr 13, 2022
+39 −27
Merged

report better error messages after Diesel queries #907

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9302ffe
report better error messages after Diesel queries
davepacheco Apr 13, 2022
12828af
fix style
davepacheco Apr 13, 2022
bc738ba
clean up implementation
davepacheco Apr 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 16 additions & 6 deletions nexus/src/db/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,10 +183,20 @@ where
DieselError::DatabaseError(kind, info) => {
PublicError::internal_error(&format_database_error(kind, &*info))
}
error => PublicError::internal_error(&format!(
"Unknown diesel error: {:?}",
error
)),
error => {
let context = match make_not_found_error() {
PublicError::ObjectNotFound { type_name, lookup_type } => {
format!("looking up {:?} {:?}", type_name, lookup_type)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a nit, but is it true that this error is only hit when "looking up" a resource? I think your suggestion of a trait that provides an additional context method is really attractive, since it would allow us to provide this string or some other information from the caller.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it depends on what you mean by "lookup". I believe it's always used in a context where we expect a specific object to exist and it doesn't. Sometimes that's because we're trying to update it or delete it, not fetch it. I could make the language more general ("accessing object") to avoid confusion.

}
_ => {
format!("during unknown operation")
}
};
PublicError::internal_error(&format!(
"Unknown diesel error {}: {:#}",
context, error
))
}
}
}

Expand All @@ -210,8 +220,8 @@ fn public_error_from_diesel_create(
)),
},
_ => PublicError::internal_error(&format!(
"Unknown diesel error: {:?}",
error
"Unknown diesel error creating {:?} called {:?}: {:#}",
resource_type, object_name, error
Comment on lines +218 to +219
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did these lines end up being useful? I think the change is probably good regardless, so I'm just curious if you ran into another opaque error message that this helped resolve.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I didn't wind up using these.

)),
}
}