Error refinement [NotFound and Conflict] #1599
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1599 +/- ##
==========================================
- Coverage 62.02% 60.60% -1.42%
==========================================
Files 167 167
Lines 12347 12319 -28
==========================================
- Hits 7658 7466 -192
- Misses 3696 3882 +186
+ Partials 993 971 -22
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Which makes all the callers more verbose. |
internal/api/v1/service/validate.go
Outdated
| @@ -61,7 +60,7 @@ func ValidateService( | |||
| srv, err := client.GetRelease(releaseName) | |||
| if err != nil { | |||
| if errors.Is(err, helmdriver.ErrReleaseNotFound) { | |||
| return apierror.NewNotFoundError(fmt.Sprintf("%s - %s", err.Error(), releaseName)) | |||
| return apierror.NewNotFoundError(errors.Wrapf(err, "release %s not found", releaseName)) | |||
There was a problem hiding this comment.
I can see this being more sensible than what we had, i.e. providing the entire base error instead of just its message.
OTOH the
- return apierror.NewNotFoundError("service not found")
+ return apierror.NewNotFoundError(errors.New("service not found"))
in line 47 above just becomes more verbose than necessary.
How about having two functions, with slightly differing signatures, i.e. the old function, keeping its signature
NewXError (msg string, ...)
and a new
NewXFromError (err error, ...)
with the new signature for where we wish to pass in an underlying error ?
And coming back to this place, as written even that is actually to verbose, as it does not use the ...string argument of the old and new function, while it could have:
| return apierror.NewNotFoundError(errors.Wrapf(err, "release %s not found", releaseName)) | |
| return apierror.NewNotFoundError(err, "release", releaseName, "not found") |
Although that runs afoul of the ", " separator used in the underlying strings.Join. A " " separator would be nicer IMHO.
Looking at other places using fmt.Errorf, fmt.Sprintf, and the like a simple reorg of the error message could get rid of many of these I believe. I.e. instead of fmt.Errorf("Foo '%s' does not exist", X) use "Foo does not exist:", X, i.e. the dynamic part is just another detail argument of the NewX... function.
There was a problem hiding this comment.
Ah, I've commented before have read this comment. 😄
I'm fine with having the two signatures! 👍
There was a problem hiding this comment.
Note that NewX functions seem to use only the err.Error() of the passed error. The errors.Wrapf in the callers effectively devolve into an extended error message. and that is something we already can have without the additional Wrap, through the details argument.
That's true, but an error usually has more information than a simple string (like the stacktrace), and it's not the common case to return an error without an error (usually an error occurred, and you need to create an HTTP error from it). https://github.com/pkg/errors/blob/master/errors.go#L16-L18
So I think that this is a valid fix: // BEFORE
if err != nil {
if k8serrors.IsNotFound(err) {
return apierror.NewNotFoundError("service not found")
= }
}
// AFTER
if err != nil {
if k8serrors.IsNotFound(err) {
return apierror.NewNotFoundError(errors.Wrap(err, "service not found"))
}
}while sometimes it's not very useful when you need to return an error without errors (but as I said it's not the most common case) // BEFORE
if theService == nil {
return apierror.NewNotFoundError("service not found")
}
// AFTER
if theService == nil {
return apierror.NewNotFoundError(errors.New("service not found"))
}So if you think that adding the Also about the stacktrace I've seen that the But I think it's not really an issue, it's just something that I've noticed and that we could add in the backlog to remove a dependency. |
enrichman
force-pushed
the
error-refinement
branch
from
5a28aa9
to
c2f2256
Compare
|
@andreas-kupries As you pointed out we are not really using the error, we are losing the information with Also in many places we are not sure on what to put into the details, and we are passing a blank string. I was thinking a bit about it, and I wanted to simplify everything a bit more. I'm going to push a new implementation, with only the string message, and without the need to specify the details. We can add the details later with a This seems to simplify everything, and it looks a bit less verbose. Please note that I'm changing only the |
|
👍 Will wait until the new changes are and then revisit. I am guessing that the |
There was a problem hiding this comment.
Good enough to me now.
The nits I saw can be done in a separate PR.
Example:
NewNotFoundError(fmt.Sprintf("Application Chart '%s' does not exist", app))
could be written as
NewNotFoundError("Application Chart", app)
with a slight change to the signature of the new helper. And the fmt.Sprintf moves inside.
Similar for NewConflictError.
Maybe as part of #1600 ?
|
Thanks, yes, I'll add those changes in the other PR, so I can merge this. I was thinking to add a |
I was having a look at the
errorspackage of the APIs and it's probably a bit "over complicated", or maybe just verbose.I wanted to simplify it a bit, and I've started just doing a small couple of things for the NotFound and Conflict errors.
I've changed the
NewNotFoundErrorsignature to accept anerrorinstead of just a message. In my opinion it makes sense to create an API error from another error, that has more context about what happened and where. So I just needed to modify the callers adding a Wrap or using a simple errors.New.I've also added a
NewConflictError, and used it (along with theNewNotFoundError) to construct the other "domain" errors.