Merge pull request #3262 from overleaf/jpa-global-query-normalize-helper

[misc] add a helper for normalizing queries and detecting ObjectIds

GitOrigin-RevId: 4f0ac53219ab5853b5499447334724c6c39c6303

services/web/app/src/Features/Helpers/Mongo.js

@@ -0,0 +1,51 @@
+const OError = require('@overleaf/o-error')
+const { ObjectId } = require('mongodb')
+const { ObjectId: MongooseObjectId } = require('mongoose').mongo
+
+function _getObjectIdInstance(id) {
+  if (typeof id === 'string') {
+    return ObjectId(id)
+  } else if (id instanceof ObjectId) {
+    return id
+  } else if (id instanceof MongooseObjectId) {
+    return ObjectId(id.toString())
+  } else {
+    throw new OError('unexpected object id', { id })
+  }
+}
+
+function normalizeQuery(query) {
+  if (!query) {
+    throw new Error('no query provided')
+  }
+  if (
+    typeof query === 'string' ||
+    query instanceof ObjectId ||
+    query instanceof MongooseObjectId
+  ) {
+    return { _id: _getObjectIdInstance(query) }
+  } else if (typeof query._id === 'string') {
+    query._id = ObjectId(query._id)
+    return query
+  } else {
+    return query
+  }
+}
+
+function normalizeMultiQuery(query) {
+  if (Array.isArray(query)) {
+    return { _id: { $in: query.map(id => _getObjectIdInstance(id)) } }
+  } else {
+    return normalizeQuery(query)
+  }
+}
+
+function isObjectIdInstance(id) {
+  return id instanceof ObjectId || id instanceof MongooseObjectId
+}
+
+module.exports = {
+  isObjectIdInstance,
+  normalizeQuery,
+  normalizeMultiQuery
+}

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

@@ -13,8 +13,8 @@
  * DS207: Consider shorter variations of null checks
  * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
  */
-const { db, ObjectId } = require('../../infrastructure/mongodb')
-const { ObjectId: MongooseObjectId } = require('mongoose').mongo
+const { db } = require('../../infrastructure/mongodb')
+const { normalizeQuery } = require('../Helpers/Mongo')
 const OError = require('@overleaf/o-error')
 const metrics = require('metrics-sharelatex')
 const async = require('async')
