From 377cd82f2039f27658fc11c60b1b6322fc53a70d Mon Sep 17 00:00:00 2001 From: Eric Mc Sween <5454374+emcsween@users.noreply.github.com> Date: Tue, 13 Feb 2024 08:15:43 -0500 Subject: [PATCH] Merge pull request #16928 from overleaf/em-hpos-hlen Add history metadata to updates sent to project-history GitOrigin-RevId: 915beaa01f2bbe48869a40b229397be8e0401852 --- .../document-updater/app/js/RangesManager.js | 218 +++++++- .../document-updater/app/js/RedisManager.js | 21 +- .../document-updater/app/js/UpdateManager.js | 95 +++- services/document-updater/app/js/Utils.js | 40 ++ services/document-updater/app/js/types.ts | 89 ++++ .../js/ApplyingUpdatesToADocTests.js | 93 ++-- .../test/acceptance/js/RangesTests.js | 464 ++++++++--------- .../js/RangesManager/RangesManagerTests.js | 474 +++++++++--------- .../unit/js/RedisManager/RedisManagerTests.js | 45 +- .../js/UpdateManager/UpdateManagerTests.js | 63 ++- 10 files changed, 951 insertions(+), 651 deletions(-) create mode 100644 services/document-updater/app/js/Utils.js create mode 100644 services/document-updater/app/js/types.ts diff --git a/services/document-updater/app/js/RangesManager.js b/services/document-updater/app/js/RangesManager.js index 0a114e7bc9..b0a399f8b0 100644 --- a/services/document-updater/app/js/RangesManager.js +++ b/services/document-updater/app/js/RangesManager.js @@ -1,7 +1,26 @@ +// @ts-check + const RangesTracker = require('@overleaf/ranges-tracker') const logger = require('@overleaf/logger') +const OError = require('@overleaf/o-error') const Metrics = require('./Metrics') const _ = require('lodash') +const { isInsert, isDelete, isComment } = require('./Utils') + +/** + * @typedef {import('./types').CommentOp} CommentOp + * @typedef {import('./types').DeleteOp} DeleteOp + * @typedef {import('./types').HistoryCommentOp} HistoryCommentOp + * @typedef {import('./types').HistoryDeleteOp} HistoryDeleteOp + * @typedef {import('./types').HistoryInsertOp} HistoryInsertOp + * @typedef {import('./types').HistoryOp} HistoryOp + * @typedef {import('./types').HistoryUpdate} HistoryUpdate + * @typedef {import('./types').InsertOp} InsertOp + * @typedef {import('./types').Op} Op + * @typedef {import('./types').Ranges} Ranges + * @typedef {import('./types').TrackedChange} TrackedChange + * @typedef {import('./types').Update} Update + */ const RANGE_DELTA_BUCKETS = [0, 1, 2, 3, 4, 5, 10, 20, 50] @@ -9,25 +28,39 @@ const RangesManager = { MAX_COMMENTS: 500, MAX_CHANGES: 2000, - applyUpdate(projectId, docId, entries, updates, newDocLines) { - if (entries == null) { - entries = {} + /** + * Apply an update to the given doc (lines and ranges) and return new ranges + * + * @param {string} projectId + * @param {string} docId + * @param {Ranges} ranges - ranges before the updates were applied + * @param {Update[]} updates + * @param {string[]} newDocLines - the document lines after the updates were applied + * @returns {{ newRanges: Ranges, rangesWereCollapsed: boolean, historyUpdates: HistoryUpdate[] }} + */ + applyUpdate(projectId, docId, ranges, updates, newDocLines) { + if (ranges == null) { + ranges = {} } if (updates == null) { updates = [] } - const { changes, comments } = _.cloneDeep(entries) + const { changes, comments } = _.cloneDeep(ranges) const rangesTracker = new RangesTracker(changes, comments) const [emptyRangeCountBefore, totalRangeCountBefore] = RangesManager._emptyRangesCount(rangesTracker) + const historyUpdates = [] for (const update of updates) { - rangesTracker.track_changes = !!update.meta.tc - if (update.meta.tc) { + rangesTracker.track_changes = Boolean(update.meta?.tc) + if (update.meta?.tc) { rangesTracker.setIdSeed(update.meta.tc) } + const historyOps = [] for (const op of update.op) { + historyOps.push(getHistoryOp(op, rangesTracker.changes)) rangesTracker.applyOp(op, { user_id: update.meta?.user_id }) } + historyUpdates.push({ ...update, op: historyOps }) } if ( @@ -74,7 +107,7 @@ const RangesManager = { }, 'applied updates to ranges' ) - return { newRanges, rangesWereCollapsed } + return { newRanges, rangesWereCollapsed, historyUpdates } }, acceptChanges(changeIds, ranges) { @@ -98,17 +131,12 @@ const RangesManager = { _getRanges(rangesTracker) { // Return the minimal data structure needed, since most documents won't have any // changes or comments - let response = {} + + const response = {} if (rangesTracker.changes != null && rangesTracker.changes.length > 0) { - if (response == null) { - response = {} - } response.changes = rangesTracker.changes } if (rangesTracker.comments != null && rangesTracker.comments.length > 0) { - if (response == null) { - response = {} - } response.comments = rangesTracker.comments } return response @@ -135,4 +163,166 @@ const RangesManager = { }, } +/** + * Calculate ops to be sent to the history system. + * + * @param {Op} op - the editor op + * @param {TrackedChange[]} changes - the list of tracked changes in the + * document before the op is applied. That list, coming from + * RangesTracker is ordered by position. + * @returns {HistoryOp} + */ +function getHistoryOp(op, changes, opts = {}) { + if (isInsert(op)) { + return getHistoryOpForInsert(op, changes) + } else if (isDelete(op)) { + return getHistoryOpForDelete(op, changes) + } else if (isComment(op)) { + return getHistoryOpForComment(op, changes) + } else { + throw new OError('Unrecognized op', { op }) + } +} + +/** + * Calculate history ops for an insert + * + * Inserts are moved forward by tracked deletes placed strictly before the + * op. When an insert is made at the same position as a tracked delete, the + * insert is placed before the tracked delete. + * + * @param {InsertOp} op + * @param {TrackedChange[]} changes + * @returns {HistoryInsertOp} + */ +function getHistoryOpForInsert(op, changes) { + let hpos = op.p + let trackedDeleteRejection = false + for (const change of changes) { + if (!isDelete(change.op)) { + // We're only interested in tracked deletes + continue + } + + if (change.op.p < op.p) { + // Tracked delete is before the op. Move the op forward. + hpos += change.op.d.length + } else if (change.op.p === op.p) { + // Tracked delete is at the same position as the op. The insert comes before + // the tracked delete so it doesn't move. + if (op.u && change.op.d.startsWith(op.i)) { + // We're undoing and the insert matches the start of the tracked + // delete. RangesManager treats this as a tracked delete rejection. We + // will note this in the op so that project-history can take the + // appropriate action. + trackedDeleteRejection = true + } + } else { + // Tracked delete is after the insert. Tracked deletes are ordered, so + // we know that all subsequent tracked deletes will be after the insert + // and we can bail out. + break + } + } + + /** @type {HistoryInsertOp} */ + const historyOp = { ...op } + if (hpos !== op.p) { + historyOp.hpos = hpos + } + if (trackedDeleteRejection) { + historyOp.trackedDeleteRejection = true + } + return historyOp +} + +/** + * Calculate history op for a delete + * + * Deletes are moved forward by tracked deletes placed before or at the position of the + * op. If a tracked delete is inside the delete, the delete is split in parts + * so that characters are deleted around the tracked delete, but the tracked + * delete itself is not deleted. + * + * @param {DeleteOp} op + * @param {TrackedChange[]} changes + * @returns {HistoryDeleteOp} + */ +function getHistoryOpForDelete(op, changes, opts = {}) { + let hpos = op.p + const hsplits = [] + for (const change of changes) { + if (!isDelete(change.op)) { + // We're only interested in tracked deletes + continue + } + + if (change.op.p <= op.p) { + // Tracked delete is before or at the position of the incoming delete. + // Move the op forward. + hpos += change.op.d.length + } else if (change.op.p < op.p + op.d.length) { + // Tracked delete inside the deleted text. Record a split for the history system. + hsplits.push({ offset: change.op.p - op.p, length: change.op.d.length }) + } else { + // We've seen all tracked deletes before or inside the delete + break + } + } + + /** @type {HistoryDeleteOp} */ + const historyOp = { ...op } + if (hpos !== op.p) { + historyOp.hpos = hpos + } + if (hsplits.length > 0) { + historyOp.hsplits = hsplits + } + return historyOp +} + +/** + * Calculate history ops for a comment + * + * Comments are moved forward by tracked deletes placed before or at the + * position of the op. If a tracked delete is inside the comment, the length of + * the comment is extended to include the tracked delete. + * + * @param {CommentOp} op + * @param {TrackedChange[]} changes + * @returns {HistoryCommentOp} + */ +function getHistoryOpForComment(op, changes) { + let hpos = op.p + let hlen = op.c.length + for (const change of changes) { + if (!isDelete(change.op)) { + // We're only interested in tracked deletes + continue + } + + if (change.op.p <= op.p) { + // Tracked delete is before or at the position of the incoming comment. + // Move the op forward. + hpos += change.op.d.length + } else if (change.op.p < op.p + op.c.length) { + // Tracked comment inside the comment. Extend the length + hlen += change.op.d.length + } else { + // We've seen all tracked deletes before or inside the comment + break + } + } + + /** @type {HistoryCommentOp} */ + const historyOp = { ...op } + if (hpos !== op.p) { + historyOp.hpos = hpos + } + if (hlen !== op.c.length) { + historyOp.hlen = hlen + } + return historyOp +} + module.exports = RangesManager diff --git a/services/document-updater/app/js/RedisManager.js b/services/document-updater/app/js/RedisManager.js index c67416ca0a..7cd038794d 100644 --- a/services/document-updater/app/js/RedisManager.js +++ b/services/document-updater/app/js/RedisManager.js @@ -8,7 +8,6 @@ const metrics = require('./Metrics') const Errors = require('./Errors') const crypto = require('crypto') const async = require('async') -const ProjectHistoryRedisManager = require('./ProjectHistoryRedisManager') const { docIsTooLarge } = require('./Limits') // Sometimes Redis calls take an unexpectedly long time. We have to be @@ -477,25 +476,7 @@ const RedisManager = { if (error) { return callback(error) } - - if (jsonOps.length > 0) { - metrics.inc('history-queue', 1, { status: 'project-history' }) - ProjectHistoryRedisManager.queueOps( - projectId, - ...jsonOps, - (error, projectUpdateCount) => { - if (error) { - // The full project history can re-sync a project in case - // updates went missing. - // Just record the error here and acknowledge the write-op. - metrics.inc('history-queue-error') - } - callback(null, projectUpdateCount) - } - ) - } else { - callback(null) - } + callback() }) }) }) diff --git a/services/document-updater/app/js/UpdateManager.js b/services/document-updater/app/js/UpdateManager.js index 5c812379a3..f38b53e851 100644 --- a/services/document-updater/app/js/UpdateManager.js +++ b/services/document-updater/app/js/UpdateManager.js @@ -3,6 +3,7 @@ const { callbackifyAll } = require('@overleaf/promise-utils') const LockManager = require('./LockManager') const RedisManager = require('./RedisManager') +const ProjectHistoryRedisManager = require('./ProjectHistoryRedisManager') const RealTimeRedisManager = require('./RealTimeRedisManager') const ShareJsUpdateManager = require('./ShareJsUpdateManager') const HistoryManager = require('./HistoryManager') @@ -14,6 +15,15 @@ const DocumentManager = require('./DocumentManager') const RangesManager = require('./RangesManager') const SnapshotManager = require('./SnapshotManager') const Profiler = require('./Profiler') +const { isInsert, isDelete } = require('./Utils') + +/** + * @typedef {import("./types").DeleteOp} DeleteOp + * @typedef {import("./types").InsertOp} InsertOp + * @typedef {import("./types").Op} Op + * @typedef {import("./types").Ranges} Ranges + * @typedef {import("./types").Update} Update + */ const UpdateManager = { async processOutstandingUpdates(projectId, docId) { @@ -82,6 +92,13 @@ const UpdateManager = { profile.log('async done').end() }, + /** + * Apply an update to the given document + * + * @param {string} projectId + * @param {string} docId + * @param {Update} update + */ async applyUpdate(projectId, docId, update) { const profile = new Profiler('applyUpdate', { project_id: projectId, @@ -92,8 +109,14 @@ const UpdateManager = { profile.log('sanitizeUpdate', { sync: true }) try { - let { lines, version, ranges, pathname, projectHistoryId } = - await DocumentManager.promises.getDoc(projectId, docId) + let { + lines, + version, + ranges, + pathname, + projectHistoryId, + historyRangesSupport, + } = await DocumentManager.promises.getDoc(projectId, docId) profile.log('getDoc') if (lines == null || version == null) { @@ -117,13 +140,20 @@ const UpdateManager = { sync: incomingUpdateVersion === previousVersion, }) - const { newRanges, rangesWereCollapsed } = RangesManager.applyUpdate( - projectId, - docId, - ranges, - appliedOps, - updatedDocLines - ) + let { newRanges, rangesWereCollapsed, historyUpdates } = + RangesManager.applyUpdate( + projectId, + docId, + ranges, + appliedOps, + updatedDocLines + ) + if (!historyRangesSupport) { + // The document has not been transitioned to include comments and + // tracked changes in its history. Send regular updates rather than the + // full history updates. + historyUpdates = appliedOps + } profile.log('RangesManager.applyUpdate', { sync: true }) UpdateManager._addProjectHistoryMetadataToOps( @@ -132,8 +162,7 @@ const UpdateManager = { projectHistoryId, lines ) - - const projectOpsLength = await RedisManager.promises.updateDocument( + await RedisManager.promises.updateDocument( projectId, docId, updatedDocLines, @@ -144,12 +173,27 @@ const UpdateManager = { ) profile.log('RedisManager.updateDocument') - HistoryManager.recordAndFlushHistoryOps( - projectId, - appliedOps, - projectOpsLength - ) - profile.log('recordAndFlushHistoryOps') + if (historyUpdates.length > 0) { + Metrics.inc('history-queue', 1, { status: 'project-history' }) + try { + const projectOpsLength = + await ProjectHistoryRedisManager.promises.queueOps( + projectId, + ...historyUpdates.map(op => JSON.stringify(op)) + ) + HistoryManager.recordAndFlushHistoryOps( + projectId, + historyUpdates, + projectOpsLength + ) + profile.log('recordAndFlushHistoryOps') + } catch (err) { + // The full project history can re-sync a project in case + // updates went missing. + // Just record the error here and acknowledge the write-op. + Metrics.inc('history-queue-error') + } + } if (rangesWereCollapsed) { Metrics.inc('doc-snapshot') @@ -259,10 +303,19 @@ const UpdateManager = { return update }, + /** + * Add metadata to ops that will be useful to project history + * + * @param {Update[]} updates + * @param {string} pathname + * @param {string} projectHistoryId + * @param {string[]} lines + */ _addProjectHistoryMetadataToOps(updates, pathname, projectHistoryId, lines) { let docLength = _.reduce(lines, (chars, line) => chars + line.length, 0) docLength += lines.length - 1 // count newline characters - updates.forEach(function (update) { + + for (const update of updates) { update.projectHistoryId = projectHistoryId if (!update.meta) { update.meta = {} @@ -279,14 +332,14 @@ const UpdateManager = { // before it's ops are applied. However, we need to track any // changes to it for the next update. for (const op of update.op) { - if (op.i != null) { + if (isInsert(op)) { docLength += op.i.length } - if (op.d != null) { + if (isDelete(op)) { docLength -= op.d.length } } - }) + } }, } diff --git a/services/document-updater/app/js/Utils.js b/services/document-updater/app/js/Utils.js new file mode 100644 index 0000000000..9965f22b0f --- /dev/null +++ b/services/document-updater/app/js/Utils.js @@ -0,0 +1,40 @@ +// @ts-check + +/** + * @typedef {import('./types').CommentOp} CommentOp + * @typedef {import('./types').DeleteOp} DeleteOp + * @typedef {import('./types').InsertOp} InsertOp + * @typedef {import('./types').Op} Op + */ + +/** + * Returns true if the op is an insert + * + * @param {Op} op + * @returns {op is InsertOp} + */ +function isInsert(op) { + return 'i' in op && op.i != null +} + +/** + * Returns true if the op is an insert + * + * @param {Op} op + * @returns {op is DeleteOp} + */ +function isDelete(op) { + return 'd' in op && op.d != null +} + +/** + * Returns true if the op is a comment + * + * @param {Op} op + * @returns {op is CommentOp} + */ +function isComment(op) { + return 'c' in op && op.c != null +} + +module.exports = { isInsert, isDelete, isComment } diff --git a/services/document-updater/app/js/types.ts b/services/document-updater/app/js/types.ts new file mode 100644 index 0000000000..224391a91d --- /dev/null +++ b/services/document-updater/app/js/types.ts @@ -0,0 +1,89 @@ +export type Update = { + op: Op[] + v: number + meta?: { + pathname?: string + doc_length?: number + tc?: boolean + user_id?: string + } + projectHistoryId?: string +} + +export type Op = InsertOp | DeleteOp | CommentOp + +export type InsertOp = { + i: string + p: number + u?: boolean +} + +export type DeleteOp = { + d: string + p: number + u?: boolean +} + +export type CommentOp = { + c: string + p: number + t: string + u?: boolean +} + +export type Ranges = { + comments?: Comment[] + changes?: TrackedChange[] +} + +export type Comment = { + id: string + op: CommentOp + metadata: { + user_id: string + ts: string + } +} + +export type TrackedChange = { + id: string + op: Op + metadata: { + user_id: string + ts: string + } +} + +export type HistoryOp = HistoryInsertOp | HistoryDeleteOp | HistoryCommentOp + +export type HistoryInsertOp = InsertOp & { + hpos?: number + trackedDeleteRejection?: boolean +} + +export type HistoryDeleteOp = DeleteOp & { + hpos?: number + hsplits?: HistoryDeleteSplit[] +} + +export type HistoryDeleteSplit = { + offset: number + length: number +} + +export type HistoryCommentOp = CommentOp & { + hpos?: number + hlen?: number +} + +export type HistoryUpdate = { + op: HistoryOp[] + v: number + meta?: { + pathname?: string + doc_length?: number + tc?: boolean + user_id?: string + } + projectHistoryId?: string +} diff --git a/services/document-updater/test/acceptance/js/ApplyingUpdatesToADocTests.js b/services/document-updater/test/acceptance/js/ApplyingUpdatesToADocTests.js index ccceb67444..8de7f091a8 100644 --- a/services/document-updater/test/acceptance/js/ApplyingUpdatesToADocTests.js +++ b/services/document-updater/test/acceptance/js/ApplyingUpdatesToADocTests.js @@ -1,11 +1,3 @@ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS101: Remove unnecessary use of Array.from - * DS207: Consider shorter variations of null checks - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ const sinon = require('sinon') const { expect } = require('chai') const async = require('async') @@ -27,14 +19,13 @@ describe('Applying updates to a doc', function () { before(function (done) { this.lines = ['one', 'two', 'three'] this.version = 42 + this.op = { + i: 'one and a half\n', + p: 4, + } this.update = { doc: this.doc_id, - op: [ - { - i: 'one and a half\n', - p: 4, - }, - ], + op: [this.op], v: this.version, } this.result = ['one', 'one and a half', 'two', 'three'] @@ -43,10 +34,8 @@ describe('Applying updates to a doc', function () { describe('when the document is not loaded', function () { before(function (done) { - ;[this.project_id, this.doc_id] = Array.from([ - DocUpdaterClient.randomId(), - DocUpdaterClient.randomId(), - ]) + this.project_id = DocUpdaterClient.randomId() + this.doc_id = DocUpdaterClient.randomId() sinon.spy(MockWebApi, 'getDocument') this.startTime = Date.now() MockWebApi.insertDoc(this.project_id, this.doc_id, { @@ -97,7 +86,7 @@ describe('Applying updates to a doc', function () { if (error != null) { throw error } - JSON.parse(updates[0]).op.should.deep.equal(this.update.op) + JSON.parse(updates[0]).op.should.deep.equal([this.op]) done() } ) @@ -158,10 +147,8 @@ describe('Applying updates to a doc', function () { describe('when the document is loaded', function () { before(function (done) { - ;[this.project_id, this.doc_id] = Array.from([ - DocUpdaterClient.randomId(), - DocUpdaterClient.randomId(), - ]) + this.project_id = DocUpdaterClient.randomId() + this.doc_id = DocUpdaterClient.randomId() MockWebApi.insertDoc(this.project_id, this.doc_id, { lines: this.lines, @@ -213,7 +200,7 @@ describe('Applying updates to a doc', function () { -1, (error, updates) => { if (error) return done(error) - JSON.parse(updates[0]).op.should.deep.equal(this.update.op) + JSON.parse(updates[0]).op.should.deep.equal([this.op]) done() } ) @@ -222,10 +209,8 @@ describe('Applying updates to a doc', function () { describe('when the document is loaded and is using project-history only', function () { before(function (done) { - ;[this.project_id, this.doc_id] = Array.from([ - DocUpdaterClient.randomId(), - DocUpdaterClient.randomId(), - ]) + this.project_id = DocUpdaterClient.randomId() + this.doc_id = DocUpdaterClient.randomId() MockWebApi.insertDoc(this.project_id, this.doc_id, { lines: this.lines, @@ -273,7 +258,7 @@ describe('Applying updates to a doc', function () { -1, (error, updates) => { if (error) return done(error) - JSON.parse(updates[0]).op.should.deep.equal(this.update.op) + JSON.parse(updates[0]).op.should.deep.equal([this.op]) done() } ) @@ -283,10 +268,8 @@ describe('Applying updates to a doc', function () { describe('when the document has been deleted', function () { describe('when the ops come in a single linear order', function () { before(function (done) { - ;[this.project_id, this.doc_id] = Array.from([ - DocUpdaterClient.randomId(), - DocUpdaterClient.randomId(), - ]) + this.project_id = DocUpdaterClient.randomId() + this.doc_id = DocUpdaterClient.randomId() const lines = ['', '', ''] MockWebApi.insertDoc(this.project_id, this.doc_id, { lines, @@ -312,7 +295,7 @@ describe('Applying updates to a doc', function () { it('should be able to continue applying updates when the project has been deleted', function (done) { let update const actions = [] - for (update of Array.from(this.updates.slice(0, 6))) { + for (update of this.updates.slice(0, 6)) { ;(update => { actions.push(callback => DocUpdaterClient.sendUpdate( @@ -327,7 +310,7 @@ describe('Applying updates to a doc', function () { actions.push(callback => DocUpdaterClient.deleteDoc(this.project_id, this.doc_id, callback) ) - for (update of Array.from(this.updates.slice(6))) { + for (update of this.updates.slice(6)) { ;(update => { actions.push(callback => DocUpdaterClient.sendUpdate( @@ -363,7 +346,7 @@ describe('Applying updates to a doc', function () { -1, (error, updates) => { if (error) return done(error) - updates = Array.from(updates).map(u => JSON.parse(u)) + updates = updates.map(u => JSON.parse(u)) for (let i = 0; i < this.updates.length; i++) { const appliedUpdate = this.updates[i] appliedUpdate.op.should.deep.equal(updates[i].op) @@ -376,10 +359,8 @@ describe('Applying updates to a doc', function () { describe('when older ops come in after the delete', function () { before(function (done) { - ;[this.project_id, this.doc_id] = Array.from([ - DocUpdaterClient.randomId(), - DocUpdaterClient.randomId(), - ]) + this.project_id = DocUpdaterClient.randomId() + this.doc_id = DocUpdaterClient.randomId() const lines = ['', '', ''] MockWebApi.insertDoc(this.project_id, this.doc_id, { lines, @@ -400,7 +381,7 @@ describe('Applying updates to a doc', function () { it('should be able to continue applying updates when the project has been deleted', function (done) { let update const actions = [] - for (update of Array.from(this.updates.slice(0, 5))) { + for (update of this.updates.slice(0, 5)) { ;(update => { actions.push(callback => DocUpdaterClient.sendUpdate( @@ -415,7 +396,7 @@ describe('Applying updates to a doc', function () { actions.push(callback => DocUpdaterClient.deleteDoc(this.project_id, this.doc_id, callback) ) - for (update of Array.from(this.updates.slice(5))) { + for (update of this.updates.slice(5)) { ;(update => { actions.push(callback => DocUpdaterClient.sendUpdate( @@ -448,10 +429,8 @@ describe('Applying updates to a doc', function () { describe('with a broken update', function () { before(function (done) { - ;[this.project_id, this.doc_id] = Array.from([ - DocUpdaterClient.randomId(), - DocUpdaterClient.randomId(), - ]) + this.project_id = DocUpdaterClient.randomId() + this.doc_id = DocUpdaterClient.randomId() this.broken_update = { doc_id: this.doc_id, v: this.version, @@ -493,7 +472,7 @@ describe('Applying updates to a doc', function () { it('should send a message with an error', function () { this.messageCallback.called.should.equal(true) - const [channel, message] = Array.from(this.messageCallback.args[0]) + const [channel, message] = this.messageCallback.args[0] channel.should.equal('applied-ops') JSON.parse(message).should.deep.include({ project_id: this.project_id, @@ -505,10 +484,8 @@ describe('Applying updates to a doc', function () { describe('when there is no version in Mongo', function () { before(function (done) { - ;[this.project_id, this.doc_id] = Array.from([ - DocUpdaterClient.randomId(), - DocUpdaterClient.randomId(), - ]) + this.project_id = DocUpdaterClient.randomId() + this.doc_id = DocUpdaterClient.randomId() MockWebApi.insertDoc(this.project_id, this.doc_id, { lines: this.lines, }) @@ -546,10 +523,8 @@ describe('Applying updates to a doc', function () { describe('when the sending duplicate ops', function () { before(function (done) { - ;[this.project_id, this.doc_id] = Array.from([ - DocUpdaterClient.randomId(), - DocUpdaterClient.randomId(), - ]) + this.project_id = DocUpdaterClient.randomId() + this.doc_id = DocUpdaterClient.randomId() MockWebApi.insertDoc(this.project_id, this.doc_id, { lines: this.lines, version: this.version, @@ -633,10 +608,8 @@ describe('Applying updates to a doc', function () { describe('when sending updates for a non-existing doc id', function () { before(function (done) { - ;[this.project_id, this.doc_id] = Array.from([ - DocUpdaterClient.randomId(), - DocUpdaterClient.randomId(), - ]) + this.project_id = DocUpdaterClient.randomId() + this.doc_id = DocUpdaterClient.randomId() this.non_existing = { doc_id: this.doc_id, v: this.version, @@ -674,7 +647,7 @@ describe('Applying updates to a doc', function () { it('should send a message with an error', function () { this.messageCallback.called.should.equal(true) - const [channel, message] = Array.from(this.messageCallback.args[0]) + const [channel, message] = this.messageCallback.args[0] channel.should.equal('applied-ops') JSON.parse(message).should.deep.include({ project_id: this.project_id, diff --git a/services/document-updater/test/acceptance/js/RangesTests.js b/services/document-updater/test/acceptance/js/RangesTests.js index 102205709c..09aaa2bcf8 100644 --- a/services/document-updater/test/acceptance/js/RangesTests.js +++ b/services/document-updater/test/acceptance/js/RangesTests.js @@ -1,15 +1,3 @@ -/* eslint-disable - no-unused-vars, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS101: Remove unnecessary use of Array.from - * DS102: Remove unnecessary code created because of implicit returns - * DS207: Consider shorter variations of null checks - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ const sinon = require('sinon') const { expect } = require('chai') const async = require('async') @@ -21,7 +9,7 @@ const DocUpdaterApp = require('./helpers/DocUpdaterApp') describe('Ranges', function () { before(function (done) { - return DocUpdaterApp.ensureRunning(done) + DocUpdaterApp.ensureRunning(done) }) describe('tracking changes from ops', function () { @@ -58,43 +46,37 @@ describe('Ranges', function () { version: 0, }) const jobs = [] - for (const update of Array.from(this.updates)) { - ;(update => { - return jobs.push(callback => - DocUpdaterClient.sendUpdate( - this.project_id, - this.doc.id, - update, - callback - ) + for (const update of this.updates) { + jobs.push(callback => + DocUpdaterClient.sendUpdate( + this.project_id, + this.doc.id, + update, + callback ) - })(update) + ) } - return DocUpdaterApp.ensureRunning(error => { + DocUpdaterApp.ensureRunning(error => { if (error != null) { throw error } - return DocUpdaterClient.preloadDoc( - this.project_id, - this.doc.id, - error => { + DocUpdaterClient.preloadDoc(this.project_id, this.doc.id, error => { + if (error != null) { + throw error + } + async.series(jobs, error => { if (error != null) { throw error } - return async.series(jobs, error => { - if (error != null) { - throw error - } - return done() - }) - } - ) + done() + }) + }) }) }) it('should update the ranges', function (done) { - return DocUpdaterClient.getDoc( + DocUpdaterClient.getDoc( this.project_id, this.doc.id, (error, res, data) => { @@ -106,12 +88,12 @@ describe('Ranges', function () { change.op.should.deep.equal({ i: '456', p: 3 }) change.id.should.equal(this.id_seed + '000001') change.metadata.user_id.should.equal(this.user_id) - return done() + done() } ) }) - return describe('Adding comments', function () { + describe('Adding comments', function () { describe('standalone', function () { before(function (done) { this.project_id = DocUpdaterClient.randomId() @@ -134,37 +116,31 @@ describe('Ranges', function () { version: 0, }) const jobs = [] - for (const update of Array.from(this.updates)) { - ;(update => { - return jobs.push(callback => - DocUpdaterClient.sendUpdate( - this.project_id, - this.doc.id, - update, - callback - ) + for (const update of this.updates) { + jobs.push(callback => + DocUpdaterClient.sendUpdate( + this.project_id, + this.doc.id, + update, + callback ) - })(update) + ) } - return DocUpdaterClient.preloadDoc( - this.project_id, - this.doc.id, - error => { + DocUpdaterClient.preloadDoc(this.project_id, this.doc.id, error => { + if (error != null) { + throw error + } + async.series(jobs, error => { if (error != null) { throw error } - return async.series(jobs, error => { - if (error != null) { - throw error - } - return setTimeout(done, 200) - }) - } - ) + setTimeout(done, 200) + }) + }) }) - return it('should update the ranges', function (done) { - return DocUpdaterClient.getDoc( + it('should update the ranges', function (done) { + DocUpdaterClient.getDoc( this.project_id, this.doc.id, (error, res, data) => { @@ -175,13 +151,13 @@ describe('Ranges', function () { const comment = ranges.comments[0] comment.op.should.deep.equal({ c: 'bar', p: 4, t: this.tid }) comment.id.should.equal(this.tid) - return done() + done() } ) }) }) - return describe('with conflicting ops needing OT', function () { + describe('with conflicting ops needing OT', function () { before(function (done) { this.project_id = DocUpdaterClient.randomId() this.user_id = DocUpdaterClient.randomId() @@ -209,37 +185,31 @@ describe('Ranges', function () { version: 0, }) const jobs = [] - for (const update of Array.from(this.updates)) { - ;(update => { - return jobs.push(callback => - DocUpdaterClient.sendUpdate( - this.project_id, - this.doc.id, - update, - callback - ) + for (const update of this.updates) { + jobs.push(callback => + DocUpdaterClient.sendUpdate( + this.project_id, + this.doc.id, + update, + callback ) - })(update) + ) } - return DocUpdaterClient.preloadDoc( - this.project_id, - this.doc.id, - error => { + DocUpdaterClient.preloadDoc(this.project_id, this.doc.id, error => { + if (error != null) { + throw error + } + async.series(jobs, error => { if (error != null) { throw error } - return async.series(jobs, error => { - if (error != null) { - throw error - } - return setTimeout(done, 200) - }) - } - ) + setTimeout(done, 200) + }) + }) }) - return it('should update the comments with the OT shifted comment', function (done) { - return DocUpdaterClient.getDoc( + it('should update the comments with the OT shifted comment', function (done) { + DocUpdaterClient.getDoc( this.project_id, this.doc.id, (error, res, data) => { @@ -249,7 +219,7 @@ describe('Ranges', function () { const { ranges } = data const comment = ranges.comments[0] comment.op.should.deep.equal({ c: 'bar', p: 7, t: this.tid }) - return done() + done() } ) }) @@ -287,30 +257,26 @@ describe('Ranges', function () { ], }, }) - return DocUpdaterClient.preloadDoc( - this.project_id, - this.doc.id, - error => { - if (error != null) { - throw error - } - return DocUpdaterClient.sendUpdate( - this.project_id, - this.doc.id, - this.update, - error => { - if (error != null) { - throw error - } - return setTimeout(done, 200) - } - ) + DocUpdaterClient.preloadDoc(this.project_id, this.doc.id, error => { + if (error != null) { + throw error } - ) + DocUpdaterClient.sendUpdate( + this.project_id, + this.doc.id, + this.update, + error => { + if (error != null) { + throw error + } + setTimeout(done, 200) + } + ) + }) }) it('should have preloaded the existing ranges', function (done) { - return DocUpdaterClient.getDoc( + DocUpdaterClient.getDoc( this.project_id, this.doc.id, (error, res, data) => { @@ -320,27 +286,23 @@ describe('Ranges', function () { const { changes } = data.ranges changes[0].op.should.deep.equal({ i: '123', p: 1 }) changes[1].op.should.deep.equal({ i: '456', p: 5 }) - return done() + done() } ) }) - return it('should flush the ranges to the persistence layer again', function (done) { - return DocUpdaterClient.flushDoc(this.project_id, this.doc.id, error => { + it('should flush the ranges to the persistence layer again', function (done) { + DocUpdaterClient.flushDoc(this.project_id, this.doc.id, error => { if (error != null) { throw error } - return MockWebApi.getDocument( - this.project_id, - this.doc.id, - (error, doc) => { - if (error) return done(error) - const { changes } = doc.ranges - changes[0].op.should.deep.equal({ i: '123', p: 1 }) - changes[1].op.should.deep.equal({ i: '456', p: 5 }) - return done() - } - ) + MockWebApi.getDocument(this.project_id, this.doc.id, (error, doc) => { + if (error) return done(error) + const { changes } = doc.ranges + changes[0].op.should.deep.equal({ i: '123', p: 1 }) + changes[1].op.should.deep.equal({ i: '456', p: 5 }) + done() + }) }) }) }) @@ -365,49 +327,45 @@ describe('Ranges', function () { lines: this.doc.lines, version: 0, }) - return DocUpdaterClient.preloadDoc( - this.project_id, - this.doc.id, - error => { - if (error != null) { - throw error - } - return DocUpdaterClient.sendUpdate( - this.project_id, - this.doc.id, - this.update, - error => { - if (error != null) { - throw error - } - return setTimeout(() => { - return DocUpdaterClient.getDoc( - this.project_id, - this.doc.id, - (error, res, data) => { - if (error != null) { - throw error - } - const { ranges } = data - const change = ranges.changes[0] - change.op.should.deep.equal({ i: '456', p: 1 }) - change.id.should.equal(this.id_seed + '000001') - change.metadata.user_id.should.equal(this.user_id) - return done() - } - ) - }, 200) - } - ) + DocUpdaterClient.preloadDoc(this.project_id, this.doc.id, error => { + if (error != null) { + throw error } - ) + DocUpdaterClient.sendUpdate( + this.project_id, + this.doc.id, + this.update, + error => { + if (error != null) { + throw error + } + setTimeout(() => { + DocUpdaterClient.getDoc( + this.project_id, + this.doc.id, + (error, res, data) => { + if (error != null) { + throw error + } + const { ranges } = data + const change = ranges.changes[0] + change.op.should.deep.equal({ i: '456', p: 1 }) + change.id.should.equal(this.id_seed + '000001') + change.metadata.user_id.should.equal(this.user_id) + done() + } + ) + }, 200) + } + ) + }) }) afterEach(function () { MockWebApi.setDocument.restore() }) it('should remove the change after accepting', function (done) { - return DocUpdaterClient.acceptChange( + DocUpdaterClient.acceptChange( this.project_id, this.doc.id, this.id_seed + '000001', @@ -415,7 +373,7 @@ describe('Ranges', function () { if (error != null) { throw error } - return DocUpdaterClient.getDoc( + DocUpdaterClient.getDoc( this.project_id, this.doc.id, (error, res, data) => { @@ -423,7 +381,7 @@ describe('Ranges', function () { throw error } expect(data.ranges.changes).to.be.undefined - return done() + done() } ) } @@ -483,45 +441,41 @@ describe('Ranges', function () { lines: this.doc.lines, version: 0, }) - return DocUpdaterClient.preloadDoc( - this.project_id, - this.doc.id, - error => { - if (error != null) { - throw error - } - return DocUpdaterClient.sendUpdate( - this.project_id, - this.doc.id, - this.update, - error => { - if (error != null) { - throw error - } - return setTimeout(() => { - return DocUpdaterClient.getDoc( - this.project_id, - this.doc.id, - (error, res, data) => { - if (error != null) { - throw error - } - const { ranges } = data - const change = ranges.comments[0] - change.op.should.deep.equal({ c: 'bar', p: 4, t: this.tid }) - change.id.should.equal(this.tid) - return done() - } - ) - }, 200) - } - ) + DocUpdaterClient.preloadDoc(this.project_id, this.doc.id, error => { + if (error != null) { + throw error } - ) + DocUpdaterClient.sendUpdate( + this.project_id, + this.doc.id, + this.update, + error => { + if (error != null) { + throw error + } + setTimeout(() => { + DocUpdaterClient.getDoc( + this.project_id, + this.doc.id, + (error, res, data) => { + if (error != null) { + throw error + } + const { ranges } = data + const change = ranges.comments[0] + change.op.should.deep.equal({ c: 'bar', p: 4, t: this.tid }) + change.id.should.equal(this.tid) + done() + } + ) + }, 200) + } + ) + }) }) - return it('should remove the comment range', function (done) { - return DocUpdaterClient.removeComment( + it('should remove the comment range', function (done) { + DocUpdaterClient.removeComment( this.project_id, this.doc.id, this.tid, @@ -530,7 +484,7 @@ describe('Ranges', function () { throw error } expect(res.statusCode).to.equal(204) - return DocUpdaterClient.getDoc( + DocUpdaterClient.getDoc( this.project_id, this.doc.id, (error, res, data) => { @@ -538,7 +492,7 @@ describe('Ranges', function () { throw error } expect(data.ranges.comments).to.be.undefined - return done() + done() } ) } @@ -569,37 +523,31 @@ describe('Ranges', function () { version: 0, }) const jobs = [] - for (const update of Array.from(this.updates)) { - ;(update => { - return jobs.push(callback => - DocUpdaterClient.sendUpdate( - this.project_id, - this.doc.id, - update, - callback - ) + for (const update of this.updates) { + jobs.push(callback => + DocUpdaterClient.sendUpdate( + this.project_id, + this.doc.id, + update, + callback ) - })(update) + ) } - return DocUpdaterClient.preloadDoc( - this.project_id, - this.doc.id, - error => { + DocUpdaterClient.preloadDoc(this.project_id, this.doc.id, error => { + if (error != null) { + throw error + } + async.series(jobs, error => { if (error != null) { throw error } - return async.series(jobs, error => { - if (error != null) { - throw error - } - return setTimeout(done, 200) - }) - } - ) + setTimeout(done, 200) + }) + }) }) - return it('should not update the ranges', function (done) { - return DocUpdaterClient.getDoc( + it('should not update the ranges', function (done) { + DocUpdaterClient.getDoc( this.project_id, this.doc.id, (error, res, data) => { @@ -608,13 +556,13 @@ describe('Ranges', function () { } const { ranges } = data expect(ranges.changes).to.be.undefined - return done() + done() } ) }) }) - return describe('deleting text surrounding a comment', function () { + describe('deleting text surrounding a comment', function () { before(function (done) { this.project_id = DocUpdaterClient.randomId() this.user_id = DocUpdaterClient.randomId() @@ -653,48 +601,42 @@ describe('Ranges', function () { }, ] const jobs = [] - for (const update of Array.from(this.updates)) { - ;(update => { - return jobs.push(callback => - DocUpdaterClient.sendUpdate( - this.project_id, - this.doc_id, - update, - callback - ) + for (const update of this.updates) { + jobs.push(callback => + DocUpdaterClient.sendUpdate( + this.project_id, + this.doc_id, + update, + callback ) - })(update) + ) } - return DocUpdaterClient.preloadDoc( - this.project_id, - this.doc_id, - error => { + DocUpdaterClient.preloadDoc(this.project_id, this.doc_id, error => { + if (error != null) { + throw error + } + async.series(jobs, function (error) { if (error != null) { throw error } - return async.series(jobs, function (error) { - if (error != null) { - throw error - } - return setTimeout(() => { - return DocUpdaterClient.getDoc( - this.project_id, - this.doc_id, - (error, res, data) => { - if (error != null) { - throw error - } - return done() + setTimeout(() => { + DocUpdaterClient.getDoc( + this.project_id, + this.doc_id, + (error, res, data) => { + if (error != null) { + throw error } - ) - }, 200) - }) - } - ) + done() + } + ) + }, 200) + }) + }) }) - return it('should write a snapshot from before the destructive change', function (done) { - return DocUpdaterClient.getDoc( + it('should write a snapshot from before the destructive change', function (done) { + DocUpdaterClient.getDoc( this.project_id, this.doc_id, (error, res, data) => { @@ -718,7 +660,7 @@ describe('Ranges', function () { p: 1, tid: this.tid, }) - return done() + done() }) } ) diff --git a/services/document-updater/test/unit/js/RangesManager/RangesManagerTests.js b/services/document-updater/test/unit/js/RangesManager/RangesManagerTests.js index cd70b2f59b..b835de4783 100644 --- a/services/document-updater/test/unit/js/RangesManager/RangesManagerTests.js +++ b/services/document-updater/test/unit/js/RangesManager/RangesManagerTests.js @@ -3,6 +3,7 @@ const { expect } = require('chai') const SandboxedModule = require('sandboxed-module') const MODULE_PATH = '../../../../app/js/RangesManager.js' +const TEST_USER_ID = 'user-id-123' describe('RangesManager', function () { beforeEach(function () { @@ -14,57 +15,29 @@ describe('RangesManager', function () { this.doc_id = 'doc-id-123' this.project_id = 'project-id-123' - this.user_id = 'user-id-123' + this.user_id = TEST_USER_ID }) describe('applyUpdate', function () { beforeEach(function () { - this.updates = [ - { - meta: { - user_id: this.user_id, - }, - op: [ - { - i: 'two ', - p: 4, - }, - ], - }, - ] - this.entries = { - comments: [ - { - op: { - c: 'three ', - p: 4, - }, - metadata: { - user_id: this.user_id, - }, - }, - ], - changes: [ - { - op: { - i: 'five', - p: 15, - }, - metadata: { - user_id: this.user_id, - }, - }, - ], + this.ops = [{ i: 'two ', p: 4 }] + this.historyOps = [{ i: 'two ', p: 4, hpos: 4 }] + this.meta = { user_id: this.user_id } + this.updates = [{ meta: this.meta, op: this.ops }] + this.ranges = { + comments: makeRanges([{ c: 'three ', p: 4 }]), + changes: makeRanges([{ i: 'five', p: 15 }]), } this.newDocLines = ['one two three four five'] - }) // old is "one three four five" + // old is "one three four five" + }) describe('successfully', function () { beforeEach(function () { this.result = this.RangesManager.applyUpdate( this.project_id, this.doc_id, - this.entries, + this.ranges, this.updates, this.newDocLines ) @@ -81,15 +54,19 @@ describe('RangesManager', function () { p: 19, }) }) + + it('should return unmodified updates for the history', function () { + expect(this.result.historyUpdates).to.deep.equal(this.updates) + }) }) describe('with empty comments', function () { beforeEach(function () { - this.entries.comments = [] + this.ranges.comments = [] this.result = this.RangesManager.applyUpdate( this.project_id, this.doc_id, - this.entries, + this.ranges, this.updates, this.newDocLines ) @@ -99,15 +76,19 @@ describe('RangesManager', function () { // Save space in redis and don't store just {} expect(this.result.newRanges.comments).to.be.undefined }) + + it('should return unmodified updates for the history', function () { + expect(this.result.historyUpdates).to.deep.equal(this.updates) + }) }) describe('with empty changes', function () { beforeEach(function () { - this.entries.changes = [] + this.ranges.changes = [] this.result = this.RangesManager.applyUpdate( this.project_id, this.doc_id, - this.entries, + this.ranges, this.updates, this.newDocLines ) @@ -117,48 +98,21 @@ describe('RangesManager', function () { // Save space in redis and don't store just {} expect(this.result.newRanges.changes).to.be.undefined }) + + it('should return unmodified updates for the history', function () { + expect(this.result.historyUpdates).to.deep.equal(this.updates) + }) }) describe('with too many comments', function () { beforeEach(function () { this.RangesManager.MAX_COMMENTS = 2 - this.updates = [ - { - meta: { - user_id: this.user_id, - }, - op: [ - { - c: 'one', - p: 0, - t: 'thread-id-1', - }, - ], - }, - ] - this.entries = { - comments: [ - { - op: { - c: 'three ', - p: 4, - t: 'thread-id-2', - }, - metadata: { - user_id: this.user_id, - }, - }, - { - op: { - c: 'four ', - p: 10, - t: 'thread-id-3', - }, - metadata: { - user_id: this.user_id, - }, - }, - ], + this.updates = makeUpdates([{ c: 'one', p: 0, t: 'thread-id-1' }]) + this.ranges = { + comments: makeRanges([ + { c: 'three ', p: 4, t: 'thread-id-2' }, + { c: 'four ', p: 10, t: 'thread-id-3' }, + ]), changes: [], } }) @@ -168,7 +122,7 @@ describe('RangesManager', function () { this.RangesManager.applyUpdate( this.project_id, this.doc_id, - this.entries, + this.ranges, this.updates, this.newDocLines ) @@ -179,41 +133,20 @@ describe('RangesManager', function () { describe('with too many changes', function () { beforeEach(function () { this.RangesManager.MAX_CHANGES = 2 - this.updates = [ - { - meta: { - user_id: this.user_id, - tc: 'track-changes-id-yes', - }, - op: [ - { - i: 'one ', - p: 0, - }, - ], - }, - ] - this.entries = { - changes: [ + this.updates = makeUpdates([{ i: 'one ', p: 0 }], { + tc: 'track-changes-id-yes', + }) + this.ranges = { + changes: makeRanges([ { - op: { - i: 'three', - p: 4, - }, - metadata: { - user_id: this.user_id, - }, + i: 'three', + p: 4, }, { - op: { - i: 'four', - p: 10, - }, - metadata: { - user_id: this.user_id, - }, + i: 'four', + p: 10, }, - ], + ]), comments: [], } this.newDocLines = ['one two three four'] @@ -224,7 +157,7 @@ describe('RangesManager', function () { this.RangesManager.applyUpdate( this.project_id, this.doc_id, - this.entries, + this.ranges, this.updates, this.newDocLines ) @@ -234,19 +167,7 @@ describe('RangesManager', function () { describe('inconsistent changes', function () { beforeEach(function () { - this.updates = [ - { - meta: { - user_id: this.user_id, - }, - op: [ - { - c: "doesn't match", - p: 0, - }, - ], - }, - ] + this.updates = makeUpdates([{ c: "doesn't match", p: 0 }]) }) it('should throw an error', function () { @@ -254,51 +175,33 @@ describe('RangesManager', function () { this.RangesManager.applyUpdate( this.project_id, this.doc_id, - this.entries, + this.ranges, this.updates, this.newDocLines ) }).to.throw( - 'Change ({"op":{"i":"five","p":15},"metadata":{"user_id":"user-id-123"}}) doesn\'t match text ("our ")' + 'Change ({"id":"1","op":{"i":"five","p":15},"metadata":{"user_id":"user-id-123"}}) doesn\'t match text ("our ")' ) }) }) describe('with an update that collapses a range', function () { beforeEach(function () { - this.updates = [ - { - meta: { - user_id: this.user_id, - }, - op: [ - { - d: 'one', - p: 0, - t: 'thread-id-1', - }, - ], - }, - ] - this.entries = { - comments: [ + this.updates = makeUpdates([{ d: 'one', p: 0, t: 'thread-id-1' }]) + this.ranges = { + comments: makeRanges([ { - op: { - c: 'n', - p: 1, - t: 'thread-id-2', - }, - metadata: { - user_id: this.user_id, - }, + c: 'n', + p: 1, + t: 'thread-id-2', }, - ], + ]), changes: [], } this.result = this.RangesManager.applyUpdate( this.project_id, this.doc_id, - this.entries, + this.ranges, this.updates, this.newDocLines ) @@ -311,49 +214,15 @@ describe('RangesManager', function () { describe('with an update that deletes ranges', function () { beforeEach(function () { - this.updates = [ - { - meta: { - user_id: this.user_id, - }, - op: [ - { - d: 'one two three four five', - p: 0, - }, - ], - }, - ] - this.entries = { - comments: [ - { - op: { - c: 'n', - p: 1, - t: 'thread-id-2', - }, - metadata: { - user_id: this.user_id, - }, - }, - ], - changes: [ - { - op: { - i: 'hello', - p: 1, - t: 'thread-id-2', - }, - metadata: { - user_id: this.user_id, - }, - }, - ], + this.updates = makeUpdates([{ d: 'one two three four five', p: 0 }]) + this.ranges = { + comments: makeRanges([{ c: 'n', p: 1, t: 'thread-id-2' }]), + changes: makeRanges([{ i: 'hello', p: 1, t: 'thread-id-2' }]), } this.result = this.RangesManager.applyUpdate( this.project_id, this.doc_id, - this.entries, + this.ranges, this.updates, this.newDocLines ) @@ -367,6 +236,156 @@ describe('RangesManager', function () { expect(this.result.rangesWereCollapsed).to.equal(true) }) }) + + describe('inserts among tracked deletes', function () { + beforeEach(function () { + // original text is "on[1]e[22] [333](three) fo[4444]ur five" + // [] denotes tracked deletes + // () denotes tracked inserts + this.ranges = { + changes: makeRanges([ + { d: '1', p: 2 }, + { d: '22', p: 3 }, + { d: '333', p: 4 }, + { i: 'three', p: 4 }, + { d: '4444', p: 12 }, + ]), + } + this.updates = makeUpdates([ + { i: 'zero ', p: 0 }, + { i: 'two ', p: 9, u: true }, + ]) + this.newDocLines = ['zero one two three four five'] + this.result = this.RangesManager.applyUpdate( + this.project_id, + this.doc_id, + this.ranges, + this.updates, + this.newDocLines + ) + }) + + it('should offset the hpos by the length of tracked deletes before the insert', function () { + expect(this.result.historyUpdates.map(x => x.op)).to.deep.equal([ + [{ i: 'zero ', p: 0 }], + // 'two' is added just before the "333" tracked delete + [{ i: 'two ', p: 9, u: true, hpos: 12 }], + ]) + }) + }) + + describe('tracked delete rejections', function () { + beforeEach(function () { + // original text is "one [two ]three four five" + // [] denotes tracked deletes + this.ranges = { + changes: makeRanges([{ d: 'two ', p: 4 }]), + } + this.updates = makeUpdates([{ i: 'tw', p: 4, u: true }]) + this.newDocLines = ['one twthree four five'] + this.result = this.RangesManager.applyUpdate( + this.project_id, + this.doc_id, + this.ranges, + this.updates, + this.newDocLines + ) + }) + + it('should mark the insert as a tracked delete rejection where appropriate', function () { + expect(this.result.historyUpdates.map(x => x.op)).to.deep.equal([ + [{ i: 'tw', p: 4, u: true, trackedDeleteRejection: true }], + ]) + }) + }) + + describe('deletes among tracked deletes', function () { + beforeEach(function () { + // original text is "on[1]e [22](three) f[333]ou[4444]r [55555]five" + // [] denotes tracked deletes + // () denotes tracked inserts + this.ranges = { + comments: [], + changes: makeRanges([ + { d: '1', p: 2 }, + { d: '22', p: 4 }, + { i: 'three', p: 4 }, + { d: '333', p: 11 }, + { d: '4444', p: 13 }, + { d: '55555', p: 15 }, + ]), + } + this.updates = makeUpdates([ + { d: 'four ', p: 10 }, + { d: 'three ', p: 4 }, + ]) + this.newDocLines = ['one five'] + this.result = this.RangesManager.applyUpdate( + this.project_id, + this.doc_id, + this.ranges, + this.updates, + this.newDocLines + ) + }) + + it('should split and offset deletes appropriately', function () { + expect(this.result.historyUpdates.map(x => x.op)).to.deep.equal([ + [ + // the "four" delete has tracked deletes inside it, add splits + { + d: 'four ', + p: 10, + hpos: 13, + hsplits: [ + { offset: 1, length: 3 }, + { offset: 3, length: 4 }, + ], + }, + ], + + // the "three" delete is offset to the right by the two first tracked + // deletes + [{ d: 'three ', p: 4, hpos: 7 }], + ]) + }) + }) + + describe('comments among tracked deletes', function () { + beforeEach(function () { + // original text is "on[1]e[22] [333](three) fo[4444]ur five" + // [] denotes tracked deletes + // () denotes tracked inserts + this.ranges = { + changes: makeRanges([ + { d: '1', p: 2 }, + { d: '22', p: 3 }, + { d: '333', p: 4 }, + { i: 'three', p: 4 }, + { d: '4444', p: 12 }, + ]), + } + this.updates = makeUpdates([ + { c: 'three ', p: 4 }, + { c: 'four ', p: 10 }, + ]) + this.newDocLines = ['one three four five'] + this.result = this.RangesManager.applyUpdate( + this.project_id, + this.doc_id, + this.ranges, + this.updates, + this.newDocLines + ) + }) + + it('should offset the hpos by the length of tracked deletes before the insert', function () { + expect(this.result.historyUpdates.map(x => x.op)).to.deep.equal([ + [{ c: 'three ', p: 4, hpos: 10 }], + [{ c: 'four ', p: 10, hpos: 16, hlen: 9 }], + ]) + }) + }) }) describe('acceptChanges', function () { @@ -381,43 +400,13 @@ describe('RangesManager', function () { this.ranges = { comments: [], - changes: [ - { - id: 'a1', - op: { - i: 'lorem', - p: 0, - }, - }, - { - id: 'a2', - op: { - i: 'ipsum', - p: 10, - }, - }, - { - id: 'a3', - op: { - i: 'dolor', - p: 20, - }, - }, - { - id: 'a4', - op: { - i: 'sit', - p: 30, - }, - }, - { - id: 'a5', - op: { - i: 'amet', - p: 40, - }, - }, - ], + changes: makeRanges([ + { i: 'lorem', p: 0 }, + { i: 'ipsum', p: 10 }, + { i: 'dolor', p: 20 }, + { i: 'sit', p: 30 }, + { i: 'amet', p: 40 }, + ]), } this.removeChangeIdsSpy = sinon.spy( this.RangesTracker.prototype, @@ -516,3 +505,28 @@ describe('RangesManager', function () { }) }) }) + +function makeRanges(ops) { + let id = 1 + const changes = [] + for (const op of ops) { + changes.push({ + id: id.toString(), + op, + metadata: { user_id: TEST_USER_ID }, + }) + id += 1 + } + return changes +} + +function makeUpdates(ops, meta = {}) { + const updates = [] + for (const op of ops) { + updates.push({ + meta: { user_id: TEST_USER_ID, ...meta }, + op: [op], + }) + } + return updates +} diff --git a/services/document-updater/test/unit/js/RedisManager/RedisManagerTests.js b/services/document-updater/test/unit/js/RedisManager/RedisManagerTests.js index 369b4b76cd..d5d041bb53 100644 --- a/services/document-updater/test/unit/js/RedisManager/RedisManagerTests.js +++ b/services/document-updater/test/unit/js/RedisManager/RedisManagerTests.js @@ -1,18 +1,18 @@ const sinon = require('sinon') -const modulePath = '../../../../app/js/RedisManager.js' const SandboxedModule = require('sandboxed-module') const Errors = require('../../../../app/js/Errors') const crypto = require('crypto') const tk = require('timekeeper') +const MODULE_PATH = '../../../../app/js/RedisManager.js' + describe('RedisManager', function () { beforeEach(function () { this.multi = { exec: sinon.stub().yields() } this.rclient = { multi: () => this.multi, srem: sinon.stub().yields() } tk.freeze(new Date()) - this.RedisManager = SandboxedModule.require(modulePath, { + this.RedisManager = SandboxedModule.require(MODULE_PATH, { requires: { - './ProjectHistoryRedisManager': (this.ProjectHistoryRedisManager = {}), '@overleaf/settings': (this.settings = { documentupdater: { logHashErrors: { write: true, read: true } }, redis: { @@ -468,13 +468,6 @@ describe('RedisManager', function () { null, null, ]) - this.ProjectHistoryRedisManager.queueOps = sinon - .stub() - .callsArgWith( - this.ops.length + 1, - null, - this.project_update_list_length - ) }) describe('with a consistent version', function () { @@ -545,16 +538,8 @@ describe('RedisManager', function () { .should.equal(true) }) - it('should push the updates into the project history ops list', function () { - this.ProjectHistoryRedisManager.queueOps - .calledWith(this.project_id, JSON.stringify(this.ops[0])) - .should.equal(true) - }) - it('should call the callback', function () { - this.callback - .calledWith(null, this.project_update_list_length) - .should.equal(true) + this.callback.should.have.been.called }) it('should not log any errors', function () { @@ -578,16 +563,8 @@ describe('RedisManager', function () { ) }) - it('should push the updates into the project history ops list', function () { - this.ProjectHistoryRedisManager.queueOps - .calledWith(this.project_id, JSON.stringify(this.ops[0])) - .should.equal(true) - }) - - it('should call the callback with the project update count only', function () { - this.callback - .calledWith(null, this.project_update_list_length) - .should.equal(true) + it('should call the callback', function () { + this.callback.should.have.been.called }) }) }) @@ -647,10 +624,6 @@ describe('RedisManager', function () { this.multi.rpush.called.should.equal(false) }) - it('should not try to enqueue project updates', function () { - this.ProjectHistoryRedisManager.queueOps.called.should.equal(false) - }) - it('should still set the doclines', function () { this.multi.mset .calledWith({ @@ -1041,9 +1014,6 @@ describe('RedisManager', function () { this.RedisManager.getDoc = sinon .stub() .callsArgWith(2, null, 'lines', 'version') - this.ProjectHistoryRedisManager.queueRenameEntity = sinon - .stub() - .yields() this.RedisManager.renameDoc( this.project_id, this.docId, @@ -1066,9 +1036,6 @@ describe('RedisManager', function () { this.RedisManager.getDoc = sinon .stub() .callsArgWith(2, null, null, null) - this.ProjectHistoryRedisManager.queueRenameEntity = sinon - .stub() - .yields() this.RedisManager.renameDoc( this.project_id, this.docId, diff --git a/services/document-updater/test/unit/js/UpdateManager/UpdateManagerTests.js b/services/document-updater/test/unit/js/UpdateManager/UpdateManagerTests.js index 16b9d044b3..ab1fa8af9a 100644 --- a/services/document-updater/test/unit/js/UpdateManager/UpdateManagerTests.js +++ b/services/document-updater/test/unit/js/UpdateManager/UpdateManagerTests.js @@ -74,6 +74,14 @@ describe('UpdateManager', function () { }, } + this.ProjectHistoryRedisManager = { + promises: { + queueOps: sinon + .stub() + .callsFake(async (projectId, ...ops) => ops.length), + }, + } + this.UpdateManager = SandboxedModule.require(MODULE_PATH, { requires: { './LockManager': this.LockManager, @@ -87,6 +95,7 @@ describe('UpdateManager', function () { './RangesManager': this.RangesManager, './SnapshotManager': this.SnapshotManager, './Profiler': this.Profiler, + './ProjectHistoryRedisManager': this.ProjectHistoryRedisManager, }, }) }) @@ -309,6 +318,11 @@ describe('UpdateManager', function () { { v: 42, op: 'mock-op-42' }, { v: 45, op: 'mock-op-45' }, ] + this.historyUpdates = [ + 'history-update-1', + 'history-update-2', + 'history-update-3', + ] this.project_ops_length = 123 this.pathname = '/a/b/c.tex' this.DocumentManager.promises.getDoc.resolves({ @@ -317,19 +331,19 @@ describe('UpdateManager', function () { ranges: this.ranges, pathname: this.pathname, projectHistoryId: this.projectHistoryId, + historyRangesSupport: false, }) this.RangesManager.applyUpdate.returns({ newRanges: this.updated_ranges, rangesWereCollapsed: false, + historyUpdates: this.historyUpdates, }) this.ShareJsUpdateManager.promises.applyUpdate = sinon.stub().resolves({ updatedDocLines: this.updatedDocLines, version: this.version, appliedOps: this.appliedOps, }) - this.RedisManager.promises.updateDocument.resolves( - this.project_ops_length - ) + this.RedisManager.promises.updateDocument.resolves() this.UpdateManager.promises._addProjectHistoryMetadataToOps = sinon.stub() }) @@ -390,9 +404,15 @@ describe('UpdateManager', function () { }) it('should push the applied ops into the history queue', function () { - this.HistoryManager.recordAndFlushHistoryOps - .calledWith(this.project_id, this.appliedOps, this.project_ops_length) - .should.equal(true) + this.ProjectHistoryRedisManager.promises.queueOps.should.have.been.calledWith( + this.project_id, + ...this.appliedOps.map(op => JSON.stringify(op)) + ) + this.HistoryManager.recordAndFlushHistoryOps.should.have.been.calledWith( + this.project_id, + this.appliedOps, + this.appliedOps.length + ) }) }) @@ -445,6 +465,7 @@ describe('UpdateManager', function () { this.RangesManager.applyUpdate.returns({ newRanges: this.updated_ranges, rangesWereCollapsed: true, + historyUpdates: this.historyUpdates, }) await this.UpdateManager.promises.applyUpdate( this.project_id, @@ -470,6 +491,36 @@ describe('UpdateManager', function () { .should.equal(true) }) }) + + describe('when history ranges are supported', function () { + beforeEach(async function () { + this.DocumentManager.promises.getDoc.resolves({ + lines: this.lines, + version: this.version, + ranges: this.ranges, + pathname: this.pathname, + projectHistoryId: this.projectHistoryId, + historyRangesSupport: true, + }) + await this.UpdateManager.promises.applyUpdate( + this.project_id, + this.doc_id, + this.update + ) + }) + + it('should push the history updates into the history queue', function () { + this.ProjectHistoryRedisManager.promises.queueOps.should.have.been.calledWith( + this.project_id, + ...this.historyUpdates.map(op => JSON.stringify(op)) + ) + this.HistoryManager.recordAndFlushHistoryOps.should.have.been.calledWith( + this.project_id, + this.historyUpdates, + this.historyUpdates.length + ) + }) + }) }) describe('_addProjectHistoryMetadataToOps', function () {