Fix ObjectId/String comparison in ensureRootDocumentIsValid (#4229)

GitOrigin-RevId: 8af2a74cd24cb5bfdfdcd1a25c16b94a66fa0843

services/web/app/src/Features/Project/ProjectEntityHandler.js

@@ -151,6 +151,11 @@ const ProjectEntityHandler = {
     DocstoreManager.getDoc(projectId, docId, options, callback)
   },
 
+  /**
+   * @param {ObjectID | string} projectId
+   * @param {ObjectID | string} docId
+   * @param {Function} callback
+   */
   getDocPathByProjectIdAndDocId(projectId, docId, callback) {
     ProjectGetter.getProjectWithoutDocLines(projectId, (err, project) => {
       if (err != null) {
@@ -159,39 +164,55 @@ const ProjectEntityHandler = {
       if (project == null) {
         return callback(new Errors.NotFoundError('no project'))
       }
-      function recursivelyFindDocInFolder(basePath, docId, folder) {
-        const docInCurrentFolder = (folder.docs || []).find(
-          currentDoc => currentDoc._id.toString() === docId.toString()
-        )
-        if (docInCurrentFolder != null) {
-          return path.join(basePath, docInCurrentFolder.name)
-        } else {
-          let docPath, childFolder
-          for (childFolder of folder.folders || []) {
-            docPath = recursivelyFindDocInFolder(
-              path.join(basePath, childFolder.name),
-              docId,
-              childFolder
-            )
-            if (docPath != null) {
-              return docPath
-            }
+      ProjectEntityHandler.getDocPathFromProjectByDocId(
+        project,
+        docId,
+        (err, docPath) => {
+          if (err) return callback(Errors.OError.tag(err))
+          if (docPath == null) {
+            return callback(new Errors.NotFoundError('no doc'))
           }
-          return null
+          callback(null, docPath)
         }
-      }
-      const docPath = recursivelyFindDocInFolder(
-        '/',
-        docId,
-        project.rootFolder[0]
       )
-      if (docPath == null) {
-        return callback(new Errors.NotFoundError('no doc'))
-      }
-      callback(null, docPath)
     })
   },
 
+  /**
+   * @param {Project} project
+   * @param {ObjectID | string} docId
+   * @param {Function} callback
+   */
+  getDocPathFromProjectByDocId(project, docId, callback) {
+    function recursivelyFindDocInFolder(basePath, docId, folder) {
+      const docInCurrentFolder = (folder.docs || []).find(
+        currentDoc => currentDoc._id.toString() === docId.toString()
+      )
+      if (docInCurrentFolder != null) {
+        return path.join(basePath, docInCurrentFolder.name)
+      } else {
+        let docPath, childFolder
+        for (childFolder of folder.folders || []) {
+          docPath = recursivelyFindDocInFolder(
+            path.join(basePath, childFolder.name),
+            docId,
+            childFolder
+          )
+          if (docPath != null) {
+            return docPath
+          }
+        }
+        return null
+      }
+    }
+    const docPath = recursivelyFindDocInFolder(
+      '/',
+      docId,
+      project.rootFolder[0]
+    )
+    callback(null, docPath)
+  },
+
   _getAllFolders(projectId, callback) {
     ProjectGetter.getProjectWithoutDocLines(projectId, (err, project) => {
       if (err != null) {

services/web/app/src/Features/Project/ProjectRootDocManager.js

@@ -205,13 +205,13 @@ module.exports = ProjectRootDocManager = {
     )
   },
 
+  /**
+   * @param {ObjectId | string} project_id
+   * @param {Function} callback
+   */
   ensureRootDocumentIsValid(project_id, callback) {
-    if (callback == null) {
-      callback = function (error) {}
-    }
-    return ProjectGetter.getProject(
+    ProjectGetter.getProjectWithoutDocLines(
       project_id,
-      { rootDoc_id: 1 },
       function (error, project) {
         if (error != null) {
           return callback(error)
@@ -221,29 +221,17 @@ module.exports = ProjectRootDocManager = {
         }
 
         if (project.rootDoc_id != null) {
-          return ProjectEntityHandler.getAllDocPathsFromProjectById(
-            project_id,
-            function (error, docPaths) {
-              if (error != null) {
-                return callback(error)
-              }
-              let rootDocValid = false
-              for (const doc_id in docPaths) {
-                const _path = docPaths[doc_id]
-                if (doc_id === project.rootDoc_id) {
-                  rootDocValid = true
-                }
-              }
-              if (rootDocValid) {
-                return callback()
-              } else {
-                return ProjectEntityUpdateHandler.unsetRootDoc(project_id, () =>
-                  ProjectRootDocManager.setRootDocAutomatically(
-                    project_id,
-                    callback
-                  )
+          ProjectEntityHandler.getDocPathFromProjectByDocId(
+            project,
+            project.rootDoc_id,
+            (err, docPath) => {
+              if (docPath) return callback()
+              ProjectEntityUpdateHandler.unsetRootDoc(project_id, () =>
+                ProjectRootDocManager.setRootDocAutomatically(
+                  project_id,
+                  callback
                 )
-              }
+              )
             }
           )
         } else {

services/web/test/unit/src/Project/ProjectRootDocManagerTests.js

@@ -12,6 +12,7 @@
  * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
  */
 const { expect } = require('chai')
+const { ObjectId } = require('mongodb')
 const sinon = require('sinon')
 const modulePath =
   '../../../../app/src/Features/Project/ProjectRootDocManager.js'
@@ -20,12 +21,15 @@ const SandboxedModule = require('sandboxed-module')
 describe('ProjectRootDocManager', function () {
   beforeEach(function () {
     this.project_id = 'project-123'
-    this.docPaths = {
-      'doc-id-1': '/chapter1.tex',
-      'doc-id-2': '/main.tex',
-      'doc-id-3': '/nested/chapter1a.tex',
-      'doc-id-4': '/nested/chapter1b.tex',
-    }
+    this.docPaths = {}
+    this.docId1 = new ObjectId()
+    this.docId2 = new ObjectId()
+    this.docId3 = new ObjectId()
+    this.docId4 = new ObjectId()
+    this.docPaths[this.docId1] = '/chapter1.tex'
+    this.docPaths[this.docId2] = '/main.tex'
+    this.docPaths[this.docId3] = '/nested/chapter1a.tex'
+    this.docPaths[this.docId4] = '/nested/chapter1b.tex'
     this.sl_req_id = 'sl-req-id-123'
     this.callback = sinon.stub()
     this.globbyFiles = ['a.tex', 'b.tex', 'main.tex']
@@ -60,7 +64,7 @@ describe('ProjectRootDocManager', function () {
       beforeEach(function (done) {
         this.docs = {
           '/chapter1.tex': {
-            _id: 'doc-id-1',
+            _id: this.docId1,
             lines: [
               'something else',
               '\\begin{document}',
@@ -69,7 +73,7 @@ describe('ProjectRootDocManager', function () {
             ],
           },
           '/main.tex': {
-            _id: 'doc-id-2',
+            _id: this.docId2,
             lines: [
               'different line',
               '\\documentclass{article}',
@@ -77,11 +81,11 @@ describe('ProjectRootDocManager', function () {
             ],
           },
           '/nested/chapter1a.tex': {
-            _id: 'doc-id-3',
+            _id: this.docId3,
             lines: ['Hello world'],
           },
           '/nested/chapter1b.tex': {
-            _id: 'doc-id-4',
+            _id: this.docId4,
             lines: ['Hello world'],
           },
         }
@@ -102,7 +106,7 @@ describe('ProjectRootDocManager', function () {
 
       it('should set the root doc to the doc containing a documentclass', function () {
         return this.ProjectEntityUpdateHandler.setRootDoc
-          .calledWith(this.project_id, 'doc-id-2')
+          .calledWith(this.project_id, this.docId2)
           .should.equal(true)
       })
     })
@@ -111,11 +115,11 @@ describe('ProjectRootDocManager', function () {
       beforeEach(function (done) {
         this.docs = {
           '/chapter1.tex': {
-            _id: 'doc-id-1',
+            _id: this.docId1,
             lines: ['\\begin{document}', 'Hello world', '\\end{document}'],
           },
           '/main.Rtex': {
-            _id: 'doc-id-2',
+            _id: this.docId2,
             lines: ['\\documentclass{article}', '\\input{chapter1}'],
           },
         }
@@ -130,7 +134,7 @@ describe('ProjectRootDocManager', function () {
 
       it('should set the root doc to the doc containing a documentclass', function () {
         return this.ProjectEntityUpdateHandler.setRootDoc
-          .calledWith(this.project_id, 'doc-id-2')
+          .calledWith(this.project_id, this.docId2)
           .should.equal(true)
       })
     })
@@ -139,11 +143,11 @@ describe('ProjectRootDocManager', function () {
       beforeEach(function (done) {
         this.docs = {
           '/chapter1.tex': {
-            _id: 'doc-id-1',
+            _id: this.docId1,
             lines: ['\\begin{document}', 'Hello world', '\\end{document}'],
           },
           '/style.bst': {
-            _id: 'doc-id-2',
+            _id: this.docId2,
             lines: ['%Example: \\documentclass{article}'],
           },
         }
@@ -346,12 +350,6 @@ describe('ProjectRootDocManager', function () {
   describe('setRootDocFromName', function () {
     describe('when there is a suitable root doc', function () {
       beforeEach(function (done) {
-        this.docPaths = {
-          'doc-id-1': '/chapter1.tex',
-          'doc-id-2': '/main.tex',
-          'doc-id-3': '/nested/chapter1a.tex',
-          'doc-id-4': '/nested/chapter1b.tex',
-        }
         this.ProjectEntityHandler.getAllDocPathsFromProjectById = sinon
           .stub()
           .callsArgWith(1, null, this.docPaths)
@@ -373,7 +371,7 @@ describe('ProjectRootDocManager', function () {
 
       it('should set the root doc to main.tex', function () {
         return this.ProjectEntityUpdateHandler.setRootDoc
-          .calledWith(this.project_id, 'doc-id-2')
+          .calledWith(this.project_id, this.docId2.toString())
           .should.equal(true)
       })
     })
@@ -401,7 +399,7 @@ describe('ProjectRootDocManager', function () {
 
       it('should set the root doc to main.tex', function () {
         return this.ProjectEntityUpdateHandler.setRootDoc
-          .calledWith(this.project_id, 'doc-id-2')
+          .calledWith(this.project_id, this.docId2.toString())
           .should.equal(true)
       })
     })
@@ -429,7 +427,7 @@ describe('ProjectRootDocManager', function () {
 
       it('should set the root doc using the basename', function () {
         return this.ProjectEntityUpdateHandler.setRootDoc
-          .calledWith(this.project_id, 'doc-id-3')
+          .calledWith(this.project_id, this.docId3.toString())
           .should.equal(true)
       })
     })
@@ -457,7 +455,7 @@ describe('ProjectRootDocManager', function () {
 
       it('should set the root doc to main.tex', function () {
         return this.ProjectEntityUpdateHandler.setRootDoc
-          .calledWith(this.project_id, 'doc-id-2')
+          .calledWith(this.project_id, this.docId2.toString())
           .should.equal(true)
       })
     })
@@ -498,7 +496,7 @@ describe('ProjectRootDocManager', function () {
 
     describe('when the root doc is set', function () {
       beforeEach(function () {
-        this.project.rootDoc_id = 'root-doc-id'
+        this.project.rootDoc_id = this.docId2
         return this.ProjectRootDocManager.ensureRootDocumentIsSet(
           this.project_id,
           this.callback
@@ -574,29 +572,32 @@ describe('ProjectRootDocManager', function () {
       this.ProjectGetter.getProject = sinon
         .stub()
         .callsArgWith(2, null, this.project)
+      this.ProjectGetter.getProjectWithoutDocLines = sinon
+        .stub()
+        .callsArgWith(1, null, this.project)
       this.ProjectEntityUpdateHandler.setRootDoc = sinon.stub().yields()
       this.ProjectEntityUpdateHandler.unsetRootDoc = sinon.stub().yields()
-      this.ProjectEntityHandler.getAllDocPathsFromProjectById = sinon
-        .stub()
-        .callsArgWith(1, null, this.docPaths)
-      return (this.ProjectRootDocManager.setRootDocAutomatically = sinon
+      this.ProjectRootDocManager.setRootDocAutomatically = sinon
         .stub()
-        .callsArgWith(1, null))
+        .callsArgWith(1, null)
     })
 
     describe('when the root doc is set', function () {
       describe('when the root doc is valid', function () {
         beforeEach(function () {
-          this.project.rootDoc_id = 'doc-id-2'
+          this.project.rootDoc_id = this.docId2
+          this.ProjectEntityHandler.getDocPathFromProjectByDocId = sinon
+            .stub()
+            .callsArgWith(2, null, this.docPaths[this.docId2])
           return this.ProjectRootDocManager.ensureRootDocumentIsValid(
             this.project_id,
             this.callback
           )
         })
 
-        it('should find the project fetching only the rootDoc_id field', function () {
-          return this.ProjectGetter.getProject
-            .calledWith(this.project_id, { rootDoc_id: 1 })
+        it('should find the project without doc lines', function () {
+          return this.ProjectGetter.getProjectWithoutDocLines
+            .calledWith(this.project_id)
             .should.equal(true)
         })
 
@@ -613,16 +614,19 @@ describe('ProjectRootDocManager', function () {
 
       describe('when the root doc is not valid', function () {
         beforeEach(function () {
-          this.project.rootDoc_id = 'bogus-doc-id'
+          this.project.rootDoc_id = new ObjectId()
+          this.ProjectEntityHandler.getDocPathFromProjectByDocId = sinon
+            .stub()
+            .callsArgWith(2, null, null)
           return this.ProjectRootDocManager.ensureRootDocumentIsValid(
             this.project_id,
             this.callback
           )
         })
 
-        it('should find the project fetching only the rootDoc_id field', function () {
-          return this.ProjectGetter.getProject
-            .calledWith(this.project_id, { rootDoc_id: 1 })
+        it('should find the project without doc lines', function () {
+          return this.ProjectGetter.getProjectWithoutDocLines
+            .calledWith(this.project_id)
             .should.equal(true)
         })
 
@@ -646,15 +650,15 @@ describe('ProjectRootDocManager', function () {
 
     describe('when the root doc is not set', function () {
       beforeEach(function () {
-        return this.ProjectRootDocManager.ensureRootDocumentIsSet(
+        return this.ProjectRootDocManager.ensureRootDocumentIsValid(
           this.project_id,
           this.callback
         )
       })
 
-      it('should find the project fetching only the rootDoc_id fiel', function () {
-        return this.ProjectGetter.getProject
-          .calledWith(this.project_id, { rootDoc_id: 1 })
+      it('should find the project without doc lines', function () {
+        return this.ProjectGetter.getProjectWithoutDocLines
+          .calledWith(this.project_id)
           .should.equal(true)
       })
 
@@ -671,8 +675,10 @@ describe('ProjectRootDocManager', function () {
 
     describe('when the project does not exist', function () {
       beforeEach(function () {
-        this.ProjectGetter.getProject = sinon.stub().callsArgWith(2, null, null)
-        return this.ProjectRootDocManager.ensureRootDocumentIsSet(
+        this.ProjectGetter.getProjectWithoutDocLines = sinon
+          .stub()
+          .callsArgWith(1, null, null)
+        return this.ProjectRootDocManager.ensureRootDocumentIsValid(
           this.project_id,
           this.callback
         )