Fix error unlocking a folder when previously deleting the metadata file

Signed-off-by: Georg Ehrke <developer@georgehrke.com>
nextcloud · Aug 17, 2020 · 9ca52f5 · 9ca52f5
1 parent 77dccde
commit 9ca52f5
Show file tree

Hide file tree

Showing 6 changed files with 67 additions and 45 deletions.
diff --git a/lib/Controller/LockingController.php b/lib/Controller/LockingController.php
@@ -144,7 +144,6 @@ public function unlockFolder(int $id): DataResponse {
 
 		$this->fileService->finalizeChanges($nodes[0]);
 		$this->metaDataStorage->saveIntermediateFile($this->userId, $id);
-		$this->metaDataStorage->deleteIntermediateFile($this->userId, $id);
 
 		try {
 			$this->lockManager->unlockFile($id, $token);

diff --git a/lib/Controller/MetaDataController.php b/lib/Controller/MetaDataController.php
@@ -188,7 +188,7 @@ public function updateMetaData(int $id, string $metaData): DataResponse {
 	 */
 	public function deleteMetaData(int $id): DataResponse {
 		try {
-			$this->metaDataStorage->deleteMetaData($this->userId, $id);
+			$this->metaDataStorage->updateMetaDataIntoIntermediateFile($this->userId, $id, '{}');
 		} catch (NotFoundException $e) {
 			throw new OCSNotFoundException($this->l10n->t('Could not find metadata for "%s"', [$id]));
 		} catch (NotPermittedException $e) {

diff --git a/lib/MetaDataStorage.php b/lib/MetaDataStorage.php
@@ -173,14 +173,20 @@ public function saveIntermediateFile(string $userId, int $id): void {
 		}
 
 		$intermediateMetaDataFile = $dir->getFile($this->intermediateMetaDataFileName);
-
-		try {
-			$finalFile = $dir->getFile($this->metaDataFileName);
-		} catch (NotFoundException $ex) {
-			$finalFile = $dir->newFile($this->metaDataFileName);
+		// If the intermediate file is empty, delete the metadata file
+		if ($intermediateMetaDataFile->getContent() === '{}') {
+			$dir->delete();
+		} else {
+			try {
+				$finalFile = $dir->getFile($this->metaDataFileName);
+			} catch (NotFoundException $ex) {
+				$finalFile = $dir->newFile($this->metaDataFileName);
+			}
+
+			$finalFile->putContent($intermediateMetaDataFile->getContent());
+			// After successfully saving, automatically delete the intermediate file
+			$intermediateMetaDataFile->delete();
 		}
-
-		$finalFile->putContent($intermediateMetaDataFile->getContent());
 	}
 
 	/**

diff --git a/tests/Unit/Controller/LockingControllerTest.php b/tests/Unit/Controller/LockingControllerTest.php
@@ -194,9 +194,6 @@ public function testUnlockFolder(bool $getUserFolderThrows,
 				$this->metaDataStorage->expects($this->once())
 					->method('saveIntermediateFile')
 					->with('john.doe', $fileId);
-				$this->metaDataStorage->expects($this->once())
-					->method('deleteIntermediateFile')
-					->with('john.doe', $fileId);
 
 				if ($unlockException) {
 					$this->lockManager->expects($this->once())

diff --git a/tests/Unit/Controller/MetaDataControllerTest.php b/tests/Unit/Controller/MetaDataControllerTest.php
@@ -301,13 +301,13 @@ public function testDeleteMetaData(?\Exception $metaDataStorageException,
 		$fileId = 42;
 		if ($metaDataStorageException) {
 			$this->metaDataStorage->expects($this->once())
-				->method('deleteMetaData')
-				->with('john.doe', $fileId)
+				->method('updateMetaDataIntoIntermediateFile')
+				->with('john.doe', $fileId, '{}')
 				->willThrowException($metaDataStorageException);
 		} else {
 			$this->metaDataStorage->expects($this->once())
-				->method('deleteMetaData')
-				->with('john.doe', $fileId);
+				->method('updateMetaDataIntoIntermediateFile')
+				->with('john.doe', $fileId, '{}');
 		}
 
 		$this->l10n->expects($this->any())

diff --git a/tests/Unit/MetaDataStorageTest.php b/tests/Unit/MetaDataStorageTest.php
@@ -328,10 +328,11 @@ public function deleteMetaDataDataProvider(): array {
 	 *
 	 * @param bool $folderExists
 	 * @param bool $intermediateFileExists
+	 * @param bool $intermediateFileIsEmpty
 	 * @param bool $finalFileExists
 	 * @param bool $expectsException
 	 */
-	public function testSaveIntermediateFile(bool $folderExists, bool $intermediateFileExists, bool $finalFileExists, bool $expectsException): void {
+	public function testSaveIntermediateFile(bool $folderExists, bool $intermediateFileExists, bool $intermediateFileIsEmpty, bool $finalFileExists, bool $expectsException): void {
 		$metaDataStorage = $this->getMockBuilder(MetaDataStorage::class)
 			->setMethods([
 				'verifyOwner',
@@ -363,35 +364,52 @@ public function testSaveIntermediateFile(bool $folderExists, bool $intermediateF
 
 			if ($intermediateFileExists) {
 				$intermediateFile = $this->createMock(ISimpleFile::class);
-				$intermediateFile->expects($this->once())
-					->method('getContent')
-					->willReturn('intermediate-file-content');
-
-				$metaDataFolder->expects($this->at(1))
-					->method('getFile')
-					->with('intermediate.meta.data')
-					->willReturn($intermediateFile);
+				if ($intermediateFileIsEmpty) {
+					$intermediateFile->expects($this->once())
+						->method('getContent')
+						->willReturn('{}');
 
-				$finalFile = $this->createMock(ISimpleFile::class);
-				$finalFile->expects($this->once())
-					->method('putContent')
-					->with('intermediate-file-content');
-
-				if ($finalFileExists) {
-					$metaDataFolder->expects($this->at(2))
+					$metaDataFolder->expects($this->at(1))
 						->method('getFile')
-						->with('meta.data')
-						->willReturn($finalFile);
+						->with('intermediate.meta.data')
+						->willReturn($intermediateFile);
+
+					$metaDataFolder->expects($this->once())
+						->method('delete');
 				} else {
-					$metaDataFolder->expects($this->at(2))
+					$intermediateFile->expects($this->exactly(2))
+						->method('getContent')
+						->willReturn('intermediate-file-content');
+
+					$metaDataFolder->expects($this->at(1))
 						->method('getFile')
-						->with('meta.data')
-						->willThrowException(new NotFoundException());
+						->with('intermediate.meta.data')
+						->willReturn($intermediateFile);
 
-					$metaDataFolder->expects($this->at(3))
-						->method('newFile')
-						->with('meta.data')
-						->willReturn($finalFile);
+					$finalFile = $this->createMock(ISimpleFile::class);
+					$finalFile->expects($this->once())
+						->method('putContent')
+						->with('intermediate-file-content');
+
+					if ($finalFileExists) {
+						$metaDataFolder->expects($this->at(2))
+							->method('getFile')
+							->with('meta.data')
+							->willReturn($finalFile);
+					} else {
+						$metaDataFolder->expects($this->at(2))
+							->method('getFile')
+							->with('meta.data')
+							->willThrowException(new NotFoundException());
+
+						$metaDataFolder->expects($this->at(3))
+							->method('newFile')
+							->with('meta.data')
+							->willReturn($finalFile);
+					}
+
+					$intermediateFile->expects($this->once())
+						->method('delete');
 				}
 			}
 		} else {
@@ -411,10 +429,12 @@ public function testSaveIntermediateFile(bool $folderExists, bool $intermediateF
 
 	public function saveIntermediateFileDataProvider(): array {
 		return [
-			[false, false, false, true],
-			[true, false, false, true],
-			[true, true, true, false],
-			[true, true, false, false],
+			[false, false, false, false, true],
+			[true, false, false, false, true],
+			[true, true, false, true, false],
+			[true, true, true, true, false],
+			[true, true, false, false, false],
+			[true, true, true, false, false],
 		];
 	}