From 4e80d65efcda6e5725a38bd4f012a67b64e3d255 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Thu, 20 Aug 2026 08:20:35 +0200 Subject: [PATCH] [ranges-tracker] add legacy orderedRejections field in tracked-changes (#36565) * [docstore] use more realistic test stubs for ranges * [monorepo] define ranges schemas in @overleaf/ranges-tracker * [ranges-tracker] add legacy orderedRejections field in tracked-changes * [monorepo] resolve @overleaf/ranges-tracker subpath imports in typescript * [real-time] validate comment thread ids and tracked change id seeds * [ranges-tracker] require ObjectId ids and tracked change metadata in ranges * [monorepo] require ObjectId tracked change ids on the accept routes * [web] use ObjectId comment thread ids in the DocumentController tests GitOrigin-RevId: 58b20dec311d85b917ef63f9efbe745a265a2de6 --- libraries/ranges-tracker/package.json | 4 + libraries/ranges-tracker/schemas.js | 88 ++++++++ .../ranges-tracker/test/unit/schemas.test.js | 129 ++++++++++++ services/docstore/app/js/HttpController.js | 4 +- services/docstore/app/js/schemas.js | 66 ------ services/docstore/package.json | 1 + .../test/acceptance/js/GettingAllDocsTests.js | 48 ++++- .../docstore/test/unit/js/schemas.test.js | 27 --- .../document-updater/app/js/HttpController.js | 9 +- services/document-updater/app/js/schemas.js | 72 ------- services/document-updater/app/js/types.ts | 14 +- .../test/acceptance/js/RangesTests.js | 8 +- .../test/acceptance/js/helpers/MockWebApi.js | 2 +- .../test/unit/js/schemas.test.js | 36 ---- services/real-time/app/js/Router.js | 10 +- .../test/acceptance/js/ApplyUpdateTests.js | 197 +++++++++++++++++- .../Features/Documents/DocumentController.mjs | 61 +----- services/web/package.json | 1 + .../src/mocks/MockDocUpdaterApi.mjs | 45 +--- .../acceptance/src/mocks/MockDocstoreApi.mjs | 43 +--- .../src/Documents/DocumentController.test.mjs | 16 +- tsconfig.backend.json | 1 + yarn.lock | 2 + 23 files changed, 501 insertions(+), 383 deletions(-) create mode 100644 libraries/ranges-tracker/schemas.js create mode 100644 libraries/ranges-tracker/test/unit/schemas.test.js delete mode 100644 services/docstore/app/js/schemas.js delete mode 100644 services/docstore/test/unit/js/schemas.test.js delete mode 100644 services/document-updater/app/js/schemas.js delete mode 100644 services/document-updater/test/unit/js/schemas.test.js diff --git a/libraries/ranges-tracker/package.json b/libraries/ranges-tracker/package.json index 023bd01d43..01e96d06b0 100644 --- a/libraries/ranges-tracker/package.json +++ b/libraries/ranges-tracker/package.json @@ -4,6 +4,7 @@ "main": "index.cjs", "files": [ "index.cjs", + "schemas.js", "types" ], "author": "Overleaf (https://www.overleaf.com)", @@ -16,6 +17,9 @@ "test:unit": "mocha --exit test/**/*.{js,cjs}", "types:check": "tsc --noEmit" }, + "dependencies": { + "@overleaf/validation-tools": "workspace:*" + }, "devDependencies": { "@types/node": "^24.5.2", "chai": "^4.3.6", diff --git a/libraries/ranges-tracker/schemas.js b/libraries/ranges-tracker/schemas.js new file mode 100644 index 0000000000..8106fd373e --- /dev/null +++ b/libraries/ranges-tracker/schemas.js @@ -0,0 +1,88 @@ +// @ts-check +'use strict' + +const { z, zz } = require('@overleaf/validation-tools') + +// Zod schemas for the ranges data (RangesTracker format) accepted by +// updateDoc. +// +// Ranges reach docstore via web from several writers and are stored (and +// later round-tripped, e.g. by project clone) without normalization, so the +// schemas model wire reality rather than the ideal shape: +// - document-updater flushes RangesTracker data: a comment's id is its thread +// id (docstore's RangeManager keeps the two in sync) and a tracked change's +// id is a RangesTracker id, i.e. an 18 character seed plus a 6 character +// increment, see libraries/ranges-tracker/index.cjs +// - history restores send id-less changes and comment ops with a `resolved` +// flag (stripped again by RangeManager), and detached comments (a zero +// length comment range at position 0), see +// overleaf-editor-core/lib/doc_updater_compatible_ranges.js +// - old tracked changes created while the fixedRemoveChange flag existed +// (removed in https://github.com/overleaf/internal/pull/23993) still carry +// it in their op until accepted/rejected +// - old tracked changes created while the orderedRejections flag existed +// (added in https://github.com/overleaf/internal/pull/22650) still carry +// it in their op until accepted/rejected. + +const insertOp = z.strictObject({ + i: z.string(), + p: z.number().int().min(0), + u: z.boolean().optional(), + fixedRemoveChange: z.boolean().optional(), + orderedRejections: z.boolean().optional(), +}) + +const deleteOp = z.strictObject({ + d: z.string(), + p: z.number().int().min(0), + u: z.boolean().optional(), + fixedRemoveChange: z.boolean().optional(), + orderedRejections: z.boolean().optional(), +}) + +const commentOp = z.strictObject({ + c: z.string(), + p: z.number().int().min(0), + t: zz.objectId(), + u: z.boolean().optional(), + // sent by history restores; removed again by RangeManager + resolved: z.boolean().optional(), +}) + +const commentMetadata = z.strictObject({ + user_id: z.string().optional(), + ts: z.string().optional(), +}) + +// tracked changes always carry the author and the time of the edit: both +// RangesTracker and history restores set them when creating the change +const trackedChangeMetadata = z.strictObject({ + user_id: z.string(), + ts: z.string(), +}) + +const comment = z.strictObject({ + id: zz.objectId().optional(), + op: commentOp, + metadata: commentMetadata.optional(), +}) + +const trackedChange = z.strictObject({ + id: zz.objectId().optional(), + op: insertOp.or(deleteOp), + metadata: trackedChangeMetadata, +}) + +const ranges = z.strictObject({ + comments: z.array(comment).optional(), + changes: z.array(trackedChange).optional(), +}) + +module.exports = { + insertOp, + deleteOp, + commentOp, + comment, + trackedChange, + ranges, +} diff --git a/libraries/ranges-tracker/test/unit/schemas.test.js b/libraries/ranges-tracker/test/unit/schemas.test.js new file mode 100644 index 0000000000..07c3e2875a --- /dev/null +++ b/libraries/ranges-tracker/test/unit/schemas.test.js @@ -0,0 +1,129 @@ +const { expect } = require('chai') +const { ranges } = require('../../schemas') + +const metadata = Object.freeze({ + user_id: '65a4f2c3b1e4d5a6f7089123', + ts: '2026-08-14T10:47:58.000Z', +}) +const threadId = '65a4f2c3b1e4d5a6f7089124' +// an 18 character RangesTracker id seed plus its 6 character increment +const changeId = '65a4f2c3b1e4d5a6f7' + '000001' + +describe('schemas', function () { + describe('ranges', function () { + it('accepts a legacy fixedRemoveChange flag on an insert op', function () { + const result = ranges.safeParse({ + changes: [ + { op: { i: 'foo', p: 0, fixedRemoveChange: true }, metadata }, + ], + }) + expect(result.success).to.equal(true) + }) + + it('accepts a legacy fixedRemoveChange flag on a delete op', function () { + const result = ranges.safeParse({ + changes: [ + { op: { d: 'foo', p: 0, fixedRemoveChange: true }, metadata }, + ], + }) + expect(result.success).to.equal(true) + }) + + it('accepts a legacy orderedRejections flag on an insert op', function () { + const result = ranges.safeParse({ + changes: [ + { op: { i: 'foo', p: 0, orderedRejections: true }, metadata }, + ], + }) + expect(result.success).to.equal(true) + }) + + it('accepts a legacy orderedRejections flag on a delete op', function () { + const result = ranges.safeParse({ + changes: [ + { op: { d: 'foo', p: 0, orderedRejections: true }, metadata }, + ], + }) + expect(result.success).to.equal(true) + }) + + it('rejects an unrecognized key on an insert op', function () { + const result = ranges.safeParse({ + changes: [{ op: { i: 'foo', p: 0, somethingElse: true }, metadata }], + }) + expect(result.success).to.equal(false) + }) + + it('rejects a tracked change without metadata', function () { + const result = ranges.safeParse({ + changes: [{ op: { i: 'foo', p: 0 } }], + }) + expect(result.success).to.equal(false) + }) + + it('rejects a tracked change without a user_id', function () { + const result = ranges.safeParse({ + changes: [{ op: { i: 'foo', p: 0 }, metadata: { ts: metadata.ts } }], + }) + expect(result.success).to.equal(false) + }) + + it('accepts a tracked change with a RangesTracker id', function () { + const result = ranges.safeParse({ + changes: [{ id: changeId, op: { i: 'foo', p: 0 }, metadata }], + }) + expect(result.success).to.equal(true) + }) + + it('rejects a tracked change with a non-ObjectId id', function () { + const result = ranges.safeParse({ + changes: [{ id: 'tc_1000001', op: { i: 'foo', p: 0 }, metadata }], + }) + expect(result.success).to.equal(false) + }) + + it('rejects a tracked change without a ts', function () { + const result = ranges.safeParse({ + changes: [ + { op: { i: 'foo', p: 0 }, metadata: { user_id: metadata.user_id } }, + ], + }) + expect(result.success).to.equal(false) + }) + + it('accepts a comment op with an ObjectId thread id', function () { + const result = ranges.safeParse({ + comments: [{ op: { c: 'foo', p: 0, t: threadId } }], + }) + expect(result.success).to.equal(true) + }) + + it('rejects a comment op with a non-ObjectId thread id', function () { + const result = ranges.safeParse({ + comments: [{ op: { c: 'foo', p: 0, t: 'thread-id-1' } }], + }) + expect(result.success).to.equal(false) + }) + + it('rejects a comment op without a thread id', function () { + const result = ranges.safeParse({ + comments: [{ op: { c: 'foo', p: 0 } }], + }) + expect(result.success).to.equal(false) + }) + + it('accepts a comment with an ObjectId id', function () { + const result = ranges.safeParse({ + comments: [{ id: threadId, op: { c: 'foo', p: 0, t: threadId } }], + }) + expect(result.success).to.equal(true) + }) + + it('rejects a comment with a non-ObjectId id', function () { + const result = ranges.safeParse({ + comments: [{ id: 'thread-id-1', op: { c: 'foo', p: 0, t: threadId } }], + }) + expect(result.success).to.equal(false) + }) + }) +}) diff --git a/services/docstore/app/js/HttpController.js b/services/docstore/app/js/HttpController.js index 85a3c8f206..b732aa10cf 100644 --- a/services/docstore/app/js/HttpController.js +++ b/services/docstore/app/js/HttpController.js @@ -6,7 +6,7 @@ import Errors from './Errors.js' import Settings from '@overleaf/settings' import { expressify } from '@overleaf/promise-utils' import { parseReq, z, zz } from '@overleaf/validation-tools' -import * as schemas from './schemas.js' +import rangesSchemas from '@overleaf/ranges-tracker/schemas.js' const projectParamsSchema = z.object({ params: z.strictObject({ @@ -73,7 +73,7 @@ const updateDocSchema = z.object({ body: z.strictObject({ lines: z.array(z.string()), version: z.number(), - ranges: schemas.ranges, + ranges: rangesSchemas.ranges, }), }) diff --git a/services/docstore/app/js/schemas.js b/services/docstore/app/js/schemas.js deleted file mode 100644 index 805193dc39..0000000000 --- a/services/docstore/app/js/schemas.js +++ /dev/null @@ -1,66 +0,0 @@ -// @ts-check -import { z } from '@overleaf/validation-tools' - -// Zod schemas for the ranges data (RangesTracker format) accepted by -// updateDoc. -// -// Ranges reach docstore via web from several writers and are stored (and -// later round-tripped, e.g. by project clone) without normalization, so the -// schemas model wire reality rather than the ideal shape: -// - document-updater flushes RangesTracker data: change/comment ids are -// RangesTracker ids (seed + increment) or thread ids, i.e. strings that are -// not always ObjectIds (and old fixtures/documents carry arbitrary string -// thread ids), see libraries/ranges-tracker/index.cjs -// - history restores send id-less changes and comment ops with a `resolved` -// flag (stripped again by RangeManager), and detached comments without an -// id, see overleaf-editor-core/lib/doc_updater_compatible_ranges.js -// - old tracked changes created while the fixedRemoveChange flag existed -// (removed in https://github.com/overleaf/internal/pull/23993) still carry -// it in their op until accepted/rejected - -const insertOp = z.strictObject({ - i: z.string(), - p: z.number().int().min(0), - u: z.boolean().optional(), - fixedRemoveChange: z.boolean().optional(), -}) - -const deleteOp = z.strictObject({ - d: z.string(), - p: z.number().int().min(0), - u: z.boolean().optional(), - fixedRemoveChange: z.boolean().optional(), -}) - -const commentOp = z.strictObject({ - c: z.string().optional(), - p: z.number().int().min(0).optional(), - // comment thread ids are usually ObjectIds, but legacy documents carry - // arbitrary strings - t: z.string().optional(), - u: z.boolean().optional(), - // sent by history restores; removed again by RangeManager - resolved: z.boolean().optional(), -}) - -const rangeMetadata = z.strictObject({ - user_id: z.string().optional(), - ts: z.string().optional(), -}) - -const comment = z.strictObject({ - id: z.string().optional(), - op: commentOp, - metadata: rangeMetadata.optional(), -}) - -const trackedChange = z.strictObject({ - id: z.string().optional(), - op: insertOp.or(deleteOp).optional(), - metadata: rangeMetadata.optional(), -}) - -export const ranges = z.strictObject({ - comments: z.array(comment).optional(), - changes: z.array(trackedChange).optional(), -}) diff --git a/services/docstore/package.json b/services/docstore/package.json index 31dcf1a7a7..5bc9b32364 100644 --- a/services/docstore/package.json +++ b/services/docstore/package.json @@ -23,6 +23,7 @@ "@overleaf/o-error": "workspace:*", "@overleaf/object-persistor": "workspace:*", "@overleaf/promise-utils": "workspace:*", + "@overleaf/ranges-tracker": "workspace:*", "@overleaf/settings": "workspace:*", "@overleaf/stream-utils": "^0.1.0", "@overleaf/validation-tools": "workspace:*", diff --git a/services/docstore/test/acceptance/js/GettingAllDocsTests.js b/services/docstore/test/acceptance/js/GettingAllDocsTests.js index 05e83f7d34..1427c22fca 100644 --- a/services/docstore/test/acceptance/js/GettingAllDocsTests.js +++ b/services/docstore/test/acceptance/js/GettingAllDocsTests.js @@ -17,12 +17,23 @@ describe('Getting all docs', function () { lines: ['one', 'two', 'three'], ranges: { comments: [ - { id: new ObjectId().toString(), op: { t: this.threadId1 } }, + { + id: new ObjectId().toString(), + op: { + c: 'c', + p: 0, + t: this.threadId1, + }, + }, ], changes: [ { id: new ObjectId().toString(), - metadata: { user_id: 'user-id-1' }, + op: { d: 'd', p: 1 }, + metadata: { + user_id: 'user-id-1', + ts: new Date().toJSON(), + }, }, ], }, @@ -35,7 +46,11 @@ describe('Getting all docs', function () { changes: [ { id: new ObjectId().toString(), - metadata: { user_id: 'user-id-2' }, + op: { i: 'd', p: 1 }, + metadata: { + user_id: 'user-id-2', + ts: new Date().toJSON(), + }, }, ], }, @@ -46,12 +61,19 @@ describe('Getting all docs', function () { lines: ['111', '222', '333'], ranges: { comments: [ - { id: new ObjectId().toString(), op: { t: this.threadId2 } }, + { + id: new ObjectId().toString(), + op: { c: 'c', p: 1, t: this.threadId2 }, + }, ], changes: [ { id: new ObjectId().toString(), - metadata: { user_id: 'anonymous-user' }, + op: { d: 'x', p: 1 }, + metadata: { + user_id: 'anonymous-user', + ts: new Date().toJSON(), + }, }, ], }, @@ -71,9 +93,21 @@ describe('Getting all docs', function () { _id: new ObjectId(), lines: ['deleted'], ranges: { - comments: [{ id: new ObjectId().toString(), op: { t: 'thread-id-3' } }], + comments: [ + { + id: new ObjectId().toString(), + op: { c: 'c', p: 1, t: new ObjectId().toString() }, + }, + ], changes: [ - { id: new ObjectId().toString(), metadata: { user_id: 'user-id-3' } }, + { + id: new ObjectId().toString(), + op: { d: 'd', p: 0 }, + metadata: { + user_id: 'user-id-3', + ts: new Date().toJSON(), + }, + }, ], }, rev: 8, diff --git a/services/docstore/test/unit/js/schemas.test.js b/services/docstore/test/unit/js/schemas.test.js deleted file mode 100644 index 8998a9e514..0000000000 --- a/services/docstore/test/unit/js/schemas.test.js +++ /dev/null @@ -1,27 +0,0 @@ -import { describe, it, expect } from 'vitest' -import { ranges } from '../../../app/js/schemas.js' - -describe('schemas', function () { - describe('ranges', function () { - it('accepts a legacy fixedRemoveChange flag on an insert op', function () { - const result = ranges.safeParse({ - changes: [{ op: { i: 'foo', p: 0, fixedRemoveChange: true } }], - }) - expect(result.success).to.equal(true) - }) - - it('accepts a legacy fixedRemoveChange flag on a delete op', function () { - const result = ranges.safeParse({ - changes: [{ op: { d: 'foo', p: 0, fixedRemoveChange: true } }], - }) - expect(result.success).to.equal(true) - }) - - it('rejects an unrecognized key on an insert op', function () { - const result = ranges.safeParse({ - changes: [{ op: { i: 'foo', p: 0, somethingElse: true } }], - }) - expect(result.success).to.equal(false) - }) - }) -}) diff --git a/services/document-updater/app/js/HttpController.js b/services/document-updater/app/js/HttpController.js index 085d8b3f0d..8bd380fbb5 100644 --- a/services/document-updater/app/js/HttpController.js +++ b/services/document-updater/app/js/HttpController.js @@ -1,7 +1,7 @@ const { expressify } = require('@overleaf/promise-utils') const { parseReq, z, zz } = require('@overleaf/validation-tools') const editorCoreSchemas = require('overleaf-editor-core/lib/schemas') -const schemas = require('./schemas') +const rangesSchemas = require('@overleaf/ranges-tracker/schemas') const DocumentManager = require('./DocumentManager') const HistoryManager = require('./HistoryManager') const ProjectManager = require('./ProjectManager') @@ -487,15 +487,14 @@ async function deleteMultipleProjects(req, res) { res.sendStatus(204) // No Content } -// tracked-change ids are RangesTracker ids (seed + increment), not ObjectIds const acceptChangesSchema = z.object({ params: z.strictObject({ project_id: zz.objectId(), doc_id: zz.objectId(), - change_id: z.string().min(1).optional(), + change_id: zz.objectId().optional(), }), body: z.strictObject({ - change_ids: z.array(z.string().min(1)).optional(), + change_ids: z.array(zz.objectId()).optional(), }), }) @@ -610,7 +609,7 @@ const addUpdateSchema = z.strictObject({ id: zz.objectId(), pathname: zz.safePath(), docLines: z.string().optional(), - ranges: schemas.ranges.optional(), + ranges: rangesSchemas.ranges.optional(), historyRangesSupport: z.boolean().optional(), // legacy filestore url for files without a created blob url: z.string().nullish(), diff --git a/services/document-updater/app/js/schemas.js b/services/document-updater/app/js/schemas.js deleted file mode 100644 index 594c0bce5a..0000000000 --- a/services/document-updater/app/js/schemas.js +++ /dev/null @@ -1,72 +0,0 @@ -// @ts-check -'use strict' - -const { z, zz } = require('@overleaf/validation-tools') - -// Zod schemas for the sharejs-text-ot ranges data (RangesTracker format). -// The types in ./types.ts are inferred from these schemas. -// -// Ranges data is persisted round-trip (docstore <-> web <-> document-updater) -// without normalization, so legacy shapes survive on old documents and the -// schemas model them explicitly: comments/changes preloaded from web may lack `id`. -// Tightening these is tracked as part of moving the shared editor payload -// schemas into overleaf-editor-core. -// -// Old tracked changes created while the fixedRemoveChange flag existed -// (removed in https://github.com/overleaf/internal/pull/23993) still carry -// it in their op until accepted/rejected. - -const insertOp = z.strictObject({ - i: z.string(), - p: z.number().int().min(0), - u: z.boolean().optional(), - fixedRemoveChange: z.boolean().optional(), -}) - -const deleteOp = z.strictObject({ - d: z.string(), - p: z.number().int().min(0), - u: z.boolean().optional(), - fixedRemoveChange: z.boolean().optional(), -}) - -const commentOp = z.strictObject({ - c: z.string(), - p: z.number().int().min(0), - t: zz.objectId(), - u: z.boolean().optional(), - // Used by project-history when restoring CommentSnapshots - resolved: z.boolean().optional(), -}) - -const rangeMetadata = z.strictObject({ - user_id: z.string(), - ts: z.string(), -}) - -const comment = z.strictObject({ - id: zz.objectId().optional(), - op: commentOp, - metadata: rangeMetadata.optional(), -}) - -// tracked-change ids are RangesTracker ids (seed + increment), not ObjectIds -const trackedChange = z.strictObject({ - id: z.string().optional(), - op: insertOp.or(deleteOp), - metadata: rangeMetadata, -}) - -const ranges = z.strictObject({ - comments: z.array(comment).optional(), - changes: z.array(trackedChange).optional(), -}) - -module.exports = { - insertOp, - deleteOp, - commentOp, - comment, - trackedChange, - ranges, -} diff --git a/services/document-updater/app/js/types.ts b/services/document-updater/app/js/types.ts index 83ff492eb5..6464a6b94c 100644 --- a/services/document-updater/app/js/types.ts +++ b/services/document-updater/app/js/types.ts @@ -4,7 +4,7 @@ import { RawEditOperation, } from 'overleaf-editor-core/lib/types' import type { z } from '@overleaf/validation-tools' -import type * as schemas from './schemas' +import type rangesSchemas from '@overleaf/ranges-tracker/schemas' export type OTType = 'sharejs-text-ot' | 'history-ot' @@ -32,25 +32,25 @@ export type HistoryOTEditOperationUpdate = Omit & { export type Op = InsertOp | DeleteOp | CommentOp | RetainOp -export type InsertOp = z.infer +export type InsertOp = z.infer export type RetainOp = { r: string p: number } -export type DeleteOp = z.infer +export type DeleteOp = z.infer -export type CommentOp = z.infer +export type CommentOp = z.infer /** * Ranges record on a document */ -export type Ranges = z.infer +export type Ranges = z.infer -export type Comment = z.infer +export type Comment = z.infer -export type TrackedChange = z.infer +export type TrackedChange = z.infer /** * Updates sent to project-history diff --git a/services/document-updater/test/acceptance/js/RangesTests.js b/services/document-updater/test/acceptance/js/RangesTests.js index 2ec36bb8fd..e55e817e70 100644 --- a/services/document-updater/test/acceptance/js/RangesTests.js +++ b/services/document-updater/test/acceptance/js/RangesTests.js @@ -307,9 +307,9 @@ describe('Ranges', function () { historyRangesSupport: true, }) await DocUpdaterClient.preloadDoc(this.project_id, this.doc.id) - this.id_seed_1 = 'tc_1' - this.id_seed_2 = 'tc_2' - this.id_seed_3 = 'tc_3' + this.id_seed_1 = '587357bd35e64f6151' + this.id_seed_2 = '587357bd35e64f6152' + this.id_seed_3 = '587357bd35e64f6153' this.updates = [ { @@ -718,7 +718,7 @@ describe('Ranges', function () { before(async function () { this.project_id = DocUpdaterClient.randomId() this.user_id = DocUpdaterClient.randomId() - this.id_seed = DocUpdaterClient.randomId() + this.id_seed = '587357bd35e64f6157' this.doc = { id: DocUpdaterClient.randomId(), lines: ['aaa'], diff --git a/services/document-updater/test/acceptance/js/helpers/MockWebApi.js b/services/document-updater/test/acceptance/js/helpers/MockWebApi.js index 07447bd38e..c8483a6bd5 100644 --- a/services/document-updater/test/acceptance/js/helpers/MockWebApi.js +++ b/services/document-updater/test/acceptance/js/helpers/MockWebApi.js @@ -11,7 +11,7 @@ const { zz, } = require('@overleaf/validation-tools') const { rawStringFileData } = require('overleaf-editor-core/lib/schemas') -const schemas = require('../../../../app/js/schemas') +const schemas = require('@overleaf/ranges-tracker/schemas') const app = express() const MAX_REQUEST_SIZE = 2 * (2 * 1024 * 1024 + 64 * 1024) diff --git a/services/document-updater/test/unit/js/schemas.test.js b/services/document-updater/test/unit/js/schemas.test.js deleted file mode 100644 index f3727f737c..0000000000 --- a/services/document-updater/test/unit/js/schemas.test.js +++ /dev/null @@ -1,36 +0,0 @@ -const { expect } = require('chai') -const { insertOp, deleteOp } = require('../../../app/js/schemas') - -describe('schemas', function () { - describe('insertOp', function () { - it('accepts a legacy fixedRemoveChange flag', function () { - const result = insertOp.safeParse({ - i: 'foo', - p: 0, - fixedRemoveChange: true, - }) - expect(result.success).to.equal(true) - }) - - it('rejects an unrecognized key', function () { - const result = insertOp.safeParse({ i: 'foo', p: 0, somethingElse: true }) - expect(result.success).to.equal(false) - }) - }) - - describe('deleteOp', function () { - it('accepts a legacy fixedRemoveChange flag', function () { - const result = deleteOp.safeParse({ - d: 'foo', - p: 0, - fixedRemoveChange: true, - }) - expect(result.success).to.equal(true) - }) - - it('rejects an unrecognized key', function () { - const result = deleteOp.safeParse({ d: 'foo', p: 0, somethingElse: true }) - expect(result.success).to.equal(false) - }) - }) -}) diff --git a/services/real-time/app/js/Router.js b/services/real-time/app/js/Router.js index 840470151d..80f0cd4591 100644 --- a/services/real-time/app/js/Router.js +++ b/services/real-time/app/js/Router.js @@ -40,7 +40,10 @@ const applyOtUpdateSchema = z.strictObject({ lastV: z.number().optional(), meta: z .strictObject({ - tc: z.string().optional(), + // the seed RangesTracker builds tracked-change ids from: the first 18 + // characters of an ObjectId, leaving 6 for the increment part, see + // RangesTracker.generateIdSeed + tc: zz.hex().length(18).optional(), }) .optional(), op: z @@ -62,10 +65,7 @@ const applyOtUpdateSchema = z.strictObject({ z.strictObject({ c: z.string(), p: z.number().int().min(0), - // comment thread ids are usually ObjectIds, but legacy - // documents carry arbitrary strings (see docstore's ranges - // schema for the same caveat) -- kept as a plain string - t: z.string(), + t: zz.objectId(), u: z.boolean().optional(), }) ) diff --git a/services/real-time/test/acceptance/js/ApplyUpdateTests.js b/services/real-time/test/acceptance/js/ApplyUpdateTests.js index e1a4b61457..8c52b5c0a2 100644 --- a/services/real-time/test/acceptance/js/ApplyUpdateTests.js +++ b/services/real-time/test/acceptance/js/ApplyUpdateTests.js @@ -340,7 +340,7 @@ describe('applyOtUpdate', function () { describe('when authorized to read-only with a comment update', function () { before(function (done) { this.comment_update = { - op: [{ c: 'foo', p: 42, t: '1234' }], + op: [{ c: 'foo', p: 42, t: FixturesManager.getRandomId() }], v: 42, } return async.series( @@ -930,6 +930,201 @@ describe('applyOtUpdate', function () { }) }) + describe('when authorized with a tracked change id seed', function () { + before(function (done) { + // RangesTracker seeds are the first 18 characters of an ObjectId + this.id_seed = FixturesManager.getRandomId().slice(0, 18) + this.update = { + op: [{ i: 'foo', p: 42 }], + v: 42, + meta: { tc: this.id_seed }, + } + return async.series( + [ + cb => { + return FixturesManager.setUpProject( + { + privilegeLevel: 'readAndWrite', + }, + (e, { project_id: projectId, user_id: userId }) => { + this.project_id = projectId + this.user_id = userId + return cb(e) + } + ) + }, + + cb => { + return FixturesManager.setUpDoc( + this.project_id, + { lines: this.lines, version: this.version, ops: this.ops }, + (e, { doc_id: docId }) => { + this.doc_id = docId + return cb(e) + } + ) + }, + + cb => { + this.client = RealTimeClient.connect(this.project_id, cb) + }, + + cb => { + return this.client.emit('joinDoc', this.doc_id, cb) + }, + + cb => { + return this.client.emit( + 'applyOtUpdate', + this.doc_id, + this.update, + cb + ) + }, + ], + done + ) + }) + + it('should push the project into the pending updates list', function (done) { + getPendingUpdatesList((error, ...rest) => { + if (error) return done(error) + const [projectId] = Array.from(rest[0]) + projectId.should.equal(this.project_id) + return done() + }) + return null + }) + + it('should push the update into redis with the id seed', function (done) { + rclient.lrange( + redisSettings.documentupdater.key_schema.pendingProjectUpdates({ + project_id: this.project_id, + }), + 0, + -1, + (error, ...rest) => { + if (error) return done(error) + let [update] = Array.from(rest[0]) + update = JSON.parse(update) + update.op.should.deep.equal(this.update.op) + update.meta.should.include({ + source: this.client.publicId, + user_id: this.user_id, + tc: this.id_seed, + }) + return done() + } + ) + return null + }) + + return after(function (done) { + return async.series( + [ + cb => clearPendingUpdatesList(cb), + cb => + rclient.del( + 'DocsWithPendingUpdates', + `${this.project_id}:${this.doc_id}`, + cb + ), + cb => + rclient.del( + redisSettings.documentupdater.key_schema.pendingProjectUpdates({ + project_id: this.project_id, + }), + cb + ), + ], + done + ) + }) + }) + + describe('when authorized with a malformed tracked change id seed', function () { + before(function (done) { + this.update = { + op: [{ i: 'foo', p: 42 }], + v: 42, + meta: { tc: 'not-an-id-seed' }, + } + return async.series( + [ + cb => { + return FixturesManager.setUpProject( + { + privilegeLevel: 'readAndWrite', + }, + (e, { project_id: projectId, user_id: userId }) => { + this.project_id = projectId + this.user_id = userId + return cb(e) + } + ) + }, + + cb => { + return FixturesManager.setUpDoc( + this.project_id, + { lines: this.lines, version: this.version, ops: this.ops }, + (e, { doc_id: docId }) => { + this.doc_id = docId + return cb(e) + } + ) + }, + + cb => { + this.client = RealTimeClient.connect(this.project_id, cb) + }, + + cb => { + return this.client.emit('joinDoc', this.doc_id, cb) + }, + + cb => { + return this.client.emit( + 'applyOtUpdate', + this.doc_id, + this.update, + error => { + this.error = error + return cb() + } + ) + }, + ], + done + ) + }) + + it('should return an error', function () { + return expect(this.error).to.exist + }) + + it('should disconnect the client', function (done) { + return setTimeout(() => { + this.client.socket.connected.should.equal(false) + return done() + }, 300) + }) + + return it('should not put the update in redis', function (done) { + rclient.llen( + redisSettings.documentupdater.key_schema.pendingProjectUpdates({ + project_id: this.project_id, + }), + (error, len) => { + if (error) return done(error) + len.should.equal(0) + return done() + } + ) + return null + }) + }) + describe('when authorized with unknown fields', function () { before(function (done) { this.update = { diff --git a/services/web/app/src/Features/Documents/DocumentController.mjs b/services/web/app/src/Features/Documents/DocumentController.mjs index 1af69dcc87..fb7b695b59 100644 --- a/services/web/app/src/Features/Documents/DocumentController.mjs +++ b/services/web/app/src/Features/Documents/DocumentController.mjs @@ -9,64 +9,7 @@ import { plainTextResponse } from '../../infrastructure/Response.mjs' import { expressify } from '@overleaf/promise-utils' import Modules from '../../infrastructure/Modules.mjs' import { z, zz, parseReq } from '../../infrastructure/Validation.mjs' - -// Zod schemas for the sharejs-text-ot ranges data (RangesTracker format) -// that document-updater flushes to this endpoint -- NOT the unrelated -// overleaf-editor-core StringFileData rawComment/rawTrackedChange shape -// (that's the canonical history-ot representation, a different shape used -// elsewhere for linked-file/history payloads). -// -// This mirrors document-updater's own app/js/schemas.js (the sender) and -// docstore's app/js/schemas.js (the next hop, which this data is forwarded -// to untouched via DocstoreManager) -- see those files' comments for why the -// fields are this permissive: ids aren't always ObjectIds (RangesTracker ids -// are seed+increment strings; legacy documents carry arbitrary string thread -// ids), and history restores send id-less changes/detached comments plus a -// `resolved` flag. -const insertOp = z.strictObject({ - i: z.string(), - p: z.number().int().min(0), - u: z.boolean().optional(), - fixedRemoveChange: z.boolean().optional(), -}) - -const deleteOp = z.strictObject({ - d: z.string(), - p: z.number().int().min(0), - u: z.boolean().optional(), - fixedRemoveChange: z.boolean().optional(), -}) - -const commentOp = z.strictObject({ - c: z.string().optional(), - p: z.number().int().min(0).optional(), - t: z.string().optional(), - u: z.boolean().optional(), - // sent by history restores; removed again by RangesManager - resolved: z.boolean().optional(), -}) - -const rangeMetadata = z.strictObject({ - user_id: z.string().optional(), - ts: z.string().optional(), -}) - -const comment = z.strictObject({ - id: z.string().optional(), - op: commentOp, - metadata: rangeMetadata.optional(), -}) - -const trackedChange = z.strictObject({ - id: z.string().optional(), - op: insertOp.or(deleteOp).optional(), - metadata: rangeMetadata.optional(), -}) - -const rangesSchema = z.strictObject({ - comments: z.array(comment).optional(), - changes: z.array(trackedChange).optional(), -}) +import rangesSchemas from '@overleaf/ranges-tracker/schemas.js' const getDocumentSchema = z.object({ params: z.strictObject({ @@ -177,7 +120,7 @@ const setDocumentSchema = z.object({ // history-ot docs) -- never the raw StringFileData shape. lines: z.array(z.string()), version: z.number().int(), - ranges: rangesSchema, + ranges: rangesSchemas.ranges, lastUpdatedAt: z.coerce.number().int().positive().nullish(), lastUpdatedBy: zz.objectId().nullish(), }), diff --git a/services/web/package.json b/services/web/package.json index d0bde62771..49261a3ba5 100644 --- a/services/web/package.json +++ b/services/web/package.json @@ -100,6 +100,7 @@ "@overleaf/o-error": "workspace:*", "@overleaf/object-persistor": "workspace:*", "@overleaf/promise-utils": "workspace:*", + "@overleaf/ranges-tracker": "workspace:*", "@overleaf/redis-wrapper": "workspace:*", "@overleaf/settings": "workspace:*", "@overleaf/stream-utils": "workspace:*", diff --git a/services/web/test/acceptance/src/mocks/MockDocUpdaterApi.mjs b/services/web/test/acceptance/src/mocks/MockDocUpdaterApi.mjs index 229439fec8..c0f3e25dcf 100644 --- a/services/web/test/acceptance/src/mocks/MockDocUpdaterApi.mjs +++ b/services/web/test/acceptance/src/mocks/MockDocUpdaterApi.mjs @@ -1,6 +1,7 @@ import AbstractMockApi from './AbstractMockApi.mjs' import { parseReq, z, zz } from '@overleaf/validation-tools' import editorCoreSchemas from 'overleaf-editor-core/lib/schemas.js' +import rangesSchemas from '@overleaf/ranges-tracker/schemas.js' const docParamsSchema = z.strictObject({ projectId: zz.objectId(), @@ -26,48 +27,6 @@ const setDocSchema = z.object({ }), }) -// Ranges data as document-updater persists it (RangesTracker format) -- -// mirrors services/document-updater/app/js/schemas.js's `ranges` export -// (this mock stands in for document-updater's own API in web's acceptance -// tests). -const insertOp = z.strictObject({ - i: z.string(), - p: z.number().int().min(0), - u: z.boolean().optional(), - fixedRemoveChange: z.boolean().optional(), -}) -const deleteOp = z.strictObject({ - d: z.string(), - p: z.number().int().min(0), - u: z.boolean().optional(), - fixedRemoveChange: z.boolean().optional(), -}) -const commentOp = z.strictObject({ - c: z.string(), - p: z.number().int().min(0), - t: zz.objectId().optional(), - u: z.boolean().optional(), - resolved: z.boolean().optional(), -}) -const rangeMetadata = z.strictObject({ - user_id: z.string(), - ts: z.string(), -}) -const comment = z.strictObject({ - id: zz.objectId().optional(), - op: commentOp, - metadata: rangeMetadata.optional(), -}) -const trackedChange = z.strictObject({ - id: z.string().optional(), - op: insertOp.or(deleteOp), - metadata: rangeMetadata, -}) -const rangesSchema = z.strictObject({ - comments: z.array(comment).optional(), - changes: z.array(trackedChange).optional(), -}) - // Mirrors services/document-updater/app/js/HttpController.js's // updateProjectSchema (renameUpdateSchema/addUpdateSchema) -- this mock's // own routes use :projectId/:docId casing rather than document-updater's @@ -85,7 +44,7 @@ const addUpdateSchema = z.strictObject({ id: zz.objectId(), pathname: zz.safePath(), docLines: z.string().optional(), - ranges: rangesSchema.optional(), + ranges: rangesSchemas.ranges.optional(), historyRangesSupport: z.boolean().optional(), // legacy filestore url for files without a created blob url: z.string().nullish(), diff --git a/services/web/test/acceptance/src/mocks/MockDocstoreApi.mjs b/services/web/test/acceptance/src/mocks/MockDocstoreApi.mjs index 461ae260df..53c4402dea 100644 --- a/services/web/test/acceptance/src/mocks/MockDocstoreApi.mjs +++ b/services/web/test/acceptance/src/mocks/MockDocstoreApi.mjs @@ -1,46 +1,7 @@ import { db, ObjectId } from '../../../../app/src/infrastructure/mongodb.mjs' import AbstractMockApi from './AbstractMockApi.mjs' import { parseReq, z, zz } from '@overleaf/validation-tools' - -// Mirrors services/docstore/app/js/schemas.js (this mock stands in for -// docstore's own API in web's acceptance tests). -const insertOp = z.strictObject({ - i: z.string(), - p: z.number().int().min(0), - u: z.boolean().optional(), - fixedRemoveChange: z.boolean().optional(), -}) -const deleteOp = z.strictObject({ - d: z.string(), - p: z.number().int().min(0), - u: z.boolean().optional(), - fixedRemoveChange: z.boolean().optional(), -}) -const commentOp = z.strictObject({ - c: z.string().optional(), - p: z.number().int().min(0).optional(), - t: z.string().optional(), - u: z.boolean().optional(), - resolved: z.boolean().optional(), -}) -const rangeMetadata = z.strictObject({ - user_id: z.string().optional(), - ts: z.string().optional(), -}) -const comment = z.strictObject({ - id: z.string().optional(), - op: commentOp, - metadata: rangeMetadata.optional(), -}) -const trackedChange = z.strictObject({ - id: z.string().optional(), - op: insertOp.or(deleteOp).optional(), - metadata: rangeMetadata.optional(), -}) -const rangesSchema = z.strictObject({ - comments: z.array(comment).optional(), - changes: z.array(trackedChange).optional(), -}) +import rangesSchemas from '@overleaf/ranges-tracker/schemas.js' const docParamsSchema = z.strictObject({ projectId: zz.objectId(), @@ -52,7 +13,7 @@ const updateDocSchema = z.object({ body: z.strictObject({ lines: z.array(z.string()), version: z.number(), - ranges: rangesSchema, + ranges: rangesSchemas.ranges, }), }) diff --git a/services/web/test/unit/src/Documents/DocumentController.test.mjs b/services/web/test/unit/src/Documents/DocumentController.test.mjs index 0d23633985..c8b5f69802 100644 --- a/services/web/test/unit/src/Documents/DocumentController.test.mjs +++ b/services/web/test/unit/src/Documents/DocumentController.test.mjs @@ -15,22 +15,24 @@ describe('DocumentController', function () { ctx.doc = { _id: '5c9a5d5b0000000000000002' } ctx.doc_lines = ['one', 'two', 'three'] ctx.version = 42 + ctx.threadId1 = '5c9a5d5b0000000000000011' + ctx.threadId2 = '5c9a5d5b0000000000000012' ctx.ranges = { comments: [ { - id: 'comment1', + id: ctx.threadId1, op: { c: 'foo', p: 123, - t: 'comment1', + t: ctx.threadId1, }, }, { - id: 'comment2', + id: ctx.threadId2, op: { c: 'bar', p: 456, - t: 'comment2', + t: ctx.threadId2, }, }, ], @@ -53,8 +55,8 @@ describe('DocumentController', function () { }, } ctx.resolvedThreadIds = [ - 'comment2', - 'comment4', // Comment in project but not in doc + ctx.threadId2, + '5c9a5d5b0000000000000014', // Comment in project but not in doc ] ctx.ProjectGetter = { @@ -159,7 +161,7 @@ describe('DocumentController', function () { pathname: ctx.pathname, projectHistoryId: ctx.project.overleaf.history.id, projectHistoryType: 'project-history', - resolvedCommentIds: ['comment2'], + resolvedCommentIds: [ctx.threadId2], historyRangesSupport: false, otMigrationStage: 0, }) diff --git a/tsconfig.backend.json b/tsconfig.backend.json index 8e4c4b392b..931066260d 100644 --- a/tsconfig.backend.json +++ b/tsconfig.backend.json @@ -22,6 +22,7 @@ "@overleaf/piece-table": ["./libraries/piece-table"], "@overleaf/promise-utils": ["./libraries/promise-utils"], "@overleaf/ranges-tracker": ["./libraries/ranges-tracker"], + "@overleaf/ranges-tracker/*": ["./libraries/ranges-tracker/*"], "@overleaf/redis-wrapper": ["./libraries/redis-wrapper"], "@overleaf/settings": ["./libraries/settings"], "@overleaf/stream-utils": ["./libraries/stream-utils"], diff --git a/yarn.lock b/yarn.lock index 3aa07e8e06..e4471e6274 100644 --- a/yarn.lock +++ b/yarn.lock @@ -7158,6 +7158,7 @@ __metadata: "@overleaf/o-error": "workspace:*" "@overleaf/object-persistor": "workspace:*" "@overleaf/promise-utils": "workspace:*" + "@overleaf/ranges-tracker": "workspace:*" "@overleaf/settings": "workspace:*" "@overleaf/stream-utils": "npm:^0.1.0" "@overleaf/validation-tools": "workspace:*" @@ -7619,6 +7620,7 @@ __metadata: version: 0.0.0-use.local resolution: "@overleaf/ranges-tracker@workspace:libraries/ranges-tracker" dependencies: + "@overleaf/validation-tools": "workspace:*" "@types/node": "npm:^24.5.2" chai: "npm:^4.3.6" core-js: "npm:~3.46.0"