Skip to content

feat(tests): Test that mtime is the same after move or rename #43689

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.

Sign up for GitHub

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

Draft
come-nc wants to merge 9 commits into
base: master
Choose a base branch
from
Draft

feat(tests): Test that mtime is the same after move or rename #43689

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
531ec42
feat(tests): Test that mtime is the same after move or rename
come-nc Feb 20, 2024
56918bf
fix(tests): Alter mtime to be in the past before checking it was pres…
come-nc Feb 20, 2024
d176d3c
fix: Fix mtime preservation on rename in KnownMtime wrapper
come-nc Feb 20, 2024
ad68b06
feat(tests): Add test for moveFromStorage
come-nc Feb 22, 2024
afcbdf5
fix(tests): Fix moveFromStorage test for encryption wrapper
come-nc Feb 22, 2024
a9c3a14
fix(files): Fix mtime preservation in Local and Common storages
come-nc Feb 22, 2024
f9b4cf4
fix(files): Preserve directory mtime on rename and move
come-nc Feb 22, 2024
d37436a
fix: Update children classes of Common to respect copy signature
come-nc Feb 22, 2024
c412f34
fixup! fix: Update children classes of Common to respect copy signature
come-nc Feb 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions apps/files_external/lib/Lib/Storage/AmazonS3.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -573,7 +573,11 @@ public function touch($path, $mtime = null) {
return true;
}

