From a9c94c41840cd0ee433bc8a42ed52ec59af5d7ca Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Wed, 18 Feb 2026 11:03:29 +0000 Subject: [PATCH] Merge pull request #31444 from overleaf/bg-jpa-use-fetch-in-persistence-manager remove requestretry from PersistenceManager GitOrigin-RevId: 1fc9ffdfa7879d7ab4f0f4683544d09fe8526f3d --- package-lock.json | 6 +- services/document-updater/app.js | 2 + services/document-updater/app/js/Errors.js | 5 + .../app/js/PersistenceManager.js | 404 ++++++++------ services/document-updater/package.json | 6 +- .../scripts/check_redis_mongo_sync_state.js | 89 +-- .../js/ApplyingUpdatesToADocTests.js | 2 +- .../acceptance/js/GettingADocumentTests.js | 179 +++++- .../acceptance/js/SettingADocumentTests.js | 159 ++++++ .../test/acceptance/js/helpers/MockWebApi.js | 91 ++- .../js/HistoryManager/HistoryManagerTests.js | 1 - .../PersistenceManagerTests.js | 524 ------------------ 12 files changed, 669 insertions(+), 799 deletions(-) delete mode 100644 services/document-updater/test/unit/js/PersistenceManager/PersistenceManagerTests.js diff --git a/package-lock.json b/package-lock.json index 5988fa0bf4..c03e5f0c4d 100644 --- a/package-lock.json +++ b/package-lock.json @@ -55141,12 +55141,11 @@ "lodash": "^4.17.21", "minimist": "^1.2.8", "mongodb-legacy": "6.1.3", - "overleaf-editor-core": "*", - "request": "2.88.2", - "requestretry": "7.1.0" + "overleaf-editor-core": "*" }, "devDependencies": { "@overleaf/migrations": "*", + "basic-auth": "^2.0.1", "chai": "^4.3.6", "chai-as-promised": "^7.1.1", "cluster-key-slot": "^1.0.5", @@ -55157,6 +55156,7 @@ "sinon": "^9.2.4", "sinon-chai": "^3.7.0", "timekeeper": "^2.0.0", + "tsscmp": "^1.0.6", "typescript": "^5.0.4" } }, diff --git a/services/document-updater/app.js b/services/document-updater/app.js index e55bcdb443..de5be9a113 100644 --- a/services/document-updater/app.js +++ b/services/document-updater/app.js @@ -220,6 +220,8 @@ app.use((error, req, res, next) => { return res.sendStatus(413) } else if (error.statusCode === 413) { return res.status(413).send('request entity too large') + } else if (error instanceof Errors.DocumentValidationError) { + return res.sendStatus(422) } else { logger.error({ err: error, req }, 'request errored') return res.status(500).send('Oops, something went wrong') diff --git a/services/document-updater/app/js/Errors.js b/services/document-updater/app/js/Errors.js index ac1f5875fa..b4b6339734 100644 --- a/services/document-updater/app/js/Errors.js +++ b/services/document-updater/app/js/Errors.js @@ -14,6 +14,9 @@ class OTTypeMismatchError extends OError { super('ot type mismatch', { got, want }) } } +// when the doc returned by web/API fails validation (e.g. missing/invalid fields) +class DocumentValidationError extends OError {} +class WebApiServerError extends OError {} module.exports = { NotFoundError, @@ -22,4 +25,6 @@ module.exports = { DeleteMismatchError, FileTooLargeError, OTTypeMismatchError, + DocumentValidationError, + WebApiServerError, } diff --git a/services/document-updater/app/js/PersistenceManager.js b/services/document-updater/app/js/PersistenceManager.js index c357812326..4160309553 100644 --- a/services/document-updater/app/js/PersistenceManager.js +++ b/services/document-updater/app/js/PersistenceManager.js @@ -1,210 +1,272 @@ -const { promisify } = require('node:util') -const { promisifyMultiResult } = require('@overleaf/promise-utils') +// @ts-check +const { setTimeout } = require('node:timers/promises') const Settings = require('@overleaf/settings') const Errors = require('./Errors') +const OError = require('@overleaf/o-error') const Metrics = require('./Metrics') const logger = require('@overleaf/logger') -const request = require('requestretry').defaults({ - maxAttempts: 2, - retryDelay: 10, -}) +const { fetchJson, RequestFailedError } = require('@overleaf/fetch-utils') +const MAX_ATTEMPTS = 2 +const RETRY_DELAY_MS = 10 // We have to be quick with HTTP calls because we're holding a lock that // expires after 30 seconds. We can't let any errors in the rest of the stack // hold us up, and need to bail out quickly if there is a problem. const MAX_HTTP_REQUEST_LENGTH = 5000 // 5 seconds -function updateMetric(method, error, response) { - // find the status, with special handling for connection timeouts - // https://github.com/request/request#timeouts - let status - if (error && error.connect === true) { - status = `${error.code} (connect)` - } else if (error) { - status = error.code - } else if (response) { - status = response.statusCode - } - - Metrics.inc(method, 1, { status }) - if (error && error.attempts > 1) { - Metrics.inc(`${method}-retries`, 1, { status: 'error' }) - } - if (response && response.attempts > 1) { - Metrics.inc(`${method}-retries`, 1, { status: 'success' }) - } -} - -function getDoc(projectId, docId, options = {}, _callback) { +async function getDocOnce(projectId, docId, options = {}) { const timer = new Metrics.Timer('persistenceManager.getDoc') - if (typeof options === 'function') { - _callback = options - options = {} - } - const callback = function (...args) { - timer.done() - _callback(...args) - } - - const urlPath = `/project/${projectId}/doc/${docId}` - const requestParams = { - url: `${Settings.apis.web.url}${urlPath}`, - method: 'GET', - headers: { - accept: 'application/json', - }, - auth: { - user: Settings.apis.web.user, - pass: Settings.apis.web.pass, - sendImmediately: true, - }, - jar: false, - timeout: MAX_HTTP_REQUEST_LENGTH, - } + const info = { projectId, docId } // for errors + const url = new URL( + `/project/${projectId}/doc/${docId}`, + Settings.apis.web.url + ) if (options.peek) { - requestParams.qs = { peek: 'true' } + // used by resyncs + url.searchParams.set('peek', 'true') } - request(requestParams, (error, res, body) => { - updateMetric('getDoc', error, res) - if (error) { - logger.error({ err: error, projectId, docId }, 'web API request failed') - return callback(new Error('error connecting to web API')) + const fetchParams = { + method: 'GET', + basicAuth: { + user: Settings.apis.web.user, + password: Settings.apis.web.pass, + }, + signal: AbortSignal.timeout(MAX_HTTP_REQUEST_LENGTH), + } + try { + const body = await fetchJson(url, fetchParams) + + if (body.lines == null) { + throw new Errors.DocumentValidationError( + 'web API response had no doc lines', + info + ) } - if (res.statusCode >= 200 && res.statusCode < 300) { - try { - body = JSON.parse(body) - } catch (e) { - return callback(e) - } - if (body.lines == null) { - return callback(new Error('web API response had no doc lines')) - } - if (body.version == null) { - return callback(new Error('web API response had no valid doc version')) - } - if (body.pathname == null) { - return callback(new Error('web API response had no valid doc pathname')) - } - if (!body.pathname) { - logger.warn( - { projectId, docId }, - 'missing pathname in PersistenceManager getDoc' - ) - Metrics.inc('pathname', 1, { - path: 'PersistenceManager.getDoc', - status: body.pathname === '' ? 'zero-length' : 'undefined', + if (body.version == null) { + throw new Errors.DocumentValidationError( + 'web API response had no valid doc version', + info + ) + } + if (body.pathname == null) { + throw new Errors.DocumentValidationError( + 'web API response had no valid doc pathname', + info + ) + } + if (!body.pathname) { + logger.warn( + { projectId, docId }, + 'missing pathname in PersistenceManager getDoc' + ) + Metrics.inc('pathname', 1, { + path: 'PersistenceManager.getDoc', + status: body.pathname === '' ? 'zero-length' : 'undefined', + }) + } + + if (body.otMigrationStage > 0) { + // Use history-ot + body.lines = { content: body.lines.join('\n') } + body.ranges = {} + } + + if (!body.projectHistoryId) { + logger.warn( + { projectId, docId }, + 'projectHistoryId not found for doc from web' + ) + } + Metrics.inc('getDoc', 1, { status: '200' }) + return { + lines: body.lines, + version: body.version, + ranges: body.ranges, + pathname: body.pathname, + projectHistoryId: body.projectHistoryId?.toString(), + historyRangesSupport: body.historyRangesSupport || false, + resolvedCommentIds: body.resolvedCommentIds || [], + } + } catch (err) { + let status + if (err instanceof RequestFailedError) { + status = err.response?.status + } else if (err instanceof Errors.DocumentValidationError) { + status = 'validation-error' + } else if (err instanceof Error && 'code' in err) { + status = err.code + } else { + status = 'unknown' + } + Metrics.inc('getDoc', 1, { status }) + if (err instanceof RequestFailedError) { + if (status === 404) { + throw new Errors.NotFoundError('doc not found', info) + } else if (status === 413) { + throw new Errors.FileTooLargeError('doc exceeds maximum size', info) + } else { + throw new Errors.WebApiServerError('error accessing web API', { + ...info, + status, }) } - - if (body.otMigrationStage > 0) { - // Use history-ot - body.lines = { content: body.lines.join('\n') } - body.ranges = {} - } - - if (!body.projectHistoryId) { - logger.warn( - { projectId, docId }, - 'projectHistoryId not found for doc from web' - ) - } - - callback( - null, - body.lines, - body.version, - body.ranges, - body.pathname, - body.projectHistoryId?.toString(), - body.historyRangesSupport || false, - body.resolvedCommentIds || [] - ) - } else if (res.statusCode === 404) { - callback(new Errors.NotFoundError(`doc not not found: ${urlPath}`)) - } else if (res.statusCode === 413) { - callback( - new Errors.FileTooLargeError(`doc exceeds maximum size: ${urlPath}`) - ) + } else if (err instanceof Errors.DocumentValidationError) { + throw err } else { - callback( - new Error(`error accessing web API: ${urlPath} ${res.statusCode}`) - ) + throw OError.tag(err, 'getDoc failed', info) } - }) + } finally { + timer.done() + } } -function setDoc( +async function setDocOnce( projectId, docId, lines, version, ranges, lastUpdatedAt, - lastUpdatedBy, - _callback + lastUpdatedBy ) { const timer = new Metrics.Timer('persistenceManager.setDoc') - const callback = function (...args) { - timer.done() - _callback(...args) - } - const urlPath = `/project/${projectId}/doc/${docId}` - request( - { - url: `${Settings.apis.web.url}${urlPath}`, - method: 'POST', - json: { - lines, - ranges, - version, - lastUpdatedBy, - lastUpdatedAt, - }, - auth: { - user: Settings.apis.web.user, - pass: Settings.apis.web.pass, - sendImmediately: true, - }, - jar: false, - timeout: MAX_HTTP_REQUEST_LENGTH, + const info = { projectId, docId } // for errors + const url = new URL( + `/project/${projectId}/doc/${docId}`, + Settings.apis.web.url + ) + const fetchParams = { + method: 'POST', + json: { + lines, + ranges, + version, + lastUpdatedBy, + lastUpdatedAt, }, - (error, res, body) => { - updateMetric('setDoc', error, res) - if (error) { - logger.error({ err: error, projectId, docId }, 'web API request failed') - return callback(new Error('error connecting to web API')) - } - if (res.statusCode >= 200 && res.statusCode < 300) { - callback(null, body) - } else if (res.statusCode === 404) { - callback(new Errors.NotFoundError(`doc not not found: ${urlPath}`)) - } else if (res.statusCode === 413) { - callback( - new Errors.FileTooLargeError(`doc exceeds maximum size: ${urlPath}`) - ) + basicAuth: { + user: Settings.apis.web.user, + password: Settings.apis.web.pass, + }, + signal: AbortSignal.timeout(MAX_HTTP_REQUEST_LENGTH), + } + try { + const result = await fetchJson(url, fetchParams) + Metrics.inc('setDoc', 1, { status: '200' }) + return result + } catch (err) { + let status + if (err instanceof RequestFailedError) { + status = err.response?.status + } else if (err instanceof Error && 'code' in err) { + status = err.code + } else { + status = 'unknown' + } + Metrics.inc('setDoc', 1, { status }) + if (err instanceof RequestFailedError) { + if (status === 404) { + throw new Errors.NotFoundError('doc not found', info) + } else if (status === 413) { + throw new Errors.FileTooLargeError('doc exceeds maximum size', info) } else { - callback( - new Error(`error accessing web API: ${urlPath} ${res.statusCode}`) - ) + throw new Errors.WebApiServerError('error accessing web API', { + ...info, + status, + }) + } + } else { + throw OError.tag(err, 'setDoc failed', info) + } + } finally { + timer.done() + } +} + +// Original set of retryable errors from requestretry +const RETRYABLE_ERRORS = new Set([ + 'ECONNRESET', + 'ENOTFOUND', + 'ESOCKETTIMEDOUT', + 'ETIMEDOUT', + 'ECONNREFUSED', + 'EHOSTUNREACH', + 'EPIPE', + 'EAI_AGAIN', + 'EBUSY', +]) + +function isRetryable(error) { + // use the same retryable errors as requestretry + if (error instanceof Errors.WebApiServerError) { + const status = error.info?.status + return ( + typeof status === 'number' && + (status === 429 || (status >= 500 && status < 600)) + ) + } else if (typeof error?.code === 'string') { + return Boolean(RETRYABLE_ERRORS.has(error.code)) + } else { + return false + } +} + +async function callWithRetries(name, fn) { + let remainingAttempts = MAX_ATTEMPTS + while (true) { + try { + const result = await fn() + if (remainingAttempts < MAX_ATTEMPTS) { + Metrics.inc(`${name}-retries`, 1, { status: 'success' }) + } + return result + } catch (err) { + remainingAttempts-- + if (remainingAttempts > 0 && isRetryable(err)) { + await setTimeout(RETRY_DELAY_MS) + continue + } else { + if (remainingAttempts < MAX_ATTEMPTS - 1) { + Metrics.inc(`${name}-retries`, 1, { status: 'error' }) + } + throw err } } - ) + } +} + +async function getDocWithRetries(projectId, docId, options = {}) { + return await callWithRetries('getDoc', async () => { + return await getDocOnce(projectId, docId, options) + }) +} + +async function setDocWithRetries( + projectId, + docId, + lines, + version, + ranges, + lastUpdatedAt, + lastUpdatedBy +) { + return await callWithRetries('setDoc', async () => { + return await setDocOnce( + projectId, + docId, + lines, + version, + ranges, + lastUpdatedAt, + lastUpdatedBy + ) + }) } module.exports = { - getDoc, - setDoc, promises: { - getDoc: promisifyMultiResult(getDoc, [ - 'lines', - 'version', - 'ranges', - 'pathname', - 'projectHistoryId', - 'historyRangesSupport', - 'resolvedCommentIds', - ]), - setDoc: promisify(setDoc), + getDoc: getDocWithRetries, + setDoc: setDocWithRetries, }, } diff --git a/services/document-updater/package.json b/services/document-updater/package.json index 9614bc2b62..7ebaabc444 100644 --- a/services/document-updater/package.json +++ b/services/document-updater/package.json @@ -35,12 +35,11 @@ "lodash": "^4.17.21", "minimist": "^1.2.8", "mongodb-legacy": "6.1.3", - "overleaf-editor-core": "*", - "request": "2.88.2", - "requestretry": "7.1.0" + "overleaf-editor-core": "*" }, "devDependencies": { "@overleaf/migrations": "*", + "basic-auth": "^2.0.1", "chai": "^4.3.6", "chai-as-promised": "^7.1.1", "cluster-key-slot": "^1.0.5", @@ -51,6 +50,7 @@ "sinon": "^9.2.4", "sinon-chai": "^3.7.0", "timekeeper": "^2.0.0", + "tsscmp": "^1.0.6", "typescript": "^5.0.4" } } diff --git a/services/document-updater/scripts/check_redis_mongo_sync_state.js b/services/document-updater/scripts/check_redis_mongo_sync_state.js index 51db47af4d..1be0d870b2 100644 --- a/services/document-updater/scripts/check_redis_mongo_sync_state.js +++ b/services/document-updater/scripts/check_redis_mongo_sync_state.js @@ -10,10 +10,7 @@ const ProjectFlusher = require('../app/js/ProjectFlusher') const ProjectManager = require('../app/js/ProjectManager') const RedisManager = require('../app/js/RedisManager') const Settings = require('@overleaf/settings') -const request = require('requestretry').defaults({ - maxAttempts: 2, - retryDelay: 10, -}) +const { fetchNothing, fetchJson } = require('@overleaf/fetch-utils') const ONLY_PROJECT_ID = process.env.ONLY_PROJECT_ID const AUTO_FIX_VERSION_MISMATCH = @@ -76,33 +73,22 @@ async function updateDocVersionInRedis(docId, redisDoc, mongoDoc) { } async function fixPartiallyDeletedDocMetadata(projectId, docId, pathname) { - await new Promise((resolve, reject) => { - request( + try { + await fetchNothing( + `http://${process.env.DOCSTORE_HOST || '127.0.0.1'}:3016/project/${projectId}/doc/${docId}`, { method: 'PATCH', - url: `http://${process.env.DOCSTORE_HOST || '127.0.0.1'}:3016/project/${projectId}/doc/${docId}`, - timeout: 60 * 1000, + signal: AbortSignal.timeout(60_000), json: { name: Path.basename(pathname), deleted: true, deletedAt: new Date(), }, - }, - (err, res, body) => { - if (err) return reject(err) - const { statusCode } = res - if (statusCode !== 204) { - return reject( - new OError('patch request to docstore failed', { - statusCode, - body, - }) - ) - } - resolve() } ) - }) + } catch (error) { + throw OError.tag(error, 'patch request to docstore failed') + } } async function getDocFromMongo(projectId, docId) { @@ -113,50 +99,25 @@ async function getDocFromMongo(projectId, docId) { throw err } } - const docstoreDoc = await new Promise((resolve, reject) => { - request( - { - url: `http://${process.env.DOCSTORE_HOST || '127.0.0.1'}:3016/project/${projectId}/doc/${docId}/peek`, - timeout: 60 * 1000, - json: true, - }, - (err, res, body) => { - if (err) return reject(err) - const { statusCode } = res - if (statusCode !== 200) { - return reject( - new OError('fallback request to docstore failed', { - statusCode, - body, - }) - ) - } - resolve(body) - } + let docstoreDoc + try { + docstoreDoc = await fetchJson( + `http://${process.env.DOCSTORE_HOST || '127.0.0.1'}:3016/project/${projectId}/doc/${docId}/peek`, + { signal: AbortSignal.timeout(60_000) } ) - }) - const deletedDocName = await new Promise((resolve, reject) => { - request( - { - url: `http://${process.env.DOCSTORE_HOST || '127.0.0.1'}:3016/project/${projectId}/doc-deleted`, - timeout: 60 * 1000, - json: true, - }, - (err, res, body) => { - if (err) return reject(err) - const { statusCode } = res - if (statusCode !== 200) { - return reject( - new OError('list deleted docs request to docstore failed', { - statusCode, - body, - }) - ) - } - resolve(body.find(doc => doc._id === docId)?.name) - } + } catch (err) { + throw OError.tag(err, 'fallback request to docstore failed') + } + let deletedDocName + try { + const body = await fetchJson( + `http://${process.env.DOCSTORE_HOST || '127.0.0.1'}:3016/project/${projectId}/doc-deleted`, + { signal: AbortSignal.timeout(60_000) } ) - }) + deletedDocName = body.find(doc => doc._id === docId)?.name + } catch (err) { + throw OError.tag(err, 'list deleted docs request to docstore failed') + } if (docstoreDoc.deleted && deletedDocName) { return { ...docstoreDoc, diff --git a/services/document-updater/test/acceptance/js/ApplyingUpdatesToADocTests.js b/services/document-updater/test/acceptance/js/ApplyingUpdatesToADocTests.js index 855c4f937e..1cea5bad63 100644 --- a/services/document-updater/test/acceptance/js/ApplyingUpdatesToADocTests.js +++ b/services/document-updater/test/acceptance/js/ApplyingUpdatesToADocTests.js @@ -882,7 +882,7 @@ describe('Applying updates to a doc', function () { JSON.parse(message).should.deep.include({ project_id: this.project_id, doc_id: this.doc_id, - error: `doc not not found: /project/${this.project_id}/doc/${this.doc_id}`, + error: 'doc not found', }) }) }) diff --git a/services/document-updater/test/acceptance/js/GettingADocumentTests.js b/services/document-updater/test/acceptance/js/GettingADocumentTests.js index 9da5c541ea..6fda581ab7 100644 --- a/services/document-updater/test/acceptance/js/GettingADocumentTests.js +++ b/services/document-updater/test/acceptance/js/GettingADocumentTests.js @@ -5,6 +5,7 @@ const MockWebApi = require('./helpers/MockWebApi') const DocUpdaterClient = require('./helpers/DocUpdaterClient') const DocUpdaterApp = require('./helpers/DocUpdaterApp') const { RequestFailedError } = require('@overleaf/fetch-utils') +const PersistenceManager = require('../../../app/js/PersistenceManager') describe('Getting a document', function () { before(async function () { @@ -49,6 +50,53 @@ describe('Getting a document', function () { }) }) + describe('when the document is not loaded and the peek option is used', function () { + before(async function () { + const origGetDocumentController = + MockWebApi.getDocumentController.bind(MockWebApi) + sinon + .stub(MockWebApi, 'getDocumentController') + .callsFake((req, res, next) => { + expect(req.query.peek).to.equal('true') + return origGetDocumentController(req, res, next) + }) + this.project_id = DocUpdaterClient.randomId() + this.doc_id = DocUpdaterClient.randomId() + sinon.spy(MockWebApi, 'getDocument') + + MockWebApi.insertDoc(this.project_id, this.doc_id, { + lines: this.lines, + version: this.version, + }) + // This is only used by the resync code and not exposed on the HTTP + // api so we are calling it directly. + this.returnedDoc = await PersistenceManager.promises.getDoc( + this.project_id, + this.doc_id, + { peek: true } + ) + }) + + after(function () { + MockWebApi.getDocumentController.restore() + MockWebApi.getDocument.restore() + }) + + it('should load the document from the web API with peek=true', function () { + MockWebApi.getDocument + .calledWith(this.project_id, this.doc_id) + .should.equal(true) + }) + + it('should return the document lines', function () { + this.returnedDoc.lines.should.deep.equal(this.lines) + }) + + it('should return the document at its current version', function () { + this.returnedDoc.version.should.equal(this.version) + }) + }) + describe('when the document is already loaded', function () { before(async function () { this.project_id = DocUpdaterClient.randomId() @@ -146,11 +194,11 @@ describe('Getting a document', function () { }) describe('when the web api returns an error', function () { - before(function () { + beforeEach(function () { sinon.stub(MockWebApi, 'getDocument').rejects(new Error('oops')) }) - after(function () { + afterEach(function () { MockWebApi.getDocument.restore() }) @@ -161,6 +209,133 @@ describe('Getting a document', function () { .to.be.rejectedWith(RequestFailedError) .and.eventually.have.nested.property('response.status', 500) }) + + it('should retry the request', async function () { + const projectId = DocUpdaterClient.randomId() + const docId = DocUpdaterClient.randomId() + await expect(DocUpdaterClient.getDoc(projectId, docId)) + .to.be.rejectedWith(RequestFailedError) + .and.eventually.have.nested.property('response.status', 500) + expect(MockWebApi.getDocument).to.be.calledTwice + }) + }) + + describe('when the web api returns a retryable error on the first attempt', function () { + beforeEach(function () { + const origGetDocumentController = + MockWebApi.getDocumentController.bind(MockWebApi) + const getDocumentStub = sinon + .stub(MockWebApi, 'getDocumentController') + .onCall(0) + .callsFake((req, res, next) => { + res.destroy() // simulate a network error + }) + getDocumentStub.onCall(1).callsFake(origGetDocumentController) + }) + + afterEach(function () { + MockWebApi.getDocumentController.restore() + }) + + it('should return 200', async function () { + const projectId = DocUpdaterClient.randomId() + const docId = DocUpdaterClient.randomId() + MockWebApi.insertDoc(projectId, docId, { + lines: this.lines, + version: this.version, + }) + + await expect( + DocUpdaterClient.getDoc(projectId, docId) + ).to.eventually.deep.include({ lines: this.lines, version: this.version }) + }) + + it('should retry the request', async function () { + const projectId = DocUpdaterClient.randomId() + const docId = DocUpdaterClient.randomId() + MockWebApi.insertDoc(projectId, docId, { + lines: this.lines, + version: this.version, + }) + await expect( + DocUpdaterClient.getDoc(projectId, docId) + ).to.eventually.deep.include({ lines: this.lines, version: this.version }) + + expect(MockWebApi.getDocumentController).to.be.calledTwice + }) + }) + + describe('when the web api returns a 413 error', function () { + beforeEach(function () { + sinon + .stub(MockWebApi, 'getDocumentController') + .callsFake((req, res, next) => { + res.sendStatus(413) + }) + }) + + afterEach(function () { + MockWebApi.getDocumentController.restore() + }) + + it('should return 413', async function () { + const projectId = DocUpdaterClient.randomId() + const docId = DocUpdaterClient.randomId() + await expect(DocUpdaterClient.getDoc(projectId, docId)) + .to.be.rejectedWith(RequestFailedError) + .and.eventually.have.nested.property('response.status', 413) + }) + + it('should not retry the request', async function () { + const projectId = DocUpdaterClient.randomId() + const docId = DocUpdaterClient.randomId() + await expect(DocUpdaterClient.getDoc(projectId, docId)) + .to.be.rejectedWith(RequestFailedError) + .and.eventually.have.nested.property('response.status', 413) + expect(MockWebApi.getDocumentController).to.be.calledOnce + }) + }) + + describe('when the web api returns an incomplete doc', function () { + afterEach(function () { + MockWebApi.getDocument.restore() + }) + + it('should return an error for missing lines', async function () { + const projectId = DocUpdaterClient.randomId() + const docId = DocUpdaterClient.randomId() + sinon + .stub(MockWebApi, 'getDocument') + .resolves({ version: 123, pathname: 'test' }) + + await expect(DocUpdaterClient.getDoc(projectId, docId)) + .to.be.rejectedWith(RequestFailedError) + .and.eventually.have.nested.property('response.status', 422) + }) + + it('should return an error for missing version', async function () { + const projectId = DocUpdaterClient.randomId() + const docId = DocUpdaterClient.randomId() + sinon + .stub(MockWebApi, 'getDocument') + .resolves({ lines: [''], pathname: 'test' }) + + await expect(DocUpdaterClient.getDoc(projectId, docId)) + .to.be.rejectedWith(RequestFailedError) + .and.eventually.have.nested.property('response.status', 422) + }) + + it('should return an error for missing pathname', async function () { + const projectId = DocUpdaterClient.randomId() + const docId = DocUpdaterClient.randomId() + sinon + .stub(MockWebApi, 'getDocument') + .resolves({ lines: [''], version: 123 }) + + await expect(DocUpdaterClient.getDoc(projectId, docId)) + .to.be.rejectedWith(RequestFailedError) + .and.eventually.have.nested.property('response.status', 422) + }) }) describe('when the web api http request takes a long time', function () { diff --git a/services/document-updater/test/acceptance/js/SettingADocumentTests.js b/services/document-updater/test/acceptance/js/SettingADocumentTests.js index 192cd8f518..f6f28b4ceb 100644 --- a/services/document-updater/test/acceptance/js/SettingADocumentTests.js +++ b/services/document-updater/test/acceptance/js/SettingADocumentTests.js @@ -757,4 +757,163 @@ describe('Setting a document', function () { }) } }) + describe('when the first request returns a connection error', function () { + beforeEach(function () { + const origSetDocumentController = + MockWebApi.setDocumentController.bind(MockWebApi) + const setDocumentStub = sinon + .stub(MockWebApi, 'setDocumentController') + .onCall(0) + .callsFake((req, res, next) => { + res.destroy() // simulate a network error + }) + setDocumentStub.onCall(1).callsFake(origSetDocumentController) + }) + + afterEach(function () { + MockWebApi.setDocumentController.restore() + }) + + it('should retry on connection error and set the document', async function () { + this.project_id = DocUpdaterClient.randomId() + this.doc_id = DocUpdaterClient.randomId() + MockWebApi.insertDoc(this.project_id, this.doc_id, { + lines: this.lines, + version: this.version, + projectHistoryId: this.project_id, + }) + await DocUpdaterClient.preloadDoc(this.project_id, this.doc_id) + + await expect( + DocUpdaterClient.setDocLines( + this.project_id, + this.doc_id, + this.newLines, + this.source, + this.user_id, + false + ) + ).to.eventually.deep.include({ rev: '123' }) + + expect(MockWebApi.setDocumentController).to.be.calledTwice + }) + }) + + describe('when the document does not exist', function () { + before(function () { + sinon.spy(MockWebApi, 'setDocumentController') + }) + after(function () { + MockWebApi.setDocumentController.restore() + }) + + it('should return 404', async function () { + this.project_id = DocUpdaterClient.randomId() + this.doc_id = DocUpdaterClient.randomId() + MockWebApi.insertDoc(this.project_id, this.doc_id, { + lines: this.lines, + version: this.version, + projectHistoryId: this.project_id, + }) + await DocUpdaterClient.preloadDoc(this.project_id, this.doc_id) + MockWebApi.clearDocs() + + await expect( + DocUpdaterClient.setDocLines( + this.project_id, + this.doc_id, + this.newLines, + this.source, + this.user_id, + false + ) + ) + .to.be.rejectedWith(RequestFailedError) + .and.eventually.have.nested.property('response.status', 404) + + expect(MockWebApi.setDocumentController).to.be.calledOnce + }) + }) + + describe('when the document is too large', function () { + beforeEach(function () { + sinon + .stub(MockWebApi, 'setDocumentController') + .callsFake((req, res, next) => { + res.sendStatus(413) // simulate a large file error + }) + }) + + afterEach(function () { + MockWebApi.setDocumentController.restore() + }) + + it('should return 413', async function () { + this.project_id = DocUpdaterClient.randomId() + this.doc_id = DocUpdaterClient.randomId() + MockWebApi.insertDoc(this.project_id, this.doc_id, { + lines: this.lines, + version: this.version, + projectHistoryId: this.project_id, + }) + await DocUpdaterClient.preloadDoc(this.project_id, this.doc_id) + MockWebApi.clearDocs() + + await expect( + DocUpdaterClient.setDocLines( + this.project_id, + this.doc_id, + this.newLines, + this.source, + this.user_id, + false + ) + ) + .to.be.rejectedWith(RequestFailedError) + .and.eventually.have.nested.property('response.status', 413) + expect(MockWebApi.setDocumentController).to.be.calledOnce + }) + }) + + describe('when the first request returns a 500 error', function () { + beforeEach(function () { + const origSetDocumentController = + MockWebApi.setDocumentController.bind(MockWebApi) + const setDocumentStub = sinon + .stub(MockWebApi, 'setDocumentController') + .onCall(0) + .callsFake((req, res, next) => { + res.sendStatus(500) + }) + setDocumentStub.onCall(1).callsFake(origSetDocumentController) + }) + + afterEach(function () { + MockWebApi.setDocumentController.restore() + }) + + it('should retry on a 500 error and set the document', async function () { + this.project_id = DocUpdaterClient.randomId() + this.doc_id = DocUpdaterClient.randomId() + MockWebApi.insertDoc(this.project_id, this.doc_id, { + lines: this.lines, + version: this.version, + projectHistoryId: this.project_id, + }) + await DocUpdaterClient.preloadDoc(this.project_id, this.doc_id) + + await expect( + DocUpdaterClient.setDocLines( + this.project_id, + this.doc_id, + this.newLines, + this.source, + this.user_id, + false + ) + ).to.eventually.deep.include({ rev: '123' }) + + expect(MockWebApi.setDocumentController).to.be.calledTwice + }) + }) }) diff --git a/services/document-updater/test/acceptance/js/helpers/MockWebApi.js b/services/document-updater/test/acceptance/js/helpers/MockWebApi.js index e4625d4bd7..32be9c1339 100644 --- a/services/document-updater/test/acceptance/js/helpers/MockWebApi.js +++ b/services/document-updater/test/acceptance/js/helpers/MockWebApi.js @@ -1,7 +1,10 @@ let MockWebApi +const basicAuth = require('basic-auth') +const tsscmp = require('tsscmp') const express = require('express') const bodyParser = require('body-parser') const { expressify } = require('@overleaf/promise-utils') +const Settings = require('@overleaf/settings') const app = express() const MAX_REQUEST_SIZE = 2 * (2 * 1024 * 1024 + 64 * 1024) @@ -32,38 +35,79 @@ module.exports = MockWebApi = { lastUpdatedAt, lastUpdatedBy ) { - const doc = - this.docs[`${projectId}:${docId}`] || - (this.docs[`${projectId}:${docId}`] = {}) + if (!(`${projectId}:${docId}` in this.docs)) { + return false + } + const doc = this.docs[`${projectId}:${docId}`] doc.lines = lines doc.version = version doc.ranges = ranges doc.pathname = '/a/b/c.tex' doc.lastUpdatedAt = lastUpdatedAt doc.lastUpdatedBy = lastUpdatedBy + return true }, async getDocument(projectId, docId) { return this.docs[`${projectId}:${docId}`] }, + async getDocumentController(req, res, next) { + try { + const doc = await this.getDocument( + req.params.project_id, + req.params.doc_id + ) + if (doc != null) { + return res.send(JSON.stringify(doc)) + } else { + return res.sendStatus(404) + } + } catch (error) { + return res.sendStatus(500) + } + }, + + async setDocumentController(req, res, next) { + try { + const ok = await this.setDocument( + req.params.project_id, + req.params.doc_id, + req.body.lines, + req.body.version, + req.body.ranges, + req.body.lastUpdatedAt, + req.body.lastUpdatedBy + ) + if (!ok) { + return res.sendStatus(404) + } + return res.json({ rev: '123' }) + } catch (error) { + return res.sendStatus(500) + } + }, + run() { + app.use((req, res, next) => { + const credentials = basicAuth(req) + if ( + !credentials || + !Settings.apis.web.user || + credentials.name !== Settings.apis.web.user || + !Settings.apis.web.pass || + !tsscmp(credentials.pass, Settings.apis.web.pass) + ) { + return res.sendStatus(401) + } else { + next() + } + }) + app.get( '/project/:project_id/doc/:doc_id', expressify(async (req, res, next) => { - try { - const doc = await this.getDocument( - req.params.project_id, - req.params.doc_id - ) - if (doc != null) { - return res.send(JSON.stringify(doc)) - } else { - return res.sendStatus(404) - } - } catch (error) { - return res.sendStatus(500) - } + await this.getDocumentController(req, res, next) }) ) @@ -71,20 +115,7 @@ module.exports = MockWebApi = { '/project/:project_id/doc/:doc_id', bodyParser.json({ limit: MAX_REQUEST_SIZE }), expressify(async (req, res, next) => { - try { - await MockWebApi.setDocument( - req.params.project_id, - req.params.doc_id, - req.body.lines, - req.body.version, - req.body.ranges, - req.body.lastUpdatedAt, - req.body.lastUpdatedBy - ) - return res.json({ rev: '123' }) - } catch (error) { - return res.sendStatus(500) - } + await this.setDocumentController(req, res, next) }) ) diff --git a/services/document-updater/test/unit/js/HistoryManager/HistoryManagerTests.js b/services/document-updater/test/unit/js/HistoryManager/HistoryManagerTests.js index 9e8c3bf9b0..7dca44c758 100644 --- a/services/document-updater/test/unit/js/HistoryManager/HistoryManagerTests.js +++ b/services/document-updater/test/unit/js/HistoryManager/HistoryManagerTests.js @@ -13,7 +13,6 @@ describe('HistoryManager', function () { beforeEach(function () { this.HistoryManager = SandboxedModule.require(modulePath, { requires: { - request: (this.request = {}), '@overleaf/fetch-utils': (this.fetchUtils = { fetchNothing: sinon.stub().resolves(), }), diff --git a/services/document-updater/test/unit/js/PersistenceManager/PersistenceManagerTests.js b/services/document-updater/test/unit/js/PersistenceManager/PersistenceManagerTests.js deleted file mode 100644 index 8c82677474..0000000000 --- a/services/document-updater/test/unit/js/PersistenceManager/PersistenceManagerTests.js +++ /dev/null @@ -1,524 +0,0 @@ -const sinon = require('sinon') -const modulePath = '../../../../app/js/PersistenceManager.js' -const SandboxedModule = require('sandboxed-module') -const Errors = require('../../../../app/js/Errors') - -describe('PersistenceManager', function () { - beforeEach(function () { - this.request = sinon.stub() - this.request.defaults = () => this.request - this.Metrics = { - Timer: class Timer {}, - inc: sinon.stub(), - } - this.Metrics.Timer.prototype.done = sinon.stub() - this.Settings = {} - - this.PersistenceManager = SandboxedModule.require(modulePath, { - requires: { - requestretry: this.request, - '@overleaf/settings': this.Settings, - './Metrics': this.Metrics, - './Errors': Errors, - }, - }) - this.project_id = 'project-id-123' - this.projectHistoryId = 'history-id-123' - this.doc_id = 'doc-id-123' - this.lines = ['one', 'two', 'three'] - this.version = 42 - this.callback = sinon.stub() - this.ranges = { comments: 'mock', entries: 'mock' } - this.pathname = '/a/b/c.tex' - this.lastUpdatedAt = Date.now() - this.lastUpdatedBy = 'last-author-id' - this.historyRangesSupport = false - this.Settings.apis = { - web: { - url: (this.url = 'www.example.com'), - user: (this.user = 'overleaf'), - pass: (this.pass = 'password'), - }, - } - }) - - describe('getDoc', function () { - beforeEach(function () { - this.webResponse = { - lines: this.lines, - version: this.version, - ranges: this.ranges, - pathname: this.pathname, - projectHistoryId: this.projectHistoryId, - historyRangesSupport: this.historyRangesSupport, - } - }) - - describe('with a successful response from the web api', function () { - beforeEach(function () { - this.request.callsArgWith( - 1, - null, - { statusCode: 200 }, - JSON.stringify(this.webResponse) - ) - this.PersistenceManager.getDoc( - this.project_id, - this.doc_id, - this.callback - ) - }) - - it('should call the web api', function () { - this.request - .calledWith({ - url: `${this.url}/project/${this.project_id}/doc/${this.doc_id}`, - method: 'GET', - headers: { - accept: 'application/json', - }, - auth: { - user: this.user, - pass: this.pass, - sendImmediately: true, - }, - jar: false, - timeout: 5000, - }) - .should.equal(true) - }) - - it('should call the callback with the doc lines, version and ranges', function () { - this.callback - .calledWith( - null, - this.lines, - this.version, - this.ranges, - this.pathname, - this.projectHistoryId, - this.historyRangesSupport - ) - .should.equal(true) - }) - - it('should time the execution', function () { - this.Metrics.Timer.prototype.done.called.should.equal(true) - }) - - it('should increment the metric', function () { - this.Metrics.inc - .calledWith('getDoc', 1, { status: 200 }) - .should.equal(true) - }) - }) - - describe('with the peek option', function () { - beforeEach(function () { - this.request.yields( - null, - { statusCode: 200 }, - JSON.stringify(this.webResponse) - ) - this.PersistenceManager.getDoc( - this.project_id, - this.doc_id, - { peek: true }, - this.callback - ) - }) - - it('should call the web api with a peek param', function () { - this.request - .calledWith({ - url: `${this.url}/project/${this.project_id}/doc/${this.doc_id}`, - qs: { peek: 'true' }, - method: 'GET', - headers: { - accept: 'application/json', - }, - auth: { - user: this.user, - pass: this.pass, - sendImmediately: true, - }, - jar: false, - timeout: 5000, - }) - .should.equal(true) - }) - }) - - describe('when request returns an error', function () { - beforeEach(function () { - this.error = new Error('oops') - this.error.code = 'EOOPS' - this.request.callsArgWith(1, this.error, null, null) - this.PersistenceManager.getDoc( - this.project_id, - this.doc_id, - this.callback - ) - }) - - it('should return a generic connection error', function () { - this.callback - .calledWith( - sinon.match - .instanceOf(Error) - .and(sinon.match.has('message', 'error connecting to web API')) - ) - .should.equal(true) - }) - - it('should time the execution', function () { - this.Metrics.Timer.prototype.done.called.should.equal(true) - }) - - it('should increment the metric', function () { - this.Metrics.inc - .calledWith('getDoc', 1, { status: 'EOOPS' }) - .should.equal(true) - }) - }) - - describe('when the request returns 404', function () { - beforeEach(function () { - this.request.callsArgWith(1, null, { statusCode: 404 }, '') - this.PersistenceManager.getDoc( - this.project_id, - this.doc_id, - this.callback - ) - }) - - it('should return a NotFoundError', function () { - this.callback - .calledWith(sinon.match.instanceOf(Errors.NotFoundError)) - .should.equal(true) - }) - - it('should time the execution', function () { - this.Metrics.Timer.prototype.done.called.should.equal(true) - }) - - it('should increment the metric', function () { - this.Metrics.inc - .calledWith('getDoc', 1, { status: 404 }) - .should.equal(true) - }) - }) - - describe('when the request returns 413', function () { - beforeEach(function () { - this.request.callsArgWith(1, null, { statusCode: 413 }, '') - this.PersistenceManager.getDoc( - this.project_id, - this.doc_id, - this.callback - ) - }) - - it('should return a FileTooLargeError', function () { - this.callback - .calledWith(sinon.match.instanceOf(Errors.FileTooLargeError)) - .should.equal(true) - }) - - it('should time the execution', function () { - this.Metrics.Timer.prototype.done.called.should.equal(true) - }) - - it('should increment the metric', function () { - this.Metrics.inc - .calledWith('getDoc', 1, { status: 413 }) - .should.equal(true) - }) - }) - - describe('when the request returns an error status code', function () { - beforeEach(function () { - this.request.callsArgWith(1, null, { statusCode: 500 }, '') - this.PersistenceManager.getDoc( - this.project_id, - this.doc_id, - this.callback - ) - }) - - it('should return an error', function () { - this.callback - .calledWith(sinon.match.instanceOf(Error)) - .should.equal(true) - }) - - it('should time the execution', function () { - this.Metrics.Timer.prototype.done.called.should.equal(true) - }) - - it('should increment the metric', function () { - this.Metrics.inc - .calledWith('getDoc', 1, { status: 500 }) - .should.equal(true) - }) - }) - - describe('when request returns an doc without lines', function () { - beforeEach(function () { - delete this.webResponse.lines - this.request.callsArgWith( - 1, - null, - { statusCode: 200 }, - JSON.stringify(this.webResponse) - ) - this.PersistenceManager.getDoc( - this.project_id, - this.doc_id, - this.callback - ) - }) - - it('should return and error', function () { - this.callback - .calledWith(sinon.match.instanceOf(Error)) - .should.equal(true) - }) - }) - - describe('when request returns an doc without a version', function () { - beforeEach(function () { - delete this.webResponse.version - this.request.callsArgWith( - 1, - null, - { statusCode: 200 }, - JSON.stringify(this.webResponse) - ) - this.PersistenceManager.getDoc( - this.project_id, - this.doc_id, - this.callback - ) - }) - - it('should return and error', function () { - this.callback - .calledWith(sinon.match.instanceOf(Error)) - .should.equal(true) - }) - }) - - describe('when request returns an doc without a pathname', function () { - beforeEach(function () { - delete this.webResponse.pathname - this.request.callsArgWith( - 1, - null, - { statusCode: 200 }, - JSON.stringify(this.webResponse) - ) - this.PersistenceManager.getDoc( - this.project_id, - this.doc_id, - this.callback - ) - }) - - it('should return and error', function () { - this.callback - .calledWith(sinon.match.instanceOf(Error)) - .should.equal(true) - }) - }) - }) - - describe('setDoc', function () { - describe('with a successful response from the web api', function () { - beforeEach(function () { - this.request.callsArgWith(1, null, { statusCode: 200 }) - this.PersistenceManager.setDoc( - this.project_id, - this.doc_id, - this.lines, - this.version, - this.ranges, - this.lastUpdatedAt, - this.lastUpdatedBy, - this.callback - ) - }) - - it('should call the web api', function () { - this.request - .calledWith({ - url: `${this.url}/project/${this.project_id}/doc/${this.doc_id}`, - json: { - lines: this.lines, - version: this.version, - ranges: this.ranges, - lastUpdatedAt: this.lastUpdatedAt, - lastUpdatedBy: this.lastUpdatedBy, - }, - method: 'POST', - auth: { - user: this.user, - pass: this.pass, - sendImmediately: true, - }, - jar: false, - timeout: 5000, - }) - .should.equal(true) - }) - - it('should call the callback without error', function () { - this.callback.calledWith(null).should.equal(true) - }) - - it('should time the execution', function () { - this.Metrics.Timer.prototype.done.called.should.equal(true) - }) - - it('should increment the metric', function () { - this.Metrics.inc - .calledWith('setDoc', 1, { status: 200 }) - .should.equal(true) - }) - }) - - describe('when request returns an error', function () { - beforeEach(function () { - this.error = new Error('oops') - this.error.code = 'EOOPS' - this.request.callsArgWith(1, this.error, null, null) - this.PersistenceManager.setDoc( - this.project_id, - this.doc_id, - this.lines, - this.version, - this.ranges, - this.lastUpdatedAt, - this.lastUpdatedBy, - this.callback - ) - }) - - it('should return a generic connection error', function () { - this.callback - .calledWith( - sinon.match - .instanceOf(Error) - .and(sinon.match.has('message', 'error connecting to web API')) - ) - .should.equal(true) - }) - - it('should time the execution', function () { - this.Metrics.Timer.prototype.done.called.should.equal(true) - }) - - it('should increment the metric', function () { - this.Metrics.inc - .calledWith('setDoc', 1, { status: 'EOOPS' }) - .should.equal(true) - }) - }) - - describe('when the request returns 404', function () { - beforeEach(function () { - this.request.callsArgWith(1, null, { statusCode: 404 }, '') - this.PersistenceManager.setDoc( - this.project_id, - this.doc_id, - this.lines, - this.version, - this.ranges, - this.lastUpdatedAt, - this.lastUpdatedBy, - this.callback - ) - }) - - it('should return a NotFoundError', function () { - this.callback - .calledWith(sinon.match.instanceOf(Errors.NotFoundError)) - .should.equal(true) - }) - - it('should time the execution', function () { - this.Metrics.Timer.prototype.done.called.should.equal(true) - }) - - it('should increment the metric', function () { - this.Metrics.inc - .calledWith('setDoc', 1, { status: 404 }) - .should.equal(true) - }) - }) - - describe('when the request returns 413', function () { - beforeEach(function () { - this.request.callsArgWith(1, null, { statusCode: 413 }, '') - this.PersistenceManager.setDoc( - this.project_id, - this.doc_id, - this.lines, - this.version, - this.ranges, - this.lastUpdatedAt, - this.lastUpdatedBy, - this.callback - ) - }) - - it('should return a FileTooLargeError', function () { - this.callback - .calledWith(sinon.match.instanceOf(Errors.FileTooLargeError)) - .should.equal(true) - }) - - it('should time the execution', function () { - this.Metrics.Timer.prototype.done.called.should.equal(true) - }) - - it('should increment the metric', function () { - this.Metrics.inc - .calledWith('setDoc', 1, { status: 413 }) - .should.equal(true) - }) - }) - - describe('when the request returns an error status code', function () { - beforeEach(function () { - this.request.callsArgWith(1, null, { statusCode: 500 }, '') - this.PersistenceManager.setDoc( - this.project_id, - this.doc_id, - this.lines, - this.version, - this.ranges, - this.lastUpdatedAt, - this.lastUpdatedBy, - this.callback - ) - }) - - it('should return an error', function () { - this.callback - .calledWith(sinon.match.instanceOf(Error)) - .should.equal(true) - }) - - it('should time the execution', function () { - this.Metrics.Timer.prototype.done.called.should.equal(true) - }) - - it('should increment the metric', function () { - this.Metrics.inc - .calledWith('setDoc', 1, { status: 500 }) - .should.equal(true) - }) - }) - }) -})