reuse l10n and request in dav folder listing

instead of having to query those once for every node

Signed-off-by: Robin Appelman <robin@icewind.nl>

apps/dav/lib/Connector/Sabre/Directory.php

@@ -35,16 +35,19 @@ namespace OCA\DAV\Connector\Sabre;
 use OC\Files\Mount\MoveableMount;
 use OC\Files\View;
 use OC\Metadata\FileMetadata;
+use OCA\DAV\AppInfo\Application;
 use OCA\DAV\Connector\Sabre\Exception\FileLocked;
 use OCA\DAV\Connector\Sabre\Exception\Forbidden;
 use OCA\DAV\Connector\Sabre\Exception\InvalidPath;
-use OCA\DAV\Upload\FutureFile;
 use OCP\Files\FileInfo;
 use OCP\Files\Folder;
 use OCP\Files\ForbiddenException;
 use OCP\Files\InvalidPathException;
 use OCP\Files\NotPermittedException;
 use OCP\Files\StorageNotAvailableException;
+use OCP\IL10N;
+use OCP\IRequest;
+use OCP\L10N\IFactory;
 use OCP\Lock\ILockingProvider;
 use OCP\Lock\LockedException;
 use Psr\Log\LoggerInterface;
@@ -203,7 +206,7 @@ class Directory extends \OCA\DAV\Connector\Sabre\Node implements \Sabre\DAV\ICol
 	 * @throws \Sabre\DAV\Exception\NotFound
 	 * @throws \Sabre\DAV\Exception\ServiceUnavailable
 	 */
