fix: Exclude homeassistant entries from null cleanup (#22995)

* exclude homeassistant entries from null cleanup * Add a test * Don't hardcode the exclude list, add one more test * implement suggested changes as per review
Koenkk · Jun 15, 2024 · d41cf43 · d41cf43
1 parent fcdae97
commit d41cf43
Show file tree

Hide file tree

Showing 4 changed files with 136 additions and 6 deletions.
diff --git a/lib/util/settings.ts b/lib/util/settings.ts
@@ -32,6 +32,7 @@ export type LogLevel = typeof LOG_LEVELS[number];
 
 // DEPRECATED ZIGBEE2MQTT_CONFIG: https://github.com/Koenkk/zigbee2mqtt/issues/4697
 const file = process.env.ZIGBEE2MQTT_CONFIG ?? data.joinPath('configuration.yaml');
+const NULLABLE_SETTINGS = ['homeassistant'];
 const ajvSetting = new Ajv({allErrors: true}).addKeyword('requiresRestart').compile(schemaJson);
 const ajvRestartRequired = new Ajv({allErrors: true})
     .addKeyword({keyword: 'requiresRestart', validate: (s: unknown) => !s}).compile(schemaJson);
@@ -484,7 +485,7 @@ export function apply(settings: Record<string, unknown>): boolean {
     getInternalSettings(); // Ensure _settings is initialized.
     /* eslint-disable-line */ // @ts-ignore
     const newSettings = objectAssignDeep.noMutate(_settings, settings);
-    utils.removeNullPropertiesFromObject(newSettings);
+    utils.removeNullPropertiesFromObject(newSettings, NULLABLE_SETTINGS);
     ajvSetting(newSettings);
     const errors = ajvSetting.errors && ajvSetting.errors.filter((e) => e.keyword !== 'required');
     if (errors?.length) {
@@ -695,11 +696,11 @@ export function changeEntityOptions(IDorName: string, newOptions: KeyValue): boo
     let validator: ValidateFunction;
     if (getDevice(IDorName)) {
         objectAssignDeep(settings.devices[getDevice(IDorName).ID], newOptions);
-        utils.removeNullPropertiesFromObject(settings.devices[getDevice(IDorName).ID]);
+        utils.removeNullPropertiesFromObject(settings.devices[getDevice(IDorName).ID], NULLABLE_SETTINGS);
         validator = ajvRestartRequiredDeviceOptions;
     } else if (getGroup(IDorName)) {
         objectAssignDeep(settings.groups[getGroup(IDorName).ID], newOptions);
-        utils.removeNullPropertiesFromObject(settings.groups[getGroup(IDorName).ID]);
+        utils.removeNullPropertiesFromObject(settings.groups[getGroup(IDorName).ID], NULLABLE_SETTINGS );
         validator = ajvRestartRequiredGroupOptions;
     } else {
         throw new Error(`Device or group '${IDorName}' does not exist`);

diff --git a/lib/util/utils.ts b/lib/util/utils.ts
@@ -162,13 +162,20 @@ export function* loadExternalConverter(moduleName: string): Generator<ExternalDe
     }
 }
 
-function removeNullPropertiesFromObject(obj: KeyValue): void {
+/**
+ * Delete all keys from passed object that have null/undefined values.
+ *
+ * @param {KeyValue} obj Object to process (in-place)
+ * @param {string[]} [ignoreKeys] Recursively ignore these keys in the object (keep null/undefined values).
+ */
+function removeNullPropertiesFromObject(obj: KeyValue, ignoreKeys: string[] = [] ): void {
     for (const key of Object.keys(obj)) {
+        if (ignoreKeys.includes(key)) continue;
         const value = obj[key];
         if (value == null) {
             delete obj[key];
         } else if (typeof value === 'object') {
-            removeNullPropertiesFromObject(value);
+            removeNullPropertiesFromObject(value, ignoreKeys);
         }
     }
 }

diff --git a/test/settings.test.js b/test/settings.test.js
@@ -761,7 +761,7 @@ describe('Settings', () => {
         expect(settings.validate()).toEqual(expect.arrayContaining([error]));
     });
 
-    it('Validate should if settings does not conform to scheme', () => {
+    it('Should validate if settings do not conform to scheme', () => {
         write(configurationFile, {
             ...minimalConfig,
             advanced: null,
@@ -923,6 +923,56 @@ describe('Settings', () => {
         expect(before).toBe(after);
     });
 
+    it('Should keep homeassistant null property on device setting change', () => {
+        write(configurationFile, {
+          devices: {
+           '0x12345678': {
+              friendly_name: 'custom discovery',
+              homeassistant: { 
+                entityXYZ: { 
+                  entity_category: null,
+                }
+              }
+            }
+          }
+	});
+        settings.changeEntityOptions('0x12345678',{disabled: true});
+
+        const actual = read(configurationFile);
+        const expected = {
+          devices: {
+            '0x12345678': {
+              friendly_name: 'custom discovery',
+              disabled: true,
+              homeassistant: { 
+                entityXYZ: { 
+                  entity_category: null,
+                }
+              }
+            },
+          }
+        };
+        expect(actual).toStrictEqual(expected);
+    });
+
+    it('Should keep homeassistant null properties on apply', async () => {
+        write(configurationFile, {
+          device_options: {
+            homeassistant: {temperature: null},
+          },
+          devices: {
+           '0x1234567812345678': {
+              friendly_name: 'custom discovery',
+              homeassistant: {humidity: null},
+            }
+          }
+	});
+        settings.reRead();
+        settings.apply({permit_join: false});
+        expect(settings.get().device_options.homeassistant).toStrictEqual({temperature: null});
+        expect(settings.get().devices['0x1234567812345678'].homeassistant).toStrictEqual({humidity: null});
+    });
+
     it('Frontend config', () => {
         write(configurationFile, {...minimalConfig,
             frontend: true,

diff --git a/test/utils.test.js b/test/utils.test.js
@@ -36,4 +36,76 @@ describe('Utils', () => {
         expect(utils.formatDate(date, 'ISO_8601_local').endsWith('+01:00')).toBeTruthy();
         Date.prototype.getTimezoneOffset = getTimezoneOffset;
     })
+    it('Removes null properties from object', () => {
+        const obj1 = {
+            ab: 0,
+            cd: false,
+            ef: null,
+            gh: '',
+            homeassistant: {
+                xyz: 'mock',
+                abcd: null,
+            },
+            nested: {
+                homeassistant: {
+                    abcd: true,
+                    xyz: null,
+                },
+                abc: {},
+                def: null,
+            },
+        };
+
+        utils.removeNullPropertiesFromObject(obj1);
+        expect(obj1).toStrictEqual({
+            ab: 0,
+            cd: false,
+            gh: '',
+            homeassistant: {
+                xyz: 'mock',
+            },
+            nested: {
+                homeassistant: {
+                    abcd: true,
+                },
+                abc: {},
+            },
+        });
+
+        const obj2 = {
+            ab: 0,
+            cd: false,
+            ef: null,
+            gh: '',
+            homeassistant: {
+                xyz: 'mock',
+                abcd: null,
+            },
+            nested: {
+                homeassistant: {
+                    abcd: true,
+                    xyz: null,
+                },
+                abc: {},
+                def: null,
+            },
+        };
+        utils.removeNullPropertiesFromObject(obj2, ['homeassistant']);
+        expect(obj2).toStrictEqual({
+            ab: 0,
+            cd: false,
+            gh: '',
+            homeassistant: {
+                xyz: 'mock',
+                abcd: null,
+            },
+            nested: {
+                homeassistant: {
+                    abcd: true,
+                    xyz: null,
+                },
+                abc: {},
+            },
+        });
+    });
 });