From 59cdcccc260d7006b8d25ef2f1d880648c8188fa Mon Sep 17 00:00:00 2001 From: Eric Mc Sween Date: Wed, 21 Sep 2022 08:01:32 -0400 Subject: [PATCH] Merge pull request #9647 from overleaf/bg-dropbox-to-overleaf-existing-doc handle updates to existing doc in dropbox to overleaf metadata GitOrigin-RevId: e82955a4a76e62fb649263a94103fdb7f322de85 --- .../app/js/DocumentManager.js | 48 ++++++++++++------- .../document-updater/app/js/HttpController.js | 4 +- .../app/js/PersistenceManager.js | 2 +- .../acceptance/js/SettingADocumentTests.js | 27 ++++++++--- .../test/acceptance/js/helpers/MockWebApi.js | 2 +- .../js/HttpController/HttpControllerTests.js | 9 ++-- .../Features/Documents/DocumentController.js | 4 +- .../Project/ProjectEntityUpdateHandler.js | 26 +++++----- .../ProjectEntityUpdateHandlerTests.js | 6 --- 9 files changed, 77 insertions(+), 51 deletions(-) diff --git a/services/document-updater/app/js/DocumentManager.js b/services/document-updater/app/js/DocumentManager.js index 4af82ae586..a8f5106daa 100644 --- a/services/document-updater/app/js/DocumentManager.js +++ b/services/document-updater/app/js/DocumentManager.js @@ -238,23 +238,32 @@ module.exports = DocumentManager = { // Otherwise we should remove it immediately since nothing else // is using it. if (alreadyLoaded) { - DocumentManager.flushDocIfLoaded(projectId, docId, error => { - if (error) { - return callback(error) + DocumentManager.flushDocIfLoaded( + projectId, + docId, + (error, result) => { + if (error) { + return callback(error) + } + callback(null, result) } - callback(null) - }) + ) } else { - DocumentManager.flushAndDeleteDoc(projectId, docId, {}, error => { - // There is no harm in flushing project history if the previous - // call failed and sometimes it is required - HistoryManager.flushProjectChangesAsync(projectId) + DocumentManager.flushAndDeleteDoc( + projectId, + docId, + {}, + (error, result) => { + // There is no harm in flushing project history if the previous + // call failed and sometimes it is required + HistoryManager.flushProjectChangesAsync(projectId) - if (error) { - return callback(error) + if (error) { + return callback(error) + } + callback(null, result) } - callback(null) - }) + ) } }) }) @@ -302,11 +311,16 @@ module.exports = DocumentManager = { ranges, lastUpdatedAt, lastUpdatedBy, - error => { + (error, result) => { if (error) { return callback(error) } - RedisManager.clearUnflushedTime(docId, callback) + RedisManager.clearUnflushedTime(docId, err => { + if (err) { + return callback(err) + } + callback(null, result) + }) } ) } @@ -321,7 +335,7 @@ module.exports = DocumentManager = { _callback(...args) } - DocumentManager.flushDocIfLoaded(projectId, docId, error => { + DocumentManager.flushDocIfLoaded(projectId, docId, (error, result) => { if (error) { if (options.ignoreFlushErrors) { logger.warn( @@ -340,7 +354,7 @@ module.exports = DocumentManager = { if (error) { return callback(error) } - callback(null) + callback(null, result) }) }) }, diff --git a/services/document-updater/app/js/HttpController.js b/services/document-updater/app/js/HttpController.js index dbe0e8ba67..b23302121d 100644 --- a/services/document-updater/app/js/HttpController.js +++ b/services/document-updater/app/js/HttpController.js @@ -163,13 +163,13 @@ function setDoc(req, res, next) { source, userId, undoing, - error => { + (error, result) => { timer.done() if (error) { return next(error) } logger.debug({ projectId, docId }, 'set doc via http') - res.sendStatus(204) // No Content + res.json(result) } ) } diff --git a/services/document-updater/app/js/PersistenceManager.js b/services/document-updater/app/js/PersistenceManager.js index 88314935bb..bdfa0dadb3 100644 --- a/services/document-updater/app/js/PersistenceManager.js +++ b/services/document-updater/app/js/PersistenceManager.js @@ -159,7 +159,7 @@ function setDoc( return callback(new Error('error connecting to web API')) } if (res.statusCode >= 200 && res.statusCode < 300) { - callback(null) + callback(null, body) } else if (res.statusCode === 404) { callback(new Errors.NotFoundError(`doc not not found: ${urlPath}`)) } else if (res.statusCode === 413) { diff --git a/services/document-updater/test/acceptance/js/SettingADocumentTests.js b/services/document-updater/test/acceptance/js/SettingADocumentTests.js index 05955df5b0..7d66e5e742 100644 --- a/services/document-updater/test/acceptance/js/SettingADocumentTests.js +++ b/services/document-updater/test/acceptance/js/SettingADocumentTests.js @@ -76,6 +76,7 @@ describe('Setting a document', function () { return done(error) } this.statusCode = res.statusCode + this.body = body done() } ) @@ -91,8 +92,8 @@ describe('Setting a document', function () { MockWebApi.setDocument.resetHistory() }) - it('should return a 204 status code', function () { - this.statusCode.should.equal(204) + it('should return a 200 status code', function () { + this.statusCode.should.equal(200) }) it('should send the updated doc lines and version to the web api', function () { @@ -141,6 +142,10 @@ describe('Setting a document', function () { } ) }) + + it('should return the mongo rev in the json response', function () { + this.body.should.deep.equal({ rev: '123' }) + }) }) describe('when the updated doc does not exist in the doc updater', function () { @@ -163,6 +168,7 @@ describe('Setting a document', function () { return done(error) } this.statusCode = res.statusCode + this.body = body setTimeout(done, 200) } ) @@ -174,8 +180,8 @@ describe('Setting a document', function () { MockWebApi.setDocument.resetHistory() }) - it('should return a 204 status code', function () { - this.statusCode.should.equal(204) + it('should return a 200 status code', function () { + this.statusCode.should.equal(200) }) it('should send the updated doc lines to the web api', function () { @@ -206,6 +212,10 @@ describe('Setting a document', function () { } ) }) + + it('should return the mongo rev in the json response', function () { + this.body.should.deep.equal({ rev: '123' }) + }) }) const DOC_TOO_LARGE_TEST_CASES = [ @@ -302,6 +312,7 @@ describe('Setting a document', function () { return done(error) } this.statusCode = res.statusCode + this.body = body setTimeout(done, 200) } ) @@ -313,8 +324,8 @@ describe('Setting a document', function () { MockWebApi.setDocument.resetHistory() }) - it('should return a 204 status code', function () { - this.statusCode.should.equal(204) + it('should return a 200 status code', function () { + this.statusCode.should.equal(200) }) it('should send the updated doc lines to the web api', function () { @@ -322,6 +333,10 @@ describe('Setting a document', function () { .calledWith(this.project_id, this.doc_id, this.newLines) .should.equal(true) }) + + it('should return the mongo rev in the json response', function () { + this.body.should.deep.equal({ rev: '123' }) + }) }) describe('with track changes', function () { diff --git a/services/document-updater/test/acceptance/js/helpers/MockWebApi.js b/services/document-updater/test/acceptance/js/helpers/MockWebApi.js index 7744b30b9e..f687e48e18 100644 --- a/services/document-updater/test/acceptance/js/helpers/MockWebApi.js +++ b/services/document-updater/test/acceptance/js/helpers/MockWebApi.js @@ -99,7 +99,7 @@ module.exports = MockWebApi = { if (error != null) { return res.sendStatus(500) } else { - return res.sendStatus(204) + return res.json({ rev: '123' }) } } ) diff --git a/services/document-updater/test/unit/js/HttpController/HttpControllerTests.js b/services/document-updater/test/unit/js/HttpController/HttpControllerTests.js index 80c3826c8e..adfe461ea7 100644 --- a/services/document-updater/test/unit/js/HttpController/HttpControllerTests.js +++ b/services/document-updater/test/unit/js/HttpController/HttpControllerTests.js @@ -17,6 +17,7 @@ describe('HttpController', function () { './RedisManager': (this.RedisManager = {}), './Metrics': (this.Metrics = {}), './Errors': Errors, + '@overleaf/settings': { max_doc_length: 2 * 1024 * 1024 }, }, }) this.Metrics.Timer = class Timer {} @@ -202,7 +203,9 @@ describe('HttpController', function () { describe('successfully', function () { beforeEach(function () { - this.DocumentManager.setDocWithLock = sinon.stub().callsArgWith(6) + this.DocumentManager.setDocWithLock = sinon + .stub() + .callsArgWith(6, null, { rev: '123' }) this.HttpController.setDoc(this.req, this.res, this.next) }) @@ -219,8 +222,8 @@ describe('HttpController', function () { .should.equal(true) }) - it('should return a successful No Content response', function () { - this.res.sendStatus.calledWith(204).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 () { diff --git a/services/web/app/src/Features/Documents/DocumentController.js b/services/web/app/src/Features/Documents/DocumentController.js index 9624bea4fd..0e4958c19c 100644 --- a/services/web/app/src/Features/Documents/DocumentController.js +++ b/services/web/app/src/Features/Documents/DocumentController.js @@ -94,7 +94,7 @@ function setDocument(req, res, next) { ranges, lastUpdatedAt, lastUpdatedBy, - error => { + (error, result) => { if (error) { OError.tag(error, 'error finding element for getDocument', { docId, @@ -106,7 +106,7 @@ function setDocument(req, res, next) { { docId, projectId }, 'finished receiving set document request from api (docupdater)' ) - res.sendStatus(200) + res.json(result) } ) } diff --git a/services/web/app/src/Features/Project/ProjectEntityUpdateHandler.js b/services/web/app/src/Features/Project/ProjectEntityUpdateHandler.js index e456218a1d..e5886c318a 100644 --- a/services/web/app/src/Features/Project/ProjectEntityUpdateHandler.js +++ b/services/web/app/src/Features/Project/ProjectEntityUpdateHandler.js @@ -201,7 +201,7 @@ const ProjectEntityUpdateHandler = { ) // path will only be present if the doc is not deleted if (!modified || isDeletedDoc) { - return callback() + return callback(null, { rev }) } // Don't need to block for marking as updated ProjectUpdateHandler.markAsUpdated( @@ -218,7 +218,12 @@ const ProjectEntityUpdateHandler = { rev, folderId: folder?._id, }, - callback + err => { + if (err) { + return callback(err) + } + callback(null, { rev, modified }) + } ) } ) @@ -741,7 +746,7 @@ const ProjectEntityUpdateHandler = { userId, docLines, source, - err => { + (err, result) => { if (err != null) { return callback(err) } @@ -749,16 +754,11 @@ const ProjectEntityUpdateHandler = { { projectId, docId: existingDoc._id }, 'notifying users that the document has been updated' ) - DocumentUpdaterHandler.flushDocToMongo( - projectId, - existingDoc._id, - err => { - if (err != null) { - return callback(err) - } - callback(null, existingDoc, existingDoc == null) - } - ) + // there is no need to flush the doc to mongo at this point as docupdater + // flushes it as part of setDoc. + // + // combine rev from response with existing doc metadata + callback(null, { ...existingDoc, ...result }, existingDoc == null) } ) } else { diff --git a/services/web/test/unit/src/Project/ProjectEntityUpdateHandlerTests.js b/services/web/test/unit/src/Project/ProjectEntityUpdateHandlerTests.js index 313c687a7b..d81acb627d 100644 --- a/services/web/test/unit/src/Project/ProjectEntityUpdateHandlerTests.js +++ b/services/web/test/unit/src/Project/ProjectEntityUpdateHandlerTests.js @@ -790,12 +790,6 @@ describe('ProjectEntityUpdateHandler', function () { .should.equal(true) }) - it('flushes the doc contents', function () { - this.DocumentUpdaterHandler.flushDocToMongo - .calledWith(projectId, this.existingDoc._id) - .should.equal(true) - }) - it('returns the doc', function () { this.callback.calledWith(null, this.existingDoc, false) })