Proper copy/move between multiple local storages #13360

icewind1991 · 2015-01-14T16:19:00Z

This allows storage backends to implement logic for efficient copying/moving between storage backends (i.e. from local storage to local storage)

Part of the work for https://github.com/owncloud/enterprise/issues/472

Next step is having the shared storage pass the copy/move operation to the underlying storage using the same interface so it trigers the local->local copy/move logic

cc @DeepDiver1975 @PVince81

Test plan @jnfrmarks @SergioBertolinSG

from/to	local	shared	mounted (e.g. SMB)	shared mounted
local
shared
mounted (e.g. SMB)
shared mounted

LukasReschke · 2015-01-14T16:21:21Z

lib/private/files/storage/local.php

+		 * @param string $targetInternalPath
+		 * @return bool
+		 */
+		public function crossMove($sourceStorage, $sourceInternalPath, $targetInternalPath) {


enforce type of $sourceStorage?

Also add PHP docs

Rename to "moveFromStorage" ?

Should we actually return false or still attempt to download the file then delete it remotely ?

PVince81 · 2015-01-14T16:33:59Z

I guess this is also a step forward to fix file id stability ? (at least when copying from local to local, not ext storages)

PVince81 · 2015-01-14T16:38:17Z

lib/private/files/storage/local.php

+				 * @var \OC\Files\Storage\Local $sourceStorage
+				 */
+				$rootStorage = new Local(['datadir' => '/']);
+				return $rootStorage->copy($sourceStorage->getSourcePath($sourceInternalPath), $this->getSourcePath($targetInternalPath));


Should we add "getSourcePath()" to the Storage interface ?
I see that method is protected on the Local storage but public in SharedStorage.

Should we add "getSourcePath()" to the Storage interface

No, it only makes sense for some backends, and the Local and Shared versions both mean something different

icewind1991 · 2015-01-14T16:42:31Z

I guess this is also a step forward to fix file id stability ? (at least when copying from local to local, not ext storages)

No, the cache operations are uneffected

icewind1991 · 2015-01-14T16:42:37Z

Added more unit tests

PVince81 · 2015-01-14T16:43:19Z

lib/private/files/view.php

+					/**
+					 * @var \OCP\Files\Storage\ICrossCopyStorage | \OCP\Files\Storage $storage2
+					 */
+					if ($storage2->instanceOfStorage('\OCP\Files\Storage\ICrossCopyStorage') and


Another idea is to have "Common" always implement "moveFromStorage" and "copyFromStorage" as a simple download + copy (streamcopy). Then have specializations like Local add an additional check and do the rename() instead.

This way the new interface + check might not even be needed ?

PVince81 · 2015-01-15T13:52:21Z

tests/lib/files/view.php

+	function testMoveBetweenStorageCrossNonLocal() {
+		$storage1 = $this->getTestStorage(true, '\Test\Files\TemporaryNoLocal');
+		$storage2 = $this->getTestStorage(true, '\Test\Files\TemporaryNoLocal');
+		$this->moveBetweenStorages($storage1, $storage2);


Add a test when moving from a "Local + Cross" storage to a "non Local non Cross" storage ?
And also the reverse way.

PVince81 · 2015-01-15T13:54:16Z

The reason I'd like to have the cross copy logic in "Common" is to have a fallback in any case.
Currently we're expecting the caller of "crossMove()" to know in what situation it is ok to call this method. From an API consumer point of view I should not have to care. I just need a method "moveFromStorage($storage1, $storage2, ...)" and that method takes care of either optimizing, if possible, or falling back to the default stream copy logic when not.

icewind1991 · 2015-01-15T14:14:56Z

@PVince81 done, this also cleans up View::copy and View::rename a bit 😄

PVince81 · 2015-01-15T14:56:52Z

lib/private/files/storage/common.php

+				}
+			}
+		} else {
+			$source = $sourceStorage->fopen($sourceInternalPath, 'r');


Question: will this go through the storage wrappers or bypass them ?

When we had this on $view level the fopen() was done on the view.

It will use the storage wrappers

PVince81 · 2015-01-15T15:02:10Z

This is already a great improvement! Thanks for the changes 😄

Can you add a unit test for the cases where moving from a cross to non-cross, etc ?

PVince81 · 2015-01-15T15:03:35Z

lib/private/files/storage/local.php

+				 * @var \OC\Files\Storage\Local $sourceStorage
+				 */
+				$rootStorage = new Local(['datadir' => '/']);
+				return $rootStorage->rename($sourceStorage->getSourcePath($sourceInternalPath), $this->getSourcePath($targetInternalPath));


Hmmm... what happens if $sourceStorage is a "local external storage mount" ?
I guess this should also work because it will rely on PHP's system rename with the absolute paths, then ?

PVince81 · 2015-01-15T15:08:42Z

@DeepDiver1975 are we sure to want this in 8.0 ?

If we only merge this PR, I have the feeling that there isn't much benefit apart from situations where people use the "Local" external storage. The benefit will only start to show itself as soon as the SharedStorage part (the next step) is implemented. (please correct me if wrong @icewind1991)

So unless both are planned for 8.0, we should probably set this PR here to 8.1.

What do you think ?

PVince81 · 2015-01-16T10:36:37Z

lib/private/files/view.php

-							$storage1->unlink($internalPath1);
-						}
-					}
+					return $storage2->moveFromStorage($storage1, $internalPath1, $internalPath2);


