From 352a3c6b7484bebb8b5bbdad8608d95f3b126431 Mon Sep 17 00:00:00 2001 From: Brett Sun Date: Mon, 8 Apr 2019 20:30:50 +0200 Subject: [PATCH 1/5] Api: propagate errors to callers of single-response APIs --- packages/aragon-rpc-messenger/src/index.js | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/packages/aragon-rpc-messenger/src/index.js b/packages/aragon-rpc-messenger/src/index.js index 1a9fbd2c..2ea03eb3 100644 --- a/packages/aragon-rpc-messenger/src/index.js +++ b/packages/aragon-rpc-messenger/src/index.js @@ -2,7 +2,7 @@ import jsonrpc from './jsonrpc' import MessagePortMessage from './providers/MessagePortMessage' import WindowMessage from './providers/WindowMessage' import DevMessage from './providers/DevMessage' -import { first, filter } from 'rxjs/operators' +import { first, filter, map } from 'rxjs/operators' export const providers = { MessagePortMessage, @@ -92,6 +92,7 @@ export default class Messenger { return this.responses().pipe( filter((message) => message.id === id) + // Let callers handle errors themselves ) } @@ -104,7 +105,14 @@ export default class Messenger { */ sendAndObserveResponse (method, params = []) { return this.sendAndObserveResponses(method, params).pipe( - first() + first(), + map((response) => { + // Emit an error if the response is an error + if (response.error) { + throw new Error(response.error) + } + return response + }) ) } } From 93bfe0c8e8742fb1debc6285583885998fac29e1 Mon Sep 17 00:00:00 2001 From: Brett Sun Date: Mon, 8 Apr 2019 20:31:19 +0200 Subject: [PATCH 2/5] Wrapper: clean up error detection in rpc handlers --- packages/aragon-wrapper/src/rpc/handlers/index.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/aragon-wrapper/src/rpc/handlers/index.js b/packages/aragon-wrapper/src/rpc/handlers/index.js index 79a4199a..2b356fd2 100644 --- a/packages/aragon-wrapper/src/rpc/handlers/index.js +++ b/packages/aragon-wrapper/src/rpc/handlers/index.js @@ -6,8 +6,8 @@ export function createResponse ({ request: { id } }, { error, value = null, kind return {} } - if (error) { - return { id, payload: error } + if (kind === 'E') { + return { id, payload: error || new Error() } } return { id, payload: value } @@ -34,7 +34,7 @@ export function createRequestHandler (request$, requestType, handler) { createResponse ), // filter empty responses caused by Notifications of kind 'C' - filter((response) => response.payload !== undefined || response.error !== undefined) + filter((response) => response.payload !== undefined) ) } From f69ccd2c3b6216f3eee64d5edba9d88c0dcc2670 Mon Sep 17 00:00:00 2001 From: Brett Sun Date: Mon, 8 Apr 2019 20:55:20 +0200 Subject: [PATCH 3/5] tests: add tests for propagating errors --- .../aragon-rpc-messenger/src/index.test.js | 70 ++++++++++++++++--- .../src/rpc/handlers/index.test.js | 15 +++- 2 files changed, 75 insertions(+), 10 deletions(-) diff --git a/packages/aragon-rpc-messenger/src/index.test.js b/packages/aragon-rpc-messenger/src/index.test.js index dcdb26db..7552b7a7 100644 --- a/packages/aragon-rpc-messenger/src/index.test.js +++ b/packages/aragon-rpc-messenger/src/index.test.js @@ -69,13 +69,14 @@ test('should encode and send the request', t => { }) test('should filter the incoming messages to responses only', (t) => { + t.plan(2) + // arrange const busMock = new Subject() const instance = new Messenger(null) instance.bus = () => busMock jsonrpcStub.isValidResponse = sinon.stub().returns(true) - // assert - t.plan(2) + // act const messages = instance.responses() messages.subscribe(value => { @@ -87,13 +88,14 @@ test('should filter the incoming messages to responses only', (t) => { }) test('should filter the incoming messages to requests only', (t) => { + t.plan(2) + // arrange const busMock = new Subject() const instance = new Messenger(null) instance.bus = () => busMock jsonrpcStub.isValidResponse = sinon.stub().returns(false) - // assert - t.plan(2) + // act const messages = instance.requests() messages.subscribe(value => { @@ -105,34 +107,61 @@ test('should filter the incoming messages to requests only', (t) => { }) test('should send and observe responses', (t) => { + t.plan(4) + // arrange const instance = new Messenger(null) sinon.stub(instance, 'send').returns(41) sinon.stub(instance, 'responses').returns(new Subject()) + const messages = instance.sendAndObserveResponses('sendEth') + // assert - t.plan(4) + messages.subscribe(value => t.is(value.data, 'thanks')) + t.is(instance.send.getCall(0).args[0], 'sendEth') + t.deepEqual(instance.send.getCall(0).args[1], []) + // act + instance.responses().next({ data: 'thanks', id: 41 }) + instance.responses().next({ data: 'thanks', id: 41 }) +}) + +test('should send and observe responses, even if errors are included', (t) => { + t.plan(5) + + // arrange + const instance = new Messenger(null) + sinon.stub(instance, 'send').returns(41) + sinon.stub(instance, 'responses').returns(new Subject()) const messages = instance.sendAndObserveResponses('sendEth') - messages.subscribe(value => t.is(value.data, 'thanks')) + // assert + messages.subscribe(value => { + if (value.data) { + t.is(value.data, 'thanks') + } else if (value.error) { + t.is(value.error, 'no thanks') + } + }) t.is(instance.send.getCall(0).args[0], 'sendEth') t.deepEqual(instance.send.getCall(0).args[1], []) + // act instance.responses().next({ data: 'thanks', id: 41 }) + instance.responses().next({ error: 'no thanks', id: 41 }) instance.responses().next({ data: 'thanks', id: 41 }) }) test('should send and observe only the first response', (t) => { + t.plan(3) + // arrange const instance = new Messenger(null) sinon.stub(instance, 'sendAndObserveResponses').returns(new Subject()) - // assert - t.plan(3) + // act const messages = instance.sendAndObserveResponse('sendAnt') // assert messages.subscribe(value => t.is(value, 'first')) - t.is(instance.sendAndObserveResponses.getCall(0).args[0], 'sendAnt') t.deepEqual(instance.sendAndObserveResponses.getCall(0).args[1], []) @@ -140,3 +169,26 @@ test('should send and observe only the first response', (t) => { instance.sendAndObserveResponses().next('second') instance.sendAndObserveResponses().next('third') }) + +test('should send and observe only the first error', (t) => { + t.plan(4) + + // arrange + const instance = new Messenger(null) + sinon.stub(instance, 'sendAndObserveResponses').returns(new Subject()) + const messages = instance.sendAndObserveResponse('sendAnt') + // assert + messages.subscribe( + value => t.fail('should not have emitted any next values'), + error => { + t.true(error instanceof Error) + t.is(error.message, 'bad first') + } + ) + t.is(instance.sendAndObserveResponses.getCall(0).args[0], 'sendAnt') + t.deepEqual(instance.sendAndObserveResponses.getCall(0).args[1], []) + + // act + instance.sendAndObserveResponses().next({ error: 'bad first' }) + instance.sendAndObserveResponses().next('second') +}) diff --git a/packages/aragon-wrapper/src/rpc/handlers/index.test.js b/packages/aragon-wrapper/src/rpc/handlers/index.test.js index 1c4a9a2f..add64ce8 100644 --- a/packages/aragon-wrapper/src/rpc/handlers/index.test.js +++ b/packages/aragon-wrapper/src/rpc/handlers/index.test.js @@ -12,7 +12,7 @@ test.afterEach.always(() => { }) test('should create a request handler', async (t) => { - t.plan(4) + t.plan(5) // arrange const requestStub = from([{ request: { @@ -33,6 +33,12 @@ test('should create a request handler', async (t) => { params: ['set', 'settings'], value: { foo: 'bar' } } + }, { + request: { + id: 'uuid5', + method: 'cache', + params: ['clear'] + } }, { request: { id: 'uuid6', @@ -56,6 +62,10 @@ test('should create a request handler', async (t) => { return Promise.reject(new Error(`no permissions to change ${request.params[1]}!!`)) } + if (request.params[0] === 'clear') { + return Promise.reject(new Error()) + } + return Promise.resolve(`resolved ${request.params[1]}`) } // act @@ -63,7 +73,10 @@ test('should create a request handler', async (t) => { // assert result.subscribe(value => { if (value.id === 'uuid1') return t.is(value.payload, 'resolved settings') + if (value.id === 'uuid4') return t.true(value.payload instanceof Error) if (value.id === 'uuid4') return t.is(value.payload.message, 'no permissions to change settings!!') + if (value.id === 'uuid5') return t.true(value.payload instanceof Error) + if (value.id === 'uuid5') return t.is(value.payload.message, '') if (value.id === 'uuid6') return t.is(value.payload, 'resolved profile') if (value.id === 'uuid8') return t.is(value.payload, null) }) From 338b0429cb77b373082d3f32c3f0b7bac80f803a Mon Sep 17 00:00:00 2001 From: Brett Sun Date: Mon, 8 Apr 2019 21:01:49 +0200 Subject: [PATCH 4/5] Rpc Messenger: create error objects immediately in the response handler --- packages/aragon-rpc-messenger/src/index.js | 11 ++++++++--- packages/aragon-rpc-messenger/src/index.test.js | 9 +++++---- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/packages/aragon-rpc-messenger/src/index.js b/packages/aragon-rpc-messenger/src/index.js index 2ea03eb3..b2f48d41 100644 --- a/packages/aragon-rpc-messenger/src/index.js +++ b/packages/aragon-rpc-messenger/src/index.js @@ -48,7 +48,13 @@ export default class Messenger { */ responses () { return this.bus().pipe( - filter(jsonrpc.isValidResponse) + filter(jsonrpc.isValidResponse), + map((response) => { + if (response.error) { + response.error = new Error(response.error) + } + return response + }) ) } @@ -107,9 +113,8 @@ export default class Messenger { return this.sendAndObserveResponses(method, params).pipe( first(), map((response) => { - // Emit an error if the response is an error if (response.error) { - throw new Error(response.error) + throw response.error } return response }) diff --git a/packages/aragon-rpc-messenger/src/index.test.js b/packages/aragon-rpc-messenger/src/index.test.js index 7552b7a7..d639187b 100644 --- a/packages/aragon-rpc-messenger/src/index.test.js +++ b/packages/aragon-rpc-messenger/src/index.test.js @@ -126,7 +126,7 @@ test('should send and observe responses', (t) => { }) test('should send and observe responses, even if errors are included', (t) => { - t.plan(5) + t.plan(6) // arrange const instance = new Messenger(null) @@ -139,7 +139,8 @@ test('should send and observe responses, even if errors are included', (t) => { if (value.data) { t.is(value.data, 'thanks') } else if (value.error) { - t.is(value.error, 'no thanks') + t.true(value.error instanceof Error) + t.is(value.error.message, 'no thanks') } }) t.is(instance.send.getCall(0).args[0], 'sendEth') @@ -147,7 +148,7 @@ test('should send and observe responses, even if errors are included', (t) => { // act instance.responses().next({ data: 'thanks', id: 41 }) - instance.responses().next({ error: 'no thanks', id: 41 }) + instance.responses().next({ error: new Error('no thanks'), id: 41 }) instance.responses().next({ data: 'thanks', id: 41 }) }) @@ -189,6 +190,6 @@ test('should send and observe only the first error', (t) => { t.deepEqual(instance.sendAndObserveResponses.getCall(0).args[1], []) // act - instance.sendAndObserveResponses().next({ error: 'bad first' }) + instance.sendAndObserveResponses().next({ error: new Error('bad first') }) instance.sendAndObserveResponses().next('second') }) From ea507e7fcb112d3ed76caa4d76540045917fb666 Mon Sep 17 00:00:00 2001 From: Brett Sun Date: Mon, 8 Apr 2019 23:35:05 +0200 Subject: [PATCH 5/5] Rpc Messenger: create error objects only after filtering for the message id --- packages/aragon-rpc-messenger/src/index.js | 17 +++++++++-------- packages/aragon-rpc-messenger/src/index.test.js | 2 +- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/packages/aragon-rpc-messenger/src/index.js b/packages/aragon-rpc-messenger/src/index.js index b2f48d41..066879bb 100644 --- a/packages/aragon-rpc-messenger/src/index.js +++ b/packages/aragon-rpc-messenger/src/index.js @@ -48,13 +48,7 @@ export default class Messenger { */ responses () { return this.bus().pipe( - filter(jsonrpc.isValidResponse), - map((response) => { - if (response.error) { - response.error = new Error(response.error) - } - return response - }) + filter(jsonrpc.isValidResponse) ) } @@ -97,7 +91,13 @@ export default class Messenger { const id = this.send(method, params) return this.responses().pipe( - filter((message) => message.id === id) + filter((message) => message.id === id), + map((response) => { + if (response.error) { + response.error = new Error(response.error) + } + return response + }) // Let callers handle errors themselves ) } @@ -113,6 +113,7 @@ export default class Messenger { return this.sendAndObserveResponses(method, params).pipe( first(), map((response) => { + // Emit an error if the response is an error if (response.error) { throw response.error } diff --git a/packages/aragon-rpc-messenger/src/index.test.js b/packages/aragon-rpc-messenger/src/index.test.js index d639187b..1e08e19c 100644 --- a/packages/aragon-rpc-messenger/src/index.test.js +++ b/packages/aragon-rpc-messenger/src/index.test.js @@ -148,7 +148,7 @@ test('should send and observe responses, even if errors are included', (t) => { // act instance.responses().next({ data: 'thanks', id: 41 }) - instance.responses().next({ error: new Error('no thanks'), id: 41 }) + instance.responses().next({ error: 'no thanks', id: 41 }) instance.responses().next({ data: 'thanks', id: 41 }) })