Handle empty files in version creation logic

Signed-off-by: Louis Chemineau <louis@chmn.me>

apps/files_trashbin/tests/StorageTest.php

@@ -35,13 +35,11 @@ use OC\Files\Filesystem;
 use OC\Files\Storage\Common;
 use OC\Files\Storage\Local;
 use OC\Files\Storage\Temporary;
-use OCA\Files_Versions\AppInfo\Application as FVApplication;
 use OCA\Files_Trashbin\AppInfo\Application;
 use OCA\Files_Trashbin\Events\MoveToTrashEvent;
 use OCA\Files_Trashbin\Storage;
 use OCA\Files_Trashbin\Trash\ITrashManager;
 use OCP\AppFramework\Bootstrap\IBootContext;
-use OCP\AppFramework\Bootstrap\IRegistrationContext;
 use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\Files\Cache\ICache;
 use OCP\Files\Folder;

apps/files_versions/lib/AppInfo/Application.php

@@ -47,10 +47,13 @@ use OCP\AppFramework\Bootstrap\IRegistrationContext;
 use OCP\Files\Events\Node\BeforeNodeCopiedEvent;
 use OCP\Files\Events\Node\BeforeNodeDeletedEvent;
 use OCP\Files\Events\Node\BeforeNodeRenamedEvent;
+use OCP\Files\Events\Node\BeforeNodeTouchedEvent;
 use OCP\Files\Events\Node\NodeCopiedEvent;
 use OCP\Files\Events\Node\NodeDeletedEvent;
 use OCP\Files\Events\Node\NodeRenamedEvent;
 use OCP\Files\Events\Node\BeforeNodeWrittenEvent;
+use OCP\Files\Events\Node\NodeCreatedEvent;
+use OCP\Files\Events\Node\NodeTouchedEvent;
 use OCP\Files\Events\Node\NodeWrittenEvent;
 use OCP\IConfig;
 use OCP\IGroupManager;