Are you sure we should return here directly ?
This would prevent proxy/hooks to be triggered in the lines below.

icewind1991 · 2015-01-16T12:36:47Z

Note that this will be easy to implement for the refactored shared storage by implementing it for the jail wrapper

ghost · 2015-04-13T13:46:31Z

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/11464/
🚀 Test PASSed.🚀

PVince81 · 2015-04-13T14:48:59Z

Will this clash / is this compatible with #15360 ?

PVince81 · 2015-04-13T14:54:21Z

Note: this also needs to be tested with encryption (once #15578 is fixed) because it might be affected.
I guess we could still first merge this and test afterwards.

Going to test this

PVince81 · 2015-04-13T15:12:59Z

from/to	local	shared	mounted (e.g. SMB)	shared mounted
local	✅	✅	✅	✅
shared	✅	✅	✅	✅
mounted (e.g. SMB)	✅	✅	✅	✅
shared mounted	✅	✅	✅	✅

Note: I also tested moving from share to share as recipient when the owner is the same, and also when the owner is different. (because the same owner would be the same local storage)

For "shared mounted" to "shared mounted", I tried with two different target storages and also the same (two different folders shared from the same user's personal ext storage)

All looks fine 👍

PVince81 · 2015-04-13T15:14:55Z

I noticed that if I move a big file from "a" to "b" where both are on the same SFTP storage, but shared separately with me, it still takes a long time. I guess this PR doesn't cover such cases ? (because the underlying storage is the same, so moving the file from one place to the other should be faster)

PVince81 · 2015-04-13T15:15:38Z

Ok, this PR is only for underlying local storages. A later step would be to do this for any type of storage.

icewind1991 · 2015-04-13T15:31:39Z

Yes, this PR doesn't optimize all cases with shared storages yet

icewind1991 · 2015-04-13T15:31:55Z

@DeepDiver1975 @LukasReschke @MorrisJobke please review

scrutinizer-notifier · 2015-04-14T10:36:14Z

A new inspection was created.

ghost · 2015-04-14T11:09:03Z

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/11489/
🚀 Test PASSed.🚀

MorrisJobke · 2015-04-14T12:35:06Z

I tested this with some of the above test cases and it works (and there are no errors in the log) 👍

Proper copy/move between multiple local storages

PVince81 · 2015-04-14T12:58:19Z

🍻

DeepDiver1975 added this to the 8.0-current milestone Jan 14, 2015

LukasReschke reviewed Jan 14, 2015
View reviewed changes

PVince81 reviewed Jan 14, 2015
View reviewed changes

PVince81 mentioned this pull request Jan 15, 2015

[WIP] Keep file ids when moving between storages #13385

Closed

4 tasks

PVince81 reviewed Jan 15, 2015
View reviewed changes

icewind1991 force-pushed the cross-storage-move branch from 6c8a8ea to 12ca5d3 Compare January 15, 2015 15:02

PVince81 reviewed Jan 15, 2015
View reviewed changes

PVince81 reviewed Jan 16, 2015
View reviewed changes

PVince81 mentioned this pull request Jan 16, 2015

File ids must stay stable #13310

Closed

3 tasks

icewind1991 force-pushed the cross-storage-move branch from 12ca5d3 to 0f96cf7 Compare January 16, 2015 12:34

icewind1991 added 9 commits April 13, 2015 15:13

Improve cross storage copy between local storages

31e9470

Implement move/copyFromStorage for shared storage

d726db7

Fix test

d16ee41

Make getSourcePath accessible for storage wrappers

c4ec8fb

properly return false if we cant delete the source file

d26c6ca

Detect storage full when doing cross storage copy/move

4047739

Properly handle copy/move failures in cross storage copy/move

0772e3b

add missing return for shared

d4c91dc

fix rebase issue

c29419e

icewind1991 force-pushed the cross-storage-move branch from 67cb749 to c29419e Compare April 13, 2015 13:13

icewind1991 mentioned this pull request Apr 13, 2015

[enc2] Move file out of share as recipient breaks file #15578

Closed

a bit more phpdoc

0f21303

MorrisJobke added a commit that referenced this pull request Apr 14, 2015

Merge pull request #13360 from owncloud/cross-storage-move

82cab25

Proper copy/move between multiple local storages

MorrisJobke merged commit 82cab25 into master Apr 14, 2015

MorrisJobke deleted the cross-storage-move branch April 14, 2015 12:35

MorrisJobke mentioned this pull request May 20, 2015

Deleting a folder with many files via client or web interface take longer #16482

Closed

PVince81 mentioned this pull request Sep 7, 2016

9.1.0: realpath error messages in OC log although file uploads succeed #26033

Closed

lock bot locked as resolved and limited conversation to collaborators Aug 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proper copy/move between multiple local storages #13360

Proper copy/move between multiple local storages #13360

icewind1991 commented Jan 14, 2015

LukasReschke Jan 14, 2015

PVince81 Jan 14, 2015

PVince81 Jan 14, 2015

PVince81 commented Jan 14, 2015

PVince81 Jan 14, 2015

icewind1991 Jan 14, 2015

icewind1991 commented Jan 14, 2015

icewind1991 commented Jan 14, 2015

PVince81 Jan 14, 2015

PVince81 Jan 15, 2015

PVince81 commented Jan 15, 2015

icewind1991 commented Jan 15, 2015

PVince81 Jan 15, 2015

icewind1991 Jan 15, 2015

PVince81 commented Jan 15, 2015

PVince81 Jan 15, 2015

icewind1991 Jan 15, 2015

PVince81 commented Jan 15, 2015

PVince81 Jan 16, 2015

icewind1991 commented Jan 16, 2015

ghost commented Apr 13, 2015

PVince81 commented Apr 13, 2015

PVince81 commented Apr 13, 2015

PVince81 commented Apr 13, 2015

PVince81 commented Apr 13, 2015

PVince81 commented Apr 13, 2015

icewind1991 commented Apr 13, 2015

icewind1991 commented Apr 13, 2015

scrutinizer-notifier commented Apr 14, 2015

ghost commented Apr 14, 2015

MorrisJobke commented Apr 14, 2015

PVince81 commented Apr 14, 2015

Proper copy/move between multiple local storages #13360

Proper copy/move between multiple local storages #13360

Conversation

icewind1991 commented Jan 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented Jan 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

icewind1991 commented Jan 14, 2015

icewind1991 commented Jan 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented Jan 15, 2015

icewind1991 commented Jan 15, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented Jan 15, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented Jan 15, 2015

Choose a reason for hiding this comment

icewind1991 commented Jan 16, 2015

ghost commented Apr 13, 2015

PVince81 commented Apr 13, 2015

PVince81 commented Apr 13, 2015

PVince81 commented Apr 13, 2015

PVince81 commented Apr 13, 2015

PVince81 commented Apr 13, 2015

icewind1991 commented Apr 13, 2015

icewind1991 commented Apr 13, 2015

scrutinizer-notifier commented Apr 14, 2015

ghost commented Apr 14, 2015

MorrisJobke commented Apr 14, 2015

PVince81 commented Apr 14, 2015