Merge pull request #658 from nextcloud/backport/610/stable18v18.0.1RC2v18.0.1RC1

[stable18] Avoid race condition when adding steps and fix step entries with duplicate versions

5 files changed, 187 insertions, 27 deletions

diff --git a/appinfo/info.xml b/appinfo/info.xml
index 5b11960d0..93220787a 100644
--- a/appinfo/info.xml
+++ b/appinfo/info.xml
@@ -32,6 +32,9 @@
 	<background-jobs>
 		<job>OCA\Text\Cron\Cleanup</job>
 	</background-jobs>
+	<commands>
+		<command>OCA\Text\Command\ResetDocument</command>
+	</commands>
 	<sabre>
 		<plugins>
 			<plugin>OCA\Text\DAV\WorkspacePlugin</plugin>
diff --git a/lib/Command/ResetDocument.php b/lib/Command/ResetDocument.php
new file mode 100644
index 000000000..ca414f7e2
--- /dev/null
+++ b/lib/Command/ResetDocument.php
@@ -0,0 +1,99 @@
+<?php
+/**
+ * @copyright Copyright (c) 2018 Julius Härtl <jus@bitgrid.net>
+ *
+ * @author Julius Härtl <jus@bitgrid.net>
+ *
+ * @license GNU AGPL version 3 or any later version
+ *
+ * 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
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+namespace OCA\Text\Command;
+
+use OCA\Text\Db\DocumentMapper;
+use OCA\Text\Db\SessionMapper;
+use OCA\Text\Db\StepMapper;
+use OCA\Text\Service\DocumentService;
+use Symfony\Component\Console\Command\Command;
+use Symfony\Component\Console\Input\InputArgument;
+use Symfony\Component\Console\Input\InputInterface;
+use Symfony\Component\Console\Output\OutputInterface;
+
+class ResetDocument extends Command {
+
+	protected $documentService;
+	protected $documentMapper;
+	protected $stepMapper;
+	protected $sessionMapper;
+
+	public function __construct(DocumentService $documentService, DocumentMapper $documentMapper, StepMapper $stepMapper, SessionMapper $sessionMapper) {
+		parent::__construct();
+
+		$this->documentService = $documentService;
+		$this->documentMapper = $documentMapper;
+		$this->stepMapper = $stepMapper;
+		$this->sessionMapper = $sessionMapper;
+	}
+
+	protected function configure() {
+		$this
+			->setName('text:reset')
+			->setDescription('Reset a text document')
+			->addArgument(
+				'file-id',
+				InputArgument::REQUIRED,
+				'File id of the document to rest'
+			)
+			->addOption(
+				'full',
+				'f',
+				null,
+				'Drop all existing steps and use the currently saved version'
+			)
+		;
+	}
+
+	/**
+	 * @param InputInterface $input
+	 * @param OutputInterface $output
+	 * @return void
+	 */
+	protected function execute(InputInterface $input, OutputInterface $output) {
+
+		$fileId = $input->getArgument('file-id');
+		$fullReset = $input->getOption('full');
+
+		if ($fullReset) {
+			$output->writeln('Full document reset');
+			$this->documentService->resetDocument($fileId, true);
+		} else {
+			$output->writeln('Trying to restore to last saved version');
+			$document = $this->documentMapper->find($fileId);
+			$deleted = $this->stepMapper->deleteAfterVersion($fileId, $document->getLastSavedVersion());
+			if ($deleted > 0) {
+				$document->setCurrentVersion($document->getLastSavedVersion());
+				$this->documentMapper->update($document);
+				$this->sessionMapper->deleteByDocumentId($fileId);
+				$output->writeln('Reverted document to the last saved version');
+			} else {
+				$output->writeln('Failed revert changes that are newer than the last saved version');
+			}
+		}
+
+	}
+}
+
+
diff --git a/lib/Db/SessionMapper.php b/lib/Db/SessionMapper.php
index 0bef02a2e..f12aad9d8 100644
--- a/lib/Db/SessionMapper.php
+++ b/lib/Db/SessionMapper.php
@@ -101,4 +101,12 @@ class SessionMapper extends QBMapper {
 		return $qb->execute();
 	}
 
