[web+document-updater] Allow appending to documents (#20745)

Co-authored-by: David Powell <david.powell@overleaf.com>
GitOrigin-RevId: f66283926e7da3edf83ada9316c3a001287e1b42
This commit is contained in:
Mathias Jakobsen
2024-12-11 14:38:58 +00:00
committed by Copybot
parent b3e6111abc
commit 0b7fb0b622
11 changed files with 649 additions and 12 deletions

View File

@@ -145,6 +145,7 @@ app.post(
)
app.post('/project/:project_id/clearState', HttpController.clearProjectState)
app.post('/project/:project_id/doc/:doc_id', HttpController.setDoc)
app.post('/project/:project_id/doc/:doc_id/append', HttpController.appendToDoc)
app.post(
'/project/:project_id/doc/:doc_id/flush',
HttpController.flushDocIfLoaded

View File

@@ -9,6 +9,8 @@ const HistoryManager = require('./HistoryManager')
const Errors = require('./Errors')
const RangesManager = require('./RangesManager')
const { extractOriginOrSource } = require('./Utils')
const { getTotalSizeOfLines } = require('./Limits')
const Settings = require('@overleaf/settings')
const MAX_UNFLUSHED_AGE = 300 * 1000 // 5 mins, document should be flushed to mongo this time after a change
@@ -112,7 +114,41 @@ const DocumentManager = {
}
},
async setDoc(projectId, docId, newLines, originOrSource, userId, undoing) {
async appendToDoc(projectId, docId, linesToAppend, originOrSource, userId) {
const { lines: currentLines } = await DocumentManager.getDoc(
projectId,
docId
)
const currentLineSize = getTotalSizeOfLines(currentLines)
const addedSize = getTotalSizeOfLines(linesToAppend)
const newlineSize = '\n'.length
if (currentLineSize + newlineSize + addedSize > Settings.max_doc_length) {
throw new Errors.FileTooLargeError(
'doc would become too large if appending this text'
)
}
return await DocumentManager.setDoc(
projectId,
docId,
currentLines.concat(linesToAppend),
originOrSource,
userId,
false,
false
)
},
async setDoc(
projectId,
docId,
newLines,
originOrSource,
userId,
undoing,
external
) {
if (newLines == null) {
throw new Error('No lines were provided to setDoc')
}
@@ -150,10 +186,12 @@ const DocumentManager = {
op,
v: version,
meta: {
type: 'external',
user_id: userId,
},
}
if (external) {
update.meta.type = 'external'
}
if (origin) {
update.meta.origin = origin
} else if (source) {
@@ -481,7 +519,15 @@ const DocumentManager = {
)
},
async setDocWithLock(projectId, docId, lines, source, userId, undoing) {
async setDocWithLock(
projectId,
docId,
lines,
source,
userId,
undoing,
external
) {
const UpdateManager = require('./UpdateManager')
return await UpdateManager.promises.lockUpdatesAndDo(
DocumentManager.setDoc,
@@ -490,7 +536,20 @@ const DocumentManager = {
lines,
source,
userId,
undoing
undoing,
external
)
},
async appendToDocWithLock(projectId, docId, lines, source, userId) {
const UpdateManager = require('./UpdateManager')
return await UpdateManager.promises.lockUpdatesAndDo(
DocumentManager.appendToDoc,
projectId,
docId,
lines,
source,
userId
)
},

View File

@@ -144,6 +144,7 @@ function setDoc(req, res, next) {
source,
userId,
undoing,
true,
(error, result) => {
timer.done()
if (error) {
@@ -155,6 +156,35 @@ function setDoc(req, res, next) {
)
}
function appendToDoc(req, res, next) {
const docId = req.params.doc_id
const projectId = req.params.project_id
const { lines, source, user_id: userId } = req.body
const timer = new Metrics.Timer('http.appendToDoc')
DocumentManager.appendToDocWithLock(
projectId,
docId,
lines,
source,
userId,
(error, result) => {
timer.done()
if (error instanceof Errors.FileTooLargeError) {
logger.warn('refusing to append to file, it would become too large')
return res.sendStatus(422)
}
if (error) {
return next(error)
}
logger.debug(
{ projectId, docId, lines, source, userId },
'appending to doc via http'
)
res.json(result)
}
)
}
function flushDocIfLoaded(req, res, next) {
const docId = req.params.doc_id
const projectId = req.params.project_id
@@ -460,6 +490,7 @@ module.exports = {
peekDoc,
getProjectDocsAndFlushIfOld,
clearProjectState,
appendToDoc,
setDoc,
flushDocIfLoaded,
deleteDoc,

View File

@@ -53,6 +53,9 @@ describe('DocumentManager', function () {
acceptChanges: sinon.stub(),
deleteComment: sinon.stub(),
}
this.Settings = {
max_doc_length: 2 * 1024 * 1024, // 2mb
}
this.DocumentManager = SandboxedModule.require(modulePath, {
requires: {
@@ -65,6 +68,7 @@ describe('DocumentManager', function () {
'./UpdateManager': this.UpdateManager,
'./RangesManager': this.RangesManager,
'./Errors': Errors,
'@overleaf/settings': this.Settings,
},
})
this.project_id = 'project-id-123'
@@ -446,7 +450,8 @@ describe('DocumentManager', function () {
this.beforeLines,
this.source,
this.user_id,
false
false,
true
)
})
@@ -480,7 +485,8 @@ describe('DocumentManager', function () {
this.beforeLines,
this.source,
this.user_id,
false
false,
true
)
})
@@ -513,7 +519,8 @@ describe('DocumentManager', function () {
this.afterLines,
this.source,
this.user_id,
false
false,
true
)
})
@@ -582,7 +589,8 @@ describe('DocumentManager', function () {
this.afterLines,
this.source,
this.user_id,
false
false,
true
)
})
@@ -618,7 +626,8 @@ describe('DocumentManager', function () {
null,
this.source,
this.user_id,
false
false,
true
)
).to.be.rejectedWith('No lines were provided to setDoc')
})
@@ -642,6 +651,7 @@ describe('DocumentManager', function () {
this.afterLines,
this.source,
this.user_id,
true,
true
)
})
@@ -650,6 +660,77 @@ describe('DocumentManager', function () {
this.ops.map(op => op.u.should.equal(true))
})
})
describe('with the external flag', function () {
beforeEach(async function () {
this.undoing = false
// Copy ops so we don't interfere with other tests
this.ops = [
{ i: 'foo', p: 4 },
{ d: 'bar', p: 42 },
]
this.DiffCodec.diffAsShareJsOp.returns(this.ops)
await this.DocumentManager.promises.setDoc(
this.project_id,
this.doc_id,
this.afterLines,
this.source,
this.user_id,
this.undoing,
true
)
})
it('should add the external type to update metadata', function () {
this.UpdateManager.promises.applyUpdate
.calledWith(this.project_id, this.doc_id, {
doc: this.doc_id,
v: this.version,
op: this.ops,
meta: {
type: 'external',
source: this.source,
user_id: this.user_id,
},
})
.should.equal(true)
})
})
describe('without the external flag', function () {
beforeEach(async function () {
this.undoing = false
// Copy ops so we don't interfere with other tests
this.ops = [
{ i: 'foo', p: 4 },
{ d: 'bar', p: 42 },
]
this.DiffCodec.diffAsShareJsOp.returns(this.ops)
await this.DocumentManager.promises.setDoc(
this.project_id,
this.doc_id,
this.afterLines,
this.source,
this.user_id,
this.undoing,
false
)
})
it('should not add the external type to update metadata', function () {
this.UpdateManager.promises.applyUpdate
.calledWith(this.project_id, this.doc_id, {
doc: this.doc_id,
v: this.version,
op: this.ops,
meta: {
source: this.source,
user_id: this.user_id,
},
})
.should.equal(true)
})
})
})
})
@@ -1136,4 +1217,68 @@ describe('DocumentManager', function () {
})
})
})
describe('appendToDoc', function () {
describe('sucessfully', function () {
beforeEach(async function () {
this.lines = ['one', 'two', 'three']
this.DocumentManager.promises.setDoc = sinon
.stub()
.resolves({ rev: '123' })
this.DocumentManager.promises.getDoc = sinon.stub().resolves({
lines: this.lines,
})
this.result = await this.DocumentManager.promises.appendToDoc(
this.project_id,
this.doc_id,
['four', 'five', 'six'],
this.source,
this.user_id
)
})
it('should call setDoc with concatenated lines', function () {
this.DocumentManager.promises.setDoc
.calledWith(
this.project_id,
this.doc_id,
['one', 'two', 'three', 'four', 'five', 'six'],
this.source,
this.user_id,
false,
false
)
.should.equal(true)
})
it('should return output from setDoc', function () {
this.result.should.deep.equal({ rev: '123' })
})
})
describe('when doc would become too big', function () {
beforeEach(async function () {
this.Settings.max_doc_length = 100
this.lines = ['one', 'two', 'three']
this.DocumentManager.promises.setDoc = sinon
.stub()
.resolves({ rev: '123' })
this.DocumentManager.promises.getDoc = sinon.stub().resolves({
lines: this.lines,
})
})
it('should fail with FileTooLarge error', async function () {
expect(
this.DocumentManager.promises.appendToDoc(
this.project_id,
this.doc_id,
['x'.repeat(1000)],
this.source,
this.user_id
)
).to.eventually.be.rejectedWith(Errors.FileTooLargeError)
})
})
})
})

