Merge pull request #14567 from overleaf/em-history-ranges-flag

Add historyRangesSupport flag to projects

GitOrigin-RevId: 1e3f24a7c6f209bbd34eaaf4caee56dc7061b3da
This commit is contained in:
Eric Mc Sween 2023-09-05 08:41:49 -04:00 committed by Copybot
parent 16c4f6219e
commit 3fa1245860
13 changed files with 162 additions and 167 deletions

2
package-lock.json generated
View file

@ -44410,6 +44410,7 @@
"mocha": "^10.2.0", "mocha": "^10.2.0",
"sandboxed-module": "^2.0.4", "sandboxed-module": "^2.0.4",
"sinon": "^9.2.4", "sinon": "^9.2.4",
"sinon-chai": "^3.7.0",
"timekeeper": "^2.0.0" "timekeeper": "^2.0.0"
} }
}, },
@ -53409,6 +53410,7 @@
"requestretry": "^7.1.0", "requestretry": "^7.1.0",
"sandboxed-module": "^2.0.4", "sandboxed-module": "^2.0.4",
"sinon": "^9.2.4", "sinon": "^9.2.4",
"sinon-chai": "^3.7.0",
"timekeeper": "^2.0.0" "timekeeper": "^2.0.0"
}, },
"dependencies": { "dependencies": {

View file

@ -39,50 +39,27 @@ module.exports = DocumentManager = {
{ projectId, docId }, { projectId, docId },
'doc not in redis so getting from persistence API' 'doc not in redis so getting from persistence API'
) )
PersistenceManager.getDoc( PersistenceManager.getDoc(projectId, docId, (error, doc) => {
projectId, if (error) {
docId, return callback(error)
(error, lines, version, ranges, pathname, projectHistoryId) => { }
logger.debug({ doc }, 'got doc from persistence API')
RedisManager.putDocInMemory(projectId, docId, doc, error => {
if (error) { if (error) {
return callback(error) return callback(error)
} }
logger.debug( callback(
{ null,
projectId, doc.lines,
docId, doc.version,
lines, doc.ranges || {},
version, doc.pathname,
pathname, doc.projectHistoryId,
projectHistoryId, null,
}, false
'got doc from persistence API'
) )
RedisManager.putDocInMemory( })
projectId, })
docId,
lines,
version,
ranges,
pathname,
projectHistoryId,
error => {
if (error) {
return callback(error)
}
callback(
null,
lines,
version,
ranges || {},
pathname,
projectHistoryId,
null,
false
)
}
)
}
)
} else { } else {
callback( callback(
null, null,
@ -517,7 +494,7 @@ module.exports = DocumentManager = {
projectId, projectId,
docId, docId,
{ peek: true }, { peek: true },
(error, lines, version, ranges, pathname, projectHistoryId) => { (error, doc) => {
if (error) { if (error) {
logger.error( logger.error(
{ projectId, docId, getDocError: error }, { projectId, docId, getDocError: error },
@ -527,10 +504,10 @@ module.exports = DocumentManager = {
} }
ProjectHistoryRedisManager.queueResyncDocContent( ProjectHistoryRedisManager.queueResyncDocContent(
projectId, projectId,
projectHistoryId, doc.projectHistoryId,
docId, docId,
lines, doc.lines,
version, doc.version,
path, // use the path from the resyncProjectStructure update path, // use the path from the resyncProjectStructure update
callback callback
) )

View file

@ -93,16 +93,16 @@ function getDoc(projectId, docId, options = {}, _callback) {
status: body.pathname === '' ? 'zero-length' : 'undefined', status: body.pathname === '' ? 'zero-length' : 'undefined',
}) })
} }
callback( callback(null, {
null, lines: body.lines,
body.lines, version: body.version,
body.version, ranges: body.ranges,
body.ranges, pathname: body.pathname,
body.pathname, projectHistoryId: body.projectHistoryId?.toString(),
body.projectHistoryId?.toString() historyRangesSupport: body.historyRangesSupport || false,
) })
} else if (res.statusCode === 404) { } else if (res.statusCode === 404) {
callback(new Errors.NotFoundError(`doc not not found: ${urlPath}`)) callback(new Errors.NotFoundError(`doc not found: ${urlPath}`))
} else if (res.statusCode === 413) { } else if (res.statusCode === 413) {
callback( callback(
new Errors.FileTooLargeError(`doc exceeds maximum size: ${urlPath}`) new Errors.FileTooLargeError(`doc exceeds maximum size: ${urlPath}`)
@ -160,7 +160,7 @@ function setDoc(
if (res.statusCode >= 200 && res.statusCode < 300) { if (res.statusCode >= 200 && res.statusCode < 300) {
callback(null, body) callback(null, body)
} else if (res.statusCode === 404) { } else if (res.statusCode === 404) {
callback(new Errors.NotFoundError(`doc not not found: ${urlPath}`)) callback(new Errors.NotFoundError(`doc not found: ${urlPath}`))
} else if (res.statusCode === 413) { } else if (res.statusCode === 413) {
callback( callback(
new Errors.FileTooLargeError(`doc exceeds maximum size: ${urlPath}`) new Errors.FileTooLargeError(`doc exceeds maximum size: ${urlPath}`)

View file

@ -30,47 +30,44 @@ const keys = Settings.redis.documentupdater.key_schema
module.exports = RedisManager = { module.exports = RedisManager = {
rclient, rclient,
putDocInMemory( putDocInMemory(projectId, docId, doc, _callback) {
projectId,
docId,
docLines,
version,
ranges,
pathname,
projectHistoryId,
_callback
) {
const timer = new metrics.Timer('redis.put-doc') const timer = new metrics.Timer('redis.put-doc')
const callback = error => { const callback = error => {
timer.done() timer.done()
_callback(error) _callback(error)
} }
const docLinesArray = docLines const docLinesJson = JSON.stringify(doc.lines)
docLines = JSON.stringify(docLines) if (docLinesJson.indexOf('\u0000') !== -1) {
if (docLines.indexOf('\u0000') !== -1) {
const error = new Error('null bytes found in doc lines') const error = new Error('null bytes found in doc lines')
// this check was added to catch memory corruption in JSON.stringify. // this check was added to catch memory corruption in JSON.stringify.
// It sometimes returned null bytes at the end of the string. // It sometimes returned null bytes at the end of the string.
logger.error({ err: error, docId, docLines }, error.message) logger.error({ err: error, docId, docLines: docLinesJson }, error.message)
return callback(error) return callback(error)
} }
// Do an optimised size check on the docLines using the serialised // Do an optimised size check on the docLines using the serialised
// length as an upper bound // length as an upper bound
const sizeBound = docLines.length const sizeBound = docLinesJson.length
if (docIsTooLarge(sizeBound, docLinesArray, Settings.max_doc_length)) { if (docIsTooLarge(sizeBound, doc.lines, Settings.max_doc_length)) {
const docSize = docLines.length const docSize = docLinesJson.length
const err = new Error('blocking doc insert into redis: doc is too large') const err = new Error('blocking doc insert into redis: doc is too large')
logger.error({ projectId, docId, err, docSize }, err.message) logger.error({ projectId, docId, err, docSize }, err.message)
return callback(err) return callback(err)
} }
const docHash = RedisManager._computeHash(docLines) const docHash = RedisManager._computeHash(docLinesJson)
// record bytes sent to redis // record bytes sent to redis
metrics.summary('redis.docLines', docLines.length, { status: 'set' }) metrics.summary('redis.docLines', docLinesJson.length, { status: 'set' })
logger.debug( logger.debug(
{ projectId, docId, version, docHash, pathname, projectHistoryId }, {
projectId,
docId,
version: doc.version,
docHash,
pathname: doc.pathname,
projectHistoryId: doc.projectHistoryId,
},
'putting doc in redis' 'putting doc in redis'
) )
RedisManager._serializeRanges(ranges, (error, ranges) => { RedisManager._serializeRanges(doc.ranges, (error, ranges) => {
if (error) { if (error) {
logger.error({ err: error, docId, projectId }, error.message) logger.error({ err: error, docId, projectId }, error.message)
return callback(error) return callback(error)
@ -82,25 +79,27 @@ module.exports = RedisManager = {
error => { error => {
if (error) return callback(error) if (error) return callback(error)
if (!pathname) { if (!doc.pathname) {
metrics.inc('pathname', 1, { metrics.inc('pathname', 1, {
path: 'RedisManager.setDoc', path: 'RedisManager.setDoc',
status: pathname === '' ? 'zero-length' : 'undefined', status: doc.pathname === '' ? 'zero-length' : 'undefined',
}) })
} }
rclient.mset( const multi = rclient.multi()
{ multi.mset({
[keys.docLines({ doc_id: docId })]: docLines, [keys.docLines({ doc_id: docId })]: docLinesJson,
[keys.projectKey({ doc_id: docId })]: projectId, [keys.projectKey({ doc_id: docId })]: projectId,
[keys.docVersion({ doc_id: docId })]: version, [keys.docVersion({ doc_id: docId })]: doc.version,
[keys.docHash({ doc_id: docId })]: docHash, [keys.docHash({ doc_id: docId })]: docHash,
[keys.ranges({ doc_id: docId })]: ranges, [keys.ranges({ doc_id: docId })]: ranges,
[keys.pathname({ doc_id: docId })]: pathname, [keys.pathname({ doc_id: docId })]: doc.pathname,
[keys.projectHistoryId({ doc_id: docId })]: projectHistoryId, [keys.projectHistoryId({ doc_id: docId })]: doc.projectHistoryId,
}, })
callback if (doc.historyRangesSupport) {
) multi.sadd(keys.historyRangesSupport(), docId)
}
multi.exec(callback)
} }
) )
}) })
@ -132,6 +131,7 @@ module.exports = RedisManager = {
keys.lastUpdatedAt({ doc_id: docId }), keys.lastUpdatedAt({ doc_id: docId }),
keys.lastUpdatedBy({ doc_id: docId }) keys.lastUpdatedBy({ doc_id: docId })
) )
multi.srem(keys.historyRangesSupport(), docId)
multi.exec((error, response) => { multi.exec((error, response) => {
if (error) { if (error) {
return callback(error) return callback(error)

View file

@ -140,6 +140,9 @@ module.exports = {
flushAndDeleteQueue() { flushAndDeleteQueue() {
return 'DocUpdaterFlushAndDeleteQueue' return 'DocUpdaterFlushAndDeleteQueue'
}, },
historyRangesSupport() {
return 'HistoryRangesSupport'
},
}, },
}, },
}, },

View file

@ -39,6 +39,7 @@
"mocha": "^10.2.0", "mocha": "^10.2.0",
"sandboxed-module": "^2.0.4", "sandboxed-module": "^2.0.4",
"sinon": "^9.2.4", "sinon": "^9.2.4",
"sinon-chai": "^3.7.0",
"timekeeper": "^2.0.0" "timekeeper": "^2.0.0"
} }
} }

View file

@ -679,7 +679,7 @@ describe('Applying updates to a doc', function () {
JSON.parse(message).should.deep.include({ JSON.parse(message).should.deep.include({
project_id: this.project_id, project_id: this.project_id,
doc_id: this.doc_id, doc_id: this.doc_id,
error: `doc not not found: /project/${this.project_id}/doc/${this.doc_id}`, error: `doc not found: /project/${this.project_id}/doc/${this.doc_id}`,
}) })
}) })
}) })

View file

@ -1,9 +1,11 @@
const chai = require('chai') const chai = require('chai')
const SandboxedModule = require('sandboxed-module') const SandboxedModule = require('sandboxed-module')
const sinon = require('sinon') const sinon = require('sinon')
const sinonChai = require('sinon-chai')
// Chai configuration // Chai configuration
chai.should() chai.should()
chai.use(sinonChai)
// Global stubs // Global stubs
const sandbox = sinon.createSandbox() const sandbox = sinon.createSandbox()

View file

@ -42,6 +42,14 @@ describe('DocumentManager', function () {
this.lastUpdatedAt = Date.now() this.lastUpdatedAt = Date.now()
this.lastUpdatedBy = 'last-author-id' this.lastUpdatedBy = 'last-author-id'
this.source = 'external-source' this.source = 'external-source'
this.doc = {
lines: this.lines,
version: this.version,
ranges: this.ranges,
pathname: this.pathname,
projectHistoryId: this.projectHistoryId,
historyRangesSupport: false,
}
}) })
afterEach(function () { afterEach(function () {
@ -373,15 +381,7 @@ describe('DocumentManager', function () {
.callsArgWith(2, null, null, null, null, null, null) .callsArgWith(2, null, null, null, null, null, null)
this.PersistenceManager.getDoc = sinon this.PersistenceManager.getDoc = sinon
.stub() .stub()
.callsArgWith( .callsArgWith(2, null, this.doc)
2,
null,
this.lines,
this.version,
this.ranges,
this.pathname,
this.projectHistoryId
)
this.RedisManager.putDocInMemory = sinon.stub().yields() this.RedisManager.putDocInMemory = sinon.stub().yields()
this.DocumentManager.getDoc(this.project_id, this.doc_id, this.callback) this.DocumentManager.getDoc(this.project_id, this.doc_id, this.callback)
}) })
@ -400,15 +400,7 @@ describe('DocumentManager', function () {
it('should set the doc in Redis', function () { it('should set the doc in Redis', function () {
this.RedisManager.putDocInMemory this.RedisManager.putDocInMemory
.calledWith( .calledWith(this.project_id, this.doc_id, this.doc)
this.project_id,
this.doc_id,
this.lines,
this.version,
this.ranges,
this.pathname,
this.projectHistoryId
)
.should.equal(true) .should.equal(true)
}) })
@ -1109,16 +1101,7 @@ describe('DocumentManager', function () {
beforeEach(function () { beforeEach(function () {
this.pathnameFromProjectStructureUpdate = '/foo/bar.tex' this.pathnameFromProjectStructureUpdate = '/foo/bar.tex'
this.RedisManager.getDoc = sinon.stub().callsArgWith(2, null) this.RedisManager.getDoc = sinon.stub().callsArgWith(2, null)
this.PersistenceManager.getDoc = sinon this.PersistenceManager.getDoc = sinon.stub().yields(null, this.doc)
.stub()
.yields(
null,
this.lines,
this.version,
this.ranges,
this.pathname,
this.projectHistoryId
)
this.ProjectHistoryRedisManager.queueResyncDocContent = sinon.stub() this.ProjectHistoryRedisManager.queueResyncDocContent = sinon.stub()
this.DocumentManager.resyncDocContents( this.DocumentManager.resyncDocContents(
this.project_id, this.project_id,

View file

@ -32,6 +32,14 @@ describe('PersistenceManager', function () {
this.pathname = '/a/b/c.tex' this.pathname = '/a/b/c.tex'
this.lastUpdatedAt = Date.now() this.lastUpdatedAt = Date.now()
this.lastUpdatedBy = 'last-author-id' this.lastUpdatedBy = 'last-author-id'
this.doc = {
lines: this.lines,
version: this.version,
ranges: this.ranges,
projectHistoryId: this.projectHistoryId,
pathname: this.pathname,
historyRangesSupport: false,
}
this.Settings.apis = { this.Settings.apis = {
web: { web: {
url: (this.url = 'www.example.com'), url: (this.url = 'www.example.com'),
@ -87,16 +95,7 @@ describe('PersistenceManager', function () {
}) })
it('should call the callback with the doc lines, version and ranges', function () { it('should call the callback with the doc lines, version and ranges', function () {
this.callback this.callback.should.have.been.calledWith(null, this.doc)
.calledWith(
null,
this.lines,
this.version,
this.ranges,
this.pathname,
this.projectHistoryId
)
.should.equal(true)
}) })
it('should time the execution', function () { it('should time the execution', function () {

View file

@ -7,7 +7,7 @@ const tk = require('timekeeper')
describe('RedisManager', function () { describe('RedisManager', function () {
beforeEach(function () { beforeEach(function () {
this.multi = { exec: sinon.stub() } this.multi = { exec: sinon.stub().yields() }
this.rclient = { multi: () => this.multi } this.rclient = { multi: () => this.multi }
tk.freeze(new Date()) tk.freeze(new Date())
this.RedisManager = SandboxedModule.require(modulePath, { this.RedisManager = SandboxedModule.require(modulePath, {
@ -63,6 +63,9 @@ describe('RedisManager', function () {
lastUpdatedAt({ doc_id: docId }) { lastUpdatedAt({ doc_id: docId }) {
return `lastUpdatedAt:${docId}` return `lastUpdatedAt:${docId}`
}, },
historyRangesSupport() {
return 'HistoryRangesSupport'
},
}, },
}, },
}, },
@ -759,16 +762,20 @@ describe('RedisManager', function () {
describe('putDocInMemory', function () { describe('putDocInMemory', function () {
beforeEach(function () { beforeEach(function () {
this.rclient.mset = sinon.stub().yields(null) this.multi.mset = sinon.stub()
this.multi.sadd = sinon.stub()
this.rclient.sadd = sinon.stub().yields() this.rclient.sadd = sinon.stub().yields()
this.lines = ['one', 'two', 'three', 'これは'] this.doc = {
this.version = 42 lines: ['one', 'two', 'three', 'これは'],
version: 42,
ranges: { comments: 'mock', entries: 'mock' },
pathname: '/a/b/c.tex',
projectHistoryId: this.projectHistoryId,
}
this.hash = crypto this.hash = crypto
.createHash('sha1') .createHash('sha1')
.update(JSON.stringify(this.lines), 'utf8') .update(JSON.stringify(this.doc.lines), 'utf8')
.digest('hex') .digest('hex')
this.ranges = { comments: 'mock', entries: 'mock' }
this.pathname = '/a/b/c.tex'
}) })
describe('with non-empty ranges', function () { describe('with non-empty ranges', function () {
@ -776,25 +783,21 @@ describe('RedisManager', function () {
this.RedisManager.putDocInMemory( this.RedisManager.putDocInMemory(
this.project_id, this.project_id,
this.docId, this.docId,
this.lines, this.doc,
this.version,
this.ranges,
this.pathname,
this.projectHistoryId,
done done
) )
}) })
it('should set all the details in a single MSET call', function () { it('should set all the details in a single MSET call', function () {
this.rclient.mset this.multi.mset
.calledWith({ .calledWith({
[`doclines:${this.docId}`]: JSON.stringify(this.lines), [`doclines:${this.docId}`]: JSON.stringify(this.doc.lines),
[`ProjectId:${this.docId}`]: this.project_id, [`ProjectId:${this.docId}`]: this.project_id,
[`DocVersion:${this.docId}`]: this.version, [`DocVersion:${this.docId}`]: this.doc.version,
[`DocHash:${this.docId}`]: this.hash, [`DocHash:${this.docId}`]: this.hash,
[`Ranges:${this.docId}`]: JSON.stringify(this.ranges), [`Ranges:${this.docId}`]: JSON.stringify(this.doc.ranges),
[`Pathname:${this.docId}`]: this.pathname, [`Pathname:${this.docId}`]: this.doc.pathname,
[`ProjectHistoryId:${this.docId}`]: this.projectHistoryId, [`ProjectHistoryId:${this.docId}`]: this.doc.projectHistoryId,
}) })
.should.equal(true) .should.equal(true)
}) })
@ -808,32 +811,33 @@ describe('RedisManager', function () {
it('should not log any errors', function () { it('should not log any errors', function () {
this.logger.error.calledWith().should.equal(false) this.logger.error.calledWith().should.equal(false)
}) })
it('should not add the document to the HistoryRangesSupport set in Redis', function () {
this.multi.sadd.should.not.have.been.calledWith('HistoryRangesSupport')
})
}) })
describe('with empty ranges', function () { describe('with empty ranges', function () {
beforeEach(function (done) { beforeEach(function (done) {
this.doc.ranges = {}
this.RedisManager.putDocInMemory( this.RedisManager.putDocInMemory(
this.project_id, this.project_id,
this.docId, this.docId,
this.lines, this.doc,
this.version,
{},
this.pathname,
this.projectHistoryId,
done done
) )
}) })
it('should unset ranges', function () { it('should unset ranges', function () {
this.rclient.mset this.multi.mset
.calledWith({ .calledWith({
[`doclines:${this.docId}`]: JSON.stringify(this.lines), [`doclines:${this.docId}`]: JSON.stringify(this.doc.lines),
[`ProjectId:${this.docId}`]: this.project_id, [`ProjectId:${this.docId}`]: this.project_id,
[`DocVersion:${this.docId}`]: this.version, [`DocVersion:${this.docId}`]: this.doc.version,
[`DocHash:${this.docId}`]: this.hash, [`DocHash:${this.docId}`]: this.hash,
[`Ranges:${this.docId}`]: null, [`Ranges:${this.docId}`]: null,
[`Pathname:${this.docId}`]: this.pathname, [`Pathname:${this.docId}`]: this.doc.pathname,
[`ProjectHistoryId:${this.docId}`]: this.projectHistoryId, [`ProjectHistoryId:${this.docId}`]: this.doc.projectHistoryId,
}) })
.should.equal(true) .should.equal(true)
}) })
@ -847,11 +851,7 @@ describe('RedisManager', function () {
this.RedisManager.putDocInMemory( this.RedisManager.putDocInMemory(
this.project_id, this.project_id,
this.docId, this.docId,
this.lines, this.doc,
this.version,
this.ranges,
this.pathname,
this.projectHistoryId,
this.callback this.callback
) )
}) })
@ -879,11 +879,7 @@ describe('RedisManager', function () {
this.RedisManager.putDocInMemory( this.RedisManager.putDocInMemory(
this.project_id, this.project_id,
this.docId, this.docId,
this.lines, this.doc,
this.version,
this.ranges,
this.pathname,
this.projectHistoryId,
this.callback this.callback
) )
}) })
@ -898,6 +894,25 @@ describe('RedisManager', function () {
.should.equal(true) .should.equal(true)
}) })
}) })
describe('with history ranges support', function () {
beforeEach(function () {
this.doc.historyRangesSupport = true
this.RedisManager.putDocInMemory(
this.project_id,
this.docId,
this.doc,
this.callback
)
})
it('should add the document to the HistoryRangesSupport set in Redis', function () {
this.multi.sadd.should.have.been.calledWith(
'HistoryRangesSupport',
this.docId
)
})
})
}) })
describe('removeDocFromMemory', function () { describe('removeDocFromMemory', function () {
@ -905,7 +920,6 @@ describe('RedisManager', function () {
this.multi.strlen = sinon.stub() this.multi.strlen = sinon.stub()
this.multi.del = sinon.stub() this.multi.del = sinon.stub()
this.multi.srem = sinon.stub() this.multi.srem = sinon.stub()
this.multi.exec.yields()
this.RedisManager.removeDocFromMemory(this.project_id, this.docId, done) this.RedisManager.removeDocFromMemory(this.project_id, this.docId, done)
}) })
@ -935,6 +949,13 @@ describe('RedisManager', function () {
.calledWith(`DocsIn:${this.project_id}`, this.docId) .calledWith(`DocsIn:${this.project_id}`, this.docId)
.should.equal(true) .should.equal(true)
}) })
it('should remove the docId from the HistoryRangesSupport set', function () {
this.multi.srem.should.have.been.calledWith(
'HistoryRangesSupport',
this.docId
)
})
}) })
describe('clearProjectState', function () { describe('clearProjectState', function () {

View file

@ -35,6 +35,11 @@ async function getDocument(req, res) {
plainTextResponse(res, lines.join('\n')) plainTextResponse(res, lines.join('\n'))
} else { } else {
const projectHistoryId = _.get(project, 'overleaf.history.id') const projectHistoryId = _.get(project, 'overleaf.history.id')
const historyRangesSupport = _.get(
project,
'overleaf.history.rangesSupportEnabled',
false
)
// all projects are now migrated to Full Project History, keeping the field // all projects are now migrated to Full Project History, keeping the field
// for API compatibility // for API compatibility
@ -47,6 +52,7 @@ async function getDocument(req, res) {
pathname: path.fileSystem, pathname: path.fileSystem,
projectHistoryId, projectHistoryId,
projectHistoryType, projectHistoryType,
historyRangesSupport,
}) })
} }
} }

View file

@ -95,6 +95,7 @@ describe('DocumentController', function () {
pathname: this.pathname, pathname: this.pathname,
projectHistoryId: this.project.overleaf.history.id, projectHistoryId: this.project.overleaf.history.id,
projectHistoryType: 'project-history', projectHistoryType: 'project-history',
historyRangesSupport: false,
}) })
) )
}) })