From c4d930bfdaa2f197eb772e56446e45aaf6866109 Mon Sep 17 00:00:00 2001 From: Vladimir Cucu <108150922+vladimir-cucu@users.noreply.github.com> Date: Wed, 6 Mar 2024 08:44:12 +0200 Subject: [PATCH] WD-9334 - fix: use callback resolve and reject (#123) --- api/client.ts | 135 ++++++++++++++++++++++++------------ api/helpers.ts | 69 +++++++++--------- api/tests/helpers.ts | 6 +- api/tests/test-client.ts | 111 +++++++++++++++++++++++++---- api/tests/test-transform.ts | 40 ++++++++--- api/tests/test-wrappers.ts | 68 ++++++++++-------- api/utils.ts | 41 ++++++++--- generator/interfaces.ts | 5 +- package.json | 2 +- 9 files changed, 330 insertions(+), 147 deletions(-) diff --git a/api/client.ts b/api/client.ts index ba05f18fc..d9c7c6b2c 100644 --- a/api/client.ts +++ b/api/client.ts @@ -20,7 +20,11 @@ import { Error as MacaroonError, MacaroonObject, } from "@canonical/macaroon-bakery/dist/macaroon"; -import type { Callback, JujuRequest } from "../generator/interfaces"; +import type { + Callback, + CloseCallback, + JujuRequest, +} from "../generator/interfaces"; import { ClassType, Facade, @@ -28,13 +32,13 @@ import { FacadeClassList, GenericFacade, } from "./types.js"; -import { createAsyncHandler } from "./utils.js"; +import { createAsyncHandler, toError } from "./utils.js"; export const CLIENT_VERSION = "3.3.2"; export interface ConnectOptions { bakery?: Bakery | null; - closeCallback: Callback; + closeCallback: CloseCallback; debug?: boolean; facades?: (ClassType | GenericFacade)[]; onWSCreated?: (ws: WebSocket) => void; @@ -113,10 +117,10 @@ function connect( const ws = new options!.wsclass!(url); const handler = createAsyncHandler(callback, resolve, reject); ws.onopen = (_evt) => { - handler(null, new Client(ws, options!)); + handler.resolve(new Client(ws, options!)); }; ws.onclose = (evt) => { - handler("cannot connect WebSocket: " + evt.reason); + handler.reject(toError("cannot connect WebSocket: " + evt.reason)); }; ws.onerror = (evt) => { console.log("--", evt); @@ -289,19 +293,36 @@ class Client { return await new Promise(async (resolve, reject) => { let response: any; try { - response = await this._admin.login(args); - } catch (error) { - reject(error); - return; - } - try { + try { + response = await this._admin.login(args); + } catch (error) { + if ( + error instanceof Error && + error.message === INVALIDCREDENTIALS_ERROR + ) { + throw new Error( + "Have you been granted permission to a model on this controller?" + ); + } else if ( + error instanceof Error && + error.message === PERMISSIONDENIED_ERROR + ) { + throw new Error( + "Ensure that you've been given 'login' permission on this controller." + ); + } else { + throw toError(error); + } + } const dischargeRequired = response["discharge-required"] || response["bakery-discharge-required"]; if (dischargeRequired) { if (!this._bakery) { reject( - "macaroon discharge required but no bakery instance provided" + new Error( + "macaroon discharge required but no bakery instance provided" + ) ); return; } @@ -314,38 +335,30 @@ class Client { return resolve(this.login(credentials, clientVersion)); }; const onFailure = (err: string | MacaroonError) => { - reject("macaroon discharge failed: " + err); + reject( + new Error( + "macaroon discharge failed: " + + (err instanceof Object && "Message" in err + ? err.Message + : err) + ) + ); }; this._bakery.discharge(dischargeRequired, onSuccess, onFailure); return; - } else if (response === REDIRECTION_ERROR) { - // This is should be handled by any user of this login method. - throw response; - } else if (response === INVALIDCREDENTIALS_ERROR) { - throw `response -Have you been granted permission to a model on this controller?`; - } else if (response === PERMISSIONDENIED_ERROR) { - throw `response -Ensure that you've been given 'login' permission on this controller.`; - } else if (typeof response === "string") { - // If the response is a string and not an object it's an error - // message and surface that back to the user. - throw response; } resolve(new Connection(this._transport, this._facades, response)); } catch (error) { - if (error !== REDIRECTION_ERROR) { - reject(error); + if (!(error instanceof Error) || error.message !== REDIRECTION_ERROR) { + reject(toError(error)); return; } // This is a model redirection error, fetch the redirection information. try { const info = await this._admin.redirectInfo(null); reject(new RedirectionError(info)); - return; } catch (error) { - reject(error); - return; + reject(toError(error)); } } }); @@ -359,7 +372,7 @@ Ensure that you've been given 'login' permission on this controller.`; another callback. It is responsibility of the callback to call the provided callback if present. */ - logout(callback?: Callback) { + logout(callback?: (code: number, callback: CloseCallback) => void) { this._transport.close(callback); } @@ -378,13 +391,15 @@ Ensure that you've been given 'login' permission on this controller.`; const REDIRECTION_ERROR = "redirection required"; const INVALIDCREDENTIALS_ERROR = "invalid entity name or password"; const PERMISSIONDENIED_ERROR = "permission denied"; -class RedirectionError { +class RedirectionError extends Error { servers: RedirectInfoResult["servers"]; caCert: string; constructor(info: RedirectInfoResult) { + super(REDIRECTION_ERROR); this.servers = info.servers; this.caCert = info["ca-cert"]; + Object.setPrototypeOf(this, RedirectionError.prototype); } } @@ -402,11 +417,11 @@ class RedirectionError { export class Transport { _ws: WebSocket; _counter: number; - _callbacks: { [k: number]: Function }; - _closeCallback: Callback; + _callbacks: { [k: number]: Callback }; + _closeCallback: CloseCallback; _debug: boolean; - constructor(ws: WebSocket, closeCallback: Callback, debug: boolean) { + constructor(ws: WebSocket, closeCallback: CloseCallback, debug: boolean) { this._ws = ws; this._counter = 0; this._callbacks = {}; @@ -436,18 +451,25 @@ export class Transport { @param resolve Function called when the request is successful. @param reject Function called when the request is not successful. */ - write(req: JujuRequest, resolve: Function, reject: Function) { + write( + req: JujuRequest, + resolve: (value: any) => void, + reject: (error: Error) => void + ) { // Check that the connection is ready and sane. const state = this._ws.readyState; if (state !== 1) { const reqStr = JSON.stringify(req); - const error = `cannot send request ${reqStr}: connection state ${state} is not open`; + const error = new Error( + `cannot send request ${reqStr}: connection state ${state} is not open` + ); reject(error); } this._counter += 1; // Include the current request id in the request. req["request-id"] = this._counter; - this._callbacks[this._counter] = resolve; + this._callbacks[this._counter] = (error, result) => + error ? reject(error) : resolve(result); const msg = JSON.stringify(req); if (this._debug) { console.debug("-->", msg); @@ -463,7 +485,7 @@ export class Transport { callback receives the close code and optionally another callback. It is responsibility of the callback to call the provided callback if present. */ - close(callback?: Callback) { + close(callback?: (code: number, callback: CloseCallback) => void) { const closeCallback = this._closeCallback; this._closeCallback = (code) => { if (callback) { @@ -482,13 +504,40 @@ export class Transport { string. */ _handle(data: string) { - const resp = JSON.parse(data); + let resp: unknown; + try { + resp = JSON.parse(data); + } catch (error) { + console.error("Unable to parse the raw response from Juju:", data, error); + return; + } + if ( + !( + resp && + typeof resp === "object" && + "request-id" in resp && + typeof resp["request-id"] === "number" && + ("error" in resp || "response" in resp) + ) + ) { + console.error( + "Parsed raw response from Juju is in incorrect format:", + resp + ); + return; + } const id = resp["request-id"]; const callback = this._callbacks[id]; delete this._callbacks[id]; - if (callback) { - callback(resp.error || resp.response); + if (!callback) { + console.error( + "Parsed raw response from Juju can't be handled. No callback available." + ); + return; } + "error" in resp + ? callback(toError(resp.error)) + : callback(null, resp.response); } } diff --git a/api/helpers.ts b/api/helpers.ts index ad4173920..b26162eec 100644 --- a/api/helpers.ts +++ b/api/helpers.ts @@ -6,7 +6,7 @@ convenience purposes. */ -import type { Callback } from "../generator/interfaces"; +import type { Callback, CallbackError } from "../generator/interfaces"; import type PingerV1 from "./facades/pinger/PingerV1.js"; import { createAsyncHandler } from "./utils.js"; @@ -75,7 +75,7 @@ function wrapAdmin(cls: any): object { const handler = createAsyncHandler(undefined, resolve, reject, transform); // Send the request to the server. - this._transport.write(req, handler); + this._transport.write(req, handler.resolve, handler.reject); }); }; @@ -106,7 +106,7 @@ function wrapAllModelWatcher(cls: any) { */ cls.prototype.next = function ( watcherId: string, - callback: any + callback: Callback ): Promise { // This method is overridden as the auto-generated one does not include the // watcherId parameter, as a result of the peculiarity of the call, which @@ -126,7 +126,7 @@ function wrapAllModelWatcher(cls: any) { }; const handler = createAsyncHandler(callback, resolve, reject, transform); // Send the request to the server. - this._transport.write(req, handler); + this._transport.write(req, handler.resolve, handler.reject); }); }; @@ -143,7 +143,7 @@ function wrapAllModelWatcher(cls: any) { */ cls.prototype.stop = function ( watcherId: string, - callback: any + callback: Callback ): Promise { // This method is overridden as the auto-generated one does not include the // watcherId parameter, as a result of the peculiarity of the call, which @@ -158,7 +158,7 @@ function wrapAllModelWatcher(cls: any) { }; const handler = createAsyncHandler(callback, resolve, reject); // Send the request to the server. - this._transport.write(req, handler); + this._transport.write(req, handler.resolve, handler.reject); }); }; @@ -189,7 +189,7 @@ function wrapAllWatcher(cls: any) { */ cls.prototype.next = function ( watcherId: string, - callback: any + callback: Callback ): Promise { // This method is overridden as the auto-generated one does not include the // watcherId parameter, as a result of the peculiarity of the call, which @@ -209,7 +209,7 @@ function wrapAllWatcher(cls: any) { }; const handler = createAsyncHandler(callback, resolve, reject, transform); // Send the request to the server. - this._transport.write(req, handler); + this._transport.write(req, handler.resolve, handler.reject); }); }; @@ -226,7 +226,7 @@ function wrapAllWatcher(cls: any) { */ cls.prototype.stop = function ( watcherId: string, - callback: any + callback: Callback ): Promise { // This method is overridden as the auto-generated one does not include the // watcherId parameter, as a result of the peculiarity of the call, which @@ -241,7 +241,7 @@ function wrapAllWatcher(cls: any) { }; const handler = createAsyncHandler(callback, resolve, reject); // Send the request to the server. - this._transport.write(req, handler); + this._transport.write(req, handler.resolve, handler.reject); }); }; @@ -271,31 +271,28 @@ function wrapClient(cls: any) { @returns A handle that can be used to stop watching, via its stop method which can be provided a callback receiving an error. */ - cls.prototype.watch = function (callback: Function): object | undefined { - if (!callback) { - callback = () => {}; - } + cls.prototype.watch = function (callback: Callback): object | undefined { // Check that the AllWatcher facade is loaded, as we will use it. const allWatcher = this._info.getFacade("allWatcher"); if (!allWatcher) { - callback("watch requires the allWatcher facade to be loaded", {}); + callback(new Error("watch requires the allWatcher facade to be loaded")); return; } let watcherId: any; // Define a function to repeatedly ask for next changes. - const next = (callback: any) => { + const next = (callback: Callback) => { if (!watcherId) { return; } - allWatcher.next(watcherId, (err: any, result: any) => { - callback(err, result); + allWatcher.next(watcherId, (error: CallbackError, result: any) => { + callback(error, result); next(callback); }); }; // Start watching. - this.watchAll((err: any, result: any) => { - if (err) { - callback(err, {}); + this.watchAll((error: CallbackError, result: any) => { + if (error) { + callback(error); return; } watcherId = result.watcherId; @@ -303,9 +300,9 @@ function wrapClient(cls: any) { }); // Return the handle allowing for stopping the watcher. return { - stop: (callback: any) => { + stop: (callback: Callback) => { if (watcherId === undefined) { - callback("watcher is not running", {}); + callback(new Error("watcher is not running")); return; } allWatcher.stop(watcherId, callback); @@ -340,31 +337,30 @@ function wrapController(cls: any) { @returns A handle that can be used to stop watching, via its stop method which can be provided a callback receiving an error. */ - cls.prototype.watch = function (callback: Function): object | undefined { - if (!callback) { - callback = () => {}; - } + cls.prototype.watch = function (callback: Callback): object | undefined { // Check that the AllModelWatcher facade is loaded, as we will use it. const allModelWatcher: any = this._info.getFacade("allModelWatcher"); if (!allModelWatcher) { - callback("watch requires the allModelWatcher facade to be loaded", {}); + callback( + new Error("watch requires the allModelWatcher facade to be loaded") + ); return; } let watcherId: any; // Define a function to repeatedly ask for next changes. - const next = (callback: any) => { + const next = (callback: Callback) => { if (!watcherId) { return; } - allModelWatcher.next(watcherId, (err: any, result: any) => { - callback(err, result); + allModelWatcher.next(watcherId, (error: CallbackError, result: any) => { + callback(error, result); next(callback); }); }; // Start watching. - this.watchAllModels((err: any, result: any) => { - if (err) { - callback(err, {}); + this.watchAllModels((error: CallbackError, result: any) => { + if (error) { + callback(error); return; } watcherId = result.watcherId; @@ -372,9 +368,9 @@ function wrapController(cls: any) { }); // Return the handle allowing for stopping the watcher. return { - stop: (callback: (arg0: string, arg1: {}) => void) => { + stop: (callback: Callback) => { if (watcherId === undefined) { - callback("watcher is not running", {}); + callback(new Error("watcher is not running")); return; } allModelWatcher.stop(watcherId, callback); @@ -389,6 +385,7 @@ function wrapController(cls: any) { type StopFunction = () => void; /** Ping repeatedly using the Pinger facade. + @param PingerFacade - An instance of the Pinger facade. @param interval - How often would you like to ping? (ms). @param callback - The callback that gets called after each ping. diff --git a/api/tests/helpers.ts b/api/tests/helpers.ts index 459f77022..a8256a8ac 100644 --- a/api/tests/helpers.ts +++ b/api/tests/helpers.ts @@ -188,7 +188,7 @@ export class MockWebSocket { } onopen() {} - onclose(_params: { reason: string }) {} + onclose(_params: { reason: string; code: number; wasClean: boolean }) {} onmessage(_params: { data: string }) {} open() { @@ -196,9 +196,9 @@ export class MockWebSocket { this.onopen(); } - close(reason: string) { + close(reason: string, code = 1000, wasClean = true) { this.readyState = 3; - this.onclose({ reason: reason }); + this.onclose({ reason, code, wasClean }); } message(msg: string) { diff --git a/api/tests/test-client.ts b/api/tests/test-client.ts index 2f6a25a06..c6d0b94f6 100644 --- a/api/tests/test-client.ts +++ b/api/tests/test-client.ts @@ -21,6 +21,7 @@ import { MockWebSocket, requestEqual, } from "./helpers"; +import { toError } from "../utils"; const fail = () => { throw new Error("Fail called"); }; @@ -55,7 +56,7 @@ describe("connect", () => { it("handles failure to connect via promise", (done) => { connect("wss://1.2.3.4", options, (err) => { - expect(err).toBe("cannot connect WebSocket: nope"); + expect(err).toStrictEqual(new Error("cannot connect WebSocket: nope")); done(); }); ws.close("nope"); @@ -63,7 +64,9 @@ describe("connect", () => { it("connect failure", (done) => { connect("wss://1.2.3.4", options, (err?: CallbackError, juju?: Client) => { - expect(err).toBe("cannot connect WebSocket: bad wolf"); + expect(err).toStrictEqual( + new Error("cannot connect WebSocket: bad wolf") + ); expect(juju).toBeFalsy(); done(); }); @@ -71,7 +74,7 @@ describe("connect", () => { ws.close("bad wolf"); }); - function validateLoginFailure(error: string) { + function validateLoginFailure(error: Error, message: string) { requestEqual(ws.lastRequest, { type: "Admin", request: "Login", @@ -85,14 +88,14 @@ describe("connect", () => { }, version: 3, }); - expect(error).toBe("bad wolf"); + expect(error).toStrictEqual(new Error(message)); } it("handles admin login failures", (done) => { connect("wss://1.2.3.4", options).then((juju: Client) => { ws.close(""); juju?.login({ username: "who", password: "secret" }).catch((error) => { - expect(error).toContain("cannot send request"); + expect(toError(error).message).toContain("cannot send request"); done(); }); }); @@ -105,7 +108,7 @@ describe("connect", () => { ?.login({ username: "who", password: "secret" }) .then(() => fail) .catch((error) => { - validateLoginFailure(error); + validateLoginFailure(error, "bad wolf"); done(); }); ws.reply({ error: "bad wolf" }); @@ -113,6 +116,40 @@ describe("connect", () => { ws.open(); }); + it("invalid credentials login failure via promise", (done) => { + connect("wss://1.2.3.4", options).then((juju: Client) => { + juju + ?.login({ username: "who", password: "secret" }) + .then(() => fail) + .catch((error) => { + validateLoginFailure( + error, + "Have you been granted permission to a model on this controller?" + ); + done(); + }); + ws.reply({ error: "invalid entity name or password" }); + }); + ws.open(); + }); + + it("permission denied login failure via promise", (done) => { + connect("wss://1.2.3.4", options).then((juju: Client) => { + juju + ?.login({ username: "who", password: "secret" }) + .then(() => fail) + .catch((error) => { + validateLoginFailure( + error, + "Ensure that you've been given 'login' permission on this controller." + ); + done(); + }); + ws.reply({ error: "permission denied" }); + }); + ws.open(); + }); + function validateRedirectionLoginFailure(error: Error) { requestEqual(ws.lastRequest, { type: "Admin", @@ -125,7 +162,6 @@ describe("connect", () => { it("login redirection error failure via promise", (done) => { connect("wss://1.2.3.4", options).then((juju: Client) => { - // juju._admin.redirectInfo = jest.fn().mockImplementation(() => null); juju ?.login({}) .then(() => fail) @@ -133,6 +169,49 @@ describe("connect", () => { validateRedirectionLoginFailure(error); done(); }); + ws.queueResponses( + new Map([ + // Reply to the redirectInfo request. + [ + 2, + { + response: { + "ca-cert": "exampleCert", + servers: [ + [ + { + Address: { + scope: "exampleScope", + type: "exampleType", + value: "exampleValue", + }, + port: 8080, + scope: "exampleScope", + type: "exampleType", + value: "exampleValue", + }, + ], + ], + }, + }, + ], + ]) + ); + // Reply to the login request. + ws.reply({ error: "redirection required" }); + }); + ws.open(); + }); + + it("login generic redirection error failure via promise", (done) => { + connect("wss://1.2.3.4", options).then((juju: Client) => { + juju + ?.login({}) + .then(() => fail) + .catch((error) => { + expect(error).toStrictEqual(new Error("bad wolf")); + done(); + }); ws.queueResponses( new Map([ // Reply to the redirectInfo request. @@ -274,8 +353,8 @@ describe("connect", () => { }, version: 3, }); - expect(error).toBe( - "macaroon discharge required but no bakery instance provided" + expect(error).toStrictEqual( + new Error("macaroon discharge required but no bakery instance provided") ); } @@ -338,7 +417,9 @@ describe("connect", () => { }, version: 3, }); - expect(error).toBe("macaroon discharge failed: bad wolf"); + expect(error).toStrictEqual( + new Error("macaroon discharge failed: bad wolf") + ); } it("login discharge required failure", (done) => { @@ -486,11 +567,11 @@ describe("connect", () => { params: {}, version: 1, }, + jest.fn(), (err: CallbackError) => { - expect(err).toBe("bad wolf"); + expect(err).toStrictEqual(new Error("bad wolf")); done(); - }, - jest.fn() + } ); // Reply to the transport test request. ws.reply({ error: "bad wolf" }); @@ -565,7 +646,9 @@ describe("connectAndLogin", () => { it("connect failure", (done) => { const creds = {}; connectAndLogin(url, creds, options).catch((error) => { - expect(error).toBe("cannot connect WebSocket: bad wolf"); + expect(error).toStrictEqual( + new Error("cannot connect WebSocket: bad wolf") + ); done(); }); // Close the WebSocket connection. diff --git a/api/tests/test-transform.ts b/api/tests/test-transform.ts index 0e87e5c05..040cd89f7 100644 --- a/api/tests/test-transform.ts +++ b/api/tests/test-transform.ts @@ -3,7 +3,7 @@ "use strict"; -import { autoBind, createAsyncHandler } from "../utils"; +import { autoBind, createAsyncHandler, toError, Label } from "../utils"; describe("autoBind", () => { interface Question { @@ -54,31 +54,37 @@ describe("autoBind", () => { }); describe("createAsyncHandler", () => { - it("returns a function and all arguments are optional", () => { + it("returns an object with reject and resolve function", () => { expect(typeof createAsyncHandler(jest.fn(), jest.fn(), jest.fn())).toBe( - "function" + "object" ); + expect( + typeof createAsyncHandler(jest.fn(), jest.fn(), jest.fn()).reject + ).toBe("function"); + expect( + typeof createAsyncHandler(jest.fn(), jest.fn(), jest.fn()).resolve + ).toBe("function"); }); it("calls callback with successful value", () => { const cb = jest.fn(); const fn = createAsyncHandler(cb, jest.fn(), jest.fn()); - fn(null, "party"); + fn.resolve("party"); expect(cb.mock.calls[0]).toEqual([null, "party"]); }); it("calls callback with error value", () => { const cb = jest.fn(); const fn = createAsyncHandler(cb, jest.fn(), jest.fn()); - fn("boo", {}); - expect(cb.mock.calls[0][0]).toEqual("boo"); + fn.reject(new Error("boo")); + expect(cb.mock.calls[0][0]).toStrictEqual(new Error("boo")); }); it("resolves promise with successful value", () => { expect( new Promise((resolve, reject) => { const fn = createAsyncHandler(undefined, resolve, reject); - fn(null, "party"); + fn.resolve("party"); }) ).resolves.toBe("party"); }); @@ -87,9 +93,9 @@ describe("createAsyncHandler", () => { expect( new Promise((resolve, reject) => { const fn = createAsyncHandler(undefined, resolve, reject); - fn("boo", "party"); + fn.reject(new Error("boo")); }) - ).rejects.toBe("boo"); + ).rejects.toStrictEqual(new Error("boo")); }); it("transforms value if function provided", () => { @@ -101,7 +107,21 @@ describe("createAsyncHandler", () => { () => null, transform ); - fn(null, 10); + fn.resolve(10); expect(cb.mock.calls[0][1]).toBe(20); }); }); + +describe("toError", () => { + it("handles error objects", () => { + expect(toError(new Error("Uh oh!"))).toStrictEqual(new Error("Uh oh!")); + }); + + it("handles error strings", () => { + expect(toError("Uh oh!")).toStrictEqual(new Error("Uh oh!")); + }); + + it("handles unknown errors", () => { + expect(toError(false)).toStrictEqual(new Error(Label.UNKNOWN_ERROR)); + }); +}); diff --git a/api/tests/test-wrappers.ts b/api/tests/test-wrappers.ts index fe646f309..8342aff93 100644 --- a/api/tests/test-wrappers.ts +++ b/api/tests/test-wrappers.ts @@ -17,8 +17,14 @@ describe("wrapAllModelWatcher", () => { static NAME = "AllModelWatcher"; static VERSION = 1; - next(_id: number, _callback: (result: Response | CallbackError) => void) {} - stop(_id: number, _callback: (result: Response | CallbackError) => void) {} + next( + _id: number, + _callback: (error: CallbackError, result?: Response) => void + ) {} + stop( + _id: number, + _callback: (error: CallbackError, result?: Response) => void + ) {} } const options = { closeCallback: jest.fn(), @@ -31,7 +37,7 @@ describe("wrapAllModelWatcher", () => { "allModelWatcher" ) as AllModelWatcherV1; const watcherId = 42; - allModelWatcher?.next(watcherId, (result) => { + allModelWatcher?.next(watcherId, (_error, result) => { requestEqual(ws.lastRequest, { type: "AllModelWatcher", request: "Next", @@ -65,7 +71,7 @@ describe("wrapAllModelWatcher", () => { ) as AllModelWatcherV1; const watcherId = 42; allModelWatcher?.next(watcherId, (err) => { - expect(err).toBe("bad wolf"); + expect(err).toStrictEqual(new Error("bad wolf")); }); // Reply to the next request. ws.reply({ error: "bad wolf" }); @@ -79,7 +85,7 @@ describe("wrapAllModelWatcher", () => { "allModelWatcher" ) as AllModelWatcherV1; const watcherId = 42; - allModelWatcher?.stop(watcherId, (result) => { + allModelWatcher?.stop(watcherId, (_error, result) => { requestEqual(ws.lastRequest, { type: "AllModelWatcher", request: "Stop", @@ -101,7 +107,7 @@ describe("wrapAllModelWatcher", () => { ) as AllModelWatcherV1; const watcherId = 42; allModelWatcher?.stop(watcherId, (err) => { - expect(err).toBe("bad wolf"); + expect(err).toStrictEqual(new Error("bad wolf")); }); // Reply to the next request. ws.reply({ error: "bad wolf" }); @@ -115,8 +121,14 @@ describe("wrapAllWatcher", () => { static NAME = "AllWatcher"; static VERSION = 0; - next(_id: number, _callback: (result: Response | CallbackError) => void) {} - stop(_id: number, _callback: (result: Response | CallbackError) => void) {} + next( + _id: number, + _callback: (error: CallbackError, result?: Response) => void + ) {} + stop( + _id: number, + _callback: (error: CallbackError, result?: Response) => void + ) {} } const options = { closeCallback: jest.fn(), @@ -127,7 +139,7 @@ describe("wrapAllWatcher", () => { makeConnection(options, (conn, ws) => { const allWatcher = conn?.info.getFacade?.("allWatcher") as AllWatcherV0; const watcherId = 42; - allWatcher?.next(watcherId, (result) => { + allWatcher?.next(watcherId, (_error, result) => { requestEqual(ws.lastRequest, { type: "AllWatcher", request: "Next", @@ -159,7 +171,7 @@ describe("wrapAllWatcher", () => { const allWatcher = conn?.info.getFacade?.("allWatcher") as AllWatcherV0; const watcherId = 42; allWatcher?.next(watcherId, (err) => { - expect(err).toBe("bad wolf"); + expect(err).toStrictEqual(new Error("bad wolf")); }); // Reply to the next request. ws.reply({ error: "bad wolf" }); @@ -171,7 +183,7 @@ describe("wrapAllWatcher", () => { makeConnection(options, (conn, ws) => { const allWatcher = conn?.info.getFacade?.("allWatcher") as AllWatcherV0; const watcherId = 42; - allWatcher.stop(watcherId, (result) => { + allWatcher.stop(watcherId, (_error, result) => { requestEqual(ws.lastRequest, { type: "AllWatcher", request: "Stop", @@ -191,7 +203,7 @@ describe("wrapAllWatcher", () => { const allWatcher = conn?.info.getFacade?.("allWatcher") as AllWatcherV0; const watcherId = 42; allWatcher.stop(watcherId, (err) => { - expect(err).toBe("bad wolf"); + expect(err).toStrictEqual(new Error("bad wolf")); }); // Reply to the next request. ws.reply({ error: "bad wolf" }); @@ -225,7 +237,7 @@ describe("wrapClient", () => { makeConnection(options, (conn, ws) => { const client = conn?.info.getFacade?.("client") as ClientV3; let callCount = 0; - client?.watch((result) => { + client?.watch((_error, result) => { callCount += 1; switch (callCount) { case 1: @@ -275,7 +287,9 @@ describe("wrapClient", () => { makeConnection(options, (conn, ws) => { const client = conn?.info.getFacade?.("client") as ClientV3; client.watch((err) => { - expect(err).toBe("watch requires the allWatcher facade to be loaded"); + expect(err).toStrictEqual( + new Error("watch requires the allWatcher facade to be loaded") + ); // Only the login request has been made, no other requests. expect(ws.requests.length).toBe(1); }); @@ -288,10 +302,10 @@ describe("wrapClient", () => { static NAME = "Client"; static VERSION = 3; watchAll(callback: Callback>) { - callback("bad wolf", {}); + callback(new Error("bad wolf"), {}); } watch(callback: Callback>) { - callback("bad wolf", {}); + callback(new Error("bad wolf"), {}); } } class AllWatcherV0 extends BaseFacade { @@ -305,7 +319,7 @@ describe("wrapClient", () => { makeConnection(options, (conn, _ws) => { const client = conn?.info.getFacade?.("client") as ClientV3; client.watch((err) => { - expect(err).toBe("bad wolf"); + expect(err).toStrictEqual(new Error("bad wolf")); }); done(); }); @@ -333,7 +347,7 @@ describe("wrapClient", () => { makeConnection(options, (conn, ws) => { const client = conn?.info.getFacade?.("client") as ClientV3; client.watch((err) => { - expect(err).toBe("bad wolf"); + expect(err).toStrictEqual(new Error("bad wolf")); }); // Reply to the next request. ws.reply({ error: "bad wolf" }); @@ -417,7 +431,7 @@ describe("wrapClient", () => { static NAME = "Client"; static VERSION = 3; addMachines(args: unknown, callback: Callback>) { - callback("bad wolf", {}); + callback(new Error("bad wolf"), {}); } } const options = { @@ -427,7 +441,7 @@ describe("wrapClient", () => { makeConnection(options, (conn, _ws) => { const client = conn?.info.getFacade?.("client") as ClientV3; client.addMachines({ series: "bionic" }, (err) => { - expect(err).toBe("bad wolf"); + expect(err).toStrictEqual(new Error("bad wolf")); }); done(); }); @@ -460,7 +474,7 @@ describe("wrapController", () => { makeConnection(options, (conn, ws) => { const controller = conn?.info.getFacade?.("controller") as ControllerV4; let callCount = 0; - controller?.watch((result) => { + controller?.watch((_error, result) => { callCount += 1; switch (callCount) { case 1: @@ -510,8 +524,8 @@ describe("wrapController", () => { makeConnection(options, (conn, ws) => { const controller = conn?.info.getFacade?.("controller") as ControllerV4; controller.watch((err) => { - expect(err).toBe( - "watch requires the allModelWatcher facade to be loaded" + expect(err).toStrictEqual( + new Error("watch requires the allModelWatcher facade to be loaded") ); // Only the login request has been made, no other requests. expect(ws.requests.length).toBe(1); @@ -525,10 +539,10 @@ describe("wrapController", () => { static NAME = "Controller"; static VERSION = 4; watchAllModels(callback: Callback>) { - callback("bad wolf", {}); + callback(new Error("bad wolf"), {}); } watch(callback: Callback>) { - callback("bad wolf", {}); + callback(new Error("bad wolf"), {}); } } class AllModelWatcherV1 extends BaseFacade { @@ -545,7 +559,7 @@ describe("wrapController", () => { makeConnection(options, (conn, _ws) => { const controller = conn?.info.getFacade?.("controller") as ControllerV4; controller.watch((err) => { - expect(err).toBe("bad wolf"); + expect(err).toStrictEqual(new Error("bad wolf")); }); done(); }); @@ -576,7 +590,7 @@ describe("wrapController", () => { makeConnection(options, (conn, ws) => { const controller = conn?.info.getFacade?.("controller") as ControllerV5; controller.watch((err) => { - expect(err).toBe("bad wolf"); + expect(err).toStrictEqual(new Error("bad wolf")); }); // Reply to the next request. ws.reply({ error: "bad wolf" }); diff --git a/api/utils.ts b/api/utils.ts index 38cb5979c..a78a756c4 100644 --- a/api/utils.ts +++ b/api/utils.ts @@ -3,6 +3,10 @@ import { Callback, CallbackError } from "../generator/interfaces"; +export enum Label { + UNKNOWN_ERROR = "Unknown error", +} + /** Automatically bind all methods of the given object to the object itself. @param obj The object whose method must be bound. @@ -34,17 +38,32 @@ export function autoBind(obj: { [k: string]: any }): void { export function createAsyncHandler( callback: Callback | undefined, resolve: (value: T) => void, - reject: (value: CallbackError) => void, + reject: (error: CallbackError) => void, transform?: (value: T) => T -): Callback { - return (err, value) => { - if (err) { - callback ? callback(err) : reject(err); - return; - } - if (transform) { - value = transform(value!); - } - callback ? callback(null, value) : resolve(value!); +): { resolve: (value: T) => void; reject: (error: CallbackError) => void } { + return { + resolve: (value: T) => { + if (transform) { + value = transform(value!); + } + callback ? callback(null, value) : resolve(value!); + }, + reject: callback ? callback : reject, }; } + +/** + Convert given input to an Error object. + + @param error - The input to be converted to an Error object. + @returns An Error object. +*/ +export function toError(error: any): Error { + if (error instanceof Error) { + return error; + } + if (typeof error === "string") { + return new Error(error); + } + return new Error(Label.UNKNOWN_ERROR); +} diff --git a/generator/interfaces.ts b/generator/interfaces.ts index 8cb7d3e8a..c9cbd1f34 100644 --- a/generator/interfaces.ts +++ b/generator/interfaces.ts @@ -55,5 +55,6 @@ export interface JujuRequest { [key: string]: any; // Some calls pass additional params e.g. AllWatcher.next passes `id`. } -export type CallbackError = string | number | null; -export type Callback = (error?: CallbackError, value?: T) => void; +export type CallbackError = Error | null; +export type Callback = (error: CallbackError, value?: T) => void; +export type CloseCallback = (code: number) => void; diff --git a/package.json b/package.json index d2bd5d715..5c68467a0 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@canonical/jujulib", - "version": "5.0.0", + "version": "6.0.0", "description": "Juju API client", "main": "dist/api/client.js", "types": "dist/api/client.d.ts",