diff --git a/docs/decisions.md b/docs/decisions.md index d96be0e2..8c2a8181 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -370,7 +370,7 @@ We aim to be highly transparent with breaking changes and maintain compatibility During the alpha stage every alpha was allowed to contain breaking changes, as long as they were documented and provided migration actions. From 2.0 onwards changes which require action from people running a cluster, such as changes to configs, stored data or the command line, are held back for the next major version. Changes to the plugin API may still be made in minor versions. -Plugin authors should check the changelog when a new minor version is released, non-trivial changes come with a migration guide such as [Migrating Plugins to 2.0](/docs/devs/plugin-migration-to-2.0.md). +Plugin authors should check the changelog when a new minor version is released, non-trivial changes come with a migration guide such as [Migrating Plugins to 2.1](/docs/devs/plugin-migration-to-2.1.md). ### Distribution diff --git a/docs/devs/plugin-migration-to-2.0.md b/docs/devs/plugin-migration-to-2.0.md index 0919fd6d..aae91383 100644 --- a/docs/devs/plugin-migration-to-2.0.md +++ b/docs/devs/plugin-migration-to-2.0.md @@ -305,30 +305,6 @@ The remaining uses of "control" for clusterioctl were renamed to "ctl": The term "control" is still used in message `src` and `dst` and in config field locations, where it covers both ctl and the web interface. -## Request errors - -`RequestError` is gone. -Errors thrown by request handlers are now received by the requester as the same class, see [Request errors](/docs/writing-plugins.md#request-errors). -Replace `RequestError` with the class that fits: - -- Invalid input: `ValidationError`. -- Operation disabled by config: `ForbiddenError`. -- Missing instance, user, save etc.: `NotFoundError`. -- Instance running, host not connected, name taken etc.: `ConflictError`. -- Timeouts: `TimeoutError`. - -`catch` blocks that check `err instanceof RequestError` should check `ClientError` instead, or the specific class. -Checks of `err.code` for remote errors can use `instanceof`, e.g. `err instanceof lib.SessionLost`. - -Other renames: - -- `PermissionError` is `PermissionDeniedError`. -- `lib.InvalidValue`, `lib.InvalidField` and `lib.InvalidAccess` are `lib.InvalidValueError`, `lib.InvalidFieldError` and `lib.InvalidAccessError`. - -Errors of classes that are not registered, like a `TypeError` from a bug, are received as `InternalServerError`. -Plugins that define their own error classes need to list them in `errors` of the plugin declaration. - - ## Static assets in the web interface `staticRoot` now includes the `static/` prefix. diff --git a/docs/devs/plugin-migration-to-2.1.md b/docs/devs/plugin-migration-to-2.1.md new file mode 100644 index 00000000..aac0a789 --- /dev/null +++ b/docs/devs/plugin-migration-to-2.1.md @@ -0,0 +1,26 @@ +# Migrating Plugins to 2.1 + +This guide covers the changes needed to bring a plugin written for 2.0 up to 2.1. +Plugins still on an alpha should start with the [2.0 guide](plugin-migration-to-2.0.md). + + +## Request errors + +`RequestError` is gone. +Errors thrown by request handlers are now received by the requester as the same class, see [Request errors](/docs/writing-plugins.md#request-errors). +Replace `RequestError` with the class that fits: + +- Invalid input: `ValidationError`. +- Operation disabled by config: `ForbiddenError`. +- Missing instance, user, save etc.: `NotFoundError`. +- Instance running, name taken etc.: `ConflictError`. +- Host not connected, instance not running: `UnavailableError`. +- Timeouts: `TimeoutError`. + +`catch` blocks that check `err instanceof RequestError` should check `ClientError` instead, or the specific class. +Checks of `err.code` for remote errors can use `instanceof`, e.g. `err instanceof lib.SessionLost`. + +The config errors `lib.InvalidValue`, `lib.InvalidField` and `lib.InvalidAccess` were renamed to `lib.InvalidValueError`, `lib.InvalidFieldError` and `lib.InvalidAccessError`. + +Errors of classes that are not registered, like a `TypeError` from a bug, are received as `InternalServerError`. +Plugins that define their own error classes need to list them in `errors` of the plugin declaration. diff --git a/docs/writing-plugins.md b/docs/writing-plugins.md index af781dea..d31d14e5 100644 --- a/docs/writing-plugins.md +++ b/docs/writing-plugins.md @@ -645,7 +645,7 @@ Registration rejects control-origin messages with no permission declaration. A callback receives `(user, message)`, where `message` is the raw wire envelope. It runs on the controller before destination payload validation and deserialization, so `message.data` is untrusted and may be missing or malformed. -Check any required payload shape before inspecting fields, and use `user.checkPermission(...)` or throw a `ClientError` such as `PermissionDeniedError` for a normal denial. +Check any required payload shape before inspecting fields, and use `user.checkPermission(...)` or throw a `ClientError` such as `PermissionError` for a normal denial. For example, this checks the payload shape needed to choose a permission; the destination schema still validates the full payload: ```js @@ -756,16 +756,17 @@ Error │ │ ├── InvalidValueError │ │ └── InvalidFieldError │ ├── ForbiddenError (403) -│ │ ├── PermissionDeniedError +│ │ ├── PermissionError │ │ └── InvalidAccessError │ ├── NotFoundError (404) │ ├── TimeoutError (408) │ └── ConflictError (409) └── ServerError (5xx) - └── InternalServerError (500) + ├── InternalServerError (500) + └── UnavailableError (503) ``` -Throw a `ClientError` subclass when the request can't be done as asked, for example: +Throw a `ClientError` subclass when the request can't be done as asked, and `UnavailableError` when the host or instance that should handle it is not connected or not running, for example: ```js controller.handle(messages.Frobnicate, async request => { diff --git a/packages/controller/src/ControlConnection.ts b/packages/controller/src/ControlConnection.ts index e8c650f5..3772ceec 100644 --- a/packages/controller/src/ControlConnection.ts +++ b/packages/controller/src/ControlConnection.ts @@ -431,12 +431,12 @@ export default class ControlConnection extends BaseConnection { // Check connectivity let sourceHostConnection = this._controller.wsServer.hostConnections.get(sourceHostId); if (!sourceHostConnection || sourceHostConnection.connector.closing) { - throw new lib.ConflictError("Source host is not connected to the controller"); + throw new lib.UnavailableError("Source host is not connected to the controller"); } let targetHostConnection = this._controller.wsServer.hostConnections.get(targetHostId); if (!targetHostConnection || targetHostConnection.connector.closing) { - throw new lib.ConflictError("Target host is not connected to the controller"); + throw new lib.UnavailableError("Target host is not connected to the controller"); } // Create stream to proxy from target to source @@ -997,7 +997,7 @@ export default class ControlConnection extends BaseConnection { async handleNoteSetRequest(request: lib.NoteSetRequest) { const { resourceType, resourceId, content } = request; if (!resourceType || !resourceId) { - throw new lib.RequestError("resourceType and resourceId must not be empty"); + throw new lib.ValidationError("resourceType and resourceId must not be empty"); } this._controller.setNote(resourceType, resourceId, content, this.user.name); } diff --git a/packages/controller/src/Controller.ts b/packages/controller/src/Controller.ts index e3924b63..4f319f67 100644 --- a/packages/controller/src/Controller.ts +++ b/packages/controller/src/Controller.ts @@ -1476,13 +1476,13 @@ export default class Controller { } connection = this.wsServer.hostConnections.get(hostId); if (!connection) { - throw new lib.ConflictError("Host containing instance is not connected"); + throw new lib.UnavailableError("Host containing instance is not connected"); } } else if (dst.type === lib.Address.host) { connection = this.wsServer.hostConnections.get(dst.id); if (!connection) { - throw new lib.ConflictError("Host is not connected"); + throw new lib.UnavailableError("Host is not connected"); } } else { @@ -1570,7 +1570,7 @@ export default class Controller { } let connection = this.wsServer.hostConnections.get(hostId); if (!connection) { - throw new lib.ConflictError("Host containing instance is not connected"); + throw new lib.UnavailableError("Host containing instance is not connected"); } return await connection.send(request); } diff --git a/packages/controller/src/ControllerRouter.ts b/packages/controller/src/ControllerRouter.ts index 7fc579db..56cce404 100644 --- a/packages/controller/src/ControllerRouter.ts +++ b/packages/controller/src/ControllerRouter.ts @@ -74,7 +74,7 @@ export default class ControllerRouter { return false; } origin.connector.sendResponseError( - new lib.ResponseError(msg || "Unroutable destination", "ConflictError"), message.src + new lib.ResponseError(msg || "Unroutable destination", "UnavailableError"), message.src ); return true; } diff --git a/packages/controller/src/InstanceManager.ts b/packages/controller/src/InstanceManager.ts index c8e08066..4323d59a 100644 --- a/packages/controller/src/InstanceManager.ts +++ b/packages/controller/src/InstanceManager.ts @@ -173,7 +173,7 @@ export default class InstanceManager { * the instance will be unassigned. * * @throws {module:lib.NotFoundError} if the instance does not exist. - * @throws {module:lib.ConflictError} if the target host is not connected. + * @throws {module:lib.UnavailableError} if the target host is not connected. */ async assignInstance( instanceId: number, @@ -191,7 +191,7 @@ export default class InstanceManager { if (hostId !== null) { newHostConnection = hostConnections.get(hostId); if (!newHostConnection) { - throw new lib.ConflictError("Target host is not connected to the controller"); + throw new lib.UnavailableError("Target host is not connected to the controller"); } } diff --git a/packages/controller/src/User.ts b/packages/controller/src/User.ts index 9a03af48..c7d50147 100644 --- a/packages/controller/src/User.ts +++ b/packages/controller/src/User.ts @@ -1,7 +1,7 @@ import { IUser, Role, SubscribableDatastore, - PermissionDeniedError, PermissionName, permissions, + PermissionError, PermissionName, permissions, } from "@clusterio/lib"; import UserRecord from "./UserRecord.js"; @@ -141,6 +141,6 @@ export default class User extends UserRecord implements IUser { } } - throw new PermissionDeniedError("Permission denied"); + throw new PermissionError("Permission denied"); } } diff --git a/packages/ctl/ctl.ts b/packages/ctl/ctl.ts index 7edbe82e..d6456cf5 100755 --- a/packages/ctl/ctl.ts +++ b/packages/ctl/ctl.ts @@ -307,7 +307,7 @@ async function startCtl() { logger.error(`Error running command: ${err.message}`); process.exitCode = 1; - } else if (err instanceof lib.ClientError) { + } else if (err instanceof lib.ClientError || err instanceof lib.UnavailableError) { logger.error(`Error sending request: ${err.message}`); process.exitCode = 1; diff --git a/packages/host/src/Host.ts b/packages/host/src/Host.ts index 1f110b78..aca92062 100644 --- a/packages/host/src/Host.ts +++ b/packages/host/src/Host.ts @@ -203,7 +203,7 @@ export class HostRouter { return false; } origin.connector.sendResponseError( - new lib.ResponseError(msg || "Instance is not running.", "ConflictError"), message.src + new lib.ResponseError(msg || "Instance is not running.", "UnavailableError"), message.src ); } return true; @@ -271,7 +271,7 @@ export class HostRouter { } if (this.host._shuttingDown) { origin.connector.sendResponseError( - new lib.ResponseError("Host is shutting down.", "ConflictError"), message.src + new lib.ResponseError("Host is shutting down.", "UnavailableError"), message.src ); return; } diff --git a/packages/host/src/Instance.ts b/packages/host/src/Instance.ts index 1eebe450..84ea8793 100644 --- a/packages/host/src/Instance.ts +++ b/packages/host/src/Instance.ts @@ -1422,7 +1422,7 @@ end`.replace(/\r?\n/g, " "); async handleInstanceSaveGameRequest() { if (this._status !== "running") { - throw new lib.ConflictError("Instance is not running"); + throw new lib.UnavailableError("Instance is not running"); } const saved = new Promise((resolve, reject) => { @@ -1432,7 +1432,7 @@ end`.replace(/\r?\n/g, " "); }; const onExit = () => { this.server.off("save-finished", onSaved); - reject(new lib.ConflictError("Instance stopped before the save finished")); + reject(new lib.UnavailableError("Instance stopped before the save finished")); }; this.server.once("save-finished", onSaved); this.server.once("exit", onExit); diff --git a/packages/lib/src/data/UserDetails.ts b/packages/lib/src/data/UserDetails.ts index b46360c9..ffdb2585 100644 --- a/packages/lib/src/data/UserDetails.ts +++ b/packages/lib/src/data/UserDetails.ts @@ -162,7 +162,7 @@ export interface IUser extends UserDetails { * * @param permission - The permission to check for. * @throws {Error} If the given permission does not exist. - * @throws {libErrors.PermissionDeniedError} if the user does noh have the given permission. + * @throws {libErrors.PermissionError} if the user does noh have the given permission. */ checkPermission(permission: PermissionName): void } diff --git a/packages/lib/src/errors.ts b/packages/lib/src/errors.ts index a17eacdf..54a33cf2 100644 --- a/packages/lib/src/errors.ts +++ b/packages/lib/src/errors.ts @@ -49,7 +49,7 @@ export class ForbiddenError extends ClientError { } /** * Thrown when a permission check fails */ -export class PermissionDeniedError extends ForbiddenError { } +export class PermissionError extends ForbiddenError { } /** * Thrown when a config field is not accessible from the current location @@ -68,7 +68,7 @@ export class TimeoutError extends ClientError { } /** * Thrown when the request conflicts with the current state, such as an - * instance that is running or a host that is not connected (HTTP 409) + * instance that is running or a name that is taken (HTTP 409) */ export class ConflictError extends ClientError { } @@ -84,6 +84,12 @@ export class ServerError extends Error { } */ export class InternalServerError extends ServerError { } +/** + * Thrown when the host or instance that should handle the request is not + * connected, not running or shutting down (HTTP 503) + */ +export class UnavailableError extends ServerError { } + /** * Signal for messages that fail validation */ diff --git a/packages/lib/src/link/link.ts b/packages/lib/src/link/link.ts index 87616e44..9a8b17bb 100644 --- a/packages/lib/src/link/link.ts +++ b/packages/lib/src/link/link.ts @@ -348,7 +348,7 @@ export class Link { * * @param message - Message to check. * @param entry - Request or Event entry for this Message. - * @throws {libErrors.PermissionDeniedError} if unauthorized. + * @throws {libErrors.PermissionError} if unauthorized. */ validatePermission(message: libData.MessageRequest | libData.MessageEvent, entry: RequestEntry | EventEntry) { } diff --git a/packages/lib/src/link/messages.ts b/packages/lib/src/link/messages.ts index 8b7d77d9..c4d95dcb 100644 --- a/packages/lib/src/link/messages.ts +++ b/packages/lib/src/link/messages.ts @@ -164,13 +164,14 @@ export const errorClasses: ErrorClass[] = [ errors.InvalidValueError, errors.InvalidFieldError, errors.ForbiddenError, - errors.PermissionDeniedError, + errors.PermissionError, errors.InvalidAccessError, errors.NotFoundError, errors.TimeoutError, errors.ConflictError, errors.ServerError, errors.InternalServerError, + errors.UnavailableError, errors.InvalidMessage, errors.SessionLost, ]; diff --git a/test/controller/ControlConnection.test.js b/test/controller/ControlConnection.test.js index 166e79c7..6cb50dd8 100644 --- a/test/controller/ControlConnection.test.js +++ b/test/controller/ControlConnection.test.js @@ -197,7 +197,7 @@ describe("controller/src/ControlConnection", function() { await connection.handleLogSetSubscriptionsRequest( new lib.LogSetSubscriptionsRequest(true, false, [], []) ); - connection.user = { checkPermission() { throw new lib.PermissionDeniedError("Permission denied"); } }; + connection.user = { checkPermission() { throw new lib.PermissionError("Permission denied"); } }; connection.checkLogPermission(); assert.equal(mockController.clusterLogger.transports.size, 0); assert.equal(connection.logTransport, null); diff --git a/test/controller/Controller.js b/test/controller/Controller.js index 31a66272..412305e0 100644 --- a/test/controller/Controller.js +++ b/test/controller/Controller.js @@ -9,7 +9,7 @@ import express from "express"; import { fileURLToPath } from "node:url"; import { - ControllerConfig, Address, ConflictError, NotFoundError, + ControllerConfig, Address, ConflictError, NotFoundError, UnavailableError, InstanceConfig, SystemInfo, Role, ModPack, AccountUpdateEvent, addPluginConfigFields, renamePermissionPrefix, } from "@clusterio/lib"; @@ -274,13 +274,13 @@ describe("controller/src/Controller", function() { mockInstanceConfig.set("instance.assigned_host", 99); assert.throws( () => controller.sendRequest(new MockEvent(), Address.fromShorthand({ instanceId: 100 })), - new ConflictError("Host containing instance is not connected") + new UnavailableError("Host containing instance is not connected") ); }); it("should error on invalid host", function() { assert.throws( () => controller.sendRequest(new MockEvent(), Address.fromShorthand({ hostId: 99 })), - new ConflictError("Host is not connected") + new UnavailableError("Host is not connected") ); }); it("should error on invalid broadcast", function() { diff --git a/test/controller/InstanceManager.test.js b/test/controller/InstanceManager.test.js index b44576f8..dc254629 100644 --- a/test/controller/InstanceManager.test.js +++ b/test/controller/InstanceManager.test.js @@ -172,7 +172,7 @@ describe("controller/InstanceManager", function () { it("should throw if host not connected", async function () { await assert.rejects( () => instances.assignInstance(1, 5), - lib.ConflictError + lib.UnavailableError ); }); @@ -204,7 +204,7 @@ describe("controller/InstanceManager", function () { await assert.rejects( () => instances.assignInstance(1, 6), - lib.ConflictError + lib.UnavailableError ); assert.equal(unassignRequest, undefined); diff --git a/test/lib/config/classes.js b/test/lib/config/classes.js index ef8299bb..538e5f27 100644 --- a/test/lib/config/classes.js +++ b/test/lib/config/classes.js @@ -835,7 +835,9 @@ describe("lib/config/classes", function() { it("should throw if dependent validation fails", function() { assert.throws( () => testInstance.set("test.base", 5), - new lib.InvalidValueError("Failed validation of dependent test.dependent: dependent must be >= base") + new lib.InvalidValueError( + "Failed validation of dependent test.dependent: dependent must be >= base" + ) ); assert.deepEqual(TestConfig.validatorCalls, [ ["test.dependent", 1], // set test.base diff --git a/test/lib/link/link.js b/test/lib/link/link.js index f0584e7d..cc7331fb 100644 --- a/test/lib/link/link.js +++ b/test/lib/link/link.js @@ -556,7 +556,7 @@ describe("lib/link/link", function() { let errored = false; connector.on("error", () => { errored = true; }); link.handle(SimpleRequest, async () => { handled = true; }); - link.validatePermission = () => { throw new lib.PermissionDeniedError("Denied"); }; + link.validatePermission = () => { throw new lib.PermissionError("Denied"); }; connector.emit("message", message); assert(!handled, "handler ran despite denial"); assert(!errored, "connector emitted error on denial"); diff --git a/test/lib/subscriptions.test.js b/test/lib/subscriptions.test.js index 64cfdd2a..461ca0ee 100644 --- a/test/lib/subscriptions.test.js +++ b/test/lib/subscriptions.test.js @@ -560,7 +560,7 @@ describe("lib/subscriptions", function() { checkPermission(permission) { if (permission === this.denied) { - throw new lib.PermissionDeniedError("Permission denied"); + throw new lib.PermissionError("Permission denied"); } } } @@ -1400,26 +1400,26 @@ describe("lib/subscriptions", function() { assert.equal(eventSubscriber.synced, true); mockControl.sendTo = async function() { - throw new lib.PermissionDeniedError("Permission denied"); + throw new lib.PermissionError("Permission denied"); }; eventSubscriber.handleAccountEvent(); await new Promise(r => setImmediate(r)); assert.equal(eventSubscriber.synced, false); assert.deepEqual(calledWith, [null, false]); - assert.ok(eventSubscriber.error instanceof lib.PermissionDeniedError); + assert.ok(eventSubscriber.error instanceof lib.PermissionError); }); it("should expose errors through the snapshot", async function() { eventSubscriber.control.sendTo = function() { - throw new lib.PermissionDeniedError("Permission denied"); + throw new lib.PermissionError("Permission denied"); }; const unsubscribe = eventSubscriber.subscribe(() => {}); await new Promise(r => setImmediate(r)); const [,, error1] = eventSubscriber.getSnapshot(); - assert.ok(error1 instanceof lib.PermissionDeniedError); + assert.ok(error1 instanceof lib.PermissionError); assert.equal(error1.message, "Permission denied"); - assert.equal(lib.Link.errorName(error1), "PermissionDeniedError"); + assert.equal(lib.Link.errorName(error1), "PermissionError"); eventSubscriber.control.sendTo = function() {}; unsubscribe(); diff --git a/test/messages/note.test.js b/test/messages/note.test.js index 96cbf5cb..d09faa53 100644 --- a/test/messages/note.test.js +++ b/test/messages/note.test.js @@ -83,11 +83,11 @@ describe("messages/note", function() { it("rejects an empty resource type or id", async function() { await assert.rejects( controlConnection.handleNoteSetRequest(new lib.NoteSetRequest("", "1", "text")), - new lib.RequestError("resourceType and resourceId must not be empty"), + new lib.ValidationError("resourceType and resourceId must not be empty"), ); await assert.rejects( controlConnection.handleNoteSetRequest(new lib.NoteSetRequest("host", "", "text")), - new lib.RequestError("resourceType and resourceId must not be empty"), + new lib.ValidationError("resourceType and resourceId must not be empty"), ); }); });