Merge pull request #885 from Cooldude2606/feature/load-plugin-validation

Allow plugins to specify requiring save patching / script commands

packages/lib/src/config/definitions.ts

@@ -716,36 +716,73 @@ function validateFieldNames(
 }
 
 /**
- * Add config fields defined by the provided plugin infos
+ * Add the config fields defined by the by a specific plugin infos
+ */
+export function addPluginFieldDefinitions(
+	pluginInfo: PluginNodeEnvInfo | PluginWebpackEnvInfo,
+	kind: "controllerConfigFields" | "hostConfigFields" | "instanceConfigFields",
+	Config: typeof classes.Config<any>
+) {
+	// Define plugin.load_plugin
+	const fieldDef: classes.FieldDefinition = {
+		title: "Load Plugin",
+		type: "boolean",
+		initialValue: true,
+		restartRequired: true,
+	};
+
+	// 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 scriptCommands = pluginInfo.features.includes("ScriptCommands");
+		if (scriptCommands) {
+			fieldDef.dependsOn.push("factorio.enable_script_commands");
+		}
+
+		if (scriptCommands || savePatching) {
+			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`);
+				}
+			};
+		}
+	}
+
+	Config.fieldDefinitions[`${pluginInfo.name}.load_plugin`] = fieldDef;
+
+	// Merge in other fields from the plugin
+	const fields = pluginInfo[kind];
+	if (fields) {
+		validateFieldNames(pluginInfo.name, fields);
+		Object.assign(Config.fieldDefinitions, fields);
+	}
+}
+
+/**
+ * Add all config fields defined by the provided plugin infos
  *
  * @param {Array<Object>} pluginInfos - Array of plugin info objects.
  */
 export function addPluginConfigFields(pluginInfos: PluginNodeEnvInfo[] | PluginWebpackEnvInfo[]) {
-	function pluginConfig(
-		pluginInfo: PluginNodeEnvInfo | PluginWebpackEnvInfo,
-		kind: "controllerConfigFields" | "hostConfigFields" | "instanceConfigFields",
-		Config: typeof ControllerConfig | typeof HostConfig | typeof InstanceConfig,
-	) {
-		(Config.fieldDefinitions as any)[`${pluginInfo.name}.load_plugin`] = {
-			title: "Load Plugin",
-			restartRequired: true,
-			type: "boolean",
-			initialValue: true,
-		};
-
-		const fields = pluginInfo[kind];
-		if (fields) {
-			validateFieldNames(pluginInfo.name, fields);
-			Object.assign(Config.fieldDefinitions, fields);
-		}
-	}
 	for (let pluginInfo of pluginInfos) {
-		pluginConfig(pluginInfo, "controllerConfigFields", ControllerConfig);
+		addPluginFieldDefinitions(pluginInfo, "controllerConfigFields", ControllerConfig);
 		if (pluginInfo.hostEntrypoint || pluginInfo.instanceEntrypoint) {
-			pluginConfig(pluginInfo, "hostConfigFields", HostConfig);
+			addPluginFieldDefinitions(pluginInfo, "hostConfigFields", HostConfig);
 		}
 		if (pluginInfo.instanceEntrypoint) {
-			pluginConfig(pluginInfo, "instanceConfigFields", InstanceConfig);
+			addPluginFieldDefinitions(pluginInfo, "instanceConfigFields", InstanceConfig);
 		}
 	}
 }

packages/lib/src/plugin.ts

@@ -7,6 +7,14 @@ import type { Logger } from "./logging";
 import type { FieldDefinition } from "./config";
 import type { PlayerStats } from "./data";
 
+
+export const PluginFeatureFlags = [
+	/** The plugin requires module code to be patched into the save */
+	"SavePatching",
+	/** The plugin requires access to script commands over rcon */
+	"ScriptCommands",
+] as const;
+
 /** Used to define the plugin export in plugins */
 export type PluginDeclaration = {
 	name: string;
@@ -26,6 +34,8 @@ export type PluginDeclaration = {
 	webEntrypoint?: string;
 	controlConfigFields?: Record<string, FieldDefinition>;
 
+	features?: (typeof PluginFeatureFlags)[number][];
+
 	messages?: any[];
 	routes?: string[];
 }

plugins/global_chat/index.ts

@@ -9,6 +9,10 @@ export const plugin: lib.PluginDeclaration = {
 	instanceEntrypoint: "dist/node/instance",
 	ctlEntrypoint: "dist/node/control",
 
+	features: [
+		"ScriptCommands",
+	],
+
 	messages: [
 		ChatEvent,
 	],

plugins/global_chat/instance.ts

@@ -16,10 +16,6 @@ export class InstancePlugin extends BaseInstancePlugin {
 	messageQueue: string[] = [];
 
 	async init() {
-		if (!this.instance.config.get("factorio.enable_script_commands")) {
-			throw new Error("global_chat plugin requires script commands.");
-		}
-
 		this.instance.handle(ChatEvent, this.handleChatEvent.bind(this));
 	}
 

plugins/inventory_sync/index.ts

@@ -45,6 +45,11 @@ export const plugin: lib.PluginDeclaration = {
 		},
 	},
 
+	features: [
+		"SavePatching",
+		"ScriptCommands",
+	],
+
 	messages: [
 		messages.AcquireRequest,
 		messages.ReleaseRequest,

plugins/inventory_sync/instance.ts

@@ -31,12 +31,6 @@ export class InstancePlugin extends BaseInstancePlugin {
 	disconnecting!: boolean;
 
 	async init() {
-		if (!this.instance.config.get("factorio.enable_save_patching")) {
-			throw new Error("inventory_sync plugin requires save patching.");
-		}
-		if (!this.instance.config.get("factorio.enable_script_commands")) {
-			throw new Error("inventory_sync plugin requires script commands.");
-		}
 		this.playersToRelease = new Set();
 		this.disconnecting = false;
 

plugins/player_auth/index.ts

@@ -33,6 +33,10 @@ export const plugin: lib.PluginDeclaration = {
 		},
 	},
 
+	features: [
+		"SavePatching",
+	],
+
 	messages: [
 		messages.FetchPlayerCodeRequest,
 		messages.SetVerifyCodeRequest,

plugins/player_auth/instance.ts

@@ -13,10 +13,6 @@ type IpcPlayerAuth = {
 
 export class InstancePlugin extends BaseInstancePlugin {
 	async init() {
-		if (!this.instance.config.get("factorio.enable_save_patching")) {
-			throw new Error("player_auth plugin requires save patching.");
-		}
-
 		this.instance.server.on(
 			"ipc-player_auth",
 			(ipcPlayerAuth: IpcPlayerAuth) => this.handleEvent(ipcPlayerAuth).catch(

plugins/research_sync/index.ts

@@ -9,6 +9,11 @@ export const plugin: lib.PluginDeclaration = {
 	instanceEntrypoint: "dist/node/instance",
 	controllerEntrypoint: "dist/node/controller",
 
+	features: [
+		"SavePatching",
+		"ScriptCommands",
+	],
+
 	messages: [
 		messages.ContributionEvent,
 		messages.ProgressEvent,

plugins/research_sync/instance.ts

@@ -27,13 +27,6 @@ export class InstancePlugin extends BaseInstancePlugin {
 	}
 
 	async init() {
-		if (!this.instance.config.get("factorio.enable_save_patching")) {
-			throw new Error("research_sync plugin requires save patching.");
-		}
-		if (!this.instance.config.get("factorio.enable_script_commands")) {
-			throw new Error("research_sync plugin requires script commands.");
-		}
-
 		this.instance.server.on("ipc-research_sync:contribution", (tech: IpcContribution) => {
 			this.researchContribution(tech).catch(err => this.unexpectedError(err));
 		});

plugins/statistics_exporter/index.ts

@@ -22,4 +22,9 @@ export const plugin: lib.PluginDeclaration = {
 			initialValue: 1,
 		},
 	},
+
+	features: [
+		"SavePatching",
+		"ScriptCommands",
+	],
 };

plugins/statistics_exporter/instance.ts

@@ -104,15 +104,6 @@ function setForceFlowStatistic(
 
 
 export class InstancePlugin extends BaseInstancePlugin {
-	async init() {
-		if (!this.instance.config.get("factorio.enable_save_patching")) {
-			throw new Error("statistics_exporter plugin requires save patching.");
-		}
-		if (!this.instance.config.get("factorio.enable_script_commands")) {
-			throw new Error("statistics_exporter plugin requires script commands.");
-		}
-	}
-
 	async gatherMetrics() {
 		let string = await this.sendRcon("/sc statistics_exporter.export()");
 		let stats: IpcStats;

plugins/subspace_storage/index.ts

@@ -49,6 +49,10 @@ export const plugin: lib.PluginDeclaration = {
 		},
 	},
 
+	features: [
+		"ScriptCommands",
+	],
+
 	messages: [
 		messages.PlaceEvent,
 		messages.RemoveRequest,

plugins/subspace_storage/instance.ts

@@ -22,10 +22,6 @@ export class InstancePlugin extends BaseInstancePlugin {
 	}
 
 	async init() {
-		if (!this.instance.config.get("factorio.enable_script_commands")) {
-			throw new Error("subspace_storage plugin requires script commands.");
-		}
-
 		this.pendingTasks = new Set();
 		this.instance.server.on("ipc-subspace_storage:output", (output: IpcItems) => {
 			this.logger.info("Received output items:");

test/integration/clusterio.js

@@ -763,9 +763,25 @@ describe("Integration of Clusterio", function() {
 
 		describe("instance send-rcon", function() {
 			requiresFactorio(this);
+			const plugins = [
+				"global_chat", "inventory_sync", "player_auth",
+				"research_sync", "statistics_exporter", "subspace_storage",
+			];
+			this.beforeAll(async function() {
+				this.timeout(10000);
+				// Disable plugins as they interfere with enable_script_commands test
+				for (const plugin of plugins) {
+					await execCtl(`instance config set test ${plugin}.load_plugin false`);
+				}
+			});
 			this.afterAll(async function() {
+				this.timeout(10000);
 				// Prevents cascading failure where enable_script_commands is expected to be true
 				await execCtl("instance config set test factorio.enable_script_commands true");
+				// Enable plugins after they were previously disabled
+				for (const plugin of plugins) {
+					await execCtl(`instance config set test ${plugin}.load_plugin true`);
+				}
 			});
 			it("sends the command", async function() {
 				slowTest(this);
@@ -1097,12 +1113,12 @@ describe("Integration of Clusterio", function() {
 			it("starts the instance with the given settings", async function() {
 				slowTest(this);
 				requiresFactorio(this);
-				await execCtl("instance config set 44 factorio.enable_save_patching false");
 				await execCtl("instance config set 44 player_auth.load_plugin false");
 				await execCtl("instance config set 44 research_sync.load_plugin false");
 				await execCtl("instance config set 44 statistics_exporter.load_plugin false");
 				await execCtl("instance config set 44 subspace_storage.load_plugin false");
 				await execCtl("instance config set 44 inventory_sync.load_plugin false");
+				await execCtl("instance config set 44 factorio.enable_save_patching false");
 
 				await execCtl("instance start 44");
 				const instance = (await getInstances()).get(44);

test/lib/config/validators.test.js

@@ -207,35 +207,124 @@ describe("lib/config/definitions/validators", function() {
 			);
 		});
 
-		it("should validate factorio.player_online_autosave_slots against autosave slots", function() {
-			const config = new InstanceConfig("controller", {
-				"factorio.settings": {
-					autosave_slots: 5,
-				},
+		describe("factorio.player_online_autosave_slots", function() {
+			it("should validate 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 when autosave_slots is not a number", function() {
+				const config = new InstanceConfig("controller", {
+					"factorio.settings": {
+						autosave_slots: null,
+					},
+				});
 
-			// 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/
-			);
+				assert.doesNotThrow(() => config.set("factorio.player_online_autosave_slots", 0));
+				assert.doesNotThrow(() => config.set("factorio.player_online_autosave_slots", 1));
+			});
 		});
 
-		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,
-				},
-			});
+		describe("plugin.load_plugin", function() {
+			const initialConfigFields = {
+				"global_chat.load_plugin": false,
+				"inventory_sync.load_plugin": false,
+				"player_auth.load_plugin": false,
+				"research_sync.load_plugin": false,
+				"statistics_exporter.load_plugin": false,
+				"subspace_storage.load_plugin": false,
+			};
+			it("should validate against factorio.enable_save_patching", function() {
+				class TestInstanceConfig extends InstanceConfig {
+					static fieldDefinitions = { ...InstanceConfig.fieldDefinitions };
+				}
 
-			assert.doesNotThrow(() => config.set("factorio.player_online_autosave_slots", 0));
-			assert.doesNotThrow(() => config.set("factorio.player_online_autosave_slots", 1));
+				lib.addPluginFieldDefinitions(
+					{name: "save_patching", features: ["SavePatching"] },
+					"instanceConfigFields", TestInstanceConfig
+				);
+
+				const config = new TestInstanceConfig("controller", initialConfigFields);
+
+				// valid: does not require script commands
+				assert.doesNotThrow(() => config.set("factorio.enable_script_commands", false));
+
+				// valid: can have load disabled
+				assert.doesNotThrow(() => config.set("save_patching.load_plugin", false));
+
+				// valid: does not require save patching when not loaded
+				assert.doesNotThrow(() => config.set("factorio.enable_save_patching", false));
+
+				// invalid: requires save patching when loaded
+				assert.throws(
+					() => config.set("save_patching.load_plugin", true),
+					/requires save patching/
+				);
+			});
+			it("should validate against factorio.enable_script_commands", function() {
+				class TestInstanceConfig extends InstanceConfig {
+					static fieldDefinitions = { ...InstanceConfig.fieldDefinitions };
+				}
+
+				lib.addPluginFieldDefinitions(
+					{ name: "script_commands", features: ["ScriptCommands"] },
+					"instanceConfigFields", TestInstanceConfig
+				);
+
+				const config = new TestInstanceConfig("controller", initialConfigFields);
+
+				// valid: does not require save patching
+				assert.doesNotThrow(() => config.set("factorio.enable_save_patching", false));
+
+				// valid: can have load disabled
+				assert.doesNotThrow(() => config.set("script_commands.load_plugin", false));
+
+				// valid: does not require script commands when not loaded
+				assert.doesNotThrow(() => config.set("factorio.enable_script_commands", false));
+
+				// invalid: requires script commands when loaded
+				assert.throws(
+					() => config.set("script_commands.load_plugin", true),
+					/requires script commands/
+				);
+			});
+			it("should allow when no feature flags are present", function() {
+				class TestInstanceConfig extends InstanceConfig {
+					static fieldDefinitions = { ...InstanceConfig.fieldDefinitions };
+				}
+
+				lib.addPluginFieldDefinitions(
+					{ name: "no_features", features: [] },
+					"instanceConfigFields", TestInstanceConfig
+				);
+
+				const config = new TestInstanceConfig("controller", initialConfigFields);
+
+				// valid: does not require save patching
+				assert.doesNotThrow(() => config.set("factorio.enable_save_patching", false));
+
+				// valid: does not require script commands
+				assert.doesNotThrow(() => config.set("factorio.enable_script_commands", false));
+
+				// valid: can have load disabled
+				assert.doesNotThrow(() => config.set("no_features.load_plugin", false));
+
+				// no invalid case
+			});
 		});
 	});