From d267a57011a56d2f593b5bbc7a153c053d3aeec8 Mon Sep 17 00:00:00 2001 From: Josh Date: Wed, 3 Jun 2026 11:06:16 -0400 Subject: [PATCH 1/4] refactor(Node): copy/move early exits/code clarity - Substantive changes are in copy() and move() - Other changes are unrelated modernization (constructor property promotion + elimination of outdated docblocks for functions already covered in interfaces) Signed-off-by: Josh --- lib/private/Files/Node/Node.php | 197 ++++++++++---------------------- 1 file changed, 59 insertions(+), 138 deletions(-) diff --git a/lib/private/Files/Node/Node.php b/lib/private/Files/Node/Node.php index a599833567fbf..19691acbc0aaa 100644 --- a/lib/private/Files/Node/Node.php +++ b/lib/private/Files/Node/Node.php @@ -28,32 +28,19 @@ // FIXME: this class really should be abstract (+1) class Node implements INode { - /** - * @var View $view - */ - protected $view; - - protected IRootFolder $root; - - /** - * @param View $view - * @param \OCP\Files\IRootFolder $root - * @param string $path - * @param FileInfo $fileInfo - */ public function __construct( - IRootFolder $root, - $view, + protected IRootFolder $root, + protected View $view, protected $path, protected ?FileInfo $fileInfo = null, protected ?INode $parent = null, private bool $infoHasSubMountsIncluded = true, ) { if (Filesystem::normalizePath($view->getRoot()) !== '/') { - throw new PreConditionNotMetException('The view passed to the node should not have any fake root set'); + throw new PreConditionNotMetException( + 'The view passed to the node should not have any fake root set' + ); } - $this->view = $view; - $this->root = $root; } /** @@ -73,6 +60,8 @@ protected function createNonExistingNode($path) { * @return FileInfo * @throws InvalidPathException * @throws NotFoundException + * + * @internal */ public function getFileInfo(bool $includeMountPoint = true) { if (!$this->fileInfo) { @@ -131,12 +120,6 @@ protected function checkPermissions($permissions) { public function delete() { } - /** - * @param int $mtime - * @throws InvalidPathException - * @throws NotFoundException - * @throws NotPermittedException - */ #[\Override] public function touch($mtime = null) { if ($this->checkPermissions(Constants::PERMISSION_UPDATE)) { @@ -163,123 +146,67 @@ public function getStorage() { return $storage; } - /** - * @return string - */ #[\Override] public function getPath() { return $this->path; } - /** - * @return string - */ #[\Override] public function getInternalPath() { return $this->getFileInfo(false)->getInternalPath(); } - /** - * @return int - * @throws InvalidPathException - * @throws NotFoundException - */ #[\Override] public function getId() { return $this->getFileInfo(false)->getId() ?? -1; } - /** - * @return array - */ #[\Override] public function stat() { return $this->view->stat($this->path); } - /** - * @return int - * @throws InvalidPathException - * @throws NotFoundException - */ #[\Override] public function getMTime() { return $this->getFileInfo()->getMTime(); } - /** - * @param bool $includeMounts - * @return int|float - * @throws InvalidPathException - * @throws NotFoundException - */ #[\Override] public function getSize($includeMounts = true): int|float { return $this->getFileInfo()->getSize($includeMounts); } - /** - * @return string - * @throws InvalidPathException - * @throws NotFoundException - */ #[\Override] public function getEtag() { return $this->getFileInfo()->getEtag(); } - /** - * @return int - * @throws InvalidPathException - * @throws NotFoundException - */ #[\Override] public function getPermissions() { return $this->getFileInfo(false)->getPermissions(); } - /** - * @return bool - * @throws InvalidPathException - * @throws NotFoundException - */ #[\Override] public function isReadable() { return $this->getFileInfo(false)->isReadable(); } - /** - * @return bool - * @throws InvalidPathException - * @throws NotFoundException - */ #[\Override] public function isUpdateable() { return $this->getFileInfo(false)->isUpdateable(); } - /** - * @return bool - * @throws InvalidPathException - * @throws NotFoundException - */ #[\Override] public function isDeletable() { return $this->getFileInfo(false)->isDeletable(); } - /** - * @return bool - * @throws InvalidPathException - * @throws NotFoundException - */ #[\Override] public function isShareable() { return $this->getFileInfo(false)->isShareable(); } /** - * @return bool * @throws InvalidPathException * @throws NotFoundException */ @@ -320,9 +247,6 @@ public function getParent(): INode|IRootFolder { return $this->parent; } - /** - * @return string - */ #[\Override] public function getName() { return basename($this->path); @@ -341,6 +265,8 @@ protected function normalizePath($path) { * * @param string $path * @return bool + * + * @internal */ public function isValidPath($path) { return Filesystem::isValidPath($path); @@ -395,28 +321,16 @@ public function getExtension(): string { return $this->getFileInfo(false)->getExtension(); } - /** - * @param int $type \OCP\Lock\ILockingProvider::LOCK_SHARED or \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE - * @throws LockedException - */ #[\Override] public function lock($type) { $this->view->lockFile($this->path, $type); } - /** - * @param int $type \OCP\Lock\ILockingProvider::LOCK_SHARED or \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE - * @throws LockedException - */ #[\Override] public function changeLock($type) { $this->view->changeLock($this->path, $type); } - /** - * @param int $type \OCP\Lock\ILockingProvider::LOCK_SHARED or \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE - * @throws LockedException - */ #[\Override] public function unlock($type) { $this->view->unlockFile($this->path, $type); @@ -433,20 +347,24 @@ public function unlock($type) { public function copy($targetPath) { $targetPath = $this->normalizePath($targetPath); $parent = $this->root->get(dirname($targetPath)); - if ($parent instanceof Folder && $this->isValidPath($targetPath) && $parent->isCreatable()) { - $nonExisting = $this->createNonExistingNode($targetPath); - $this->sendHooks(['preCopy'], [$this, $nonExisting]); - $this->sendHooks(['preWrite'], [$nonExisting]); - if (!$this->view->copy($this->path, $targetPath)) { - throw new NotPermittedException('Could not copy ' . $this->path . ' to ' . $targetPath); - } - $targetNode = $this->root->get($targetPath); - $this->sendHooks(['postCopy'], [$this, $targetNode]); - $this->sendHooks(['postWrite'], [$targetNode]); - return $targetNode; - } else { + + if (!($parent instanceof Folder) || !$this->isValidPath($targetPath) || !$parent->isCreatable()) { throw new NotPermittedException('No permission to copy to path ' . $targetPath); } + + $nonExisting = $this->createNonExistingNode($targetPath); + $this->sendHooks(['preCopy'], [$this, $nonExisting]); + $this->sendHooks(['preWrite'], [$nonExisting]); + + if (!$this->view->copy($this->path, $targetPath)) { + throw new NotPermittedException('Could not copy ' . $this->path . ' to ' . $targetPath); + } + + $targetNode = $this->root->get($targetPath); + $this->sendHooks(['postCopy'], [$this, $targetNode]); + $this->sendHooks(['postWrite'], [$targetNode]); + + return $targetNode; } /** @@ -461,40 +379,43 @@ public function copy($targetPath) { public function move($targetPath) { $targetPath = $this->normalizePath($targetPath); $parent = $this->root->get(dirname($targetPath)); - if ( - ($parent instanceof Folder) - && $this->isValidPath($targetPath) - && ( - $parent->isCreatable() - || ( - $parent->getInternalPath() === '' - && ($parent->getMountPoint() instanceof IMovableMount) - ) - ) - ) { - $nonExisting = $this->createNonExistingNode($targetPath); - $this->sendHooks(['preRename'], [$this, $nonExisting]); - $this->sendHooks(['preWrite'], [$nonExisting]); - if (!$this->view->rename($this->path, $targetPath)) { - throw new NotPermittedException('Could not move ' . $this->path . ' to ' . $targetPath); - } - $mountPoint = $this->getMountPoint(); - if ($mountPoint) { - // update the cached fileinfo with the new (internal) path - /** @var \OC\Files\FileInfo $oldFileInfo */ - $oldFileInfo = $this->getFileInfo(); - $this->fileInfo = new \OC\Files\FileInfo($targetPath, $oldFileInfo->getStorage(), $mountPoint->getInternalPath($targetPath), $oldFileInfo->getData(), $mountPoint, $oldFileInfo->getOwner()); - } + $isRootMovable = $parent->getInternalPath() === '' && $parent->getMountPoint() instanceof IMovableMount; + $canCreateInFolder = $parent->isCreatable() || $isRootMovable; - $targetNode = $this->root->get($targetPath); - $this->sendHooks(['postRename'], [$this, $targetNode]); - $this->sendHooks(['postWrite'], [$targetNode]); - $this->path = $targetPath; - return $targetNode; - } else { - throw new NotPermittedException('No permission to move to path ' . $targetPath); + if (!($parent instanceof Folder) || !$this->isValidPath($targetPath) || !$canCreateInFolder) { + throw new NotPermittedException('No permission to move to path ' . $targetPath); } + + $nonExisting = $this->createNonExistingNode($targetPath); + $this->sendHooks(['preRename'], [$this, $nonExisting]); + $this->sendHooks(['preWrite'], [$nonExisting]); + + if (!$this->view->rename($this->path, $targetPath)) { + throw new NotPermittedException('Could not move ' . $this->path . ' to ' . $targetPath); + } + + $mountPoint = $this->getMountPoint(); + if ($mountPoint) { + // update the cached fileinfo with the new (internal) path + /** @var \OC\Files\FileInfo $oldFileInfo */ + $oldFileInfo = $this->getFileInfo(); + $this->fileInfo = new \OC\Files\FileInfo( + $targetPath, + $oldFileInfo->getStorage(), + $mountPoint->getInternalPath($targetPath), + $oldFileInfo->getData(), + $mountPoint, + $oldFileInfo->getOwner() + ); + } + + $targetNode = $this->root->get($targetPath); + $this->sendHooks(['postRename'], [$this, $targetNode]); + $this->sendHooks(['postWrite'], [$targetNode]); + $this->path = $targetPath; + + return $targetNode; } #[\Override] From 0abc730b29d10a01164faa71ef00e68c43a50504 Mon Sep 17 00:00:00 2001 From: Josh Date: Wed, 3 Jun 2026 11:09:59 -0400 Subject: [PATCH 2/4] docs(Node): add exceptions to interface copy() Signed-off-by: Josh --- lib/public/Files/Node.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/public/Files/Node.php b/lib/public/Files/Node.php index 727cb2f4da1c0..f183f50f9b84a 100644 --- a/lib/public/Files/Node.php +++ b/lib/public/Files/Node.php @@ -48,6 +48,9 @@ public function delete(); * * @param string $targetPath the absolute target path * @return Node + * @throws InvalidPathException + * @throws NotFoundException + * @throws NotPermittedException if copy not allowed or failed * @since 6.0.0 */ public function copy($targetPath); From 5da2eba811605cd1aee17dd7b218b30328c4056c Mon Sep 17 00:00:00 2001 From: Josh Date: Wed, 3 Jun 2026 11:16:16 -0400 Subject: [PATCH 3/4] chore(Node): drop remainder of unnecessary (duplicate) docblocks Signed-off-by: Josh --- lib/private/Files/Node/Node.php | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/lib/private/Files/Node/Node.php b/lib/private/Files/Node/Node.php index 19691acbc0aaa..0c3f26f68e704 100644 --- a/lib/private/Files/Node/Node.php +++ b/lib/private/Files/Node/Node.php @@ -336,13 +336,6 @@ public function unlock($type) { $this->view->unlockFile($this->path, $type); } - /** - * @param string $targetPath - * @return INode - * @throws InvalidPathException - * @throws NotFoundException - * @throws NotPermittedException if copy not allowed or failed - */ #[\Override] public function copy($targetPath) { $targetPath = $this->normalizePath($targetPath); @@ -367,14 +360,6 @@ public function copy($targetPath) { return $targetNode; } - /** - * @param string $targetPath - * @return INode - * @throws InvalidPathException - * @throws NotFoundException - * @throws NotPermittedException if move not allowed or failed - * @throws LockedException - */ #[\Override] public function move($targetPath) { $targetPath = $this->normalizePath($targetPath); @@ -438,10 +423,6 @@ public function getParentId(): int { return $this->fileInfo->getParentId(); } - /** - * @inheritDoc - * @return array - */ #[\Override] public function getMetadata(): array { return $this->fileInfo->getMetadata(); From 48858c5d7174dfd69a4d95f94e45cc774f5f6504 Mon Sep 17 00:00:00 2001 From: Josh Date: Wed, 3 Jun 2026 11:57:32 -0400 Subject: [PATCH 4/4] refactor(Node): eliminate duplicate copy/move logic via shared helper +Clearer variable naming +Clear delineate NotPermissionException failure modes Signed-off-by: Josh --- lib/private/Files/Node/Node.php | 89 ++++++++++++++++++++------------- 1 file changed, 54 insertions(+), 35 deletions(-) diff --git a/lib/private/Files/Node/Node.php b/lib/private/Files/Node/Node.php index 0c3f26f68e704..87d25e46d8a66 100644 --- a/lib/private/Files/Node/Node.php +++ b/lib/private/Files/Node/Node.php @@ -338,16 +338,10 @@ public function unlock($type) { #[\Override] public function copy($targetPath) { - $targetPath = $this->normalizePath($targetPath); - $parent = $this->root->get(dirname($targetPath)); - - if (!($parent instanceof Folder) || !$this->isValidPath($targetPath) || !$parent->isCreatable()) { - throw new NotPermittedException('No permission to copy to path ' . $targetPath); - } + [$targetPath, $targetPlaceholder] = $this->prepareTargetPath($targetPath); - $nonExisting = $this->createNonExistingNode($targetPath); - $this->sendHooks(['preCopy'], [$this, $nonExisting]); - $this->sendHooks(['preWrite'], [$nonExisting]); + $this->sendHooks(['preCopy'], [$this, $targetPlaceholder]); + $this->sendHooks(['preWrite'], [$targetPlaceholder]); if (!$this->view->copy($this->path, $targetPath)) { throw new NotPermittedException('Could not copy ' . $this->path . ' to ' . $targetPath); @@ -362,38 +356,16 @@ public function copy($targetPath) { #[\Override] public function move($targetPath) { - $targetPath = $this->normalizePath($targetPath); - $parent = $this->root->get(dirname($targetPath)); - - $isRootMovable = $parent->getInternalPath() === '' && $parent->getMountPoint() instanceof IMovableMount; - $canCreateInFolder = $parent->isCreatable() || $isRootMovable; - - if (!($parent instanceof Folder) || !$this->isValidPath($targetPath) || !$canCreateInFolder) { - throw new NotPermittedException('No permission to move to path ' . $targetPath); - } + [$targetPath, $targetPlaceholder] = $this->prepareTargetPath($targetPath, true); - $nonExisting = $this->createNonExistingNode($targetPath); - $this->sendHooks(['preRename'], [$this, $nonExisting]); - $this->sendHooks(['preWrite'], [$nonExisting]); + $this->sendHooks(['preRename'], [$this, $targetPlaceholder]); + $this->sendHooks(['preWrite'], [$targetPlaceholder]); if (!$this->view->rename($this->path, $targetPath)) { throw new NotPermittedException('Could not move ' . $this->path . ' to ' . $targetPath); } - $mountPoint = $this->getMountPoint(); - if ($mountPoint) { - // update the cached fileinfo with the new (internal) path - /** @var \OC\Files\FileInfo $oldFileInfo */ - $oldFileInfo = $this->getFileInfo(); - $this->fileInfo = new \OC\Files\FileInfo( - $targetPath, - $oldFileInfo->getStorage(), - $mountPoint->getInternalPath($targetPath), - $oldFileInfo->getData(), - $mountPoint, - $oldFileInfo->getOwner() - ); - } + $this->updateCachedFileInfoAfterMove($targetPath); $targetNode = $this->root->get($targetPath); $this->sendHooks(['postRename'], [$this, $targetNode]); @@ -403,6 +375,53 @@ public function move($targetPath) { return $targetNode; } + private function prepareTargetPath(string $targetPath, bool $allowRootMovable = false): array { + $targetPath = $this->normalizePath($targetPath); + $targetParent = $this->root->get(dirname($targetPath)); + + if (!($targetParent instanceof Folder)) { + throw new NotPermittedException('Target parent is not a folder: ' . dirname($targetPath)); + } + + if (!$this->isValidPath($targetPath)) { + throw new NotPermittedException('Invalid target path: ' . $targetPath); + } + + $isMovableRootMountTarget = $allowRootMovable + && $targetParent->getInternalPath() === '' + && $targetParent->getMountPoint() instanceof IMovableMount; + + $canWriteToTargetParent = $targetParent->isCreatable() || $isMovableRootMountTarget; + + if (!$canWriteToTargetParent) { + throw new NotPermittedException('No permission to write to path ' . $targetPath); + } + + return [ + $targetPath, + $this->createNonExistingNode($targetPath), + ]; + } + + private function updateCachedFileInfoAfterMove(string $targetPath): void { + $mountPoint = $this->getMountPoint(); + if (!$mountPoint) { + return; + } + + // update the cached fileinfo with the new (internal) path + /** @var \OC\Files\FileInfo $oldFileInfo */ + $oldFileInfo = $this->getFileInfo(); + $this->fileInfo = new \OC\Files\FileInfo( + $targetPath, + $oldFileInfo->getStorage(), + $mountPoint->getInternalPath($targetPath), + $oldFileInfo->getData(), + $mountPoint, + $oldFileInfo->getOwner() + ); + } + #[\Override] public function getCreationTime(): int { return $this->getFileInfo()->getCreationTime();