From 0f2a5403db10e2018ea995dc64fc6ecf697b6afc Mon Sep 17 00:00:00 2001 From: Nick Craig-Wood Date: Sun, 14 Oct 2018 14:41:26 +0100 Subject: [PATCH] acd,box,onedrive,opendrive,ploud: fix Features() retaining the original receiver Before this change the Features() method would return a different Fs to that the Features() method was called on if the remote was instantiated on a file. The practical effect of this is that optional features, eg `rclone about` wouldn't work properly when called on a file, and likely this has been causing low level problems for users of these backends for ages. Ideally there would be a test for this, but it turns out that this is really hard, so instead of that all the backends have been converted to not copy the Fs and a big warning comment inserted for future readers. Fixes #2182 --- backend/amazonclouddrive/amazonclouddrive.go | 17 +++++++++++------ backend/box/box.go | 19 ++++++++++++------- backend/onedrive/onedrive.go | 17 +++++++++++------ backend/opendrive/opendrive.go | 17 +++++++++++------ backend/pcloud/pcloud.go | 17 +++++++++++------ 5 files changed, 56 insertions(+), 31 deletions(-) diff --git a/backend/amazonclouddrive/amazonclouddrive.go b/backend/amazonclouddrive/amazonclouddrive.go index 152201604..9c5692c78 100644 --- a/backend/amazonclouddrive/amazonclouddrive.go +++ b/backend/amazonclouddrive/amazonclouddrive.go @@ -312,16 +312,16 @@ func NewFs(name, root string, m configmap.Mapper) (fs.Fs, error) { if err != nil { // Assume it is a file newRoot, remote := dircache.SplitPath(root) - newF := *f - newF.dirCache = dircache.New(newRoot, f.trueRootID, &newF) - newF.root = newRoot + tempF := *f + tempF.dirCache = dircache.New(newRoot, f.trueRootID, &tempF) + tempF.root = newRoot // Make new Fs which is the parent - err = newF.dirCache.FindRoot(false) + err = tempF.dirCache.FindRoot(false) if err != nil { // No root so return old f return f, nil } - _, err := newF.newObjectWithInfo(remote, nil) + _, err := tempF.newObjectWithInfo(remote, nil) if err != nil { if err == fs.ErrorObjectNotFound { // File doesn't exist so return old f @@ -329,8 +329,13 @@ func NewFs(name, root string, m configmap.Mapper) (fs.Fs, error) { } return nil, err } + // XXX: update the old f here instead of returning tempF, since + // `features` were already filled with functions having *f as a receiver. + // See https://github.com/ncw/rclone/issues/2182 + f.dirCache = tempF.dirCache + f.root = tempF.root // return an error with an fs which points to the parent - return &newF, fs.ErrorIsFile + return f, fs.ErrorIsFile } return f, nil } diff --git a/backend/box/box.go b/backend/box/box.go index f814e1eea..34688cc34 100644 --- a/backend/box/box.go +++ b/backend/box/box.go @@ -283,16 +283,16 @@ func NewFs(name, root string, m configmap.Mapper) (fs.Fs, error) { if err != nil { // Assume it is a file newRoot, remote := dircache.SplitPath(root) - newF := *f - newF.dirCache = dircache.New(newRoot, rootID, &newF) - newF.root = newRoot + tempF := *f + tempF.dirCache = dircache.New(newRoot, rootID, &tempF) + tempF.root = newRoot // Make new Fs which is the parent - err = newF.dirCache.FindRoot(false) + err = tempF.dirCache.FindRoot(false) if err != nil { // No root so return old f return f, nil } - _, err := newF.newObjectWithInfo(remote, nil) + _, err := tempF.newObjectWithInfo(remote, nil) if err != nil { if err == fs.ErrorObjectNotFound { // File doesn't exist so return old f @@ -300,9 +300,14 @@ func NewFs(name, root string, m configmap.Mapper) (fs.Fs, error) { } return nil, err } - f.features.Fill(&newF) + f.features.Fill(&tempF) + // XXX: update the old f here instead of returning tempF, since + // `features` were already filled with functions having *f as a receiver. + // See https://github.com/ncw/rclone/issues/2182 + f.dirCache = tempF.dirCache + f.root = tempF.root // return an error with an fs which points to the parent - return &newF, fs.ErrorIsFile + return f, fs.ErrorIsFile } return f, nil } diff --git a/backend/onedrive/onedrive.go b/backend/onedrive/onedrive.go index 8b97b183b..e5e3290c9 100644 --- a/backend/onedrive/onedrive.go +++ b/backend/onedrive/onedrive.go @@ -448,16 +448,16 @@ func NewFs(name, root string, m configmap.Mapper) (fs.Fs, error) { if err != nil { // Assume it is a file newRoot, remote := dircache.SplitPath(root) - newF := *f - newF.dirCache = dircache.New(newRoot, rootInfo.ID, &newF) - newF.root = newRoot + tempF := *f + tempF.dirCache = dircache.New(newRoot, rootInfo.ID, &tempF) + tempF.root = newRoot // Make new Fs which is the parent - err = newF.dirCache.FindRoot(false) + err = tempF.dirCache.FindRoot(false) if err != nil { // No root so return old f return f, nil } - _, err := newF.newObjectWithInfo(remote, nil) + _, err := tempF.newObjectWithInfo(remote, nil) if err != nil { if err == fs.ErrorObjectNotFound { // File doesn't exist so return old f @@ -465,8 +465,13 @@ func NewFs(name, root string, m configmap.Mapper) (fs.Fs, error) { } return nil, err } + // XXX: update the old f here instead of returning tempF, since + // `features` were already filled with functions having *f as a receiver. + // See https://github.com/ncw/rclone/issues/2182 + f.dirCache = tempF.dirCache + f.root = tempF.root // return an error with an fs which points to the parent - return &newF, fs.ErrorIsFile + return f, fs.ErrorIsFile } return f, nil } diff --git a/backend/opendrive/opendrive.go b/backend/opendrive/opendrive.go index 1d2e8203f..24673c882 100644 --- a/backend/opendrive/opendrive.go +++ b/backend/opendrive/opendrive.go @@ -177,17 +177,17 @@ func NewFs(name, root string, m configmap.Mapper) (fs.Fs, error) { if err != nil { // Assume it is a file newRoot, remote := dircache.SplitPath(root) - newF := *f - newF.dirCache = dircache.New(newRoot, "0", &newF) - newF.root = newRoot + tempF := *f + tempF.dirCache = dircache.New(newRoot, "0", &tempF) + tempF.root = newRoot // Make new Fs which is the parent - err = newF.dirCache.FindRoot(false) + err = tempF.dirCache.FindRoot(false) if err != nil { // No root so return old f return f, nil } - _, err := newF.newObjectWithInfo(remote, nil) + _, err := tempF.newObjectWithInfo(remote, nil) if err != nil { if err == fs.ErrorObjectNotFound { // File doesn't exist so return old f @@ -195,8 +195,13 @@ func NewFs(name, root string, m configmap.Mapper) (fs.Fs, error) { } return nil, err } + // XXX: update the old f here instead of returning tempF, since + // `features` were already filled with functions having *f as a receiver. + // See https://github.com/ncw/rclone/issues/2182 + f.dirCache = tempF.dirCache + f.root = tempF.root // return an error with an fs which points to the parent - return &newF, fs.ErrorIsFile + return f, fs.ErrorIsFile } return f, nil } diff --git a/backend/pcloud/pcloud.go b/backend/pcloud/pcloud.go index ec390e513..1a97105a4 100644 --- a/backend/pcloud/pcloud.go +++ b/backend/pcloud/pcloud.go @@ -276,16 +276,16 @@ func NewFs(name, root string, m configmap.Mapper) (fs.Fs, error) { if err != nil { // Assume it is a file newRoot, remote := dircache.SplitPath(root) - newF := *f - newF.dirCache = dircache.New(newRoot, rootID, &newF) - newF.root = newRoot + tempF := *f + tempF.dirCache = dircache.New(newRoot, rootID, &tempF) + tempF.root = newRoot // Make new Fs which is the parent - err = newF.dirCache.FindRoot(false) + err = tempF.dirCache.FindRoot(false) if err != nil { // No root so return old f return f, nil } - _, err := newF.newObjectWithInfo(remote, nil) + _, err := tempF.newObjectWithInfo(remote, nil) if err != nil { if err == fs.ErrorObjectNotFound { // File doesn't exist so return old f @@ -293,8 +293,13 @@ func NewFs(name, root string, m configmap.Mapper) (fs.Fs, error) { } return nil, err } + // XXX: update the old f here instead of returning tempF, since + // `features` were already filled with functions having *f as a receiver. + // See https://github.com/ncw/rclone/issues/2182 + f.dirCache = tempF.dirCache + f.root = tempF.root // return an error with an fs which points to the parent - return &newF, fs.ErrorIsFile + return f, fs.ErrorIsFile } return f, nil }