chore: Address review commentsbugfix/cleanup-s3-multipart

Signed-off-by: Julius Härtl <jus@bitgrid.net>

10 files changed, 72 insertions, 51 deletions

diff --git a/apps/dav/lib/Server.php b/apps/dav/lib/Server.php
index bec76a03236..70cbeea1aa7 100644
--- a/apps/dav/lib/Server.php
+++ b/apps/dav/lib/Server.php
@@ -194,7 +194,7 @@ class Server {
 
 		$this->server->addPlugin(new CopyEtagHeaderPlugin());
 		$this->server->addPlugin(new RequestIdHeaderPlugin(\OC::$server->get(IRequest::class)));
-		$this->server->addPlugin(new ChunkingV2Plugin(\OCP\Server::get(ICacheFactory::class)));
+		$this->server->addPlugin(new ChunkingV2Plugin(\OCP\Server::get(ICacheFactory::class), \OCP\Server::get(LoggerInterface::class)));
 		$this->server->addPlugin(new ChunkingPlugin());
 
 		// allow setup of additional plugins
diff --git a/apps/dav/lib/Upload/ChunkingV2Plugin.php b/apps/dav/lib/Upload/ChunkingV2Plugin.php
index 846b4ea42ba..72d9cd73e44 100644
--- a/apps/dav/lib/Upload/ChunkingV2Plugin.php
+++ b/apps/dav/lib/Upload/ChunkingV2Plugin.php
@@ -1,6 +1,7 @@
 <?php
 
 declare(strict_types=1);
+
 /**
  * SPDX-FileCopyrightText: 2021 Nextcloud GmbH and Nextcloud contributors
  * SPDX-License-Identifier: AGPL-3.0-or-later
@@ -27,12 +28,13 @@ use OCP\ICache;
 use OCP\ICacheFactory;
 use OCP\IConfig;
 use OCP\Lock\ILockingProvider;
+use Psr\Container\ContainerExceptionInterface;
+use Psr\Container\NotFoundExceptionInterface;
+use Psr\Log\LoggerInterface;
 use Sabre\DAV\Exception\BadRequest;
 use Sabre\DAV\Exception\InsufficientStorage;
 use Sabre\DAV\Exception\NotFound;
 use Sabre\DAV\Exception\PreconditionFailed;
-use Sabre\DAV\ICollection;
-use Sabre\DAV\INode;
 use Sabre\DAV\Server;
 use Sabre\DAV\ServerPlugin;
 use Sabre\HTTP\RequestInterface;
@@ -40,12 +42,9 @@ use Sabre\HTTP\ResponseInterface;
 use Sabre\Uri;
 
 class ChunkingV2Plugin extends ServerPlugin {
-	/** @var Server */
-	private $server;
-	/** @var UploadFolder */
-	private $uploadFolder;
-	/** @var ICache */
-	private $cache;
+	private ?Server $server = null;
+	private ?UploadFolder $uploadFolder;
+	private ICache $cache;
 
 	private ?string $uploadId = null;
 	private ?string $uploadPath = null;
@@ -59,14 +58,14 @@ class ChunkingV2Plugin extends ServerPlugin {
 
 	private const DESTINATION_HEADER = 'Destination';
 
-	public function __construct(ICacheFactory $cacheFactory) {
+	public function __construct(ICacheFactory $cacheFactory, private LoggerInterface $logger) {
 		$this->cache = $cacheFactory->createDistributed(self::CACHE_KEY);
 	}
 
 	/**
 	 * @inheritdoc
 	 */
-	public function initialize(Server $server) {
+	public function initialize(Server $server): void {
 		$server->on('afterMethod:MKCOL', [$this, 'afterMkcol']);
 		$server->on('beforeMethod:PUT', [$this, 'beforePut']);
 		$server->on('beforeMethod:DELETE', [$this, 'beforeDelete']);
@@ -75,12 +74,7 @@ class ChunkingV2Plugin extends ServerPlugin {
 		$this->server = $server;
 	}
 
-	/**
-	 * @param string $path
-	 * @param bool $createIfNotExists
-	 * @return FutureFile|UploadFile|ICollection|INode
-	 */
-	private function getUploadFile(string $path, bool $createIfNotExists = false) {
+	private function getUploadFile(string $path, bool $createIfNotExists = false): UploadFile|File {
 		try {
 			$actualFile = $this->server->tree->getNodeForPath($path);
 			// Only directly upload to the target file if it is on the same storage
@@ -109,6 +103,7 @@ class ChunkingV2Plugin extends ServerPlugin {
 			$this->prepareUpload($request->getPath());
 			$this->checkPrerequisites(false);
 		} catch (BadRequest|StorageInvalidException|NotFound $e) {
+			$this->logger->debug('Preconditions for chunking v2 not matched, fallback to v1', ['exception' => $e]);
 			return true;
 		}
 
@@ -133,12 +128,14 @@ class ChunkingV2Plugin extends ServerPlugin {
 			$this->prepareUpload(dirname($request->getPath()));
 			$this->checkPrerequisites();
 		} catch (StorageInvalidException|BadRequest|NotFound $e) {
+			$this->logger->debug('Preconditions for chunking v2 not matched, fallback to v1', ['exception' => $e]);
 			return true;
 		}
 
 		[$storage, $storagePath] = $this->getUploadStorage($this->uploadPath);
 
 		$chunkName = basename($request->getPath());
+		// S3 Multipart upload allows up to 10000 chunks at maximum and required them to be numeric
 		$partId = is_numeric($chunkName) ? (int)$chunkName : -1;
 		if (!($partId >= 1 && $partId <= 10000)) {
 			throw new BadRequest('Invalid chunk name, must be numeric between 1 and 10000');
@@ -151,7 +148,7 @@ class ChunkingV2Plugin extends ServerPlugin {
 		if ($this->uploadFolder->childExists(self::TEMP_TARGET) && $this->uploadPath) {
 			/** @var UploadFile $tempTargetFile */
 			$tempTargetFile = $this->uploadFolder->getChild(self::TEMP_TARGET);
-			[$destinationDir, $destinationName] = Uri\split($this->uploadPath);
+			[$destinationDir, ] = Uri\split($this->uploadPath);
 			/** @var Directory $destinationParent */
 			$destinationParent = $this->server->tree->getNodeForPath($destinationDir);
 			$free = $destinationParent->getNode()->getFreeSpace();
@@ -173,14 +170,15 @@ class ChunkingV2Plugin extends ServerPlugin {
 		return false;
 	}
 
-	public function beforeMove($sourcePath, $destination): bool {
+	public function beforeMove(string $sourcePath, string $destination): bool {
 		try {
 			$this->prepareUpload(dirname($sourcePath));
 			$this->checkPrerequisites();
-		} catch (StorageInvalidException|BadRequest|NotFound|PreconditionFailed $e) {
+		} catch (StorageInvalidException|BadRequest|NotFound $e) {
+			$this->logger->debug('Preconditions for chunking v2 not matched, fallback to v1', ['exception' => $e]);
 			return true;
 		}
-		[$storage, $storagePath] = $this->getUploadStorage($this->uploadPath);
+		[$storage, ] = $this->getUploadStorage($this->uploadPath);
 
 		$targetFile = $this->getUploadFile($this->uploadPath);
 
@@ -239,11 +237,12 @@ class ChunkingV2Plugin extends ServerPlugin {
 		return false;
 	}
 
-	public function beforeDelete(RequestInterface $request, ResponseInterface $response) {
+	public function beforeDelete(RequestInterface $request, ResponseInterface $response): bool {
 		try {
 			$this->prepareUpload(dirname($request->getPath()));
 			$this->checkPrerequisites();
 		} catch (StorageInvalidException|BadRequest|NotFound $e) {
+			$this->logger->debug('Preconditions for chunking v2 not matched, fallback to v1', ['exception' => $e]);
 			return true;
 		}
 
@@ -256,6 +255,8 @@ class ChunkingV2Plugin extends ServerPlugin {
 	 * @throws BadRequest
 	 * @throws PreconditionFailed
 	 * @throws StorageInvalidException
+	 * @throws ContainerExceptionInterface
+	 * @throws NotFoundExceptionInterface
 	 */
 	private function checkPrerequisites(bool $checkUploadMetadata = true): void {
 		$distributedCacheConfig = \OCP\Server::get(IConfig::class)->getSystemValue('memcache.distributed', null);
@@ -266,10 +267,12 @@ class ChunkingV2Plugin extends ServerPlugin {
 		if (!$this->uploadFolder instanceof UploadFolder || empty($this->server->httpRequest->getHeader(self::DESTINATION_HEADER))) {
 			throw new BadRequest('Skipping chunked file writing as the destination header was not passed');
 		}
-		if (!$this->uploadFolder->getStorage()->instanceOfStorage(IChunkedFileWrite::class)) {
+		/** @var ObjectStoreStorage $storage */
+		$storage = $this->uploadFolder->getStorage();
+		if (!$storage->instanceOfStorage(IChunkedFileWrite::class)) {
 			throw new StorageInvalidException('Storage does not support chunked file writing');
 		}
-		if ($this->uploadFolder->getStorage()->instanceOfStorage(ObjectStoreStorage::class) && !$this->uploadFolder->getStorage()->getObjectStore() instanceof IObjectStoreMultiPartUpload) {
+		if ($storage->instanceOfStorage(ObjectStoreStorage::class) && !$storage->getObjectStore() instanceof IObjectStoreMultiPartUpload) {
 			throw new StorageInvalidException('Storage does not support multi part uploads');
 		}
 
@@ -300,8 +303,12 @@ class ChunkingV2Plugin extends ServerPlugin {
 	/**
 	 * @throws NotFound
 	 */
-	public function prepareUpload($path): void {
-		$this->uploadFolder = $this->server->tree->getNodeForPath($path);
+	public function prepareUpload(string $path): void {
+		$uploadFolder = $this->server->tree->getNodeForPath($path);
+		if (!$uploadFolder instanceof UploadFolder) {
+			throw new \RuntimeException('Unexpected node returned, should be the upload folder');
+		}
+		$this->uploadFolder = $uploadFolder;
 		$uploadMetadata = $this->cache->get($this->uploadFolder->getName());
 		$this->uploadId = $uploadMetadata[self::UPLOAD_ID] ?? null;
 		$this->uploadPath = $uploadMetadata[self::UPLOAD_TARGET_PATH] ?? null;
@@ -329,7 +336,7 @@ class ChunkingV2Plugin extends ServerPlugin {
 		try {
 			$uploadFileAbsolutePath = $uploadFile->getFileInfo()->getPath();
 			if ($uploadFileAbsolutePath !== $targetAbsolutePath) {
-				$uploadFile = $rootFolder->get($uploadFile->getFileInfo()->getPath());
+				$uploadFile = $rootFolder->get($uploadFile->getPath());
 				if ($exists) {
 					$uploadFile->copy($targetAbsolutePath);
 				} else {
diff --git a/apps/dav/lib/Upload/FutureFile.php b/apps/dav/lib/Upload/FutureFile.php
index 5ef15bd2b56..d67414d6d73 100644
--- a/apps/dav/lib/Upload/FutureFile.php
+++ b/apps/dav/lib/Upload/FutureFile.php
@@ -49,7 +49,7 @@ class FutureFile implements \Sabre\DAV\IFile {
 		return AssemblyStream::wrap($nodes);
 	}
 
-	public function getPath() {
+	public function getPath(): string {
 		return $this->root->getFileInfo()->getInternalPath() . '/.file';
 	}
 
diff --git a/apps/dav/lib/Upload/PartFile.php b/apps/dav/lib/Upload/PartFile.php
index a711fe083b9..d9190fc1789 100644
--- a/apps/dav/lib/Upload/PartFile.php
+++ b/apps/dav/lib/Upload/PartFile.php
@@ -1,10 +1,13 @@
 <?php
 
+declare(strict_types=1);
+
 /**
  * SPDX-FileCopyrightText: 2021-2024 Nextcloud GmbH and Nextcloud contributors
  * SPDX-FileCopyrightText: 2016 ownCloud, Inc.
  * SPDX-License-Identifier: AGPL-3.0-only
  */
+
 namespace OCA\DAV\Upload;
 
 use OCA\DAV\Connector\Sabre\Directory;
@@ -16,10 +19,8 @@ use Sabre\DAV\IFile;
  * but handled directly by external storage services like S3 with Multipart Upload
  */
 class PartFile implements IFile {
-	/** @var Directory */
-	private $root;
-	/** @var array */
-	private $partInfo;
+	private Directory $root;
+	private array $partInfo;
 
 	public function __construct(Directory $root, array $partInfo) {
 		$this->root = $root;
@@ -40,7 +41,7 @@ class PartFile implements IFile {
 		throw new Forbidden('Permission denied to get this file');
 	}
 
-	public function getPath() {
+	public function getPath(): string {
 		return $this->root->getFileInfo()->getInternalPath() . '/' . $this->partInfo['PartNumber'];
 	}
 
diff --git a/apps/dav/lib/Upload/UploadFile.php b/apps/dav/lib/Upload/UploadFile.php
index ae64760e0ce..26d8c4e321c 100644
--- a/apps/dav/lib/Upload/UploadFile.php
+++ b/apps/dav/lib/Upload/UploadFile.php
@@ -27,7 +27,7 @@ class UploadFile implements IFile {
 		return $this->file->get();
 	}
 
-	public function getId() {
+	public function getId(): int {
 		return $this->file->getId();
 	}
 
@@ -71,7 +71,7 @@ class UploadFile implements IFile {
 		return $this->file;
 	}
 
-	public function getNode() {
+	public function getNode(): \OCP\Files\File {
 		return $this->file->getNode();
 	}
 }
diff --git a/apps/dav/lib/Upload/UploadFolder.php b/apps/dav/lib/Upload/UploadFolder.php
index 3f84f7a8597..e3eab5ebbb2 100644
--- a/apps/dav/lib/Upload/UploadFolder.php
+++ b/apps/dav/lib/Upload/UploadFolder.php
@@ -11,16 +11,14 @@ use OC\Files\ObjectStore\ObjectStoreStorage;
 use OCA\DAV\Connector\Sabre\Directory;
 use OCP\Files\ObjectStore\IObjectStoreMultiPartUpload;
 use OCP\Files\Storage\IStorage;
+use OCP\ICacheFactory;
 use Sabre\DAV\Exception\Forbidden;
 use Sabre\DAV\ICollection;
 
 class UploadFolder implements ICollection {
-	/** @var Directory */
-	private $node;
-	/** @var CleanupService */
-	private $cleanupService;
-	/** @var IStorage */
-	private $storage;
+	private Directory $node;
+	private CleanupService $cleanupService;
+	private IStorage $storage;
 
 	public function __construct(Directory $node, CleanupService $cleanupService, IStorage $storage) {
 		$this->node = $node;
@@ -66,7 +64,7 @@ class UploadFolder implements ICollection {
 			/** @var ObjectStoreStorage $storage */
 			$objectStore = $this->storage->getObjectStore();
 			if ($objectStore instanceof IObjectStoreMultiPartUpload) {
-				$cache = \OC::$server->getMemCacheFactory()->createDistributed(ChunkingV2Plugin::CACHE_KEY);
+				$cache = \OCP\Server::get(ICacheFactory::class)->createDistributed(ChunkingV2Plugin::CACHE_KEY);
 				$uploadSession = $cache->get($this->getName());
 				if ($uploadSession) {
 					$uploadId = $uploadSession[ChunkingV2Plugin::UPLOAD_ID];
@@ -108,7 +106,7 @@ class UploadFolder implements ICollection {
 		return $this->node->getLastModified();
 	}
 
-	public function getStorage() {
+	public function getStorage(): IStorage {
 		return $this->storage;
 	}
 }
diff --git a/apps/dav/lib/Upload/UploadHome.php b/apps/dav/lib/Upload/UploadHome.php
index 3e7e3c6c986..8dd59604a8b 100644
--- a/apps/dav/lib/Upload/UploadHome.php
+++ b/apps/dav/lib/Upload/UploadHome.php
@@ -10,6 +10,7 @@ namespace OCA\DAV\Upload;
 use OC\Files\Filesystem;
 use OC\Files\View;
 use OCA\DAV\Connector\Sabre\Directory;
+use OCP\Files\Storage\IStorage;
 use Sabre\DAV\Exception\Forbidden;
 use Sabre\DAV\ICollection;
 
@@ -69,13 +70,13 @@ class UploadHome implements ICollection {
 	/**
 	 * @return Directory
 	 */
-	private function impl() {
+	private function impl(): Directory {
 		$view = $this->getView();
 		$rootInfo = $view->getFileInfo('');
 		return new Directory($view, $rootInfo);
 	}
 
-	private function getView() {
+	private function getView(): View {
 		$rootView = new View();
 		$user = \OC::$server->getUserSession()->getUser();
 		Filesystem::initMountPoints($user->getUID());
@@ -85,7 +86,7 @@ class UploadHome implements ICollection {
 		return new View('/' . $user->getUID() . '/uploads');
 	}
 
-	private function getStorage() {
+	private function getStorage(): IStorage {
 		$view = $this->getView();
 		$storage = $view->getFileInfo('')->getStorage();
 		return $storage;
diff --git a/lib/private/Files/ObjectStore/ObjectStoreStorage.php b/lib/private/Files/ObjectStore/ObjectStoreStorage.php
index 389f744eab4..3494a7db806 100644
--- a/lib/private/Files/ObjectStore/ObjectStoreStorage.php
+++ b/lib/private/Files/ObjectStore/ObjectStoreStorage.php
@@ -661,10 +661,6 @@ class ObjectStoreStorage extends \OC\Files\Storage\Common implements IChunkedFil
 		return $this->objectStore->initiateMultipartUpload($urn);
 	}
 
-	/**
-	 *
-	 * @throws GenericFileException
-	 */
 	public function putChunkedWritePart(
 		string $targetPath,
 		string $writeToken,
@@ -675,6 +671,11 @@ class ObjectStoreStorage extends \OC\Files\Storage\Common implements IChunkedFil
 		if (!$this->objectStore instanceof IObjectStoreMultiPartUpload) {
 			throw new GenericFileException('Object store does not support multipart upload');
 		}
+
+		if (!is_numeric($chunkId)) {
+			throw new GenericFileException('Chunk ID must be numeric for S3 multipart upload');
+		}
+
 		$cacheEntry = $this->getCache()->get($targetPath);
 		$urn = $this->getURN($cacheEntry->getId());
 
diff --git a/lib/public/Files/ObjectStore/IObjectStoreMultiPartUpload.php b/lib/public/Files/ObjectStore/IObjectStoreMultiPartUpload.php
index 7989e27dfc8..c3a04da2201 100644
--- a/lib/public/Files/ObjectStore/IObjectStoreMultiPartUpload.php
+++ b/lib/public/Files/ObjectStore/IObjectStoreMultiPartUpload.php
@@ -5,6 +5,7 @@ declare(strict_types=1);
  * SPDX-FileCopyrightText: 2021 Nextcloud GmbH and Nextcloud contributors
  * SPDX-License-Identifier: AGPL-3.0-or-later
  */
+
 namespace OCP\Files\ObjectStore;
 
 use Aws\Result;
@@ -35,6 +36,15 @@ interface IObjectStoreMultiPartUpload {
 
 	/**
 	 * @since 26.0.0
+	 * @return array of the parts in ListParts response
+	 *
+	 * Sample data:
+	 * [[
+	 * 		'PartNumber' => 1,
+	 * 		'LastModified' => '2010-11-10T20:48:34.000Z',
+	 * 		'ETag' => '"7778aef83f66abc1fa1e8477f296d394"',
+	 * 		'Size' => 10485760,
+	 * ]]
 	 */
 	public function getMultipartUploads(string $urn, string $uploadId): array;
 }
diff --git a/lib/public/Files/Storage/IChunkedFileWrite.php b/lib/public/Files/Storage/IChunkedFileWrite.php
index 1095ee7cbfc..fd205ea2764 100644
--- a/lib/public/Files/Storage/IChunkedFileWrite.php
+++ b/lib/public/Files/Storage/IChunkedFileWrite.php
@@ -1,10 +1,12 @@
 <?php
+
+declare(strict_types=1);
+
 /**
  * SPDX-FileCopyrightText: 2021 Nextcloud GmbH and Nextcloud contributors
  * SPDX-License-Identifier: AGPL-3.0-or-later
  */
 
-declare(strict_types=1);
 
 
 namespace OCP\Files\Storage;
@@ -29,6 +31,7 @@ interface IChunkedFileWrite extends IStorage {
 	 * @param string $chunkId
 	 * @param resource $data
 	 * @param int|null $size
+	 * @return array|null
 	 * @throws GenericFileException
 	 * @since 26.0.0
 	 */