-
Notifications
You must be signed in to change notification settings - Fork 91
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
Ensure document stays consistent with y.js state file #5477
Conversation
$steps = $this->documentService->getSteps($document->getId(), 0); | ||
if (empty($steps)) { | ||
$this->logger->debug('Empty steps, loading content for ' . $file->getId()); | ||
$content = $this->loadContent($file); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be another cause of #5420 and is not needed as we only want to load from the file content on the initial session creation
lib/Service/DocumentService.php
Outdated
@@ -422,11 +422,12 @@ public function resetDocument(int $documentId, bool $force = false): void { | |||
|
|||
$this->unlock($documentId); | |||
|
|||
$this->stepMapper->deleteAll($documentId); | |||
$this->stepMapper->deleteBeforeVersion($documentId, $document->getLastSavedVersion()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should still be fine as a cleanup as all steps in the saved version are also persisted in the y.js state, right @max-nextcloud ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually changed the logic of resetDocument()
in #5481 to always delete all session data: document, sessions and steps from database as well as yjs document state file. And that will happen in the following scenarios:
- A document gets changed from outside text and there's no unsaved steps
- Cleanup background job runs for a stale document session (and no unsaved steps)
- occ command
text:reset
is run
I wonder whether the second case should just remove sessions and steps instead.
1e10b1a
to
38f3d79
Compare
Need to check the failing test:
I feel we might want to adjust that to account for the changed behaviour. |
950b633
to
e28cf61
Compare
e28cf61
to
aee2afa
Compare
Signed-off-by: Julius Härtl <[email protected]>
Signed-off-by: Julius Härtl <[email protected]>
aee2afa
to
a63daee
Compare
I changed the cypress test to expect empty file content on I also did some manual testing with removing the session connections (from |
Contributes to #5476