View File

@@ -209,7 +209,7 @@ describe('HttpController', function () {
beforeEach(function () {
this.DocumentManager.setDocWithLock = sinon
.stub()
.callsArgWith(6, null, { rev: '123' })
.callsArgWith(7, null, { rev: '123' })
this.HttpController.setDoc(this.req, this.res, this.next)
})
@@ -221,7 +221,8 @@ describe('HttpController', function () {
this.lines,
this.source,
this.user_id,
this.undoing
this.undoing,
true
)
.should.equal(true)
})
@@ -255,7 +256,7 @@ describe('HttpController', function () {
beforeEach(function () {
this.DocumentManager.setDocWithLock = sinon
.stub()
.callsArgWith(6, new Error('oops'))
.callsArgWith(7, new Error('oops'))
this.HttpController.setDoc(this.req, this.res, this.next)
})
@@ -1103,4 +1104,96 @@ describe('HttpController', function () {
})
})
})
describe('appendToDoc', function () {
beforeEach(function () {
this.lines = ['one', 'two', 'three']
this.source = 'dropbox'
this.user_id = 'user-id-123'
this.req = {
headers: {},
params: {
project_id: this.project_id,
doc_id: this.doc_id,
},
query: {},
body: {
lines: this.lines,
source: this.source,
user_id: this.user_id,
undoing: (this.undoing = true),
},
}
})
describe('successfully', function () {
beforeEach(function () {
this.DocumentManager.appendToDocWithLock = sinon
.stub()
.callsArgWith(5, null, { rev: '123' })
this.HttpController.appendToDoc(this.req, this.res, this.next)
})
it('should append to the doc', function () {
this.DocumentManager.appendToDocWithLock
.calledWith(
this.project_id,
this.doc_id,
this.lines,
this.source,
this.user_id
)
.should.equal(true)
})
it('should return a json response with the document rev from web', function () {
this.res.json.calledWithMatch({ rev: '123' }).should.equal(true)
})
it('should log the request', function () {
this.logger.debug
.calledWith(
{
docId: this.doc_id,
projectId: this.project_id,
lines: this.lines,
source: this.source,
userId: this.user_id,
},
'appending to doc via http'
)
.should.equal(true)
})
it('should time the request', function () {
this.Metrics.Timer.prototype.done.called.should.equal(true)
})
})
describe('when an errors occurs', function () {
beforeEach(function () {
this.DocumentManager.appendToDocWithLock = sinon
.stub()
.callsArgWith(5, new Error('oops'))
this.HttpController.appendToDoc(this.req, this.res, this.next)
})
it('should call next with the error', function () {
this.next.calledWith(sinon.match.instanceOf(Error)).should.equal(true)
})
})
describe('when the payload is too large', function () {
beforeEach(function () {
this.DocumentManager.appendToDocWithLock = sinon
.stub()
.callsArgWith(5, new Errors.FileTooLargeError())
this.HttpController.appendToDoc(this.req, this.res, this.next)
})
it('should send back a 422 response', function () {
this.res.sendStatus.calledWith(422).should.equal(true)
})
})
})
})

