diff options
author | Julius Härtl <jus@bitgrid.net> | 2020-02-05 17:10:48 +0300 |
---|---|---|
committer | GitHub <noreply@github.com> | 2020-02-05 17:10:48 +0300 |
commit | 76d1d57d24433d1f621d1bef69e740aa2994b3d3 (patch) | |
tree | b7c1209dd07dba5debe313c6d0ad7eb502c8de09 | |
parent | a4a4ec88efe726386ddc4aef8511b2d056dacfd7 (diff) | |
parent | b754f6b75d704359d239b2ac8d63ee1cc83ba5c1 (diff) |
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
-rw-r--r-- | appinfo/info.xml | 3 | ||||
-rw-r--r-- | lib/Command/ResetDocument.php | 99 | ||||
-rw-r--r-- | lib/Db/SessionMapper.php | 8 | ||||
-rw-r--r-- | lib/Db/StepMapper.php | 11 | ||||
-rw-r--r-- | lib/Service/DocumentService.php | 93 |
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); |