All: Fixes #5051: Fixed error that could prevent a revision from being created, and that would prevent the revision service from processing the rest of the notes

packages/lib/models/Note.ts

@@ -633,7 +633,7 @@ export default class Note extends BaseItem {
 		return n.updated_time < date;
 	}
 
-	static async save(o: NoteEntity, options: any = null) {
+	public static async save(o: NoteEntity, options: any = null): Promise<NoteEntity> {
 		const isNew = this.isNew(o, options);
 
 		// If true, this is a provisional note - it will be saved permanently

packages/lib/models/Revision.test.js

@@ -1,92 +0,0 @@
-const { setupDatabaseAndSynchronizer, switchClient } = require('../testing/test-utils.js');
-const Note = require('../models/Note').default;
-const Revision = require('../models/Revision').default;
-
-describe('models_Revision', function() {
-
-	beforeEach(async (done) => {
-		await setupDatabaseAndSynchronizer(1);
-		await switchClient(1);
-		done();
-	});
-
-	it('should create patches of text and apply it', (async () => {
-		const note1 = await Note.save({ body: 'my note\nsecond line' });
-
-		const patch = Revision.createTextPatch(note1.body, 'my new note\nsecond line');
-		const merged = Revision.applyTextPatch(note1.body, patch);
-
-		expect(merged).toBe('my new note\nsecond line');
-	}));
-
-	it('should create patches of objects and apply it', (async () => {
-		const oldObject = {
-			one: '123',
-			two: '456',
-			three: '789',
-		};
-
-		const newObject = {
-			one: '123',
-			three: '999',
-		};
-
-		const patch = Revision.createObjectPatch(oldObject, newObject);
-		const merged = Revision.applyObjectPatch(oldObject, patch);
-
-		expect(JSON.stringify(merged)).toBe(JSON.stringify(newObject));
-	}));
-
-	it('should move target revision to the top', (async () => {
-		const revs = [
-			{ id: '123' },
-			{ id: '456' },
-			{ id: '789' },
-		];
-
-		let newRevs;
-		newRevs = Revision.moveRevisionToTop({ id: '456' }, revs);
-		expect(newRevs[0].id).toBe('123');
-		expect(newRevs[1].id).toBe('789');
-		expect(newRevs[2].id).toBe('456');
-
-		newRevs = Revision.moveRevisionToTop({ id: '789' }, revs);
-		expect(newRevs[0].id).toBe('123');
-		expect(newRevs[1].id).toBe('456');
-		expect(newRevs[2].id).toBe('789');
-	}));
-
-	it('should create patch stats', (async () => {
-		const tests = [
-			{
-				patch: `@@ -625,16 +625,48 @@
- rrupted download
-+%0A- %5B %5D Fix mobile screen options`,
-				expected: [-0, +32],
-			},
-			{
-				patch: `@@ -564,17 +564,17 @@
- ages%0A- %5B
-- 
-+x
- %5D Check `,
-				expected: [-1, +1],
-			},
-			{
-				patch: `@@ -1022,56 +1022,415 @@
- .%0A%0A#
-- How to view a note history%0A%0AWhile all the apps 
-+%C2%A0How does it work?%0A%0AAll the apps save a version of the modified notes every 10 minutes.
- %0A%0A# `,
-				expected: [-(19 + 27 + 2), 17 + 67 + 4],
-			},
-		];
-
-		for (const test of tests) {
-			const stats = Revision.patchStats(test.patch);
-			expect(stats.removed).toBe(-test.expected[0]);
-			expect(stats.added).toBe(test.expected[1]);
-		}
-	}));
-
-});

packages/lib/models/Revision.test.ts

@@ -0,0 +1,194 @@
+import { expectNotThrow, naughtyStrings, setupDatabaseAndSynchronizer, switchClient } from '../testing/test-utils';
+import Note from '../models/Note';
+import Revision from '../models/Revision';
+
+describe('models/Revision', function() {
+
+	beforeEach(async (done) => {
+		await setupDatabaseAndSynchronizer(1);
+		await switchClient(1);
+		done();
+	});
+
+	it('should create patches of text and apply it', (async () => {
+		const note1 = await Note.save({ body: 'my note\nsecond line' });
+
+		const patch = Revision.createTextPatch(note1.body, 'my new note\nsecond line');
+		const merged = Revision.applyTextPatch(note1.body, patch);
+
+		expect(merged).toBe('my new note\nsecond line');
+	}));
+
+	it('should check if it is an empty revision', async () => {
+		const testCases = [
+			[false, {
+				title_diff: '',
+				body_diff: '',
+				metadata_diff: '{"new":{"id":"aaa"},"deleted":[]}',
+			}],
+			[true, {
+				title_diff: '',
+				body_diff: '',
+				metadata_diff: '',
+			}],
+			[true, {
+				title_diff: '[]',
+				body_diff: '',
+				metadata_diff: '{"new":{},"deleted":[]}',
+			}],
+			[true, {
+				title_diff: '',
+				body_diff: '[]',
+				metadata_diff: '{"new":{},"deleted":[]}',
+			}],
+			[false, {
+				title_diff: '[{"diffs":[[1,"hello"]],"start1":0,"start2":0,"length1":0,"length2":5}]',
+				body_diff: '[]',
+				metadata_diff: '{"new":{},"deleted":[]}',
+			}],
+		];
+
+		for (const t of testCases) {
+			const [expected, input] = t;
+			expect(Revision.isEmptyRevision(input as any)).toBe(expected);
+		}
+	});
+
+	it('should not fail to create revisions on naughty strings', (async () => {
+		// Previously this pattern would fail:
+		// - Create a patch between an empty string and smileys
+		// - Use that patch on the empty string to get back the smileys
+		// - Create a patch between those smileys and new smileys
+		// https://github.com/JackuB/diff-match-patch/issues/22
+
+		const nss = await naughtyStrings();
+
+		// First confirm that it indeed fails with the legacy approach.
+		let errorCount = 0;
+
+		for (let i = 0; i < nss.length - 1; i++) {
+			const ns1 = nss[i];
+			const ns2 = nss[i + 1];
+			try {
+				const patchText = Revision.createTextPatchLegacy('', ns1);
+				const patchedText = Revision.applyTextPatchLegacy('', patchText);
+				Revision.createTextPatchLegacy(patchedText, ns2);
+			} catch (error) {
+				errorCount++;
+			}
+		}
+
+		expect(errorCount).toBe(10);
+
+		// Now feed the naughty list again but using the new approach. In that
+		// case it should work fine.
+		await expectNotThrow(async () => {
+			for (let i = 0; i < nss.length - 1; i++) {
+				const ns1 = nss[i];
+				const ns2 = nss[i + 1];
+				const patchText = Revision.createTextPatch('', ns1);
+				const patchedText = Revision.applyTextPatch('', patchText);
+				Revision.createTextPatch(patchedText, ns2);
+			}
+		});
+	}));
+
+	it('should successfully handle legacy patches', async () => {
+		// The code should handle applying a series of new style patches and
+		// legacy patches, and the correct text should be recovered at the end.
+		const changes = [
+			'',
+			'one',
+			'one three',
+			'one two three',
+		];
+
+		const patches = [
+			Revision.createTextPatch(changes[0], changes[1]),
+			Revision.createTextPatchLegacy(changes[1], changes[2]),
+			Revision.createTextPatch(changes[2], changes[3]),
+		];
+
+		// Sanity check - verify that the patches are as expected
+		expect(patches[0].substr(0, 2)).toBe('[{'); // New
+		expect(patches[1].substr(0, 2)).toBe('@@'); // Legacy
+		expect(patches[2].substr(0, 2)).toBe('[{'); // New
+
+		let finalString = Revision.applyTextPatch(changes[0], patches[0]);
+		finalString = Revision.applyTextPatch(finalString, patches[1]);
+		finalString = Revision.applyTextPatch(finalString, patches[2]);
+
+		expect(finalString).toBe('one two three');
+	});
+
+	it('should create patches of objects and apply it', (async () => {
+		const oldObject = {
+			one: '123',
+			two: '456',
+			three: '789',
+		};
+
+		const newObject = {
+			one: '123',
+			three: '999',
+		};
+
+		const patch = Revision.createObjectPatch(oldObject, newObject);
+		const merged = Revision.applyObjectPatch(oldObject, patch);
+
+		expect(JSON.stringify(merged)).toBe(JSON.stringify(newObject));
+	}));
+
+	it('should move target revision to the top', (async () => {
+		const revs = [
+			{ id: '123' },
+			{ id: '456' },
+			{ id: '789' },
+		];
+
+		let newRevs;
+		newRevs = Revision.moveRevisionToTop({ id: '456' }, revs);
+		expect(newRevs[0].id).toBe('123');
+		expect(newRevs[1].id).toBe('789');
+		expect(newRevs[2].id).toBe('456');
+
+		newRevs = Revision.moveRevisionToTop({ id: '789' }, revs);
+		expect(newRevs[0].id).toBe('123');
+		expect(newRevs[1].id).toBe('456');
+		expect(newRevs[2].id).toBe('789');
+	}));
+
+	it('should create patch stats', (async () => {
+		const tests = [
+			{
+				patch: `@@ -625,16 +625,48 @@
+ rrupted download
++%0A- %5B %5D Fix mobile screen options`,
+				expected: [-0, +32],
+			},
+			{
+				patch: `@@ -564,17 +564,17 @@
+ ages%0A- %5B
+- 
++x
+ %5D Check `,
+				expected: [-1, +1],
+			},
+			{
+				patch: `@@ -1022,56 +1022,415 @@
+ .%0A%0A#
+- How to view a note history%0A%0AWhile all the apps 
++%C2%A0How does it work?%0A%0AAll the apps save a version of the modified notes every 10 minutes.
+ %0A%0A# `,
+				expected: [-(19 + 27 + 2), 17 + 67 + 4],
+			},
+		];
+
+		for (const test of tests) {
+			const stats = Revision.patchStats(test.patch);
+			expect(stats.removed).toBe(-test.expected[0]);
+			expect(stats.added).toBe(test.expected[1]);
+		}
+	}));
+
+});

packages/lib/models/Revision.ts

@@ -17,17 +17,54 @@ export default class Revision extends BaseItem {
 		return BaseModel.TYPE_REVISION;
 	}
 
-	static createTextPatch(oldText: string, newText: string) {
+	public static createTextPatchLegacy(oldText: string, newText: string): string {
 		return dmp.patch_toText(dmp.patch_make(oldText, newText));
 	}
 
-	static applyTextPatch(text: string, patch: string) {
+	public static createTextPatch(oldText: string, newText: string): string {
+		return JSON.stringify(dmp.patch_make(oldText, newText));
+	}
+
+	public static applyTextPatchLegacy(text: string, patch: string): string {
 		patch = dmp.patch_fromText(patch);
 		const result = dmp.patch_apply(patch, text);
 		if (!result || !result.length) throw new Error('Could not apply patch');
 		return result[0];
 	}
 
+	private static isLegacyPatch(patch: string): boolean {
+		return patch && patch.indexOf('@@') === 0;
+	}
+
+	private static isNewPatch(patch: string): boolean {
+		if (!patch) return true;
+		return patch.indexOf('[{') === 0;
+	}
+
+	public static applyTextPatch(text: string, patch: string): string {
+		if (this.isLegacyPatch(patch)) {
+			return this.applyTextPatchLegacy(text, patch);
+		} else {
+			const result = dmp.patch_apply(JSON.parse(patch), text);
+			if (!result || !result.length) throw new Error('Could not apply patch');
+			return result[0];
+		}
+	}
+
+	public static isEmptyRevision(rev: RevisionEntity): boolean {
+		if (this.isLegacyPatch(rev.title_diff) && rev.title_diff) return false;
+		if (this.isLegacyPatch(rev.body_diff) && rev.body_diff) return false;
+
+		if (this.isNewPatch(rev.title_diff) && rev.title_diff && rev.title_diff !== '[]') return false;
+		if (this.isNewPatch(rev.body_diff) && rev.body_diff && rev.body_diff !== '[]') return false;
+
+		const md = rev.metadata_diff ? JSON.parse(rev.metadata_diff) : {};
+		if (md.new && Object.keys(md.new).length) return false;
+		if (md.deleted && Object.keys(md.deleted).length) return false;
+
+		return true;
+	}
+
 	static createObjectPatch(oldObject: any, newObject: any) {
 		if (!oldObject) oldObject = {};