From fc1ad5299a9dc1674d2e2a542ad2a73ac472d33e Mon Sep 17 00:00:00 2001 From: Evan Hahn Date: Mon, 21 Oct 2024 23:34:09 +0000 Subject: [PATCH] Add some failure codes when adding a server Partly addresses #925. --- src/lib/error-with-code.js | 20 ----------- src/lib/error.js | 47 ++++++++++++++++++++++++++ src/member-api.js | 31 +++++++++++++++-- test-e2e/server.js | 15 ++++++--- test/lib/error-with-code.js | 10 ------ test/lib/error.js | 66 +++++++++++++++++++++++++++++++++++++ 6 files changed, 153 insertions(+), 36 deletions(-) delete mode 100644 src/lib/error-with-code.js create mode 100644 src/lib/error.js delete mode 100644 test/lib/error-with-code.js create mode 100644 test/lib/error.js diff --git a/src/lib/error-with-code.js b/src/lib/error-with-code.js deleted file mode 100644 index 5cb26e1b..00000000 --- a/src/lib/error-with-code.js +++ /dev/null @@ -1,20 +0,0 @@ -/** - * Create an `Error` with a `code` property. - * - * @example - * const err = new ErrorWithCode('INVALID_DATA', 'data was invalid') - * err.message - * // => 'data was invalid' - * err.code - * // => 'INVALID_DATA' - */ -export class ErrorWithCode extends Error { - /** - * @param {string} code - * @param {string} message - */ - constructor(code, message) { - super(message) - /** @readonly */ this.code = code - } -} diff --git a/src/lib/error.js b/src/lib/error.js new file mode 100644 index 00000000..41cbe554 --- /dev/null +++ b/src/lib/error.js @@ -0,0 +1,47 @@ +/** + * Create an `Error` with a `code` property. + * + * @example + * const err = new ErrorWithCode('INVALID_DATA', 'data was invalid') + * err.message + * // => 'data was invalid' + * err.code + * // => 'INVALID_DATA' + */ +export class ErrorWithCode extends Error { + /** + * @param {string} code + * @param {string} message + * @param {object} [options] + * @param {unknown} [options.cause] + */ + constructor(code, message, options) { + super(message, options) + /** @readonly */ this.code = code + } +} + +/** + * Get the error message from an object if possible. + * Otherwise, stringify the argument. + * + * @param {unknown} maybeError + * @returns {string} + * @example + * try { + * // do something + * } catch (err) { + * console.error(getErrorMessage(err)) + * } + */ +export function getErrorMessage(maybeError) { + if (maybeError && typeof maybeError === 'object' && 'message' in maybeError) { + try { + const { message } = maybeError + if (typeof message === 'string') return message + } catch (_err) { + // Ignored + } + } + return 'unknown error' +} diff --git a/src/member-api.js b/src/member-api.js index 3bdc2abc..670ca942 100644 --- a/src/member-api.js +++ b/src/member-api.js @@ -16,7 +16,7 @@ import { keyBy } from './lib/key-by.js' import { abortSignalAny } from './lib/ponyfills.js' import timingSafeEqual from './lib/timing-safe-equal.js' import { isHostnameIpAddress } from './lib/is-hostname-ip-address.js' -import { ErrorWithCode } from './lib/error-with-code.js' +import { ErrorWithCode } from './lib/error.js' import { MEMBER_ROLE_ID, ROLES, isRoleIdForNewInvite } from './roles.js' import { wsCoreReplicator } from './server/ws-core-replicator.js' /** @@ -265,6 +265,17 @@ export class MemberApi extends TypedEmitter { /** * Add a server peer. * + * Can reject with any of the following error codes (accessed via `err.code`): + * + * - `INVALID_URL`: the base URL is invalid, likely due to user error. + * - `NETWORK_ERROR`: there was an issue connecting to the server. Is the + * device online? Is the server online? + * - `INVALID_SERVER_RESPONSE`: we connected to the server but it returned + * an unexpected response. Is the server running a compatible version of + * CoMapeo Cloud? + * + * If `err.code` is not specified, that indicates a bug in this module. + * * @param {string} baseUrl * @param {object} [options] * @param {boolean} [options.dangerouslyAllowInsecureConnections] @@ -375,7 +386,23 @@ export class MemberApi extends TypedEmitter { : 'wss:' const websocket = new WebSocket(websocketUrl) - await pEvent(websocket, 'open') + + try { + await pEvent(websocket, 'open', { rejectionEvents: ['error'] }) + } catch (rejectionEvent) { + throw new ErrorWithCode( + // It's difficult for us to reliably disambiguate between "network error" + // and "invalid response from server" here, so we just say it was an + // invalid server response. + 'INVALID_SERVER_RESPONSE', + 'Failed to open the socket', + rejectionEvent && + typeof rejectionEvent === 'object' && + 'error' in rejectionEvent + ? { cause: rejectionEvent.error } + : { cause: rejectionEvent } + ) + } const onErrorPromise = pEvent(websocket, 'error') diff --git a/test-e2e/server.js b/test-e2e/server.js index 974d38c8..a625fdf0 100644 --- a/test-e2e/server.js +++ b/test-e2e/server.js @@ -180,10 +180,17 @@ test("fails if first request succeeds but sync doesn't", async (t) => { project.$member.addServerPeer(serverBaseUrl, { dangerouslyAllowInsecureConnections: true, }), - { - // TODO - // code: 'INVALID_SERVER_RESPONSE', - message: /404/, + (err) => { + assert(err instanceof Error, 'receives an error') + assert('code' in err, 'gets an error code') + assert.equal( + err.code, + 'INVALID_SERVER_RESPONSE', + 'gets the correct error code' + ) + assert(err.cause instanceof Error, 'error has a cause') + assert(err.cause.message.includes('404'), 'error cause is an HTTP 404') + return true } ) }) diff --git a/test/lib/error-with-code.js b/test/lib/error-with-code.js deleted file mode 100644 index c1db531f..00000000 --- a/test/lib/error-with-code.js +++ /dev/null @@ -1,10 +0,0 @@ -import assert from 'node:assert/strict' -import test from 'node:test' -import { ErrorWithCode } from '../../src/lib/error-with-code.js' - -test('ErrorWithCode', () => { - const err = new ErrorWithCode('MY_CODE', 'my message') - assert.equal(err.code, 'MY_CODE') - assert.equal(err.message, 'my message') - assert(err instanceof Error) -}) diff --git a/test/lib/error.js b/test/lib/error.js new file mode 100644 index 00000000..c7de6ea9 --- /dev/null +++ b/test/lib/error.js @@ -0,0 +1,66 @@ +import assert from 'node:assert/strict' +import test, { describe } from 'node:test' +import { ErrorWithCode, getErrorMessage } from '../../src/lib/error.js' + +describe('ErrorWithCode', () => { + test('ErrorWithCode with two arguments', () => { + const err = new ErrorWithCode('MY_CODE', 'my message') + assert.equal(err.code, 'MY_CODE') + assert.equal(err.message, 'my message') + assert(err instanceof Error) + }) + + test('ErrorWithCode with three arguments', () => { + const otherError = new Error('hello') + const err = new ErrorWithCode('MY_CODE', 'my message', { + cause: otherError, + }) + assert.equal(err.code, 'MY_CODE') + assert.equal(err.message, 'my message') + assert.equal(err.cause, otherError) + assert(err instanceof Error) + }) +}) + +describe('getErrorMessage', () => { + test('from objects without a string message', () => { + const testCases = [ + undefined, + null, + ['ignored'], + { message: 123 }, + { + get message() { + throw new Error('this should not crash') + }, + }, + ] + + for (const testCase of testCases) { + assert.equal(getErrorMessage(testCase), 'unknown error') + } + }) + + test('from objects with a string message', () => { + class WithInheritedMessage { + get message() { + return 'foo' + } + } + + const testCases = [ + { message: 'foo' }, + new Error('foo'), + { + get message() { + return 'foo' + }, + }, + new WithInheritedMessage(), + ] + + for (const testCase of testCases) { + assert.equal(getErrorMessage(testCase), 'foo') + } + }) +})