View File

@@ -114,6 +114,23 @@ function setDocument(projectId, docId, userId, docLines, source, callback) {
)
}
function appendToDocument(projectId, docId, userId, lines, source, callback) {
_makeRequest(
{
path: `/project/${projectId}/doc/${docId}/append`,
method: 'POST',
json: {
lines,
source,
user_id: userId,
},
},
projectId,
'append-to-document',
callback
)
}
function getProjectDocsIfMatch(projectId, projectStateHash, callback) {
// If the project state hasn't changed, we can get all the latest
// docs from redis via the docupdater. Otherwise we will need to
@@ -533,6 +550,7 @@ module.exports = {
deleteDoc,
getDocument,
setDocument,
appendToDocument,
getProjectDocsIfMatch,
clearProjectState,
acceptChanges,
@@ -566,5 +584,6 @@ module.exports = {
blockProject: promisify(blockProject),
unblockProject: promisify(unblockProject),
updateProjectStructure: promisify(updateProjectStructure),
appendToDocument: promisify(appendToDocument),
},
}

View File

@@ -108,6 +108,26 @@ const EditorController = {
)
},
appendToDoc(projectId, docId, docLines, source, userId, callback) {
ProjectEntityUpdateHandler.appendToDoc(
projectId,
docId,
docLines,
source,
userId,
function (err, doc) {
if (err) {
OError.tag(err, 'error appending to doc', {
projectId,
docId,
})
return callback(err)
}
callback(err, doc)
}
)
},
upsertDoc(projectId, folderId, docName, docLines, source, userId, callback) {
ProjectEntityUpdateHandler.upsertDoc(
projectId,

View File

@@ -515,6 +515,24 @@ const upsertDoc = wrapWithLock(
}
)
const appendToDoc = wrapWithLock(
async (projectId, docId, lines, source, userId) => {
const { element } = await ProjectLocator.promises.findElement({
project_id: projectId,
element_id: docId,
type: 'doc',
})
return await DocumentUpdaterHandler.promises.appendToDocument(
projectId,
element._id,
userId,
lines,
source
)
}
)
const upsertFile = wrapWithLock({
async beforeLock(
projectId,
@@ -1212,6 +1230,8 @@ const ProjectEntityUpdateHandler = {
upsertDoc: callbackifyMultiResult(upsertDoc, ['doc', 'isNew']),
appendToDoc: callbackify(appendToDoc),
upsertDocWithPath: callbackifyMultiResult(upsertDocWithPath, [
'doc',
'isNew',
@@ -1253,6 +1273,7 @@ const ProjectEntityUpdateHandler = {
upsertDocWithPath,
upsertFile,
upsertFileWithPath,
appendToDocWithPath: appendToDoc,
},
async _addDocAndSendToTpds(projectId, folderId, doc) {

View File

@@ -1608,4 +1608,96 @@ describe('DocumentUpdaterHandler', function () {
)
})
})
describe('appendToDocument', function () {
describe('successfully', function () {
beforeEach(function () {
this.body = {
rev: 1,
}
this.request.callsArgWith(1, null, { statusCode: 200 }, this.body)
this.handler.appendToDocument(
this.project_id,
this.doc_id,
this.user_id,
this.lines,
this.source,
this.callback
)
})
it('should append to the document in the document updater', function () {
this.request
.calledWith({
url: `${this.settings.apis.documentupdater.url}/project/${this.project_id}/doc/${this.doc_id}/append`,
json: {
lines: this.lines,
source: this.source,
user_id: this.user_id,
},
method: 'POST',
timeout: 30 * 1000,
})
.should.equal(true)
})
it('should call the callback with no error', function () {
this.callback.calledWith(null).should.equal(true)
})
})
describe('when the document updater API returns an error', function () {
beforeEach(function () {
this.request.callsArgWith(
1,
new Error('something went wrong'),
null,
null
)
this.handler.appendToDocument(
this.project_id,
this.doc_id,
this.user_id,
this.lines,
this.source,
this.callback
)
})
it('should return an error to the callback', function () {
this.callback
.calledWith(sinon.match.instanceOf(Error))
.should.equal(true)
})
})
describe('when the document updater returns a failure error code', function () {
beforeEach(function () {
this.request.callsArgWith(1, null, { statusCode: 500 }, '')
this.handler.appendToDocument(
this.project_id,
this.doc_id,
this.user_id,
this.lines,
this.source,
this.callback
)
})
it('should return the callback with an error', function () {
this.callback
.calledWith(
sinon.match
.instanceOf(Error)
.and(
sinon.match.has(
'message',
'document updater returned a failure status code: 500'
)
)
)
.should.equal(true)
})
})
})
})

View File

@@ -14,6 +14,7 @@
const SandboxedModule = require('sandboxed-module')
const sinon = require('sinon')
const { expect } = require('chai')
const OError = require('@overleaf/o-error')
const modulePath = require('path').join(
__dirname,
@@ -1026,4 +1027,56 @@ describe('EditorController', function () {
})
})
})
describe('appendToDoc', function () {
describe('on success', function () {
beforeEach(function () {
this.docId = 'doc-1'
this.ProjectEntityUpdateHandler.appendToDoc = sinon
.stub()
.yields(null, { rev: '1' })
this.EditorController.appendToDoc(
this.project_id,
this.docId,
this.docLines,
this.source,
this.user_id,
this.callback
)
})
it('appends to the doc using the project entity handler', function () {
this.ProjectEntityUpdateHandler.appendToDoc
.calledWith(this.project_id, this.docId, this.docLines, this.source)
.should.equal(true)
})
})
describe('on error', function () {
beforeEach(function () {
this.docId = 'doc-1'
this.ProjectEntityUpdateHandler.appendToDoc = sinon
.stub()
.yields(new Error('foo'))
this.EditorController.appendToDoc(
this.project_id,
this.docId,
this.docLines,
this.source,
this.user_id,
this.callback
)
})
it('tries to append to the doc using the project entity handler', function () {
this.ProjectEntityUpdateHandler.appendToDoc
.calledWith(this.project_id, this.docId, this.docLines, this.source)
.should.equal(true)
})
it('tags the error', function () {
this.callback.calledWith(sinon.match.instanceOf(OError))
})
})
})
})

