fix(personaIdentifier lock): Add lockedAt field to handle expired loc…

…ks on personaIdentifiers (LLC-1877) (#400)

.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

src/config.ts

@@ -27,6 +27,12 @@ const getMongoNumberOption = (option?: string) => {
   return defaultTo<number|undefined>(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'),

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();
+  }
+}

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';
   }
 }

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;

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<GetIdentifierResult & Lockable>;
+};
 
 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<GetIdentifierResult & Lockable>;
-};
-
 const retryCreateUpdateIdentifierPersona = (config: Config) =>
   async (opts: TheCreateUpdateIdentifierPersonaOptions):
   Promise<CreateUpdateIdentifierPersonaResult> => {
@@ -116,7 +133,8 @@ const retryCreateUpdateIdentifierPersona = (config: Config) =>
 
   return promiseRetry<CreateUpdateIdentifierPersonaResult>(async (retry) => {
     try {
-      return await createUpdateIdentifierPersonaFn(opts);
+      const res = await createUpdateIdentifierPersonaFn(opts);
+      return res;
     } catch (err) {
       /* istanbul ignore else */
       if (err instanceof Locked) {

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 };

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 };
   };
 };

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 };
   };
 };

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,
     }));
 

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();

src/mongoModelsRepo/overwriteIdentifier.ts

@@ -30,6 +30,9 @@ export default (config: Config) => {
         ifi,
         organisation: new ObjectID(organisation),
       },
+      $unset: {
+        lockedAt: '',
+      },
     };
 
     return createOrUpdateIdentifier(config)({

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;
   };
 };

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');
+  });
+});