lib/versioner: Improve error messages (fixes #7354) (#7357)

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()

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
 }
 

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
 		}
 	}
 

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)
 	}
 

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) {

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 {

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")
 }