diff --git a/.env.example b/.env.example index c627c0e26..a19bcef45 100644 --- a/.env.example +++ b/.env.example @@ -5,6 +5,9 @@ MODELS_REPO=memory STORAGE_REPO=memory DEFAULT_TIMEOUT_MS=300000 +# Persona Identifier lock expiration time in ms (default 30000) +#IDENTIFIER_LOCK_EXPIRATION_MS=30000 + # Winston WINSTON_CONSOLE_LEVEL=info WINSTON_CLOUDWATCH_ENABLED=false diff --git a/src/config.ts b/src/config.ts index 900ed7561..3dc8a89f8 100644 --- a/src/config.ts +++ b/src/config.ts @@ -27,6 +27,12 @@ const getMongoNumberOption = (option?: string) => { return defaultTo(Number(option), undefined); }; +const DEFAULT_IDENTIFIER_LOCK_EXPIRATION_MS = 30000; // 30 seconds. +export const IDENTIFIER_LOCK_EXPIRATION_MS = getNumberOption( + process.env.IDENTIFIER_LOCK_EXPIRATION_MS, + DEFAULT_IDENTIFIER_LOCK_EXPIRATION_MS, +); + export default { defaultTimeout: getNumberOption(process.env.DEFAULT_TIMEOUT_MS, DEFAULT_TIMEOUT_MS), lang: getStringOption(process.env.LANG, 'en'), diff --git a/src/errors/ExpiredLock.ts b/src/errors/ExpiredLock.ts new file mode 100644 index 000000000..7dd8ebf46 --- /dev/null +++ b/src/errors/ExpiredLock.ts @@ -0,0 +1,11 @@ +import BaseError from 'jscommons/dist/errors/BaseError'; +import Identifier from '../models/Identifier'; + +export class ExpiredLock extends BaseError { + constructor( + public identifier: Identifier, + public ignorePersonaId: boolean, + ) { + super(); + } +} diff --git a/src/errors/Locked.ts b/src/errors/Locked.ts index 8ee2b01f8..6b8bc1eeb 100644 --- a/src/errors/Locked.ts +++ b/src/errors/Locked.ts @@ -3,7 +3,10 @@ import BaseError from 'jscommons/dist/errors/BaseError'; import Identifier from '../models/Identifier'; export default class extends BaseError { + public readonly message: string; + constructor(public identifier: Identifier) { super(); + this.message = 'Locked'; } } diff --git a/src/models/Identifier.ts b/src/models/Identifier.ts index e74ad6d53..5ff416302 100644 --- a/src/models/Identifier.ts +++ b/src/models/Identifier.ts @@ -7,4 +7,8 @@ interface Identifier extends BaseModel { readonly ifi: Ifi; } +export interface IdentifierWithPersona extends Identifier { + readonly persona: string; +} + export default Identifier; diff --git a/src/mongoModelsRepo/createUpdateIdentifierPersona.ts b/src/mongoModelsRepo/createUpdateIdentifierPersona.ts index 1fa1f47bf..e0f76c2e8 100644 --- a/src/mongoModelsRepo/createUpdateIdentifierPersona.ts +++ b/src/mongoModelsRepo/createUpdateIdentifierPersona.ts @@ -1,5 +1,8 @@ import NoModel from 'jscommons/dist/errors/NoModel'; +import { ObjectID } from 'mongodb'; import * as promiseRetry from 'promise-retry'; + +import { ExpiredLock } from '../errors/ExpiredLock'; import Locked from '../errors/Locked'; import CreateUpdateIdentifierPersonaOptions // tslint:disable-line:import-spacing from '../repoFactory/options/CreateUpdateIdentifierPersonaOptions'; @@ -9,11 +12,15 @@ import CreateUpdateIdentifierPersonaResult // tslint:disable-line:import-spacing import GetIdentifierResult from '../repoFactory/results/GetIdentifierResult'; import Lockable from '../repoFactory/utils/Lockable'; import Config from './Config'; -import createPersona from './createPersona'; import getTheIdentifier from './getIdentifier'; import getIdentifierByIfi from './getIdentifierByIfi'; -import setIdentifierPersona from './setIdentifierPersona'; import createIdentifier from './utils/createIdentifier'; +import createOrUpdateIdentifier from './utils/createOrUpdateIdentifier'; +import { createPersonaAndAddToIdentifier } from './utils/createPersonaAndAddToIdentifier'; + +type TheCreateUpdateIdentifierPersonaOptions = CreateUpdateIdentifierPersonaOptions & { + readonly getIdentifier?: (opts: GetIdentifierOptions) => Promise; +}; const create = (config: Config) => async ({ @@ -32,23 +39,10 @@ const create = (config: Config) => throw new Locked(identifier); } - const { persona } = await createPersona(config)({ - name: personaName, - organisation, - }); - - const { identifier: updatedIdentifier } = await setIdentifierPersona(config)({ - id: identifier.id, - organisation, - persona: persona.id, + return await createPersonaAndAddToIdentifier(config)({ + identifier, + personaName, }); - - return { - identifier: updatedIdentifier, - identifierId: identifier.id, - personaId: persona.id, - wasCreated, - }; }; const createUpdateIdentifierPersona = (config: Config) => @@ -70,7 +64,7 @@ const createUpdateIdentifierPersona = (config: Config) => organisation, }); - if ( locked === true ) { + if (locked === true) { // We are locked, wait for unlock throw new Locked(foundIdentifier); } @@ -85,8 +79,8 @@ const createUpdateIdentifierPersona = (config: Config) => // currently it doesn't get updated return { - identifier: foundIdentifier, identifierId, + identifier: foundIdentifier, personaId: foundIdentifier.persona, wasCreated: false, }; @@ -99,15 +93,38 @@ const createUpdateIdentifierPersona = (config: Config) => personaName, }); } + if (err instanceof ExpiredLock) { + const { + identifier: expiredIdentifier, + ignorePersonaId, + } = err; + + const { identifier: identifierWithoutPersona } = await createOrUpdateIdentifier(config)({ + filter: { + _id: new ObjectID(expiredIdentifier.id), + organisation: new ObjectID(organisation), + }, + update: { + $set: { lockedAt: new Date() }, + ...( + ignorePersonaId + ? { $unset: { persona: '' } } + : {} + ), + }, + upsert: false, + }); + + return await createPersonaAndAddToIdentifier(config)({ + identifier: identifierWithoutPersona, + personaName, + }); + } + throw err; } - }; -type TheCreateUpdateIdentifierPersonaOptions = CreateUpdateIdentifierPersonaOptions & { - readonly getIdentifier?: (opts: GetIdentifierOptions) => Promise; -}; - const retryCreateUpdateIdentifierPersona = (config: Config) => async (opts: TheCreateUpdateIdentifierPersonaOptions): Promise => { @@ -116,7 +133,8 @@ const retryCreateUpdateIdentifierPersona = (config: Config) => return promiseRetry(async (retry) => { try { - return await createUpdateIdentifierPersonaFn(opts); + const res = await createUpdateIdentifierPersonaFn(opts); + return res; } catch (err) { /* istanbul ignore else */ if (err instanceof Locked) { diff --git a/src/mongoModelsRepo/getAttribute.ts b/src/mongoModelsRepo/getAttribute.ts index 9634a1cda..db26d7777 100644 --- a/src/mongoModelsRepo/getAttribute.ts +++ b/src/mongoModelsRepo/getAttribute.ts @@ -25,8 +25,7 @@ export default (config: Config) => { id: document._id.toString(), key: document.key, organisation: document.organisation.toString(), - /* istanbul ignore next */ // shouldnt be null.. - personaId: document.personaId === null ? null : document.personaId.toString(), + personaId: document.personaId?.toString(), value: document.value, }; return { attribute }; diff --git a/src/mongoModelsRepo/getIdentifier.ts b/src/mongoModelsRepo/getIdentifier.ts index d637348fe..5feaf28b0 100644 --- a/src/mongoModelsRepo/getIdentifier.ts +++ b/src/mongoModelsRepo/getIdentifier.ts @@ -1,5 +1,9 @@ import NoModel from 'jscommons/dist/errors/NoModel'; import { ObjectID } from 'mongodb'; + +import { IDENTIFIER_LOCK_EXPIRATION_MS } from '../config'; +import { ExpiredLock } from '../errors/ExpiredLock'; +import Identifier from '../models/Identifier'; import GetIdentifierOptions from '../repoFactory/options/GetIdentifierOptions'; import GetIdentifierResult from '../repoFactory/results/GetIdentifierResult'; import Lockable from '../repoFactory/utils/Lockable'; @@ -22,13 +26,19 @@ export default (config: Config) => { throw new NoModel('Identifier'); } - const identifier = { + const identifier: Identifier = { id: document._id.toString(), ifi: document.ifi, organisation: document.organisation.toString(), - /* istanbul ignore next */ // shouldnt be null.. - persona: document.persona === null ? null : document.persona.toString(), + persona: document.persona?.toString(), }; + + const lockAge = (new Date()).getTime() - (document.lockedAt?.getTime() ?? 0); + + if (document.locked && lockAge > IDENTIFIER_LOCK_EXPIRATION_MS) { + throw new ExpiredLock(identifier, document.lockedAt === undefined); + } + return { identifier, locked: document.locked }; }; }; diff --git a/src/mongoModelsRepo/getIdentifierByIfi.ts b/src/mongoModelsRepo/getIdentifierByIfi.ts index ed63c866a..4234586cc 100644 --- a/src/mongoModelsRepo/getIdentifierByIfi.ts +++ b/src/mongoModelsRepo/getIdentifierByIfi.ts @@ -20,8 +20,7 @@ export default (config: Config) => { } const identifierId = document._id.toString(); - /* istanbul ignore next */ // shouldnt be null.. - const personaId = document.persona === null ? null : document.persona.toString(); + const personaId = document.persona?.toString(); return { identifierId, personaId }; }; }; diff --git a/src/mongoModelsRepo/getPersonaAttributes.ts b/src/mongoModelsRepo/getPersonaAttributes.ts index 2bd640cbe..9f20f6e04 100644 --- a/src/mongoModelsRepo/getPersonaAttributes.ts +++ b/src/mongoModelsRepo/getPersonaAttributes.ts @@ -32,8 +32,7 @@ export default (config: Config) => { id: document._id.toString(), key: document.key, organisation: document.organisation.toString(), - /* istanbul ignore next */ // shouldnt be null.. - personaId: document.personaId === null ? null : document.personaId.toString(), + personaId: document.personaId?.toString(), value: document.value, })); diff --git a/src/mongoModelsRepo/getPersonaIdentifiers.ts b/src/mongoModelsRepo/getPersonaIdentifiers.ts index bd9a53948..324947739 100644 --- a/src/mongoModelsRepo/getPersonaIdentifiers.ts +++ b/src/mongoModelsRepo/getPersonaIdentifiers.ts @@ -32,8 +32,7 @@ export default (config: Config) => { id: document._id.toString(), ifi: document.ifi, organisation: document.organisation.toString(), - /* istanbul ignore next */ // shouldnt be null.. - persona: document.persona === null ? null : document.persona.toString(), + persona: document.persona?.toString(), })); const identifiers = await formattedDocuments.toArray(); diff --git a/src/mongoModelsRepo/overwriteIdentifier.ts b/src/mongoModelsRepo/overwriteIdentifier.ts index cdf5da1f0..0f97a8943 100644 --- a/src/mongoModelsRepo/overwriteIdentifier.ts +++ b/src/mongoModelsRepo/overwriteIdentifier.ts @@ -30,6 +30,9 @@ export default (config: Config) => { ifi, organisation: new ObjectID(organisation), }, + $unset: { + lockedAt: '', + }, }; return createOrUpdateIdentifier(config)({ diff --git a/src/mongoModelsRepo/setIdentifierPersona.ts b/src/mongoModelsRepo/setIdentifierPersona.ts index aeb2d0600..ec20bc705 100644 --- a/src/mongoModelsRepo/setIdentifierPersona.ts +++ b/src/mongoModelsRepo/setIdentifierPersona.ts @@ -24,12 +24,15 @@ export default (config: Config) => { locked: false, persona: new ObjectID(persona), }, + $unset: { + lockedAt: '', + }, }; return await createOrUpdateIdentifier(config)({ filter, update, upsert: false, - }); + }) as SetIdentifierPersonaResult; }; }; diff --git a/src/mongoModelsRepo/tests/createPersonaAndAddToIdentifier.test.ts b/src/mongoModelsRepo/tests/createPersonaAndAddToIdentifier.test.ts new file mode 100644 index 000000000..9900134c4 --- /dev/null +++ b/src/mongoModelsRepo/tests/createPersonaAndAddToIdentifier.test.ts @@ -0,0 +1,72 @@ +import * as assert from 'assert'; +import { + MongoClient, +} from 'mongodb'; + +import config from '../../config'; +import repoFactory from '../../repoFactory'; +import ServiceConfig from '../../service/Config'; +import { TEST_IFI, TEST_ORGANISATION } from '../../tests/utils/values'; +import createUpdateIdentifierPersona from '../createUpdateIdentifierPersona'; +import creatIdentifier from '../utils/createIdentifier'; +import { createPersonaAndAddToIdentifier } from '../utils/createPersonaAndAddToIdentifier'; + +describe('', async () => { + + let serviceConfig: ServiceConfig; // tslint:disable-line:no-let + beforeEach(async () => { + const repoFacade = repoFactory(); + serviceConfig = {repo: repoFacade}; + await serviceConfig.repo.clearRepo(); + }); + + const generateMockDb = async () => { + return (await MongoClient.connect( + config.mongoModelsRepo.url, + config.mongoModelsRepo.options, + )).db(); + }; + + it( + 'Should create a new persona and add toidentifier if identifier doens\'t have one', + async () => { + const dbConfig = { db: generateMockDb() }; + + const { identifier: testNewIdentifier } = await creatIdentifier(dbConfig)({ + ifi: TEST_IFI, + organisation: TEST_ORGANISATION, + }); + + const { + identifier: identifierWithPersona, + wasCreated, + } = await createPersonaAndAddToIdentifier(dbConfig)({ + identifier: testNewIdentifier, + personaName: 'David Tennant', + }); + + assert.equal(identifierWithPersona.persona !== undefined, true, 'Should create and add persona to identifier'); + assert.equal(wasCreated, true, 'Should create a new persona document'); + }); + + it('Should just return identifier unchanged if it already has persona', async () => { + const dbConfig = { db: generateMockDb() }; + + const { identifier: identifierWithPersona } = await createUpdateIdentifierPersona(dbConfig)({ + ifi: TEST_IFI, + organisation: TEST_ORGANISATION, + personaName: 'David Tester', + }); + + const { + identifier: unchangedIdentifier, + wasCreated, + } = await createPersonaAndAddToIdentifier(dbConfig)({ + identifier: identifierWithPersona, + personaName: 'Dave Tester', + }); + + assert.deepEqual(unchangedIdentifier, identifierWithPersona, 'Identifier should be unchanged'); + assert.equal(wasCreated, false, 'We shouldn\'t have created anything, so should be false'); + }); +}); diff --git a/src/mongoModelsRepo/tests/createUpdateIdentifierPersona/createUpdateIdentifierPersona.test.ts b/src/mongoModelsRepo/tests/createUpdateIdentifierPersona/createUpdateIdentifierPersona.test.ts index aa49c9015..693b7483c 100644 --- a/src/mongoModelsRepo/tests/createUpdateIdentifierPersona/createUpdateIdentifierPersona.test.ts +++ b/src/mongoModelsRepo/tests/createUpdateIdentifierPersona/createUpdateIdentifierPersona.test.ts @@ -5,6 +5,7 @@ import { FindOneAndReplaceOption, MongoClient, } from 'mongodb'; + import config from '../../../config'; import Locked from '../../../errors/Locked'; import repoFactory from '../../../repoFactory'; @@ -12,7 +13,7 @@ import ServiceConfig from '../../../service/Config'; import { TEST_IFI, TEST_ORGANISATION } from '../../../tests/utils/values'; import createUpdateIdentifierPersona from '../../createUpdateIdentifierPersona'; -describe('createUpdateIdentifierPersona mongo', () => { +describe('createUpdateIdentifierPersona mongo', async () => { // Only test mongo repo /* istanbul ignore next */ @@ -27,43 +28,42 @@ describe('createUpdateIdentifierPersona mongo', () => { await serviceConfig.repo.clearRepo(); }); - it('Should throw locked if was not created', async () => { - - const generateMockDb = async () => { - const db = (await MongoClient.connect( - config.mongoModelsRepo.url, - config.mongoModelsRepo.options, - )).db(); + const generateMockDb = async () => { + const db = (await MongoClient.connect( + config.mongoModelsRepo.url, + config.mongoModelsRepo.options, + )).db(); - return { - ...db, - collection: (name: string) => { - /* istanbul ignore next */ - if (name !== 'personaIdentifiers') { - return db.collection(name); - } - const collection2 = db.collection(name); + return { + ...db, + collection: (name: string) => { + /* istanbul ignore next */ + if (name !== 'personaIdentifiers') { + return db.collection(name); + } + const collection2 = db.collection(name); - return Object.setPrototypeOf({ - ...collection2, - findOneAndUpdate: async ( - filter: Object, - update: Object, - options: FindOneAndReplaceOption, - ): Promise> => { - const result = await collection2.findOneAndUpdate(filter, update, options); - return { - ...result, - lastErrorObject: { - upserted: undefined, - }, - }; - }, - }, Object.getPrototypeOf(collection2)); - }, - } as Db; - }; + return Object.setPrototypeOf({ + ...collection2, + findOneAndUpdate: async ( + filter: Object, + update: Object, + options: FindOneAndReplaceOption, + ): Promise> => { + const result = await collection2.findOneAndUpdate(filter, update, options); + return { + ...result, + lastErrorObject: { + upserted: undefined, + }, + }; + }, + }, Object.getPrototypeOf(collection2)); + }, + } as Db; + }; + it('Should throw locked if was not created', async () => { const resultPromise = createUpdateIdentifierPersona({ db: generateMockDb() })({ ifi: TEST_IFI, organisation: TEST_ORGANISATION, diff --git a/src/mongoModelsRepo/tests/createUpdateIdentifierPersona/createUpdateIdentifierPersonaLockedAtHandling.test.ts b/src/mongoModelsRepo/tests/createUpdateIdentifierPersona/createUpdateIdentifierPersonaLockedAtHandling.test.ts new file mode 100644 index 000000000..8f10d3e21 --- /dev/null +++ b/src/mongoModelsRepo/tests/createUpdateIdentifierPersona/createUpdateIdentifierPersonaLockedAtHandling.test.ts @@ -0,0 +1,191 @@ +// tslint:disable:max-file-line-count +import * as assert from 'assert'; +import assertError from 'jscommons/dist/tests/utils/assertError'; +import { + MongoClient, + ObjectID, +} from 'mongodb'; + +import config from '../../../config'; +import Locked from '../../../errors/Locked'; +import repoFactory from '../../../repoFactory'; +import ServiceConfig from '../../../service/Config'; +import { TEST_IFI, TEST_ORGANISATION } from '../../../tests/utils/values'; +import Config from '../../Config'; +import createUpdateIdentifierPersona from '../../createUpdateIdentifierPersona'; +import createOrUpdateIdentifier from '../../utils/createOrUpdateIdentifier'; + +describe('createUpdateIdentifierPersona identifier lockedAt handling mongo', async () => { + // Only test mongo repo + /* istanbul ignore next */ + if (config.repoFactory.modelsRepoName !== 'mongo') { + return; + } + + let serviceConfig: ServiceConfig; // tslint:disable-line:no-let + beforeEach(async () => { + const repoFacade = repoFactory(); + serviceConfig = {repo: repoFacade}; + await serviceConfig.repo.clearRepo(); + }); + + const generateMockDb = async () => ( + await MongoClient.connect( + config.mongoModelsRepo.url, + config.mongoModelsRepo.options, + )).db(); + + const getPersonaIdentifiersCollection = async (dbConfig: Config) => ( + (await dbConfig.db).collection('personaIdentifiers') + ); + + it('Should throw a Locked Error for a locked personaIdentifier, ' + + 'with a persona still attached and with a recent lockedAt field', + async () => { + const dbConfig = { db: generateMockDb() }; + + const actor = { + ifi: TEST_IFI, + organisation: TEST_ORGANISATION, + personaName: 'Davey Jones', + }; + // Make a new identifier and persona. + const { identifier: testIdentifier } = await createUpdateIdentifierPersona(dbConfig)(actor); + + // Set locked: true and remove lockedAt. + await createOrUpdateIdentifier(dbConfig)({ + filter: { + _id: new ObjectID(testIdentifier.id), + organisation: new ObjectID(testIdentifier.organisation), + }, + update: { + $set: { locked: true, lockedAt: new Date() }, + }, + upsert: false, + }); + + const resultPromise = createUpdateIdentifierPersona(dbConfig)(actor); + + await assertError(Locked, resultPromise); + + // the document should still be locked and no lockedAt should be recent (< 30s) + const identifierDocument = await (await getPersonaIdentifiersCollection(dbConfig)) + .findOne({ _id: new ObjectID(testIdentifier.id) }); + const lockAge = (new Date()).getTime() - identifierDocument.lockedAt.getTime(); + assert.equal(identifierDocument.locked, true, 'Identifier should still be locked'); + assert.equal(lockAge < 30000, + true, + 'Identifier document\'s locked at should be recent (< 30s)', + ); + }); + + it('Should throw a Locked Error for a locked personaIdentifier' + + 'without a persona and with a recent lockedAt field', + async () => { + const dbConfig = { db: generateMockDb() }; + + const actor = { + ifi: TEST_IFI, + organisation: TEST_ORGANISATION, + personaName: 'Dave Chappelle', + }; + // Make a new identifier and persona. + const { identifier: testIdentifier } = await createUpdateIdentifierPersona(dbConfig)(actor); + + // Set locked: true and remove lockedAt. + await createOrUpdateIdentifier(dbConfig)({ + filter: { + _id: new ObjectID(testIdentifier.id), + organisation: new ObjectID(testIdentifier.organisation), + }, + update: { + $set: { locked: true, lockedAt: new Date() }, + $unset: { persona: '' }, + }, + upsert: false, + }); + + const resultPromise = createUpdateIdentifierPersona(dbConfig)(actor); + + await assertError(Locked, resultPromise); + + // the document should still be locked and no lockedAt should be recent (< 30s) + const identifierDocument = await (await getPersonaIdentifiersCollection(dbConfig)) + .findOne({ _id: new ObjectID(testIdentifier.id) }); + const lockAge = (new Date()).getTime() - identifierDocument.lockedAt.getTime(); + assert.equal(identifierDocument.locked, true, 'Identifier should still be locked'); + assert.equal( + lockAge < 30000, + true, + 'Identifier document\'s locked at should be recent (< 30s)', + ); + }); + + it('Should fix a locked personaIdentifier with an expired lockedAt field (no persona attached)', + async () => { + const dbConfig = { db: generateMockDb() }; + + const actor = { + ifi: TEST_IFI, + organisation: TEST_ORGANISATION, + personaName: 'Davey Jones', + }; + // Make a new identifier and persona. + const { identifier: testIdentifier } = await createUpdateIdentifierPersona(dbConfig)(actor); + // Set locked: true and remove lockedAt. + const now = new Date().getTime(); + const oldDate = new Date(now - 35000); + await createOrUpdateIdentifier(dbConfig)({ + filter: { + _id: new ObjectID(testIdentifier.id), + organisation: new ObjectID(testIdentifier.organisation), + }, + update: { + $set: { locked: true, lockedAt: oldDate }, + $unset: { persona: '' }, + }, + upsert: false, + }); + + const { + identifier: correctedIdentifier, + wasCreated, + } = await createUpdateIdentifierPersona(dbConfig)(actor); + + // should make new persona and put its _id in the identifier's persona field, + // identifierId unchanged, wasCreated true. + assert.equal(correctedIdentifier.id, testIdentifier.id, 'Identifier\'s id should not change'); + assert.deepEqual( + correctedIdentifier.ifi, + testIdentifier.ifi, + 'Identifier\'s ifi should not change', + ); + assert.equal( + correctedIdentifier.organisation, + testIdentifier.organisation, + 'Identifier\'s organisation should not change', + ); + + assert.equal( + correctedIdentifier.persona !== undefined, + true, + 'New persona should have made and added to the identifier', + ); + + assert.equal( + wasCreated, + true, + 'New persona should have been created, so wasCreated should be true', + ); + + // the document should have locked: false, and no lockedAt + const identifierDocument = await (await getPersonaIdentifiersCollection(dbConfig)) + .findOne({ _id: new ObjectID(correctedIdentifier.id) }); + assert.equal(identifierDocument.locked, false, 'Identifier should now be unlocked'); + assert.equal( + identifierDocument.lockedAt, + undefined, + 'Identifier document should not have lockedAt field', + ); + }); +}); diff --git a/src/mongoModelsRepo/tests/createUpdateIdentifierPersona/createUpdateIdentifierPersonaOldLockHandling.test.ts b/src/mongoModelsRepo/tests/createUpdateIdentifierPersona/createUpdateIdentifierPersonaOldLockHandling.test.ts new file mode 100644 index 000000000..a04e7681f --- /dev/null +++ b/src/mongoModelsRepo/tests/createUpdateIdentifierPersona/createUpdateIdentifierPersonaOldLockHandling.test.ts @@ -0,0 +1,93 @@ +import * as assert from 'assert'; +import { + MongoClient, + ObjectID, +} from 'mongodb'; + +import config from '../../../config'; +import repoFactory from '../../../repoFactory'; +import ServiceConfig from '../../../service/Config'; +import { TEST_IFI, TEST_ORGANISATION } from '../../../tests/utils/values'; +import Config from '../../Config'; +import createUpdateIdentifierPersona from '../../createUpdateIdentifierPersona'; +import createOrUpdateIdentifier from '../../utils/createOrUpdateIdentifier'; + +describe('createUpdateIdentifierPersona identifier old lock handling mongo', async () => { + // Only test mongo repo + /* istanbul ignore next */ + if (config.repoFactory.modelsRepoName !== 'mongo') { + return; + } + + let serviceConfig: ServiceConfig; // tslint:disable-line:no-let + beforeEach(async () => { + const repoFacade = repoFactory(); + serviceConfig = {repo: repoFacade}; + await serviceConfig.repo.clearRepo(); + }); + + const generateMockDb = async () => ( + await MongoClient.connect( + config.mongoModelsRepo.url, + config.mongoModelsRepo.options, + )).db(); + + const getPersonaIdentifiersCollection = async (dbConfig: Config) => ( + (await dbConfig.db).collection('personaIdentifiers') + ); + + it('Should handle a locked personaIdentifier without a lockedAt field', async () => { + const dbConfig = { db: generateMockDb() }; + + const actor = { + ifi: TEST_IFI, + organisation: TEST_ORGANISATION, + personaName: 'Santan Dave', + }; + // Make a new identifier and persona. + const { identifier: testIdentifier } = await createUpdateIdentifierPersona(dbConfig)(actor); + + // Set locked: true and remove lockedAt. + await createOrUpdateIdentifier(dbConfig)({ + filter: { + _id: new ObjectID(testIdentifier.id), + organisation: new ObjectID(testIdentifier.organisation), + }, + update: { + $set: { locked: true }, + $unset: { lockedAt: ''}, + }, + upsert: false, + }); + + // run createUpdateIdentifierPersona again with the same actor/ifi + const { + identifier: correctedIdentifier, + wasCreated, + } = await createUpdateIdentifierPersona(dbConfig)(actor); + + // should replace persona field with new persona's _id, identifierId unchanged, wasCreated true. + assert.equal(correctedIdentifier.id, testIdentifier.id, 'Identifier\'s id should not change'); + assert.deepEqual( + correctedIdentifier.ifi, + testIdentifier.ifi, + 'Identifier\'s ifi should not change', + ); + assert.equal( + correctedIdentifier.organisation, + testIdentifier.organisation, + 'Identifier\'s organisation should not change', + ); + + assert.notEqual(correctedIdentifier.persona, testIdentifier.persona, 'Persona should have been replaced with a new one'); + + assert.equal(wasCreated, true, 'New persona should have been created, so wasCreated should be true'); + + // the document should have locked: false, and no lockedAt + const identifierDocument = await (await getPersonaIdentifiersCollection(dbConfig)) + .findOne({ _id: new ObjectID(correctedIdentifier.id) }); + + assert.equal(identifierDocument.locked, false, 'Identifier should now be unlocked'); + assert.equal(identifierDocument.lockedAt, undefined, 'Identifier document should not have lockedAt field'); + }); +}); diff --git a/src/mongoModelsRepo/tests/createUpdateIdentifierPersona/createUpdateIdentifierPersonaRetry.test.ts b/src/mongoModelsRepo/tests/createUpdateIdentifierPersona/createUpdateIdentifierPersonaRetry.test.ts index 06e6646e3..511871ba9 100644 --- a/src/mongoModelsRepo/tests/createUpdateIdentifierPersona/createUpdateIdentifierPersonaRetry.test.ts +++ b/src/mongoModelsRepo/tests/createUpdateIdentifierPersona/createUpdateIdentifierPersonaRetry.test.ts @@ -45,7 +45,7 @@ describe('createUpdateIdentifierPersona mongo retry', () => { return client.db(); }; - it('Should error aftery trying 3 times and the identifier is locked', async () => { + it('Should error after trying 3 times and the identifier is locked', async () => { const repoConfig = { db: getMongoDB() }; // Create mock diff --git a/src/mongoModelsRepo/updateIdentifier.ts b/src/mongoModelsRepo/updateIdentifier.ts index 216906463..5dd342721 100644 --- a/src/mongoModelsRepo/updateIdentifier.ts +++ b/src/mongoModelsRepo/updateIdentifier.ts @@ -33,8 +33,7 @@ export default (config: Config) => { id: result.value._id.toString(), ifi: result.value.ifi, organisation: result.value.organisation.toString(), - /* istanbul ignore next */ // shouldnt be null.. - persona: result.value.persona === null ? null : result.value.persona.toString(), + persona: result.value.persona?.toString(), }; return { diff --git a/src/mongoModelsRepo/utils/createIdentifier.ts b/src/mongoModelsRepo/utils/createIdentifier.ts index bdd0ca2a8..06948837c 100644 --- a/src/mongoModelsRepo/utils/createIdentifier.ts +++ b/src/mongoModelsRepo/utils/createIdentifier.ts @@ -18,14 +18,23 @@ export interface Result { readonly wasCreated: boolean; } +interface Update { + $setOnInsert: { + ifi: Ifi; + locked: boolean; + organisation: ObjectID; + persona?: ObjectID; + lockedAt?: Date; + }; +} + export default (config: Config) => { return async ({ persona, locked = ((persona === undefined) ? true : false), - organisation, ifi, + organisation, }: Options & Lockable): Promise => { - if (!locked && persona === undefined) { throw new PersonaNotSetAndUnlocked(); } @@ -40,15 +49,22 @@ export default (config: Config) => { // Sets properties when the Identifier is created (not found). // Docs: https://docs.mongodb.com/manual/reference/operator/update/setOnInsert/ - const update = { + const update: Update = { $setOnInsert: { ifi, - locked, + locked, // sets lock organisation: new ObjectID(organisation), - persona: new ObjectID(persona), }, }; + if (persona !== undefined) { + update.$setOnInsert.persona = new ObjectID(persona); + } + + if (locked) { + update.$setOnInsert.lockedAt = new Date(); + } + return createOrUpdateIdentifier(config)({ filter, update, diff --git a/src/mongoModelsRepo/utils/createOrUpdateIdentifier.ts b/src/mongoModelsRepo/utils/createOrUpdateIdentifier.ts index 0a7fa7068..5e8a6c694 100644 --- a/src/mongoModelsRepo/utils/createOrUpdateIdentifier.ts +++ b/src/mongoModelsRepo/utils/createOrUpdateIdentifier.ts @@ -37,8 +37,7 @@ const createOrUpdateIdentifier = (config: Config) => async ({ // Formats the document into an identifier to be returned. const document = opResult.value; - /* istanbul ignore next */ // service should never be being passed a null persona - const personaString = document.persona ? document.persona.toString() : null; + const personaString = document.persona?.toString(); const identifier: Identifier = { id: document._id.toString(), ifi: document.ifi, diff --git a/src/mongoModelsRepo/utils/createPersonaAndAddToIdentifier.ts b/src/mongoModelsRepo/utils/createPersonaAndAddToIdentifier.ts new file mode 100644 index 000000000..0aa3282d8 --- /dev/null +++ b/src/mongoModelsRepo/utils/createPersonaAndAddToIdentifier.ts @@ -0,0 +1,59 @@ +import { ObjectID } from 'mongodb'; + +import { CreatePersonaAndAddToIdentifierOptions } from '../../repoFactory/options/CreatePersonaAndAddToIdentifierOptions.types'; +import { CreatePersonaAndAddToIdentifierResult } from '../../repoFactory/results/CreatePersonaAndAddToIdentifierResult.types'; +import Config from '../Config'; +import createPersona from '../createPersona'; +import setIdentifierPersona from '../setIdentifierPersona'; +import createOrUpdateIdentifier from './createOrUpdateIdentifier'; + +export const createPersonaAndAddToIdentifier = (config: Config) => + async ({ + identifier, + personaName, + }: CreatePersonaAndAddToIdentifierOptions): + Promise => { + if (identifier.persona === undefined) { + const { persona } = await createPersona(config)({ + name: personaName, + organisation: identifier.organisation, + }); + + const { identifier: updatedIdentifier } = await setIdentifierPersona(config)({ + id: identifier.id, + organisation: identifier.organisation, + persona: persona.id, + }); + + return { + identifier: updatedIdentifier, + identifierId: updatedIdentifier.id, + personaId: updatedIdentifier.persona, + wasCreated: true, + }; + } else { + // Identifier has a persona, but is still locked. + // This shouldn't happen, but we will unlock it. + const filter = { + _id: new ObjectID(identifier.id), + organisation: new ObjectID(identifier.organisation), + }; + const update = { + $set: { locked: false }, + $unset: { lockedAt: '' }, + }; + + await createOrUpdateIdentifier(config)({ + filter, + update, + upsert: false, + }); + + return { + identifier, + identifierId: identifier.id, + personaId: identifier.persona, + wasCreated: false, + }; + } + }; diff --git a/src/repoFactory/options/CreatePersonaAndAddToIdentifierOptions.types.ts b/src/repoFactory/options/CreatePersonaAndAddToIdentifierOptions.types.ts new file mode 100644 index 000000000..1ec67d245 --- /dev/null +++ b/src/repoFactory/options/CreatePersonaAndAddToIdentifierOptions.types.ts @@ -0,0 +1,6 @@ +import Identifier from '../../models/Identifier'; + +export interface CreatePersonaAndAddToIdentifierOptions { + readonly identifier: Identifier; + readonly personaName?: string; +} diff --git a/src/repoFactory/results/CreatePersonaAndAddToIdentifierResult.types.ts b/src/repoFactory/results/CreatePersonaAndAddToIdentifierResult.types.ts new file mode 100644 index 000000000..eb0f8becc --- /dev/null +++ b/src/repoFactory/results/CreatePersonaAndAddToIdentifierResult.types.ts @@ -0,0 +1,8 @@ +import Identifier, { IdentifierWithPersona } from '../../models/Identifier'; + +export interface CreatePersonaAndAddToIdentifierResult { + readonly identifierId: string; + readonly identifier: Identifier; + readonly wasCreated: boolean; + readonly personaId: string; +} diff --git a/src/repoFactory/results/SetIdentifierPersonaResult.ts b/src/repoFactory/results/SetIdentifierPersonaResult.ts index 189cd0575..0bb17d112 100644 --- a/src/repoFactory/results/SetIdentifierPersonaResult.ts +++ b/src/repoFactory/results/SetIdentifierPersonaResult.ts @@ -1,5 +1,5 @@ -import Identifier from '../../models/Identifier'; +import { IdentifierWithPersona } from '../../models/Identifier'; export default interface SetIdentifierPersonaResult { - readonly identifier: Identifier; + readonly identifier: IdentifierWithPersona; }