+	public function deleteByDocumentId($documentId) {
+		/* @var $qb IQueryBuilder */
+		$qb = $this->db->getQueryBuilder();
+		$qb->delete($this->getTableName())
+			->where($qb->expr()->eq('document_id', $qb->createNamedParameter($documentId)));
+		return $qb->execute();
+	}
+
 }
diff --git a/lib/Db/StepMapper.php b/lib/Db/StepMapper.php
index 4546c92d2..44b7eafd0 100644
--- a/lib/Db/StepMapper.php
+++ b/lib/Db/StepMapper.php
@@ -47,6 +47,7 @@ class StepMapper extends QBMapper {
 		$qb
 			->setMaxResults(100)
 			->orderBy('version')
+			->orderBy('id')
 			->execute();
 
 		return $this->findEntities($qb);
@@ -68,4 +69,14 @@ class StepMapper extends QBMapper {
 			->andWhere($qb->expr()->lte('version', $qb->createNamedParameter($version)))
 			->execute();
 	}
+
+	public function deleteAfterVersion($documentId, $version): int {
+		/* @var $qb IQueryBuilder */
+		$qb = $this->db->getQueryBuilder();
+		return $qb->delete($this->getTableName())
+			->where($qb->expr()->eq('document_id', $qb->createNamedParameter($documentId)))
+			->andWhere($qb->expr()->gt('version', $qb->createNamedParameter($version)))
+			->execute();
+	}
+
 }
diff --git a/lib/Service/DocumentService.php b/lib/Service/DocumentService.php
index af19f7bc0..467dc4c80 100644
--- a/lib/Service/DocumentService.php
+++ b/lib/Service/DocumentService.php
@@ -29,6 +29,7 @@ use \InvalidArgumentException;
 use OCA\Text\Db\Session;
 use OCP\DirectEditing\IManager;
 use OCP\IRequest;
+use OCP\Lock\ILockingProvider;
 use function json_encode;
 use OC\Files\Node\File;
 use OCA\Text\Db\Document;
