-
Notifications
You must be signed in to change notification settings - Fork 111
Implement collaborative locking #2248
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
8c00c8e
53d1012
5c9225e
fa139cf
ede33b5
ff819af
de93bce
42a7882
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
Signed-off-by: Julius Härtl <[email protected]> TMP: Switch to OCP branch for tests Signed-off-by: Julius Härtl <[email protected]> Let locked files open in read only Signed-off-by: Julius Härtl <[email protected]> Adapt LockScope to LockContext rename Signed-off-by: Julius Härtl <[email protected]>
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,11 +27,18 @@ | |
| namespace OCA\Text\Service; | ||
|
|
||
| use \InvalidArgumentException; | ||
| use OCA\Text\AppInfo\Application; | ||
| use OCA\Text\Db\Session; | ||
| use OCA\Text\Db\SessionMapper; | ||
| use OCP\DirectEditing\IManager; | ||
| use OCP\Files\Lock\ILock; | ||
| use OCP\Files\Lock\ILockManager; | ||
| use OCP\Files\Lock\LockContext; | ||
| use OCP\Files\Lock\NoLockProviderException; | ||
| use OCP\Files\Lock\OwnerLockedException; | ||
| use OCP\IRequest; | ||
| use OCP\Lock\ILockingProvider; | ||
| use OCP\PreConditionNotMetException; | ||
| use function json_encode; | ||
| use OC\Files\Node\File; | ||
| use OCA\Text\Db\Document; | ||
|
|
@@ -67,44 +74,18 @@ class DocumentService { | |
| */ | ||
| public const AUTOSAVE_MINIMUM_DELAY = 10; | ||
|
|
||
| /** | ||
| * @var string|null | ||
| */ | ||
| private $userId; | ||
| /** | ||
| * @var DocumentMapper | ||
| */ | ||
| private $documentMapper; | ||
| /** | ||
| * @var SessionMapper | ||
| */ | ||
| private $sessionMapper; | ||
| /** | ||
| * @var ILogger | ||
| */ | ||
| private $logger; | ||
| /** | ||
| * @var ShareManager | ||
| */ | ||
| private $shareManager; | ||
| /** | ||
| * @var StepMapper | ||
| */ | ||
| private $stepMapper; | ||
| /** | ||
| * @var IRootFolder | ||
| */ | ||
| private $rootFolder; | ||
| /** | ||
| * @var ICache | ||
| */ | ||
| private $cache; | ||
| /** | ||
| * @var IAppData | ||
| */ | ||
| private $appData; | ||
|
|
||
| public function __construct(DocumentMapper $documentMapper, StepMapper $stepMapper, SessionMapper $sessionMapper, IAppData $appData, $userId, IRootFolder $rootFolder, ICacheFactory $cacheFactory, ILogger $logger, ShareManager $shareManager, IRequest $request, IManager $directManager, ILockingProvider $lockingProvider) { | ||
| private ?string $userId; | ||
| private DocumentMapper $documentMapper; | ||
| private SessionMapper $sessionMapper; | ||
| private ILogger $logger; | ||
| private ShareManager $shareManager; | ||
| private StepMapper $stepMapper; | ||
| private IRootFolder $rootFolder; | ||
| private ICache $cache; | ||
| private IAppData $appData; | ||
| private ILockManager $lockManager; | ||
|
|
||
| public function __construct(DocumentMapper $documentMapper, StepMapper $stepMapper, SessionMapper $sessionMapper, IAppData $appData, $userId, IRootFolder $rootFolder, ICacheFactory $cacheFactory, ILogger $logger, ShareManager $shareManager, IRequest $request, IManager $directManager, ILockingProvider $lockingProvider, ILockManager $lockManager) { | ||
| $this->documentMapper = $documentMapper; | ||
| $this->stepMapper = $stepMapper; | ||
| $this->sessionMapper = $sessionMapper; | ||
|
|
@@ -115,7 +96,7 @@ public function __construct(DocumentMapper $documentMapper, StepMapper $stepMapp | |
| $this->logger = $logger; | ||
| $this->shareManager = $shareManager; | ||
| $this->lockingProvider = $lockingProvider; | ||
|
|
||
| $this->lockManager = $lockManager; | ||
| $token = $request->getParam('token'); | ||
| if ($this->userId === null && $token !== null) { | ||
| try { | ||
|
|
@@ -328,7 +309,13 @@ public function autosave($file, $documentId, $version, $autoaveDocument, $force | |
| } | ||
| $this->cache->set('document-save-lock-' . $documentId, true, 10); | ||
| try { | ||
| $file->putContent($autoaveDocument); | ||
| $this->lockManager->runInScope(new LockContext( | ||
| $file, | ||
| ILock::TYPE_APP, | ||
| Application::APP_NAME | ||
| ), function () use ($file, $autoaveDocument) { | ||
| $file->putContent($autoaveDocument); | ||
| }); | ||
| } catch (LockedException $e) { | ||
| // Ignore lock since it might occur when multiple people save at the same time | ||
| return $document; | ||
|
|
@@ -348,6 +335,8 @@ public function autosave($file, $documentId, $version, $autoaveDocument, $force | |
| */ | ||
| public function resetDocument($documentId, $force = false): void { | ||
| try { | ||
| $this->unlock($documentId); | ||
|
|
||
| $document = $this->documentMapper->find($documentId); | ||
|
|
||
| if ($force || !$this->hasUnsavedChanges($document)) { | ||
|
|
@@ -490,4 +479,41 @@ private function ensureDocumentsFolder(): bool { | |
|
|
||
| return true; | ||
| } | ||
|
|
||
| public function lock(int $fileId): bool { | ||
| if (!$this->lockManager->isLockProviderAvailable()) { | ||
| return true; | ||
| } | ||
|
|
||
| $file = $this->getFileById($fileId); | ||
| try { | ||
| $this->lockManager->lock(new LockContext( | ||
| $file, | ||
| ILock::TYPE_APP, | ||
| Application::APP_NAME | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Out of topic but this could become
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, something for a separate PR. |
||
| )); | ||
| } catch (NoLockProviderException $e) { | ||
| } catch (OwnerLockedException $e) { | ||
| return false; | ||
| } catch (PreConditionNotMetException $e) { | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| public function unlock(int $fileId): void { | ||
| if (!$this->lockManager->isLockProviderAvailable()) { | ||
| return; | ||
| } | ||
|
|
||
| $file = $this->getFileById($fileId); | ||
| try { | ||
| $this->lockManager->unlock(new LockContext( | ||
| $file, | ||
| ILock::TYPE_APP, | ||
| Application::APP_NAME | ||
| )); | ||
| } catch (NoLockProviderException $e) { | ||
| } catch (PreConditionNotMetException $e) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No warning logs here?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd let the actual lock provider handle that, from the text app side there is no special interest if we unlock a file that has already been unlocked. |
||
| } | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.