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

@@ -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.

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

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.

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])

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<Fields>) => 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<Fields>) => 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) {

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<any>) => 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}`);
+					}
 				}
 			};
 		}

plugins/subspace_storage/index.ts

@@ -46,6 +46,7 @@ export const plugin: lib.PluginDeclaration = {
 	},
 
 	features: [
+		"SavePatching",
 		"ScriptCommands",
 	],
 

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");

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 };