@@ -105,6 +108,9 @@ class Application extends App implements IBootstrap {
 		$context->registerEventListener(LoadAdditionalScriptsEvent::class, LoadAdditionalListener::class);
 		$context->registerEventListener(LoadSidebar::class, LoadSidebarListener::class);
 
+		$context->registerEventListener(NodeCreatedEvent::class, FileEventsListener::class);
+		$context->registerEventListener(BeforeNodeTouchedEvent::class, FileEventsListener::class);
+		$context->registerEventListener(NodeTouchedEvent::class, FileEventsListener::class);
 		$context->registerEventListener(BeforeNodeWrittenEvent::class, FileEventsListener::class);
 		$context->registerEventListener(NodeWrittenEvent::class, FileEventsListener::class);
 		$context->registerEventListener(BeforeNodeDeletedEvent::class, FileEventsListener::class);

apps/files_versions/lib/Db/VersionsMapper.php

@@ -52,13 +52,28 @@ class VersionsMapper extends QBMapper {
 		return $this->findEntities($qb);
 	}
 
+	/**
+	 * @return VersionEntity
+	 */
+	public function findCurrentVersionForFileId(int $fileId): VersionEntity {
+		$qb = $this->db->getQueryBuilder();
+
+		$qb->select('*')
+			 ->from($this->getTableName())
+			 ->where($qb->expr()->eq('file_id', $qb->createNamedParameter($fileId)))
+			 ->orderBy('timestamp', 'DESC')
+			 ->setMaxResults(1);
+
+		return $this->findEntity($qb);
+	}
+
 	public function findVersionForFileId(int $fileId, int $timestamp): VersionEntity {
 		$qb = $this->db->getQueryBuilder();
 
 		$qb->select('*')
 			 ->from($this->getTableName())
 			 ->where($qb->expr()->eq('file_id', $qb->createNamedParameter($fileId)))
-			 ->where($qb->expr()->eq('timestamp', $qb->createNamedParameter($timestamp)));
+			 ->andWhere($qb->expr()->eq('timestamp', $qb->createNamedParameter($timestamp)));
 
 		return $this->findEntity($qb);
 	}

apps/files_versions/lib/Listener/FileEventsListener.php

@@ -43,11 +43,15 @@ use OCP\EventDispatcher\IEventListener;
 use OCP\Files\Events\Node\BeforeNodeCopiedEvent;
 use OCP\Files\Events\Node\BeforeNodeDeletedEvent;
 use OCP\Files\Events\Node\BeforeNodeRenamedEvent;
+use OCP\Files\Events\Node\BeforeNodeTouchedEvent;
 use OCP\Files\Events\Node\BeforeNodeWrittenEvent;
 use OCP\Files\Events\Node\NodeCopiedEvent;
+use OCP\Files\Events\Node\NodeCreatedEvent;
 use OCP\Files\Events\Node\NodeDeletedEvent;
 use OCP\Files\Events\Node\NodeRenamedEvent;
+use OCP\Files\Events\Node\NodeTouchedEvent;
 use OCP\Files\Events\Node\NodeWrittenEvent;
+use OCP\Files\Folder;
 use OCP\Files\IMimeTypeLoader;
 use OCP\Files\IRootFolder;
 use OCP\Files\Node;
@@ -56,9 +60,13 @@ class FileEventsListener implements IEventListener {
 	private IRootFolder $rootFolder;
 	private VersionsMapper $versionsMapper;
 	/**
-	 * @var array<int, bool>
+	 * @var array<int, array>
 	 */
-	private array $versionsCreated = [];
+	private array $writeHookInfo = [];
+	/**
+	 * @var array<int, Node>
+	 */
+	private array $nodesTouched = [];
 	/**
 	 * @var array<string, Node>
 	 */
@@ -76,6 +84,18 @@ class FileEventsListener implements IEventListener {
 	}
 
 	public function handle(Event $event): void {
+		if ($event instanceof NodeCreatedEvent) {
+			$this->created($event->getNode());
+		}
+
+		if ($event instanceof BeforeNodeTouchedEvent) {
+			$this->pre_touch_hook($event->getNode());
+		}
+
+		if ($event instanceof NodeTouchedEvent) {
+			$this->touch_hook($event->getNode());
+		}
+
 		if ($event instanceof BeforeNodeWrittenEvent) {
 			$this->write_hook($event->getNode());
 		}
@@ -109,37 +129,37 @@ class FileEventsListener implements IEventListener {
 		}
 	}
 
-	/**
-	 * listen to write event.
-	 */
-	public function write_hook(Node $node): void {
-		// $node is be nonexisting on file creation.
-		if ($node instanceof NonExistingFolder || $node instanceof NonExistingFile) {
+	public function pre_touch_hook(Node $node): void {
+		// Do not handle folders.
+		if ($node instanceof Folder) {
 			return;
 		}
 
-		$userFolder = $this->rootFolder->getUserFolder($node->getOwner()->getUID());
-		$path = $userFolder->getRelativePath($node->getPath());
-		$result = Storage::store($path);
-
-		if ($result === false) {
+		// $node is a non-existing on file creation.
+		if ($node instanceof NonExistingFile) {
 			return;
 		}
 
-		// Store the result of the version creation so it can be used in post_write_hook.
-		$this->versionsCreated[$node->getId()] = true;
+		$this->nodesTouched[$node->getId()] = $node;
 	}
 
-	/**
-	 * listen to post_write event.
-	 */
-	public function post_write_hook(Node $node): void {
-		if (!array_key_exists($node->getId(), $this->versionsCreated)) {
+	public function touch_hook(Node $node): void {
+		$previousNode = $this->nodesTouched[$node->getId()] ?? null;
+
+		if ($previousNode === null) {
 			return;
 		}
 
-		unset($this->versionsCreated[$node->getId()]);
+		unset($this->nodesTouched[$node->getId()]);
 
+		// We update the timestamp of the version entity associated with the previousNode.
+		$versionEntity = $this->versionsMapper->findVersionForFileId($previousNode->getId(), $previousNode->getMTime());
+		// Create a version in the DB for the current content.
+		$versionEntity->setTimestamp($node->getMTime());
+		$this->versionsMapper->update($versionEntity);
+	}
+
+	public function created(Node $node): void {
 		$versionEntity = new VersionEntity();
 		$versionEntity->setFileId($node->getId());
 		$versionEntity->setTimestamp($node->getMTime());
@@ -149,6 +169,68 @@ class FileEventsListener implements IEventListener {
 		$this->versionsMapper->insert($versionEntity);
 	}
 
+	/**
+	 * listen to write event.
+	 */
+	public function write_hook(Node $node): void {
+		// Do not handle folders.
+		if ($node instanceof Folder) {
+			return;
+		}
+
+		// $node is a non-existing on file creation.
+		if ($node instanceof NonExistingFile) {
+			return;
+		}
+
+		$userFolder = $this->rootFolder->getUserFolder($node->getOwner()->getUID());
+		$path = $userFolder->getRelativePath($node->getPath());
+		$result = Storage::store($path);
+
+		// Store the result of the version creation so it can be used in post_write_hook.
+		$this->writeHookInfo[$node->getId()] = [
+			'previousNode' => $node,
+			'versionCreated' => $result !== false
+		];
+	}
+
+	/**
+	 * listen to post_write event.
+	 */
+	public function post_write_hook(Node $node): void {
+		// Do not handle folders.
+		if ($node instanceof Folder) {
+			return;
+		}
+
+		$writeHookInfo = $this->writeHookInfo[$node->getId()] ?? null;
+
+		if ($writeHookInfo === null) {
+			return;
+		}
+
+		if ($writeHookInfo['versionCreated']) {
+			// If a new version was created, insert a version in the DB for the current content.
+			$versionEntity = new VersionEntity();
+			$versionEntity->setFileId($node->getId());
+			$versionEntity->setTimestamp($node->getMTime());
+			$versionEntity->setSize($node->getSize());
+			$versionEntity->setMimetype($this->mimeTypeLoader->getId($node->getMimetype()));
+			$versionEntity->setMetadata([]);
+			$this->versionsMapper->insert($versionEntity);
+		} else {
+			// If no new version was stored in the FS, no new version should be added in the DB.
+			// So we simply update the associated version.
+			$currentVersionEntity = $this->versionsMapper->findVersionForFileId($node->getId(), $writeHookInfo['previousNode']->getMtime());
+			$currentVersionEntity->setTimestamp($node->getMTime());
+			$currentVersionEntity->setSize($node->getSize());
+			$currentVersionEntity->setMimetype($this->mimeTypeLoader->getId($node->getMimetype()));
+			$this->versionsMapper->update($currentVersionEntity);
+		}
+
+		unset($this->versionsCreated[$node->getId()]);
+	}
+
 	/**
 	 * Erase versions of deleted file
 	 *
@@ -156,6 +238,7 @@ class FileEventsListener implements IEventListener {
 	 * cleanup the versions directory if the actual file gets deleted
 	 */
 	public function remove_hook(Node $node): void {
+		// Need to normalize the path as there is an issue with path concatenation in View.php::getAbsolutePath.
 		$path = Filesystem::normalizePath($node->getPath());
 		if (!array_key_exists($path, $this->versionsDeleted)) {
 			return;

apps/files_versions/lib/Sabre/VersionFile.php

@@ -89,7 +89,7 @@ class VersionFile implements IFile {
 	}
 
 	public function getLabel(): ?string {
-		if ($this->version instanceof INameableVersion) {
+		if ($this->version instanceof INameableVersion && $this->version->getSourceFile()->getSize() > 0) {
 			return $this->version->getLabel();
 		} else {
 			return null;

apps/files_versions/lib/Versions/LegacyVersionsBackend.php

@@ -82,15 +82,13 @@ class LegacyVersionsBackend implements IVersionBackend, INameableVersionBackend,
 		}
 
 		// Insert the entry in the DB for the current version.
-		if ($file2->getSize() > 0) {
-			$versionEntity = new VersionEntity();
-			$versionEntity->setFileId($file2->getId());
-			$versionEntity->setTimestamp($file2->getMTime());
-			$versionEntity->setSize($file2->getSize());
-			$versionEntity->setMimetype($this->mimeTypeLoader->getId($file2->getMimetype()));
-			$versionEntity->setMetadata([]);
-			$this->versionsMapper->insert($versionEntity);
-		}
+		$versionEntity = new VersionEntity();
+		$versionEntity->setFileId($file2->getId());
+		$versionEntity->setTimestamp($file2->getMTime());
+		$versionEntity->setSize($file2->getSize());
+		$versionEntity->setMimetype($this->mimeTypeLoader->getId($file2->getMimetype()));
+		$versionEntity->setMetadata([]);
+		$this->versionsMapper->insert($versionEntity);
 
 		// Insert entries in the DB for existing versions.
 		$versionsOnFS = Storage::getVersions($user->getUID(), $userFolder->getRelativePath($file2->getPath()));

apps/files_versions/tests/StorageTest.php

@@ -44,6 +44,8 @@ class StorageTest extends TestCase {
 	protected function setUp(): void {
 		parent::setUp();
 
+		\OC::$server->boot();
+
 		$expiration = $this->createMock(Expiration::class);
 		$expiration->method('getMaxAgeAsTimestamp')
 			->willReturnCallback(function () {