public function copy($source, $target, $isFile = null) {
/**
* @param string $source
* @param string $target
*/
public function copy($source, $target, bool $preserveMtime = false, ?bool $isFile = null): bool {
$source = $this->normalizePath($source);
$target = $this->normalizePath($target);

Expand Down Expand Up @@ -607,7 +611,7 @@ public function copy($source, $target, $isFile = null) {
foreach ($this->getDirectoryContent($source) as $item) {
$childSource = $source . '/' . $item['name'];
$childTarget = $target . '/' . $item['name'];
$this->copy($childSource, $childTarget, $item['mimetype'] !== FileInfo::MIMETYPE_FOLDER);
$this->copy($childSource, $childTarget, $preserveMtime, $item['mimetype'] !== FileInfo::MIMETYPE_FOLDER);
}
}

Expand Down
8 changes: 6 additions & 2 deletions apps/files_external/lib/Lib/Storage/SFTP.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -519,9 +519,13 @@ public function writeStream(string $path, $stream, int $size = null): int {
}
}

public function copy($source, $target) {
/**
* @param string $source
* @param string $target
*/
public function copy($source, $target, bool $preserveMtime = false): bool {
if ($this->is_dir($source) || $this->is_dir($target)) {
return parent::copy($source, $target);
return parent::copy($source, $target, $preserveMtime);
} else {
$absSource = $this->absPath($source);
$absTarget = $this->absPath($target);
Expand Down
14 changes: 12 additions & 2 deletions apps/files_external/lib/Lib/Storage/Swift.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -483,7 +483,11 @@ public function touch($path, $mtime = null) {
}
}

public function copy($source, $target) {
/**
* @param string $source
* @param string $target
*/
public function copy($source, $target, bool $preserveMtime = false): bool {
$source = $this->normalizePath($source);
$target = $this->normalizePath($target);

Expand All @@ -502,6 +506,12 @@ public function copy($source, $target) {
// invalidate target object to force repopulation on fetch
$this->objectCache->remove($target);
$this->objectCache->remove($target . '/');
if ($preserveMtime) {
$mTime = $this->filemtime($source);
if (is_int($mTime)) {
$this->touch($target, $mTime);
}
}
} catch (BadResponseError $e) {
\OC::$server->get(LoggerInterface::class)->error($e->getMessage(), [
'exception' => $e,
Expand Down Expand Up @@ -534,7 +544,7 @@ public function copy($source, $target) {

$source = $source . '/' . $file;
$target = $target . '/' . $file;
$this->copy($source, $target);
$this->copy($source, $target, $preserveMtime);
}
} else {
//file does not exist
Expand Down
2 changes: 1 addition & 1 deletion apps/files_trashbin/tests/StorageTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@
use Test\Traits\MountProviderTrait;

class TemporaryNoCross extends Temporary {
public function copyFromStorage(IStorage $sourceStorage, $sourceInternalPath, $targetInternalPath, $preserveMtime = null) {
public function copyFromStorage(IStorage $sourceStorage, $sourceInternalPath, $targetInternalPath, bool $preserveMtime = false): bool {
return Common::copyFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath, $preserveMtime);
}

Expand Down
1 change: 1 addition & 0 deletions lib/composer/composer/autoload_classmap.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -413,6 +413,7 @@
'OCP\\Files\\Storage\\IChunkedFileWrite' => $baseDir . '/lib/public/Files/Storage/IChunkedFileWrite.php',
'OCP\\Files\\Storage\\IDisableEncryptionStorage' => $baseDir . '/lib/public/Files/Storage/IDisableEncryptionStorage.php',
'OCP\\Files\\Storage\\ILockingStorage' => $baseDir . '/lib/public/Files/Storage/ILockingStorage.php',
'OCP\\Files\\Storage\\IMtimePreserving' => $baseDir . '/lib/public/Files/Storage/IMtimePreserving.php',
'OCP\\Files\\Storage\\INotifyStorage' => $baseDir . '/lib/public/Files/Storage/INotifyStorage.php',
'OCP\\Files\\Storage\\IReliableEtagStorage' => $baseDir . '/lib/public/Files/Storage/IReliableEtagStorage.php',
'OCP\\Files\\Storage\\IStorage' => $baseDir . '/lib/public/Files/Storage/IStorage.php',
Expand Down
1 change: 1 addition & 0 deletions lib/composer/composer/autoload_static.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,6 +446,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2
'OCP\\Files\\Storage\\IChunkedFileWrite' => __DIR__ . '/../../..' . '/lib/public/Files/Storage/IChunkedFileWrite.php',
'OCP\\Files\\Storage\\IDisableEncryptionStorage' => __DIR__ . '/../../..' . '/lib/public/Files/Storage/IDisableEncryptionStorage.php',
'OCP\\Files\\Storage\\ILockingStorage' => __DIR__ . '/../../..' . '/lib/public/Files/Storage/ILockingStorage.php',
'OCP\\Files\\Storage\\IMtimePreserving' => __DIR__ . '/../../..' . '/lib/public/Files/Storage/IMtimePreserving.php',
'OCP\\Files\\Storage\\INotifyStorage' => __DIR__ . '/../../..' . '/lib/public/Files/Storage/INotifyStorage.php',
'OCP\\Files\\Storage\\IReliableEtagStorage' => __DIR__ . '/../../..' . '/lib/public/Files/Storage/IReliableEtagStorage.php',
'OCP\\Files\\Storage\\IStorage' => __DIR__ . '/../../..' . '/lib/public/Files/Storage/IStorage.php',
Expand Down
6 changes: 3 additions & 3 deletions lib/private/Files/ObjectStore/ObjectStoreStorage.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -596,7 +596,7 @@ public function copyFromStorage(
$sourceInternalPath,
$targetInternalPath,
$preserveMtime = false
) {
): bool {
if ($sourceStorage->instanceOfStorage(ObjectStoreStorage::class)) {
/** @var ObjectStoreStorage $sourceStorage */
if ($sourceStorage->getObjectStore()->getStorageId() === $this->getObjectStore()->getStorageId()) {
Expand All @@ -614,10 +614,10 @@ public function copyFromStorage(
}
}

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

public function copy($source, $target) {
public function copy($source, $target, bool $preserveMtime = false): bool {
$source = $this->normalizePath($source);
$target = $this->normalizePath($target);

Expand Down
37 changes: 26 additions & 11 deletions lib/private/Files/Storage/Common.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
use OCP\Files\InvalidPathException;
use OCP\Files\ReservedWordException;
use OCP\Files\Storage\ILockingStorage;
use OCP\Files\Storage\IMtimePreserving;
use OCP\Files\Storage\IStorage;
use OCP\Files\Storage\IWriteStreamStorage;
use OCP\Lock\ILockingProvider;
Expand All @@ -78,7 +79,7 @@
* Some \OC\Files\Storage\Common methods call functions which are first defined
* in classes which extend it, e.g. $this->stat() .
*/
abstract class Common implements Storage, ILockingStorage, IWriteStreamStorage {
abstract class Common implements Storage, ILockingStorage, IWriteStreamStorage, IMtimePreserving {
use LocalTempFileTrait;

protected $cache;
Expand Down Expand Up @@ -222,30 +223,37 @@ public function rename($source, $target) {
$this->remove($target);

$this->removeCachedFile($source);
return $this->copy($source, $target) and $this->remove($source);
return $this->copy($source, $target, true) && $this->remove($source);
}

public function copy($source, $target) {
public function copy($source, $target, bool $preserveMtime = false): bool {
if ($this->is_dir($source)) {
$this->remove($target);
$dir = $this->opendir($source);
$this->mkdir($target);
while ($file = readdir($dir)) {
if (!Filesystem::isIgnoredDir($file)) {
if (!$this->copy($source . '/' . $file, $target . '/' . $file)) {
if (!$this->copy($source . '/' . $file, $target . '/' . $file, $preserveMtime)) {
closedir($dir);
return false;
}
}
}
closedir($dir);
if ($preserveMtime) {
$mtime = $this->filemtime($source);
$this->touch($target, is_int($mtime) ? $mtime : null);
}
return true;
} else {
$sourceStream = $this->fopen($source, 'r');
$targetStream = $this->fopen($target, 'w');
[, $result] = \OC_Helper::streamCopy($sourceStream, $targetStream);
if (!$result) {
\OCP\Server::get(LoggerInterface::class)->warning("Failed to write data while copying $source to $target");
} elseif ($preserveMtime) {
$mtime = $this->filemtime($source);
$this->touch($target, is_int($mtime) ? $mtime : null);
}
$this->removeCachedFile($target);
return $result;
Expand Down Expand Up @@ -615,21 +623,28 @@ public function getMountOption($name, $default = null) {
* @param bool $preserveMtime
* @return bool
*/
public function copyFromStorage(IStorage $sourceStorage, $sourceInternalPath, $targetInternalPath, $preserveMtime = false) {
public function copyFromStorage(IStorage $sourceStorage, $sourceInternalPath, $targetInternalPath, bool $preserveMtime = false): bool {
if ($sourceStorage === $this) {
return $this->copy($sourceInternalPath, $targetInternalPath);
return $this->copy($sourceInternalPath, $targetInternalPath, $preserveMtime);
}

if ($sourceStorage->is_dir($sourceInternalPath)) {
$dh = $sourceStorage->opendir($sourceInternalPath);
$result = $this->mkdir($targetInternalPath);
if (is_resource($dh)) {
$result = true;
while ($result and ($file = readdir($dh)) !== false) {
if (!$this->is_dir($targetInternalPath)) {
$result = $this->mkdir($targetInternalPath);
} else {
$result = true;
}
while ($result && ($file = readdir($dh)) !== false) {
if (!Filesystem::isIgnoredDir($file)) {
$result &= $this->copyFromStorage($sourceStorage, $sourceInternalPath . '/' . $file, $targetInternalPath . '/' . $file);
$result = $this->copyFromStorage($sourceStorage, $sourceInternalPath . '/' . $file, $targetInternalPath . '/' . $file, $preserveMtime);
}
}
if ($result && $preserveMtime) {
$mtime = $sourceStorage->filemtime($sourceInternalPath);
$this->touch($targetInternalPath, is_int($mtime) ? $mtime : null);
}
}
} else {
$source = $sourceStorage->fopen($sourceInternalPath, 'r');
Expand All @@ -655,7 +670,7 @@ public function copyFromStorage(IStorage $sourceStorage, $sourceInternalPath, $t
$this->getCache()->remove($targetInternalPath);
}
}
return (bool)$result;
return $result;
}

/**
Expand Down
2 changes: 1 addition & 1 deletion lib/private/Files/Storage/DAV.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -565,7 +565,7 @@ public function rename($source, $target) {
}

/** {@inheritdoc} */
public function copy($source, $target) {
public function copy($source, $target, bool $preserveMtime = false): bool {
$this->init();
$source = $this->cleanPath($source);
$target = $this->cleanPath($target);
Expand Down
4 changes: 2 additions & 2 deletions lib/private/Files/Storage/FailedStorage.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ public function rename($source, $target) {
throw new StorageNotAvailableException($this->e->getMessage(), $this->e->getCode(), $this->e);
}

public function copy($source, $target) {
public function copy($source, $target, bool $preserveMtime = false): bool {
throw new StorageNotAvailableException($this->e->getMessage(), $this->e->getCode(), $this->e);
}

Expand Down Expand Up @@ -180,7 +180,7 @@ public function verifyPath($path, $fileName) {
return true;
}

public function copyFromStorage(IStorage $sourceStorage, $sourceInternalPath, $targetInternalPath, $preserveMtime = false) {
public function copyFromStorage(IStorage $sourceStorage, $sourceInternalPath, $targetInternalPath, bool $preserveMtime = false): bool {
throw new StorageNotAvailableException($this->e->getMessage(), $this->e->getCode(), $this->e);
}

Expand Down
21 changes: 12 additions & 9 deletions lib/private/Files/Storage/Local.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,21 +398,27 @@ public function rename($source, $target): bool {
return $this->copy($source, $target) && $this->unlink($source);
}

public function copy($source, $target) {
public function copy($source, $target, bool $preserveMtime = false): bool {
if ($this->is_dir($source)) {
return parent::copy($source, $target);
return parent::copy($source, $target, $preserveMtime);
} else {
$oldMask = umask($this->defUMask);
if ($this->unlinkOnTruncate) {
$this->unlink($target);
}
$result = copy($this->getSourcePath($source), $this->getSourcePath($target));
$sourceInternalPath = $this->getSourcePath($source);
$targetInternalPath = $this->getSourcePath($target);
$result = copy($sourceInternalPath, $targetInternalPath);

Check failure

Code scanning / Psalm

TaintedFile

Detected tainted file handling

Check failure

Code scanning / Psalm

TaintedFile

Detected tainted file handling
umask($oldMask);
if ($this->caseInsensitive) {
if (mb_strtolower($target) === mb_strtolower($source) && !$this->file_exists($target)) {
return false;
}
}
if ($result && $preserveMtime) {
$mtime = $this->filemtime($sourceInternalPath);
$this->touch($targetInternalPath, is_int($mtime) ? $mtime : null);
}
return $result;
}
}
Expand Down Expand Up @@ -590,13 +596,10 @@ private function canDoCrossStorageMove(IStorage $sourceStorage) {
}

/**
* @param IStorage $sourceStorage
* @param string $sourceInternalPath
* @param string $targetInternalPath
* @param bool $preserveMtime
* @return bool
*/
public function copyFromStorage(IStorage $sourceStorage, $sourceInternalPath, $targetInternalPath, $preserveMtime = false) {
public function copyFromStorage(IStorage $sourceStorage, $sourceInternalPath, $targetInternalPath, bool $preserveMtime = false): bool {
if ($this->canDoCrossStorageMove($sourceStorage)) {
if ($sourceStorage->instanceOfStorage(Jail::class)) {
/**
Expand All @@ -608,9 +611,9 @@ public function copyFromStorage(IStorage $sourceStorage, $sourceInternalPath, $t
* @var \OC\Files\Storage\Local $sourceStorage
*/
$rootStorage = new Local(['datadir' => '/']);
return $rootStorage->copy($sourceStorage->getSourcePath($sourceInternalPath), $this->getSourcePath($targetInternalPath));
return $rootStorage->copy($sourceStorage->getSourcePath($sourceInternalPath), $this->getSourcePath($targetInternalPath), $preserveMtime);
} else {
return parent::copyFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath);
return parent::copyFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath, $preserveMtime);
}
}

Expand Down
22 changes: 12 additions & 10 deletions lib/private/Files/Storage/PolyFill/CopyDirectory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,35 +64,37 @@ abstract public function opendir($path);
*/
abstract public function mkdir($path);

public function copy($source, $target) {
/**
* Copy file or folder
*
* @param string $source
* @param string $target
*/
public function copy($source, $target, bool $preserveMtime = false): bool {
if ($this->is_dir($source)) {
if ($this->file_exists($target)) {
$this->unlink($target);
}
$this->mkdir($target);
return $this->copyRecursive($source, $target);
return $this->copyRecursive($source, $target, $preserveMtime);
} else {
return parent::copy($source, $target);
return parent::copy($source, $target, $preserveMtime);
}
}

/**
* For adapters that don't support copying folders natively
*
* @param $source
* @param $target
* @return bool
*/
protected function copyRecursive($source, $target) {
protected function copyRecursive(string $source, string $target, bool $preserveMtime = false): bool {
$dh = $this->opendir($source);
$result = true;
while ($file = readdir($dh)) {
if (!\OC\Files\Filesystem::isIgnoredDir($file)) {
if ($this->is_dir($source . '/' . $file)) {
$this->mkdir($target . '/' . $file);
$result = $this->copyRecursive($source . '/' . $file, $target . '/' . $file);
$result = $this->copyRecursive($source . '/' . $file, $target . '/' . $file, $preserveMtime);
} else {
$result = parent::copy($source . '/' . $file, $target . '/' . $file);
$result = parent::copy($source . '/' . $file, $target . '/' . $file, $preserveMtime);
}
if (!$result) {
break;
Expand Down
4 changes: 2 additions & 2 deletions lib/private/Files/Storage/Wrapper/Encryption.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -824,9 +824,9 @@ private function copyBetweenStorage(
$result = true;
}
if (is_resource($dh)) {
while ($result and ($file = readdir($dh)) !== false) {
while ($result && ($file = readdir($dh)) !== false) {
if (!Filesystem::isIgnoredDir($file)) {
$result &= $this->copyFromStorage($sourceStorage, $sourceInternalPath . '/' . $file, $targetInternalPath . '/' . $file, false, $isRename);
$result = $this->copyFromStorage($sourceStorage, $sourceInternalPath . '/' . $file, $targetInternalPath . '/' . $file, $preserveMtime, $isRename);
}
}
}
Expand Down
5 changes: 3 additions & 2 deletions lib/private/Files/Storage/Wrapper/KnownMtime.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ public function unlink($path) {
public function rename($source, $target) {
$result = parent::rename($source, $target);
if ($result) {
$this->knowMtimes->set($target, $this->clock->now()->getTimestamp());
$this->knowMtimes->set($target, $this->filemtime($source));
$this->knowMtimes->set($source, $this->clock->now()->getTimestamp());
}
return $result;
Expand Down Expand Up @@ -125,9 +125,10 @@ public function copyFromStorage(IStorage $sourceStorage, $sourceInternalPath, $t
}

public function moveFromStorage(IStorage $sourceStorage, $sourceInternalPath, $targetInternalPath) {
$mTime = $sourceStorage->filemtime($sourceInternalPath);
$result = parent::moveFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath);
if ($result) {
$this->knowMtimes->set($targetInternalPath, $this->clock->now()->getTimestamp());
$this->knowMtimes->set($targetInternalPath, $mTime);
}
return $result;
}
Expand Down
Loading