From 4a2c164188612a5ae7864119a8a40dee50f94e43 Mon Sep 17 00:00:00 2001 From: Bastiaan <17990055+bbassie@users.noreply.github.com> Date: Wed, 4 Nov 2026 02:19:10 +0100 Subject: [PATCH] Merge pull request #1183 from bbassie/fix/save-patching-plugin-default Default load_plugin to off when the instance lacks required features --- docs/config-system.md | 1 + docs/configuration.md | 4 +- docs/writing-plugins.md | 1 + packages/controller/src/InstanceManager.ts | 8 ++-- packages/lib/src/config/classes.ts | 26 +++++++------ packages/lib/src/config/definitions.ts | 43 ++++++++++------------ plugins/subspace_storage/index.ts | 1 + test/lib/config/classes.js | 14 +++++++ test/lib/config/validators.test.js | 43 ++++++++++++++++++++++ 9 files changed, 99 insertions(+), 42 deletions(-) diff --git a/docs/config-system.md b/docs/config-system.md index 1102d43e..f1d9e5c7 100644 --- a/docs/config-system.md +++ b/docs/config-system.md @@ -45,6 +45,7 @@ For plugins the group name is the name of the plugin, e.g., the `level` field of **initialValue** (optional if optional is true): Value this config field should take in a newly initialized config. This can also be a synchronous function returning the value to use. + The function is called with the config after its stored fields are loaded, so it can base the value on other fields. **validator** (optional): Function called with the value being set and the config it belongs to. diff --git a/docs/configuration.md b/docs/configuration.md index 41683905..f9811098 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -789,9 +789,9 @@ Defaults to 5. Whether to load the given plugin on the instance. Note that plugins need to be loaded on the controller and the host in order for them to be loaded on instances. -Loading a plugin that requires save patching or script commands is rejected while factorio.enable_save_patching or factorio.enable_script_commands respectively is disabled on the instance. +Loading a plugin that requires save patching, script commands or Lua UDP is rejected while factorio.enable_save_patching, factorio.enable_script_commands or factorio.enable_lua_udp respectively is disabled on the instance. -Defaults to true. +Defaults to true, or false if the instance does not have the features the plugin requires. ## Ctl Configuration diff --git a/docs/writing-plugins.md b/docs/writing-plugins.md index fd571c0e..18d32b64 100644 --- a/docs/writing-plugins.md +++ b/docs/writing-plugins.md @@ -319,6 +319,7 @@ export const plugin = { ``` Enabling the plugin on an instance that has the feature turned off then fails config validation, so the mistake is caught when the config is edited instead of when the instance starts. +Instances that have the feature turned off get the plugin disabled by default when it is added. Other constraints on config fields are expressed with a `validator` on the field definition, which throws when the value is not acceptable. Use `dependsOn` when the check involves other fields so that the validator runs when those change too. diff --git a/packages/controller/src/InstanceManager.ts b/packages/controller/src/InstanceManager.ts index c9feda88..572243c0 100644 --- a/packages/controller/src/InstanceManager.ts +++ b/packages/controller/src/InstanceManager.ts @@ -269,13 +269,13 @@ export default class InstanceManager { ) as InheritableField[]; } - /** Default value of a field, or undefined if it is generated */ - private static _defaultValue(name: InheritableField): unknown { + /** Default value of a field for the given config */ + private static _defaultValue(name: InheritableField, config: lib.InstanceConfig): unknown { const def = lib.InstanceConfig.fieldDefinitions[name]; if (name === "factorio.settings") { return InstanceManager.DefaultFactorioSettings; } - return typeof def.initialValue === "function" ? undefined : def.initialValue ?? null; + return typeof def.initialValue === "function" ? def.initialValue(config) : def.initialValue ?? null; } /** Values the given inherited fields take from the template, with object overrides applied */ @@ -345,7 +345,7 @@ export default class InstanceManager { for (const name of InstanceManager._inheritableFields()) { const value = instance.config.get(name) as any; const templateValue = template.config.get(name) as any; - const defaultValue = InstanceManager._defaultValue(name) as any; + const defaultValue = InstanceManager._defaultValue(name, instance.config) as any; if (lib.InstanceConfig.fieldDefinitions[name].type === "object") { inherited[name] = Object.keys(value ?? {}).filter(prop => !( isDeepStrictEqual(value[prop], templateValue?.[prop]) diff --git a/packages/lib/src/config/classes.ts b/packages/lib/src/config/classes.ts index 84367ad6..c6871929 100644 --- a/packages/lib/src/config/classes.ts +++ b/packages/lib/src/config/classes.ts @@ -88,10 +88,11 @@ export interface FieldDefinition< optional?: Type extends null ? true : false; /** * Value this config field should take in a newly initialized config. - * This can also be a synchronous function returning the value to use. + * This can also be a synchronous function returning the value to use, + * it is called after the stored fields are loaded so that it can read them. * Promise results are not awaited. */ - initialValue?: unknown | (() => unknown); + initialValue?: unknown | ((config: Config) => unknown); /** * Array of all values that are acceptible. Useful for making * enumerated values. @@ -140,9 +141,6 @@ function defaultValue(def: FieldDefinition) { if (def.initialValue === undefined) { return null; } - if (typeof def.initialValue === "function") { - return def.initialValue(); - } return def.initialValue; } @@ -246,14 +244,12 @@ export class Config< } super(); + const defs = Object.entries(this.constructor.fieldDefinitions).filter( + ([name, def]) => this._checkAccess(name, def, location, ConfigAccess.read, false) + ); + // Function initial values are left undefined until the stored fields are loaded this.fields = Object.fromEntries( - Object.entries( - this.constructor.fieldDefinitions - ).filter( - ([name, def]) => this._checkAccess(name, def, location, ConfigAccess.read, false) - ).map( - ([name, def]) => [name, defaultValue(def)] - ) + defs.map(([name, def]) => [name, typeof def.initialValue === "function" ? undefined : defaultValue(def)]) ) as Fields; if (fields) { @@ -264,6 +260,12 @@ export class Config< this.update(migrated, false, location); } + for (const [name, def] of defs) { + if (this.fields[name as keyof Fields] === undefined) { + this.fields[name as keyof Fields] = (def.initialValue as (config: Config) => any)(this); + } + } + for (const [pluginName, version] of this.constructor.pluginVersions) { const name = `${pluginName}.version` as keyof Fields & string; if (name in this.fields && this.fields[name] !== version) { diff --git a/packages/lib/src/config/definitions.ts b/packages/lib/src/config/definitions.ts index 256441db..5fd9e64f 100644 --- a/packages/lib/src/config/definitions.ts +++ b/packages/lib/src/config/definitions.ts @@ -878,36 +878,31 @@ export function addPluginFieldDefinitions( // The validator is dynamic based on the feature requirements of the plugin if (kind === "instanceConfigFields" && pluginInfo.features) { - fieldDef.dependsOn = []; - - const savePatching = pluginInfo.features.includes("SavePatching"); - if (savePatching) { - fieldDef.dependsOn.push("factorio.enable_save_patching"); + const requirements: [string, string][] = []; + if (pluginInfo.features.includes("SavePatching")) { + requirements.push(["factorio.enable_save_patching", "save patching"]); + } + if (pluginInfo.features.includes("ScriptCommands")) { + requirements.push(["factorio.enable_script_commands", "script commands"]); + } + if (pluginInfo.features.includes("LuaUdp")) { + requirements.push(["factorio.enable_lua_udp", "Lua UDP"]); } - const scriptCommands = pluginInfo.features.includes("ScriptCommands"); - if (scriptCommands) { - fieldDef.dependsOn.push("factorio.enable_script_commands"); - } - - const luaUdp = pluginInfo.features.includes("LuaUdp"); - if (luaUdp) { - fieldDef.dependsOn.push("factorio.enable_lua_udp"); - } - - if (scriptCommands || savePatching || luaUdp) { + if (requirements.length) { + fieldDef.dependsOn = requirements.map(([field]) => field); + // Default to not loaded when the instance does not meet the requirements + fieldDef.initialValue = (config: classes.Config) => requirements.every( + ([field]) => config.get(field) + ); fieldDef.validator = (enabled, config) => { if (!enabled) { return; } - if (savePatching && !config.get("factorio.enable_save_patching")) { - throw new Error(`Plugin ${pluginInfo.name} requires save patching`); - } - if (scriptCommands && !config.get("factorio.enable_script_commands")) { - throw new Error(`Plugin ${pluginInfo.name} requires script commands`); - } - if (luaUdp && !config.get("factorio.enable_lua_udp")) { - throw new Error(`Plugin ${pluginInfo.name} requires Lua UDP`); + for (const [field, description] of requirements) { + if (!config.get(field)) { + throw new Error(`Plugin ${pluginInfo.name} requires ${description}`); + } } }; } diff --git a/plugins/subspace_storage/index.ts b/plugins/subspace_storage/index.ts index bdbecf03..295268cd 100644 --- a/plugins/subspace_storage/index.ts +++ b/plugins/subspace_storage/index.ts @@ -46,6 +46,7 @@ export const plugin: lib.PluginDeclaration = { }, features: [ + "SavePatching", "ScriptCommands", ], diff --git a/test/lib/config/classes.js b/test/lib/config/classes.js index 538e5f27..b835fc35 100644 --- a/test/lib/config/classes.js +++ b/test/lib/config/classes.js @@ -145,6 +145,20 @@ describe("lib/config/classes", function() { assert.equal(testInstance.get("test.func"), 42); assert.equal(testInstance.get("test.priv"), null); }); + it("should call initialValue functions with the loaded config", function() { + class DerivedConfig extends lib.Config { + static defaultAccess = ["local"]; + static fieldDefinitions = { + "test.base": { type: "number", initialValue: 1 }, + "test.derived": { type: "number", initialValue: config => config.get("test.base") * 2 }, + }; + } + assert.equal(new DerivedConfig("local").get("test.derived"), 2); + assert.equal(new DerivedConfig("local", { "test.base": 5 }).get("test.derived"), 10); + assert.equal(new DerivedConfig("local", { "test.base": 5, "test.derived": 3 }).get("test.derived"), 3); + const invalid = new DerivedConfig("local", { "test.base": 5, "test.derived": "x" }); + assert.equal(invalid.get("test.derived"), 10); + }); it("should not initalize inaccessible fields", function() { let testInstance = new TestConfig("remote"); assert.equal(testInstance.fields["test.enum"], "b"); diff --git a/test/lib/config/validators.test.js b/test/lib/config/validators.test.js index db5113f4..da345b9e 100644 --- a/test/lib/config/validators.test.js +++ b/test/lib/config/validators.test.js @@ -369,6 +369,49 @@ describe("lib/config/definitions/validators", function() { assert.doesNotThrow(() => config.set("factorio.enable_lua_udp", true)); assert.doesNotThrow(() => config.set("lua_udp.load_plugin", true)); }); + it("should default load_plugin to false when a required feature is disabled", function() { + class TestInstanceConfig extends InstanceConfig { + static fieldDefinitions = { ...InstanceConfig.fieldDefinitions }; + } + + lib.addPluginFieldDefinitions( + { name: "requires_features", features: ["SavePatching", "ScriptCommands"] }, + "instanceConfigFields", TestInstanceConfig + ); + + assert.equal(new TestInstanceConfig("controller").get("requires_features.load_plugin"), true); + for (const field of ["factorio.enable_save_patching", "factorio.enable_script_commands"]) { + const config = new TestInstanceConfig("controller", { [field]: false }); + assert.equal(config.get("requires_features.load_plugin"), false); + // Other fields can be edited since the config is valid + assert.doesNotThrow(() => config.set("instance.name", "test")); + } + + // An explicit value is kept + const config = new TestInstanceConfig("controller", { + "factorio.enable_save_patching": false, + "requires_features.load_plugin": true, + }); + assert.equal(config.get("requires_features.load_plugin"), true); + }); + it("should reject disabling a feature required by a loaded plugin", function() { + class TestInstanceConfig extends InstanceConfig { + static fieldDefinitions = { ...InstanceConfig.fieldDefinitions }; + static _fieldDependents; // Don't reuse the cache from InstanceConfig + } + + lib.addPluginFieldDefinitions( + { name: "save_patching", features: ["SavePatching"] }, + "instanceConfigFields", TestInstanceConfig + ); + + const config = new TestInstanceConfig("controller"); + assert.throws( + () => config.set("factorio.enable_save_patching", false), + /requires save patching/ + ); + assert.equal(config.get("factorio.enable_save_patching"), true); + }); it("should allow when no feature flags are present", function() { class TestInstanceConfig extends InstanceConfig { static fieldDefinitions = { ...InstanceConfig.fieldDefinitions };