-	public function getChild($name, $info = null) {
+	public function getChild($name, $info = null, IRequest $request = null, IL10N $l10n = null) {
 		if (!$this->info->isReadable()) {
 			// avoid detecting files through this way
 			throw new NotFound();
@@ -230,7 +233,7 @@ class Directory extends \OCA\DAV\Connector\Sabre\Node implements \Sabre\DAV\ICol
 		if ($info->getMimeType() === FileInfo::MIMETYPE_FOLDER) {
 			$node = new \OCA\DAV\Connector\Sabre\Directory($this->fileView, $info, $this->tree, $this->shareManager);
 		} else {
-			$node = new \OCA\DAV\Connector\Sabre\File($this->fileView, $info, $this->shareManager);
+			$node = new \OCA\DAV\Connector\Sabre\File($this->fileView, $info, $this->shareManager, $request, $l10n);
 		}
 		if ($this->tree) {
 			$this->tree->cacheNode($node);
@@ -265,8 +268,11 @@ class Directory extends \OCA\DAV\Connector\Sabre\Node implements \Sabre\DAV\ICol
 		}
 
 		$nodes = [];
+		$request = \OC::$server->get(IRequest::class);
+		$l10nFactory = \OC::$server->get(IFactory::class);
+		$l10n = $l10nFactory->get(Application::APP_ID);
 		foreach ($folderContent as $info) {
-			$node = $this->getChild($info->getName(), $info);
+			$node = $this->getChild($info->getName(), $info, $request, $l10n);
 			$nodes[] = $node;
 		}
 		$this->dirContent = $nodes;

apps/dav/lib/Connector/Sabre/File.php

@@ -63,6 +63,7 @@ use OCP\Files\NotPermittedException;
 use OCP\Files\Storage;
 use OCP\Files\StorageNotAvailableException;
 use OCP\IL10N;
+use OCP\IRequest;
 use OCP\L10N\IFactory as IL10NFactory;
 use OCP\Lock\ILockingProvider;
 use OCP\Lock\LockedException;
@@ -77,8 +78,7 @@ use Sabre\DAV\Exception\ServiceUnavailable;
 use Sabre\DAV\IFile;
 
 class File extends Node implements IFile {
-	protected $request;
-
+	protected IRequest $request;
 	protected IL10N $l10n;
 
 	/** @var array<string, FileMetadata> */
@@ -89,21 +89,26 @@ class File extends Node implements IFile {
 	 *
 	 * @param \OC\Files\View $view
 	 * @param \OCP\Files\FileInfo $info
-	 * @param \OCP\Share\IManager $shareManager
-	 * @param \OC\AppFramework\Http\Request $request
+	 * @param ?\OCP\Share\IManager $shareManager
+	 * @param ?IRequest $request
+	 * @param ?IL10N $l10n
 	 */
-	public function __construct(View $view, FileInfo $info, IManager $shareManager = null, Request $request = null) {
+	public function __construct(View $view, FileInfo $info, IManager $shareManager = null, IRequest $request = null, IL10N $l10n = null) {
 		parent::__construct($view, $info, $shareManager);
 
-		// Querying IL10N directly results in a dependency loop
-		/** @var IL10NFactory $l10nFactory */
-		$l10nFactory = \OC::$server->get(IL10NFactory::class);
-		$this->l10n = $l10nFactory->get(Application::APP_ID);
+		if ($l10n) {
+			$this->l10n = $l10n;
+		} else {
+			// Querying IL10N directly results in a dependency loop
+			/** @var IL10NFactory $l10nFactory */
+			$l10nFactory = \OC::$server->get(IL10NFactory::class);
+			$this->l10n = $l10nFactory->get(Application::APP_ID);
+		}
 
 		if (isset($request)) {
 			$this->request = $request;
 		} else {
-			$this->request = \OC::$server->getRequest();
+			$this->request = \OC::$server->get(IRequest::class);
 		}
 	}
 
@@ -149,7 +154,8 @@ class File extends Node implements IFile {
 		$this->verifyPath();
 
 		// chunked handling
-		if (isset($_SERVER['HTTP_OC_CHUNKED'])) {
+		$chunkedHeader = $this->request->getHeader('oc-chunked');
+		if ($chunkedHeader) {
 			try {
 				return $this->createFileChunked($data);
 			} catch (\Exception $e) {
@@ -272,8 +278,9 @@ class File extends Node implements IFile {
 
 			if ($result === false) {
 				$expected = -1;
-				if (isset($_SERVER['CONTENT_LENGTH'])) {
-					$expected = (int)$_SERVER['CONTENT_LENGTH'];
+				$lengthHeader = $this->request->getHeader('content-length');
+				if ($lengthHeader) {
+					$expected = (int)$lengthHeader;
 				}
 				if ($expected !== 0) {
 					throw new Exception(
@@ -291,8 +298,9 @@ class File extends Node implements IFile {
 			// if content length is sent by client:
 			// double check if the file was fully received
 			// compare expected and actual size
-			if (isset($_SERVER['CONTENT_LENGTH']) && $_SERVER['REQUEST_METHOD'] === 'PUT') {
-				$expected = (int)$_SERVER['CONTENT_LENGTH'];
+			$lengthHeader = $this->request->getHeader('content-length');
+			if ($lengthHeader && $this->request->getMethod() === 'PUT') {
+				$expected = (int)$lengthHeader;
 				if ($count !== $expected) {
 					throw new BadRequest(
 						$this->l10n->t(
@@ -374,8 +382,9 @@ class File extends Node implements IFile {
 			}
 
 			// allow sync clients to send the mtime along in a header
-			if (isset($this->request->server['HTTP_X_OC_MTIME'])) {
-				$mtime = $this->sanitizeMtime($this->request->server['HTTP_X_OC_MTIME']);
+			$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');
 				}
@@ -386,8 +395,9 @@ class File extends Node implements IFile {
 			];
 
 			// allow sync clients to send the creation time along in a header
-			if (isset($this->request->server['HTTP_X_OC_CTIME'])) {
-				$ctime = $this->sanitizeMtime($this->request->server['HTTP_X_OC_CTIME']);
+			$ctimeHeader = $this->request->getHeader('x-oc-ctime');
+			if ($ctimeHeader) {
+				$ctime = $this->sanitizeMtime($ctimeHeader);
 				$fileInfoUpdate['creation_time'] = $ctime;
 				$this->header('X-OC-CTime: accepted');
 			}
@@ -400,8 +410,9 @@ class File extends Node implements IFile {
 
 			$this->refreshInfo();
 
-			if (isset($this->request->server['HTTP_OC_CHECKSUM'])) {
-				$checksum = trim($this->request->server['HTTP_OC_CHECKSUM']);
+			$checksumHeader = $this->request->getHeader('oc-checksum');
+			if ($checksumHeader) {
+				$checksum = trim($checksumHeader);
 				$this->setChecksum($checksum);
 			} elseif ($this->getChecksum() !== null && $this->getChecksum() !== '') {
 				$this->setChecksum('');
@@ -557,7 +568,7 @@ class File extends Node implements IFile {
 		$mimeType = $this->info->getMimetype();
 
 		// PROPFIND needs to return the correct mime type, for consistency with the web UI
-		if (isset($_SERVER['REQUEST_METHOD']) && $_SERVER['REQUEST_METHOD'] === 'PROPFIND') {
+		if ($this->request->getMethod() === 'PROPFIND') {
 			return $mimeType;
 		}
 		return \OC::$server->getMimeTypeDetector()->getSecureMimeType($mimeType);
@@ -599,9 +610,10 @@ class File extends Node implements IFile {
 		$bytesWritten = $chunk_handler->store($info['index'], $data);
 
 		//detect aborted upload
-		if (isset($_SERVER['REQUEST_METHOD']) && $_SERVER['REQUEST_METHOD'] === 'PUT') {
-			if (isset($_SERVER['CONTENT_LENGTH'])) {
-				$expected = (int)$_SERVER['CONTENT_LENGTH'];
+		if ($this->request->getMethod() === 'PUT') {
+			$lengthHeader = $this->request->getHeader('content-length');
+			if ($lengthHeader) {
+				$expected = (int)$lengthHeader;
 				if ($bytesWritten !== $expected) {
 					$chunk_handler->remove($info['index']);
 					throw new BadRequest(
@@ -667,8 +679,9 @@ class File extends Node implements IFile {
 				}
 
 				// allow sync clients to send the mtime along in a header
-				if (isset($this->request->server['HTTP_X_OC_MTIME'])) {
-					$mtime = $this->sanitizeMtime($this->request->server['HTTP_X_OC_MTIME']);
+				$mtimeHeader = $this->request->getHeader('x-oc-mtime');
+				if ($mtimeHeader !== '') {
+					$mtime = $this->sanitizeMtime($mtimeHeader);
 					if ($targetStorage->touch($targetInternalPath, $mtime)) {
 						$this->header('X-OC-MTime: accepted');
 					}
@@ -684,8 +697,9 @@ class File extends Node implements IFile {
 				// FIXME: should call refreshInfo but can't because $this->path is not the of the final file
 				$info = $this->fileView->getFileInfo($targetPath);
 
-				if (isset($this->request->server['HTTP_OC_CHECKSUM'])) {
-					$checksum = trim($this->request->server['HTTP_OC_CHECKSUM']);
+				$checksumHeader = $this->request->getHeader('oc-checksum');
+				if ($checksumHeader) {
+					$checksum = trim($checksumHeader);
 					$this->fileView->putFileInfo($targetPath, ['checksum' => $checksum]);
 				} elseif ($info->getChecksum() !== null && $info->getChecksum() !== '') {
 					$this->fileView->putFileInfo($this->path, ['checksum' => '']);

apps/dav/tests/unit/Connector/Sabre/FileTest.php

@@ -73,9 +73,6 @@ class FileTest extends TestCase {
 
 	protected function setUp(): void {
 		parent::setUp();
-		unset($_SERVER['HTTP_OC_CHUNKED']);
-		unset($_SERVER['CONTENT_LENGTH']);
-		unset($_SERVER['REQUEST_METHOD']);
 
 		\OC_Hook::clear();
 
@@ -91,7 +88,6 @@ class FileTest extends TestCase {
 	protected function tearDown(): void {
 		$userManager = \OC::$server->getUserManager();
 		$userManager->get($this->user)->delete();
-		unset($_SERVER['HTTP_OC_CHUNKED']);
 
 		parent::tearDown();
 	}
@@ -271,13 +267,17 @@ class FileTest extends TestCase {
 			->method('getRelativePath')
 			->willReturnArgument(0);
 
-		$_SERVER['HTTP_OC_CHUNKED'] = true;
+		$request = new Request([
+			'server' => [
+				'HTTP_OC_CHUNKED' => 'true'
+			]
+		], $this->requestId, $this->config, null);
 
 		$info = new \OC\Files\FileInfo('/test.txt-chunking-12345-2-0', $this->getMockStorage(), null, [
 			'permissions' => \OCP\Constants::PERMISSION_ALL,
 			'type' => FileInfo::TYPE_FOLDER,
 		], null);
-		$file = new \OCA\DAV\Connector\Sabre\File($view, $info);
+		$file = new \OCA\DAV\Connector\Sabre\File($view, $info, null, $request);
 
 		// put first chunk
 		$file->acquireLock(ILockingProvider::LOCK_SHARED);
@@ -288,7 +288,7 @@ class FileTest extends TestCase {
 			'permissions' => \OCP\Constants::PERMISSION_ALL,
 			'type' => FileInfo::TYPE_FOLDER,
 		], null);
-		$file = new \OCA\DAV\Connector\Sabre\File($view, $info);
+		$file = new \OCA\DAV\Connector\Sabre\File($view, $info, null, $request);
 
 		// action
 		$caughtException = null;
@@ -421,7 +421,7 @@ class FileTest extends TestCase {
 	public function testPutSingleFileLegalMtime($requestMtime, $resultMtime): void {
 		$request = new Request([
 			'server' => [
-				'HTTP_X_OC_MTIME' => $requestMtime,
+				'HTTP_X_OC_MTIME' => (string)$requestMtime,
 			]
 		], $this->requestId, $this->config, null);
 		$file = 'foo.txt';
@@ -444,11 +444,11 @@ class FileTest extends TestCase {
 	public function testChunkedPutLegalMtime($requestMtime, $resultMtime): void {
 		$request = new Request([
 			'server' => [
-				'HTTP_X_OC_MTIME' => $requestMtime,
+				'HTTP_X_OC_MTIME' => (string)$requestMtime,
+				'HTTP_OC_CHUNKED' => 'true'
 			]
 		], $this->requestId, $this->config, null);
 
-		$_SERVER['HTTP_OC_CHUNKED'] = true;
 		$file = 'foo.txt';
 
 		if ($resultMtime === null) {
@@ -467,9 +467,13 @@ class FileTest extends TestCase {
 	 * Test putting a file using chunking
 	 */
 	public function testChunkedPut(): void {
-		$_SERVER['HTTP_OC_CHUNKED'] = true;
-		$this->assertNull($this->doPut('/test.txt-chunking-12345-2-0'));
-		$this->assertNotEmpty($this->doPut('/test.txt-chunking-12345-2-1'));
+		$request = new Request([
+			'server' => [
+				'HTTP_OC_CHUNKED' => 'true'
+			]
+		], $this->requestId, $this->config, null);
+		$this->assertNull($this->doPut('/test.txt-chunking-12345-2-0', null, $request));
+		$this->assertNotEmpty($this->doPut('/test.txt-chunking-12345-2-1', null, $request));
 	}
 
 	/**
@@ -580,9 +584,13 @@ class FileTest extends TestCase {
 	public function testPutChunkedFileTriggersHooks(): void {
 		HookHelper::setUpHooks();
 
-		$_SERVER['HTTP_OC_CHUNKED'] = true;
-		$this->assertNull($this->doPut('/foo.txt-chunking-12345-2-0'));
-		$this->assertNotEmpty($this->doPut('/foo.txt-chunking-12345-2-1'));
+		$request = new Request([
+			'server' => [
+				'HTTP_OC_CHUNKED' => 'true'
+			]
+		], $this->requestId, $this->config, null);
+		$this->assertNull($this->doPut('/foo.txt-chunking-12345-2-0', null, $request));
+		$this->assertNotEmpty($this->doPut('/foo.txt-chunking-12345-2-1', null, $request));
 
 		$this->assertCount(4, HookHelper::$hookCalls);
 		$this->assertHookCall(
@@ -616,9 +624,13 @@ class FileTest extends TestCase {
 
 		HookHelper::setUpHooks();
 
-		$_SERVER['HTTP_OC_CHUNKED'] = true;
-		$this->assertNull($this->doPut('/foo.txt-chunking-12345-2-0'));
-		$this->assertNotEmpty($this->doPut('/foo.txt-chunking-12345-2-1'));
+		$request = new Request([
+			'server' => [
+				'HTTP_OC_CHUNKED' => 'true'
+			]
+		], $this->requestId, $this->config, null);
+		$this->assertNull($this->doPut('/foo.txt-chunking-12345-2-0', null, $request));
+		$this->assertNotEmpty($this->doPut('/foo.txt-chunking-12345-2-1', null, $request));
 
 		$this->assertCount(4, HookHelper::$hookCalls);
 		$this->assertHookCall(
@@ -693,15 +705,19 @@ class FileTest extends TestCase {
 			->method('filesize')
 			->willReturn(123456);
 
-		$_SERVER['CONTENT_LENGTH'] = 123456;
-		$_SERVER['REQUEST_METHOD'] = 'PUT';
+		$request = new Request([
+			'server' => [
+				'CONTENT_LENGTH' => '123456',
+			],
+			'method' => 'PUT',
+		], $this->requestId, $this->config, null);
 
 		$info = new \OC\Files\FileInfo('/test.txt', $this->getMockStorage(), null, [
 			'permissions' => \OCP\Constants::PERMISSION_ALL,
 			'type' => FileInfo::TYPE_FOLDER,
 		], null);
 
-		$file = new \OCA\DAV\Connector\Sabre\File($view, $info);
+		$file = new \OCA\DAV\Connector\Sabre\File($view, $info, null, $request);
 
 		// action
 		$thrown = false;
@@ -764,13 +780,17 @@ class FileTest extends TestCase {
 		// simulate situation where the target file is locked
 		$view->lockFile('/test.txt', ILockingProvider::LOCK_EXCLUSIVE);
 
-		$_SERVER['HTTP_OC_CHUNKED'] = true;
+		$request = new Request([
+			'server' => [
+				'HTTP_OC_CHUNKED' => 'true'
+			]
+		], $this->requestId, $this->config, null);
 
 		$info = new \OC\Files\FileInfo('/' . $this->user . '/files/test.txt-chunking-12345-2-0', $this->getMockStorage(), null, [
 			'permissions' => \OCP\Constants::PERMISSION_ALL,
 			'type' => FileInfo::TYPE_FOLDER,
 		], null);
-		$file = new \OCA\DAV\Connector\Sabre\File($view, $info);
+		$file = new \OCA\DAV\Connector\Sabre\File($view, $info, null, $request);
 		$file->acquireLock(ILockingProvider::LOCK_SHARED);
 		$this->assertNull($file->put('test data one'));
 		$file->releaseLock(ILockingProvider::LOCK_SHARED);
@@ -779,7 +799,7 @@ class FileTest extends TestCase {
 			'permissions' => \OCP\Constants::PERMISSION_ALL,
 			'type' => FileInfo::TYPE_FOLDER,
 		], null);
-		$file = new \OCA\DAV\Connector\Sabre\File($view, $info);
+		$file = new \OCA\DAV\Connector\Sabre\File($view, $info, null, $request);
 
 		// action
 		$thrown = false;
@@ -872,15 +892,19 @@ class FileTest extends TestCase {
 			->method('filesize')
 			->willReturn(123456);
 
-		$_SERVER['CONTENT_LENGTH'] = 12345;
-		$_SERVER['REQUEST_METHOD'] = 'PUT';
+		$request = new Request([
+			'server' => [
+				'CONTENT_LENGTH' => '123456',
+			],
+			'method' => 'PUT',
+		], $this->requestId, $this->config, null);
 
 		$info = new \OC\Files\FileInfo('/test.txt', $this->getMockStorage(), null, [
 			'permissions' => \OCP\Constants::PERMISSION_ALL,
 			'type' => FileInfo::TYPE_FOLDER,
 		], null);
 
-		$file = new \OCA\DAV\Connector\Sabre\File($view, $info);
+		$file = new \OCA\DAV\Connector\Sabre\File($view, $info, null, $request);
 
 		// action
 		$thrown = false;

apps/dav/tests/unit/Connector/Sabre/RequestTest/RequestTestCase.php

@@ -32,7 +32,9 @@ use OC\Files\View;
 use OCA\DAV\Connector\Sabre\Server;
 use OCA\DAV\Connector\Sabre\ServerFactory;
 use OCP\EventDispatcher\IEventDispatcher;
+use OCP\IConfig;
 use OCP\IRequest;
+use OCP\IRequestId;
 use Psr\Log\LoggerInterface;
 use Sabre\HTTP\Request;
 use Test\TestCase;
@@ -58,8 +60,6 @@ abstract class RequestTestCase extends TestCase {
 	protected function setUp(): void {
 		parent::setUp();
 
-		unset($_SERVER['HTTP_OC_CHUNKED']);
-
 		$this->serverFactory = new ServerFactory(
 			\OC::$server->getConfig(),
 			\OC::$server->get(LoggerInterface::class),
@@ -106,20 +106,25 @@ abstract class RequestTestCase extends TestCase {
 
 		// since sabre catches all exceptions we need to save them and throw them from outside the sabre server
 
-		$originalServer = $_SERVER;
-
+		$serverParams = [];
 		if (is_array($headers)) {
 			foreach ($headers as $header => $value) {
-				$_SERVER['HTTP_' . strtoupper(str_replace('-', '_', $header))] = $value;
+				$serverParams['HTTP_' . strtoupper(str_replace('-', '_', $header))] = $value;
 			}
 		}
+		$ncRequest = new \OC\AppFramework\Http\Request([
+			'server' => $serverParams
+		], $this->createMock(IRequestId::class), $this->createMock(IConfig::class), null);
+
+		$this->overwriteService(IRequest::class, $ncRequest);
 
 		$result = $this->makeRequest($server, $request);
 
+		$this->restoreService(IRequest::class);
+
 		foreach ($exceptionPlugin->getExceptions() as $exception) {
 			throw $exception;
 		}
-		$_SERVER = $originalServer;
 		return $result;
 	}