From 79bdc6074357181a152ef61f4daf05574e2b88d0 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Tue, 6 Oct 2020 12:31:34 +0200 Subject: [PATCH] Merge pull request #3262 from overleaf/jpa-global-query-normalize-helper [misc] add a helper for normalizing queries and detecting ObjectIds GitOrigin-RevId: 4f0ac53219ab5853b5499447334724c6c39c6303 --- .../web/app/src/Features/Helpers/Mongo.js | 51 +++++ .../app/src/Features/Project/ProjectGetter.js | 22 +-- .../web/app/src/Features/User/UserGetter.js | 24 +-- .../web/app/src/Features/User/UserUpdater.js | 17 +- .../UserMembership/UserMembershipViewModel.js | 10 +- .../web/test/acceptance/src/MongoHelper.js | 184 ++++++++++++++++++ .../unit/src/Project/ProjectGetterTests.js | 2 + .../web/test/unit/src/User/UserGetterTests.js | 7 +- .../test/unit/src/User/UserUpdaterTests.js | 2 + .../UserMembershipViewModelTests.js | 5 + 10 files changed, 268 insertions(+), 56 deletions(-) create mode 100644 services/web/app/src/Features/Helpers/Mongo.js create mode 100644 services/web/test/acceptance/src/MongoHelper.js diff --git a/services/web/app/src/Features/Helpers/Mongo.js b/services/web/app/src/Features/Helpers/Mongo.js new file mode 100644 index 0000000000..07eae214c8 --- /dev/null +++ b/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 +} diff --git a/services/web/app/src/Features/Project/ProjectGetter.js b/services/web/app/src/Features/Project/ProjectGetter.js index b5655a8ef5..75244a1b5d 100644 --- a/services/web/app/src/Features/Project/ProjectGetter.js +++ b/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) } diff --git a/services/web/app/src/Features/User/UserGetter.js b/services/web/app/src/Features/User/UserGetter.js index 50c9778e1e..0a0575752b 100644 --- a/services/web/app/src/Features/User/UserGetter.js +++ b/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, diff --git a/services/web/app/src/Features/User/UserUpdater.js b/services/web/app/src/Features/User/UserUpdater.js index e99f639f33..acd850a920 100644 --- a/services/web/app/src/Features/User/UserUpdater.js +++ b/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) diff --git a/services/web/app/src/Features/UserMembership/UserMembershipViewModel.js b/services/web/app/src/Features/UserMembership/UserMembershipViewModel.js index f098613b08..996361cf0a 100644 --- a/services/web/app/src/Features/UserMembership/UserMembershipViewModel.js +++ b/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)) } diff --git a/services/web/test/acceptance/src/MongoHelper.js b/services/web/test/acceptance/src/MongoHelper.js new file mode 100644 index 0000000000..0491d3befd --- /dev/null +++ b/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) + }) + }) + }) + }) +}) diff --git a/services/web/test/unit/src/Project/ProjectGetterTests.js b/services/web/test/unit/src/Project/ProjectGetterTests.js index c33b35f76b..43f0be083d 100644 --- a/services/web/test/unit/src/Project/ProjectGetterTests.js +++ b/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() {} diff --git a/services/web/test/unit/src/User/UserGetterTests.js b/services/web/test/unit/src/User/UserGetterTests.js index f0ed4e1f1c..4e37a72c71 100644 --- a/services/web/test/unit/src/User/UserGetterTests.js +++ b/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 diff --git a/services/web/test/unit/src/User/UserUpdaterTests.js b/services/web/test/unit/src/User/UserUpdaterTests.js index 0d867d442b..4a2e313f53 100644 --- a/services/web/test/unit/src/User/UserUpdaterTests.js +++ b/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': { diff --git a/services/web/test/unit/src/UserMembership/UserMembershipViewModelTests.js b/services/web/test/unit/src/UserMembership/UserMembershipViewModelTests.js index 4fc46733b6..96c04df0cb 100644 --- a/services/web/test/unit/src/UserMembership/UserMembershipViewModelTests.js +++ b/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 } })