[document-updater] drop support for tc in briefly (#35864)

GitOrigin-RevId: 00d04d911249b81abc5eb4f9d9dfde4c7f2316b3
This commit is contained in:
Jakob Ackermann
2026-09-17 11:39:22 +00:00
committed by Copybot
parent 7a0a9c3ce5
commit 8fa62d2864
11 changed files with 190 additions and 467 deletions
-4
View File
@@ -176,10 +176,6 @@ app.post(
'/project/:project_id/doc/:doc_id/change/accept',
HttpController.acceptChanges
)
app.post(
'/project/:project_id/doc/:doc_id/change/reject',
HttpController.rejectChanges
)
app.post(
'/project/:project_id/doc/:doc_id/comment/:comment_id/resolve',
HttpController.resolveComment
@@ -359,12 +359,15 @@ const DocumentManager = {
pathname,
projectHistoryId,
historyRangesSupport,
type,
} = await DocumentManager.getDoc(projectId, docId)
if (lines == null || version == null) {
throw new Errors.NotFoundError(`document not found: ${docId}`)
}
if (type === 'history-ot') {
throw new Errors.OTTypeMismatchError('sharejs-text-ot', 'history-ot')
}
// TODO(24596): tc support for history-ot
const newRanges = RangesManager.acceptChanges(
projectId,
docId,
@@ -409,64 +412,6 @@ const DocumentManager = {
return changeContributors
},
async rejectChanges(projectId, docId, changeIds, userId) {
const UpdateManager = require('./UpdateManager')
const HistoryOTUpdateManager = require('./HistoryOTUpdateManager')
const { lines, version, ranges } = await DocumentManager.getDoc(
projectId,
docId
)
if (lines == null || version == null) {
throw new Errors.NotFoundError(`document not found: ${docId}`)
}
const changesToReject = ranges.changes
? ranges.changes.filter(change => changeIds.includes(change.id))
: []
// Apply inverted operations for rejected changes (based on reject-changes.ts logic)
// Sort changes in reverse order by position to avoid conflicts
changesToReject.sort((a, b) => b.op.p - a.op.p)
const ops = []
for (const change of changesToReject) {
if (change.op.i) {
const deleteOp = {
p: change.op.p,
d: change.op.i,
u: true,
}
ops.push(deleteOp)
} else if (change.op.d) {
const insertOp = {
p: change.op.p,
i: change.op.d,
u: true,
}
ops.push(insertOp)
}
}
const update = {
doc: docId,
op: ops,
v: version,
meta: {
user_id: userId,
ts: new Date().toISOString(),
},
}
if (HistoryOTUpdateManager.isHistoryOTEditOperationUpdate(update)) {
await HistoryOTUpdateManager.applyUpdate(projectId, docId, update)
} else {
await UpdateManager.promises.applyUpdate(projectId, docId, update)
}
return { rejectedChangeIds: changesToReject.map(c => c.id) }
},
async updateCommentState(projectId, docId, commentId, userId, resolved) {
const { lines, version, pathname, historyRangesSupport, type } =
await DocumentManager.getDoc(projectId, docId)
@@ -780,17 +725,6 @@ const DocumentManager = {
return changeContributors
},
async rejectChangesWithLock(projectId, docId, changeIds, userId) {
const UpdateManager = require('./UpdateManager')
return await UpdateManager.promises.lockUpdatesAndDo(
DocumentManager.rejectChanges,
projectId,
docId,
changeIds,
userId
)
},
async updateCommentStateWithLock(
projectId,
docId,
@@ -10,16 +10,14 @@ const Metrics = require('./Metrics')
const DeleteQueueManager = require('./DeleteQueueManager')
const { getTotalSizeOfLines } = require('./Limits')
const { StringFileData } = require('overleaf-editor-core')
const { addTrackedDeletesToContent } = require('./Utils')
const HistoryConversions = require('./HistoryConversions')
async function getDoc(req, res) {
let fromVersion
const docId = req.params.doc_id
const projectId = req.params.project_id
const historyRanges = req.query.historyRanges === 'true'
logger.debug({ projectId, docId, historyRanges }, 'getting doc via http')
logger.debug({ projectId, docId }, 'getting doc via http')
const timer = new Metrics.Timer('http.getDoc')
if (req.query.fromVersion != null) {
@@ -35,48 +33,26 @@ async function getDoc(req, res) {
fromVersion
)
timer.done()
logger.debug({ projectId, docId, historyRanges }, 'got doc via http')
logger.debug({ projectId, docId }, 'got doc via http')
if (lines == null || version == null) {
throw new Errors.NotFoundError('document not found')
}
if (!Array.isArray(lines) && req.query.historyOTSupport !== 'true') {
const file = StringFileData.fromRaw(lines)
// TODO(24596): tc support for history-ot
lines = file.getLines()
;({ lines, ranges } = HistoryConversions.fromHistoryOT(lines))
}
if (historyRanges) {
const docContentWithTrackedDeletes = addTrackedDeletesToContent(
lines.join('\n'),
ranges?.changes ?? []
)
const docLinesWithTrackedDeletes = docContentWithTrackedDeletes.split('\n')
const rangesWithTrackedDeletes = HistoryConversions.toHistoryRanges(ranges)
res.json({
id: docId,
lines: docLinesWithTrackedDeletes,
version,
ops,
ranges: rangesWithTrackedDeletes,
pathname,
ttlInS: RedisManager.DOC_OPS_TTL,
type,
})
} else {
res.json({
id: docId,
lines,
version,
ops,
ranges,
pathname,
ttlInS: RedisManager.DOC_OPS_TTL,
type,
})
}
res.json({
id: docId,
lines,
version,
ops,
ranges,
pathname,
ttlInS: RedisManager.DOC_OPS_TTL,
type,
})
}
async function getComment(req, res) {
@@ -378,28 +354,6 @@ async function acceptChanges(req, res) {
res.status(200).json({ changeContributors })
}
async function rejectChanges(req, res) {
const { project_id: projectId, doc_id: docId } = req.params
const changeIds = req.body.change_ids
const userId = req.body.user_id
logger.debug(
{ projectId, docId },
`rejecting ${changeIds.length} changes via http`
)
const response = await DocumentManager.promises.rejectChangesWithLock(
projectId,
docId,
changeIds,
userId
)
logger.debug(
{ projectId, docId, changeIds, response },
`rejected ${changeIds.length} changes via http`
)
res.json(response)
}
async function resolveComment(req, res) {
const {
project_id: projectId,
@@ -567,7 +521,6 @@ module.exports = {
deleteProject: expressify(deleteProject),
deleteMultipleProjects: expressify(deleteMultipleProjects),
acceptChanges: expressify(acceptChanges),
rejectChanges: expressify(rejectChanges),
resolveComment: expressify(resolveComment),
reopenComment: expressify(reopenComment),
deleteComment: expressify(deleteComment),
@@ -128,6 +128,70 @@ describe('Getting a document', function () {
})
})
describe('when the document is a history-ot doc with tracked changes and comments', function () {
before(async function () {
this.project_id = DocUpdaterClient.randomId()
this.doc_id = DocUpdaterClient.randomId()
this.user_id = DocUpdaterClient.randomId()
this.thread_id = DocUpdaterClient.randomId()
this.ts = new Date().toISOString()
MockWebApi.insertDoc(this.project_id, this.doc_id, {
lines: this.lines,
version: this.version,
otMigrationStage: 1,
ranges: {
changes: [
{
id: DocUpdaterClient.randomId(),
op: { p: 4, d: 'one and a half\n' },
metadata: { user_id: this.user_id, ts: this.ts },
},
],
comments: [
{
id: this.thread_id,
op: { p: 0, c: 'one', t: this.thread_id },
},
],
},
})
this.returnedDoc = await DocUpdaterClient.getDoc(
this.project_id,
this.doc_id
)
})
it('should return the doc as a history-ot doc', function () {
this.returnedDoc.type.should.equal('history-ot')
})
it('should return the document lines without tracked deletes', function () {
this.returnedDoc.lines.should.deep.equal(this.lines)
})
it('should return the tracked changes in editor format with fresh ids', function () {
expect(this.returnedDoc.ranges.changes).to.have.length(1)
const [change] = this.returnedDoc.ranges.changes
expect(change.id).to.match(/^[0-9a-f]{24}$/)
expect(change.op).to.deep.equal({ p: 4, d: 'one and a half\n' })
expect(change.metadata).to.deep.equal({
user_id: this.user_id,
ts: this.ts,
})
})
it('should return the comments in editor format', function () {
expect(this.returnedDoc.ranges.comments).to.deep.equal([
{
id: this.thread_id,
op: { p: 0, c: 'one', t: this.thread_id, resolved: false },
},
])
})
})
describe('when the request asks for some recent ops', function () {
before(async function () {
this.project_id = DocUpdaterClient.randomId()
@@ -11,6 +11,7 @@ const projectHistoryRedis = require('@overleaf/redis-wrapper').createClient(
)
const ProjectHistoryKeys = Settings.redis.project_history.key_schema
const { RequestFailedError } = require('@overleaf/fetch-utils')
const { db, ObjectId } = require('../../../app/js/mongodb')
const MockWebApi = require('./helpers/MockWebApi')
const DocUpdaterClient = require('./helpers/DocUpdaterClient')
@@ -453,6 +454,60 @@ describe('Ranges', function () {
})
})
describe('accepting changes in a history-ot doc', function () {
beforeEach(async function () {
this.project_id = DocUpdaterClient.randomId()
this.user_id = DocUpdaterClient.randomId()
this.doc = {
id: DocUpdaterClient.randomId(),
lines: ['one', 'one and a half', 'two', 'three'],
}
this.update = {
doc: this.doc.id,
op: [
{
textOperation: [
4,
{
r: 'one and a half\n'.length,
tracking: {
type: 'delete',
userId: this.user_id,
ts: new Date().toISOString(),
},
},
9,
],
},
],
v: 0,
meta: { source: 'random-publicId' },
}
MockWebApi.insertDoc(this.project_id, this.doc.id, {
lines: this.doc.lines,
version: 0,
otMigrationStage: 1,
})
await DocUpdaterClient.preloadDoc(this.project_id, this.doc.id)
await DocUpdaterClient.sendUpdate(
this.project_id,
this.doc.id,
this.update
)
await DocUpdaterClient.waitForPendingUpdates(this.project_id, this.doc.id)
})
it('should refuse to accept changes with a 422', async function () {
const doc = await DocUpdaterClient.getDoc(this.project_id, this.doc.id)
const changeIds = doc.ranges.changes.map(change => change.id)
await expect(
DocUpdaterClient.acceptChanges(this.project_id, this.doc.id, changeIds)
)
.to.be.rejectedWith(RequestFailedError)
.and.eventually.have.nested.property('response.status', 422)
})
})
describe('deleting a comment range', function () {
before(async function () {
this.project_id = DocUpdaterClient.randomId()
@@ -1,219 +0,0 @@
const sinon = require('sinon')
const { expect } = require('chai')
const MockWebApi = require('./helpers/MockWebApi')
const DocUpdaterClient = require('./helpers/DocUpdaterClient')
const DocUpdaterApp = require('./helpers/DocUpdaterApp')
const sandbox = sinon.createSandbox()
describe('Rejecting Changes', function () {
before(async function () {
await DocUpdaterApp.ensureRunning()
})
describe('rejecting a single change', function () {
beforeEach(async function () {
this.project_id = DocUpdaterClient.randomId()
this.user_id = DocUpdaterClient.randomId()
this.doc = {
id: DocUpdaterClient.randomId(),
lines: ['the brown fox jumps over the lazy dog'],
}
MockWebApi.insertDoc(this.project_id, this.doc.id, {
lines: this.doc.lines,
version: 0,
historyRangesSupport: true,
})
this.id_seed = 'tc_reject_test'
this.update = {
doc: this.doc.id,
op: [{ i: 'quick ', p: 4 }],
v: 0,
meta: {
user_id: this.user_id,
tc: this.id_seed,
},
}
await DocUpdaterClient.sendUpdate(
this.project_id,
this.doc.id,
this.update
)
})
afterEach(function () {
sandbox.restore()
})
it('should reject the change and restore the original text', async function () {
const doc1 = await DocUpdaterClient.getDoc(this.project_id, this.doc.id)
expect(doc1.ranges.changes).to.have.length(1)
const change = doc1.ranges.changes[0]
expect(change.op).to.deep.equal({ i: 'quick ', p: 4 })
expect(change.id).to.equal(this.id_seed + '000001')
expect(doc1.lines).to.deep.equal([
'the quick brown fox jumps over the lazy dog',
])
const { rejectedChangeIds } = await DocUpdaterClient.rejectChanges(
this.project_id,
this.doc.id,
[change.id],
this.user_id
)
expect(rejectedChangeIds).to.be.an('array')
expect(rejectedChangeIds).to.include(change.id)
const doc2 = await DocUpdaterClient.getDoc(this.project_id, this.doc.id)
expect(doc2.ranges.changes || []).to.have.length(0)
expect(doc2.lines).to.deep.equal([
'the brown fox jumps over the lazy dog',
])
})
it('should return 200 status code with rejectedChangeIds on successful rejection', async function () {
const data = await DocUpdaterClient.getDoc(this.project_id, this.doc.id)
const changeId = data.ranges.changes[0].id
const { rejectedChangeIds } = await DocUpdaterClient.rejectChanges(
this.project_id,
this.doc.id,
[changeId],
this.user_id
)
expect(rejectedChangeIds).to.be.an('array')
expect(rejectedChangeIds).to.include(changeId)
})
})
describe('rejecting multiple changes', function () {
beforeEach(async function () {
this.project_id = DocUpdaterClient.randomId()
this.user_id = DocUpdaterClient.randomId()
this.doc = {
id: DocUpdaterClient.randomId(),
lines: ['the brown fox jumps over the lazy dog'],
}
MockWebApi.insertDoc(this.project_id, this.doc.id, {
lines: this.doc.lines,
version: 0,
historyRangesSupport: true,
})
this.id_seed_1 = 'tc_reject_1'
this.id_seed_2 = 'tc_reject_2'
this.updates = [
{
doc: this.doc.id,
op: [{ i: 'quick ', p: 4 }],
v: 0,
meta: {
user_id: this.user_id,
tc: this.id_seed_1,
},
},
{
doc: this.doc.id,
op: [{ d: 'lazy ', p: 35 }],
v: 1,
meta: {
user_id: this.user_id,
tc: this.id_seed_2,
},
},
]
await DocUpdaterClient.sendUpdates(
this.project_id,
this.doc.id,
this.updates
)
})
afterEach(function () {
sandbox.restore()
})
it('should reject multiple changes in order', async function () {
const data = await DocUpdaterClient.getDoc(this.project_id, this.doc.id)
expect(data.ranges.changes).to.have.length(2)
expect(data.lines).to.deep.equal([
'the quick brown fox jumps over the dog',
])
const changeIds = data.ranges.changes.map(change => change.id)
const { rejectedChangeIds } = await DocUpdaterClient.rejectChanges(
this.project_id,
this.doc.id,
changeIds,
this.user_id
)
expect(rejectedChangeIds).to.be.an('array')
expect(rejectedChangeIds).to.have.length(2)
expect(rejectedChangeIds).to.include.members(changeIds)
const data2 = await DocUpdaterClient.getDoc(this.project_id, this.doc.id)
expect(data2.ranges.changes || []).to.have.length(0)
expect(data2.lines).to.deep.equal([
'the brown fox jumps over the lazy dog',
])
})
})
describe('error cases', function () {
beforeEach(async function () {
this.project_id = DocUpdaterClient.randomId()
this.user_id = DocUpdaterClient.randomId()
this.doc = {
id: DocUpdaterClient.randomId(),
lines: ['the brown fox jumps over the lazy dog'],
}
MockWebApi.insertDoc(this.project_id, this.doc.id, {
lines: this.doc.lines,
version: 0,
historyRangesSupport: true,
})
await DocUpdaterApp.ensureRunning()
})
it('should handle rejection of non-existent changes gracefully', async function () {
const nonExistentChangeId = 'nonexistent_change_id'
const { rejectedChangeIds } = await DocUpdaterClient.rejectChanges(
this.project_id,
this.doc.id,
[nonExistentChangeId],
this.user_id
)
// Should still return 200 with empty rejectedChangeIds if no changes were found to reject
expect(rejectedChangeIds).to.be.an('array')
expect(rejectedChangeIds).to.have.length(0)
})
it('should handle empty change_ids array', async function () {
const { rejectedChangeIds } = await DocUpdaterClient.rejectChanges(
this.project_id,
this.doc.id,
[],
this.user_id
)
expect(rejectedChangeIds).to.be.an('array')
expect(rejectedChangeIds).to.have.length(0)
})
})
})
@@ -587,6 +587,15 @@ describe('Setting a document', function () {
expect(doc.lines).to.deep.equal(this.newLines)
})
it('should provide the tracked changes in editor format when getting the doc', async function () {
const doc = await DocUpdaterClient.getDoc(this.project_id, this.doc_id)
expect(doc.ranges.changes).to.have.length(1)
const [change] = doc.ranges.changes
expect(change.id).to.match(/^[0-9a-f]{24}$/)
expect(change.op).to.deep.equal({ p: 4, d: 'one and a half\n' })
expect(change.metadata).to.deep.equal({ user_id: userId, ts })
})
const cases = [
{
name: 'when resetting the content',
@@ -846,6 +855,17 @@ describe('Setting a document', function () {
)
expect(doc.lines).to.deep.equal(['one', 'two'])
})
it('should provide the tracked deletes in editor format when getting the doc', async function () {
const doc = await DocUpdaterClient.getDoc(
this.project_id,
this.doc_id
)
expect(doc.ranges.changes.map(change => change.op)).to.deep.equal([
{ p: 4, d: 'one and a half\n' },
{ p: 7, d: '\nthree' },
])
})
})
describe('when appending content', function () {
@@ -887,6 +907,17 @@ describe('Setting a document', function () {
)
expect(doc.lines).to.deep.equal(['one', 'two', 'three', 'four'])
})
it('should provide the tracked insert in editor format when getting the doc', async function () {
const doc = await DocUpdaterClient.getDoc(
this.project_id,
this.doc_id
)
expect(doc.ranges.changes.map(change => change.op)).to.deep.equal([
{ p: 4, d: 'one and a half\n' },
{ p: 13, i: '\nfour' },
])
})
})
})
})
@@ -204,16 +204,6 @@ module.exports = DocUpdaterClient = {
)
},
async rejectChanges(projectId, docId, changeIds, userId) {
return await fetchJson(
`http://127.0.0.1:3003/project/${projectId}/doc/${docId}/change/reject`,
{
method: 'POST',
json: { change_ids: changeIds, user_id: userId },
}
)
},
async getComment(projectId, docId, commentId) {
return await fetchJson(
`http://127.0.0.1:3003/project/${projectId}/doc/${docId}/comment/${commentId}`
@@ -149,7 +149,6 @@ describe('HttpController', function () {
{
docId: this.doc_id,
projectId: this.project_id,
historyRanges: false,
},
'getting doc via http'
)
@@ -203,69 +202,6 @@ describe('HttpController', function () {
{
docId: this.doc_id,
projectId: this.project_id,
historyRanges: false,
},
'getting doc via http'
)
.should.equal(true)
})
it('should time the request', function () {
this.Metrics.Timer.prototype.done.called.should.equal(true)
})
})
describe('when historyRanges query param is true', function () {
beforeEach(async function () {
this.DocumentManager.promises.getDocAndRecentOpsWithLock.resolves({
lines: this.lines,
version: this.version,
ops: [],
ranges: this.ranges,
pathname: this.pathname,
projectHistoryId: this.projectHistoryId,
type: 'sharejs-text-ot',
})
this.req.query = { historyRanges: 'true' }
await this.HttpController.getDoc(this.req, this.res, this.next)
})
it('should get the doc', function () {
this.DocumentManager.promises.getDocAndRecentOpsWithLock.should.have.been.calledWith(
this.project_id,
this.doc_id,
-1
)
})
it('should return the doc as JSON with history ranges processing', function () {
this.res.json.should.have.been.calledWith({
id: this.doc_id,
lines: this.lines,
version: this.version,
ops: [],
ranges: this.ranges,
pathname: this.pathname,
ttlInS: 42,
type: 'sharejs-text-ot',
})
})
it('should call addTrackedDeletesToContent for history ranges processing', function () {
this.Utils.addTrackedDeletesToContent.called.should.equal(true)
})
it('should call toHistoryRanges for range conversion', function () {
this.HistoryConversions.toHistoryRanges.called.should.equal(true)
})
it('should log the request with historyRanges: true', function () {
this.logger.debug
.calledWith(
{
docId: this.doc_id,
projectId: this.project_id,
historyRanges: true,
},
'getting doc via http'
)
@@ -101,19 +101,6 @@ async function getDocument(projectId, docId, fromVersion) {
}
}
/**
* Get a document with its history ranges
* @param {string} projectId
* @param {string} docId
*/
async function getDocumentWithHistoryRanges(projectId, docId) {
const doc = await fetchJson(
`${BASE_URL}/project/${projectId}/doc/${docId}?historyRanges=true`,
{ signal: AbortSignal.timeout(REQUEST_TIMEOUT_MS) }
)
return doc
}
async function setDocument(
projectId,
docId,
@@ -238,23 +225,6 @@ async function acceptChanges(projectId, docId, changeIds, userId) {
)
}
/**
* @param {string} projectId
* @param {string} docId
* @param {string[]} changeIds
*/
async function rejectChanges(projectId, docId, changeIds, userId) {
const { rejectedChangeIds } = await fetchJson(
`${BASE_URL}/project/${projectId}/doc/${docId}/change/reject`,
{
method: 'POST',
json: { change_ids: changeIds, user_id: userId },
signal: AbortSignal.timeout(REQUEST_TIMEOUT_MS),
}
)
return { rejectedChangeIds }
}
/**
* @param {string} projectId
* @param {string} docId
@@ -554,7 +524,6 @@ const DocumentUpdaterHandler = {
getProjectDocsIfMatch,
clearProjectState,
acceptChanges,
rejectChanges,
resolveThread,
reopenThread,
deleteThread,
@@ -562,7 +531,6 @@ const DocumentUpdaterHandler = {
blockProject,
unblockProject,
updateProjectStructure,
getDocumentWithHistoryRanges,
}
export default {
@@ -5,6 +5,7 @@ class MockDocUpdaterApi extends AbstractMockApi {
this.updates = {}
this.docsByProject = new Map()
this.receivedSetDocRequests = []
this.receivedGetDocRequests = []
}
getReceivedSetDocRequests(projectId) {
@@ -13,6 +14,12 @@ class MockDocUpdaterApi extends AbstractMockApi {
)
}
getReceivedGetDocRequests(projectId) {
return this.receivedGetDocRequests.filter(
request => request.projectId === projectId
)
}
getProjectStructureUpdates(projectId) {
return this.updates[projectId] || { updates: [] }
}
@@ -30,13 +37,13 @@ class MockDocUpdaterApi extends AbstractMockApi {
this.updates[projectId].version = version
}
setDoc(projectId, docId, lines, ranges) {
setDoc(projectId, docId, lines, ranges, version = 0) {
let docsById = this.docsByProject.get(projectId)
if (docsById == null) {
docsById = new Map()
this.docsByProject.set(projectId, docsById)
}
docsById.set(docId, { id: docId, lines, ranges })
docsById.set(docId, { id: docId, lines, ranges, version })
}
applyRoutes() {
@@ -61,13 +68,21 @@ class MockDocUpdaterApi extends AbstractMockApi {
}
)
this.app.post(
'/project/:projectId/doc/:docId/change/reject',
(req, res) => {
const { change_ids: changeIds } = req.body
res.json({ rejectedChangeIds: changeIds })
this.app.get('/project/:projectId/doc/:docId', (req, res) => {
const { projectId, docId } = req.params
this.receivedGetDocRequests.push({ projectId, docId, query: req.query })
const doc = this.docsByProject.get(projectId)?.get(docId)
if (doc == null) {
return res.sendStatus(404)
}
)
res.json({
id: doc.id,
lines: doc.lines,
version: doc.version,
ranges: doc.ranges,
ops: [],
})
})
this.app.post('/project/:projectId/doc/:doc_id', (req, res) => {
const { projectId, doc_id: docId } = req.params