Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proper copy/move between multiple local storages #13360

Merged
merged 12 commits into from
Apr 14, 2015
34 changes: 34 additions & 0 deletions apps/files_sharing/lib/sharedstorage.php
Original file line number Diff line number Diff line change
Expand Up @@ -583,4 +583,38 @@ public function unshareStorage() {
return $result;
}

/**
* Resolve the path for the source of the share
*
* @param string $path
* @return array
*/
private function resolvePath($path) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add PHPDoc

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still applies 🙈

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

$source = $this->getSourcePath($path);
return \OC\Files\Filesystem::resolvePath($source);
}

/**
* @param \OCP\Files\Storage $sourceStorage
* @param string $sourceInternalPath
* @param string $targetInternalPath
* @return bool
*/
public function copyFromStorage(\OCP\Files\Storage $sourceStorage, $sourceInternalPath, $targetInternalPath) {
/** @var \OCP\Files\Storage $targetStorage */
list($targetStorage, $targetInternalPath) = $this->resolvePath($targetInternalPath);
return $targetStorage->copyFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath);
}

/**
* @param \OCP\Files\Storage $sourceStorage
* @param string $sourceInternalPath
* @param string $targetInternalPath
* @return bool
*/
public function moveFromStorage(\OCP\Files\Storage $sourceStorage, $sourceInternalPath, $targetInternalPath) {
/** @var \OCP\Files\Storage $targetStorage */
list($targetStorage, $targetInternalPath) = $this->resolvePath($targetInternalPath);
return $targetStorage->moveFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath);
}
}
70 changes: 64 additions & 6 deletions apps/files_sharing/tests/sharedstorage.php
Original file line number Diff line number Diff line change
Expand Up @@ -117,21 +117,21 @@ function testRenamePartFile() {
$this->assertTrue($user2View->file_exists($this->folder));

// create part file
$result = $user2View->file_put_contents($this->folder. '/foo.txt.part', 'some test data');
$result = $user2View->file_put_contents($this->folder . '/foo.txt.part', 'some test data');

$this->assertTrue(is_int($result));
// rename part file to real file
$result = $user2View->rename($this->folder. '/foo.txt.part', $this->folder. '/foo.txt');
$result = $user2View->rename($this->folder . '/foo.txt.part', $this->folder . '/foo.txt');

$this->assertTrue($result);

// check if the new file really exists
$this->assertTrue($user2View->file_exists( $this->folder. '/foo.txt'));
$this->assertTrue($user2View->file_exists($this->folder . '/foo.txt'));

// check if the rename also affected the owner
self::loginHelper(self::TEST_FILES_SHARING_API_USER1);

$this->assertTrue($this->view->file_exists( $this->folder. '/foo.txt'));
$this->assertTrue($this->view->file_exists($this->folder . '/foo.txt'));

//cleanup
\OCP\Share::unshare('folder', $fileinfo['fileid'], \OCP\Share::SHARE_TYPE_USER,
Expand All @@ -144,7 +144,7 @@ public function testFilesize() {
$fileinfoFile = $this->view->getFileInfo($this->filename);

$folderSize = $this->view->filesize($this->folder);
$file1Size = $this->view->filesize($this->folder. $this->filename);
$file1Size = $this->view->filesize($this->folder . $this->filename);
$file2Size = $this->view->filesize($this->filename);

$result = \OCP\Share::shareItem('folder', $fileinfoFolder['fileid'], \OCP\Share::SHARE_TYPE_USER,
Expand Down Expand Up @@ -373,11 +373,69 @@ function testMountSharesOtherUser() {
$this->assertTrue($rootView->file_exists('/' . self::TEST_FILES_SHARING_API_USER3 . '/files/' . $this->filename));

// make sure we didn't double setup shares for user 2 or mounted the shares for user 3 in user's 2 home
$this->assertFalse($rootView->file_exists('/' . self::TEST_FILES_SHARING_API_USER2 . '/files/' . $this->folder .' (2)'));
$this->assertFalse($rootView->file_exists('/' . self::TEST_FILES_SHARING_API_USER2 . '/files/' . $this->folder . ' (2)'));
$this->assertFalse($rootView->file_exists('/' . self::TEST_FILES_SHARING_API_USER2 . '/files/' . $this->filename));

//cleanup
self::loginHelper(self::TEST_FILES_SHARING_API_USER1);
$this->view->unlink($this->folder);
}

public function testCopyFromStorage() {
$folderInfo = $this->view->getFileInfo($this->folder);
self::loginHelper(self::TEST_FILES_SHARING_API_USER1);

// share 2 different files with 2 different users
\OCP\Share::shareItem('folder', $folderInfo['fileid'], \OCP\Share::SHARE_TYPE_USER,
self::TEST_FILES_SHARING_API_USER2, 31);

self::loginHelper(self::TEST_FILES_SHARING_API_USER2);
$view = new \OC\Files\View('/' . self::TEST_FILES_SHARING_API_USER2 . '/files');
$this->assertTrue($view->file_exists($this->folder));

/**
* @var \OCP\Files\Storage $sharedStorage
*/
list($sharedStorage,) = $view->resolvePath($this->folder);
$this->assertTrue($sharedStorage->instanceOfStorage('OCA\Files_Sharing\ISharedStorage'));

$sourceStorage = new \OC\Files\Storage\Temporary(array());
$sourceStorage->file_put_contents('foo.txt', 'asd');

$sharedStorage->copyFromStorage($sourceStorage, 'foo.txt', 'bar.txt');
$this->assertTrue($sharedStorage->file_exists('bar.txt'));
$this->assertEquals('asd', $sharedStorage->file_get_contents('bar.txt'));

self::loginHelper(self::TEST_FILES_SHARING_API_USER1);
$this->view->unlink($this->folder);
}

public function testMoveFromStorage() {
$folderInfo = $this->view->getFileInfo($this->folder);
self::loginHelper(self::TEST_FILES_SHARING_API_USER1);

// share 2 different files with 2 different users
\OCP\Share::shareItem('folder', $folderInfo['fileid'], \OCP\Share::SHARE_TYPE_USER,
self::TEST_FILES_SHARING_API_USER2, 31);

self::loginHelper(self::TEST_FILES_SHARING_API_USER2);
$view = new \OC\Files\View('/' . self::TEST_FILES_SHARING_API_USER2 . '/files');
$this->assertTrue($view->file_exists($this->folder));

/**
* @var \OCP\Files\Storage $sharedStorage
*/
list($sharedStorage,) = $view->resolvePath($this->folder);
$this->assertTrue($sharedStorage->instanceOfStorage('OCA\Files_Sharing\ISharedStorage'));

$sourceStorage = new \OC\Files\Storage\Temporary(array());
$sourceStorage->file_put_contents('foo.txt', 'asd');

$sharedStorage->moveFromStorage($sourceStorage, 'foo.txt', 'bar.txt');
$this->assertTrue($sharedStorage->file_exists('bar.txt'));
$this->assertEquals('asd', $sharedStorage->file_get_contents('bar.txt'));

self::loginHelper(self::TEST_FILES_SHARING_API_USER1);
$this->view->unlink($this->folder);
}
}
9 changes: 5 additions & 4 deletions apps/files_trashbin/tests/storage.php
Original file line number Diff line number Diff line change
Expand Up @@ -202,12 +202,13 @@ public function testSingleStorageDeleteFail() {

$cache = $storage->getCache();

Filesystem::mount($storage, [], '/' . $this->user . '/files');
Filesystem::mount($storage, [], '/' . $this->user);
$storage->mkdir('files');
$this->userView->file_put_contents('test.txt', 'foo');
$this->assertTrue($storage->file_exists('test.txt'));
$this->assertTrue($storage->file_exists('files/test.txt'));
$this->assertFalse($this->userView->unlink('test.txt'));
$this->assertTrue($storage->file_exists('test.txt'));
$this->assertTrue($cache->inCache('test.txt'));
$this->assertTrue($storage->file_exists('files/test.txt'));
$this->assertTrue($cache->inCache('files/test.txt'));

// file should not be in the trashbin
$results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/files/');
Expand Down
55 changes: 55 additions & 0 deletions lib/private/files/storage/common.php
Original file line number Diff line number Diff line change
Expand Up @@ -525,4 +525,59 @@ public function setMountOptions(array $options) {
public function getMountOption($name, $default = null) {
return isset($this->mountOptions[$name]) ? $this->mountOptions[$name] : $default;
}
/**
* @param \OCP\Files\Storage $sourceStorage
* @param string $sourceInternalPath
* @param string $targetInternalPath
* @param bool $preserveMtime
* @return bool
*/
public function copyFromStorage(\OCP\Files\Storage $sourceStorage, $sourceInternalPath, $targetInternalPath, $preserveMtime = false) {
if ($sourceStorage->is_dir($sourceInternalPath)) {
$dh = $sourceStorage->opendir($sourceInternalPath);
$result = $this->mkdir($targetInternalPath);
if (is_resource($dh)) {
while ($result and ($file = readdir($dh)) !== false) {
if (!Filesystem::isIgnoredDir($file)) {
$result &= $this->copyFromStorage($sourceStorage, $sourceInternalPath . '/' . $file, $targetInternalPath . '/' . $file);
}
}
}
} else {
$source = $sourceStorage->fopen($sourceInternalPath, 'r');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will use the storage wrappers

$target = $this->fopen($targetInternalPath, 'w');
list(, $result) = \OC_Helper::streamCopy($source, $target);
if ($result and $preserveMtime) {
$this->touch($targetInternalPath, $sourceStorage->filemtime($sourceInternalPath));
}
fclose($source);
fclose($target);

if (!$result) {
// delete partially written target file
$this->unlink($targetInternalPath);
// delete cache entry that was created by fopen
$this->getCache()->remove($targetInternalPath);
}
}
return (bool)$result;
}

/**
* @param \OCP\Files\Storage $sourceStorage
* @param string $sourceInternalPath
* @param string $targetInternalPath
* @return bool
*/
public function moveFromStorage(\OCP\Files\Storage $sourceStorage, $sourceInternalPath, $targetInternalPath) {
$result = $this->copyFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath, true);
if ($result) {
if ($sourceStorage->is_dir($sourceInternalPath)) {
$result &= $sourceStorage->rmdir($sourceInternalPath);
} else {
$result &= $sourceStorage->unlink($sourceInternalPath);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add return

return $result;
}
}
38 changes: 37 additions & 1 deletion lib/private/files/storage/local.php
Original file line number Diff line number Diff line change
Expand Up @@ -322,7 +322,7 @@ public function hasUpdated($path, $time) {
* @param string $path
* @return string
*/
protected function getSourcePath($path) {
public function getSourcePath($path) {
$fullPath = $this->datadir . $path;
return $fullPath;
}
Expand Down Expand Up @@ -353,5 +353,41 @@ public function getETag($path) {
return parent::getETag($path);
}
}

/**
* @param \OCP\Files\Storage $sourceStorage
* @param string $sourceInternalPath
* @param string $targetInternalPath
* @return bool
*/
public function copyFromStorage(\OCP\Files\Storage $sourceStorage, $sourceInternalPath, $targetInternalPath) {
if($sourceStorage->instanceOfStorage('\OC\Files\Storage\Local')){
/**
* @var \OC\Files\Storage\Local $sourceStorage
*/
$rootStorage = new Local(['datadir' => '/']);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

datadir being / is wrong on windows, but Windows is also \OC\Files\Storage\Local

return $rootStorage->copy($sourceStorage->getSourcePath($sourceInternalPath), $this->getSourcePath($targetInternalPath));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} else {
return parent::copyFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath);
}
}

/**
* @param \OCP\Files\Storage $sourceStorage
* @param string $sourceInternalPath
* @param string $targetInternalPath
* @return bool
*/
public function moveFromStorage(\OCP\Files\Storage $sourceStorage, $sourceInternalPath, $targetInternalPath) {
if ($sourceStorage->instanceOfStorage('\OC\Files\Storage\Local')) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this will also catch the Windows local storage, right ?

/**
* @var \OC\Files\Storage\Local $sourceStorage
*/
$rootStorage = new Local(['datadir' => '/']);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

datadir being / is wrong on windows, but Windows is also \OC\Files\Storage\Local

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

atm, this implementation is not used on windows

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't see a point in this PR that would cause this not being ran on windows?!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since Windows support is dropped for master => Not a problem anymore.

return $rootStorage->rename($sourceStorage->getSourcePath($sourceInternalPath), $this->getSourcePath($targetInternalPath));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

} else {
return parent::moveFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath);
}
}
}
}
39 changes: 37 additions & 2 deletions lib/private/files/storage/wrapper/quota.php
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,14 @@ public function getQuota() {

/**
* @param string $path
* @param \OC\Files\Storage\Storage $storage
*/
protected function getSize($path) {
$cache = $this->getCache();
protected function getSize($path, $storage = null) {
if (is_null($storage)) {
$cache = $this->getCache();
} else {
$cache = $storage->getCache();
}
$data = $cache->get($path);
if (is_array($data) and isset($data['size'])) {
return $data['size'];
Expand Down Expand Up @@ -141,4 +146,34 @@ public function fopen($path, $mode) {
return $source;
}
}

/**
* @param \OCP\Files\Storage $sourceStorage
* @param string $sourceInternalPath
* @param string $targetInternalPath
* @return bool
*/
public function copyFromStorage(\OCP\Files\Storage $sourceStorage, $sourceInternalPath, $targetInternalPath) {
$free = $this->free_space('');
if ($free < 0 or $this->getSize($sourceInternalPath, $sourceStorage) < $free) {
return $this->storage->copyFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath);
} else {
return false;
}
}

/**
* @param \OCP\Files\Storage $sourceStorage
* @param string $sourceInternalPath
* @param string $targetInternalPath
* @return bool
*/
public function moveFromStorage(\OCP\Files\Storage $sourceStorage, $sourceInternalPath, $targetInternalPath) {
$free = $this->free_space('');
if ($free < 0 or $this->getSize($sourceInternalPath, $sourceStorage) < $free) {
return $this->storage->moveFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath);
} else {
return false;
}
}
}
20 changes: 20 additions & 0 deletions lib/private/files/storage/wrapper/wrapper.php
Original file line number Diff line number Diff line change
Expand Up @@ -505,4 +505,24 @@ public function getDirectDownload($path) {
public function verifyPath($path, $fileName) {
$this->storage->verifyPath($path, $fileName);
}

/**
* @param \OCP\Files\Storage $sourceStorage
* @param string $sourceInternalPath
* @param string $targetInternalPath
* @return bool
*/
public function copyFromStorage(\OCP\Files\Storage $sourceStorage, $sourceInternalPath, $targetInternalPath) {
return $this->storage->copyFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath);
}

/**
* @param \OCP\Files\Storage $sourceStorage
* @param string $sourceInternalPath
* @param string $targetInternalPath
* @return bool
*/
public function moveFromStorage(\OCP\Files\Storage $sourceStorage, $sourceInternalPath, $targetInternalPath) {
return $this->storage->moveFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath);
}
}
Loading