diff --git a/lib/util/onboarding.ts b/lib/util/onboarding.ts index 483360fbf8..ac16783fa0 100644 --- a/lib/util/onboarding.ts +++ b/lib/util/onboarding.ts @@ -1,11 +1,10 @@ import {existsSync, mkdirSync} from "node:fs"; import {createServer} from "node:http"; import {parse} from "node:querystring"; - import {findAllDevices} from "zigbee-herdsman/dist/adapter/adapterDiscovery"; - import data from "./data"; import * as settings from "./settings"; +import {YAMLFileException} from "./yaml"; type OnboardSettings = { mqtt_base_topic?: string; @@ -518,6 +517,27 @@ async function startFailureServer(errors: string): Promise { await new Promise((resolve) => server?.close(resolve)); } +async function onSettingsErrors(errors: string[]): Promise { + let pErrors = ""; + + console.error("\n\n!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!"); + console.error(" READ THIS CAREFULLY\n"); + console.error("Refusing to start because configuration is not valid, found the following errors:"); + + for (const error of errors) { + console.error(`- ${error}`); + + pErrors += `

- ${escapeHtml(error)}

`; + } + + console.error("\nIf you don't know how to solve this, read https://www.zigbee2mqtt.io/guide/configuration"); + console.error("\n!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!\n\n"); + + if (!process.env.Z2M_ONBOARD_NO_SERVER && !process.env.Z2M_ONBOARD_NO_FAILURE_PAGE) { + await startFailureServer(pErrors); + } +} + export async function onboard(): Promise { if (!existsSync(data.getPath())) { mkdirSync(data.getPath(), {recursive: true}); @@ -525,15 +545,38 @@ export async function onboard(): Promise { const confExists = existsSync(data.joinPath("configuration.yaml")); - if (!confExists) { - settings.writeMinimalDefaults(); - } else { + if (confExists) { + // initial caching, ensure file is valid yaml first + try { + settings.getPersistedSettings(); + } catch (error) { + await onSettingsErrors( + error instanceof YAMLFileException + ? [`Your configuration file: '${error.file}' is invalid (use https://jsonformatter.org/yaml-validator to find and fix the issue)`] + : [`${error}`], + ); + + return false; + } + // migrate first const {migrateIfNecessary} = await import("./settingsMigration.js"); migrateIfNecessary(); + + // make sure existing settings are valid before applying envs + const errors = settings.validateNonRequired(); + + if (errors.length > 0) { + await onSettingsErrors(errors); + + return false; + } + // trigger initial writing of `ZIGBEE2MQTT_CONFIG_*` ENVs settings.write(); + } else { + settings.writeMinimalDefaults(); } // use `configuration.yaml` file to detect "brand new install" @@ -553,24 +596,7 @@ export async function onboard(): Promise { const errors = settings.validate(); if (errors.length > 0) { - let pErrors = ""; - - console.error("\n\n!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!"); - console.error(" READ THIS CAREFULLY\n"); - console.error("Refusing to start because configuration is not valid, found the following errors:"); - - for (const error of errors) { - console.error(`- ${error}`); - - pErrors += `

- ${escapeHtml(error)}

`; - } - - console.error("\nIf you don't know how to solve this, read https://www.zigbee2mqtt.io/guide/configuration"); - console.error("\n!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!\n\n"); - - if (!process.env.Z2M_ONBOARD_NO_SERVER && !process.env.Z2M_ONBOARD_NO_FAILURE_PAGE) { - await startFailureServer(pErrors); - } + await onSettingsErrors(errors); return false; } diff --git a/lib/util/settings.ts b/lib/util/settings.ts index 08e6a5676c..aef409e2a1 100644 --- a/lib/util/settings.ts +++ b/lib/util/settings.ts @@ -7,7 +7,7 @@ import objectAssignDeep from "object-assign-deep"; import data from "./data"; import schemaJson from "./settings.schema.json"; import utils from "./utils"; -import yaml, {YAMLFileException} from "./yaml"; +import yaml from "./yaml"; export {schemaJson}; // When updating also update: @@ -260,15 +260,7 @@ export function write(): void { } export function validate(): string[] { - try { - getPersistedSettings(); - } catch (error) { - if (error instanceof YAMLFileException) { - return [`Your YAML file: '${error.file}' is invalid (use https://jsonformatter.org/yaml-validator to find and fix the issue)`]; - } - - return [`${error}`]; - } + getPersistedSettings(); if (!ajvSetting(_settings)) { // biome-ignore lint/style/noNonNullAssertion: When `ajvSetting()` return false it always has `errors` @@ -321,6 +313,19 @@ export function validate(): string[] { return errors; } +export function validateNonRequired(): string[] { + getPersistedSettings(); + + if (!ajvSetting(_settings)) { + // biome-ignore lint/style/noNonNullAssertion: When `ajvSetting()` return false it always has `errors` + const errors = ajvSetting.errors!.filter((e) => e.keyword !== "required"); + + return errors.map((v) => `${v.instancePath.substring(1)} ${v.message}`); + } + + return []; +} + function read(): Partial { const s = yaml.read(CONFIG_FILE_PATH) as Partial; @@ -478,12 +483,12 @@ export function apply(settings: Record, throwOnError = true): b const newSettings = objectAssignDeep.noMutate(_settings, settings); utils.removeNullPropertiesFromObject(newSettings, NULLABLE_SETTINGS); - ajvSetting(newSettings); - if (throwOnError) { - const errors = ajvSetting.errors?.filter((e) => e.keyword !== "required"); + if (!ajvSetting(newSettings) && throwOnError) { + // biome-ignore lint/style/noNonNullAssertion: When `ajvSetting()` return false it always has `errors` + const errors = ajvSetting.errors!.filter((e) => e.keyword !== "required"); - if (errors?.length) { + if (errors.length) { const error = errors[0]; throw new Error(`${error.instancePath.substring(1)} ${error.message}`); } diff --git a/lib/util/yaml.ts b/lib/util/yaml.ts index 1d515e15d8..a998322546 100644 --- a/lib/util/yaml.ts +++ b/lib/util/yaml.ts @@ -21,7 +21,7 @@ export class YAMLFileException extends YAMLException { function read(file: string): KeyValue { try { const result = yaml.load(fs.readFileSync(file, "utf8")); - assert(result instanceof Object); + assert(result instanceof Object, `The content of ${file} is expected to be an object`); return result as KeyValue; } catch (error) { if (error instanceof YAMLException) { diff --git a/test/onboarding.test.ts b/test/onboarding.test.ts index 7c31b734c0..047339d19b 100644 --- a/test/onboarding.test.ts +++ b/test/onboarding.test.ts @@ -1,8 +1,8 @@ // biome-ignore assist/source/organizeImports: import mocks first import * as data from "./mocks/data"; -import {rmSync} from "node:fs"; - +import {rmSync, writeFileSync} from "node:fs"; +import {join} from "node:path"; import type {IncomingMessage, OutgoingHttpHeader, OutgoingHttpHeaders, RequestListener, Server, ServerResponse} from "node:http"; import type {findAllDevices} from "zigbee-herdsman/dist/adapter/adapterDiscovery"; import {onboard} from "../lib/util/onboarding"; @@ -752,7 +752,13 @@ describe("Onboarding", () => { }); it("handles validation failure", async () => { - settings.set(["serial", "adapter"], "emberz"); + const reReadSpy = vi.spyOn(settings, "reRead"); + + // set after onboarding server is done to reach bottom code path + reReadSpy.mockImplementationOnce(() => { + settings.set(["serial", "adapter"], "emberz"); + settings.reRead(); + }); let p; const getHtml = await new Promise((resolve, reject) => { @@ -769,6 +775,88 @@ describe("Onboarding", () => { await expect(p).resolves.toStrictEqual(false); expect(getHtml).toContain("adapter must be equal to one of the allowed values"); + + reReadSpy.mockRestore(); + }); + + it("handles non-required validation failure before applying envs", async () => { + settings.set(["serial"], "/dev/ttyUSB0"); + + let p; + const getHtml = await new Promise((resolve, reject) => { + mockHttpOnListen.mockImplementationOnce(async () => { + try { + resolve(await runFailure()); + } catch (error) { + reject(error); + } + }); + + p = onboard(); + }); + + await expect(p).resolves.toStrictEqual(false); + expect(getHtml).toContain("serial must be object"); + }); + + it("handles invalid yaml file", async () => { + settings.testing.clear(); + + const configFile = join(data.mockDir, "configuration.yaml"); + + writeFileSync( + configFile, + ` + good: 9 + \t wrong + `, + ); + + let p; + const getHtml = await new Promise((resolve, reject) => { + mockHttpOnListen.mockImplementationOnce(async () => { + try { + resolve(await runFailure()); + } catch (error) { + reject(error); + } + }); + + p = onboard(); + }); + + await expect(p).resolves.toStrictEqual(false); + expect(getHtml).toContain("Your configuration file"); + expect(getHtml).toContain("is invalid"); + + data.removeConfiguration(); + }); + + it("handles error while loading yaml file", async () => { + settings.testing.clear(); + + const configFile = join(data.mockDir, "configuration.yaml"); + + writeFileSync(configFile, "badfile"); + + let p; + const getHtml = await new Promise((resolve, reject) => { + mockHttpOnListen.mockImplementationOnce(async () => { + try { + resolve(await runFailure()); + } catch (error) { + reject(error); + } + }); + + p = onboard(); + }); + + await expect(p).resolves.toStrictEqual(false); + expect(getHtml).toContain("AssertionError"); + expect(getHtml).toContain("expected to be an object"); + + data.removeConfiguration(); }); it("handles creating data path", async () => { diff --git a/test/settings.test.ts b/test/settings.test.ts index 0d878d7605..d5695ba36c 100644 --- a/test/settings.test.ts +++ b/test/settings.test.ts @@ -847,25 +847,6 @@ describe("Settings", () => { expect(settings.get().blocklist).toStrictEqual(["0x123", "0x1234"]); }); - it("Should throw error when yaml file is invalid", () => { - fs.writeFileSync( - configurationFile, - ` - good: 9 - \t wrong - `, - ); - - settings.testing.clear(); - const error = `Your YAML file: '${configurationFile}' is invalid (use https://jsonformatter.org/yaml-validator to find and fix the issue)`; - expect(settings.validate()).toEqual(expect.arrayContaining([error])); - }); - - it("Should throw error when yaml file does not exist", () => { - settings.testing.clear(); - expect(settings.validate()[0]).toContain("ENOENT: no such file or directory, open "); - }); - it("Configuration shouldnt be valid when invalid QOS value is used", () => { write(configurationFile, { ...minimalConfig,