diff --git a/cypress/e2e/SessionApi.spec.js b/cypress/e2e/SessionApi.spec.js index c06c932ecb5..179f5b966f1 100644 --- a/cypress/e2e/SessionApi.spec.js +++ b/cypress/e2e/SessionApi.spec.js @@ -350,7 +350,8 @@ describe('The session Api', function() { }) // Failed with a probability of ~ 50% initially - it('ignores steps stored after close cleaned up', function() { + // Skipped for now since the behaviour chanced by not cleaning up the state on close/create + it.skip('ignores steps stored after close cleaned up', function() { cy.pushAndClose({ connection, steps: [messages.update], version }) cy.createTextSession(undefined, { filePath: '', shareToken }) .then(con => { diff --git a/cypress/e2e/sync.spec.js b/cypress/e2e/sync.spec.js index a38fa206758..5093dd07842 100644 --- a/cypress/e2e/sync.spec.js +++ b/cypress/e2e/sync.spec.js @@ -95,14 +95,14 @@ describe('Sync', () => { .should('include', 'after the lost connection') }) - it('passes the doc content from one session to the next', () => { + it.only('passes the doc content from one session to the next', () => { cy.closeFile() cy.intercept({ method: 'PUT', url: '**/apps/text/session/create' }) .as('create') cy.openTestFile() cy.wait('@create', { timeout: 10000 }) .its('response.body') - .should('have.property', 'content') + .should('have.property', 'documentState') .should('not.be.empty') cy.getContent().find('h2').should('contain', 'Hello world') cy.getContent().find('li').should('contain', 'Saving the doc saves the doc state') diff --git a/lib/Controller/PublicSessionController.php b/lib/Controller/PublicSessionController.php index 6574edc2e3d..e8500723f9b 100644 --- a/lib/Controller/PublicSessionController.php +++ b/lib/Controller/PublicSessionController.php @@ -66,8 +66,8 @@ protected function isPasswordProtected(): bool { * @NoAdminRequired * @PublicPage */ - public function create(string $token, string $file = null, $guestName = null, bool $forceRecreate = false): DataResponse { - return $this->apiService->create(null, $file, $token, $guestName, $forceRecreate); + public function create(string $token, string $file = null, $guestName = null): DataResponse { + return $this->apiService->create(null, $file, $token, $guestName); } /** diff --git a/lib/Controller/SessionController.php b/lib/Controller/SessionController.php index 63f5d8f4c9c..5b7a2225337 100644 --- a/lib/Controller/SessionController.php +++ b/lib/Controller/SessionController.php @@ -53,8 +53,8 @@ public function __construct(string $appName, IRequest $request, ApiService $apiS /** * @NoAdminRequired */ - public function create(int $fileId = null, string $file = null, bool $forceRecreate = false): DataResponse { - return $this->apiService->create($fileId, $file, null, null, $forceRecreate); + public function create(int $fileId = null, string $file = null): DataResponse { + return $this->apiService->create($fileId, $file, null, null); } /** diff --git a/lib/Cron/Cleanup.php b/lib/Cron/Cleanup.php index 05475de4bc7..7d2d1359130 100644 --- a/lib/Cron/Cleanup.php +++ b/lib/Cron/Cleanup.php @@ -28,8 +28,6 @@ namespace OCA\Text\Cron; -use OCA\Text\Db\Session; -use OCA\Text\Exception\DocumentHasUnsavedChangesException; use OCA\Text\Service\DocumentService; use OCA\Text\Service\AttachmentService; use OCA\Text\Service\SessionService; @@ -57,29 +55,25 @@ public function __construct(ITimeFactory $time, } protected function run($argument) { - $this->logger->debug('Run cleanup job for text sessions'); - $inactive = $this->sessionService->findAllInactive(); - /** @var Session $session */ - foreach ($inactive as $session) { - $activeSessions = $this->sessionService->getActiveSessions($session->getDocumentId()); - if (count($activeSessions) > 0) { + $this->logger->debug('Run cleanup job for text documents'); + $documents = $this->documentService->getAll(); + foreach ($documents as $document) { + $allSessions = $this->sessionService->getAllSessions($document->getId()); + if (count($allSessions) > 0) { + // Do not reset if there are any sessions left + // Inactive sessions will get removed further down and will trigger a reset next time continue; } - $this->documentService->unlock($session->getDocumentId()); - - try { - $this->logger->debug('Resetting document ' . $session->getDocumentId() . ''); - $this->documentService->resetDocument($session->getDocumentId()); - $this->attachmentService->cleanupAttachments($session->getDocumentId()); - $this->logger->debug('Resetting document ' . $session->getDocumentId() . ''); - } catch (DocumentHasUnsavedChangesException $e) { - $this->logger->info('Document ' . $session->getDocumentId() . ' has not been reset, as it has unsaved changes'); - } catch (\Throwable $e) { - $this->logger->error('Document ' . $session->getDocumentId() . ' has not been reset, as an error occured', ['exception' => $e]); + if ($this->documentService->hasUnsavedChanges($document)) { + continue; } + + $this->documentService->resetDocument($document->getId()); } - $removedSessions = $this->sessionService->removeInactiveSessions(null); + + $this->logger->debug('Run cleanup job for text sessions'); + $removedSessions = $this->sessionService->removeInactiveSessionsWithoutSteps(null); $this->logger->debug('Removed ' . $removedSessions . ' inactive sessions'); } } diff --git a/lib/Db/DocumentMapper.php b/lib/Db/DocumentMapper.php index 9449bed44bc..33d11d31709 100644 --- a/lib/Db/DocumentMapper.php +++ b/lib/Db/DocumentMapper.php @@ -54,4 +54,13 @@ public function find($documentId): Document { } return Document::fromRow($data); } + + public function findAll(): array { + $qb = $this->db->getQueryBuilder(); + $result = $qb->select('*') + ->from($this->getTableName()) + ->execute(); + + return $this->findEntities($qb); + } } diff --git a/lib/Db/SessionMapper.php b/lib/Db/SessionMapper.php index 62851b85969..0abc6881606 100644 --- a/lib/Db/SessionMapper.php +++ b/lib/Db/SessionMapper.php @@ -99,7 +99,7 @@ public function findAllInactive() { return $this->findEntities($qb); } - public function deleteInactive($documentId = -1) { + public function deleteInactiveWithoutSteps($documentId = -1) { $qb = $this->db->getQueryBuilder(); $qb->select('session_id') ->from('text_steps'); diff --git a/lib/Service/ApiService.php b/lib/Service/ApiService.php index 4b754fcede1..5e74c876ff4 100644 --- a/lib/Service/ApiService.php +++ b/lib/Service/ApiService.php @@ -31,7 +31,6 @@ use OC\Files\Node\File; use OCA\Files_Sharing\SharedStorage; use OCA\Text\AppInfo\Application; -use OCA\Text\Exception\DocumentHasUnsavedChangesException; use OCA\Text\Exception\DocumentSaveConflictException; use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Http; @@ -72,7 +71,7 @@ public function __construct(IRequest $request, $this->l10n = $l10n; } - public function create($fileId = null, $filePath = null, $token = null, $guestName = null, bool $forceRecreate = false): DataResponse { + public function create($fileId = null, $filePath = null, $token = null, $guestName = null): DataResponse { try { /** @var File $file */ if ($token) { @@ -112,18 +111,16 @@ public function create($fileId = null, $filePath = null, $token = null, $guestNa $readOnly = $this->documentService->isReadOnly($file, $token); - $this->sessionService->removeInactiveSessions($file->getId()); - $remainingSessions = $this->sessionService->getAllSessions($file->getId()); - $freshSession = false; - if ($forceRecreate || count($remainingSessions) === 0) { - $freshSession = true; - try { - $this->documentService->resetDocument($file->getId(), $forceRecreate); - } catch (DocumentHasUnsavedChangesException $e) { - } - } + $this->sessionService->removeInactiveSessionsWithoutSteps($file->getId()); + $document = $this->documentService->getDocument($file); + $freshSession = $document === null; - $document = $this->documentService->createDocument($file); + if ($freshSession) { + $this->logger->info('Create new document of ' . $file->getId()); + $document = $this->documentService->createDocument($file); + } else { + $this->logger->info('Keep previous document of ' . $file->getId()); + } } catch (Exception $e) { $this->logger->error($e->getMessage(), ['exception' => $e]); return new DataResponse('Failed to create the document session', 500); @@ -132,20 +129,22 @@ public function create($fileId = null, $filePath = null, $token = null, $guestNa $session = $this->sessionService->initSession($document->getId(), $guestName); if ($freshSession) { - $this->logger->debug('Starting a fresh session'); + $this->logger->debug('Starting a fresh editing session for ' . $file->getId()); $documentState = null; $content = $this->loadContent($file); } else { - $this->logger->debug('Loading existing session'); + $this->logger->debug('Loading existing session for ' . $file->getId()); $content = null; try { $stateFile = $this->documentService->getStateFile($document->getId()); $documentState = $stateFile->getContent(); } catch (NotFoundException $e) { + $this->logger->debug('State file not found for ' . $file->getId()); $documentState = ''; // no state saved yet. // If there are no steps yet we might still need the content. $steps = $this->documentService->getSteps($document->getId(), 0); if (empty($steps)) { + $this->logger->debug('Empty steps, loading content for ' . $file->getId()); $content = $this->loadContent($file); } } @@ -173,14 +172,10 @@ public function create($fileId = null, $filePath = null, $token = null, $guestNa public function close($documentId, $sessionId, $sessionToken): DataResponse { $this->sessionService->closeSession($documentId, $sessionId, $sessionToken); - $this->sessionService->removeInactiveSessions($documentId); + $this->sessionService->removeInactiveSessionsWithoutSteps($documentId); $activeSessions = $this->sessionService->getActiveSessions($documentId); if (count($activeSessions) === 0) { - try { - $this->documentService->resetDocument($documentId); - $this->attachmentService->cleanupAttachments($documentId); - } catch (DocumentHasUnsavedChangesException $e) { - } + $this->documentService->unlock($documentId); } return new DataResponse([]); } diff --git a/lib/Service/DocumentService.php b/lib/Service/DocumentService.php index 4dc27ac8406..6c3462a53b4 100644 --- a/lib/Service/DocumentService.php +++ b/lib/Service/DocumentService.php @@ -112,6 +112,14 @@ public function __construct(DocumentMapper $documentMapper, StepMapper $stepMapp } } + public function getDocument(File $file): ?Document { + try { + return $this->documentMapper->find($file->getId()); + } catch (DoesNotExistException|NotFoundException $e) { + return null; + } + } + /** * @param File $file * @return Entity @@ -127,7 +135,7 @@ public function createDocument(File $file): Document { // This way the user can still resolve conflicts in the editor view $stepsVersion = $this->stepMapper->getLatestVersion($document->getId()); if ($stepsVersion && ($document->getLastSavedVersion() !== $stepsVersion)) { - $this->logger->debug('Unsaved steps but collission with file, continue collaborative editing'); + $this->logger->debug('Unsaved steps, continue collaborative editing'); return $document; } return $document; @@ -258,6 +266,7 @@ private function insertSteps($documentId, $sessionId, $steps, $version): int { } $stepsVersion = $this->stepMapper->getLatestVersion($document->getId()); $newVersion = $stepsVersion + count($steps); + $this->logger->debug("Adding steps to $documentId: bumping version from $stepsVersion to $newVersion"); $this->cache->set('document-version-' . $document->getId(), $newVersion); $step = new Step(); $step->setData($stepsJson); @@ -332,6 +341,22 @@ public function autosave(?File $file, int $documentId, int $version, ?string $au return $document; } + if (empty($autoaveDocument)) { + $this->logger->debug('Saving empty document', [ + 'requestVersion' => $version, + 'requestAutosaveDocument' => $autoaveDocument, + 'requestDocumentState' => $documentState, + 'document' => $document->jsonSerialize(), + 'fileSizeBeforeSave' => $file->getSize(), + 'steps' => array_map(function (Step $step) { + return $step->jsonSerialize(); + }, $this->stepMapper->find($documentId, 0)), + 'sessions' => array_map(function (Session $session) { + return $session->jsonSerialize(); + }, $this->sessionMapper->findAll($documentId)) + ]); + } + $this->cache->set('document-save-lock-' . $documentId, true, 10); try { $this->lockManager->runInScope(new LockContext( @@ -344,46 +369,49 @@ public function autosave(?File $file, int $documentId, int $version, ?string $au $this->writeDocumentState($file->getId(), $documentState); } }); + $document->setLastSavedVersion($stepsVersion); + $document->setLastSavedVersionTime(time()); + $document->setLastSavedVersionEtag($file->getEtag()); + $this->documentMapper->update($document); } catch (LockedException $e) { // Ignore lock since it might occur when multiple people save at the same time return $document; + } finally { + $this->cache->remove('document-save-lock-' . $documentId); } - $document->setLastSavedVersion($stepsVersion); - $document->setLastSavedVersionTime(time()); - $document->setLastSavedVersionEtag($file->getEtag()); - $this->documentMapper->update($document); - $this->cache->remove('document-save-lock-' . $documentId); return $document; } /** - * @param $documentId - * @param bool $force * @throws DocumentHasUnsavedChangesException + * @throws Exception + * @throws NotPermittedException */ public function resetDocument(int $documentId, bool $force = false): void { try { - $this->unlock($documentId); - $document = $this->documentMapper->find($documentId); - - if ($force || !$this->hasUnsavedChanges($document)) { - $this->stepMapper->deleteAll($documentId); - $this->sessionMapper->deleteByDocumentId($documentId); - $this->documentMapper->delete($document); - - try { - $this->getStateFile($documentId)->delete(); - } catch (NotFoundException $e) { - } catch (NotPermittedException $e) { - } - } elseif ($this->hasUnsavedChanges($document)) { + if (!$force && $this->hasUnsavedChanges($document)) { + $this->logger->debug('did not reset document for ' . $documentId); throw new DocumentHasUnsavedChangesException('Did not reset document, as it has unsaved changes'); } - } catch (DoesNotExistException $e) { + + $this->unlock($documentId); + + $this->stepMapper->deleteAll($documentId); + $this->sessionMapper->deleteByDocumentId($documentId); + $this->documentMapper->delete($document); + + $this->getStateFile($documentId)->delete(); + $this->logger->debug('document reset for ' . $documentId); + } catch (DoesNotExistException|NotFoundException $e) { + // Ignore if document not found or state file not found } } + public function getAll() { + return $this->documentMapper->findAll(); + } + /** * @param Session $session * @param $shareToken diff --git a/lib/Service/SessionService.php b/lib/Service/SessionService.php index 2b4e814079b..4bf0a5e105c 100644 --- a/lib/Service/SessionService.php +++ b/lib/Service/SessionService.php @@ -146,9 +146,9 @@ public function findAllInactive() { return $this->sessionMapper->findAllInactive(); } - public function removeInactiveSessions($documentId = -1) { + public function removeInactiveSessionsWithoutSteps($documentId = -1) { // No need to clear the cache here as we already set a TTL - return $this->sessionMapper->deleteInactive($documentId); + return $this->sessionMapper->deleteInactiveWithoutSteps($documentId); } /**