Error handling #49
Error handling #49
Conversation
pkg/store/filesystem/io.go
Outdated
| ) | ||
|
|
||
| // Unmarshal file into record | ||
| func (s Store) parseRecordFromFile(record proto.Message, filePath string) error { | ||
| file, err := os.Open(filePath) | ||
| if err != nil { | ||
| s.Logger.Err(err).Msgf("error reading file %v: file not found", filePath) | ||
| return err | ||
| return merrors.FromError(err) |
There was a problem hiding this comment.
@refs Thinking about it, I don't like that we're logging and building micro-errors here. Following the rule that errors should only be dealt with in one place, I'd vote for just returning the original error here without logging and then deal with logging and micro-errors in the places where the parsing and writing is used. Right now it's inconsistent and hard to follow, as we're e.g. in bundles.go sometimes returning an error an sometimes building a micro-error. Even if we have to write more code, I'd still like to remove building the errors in io.go and move it to bundles.go etc.
Let me know what you think.
There was a problem hiding this comment.
essentially I agree with this, I thought of this while building this. Otherwise errors will be logged twice; which is good for root cause analysis, but then we could include more context to the message.
There was a problem hiding this comment.
We should agree on a common practice. AFAICT it would make sense to wrap errors with the macro errors in the handlers.
There was a problem hiding this comment.
Let's go with wrapping an error's detail on handlers 👍 I'll update
pkg/store/filesystem/io.go
Outdated
| ) | ||
|
|
||
| // Unmarshal file into record | ||
| func (s Store) parseRecordFromFile(record proto.Message, filePath string) error { | ||
| file, err := os.Open(filePath) | ||
| if err != nil { | ||
| s.Logger.Err(err).Msgf("error reading file %v: file not found", filePath) | ||
| return err | ||
| return merrors.FromError(err) |
There was a problem hiding this comment.
We should agree on a common practice. AFAICT it would make sense to wrap errors with the macro errors in the handlers.
There was a problem hiding this comment.
I can't request changes, since I built the PR with cherry-picking 😅 there are two places where you forgot to change back to returning the plain error instead of a new micro error.
Moving the merrors to the handlers feels much better now. Thanks for the changes! 😍
pkg/service/v0/validator.go
Outdated
| @@ -5,6 +5,7 @@ import ( | |||
|
|
|||
| validation "github.com/go-ozzo/ozzo-validation/v4" | |||
| "github.com/go-ozzo/ozzo-validation/v4/is" | |||
| merrors "github.com/micro/go-micro/v2/errors" | |||
There was a problem hiding this comment.
import should not be required in here anymore
pkg/service/v0/validator.go
Outdated
| } | ||
|
|
||
| // validateResource is an internal helper for validating the content of a resource. | ||
| func validateResource(resource *proto.Resource) error { | ||
| if err := validation.Validate(&resource, validation.Required); err != nil { | ||
| return merrors.FromError(err) |
pkg/store/filesystem/bundles.go
Outdated
| @@ -23,7 +23,7 @@ func (s Store) ListBundles(bundleType proto.Bundle_Type) ([]*proto.Bundle, error | |||
| bundlesFolder := s.buildFolderPathForBundles(false) | |||
| bundleFiles, err := ioutil.ReadDir(bundlesFolder) | |||
| if err != nil { | |||
| return []*proto.Bundle{}, nil | |||
| return []*proto.Bundle{}, merrors.FromError(err) | |||
There was a problem hiding this comment.
replace with return []*proto.Bundle{}, err
There was a problem hiding this comment.
I'd restrain from returning an error here, as callers would check for it. Either return an empty bundle or an error, not both. I'd leave it as it was.
Adopt go-micro's style of handling errors. Cherry-picked from here