Store/update entity sha after importing from GitHub (#23935)

* Refactor WebApiManager
* Store/update entity sha after importing

GitOrigin-RevId: 604bf3b8010140355c21afca01a54237a301eb92

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,
 }

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,

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,
+      })
     })
   })
 

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)
+    })
   })
 })