feat(service): Refactor Error#489
Draft
lcian wants to merge 7 commits into
Draft
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This changes
objectstore_service::Errorfrom athiserrorenum to anErrorstruct with a mandatoryErrorKindand optionaldescriptionandsource.This new
Errorstruct has almost noFrom<T>implementations.The intention here is to force the user to always specify a
kindinstead of blindly propagating everything with?, which is one of the issues that our previous approach had.objectstore_server::ApiErrorResponsecan now convert service errors to the appropriate HTTP status code by looking at the kind, rather than mapping a variant (i.e. the type of the inner error in the previous approach) to a certain status code.Implementation
For every kind, we have the associated functions
Error::$kindwhich takes the message and the source, and the simplerError::$kind_msgfor when we don't have a source to attach.An additional
Error::from_reqwestutil is provided that encapsulates some logic which we would have in multiple places where we want to transparently map GCS statuses to our statuses.This applies in particular to multipart requests, where Objectstore acts more as a proxy and where statuses are more varied, but also to e.g. 502s, which we can now easily surface transparently as 502s instead of 500s, to let the client know it can retry the operation.
As a downside, this makes existing code that had a lot of
?s (e.g. thelocal_fsbackend) more verbose, needing a.map_err(|e| Error::$kind(msg, e))every time.A middle-ground would be to implement
From<T> for ServiceErrorfor some types where we know where we 99% of the time wantErrorKind::Internal, e.g.serde_json::Error,io::Error, and the like.Or even have a generic impl of
Fromthat defaults toInternal, but that's something I would like to avoid as it would make it way too easy to produce 500s, just like with the previous impl.Close FS-358