@@ -91,7 +91,6 @@ const ProjectGetter = {
   },
 
   getProjectWithoutLock(project_id, projection, callback) {
-    let query
     if (typeof projection === 'function' && callback == null) {
       callback = projection
       projection = {}
@@ -103,19 +102,10 @@ const ProjectGetter = {
       return callback(new Error('projection is not an object'))
     }
 
-    if (typeof project_id === 'string') {
-      query = { _id: ObjectId(project_id) }
-    } else if (project_id instanceof ObjectId) {
-      query = { _id: project_id }
-    } else if (project_id instanceof MongooseObjectId) {
-      query = { _id: ObjectId(project_id.toString()) }
-    } else if (
-      (project_id != null ? project_id.toString().length : undefined) === 24
-    ) {
-      // sometimes mongoose ids are hard to identify, this will catch them
-      query = { _id: ObjectId(project_id.toString()) }
-    } else {
-      const err = new Error('malformed get request')
+    let query
+    try {
+      query = normalizeQuery(project_id)
+    } catch (err) {
       return callback(err)
     }
 

services/web/app/src/Features/User/UserGetter.js

@@ -1,5 +1,4 @@
-const { db, ObjectId } = require('../../infrastructure/mongodb')
-const { ObjectId: MongooseObjectId } = require('mongoose').mongo
+const { db } = require('../../infrastructure/mongodb')
 const metrics = require('metrics-sharelatex')
 const logger = require('logger-sharelatex')
 const { promisifyAll } = require('../../util/promises')
@@ -7,6 +6,7 @@ const { getUserAffiliations } = require('../Institutions/InstitutionsAPI')
 const InstitutionsHelper = require('../Institutions/InstitutionsHelper')
 const Errors = require('../Errors/Errors')
 const Features = require('../../infrastructure/Features')
+const { normalizeQuery, normalizeMultiQuery } = require('../Helpers/Mongo')
 
 const UserGetter = {
   getUser(query, projection, callback) {
@@ -131,7 +131,7 @@ const UserGetter = {
 
   getUsers(query, projection, callback) {
     try {
-      query = normalizeQuery(query)
+      query = normalizeMultiQuery(query)
       db.users.find(query, { projection }).toArray(callback)
     } catch (err) {
       callback(err)
@@ -149,24 +149,6 @@ const UserGetter = {
   }
 }
 
-function normalizeQuery(query) {
-  if (!query) {
-    throw new Error('no query provided')
-  }
-  if (typeof query === 'string') {
-    return { _id: ObjectId(query) }
-  } else if (query instanceof MongooseObjectId) {
-    return { _id: ObjectId(query.toString()) }
-  } else if (query instanceof ObjectId) {
-    return { _id: query }
-  } else if (Array.isArray(query)) {
-    const userIds = query.map(u => ObjectId(u.toString()))
-    return { _id: { $in: userIds } }
-  } else {
-    return query
-  }
-}
-
 var decorateFullEmails = (
   defaultEmail,
   emailsData,

services/web/app/src/Features/User/UserUpdater.js

@@ -1,7 +1,7 @@
 const logger = require('logger-sharelatex')
 const OError = require('@overleaf/o-error')
-const { db, ObjectId } = require('../../infrastructure/mongodb')
-const { ObjectId: MongooseObjectId } = require('mongoose').mongo
+const { db } = require('../../infrastructure/mongodb')
+const { normalizeQuery } = require('../Helpers/Mongo')
 const metrics = require('metrics-sharelatex')
 const async = require('async')
 const { callbackify, promisify } = require('util')
@@ -193,14 +193,11 @@ const UserUpdater = {
     if (callback == null) {
       callback = () => {}
     }
-    if (typeof query === 'string') {
-      query = { _id: ObjectId(query) }
-    } else if (query instanceof ObjectId) {
-      query = { _id: query }
-    } else if (query instanceof MongooseObjectId) {
-      query = { _id: ObjectId(query.toString()) }
-    } else if (typeof query._id === 'string') {
-      query._id = ObjectId(query._id)
+
+    try {
+      query = normalizeQuery(query)
+    } catch (err) {
+      return callback(err)
     }
 
     db.users.updateOne(query, update, callback)

services/web/app/src/Features/UserMembership/UserMembershipViewModel.js

@@ -11,9 +11,8 @@
  * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
  */
 let UserMembershipViewModel
-const { ObjectId } = require('mongodb')
-const { ObjectId: MongooseObjectId } = require('mongoose').mongo
 const UserGetter = require('../User/UserGetter')
+const { isObjectIdInstance } = require('../Helpers/Mongo')
 
 module.exports = UserMembershipViewModel = {
   build(userOrEmail) {
@@ -28,12 +27,7 @@ module.exports = UserMembershipViewModel = {
     if (callback == null) {
       callback = function(error, viewModel) {}
     }
-    if (
-      !(
-        userOrIdOrEmail instanceof ObjectId ||
-        userOrIdOrEmail instanceof MongooseObjectId
-      )
-    ) {
+    if (!isObjectIdInstance(userOrIdOrEmail)) {
       // userOrIdOrEmail is a user or an email and can be parsed by #build
       return callback(null, UserMembershipViewModel.build(userOrIdOrEmail))
     }

services/web/test/acceptance/src/MongoHelper.js

@@ -0,0 +1,184 @@
+const { expect } = require('chai')
+
+const { ObjectId: NativeObjectId } = require('mongodb')
+const { ObjectId: MongooseObjectId } = require('mongoose').mongo
+
+const { User: UserModel } = require('../../../app/src/models/User')
+const { db } = require('../../../app/src/infrastructure/mongodb')
+const {
+  normalizeQuery,
+  normalizeMultiQuery
+} = require('../../../app/src/Features/Helpers/Mongo')
+
+const User = require('./helpers/User').promises
+
+describe('MongoTests', function() {
+  let userIdAsString, userEmail, userIds
+  beforeEach(async function setUpUsers() {
+    // the first user in the db should not match the target user
+    const otherUser = new User()
+    await otherUser.ensureUserExists()
+
+    const user = new User()
+    await user.ensureUserExists()
+    userIdAsString = user.id
+    userEmail = user.email
+
+    // the last user in the db should not match the target user
+    const yetAnotherUser = new User()
+    await yetAnotherUser.ensureUserExists()
+
+    userIds = [otherUser.id, user.id, yetAnotherUser.id]
+  })
+
+  describe('normalizeQuery', function() {
+    async function expectToWork(blob) {
+      const query = normalizeQuery(blob)
+
+      expect(query).to.exist
+      expect(query._id).to.be.instanceof(NativeObjectId)
+      expect(query._id).to.deep.equal(NativeObjectId(userIdAsString))
+
+      const user = await db.users.findOne(query)
+      expect(user).to.exist
+      expect(user.email).to.equal(userEmail)
+    }
+
+    it('should work with the user id as string', async function() {
+      await expectToWork(userIdAsString)
+    })
+
+    it('should work with the user id in an object', async function() {
+      await expectToWork({ _id: userIdAsString })
+    })
+
+    it('should pass back the object with id', function() {
+      const inputQuery = { _id: userIdAsString, other: 1 }
+      const query = normalizeMultiQuery(inputQuery)
+      expect(inputQuery).to.equal(query)
+    })
+
+    describe('with an ObjectId from mongoose', function() {
+      let user
+      beforeEach(async function getUser() {
+        user = await UserModel.findById(userIdAsString).exec()
+        expect(user).to.exist
+        expect(user._id).to.exist
+        expect(user.email).to.equal(userEmail)
+      })
+
+      it('should have a mongoose ObjectId', function() {
+        expect(user._id).to.be.instanceof(MongooseObjectId)
+      })
+
+      it('should work with the users _id field', async function() {
+        await expectToWork(user._id)
+      })
+    })
+
+    describe('with an ObjectId from the native driver', function() {
+      let user
+      beforeEach(async function getUser() {
+        user = await db.users.findOne({ _id: NativeObjectId(userIdAsString) })
+        expect(user).to.exist
+        expect(user._id).to.exist
+        expect(user.email).to.equal(userEmail)
+      })
+
+      it('should have a native ObjectId', function() {
+        expect(user._id).to.be.instanceof(NativeObjectId)
+      })
+
+      it('should work with the users _id field', async function() {
+        await expectToWork(user._id)
+      })
+    })
+  })
+
+  describe('normalizeMultiQuery', function() {
+    let ghost
+    beforeEach(async function addGhost() {
+      // add a user which is not part of the initial three users
+      ghost = new User()
+      ghost.emails[0].email = ghost.email = 'ghost@ghost.com'
+      await ghost.ensureUserExists()
+    })
+
+    async function expectToFindTheThreeUsers(query) {
+      const users = await db.users.find(query).toArray()
+
+      expect(users).to.have.length(3)
+      expect(users.map(user => user._id.toString()).sort()).to.deep.equal(
+        userIds.sort()
+      )
+    }
+
+    describe('with an array as query', function() {
+      function expectInQueryWithNativeObjectIds(query) {
+        expect(query).to.exist
+        expect(query._id).to.exist
+        expect(query._id.$in).to.exist
+        expect(
+          query._id.$in.map(id => id instanceof NativeObjectId)
+        ).to.deep.equal([true, true, true])
+      }
+
+      it('should transform all strings to native ObjectIds', function() {
+        const query = normalizeMultiQuery(userIds)
+        expectInQueryWithNativeObjectIds(query)
+      })
+      it('should transform all Mongoose ObjectIds to native ObjectIds', function() {
+        const query = normalizeMultiQuery(userIds.map(MongooseObjectId))
+        expectInQueryWithNativeObjectIds(query)
+      })
+      it('should leave all native Objects as native ObjectIds', function() {
+        const query = normalizeMultiQuery(userIds.map(NativeObjectId))
+        expectInQueryWithNativeObjectIds(query)
+      })
+
+      it('should find the three users from string ids', async function() {
+        const query = normalizeMultiQuery(userIds)
+        await expectToFindTheThreeUsers(query)
+      })
+      it('should find the three users from Mongoose ObjectIds', async function() {
+        const query = normalizeMultiQuery(userIds.map(MongooseObjectId))
+        await expectToFindTheThreeUsers(query)
+      })
+      it('should find the three users from native ObjectIds', async function() {
+        const query = normalizeMultiQuery(userIds.map(NativeObjectId))
+        await expectToFindTheThreeUsers(query)
+      })
+    })
+
+    describe('with an object as query', function() {
+      beforeEach(async function addHiddenFlag() {
+        // add a mongo field that does not exist on the other users
+        await ghost.mongoUpdate({ $set: { hidden: 1 } })
+      })
+
+      it('should pass through the query', function() {
+        const inputQuery = { complex: 1 }
+        const query = normalizeMultiQuery(inputQuery)
+        expect(inputQuery).to.equal(query)
+      })
+
+      describe('when searching for hidden users', function() {
+        it('should match the ghost only', async function() {
+          const query = normalizeMultiQuery({ hidden: 1 })
+
+          const users = await db.users.find(query).toArray()
+          expect(users).to.have.length(1)
+          expect(users[0]._id.toString()).to.equal(ghost.id)
+        })
+      })
+
+      describe('when searching for non hidden users', function() {
+        it('should find the three users', async function() {
+          const query = normalizeMultiQuery({ hidden: { $exists: false } })
+
+          await expectToFindTheThreeUsers(query)
+        })
+      })
+    })
+  })
+})

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

@@ -18,6 +18,7 @@ const { expect } = chai
 const modulePath = '../../../../app/src/Features/Project/ProjectGetter.js'
 const SandboxedModule = require('sandboxed-module')
 const { ObjectId } = require('mongodb')
+const { normalizeQuery } = require('../../../../app/src/Features/Helpers/Mongo')
 const { assert } = require('chai')
 
 describe('ProjectGetter', function() {
@@ -59,6 +60,7 @@ describe('ProjectGetter', function() {
             return project_id
           }
         },
+        '../Helpers/Mongo': { normalizeQuery },
         'logger-sharelatex': {
           err() {},
           log() {}

services/web/test/unit/src/User/UserGetterTests.js

@@ -10,6 +10,10 @@ const modulePath = path.join(
 )
 const { expect } = require('chai')
 const Errors = require('../../../../app/src/Features/Errors/Errors')
+const {
+  normalizeQuery,
+  normalizeMultiQuery
+} = require('../../../../app/src/Features/Helpers/Mongo')
 
 describe('UserGetter', function() {
   beforeEach(function() {
@@ -45,6 +49,7 @@ describe('UserGetter', function() {
         console: console
       },
       requires: {
+        '../Helpers/Mongo': { normalizeQuery, normalizeMultiQuery },
         'logger-sharelatex': {
           log() {}
         },
@@ -66,7 +71,7 @@ describe('UserGetter', function() {
 
   describe('getUser', function() {
     it('should get user', function(done) {
-      const query = { _id: 'foo' }
+      const query = { _id: '000000000000000000000000' }
       const projection = { email: 1 }
       this.UserGetter.getUser(query, projection, (error, user) => {
         expect(error).to.not.exist

services/web/test/unit/src/User/UserUpdaterTests.js

@@ -8,6 +8,7 @@ const modulePath = path.join(
 )
 const tk = require('timekeeper')
 const { expect } = require('chai')
+const { normalizeQuery } = require('../../../../app/src/Features/Helpers/Mongo')
 
 describe('UserUpdater', function() {
   beforeEach(function() {
@@ -49,6 +50,7 @@ describe('UserUpdater', function() {
         console: console
       },
       requires: {
+        '../Helpers/Mongo': { normalizeQuery },
         'logger-sharelatex': this.logger,
         '../../infrastructure/mongodb': this.mongodb,
         'metrics-sharelatex': {

services/web/test/unit/src/UserMembership/UserMembershipViewModelTests.js

@@ -20,6 +20,10 @@ const { ObjectId } = require('mongodb')
 const modulePath =
   '../../../../app/src/Features/UserMembership/UserMembershipViewModel'
 const SandboxedModule = require('sandboxed-module')
+const {
+  isObjectIdInstance,
+  normalizeQuery
+} = require('../../../../app/src/Features/Helpers/Mongo')
 
 describe('UserMembershipViewModel', function() {
   beforeEach(function() {
@@ -30,6 +34,7 @@ describe('UserMembershipViewModel', function() {
       },
       requires: {
         mongodb: { ObjectId },
+        '../Helpers/Mongo': { isObjectIdInstance, normalizeQuery },
         '../User/UserGetter': this.UserGetter
       }
     })