From d05a529c52d3459da34ad28c47ae967926841a36 Mon Sep 17 00:00:00 2001 From: Cooldude2606 <25043174+Cooldude2606@users.noreply.github.com> Date: Tue, 3 Mar 2026 22:22:59 +0000 Subject: [PATCH 01/20] Improve config field definition type --- packages/controller/controller.ts | 2 +- packages/lib/src/config/classes.ts | 31 ++++++++++++++++++++------ packages/lib/src/config/definitions.ts | 3 +-- 3 files changed, 26 insertions(+), 10 deletions(-) diff --git a/packages/controller/controller.ts b/packages/controller/controller.ts index 85b20875..16b2d343 100644 --- a/packages/controller/controller.ts +++ b/packages/controller/controller.ts @@ -387,7 +387,7 @@ async function initialize(): Promise { controllerConfig.set("controller.version", version); // Allows tracking last loaded version - if (!controllerConfig.get("controller.auth_secret")) { + if (controllerConfig.get("controller.auth_secret") === "") { logger.info("Generating new controller authentication secret"); let asyncRandomBytes = util.promisify(crypto.randomBytes); let bytes = await asyncRandomBytes(256); diff --git a/packages/lib/src/config/classes.ts b/packages/lib/src/config/classes.ts index 464dd943..cbd15a73 100644 --- a/packages/lib/src/config/classes.ts +++ b/packages/lib/src/config/classes.ts @@ -63,14 +63,24 @@ export const ConfigSchema = Type.Record(Type.String(), Type.Union([ ])); export type ConfigSchema = Static; -type FieldType = "boolean" | "string" | "number" | "object"; type FieldValue = null | boolean | string | number | object; -export interface FieldDefinition { + +type FieldType = + T extends boolean ? "boolean" : + T extends string ? "string" : + T extends number ? "number" : + T extends object ? "object" : + never; + +export interface FieldDefinition< + Type extends FieldValue = any, + Fields extends Record = any, +> { /** * string declaring the type of the config value, supports boolean, * string, number and object. */ - type: FieldType; + type: FieldType; /** * Text used to identify the config field in user interfaces. */ @@ -79,6 +89,11 @@ export interface FieldDefinition { * Text used to describe the config field in user interfaces. */ description?: string; + /** + * Validation function ran to confirm the value is consistent with other config values. + * Should throw an error if the value in invalid / inconsistent + */ + extendedValidation?: (value: Type, config: Config) => void; /** * Value to use for the autocomplete attribute in the Web UI's input element. * Useful for making browsers behave properly with credential inputs. @@ -93,7 +108,7 @@ export interface FieldDefinition { /** * True if this field can be set to null. Defaults to false. */ - optional?: boolean; + optional?: Type extends null ? true : false; /** * Value this config field should take in a newly initialized config. * This can also be an function returning the value to use. @@ -137,8 +152,10 @@ export interface FieldDefinition { hidden?: boolean; } -export type ConfigDefs = { - [Prop in keyof Fields]: FieldDefinition; +export type ConfigDefs< + Fields extends Record +> = { + [Prop in keyof Fields]: FieldDefinition; } function defaultValue(def: FieldDefinition) { @@ -177,7 +194,7 @@ export enum ConfigAccess { * @extends events.EventEmitter */ export class Config< - Fields extends { [Field in keyof Fields]: FieldValue }, + Fields extends Record > extends events.EventEmitter { /** * Mapping of config name to field meta data diff --git a/packages/lib/src/config/definitions.ts b/packages/lib/src/config/definitions.ts index 18d30609..21eea55c 100644 --- a/packages/lib/src/config/definitions.ts +++ b/packages/lib/src/config/definitions.ts @@ -144,7 +144,7 @@ export class ControllerConfig extends classes.Config { "Do not share this.", restartRequired: true, type: "string", - optional: true, + initialValue: "", }, "controller.heartbeat_interval": { title: "Heartbeat Interval", @@ -495,7 +495,6 @@ export class InstanceConfig extends classes.Config { "the process. Set to 0 to disable.", type: "number", initialValue: 300, - optional: true, }, "factorio.game_port": { description: "UDP port to run game on, uses a port in host.factorio_port_range if null", From d0604f892b114079e1a6f4f3bfb9def32750df47 Mon Sep 17 00:00:00 2001 From: Cooldude2606 <25043174+Cooldude2606@users.noreply.github.com> Date: Tue, 3 Mar 2026 23:55:23 +0000 Subject: [PATCH 02/20] Add simple validators to config --- packages/lib/src/config/classes.ts | 11 ++++- packages/lib/src/config/definitions.ts | 61 ++++++++++++++++++++++++- packages/lib/src/config/validators.ts | 62 ++++++++++++++++++++++++++ 3 files changed, 131 insertions(+), 3 deletions(-) create mode 100644 packages/lib/src/config/validators.ts diff --git a/packages/lib/src/config/classes.ts b/packages/lib/src/config/classes.ts index cbd15a73..2fc93982 100644 --- a/packages/lib/src/config/classes.ts +++ b/packages/lib/src/config/classes.ts @@ -530,12 +530,13 @@ export class Config< if (!def) { throw new InvalidField(`No field named '${name}'`); } + this._checkAccess(name, def, remote, ConfigAccess.write, true); if (remote !== this.location) { this._checkAccess(name, def, this.location, ConfigAccess.write, true); } - // Empty strings are treates as null + // Empty strings are treat as null if (value === "") { value = null; } @@ -576,6 +577,14 @@ export class Config< throw new InvalidValue(`Expected type of ${name} to be ${def.type}, not ${basicType(value)}`); } + if (def.extendedValidation) { + try { + def.extendedValidation(value, this); + } catch (err: any) { + throw new InvalidValue(`Failed validation of ${name}: ${err.message}`); + } + } + this._set(name, value); } diff --git a/packages/lib/src/config/definitions.ts b/packages/lib/src/config/definitions.ts index 21eea55c..798f90b3 100644 --- a/packages/lib/src/config/definitions.ts +++ b/packages/lib/src/config/definitions.ts @@ -1,7 +1,9 @@ // Core definitions for the configuration system import * as classes from "./classes"; +import * as validate from "./validators"; import type { PluginNodeEnvInfo, PluginWebpackEnvInfo } from "../plugin"; import { Static } from "@sinclair/typebox"; +import { isTargetVersion } from "../data"; type configFromJSON = (...args: Parameters) => T; type configFromFile = (...args: Parameters) => Promise; @@ -73,18 +75,21 @@ export class ControllerConfig extends classes.Config { restartRequired: true, type: "string", initialValue: "mods", + extendedValidation: validate.filePath, }, "controller.database_directory": { title: "Database directory", description: "Directory where item and configuration data is stored.", type: "string", initialValue: "database", + extendedValidation: validate.filePath, }, "controller.autosave_interval": { title: "Autosave Interval", description: "Interval in seconds to autosave data in memory to disk.", type: "number", initialValue: 60, + extendedValidation: validate.greaterThanZero, }, "controller.http_port": { title: "HTTP Port", @@ -93,6 +98,10 @@ export class ControllerConfig extends classes.Config { type: "number", optional: true, initialValue: 8080, + extendedValidation: validate.optional(validate.all( + validate.integer, + validate.greaterThanZero + )), }, "controller.https_port": { title: "HTTPS Port", @@ -100,6 +109,10 @@ export class ControllerConfig extends classes.Config { restartRequired: true, type: "number", optional: true, + extendedValidation: validate.optional(validate.all( + validate.integer, + validate.greaterThanZero + )), }, "controller.bind_address": { title: "Bind Address", @@ -107,6 +120,7 @@ export class ControllerConfig extends classes.Config { restartRequired: true, type: "string", optional: true, + // TODO extendedValidation, valid addresses including IPV4, IPV6, and localhost }, "controller.trusted_proxies": { title: "Trusted Proxies", @@ -114,12 +128,14 @@ export class ControllerConfig extends classes.Config { "Comma separated list of IP addresses and/or CIDR blocks to trust the X-Forwarded-For header on", type: "string", optional: true, + // TODO extendedValidation, see parseTrustedProxies }, "controller.public_url": { title: "Public URL", description: "Public facing URL the controller is hosted on, including the protocol.", type: "string", optional: true, + // TODO extendedValidation, valid url including the protocol }, "controller.tls_certificate": { title: "TLS Certificate", @@ -127,6 +143,7 @@ export class ControllerConfig extends classes.Config { restartRequired: true, type: "string", optional: true, + extendedValidation: validate.optional(validate.filePath), }, "controller.tls_private_key": { title: "TLS Private Key", @@ -134,6 +151,7 @@ export class ControllerConfig extends classes.Config { restartRequired: true, type: "string", optional: true, + extendedValidation: validate.optional(validate.filePath), }, "controller.auth_secret": { access: ["controller"], @@ -151,30 +169,35 @@ export class ControllerConfig extends classes.Config { description: "Interval heartbeats are sent out on WebSocket connections.", type: "number", initialValue: 15, + extendedValidation: validate.greaterThanZero, }, "controller.session_timeout": { title: "Session Timeout", description: "Time in seconds before giving up resuming a dropped WebSocket session.", type: "number", initialValue: 60, + extendedValidation: validate.greaterThanZero, }, "controller.metrics_timeout": { title: "Metrics Timeout", description: "Timeout in seconds for metrics gathering from hosts.", type: "number", initialValue: 8, + extendedValidation: validate.greaterThanZero, }, "controller.system_metrics_interval": { title: "System Metrics Interval", description: "Interval in seconds to collect and update system metrics for the Web UI", type: "number", initialValue: 10, + extendedValidation: validate.greaterThanZero, }, "controller.proxy_stream_timeout": { title: "Proxy Stream Timeout", description: "Timeout in seconds for proxy streams to start flowing.", type: "number", initialValue: 15, + extendedValidation: validate.greaterThanZero, }, "controller.factorio_username": { title: "Factorio Username", @@ -190,6 +213,7 @@ export class ControllerConfig extends classes.Config { type: "string", credential: ["controller"], optional: true, + // TODO extendedValidation, lowercase hex characters, maybe length? cannot be set without username }, "controller.share_factorio_credentials_with_hosts": { title: "Share Factorio credentials with Hosts", @@ -219,12 +243,14 @@ export class ControllerConfig extends classes.Config { description: "Duration in minutes to cache mod portal API responses.", type: "number", initialValue: 30, + extendedValidation: validate.greaterThanEqualZero, // Zero is allowed here to disable cache }, "controller.mod_portal_page_size": { title: "Mod Portal Page Size", description: "Maximum number of results per page when querying the Factorio mod portal API.", type: "number", initialValue: 1000, + extendedValidation: validate.greaterThanZero, }, "controller.allow_remote_updates": { description: "When true, allows a remote event to trigger a clusterio update via npm", @@ -300,6 +326,7 @@ export class HostConfig extends classes.Config { description: "Path to directory to look for factorio installs", type: "string", initialValue: "factorio", + extendedValidation: validate.filePath, }, "host.mods_directory": { title: "Mods Directory", @@ -307,18 +334,21 @@ export class HostConfig extends classes.Config { restartRequired: true, type: "string", initialValue: "mods", + extendedValidation: validate.filePath, }, "host.instances_directory": { description: "Path to directory to store instances in.", restartRequired: true, type: "string", initialValue: "instances", + extendedValidation: validate.filePath, }, "host.controller_url": { description: "URL to connect to the controller at", restartRequired: true, type: "string", initialValue: "http://localhost:8080/", + // TODO extendedValidation, valid url including the protocol }, "host.controller_token": { access: ["host"], @@ -326,12 +356,14 @@ export class HostConfig extends classes.Config { restartRequired: true, type: "string", initialValue: "enter token here", + // TODO extendedValidation, valid JWT with matching host id }, "host.tls_ca": { description: "Path to Certificate Authority to validate TLS connection to controller against.", restartRequired: true, type: "string", optional: true, + extendedValidation: validate.optional(validate.filePath), }, "host.public_address": { description: "Public facing address players should connect to in order to join instances on this host", @@ -345,6 +377,7 @@ export class HostConfig extends classes.Config { "ranges separated with a dash.", type: "string", initialValue: "34100-34199", + // TODO extendedValidation, port range }, "host.factorio_username": { title: "Factorio Username", @@ -364,12 +397,14 @@ export class HostConfig extends classes.Config { type: "string", credential: ["host"], optional: true, + // TODO extendedValidation, lowercase hex characters, maybe length? cannot be set without username }, "host.max_reconnect_delay": { title: "Max Reconnect Delay", description: "Maximum delay to wait before attempting to reconnect WebSocket", type: "number", initialValue: 60, + extendedValidation: validate.greaterThanZero, }, "host.allow_remote_updates": { description: "When true, allows a remote event to trigger a clusterio update via npm", @@ -463,6 +498,7 @@ export class InstanceConfig extends classes.Config { type: "number", optional: true, hidden: true, + // Internal value, extendedValidation not used }, "instance.auto_start": { description: "Automatically start this instance when the host hosting it is started up", @@ -480,6 +516,11 @@ export class InstanceConfig extends classes.Config { type: "string", initialValue: "latest", inputComponent: "target_version", + extendedValidation: function(value) { + if (!isTargetVersion(value)) { + throw new Error("Value must be be 'latest', or match X.Y, or match X.Y.Z"); + } + }, }, "factorio.executable_path": { description: @@ -488,6 +529,7 @@ export class InstanceConfig extends classes.Config { restartRequired: true, type: "string", optional: true, + extendedValidation: validate.optional(validate.filePath), }, "factorio.shutdown_timeout": { description: @@ -495,23 +537,27 @@ export class InstanceConfig extends classes.Config { "the process. Set to 0 to disable.", type: "number", initialValue: 300, + extendedValidation: validate.greaterThanEqualZero, }, "factorio.game_port": { description: "UDP port to run game on, uses a port in host.factorio_port_range if null", restartRequired: true, type: "number", optional: true, + extendedValidation: validate.optional(validate.greaterThanZero), }, "factorio.host_assigned_game_port": { access: ["host"], type: "number", optional: true, + // Internal value, extendedValidation not used }, "factorio.rcon_port": { description: "TCP port to run RCON on, uses a random port if null", restartRequired: true, type: "number", optional: true, + extendedValidation: validate.optional(validate.greaterThanZero), }, "factorio.rcon_password": { credential: ["host", "controller"], @@ -526,6 +572,13 @@ export class InstanceConfig extends classes.Config { "autosave pool with this many slots. Requires autosaves to be enabled to work. Set to 0 to disable.", type: "number", initialValue: 5, + extendedValidation: function(value, config) { + const factorioSettings = config.get("factorio.settings"); + const autosaveSlots = factorioSettings.autosave_slot; + if (typeof autosaveSlots === "number" && value < autosaveSlots) { + throw new Error("Value cannot be less than the number of autosave slots"); + } + }, }, "factorio.mod_pack_id": { title: "Mod Pack", @@ -574,6 +627,7 @@ export class InstanceConfig extends classes.Config { ], type: "object", initialValue: {}, // See create instance handler in controller. + // TODO extendedValidation, we could check keys and types here if we wanted }, "factorio.verbose_logging": { description: "Enable verbose logging on the Factorio server", @@ -616,6 +670,7 @@ export class InstanceConfig extends classes.Config { restartRequired: true, type: "number", initialValue: 5, + extendedValidation: validate.greaterThanZero, }, }; } @@ -650,18 +705,20 @@ export class ControlConfig extends classes.Config { description: "Path to Certificate Authority to validate TLS connection to controller against.", type: "string", optional: true, + extendedValidation: validate.optional(validate.filePath), }, "control.max_reconnect_delay": { title: "Max Reconnect Delay", description: "Maximum delay to wait before attempting to reconnect WebSocket", type: "number", initialValue: 60, + extendedValidation: validate.greaterThanEqualZero, }, }; } -function validateFields( +function validateFieldNames( pluginName: string, fields: Record, ) { @@ -694,7 +751,7 @@ export function addPluginConfigFields(pluginInfos: PluginNodeEnvInfo[] | PluginW const fields = pluginInfo[kind]; if (fields) { - validateFields(pluginInfo.name, fields); + validateFieldNames(pluginInfo.name, fields); Object.assign(Config.fieldDefinitions, fields); } } diff --git a/packages/lib/src/config/validators.ts b/packages/lib/src/config/validators.ts new file mode 100644 index 00000000..d4f1dabd --- /dev/null +++ b/packages/lib/src/config/validators.ts @@ -0,0 +1,62 @@ +import path from "node:path"; + +/** Combine multiple validators into a single one. */ +export function all ( + ...validators: ((value: V, config: C) => void)[] +) { + return function (value: V, config: C) { + for (const validator of validators) { + validator(value, config); + } + }; +} + +/** Skip validation of optional fields when null */ +export function optional ( + validator: (value: V, config: C) => void +) { + return function (value: V | null, config: C) { + if (value === null) { return; } + validator(value, config); + }; +} + +/** Value must be greater than the minimum value */ +export function greaterThan(min: number) { + return function (value: number) { + if (value <= min) { + throw new Error(`Value must be greater than ${min}`); + } + }; +} + +/** Value must be greater than 0 */ +export const greaterThanZero = greaterThan(0); + +/** Value must be greater than or equal to the minimum value */ +export function greaterThanEqual(min: number) { + return function (value: number) { + if (value < min) { + throw new Error(`Value must be greater than ${min}`); + } + }; +} + +/** Value must be greater than or equal to 0 */ +export const greaterThanEqualZero = greaterThanEqual(0); + +/** Value must be an integer value */ +export function integer(value: number) { + if (!Number.isInteger(value)) { + throw new Error("Value must be an integer"); + } +} + +/** Value must be a valid file path, it does not need to exist */ +export function filePath(value: string) { + try { + path.resolve(value); + } catch (err: any) { + throw new Error(`Value must be a valid path: ${err.message}`); + } +} From b2daf26e6cb775ed2a82296e194faba80b688bfc Mon Sep 17 00:00:00 2001 From: Cooldude2606 <25043174+Cooldude2606@users.noreply.github.com> Date: Wed, 4 Mar 2026 00:28:29 +0000 Subject: [PATCH 03/20] Add warnings to web ui --- packages/lib/src/config/definitions.ts | 4 ++-- packages/lib/src/config/validators.ts | 2 +- packages/web_ui/src/components/BaseConfigTree.tsx | 12 ++++++++++++ 3 files changed, 15 insertions(+), 3 deletions(-) diff --git a/packages/lib/src/config/definitions.ts b/packages/lib/src/config/definitions.ts index 798f90b3..dc4ca4d9 100644 --- a/packages/lib/src/config/definitions.ts +++ b/packages/lib/src/config/definitions.ts @@ -3,7 +3,7 @@ import * as classes from "./classes"; import * as validate from "./validators"; import type { PluginNodeEnvInfo, PluginWebpackEnvInfo } from "../plugin"; import { Static } from "@sinclair/typebox"; -import { isTargetVersion } from "../data"; +import { isTargetVersion } from "../data/version"; type configFromJSON = (...args: Parameters) => T; type configFromFile = (...args: Parameters) => Promise; @@ -574,7 +574,7 @@ export class InstanceConfig extends classes.Config { initialValue: 5, extendedValidation: function(value, config) { const factorioSettings = config.get("factorio.settings"); - const autosaveSlots = factorioSettings.autosave_slot; + const autosaveSlots = factorioSettings.autosave_slots; if (typeof autosaveSlots === "number" && value < autosaveSlots) { throw new Error("Value cannot be less than the number of autosave slots"); } diff --git a/packages/lib/src/config/validators.ts b/packages/lib/src/config/validators.ts index d4f1dabd..fa26c7c1 100644 --- a/packages/lib/src/config/validators.ts +++ b/packages/lib/src/config/validators.ts @@ -1,4 +1,4 @@ -import path from "node:path"; +import path from "path"; /** Combine multiple validators into a single one. */ export function all ( diff --git a/packages/web_ui/src/components/BaseConfigTree.tsx b/packages/web_ui/src/components/BaseConfigTree.tsx index 07e3c6ab..dc9e94c5 100644 --- a/packages/web_ui/src/components/BaseConfigTree.tsx +++ b/packages/web_ui/src/components/BaseConfigTree.tsx @@ -391,6 +391,18 @@ function computeTreeData( help={errorFields.get(fieldName)} tooltip={def.description} valuePropName={def.type === "boolean" ? "checked" : "value"} + rules={[{ + validator: function(_, fieldValue) { + if (def.extendedValidation) { + try { + def.extendedValidation(fieldValue, config); + } catch (err: any) { + return Promise.reject(err); + } + } + return Promise.resolve(); + }, + }]} > {renderInput(control.inputComponents, def, !canWrite)} ; From 687a384fe4905c3e8320a1e8b105427122150d98 Mon Sep 17 00:00:00 2001 From: Cooldude2606 <25043174+Cooldude2606@users.noreply.github.com> Date: Wed, 4 Mar 2026 14:34:09 +0000 Subject: [PATCH 04/20] Use an async arrow function for webui rules Co-authored-by: Hornwitser --- packages/web_ui/src/components/BaseConfigTree.tsx | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/packages/web_ui/src/components/BaseConfigTree.tsx b/packages/web_ui/src/components/BaseConfigTree.tsx index dc9e94c5..73c2379b 100644 --- a/packages/web_ui/src/components/BaseConfigTree.tsx +++ b/packages/web_ui/src/components/BaseConfigTree.tsx @@ -392,15 +392,10 @@ function computeTreeData( tooltip={def.description} valuePropName={def.type === "boolean" ? "checked" : "value"} rules={[{ - validator: function(_, fieldValue) { + validator: async (_, fieldValue) => { if (def.extendedValidation) { - try { - def.extendedValidation(fieldValue, config); - } catch (err: any) { - return Promise.reject(err); - } + def.extendedValidation(fieldValue, config); } - return Promise.resolve(); }, }]} > From 014b18caf87f57f13065f282e79b5df9300cbd39 Mon Sep 17 00:00:00 2001 From: Cooldude2606 <25043174+Cooldude2606@users.noreply.github.com> Date: Wed, 4 Mar 2026 21:02:39 +0000 Subject: [PATCH 05/20] Fix type of controller.auth_secret --- packages/controller/controller.ts | 5 +++-- packages/controller/src/Controller.ts | 10 +++++---- packages/controller/src/UserManager.ts | 3 ++- packages/controller/src/WsServer.ts | 28 +++++--------------------- packages/controller/src/routes.ts | 14 ++----------- packages/lib/src/config/classes.ts | 2 +- packages/lib/src/config/definitions.ts | 4 ++-- 7 files changed, 21 insertions(+), 45 deletions(-) diff --git a/packages/controller/controller.ts b/packages/controller/controller.ts index 16b2d343..95c6ec93 100644 --- a/packages/controller/controller.ts +++ b/packages/controller/controller.ts @@ -182,7 +182,8 @@ async function handleBootstrapCommand( // eslint-disable-next-line no-console console.log(jwt.sign( { aud: "host", host: args.id }, - Buffer.from(controllerConfig.get("controller.auth_secret"), "base64") + // Auth secret is never null after startup + Buffer.from(controllerConfig.get("controller.auth_secret")!, "base64") )); } else if (subCommand === "create-ctl-config") { @@ -387,7 +388,7 @@ async function initialize(): Promise { controllerConfig.set("controller.version", version); // Allows tracking last loaded version - if (controllerConfig.get("controller.auth_secret") === "") { + if (!controllerConfig.get("controller.auth_secret")) { logger.info("Generating new controller authentication secret"); let asyncRandomBytes = util.promisify(crypto.randomBytes); let bytes = await asyncRandomBytes(256); diff --git a/packages/controller/src/Controller.ts b/packages/controller/src/Controller.ts index 7ad03e2c..628073cc 100644 --- a/packages/controller/src/Controller.ts +++ b/packages/controller/src/Controller.ts @@ -261,6 +261,11 @@ export default class Controller { this.users.records.on("update", this.usersUpdated.bind(this)); } + get authSecret() { + // Auth secret is never null after startup + return Buffer.from(this.config.get("controller.auth_secret")!, "base64"); + } + async start(args: ControllerArgs) { if (this._state !== "new") { throw new Error(`Cannot start in state ${this._state}`); @@ -876,10 +881,7 @@ export default class Controller { * @param hostId - ID of host to generate a token for. */ generateHostToken(hostId: number): string { - return jwt.sign( - { aud: "host", host: hostId }, - Buffer.from(this.config.get("controller.auth_secret"), "base64") - ); + return jwt.sign({ aud: "host", host: hostId }, this.authSecret); } hostsUpdated(hosts: HostRecord[]) { diff --git a/packages/controller/src/UserManager.ts b/packages/controller/src/UserManager.ts index 2c727f30..d1304d14 100644 --- a/packages/controller/src/UserManager.ts +++ b/packages/controller/src/UserManager.ts @@ -151,7 +151,8 @@ export default class UserManager { signUserToken(user: UserRecord): string { return jwt.sign( { aud: "user", user: user.id }, - Buffer.from(this._controllerConfig.get("controller.auth_secret"), "base64") + // auth secret is never null after startup + Buffer.from(this._controllerConfig.get("controller.auth_secret")!, "base64") ); } diff --git a/packages/controller/src/WsServer.ts b/packages/controller/src/WsServer.ts index 819f8970..c2b33c81 100644 --- a/packages/controller/src/WsServer.ts +++ b/packages/controller/src/WsServer.ts @@ -223,10 +223,7 @@ ${err.stack}` createSession(dst: lib.Address): [ WsServerConnector, string ] { let sessionId = this.nextSessionId; this.nextSessionId += 1; - let sessionToken = jwt.sign( - { aud: this.sessionAud, sid: sessionId }, - Buffer.from(this.controller.config.get("controller.auth_secret"), "base64"), - ); + let sessionToken = jwt.sign({ aud: this.sessionAud, sid: sessionId }, this.controller.authSecret); let sessionTimeout = this.controller.config.get("controller.session_timeout"); let heartbeatInterval = this.controller.config.get("controller.heartbeat_interval"); let connector = new WsServerConnector(dst, sessionId, sessionTimeout, heartbeatInterval); @@ -247,13 +244,8 @@ ${err.stack}` } try { - let tokenPayload = jwt.verify( - data.token, - Buffer.from(this.controller.config.get("controller.auth_secret"), "base64"), - // migrate: allow pre-rename tokens issued to hosts before alpha-14 - { audience: ["host", "slave"] } - ); - + // migrate: allow pre-rename tokens issued to hosts before alpha-14 + const tokenPayload = jwt.verify(data.token, this.controller.authSecret, { audience: ["host", "slave"] }); if (typeof tokenPayload === "string") { throw new Error("unexpected JsonWebToken type"); } @@ -324,12 +316,7 @@ ${err.stack}` async registerControl(data: lib.RegisterControlData, socket: WebSocket, req: IncomingMessage) { let user; try { - let tokenPayload = jwt.verify( - data.token, - Buffer.from(this.controller.config.get("controller.auth_secret"), "base64"), - { audience: "user" } - ); - + const tokenPayload = jwt.verify(data.token, this.controller.authSecret, { audience: "user" }); if (typeof tokenPayload === "string") { throw new Error("unexpected JsonWebToken type"); } @@ -376,12 +363,7 @@ ${err.stack}` resumeSession(data: lib.ResumeData, socket: WebSocket, req: IncomingMessage) { let connector; try { - let payload = jwt.verify( - data.sessionToken, - Buffer.from(this.controller.config.get("controller.auth_secret"), "base64"), - { audience: this.sessionAud } - ); - + const payload = jwt.verify(data.sessionToken, this.controller.authSecret, { audience: this.sessionAud }); if (typeof payload === "string") { throw new Error("unexpected JsonWebToken type"); } diff --git a/packages/controller/src/routes.ts b/packages/controller/src/routes.ts index a47ceb2d..6b578dbd 100644 --- a/packages/controller/src/routes.ts +++ b/packages/controller/src/routes.ts @@ -148,12 +148,7 @@ function validateHostToken(req: Request, res: Response, next: any) { } try { - const tokenPayload = jwt.verify( - token, - Buffer.from(req.app.locals.controller.config.get("controller.auth_secret"), "base64"), - { audience: "host" } - ); - + const tokenPayload = jwt.verify(token, req.app.locals.controller.authSecret, { audience: "host" }); if (typeof tokenPayload === "string") { throw new Error("unexpected JsonWebToken type"); } @@ -180,12 +175,7 @@ function validateUserToken(req: Request, res: Response, next: any) { } try { - let tokenPayload = jwt.verify( - token, - Buffer.from(req.app.locals.controller.config.get("controller.auth_secret"), "base64"), - { audience: "user" } - ); - + const tokenPayload = jwt.verify(token, req.app.locals.controller.authSecret, { audience: "user" }); if (typeof tokenPayload === "string") { throw new Error("unexpected JsonWebToken type"); } diff --git a/packages/lib/src/config/classes.ts b/packages/lib/src/config/classes.ts index 2fc93982..0583342f 100644 --- a/packages/lib/src/config/classes.ts +++ b/packages/lib/src/config/classes.ts @@ -536,7 +536,7 @@ export class Config< this._checkAccess(name, def, this.location, ConfigAccess.write, true); } - // Empty strings are treat as null + // Empty strings are treated as null if (value === "") { value = null; } diff --git a/packages/lib/src/config/definitions.ts b/packages/lib/src/config/definitions.ts index dc4ca4d9..2461c76f 100644 --- a/packages/lib/src/config/definitions.ts +++ b/packages/lib/src/config/definitions.ts @@ -20,7 +20,7 @@ export interface ControllerConfigFields { "controller.public_url": string | null; "controller.tls_certificate": string | null; "controller.tls_private_key": string | null; - "controller.auth_secret": string; + "controller.auth_secret": string | null; "controller.heartbeat_interval": number; "controller.session_timeout": number; "controller.metrics_timeout": number; @@ -162,7 +162,7 @@ export class ControllerConfig extends classes.Config { "Do not share this.", restartRequired: true, type: "string", - initialValue: "", + optional: true, }, "controller.heartbeat_interval": { title: "Heartbeat Interval", From 61232e3537d6cd9903eb8ec55a830f60cd8a99b2 Mon Sep 17 00:00:00 2001 From: Cooldude2606 <25043174+Cooldude2606@users.noreply.github.com> Date: Wed, 4 Mar 2026 21:07:30 +0000 Subject: [PATCH 06/20] Export validators on Config class as static --- packages/lib/src/config/classes.ts | 4 ++ packages/lib/src/config/definitions.ts | 62 +++++++++++++------------- 2 files changed, 35 insertions(+), 31 deletions(-) diff --git a/packages/lib/src/config/classes.ts b/packages/lib/src/config/classes.ts index 0583342f..1a366e5d 100644 --- a/packages/lib/src/config/classes.ts +++ b/packages/lib/src/config/classes.ts @@ -8,6 +8,7 @@ import { basicType } from "../helpers"; import * as libSchema from "../schema"; import { StringEnum } from "../data/composites"; import { safeOutputFile } from "../file_ops"; +import * as validators from "./validators"; const ConfigLocation = StringEnum(["controller", "host", "control"]); export type ConfigLocation = Static; @@ -203,6 +204,9 @@ export class Config< declare static fieldDefinitions: ConfigDefs; declare ["constructor"]: typeof Config; + /** A set of common validators and validator factories */ + static validators = validators; + /** * Handle migration between clusterio versions * diff --git a/packages/lib/src/config/definitions.ts b/packages/lib/src/config/definitions.ts index 2461c76f..dc8f80bd 100644 --- a/packages/lib/src/config/definitions.ts +++ b/packages/lib/src/config/definitions.ts @@ -1,6 +1,6 @@ // Core definitions for the configuration system import * as classes from "./classes"; -import * as validate from "./validators"; +import * as validators from "./validators"; import type { PluginNodeEnvInfo, PluginWebpackEnvInfo } from "../plugin"; import { Static } from "@sinclair/typebox"; import { isTargetVersion } from "../data/version"; @@ -75,21 +75,21 @@ export class ControllerConfig extends classes.Config { restartRequired: true, type: "string", initialValue: "mods", - extendedValidation: validate.filePath, + extendedValidation: validators.filePath, }, "controller.database_directory": { title: "Database directory", description: "Directory where item and configuration data is stored.", type: "string", initialValue: "database", - extendedValidation: validate.filePath, + extendedValidation: validators.filePath, }, "controller.autosave_interval": { title: "Autosave Interval", description: "Interval in seconds to autosave data in memory to disk.", type: "number", initialValue: 60, - extendedValidation: validate.greaterThanZero, + extendedValidation: validators.greaterThanZero, }, "controller.http_port": { title: "HTTP Port", @@ -98,9 +98,9 @@ export class ControllerConfig extends classes.Config { type: "number", optional: true, initialValue: 8080, - extendedValidation: validate.optional(validate.all( - validate.integer, - validate.greaterThanZero + extendedValidation: validators.optional(validators.all( + validators.integer, + validators.greaterThanZero )), }, "controller.https_port": { @@ -109,9 +109,9 @@ export class ControllerConfig extends classes.Config { restartRequired: true, type: "number", optional: true, - extendedValidation: validate.optional(validate.all( - validate.integer, - validate.greaterThanZero + extendedValidation: validators.optional(validators.all( + validators.integer, + validators.greaterThanZero )), }, "controller.bind_address": { @@ -143,7 +143,7 @@ export class ControllerConfig extends classes.Config { restartRequired: true, type: "string", optional: true, - extendedValidation: validate.optional(validate.filePath), + extendedValidation: validators.optional(validators.filePath), }, "controller.tls_private_key": { title: "TLS Private Key", @@ -151,7 +151,7 @@ export class ControllerConfig extends classes.Config { restartRequired: true, type: "string", optional: true, - extendedValidation: validate.optional(validate.filePath), + extendedValidation: validators.optional(validators.filePath), }, "controller.auth_secret": { access: ["controller"], @@ -169,35 +169,35 @@ export class ControllerConfig extends classes.Config { description: "Interval heartbeats are sent out on WebSocket connections.", type: "number", initialValue: 15, - extendedValidation: validate.greaterThanZero, + extendedValidation: validators.greaterThanZero, }, "controller.session_timeout": { title: "Session Timeout", description: "Time in seconds before giving up resuming a dropped WebSocket session.", type: "number", initialValue: 60, - extendedValidation: validate.greaterThanZero, + extendedValidation: validators.greaterThanZero, }, "controller.metrics_timeout": { title: "Metrics Timeout", description: "Timeout in seconds for metrics gathering from hosts.", type: "number", initialValue: 8, - extendedValidation: validate.greaterThanZero, + extendedValidation: validators.greaterThanZero, }, "controller.system_metrics_interval": { title: "System Metrics Interval", description: "Interval in seconds to collect and update system metrics for the Web UI", type: "number", initialValue: 10, - extendedValidation: validate.greaterThanZero, + extendedValidation: validators.greaterThanZero, }, "controller.proxy_stream_timeout": { title: "Proxy Stream Timeout", description: "Timeout in seconds for proxy streams to start flowing.", type: "number", initialValue: 15, - extendedValidation: validate.greaterThanZero, + extendedValidation: validators.greaterThanZero, }, "controller.factorio_username": { title: "Factorio Username", @@ -243,14 +243,14 @@ export class ControllerConfig extends classes.Config { description: "Duration in minutes to cache mod portal API responses.", type: "number", initialValue: 30, - extendedValidation: validate.greaterThanEqualZero, // Zero is allowed here to disable cache + extendedValidation: validators.greaterThanEqualZero, // Zero is allowed here to disable cache }, "controller.mod_portal_page_size": { title: "Mod Portal Page Size", description: "Maximum number of results per page when querying the Factorio mod portal API.", type: "number", initialValue: 1000, - extendedValidation: validate.greaterThanZero, + extendedValidation: validators.greaterThanZero, }, "controller.allow_remote_updates": { description: "When true, allows a remote event to trigger a clusterio update via npm", @@ -326,7 +326,7 @@ export class HostConfig extends classes.Config { description: "Path to directory to look for factorio installs", type: "string", initialValue: "factorio", - extendedValidation: validate.filePath, + extendedValidation: validators.filePath, }, "host.mods_directory": { title: "Mods Directory", @@ -334,14 +334,14 @@ export class HostConfig extends classes.Config { restartRequired: true, type: "string", initialValue: "mods", - extendedValidation: validate.filePath, + extendedValidation: validators.filePath, }, "host.instances_directory": { description: "Path to directory to store instances in.", restartRequired: true, type: "string", initialValue: "instances", - extendedValidation: validate.filePath, + extendedValidation: validators.filePath, }, "host.controller_url": { description: "URL to connect to the controller at", @@ -363,7 +363,7 @@ export class HostConfig extends classes.Config { restartRequired: true, type: "string", optional: true, - extendedValidation: validate.optional(validate.filePath), + extendedValidation: validators.optional(validators.filePath), }, "host.public_address": { description: "Public facing address players should connect to in order to join instances on this host", @@ -404,7 +404,7 @@ export class HostConfig extends classes.Config { description: "Maximum delay to wait before attempting to reconnect WebSocket", type: "number", initialValue: 60, - extendedValidation: validate.greaterThanZero, + extendedValidation: validators.greaterThanZero, }, "host.allow_remote_updates": { description: "When true, allows a remote event to trigger a clusterio update via npm", @@ -529,7 +529,7 @@ export class InstanceConfig extends classes.Config { restartRequired: true, type: "string", optional: true, - extendedValidation: validate.optional(validate.filePath), + extendedValidation: validators.optional(validators.filePath), }, "factorio.shutdown_timeout": { description: @@ -537,14 +537,14 @@ export class InstanceConfig extends classes.Config { "the process. Set to 0 to disable.", type: "number", initialValue: 300, - extendedValidation: validate.greaterThanEqualZero, + extendedValidation: validators.greaterThanEqualZero, }, "factorio.game_port": { description: "UDP port to run game on, uses a port in host.factorio_port_range if null", restartRequired: true, type: "number", optional: true, - extendedValidation: validate.optional(validate.greaterThanZero), + extendedValidation: validators.optional(validators.greaterThanZero), }, "factorio.host_assigned_game_port": { access: ["host"], @@ -557,7 +557,7 @@ export class InstanceConfig extends classes.Config { restartRequired: true, type: "number", optional: true, - extendedValidation: validate.optional(validate.greaterThanZero), + extendedValidation: validators.optional(validators.greaterThanZero), }, "factorio.rcon_password": { credential: ["host", "controller"], @@ -670,7 +670,7 @@ export class InstanceConfig extends classes.Config { restartRequired: true, type: "number", initialValue: 5, - extendedValidation: validate.greaterThanZero, + extendedValidation: validators.greaterThanZero, }, }; } @@ -705,14 +705,14 @@ export class ControlConfig extends classes.Config { description: "Path to Certificate Authority to validate TLS connection to controller against.", type: "string", optional: true, - extendedValidation: validate.optional(validate.filePath), + extendedValidation: validators.optional(validators.filePath), }, "control.max_reconnect_delay": { title: "Max Reconnect Delay", description: "Maximum delay to wait before attempting to reconnect WebSocket", type: "number", initialValue: 60, - extendedValidation: validate.greaterThanEqualZero, + extendedValidation: validators.greaterThanEqualZero, }, }; } From 0fa96521c5ebced24c5430b040ffa5ade65874c7 Mon Sep 17 00:00:00 2001 From: Cooldude2606 <25043174+Cooldude2606@users.noreply.github.com> Date: Wed, 4 Mar 2026 21:08:20 +0000 Subject: [PATCH 07/20] Rename extendedValidation to validator --- packages/lib/src/config/classes.ts | 6 +-- packages/lib/src/config/definitions.ts | 56 +++++++++++++------------- 2 files changed, 31 insertions(+), 31 deletions(-) diff --git a/packages/lib/src/config/classes.ts b/packages/lib/src/config/classes.ts index 1a366e5d..6c572979 100644 --- a/packages/lib/src/config/classes.ts +++ b/packages/lib/src/config/classes.ts @@ -94,7 +94,7 @@ export interface FieldDefinition< * Validation function ran to confirm the value is consistent with other config values. * Should throw an error if the value in invalid / inconsistent */ - extendedValidation?: (value: Type, config: Config) => void; + validator?: (value: Type, config: Config) => void; /** * Value to use for the autocomplete attribute in the Web UI's input element. * Useful for making browsers behave properly with credential inputs. @@ -581,9 +581,9 @@ export class Config< throw new InvalidValue(`Expected type of ${name} to be ${def.type}, not ${basicType(value)}`); } - if (def.extendedValidation) { + if (def.validator) { try { - def.extendedValidation(value, this); + def.validator(value, this); } catch (err: any) { throw new InvalidValue(`Failed validation of ${name}: ${err.message}`); } diff --git a/packages/lib/src/config/definitions.ts b/packages/lib/src/config/definitions.ts index dc8f80bd..2a560353 100644 --- a/packages/lib/src/config/definitions.ts +++ b/packages/lib/src/config/definitions.ts @@ -75,21 +75,21 @@ export class ControllerConfig extends classes.Config { restartRequired: true, type: "string", initialValue: "mods", - extendedValidation: validators.filePath, + validator: validators.filePath, }, "controller.database_directory": { title: "Database directory", description: "Directory where item and configuration data is stored.", type: "string", initialValue: "database", - extendedValidation: validators.filePath, + validator: validators.filePath, }, "controller.autosave_interval": { title: "Autosave Interval", description: "Interval in seconds to autosave data in memory to disk.", type: "number", initialValue: 60, - extendedValidation: validators.greaterThanZero, + validator: validators.greaterThanZero, }, "controller.http_port": { title: "HTTP Port", @@ -98,7 +98,7 @@ export class ControllerConfig extends classes.Config { type: "number", optional: true, initialValue: 8080, - extendedValidation: validators.optional(validators.all( + validator: validators.optional(validators.all( validators.integer, validators.greaterThanZero )), @@ -109,7 +109,7 @@ export class ControllerConfig extends classes.Config { restartRequired: true, type: "number", optional: true, - extendedValidation: validators.optional(validators.all( + validator: validators.optional(validators.all( validators.integer, validators.greaterThanZero )), @@ -143,7 +143,7 @@ export class ControllerConfig extends classes.Config { restartRequired: true, type: "string", optional: true, - extendedValidation: validators.optional(validators.filePath), + validator: validators.optional(validators.filePath), }, "controller.tls_private_key": { title: "TLS Private Key", @@ -151,7 +151,7 @@ export class ControllerConfig extends classes.Config { restartRequired: true, type: "string", optional: true, - extendedValidation: validators.optional(validators.filePath), + validator: validators.optional(validators.filePath), }, "controller.auth_secret": { access: ["controller"], @@ -169,35 +169,35 @@ export class ControllerConfig extends classes.Config { description: "Interval heartbeats are sent out on WebSocket connections.", type: "number", initialValue: 15, - extendedValidation: validators.greaterThanZero, + validator: validators.greaterThanZero, }, "controller.session_timeout": { title: "Session Timeout", description: "Time in seconds before giving up resuming a dropped WebSocket session.", type: "number", initialValue: 60, - extendedValidation: validators.greaterThanZero, + validator: validators.greaterThanZero, }, "controller.metrics_timeout": { title: "Metrics Timeout", description: "Timeout in seconds for metrics gathering from hosts.", type: "number", initialValue: 8, - extendedValidation: validators.greaterThanZero, + validator: validators.greaterThanZero, }, "controller.system_metrics_interval": { title: "System Metrics Interval", description: "Interval in seconds to collect and update system metrics for the Web UI", type: "number", initialValue: 10, - extendedValidation: validators.greaterThanZero, + validator: validators.greaterThanZero, }, "controller.proxy_stream_timeout": { title: "Proxy Stream Timeout", description: "Timeout in seconds for proxy streams to start flowing.", type: "number", initialValue: 15, - extendedValidation: validators.greaterThanZero, + validator: validators.greaterThanZero, }, "controller.factorio_username": { title: "Factorio Username", @@ -243,14 +243,14 @@ export class ControllerConfig extends classes.Config { description: "Duration in minutes to cache mod portal API responses.", type: "number", initialValue: 30, - extendedValidation: validators.greaterThanEqualZero, // Zero is allowed here to disable cache + validator: validators.greaterThanEqualZero, // Zero is allowed here to disable cache }, "controller.mod_portal_page_size": { title: "Mod Portal Page Size", description: "Maximum number of results per page when querying the Factorio mod portal API.", type: "number", initialValue: 1000, - extendedValidation: validators.greaterThanZero, + validator: validators.greaterThanZero, }, "controller.allow_remote_updates": { description: "When true, allows a remote event to trigger a clusterio update via npm", @@ -326,7 +326,7 @@ export class HostConfig extends classes.Config { description: "Path to directory to look for factorio installs", type: "string", initialValue: "factorio", - extendedValidation: validators.filePath, + validator: validators.filePath, }, "host.mods_directory": { title: "Mods Directory", @@ -334,14 +334,14 @@ export class HostConfig extends classes.Config { restartRequired: true, type: "string", initialValue: "mods", - extendedValidation: validators.filePath, + validator: validators.filePath, }, "host.instances_directory": { description: "Path to directory to store instances in.", restartRequired: true, type: "string", initialValue: "instances", - extendedValidation: validators.filePath, + validator: validators.filePath, }, "host.controller_url": { description: "URL to connect to the controller at", @@ -363,7 +363,7 @@ export class HostConfig extends classes.Config { restartRequired: true, type: "string", optional: true, - extendedValidation: validators.optional(validators.filePath), + validator: validators.optional(validators.filePath), }, "host.public_address": { description: "Public facing address players should connect to in order to join instances on this host", @@ -404,7 +404,7 @@ export class HostConfig extends classes.Config { description: "Maximum delay to wait before attempting to reconnect WebSocket", type: "number", initialValue: 60, - extendedValidation: validators.greaterThanZero, + validator: validators.greaterThanZero, }, "host.allow_remote_updates": { description: "When true, allows a remote event to trigger a clusterio update via npm", @@ -516,7 +516,7 @@ export class InstanceConfig extends classes.Config { type: "string", initialValue: "latest", inputComponent: "target_version", - extendedValidation: function(value) { + validator: function(value) { if (!isTargetVersion(value)) { throw new Error("Value must be be 'latest', or match X.Y, or match X.Y.Z"); } @@ -529,7 +529,7 @@ export class InstanceConfig extends classes.Config { restartRequired: true, type: "string", optional: true, - extendedValidation: validators.optional(validators.filePath), + validator: validators.optional(validators.filePath), }, "factorio.shutdown_timeout": { description: @@ -537,14 +537,14 @@ export class InstanceConfig extends classes.Config { "the process. Set to 0 to disable.", type: "number", initialValue: 300, - extendedValidation: validators.greaterThanEqualZero, + validator: validators.greaterThanEqualZero, }, "factorio.game_port": { description: "UDP port to run game on, uses a port in host.factorio_port_range if null", restartRequired: true, type: "number", optional: true, - extendedValidation: validators.optional(validators.greaterThanZero), + validator: validators.optional(validators.greaterThanZero), }, "factorio.host_assigned_game_port": { access: ["host"], @@ -557,7 +557,7 @@ export class InstanceConfig extends classes.Config { restartRequired: true, type: "number", optional: true, - extendedValidation: validators.optional(validators.greaterThanZero), + validator: validators.optional(validators.greaterThanZero), }, "factorio.rcon_password": { credential: ["host", "controller"], @@ -572,7 +572,7 @@ export class InstanceConfig extends classes.Config { "autosave pool with this many slots. Requires autosaves to be enabled to work. Set to 0 to disable.", type: "number", initialValue: 5, - extendedValidation: function(value, config) { + validator: function(value, config) { const factorioSettings = config.get("factorio.settings"); const autosaveSlots = factorioSettings.autosave_slots; if (typeof autosaveSlots === "number" && value < autosaveSlots) { @@ -670,7 +670,7 @@ export class InstanceConfig extends classes.Config { restartRequired: true, type: "number", initialValue: 5, - extendedValidation: validators.greaterThanZero, + validator: validators.greaterThanZero, }, }; } @@ -705,14 +705,14 @@ export class ControlConfig extends classes.Config { description: "Path to Certificate Authority to validate TLS connection to controller against.", type: "string", optional: true, - extendedValidation: validators.optional(validators.filePath), + validator: validators.optional(validators.filePath), }, "control.max_reconnect_delay": { title: "Max Reconnect Delay", description: "Maximum delay to wait before attempting to reconnect WebSocket", type: "number", initialValue: 60, - extendedValidation: validators.greaterThanEqualZero, + validator: validators.greaterThanEqualZero, }, }; } From d76902cebe7d061398993b72928a048ade54be94 Mon Sep 17 00:00:00 2001 From: Cooldude2606 <25043174+Cooldude2606@users.noreply.github.com> Date: Thu, 5 Mar 2026 00:01:55 +0000 Subject: [PATCH 08/20] Add imperfect dependency validation --- packages/lib/src/config/classes.ts | 79 +++++++++++++++++++ packages/lib/src/config/definitions.ts | 1 + .../web_ui/src/components/BaseConfigTree.tsx | 66 +++++++++++++--- plugins/player_auth/controller.ts | 9 ++- 4 files changed, 139 insertions(+), 16 deletions(-) diff --git a/packages/lib/src/config/classes.ts b/packages/lib/src/config/classes.ts index 6c572979..48fde889 100644 --- a/packages/lib/src/config/classes.ts +++ b/packages/lib/src/config/classes.ts @@ -95,6 +95,11 @@ export interface FieldDefinition< * Should throw an error if the value in invalid / inconsistent */ validator?: (value: Type, config: Config) => void; + /** + * Names the config fields that the validator depends on. + * This will ensure it is checked when these fields are changed. + */ + dependsOn?: string[]; /** * Value to use for the autocomplete attribute in the Web UI's input element. * Useful for making browsers behave properly with credential inputs. @@ -265,6 +270,46 @@ export class Config< } } + // TODO add stage, stageProp, revertStaging, and commitStaging + /** Staging is invalided when _set is called, so a new one is made with the new state */ + private _staging?: this; + get staging(): typeof this { + if (this._staging) { return this._staging; } + this._staging = new this.constructor(this.location, this.fields) as this; + return this._staging; + } + + private static _fieldDependents: Map } + }[]>; + + /** Map of all fields whose validators depend on the given field */ + static get fieldDependents(): typeof this._fieldDependents { + if (this._fieldDependents) { + return this._fieldDependents; + } + + const dependents = new Map(); + + for (const [name, def] of Object.entries(this.fieldDefinitions)) { + if (!def.validator || !def.dependsOn) { continue; } + + for (const otherName of def.dependsOn) { + const fieldDependents = dependents.get(otherName); + if (fieldDependents) { + fieldDependents.push({name, def}); + } else { + dependents.set(otherName, [{name, def}]); + } + } + } + + // The type is correct because of "!def.validator", not clear why this doesn't infer + this._fieldDependents = dependents as any; + return this._fieldDependents; + } + /** * Check access for a field * @@ -309,6 +354,7 @@ export class Config< * @param notify - False to skip setting dirty flags and emitting events */ _set(name: keyof Fields & string, value: FieldValue, notify: boolean = true) { + this._staging = undefined; const prev = this.fields[name]; this.fields[name] = value as any; if (notify && !isDeepStrictEqual(value, prev)) { @@ -589,6 +635,18 @@ export class Config< } } + const dependents = this.constructor.fieldDependents.get(name); + if (dependents) { + // this.staging.stage(name, value, remote); // TODO need stage to avoid self reference + for (const dependent of dependents) { + try { + dependent.def.validator(this.get(dependent.name as keyof Fields & string, remote), this.staging); + } catch (err: any) { + throw new InvalidValue(`Failed validation of dependent ${dependent.name}: ${err.message}`); + } + } + } + this._set(name, value); } @@ -615,6 +673,7 @@ export class Config< if (!def) { throw new InvalidField(`No field named '${name}'`); } + this._checkAccess(name, def, remote, ConfigAccess.write, true); if (remote !== this.location) { this._checkAccess(name, def, this.location, ConfigAccess.write, true); @@ -633,6 +692,26 @@ export class Config< delete updated[prop]; } + if (def.validator) { + try { + def.validator(updated, this); + } catch (err: any) { + throw new InvalidValue(`Failed validation of ${name}: ${err.message}`); + } + } + + const dependents = this.constructor.fieldDependents.get(name); + if (dependents) { + // this.staging.setProp(name, prop, value, remote); // TODO need stageProp to avoid self reference + for (const dependent of dependents) { + try { + dependent.def.validator(this.get(dependent.name as keyof Fields & string, remote), this.staging); + } catch (err: any) { + throw new InvalidValue(`Failed validation of dependent ${dependent.name}: ${err.message}`); + } + } + } + this._set(name, updated); } } diff --git a/packages/lib/src/config/definitions.ts b/packages/lib/src/config/definitions.ts index 2a560353..498c018a 100644 --- a/packages/lib/src/config/definitions.ts +++ b/packages/lib/src/config/definitions.ts @@ -572,6 +572,7 @@ export class InstanceConfig extends classes.Config { "autosave pool with this many slots. Requires autosaves to be enabled to work. Set to 0 to disable.", type: "number", initialValue: 5, + dependsOn: ["factorio.settings", "factorio.settings.autosave_slots"], validator: function(value, config) { const factorioSettings = config.get("factorio.settings"); const autosaveSlots = factorioSettings.autosave_slots; diff --git a/packages/web_ui/src/components/BaseConfigTree.tsx b/packages/web_ui/src/components/BaseConfigTree.tsx index 73c2379b..a17a11dd 100644 --- a/packages/web_ui/src/components/BaseConfigTree.tsx +++ b/packages/web_ui/src/components/BaseConfigTree.tsx @@ -161,7 +161,7 @@ export default function BaseConfigTree(props: BaseConfigTreeProps) { let value = form.getFieldValue(field); let request; if (propsMap.has(field)) { - let [fieldName, prop] = propsMap.get(field); + let [fieldName, prop] = propsMap.get(field)!; request = props.setProp(fieldName, prop, value); } else { @@ -254,7 +254,7 @@ function computeTreeData( }; let treeData = []; - let propsMap = new Map(); + const propsMap = new Map(); const groups = new Map(); for (let [fieldName, def] of Object.entries(ConfigClass.fieldDefinitions)) { const [groupName, field] = lib.splitOn(".", fieldName); @@ -312,17 +312,40 @@ function computeTreeData( {`"${prop}"`} {restart && restartTip} } - rules={[{ validator: (_, fieldValue) => { + dependencies={def.dependsOn} + rules={[({getFieldValue}) => ({ validator: async (_, fieldValue) => { if (!fieldValue.length) { - return Promise.reject(new Error("Will be removed")); + throw new Error("Will be removed"); } + try { - JSON.parse(fieldValue); + fieldValue = JSON.parse(fieldValue); } catch (err) { - return Promise.reject(new Error("Must be valid json")); + throw new Error("Must be valid json"); } - return Promise.resolve(); - }}]} + + if (!def.validator) { return; } + + // TODO this needs to use stage and stageProp to avoid deadlocks + const staging = config.staging; + staging.setProp(fieldName, prop, fieldValue); + + for (const dependsOn of def.dependsOn ?? []) { + const dependsOnDef = ConfigClass.fieldDefinitions[dependsOn as any] as typeof def; + try { + if (propsMap.has(dependsOn)) { + const [name, otherProp] = propsMap.get(dependsOn)!; + staging.setProp(name, otherProp, JSON.parse(getFieldValue(dependsOn))); + } else if (dependsOnDef.type !== "object") { + staging.set(dependsOn, getFieldValue(dependsOn)); + } + } catch (err) { + throw new Error(`Failed to fetch field: ${dependsOn}`); + } + } + + def.validator(fieldValue, staging); + }})]} > , @@ -390,14 +413,33 @@ function computeTreeData( validateStatus={errorFields.has(fieldName) ? "error" : undefined} help={errorFields.get(fieldName)} tooltip={def.description} + dependencies={def.dependsOn} valuePropName={def.type === "boolean" ? "checked" : "value"} - rules={[{ + rules={[({getFieldValue}) => ({ validator: async (_, fieldValue) => { - if (def.extendedValidation) { - def.extendedValidation(fieldValue, config); + if (!def.validator) { return; } + + // TODO this needs to use stage and stageProp to avoid deadlocks + const staging = config.staging; + staging.set(fieldName, fieldValue); + + for (const dependsOn of def.dependsOn ?? []) { + const dependsOnDef = ConfigClass.fieldDefinitions[dependsOn as any] as typeof def; + try { + if (propsMap.has(dependsOn)) { + const [name, prop] = propsMap.get(dependsOn)!; + staging.setProp(name, prop, JSON.parse(getFieldValue(dependsOn))); + } else if (dependsOnDef.type !== "object") { + staging.set(dependsOn, getFieldValue(dependsOn)); + } + } catch (err) { + throw new Error(`Failed to fetch field: ${dependsOn}`); + } } + + def.validator(fieldValue, staging); }, - }]} + })]} > {renderInput(control.inputComponents, def, !canWrite)} ; diff --git a/plugins/player_auth/controller.ts b/plugins/player_auth/controller.ts index 0fc43226..a1b51e53 100644 --- a/plugins/player_auth/controller.ts +++ b/plugins/player_auth/controller.ts @@ -90,7 +90,6 @@ export class ControllerPlugin extends BaseControllerPlugin { for (let entry of this.players.values()) { if (entry.playerCode === playerCode && entry.expiresMs > Date.now()) { let verifyCode = await generateCode(this.controller.config.get("player_auth.code_length")); - let secret = Buffer.from(this.controller.config.get("controller.auth_secret"), "base64"); let verifyToken = jwt.sign( { aud: "player_auth.verify_code", @@ -98,7 +97,7 @@ export class ControllerPlugin extends BaseControllerPlugin { verify_code: verifyCode, player_code: playerCode, }, - secret + this.controller.authSecret ); res.send({ verify_code: verifyCode, verify_token: verifyToken }); @@ -133,9 +132,11 @@ export class ControllerPlugin extends BaseControllerPlugin { return; } - let secret = Buffer.from(this.controller.config.get("controller.auth_secret"), "base64"); try { - let payload = jwt.verify(verifyToken, secret, { audience: "player_auth.verify_code" }) as jwt.JwtPayload; + let payload = jwt.verify( + verifyToken, this.controller.authSecret, { audience: "player_auth.verify_code" } + ) as jwt.JwtPayload; + if (payload.verify_code !== verifyCode) { throw new Error("invalid verify_code"); } From 6e8290cc08618a5a3d429474a5e320f030c207f3 Mon Sep 17 00:00:00 2001 From: Cooldude2606 <25043174+Cooldude2606@users.noreply.github.com> Date: Tue, 10 Mar 2026 22:56:50 +0000 Subject: [PATCH 09/20] Add full staging to config class --- packages/lib/src/config/classes.ts | 230 ++++++++++++++++++++++------- 1 file changed, 178 insertions(+), 52 deletions(-) diff --git a/packages/lib/src/config/classes.ts b/packages/lib/src/config/classes.ts index 48fde889..c5834268 100644 --- a/packages/lib/src/config/classes.ts +++ b/packages/lib/src/config/classes.ts @@ -9,6 +9,7 @@ import * as libSchema from "../schema"; import { StringEnum } from "../data/composites"; import { safeOutputFile } from "../file_ops"; import * as validators from "./validators"; +import { string } from "yargs"; const ConfigLocation = StringEnum(["controller", "host", "control"]); export type ConfigLocation = Static; @@ -270,13 +271,17 @@ export class Config< } } - // TODO add stage, stageProp, revertStaging, and commitStaging - /** Staging is invalided when _set is called, so a new one is made with the new state */ - private _staging?: this; - get staging(): typeof this { - if (this._staging) { return this._staging; } - this._staging = new this.constructor(this.location, this.fields) as this; - return this._staging; + /** A staging version of the config with changes tracked, invalided by revertStaging */ + private _staging?: { config: Config, changes: Set }; + + /** Get the latest version of staging */ + get staging(): Config { + if (this._staging) { return this._staging.config; } + const config = new this.constructor(this.location, this.fields) as this; + const changes = new Set(); + config.on("fieldChanged", name => changes.add(name as keyof Fields & string)); + this._staging = { config, changes }; + return config; } private static _fieldDependents: Map(name: string, def: T, value: unknown) { + // Empty strings are treated as null + if (value === "") { + value = null; + } + + // If def is an enum then it must be a value within it + if (def.enum && !def.enum.includes(value)) { + throw new InvalidValue(`Expected one of [${def.enum.join(", ")}], not ${value}`); + } + + // If it is a string, then attempt to coerce it + if (typeof value === "string") { + if (def.type === "boolean") { + if (value === "true") { + value = true; + } else if (value === "false") { + value = false; + } + + } else if (def.type === "number") { + const numberRegExp = /^[+-]?(Infinity|\d+\.(\d+)?([eE][+-]?\d+)?|\.?\d+([eE][+-]?\d+)?)$/; + if (numberRegExp.test(value.trim())) { + value = Number.parseFloat(value); + } + + } else if (def.type === "object") { + try { + value = JSON.parse(value); + } catch (err: any) { + throw new InvalidValue(`Error parsing value for ${name}: ${err.message}`); + } + } + } + + // Check if it is allowed to be null + if (value === null) { + if (!def.optional) { + throw new InvalidValue(`Field ${name} cannot be null`); + } + + // Check if is the correct type + } else if (basicType(value) !== def.type) { + throw new InvalidValue(`Expected type of ${name} to be ${def.type}, not ${basicType(value)}`); + } + + return value as FieldType; + } + /** * Check access for a field * @@ -354,9 +418,9 @@ export class Config< * @param notify - False to skip setting dirty flags and emitting events */ _set(name: keyof Fields & string, value: FieldValue, notify: boolean = true) { - this._staging = undefined; const prev = this.fields[name]; this.fields[name] = value as any; + this.revertStaging(); if (notify && !isDeepStrictEqual(value, prev)) { const def = this.constructor.fieldDefinitions[name]; if (def.restartRequired) { @@ -575,8 +639,7 @@ export class Config< * @throws {InvalidValue} if value is not allowed for the field. */ set(name: Field, newValue: Fields[Field], remote = this.location) { - let value: FieldValue = newValue; - let def = this.constructor.fieldDefinitions[name]; + const def = this.constructor.fieldDefinitions[name]; if (!def) { throw new InvalidField(`No field named '${name}'`); } @@ -586,46 +649,7 @@ export class Config< this._checkAccess(name, def, this.location, ConfigAccess.write, true); } - // Empty strings are treated as null - if (value === "") { - value = null; - } - - if (def.enum && !def.enum.includes(value)) { - throw new InvalidValue(`Expected one of [${def.enum.join(", ")}], not ${value}`); - } - - if (typeof value === "string") { - if (def.type === "boolean") { - if (value === "true") { - value = true; - } else if (value === "false") { - value = false; - } - - } else if (def.type === "number") { - let numberRegExp = /^[+-]?(Infinity|\d+\.(\d+)?([eE][+-]?\d+)?|\.?\d+([eE][+-]?\d+)?)$/; - if (numberRegExp.test(value.trim())) { - value = Number.parseFloat(value); - } - - } else if (def.type === "object") { - try { - value = JSON.parse(value); - } catch (err: any) { - throw new InvalidValue(`Error parsing value for ${name}: ${err.message}`); - } - } - } - - if (value === null) { - if (!def.optional) { - throw new InvalidValue(`Field ${name} cannot be null`); - } - - } else if (basicType(value) !== def.type) { - throw new InvalidValue(`Expected type of ${name} to be ${def.type}, not ${basicType(value)}`); - } + const value = Config._coerceValue(name, def, newValue); if (def.validator) { try { @@ -637,7 +661,7 @@ export class Config< const dependents = this.constructor.fieldDependents.get(name); if (dependents) { - // this.staging.stage(name, value, remote); // TODO need stage to avoid self reference + this.staging._set(name, value); for (const dependent of dependents) { try { dependent.def.validator(this.get(dependent.name as keyof Fields & string, remote), this.staging); @@ -702,7 +726,7 @@ export class Config< const dependents = this.constructor.fieldDependents.get(name); if (dependents) { - // this.staging.setProp(name, prop, value, remote); // TODO need stageProp to avoid self reference + this.staging._set(name, updated); for (const dependent of dependents) { try { dependent.def.validator(this.get(dependent.name as keyof Fields & string, remote), this.staging); @@ -714,4 +738,106 @@ export class Config< this._set(name, updated); } + + /** + * Similar to set but only validates the field type and not other constraints. + * The value is stored in staging and must be committed for the change to take effect. + * + * @param name - Name of field to set. + * @param newValue - Value to set for field. + * @param remote - Location this field is being set from. + * @throws {InvalidField} if field is not defined. + * @throws {InvalidValue} if value is not allowed for the field. + */ + stage(name: Field, newValue: Fields[Field], remote = this.location) { + const def = this.constructor.fieldDefinitions[name]; + if (!def) { + throw new InvalidField(`No field named '${name}'`); + } + + this._checkAccess(name, def, remote, ConfigAccess.write, true); + if (remote !== this.location) { + this._checkAccess(name, def, this.location, ConfigAccess.write, true); + } + + const value = Config._coerceValue(name, def, newValue); + this.staging._set(name, value); + } + + /** + * Similar to setProp but only validates the field type and not other constraints. + * The value is stored in staging and must be committed for the change to take effect. + * + * @param name - Name of field to set property on. + * @param prop - Name of property to set on field. + * @param value - the value to set the property to. + * @param remote - Location this property is being set from + * @throws {InvalidField} if field is not defined. + * @throws {InvalidValue} if field is not an object. + */ + stageProp( + name: Field, + prop: string, + value?: unknown, + remote = this.location, + ) { + const def = this.constructor.fieldDefinitions[name]; + if (!def) { + throw new InvalidField(`No field named '${name}'`); + } + + this._checkAccess(name, def, remote, ConfigAccess.write, true); + if (remote !== this.location) { + this._checkAccess(name, def, this.location, ConfigAccess.write, true); + } + + if (def.type !== "object") { + throw new InvalidValue(`Cannot set property on non-object field '${name}'`); + } + + const prev = this.fields[name] as Record; + const updated = {...prev || {}}; + + if (value !== undefined) { + updated[prop] = value; + } else { + delete updated[prop]; + } + + this.staging._set(name, updated); + } + + /** + * Commit all changes in stating after they have been validated. + * + * @throws {InvalidValue} if a value is not allowed for its field. + */ + commitStaging() { + const staging = this._staging; + if (!staging) { + return; // Nothing to commit + } + + // Validate all fields using the current state of staging + const config = staging.config; + for (const [fieldName, fieldDef] of Object.entries(this.constructor.fieldDefinitions)) { + if (fieldDef.validator) { + try { + fieldDef.validator(config.fields[fieldName as keyof Fields], config); + } catch (err: any) { + throw new InvalidValue(`Failed validation of ${fieldName}: ${err.message}`); + } + } + } + + // Validation success, set all values which have changed + for (const fieldName of staging.changes) { + this._set(fieldName, config.fields[fieldName]); + } + } + + /** Revert all changes in staging */ + revertStaging() { + this._staging = undefined; + } } From c43f752eafb409f9dd33a1b66d90709e8cbe2b73 Mon Sep 17 00:00:00 2001 From: Cooldude2606 <25043174+Cooldude2606@users.noreply.github.com> Date: Tue, 10 Mar 2026 23:16:58 +0000 Subject: [PATCH 10/20] Add ConfigSet requests --- packages/controller/src/ControlConnection.ts | 55 ++++++++++++++++++-- packages/host/src/Host.ts | 28 +++++++++- packages/lib/src/data/messages_controller.ts | 22 ++++++++ packages/lib/src/data/messages_host.ts | 22 ++++++++ packages/lib/src/data/messages_instance.ts | 24 +++++++++ packages/lib/src/link/messages.ts | 3 ++ 6 files changed, 148 insertions(+), 6 deletions(-) diff --git a/packages/controller/src/ControlConnection.ts b/packages/controller/src/ControlConnection.ts index 673a9277..5e827a89 100644 --- a/packages/controller/src/ControlConnection.ts +++ b/packages/controller/src/ControlConnection.ts @@ -73,6 +73,7 @@ export default class ControlConnection extends BaseConnection { this.handle(lib.ControllerRestartRequest, this.handleControllerRestartRequest.bind(this)); this.handle(lib.ControllerUpdateRequest, this.handleControllerUpdateRequest.bind(this)); this.handle(lib.ControllerConfigGetRequest, this.handleControllerConfigGetRequest.bind(this)); + this.handle(lib.ControllerConfigSetRequest, this.handleControllerConfigSetRequest.bind(this)); this.handle(lib.ControllerConfigSetFieldRequest, this.handleControllerConfigSetFieldRequest.bind(this)); this.handle(lib.ControllerConfigSetPropRequest, this.handleControllerConfigSetPropRequest.bind(this)); this.handle(lib.HostRevokeTokensRequest, this.handleHostRevokeTokensRequest.bind(this)); @@ -84,6 +85,7 @@ export default class ControlConnection extends BaseConnection { this.handle(lib.InstanceCreateRequest, this.handleInstanceCreateRequest.bind(this)); this.handle(lib.InstanceDeleteRequest, this.handleInstanceDeleteRequest.bind(this)); this.handle(lib.InstanceConfigGetRequest, this.handleInstanceConfigGetRequest.bind(this)); + this.handle(lib.InstanceConfigSetRequest, this.handleInstanceConfigSetRequest.bind(this)); this.handle(lib.InstanceConfigSetFieldRequest, this.handleInstanceConfigSetFieldRequest.bind(this)); this.handle(lib.InstanceConfigSetPropRequest, this.handleInstanceConfigSetPropRequest.bind(this)); this.handle(lib.InstanceAssignRequest, this.handleInstanceAssignRequest.bind(this)); @@ -196,6 +198,26 @@ export default class ControlConnection extends BaseConnection { return this._controller.config.toRemote("control"); } + async handleControllerConfigSetRequest(request: lib.ControllerConfigSetRequest) { + try { + for (const [fieldName, fieldValue] of Object.entries(request.fields)) { + if (typeof fieldValue === "object") { + for (const [propName, propValue] of Object.entries(fieldValue)) { + this._controller.config.stageProp( + fieldName as keyof lib.ControllerConfigFields, propName, propValue, "control" + ); + } + } else { + this._controller.config.stage(fieldName as keyof lib.ControllerConfigFields, fieldValue, "control"); + } + } + + this._controller.config.commitStaging(); + } finally { + this._controller.config.revertStaging(); + } + } + async handleControllerConfigSetFieldRequest(request: lib.ControllerConfigSetFieldRequest) { this._controller.config.set(request.field as keyof lib.ControllerConfigFields, request.value, "control"); } @@ -281,17 +303,41 @@ export default class ControlConnection extends BaseConnection { return instance.config.toRemote("control"); } - async handleInstanceConfigSetFieldRequest(request: lib.InstanceConfigSetFieldRequest) { - let instance = this._controller.instances.getForRequest(request.instanceId); - if (request.field === "instance.assigned_host") { + private static validateInstanceConfigSetField(fieldName: string) { + if (fieldName === "instance.assigned_host") { throw new lib.RequestError("instance.assigned_host must be set through the assign-host interface"); } - if (request.field === "instance.id") { + if (fieldName === "instance.id") { // XXX is this worth implementing? It's race condition galore. throw new lib.RequestError("Setting instance.id is not supported"); } + } + async handleInstanceConfigSetRequest(request: lib.InstanceConfigSetRequest) { + try { + for (const [fieldName, fieldValue] of Object.entries(request.fields)) { + ControlConnection.validateInstanceConfigSetField(fieldName); + if (typeof fieldValue === "object") { + for (const [propName, propValue] of Object.entries(fieldValue)) { + this._controller.config.stageProp( + fieldName as keyof lib.ControllerConfigFields, propName, propValue, "control" + ); + } + } else { + this._controller.config.stage(fieldName as keyof lib.ControllerConfigFields, fieldValue, "control"); + } + } + + this._controller.config.commitStaging(); + } finally { + this._controller.config.revertStaging(); + } + } + + async handleInstanceConfigSetFieldRequest(request: lib.InstanceConfigSetFieldRequest) { + const instance = this._controller.instances.getForRequest(request.instanceId); + ControlConnection.validateInstanceConfigSetField(request.field); instance.config.set(request.field as keyof lib.InstanceConfigFields, request.value, "control"); await this._controller.instanceConfigUpdated(instance); } @@ -299,6 +345,7 @@ export default class ControlConnection extends BaseConnection { async handleInstanceConfigSetPropRequest(request: lib.InstanceConfigSetPropRequest) { let instance = this._controller.instances.getForRequest(request.instanceId); let { field, prop, value } = request; + ControlConnection.validateInstanceConfigSetField(field); instance.config.setProp(field as keyof lib.InstanceConfigFields, prop, value, "control"); await this._controller.instanceConfigUpdated(instance); } diff --git a/packages/host/src/Host.ts b/packages/host/src/Host.ts index e59d6593..f33bb604 100644 --- a/packages/host/src/Host.ts +++ b/packages/host/src/Host.ts @@ -390,6 +390,7 @@ export default class Host extends lib.Link { this.handle(lib.HostRestartRequest, this.handleHostRestartRequest.bind(this)); this.handle(lib.HostUpdateRequest, this.handleHostUpdateRequest.bind(this)); this.handle(lib.HostConfigGetRequest, this.handleHostConfigGetRequest.bind(this)); + this.handle(lib.HostConfigSetRequest, this.handleHostConfigSetRequest.bind(this)); this.handle(lib.HostConfigSetFieldRequest, this.handleHostConfigSetFieldRequest.bind(this)); this.handle(lib.HostConfigSetPropRequest, this.handleHostConfigSetPropRequest.bind(this)); this.handle(lib.HostMetricsRequest, this.handleHostMetricsRequest.bind(this)); @@ -503,18 +504,41 @@ export default class Host extends lib.Link { return this.config.toRemote("control"); } - async handleHostConfigSetFieldRequest(request: lib.HostConfigSetFieldRequest) { - if (request.field === "host.id") { + private static validateHostConfigSetField(fieldName: string) { + if (fieldName === "host.id") { // Changing host.id at runtime is not worth the effort to // support and will break a lot of things if it is allowed. throw new lib.RequestError("Setting 'host.id' while host is running is not supported"); } + } + async handleHostConfigSetRequest(request: lib.HostConfigSetRequest) { + try { + for (const [fieldName, fieldValue] of Object.entries(request.fields)) { + Host.validateHostConfigSetField(fieldName); + if (typeof fieldValue === "object") { + for (const [propName, propValue] of Object.entries(fieldValue)) { + this.config.stageProp(fieldName as keyof lib.HostConfigFields, propName, propValue, "control"); + } + } else { + this.config.stage(fieldName as keyof lib.HostConfigFields, fieldValue, "control"); + } + } + + this.config.commitStaging(); + } finally { + this.config.revertStaging(); + } + } + + async handleHostConfigSetFieldRequest(request: lib.HostConfigSetFieldRequest) { + Host.validateHostConfigSetField(request.field); this.config.set(request.field as keyof lib.HostConfigFields, request.value, "control"); } async handleHostConfigSetPropRequest(request: lib.HostConfigSetPropRequest) { let { field, prop, value } = request; + Host.validateHostConfigSetField(field); this.config.setProp(field as keyof lib.HostConfigFields, prop, value, "control"); } diff --git a/packages/lib/src/data/messages_controller.ts b/packages/lib/src/data/messages_controller.ts index 2c74b4c2..d4206a39 100644 --- a/packages/lib/src/data/messages_controller.ts +++ b/packages/lib/src/data/messages_controller.ts @@ -37,6 +37,28 @@ export class ControllerConfigGetRequest { static Response = plainJson(ControllerConfig.jsonSchema); } +export class ControllerConfigSetRequest { + declare ["constructor"]: typeof ControllerConfigSetRequest; + static type = "request" as const; + static src = "control" as const; + static dst = "controller" as const; + static permission = "core.controller.update_config" as const; + + constructor( + public fields: Record>, + ) { } + + static jsonSchema = Type.Object({ + "fields": Type.Record(Type.String(), Type.Union([ + Type.String(), Type.Record(Type.String(), Type.String()), + ])), + }); + + static fromJSON(json: Static) { + return new this(json.fields); + } +} + export class ControllerConfigSetFieldRequest { declare ["constructor"]: typeof ControllerConfigSetFieldRequest; static type = "request" as const; diff --git a/packages/lib/src/data/messages_host.ts b/packages/lib/src/data/messages_host.ts index f1499992..67cf060b 100644 --- a/packages/lib/src/data/messages_host.ts +++ b/packages/lib/src/data/messages_host.ts @@ -37,6 +37,28 @@ export class HostConfigGetRequest { static Response = plainJson(HostConfig.jsonSchema); } +export class HostConfigSetRequest { + declare ["constructor"]: typeof HostConfigSetRequest; + static type = "request" as const; + static src = ["control", "controller"] as const; + static dst = "host" as const; + static permission = "core.host.update_config" as const; + + constructor( + public fields: Record>, + ) { } + + static jsonSchema = Type.Object({ + "fields": Type.Record(Type.String(), Type.Union([ + Type.String(), Type.Record(Type.String(), Type.String()), + ])), + }); + + static fromJSON(json: Static) { + return new this(json.fields); + } +} + export class HostConfigSetFieldRequest { declare ["constructor"]: typeof HostConfigSetFieldRequest; static type = "request" as const; diff --git a/packages/lib/src/data/messages_instance.ts b/packages/lib/src/data/messages_instance.ts index d138f3ea..7c9d7693 100644 --- a/packages/lib/src/data/messages_instance.ts +++ b/packages/lib/src/data/messages_instance.ts @@ -102,6 +102,30 @@ export class InstanceConfigGetRequest { } } +export class InstanceConfigSetRequest { + declare ["constructor"]: typeof InstanceConfigSetRequest; + static type = "request" as const; + static src = "control" as const; + static dst = "controller" as const; + static permission = "core.instance.update_config" as const; + + constructor( + public instanceId: number, + public fields: Record>, + ) { } + + static jsonSchema = Type.Object({ + "instanceId": Type.Integer(), + "fields": Type.Record(Type.String(), Type.Union([ + Type.String(), Type.Record(Type.String(), Type.String()), + ])), + }); + + static fromJSON(json: Static) { + return new this(json.instanceId, json.fields); + } +} + export class InstanceConfigSetFieldRequest { declare ["constructor"]: typeof InstanceConfigSetFieldRequest; static type = "request" as const; diff --git a/packages/lib/src/link/messages.ts b/packages/lib/src/link/messages.ts index 299cf3ae..419385ad 100644 --- a/packages/lib/src/link/messages.ts +++ b/packages/lib/src/link/messages.ts @@ -23,6 +23,7 @@ export const dataClasses: (RequestClass | EventClass) controller.ControllerRestartRequest, controller.ControllerUpdateRequest, controller.ControllerConfigGetRequest, + controller.ControllerConfigSetRequest, controller.ControllerConfigSetFieldRequest, controller.ControllerConfigSetPropRequest, controller.HostGenerateTokenRequest, @@ -43,6 +44,7 @@ export const dataClasses: (RequestClass | EventClass) host.HostRestartRequest, host.HostUpdateRequest, host.HostConfigGetRequest, + host.HostConfigSetRequest, host.HostConfigSetFieldRequest, host.HostConfigSetPropRequest, host.HostListRequest, @@ -59,6 +61,7 @@ export const dataClasses: (RequestClass | EventClass) instance.InstanceDetailsUpdatesEvent, instance.InstanceCreateRequest, instance.InstanceConfigGetRequest, + instance.InstanceConfigSetRequest, instance.InstanceConfigSetFieldRequest, instance.InstanceConfigSetPropRequest, instance.InstanceAssignRequest, From 9d807fc6d68355272813e40acfff563ba4e0ba6a Mon Sep 17 00:00:00 2001 From: Cooldude2606 <25043174+Cooldude2606@users.noreply.github.com> Date: Tue, 10 Mar 2026 23:38:22 +0000 Subject: [PATCH 11/20] Update webui config to use bulk set --- .../web_ui/src/components/BaseConfigTree.tsx | 61 ++++++++----------- .../src/components/ControllerConfigTree.tsx | 23 +------ .../web_ui/src/components/HostConfigTree.tsx | 20 +----- .../src/components/InstanceConfigTree.tsx | 20 +----- 4 files changed, 33 insertions(+), 91 deletions(-) diff --git a/packages/web_ui/src/components/BaseConfigTree.tsx b/packages/web_ui/src/components/BaseConfigTree.tsx index a17a11dd..00f5962d 100644 --- a/packages/web_ui/src/components/BaseConfigTree.tsx +++ b/packages/web_ui/src/components/BaseConfigTree.tsx @@ -9,6 +9,7 @@ import * as lib from "@clusterio/lib"; import ControlContext from "./ControlContext"; import { InputComponent, InputComponentProps } from "../BaseWebPlugin"; import type { Control } from "../util/websocket"; +import { notifyErrorHandler } from "../util/notify"; const { Title } = Typography; @@ -67,8 +68,7 @@ function renderInput(inputComponents: Record, def: lib.F type BaseConfigTreeProps = { ConfigClass: typeof lib.ControllerConfig | typeof lib.HostConfig | typeof lib.InstanceConfig; retrieveConfig: () => Promise; - setField: (field:string, value:any) => Promise; - setProp: (field:string, prop:string, value:any) => Promise; + setConfig: (fields: Record>) => Promise; id?: number; onApply?: () => void; available?: boolean; @@ -157,32 +157,25 @@ export default function BaseConfigTree(props: BaseConfigTreeProps) { loading={applying} onClick={async () => { setApplying(true); - for (let field of changedFields) { - let value = form.getFieldValue(field); - let request; + const fields: Record> = {}; + for (const field of changedFields) { + const value = form.getFieldValue(field); if (propsMap.has(field)) { - let [fieldName, prop] = propsMap.get(field)!; - request = props.setProp(fieldName, prop, value); + const [fieldName, prop] = propsMap.get(field)!; + const fieldValue = (fields[fieldName] ?? {}) as Record; + fields[fieldName] = fieldValue; + fieldValue[prop] = value ? JSON.parse(value) : undefined; } else { - request = props.setField(field, value === null ? "" : String(value)); + fields[field] = value === null ? "" : String(value); } + } - try { - await request; - setChangedFields(changed => { - let newChanged = new Set(changed); - newChanged.delete(field); - return newChanged; - }); - - } catch (err: any) { - setErrorFields(errors => { - let newErrors = new Map(errors); - newErrors.set(field, err.message); - return newErrors; - }); - } + try { + await props.setConfig(fields); + setChangedFields(new Set()); + } catch (err: any) { + notifyErrorHandler(err.message); } await updateConfig(); @@ -325,26 +318,23 @@ function computeTreeData( } if (!def.validator) { return; } - - // TODO this needs to use stage and stageProp to avoid deadlocks - const staging = config.staging; - staging.setProp(fieldName, prop, fieldValue); + config.stageProp(fieldName, prop, fieldValue); for (const dependsOn of def.dependsOn ?? []) { const dependsOnDef = ConfigClass.fieldDefinitions[dependsOn as any] as typeof def; try { if (propsMap.has(dependsOn)) { const [name, otherProp] = propsMap.get(dependsOn)!; - staging.setProp(name, otherProp, JSON.parse(getFieldValue(dependsOn))); + config.stageProp(name, otherProp, JSON.parse(getFieldValue(dependsOn))); } else if (dependsOnDef.type !== "object") { - staging.set(dependsOn, getFieldValue(dependsOn)); + config.stage(dependsOn, getFieldValue(dependsOn)); } } catch (err) { throw new Error(`Failed to fetch field: ${dependsOn}`); } } - def.validator(fieldValue, staging); + def.validator(fieldValue, config.staging); }})]} > @@ -418,26 +408,23 @@ function computeTreeData( rules={[({getFieldValue}) => ({ validator: async (_, fieldValue) => { if (!def.validator) { return; } - - // TODO this needs to use stage and stageProp to avoid deadlocks - const staging = config.staging; - staging.set(fieldName, fieldValue); + config.stage(fieldName, fieldValue); for (const dependsOn of def.dependsOn ?? []) { const dependsOnDef = ConfigClass.fieldDefinitions[dependsOn as any] as typeof def; try { if (propsMap.has(dependsOn)) { const [name, prop] = propsMap.get(dependsOn)!; - staging.setProp(name, prop, JSON.parse(getFieldValue(dependsOn))); + config.stageProp(name, prop, JSON.parse(getFieldValue(dependsOn))); } else if (dependsOnDef.type !== "object") { - staging.set(dependsOn, getFieldValue(dependsOn)); + config.stage(dependsOn, getFieldValue(dependsOn)); } } catch (err) { throw new Error(`Failed to fetch field: ${dependsOn}`); } } - def.validator(fieldValue, staging); + def.validator(fieldValue, config.staging); }, })]} > diff --git a/packages/web_ui/src/components/ControllerConfigTree.tsx b/packages/web_ui/src/components/ControllerConfigTree.tsx index a369fab7..aecd5c03 100644 --- a/packages/web_ui/src/components/ControllerConfigTree.tsx +++ b/packages/web_ui/src/components/ControllerConfigTree.tsx @@ -13,30 +13,13 @@ export default function ControllerConfigTree() { return await control.send(new lib.ControllerConfigGetRequest()); } - async function setField(field: string, value: any) { - await control.send(new lib.ControllerConfigSetFieldRequest( - field, - String(value), - )); - } - - async function setProp(field: string, prop: string, value: any) { - if (value) { - try { - value = JSON.parse(value); - } catch (err) { - return; - } - } else { - value = undefined; - } - await control.send(new lib.ControllerConfigSetPropRequest(field, prop, value)); + async function setConfig(fields: Record>) { + await control.send(new lib.ControllerConfigSetRequest(fields)); } return ; } diff --git a/packages/web_ui/src/components/HostConfigTree.tsx b/packages/web_ui/src/components/HostConfigTree.tsx index aef520f0..caf8d4a1 100644 --- a/packages/web_ui/src/components/HostConfigTree.tsx +++ b/packages/web_ui/src/components/HostConfigTree.tsx @@ -13,28 +13,14 @@ export default function HostConfigTree(props: { id: number, available: boolean } return await control.sendTo({ hostId: props.id }, new lib.HostConfigGetRequest()); } - async function setField(field: string, value: any) { - await control.sendTo({ hostId: props.id }, new lib.HostConfigSetFieldRequest(field, value)); - } - - async function setProp(field: string, prop: string, value: any) { - if (value) { - try { - value = JSON.parse(value); - } catch (err) { - return; - } - } else { - value = undefined; - } - await control.sendTo({ hostId: props.id }, new lib.HostConfigSetPropRequest(field, prop, value)); + async function setConfig(fields: Record>) { + await control.send(new lib.HostConfigSetRequest(fields)); } return ; diff --git a/packages/web_ui/src/components/InstanceConfigTree.tsx b/packages/web_ui/src/components/InstanceConfigTree.tsx index c9a45530..ff028ddd 100644 --- a/packages/web_ui/src/components/InstanceConfigTree.tsx +++ b/packages/web_ui/src/components/InstanceConfigTree.tsx @@ -17,28 +17,14 @@ export default function InstanceConfigTree(props: InstanceConfigTreeProps) { return await control.send(new lib.InstanceConfigGetRequest(props.id)); } - async function setField(field: string, value: any) { - await control.send(new lib.InstanceConfigSetFieldRequest(props.id, field, value)); - } - - async function setProp(field: string, prop: string, value: any) { - if (value) { - try { - value = JSON.parse(value); - } catch (err) { - return; - } - } else { - value = undefined; - } - await control.send(new lib.InstanceConfigSetPropRequest(props.id, field, prop, value)); + async function setConfig(fields: Record>) { + await control.send(new lib.InstanceConfigSetRequest(props.id, fields)); } return ; } From 414ef0bc8bbb3d448ab69987beeb373935eab9e6 Mon Sep 17 00:00:00 2001 From: Cooldude2606 <25043174+Cooldude2606@users.noreply.github.com> Date: Wed, 11 Mar 2026 00:28:36 +0000 Subject: [PATCH 12/20] Some fixes to config setting --- packages/controller/src/ControlConnection.ts | 11 ++++++----- packages/lib/src/config/classes.ts | 14 +++++++------- packages/lib/src/data/messages_controller.ts | 4 ++-- packages/lib/src/data/messages_host.ts | 4 ++-- packages/lib/src/data/messages_instance.ts | 4 ++-- packages/web_ui/src/components/BaseConfigTree.tsx | 10 +++++++++- 6 files changed, 28 insertions(+), 19 deletions(-) diff --git a/packages/controller/src/ControlConnection.ts b/packages/controller/src/ControlConnection.ts index 5e827a89..aa2350f3 100644 --- a/packages/controller/src/ControlConnection.ts +++ b/packages/controller/src/ControlConnection.ts @@ -315,23 +315,24 @@ export default class ControlConnection extends BaseConnection { } async handleInstanceConfigSetRequest(request: lib.InstanceConfigSetRequest) { + const instance = this._controller.instances.getForRequest(request.instanceId); try { for (const [fieldName, fieldValue] of Object.entries(request.fields)) { ControlConnection.validateInstanceConfigSetField(fieldName); if (typeof fieldValue === "object") { for (const [propName, propValue] of Object.entries(fieldValue)) { - this._controller.config.stageProp( - fieldName as keyof lib.ControllerConfigFields, propName, propValue, "control" + instance.config.stageProp( + fieldName as keyof lib.InstanceConfigFields, propName, propValue, "control" ); } } else { - this._controller.config.stage(fieldName as keyof lib.ControllerConfigFields, fieldValue, "control"); + instance.config.stage(fieldName as keyof lib.InstanceConfigFields, fieldValue, "control"); } } - this._controller.config.commitStaging(); + instance.config.commitStaging(); } finally { - this._controller.config.revertStaging(); + instance.config.revertStaging(); } } diff --git a/packages/lib/src/config/classes.ts b/packages/lib/src/config/classes.ts index c5834268..02760cde 100644 --- a/packages/lib/src/config/classes.ts +++ b/packages/lib/src/config/classes.ts @@ -9,7 +9,6 @@ import * as libSchema from "../schema"; import { StringEnum } from "../data/composites"; import { safeOutputFile } from "../file_ops"; import * as validators from "./validators"; -import { string } from "yargs"; const ConfigLocation = StringEnum(["controller", "host", "control"]); export type ConfigLocation = Static; @@ -208,7 +207,7 @@ export class Config< * Note that mutating this object will lead to unexpected behaviour. */ declare static fieldDefinitions: ConfigDefs; - declare ["constructor"]: typeof Config; + declare ["constructor"]: typeof Config; /** A set of common validators and validator factories */ static validators = validators; @@ -272,14 +271,14 @@ export class Config< } /** A staging version of the config with changes tracked, invalided by revertStaging */ - private _staging?: { config: Config, changes: Set }; + private _staging?: { config: Config, changes: Set}; /** Get the latest version of staging */ get staging(): Config { if (this._staging) { return this._staging.config; } - const config = new this.constructor(this.location, this.fields) as this; - const changes = new Set(); - config.on("fieldChanged", name => changes.add(name as keyof Fields & string)); + const config = new this.constructor(this.location, this.fields); + const changes = new Set(); + config.on("fieldChanged", name => changes.add(name)); this._staging = { config, changes }; return config; } @@ -832,7 +831,8 @@ export class Config< // Validation success, set all values which have changed for (const fieldName of staging.changes) { - this._set(fieldName, config.fields[fieldName]); + const field = fieldName as keyof Fields & string; + this._set(field, config.fields[field]); } } diff --git a/packages/lib/src/data/messages_controller.ts b/packages/lib/src/data/messages_controller.ts index d4206a39..69ffb3a0 100644 --- a/packages/lib/src/data/messages_controller.ts +++ b/packages/lib/src/data/messages_controller.ts @@ -45,12 +45,12 @@ export class ControllerConfigSetRequest { static permission = "core.controller.update_config" as const; constructor( - public fields: Record>, + public fields: Record>, ) { } static jsonSchema = Type.Object({ "fields": Type.Record(Type.String(), Type.Union([ - Type.String(), Type.Record(Type.String(), Type.String()), + Type.String(), Type.Record(Type.String(), Type.Unknown()), ])), }); diff --git a/packages/lib/src/data/messages_host.ts b/packages/lib/src/data/messages_host.ts index 67cf060b..e707d856 100644 --- a/packages/lib/src/data/messages_host.ts +++ b/packages/lib/src/data/messages_host.ts @@ -45,12 +45,12 @@ export class HostConfigSetRequest { static permission = "core.host.update_config" as const; constructor( - public fields: Record>, + public fields: Record>, ) { } static jsonSchema = Type.Object({ "fields": Type.Record(Type.String(), Type.Union([ - Type.String(), Type.Record(Type.String(), Type.String()), + Type.String(), Type.Record(Type.String(), Type.Unknown()), ])), }); diff --git a/packages/lib/src/data/messages_instance.ts b/packages/lib/src/data/messages_instance.ts index 7c9d7693..0b87461b 100644 --- a/packages/lib/src/data/messages_instance.ts +++ b/packages/lib/src/data/messages_instance.ts @@ -111,13 +111,13 @@ export class InstanceConfigSetRequest { constructor( public instanceId: number, - public fields: Record>, + public fields: Record>, ) { } static jsonSchema = Type.Object({ "instanceId": Type.Integer(), "fields": Type.Record(Type.String(), Type.Union([ - Type.String(), Type.Record(Type.String(), Type.String()), + Type.String(), Type.Record(Type.String(), Type.Unknown()), ])), }); diff --git a/packages/web_ui/src/components/BaseConfigTree.tsx b/packages/web_ui/src/components/BaseConfigTree.tsx index 00f5962d..fcc7c1e8 100644 --- a/packages/web_ui/src/components/BaseConfigTree.tsx +++ b/packages/web_ui/src/components/BaseConfigTree.tsx @@ -164,7 +164,15 @@ export default function BaseConfigTree(props: BaseConfigTreeProps) { const [fieldName, prop] = propsMap.get(field)!; const fieldValue = (fields[fieldName] ?? {}) as Record; fields[fieldName] = fieldValue; - fieldValue[prop] = value ? JSON.parse(value) : undefined; + try { + fieldValue[prop] = value ? JSON.parse(value) : undefined; + } catch (err: any) { + setErrorFields(errors => { + const newErrors = new Map(errors); + newErrors.set(fieldName, err.message); + return newErrors; + }); + } } else { fields[field] = value === null ? "" : String(value); From 2510d47023cdbe1f703059ee10eb42d34fd758e5 Mon Sep 17 00:00:00 2001 From: Cooldude2606 <25043174+Cooldude2606@users.noreply.github.com> Date: Mon, 16 Mar 2026 23:20:40 +0000 Subject: [PATCH 13/20] Add tests --- packages/controller/src/routes.ts | 2 +- packages/lib/src/config/validators.ts | 6 +- test/lib/config/classes.js | 256 +++++++++++++++++++++----- test/lib/config/migrations.js | 2 +- test/lib/config/validators.test.js | 245 ++++++++++++++++++++++++ test/mock.js | 21 ++- 6 files changed, 476 insertions(+), 56 deletions(-) create mode 100644 test/lib/config/validators.test.js diff --git a/packages/controller/src/routes.ts b/packages/controller/src/routes.ts index 6b578dbd..634fc461 100644 --- a/packages/controller/src/routes.ts +++ b/packages/controller/src/routes.ts @@ -1,5 +1,5 @@ import type { Application, Request, Response } from "express"; -import type Controller from "./Controller"; +import Controller from "./Controller"; import busboy from "busboy"; import crypto from "crypto"; diff --git a/packages/lib/src/config/validators.ts b/packages/lib/src/config/validators.ts index fa26c7c1..3d12f134 100644 --- a/packages/lib/src/config/validators.ts +++ b/packages/lib/src/config/validators.ts @@ -52,7 +52,11 @@ export function integer(value: number) { } } -/** Value must be a valid file path, it does not need to exist */ +/** + * Value must be a valid file path, it does not need to exist + * On posix this will only error for non string values, which isn't very useful + * However, this could be expanded in the future to check system OS + */ export function filePath(value: string) { try { path.resolve(value); diff --git a/test/lib/config/classes.js b/test/lib/config/classes.js index 4003d9b6..83e447af 100644 --- a/test/lib/config/classes.js +++ b/test/lib/config/classes.js @@ -41,9 +41,49 @@ describe("lib/config/classes", function() { "test.priv": { access: ["local"], type: "string", optional: true }, "test.cred": { credential: ["local"] }, "test.rdo": { readonly: ["local"] }, + "test.base": { type: "number", initialValue: 1 }, + "test.dependent": { + type: "number", + initialValue: 1, + dependsOn: ["test.base"], + validator(value, config) { + if (value < config.get("test.base")) { + throw new Error("dependent must be >= base"); + } + }, + }, + "test.validated": { + type: "number", + optional: true, + validator(value) { + if (value !== null && value < 0) { + throw new Error("must be positive"); + } + }, + }, + "test.objValidated": { + type: "object", + initialValue: {}, + validator(value) { + if (value && value.bad !== undefined) { + throw new Error("bad key"); + } + }, + }, }; } + const testConfigDefault = Object.fromEntries( + Object.entries(TestConfig.fieldDefinitions).map( + ([fieldName, fieldDef]) => ([ + fieldName, + typeof fieldDef.initialValue === "function" + ? fieldDef.initialValue() + : (fieldDef.initialValue ?? null), + ]) + ) + ); + describe("constructor", function() { it("should construct an instance", function() { let config = new TestConfig("local"); @@ -80,41 +120,37 @@ describe("lib/config/classes", function() { const testInstance = new TestConfig("local", undefined, "filepath"); assert.equal(testInstance.filepath, "filepath"); }); + it("should throw if a required field has no initialValue", function() { + this.skip(); // TODO known bug that was discovered but of of scope for #856 + class BrokenConfig extends lib.Config { + static fieldDefinitions = { + "test.required": { type: "string" }, + }; + } + + assert.throws( + () => new BrokenConfig("local"), + ); + }); }); describe(".toJSON()", function() { it("should serialize a basic config", function() { - let testInstance = new TestConfig("local"); - assert.deepEqual(testInstance.toJSON(), { - "alpha.foo": null, - "beta.bar": {}, - "test.enum": "b", - "test.test": null, - "test.func": 42, - "test.bool": false, - "test.json": {}, - "test.rest": {}, - "test.priv": null, - "test.cred": null, - "test.rdo": null, - }); + const testInstance = new TestConfig("local"); + assert.deepEqual(testInstance.toJSON(), testConfigDefault); }); }); describe(".toRemote()", function() { it("should leave out inaccessible fields", function() { - let testInstance = new TestConfig("local"); - assert.deepEqual(testInstance.toRemote("remote"), { - "alpha.foo": null, - "beta.bar": {}, - "test.enum": "b", - "test.test": null, - "test.func": 42, - "test.bool": false, - "test.json": {}, - "test.rest": {}, - "test.rdo": null, - }); + const testInstance = new TestConfig("local"); + const { "test.cred": _cred, "test.priv": _priv, ...expected } = testConfigDefault; + assert.deepEqual(testInstance.toRemote("remote"), expected); + }); + it("should filter fields when filter argument is provided", function() { + const testInstance = new TestConfig("local"); + const filtered = testInstance.toRemote("local", ["alpha.foo"]); + assert.deepEqual(filtered, { "alpha.foo": null }); }); }); @@ -165,6 +201,7 @@ describe("lib/config/classes", function() { it("should preserve unknown fields when serialized back", function() { let testFields = { + ...testConfigDefault, "extra.blah": true, "extra.spam": "foobar", "alpha.foo": "true", @@ -175,10 +212,6 @@ describe("lib/config/classes", function() { "test.bool": false, "test.json": {}, "test.rest": {}, - "test.priv": null, - "test.cred": null, - "test.rdo": null, - "test.alpha": null, "test.beta": "decay", "test.gamma": 99, }; @@ -245,6 +278,7 @@ describe("lib/config/classes", function() { }); it("should preserve unknown fields", async function() { const testFields = { + ...testConfigDefault, "extra.blah": true, "extra.spam": "foobar", "alpha.foo": "true", @@ -255,10 +289,6 @@ describe("lib/config/classes", function() { "test.bool": false, "test.json": {}, "test.rest": {}, - "test.priv": null, - "test.cred": null, - "test.rdo": null, - "test.alpha": null, "test.beta": "decay", "test.gamma": 99, }; @@ -347,19 +377,10 @@ describe("lib/config/classes", function() { "beta.bar": { value: 30 }, }, false); assert.deepEqual(testInstance.toJSON(), { + ...testConfigDefault, "extra.blah": true, "extra.spam": "baz", - "alpha.foo": null, "beta.bar": { value: 30 }, - "test.bool": false, - "test.enum": "b", - "test.func": 42, - "test.json": {}, - "test.rest": {}, - "test.priv": null, - "test.cred": null, - "test.rdo": null, - "test.test": null, }); }); @@ -709,6 +730,29 @@ describe("lib/config/classes", function() { testInstance.set("test.enum", "b"); assert.deepEqual(testInstance.restartRequired, true, "restart required not set"); }); + it("should run field validator successfully", function() { + testInstance.set("test.validated", 5); + assert.equal(testInstance.get("test.validated"), 5); + }); + + it("should throw if validator fails", function() { + assert.throws( + () => testInstance.set("test.validated", -1), + new lib.InvalidValue("Failed validation of test.validated: must be positive") + ); + }); + it("should validate dependent fields", function() { + testInstance.set("test.dependent", 10); + testInstance.set("test.base", 5); + assert.equal(testInstance.get("test.dependent"), 10); + assert.equal(testInstance.get("test.base"), 5); + }); + it("should throw if dependent validation fails", function() { + assert.throws( + () => testInstance.set("test.base", 5), + new lib.InvalidValue("Failed validation of dependent test.dependent: dependent must be >= base") + ); + }); }); describe(".setProp()", function() { @@ -786,6 +830,130 @@ describe("lib/config/classes", function() { testInstance.setProp("test.rest", "test", true); assert.deepEqual(testInstance.restartRequired, true, "restart required not set"); }); + it("should run validator when setting object property", function() { + testInstance.setProp("test.objValidated", "good", true); + assert.deepEqual(testInstance.get("test.objValidated"), { good: true }); + }); + it("should throw if object validator fails", function() { + assert.throws( + () => testInstance.setProp("test.objValidated", "bad", true), + new lib.InvalidValue("Failed validation of test.objValidated: bad key") + ); + }); + }); + + describe("get .staging", function() { + it("should create staging config and reuse it", function() { + const testInstance = new TestConfig("local"); + + const first = testInstance.staging; + const second = testInstance.staging; + + assert.equal(first, second); + }); + it("should return new instance if invalided", function() { + const testInstance = new TestConfig("local"); + + const first = testInstance.staging; + testInstance.set("alpha.foo", "value"); + const second = testInstance.staging; + + assert.notEqual(first, second); + }); + }); + + describe(".stage()", function() { + let testInstance; + beforeEach(function() { + testInstance = new TestConfig("local"); + }); + + it("should stage a value without committing", function() { + testInstance.stage("alpha.foo", "value"); + + assert.equal(testInstance.get("alpha.foo"), null); + assert.equal(testInstance.staging.get("alpha.foo"), "value"); + }); + it("should throw if field does not exist", function() { + assert.throws( + () => testInstance.stage("invalid", "value"), + new lib.InvalidField("No field named 'invalid'") + ); + }); + }); + + describe(".stageProp()", function() { + let testInstance; + beforeEach(function() { + testInstance = new TestConfig("local"); + }); + + it("should stage object property change", function() { + testInstance.stageProp("test.json", "prop", true); + + assert.deepEqual(testInstance.get("test.json"), {}); + assert.deepEqual(testInstance.staging.get("test.json"), { prop: true }); + }); + it("should delete property when value undefined", function() { + testInstance.set("test.json", { a: 1 }); + + testInstance.stageProp("test.json", "a", undefined); + + assert.deepEqual(testInstance.staging.get("test.json"), {}); + assert.deepEqual(testInstance.get("test.json"), { a: 1 }); + }); + it("should throw if field is not object", function() { + assert.throws( + () => testInstance.stageProp("test.enum", "prop", true), + new lib.InvalidValue("Cannot set property on non-object field 'test.enum'") + ); + }); + }); + + describe(".commitStaging()", function() { + let testInstance; + beforeEach(function() { + testInstance = new TestConfig("local"); + }); + + it("should do nothing if staging does not exist", function() { + testInstance.commitStaging(); + assert.equal(testInstance.get("alpha.foo"), null); + }); + it("should commit staged values", function() { + testInstance.stage("alpha.foo", "value"); + + assert.equal(testInstance.get("alpha.foo"), null); + testInstance.commitStaging(); + assert.equal(testInstance.get("alpha.foo"), "value"); + }); + it("should throw if staged validation fails", function() { + testInstance.stage("test.validated", -1); + + assert.throws( + () => testInstance.commitStaging(), + new lib.InvalidValue("Failed validation of test.validated: must be positive") + ); + }); + it("should commit stages values with dependent validation", function() { + assert.throws(() => testInstance.set("test.base", 10)); + testInstance.stage("test.base", 10); + testInstance.stage("test.dependent", 15); + // The above ordering would error if set was used + testInstance.commitStaging(); + assert.equal(testInstance.get("test.base"), 10); + assert.equal(testInstance.get("test.dependent"), 15); + }); + }); + + describe(".revertStaging()", function() { + it("should discard staged changes", function() { + const testInstance = new TestConfig("local"); + testInstance.stage("alpha.foo", "value"); + testInstance.revertStaging(); + assert.equal(testInstance.staging.get("alpha.foo"), null); + assert.equal(testInstance.get("alpha.foo"), null); + }); }); }); }); diff --git a/test/lib/config/migrations.js b/test/lib/config/migrations.js index e18719ff..cb88c6d7 100644 --- a/test/lib/config/migrations.js +++ b/test/lib/config/migrations.js @@ -4,7 +4,7 @@ const assert = require("assert").strict; // Each migration test should indicate which version introduced the migration -describe("lib/config/migrations", function() { +describe("lib/config/definitions/migrations", function() { describe("Controller Config", function() { it("should migrate 'external_address' to 'public_url'", function() { // Alpha 19 const config = lib.ControllerConfig.fromJSON({ diff --git a/test/lib/config/validators.test.js b/test/lib/config/validators.test.js new file mode 100644 index 00000000..876d95c5 --- /dev/null +++ b/test/lib/config/validators.test.js @@ -0,0 +1,245 @@ +"use strict"; +const assert = require("assert").strict; +const lib = require("@clusterio/lib"); +const validators = lib.Config.validators; +const { ControllerConfig, HostConfig, InstanceConfig, ControlConfig } = lib; + +describe("lib/config/validators", function() { + describe("all", function() { + it("should call all validators in order", function() { + let calls = []; + + function v1(value, config) { + calls.push("v1"); + } + function v2(value, config) { + calls.push("v2"); + } + + const combined = validators.all(v1, v2); + combined("value", {}); + + assert.deepEqual(calls, ["v1", "v2"], "validators were not called in order"); + }); + + it("should propagate errors from validators", function() { + function v1() {} + function v2() { + throw new Error("failure"); + } + + const combined = validators.all(v1, v2); + + assert.throws( + () => combined("value", {}), + /failure/, + "error from validator was not propagated" + ); + }); + }); + + describe("optional", function() { + it("should skip validation when value is null", function() { + let called = false; + + function validator() { + called = true; + } + + const wrapped = validators.optional(validator); + wrapped(null, {}); + + assert.equal(called, false, "validator should not have been called"); + }); + + it("should run validator when value is not null", function() { + let called = false; + + function validator() { + called = true; + } + + const wrapped = validators.optional(validator); + wrapped("value", {}); + + assert.equal(called, true, "validator was not called"); + }); + }); + + describe("greaterThan", function() { + it("should allow values greater than min", function() { + const validator = validators.greaterThan(5); + assert.doesNotThrow(() => validator(6)); + }); + + it("should throw if value equals min", function() { + const validator = validators.greaterThan(5); + + assert.throws( + () => validator(5), + /Value must be greater than 5/, + "did not throw on equal value" + ); + }); + + it("should throw if value is less than min", function() { + const validator = validators.greaterThan(5); + + assert.throws( + () => validator(4), + /Value must be greater than 5/, + "did not throw on smaller value" + ); + }); + }); + + describe("greaterThanZero", function() { + it("should allow values greater than zero", function() { + assert.doesNotThrow(() => validators.greaterThanZero(1)); + }); + + it("should throw on zero", function() { + assert.throws( + () => validators.greaterThanZero(0), + /Value must be greater than 0/ + ); + }); + + it("should throw on negative values", function() { + assert.throws( + () => validators.greaterThanZero(-1), + /Value must be greater than 0/ + ); + }); + }); + + describe("greaterThanEqual", function() { + it("should allow values greater than min", function() { + const validator = validators.greaterThanEqual(5); + assert.doesNotThrow(() => validator(6)); + }); + + it("should allow values equal to min", function() { + const validator = validators.greaterThanEqual(5); + assert.doesNotThrow(() => validator(5)); + }); + + it("should throw if value is less than min", function() { + const validator = validators.greaterThanEqual(5); + + assert.throws( + () => validator(4), + /Value must be greater than 5/, + "did not throw on smaller value" + ); + }); + }); + + describe("greaterThanEqualZero", function() { + it("should allow zero", function() { + assert.doesNotThrow(() => validators.greaterThanEqualZero(0)); + }); + + it("should allow positive values", function() { + assert.doesNotThrow(() => validators.greaterThanEqualZero(1)); + }); + + it("should throw on negative values", function() { + assert.throws( + () => validators.greaterThanEqualZero(-1), + /Value must be greater than 0/ + ); + }); + }); + + describe("integer", function() { + it("should allow integer values", function() { + assert.doesNotThrow(() => validators.integer(10)); + }); + + it("should throw on non-integer values", function() { + assert.throws( + () => validators.integer(1.5), + /Value must be an integer/, + "did not throw on float" + ); + }); + }); + + describe("filePath", function() { + it("should accept valid paths", function() { + assert.doesNotThrow(() => validators.filePath("test/file.txt")); + assert.doesNotThrow(() => validators.filePath("/absolute/path")); + }); + + it("should throw if value is not a string", function() { + // This is the only case that will cause path.resolve to throw on posix + assert.throws( + () => validators.filePath(null), + /Value must be a valid path/ + ); + }); + }); +}); + +describe("lib/config/definitions/validators", function() { + describe("Controller Config", function() { + + }); + + describe("Host Config", function() { + + }); + + describe("Instance Config", function() { + it("should validate factorio.version", function() { + const config = new InstanceConfig("controller"); + + // valid cases + assert.doesNotThrow(() => config.set("factorio.version", "latest")); + assert.doesNotThrow(() => config.set("factorio.version", "1.1")); + assert.doesNotThrow(() => config.set("factorio.version", "1.1.87")); + + // invalid case + assert.throws( + () => config.set("factorio.version", "invalidValue"), + /Value must be be 'latest', or match X.Y, or match X.Y.Z/ + ); + }); + + it("should validate factorio.player_online_autosave_slots against autosave slots", function() { + const config = new InstanceConfig("controller", { + "factorio.settings": { + autosave_slots: 5, + }, + }); + + // valid: equal + assert.doesNotThrow(() => config.set("factorio.player_online_autosave_slots", 5)); + + // valid: greater + assert.doesNotThrow(() => config.set("factorio.player_online_autosave_slots", 6)); + + // invalid: less than autosave_slots + assert.throws( + () => config.set("factorio.player_online_autosave_slots", 4), + /Value cannot be less than the number of autosave slots/ + ); + }); + + it("should allow player_online_autosave_slots when autosave_slots is not a number", function() { + const config = new InstanceConfig("controller", { + "factorio.settings": { + autosave_slots: null, + }, + }); + + assert.doesNotThrow(() => config.set("factorio.player_online_autosave_slots", 0)); + assert.doesNotThrow(() => config.set("factorio.player_online_autosave_slots", 1)); + }); + }); + + describe("Control Config", function() { + + }); +}); diff --git a/test/mock.js b/test/mock.js index d505f7f1..dd1c5d2f 100644 --- a/test/mock.js +++ b/test/mock.js @@ -195,6 +195,10 @@ class MockController { this.handles = new Map(); } + get authSecret() { + return Buffer.from(this.config.get("controller.auth_secret"), "base64"); + } + handle(eventClass, handler) { this.handles.set(eventClass, handler); } @@ -237,16 +241,15 @@ async function createInstancePlugin(InstancePluginClass, info) { return plugin; } - module.exports = { - MockLogger, - MockSocket, - MockConnector, - MockServer, - MockInstance, - MockHost, - MockControl, - MockController, + MockLogger: MockLogger, + MockSocket: MockSocket, + MockConnector: MockConnector, + MockServer: MockServer, + MockInstance: MockInstance, + MockHost: MockHost, + MockControl: MockControl, + MockController: MockController, createControllerPlugin, createInstancePlugin, From 4986c9af99d0e7b17c4b708cf4c5583d58ab842f Mon Sep 17 00:00:00 2001 From: Cooldude2606 <25043174+Cooldude2606@users.noreply.github.com> Date: Tue, 17 Mar 2026 21:31:46 +0000 Subject: [PATCH 14/20] Complete coverage of config classes --- packages/lib/src/config/classes.ts | 2 +- test/lib/config/classes.js | 389 +++++++++++++++++------------ 2 files changed, 224 insertions(+), 167 deletions(-) diff --git a/packages/lib/src/config/classes.ts b/packages/lib/src/config/classes.ts index 02760cde..e3bb4009 100644 --- a/packages/lib/src/config/classes.ts +++ b/packages/lib/src/config/classes.ts @@ -794,7 +794,7 @@ export class Config< throw new InvalidValue(`Cannot set property on non-object field '${name}'`); } - const prev = this.fields[name] as Record; + const prev = this.staging.get(name) as Record; const updated = {...prev || {}}; if (value !== undefined) { diff --git a/test/lib/config/classes.js b/test/lib/config/classes.js index 83e447af..291bdd98 100644 --- a/test/lib/config/classes.js +++ b/test/lib/config/classes.js @@ -1,5 +1,6 @@ "use strict"; const lib = require("@clusterio/lib"); +const { optional } = require("@clusterio/lib/dist/node/src/config/validators"); const fs = require("fs-extra"); const path = require("path"); const assert = require("assert").strict; @@ -52,6 +53,16 @@ describe("lib/config/classes", function() { } }, }, + "test.objDependent": { + type: "object", + initialValue: {}, + dependsOn: ["test.base"], + validator(value, config) { + if (value.base !== undefined && value.base !== config.get("test.base")) { + throw new Error("base prop must match test.base"); + } + }, + }, "test.validated": { type: "number", optional: true, @@ -592,6 +603,111 @@ describe("lib/config/classes", function() { }); }); + /** + * @param {typeof lib.Config.prototype.set} set + * @param {typeof lib.Config.prototype.get} get + * @param {(config: lib.Config) => void} replaceConfig + */ + function testAccessAndCoerce(set, get, replaceConfig) { + it("should set the value", function() { + set("alpha.foo", "new value"); + assert.equal(get("alpha.foo"), "new value"); + }); + it("should throw if field does not exist", function() { + assert.throws( + () => set("test.bar", 1), + new lib.InvalidField("No field named 'test.bar'") + ); + }); + it("should throw if field is remotely inaccessible", function() { + assert.throws( + () => set("test.priv", "bad", "remote"), + new lib.InvalidAccess("Field 'test.priv' is not accessible from remote") + ); + }); + it("should throw if field is locally inaccessible", function() { + replaceConfig(new TestConfig("remote")); + assert.throws( + () => set("test.priv", "bad", "local"), + new lib.InvalidAccess("Field 'test.priv' is not accessible from remote") + ); + }); + it("should throw if field is not in enum", function() { + assert.throws( + () => set("test.enum", "bar"), + new Error("Expected one of [a, b, c], not bar") + ); + }); + it("should work if field is in enum", function() { + set("test.enum", "c"); + assert.equal(get("test.enum"), "c"); + }); + it("should throw if field is not optional", function() { + assert.throws( + () => set("test.func", null), + new Error("Field test.func cannot be null") + ); + }); + it("should work if field is optional", function() { + set("test.test", "spam"); + assert.equal(get("test.test"), "spam"); + }); + it("should throw if field is of wrong type", function() { + assert.throws( + () => set("test.test", 1), + new Error("Expected type of test.test to be string, not number") + ); + }); + it("should treat empty string as null", function() { + set("test.test", ""); + assert.equal(get("test.test"), null); + }); + it("should auto convert string to boolean if possible", function() { + set("test.bool", "true"); + assert.equal(get("test.bool"), true); + set("test.bool", "false"); + assert.equal(get("test.bool"), false); + + assert.throws( + () => set("test.bool", "blah"), + new lib.InvalidValue("Expected type of test.bool to be boolean, not string") + ); + }); + it("should auto convert string to number if possible", function() { + for (let s of [ + "1", + "+23", + ".23e5", + "-.1e-2", + "100.0001", + "Infinity", + ]) { + set("test.func", s); + assert.equal(get("test.func"), Number.parseFloat(s)); + } + + assert.throws( + () => set("test.func", "blah"), + new lib.InvalidValue("Expected type of test.func to be number, not string") + ); + }); + it("should auto convert string to object", function() { + set("test.json", '{"json": true}'); + assert.deepEqual(get("test.json"), { json: true }); + + let errMsg; + try { + JSON.parse("blah"); + } catch (err) { + errMsg = err.message; + } + assert.throws( + () => set("test.json", "blah"), + new lib.InvalidValue(`Error parsing value for test.json: ${errMsg}`) + ); + }); + } + describe(".set()", function() { let testInstance; beforeEach(function() { @@ -602,114 +718,11 @@ describe("lib/config/classes", function() { }); }); - it("should set the value", function() { - testInstance.set("alpha.foo", "new value"); - assert.equal(testInstance.get("alpha.foo"), "new value"); - }); - - it("should throw if field does not exist", function() { - assert.throws( - () => testInstance.set("test.bar", 1), - new lib.InvalidField("No field named 'test.bar'") - ); - }); - - it("should throw if field is remotely inaccessible", function() { - assert.throws( - () => testInstance.set("test.priv", "bad", "remote"), - new lib.InvalidAccess("Field 'test.priv' is not accessible from remote") - ); - }); - it("should throw if field is locally inaccessible", function() { - testInstance = new TestConfig("remote"); - assert.throws( - () => testInstance.set("test.priv", "bad", "local"), - new lib.InvalidAccess("Field 'test.priv' is not accessible from remote") - ); - }); - - it("should throw if field is not in enum", function() { - assert.throws( - () => testInstance.set("test.enum", "bar"), - new Error("Expected one of [a, b, c], not bar") - ); - }); - - it("should work if field is in enum", function() { - testInstance.set("test.enum", "c"); - assert.equal(testInstance.get("test.enum"), "c"); - }); - - it("should throw if field is not optional", function() { - assert.throws( - () => testInstance.set("test.func", null), - new Error("Field test.func cannot be null") - ); - }); - - it("should work if field is optional", function() { - testInstance.set("test.test", "spam"); - assert.equal(testInstance.get("test.test"), "spam"); - }); - - it("should throw if field is of wrong type", function() { - assert.throws( - () => testInstance.set("test.test", 1), - new Error("Expected type of test.test to be string, not number") - ); - }); - - it("should treat empty string as null", function() { - testInstance.set("test.test", ""); - assert.equal(testInstance.get("test.test"), null); - }); - - it("should auto convert string to boolean if possible", function() { - testInstance.set("test.bool", "true"); - assert.equal(testInstance.get("test.bool"), true); - testInstance.set("test.bool", "false"); - assert.equal(testInstance.get("test.bool"), false); - - assert.throws( - () => testInstance.set("test.bool", "blah"), - new lib.InvalidValue("Expected type of test.bool to be boolean, not string") - ); - }); - - it("should auto convert string to number if possible", function() { - for (let s of [ - "1", - "+23", - ".23e5", - "-.1e-2", - "100.0001", - "Infinity", - ]) { - testInstance.set("test.func", s); - assert.equal(testInstance.get("test.func"), Number.parseFloat(s)); - } - - assert.throws( - () => testInstance.set("test.func", "blah"), - new lib.InvalidValue("Expected type of test.func to be number, not string") - ); - }); - - it("should auto convert string to object", function() { - testInstance.set("test.json", '{"json": true}'); - assert.deepEqual(testInstance.get("test.json"), { json: true }); - - let errMsg; - try { - JSON.parse("blah"); - } catch (err) { - errMsg = err.message; - } - assert.throws( - () => testInstance.set("test.json", "blah"), - new lib.InvalidValue(`Error parsing value for test.json: ${errMsg}`) - ); - }); + testAccessAndCoerce( + (...args) => testInstance.set(...args), + (...args) => testInstance.get(...args), + config => { testInstance = config; }, + ); it("should notify fieldChanged listeners", function() { const changes = []; testInstance.on("fieldChanged", (name, value, prev) => changes.push([name, value, prev])); @@ -734,7 +747,6 @@ describe("lib/config/classes", function() { testInstance.set("test.validated", 5); assert.equal(testInstance.get("test.validated"), 5); }); - it("should throw if validator fails", function() { assert.throws( () => testInstance.set("test.validated", -1), @@ -755,6 +767,55 @@ describe("lib/config/classes", function() { }); }); + /** + * @param {typeof lib.Config.prototype.set} set + * @param {typeof lib.Config.prototype.setProp} setProp + * @param {typeof lib.Config.prototype.get} get + * @param {(config: lib.Config) => void} replaceConfig + */ + function testAccessAndCoerceProp(set, setProp, get, replaceConfig) { + it("should throw if field does not exist", function() { + assert.throws( + () => setProp("test.bar", 1), + new lib.InvalidField("No field named 'test.bar'") + ); + }); + it("should throw if field is remotely inaccessible", function() { + assert.throws( + () => setProp("test.priv", "prop", "bad", "remote"), + new lib.InvalidAccess("Field 'test.priv' is not accessible from remote") + ); + }); + it("should throw if field is locally inaccessible", function() { + replaceConfig(new TestConfig("remote")); + assert.throws( + () => setProp("test.priv", "prop", "bad", "local"), + new lib.InvalidAccess("Field 'test.priv' is not accessible from remote") + ); + }); + it("should throw if field is not an object", function() { + assert.throws( + () => setProp("test.enum", "prop", "a"), + new lib.InvalidField("Cannot set property on non-object field 'test.enum'") + ); + }); + it("should work if field is an object", function() { + set("test.json", { prev: 32, test: false }); + setProp("test.json", "test", true); + assert.deepEqual(get("test.json"), { prev: 32, test: true }); + }); + it("should handle field being null", function() { + set("test.json", null); + setProp("test.json", "test", true); + assert.deepEqual(get("test.json"), { test: true }); + }); + it("should unset field if passed undefined", function() { + set("test.json", { test: true, extra: "yes" }); + setProp("test.json", "extra", undefined); + assert.deepEqual(get("test.json"), { test: true }); + }); + } + describe(".setProp()", function() { let testInstance; beforeEach(function() { @@ -765,51 +826,12 @@ describe("lib/config/classes", function() { }); }); - it("should throw if field does not exist", function() { - assert.throws( - () => testInstance.setProp("test.bar", 1), - new lib.InvalidField("No field named 'test.bar'") - ); - }); - - it("should throw if field is remotely inaccessible", function() { - assert.throws( - () => testInstance.setProp("test.priv", "prop", "bad", "remote"), - new lib.InvalidAccess("Field 'test.priv' is not accessible from remote") - ); - }); - it("should throw if field is locally inaccessible", function() { - testInstance = new TestConfig("remote"); - assert.throws( - () => testInstance.setProp("test.priv", "prop", "bad", "local"), - new lib.InvalidAccess("Field 'test.priv' is not accessible from remote") - ); - }); - - it("should throw if field is not an object", function() { - assert.throws( - () => testInstance.setProp("test.enum", "prop", "a"), - new lib.InvalidField("Cannot set property on non-object field 'test.enum'") - ); - }); - - it("should work if field is an object", function() { - testInstance.set("test.json", { prev: 32, test: false }); - testInstance.setProp("test.json", "test", true); - assert.deepEqual(testInstance.get("test.json"), { prev: 32, test: true }); - }); - - it("should handle field being null", function() { - testInstance.set("test.json", null); - testInstance.setProp("test.json", "test", true); - assert.deepEqual(testInstance.get("test.json"), { test: true }); - }); - - it("should unset field if passed undefined", function() { - testInstance.set("test.json", { test: true, extra: "yes" }); - testInstance.setProp("test.json", "extra", undefined); - assert.deepEqual(testInstance.get("test.json"), { test: true }); - }); + testAccessAndCoerceProp( + (...args) => testInstance.set(...args), + (...args) => testInstance.setProp(...args), + (...args) => testInstance.get(...args), + config => { testInstance = config; }, + ); it("should notify fieldChanged listeners", function() { const changes = []; testInstance.on("fieldChanged", (name, value, prev) => changes.push([name, value, prev])); @@ -840,6 +862,16 @@ describe("lib/config/classes", function() { new lib.InvalidValue("Failed validation of test.objValidated: bad key") ); }); + it("should validate dependent fields", function() { + testInstance.setProp("test.objDependent", "base", 1); + assert.deepEqual(testInstance.get("test.objDependent"), { base: 1 }); + }); + it("should throw if dependent validation fails", function() { + assert.throws( + () => testInstance.setProp("test.objDependent", "base", 5), + new lib.InvalidValue("Failed validation of test.objDependent: base prop must match test.base") + ); + }); }); describe("get .staging", function() { @@ -874,12 +906,11 @@ describe("lib/config/classes", function() { assert.equal(testInstance.get("alpha.foo"), null); assert.equal(testInstance.staging.get("alpha.foo"), "value"); }); - it("should throw if field does not exist", function() { - assert.throws( - () => testInstance.stage("invalid", "value"), - new lib.InvalidField("No field named 'invalid'") - ); - }); + testAccessAndCoerce( + (...args) => testInstance.stage(...args), + (...args) => testInstance.staging.get(...args), + config => { testInstance = config; }, + ); }); describe(".stageProp()", function() { @@ -888,6 +919,12 @@ describe("lib/config/classes", function() { testInstance = new TestConfig("local"); }); + testAccessAndCoerceProp( + (...args) => testInstance.stage(...args), + (...args) => testInstance.stageProp(...args), + (...args) => testInstance.staging.get(...args), + config => { testInstance = config; }, + ); it("should stage object property change", function() { testInstance.stageProp("test.json", "prop", true); @@ -902,12 +939,6 @@ describe("lib/config/classes", function() { assert.deepEqual(testInstance.staging.get("test.json"), {}); assert.deepEqual(testInstance.get("test.json"), { a: 1 }); }); - it("should throw if field is not object", function() { - assert.throws( - () => testInstance.stageProp("test.enum", "prop", true), - new lib.InvalidValue("Cannot set property on non-object field 'test.enum'") - ); - }); }); describe(".commitStaging()", function() { @@ -944,6 +975,32 @@ describe("lib/config/classes", function() { assert.equal(testInstance.get("test.base"), 10); assert.equal(testInstance.get("test.dependent"), 15); }); + it("should notify fieldChanged listeners", function() { + const changes = []; + testInstance.on("fieldChanged", (name, value, prev) => changes.push([name, value, prev])); + + testInstance.stage("alpha.foo", "foo"); + testInstance.stage("test.test", "bar"); + testInstance.commitStaging(); + + assert.deepEqual(changes, [ + ["alpha.foo", "foo", null], + ["test.test", "bar", null], + ], "fieldChanged emitted incorrectly"); + }); + it("should set the dirty flag", function() { + testInstance.stage("test.test", "foo"); + testInstance.commitStaging(); + assert.deepEqual(testInstance.dirty, true, "dirty flag not set"); + }); + it("should set the requires restart flag", function() { + testInstance.stage("test.test", "foo"); + testInstance.commitStaging(); + assert.deepEqual(testInstance.restartRequired, false, "restart required when no restart field changed"); + testInstance.stage("test.enum", "a"); + testInstance.commitStaging(); + assert.deepEqual(testInstance.restartRequired, true, "restart required not set"); + }); }); describe(".revertStaging()", function() { From 8217acef3f30468ba44b4e3d7adc7f4f9d82650e Mon Sep 17 00:00:00 2001 From: Cooldude2606 <25043174+Cooldude2606@users.noreply.github.com> Date: Tue, 17 Mar 2026 23:42:41 +0000 Subject: [PATCH 15/20] Add coverage of config requests --- test/messages/controller.test.js | 343 +++++++++++++++++++++++++++++++ test/messages/host.test.js | 137 ++++++++++++ 2 files changed, 480 insertions(+) diff --git a/test/messages/controller.test.js b/test/messages/controller.test.js index a4d3c66e..3042f82e 100644 --- a/test/messages/controller.test.js +++ b/test/messages/controller.test.js @@ -3,12 +3,15 @@ const assert = require("assert").strict; const lib = require("@clusterio/lib"); const { Controller, ControlConnection } = require("@clusterio/controller"); +const { testMatrix, testRoundTripJsonSerialisable } = require("../common"); describe("messages/controller", function() { /** @type {Controller} */ let controller; /** @type {ControlConnection} */ let controlConnection; + /** @type {lib.InstanceConfig} */ + let instanceConfig; beforeEach(function() { const controllerConfig = new lib.ControllerConfig("controller"); @@ -17,6 +20,8 @@ describe("messages/controller", function() { lib.Address.fromShorthand({ controlId: 1 }), ); controller = new Controller(lib.logger, [], controllerConfig); + instanceConfig = new lib.InstanceConfig("controller"); + controller.instances.createInstance(instanceConfig); const user = controller.users.getOrCreateUser("test"); controlConnection = new ControlConnection({ version: "2.0.0" }, connection, controller, user, 1); }); @@ -35,6 +40,344 @@ describe("messages/controller", function() { }); }); + describe("ControllerConfigGetRequest", function() { + it("runs", async function() { + const config = await controlConnection.handleControllerConfigGetRequest( + new lib.ControllerConfigGetRequest() + ); + assert.deepEqual(config, controller.config.toRemote("control")); + }); + }); + + describe("ControllerConfigSetFieldRequest", function() { + it("is round trip json serialisable", function() { + testRoundTripJsonSerialisable(lib.ControllerConfigSetFieldRequest, testMatrix( + ["controller.name", "invalid"], // field + ["Foo Bar", "true", "5", "{}"], // value + )); + }); + it("runs", async function() { + await controlConnection.handleControllerConfigSetFieldRequest( + new lib.ControllerConfigSetFieldRequest("controller.name", "Foo Bar") + ); + assert.equal(controller.config.get("controller.name"), "Foo Bar"); + }); + it("rejects if the field does not exist", async function() { + await assert.rejects( + controlConnection.handleControllerConfigSetFieldRequest( + new lib.ControllerConfigSetFieldRequest("invalid", "Foo Bar") + ), + /No field named 'invalid'/ + ); + }); + it("rejects if the field is inaccessible", async function() { + await assert.rejects( + controlConnection.handleControllerConfigSetFieldRequest( + new lib.ControllerConfigSetFieldRequest("controller.version", "2.0.0") + ), + /Field 'controller.version' is not accessible from control/ + ); + }); + }); + + describe("ControllerConfigSetPropRequest", function() { + it("is round trip json serialisable", function() { + testRoundTripJsonSerialisable(lib.ControllerConfigSetPropRequest, testMatrix( + ["controller.name", "invalid"], // field + ["foo", "bar"], // prop + ["Foo Bar", true, 5, {}], // value + )); + }); + it("runs", async function() { + this.skip(); // No object fields on controller + }); + it("rejects if the field does not exist", async function() { + await assert.rejects( + controlConnection.handleControllerConfigSetPropRequest( + new lib.ControllerConfigSetPropRequest("invalid", "foo", "bar") + ), + /No field named 'invalid'/ + ); + }); + it("rejects if the field is not an object", async function() { + await assert.rejects( + controlConnection.handleControllerConfigSetPropRequest( + new lib.ControllerConfigSetPropRequest("controller.name", "foo", "bar") + ), + /Cannot set property on non-object field 'controller\.name'/ + ); + }); + it("rejects if the field is inaccessible", async function() { + await assert.rejects( + controlConnection.handleControllerConfigSetPropRequest( + new lib.ControllerConfigSetPropRequest("controller.version", "foo", "bar") + ), + /Field 'controller\.version' is not accessible from control/ + ); + }); + }); + + describe("ControllerConfigSetRequest", function() { + it("is round trip json serialisable", function() { + testRoundTripJsonSerialisable(lib.ControllerConfigSetRequest, testMatrix( + [ // fields + { "foo": "bar", "baz": { "cat": "dog" } }, + { "foo": "5", "baz": { "cat": "5" } }, + { "foo": "true", "baz": { "cat": "true" } }, + { "foo": "{}", "baz": { "cat": "{}" } }, + ], + )); + }); + it("runs", async function() { + await controlConnection.handleControllerConfigSetRequest(new lib.ControllerConfigSetRequest({ + "controller.name": "Foo Bar", + "controller.mods_directory": "Bar Baz", + })); + assert.equal(controller.config.get("controller.name"), "Foo Bar"); + assert.equal(controller.config.get("controller.mods_directory"), "Bar Baz"); + }); + it("rejects if the field does not exist", async function() { + await assert.rejects( + controlConnection.handleControllerConfigSetRequest(new lib.ControllerConfigSetRequest({ + "controller.name": "Foo Bar", + "invalid": "Bar Baz", + })), + /No field named 'invalid'/ + ); + assert.equal(controller.config.get("controller.name"), "Your Cluster"); + }); + it("rejects if the field is not an object", async function() { + const prevDir = controller.config.get("controller.mods_directory"); + await assert.rejects( + controlConnection.handleControllerConfigSetRequest(new lib.ControllerConfigSetRequest({ + "controller.name": "Foo Bar", + "controller.mods_directory": { + "foo": "bar", + }, + })), + /Cannot set property on non-object field 'controller\.mods_directory'/ + ); + assert.equal(controller.config.get("controller.name"), "Your Cluster"); + assert.equal(controller.config.get("controller.mods_directory"), prevDir); + }); + it("rejects if the field is inaccessible", async function() { + const prevVersion = controller.config.get("controller.version"); + await assert.rejects( + controlConnection.handleControllerConfigSetRequest(new lib.ControllerConfigSetRequest({ + "controller.name": "Foo Bar", + "controller.version": "2.0.0", + })), + /Field 'controller\.version' is not accessible from control/ + ); + assert.equal(controller.config.get("controller.name"), "Your Cluster"); + assert.equal(controller.config.get("controller.version"), prevVersion); + }); + }); + + describe("InstanceConfigGetRequest", function() { + it("runs", async function() { + const config = await controlConnection.handleInstanceConfigGetRequest( + new lib.InstanceConfigGetRequest(instanceConfig.get("instance.id")) + ); + assert.deepEqual(config, instanceConfig.toRemote("control")); + }); + }); + + describe("InstanceConfigSetFieldRequest", function() { + it("is round trip json serialisable", function() { + testRoundTripJsonSerialisable(lib.InstanceConfigSetFieldRequest, testMatrix( + [1], // instance id + ["controller.name", "invalid"], // field + ["Foo Bar", "true", "5", "{}"], // value + )); + }); + it("runs", async function() { + await controlConnection.handleInstanceConfigSetFieldRequest( + new lib.InstanceConfigSetFieldRequest(instanceConfig.get("instance.id"), "instance.name", "Foo Bar") + ); + assert.equal(instanceConfig.get("instance.name"), "Foo Bar"); + }); + it("rejects if the field does not exist", async function() { + await assert.rejects( + controlConnection.handleInstanceConfigSetFieldRequest( + new lib.InstanceConfigSetFieldRequest(instanceConfig.get("instance.id"), "invalid", "Foo Bar") + ), + /No field named 'invalid'/ + ); + }); + it("rejects if the field is inaccessible", async function() { + await assert.rejects( + controlConnection.handleInstanceConfigSetFieldRequest( + new lib.InstanceConfigSetFieldRequest( + instanceConfig.get("instance.id"), "factorio.host_assigned_game_port", "3000" + ) + ), + /Field 'factorio\.host_assigned_game_port' is not accessible from control/ + ); + }); + it("rejects setting 'instance.assigned_host'", async function() { + await assert.rejects( + controlConnection.handleInstanceConfigSetFieldRequest( + new lib.InstanceConfigSetFieldRequest( + instanceConfig.get("instance.id"), "instance.assigned_host", 5 + ) + ), + /instance\.assigned_host must be set through the assign-host interface/ + ); + }); + it("rejects setting 'instance.id'", async function() { + await assert.rejects( + controlConnection.handleInstanceConfigSetFieldRequest( + new lib.InstanceConfigSetFieldRequest( + instanceConfig.get("instance.id"), "instance.id", 5 + ) + ), + /Setting instance\.id is not supported/ + ); + }); + }); + + describe("InstanceConfigSetPropRequest", function() { + it("is round trip json serialisable", function() { + testRoundTripJsonSerialisable(lib.InstanceConfigSetPropRequest, testMatrix( + [1], // instance id + ["controller.name", "invalid"], // field + ["foo", "bar"], // prop + ["Foo Bar", true, 5, {}], // value + )); + }); + it("runs", async function() { + controlConnection.handleInstanceConfigSetPropRequest( + new lib.InstanceConfigSetPropRequest( + instanceConfig.get("instance.id"), "factorio.settings", "name", "Foo Bar" + ) + ); + const { name, ...rest } = instanceConfig.get("factorio.settings"); + assert.equal(name, "Foo Bar"); + }); + it("rejects if the field does not exist", async function() { + await assert.rejects( + controlConnection.handleInstanceConfigSetPropRequest( + new lib.InstanceConfigSetPropRequest(instanceConfig.get("instance.id"), "invalid", "foo", "bar") + ), + /No field named 'invalid'/ + ); + }); + it("rejects if the field is not an object", async function() { + await assert.rejects( + controlConnection.handleInstanceConfigSetPropRequest( + new lib.InstanceConfigSetPropRequest( + instanceConfig.get("instance.id"), "instance.name", "foo", "bar" + ) + ), + /Cannot set property on non-object field 'instance\.name'/ + ); + }); + it("rejects if the field is inaccessible", async function() { + await assert.rejects( + controlConnection.handleInstanceConfigSetPropRequest( + new lib.InstanceConfigSetPropRequest( + instanceConfig.get("instance.id"), "factorio.host_assigned_game_port", "foo", "bar" + ) + ), + /Field 'factorio\.host_assigned_game_port' is not accessible from control/ + ); + }); + }); + + describe("InstanceConfigSetRequest", function() { + it("is round trip json serialisable", function() { + testRoundTripJsonSerialisable(lib.InstanceConfigSetRequest, testMatrix( + [1], // instance id + [ // fields + { "foo": "bar", "baz": { "cat": "dog" } }, + { "foo": "5", "baz": { "cat": "5" } }, + { "foo": "true", "baz": { "cat": "true" } }, + { "foo": "{}", "baz": { "cat": "{}" } }, + ], + )); + }); + it("runs", async function() { + await controlConnection.handleInstanceConfigSetRequest(new lib.InstanceConfigSetRequest( + instanceConfig.get("instance.id"), { + "instance.name": "Foo Bar", + "factorio.executable_path": "Bar Baz", + }) + ); + assert.equal(instanceConfig.get("instance.name"), "Foo Bar"); + assert.equal(instanceConfig.get("factorio.executable_path"), "Bar Baz"); + }); + it("rejects if the field does not exist", async function() { + await assert.rejects( + controlConnection.handleInstanceConfigSetRequest(new lib.InstanceConfigSetRequest( + instanceConfig.get("instance.id"), { + "instance.name": "Foo Bar", + "invalid": "Bar Baz", + }) + ), + /No field named 'invalid'/ + ); + assert.equal(instanceConfig.get("instance.name"), "New Instance"); + }); + it("rejects if the field is not an object", async function() { + const prevPath = instanceConfig.get("factorio.executable_path"); + await assert.rejects( + controlConnection.handleInstanceConfigSetRequest(new lib.InstanceConfigSetRequest( + instanceConfig.get("instance.id"), { + "instance.name": "Foo Bar", + "factorio.executable_path": { + "foo": "bar", + }, + }) + ), + /Cannot set property on non-object field 'factorio\.executable_path'/ + ); + assert.equal(instanceConfig.get("instance.name"), "New Instance"); + assert.equal(instanceConfig.get("factorio.executable_path"), prevPath); + }); + it("rejects if the field is inaccessible", async function() { + await assert.rejects( + controlConnection.handleInstanceConfigSetRequest(new lib.InstanceConfigSetRequest( + instanceConfig.get("instance.id"), { + "instance.name": "Foo Bar", + "factorio.host_assigned_game_port": "Bar Baz", + }) + ), + /Field 'factorio\.host_assigned_game_port' is not accessible from control/ + ); + assert.equal(instanceConfig.get("instance.name"), "New Instance"); + // Can't check value of host_assigned_game_port as it is not accessible on controller + }); + it("rejects setting 'instance.assigned_host'", async function() { + const prevHost = instanceConfig.get("instance.assigned_host"); + await assert.rejects( + controlConnection.handleInstanceConfigSetRequest(new lib.InstanceConfigSetRequest( + instanceConfig.get("instance.id"), { + "instance.name": "Foo Bar", + "instance.assigned_host": 5, + }) + ), + /instance\.assigned_host must be set through the assign-host interface/ + ); + assert.equal(instanceConfig.get("instance.name"), "New Instance"); + assert.equal(instanceConfig.get("instance.assigned_host"), prevHost); + }); + it("rejects setting 'instance.id'", async function() { + const prevId = instanceConfig.get("instance.id"); + await assert.rejects( + controlConnection.handleInstanceConfigSetRequest(new lib.InstanceConfigSetRequest( + instanceConfig.get("instance.id"), { + "instance.name": "Foo Bar", + "instance.id": 5, + }) + ), + /Setting instance\.id is not supported/ + ); + assert.equal(instanceConfig.get("instance.name"), "New Instance"); + assert.equal(instanceConfig.get("instance.id"), prevId); + }); + }); + describe("External", function() { it("runs", async function() { let callCount = 0; diff --git a/test/messages/host.test.js b/test/messages/host.test.js index 56b1d121..285d37c4 100644 --- a/test/messages/host.test.js +++ b/test/messages/host.test.js @@ -3,6 +3,7 @@ const assert = require("assert").strict; const lib = require("@clusterio/lib"); const { Host } = require("@clusterio/host"); +const { testMatrix, testRoundTripJsonSerialisable } = require("../common"); describe("messages/host", function() { /** @type {Host} */ @@ -30,4 +31,140 @@ describe("messages/host", function() { ); }); }); + + describe("HostConfigGetRequest", function() { + it("runs", async function() { + const config = await host.handleHostConfigGetRequest(new lib.HostConfigGetRequest()); + assert.deepEqual(config, host.config.toRemote("control")); + }); + }); + + describe("HostConfigSetFieldRequest", function() { + it("is round trip json serialisable", function() { + testRoundTripJsonSerialisable(lib.HostConfigSetFieldRequest, testMatrix( + ["controller.name", "invalid"], // field + ["Foo Bar", "true", "5", "{}"], // value + )); + }); + it("runs", async function() { + await host.handleHostConfigSetFieldRequest(new lib.HostConfigSetFieldRequest("host.name", "Foo Bar")); + assert.equal(host.config.get("host.name"), "Foo Bar"); + }); + it("rejects if the field does not exist", async function() { + await assert.rejects( + host.handleHostConfigSetFieldRequest(new lib.HostConfigSetFieldRequest("invalid", "Foo Bar")), + /No field named 'invalid'/ + ); + }); + it("rejects if the field is inaccessible", async function() { + await assert.rejects( + host.handleHostConfigSetFieldRequest(new lib.HostConfigSetFieldRequest("host.version", "2.0.0")), + /Field 'host.version' is not accessible from control/ + ); + }); + it("rejects setting 'host.id'", async function() { + await assert.rejects( + host.handleHostConfigSetFieldRequest(new lib.HostConfigSetFieldRequest("host.id", 5)), + /Setting 'host.id' while host is running is not supported/ + ); + }); + }); + + describe("HostConfigSetPropRequest", function() { + it("is round trip json serialisable", function() { + testRoundTripJsonSerialisable(lib.HostConfigSetPropRequest, testMatrix( + ["controller.name", "invalid"], // field + ["foo", "bar"], // prop + ["Foo Bar", true, 5, {}], // value + )); + }); + it("runs", async function() { + this.skip(); // No object fields on host + }); + it("rejects if the field does not exist", async function() { + await assert.rejects( + host.handleHostConfigSetPropRequest(new lib.HostConfigSetPropRequest("invalid", "foo", "bar")), + /No field named 'invalid'/ + ); + }); + it("rejects if the field is not an object", async function() { + await assert.rejects( + host.handleHostConfigSetPropRequest(new lib.HostConfigSetPropRequest("host.name", "foo", "bar")), + /Cannot set property on non-object field 'host\.name'/ + ); + }); + it("rejects if the field is inaccessible", async function() { + await assert.rejects( + host.handleHostConfigSetPropRequest(new lib.HostConfigSetPropRequest("host.version", "foo", "bar")), + /Field 'host\.version' is not accessible from control/ + ); + }); + }); + + describe("HostConfigSetRequest", function() { + it("is round trip json serialisable", function() { + testRoundTripJsonSerialisable(lib.HostConfigSetRequest, testMatrix( + [ // fields + { "foo": "bar", "baz": { "cat": "dog" } }, + { "foo": "5", "baz": { "cat": "5" } }, + { "foo": "true", "baz": { "cat": "true" } }, + { "foo": "{}", "baz": { "cat": "{}" } }, + ], + )); + }); + it("runs", async function() { + await host.handleHostConfigSetRequest(new lib.HostConfigSetRequest({ + "host.name": "Foo Bar", + "host.factorio_directory": "Bar Baz", + })); + assert.equal(host.config.get("host.name"), "Foo Bar"); + assert.equal(host.config.get("host.factorio_directory"), "Bar Baz"); + }); + it("rejects if the field does not exist", async function() { + await assert.rejects( + host.handleHostConfigSetRequest(new lib.HostConfigSetRequest({ + "host.name": "Foo Bar", + "invalid": "Bar Baz", + })), + /No field named 'invalid'/ + ); + assert.equal(host.config.get("host.name"), "New Host"); + }); + it("rejects if the field is not an object", async function() { + const prevDir = host.config.get("host.factorio_directory"); + await assert.rejects( + host.handleHostConfigSetRequest(new lib.HostConfigSetRequest({ + "host.name": "Foo Bar", + "host.factorio_directory": { + "foo": "bar", + }, + })), + /Cannot set property on non-object field 'host\.factorio_directory'/ + ); + assert.equal(host.config.get("host.name"), "New Host"); + assert.equal(host.config.get("host.factorio_directory"), prevDir); + }); + it("rejects if the field is inaccessible", async function() { + const prevVersion = host.config.get("host.version"); + await assert.rejects( + host.handleHostConfigSetRequest(new lib.HostConfigSetRequest({ + "host.name": "Foo Bar", + "host.version": "2.0.0", + })), + /Field 'host\.version' is not accessible from control/ + ); + assert.equal(host.config.get("host.name"), "New Host"); + assert.equal(host.config.get("host.version"), prevVersion); + }); + it("rejects setting 'host.id'", async function() { + await assert.rejects( + host.handleHostConfigSetRequest(new lib.HostConfigSetRequest({ + "host.name": "Foo Bar", + "host.id": 5, + })), + /Setting 'host.id' while host is running is not supported/ + ); + assert.equal(host.config.get("host.name"), "New Host"); + }); + }); }); From ed4fe3230d5307d51b5bd537990ad19e953710b7 Mon Sep 17 00:00:00 2001 From: Cooldude2606 <25043174+Cooldude2606@users.noreply.github.com> Date: Wed, 18 Mar 2026 00:03:44 +0000 Subject: [PATCH 16/20] Fixed test causing missed coverage --- test/lib/config/classes.js | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/test/lib/config/classes.js b/test/lib/config/classes.js index 291bdd98..12a8f1bd 100644 --- a/test/lib/config/classes.js +++ b/test/lib/config/classes.js @@ -46,11 +46,15 @@ describe("lib/config/classes", function() { "test.dependent": { type: "number", initialValue: 1, - dependsOn: ["test.base"], + dependsOn: ["test.base", "test.objDependent"], validator(value, config) { if (value < config.get("test.base")) { throw new Error("dependent must be >= base"); } + const dep = config.get("test.objDependent")?.base; + if (dep === 0) { + throw new Error("objDependent cannot be 0"); + } }, }, "test.objDependent": { @@ -58,7 +62,7 @@ describe("lib/config/classes", function() { initialValue: {}, dependsOn: ["test.base"], validator(value, config) { - if (value.base !== undefined && value.base !== config.get("test.base")) { + if (value.base && value.base !== config.get("test.base")) { throw new Error("base prop must match test.base"); } }, @@ -867,9 +871,10 @@ describe("lib/config/classes", function() { assert.deepEqual(testInstance.get("test.objDependent"), { base: 1 }); }); it("should throw if dependent validation fails", function() { + testInstance.set("test.base", 0); assert.throws( - () => testInstance.setProp("test.objDependent", "base", 5), - new lib.InvalidValue("Failed validation of test.objDependent: base prop must match test.base") + () => testInstance.setProp("test.objDependent", "base", 0), + new lib.InvalidValue("Failed validation of dependent test.dependent: objDependent cannot be 0") ); }); }); From f0fb52ab7454c4577ba706451042e43d205a9ff5 Mon Sep 17 00:00:00 2001 From: Cooldude2606 <25043174+Cooldude2606@users.noreply.github.com> Date: Wed, 18 Mar 2026 00:17:17 +0000 Subject: [PATCH 17/20] Revert change to mock exports --- test/mock.js | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/test/mock.js b/test/mock.js index dd1c5d2f..7cb9901f 100644 --- a/test/mock.js +++ b/test/mock.js @@ -241,15 +241,16 @@ async function createInstancePlugin(InstancePluginClass, info) { return plugin; } + module.exports = { - MockLogger: MockLogger, - MockSocket: MockSocket, - MockConnector: MockConnector, - MockServer: MockServer, - MockInstance: MockInstance, - MockHost: MockHost, - MockControl: MockControl, - MockController: MockController, + MockLogger, + MockSocket, + MockConnector, + MockServer, + MockInstance, + MockHost, + MockControl, + MockController, createControllerPlugin, createInstancePlugin, From 6ade75e4cd2631e35df4bdd2e9a5b16f19c515a2 Mon Sep 17 00:00:00 2001 From: Cooldude2606 <25043174+Cooldude2606@users.noreply.github.com> Date: Fri, 20 Mar 2026 17:07:54 +0000 Subject: [PATCH 18/20] Fix incorrect config validators --- packages/lib/src/config/definitions.ts | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/packages/lib/src/config/definitions.ts b/packages/lib/src/config/definitions.ts index 2d3fa645..37eb17fa 100644 --- a/packages/lib/src/config/definitions.ts +++ b/packages/lib/src/config/definitions.ts @@ -190,7 +190,7 @@ export class ControllerConfig extends classes.Config { description: "Interval in seconds to collect and update system metrics for the Web UI", type: "number", initialValue: 10, - validator: validators.greaterThanZero, + validator: validators.greaterThanEqualZero, }, "controller.proxy_stream_timeout": { title: "Proxy Stream Timeout", @@ -213,7 +213,6 @@ export class ControllerConfig extends classes.Config { type: "string", credential: ["controller"], optional: true, - // TODO extendedValidation, lowercase hex characters, maybe length? cannot be set without username }, "controller.share_factorio_credentials_with_hosts": { title: "Share Factorio credentials with Hosts", @@ -389,7 +388,6 @@ export class HostConfig extends classes.Config { type: "string", credential: ["host"], optional: true, - // TODO extendedValidation, lowercase hex characters, maybe length? cannot be set without username }, "host.max_reconnect_delay": { title: "Max Reconnect Delay", @@ -536,7 +534,7 @@ export class InstanceConfig extends classes.Config { restartRequired: true, type: "number", optional: true, - validator: validators.optional(validators.greaterThanZero), + validator: validators.optional(validators.greaterThanEqualZero), }, "factorio.host_assigned_game_port": { access: ["host"], @@ -549,7 +547,7 @@ export class InstanceConfig extends classes.Config { restartRequired: true, type: "number", optional: true, - validator: validators.optional(validators.greaterThanZero), + validator: validators.optional(validators.greaterThanEqualZero), }, "factorio.rcon_password": { credential: ["host", "controller"], From 5d0985cafcb0e30c32c3e5e2900359fc0877296d Mon Sep 17 00:00:00 2001 From: Cooldude2606 <25043174+Cooldude2606@users.noreply.github.com> Date: Fri, 20 Mar 2026 17:42:48 +0000 Subject: [PATCH 19/20] Fix tests not checking that validators are called --- test/lib/config/classes.js | 59 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/test/lib/config/classes.js b/test/lib/config/classes.js index 12a8f1bd..8fa39999 100644 --- a/test/lib/config/classes.js +++ b/test/lib/config/classes.js @@ -11,6 +11,7 @@ describe("lib/config/classes", function() { describe("Config", function() { class TestConfig extends lib.Config { static defaultAccess = ["local", "remote"]; + static validatorCalls = []; static migrations(config) { if (config.hasOwnProperty("test.migration")) { @@ -48,6 +49,7 @@ describe("lib/config/classes", function() { initialValue: 1, dependsOn: ["test.base", "test.objDependent"], validator(value, config) { + TestConfig.validatorCalls.push(["test.dependent", value]); if (value < config.get("test.base")) { throw new Error("dependent must be >= base"); } @@ -62,6 +64,7 @@ describe("lib/config/classes", function() { initialValue: {}, dependsOn: ["test.base"], validator(value, config) { + TestConfig.validatorCalls.push(["test.objDependent", value]); if (value.base && value.base !== config.get("test.base")) { throw new Error("base prop must match test.base"); } @@ -71,6 +74,7 @@ describe("lib/config/classes", function() { type: "number", optional: true, validator(value) { + TestConfig.validatorCalls.push(["test.validated", value]); if (value !== null && value < 0) { throw new Error("must be positive"); } @@ -80,6 +84,7 @@ describe("lib/config/classes", function() { type: "object", initialValue: {}, validator(value) { + TestConfig.validatorCalls.push(["test.objValidated", value]); if (value && value.bad !== undefined) { throw new Error("bad key"); } @@ -715,6 +720,7 @@ describe("lib/config/classes", function() { describe(".set()", function() { let testInstance; beforeEach(function() { + TestConfig.validatorCalls = []; testInstance = new TestConfig("local", { "test.enum": "a", "test.test": "blah", @@ -750,24 +756,38 @@ describe("lib/config/classes", function() { it("should run field validator successfully", function() { testInstance.set("test.validated", 5); assert.equal(testInstance.get("test.validated"), 5); + assert.deepEqual(TestConfig.validatorCalls, [ + ["test.validated", 5], // set test.validated + ]); }); it("should throw if validator fails", function() { assert.throws( () => testInstance.set("test.validated", -1), new lib.InvalidValue("Failed validation of test.validated: must be positive") ); + assert.deepEqual(TestConfig.validatorCalls, [ + ["test.validated", -1], // set test.validated + ]); }); it("should validate dependent fields", function() { testInstance.set("test.dependent", 10); testInstance.set("test.base", 5); assert.equal(testInstance.get("test.dependent"), 10); assert.equal(testInstance.get("test.base"), 5); + assert.deepEqual(TestConfig.validatorCalls, [ + ["test.dependent", 10], // set test.dependent + ["test.dependent", 10], // set test.base + ["test.objDependent", {}], // set test.base + ]); }); it("should throw if dependent validation fails", function() { assert.throws( () => testInstance.set("test.base", 5), new lib.InvalidValue("Failed validation of dependent test.dependent: dependent must be >= base") ); + assert.deepEqual(TestConfig.validatorCalls, [ + ["test.dependent", 1], // set test.base + ]); }); }); @@ -823,6 +843,7 @@ describe("lib/config/classes", function() { describe(".setProp()", function() { let testInstance; beforeEach(function() { + TestConfig.validatorCalls = []; testInstance = new TestConfig("local", { "test.enum": "a", "test.test": "blah", @@ -859,16 +880,26 @@ describe("lib/config/classes", function() { it("should run validator when setting object property", function() { testInstance.setProp("test.objValidated", "good", true); assert.deepEqual(testInstance.get("test.objValidated"), { good: true }); + assert.deepEqual(TestConfig.validatorCalls, [ + ["test.objValidated", { good: true }], // setProp test.objValidated good + ]); }); it("should throw if object validator fails", function() { assert.throws( () => testInstance.setProp("test.objValidated", "bad", true), new lib.InvalidValue("Failed validation of test.objValidated: bad key") ); + assert.deepEqual(TestConfig.validatorCalls, [ + ["test.objValidated", { bad: true }], // setProp test.objValidated bad + ]); }); it("should validate dependent fields", function() { testInstance.setProp("test.objDependent", "base", 1); assert.deepEqual(testInstance.get("test.objDependent"), { base: 1 }); + assert.deepEqual(TestConfig.validatorCalls, [ + ["test.objDependent", { base: 1 }], // setProp test.objDependent base + ["test.dependent", 1], // setProp test.objDependent base + ]); }); it("should throw if dependent validation fails", function() { testInstance.set("test.base", 0); @@ -876,6 +907,12 @@ describe("lib/config/classes", function() { () => testInstance.setProp("test.objDependent", "base", 0), new lib.InvalidValue("Failed validation of dependent test.dependent: objDependent cannot be 0") ); + assert.deepEqual(TestConfig.validatorCalls, [ + ["test.dependent", 1], // set test.base + ["test.objDependent", {}], // set test.base + ["test.objDependent", { base: 0 }], // setProp test.objDependent base + ["test.dependent", 1], // setProp test.objDependent base + ]); }); }); @@ -949,6 +986,7 @@ describe("lib/config/classes", function() { describe(".commitStaging()", function() { let testInstance; beforeEach(function() { + TestConfig.validatorCalls = []; testInstance = new TestConfig("local"); }); @@ -962,6 +1000,13 @@ describe("lib/config/classes", function() { assert.equal(testInstance.get("alpha.foo"), null); testInstance.commitStaging(); assert.equal(testInstance.get("alpha.foo"), "value"); + assert.deepEqual(TestConfig.validatorCalls, [ + // commitStaging, all values are checked once + ["test.dependent", 1], + ["test.objDependent", {}], + ["test.validated", null], + ["test.objValidated", {}], + ]); }); it("should throw if staged validation fails", function() { testInstance.stage("test.validated", -1); @@ -970,6 +1015,12 @@ describe("lib/config/classes", function() { () => testInstance.commitStaging(), new lib.InvalidValue("Failed validation of test.validated: must be positive") ); + assert.deepEqual(TestConfig.validatorCalls, [ + // commitStaging, all values are checked once + ["test.dependent", 1], + ["test.objDependent", {}], + ["test.validated", -1], // throws, so no fourth item + ]); }); it("should commit stages values with dependent validation", function() { assert.throws(() => testInstance.set("test.base", 10)); @@ -979,6 +1030,14 @@ describe("lib/config/classes", function() { testInstance.commitStaging(); assert.equal(testInstance.get("test.base"), 10); assert.equal(testInstance.get("test.dependent"), 15); + assert.deepEqual(TestConfig.validatorCalls, [ + ["test.dependent", 1], // set test.base + // commitStaging, all values are checked once + ["test.dependent", 15], + ["test.objDependent", {}], + ["test.validated", null], + ["test.objValidated", {}], + ]); }); it("should notify fieldChanged listeners", function() { const changes = []; From 93271730eb3fe33dcb0867de2966d1eec24d6399 Mon Sep 17 00:00:00 2001 From: Cooldude2606 <25043174+Cooldude2606@users.noreply.github.com> Date: Fri, 20 Mar 2026 18:56:55 +0000 Subject: [PATCH 20/20] Make configs readonly inside validators --- packages/lib/src/config/classes.ts | 109 ++++++++++++++++++++++------- test/lib/config/classes.js | 46 ++++++++++++ 2 files changed, 129 insertions(+), 26 deletions(-) diff --git a/packages/lib/src/config/classes.ts b/packages/lib/src/config/classes.ts index e3bb4009..b243cda2 100644 --- a/packages/lib/src/config/classes.ts +++ b/packages/lib/src/config/classes.ts @@ -230,6 +230,8 @@ export class Config< private fields: Fields; private _unknownFields: Record = {}; + /** Set to true during validators to prevent edits */ + private _readonly = false; // TODO replace with "using config.asReadonly()" /** Set to true when a field in the config is changed. */ private dirty = false; /** Set to true when a 'restart required' field in the config is changed. */ @@ -274,6 +276,7 @@ export class Config< private _staging?: { config: Config, changes: Set}; /** Get the latest version of staging */ + // TODO replace with "using config.startStating()" get staging(): Config { if (this._staging) { return this._staging.config; } const config = new this.constructor(this.location, this.fields); @@ -417,9 +420,14 @@ export class Config< * @param notify - False to skip setting dirty flags and emitting events */ _set(name: keyof Fields & string, value: FieldValue, notify: boolean = true) { + if (this._readonly) { + throw new Error("config is readonly and cannot have values changed"); + } + const prev = this.fields[name]; this.fields[name] = value as any; this.revertStaging(); + if (notify && !isDeepStrictEqual(value, prev)) { const def = this.constructor.fieldDefinitions[name]; if (def.restartRequired) { @@ -650,23 +658,37 @@ export class Config< const value = Config._coerceValue(name, def, newValue); - if (def.validator) { - try { - def.validator(value, this); - } catch (err: any) { - throw new InvalidValue(`Failed validation of ${name}: ${err.message}`); + const _readonly = this._readonly; + this.staging._readonly = true; + this._readonly = true; + try { + if (def.validator) { + try { + def.validator(value, this); + } catch (err: any) { + throw new InvalidValue(`Failed validation of ${name}: ${err.message}`); + } } + } finally { + this._readonly = _readonly; + this.staging._readonly = _readonly; } const dependents = this.constructor.fieldDependents.get(name); if (dependents) { + // This mimics stage + commit but optimised for a single value this.staging._set(name, value); - for (const dependent of dependents) { - try { - dependent.def.validator(this.get(dependent.name as keyof Fields & string, remote), this.staging); - } catch (err: any) { - throw new InvalidValue(`Failed validation of dependent ${dependent.name}: ${err.message}`); + this.staging._readonly = true; + try { + for (const dependent of dependents) { + try { + dependent.def.validator(this.get(dependent.name as any, remote), this.staging); + } catch (err: any) { + throw new InvalidValue(`Failed validation of dependent ${dependent.name}: ${err.message}`); + } } + } finally { + this.revertStaging(); } } @@ -715,23 +737,37 @@ export class Config< delete updated[prop]; } - if (def.validator) { - try { - def.validator(updated, this); - } catch (err: any) { - throw new InvalidValue(`Failed validation of ${name}: ${err.message}`); + const _readonly = this._readonly; + this.staging._readonly = true; + this._readonly = true; + try { + if (def.validator) { + try { + def.validator(updated, this); + } catch (err: any) { + throw new InvalidValue(`Failed validation of ${name}: ${err.message}`); + } } + } finally { + this._readonly = _readonly; + this.staging._readonly = _readonly; } const dependents = this.constructor.fieldDependents.get(name); if (dependents) { + // This mimics stage + commit but optimised for a single value this.staging._set(name, updated); - for (const dependent of dependents) { - try { - dependent.def.validator(this.get(dependent.name as keyof Fields & string, remote), this.staging); - } catch (err: any) { - throw new InvalidValue(`Failed validation of dependent ${dependent.name}: ${err.message}`); + this.staging._readonly = true; + try { + for (const dependent of dependents) { + try { + dependent.def.validator(this.get(dependent.name as any, remote), this.staging); + } catch (err: any) { + throw new InvalidValue(`Failed validation of dependent ${dependent.name}: ${err.message}`); + } } + } finally { + this.revertStaging(); } } @@ -749,6 +785,10 @@ export class Config< * @throws {InvalidValue} if value is not allowed for the field. */ stage(name: Field, newValue: Fields[Field], remote = this.location) { + if (this._readonly) { + throw new Error("config is readonly and cannot have values changed"); + } + const def = this.constructor.fieldDefinitions[name]; if (!def) { throw new InvalidField(`No field named '${name}'`); @@ -780,6 +820,10 @@ export class Config< value?: unknown, remote = this.location, ) { + if (this._readonly) { + throw new Error("config is readonly and cannot have values changed"); + } + const def = this.constructor.fieldDefinitions[name]; if (!def) { throw new InvalidField(`No field named '${name}'`); @@ -812,6 +856,10 @@ export class Config< * @throws {InvalidValue} if a value is not allowed for its field. */ commitStaging() { + if (this._readonly) { + throw new Error("config is readonly and cannot have values changed"); + } + const staging = this._staging; if (!staging) { return; // Nothing to commit @@ -819,14 +867,20 @@ export class Config< // Validate all fields using the current state of staging const config = staging.config; - for (const [fieldName, fieldDef] of Object.entries(this.constructor.fieldDefinitions)) { - if (fieldDef.validator) { - try { - fieldDef.validator(config.fields[fieldName as keyof Fields], config); - } catch (err: any) { - throw new InvalidValue(`Failed validation of ${fieldName}: ${err.message}`); + const _readonly = this._readonly; + config._readonly = true; + try { + for (const [fieldName, fieldDef] of Object.entries(this.constructor.fieldDefinitions)) { + if (fieldDef.validator) { + try { + fieldDef.validator(config.fields[fieldName as keyof Fields], config); + } catch (err: any) { + throw new InvalidValue(`Failed validation of ${fieldName}: ${err.message}`); + } } } + } finally { + config._readonly = _readonly; } // Validation success, set all values which have changed @@ -838,6 +892,9 @@ export class Config< /** Revert all changes in staging */ revertStaging() { + if (this._readonly) { + throw new Error("config is readonly and cannot have values changed"); + } this._staging = undefined; } } diff --git a/test/lib/config/classes.js b/test/lib/config/classes.js index 8fa39999..6db7014e 100644 --- a/test/lib/config/classes.js +++ b/test/lib/config/classes.js @@ -90,6 +90,29 @@ describe("lib/config/classes", function() { } }, }, + "test.badValidator": { + type: "string", + optional: true, + dependsOn: ["test.base"], + validator(value, config) { + if (config.get("test.base") < 1) { + return; + } + if (value === "set") { + config.set("alpha.foo", "BAD"); + } else if (value === "setProp") { + config.setProp("beta.bar", "baz", "BAD"); + } else if (value === "stage") { + config.stage("alpha.foo", "BAD"); + } else if (value === "stageProp") { + config.stageProp("beta.bar", "baz", "BAD"); + } else if (value === "commitStaging") { + config.commitStaging(); + } else if (value === "revertStaging") { + config.revertStaging(); + } + }, + }, }; } @@ -1076,5 +1099,28 @@ describe("lib/config/classes", function() { assert.equal(testInstance.get("alpha.foo"), null); }); }); + + describe("is readonly in validators", function() { + for (const method of [ + "set", "setProp", "stage", "stageProp", "commitStaging", "revertStaging", + ]) { + it(`${method} should throw if called inside a validator`, function() { + const testInstance = new TestConfig("local"); + assert.throws( + () => testInstance.set("test.badValidator", method), + /config is readonly and cannot have values changed/ + ); + }); + it(`${method} should throw if called inside a dependent validator`, function() { + const testInstance = new TestConfig("local"); + testInstance.set("test.base", 0); + testInstance.set("test.badValidator", method); + assert.throws( + () => testInstance.set("test.base", 1), + /config is readonly and cannot have values changed/ + ); + }); + } + }); }); });