From 2f513700ecffb521bd0fc0b4fa9f1e9c248242ec Mon Sep 17 00:00:00 2001 From: Eric Mc Sween <5454374+emcsween@users.noreply.github.com> Date: Tue, 13 Feb 2024 11:36:05 -0500 Subject: [PATCH] Merge pull request #17010 from overleaf/em-doc-length Add history_doc_length property to history updates GitOrigin-RevId: ccad09f23ae9c038480fb7228a987d8fc6fb6274 --- .../document-updater/app/js/UpdateManager.js | 53 ++++++++++++++--- services/document-updater/app/js/types.ts | 3 +- .../js/UpdateManager/UpdateManagerTests.js | 59 +++++++++++++++---- 3 files changed, 91 insertions(+), 24 deletions(-) diff --git a/services/document-updater/app/js/UpdateManager.js b/services/document-updater/app/js/UpdateManager.js index f38b53e851..f68ae79ec9 100644 --- a/services/document-updater/app/js/UpdateManager.js +++ b/services/document-updater/app/js/UpdateManager.js @@ -19,6 +19,7 @@ const { isInsert, isDelete } = require('./Utils') /** * @typedef {import("./types").DeleteOp} DeleteOp + * @typedef {import("./types").HistoryUpdate } HistoryUpdate * @typedef {import("./types").InsertOp} InsertOp * @typedef {import("./types").Op} Op * @typedef {import("./types").Ranges} Ranges @@ -156,12 +157,6 @@ const UpdateManager = { } profile.log('RangesManager.applyUpdate', { sync: true }) - UpdateManager._addProjectHistoryMetadataToOps( - appliedOps, - pathname, - projectHistoryId, - lines - ) await RedisManager.promises.updateDocument( projectId, docId, @@ -173,6 +168,15 @@ const UpdateManager = { ) profile.log('RedisManager.updateDocument') + UpdateManager._addMetadataToHistoryUpdates( + historyUpdates, + pathname, + projectHistoryId, + lines, + ranges, + historyRangesSupport + ) + if (historyUpdates.length > 0) { Metrics.inc('history-queue', 1, { status: 'project-history' }) try { @@ -304,16 +308,31 @@ const UpdateManager = { }, /** - * Add metadata to ops that will be useful to project history + * Add metadata that will be useful to project history * - * @param {Update[]} updates + * @param {HistoryUpdate[]} updates * @param {string} pathname * @param {string} projectHistoryId * @param {string[]} lines + * @param {Ranges} ranges + * @param {boolean} historyRangesSupport */ - _addProjectHistoryMetadataToOps(updates, pathname, projectHistoryId, lines) { + _addMetadataToHistoryUpdates( + updates, + pathname, + projectHistoryId, + lines, + ranges, + historyRangesSupport + ) { let docLength = _.reduce(lines, (chars, line) => chars + line.length, 0) docLength += lines.length - 1 // count newline characters + let historyDocLength = docLength + for (const change of ranges.changes ?? []) { + if ('d' in change.op) { + historyDocLength += change.op.d.length + } + } for (const update of updates) { update.projectHistoryId = projectHistoryId @@ -322,6 +341,10 @@ const UpdateManager = { } update.meta.pathname = pathname update.meta.doc_length = docLength + if (historyRangesSupport && historyDocLength !== docLength) { + update.meta.history_doc_length = historyDocLength + } + // Each update may contain multiple ops, i.e. // [{ // ops: [{i: "foo", p: 4}, {d: "bar", p:8}] @@ -334,9 +357,21 @@ const UpdateManager = { for (const op of update.op) { if (isInsert(op)) { docLength += op.i.length + if (!op.trackedDeleteRejection) { + // Tracked delete rejections end up retaining characters rather + // than inserting + historyDocLength += op.i.length + } } if (isDelete(op)) { docLength -= op.d.length + if (!update.meta.tc || op.u) { + // This is either a regular delete or a tracked insert rejection. + // It will be translated to a delete in history. Tracked deletes + // are translated into retains and don't change the history doc + // length. + historyDocLength -= op.d.length + } } } } diff --git a/services/document-updater/app/js/types.ts b/services/document-updater/app/js/types.ts index 224391a91d..8739723070 100644 --- a/services/document-updater/app/js/types.ts +++ b/services/document-updater/app/js/types.ts @@ -2,8 +2,6 @@ export type Update = { op: Op[] v: number meta?: { - pathname?: string - doc_length?: number tc?: boolean user_id?: string } @@ -82,6 +80,7 @@ export type HistoryUpdate = { meta?: { pathname?: string doc_length?: number + history_doc_length?: number tc?: boolean user_id?: string } diff --git a/services/document-updater/test/unit/js/UpdateManager/UpdateManagerTests.js b/services/document-updater/test/unit/js/UpdateManager/UpdateManagerTests.js index ab1fa8af9a..9bcbf0445b 100644 --- a/services/document-updater/test/unit/js/UpdateManager/UpdateManagerTests.js +++ b/services/document-updater/test/unit/js/UpdateManager/UpdateManagerTests.js @@ -12,6 +12,7 @@ describe('UpdateManager', function () { this.projectHistoryId = 'history-id-123' this.doc_id = 'document-id-123' this.lockValue = 'mock-lock-value' + this.pathname = '/a/b/c.tex' this.Metrics = { inc: sinon.stub(), @@ -324,7 +325,6 @@ describe('UpdateManager', function () { 'history-update-3', ] this.project_ops_length = 123 - this.pathname = '/a/b/c.tex' this.DocumentManager.promises.getDoc.resolves({ lines: this.lines, version: this.version, @@ -344,7 +344,7 @@ describe('UpdateManager', function () { appliedOps: this.appliedOps, }) this.RedisManager.promises.updateDocument.resolves() - this.UpdateManager.promises._addProjectHistoryMetadataToOps = sinon.stub() + this.UpdateManager.promises._addMetadataToHistoryUpdates = sinon.stub() }) describe('normally', function () { @@ -395,7 +395,7 @@ describe('UpdateManager', function () { }) it('should add metadata to the ops', function () { - this.UpdateManager.promises._addProjectHistoryMetadataToOps.should.have.been.calledWith( + this.UpdateManager.promises._addMetadataToHistoryUpdates.should.have.been.calledWith( this.appliedOps, this.pathname, this.projectHistoryId, @@ -523,13 +523,14 @@ describe('UpdateManager', function () { }) }) - describe('_addProjectHistoryMetadataToOps', function () { + describe('_addMetadataToHistoryUpdates', function () { it('should add projectHistoryId, pathname and doc_length metadata to the ops', function () { const lines = ['some', 'test', 'data'] - const appliedOps = [ + const historyUpdates = [ { v: 42, op: [ + { i: 'bing', p: 12, trackedDeleteRejection: true }, { i: 'foo', p: 4 }, { i: 'bar', p: 6 }, ], @@ -539,27 +540,45 @@ describe('UpdateManager', function () { op: [ { d: 'qux', p: 4 }, { i: 'bazbaz', p: 14 }, + { d: 'bong', p: 28, u: true }, ], + meta: { + tc: 'tracking-info', + }, + }, + { + v: 47, + op: [{ d: 'so', p: 0 }], }, { v: 49, op: [{ i: 'penguin', p: 18 }] }, ] - this.UpdateManager._addProjectHistoryMetadataToOps( - appliedOps, + const ranges = { + changes: [ + { op: { d: 'bingbong', p: 12 } }, + { op: { i: 'test', p: 5 } }, + ], + } + this.UpdateManager._addMetadataToHistoryUpdates( + historyUpdates, this.pathname, this.projectHistoryId, - lines + lines, + ranges, + true ) - appliedOps.should.deep.equal([ + historyUpdates.should.deep.equal([ { projectHistoryId: this.projectHistoryId, v: 42, op: [ + { i: 'bing', p: 12, trackedDeleteRejection: true }, { i: 'foo', p: 4 }, { i: 'bar', p: 6 }, ], meta: { pathname: this.pathname, doc_length: 14, + history_doc_length: 22, }, }, { @@ -568,11 +587,24 @@ describe('UpdateManager', function () { op: [ { d: 'qux', p: 4 }, { i: 'bazbaz', p: 14 }, + { d: 'bong', p: 28, u: true }, ], meta: { pathname: this.pathname, - doc_length: 20, - }, // 14 + 'foo' + 'bar' + doc_length: 24, // 14 + 'bing' + 'foo' + 'bar' + history_doc_length: 28, // 22 + 'foo' + 'bar' + tc: 'tracking-info', + }, + }, + { + projectHistoryId: this.projectHistoryId, + v: 47, + op: [{ d: 'so', p: 0 }], + meta: { + pathname: this.pathname, + doc_length: 23, // 24 - 'qux' + 'bazbaz' - 'bong' + history_doc_length: 30, // 28 - 'bong' + 'bazbaz' + }, }, { projectHistoryId: this.projectHistoryId, @@ -580,8 +612,9 @@ describe('UpdateManager', function () { op: [{ i: 'penguin', p: 18 }], meta: { pathname: this.pathname, - doc_length: 23, - }, // 14 - 'qux' + 'bazbaz' + doc_length: 21, // 23 - 'so' + history_doc_length: 28, // 30 - 'so' + }, }, ]) })