From 3d0af3811596f7fe4d21e2178d0081c3ffaf8a86 Mon Sep 17 00:00:00 2001 From: Kristjan ESPERANTO <35647502+KristjanESPERANTO@users.noreply.github.com> Date: Wed, 6 May 2026 09:44:46 +0200 Subject: [PATCH 1/2] refactor(utils): drop ajv from validateModulePositions The previous schema only enforced basic structure checks, so ajv added unnecessary complexity for this single validation path. Replace ajv compile/validate with explicit JavaScript checks while keeping existing behavior for required module entries, position type validation, and unknown-position warnings. Also removes a fragile ajv error-formatting path and fixes the JSDoc parameter type: data is the full config object, not a string. --- js/utils.js | 85 +++++++++++++++++++---------------------------- package-lock.json | 6 ++-- package.json | 1 - 3 files changed, 39 insertions(+), 53 deletions(-) diff --git a/js/utils.js b/js/utils.js index e947bee9ba..23955af848 100644 --- a/js/utils.js +++ b/js/utils.js @@ -8,13 +8,11 @@ const discoveredPositionsJSFilename = "js/positions.js"; const { styleText } = require("node:util"); const Log = require("logger"); -const Ajv = require("ajv"); const globals = require("globals"); const { Linter } = require("eslint"); const { getConfigFilePath } = require("#server_functions"); const linter = new Linter({ configType: "flat" }); -const ajv = new Ajv(); const requireFromString = (src) => { const m = new module.constructor(); @@ -227,67 +225,54 @@ const checkConfigFile = (configObject) => { }; /** + * Validates the modules array in the config object. + * Checks that: + * - `modules` is an array + * - every entry has a `module` property of type string + * - every entry's `position` (if set) is a known region from index.html * - * @param {string} data - The content of the configuration file to validate. + * Unknown positions produce a warning; structural errors are fatal. + * @param {object} data - The full config object to validate. */ const validateModulePositions = (data) => { Log.info("Checking modules structure configuration ..."); const positionList = getModulePositions(); - // Make Ajv schema configuration of modules config - // Only scan "module" and "position" - const schema = { - type: "object", - properties: { - modules: { - type: "array", - items: { - type: "object", - properties: { - module: { - type: "string" - }, - position: { - type: "string" - } - }, - required: ["module"] - } - } - } - }; + // `modules` always exists (defaults.js provides a default array), but guard against it being overridden with a non-array value + if (data.modules !== undefined && !Array.isArray(data.modules)) { + Log.error("This module configuration contains errors:\nmodules must be an array"); + process.exit(1); + } - // Scan all modules - const validate = ajv.compile(schema); + // Validate each module entry + for (const [index, mod] of (data.modules ?? []).entries()) { + // Each module entry must be an object so we can safely inspect its fields + if (mod === null || typeof mod !== "object" || Array.isArray(mod)) { + Log.error(`This module configuration contains errors:\n${JSON.stringify(mod, null, 2)}\nmodule entry must be an object`); + process.exit(1); + } - const valid = validate(data); - if (valid) { - Log.info(styleText("green", "Your modules structure configuration doesn't contain errors :)")); + // `module` (the module name) is required and must be a string + if (typeof mod.module !== "string") { + Log.error(`This module configuration contains errors:\n${JSON.stringify(mod, null, 2)}\nmodule: must be a string`); + process.exit(1); + } - // Check for unknown positions (warning only, not an error) - if (data.modules) { - for (const [index, module] of data.modules.entries()) { - if (module.position && !positionList.includes(module.position)) { - Log.warn(`Module ${index} ("${module.module}") uses unknown position: "${module.position}"`); - Log.warn(`Known positions are: ${positionList.join(", ")}`); - } - } + // `position` is optional, but must be a string when provided + if (mod.position !== undefined && typeof mod.position !== "string") { + Log.error(`This module configuration contains errors:\n${JSON.stringify(mod, null, 2)}\nposition: must be a string`); + process.exit(1); } - } else { - const module = validate.errors[0].instancePath.split("/")[2]; - const position = validate.errors[0].instancePath.split("/")[3]; - let errorMessage = "This module configuration contains errors:"; - errorMessage += `\n${JSON.stringify(data.modules[module], null, 2)}`; - if (position) { - errorMessage += `\n${position}: ${validate.errors[0].message}`; - errorMessage += `\n${JSON.stringify(validate.errors[0].params.allowedValues, null, 2).slice(1, -1)}`; - } else { - errorMessage += validate.errors[0].message; + + // `position` is optional, but when set it must match a known region + if (mod.position && !positionList.includes(mod.position)) { + Log.warn(`Module ${index} ("${mod.module}") uses unknown position: "${mod.position}"`); + Log.warn(`Known positions are: ${positionList.join(", ")}`); } - Log.error(errorMessage); - process.exit(1); } + + Log.info(styleText("green", "Your modules structure configuration doesn't contain errors :)")); }; module.exports = { loadConfig, getModulePositions, moduleHasValidPosition, getAvailableModulePositions, checkConfigFile }; diff --git a/package-lock.json b/package-lock.json index 6cc6fe52be..aa0437eb67 100644 --- a/package-lock.json +++ b/package-lock.json @@ -7,13 +7,11 @@ "": { "name": "magicmirror", "version": "2.37.0-develop", - "hasInstallScript": true, "license": "MIT", "dependencies": { "@fontsource/roboto": "^5.2.10", "@fontsource/roboto-condensed": "^5.2.8", "@fortawesome/fontawesome-free": "^7.2.0", - "ajv": "^8.20.0", "animate.css": "^4.1.1", "croner": "^10.0.1", "eslint": "^10.3.0", @@ -2970,6 +2968,7 @@ "version": "8.20.0", "resolved": "https://registry.npmjs.org/ajv/-/ajv-8.20.0.tgz", "integrity": "sha512-Thbli+OlOj+iMPYFBVBfJ3OmCAnaSyNn4M1vz9T6Gka5Jt9ba/HIR56joy65tY6kx/FCF5VXNB819Y7/GUrBGA==", + "dev": true, "license": "MIT", "dependencies": { "fast-deep-equal": "^3.1.3", @@ -5119,6 +5118,7 @@ "version": "3.1.2", "resolved": "https://registry.npmjs.org/fast-uri/-/fast-uri-3.1.2.tgz", "integrity": "sha512-rVjf7ArG3LTk+FS6Yw81V1DLuZl1bRbNrev6Tmd/9RaroeeRRJhAt7jg/6YFxbvAQXUCavSoZhPPj6oOx+5KjQ==", + "dev": true, "funding": [ { "type": "github", @@ -6332,6 +6332,7 @@ "version": "1.0.0", "resolved": "https://registry.npmjs.org/json-schema-traverse/-/json-schema-traverse-1.0.0.tgz", "integrity": "sha512-NM8/P9n3XjXhIZn1lLhkFaACTOURQXjWhV4BA/RnOv8xvgqtqpAX9IO4mRQxSx1Rlo4tqzeqb0sOlruaOy3dug==", + "dev": true, "license": "MIT" }, "node_modules/json-stable-stringify-without-jsonify": { @@ -8669,6 +8670,7 @@ "version": "2.0.2", "resolved": "https://registry.npmjs.org/require-from-string/-/require-from-string-2.0.2.tgz", "integrity": "sha512-Xf0nWe6RseziFMu+Ap9biiUbmplq6S9/p+7w7YXP/JBHhrUDDUhwa+vANyubuqfZWTveU//DYVGsDG7RKL/vEw==", + "dev": true, "license": "MIT", "engines": { "node": ">=0.10.0" diff --git a/package.json b/package.json index f479c4ef93..d7c0548195 100644 --- a/package.json +++ b/package.json @@ -89,7 +89,6 @@ "@fontsource/roboto": "^5.2.10", "@fontsource/roboto-condensed": "^5.2.8", "@fortawesome/fontawesome-free": "^7.2.0", - "ajv": "^8.20.0", "animate.css": "^4.1.1", "croner": "^10.0.1", "eslint": "^10.3.0", From 7d52f35b53aef85098df229029bae933d3f58de2 Mon Sep 17 00:00:00 2001 From: Kristjan ESPERANTO <35647502+KristjanESPERANTO@users.noreply.github.com> Date: Wed, 6 May 2026 10:03:14 +0200 Subject: [PATCH 2/2] test(utils): add config module validation tests --- tests/unit/classes/utils_spec.js | 83 ++++++++++++++++++++++++++++++++ 1 file changed, 83 insertions(+) create mode 100644 tests/unit/classes/utils_spec.js diff --git a/tests/unit/classes/utils_spec.js b/tests/unit/classes/utils_spec.js new file mode 100644 index 0000000000..ef771bd5ef --- /dev/null +++ b/tests/unit/classes/utils_spec.js @@ -0,0 +1,83 @@ +const fs = require("node:fs"); + +const Log = require("../../../js/logger"); +const { checkConfigFile } = require("../../../js/utils"); + +const createConfigObject = (modules) => ({ + configFilename: "config.js", + configContentFull: "module.exports = { modules: [] };", + fullConf: { modules } +}); + +const runCheck = (modules) => { + checkConfigFile(createConfigObject(modules)); +}; + +const expectExitForModules = (modules) => { + vi.spyOn(process, "exit").mockImplementation((code) => { + throw new Error(`process.exit:${code}`); + }); + + expect(() => { + runCheck(modules); + }).toThrow("process.exit:1"); +}; + +describe("utils", () => { + let originalReadFileSync; + + beforeEach(() => { + originalReadFileSync = fs.readFileSync; + + vi.spyOn(fs, "readFileSync").mockImplementation((fileName, ...args) => { + if (fileName === "index.html") { + return "
\n
"; + } + + return originalReadFileSync.call(fs, fileName, ...args); + }); + + vi.spyOn(fs, "writeFileSync").mockImplementation(() => {}); + vi.spyOn(Log, "info").mockImplementation(() => {}); + vi.spyOn(Log, "warn").mockImplementation(() => {}); + vi.spyOn(Log, "error").mockImplementation(() => {}); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + it("accepts valid module entries", () => { + expect(() => { + runCheck([ + { module: "clock", position: "top_bar" }, + { module: "newsfeed" } + ]); + }).not.toThrow(); + expect(Log.error).not.toHaveBeenCalled(); + }); + + it("exits when modules is not an array", () => { + expectExitForModules("not-an-array"); + expect(Log.error).toHaveBeenCalledWith("This module configuration contains errors:\nmodules must be an array"); + }); + + it("exits when module field is missing or not a string", () => { + expectExitForModules([{ module: 123, position: "top_bar" }]); + expect(Log.error).toHaveBeenCalled(); + expect(Log.error.mock.calls[0][0]).toContain("module: must be a string"); + }); + + it("warns for unknown positions without exiting", () => { + const exitSpy = vi.spyOn(process, "exit").mockImplementation((code) => { + throw new Error(`process.exit:${code}`); + }); + + expect(() => { + runCheck([{ module: "clock", position: "made_up_region" }]); + }).not.toThrow(); + expect(exitSpy).not.toHaveBeenCalled(); + expect(Log.warn).toHaveBeenCalled(); + expect(Log.warn.mock.calls[0][0]).toContain("uses unknown position"); + }); +});