Merge pull request #616 from Hornwitser/fix-mod-settings-types

Handle bad mod setting types

CHANGELOG.md

@@ -37,6 +37,8 @@ Many thanks to the following for contributing to this release:
 - Fixed plugin events being invoked while a host connection was in an invalid state.
 - Suppresed bogus warning about event listener leak in the browser's console [#600](https://github.com/clusterio/clusterio/pull/600).
 - Fixed installer breaking on Windows after Node.js released a security fix [#614](https://github.com/clusterio/clusterio/pull/614).
+- During a data export the mod settings in mod packs will now be corrected to the type of the settings prototype if the type is incorrect.
+- Fixed missing color setting causing the Mod Pack view in the Web UI to show an error [#609](https://github.com/clusterio/clusterio/issues/609).
 
 Many thanks to the following for contributing to this release:  
 [@Cooldude2606](https://github.com/Cooldude2606)

package.json

@@ -4,7 +4,7 @@
 	"scripts": {
 		"build": "tsc --build",
 		"watch": "tsc --build --watch",
-		"test": "pnpm run build && mocha --check-leaks -R spec --recursive test \"plugins/*/test/**\"",
+		"test": "pnpm run build && mocha --check-leaks -R spec --recursive test \"plugins/*/test/**\" --exclude \"test/manual/**\"",
 		"fast-test": "FAST_TEST=y pnpm test",
 		"cover": "nyc pnpm test",
 		"fast-cover": "FAST_TEST=y nyc pnpm test",

packages/lib/src/data/ModPack.ts

@@ -334,10 +334,11 @@ export default class ModPack {
 	}
 
 	/**
-	 * Fill in missing settings with their default values
+	 * Fill in missing and incorrect settings with their default values
 	 *
-	 * Uses the provided setting prototypes to add any missing mod settings
-	 * in the mod pack with the default value from the prototype.
+	 * Uses the provided setting prototypes to correct any missing and or
+	 * incorrect mod settings in the mod pack with the default value from
+	 * the prototype.
 	 *
 	 * @param settingPrototypes -
 	 *     Setting prototypes exported from the game.
@@ -360,13 +361,23 @@ export default class ModPack {
 				logger.warn(`Ignoring ${prototype.name} with missing default_value`);
 				continue;
 			}
-			if (this.settings[settingType].get(prototype.name)) {
-				continue;
+			let value = this.settings[settingType].get(prototype.name)?.value ?? prototype.default_value;
+			const type = prototype.type;
+			if (
+				type === "bool-setting" && typeof value !== "boolean"
+				|| type === "color-setting" && typeof value !== "object"
+				|| (type === "int-setting" || type === "double-setting") && typeof value !== "number"
+			) {
+				value = prototype.default_value;
 			}
-
-			let value = prototype.default_value;
-			if (prototype.type === "color-setting") {
-				value = libLuaTools.normalizeColor(value);
+			if (type === "string-setting" && typeof value !== "string") {
+				value = typeof value === "object" ? prototype.default_value : String(value ?? "");
+			}
+			if (type === "color-setting") {
+				if (typeof value !== "object") {
+					value = prototype.default_value;
+				}
+				value = libLuaTools.normalizeColor(value as object);
 			}
 			this.settings[settingType].set(prototype.name, { value });
 		}

packages/web_ui/src/components/ModPackViewPage.tsx

@@ -424,7 +424,15 @@ function InputColor(props: {
 	value: lib.ModSettingColor,
 	onChange: (value: lib.ModSettingColor) => void,
 }) {
-	const value = props.value;
+	// In certain edge cases it's possible a value for this mod setting does not exist
+	// or is of the wrong type. Fallback to black if this is the case.
+	function isColor(input: unknown) {
+		if (typeof input !== "object" || input === null) {
+			return false;
+		}
+		return "r" in input && "g" in input && "b" in input && "a" in input;
+	}
+	const value = isColor(props.value) ? props.value : { r: 0, g: 0, b: 0, a: 1 };
 	return <ColorPicker
 		defaultFormat="rgb"
 		defaultValue={`rgba(${value.r * 255}, ${value.g * 255}, ${value.b * 255}, ${value.a}`}
@@ -774,7 +782,7 @@ export default function ModPackViewPage() {
 						continue;
 					}
 				}
-				if (modifiedModPack!.settings[scope].get(settingName)!.value !== value) {
+				if (modifiedModPack!.settings[scope].get(settingName)?.value !== value) {
 					pushChange({ type: "settings.set", scope, name: settingName, value: { value }});
 				}
 			}

test/lib/data/ModPack.js

@@ -179,6 +179,84 @@ describe("lib/data/ModPack", function() {
 					"runtime-per-user": {},
 				});
 			});
+			it("should coerce values of the incorrect type into the correct type", function() {
+				const boolDefault = true;
+				const intDefault = 123;
+				const doubleDefault = 1234.5;
+				const stringDefault = "default";
+				const colorDefault = { r: 1, g: 0, b: 1, a: 1};
+				const settingDefaultValues = {
+					"bool-setting": boolDefault,
+					"int-setting": intDefault,
+					"double-setting": doubleDefault,
+					"string-setting": stringDefault,
+					"color-setting": colorDefault,
+				};
+				const possibleValues = ["bool", "int", "double", "string", "color"];
+				const settingPrototypes = {
+					"bool-setting": {},
+					"int-setting": {},
+					"double-setting": {},
+					"string-setting": {},
+					"color-setting": {},
+				};
+				for (const [type, default_value] of Object.entries(settingDefaultValues)) {
+					for (const value of possibleValues) {
+						const settingName = `${type}-with-${value}-value`;
+						settingPrototypes[type][settingName] = {
+							type,
+							name: settingName,
+							setting_type: "startup",
+							default_value,
+						};
+					}
+				};
+
+				const pack = new ModPack();
+				const settingValues = {
+					"bool": false,
+					"int": 1,
+					"double": 2.5,
+					"string": "str",
+					"color": { r: 0, g: 1, b: 0, a: 1 },
+				};
+				for (const type of Object.keys(settingDefaultValues)) {
+					for (const [valueType, value] of Object.entries(settingValues)) {
+						pack.settings.startup.set(`${type}-with-${valueType}-value`, { value });
+					}
+				}
+				pack.fillDefaultSettings(settingPrototypes);
+				assert.deepEqual(
+					pack.settings["startup"],
+					new Map([
+						["bool-setting-with-bool-value", false],
+						["bool-setting-with-int-value", boolDefault],
+						["bool-setting-with-double-value", boolDefault],
+						["bool-setting-with-string-value", boolDefault],
+						["bool-setting-with-color-value", boolDefault],
+						["int-setting-with-bool-value", intDefault],
+						["int-setting-with-int-value", 1],
+						["int-setting-with-double-value", 2.5],
+						["int-setting-with-string-value", intDefault],
+						["int-setting-with-color-value", intDefault],
+						["double-setting-with-bool-value", doubleDefault],
+						["double-setting-with-int-value", 1],
+						["double-setting-with-double-value", 2.5],
+						["double-setting-with-string-value", doubleDefault],
+						["double-setting-with-color-value", doubleDefault],
+						["string-setting-with-bool-value", "false"],
+						["string-setting-with-int-value", "1"],
+						["string-setting-with-double-value", "2.5"],
+						["string-setting-with-string-value", "str"],
+						["string-setting-with-color-value", stringDefault],
+						["color-setting-with-bool-value", colorDefault],
+						["color-setting-with-int-value", colorDefault],
+						["color-setting-with-double-value", colorDefault],
+						["color-setting-with-string-value", colorDefault],
+						["color-setting-with-color-value", { r: 0, g: 1, b: 0, a: 1}],
+					].map(([k, v]) => [k, { value: v }])),
+				);
+			});
 		});
 
 		describe(".fromModPackString()", function() {

test/manual/README.md

@@ -0,0 +1,35 @@
+# Manual Tests
+
+This folder contains files used for performing manual tests that are non-trivial to automate.
+
+## Setup test mod
+
+Run `node packages/lib/dist/node/build_mod.js --source-dir test/manual/test_mod/ --output-dir mods/` to create the test mod.
+
+## Test handling of bad setting values
+
+1.  Create a ModPack with missmatched values with `node test/manual/test_settings_mod_pack.js` and import it.
+2.  Run a data export on an instance with the mod pack assigned, the values will get coerced to the correct types.
+3.  Use ctl to set the settings back to their wrong values:
+
+    ```bash
+    SETTING_TYPES=(bool-setting int-setting double-setting string-setting color-setting)
+    VALUE_TYPES=(bool int double string color missing)
+    VALUES=(
+        --bool-setting false
+        --int-setting 1
+        --double-setting 0.5
+        --string-setting str
+        --color-setting '{"r":0,"g":1,"b":0,"a":1}'
+        --remove-setting
+    )
+    node packages/ctl mod-pack edit test-settings $(
+        for TYPE in "${SETTING_TYPES[@]}" ; do
+            for (( I = 0; I < ${#VALUE_TYPES[@]}; I++ )); do
+                printf "%s startup %s-with-%s-value %s " ${VALUES[((I * 2))]} $TYPE ${VALUE_TYPES[I]} ${VALUES[((I * 2 + 1))]}
+            done
+        done
+    )
+    ```
+
+4.  Verify that the Web UI gracefully handles all combinations of wrong settings types.

test/manual/test_mod/info.json

@@ -0,0 +1,7 @@
+{
+	"name": "test_mod",
+	"version": "0.0.0",
+	"title": "test_mod",
+	"author": "",
+	"factorio_version": "1.1"
+}

test/manual/test_mod/settings.lua

@@ -0,0 +1,22 @@
+local settings = {
+	["bool-setting"] = true,
+	["int-setting"] = 123,
+	["double-setting"] = 1234.5,
+	["string-setting"] = "string",
+	["color-setting"] = { r = 1, g = 0, b = 1, a = 1 },
+}
+
+local possible_values = { "missing", "bool", "int", "double", "string", "color" }
+
+for name, default_value in pairs(settings) do
+	for _, value in ipairs(possible_values) do
+		data:extend({
+			{
+				type = name,
+				name = name .. "-with-" .. value .. "-value",
+				setting_type = "startup",
+				default_value = default_value,
+			}
+		})
+	end
+end

test/manual/test_settings_mod_pack.js

@@ -0,0 +1,28 @@
+"use strict";
+const { ModPack } = require("@clusterio/lib");
+
+const pack = new ModPack();
+pack.name = "test-settings";
+pack.mods.set("test_mod", { name: "test_mod", enabled: true, version: "0.0.0" });
+const settingTypes = [
+	"bool-setting",
+	"int-setting",
+	"double-setting",
+	"string-setting",
+	"color-setting",
+];
+const settingValues = {
+	"bool": false,
+	"int": 1,
+	"double": 0.5,
+	"string": "str",
+	"color": { r: 0, g: 1, b: 0, a: 1 },
+	// "missing":
+};
+for (const type of settingTypes) {
+	for (const [valueType, value] of Object.entries(settingValues)) {
+		pack.settings.startup.set(`${type}-with-${valueType}-value`, { value });
+	}
+}
+/* eslint-disable no-console */
+console.log(pack.toModPackString());