Skip to content

Commit

Permalink
review feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
James Bush committed Dec 9, 2019
1 parent c9c0edb commit 7045525
Show file tree
Hide file tree
Showing 4 changed files with 152 additions and 94 deletions.
35 changes: 22 additions & 13 deletions src/lib/model/AccountsModel.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -152,18 +152,20 @@ class AccountsModel {
// cancel the timeout handler
clearTimeout(timeout);

// stop listening for account creation response messages
this.cache.unsubscribe(requestKey, subId).then(() => {
this.logger.log('Account creation subscriber unsubscribed');
// stop listening for account creation response messages.
// no need to await for the unsubscribe to complete.
// we dont really care if the unsubscribe fails but we should log it regardless
this.cache.unsubscribe(requestKey, subId).catch(e => {
this.logger.log(`Error unsubscribing (in callback) ${requestKey} ${subId}: ${e.stack || util.inspect(e)}`);
});

if (error) {
return reject(error);
}
if (error) {
return reject(error);
}

const response = message.data;
this.logger.push({ response }).log('Account creation response received');
return resolve(response);
});
const response = message.data;
this.logger.push({ response }).log('Account creation response received');
return resolve(response);
}
catch(err) {
return reject(err);
Expand All @@ -173,9 +175,13 @@ class AccountsModel {
// set up a timeout for the request
const timeout = setTimeout(() => {
const err = new Error(`Timeout waiting for response to account creation request ${request.requestId}`);
this.cache.unsubscribe(requestKey, subId).then(() => {
reject(err);

// we dont really care if the unsubscribe fails but we should log it regardless
this.cache.unsubscribe(requestKey, subId).catch(e => {
this.logger.log(`Error unsubscribing (in timeout handler) ${requestKey} ${subId}: ${e.stack || util.inspect(e)}`);
});

return reject(err);
}, this.requestProcessingTimeoutSeconds * 1000);

// now we have a timeout handler and a cache subscriber hooked up we can fire off
Expand All @@ -187,9 +193,12 @@ class AccountsModel {
catch(err) {
// cancel the timout and unsubscribe before rejecting the promise
clearTimeout(timeout);

// we dont really care if the unsubscribe fails but we should log it regardless
this.cache.unsubscribe(requestKey, subId).catch(e => {
this.logger.log(`Error unsubscribing ${requestKey} ${subId}: ${e.stack || util.inspect(e)}`);
this.logger.log(`Error unsubscribing (in error handler) ${requestKey} ${subId}: ${e.stack || util.inspect(e)}`);
});

return reject(err);
}
});
Expand Down
145 changes: 88 additions & 57 deletions src/lib/model/OutboundTransfersModel.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -197,38 +197,42 @@ class OutboundTransfersModel {
this.logger.push({ payee }).log('Payee resolved');

// stop listening for payee resolution messages
this.cache.unsubscribe(payeeKey, subId).then(() => {
// check we got the right payee and info we need
if(payee.partyIdInfo.partyIdType !== this.data.to.idType) {
const err = new Error(`Expecting resolved payee party IdType to be ${this.data.to.idType} but got ${payee.partyIdInfo.partyIdType}`);
return reject(err);
}
// no need to await for the unsubscribe to complete.
// we dont really care if the unsubscribe fails but we should log it regardless
this.cache.unsubscribe(payeeKey, subId).catch(e => {
this.logger.log(`Error unsubscribing (in callback) ${payeeKey} ${subId}: ${e.stack || util.inspect(e)}`);
});

if(payee.partyIdInfo.partyIdentifier !== this.data.to.idValue) {
const err = new Error(`Expecting resolved payee party identifier to be ${this.data.to.idValue} but got ${payee.partyIdInfo.partyIdentifier}`);
return reject(err);
}
// check we got the right payee and info we need
if(payee.partyIdInfo.partyIdType !== this.data.to.idType) {
const err = new Error(`Expecting resolved payee party IdType to be ${this.data.to.idType} but got ${payee.partyIdInfo.partyIdType}`);
return reject(err);
}

if(!payee.partyIdInfo.fspId) {
const err = new Error(`Expecting resolved payee party to have an FSPID: ${util.inspect(payee.partyIdInfo)}`);
return reject(err);
}
if(payee.partyIdInfo.partyIdentifier !== this.data.to.idValue) {
const err = new Error(`Expecting resolved payee party identifier to be ${this.data.to.idValue} but got ${payee.partyIdInfo.partyIdentifier}`);
return reject(err);
}

// now we got the payee, add the details to our data so we can use it
// in the quote request
this.data.to.fspId = payee.partyIdInfo.fspId;
if(!payee.partyIdInfo.fspId) {
const err = new Error(`Expecting resolved payee party to have an FSPID: ${util.inspect(payee.partyIdInfo)}`);
return reject(err);
}

if(payee.personalInfo) {
if(payee.personalInfo.complexName) {
this.data.to.firstName = payee.personalInfo.complexName.firstName || this.data.to.firstName;
this.data.to.middleName = payee.personalInfo.complexName.middleName || this.data.to.middleName;
this.data.to.lastName = payee.personalInfo.complexName.lastName || this.data.to.lastName;
}
this.data.to.dateOfBirth = payee.personalInfo.dateOfBirth;
// now we got the payee, add the details to our data so we can use it
// in the quote request
this.data.to.fspId = payee.partyIdInfo.fspId;

if(payee.personalInfo) {
if(payee.personalInfo.complexName) {
this.data.to.firstName = payee.personalInfo.complexName.firstName || this.data.to.firstName;
this.data.to.middleName = payee.personalInfo.complexName.middleName || this.data.to.middleName;
this.data.to.lastName = payee.personalInfo.complexName.lastName || this.data.to.lastName;
}
this.data.to.dateOfBirth = payee.personalInfo.dateOfBirth;
}

return resolve(payee);
});
return resolve(payee);
}
catch(err) {
return reject(err);
Expand All @@ -238,9 +242,13 @@ class OutboundTransfersModel {
// set up a timeout for the resolution
const timeout = setTimeout(() => {
const err = new BackendError(`Timeout resolving payee for transfer ${this.data.transferId}`, 504);
this.cache.unsubscribe(payeeKey, subId).then(() => {
reject(err);

// we dont really care if the unsubscribe fails but we should log it regardless
this.cache.unsubscribe(payeeKey, subId).catch(e => {
this.logger.log(`Error unsubscribing (in timeout handler) ${payeeKey} ${subId}: ${e.stack || util.inspect(e)}`);
});

return reject(err);
}, ASYNC_TIMEOUT_MILLS);

// now we have a timeout handler and a cache subscriber hooked up we can fire off
Expand All @@ -252,9 +260,12 @@ class OutboundTransfersModel {
catch(err) {
// cancel the timout and unsubscribe before rejecting the promise
clearTimeout(timeout);

// we dont really care if the unsubscribe fails but we should log it regardless
this.cache.unsubscribe(payeeKey, subId).catch(e => {
this.logger.log(`Error unsubscribing ${payeeKey} ${subId}: ${e.stack || util.inspect(e)}`);
});

return reject(err);
}
});
Expand Down Expand Up @@ -303,20 +314,24 @@ class OutboundTransfersModel {
clearTimeout(timeout);

// stop listening for payee resolution messages
this.cache.unsubscribe(quoteKey, subId).then(() => {
if (error) {
return reject(error);
}
// no need to await for the unsubscribe to complete.
// we dont really care if the unsubscribe fails but we should log it regardless
this.cache.unsubscribe(quoteKey, subId).catch(e => {
this.logger.log(`Error unsubscribing (in callback) ${quoteKey} ${subId}: ${e.stack || util.inspect(e)}`);
});

const quoteResponseBody = message.data;
const quoteResponseHeaders = message.headers;
this.logger.push({ quoteResponseBody }).log('Quote response received');
if (error) {
return reject(error);
}

this.data.quoteResponse = quoteResponseBody;
this.data.quoteResponseSource = quoteResponseHeaders['fspiop-source'];
const quoteResponseBody = message.data;
const quoteResponseHeaders = message.headers;
this.logger.push({ quoteResponseBody }).log('Quote response received');

return resolve(quote);
});
this.data.quoteResponse = quoteResponseBody;
this.data.quoteResponseSource = quoteResponseHeaders['fspiop-source'];

return resolve(quote);
}
catch(err) {
return reject(err);
Expand All @@ -326,9 +341,13 @@ class OutboundTransfersModel {
// set up a timeout for the request
const timeout = setTimeout(() => {
const err = new BackendError(`Timeout requesting quote for transfer ${this.data.transferId}`, 504);
this.cache.unsubscribe(quoteKey, subId).then(() => {
reject(err);

// we dont really care if the unsubscribe fails but we should log it regardless
this.cache.unsubscribe(quoteKey, subId).catch(e => {
this.logger.log(`Error unsubscribing (in timeout handler) ${requestKey} ${subId}: ${e.stack || util.inspect(e)}`);
});

return reject(err);
}, ASYNC_TIMEOUT_MILLS);

// now we have a timeout handler and a cache subscriber hooked up we can fire off
Expand All @@ -340,9 +359,12 @@ class OutboundTransfersModel {
catch(err) {
// cancel the timout and unsubscribe before rejecting the promise
clearTimeout(timeout);

// we dont really care if the unsubscribe fails but we should log it regardless
this.cache.unsubscribe(quoteKey, subId).catch(e => {
this.logger.log(`Error unsubscribing ${quoteKey} ${subId}: ${e.stack || util.inspect(e)}`);
this.logger.log(`Error unsubscribing (in error handler) ${quoteKey} ${subId}: ${e.stack || util.inspect(e)}`);
});

return reject(err);
}
});
Expand Down Expand Up @@ -435,21 +457,23 @@ class OutboundTransfersModel {
clearTimeout(timeout);

// stop listening for transfer fulfil messages
this.cache.unsubscribe(transferKey, subId).then(() => {
if (error) {
return reject(error);
}
this.cache.unsubscribe(transferKey, subId).catch(e => {
this.logger.log(`Error unsubscribing (in callback) ${transferKey} ${subId}: ${e.stack || util.inspect(e)}`);
});

const fulfil = message.data;
this.logger.push({ fulfil }).log('Transfer fulfil received');
this.data.fulfil = fulfil;
if (error) {
return reject(error);
}

if(this.checkIlp && !this.ilp.validateFulfil(fulfil.fulfilment, this.data.quoteResponse.condition)) {
throw new Error('Invalid fulfilment received from peer DFSP.');
}
const fulfil = message.data;
this.logger.push({ fulfil }).log('Transfer fulfil received');
this.data.fulfil = fulfil;

return resolve(fulfil);
});
if(this.checkIlp && !this.ilp.validateFulfil(fulfil.fulfilment, this.data.quoteResponse.condition)) {
throw new Error('Invalid fulfilment received from peer DFSP.');
}

return resolve(fulfil);
}
catch(err) {
return reject(err);
Expand All @@ -459,9 +483,13 @@ class OutboundTransfersModel {
// set up a timeout for the request
const timeout = setTimeout(() => {
const err = new BackendError(`Timeout waiting for fulfil for transfer ${this.data.transferId}`, 504);
this.cache.unsubscribe(transferKey, subId).then(() => {
reject(err);

// we dont really care if the unsubscribe fails but we should log it regardless
this.cache.unsubscribe(transferKey, subId).catch(e => {
this.logger.log(`Error unsubscribing (in timeout handler) ${transferKey} ${subId}: ${e.stack || util.inspect(e)}`);
});

return reject(err);
}, ASYNC_TIMEOUT_MILLS);

// now we have a timeout handler and a cache subscriber hooked up we can fire off
Expand All @@ -473,9 +501,12 @@ class OutboundTransfersModel {
catch(err) {
// cancel the timout and unsubscribe before rejecting the promise
clearTimeout(timeout);

// we dont really care if the unsubscribe fails but we should log it regardless
this.cache.unsubscribe(transferKey, subId).catch(e => {
this.logger.log(`Error unsubscribing ${transferKey} ${subId}: ${e.stack || util.inspect(e)}`);
this.logger.log(`Error unsubscribing (in error handler) ${transferKey} ${subId}: ${e.stack || util.inspect(e)}`);
});

return reject(err);
}
});
Expand Down
14 changes: 11 additions & 3 deletions src/test/unit/lib/cache/cache.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ const defaultConfig = {
};


const dummyPubMessage = {
const dummyPubMessageTemplate = {
data: 12345,
test: '98765'
};
Expand All @@ -37,8 +37,15 @@ const createCache = async() => {
};


let dummyPubMessage;


describe('Cache', () => {

beforeEach(() => {
dummyPubMessage = JSON.parse(JSON.stringify(dummyPubMessageTemplate));
});

test('Makes connections to redis server for cache operations', async () => {
const cache = await createCache();
expect(cache).not.toBeFalsy();
Expand All @@ -53,7 +60,8 @@ describe('Cache', () => {

test('Makes subscriber callbacks on the correct channels when messages arrive', async () => {
const cache = await createCache();
const msg1 = JSON.parse(JSON.stringify(dummyPubMessage));

const msg1 = dummyPubMessage;

// make the messages different
const msg2 = JSON.parse(JSON.stringify(dummyPubMessage));
Expand Down Expand Up @@ -116,7 +124,7 @@ describe('Cache', () => {

test('Unsubscribed callbacks do not get called when messages arrive', async () => {
const cache = await createCache();
const msg1 = JSON.parse(JSON.stringify(dummyPubMessage));
const msg1 = dummyPubMessage;

const chan = 'dummychannel1';

Expand Down
Loading

0 comments on commit 7045525

Please sign in to comment.