From eedf5367fc0cf5befcd3600176632e4b05a24b94 Mon Sep 17 00:00:00 2001 From: Alf Eaton Date: Mon, 10 Mar 2025 11:40:48 +0000 Subject: [PATCH] Store/update entity sha after importing from GitHub (#23935) * Refactor WebApiManager * Store/update entity sha after importing GitOrigin-RevId: 604bf3b8010140355c21afca01a54237a301eb92 --- .../ThirdPartyDataStore/TpdsController.mjs | 35 +++++++--- .../ThirdPartyDataStore/UpdateMerger.js | 14 ++-- .../TpdsControllerTests.mjs | 65 +++++++++++-------- .../ThirdPartyDataStore/UpdateMergerTests.js | 54 ++++++++++++--- 4 files changed, 118 insertions(+), 50 deletions(-) diff --git a/services/web/app/src/Features/ThirdPartyDataStore/TpdsController.mjs b/services/web/app/src/Features/ThirdPartyDataStore/TpdsController.mjs index 1db15aa932..2b8667bed4 100644 --- a/services/web/app/src/Features/ThirdPartyDataStore/TpdsController.mjs +++ b/services/web/app/src/Features/ThirdPartyDataStore/TpdsController.mjs @@ -138,36 +138,50 @@ async function updateFolder(req, res) { // .gitignore, etc because people are generally more explicit with the files // they want in git. -async function updateProjectContents(req, res, next) { +async function updateProjectContents(req, res) { const projectId = req.params.project_id const path = `/${req.params[0]}` // UpdateMerger expects leading slash const source = req.headers['x-update-source'] || 'unknown' try { - await UpdateMerger.promises.mergeUpdate(null, projectId, path, req, source) + const metadata = await UpdateMerger.promises.mergeUpdate( + null, + projectId, + path, + req, + source + ) + res.json({ + entityId: metadata.entityId.toString(), + rev: metadata.rev, + }) } catch (error) { if ( error instanceof Errors.InvalidNameError || error instanceof Errors.DuplicateNameError ) { - return res.sendStatus(422) + res.sendStatus(422) } else { throw error } } - res.sendStatus(200) } -async function deleteProjectContents(req, res, next) { +async function deleteProjectContents(req, res) { const projectId = req.params.project_id const path = `/${req.params[0]}` // UpdateMerger expects leading slash const source = req.headers['x-update-source'] || 'unknown' - await UpdateMerger.promises.deleteUpdate(null, projectId, path, source) - res.sendStatus(200) + const entityId = await UpdateMerger.promises.deleteUpdate( + null, + projectId, + path, + source + ) + res.json({ entityId }) } -async function getQueues(req, res, next) { +async function getQueues(req, res) { const userId = SessionManager.getLoggedInUserId(req.session) res.json(await TpdsQueueManager.promises.getQueues(userId)) } @@ -206,6 +220,11 @@ export default { deleteProjectContents: expressify(deleteProjectContents), getQueues: expressify(getQueues), + promises: { + deleteProjectContents, + updateProjectContents, + }, + // for tests only parseParams, } diff --git a/services/web/app/src/Features/ThirdPartyDataStore/UpdateMerger.js b/services/web/app/src/Features/ThirdPartyDataStore/UpdateMerger.js index e073ccdd54..e67d5fc30d 100644 --- a/services/web/app/src/Features/ThirdPartyDataStore/UpdateMerger.js +++ b/services/web/app/src/Features/ThirdPartyDataStore/UpdateMerger.js @@ -12,15 +12,15 @@ const { pipeline } = require('stream/promises') async function mergeUpdate(userId, projectId, path, updateRequest, source) { const fsPath = await writeUpdateToDisk(projectId, updateRequest) + try { - const metadata = await _mergeUpdate(userId, projectId, path, fsPath, source) - return metadata + // note: important to await here so file reading finishes before cleaning up below + return await _mergeUpdate(userId, projectId, path, fsPath, source) } finally { - try { - await fsPromises.unlink(fsPath) - } catch (err) { + // note: not awaited or thrown + fsPromises.unlink(fsPath).catch(err => { logger.err({ err, projectId, fsPath }, 'error deleting file') - } + }) } } @@ -128,7 +128,7 @@ async function _mergeUpdate(userId, projectId, path, fsPath, source) { async function deleteUpdate(userId, projectId, path, source) { try { - await EditorController.promises.deleteEntityWithPath( + return await EditorController.promises.deleteEntityWithPath( projectId, path, source, diff --git a/services/web/test/unit/src/ThirdPartyDataStore/TpdsControllerTests.mjs b/services/web/test/unit/src/ThirdPartyDataStore/TpdsControllerTests.mjs index 10e90ed8e6..4dd72b117f 100644 --- a/services/web/test/unit/src/ThirdPartyDataStore/TpdsControllerTests.mjs +++ b/services/web/test/unit/src/ThirdPartyDataStore/TpdsControllerTests.mjs @@ -23,14 +23,14 @@ describe('TpdsController', function () { this.TpdsUpdateHandler = { promises: { newUpdate: sinon.stub().resolves(this.metadata), - deleteUpdate: sinon.stub().resolves(), + deleteUpdate: sinon.stub().resolves(this.metadata.entityId), createFolder: sinon.stub().resolves(), }, } this.UpdateMerger = { promises: { - mergeUpdate: sinon.stub().resolves(this.file), - deleteUpdate: sinon.stub().resolves(), + mergeUpdate: sinon.stub().resolves(this.metadata), + deleteUpdate: sinon.stub().resolves(this.metadata.entityId), }, } this.NotificationsBuilder = { @@ -377,7 +377,7 @@ describe('TpdsController', function () { }) describe('updateProjectContents', function () { - beforeEach(function (done) { + beforeEach(async function () { this.req = { params: { 0: (this.path = 'chapters/main.tex'), @@ -390,34 +390,38 @@ describe('TpdsController', function () { 'x-update-source': (this.source = 'github'), }, } + this.res = { - sendStatus: sinon.stub().callsFake(() => { - done() - }), + json: sinon.stub(), + sendStatus: sinon.stub(), } - this.TpdsController.updateProjectContents(this.req, this.res, this.next) + await this.TpdsController.promises.updateProjectContents( + this.req, + this.res + ) }) it('should merge the update', function () { - this.UpdateMerger.promises.mergeUpdate - .calledWith( - null, - this.project_id, - `/${this.path}`, - this.req, - this.source - ) - .should.equal(true) + this.UpdateMerger.promises.mergeUpdate.should.be.calledWith( + null, + this.project_id, + `/${this.path}`, + this.req, + this.source + ) }) it('should return a success', function () { - this.res.sendStatus.calledWith(200).should.equal(true) + this.res.json.should.be.calledWith({ + entityId: this.metadata.entityId.toString(), + rev: this.metadata.rev, + }) }) }) describe('deleteProjectContents', function () { - beforeEach(function (done) { + beforeEach(async function () { this.req = { params: { 0: (this.path = 'chapters/main.tex'), @@ -431,22 +435,29 @@ describe('TpdsController', function () { }, } this.res = { - sendStatus: sinon.stub().callsFake(() => { - done() - }), + sendStatus: sinon.stub(), + json: sinon.stub(), } - this.TpdsController.deleteProjectContents(this.req, this.res, this.next) + await this.TpdsController.promises.deleteProjectContents( + this.req, + this.res + ) }) it('should delete the file', function () { - this.UpdateMerger.promises.deleteUpdate - .calledWith(null, this.project_id, `/${this.path}`, this.source) - .should.equal(true) + this.UpdateMerger.promises.deleteUpdate.should.be.calledWith( + null, + this.project_id, + `/${this.path}`, + this.source + ) }) it('should return a success', function () { - this.res.sendStatus.calledWith(200).should.equal(true) + this.res.json.should.be.calledWith({ + entityId: this.metadata.entityId, + }) }) }) diff --git a/services/web/test/unit/src/ThirdPartyDataStore/UpdateMergerTests.js b/services/web/test/unit/src/ThirdPartyDataStore/UpdateMergerTests.js index 1426d48110..dbeda6f4e5 100644 --- a/services/web/test/unit/src/ThirdPartyDataStore/UpdateMergerTests.js +++ b/services/web/test/unit/src/ThirdPartyDataStore/UpdateMergerTests.js @@ -63,7 +63,7 @@ describe('UpdateMerger :', function () { this.EditorController = { promises: { - deleteEntityWithPath: sinon.stub().resolves(), + deleteEntityWithPath: sinon.stub().resolves(new ObjectId()), upsertDocWithPath: sinon .stub() .resolves({ doc: this.doc, folder: this.folder }), @@ -117,7 +117,7 @@ describe('UpdateMerger :', function () { binary: false, encoding: 'utf-8', }) - await this.UpdateMerger.promises.mergeUpdate( + this.mergeUpdateResult = await this.UpdateMerger.promises.mergeUpdate( this.userId, this.projectId, this.docPath, @@ -145,12 +145,17 @@ describe('UpdateMerger :', function () { it('removes the temp file from disk', function () { expect(this.fsPromises.unlink).to.have.been.calledWith(this.fsPath) }) + + it('returns the entity id and rev', function () { + expect(this.mergeUpdateResult.entityId).to.be.instanceOf(ObjectId) + expect(this.mergeUpdateResult.rev).to.equal(2) + }) }) describe('file updates for a new file ', function () { beforeEach(async function () { this.FileTypeManager.promises.getType.resolves({ binary: true }) - await this.UpdateMerger.promises.mergeUpdate( + this.mergeUpdateResult = await this.UpdateMerger.promises.mergeUpdate( this.userId, this.projectId, this.filePath, @@ -179,6 +184,11 @@ describe('UpdateMerger :', function () { it('removes the temp file from disk', function () { expect(this.fsPromises.unlink).to.have.been.calledWith(this.fsPath) }) + + it('returns the entity id and rev', function () { + expect(this.mergeUpdateResult.entityId).to.be.instanceOf(ObjectId) + expect(this.mergeUpdateResult.rev).to.equal(6) + }) }) describe('doc updates for an existing doc', function () { @@ -187,7 +197,7 @@ describe('UpdateMerger :', function () { binary: false, encoding: 'utf-8', }) - await this.UpdateMerger.promises.mergeUpdate( + this.mergeUpdateResult = await this.UpdateMerger.promises.mergeUpdate( this.userId, this.projectId, this.existingDocPath, @@ -215,12 +225,17 @@ describe('UpdateMerger :', function () { it('removes the temp file from disk', function () { expect(this.fsPromises.unlink).to.have.been.calledWith(this.fsPath) }) + + it('returns the entity id and rev', function () { + expect(this.mergeUpdateResult.entityId).to.be.instanceOf(ObjectId) + expect(this.mergeUpdateResult.rev).to.equal(2) + }) }) describe('file updates for an existing file', function () { beforeEach(async function () { this.FileTypeManager.promises.getType.resolves({ binary: true }) - await this.UpdateMerger.promises.mergeUpdate( + this.mergeUpdateResult = await this.UpdateMerger.promises.mergeUpdate( this.userId, this.projectId, this.existingFilePath, @@ -249,13 +264,18 @@ describe('UpdateMerger :', function () { it('removes the temp file from disk', function () { expect(this.fsPromises.unlink).to.have.been.calledWith(this.fsPath) }) + + it('returns the entity id and rev', function () { + expect(this.mergeUpdateResult.entityId).to.be.instanceOf(ObjectId) + expect(this.mergeUpdateResult.rev).to.equal(6) + }) }) }) describe('file updates for an existing doc', function () { beforeEach(async function () { this.FileTypeManager.promises.getType.resolves({ binary: true }) - await this.UpdateMerger.promises.mergeUpdate( + this.mergeUpdateResult = await this.UpdateMerger.promises.mergeUpdate( this.userId, this.projectId, this.existingDocPath, @@ -284,12 +304,17 @@ describe('UpdateMerger :', function () { it('removes the temp file from disk', function () { expect(this.fsPromises.unlink).to.have.been.calledWith(this.fsPath) }) + + it('returns the entity id and rev', function () { + expect(this.mergeUpdateResult.entityId).to.be.instanceOf(ObjectId) + expect(this.mergeUpdateResult.rev).to.equal(6) + }) }) describe('doc updates for an existing file', function () { beforeEach(async function () { this.FileTypeManager.promises.getType.resolves({ binary: true }) - await this.UpdateMerger.promises.mergeUpdate( + this.mergeUpdateResult = await this.UpdateMerger.promises.mergeUpdate( this.userId, this.projectId, this.existingFilePath, @@ -323,11 +348,16 @@ describe('UpdateMerger :', function () { it('removes the temp file from disk', function () { expect(this.fsPromises.unlink).to.have.been.calledWith(this.fsPath) }) + + it('returns the entity id and rev', function () { + expect(this.mergeUpdateResult.entityId).to.be.instanceOf(ObjectId) + expect(this.mergeUpdateResult.rev).to.equal(6) + }) }) describe('deleteUpdate', function () { beforeEach(async function () { - await this.UpdateMerger.promises.deleteUpdate( + this.deleteUpdateResult = await this.UpdateMerger.promises.deleteUpdate( this.userId, this.projectId, this.docPath, @@ -335,6 +365,10 @@ describe('UpdateMerger :', function () { ) }) + afterEach(function () { + delete this.deleteUpdateResult + }) + it('should delete the entity in the editor controller', function () { expect( this.EditorController.promises.deleteEntityWithPath @@ -345,5 +379,9 @@ describe('UpdateMerger :', function () { this.userId ) }) + + it('returns the entity id', function () { + expect(this.deleteUpdateResult).to.be.instanceOf(ObjectId) + }) }) })