Skip to content

Commit

Permalink
FABN-1226: Better handling of query error responses
Browse files Browse the repository at this point in the history 
Query handlers throw on peer error responses instead of treating
them as communication failures and querying additional peers.

Updated TypeScript definitions to include information required
for client code to make the same distinction, which was already
included in the actual JavaScript objects.

Change-Id: Ia0f2f2d763ac7b5f0fdd421838e2180a0bddf323
Signed-off-by: Mark S. Lewis <mark_lewis@uk.ibm.com>
  • Loading branch information
@bestbeforetoday
bestbeforetoday committed May 8, 2019
1 parent 4f62e20 commit 81ec3e9
Show file tree
Hide file tree
Showing 14 changed files with 174 additions and 151 deletions.
6 changes: 5 additions & 1 deletion fabric-client/types/index.d.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -287,7 +287,11 @@ declare namespace Client { // tslint:disable-line:no-namespace
info?: string;
}

export type ProposalResponseObject = [Array<Client.ProposalResponse | Error>, Client.Proposal];
export interface ProposalErrorResponse extends Error {
isProposalResponse?: boolean;
}

export type ProposalResponseObject = [Array<Client.ProposalResponse | Client.ProposalErrorResponse>, Client.Proposal];

export interface OrdererRequest {
txId?: TransactionId;
Expand Down
18 changes: 11 additions & 7 deletions fabric-network/lib/impl/query/roundrobinqueryhandler.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,23 +16,27 @@ class RoundRobinQueryHandler {
constructor(peers) {
logger.debug('constructor: peers=%j', peers.map((peer) => peer.getName()));
this._peers = peers;
this._startPeerIndex = 0;
this._currentPeerIndex = 0;
}

async evaluate(query) {
const startPeerIndex = this._currentPeerIndex++;
const errorMessages = [];

for (let i = 0; i < this._peers.length; i++) {
const index = (this._startPeerIndex + i) % this._peers.length;
const peer = this._peers[index];
const peerIndex = (startPeerIndex + i) % this._peers.length;

const peer = this._peers[peerIndex];
const results = await query.evaluate([peer]);
const result = results[peer.getName()];
if (result instanceof Error) {
errorMessages.push(result.message);
} else {
this._startPeerIndex = (index + 1) % this._peers.length;

if (!(result instanceof Error)) {
return result;
}
if (result.isProposalResponse) {
throw result;
}
errorMessages.push(result.message);
}

const message = util.format('No peers available to query. Errors: %j', errorMessages);
Expand Down
19 changes: 12 additions & 7 deletions fabric-network/lib/impl/query/singlequeryhandler.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,23 +16,28 @@ class SingleQueryHandler {
constructor(peers) {
logger.debug('constructor: peers=%j', peers.map((peer) => peer.getName()));
this._peers = peers;
this._startPeerIndex = 0;
this._currentPeerIndex = 0;
}

async evaluate(query) {
const startPeerIndex = this._currentPeerIndex;
const errorMessages = [];

for (let i = 0; i < this._peers.length; i++) {
const index = (this._startPeerIndex + i) % this._peers.length;
const peer = this._peers[index];
const peerIndex = (startPeerIndex + i) % this._peers.length;
this._currentPeerIndex = peerIndex;

const peer = this._peers[peerIndex];
const results = await query.evaluate([peer]);
const result = results[peer.getName()];
if (result instanceof Error) {
errorMessages.push(result);
} else {
this._startPeerIndex = index;

if (!(result instanceof Error)) {
return result;
}
if (result.isProposalResponse) {
throw result;
}
errorMessages.push(result.message);
}

const message = util.format('No peers available to query. Errors: %j', errorMessages);
Expand Down
32 changes: 26 additions & 6 deletions fabric-network/test/impl/query/queryhandlers.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,25 +15,31 @@ const expect = chai.expect;
chai.use(require('chai-as-promised'));

describe('QueryHandlers', () => {
const queryResult = Buffer.from('QUERY_RESULT');
const querySuccessResult = Buffer.from('SUCCESS_QUERY_RESULT');
const queryErrorResult = new Error('ERROR_QUERY_RESULT');
queryErrorResult.isProposalResponse = true;
const queryFailMessage = 'QUERY_FAILED';

let failedPeers;
let errorPeers;
let fakeEvaluate;
let stubQuery;
let stubPeer1;
let stubPeer2;

beforeEach(() => {
failedPeers = [];
errorPeers = [];
fakeEvaluate = sinon.fake(async (peers) => {
const results = [];
for (const peer of peers) {
const peerName = peer.getName();
if (failedPeers.includes(peer)) {
results[peerName] = new Error(`${queryFailMessage}: ${peerName}`);
} else if (errorPeers.includes(peer)) {
results[peerName] = queryErrorResult;
} else {
results[peerName] = queryResult;
results[peerName] = querySuccessResult;
}
}
return results;
Expand Down Expand Up @@ -78,7 +84,7 @@ describe('QueryHandlers', () => {

it('returns query result', async () => {
const result = await strategy.evaluate(stubQuery);
expect(result).to.equal(queryResult);
expect(result).to.equal(querySuccessResult);
});

it('queries second peer if first fails', async () => {
Expand Down Expand Up @@ -116,7 +122,7 @@ describe('QueryHandlers', () => {

const result = await strategy.evaluate(stubQuery);

expect(result).equals(queryResult);
expect(result).equals(querySuccessResult);
});

it('throws if all peers fail', () => {
Expand All @@ -125,6 +131,13 @@ describe('QueryHandlers', () => {
return expect(strategy.evaluate(stubQuery))
.to.be.rejectedWith(queryFailMessage);
});

it('throws if peer returns error response', () => {
errorPeers = [stubPeer1];

return expect(strategy.evaluate(stubQuery))
.to.be.rejectedWith(queryErrorResult);
});
});

describe('RoundRobinQueryHandler', () => {
Expand Down Expand Up @@ -160,7 +173,7 @@ describe('QueryHandlers', () => {

it('returns query result', async () => {
const result = await strategy.evaluate(stubQuery);
expect(result).to.equal(queryResult);
expect(result).to.equal(querySuccessResult);
});

it('queries second peer if first fails', async () => {
Expand All @@ -187,7 +200,7 @@ describe('QueryHandlers', () => {

const result = await strategy.evaluate(stubQuery);

expect(result).equals(queryResult);
expect(result).equals(querySuccessResult);
});

it('throws if all peers fail', () => {
Expand All @@ -196,5 +209,12 @@ describe('QueryHandlers', () => {
return expect(strategy.evaluate(stubQuery))
.to.be.rejectedWith(queryFailMessage);
});

it('throws if peer returns error response', () => {
errorPeers = [stubPeer1];

return expect(strategy.evaluate(stubQuery))
.to.be.rejectedWith(queryErrorResult);
});
});
});
2 changes: 1 addition & 1 deletion fabric-network/types/index.d.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ export interface Query {
}

export interface QueryResults {
[peerName: string]: Buffer | Error;
[peerName: string]: Buffer | Client.ProposalErrorResponse;
}

export class Gateway {
Expand Down
10 changes: 9 additions & 1 deletion test/fixtures/src/node_cc/example_cc/chaincode.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,10 @@ const Chaincode = class {
return this.query(stub, args);
}

if (fcn === 'returnError') {
return this.returnError(stub, args);
}

if (fcn === 'throwError') {
return this.throwError(stub, args);
}
Expand Down Expand Up @@ -205,8 +209,12 @@ const Chaincode = class {
return shim.success(Buffer.from(Aval.toString()));
}

async returnError(stub, args) {
return shim.error(new Error(args[0] || 'returnError: chaincode error response'));
}

async throwError(stub, args) {
return shim.error(new Error('throwError: an error occurred'));
throw new Error(args[0] || 'throwError: chaincode error thrown');
}

async call(stub, args) {
Expand Down
10 changes: 9 additions & 1 deletion test/fixtures/src/node_cc/example_cc1/chaincode.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,10 @@ const Chaincode = class {
return this.query(stub, args);
}

if (fcn === 'returnError') {
return this.returnError(stub, args);
}

if (fcn === 'throwError') {
return this.throwError(stub, args);
}
Expand Down Expand Up @@ -158,8 +162,12 @@ const Chaincode = class {
return shim.success(Buffer.from(Aval.toString()));
}

async returnError(stub, args) {
return shim.error(new Error(args[0] || 'returnError: chaincode error response'));
}

async throwError(stub, args) {
return shim.error(new Error('throwError: an error occurred'));
throw new Error(args[0] || 'throwError: chaincode error thrown');
}

async testTransient(stub) {
Expand Down
4 changes: 2 additions & 2 deletions test/integration/e2e/invoke-transaction.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ test('\n\n***** End-to-end flow: invoke transaction to move money *****\n\n', as
}

try {
expectedResult = new Error('throwError: an error occurred');
await e2eUtils.invokeChaincode('org2', 'v0', chaincodeId, t, false/* useStore*/, 'throwError', args, expectedResult);
expectedResult = new Error('returnError: an error occurred');
await e2eUtils.invokeChaincode('org2', 'v0', chaincodeId, t, false/* useStore*/, 'returnError', [expectedResult.message], expectedResult);
} catch (err) {
t.fail('Failed to query chaincode on the channel. ' + err.stack ? err.stack : err);
}
Expand Down
4 changes: 2 additions & 2 deletions test/integration/e2e/query.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ test('\n\n***** End-to-end flow: query chaincode *****\n\n', async (t) => {
}

try {
expectedResult = new Error('throwError: an error occurred');
const result = await e2eUtils.queryChaincode('org2', 'v0', targets, 'throwError', args, expectedResult, chaincodeId, t);
expectedResult = new Error('returnError: an error occurred');
const result = await e2eUtils.queryChaincode('org2', 'v0', targets, 'returnError', [expectedResult.message], expectedResult, chaincodeId, t);
if (result) {
t.pass('Successfully handled error from query');
} else {
Expand Down
4 changes: 2 additions & 2 deletions test/integration/nodechaincode/invoke.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ test('\n\n***** Node-Chaincode End-to-end flow: invoke transaction to move money
}

try {
expectedResult = new Error('throwError: an error occurred');
const result = await e2eUtils.invokeChaincode('org2', 'v0', chaincodeId, t, false/* useStore*/, 'throwError', args, expectedResult);
expectedResult = new Error('returnError: an error occurred');
const result = await e2eUtils.invokeChaincode('org2', 'v0', chaincodeId, t, false/* useStore*/, 'returnError', [expectedResult.message], expectedResult);
if (result) {
t.pass('Successfully handled invocation errors');
} else {
Expand Down
4 changes: 2 additions & 2 deletions test/integration/nodechaincode/query.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ test('\n\n***** Node-Chaincode End-to-end flow: query chaincode *****\n\n', asyn
}

try {
expectedResult = new Error('throwError: an error occurred');
const result = await e2eUtils.queryChaincode('org2', 'v0', targets, 'throwError', args, expectedResult, chaincodeId, t);
expectedResult = new Error('returnError: an error occurred');
const result = await e2eUtils.queryChaincode('org2', 'v0', targets, 'returnError', [expectedResult.message], expectedResult, chaincodeId, t);
if (result) {
t.pass('Sucessfully handled error from a query');
} else {
Expand Down
52 changes: 20 additions & 32 deletions test/typescript/integration/network-e2e/invoke.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,21 @@ async function getInternalEventHubForOrg(gateway: Gateway, orgMSP: string): Prom
return eventHubManager.getEventHub(orgPeer);
}

async function testErrorResponse(t: any, contract: Contract): Promise<void> {
const errorMessage = 'TRANSACTION_ERROR_RESPONSE_MESSAGE';

try {
const response = await contract.submitTransaction('returnError', errorMessage);
t.fail('Transaction "returnError" should have thrown an error. Got response: ' + response.toString());
} catch (expectedErr) {
if (expectedErr.message.includes(errorMessage)) {
t.pass('Successfully handled invocation errors');
} else {
t.fail('Unexpected exception: ' + expectedErr);
}
}
}

test('\n\n***** Network End-to-end flow: import identity into wallet and configure tls *****\n\n', async (t: any) => {
try {
await inMemoryIdentitySetup();
Expand Down Expand Up @@ -761,7 +776,6 @@ test('\n\n***** Network End-to-end flow: invoke transaction while channel\'s eve

test('\n\n***** Network End-to-end flow: handle transaction error *****\n\n', async (t: any) => {
const gateway = new Gateway();

try {
const contract = await createContract(t, gateway, {
clientTlsIdentity: 'tlsId',
Expand All @@ -772,15 +786,7 @@ test('\n\n***** Network End-to-end flow: handle transaction error *****\n\n', as
wallet: inMemoryWallet,
});

const response = await contract.submitTransaction('throwError', 'a', 'b', '100');
t.fail('Transaction "throwError" should have thrown an error. Got response: ' + response.toString());
} catch (expectedErr) {
t.comment(expectedErr.message);
if (expectedErr.message.includes('throwError: an error occurred')) {
t.pass('Successfully handled invocation errors');
} else {
t.fail('Unexpected exception: ' + expectedErr.message);
}
await testErrorResponse(t, contract);
} finally {
gateway.disconnect();
}
Expand Down Expand Up @@ -822,7 +828,7 @@ test('\n\n***** Network End-to-end flow: invoke transaction to move money using
const contract = await network.getContract(chaincodeId);
t.pass('Got the contract, about to submit "move" transaction');

let response = await contract.submitTransaction('move', 'a', 'b', '100');
const response = await contract.submitTransaction('move', 'a', 'b', '100');

const expectedResult = 'move succeed';
if (response.toString() === expectedResult) {
Expand All @@ -831,16 +837,7 @@ test('\n\n***** Network End-to-end flow: invoke transaction to move money using
t.fail('Unexpected response from transaction chaincode: ' + response);
}

try {
response = await contract.submitTransaction('throwError', 'a', 'b', '100');
t.fail('Transaction "throwError" should have thrown an error. Got response: ' + response.toString());
} catch (expectedErr) {
if (expectedErr.message.includes('throwError: an error occurred')) {
t.pass('Successfully handled invocation errors');
} else {
t.fail('Unexpected exception: ' + expectedErr.message);
}
}
await testErrorResponse(t, contract);
} catch (err) {
t.fail('Failed to invoke transaction chaincode on channel. ' + err.stack ? err.stack : err);
} finally {
Expand Down Expand Up @@ -889,7 +886,7 @@ test('\n\n***** Network End-to-end flow: invoke transaction to move money using
const contract = await network.getContract(chaincodeId);
t.pass('Got the contract, about to submit "move" transaction');

let response = await contract.submitTransaction('move', 'a', 'b', '100');
const response = await contract.submitTransaction('move', 'a', 'b', '100');

const expectedResult = 'move succeed';
if (response.toString() === expectedResult) {
Expand All @@ -898,16 +895,7 @@ test('\n\n***** Network End-to-end flow: invoke transaction to move money using
t.fail('Unexpected response from transaction chaincode: ' + response);
}

try {
response = await contract.submitTransaction('throwError', 'a', 'b', '100');
t.fail('Transaction "throwError" should have thrown an error. Got response: ' + response.toString());
} catch (expectedErr) {
if (expectedErr.message.includes('throwError: an error occurred')) {
t.pass('Successfully handled invocation errors');
} else {
t.fail('Unexpected exception: ' + expectedErr.message);
}
}
await testErrorResponse(t, contract);
} catch (err) {
t.fail('Failed to invoke transaction chaincode on channel. ' + err.stack ? err.stack : err);
} finally {
Expand Down
Loading

0 comments on commit 81ec3e9

Please sign in to comment.