diff --git a/src/controller/org.controller/org.controller.js b/src/controller/org.controller/org.controller.js index 16d4a67ca..662aaa6e8 100644 --- a/src/controller/org.controller/org.controller.js +++ b/src/controller/org.controller/org.controller.js @@ -17,10 +17,8 @@ const validateUUID = require('uuid').validate */ async function getOrgs (req, res, next) { try { - const session = await mongoose.startSession() const repo = req.ctx.repositories.getBaseOrgRepository() const CONSTANTS = getConstants() - let returnValue // temporary measure to allow tests to work after fixing #920 // tests required changing the global limit to force pagination @@ -32,11 +30,7 @@ async function getOrgs (req, res, next) { options.sort = { short_name: 'asc' } options.page = req.ctx.query.page ? parseInt(req.ctx.query.page) : CONSTANTS.PAGINATOR_PAGE // if 'page' query parameter is not defined, set 'page' to the default page value - try { - returnValue = await repo.getAllOrgs({ ...options, session }, true) - } finally { - await session.endSession() - } + const returnValue = await repo.getAllOrgs({ ...options }, true) logger.info({ uuid: req.ctx.uuid, message: 'The orgs were sent to the user.' }) return res.status(200).json(returnValue) @@ -58,7 +52,6 @@ async function getOrgs (req, res, next) { */ async function getOrg (req, res, next) { try { - const session = await mongoose.startSession() const repo = req.ctx.repositories.getBaseOrgRepository() const requesterOrgShortName = req.ctx.org const identifier = req.ctx.params.identifier @@ -67,26 +60,21 @@ async function getOrg (req, res, next) { let returnValue try { - session.startTransaction() - const requesterOrg = await repo.findOneByShortName(requesterOrgShortName, { session }, returnLegacyFormat) + const requesterOrg = await repo.findOneByShortName(requesterOrgShortName, {}, returnLegacyFormat) const requesterOrgIdentifier = identifierIsUUID ? requesterOrg.UUID : requesterOrgShortName - const isSecretariat = await repo.isSecretariat(requesterOrg, { session }, returnLegacyFormat) + const isSecretariat = await repo.isSecretariat(requesterOrg, {}, returnLegacyFormat) if (requesterOrgIdentifier !== identifier && !isSecretariat) { logger.info({ uuid: req.ctx.uuid, message: identifier + ' organization can only be viewed by the users of the same organization or the Secretariat.' }) return res.status(403).json(error.notSameOrgOrSecretariat()) } - returnValue = await repo.getOrg(identifier, identifierIsUUID, { session }, returnLegacyFormat) + returnValue = await repo.getOrg(identifier, identifierIsUUID, {}, returnLegacyFormat) } catch (error) { - await session.abortTransaction() // Handle the specific error thrown by BaseOrgRepository.createOrg if (error.message && error.message.includes('Unknown Org type requested')) { return res.status(400).json({ message: error.message }) } - throw error - } finally { - await session.endSession() } if (!returnValue) { // an empty result can only happen if the requestor is the Secretariat logger.info({ uuid: req.ctx.uuid, message: identifier + ' organization does not exist.' }) @@ -336,7 +324,7 @@ async function updateOrg (req, res, next) { const shortNameUrlParameter = req.ctx.params.shortname const orgRepository = req.ctx.repositories.getBaseOrgRepository() - const session = await mongoose.startSession() + const session = await mongoose.startSession({ causalConsistency: false }) let responseMessage // Get the query parameters as JSON // These are validated by the middleware in org/index.js diff --git a/src/controller/registry-org.controller/registry-org.controller.js b/src/controller/registry-org.controller/registry-org.controller.js index 1f48131d0..7623505c0 100644 --- a/src/controller/registry-org.controller/registry-org.controller.js +++ b/src/controller/registry-org.controller/registry-org.controller.js @@ -71,7 +71,6 @@ async function getAllOrgs (req, res, next) { */ async function getOrg (req, res, next) { try { - const session = await mongoose.startSession() const repo = req.ctx.repositories.getBaseOrgRepository() const conversationRepo = req.ctx.repositories.getConversationRepository() // User passed in parameter to filter for @@ -81,32 +80,28 @@ async function getOrg (req, res, next) { let returnValue try { - session.startTransaction() - const requesterOrg = await repo.findOneByShortName(requesterOrgShortName, { session }) + const requesterOrg = await repo.findOneByShortName(requesterOrgShortName) const requesterOrgIdentifier = identifierIsUUID ? requesterOrg.UUID : requesterOrgShortName - const isSecretariat = await repo.isSecretariat(requesterOrg, { session }) + const isSecretariat = await repo.isSecretariat(requesterOrg) if (requesterOrgIdentifier !== identifier && !isSecretariat) { logger.info({ uuid: req.ctx.uuid, message: identifier + ' organization can only be viewed by the users of the same organization or the Secretariat.' }) return res.status(403).json(error.notSameOrgOrSecretariat()) } - returnValue = await repo.getOrg(identifier, identifierIsUUID, { session }) + returnValue = await repo.getOrg(identifier, identifierIsUUID) if (returnValue) { // fetch conversation - const conversation = await conversationRepo.getAllByTargetUUID(returnValue.UUID, isSecretariat, { session }) + const conversation = await conversationRepo.getAllByTargetUUID(returnValue.UUID, isSecretariat) returnValue.conversation = conversation?.length ? _.map(conversation, c => _.omit(c, ['__v', '_id', 'UUID', 'previous_conversation_uuid', 'next_conversation_uuid', 'target_uuid', 'visibility'])) : undefined } } catch (error) { - await session.abortTransaction() // Handle the specific error thrown by BaseOrgRepository.createOrg if (error.message && error.message.includes('Unknown Org type requested')) { return res.status(400).json({ message: error.message }) } throw error - } finally { - await session.endSession() } if (!returnValue) { // an empty result can only happen if the requestor is the Secretariat logger.info({ uuid: req.ctx.uuid, message: identifier + ' organization does not exist.' }) @@ -134,7 +129,7 @@ async function getOrg (req, res, next) { */ async function createOrg (req, res, next) { try { - const session = await mongoose.startSession() + const session = await mongoose.startSession({ causalConsistency: false }) const repo = req.ctx.repositories.getBaseOrgRepository() const body = req.ctx.body const isSecretariat = await repo.isSecretariatByShortName(req.ctx.org, { session }) @@ -232,7 +227,7 @@ async function createOrg (req, res, next) { */ async function updateOrg (req, res, next) { try { - const session = await mongoose.startSession() + const session = await mongoose.startSession({ causalConsistency: false }) const shortName = req.ctx.params.shortname const repo = req.ctx.repositories.getBaseOrgRepository() const userRepo = req.ctx.repositories.getBaseUserRepository() @@ -318,15 +313,18 @@ async function updateOrg (req, res, next) { } } + // Update Org full will cause a write to the Conversations collection, to avoid a read-after-write issue, we need to get the previous conversation data first + const previousConversation = await conversationRepo.getAllByTargetUUID(await repo.getOrgUUID(shortName, { session }), isSecretariat, { session }) || [] + updatedOrg = await repo.updateOrgFull(shortName, req.ctx.body, { session }, false, requestingUser.UUID, isAdmin, isSecretariat) jointApprovalRequired = _.get(updatedOrg, 'joint_approval_required', false) _.unset(updatedOrg, 'joint_approval_required') - - await session.commitTransaction() - session.startTransaction() - // Checking for existing Conversations - const existingConversations = await conversationRepo.getAllByTargetUUID(updatedOrg.UUID, isSecretariat, { session }) || [] - updatedOrg.conversation = existingConversations.map(c => _.omit(c, ['__v', '_id', 'previous_conversation_uuid', 'next_conversation_uuid'])) + // append previous conversations to any conversations that are in the org already + const currentConversations = Array.isArray(updatedOrg?.conversation) ? updatedOrg.conversation : [] + const prevConversations = Array.isArray(previousConversation) ? previousConversation : [] + if (updatedOrg) { + updatedOrg.conversation = [...currentConversations, ...prevConversations].map(c => _.omit(c, ['__v', '_id', 'previous_conversation_uuid', 'next_conversation_uuid'])) + } await session.commitTransaction() } catch (updateErr) { @@ -387,7 +385,7 @@ async function updateOrg (req, res, next) { */ async function deleteOrg (req, res, next) { try { - const session = await mongoose.startSession() + const session = await mongoose.startSession({ causalConsistency: false }) const repo = req.ctx.repositories.getBaseOrgRepository() const shortName = req.ctx.params.identifier @@ -500,7 +498,7 @@ async function getUsers (req, res, next) { * Called by POST /api/registryOrg/:shortname/user */ async function createUserByOrg (req, res, next) { - const session = await mongoose.startSession() + const session = await mongoose.startSession({ causalConsistency: false }) try { const body = req.ctx.body const userRepo = req.ctx.repositories.getBaseUserRepository() diff --git a/src/controller/registry-user.controller/registry-user.controller.js b/src/controller/registry-user.controller/registry-user.controller.js index fdd11eaf7..658a24ed2 100644 --- a/src/controller/registry-user.controller/registry-user.controller.js +++ b/src/controller/registry-user.controller/registry-user.controller.js @@ -21,9 +21,7 @@ const _ = require('lodash') async function getAllUsers (req, res, next) { try { const CONSTANTS = getConstants() - const session = await mongoose.startSession() const repo = req.ctx.repositories.getBaseUserRepository() - let returnValue // temporary measure to allow tests to work after fixing #920 // tests required changing the global limit to force pagination @@ -35,36 +33,32 @@ async function getAllUsers (req, res, next) { options.sort = { short_name: 'asc' } options.page = req.ctx.query.page ? parseInt(req.ctx.query.page) : CONSTANTS.PAGINATOR_PAGE // if 'page' query parameter is not defined, set 'page' to the default page value - try { - returnValue = await repo.getAllUsers(options) - // Hydrate roles - const orgRepo = req.ctx.repositories.getBaseOrgRepository() - const distinctOrgUUIDs = [...new Set(returnValue.users.map(u => u.org_UUID))] - - // Fetch all relevant orgs in one go (or in parallel) if possible, but map is easy for now - // Since we don't have a "getManyOrgsByUUID", we might need to do it one by one or improve repository - // For now, let's iterate and fetch. It's not optimal but safe given repo limitations. - // Optimization: We can build a map of orgUUID -> orgObject - const orgMap = {} - for (const uuid of distinctOrgUUIDs) { - // We need the org content to get admins - const org = await orgRepo.findOneByUUID(uuid) - if (org) { - orgMap[uuid] = org - } + const returnValue = await repo.getAllUsers(options) + // Hydrate roles + const orgRepo = req.ctx.repositories.getBaseOrgRepository() + const distinctOrgUUIDs = [...new Set(returnValue.users.map(u => u.org_UUID))] + + // Fetch all relevant orgs in one go (or in parallel) if possible, but map is easy for now + // Since we don't have a "getManyOrgsByUUID", we might need to do it one by one or improve repository + // For now, let's iterate and fetch. It's not optimal but safe given repo limitations. + // Optimization: We can build a map of orgUUID -> orgObject + const orgMap = {} + for (const uuid of distinctOrgUUIDs) { + // We need the org content to get admins + const org = await orgRepo.findOneByUUID(uuid) + if (org) { + orgMap[uuid] = org } - - returnValue.users.forEach(user => { - const org = orgMap[user.org_UUID] - if (org && org.admins && org.admins.includes(user.UUID)) { - user.role = 'ADMIN' - } - // If not admin, leave as is (undefined or empty or whatever it was) - }) - } finally { - await session.endSession() } + returnValue.users.forEach(user => { + const org = orgMap[user.org_UUID] + if (org && org.admins && org.admins.includes(user.UUID)) { + user.role = 'ADMIN' + } + // If not admin, leave as is (undefined or empty or whatever it was) + }) + logger.info({ uuid: req.ctx.uuid, message: 'The user information was sent to the secretariat user.' }) return res.status(200).json(returnValue) } catch (err) { @@ -143,7 +137,7 @@ async function getUser (req, res, next) { } async function createUser (req, res, next) { - const session = await mongoose.startSession() + const session = await mongoose.startSession({ causalConsistency: false }) try { const orgRepo = req.ctx.repositories.getBaseOrgRepository() const userRepo = req.ctx.repositories.getBaseUserRepository() diff --git a/src/repositories/baseOrgRepository.js b/src/repositories/baseOrgRepository.js index ce33ed155..eea3a8230 100644 --- a/src/repositories/baseOrgRepository.js +++ b/src/repositories/baseOrgRepository.js @@ -331,7 +331,6 @@ class BaseOrgRepository extends BaseRepository { // In the future we may be able to dynamically detect, but for now we will take a boolean let legacyObjectRaw = null let registryObjectRaw = null - let legacyObject = null let registryObject = null const legacyOrgRepo = new OrgRepository() const ReviewObjectRepository = require('./reviewObjectRepository') @@ -448,8 +447,9 @@ class BaseOrgRepository extends BaseRepository { // The legacy way of doing this, the way this is written under the hood there is no other way // This await does not return a value, even though there is a return in it. :shrugg: + let postUpdate = {} if (isSecretariat) { - await legacyOrgRepo.updateByOrgUUID(sharedUUID, legacyObjectRaw, options) + postUpdate = await legacyOrgRepo.updateByOrgUUID(sharedUUID, legacyObjectRaw, options) } // If we are not a secretariat, then we need to return the uuid of the review object. @@ -459,12 +459,9 @@ class BaseOrgRepository extends BaseRepository { if (isLegacyObject) { // This gets us the mongoose object that has all the right data in it, the "legacyObjectRaw" is the custom JSON we are sending. NOT the post written object. - legacyObject = await legacyOrgRepo.findOneByShortName( - legacyObjectRaw.short_name, - options - ) // Convert the actual model, back to a json model - const legacyObjectRawJson = legacyObject.toObject() + + const legacyObjectRawJson = postUpdate.toObject() // Remove private stuff delete legacyObjectRawJson.__v delete legacyObjectRawJson._id @@ -785,11 +782,11 @@ class BaseOrgRepository extends BaseRepository { updatedRegistryOrg = _.merge(registryOrg, _.omit(registryObjectRaw, jointApprovalFieldsRegistry)) updatedLegacyOrg = _.merge(legacyOrg, _.omit(legacyObjectRaw, jointApprovalFieldsLegacy)) } - // handle conversation const requestingUser = await userRepo.findUserByUUID(requestingUserUUID, options) + const conversationArray = [] if (conversation) { - await conversationRepo.createConversation(registryOrg.UUID, conversation, requestingUser, isSecretariat, options) + conversationArray.push(await conversationRepo.createConversation(registryOrg.UUID, conversation, requestingUser, isSecretariat, options)) } // ADD AUDIT ENTRY AUTOMATICALLY for the registry object before it gets saved. @@ -826,6 +823,7 @@ class BaseOrgRepository extends BaseRepository { } console.log('Audit entry created for registry object') } catch (auditError) { + console.error('Audit entry creation failed:', auditError) } } @@ -878,6 +876,7 @@ class BaseOrgRepository extends BaseRepository { } const plainJavascriptRegistryOrg = updatedRegistryOrg.toObject() + plainJavascriptRegistryOrg.conversation = conversationArray // Remove private things delete plainJavascriptRegistryOrg.__v delete plainJavascriptRegistryOrg._id diff --git a/src/repositories/baseRepository.js b/src/repositories/baseRepository.js index 1a179d157..046a1ad77 100644 --- a/src/repositories/baseRepository.js +++ b/src/repositories/baseRepository.js @@ -59,7 +59,7 @@ class BaseRepository { } async findOneAndUpdate (query = {}, set = {}, options = {}) { - return this.collection.findOneAndUpdate(query, set, options) + return this.collection.findOneAndUpdate(query, set, { ...options, new: true }) } async findOneAndReplace (query = {}, replacement = {}) { diff --git a/src/repositories/orgRepository.js b/src/repositories/orgRepository.js index 1940f4695..099cc2fa8 100644 --- a/src/repositories/orgRepository.js +++ b/src/repositories/orgRepository.js @@ -24,7 +24,8 @@ class OrgRepository extends BaseRepository { // The filter to find the document const filter = { UUID: orgUUID } const updatePayload = { $set: updateData } - return this.collection.findOneAndUpdate(filter, updatePayload, executeOptions) + const data = await this.collection.findOneAndUpdate(filter, updatePayload, { ...executeOptions, new: true }) + return data } async isSecretariat (org, options = {}) { diff --git a/test/unit-tests/org/orgCreateTest.js b/test/unit-tests/org/orgCreateTest.js index cf098cdcb..9b9485ffd 100644 --- a/test/unit-tests/org/orgCreateTest.js +++ b/test/unit-tests/org/orgCreateTest.js @@ -196,8 +196,6 @@ describe('Testing the ORG_CREATE_SINGLE controller', () => { beforeEach(() => { sinon.stub(baseOrgRepo, 'findOneByShortName').resolves(null) - updateOrgStub = sinon.stub(OrgRepository.prototype, 'updateByOrgUUID').resolves(true) - aggregateOrgStub = sinon.stub(orgRepo, 'aggregate') aggregateRegOrgStub = sinon.stub(baseOrgRepo, 'aggregate') @@ -224,6 +222,7 @@ describe('Testing the ORG_CREATE_SINGLE controller', () => { body: testOrgPayload } } + sinon.stub(OrgRepository.prototype, 'updateByOrgUUID').resolves(fakeMongooseDocument) sinon.stub(OrgRepository.prototype, 'findOneByShortName').resolves(fakeMongooseDocument) await orgController.ORG_CREATE_SINGLE(req, res, next) @@ -250,6 +249,7 @@ describe('Testing the ORG_CREATE_SINGLE controller', () => { body: testOrgPayload } } + sinon.stub(OrgRepository.prototype, 'updateByOrgUUID').resolves(fakeMongooseDocument) sinon.stub(OrgRepository.prototype, 'findOneByShortName').resolves(fakeMongooseDocument) await orgController.ORG_CREATE_SINGLE(req, res, next) @@ -278,9 +278,11 @@ describe('Testing the ORG_CREATE_SINGLE controller', () => { body: testOrgPayload } } + const test = await new Org(fakeLegacySavedObjectCisco) sinon.stub(OrgRepository.prototype, 'findOneByShortName').resolves(await new Org(fakeLegacySavedObjectCisco)) sinon.stub(CNAOrgModel.prototype, 'save').resolves(fakeLegacySavedObjectCisco) + sinon.stub(OrgRepository.prototype, 'updateByOrgUUID').resolves(test) await orgController.ORG_CREATE_SINGLE(req, res, next) expect(status.args[0][0]).to.equal(200) @@ -308,8 +310,10 @@ describe('Testing the ORG_CREATE_SINGLE controller', () => { body: testOrgPayload } } - sinon.stub(OrgRepository.prototype, 'findOneByShortName').resolves(await new Org(expectedCreatedOrg)) + const test = await new Org(expectedCreatedOrg) + sinon.stub(OrgRepository.prototype, 'findOneByShortName').resolves(test) sinon.stub(CNAOrgModel.prototype, 'save').resolves(expectedCreatedOrg) + sinon.stub(OrgRepository.prototype, 'updateByOrgUUID').resolves(test) await orgController.ORG_CREATE_SINGLE(req, res, next) expect(status.args[0][0]).to.equal(200) @@ -335,9 +339,10 @@ describe('Testing the ORG_CREATE_SINGLE controller', () => { body: testOrgPayload } } - - sinon.stub(OrgRepository.prototype, 'findOneByShortName').resolves(await new Org(expectedCreatedOrg)) + const test = await new Org(expectedCreatedOrg) + sinon.stub(OrgRepository.prototype, 'findOneByShortName').resolves(test) sinon.stub(CNAOrgModel.prototype, 'save').resolves(expectedCreatedOrg) + sinon.stub(OrgRepository.prototype, 'updateByOrgUUID').resolves(test) await orgController.ORG_CREATE_SINGLE(req, res, next) expect(status.args[0][0]).to.equal(200) @@ -359,9 +364,10 @@ describe('Testing the ORG_CREATE_SINGLE controller', () => { body: testOrgPayload } } - - sinon.stub(OrgRepository.prototype, 'findOneByShortName').resolves(await new Org(testOrgPayload)) + const test = await new Org({ ...testOrgPayload, policies: { id_quota: 0 } }) + sinon.stub(OrgRepository.prototype, 'findOneByShortName').resolves(test) sinon.stub(ADPOrgModel.prototype, 'save').resolves(testOrgPayload) + updateOrgStub = sinon.stub(OrgRepository.prototype, 'updateByOrgUUID').resolves(test) await orgController.ORG_CREATE_SINGLE(req, res, next) expect(status.args[0][0]).to.equal(200)