@@ -97,7 +98,7 @@ class DocumentService {
 	 */
 	private $appData;
 
-	public function __construct(DocumentMapper $documentMapper, StepMapper $stepMapper, IAppData $appData, $userId, IRootFolder $rootFolder, ICacheFactory $cacheFactory, ILogger $logger, ShareManager $shareManager, IRequest $request, IManager $directManager) {
+	public function __construct(DocumentMapper $documentMapper, StepMapper $stepMapper, IAppData $appData, $userId, IRootFolder $rootFolder, ICacheFactory $cacheFactory, ILogger $logger, ShareManager $shareManager, IRequest $request, IManager $directManager, ILockingProvider $lockingProvider) {
 		$this->documentMapper = $documentMapper;
 		$this->stepMapper = $stepMapper;
 		$this->userId = $userId;
@@ -106,6 +107,7 @@ class DocumentService {
 		$this->cache = $cacheFactory->createDistributed('text');
 		$this->logger = $logger;
 		$this->shareManager = $shareManager;
+		$this->lockingProvider = $lockingProvider;
 		try {
 			$this->appData->getFolder('documents');
 		} catch (NotFoundException $e) {
@@ -189,38 +191,75 @@ class DocumentService {
 	 * @throws VersionMismatchException
 	 */
 	public function addStep($documentId, $sessionId, $steps, $version): array {
-		// TODO check cache
-		$document = $this->documentMapper->find($documentId);
-		if ($version !== $document->getCurrentVersion()) {
+		$document = null;
+		$oldVersion = null;
+
+		try {
+			$this->lockingProvider->acquireLock('document-push-lock-' . $documentId, ILockingProvider::LOCK_EXCLUSIVE);
+		} catch (LockedException $e) {
 			throw new VersionMismatchException('Version does not match');
 		}
-		$stepsJson = json_encode($steps);
-		if (!is_array($steps) || $stepsJson === null) {
-			throw new InvalidArgumentException('Failed to encode steps');
-		}
-		$validStepTypes = ['addMark', 'removeMark', 'replace', 'replaceAround'];
-		foreach ($steps as $step) {
-			if (array_key_exists('stepType', $step) && !in_array($step['stepType'], $validStepTypes, true)) {
-				throw new InvalidArgumentException('Invalid step data');
+
+		try {
+			$document = $this->documentMapper->find($documentId);
+			if ($version !== $document->getCurrentVersion()) {
+				throw new VersionMismatchException('Version does not match');
+			}
+			$stepsJson = json_encode($steps);
+			if (!is_array($steps) || $stepsJson === null) {
+				throw new InvalidArgumentException('Failed to encode steps');
 			}
+			$validStepTypes = ['addMark', 'removeMark', 'replace', 'replaceAround'];
+			foreach ($steps as $step) {
+				if (array_key_exists('stepType', $step) && !in_array($step['stepType'], $validStepTypes, true)) {
+					throw new InvalidArgumentException('Invalid step data');
+				}
+			}
+			$oldVersion = $document->getCurrentVersion();
+			$newVersion = $oldVersion + count($steps);
+			$this->cache->set('document-version-' . $document->getId(), $newVersion);
+			$document->setCurrentVersion($newVersion);
+			$this->documentMapper->update($document);
+			$step = new Step();
+			$step->setData($stepsJson);
+			$step->setSessionId($sessionId);
+			$step->setDocumentId($documentId);
+			$step->setVersion($version + 1);
+			$this->stepMapper->insert($step);
+			// TODO write steps to cache for quicker reading
+			$this->lockingProvider->releaseLock('document-push-lock-' . $documentId, ILockingProvider::LOCK_EXCLUSIVE);
+			return $steps;
+		} catch (DoesNotExistException $e) {
+			$this->lockingProvider->releaseLock('document-push-lock-' . $documentId, ILockingProvider::LOCK_EXCLUSIVE);
+			throw $e;
+		} catch (\Throwable $e) {
+			if ($document !== null && $oldVersion !== null) {
+				$this->logger->logException($e, ['message' => 'This should never happen. An error occured when storing the version, trying to recover the last stable one']);
+				$document->setCurrentVersion($oldVersion);
+				$this->documentMapper->update($document);
+				$this->cache->set('document-version-' . $document->getId(), $oldVersion);
+				$this->stepMapper->deleteAfterVersion($documentId, $oldVersion);
+			}
+			$this->lockingProvider->releaseLock('document-push-lock-' . $documentId, ILockingProvider::LOCK_EXCLUSIVE);
+			throw $e;
 		}
-		$newVersion = $document->getCurrentVersion() + count($steps);
-		$document->setCurrentVersion($newVersion);
-		$this->documentMapper->update($document);
-		$step = new Step();
-		$step->setData($stepsJson);
-		$step->setSessionId($sessionId);
-		$step->setDocumentId($documentId);
-		$step->setVersion($version+1);
-		$this->stepMapper->insert($step);
-		$this->cache->set('document-version-'.$document->getId(), $newVersion);
-		// TODO restore old version for document if adding steps has failed
-		// TODO write steps to cache for quicker reading
-		return $steps;
 	}
 
 	public function getSteps($documentId, $lastVersion) {
-		return $this->stepMapper->find($documentId, $lastVersion);
+		$steps = $this->stepMapper->find($documentId, $lastVersion);
+		//return $steps;
+		$unique_array = [];
+		foreach($steps as $step) {
+			$version = $step->getVersion();
+			if (!array_key_exists($version, $unique_array)) {
+				$unique_array[(string)$version] = $step;
+			} else {
+				// found duplicate step
+				// FIXME: verify that this version is the correct one
+				//$this->stepMapper->delete($step);
+			}
+		}
+		return array_values($unique_array);
 	}
 
 	/**
@@ -280,7 +319,7 @@ class DocumentService {
 			// Ignore lock since it might occur when multiple people save at the same time
 			return $document;
 		}
-		$document->setLastSavedVersion($version);
+		$document->setLastSavedVersion($document->getCurrentVersion());
 		$document->setLastSavedVersionTime(time());
 		$document->setLastSavedVersionEtag($file->getEtag());
 		$this->documentMapper->update($document);