From f5590c33459a24e4eb8448ae2d6aa648126e8543 Mon Sep 17 00:00:00 2001 From: Simon Frei Date: Fri, 12 Feb 2021 20:30:51 +0100 Subject: [PATCH] lib/versioner: Improve error messages (fixes #7354) (#7357) --- lib/api/api.go | 10 +++++++++- lib/api/mocked_model_test.go | 2 +- lib/model/model.go | 8 ++++---- lib/model/model_test.go | 3 ++- lib/versioner/external.go | 10 +++++++--- lib/versioner/util.go | 4 ++-- lib/versioner/versioner.go | 34 +++++++++++++++++++++++++++++++++- 7 files changed, 58 insertions(+), 13 deletions(-) diff --git a/lib/api/api.go b/lib/api/api.go index 7565d0e14..4313d7702 100644 --- a/lib/api/api.go +++ b/lib/api/api.go @@ -1545,7 +1545,7 @@ func (s *service) postFolderVersionsRestore(w http.ResponseWriter, r *http.Reque http.Error(w, err.Error(), 500) return } - sendJSON(w, ferr) + sendJSON(w, errorStringMap(ferr)) } func (s *service) getFolderErrors(w http.ResponseWriter, r *http.Request) { @@ -1839,6 +1839,14 @@ func shouldRegenerateCertificate(cert tls.Certificate) error { return nil } +func errorStringMap(errs map[string]error) map[string]*string { + out := make(map[string]*string, len(errs)) + for s, e := range errs { + out[s] = errorString(e) + } + return out +} + func errorString(err error) *string { if err != nil { msg := err.Error() diff --git a/lib/api/mocked_model_test.go b/lib/api/mocked_model_test.go index f38f16f76..6048aaadf 100644 --- a/lib/api/mocked_model_test.go +++ b/lib/api/mocked_model_test.go @@ -96,7 +96,7 @@ func (m *mockedModel) GetFolderVersions(folder string) (map[string][]versioner.F return nil, nil } -func (m *mockedModel) RestoreFolderVersions(folder string, versions map[string]time.Time) (map[string]string, error) { +func (m *mockedModel) RestoreFolderVersions(folder string, versions map[string]time.Time) (map[string]error, error) { return nil, nil } diff --git a/lib/model/model.go b/lib/model/model.go index 2d61ff313..694f7588a 100644 --- a/lib/model/model.go +++ b/lib/model/model.go @@ -88,7 +88,7 @@ type Model interface { SetIgnores(folder string, content []string) error GetFolderVersions(folder string) (map[string][]versioner.FileVersion, error) - RestoreFolderVersions(folder string, versions map[string]time.Time) (map[string]string, error) + RestoreFolderVersions(folder string, versions map[string]time.Time) (map[string]error, error) DBSnapshot(folder string) (*db.Snapshot, error) NeedFolderFiles(folder string, page, perpage int) ([]db.FileInfoTruncated, []db.FileInfoTruncated, []db.FileInfoTruncated, error) @@ -2624,7 +2624,7 @@ func (m *model) GetFolderVersions(folder string) (map[string][]versioner.FileVer return ver.GetVersions() } -func (m *model) RestoreFolderVersions(folder string, versions map[string]time.Time) (map[string]string, error) { +func (m *model) RestoreFolderVersions(folder string, versions map[string]time.Time) (map[string]error, error) { m.fmut.RLock() err := m.checkFolderRunningLocked(folder) fcfg := m.folderCfgs[folder] @@ -2637,11 +2637,11 @@ func (m *model) RestoreFolderVersions(folder string, versions map[string]time.Ti return nil, errNoVersioner } - restoreErrors := make(map[string]string) + restoreErrors := make(map[string]error) for file, version := range versions { if err := ver.Restore(file, version); err != nil { - restoreErrors[file] = err.Error() + restoreErrors[file] = err } } diff --git a/lib/model/model_test.go b/lib/model/model_test.go index 2e6cde6c0..b7f3c830e 100644 --- a/lib/model/model_test.go +++ b/lib/model/model_test.go @@ -26,6 +26,7 @@ import ( "testing" "time" + "github.com/pkg/errors" "github.com/syncthing/syncthing/lib/config" "github.com/syncthing/syncthing/lib/db" "github.com/syncthing/syncthing/lib/db/backend" @@ -2859,7 +2860,7 @@ func TestVersionRestore(t *testing.T) { ferr, err := m.RestoreFolderVersions("default", restore) must(t, err) - if err, ok := ferr["something"]; len(ferr) > 1 || !ok || err != "cannot restore on top of a directory" { + if err, ok := ferr["something"]; len(ferr) > 1 || !ok || !errors.Is(err, versioner.ErrDirectory) { t.Fatalf("incorrect error or count: %d %s", len(ferr), ferr) } diff --git a/lib/versioner/external.go b/lib/versioner/external.go index f786f7e40..58a7ecbf9 100644 --- a/lib/versioner/external.go +++ b/lib/versioner/external.go @@ -9,6 +9,7 @@ package versioner import ( "context" "errors" + "fmt" "os" "os/exec" "runtime" @@ -64,12 +65,12 @@ func (v external) Archive(filePath string) error { l.Debugln("archiving", filePath) if v.command == "" { - return errors.New("Versioner: command is empty, please enter a valid command") + return errors.New("command is empty, please enter a valid command") } words, err := shellquote.Split(v.command) if err != nil { - return errors.New("Versioner: command is invalid: " + err.Error()) + return fmt.Errorf("command is invalid: %w", err) } context := map[string]string{ @@ -99,6 +100,9 @@ func (v external) Archive(filePath string) error { combinedOutput, err := cmd.CombinedOutput() l.Debugln("external command output:", string(combinedOutput)) if err != nil { + if eerr, ok := err.(*exec.ExitError); ok && len(eerr.Stderr) > 0 { + return fmt.Errorf("%v: %v", err, string(eerr.Stderr)) + } return err } @@ -106,7 +110,7 @@ func (v external) Archive(filePath string) error { if _, err = v.filesystem.Lstat(filePath); fs.IsNotExist(err) { return nil } - return errors.New("Versioner: file was not removed by external script") + return errors.New("file was not removed by external script") } func (v external) GetVersions() (map[string][]FileVersion, error) { diff --git a/lib/versioner/util.go b/lib/versioner/util.go index 3dc7cbec5..a3e9e601d 100644 --- a/lib/versioner/util.go +++ b/lib/versioner/util.go @@ -23,7 +23,7 @@ import ( ) var ( - errDirectory = errors.New("cannot restore on top of a directory") + ErrDirectory = errors.New("cannot restore on top of a directory") errNotFound = errors.New("version not found") errFileAlreadyExists = errors.New("file already exists") ) @@ -196,7 +196,7 @@ func restoreFile(method fs.CopyRangeMethod, src, dst fs.Filesystem, filePath str if info, err := dst.Lstat(filePath); err == nil { switch { case info.IsDir(): - return errDirectory + return ErrDirectory case info.IsSymlink(): // Remove existing symlinks (as we don't want to archive them) if err := dst.Remove(filePath); err != nil { diff --git a/lib/versioner/versioner.go b/lib/versioner/versioner.go index 372488294..5c96c3803 100644 --- a/lib/versioner/versioner.go +++ b/lib/versioner/versioner.go @@ -47,5 +47,37 @@ func New(cfg config.FolderConfiguration) (Versioner, error) { return nil, fmt.Errorf("requested versioning type %q does not exist", cfg.Type) } - return fac(cfg), nil + return &versionerWithErrorContext{ + Versioner: fac(cfg), + vtype: cfg.Versioning.Type, + }, nil +} + +type versionerWithErrorContext struct { + Versioner + vtype string +} + +func (v *versionerWithErrorContext) wrapError(err error, op string) error { + if err != nil { + return fmt.Errorf("%s versioner: %v: %w", v.vtype, op, err) + } + return nil +} + +func (v *versionerWithErrorContext) Archive(filePath string) error { + return v.wrapError(v.Versioner.Archive(filePath), "archive") +} + +func (v *versionerWithErrorContext) GetVersions() (map[string][]FileVersion, error) { + versions, err := v.Versioner.GetVersions() + return versions, v.wrapError(err, "get versions") +} + +func (v *versionerWithErrorContext) Restore(filePath string, versionTime time.Time) error { + return v.wrapError(v.Versioner.Restore(filePath, versionTime), "restore") +} + +func (v *versionerWithErrorContext) Clean(ctx context.Context) error { + return v.wrapError(v.Versioner.Clean(ctx), "clean") }