From 00fe37e7ec58c641bf69d1d0f7a9829d388f10b3 Mon Sep 17 00:00:00 2001 From: David Christofas Date: Mon, 28 Jun 2021 16:38:45 +0200 Subject: [PATCH] fix delete in the owncloud storage driver (#1833) --- .../fix-delete-owncloud-storage-driver.md | 6 ++++++ pkg/storage/fs/owncloud/owncloud.go | 20 +++++++++++-------- .../expected-failures-on-OWNCLOUD-storage.md | 6 ------ 3 files changed, 18 insertions(+), 14 deletions(-) create mode 100644 changelog/unreleased/fix-delete-owncloud-storage-driver.md diff --git a/changelog/unreleased/fix-delete-owncloud-storage-driver.md b/changelog/unreleased/fix-delete-owncloud-storage-driver.md new file mode 100644 index 0000000000..b36497b012 --- /dev/null +++ b/changelog/unreleased/fix-delete-owncloud-storage-driver.md @@ -0,0 +1,6 @@ +Bugfix: Properly handle name collisions for deletes in the owncloud driver + +In the owncloud storage driver when we delete a file we append the deletion time to the file name. +If two fast consecutive deletes happened, the deletion time would be the same and if the two files had the same name we ended up with only one file in the trashbin. + +https://github.com/cs3org/reva/pull/1833 diff --git a/pkg/storage/fs/owncloud/owncloud.go b/pkg/storage/fs/owncloud/owncloud.go index bc4fcf0058..1e59ecb1e0 100644 --- a/pkg/storage/fs/owncloud/owncloud.go +++ b/pkg/storage/fs/owncloud/owncloud.go @@ -1526,15 +1526,19 @@ func (fs *ocfs) trash(ctx context.Context, ip string, rp string, origin string) // move to trash location dtime := time.Now().Unix() tgt := filepath.Join(rp, fmt.Sprintf("%s.d%d", filepath.Base(ip), dtime)) + + // The condition reads: "if the file exists" + // I know this check is hard to read because of the double negation + // but this way we avoid to duplicate the code following the if block. + // If two deletes happen fast consecutively they will have the same `dtime`, + // therefore we have to increase the 'dtime' to avoid collisions. + if _, err := os.Stat(tgt); !errors.Is(err, os.ErrNotExist) { + // timestamp collision, try again with higher value: + dtime++ + tgt = filepath.Join(rp, fmt.Sprintf("%s.d%d", filepath.Base(ip), dtime)) + } if err := os.Rename(ip, tgt); err != nil { - if os.IsExist(err) { - // timestamp collision, try again with higher value: - dtime++ - tgt := filepath.Join(rp, fmt.Sprintf("%s.d%d", filepath.Base(ip), dtime)) - if err := os.Rename(ip, tgt); err != nil { - return errors.Wrap(err, "ocfs: could not move item to trash") - } - } + return errors.Wrap(err, "ocfs: could not move item to trash") } return fs.propagate(ctx, filepath.Dir(ip)) diff --git a/tests/acceptance/expected-failures-on-OWNCLOUD-storage.md b/tests/acceptance/expected-failures-on-OWNCLOUD-storage.md index 375eb07231..73ab290a4d 100644 --- a/tests/acceptance/expected-failures-on-OWNCLOUD-storage.md +++ b/tests/acceptance/expected-failures-on-OWNCLOUD-storage.md @@ -27,12 +27,6 @@ The following scenarios fail on OWNCLOUD storage but not on OCIS storage: - [apiTrashbin/trashbinFilesFolders.feature:278](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinFilesFolders.feature#L278) - [apiTrashbin/trashbinFilesFolders.feature:279](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinFilesFolders.feature#L279) -#### [cannot restore to a different file-name](https://github.com/owncloud/ocis/issues/1122) -- [apiTrashbin/trashbinRestore.feature:108](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L108) -- [apiTrashbin/trashbinRestore.feature:109](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L109) -- [apiTrashbin/trashbinRestore.feature:110](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L110) -- [apiTrashbin/trashbinRestore.feature:111](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L111) - #### [href in trashbin PROPFIND response is wrong](https://github.com/owncloud/ocis/issues/1120) #### [cannot restore to a different file-name](https://github.com/owncloud/ocis/issues/1122) - [apiTrashbin/trashbinRestore.feature:309](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L309)