chore: Address review comments

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

apps/dav/lib/Upload/ChunkingV2Plugin.php

@@ -2,7 +2,7 @@
 
 declare(strict_types=1);
 /*
- * @copyright Copyright (c) 2021 Julius Härtl <jus@bitgrid.net>
+ * @copyright Copyright (c) 2023 Julius Härtl <jus@bitgrid.net>
  *
  * @author Julius Härtl <jus@bitgrid.net>
  *
@@ -45,21 +45,16 @@
 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;
 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;
@@ -80,7 +75,7 @@ public function __construct(ICacheFactory $cacheFactory) {
 	/**
 	 * @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']);
@@ -89,12 +84,7 @@ public function initialize(Server $server) {
 		$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
@@ -153,6 +143,7 @@ public function beforePut(RequestInterface $request, ResponseInterface $response
 		[$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');
@@ -165,7 +156,7 @@ public function beforePut(RequestInterface $request, ResponseInterface $response
 		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 = $storage->free_space($destinationParent->getInternalPath());
@@ -187,14 +178,14 @@ public function beforePut(RequestInterface $request, ResponseInterface $response
 		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) {
 			return true;
 		}
-		[$storage, $storagePath] = $this->getUploadStorage($this->uploadPath);
+		[$storage, ] = $this->getUploadStorage($this->uploadPath);
 
 		$targetFile = $this->getUploadFile($this->uploadPath);
 
@@ -253,7 +244,7 @@ public function beforeMove($sourcePath, $destination): bool {
 		return false;
 	}
 
-	public function beforeDelete(RequestInterface $request, ResponseInterface $response) {
+	public function beforeDelete(RequestInterface $request, ResponseInterface $response): bool {
 		try {
 			$this->prepareUpload($request->getPath());
 			if (!$this->uploadFolder instanceof UploadFolder) {
@@ -308,8 +299,12 @@ protected function sanitizeMtime(string $mtimeFromRequest): int {
 	/**
 	 * @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;

apps/dav/lib/Upload/FutureFile.php

@@ -65,7 +65,7 @@ public function get() {
 		return AssemblyStream::wrap($nodes);
 	}
 
-	public function getPath() {
+	public function getPath(): string {
 		return $this->root->getFileInfo()->getInternalPath() . '/.file';
 	}
 

apps/dav/lib/Upload/PartFile.php

@@ -1,26 +1,28 @@
 <?php
-/**
- * @copyright Copyright (c) 2016, ownCloud, Inc.
+
+declare(strict_types=1);
+/*
+ * @copyright Copyright (c) 2023 Julius Härtl <jus@bitgrid.net>
  *
- * @author Christoph Wurst <christoph@winzerhof-wurst.at>
- * @author Lukas Reschke <lukas@statuscode.ch>
- * @author Thomas Müller <thomas.mueller@tmit.eu>
+ * @author Julius Härtl <jus@bitgrid.net>
  *
- * @license AGPL-3.0
+ * @license GNU AGPL version 3 or any later version
  *
- * This code is free software: you can redistribute it and/or modify
- * it under the terms of the GNU Affero General Public License, version 3,
- * as published by the Free Software Foundation.
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
  *
  * This program is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
  * GNU Affero General Public License for more details.
  *
- * You should have received a copy of the GNU Affero General Public License, version 3,
- * along with this program. If not, see <http://www.gnu.org/licenses/>
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
  *
  */
+
 namespace OCA\DAV\Upload;
 
 use OCA\DAV\Connector\Sabre\Directory;
@@ -32,10 +34,8 @@
  * 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;
@@ -56,7 +56,7 @@ public function get() {
 		throw new Forbidden('Permission denied to get this file');
 	}
 
