diff --git a/apps/dav/lib/Connector/Sabre/File.php b/apps/dav/lib/Connector/Sabre/File.php index 75baff228d6..769d94957c4 100644 --- a/apps/dav/lib/Connector/Sabre/File.php +++ b/apps/dav/lib/Connector/Sabre/File.php @@ -31,6 +31,7 @@ use OCP\Files\InvalidPathException; use OCP\Files\LockNotAcquiredException; use OCP\Files\NotFoundException; use OCP\Files\NotPermittedException; +use OCP\Files\Storage\IStorage; use OCP\Files\Storage\IWriteStreamStorage; use OCP\Files\StorageNotAvailableException; use OCP\IConfig; @@ -327,51 +328,7 @@ class File extends Node implements IFile { } } - // since we skipped the view we need to scan and emit the hooks ourselves - $storage->getUpdater()->update($internalPath); - - try { - $this->changeLock(ILockingProvider::LOCK_SHARED); - } catch (LockedException $e) { - throw new FileLocked($e->getMessage(), $e->getCode(), $e); - } - - // allow sync clients to send the mtime along in a header - $mtimeHeader = $this->request->getHeader('x-oc-mtime'); - if ($mtimeHeader !== '') { - $mtime = $this->sanitizeMtime($mtimeHeader); - if ($this->fileView->touch($this->path, $mtime)) { - $this->header('X-OC-MTime: accepted'); - } - } - - $fileInfoUpdate = [ - 'upload_time' => time() - ]; - - // allow sync clients to send the creation time along in a header - $ctimeHeader = $this->request->getHeader('x-oc-ctime'); - if ($ctimeHeader) { - $ctime = $this->sanitizeMtime($ctimeHeader); - $fileInfoUpdate['creation_time'] = $ctime; - $this->header('X-OC-CTime: accepted'); - } - - $this->fileView->putFileInfo($this->path, $fileInfoUpdate); - - if ($view) { - $this->emitPostHooks($exists); - } - - $this->refreshInfo(); - - $checksumHeader = $this->request->getHeader('oc-checksum'); - if ($checksumHeader) { - $checksum = trim($checksumHeader); - $this->setChecksum($checksum); - } elseif ($this->getChecksum() !== null && $this->getChecksum() !== '') { - $this->setChecksum(''); - } + $this->finalizeUpload($storage, $internalPath, $exists, $view); } catch (StorageNotAvailableException $e) { throw new ServiceUnavailable($this->l10n->t('Failed to check file size: %1$s', [$e->getMessage()]), 0, $e); } @@ -379,6 +336,58 @@ class File extends Node implements IFile { return '"' . $this->info->getEtag() . '"'; } + private function finalizeUpload(IStorage $storage, string $internalPath, bool $exists, ?View $view): void { + // Since we skipped the view for the final publish step, finalize the file + // state explicitly here: update cache/bookkeeping, persist metadata, then + // downgrade to a shared lock before emitting post-write hooks so listeners + // can still access the file. + $storage->getUpdater()->update($internalPath); + + $fileInfoUpdate = [ + 'upload_time' => time(), + ]; + + // allow sync clients to send the mtime along in a header + $mtimeHeader = $this->request->getHeader('x-oc-mtime'); + if ($mtimeHeader !== '') { + $mtime = $this->sanitizeMtime($mtimeHeader); + if ($this->fileView->touch($this->path, $mtime)) { + $this->header('X-OC-MTime: accepted'); + } + } + + // allow sync clients to send the creation time along in a header + $ctimeHeader = $this->request->getHeader('x-oc-ctime'); + if ($ctimeHeader !== '') { + $ctime = $this->sanitizeMtime($ctimeHeader); + $fileInfoUpdate['creation_time'] = $ctime; + $this->header('X-OC-CTime: accepted'); + } + + // Persist checksum before post hooks so observers see fully finalized metadata. + $checksumHeader = $this->request->getHeader('oc-checksum'); + if ($checksumHeader) { + $fileInfoUpdate['checksum'] = trim($checksumHeader); + } elseif ($this->getChecksum() !== null && $this->getChecksum() !== '') { + $fileInfoUpdate['checksum'] = ''; + } + + $this->fileView->putFileInfo($this->path, $fileInfoUpdate); + $this->refreshInfo(); + + // Downgrade to shared lock before post hooks so legacy hook consumers can + // still access the file during post_write. + try { + $this->changeLock(ILockingProvider::LOCK_SHARED); + } catch (LockedException $e) { + throw new FileLocked($e->getMessage(), $e->getCode(), $e); + } + + if ($view) { + $this->emitPostHooks($exists); + } + } + private function getPartFileBasePath($path) { $partFileInStorage = Server::get(IConfig::class)->getSystemValue('part_file_in_storage', true); if ($partFileInStorage) { diff --git a/apps/dav/tests/unit/Connector/Sabre/FileTest.php b/apps/dav/tests/unit/Connector/Sabre/FileTest.php index 8c5b04486dd..64d0a856002 100644 --- a/apps/dav/tests/unit/Connector/Sabre/FileTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/FileTest.php @@ -230,7 +230,8 @@ class FileTest extends TestCase { null, [ 'permissions' => Constants::PERMISSION_ALL, - 'type' => FileInfo::TYPE_FOLDER, + 'type' => FileInfo::TYPE_FILE, + 'checksum' => '', ], null ); @@ -776,7 +777,13 @@ class FileTest extends TestCase { } /** - * Test whether locks are set before and after the operation + * Test that PUT keeps hook-time lock semantics compatible: + * - pre-write hooks run while the file is shared-locked + * - post-write hooks also run while the file is shared-locked + * + * Post-write hooks are expected to observe a fully finalized file state, + * but should still be able to access the file without exclusive-lock + * contention. */ public function testPutLocking(): void { $view = new View('/' . $this->user . '/files/'); @@ -788,7 +795,8 @@ class FileTest extends TestCase { null, [ 'permissions' => Constants::PERMISSION_ALL, - 'type' => FileInfo::TYPE_FOLDER, + 'type' => FileInfo::TYPE_FILE, + 'checksum' => '', ], null ); @@ -810,8 +818,8 @@ class FileTest extends TestCase { ->addMethods(['writeCallback', 'postWriteCallback']) ->getMock(); - // both pre and post hooks might need access to the file, - // so only shared lock is acceptable + // Pre-write hooks should run under a shared lock so observers can safely + // inspect the target while the write is in progress. $eventHandler->expects($this->once()) ->method('writeCallback') ->willReturnCallback( @@ -820,6 +828,10 @@ class FileTest extends TestCase { $wasLockedPre = $wasLockedPre && !$this->isFileLocked($view, $path, ILockingProvider::LOCK_EXCLUSIVE); } ); + + // Post-write hooks should also run under a shared lock. They are expected to + // see fully finalized metadata/state, but still be able to access the file + // during the callback. $eventHandler->expects($this->once()) ->method('postWriteCallback') ->willReturnCallback( @@ -850,8 +862,8 @@ class FileTest extends TestCase { // afterMethod unlocks $view->unlockFile($path, ILockingProvider::LOCK_SHARED); - $this->assertTrue($wasLockedPre, 'File was locked during pre-hooks'); - $this->assertTrue($wasLockedPost, 'File was locked during post-hooks'); + $this->assertTrue($wasLockedPre, 'File was shared-locked during pre-hooks'); + $this->assertTrue($wasLockedPost, 'File was shared-locked during post-hooks'); $this->assertFalse( $this->isFileLocked($view, $path, ILockingProvider::LOCK_SHARED), @@ -1015,7 +1027,8 @@ class FileTest extends TestCase { null, [ 'permissions' => Constants::PERMISSION_ALL, - 'type' => FileInfo::TYPE_FOLDER, + 'type' => FileInfo::TYPE_FILE, + 'checksum' => '', ], null ); diff --git a/lib/private/Files/FileInfo.php b/lib/private/Files/FileInfo.php index 213ee4ceec6..f049a146853 100644 --- a/lib/private/Files/FileInfo.php +++ b/lib/private/Files/FileInfo.php @@ -368,7 +368,7 @@ class FileInfo implements \OCP\Files\FileInfo, \ArrayAccess { * @inheritdoc */ public function getChecksum() { - return $this->data['checksum']; + return $this->data['checksum'] ?? ''; } public function getExtension(): string {