diff --git a/packages/lib/models/Revision.ts b/packages/lib/models/Revision.ts index 748771e0f..7ac17dd56 100644 --- a/packages/lib/models/Revision.ts +++ b/packages/lib/models/Revision.ts @@ -1,5 +1,5 @@ import BaseModel, { ModelType } from '../BaseModel'; -import { RevisionEntity } from '../services/database/types'; +import { RevisionEntity, StringOrSqlQuery } from '../services/database/types'; import BaseItem from './BaseItem'; const DiffMatchPatch = require('diff-match-patch'); import * as ArrayUtils from '../ArrayUtils'; @@ -305,43 +305,71 @@ export default class Revision extends BaseItem { // and modify that revision into a "merged" one. const cutOffDate = Date.now() - ttl; - const revisions = await this.modelSelectAll('SELECT * FROM revisions WHERE item_updated_time < ? ORDER BY item_updated_time DESC', [cutOffDate]); - const doneItems: Record = {}; + const allOldRevisions: RevisionEntity[] = await this.modelSelectAll( + 'SELECT * FROM revisions WHERE item_updated_time < ? ORDER BY item_updated_time DESC', + [cutOffDate], + ); - for (const rev of revisions) { - const doneKey = `${rev.item_type}_${rev.item_id}`; - if (doneItems[doneKey]) continue; + const itemIdToOldRevisions = new Map(); + for (const rev of allOldRevisions) { + const itemId = rev.item_id; + if (!itemIdToOldRevisions.has(itemId)) { + itemIdToOldRevisions.set(itemId, []); + } + itemIdToOldRevisions.get(itemId).push(rev); + } - const keptRev = await this.modelSelectOne('SELECT * FROM revisions WHERE item_updated_time >= ? AND item_type = ? AND item_id = ? ORDER BY item_updated_time ASC LIMIT 1', [cutOffDate, rev.item_type, rev.item_id]); + const deleteOldRevisionsForItem = async (itemType: ModelType, itemId: string, oldRevisions: RevisionEntity[]) => { + const keptRev = await this.modelSelectOne( + 'SELECT * FROM revisions WHERE item_updated_time >= ? AND item_type = ? AND item_id = ? ORDER BY item_updated_time ASC LIMIT 1', + [cutOffDate, itemType, itemId], + ); + const queries: StringOrSqlQuery[] = []; + if (!keptRev) { + const hasEncrypted = await this.modelSelectOne( + 'SELECT * FROM revisions WHERE encryption_applied = 1 AND item_updated_time < ? AND item_id = ?', + [cutOffDate, itemId], + ); + if (hasEncrypted) { + throw new JoplinError('One of the revision to be deleted is encrypted', 'revision_encrypted'); + } + } else { + // Note: we don't need to check for encrypted rev here because + // mergeDiff will already throw the revision_encrypted exception + // if a rev is encrypted. + const merged = await this.mergeDiffs(keptRev); + + const titleDiff = this.createTextPatch('', merged.title); + const bodyDiff = this.createTextPatch('', merged.body); + const metadataDiff = this.createObjectPatch({}, merged.metadata); + queries.push({ + sql: 'UPDATE revisions SET title_diff = ?, body_diff = ?, metadata_diff = ? WHERE id = ?', + params: [titleDiff, bodyDiff, metadataDiff, keptRev.id], + }); + } + + await this.batchDelete(oldRevisions.map(item => item.id), { sourceDescription: 'Revision.deleteOldRevisions' }); + if (queries.length) { + await this.db().transactionExecBatch(queries); + } + }; + + for (const [itemId, oldRevisions] of itemIdToOldRevisions.entries()) { + if (!oldRevisions.length) { + throw new Error('Invalid state: There must be at least one old revision per item to be processed.'); + } + + const latestOldRevision = oldRevisions[oldRevisions.length - 1]; try { - const deleteQueryCondition = 'item_updated_time < ? AND item_id = ?'; - const deleteQueryParams = [cutOffDate, rev.item_id]; - const deleteQuery = { sql: `DELETE FROM revisions WHERE ${deleteQueryCondition}`, params: deleteQueryParams }; - - if (!keptRev) { - const hasEncrypted = await this.modelSelectOne(`SELECT * FROM revisions WHERE encryption_applied = 1 AND ${deleteQueryCondition}`, deleteQueryParams); - if (hasEncrypted) throw new JoplinError('One of the revision to be deleted is encrypted', 'revision_encrypted'); - await this.db().transactionExecBatch([deleteQuery]); - } else { - // Note: we don't need to check for encrypted rev here because - // mergeDiff will already throw the revision_encrypted exception - // if a rev is encrypted. - const merged = await this.mergeDiffs(keptRev); - - const queries = [deleteQuery, { sql: 'UPDATE revisions SET title_diff = ?, body_diff = ?, metadata_diff = ? WHERE id = ?', params: [this.createTextPatch('', merged.title), this.createTextPatch('', merged.body), this.createObjectPatch({}, merged.metadata), keptRev.id] }]; - - await this.db().transactionExecBatch(queries); - } + await deleteOldRevisionsForItem(latestOldRevision.item_type, itemId, oldRevisions); } catch (error) { if (error.code === 'revision_encrypted') { - this.logger().info(`Aborted deletion of old revisions for item "${rev.item_id}" (rev "${rev.id}") because one of the revisions is still encrypted`, error); + this.logger().info(`Aborted deletion of old revisions for item "${itemId}" (latest old rev "${latestOldRevision.id}") because one of the revisions is still encrypted`, error); } else { throw error; } } - - doneItems[doneKey] = true; } } diff --git a/packages/lib/services/RevisionService.test.ts b/packages/lib/services/RevisionService.test.ts index 955b641bc..14e70ebab 100644 --- a/packages/lib/services/RevisionService.test.ts +++ b/packages/lib/services/RevisionService.test.ts @@ -6,6 +6,32 @@ import Revision from '../models/Revision'; import BaseModel, { ModelType } from '../BaseModel'; import RevisionService from '../services/RevisionService'; import { MarkupLanguage } from '../../renderer'; +import { NoteEntity } from './database/types'; + +interface CreateTestRevisionOptions { + // How long to pause (in milliseconds) between each note modification. + // For example, [10, 20] would modify the note twice, with pauses of 10ms and 20ms. + delaysBetweenModifications: number[]; +} + +const createTestRevisions = async ( + noteProperties: Partial, + { delaysBetweenModifications }: CreateTestRevisionOptions, +) => { + const note = await Note.save({ + title: 'note', + ...noteProperties, + }); + + let counter = 0; + for (const delay of delaysBetweenModifications) { + jest.advanceTimersByTime(delay); + await Note.save({ ...noteProperties, id: note.id, title: `note REV${counter++}` }); + await revisionService().collectRevisions(); + } + + return note; +}; describe('services/RevisionService', () => { @@ -13,6 +39,8 @@ describe('services/RevisionService', () => { await setupDatabaseAndSynchronizer(1); await switchClient(1); Setting.setValue('revisionService.intervalBetweenRevisions', 0); + + jest.useFakeTimers({ advanceTimers: true }); }); it('should create diff and rebuild notes', (async () => { @@ -185,6 +213,25 @@ describe('services/RevisionService', () => { } })); + it('should not error on revisions for missing (not downloaded yet/permanently deleted) notes', async () => { + Setting.setValue('revisionService.intervalBetweenRevisions', 100); + + const note = await createTestRevisions({ + share_id: 'test-share-id', + }, { delaysBetweenModifications: [200, 400, 600, 8_000] }); + const getNoteRevisions = () => { + return Revision.allByType(BaseModel.TYPE_NOTE, note.id); + }; + expect(await getNoteRevisions()).toHaveLength(4); + + await Note.delete(note.id, { toTrash: false, sourceDescription: 'tests/RevisionService' }); + + await revisionService().deleteOldRevisions(4_000); + + // Should leave newer revisions (handle the case where revisions are downloaded before the note). + expect(await getNoteRevisions()).toHaveLength(1); + }); + it('should handle conflicts', (async () => { const service = new RevisionService(); diff --git a/packages/lib/services/synchronizer/Synchronizer.revisions.test.ts b/packages/lib/services/synchronizer/Synchronizer.revisions.test.ts index 1bb6cbaa9..729d9cc7d 100644 --- a/packages/lib/services/synchronizer/Synchronizer.revisions.test.ts +++ b/packages/lib/services/synchronizer/Synchronizer.revisions.test.ts @@ -181,4 +181,53 @@ describe('Synchronizer.revisions', () => { expect((await Revision.all()).length).toBe(0); })); + it('should delete old revisions remotely when deleted locally', async () => { + Setting.setValue('revisionService.intervalBetweenRevisions', 100); + jest.useFakeTimers({ advanceTimers: true }); + + const note = await Note.save({ title: 'note' }); + const getNoteRevisions = () => { + return Revision.allByType(BaseModel.TYPE_NOTE, note.id); + }; + jest.advanceTimersByTime(200); + + await Note.save({ id: note.id, title: 'note REV0' }); + jest.advanceTimersByTime(200); + + await revisionService().collectRevisions(); // REV0 + expect(await getNoteRevisions()).toHaveLength(1); + + jest.advanceTimersByTime(200); + + await Note.save({ id: note.id, title: 'note REV1' }); + await revisionService().collectRevisions(); // REV1 + expect(await getNoteRevisions()).toHaveLength(2); + + // Should sync the revisions + await synchronizer().start(); + await switchClient(2); + await synchronizer().start(); + + expect(await getNoteRevisions()).toHaveLength(2); + await revisionService().deleteOldRevisions(100); + expect(await getNoteRevisions()).toHaveLength(0); + + await synchronizer().start(); + expect(await getNoteRevisions()).toHaveLength(0); + + // Syncing a new client should not download the deleted revisions + await setupDatabaseAndSynchronizer(3); + await switchClient(3); + await synchronizer().start(); + expect(await getNoteRevisions()).toHaveLength(0); + + // After switching back to the original client, syncing should locally delete + // the remotely deleted revisions. + await switchClient(1); + expect(await getNoteRevisions()).toHaveLength(2); + await synchronizer().start(); + expect(await getNoteRevisions()).toHaveLength(0); + + jest.useRealTimers(); + }); });