View File

@@ -3167,4 +3167,107 @@ describe('ProjectEntityUpdateHandler', function () {
})
})
})
describe('appendToDoc', function () {
describe('when document cannot be found', function () {
beforeEach(function (done) {
this.appendedLines = ['5678', 'def']
this.DocumentUpdaterHandler.promises.appendToDocument = sinon.stub()
this.ProjectLocator.promises.findElement = sinon.stub()
this.ProjectLocator.promises.findElement
.withArgs({ project_id: projectId, element_id: docId, type: 'doc' })
.rejects(new Errors.NotFoundError())
this.ProjectEntityUpdateHandler.appendToDoc(
projectId,
docId,
this.appendedLines,
this.source,
userId,
(...args) => {
this.callback(...args)
done()
}
)
})
it('should not talk to DocumentUpdaterHandler', function () {
this.DocumentUpdaterHandler.promises.appendToDocument.should.not.have
.been.called
})
it('should throw the error', function () {
this.callback.should.have.been.calledWith(
sinon.match.instanceOf(Errors.NotFoundError)
)
})
})
describe('when document is found', function () {
beforeEach(function (done) {
this.appendedLines = ['5678', 'def']
this.DocumentUpdaterHandler.promises.appendToDocument = sinon.stub()
this.DocumentUpdaterHandler.promises.appendToDocument
.withArgs(projectId, docId, userId, this.appendedLines, this.source)
.resolves({ rev: 1 })
this.ProjectLocator.promises.findElement = sinon.stub()
this.ProjectLocator.promises.findElement
.withArgs({ project_id: projectId, element_id: docId, type: 'doc' })
.resolves({ element: { _id: docId } })
this.ProjectEntityUpdateHandler.appendToDoc(
projectId,
docId,
this.appendedLines,
this.source,
userId,
(...args) => {
this.callback(...args)
done()
}
)
})
it('should forward call to DocumentUpdaterHandler.appendToDocument', function () {
this.DocumentUpdaterHandler.promises.appendToDocument.should.have.been.calledWith(
projectId,
docId,
userId,
this.appendedLines,
this.source
)
})
it('should return the response from DocumentUpdaterHandler', function () {
this.callback.should.have.been.calledWith(null, { rev: 1 })
})
})
describe('when DocumentUpdater throws an error', function () {
beforeEach(function (done) {
this.appendedLines = ['5678', 'def']
this.DocumentUpdaterHandler.promises.appendToDocument = sinon.stub()
this.DocumentUpdaterHandler.promises.appendToDocument.rejects(
new Error()
)
this.ProjectLocator.promises.findElement = sinon.stub()
this.ProjectLocator.promises.findElement
.withArgs({ project_id: projectId, element_id: docId, type: 'doc' })
.resolves({ element: { _id: docId } })
this.ProjectEntityUpdateHandler.appendToDoc(
projectId,
docId,
this.appendedLines,
this.source,
userId,
(...args) => {
this.callback(...args)
done()
}
)
})
it('should return the response from DocumentUpdaterHandler', function () {
this.callback.should.have.been.calledWith(sinon.match.instanceOf(Error))
})
})
})
})