-	public function getPath() {
+	public function getPath(): string {
 		return $this->root->getFileInfo()->getInternalPath() . '/' . $this->partInfo['PartNumber'];
 	}
 

apps/dav/lib/Upload/UploadFile.php

@@ -44,7 +44,7 @@ public function get() {
 		return $this->file->get();
 	}
 
-	public function getId() {
+	public function getId(): int {
 		return $this->file->getId();
 	}
 
@@ -88,7 +88,7 @@ public function getFile(): File {
 		return $this->file;
 	}
 
-	public function getNode() {
+	public function getNode(): \OCP\Files\File {
 		return $this->file->getNode();
 	}
 }

apps/dav/lib/Upload/UploadFolder.php

@@ -28,16 +28,14 @@
 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;
@@ -75,7 +73,7 @@ public function getChildren() {
 			/** @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];
@@ -117,7 +115,7 @@ public function getLastModified() {
 		return $this->node->getLastModified();
 	}
 
-	public function getStorage() {
+	public function getStorage(): IStorage {
 		return $this->storage;
 	}
 }

apps/dav/lib/Upload/UploadHome.php

@@ -28,6 +28,7 @@
 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;
 
@@ -87,13 +88,13 @@ public function getLastModified() {
 	/**
 	 * @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());
@@ -103,7 +104,7 @@ private function getView() {
 		return new View('/' . $user->getUID() . '/uploads');
 	}
 
-	private function getStorage() {
+	private function getStorage(): IStorage {
 		$view = $this->getView();
 		$storage = $view->getFileInfo('')->getStorage();
 		return $storage;

lib/private/Files/ObjectStore/ObjectStoreStorage.php

@@ -642,23 +642,22 @@ public function startChunkedWrite(string $targetPath): string {
 	}
 
 	/**
-	 *
+	 * @return array
 	 * @throws GenericFileException
 	 */
-	public function putChunkedWritePart(string $targetPath, string $writeToken, string $chunkId, $data, $size = null): ?array {
+	public function putChunkedWritePart(string $targetPath, string $writeToken, string $chunkId, $data, $size = null): void {
 		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());
 
-		$result = $this->objectStore->uploadMultipartPart($urn, $writeToken, (int)$chunkId, $data, $size);
-
-		$parts[$chunkId] = [
-			'PartNumber' => $chunkId,
-			'ETag' => trim($result->get('ETag'), '"')
-		];
-		return $parts[$chunkId];
+		$this->objectStore->uploadMultipartPart($urn, $writeToken, (int)$chunkId, $data, $size);
 	}
 
 	public function completeChunkedWrite(string $targetPath, string $writeToken): int {

lib/public/Files/ObjectStore/IObjectStoreMultiPartUpload.php

@@ -1,6 +1,9 @@
 <?php
+
+declare(strict_types=1);
+
 /*
- * @copyright Copyright (c) 2021 Julius Härtl <jus@bitgrid.net>
+ * @copyright Copyright (c) 2023 Julius Härtl <jus@bitgrid.net>
  *
  * @author Julius Härtl <jus@bitgrid.net>
  *
@@ -21,7 +24,6 @@
  *
  */
 
-declare(strict_types=1);
 
 
 namespace OCP\Files\ObjectStore;
@@ -54,6 +56,15 @@ public function abortMultipartUpload(string $urn, string $uploadId): void;
 
 	/**
 	 * @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;
 }

lib/public/Files/Storage/IChunkedFileWrite.php

@@ -1,6 +1,9 @@
 <?php
+
+declare(strict_types=1);
+
 /*
- * @copyright Copyright (c) 2021 Julius Härtl <jus@bitgrid.net>
+ * @copyright Copyright (c) 2023 Julius Härtl <jus@bitgrid.net>
  *
  * @author Julius Härtl <jus@bitgrid.net>
  *
@@ -21,7 +24,6 @@
  *
  */
 
-declare(strict_types=1);
 
 
 namespace OCP\Files\Storage;
@@ -49,7 +51,7 @@ public function startChunkedWrite(string $targetPath): string;
 	 * @throws GenericFileException
 	 * @since 26.0.0
 	 */
-	public function putChunkedWritePart(string $targetPath, string $writeToken, string $chunkId, $data, int $size = null): ?array;
+	public function putChunkedWritePart(string $targetPath, string $writeToken, string $chunkId, $data, int $size = null): void;
 
 	/**
 	 * @param string $targetPath