From 81335e5b4b0ed0e1dacdd764de1711f5a1481f7f Mon Sep 17 00:00:00 2001 From: Oleksandr Masliuchenko Date: Thu, 8 Feb 2024 21:54:05 +0200 Subject: [PATCH] fix: Remove dependency on predefined list of endpoints (`parseEntityID()` function) (#21264) * Add function resolveEntityByID() function as an improved alternative for parseEntityID() * Port deviceGroupMembership.ts from parseEntityID() to resolveEntityByID() * Migrade groups.ts from parseEntityID() to resolveEntityByID() * Improve test coverage on groups functionality * Migrate from parseEntityID() to resolveEntityByID() for bridge functionality * Migrate from parseEntityID() to resolveEntityByID() for bind functionality * Finally get rid of parseEntityID() function * Fix linter issues * Move resolveEntityAndEndpoint() function to zigbee.ts --------- Co-authored-by: Koen Kanters --- lib/extension/bind.ts | 17 +++++--- lib/extension/bridge.ts | 14 ++++-- lib/extension/groups.ts | 29 +++++++++---- lib/extension/legacy/deviceGroupMembership.ts | 13 ++++-- lib/util/utils.ts | 8 +--- lib/zigbee.ts | 33 ++++++++++++++ test/bind.test.js | 32 +++++++++++++- test/bridge.test.js | 43 +++++++++++++++++++ test/group.test.js | 26 ++++++++++- 9 files changed, 180 insertions(+), 35 deletions(-) diff --git a/lib/extension/bind.ts b/lib/extension/bind.ts index 39041bca1e..8986831a54 100755 --- a/lib/extension/bind.ts +++ b/lib/extension/bind.ts @@ -222,25 +222,28 @@ export default class Bind extends Extension { const message = utils.parseJSON(data.message, data.message); let error = null; - const parsedSource = utils.parseEntityID(sourceKey); - const parsedTarget = utils.parseEntityID(targetKey); - const source = this.zigbee.resolveEntity(parsedSource.ID); - const target = targetKey === 'default_bind_group' ? - defaultBindGroup : this.zigbee.resolveEntity(parsedTarget.ID); + const parsedSource = this.zigbee.resolveEntityAndEndpoint(sourceKey); + const parsedTarget = this.zigbee.resolveEntityAndEndpoint(targetKey); + const source = parsedSource.entity; + const target = targetKey === 'default_bind_group' ? defaultBindGroup : parsedTarget.entity; const responseData: KeyValue = {from: sourceKey, to: targetKey}; if (!source || !(source instanceof Device)) { error = `Source device '${sourceKey}' does not exist`; + } else if (parsedSource.endpointID && !parsedSource.endpoint) { + error = `Source device '${parsedSource.ID}' does not have endpoint '${parsedSource.endpointID}'`; } else if (!target) { error = `Target device or group '${targetKey}' does not exist`; + } else if (target instanceof Device && parsedTarget.endpointID && !parsedTarget.endpoint) { + error = `Target device '${parsedTarget.ID}' does not have endpoint '${parsedTarget.endpointID}'`; } else { const successfulClusters: string[] = []; const failedClusters = []; const attemptedClusters = []; - const bindSource: zh.Endpoint = source.endpoint(parsedSource.endpoint); + const bindSource: zh.Endpoint = parsedSource.endpoint; let bindTarget: number | zh.Group | zh.Endpoint = null; - if (target instanceof Device) bindTarget = target.endpoint(parsedTarget.endpoint); + if (target instanceof Device) bindTarget = parsedTarget.endpoint; else if (target instanceof Group) bindTarget = target.zh; else bindTarget = Number(target.ID); diff --git a/lib/extension/bridge.ts b/lib/extension/bridge.ts index f195835281..362f4048d7 100644 --- a/lib/extension/bridge.ts +++ b/lib/extension/bridge.ts @@ -430,8 +430,13 @@ export default class Bridge extends Extension { throw new Error(`Invalid payload`); } - const parsedID = utils.parseEntityID(message.id); - const endpoint = (this.getEntity('device', parsedID.ID) as Device).endpoint(parsedID.endpoint); + const device = this.zigbee.resolveEntityAndEndpoint(message.id); + if (!device.entity) throw new Error(`Device '${message.id}' does not exist`); + + const endpoint = device.endpoint; + if (device.endpointID && !endpoint) { + throw new Error(`Device '${device.ID}' does not have endpoint '${device.endpointID}'`); + } const coordinatorEndpoint = this.zigbee.firstCoordinatorEndpoint(); await endpoint.bind(message.cluster, coordinatorEndpoint); @@ -457,8 +462,9 @@ export default class Bridge extends Extension { throw new Error(`Invalid payload`); } - const parsedID = utils.parseEntityID(message.id); - const device = this.getEntity('device', parsedID.ID) as Device; + const device = this.zigbee.resolveEntityAndEndpoint(message.id).entity as Device; + if (!device) throw new Error(`Device '${message.id}' does not exist`); + const source = await zhc.generateExternalDefinitionSource(device.zh); return utils.getResponse(message, {id: message.id, source}, null); diff --git a/lib/extension/groups.ts b/lib/extension/groups.ts index d107eecdbb..b090e5ba66 100644 --- a/lib/extension/groups.ts +++ b/lib/extension/groups.ts @@ -68,10 +68,13 @@ export default class Groups extends Extension { const groupID = settingGroup.ID; const zigbeeGroup = zigbeeGroups.find((g) => g.ID === groupID) || this.zigbee.createGroup(groupID); const settingsEndpoint = settingGroup.devices.map((d) => { - const parsed = utils.parseEntityID(d); - const entity = this.zigbee.resolveEntity(parsed.ID) as Device; + const parsed = this.zigbee.resolveEntityAndEndpoint(d); + const entity = parsed.entity as Device; if (!entity) logger.error(`Cannot find '${d}' of group '${settingGroup.friendly_name}'`); - return {'endpoint': entity?.endpoint(parsed.endpoint), 'name': entity?.name}; + if (parsed.endpointID && !parsed.endpoint) { + logger.error(`Cannot find endpoint '${parsed.endpointID}' of device '${parsed.ID}'`); + } + return {'endpoint': parsed.endpoint, 'name': entity?.name}; }).filter((e) => e.endpoint != null); // In settings but not in zigbee @@ -238,8 +241,8 @@ export default class Groups extends Extension { type = 'remove_all'; } - const parsedEntity = utils.parseEntityID(data.message); - resolvedEntityDevice = this.zigbee.resolveEntity(parsedEntity.ID) as Device; + const parsedEntity = this.zigbee.resolveEntityAndEndpoint(data.message); + resolvedEntityDevice = parsedEntity.entity as Device; if (!resolvedEntityDevice || !(resolvedEntityDevice instanceof Device)) { logger.error(`Device '${data.message}' does not exist`); @@ -256,7 +259,12 @@ export default class Groups extends Extension { return null; } - resolvedEntityEndpoint = resolvedEntityDevice.endpoint(parsedEntity.endpoint); + + resolvedEntityEndpoint = parsedEntity.endpoint; + if (parsedEntity.endpointID && !resolvedEntityEndpoint) { + logger.error(`Device '${parsedEntity.ID}' does not have endpoint '${parsedEntity.endpointID}'`); + return null; + } } else if (topicRegexMatch) { type = topicRegexMatch[1] as 'remove' | 'add' | 'remove_all'; const message = JSON.parse(data.message); @@ -271,13 +279,16 @@ export default class Groups extends Extension { } } - const parsed = utils.parseEntityID(message.device); - resolvedEntityDevice = this.zigbee.resolveEntity(parsed.ID) as Device; + const parsed = this.zigbee.resolveEntityAndEndpoint(message.device); + resolvedEntityDevice = parsed?.entity as Device; if (!error && (!resolvedEntityDevice || !(resolvedEntityDevice instanceof Device))) { error = `Device '${message.device}' does not exist`; } if (!error) { - resolvedEntityEndpoint = resolvedEntityDevice.endpoint(parsed.endpoint); + resolvedEntityEndpoint = parsed.endpoint; + if (parsed.endpointID && !resolvedEntityEndpoint) { + error = `Device '${parsed.ID}' does not have endpoint '${parsed.endpointID}'`; + } } } diff --git a/lib/extension/legacy/deviceGroupMembership.ts b/lib/extension/legacy/deviceGroupMembership.ts index 55d32931c9..9f51a7354d 100644 --- a/lib/extension/legacy/deviceGroupMembership.ts +++ b/lib/extension/legacy/deviceGroupMembership.ts @@ -1,7 +1,6 @@ /* istanbul ignore file */ import * as settings from '../../util/settings'; import logger from '../../util/logger'; -import utils from '../../util/utils'; import Extension from '../extension'; import bind from 'bind-decorator'; import Device from '../../model/device'; @@ -19,13 +18,19 @@ export default class DeviceGroupMembership extends Extension { return null; } - const parsed = utils.parseEntityID(match[1]); - const device = this.zigbee.resolveEntity(parsed.ID) as Device; + const parsed = this.zigbee.resolveEntityAndEndpoint(match[1]); + const device = parsed?.entity as Device; if (!device || !(device instanceof Device)) { logger.error(`Device '${match[1]}' does not exist`); return; } - const endpoint = device.endpoint(parsed.endpoint); + + const endpoint = parsed.endpoint; + if (parsed.endpointID && !endpoint) { + logger.error(`Device '${parsed.ID}' does not have endpoint '${parsed.endpointID}'`); + return; + } + const response = await endpoint.command( `genGroups`, 'getMembership', {groupcount: 0, grouplist: []}, {}, ); diff --git a/lib/util/utils.ts b/lib/util/utils.ts index 9ebb910720..a161928dfe 100644 --- a/lib/util/utils.ts +++ b/lib/util/utils.ts @@ -309,12 +309,6 @@ function isAvailabilityEnabledForEntity(entity: Device | Group, settings: Settin return !blocklist.includes(entity.name) && !blocklist.includes(entity.ieeeAddr); } -const entityIDRegex = new RegExp(`^(.+?)(?:/(${endpointNames.join('|')}|\\d+))?$`); -function parseEntityID(ID: string): {ID: string, endpoint: string} { - const match = ID.match(entityIDRegex); - return match && {ID: match[1], endpoint: match[2]}; -} - function isEndpoint(obj: unknown): obj is zh.Endpoint { return obj.constructor.name.toLowerCase() === 'endpoint'; } @@ -436,7 +430,7 @@ export default { endpointNames, capitalize, getZigbee2MQTTVersion, getDependencyVersion, formatDate, objectHasProperties, equalsPartial, getObjectProperty, getResponse, parseJSON, loadModuleFromText, loadModuleFromFile, removeNullPropertiesFromObject, toNetworkAddressHex, toSnakeCase, - parseEntityID, isEndpoint, isZHGroup, hours, minutes, seconds, validateFriendlyName, sleep, + isEndpoint, isZHGroup, hours, minutes, seconds, validateFriendlyName, sleep, sanitizeImageParameter, isAvailabilityEnabledForEntity, publishLastSeen, availabilityPayload, getAllFiles, filterProperties, flatten, arrayUnique, clone, computeSettingsToChange, getScenes, }; diff --git a/lib/zigbee.ts b/lib/zigbee.ts index 1d2d03cf54..05ad386a8c 100644 --- a/lib/zigbee.ts +++ b/lib/zigbee.ts @@ -11,6 +11,8 @@ import * as ZHEvents from 'zigbee-herdsman/dist/controller/events'; import bind from 'bind-decorator'; import {randomInt} from 'crypto'; +const entityIDRegex = new RegExp(`^(.+?)(?:/(.+))?$`); + export default class Zigbee { private herdsman: Controller; private eventBus: EventBus; @@ -283,6 +285,37 @@ export default class Zigbee { } } + resolveEntityAndEndpoint(ID: string) + : {ID: string, entity: Device | Group, endpointID: string, endpoint: zh.Endpoint} { + // This function matches the following entity formats: + // device_name (just device name) + // device_name/ep_name (device name and endpoint numeric ID or name) + // device/name (device name with slashes) + // device/name/ep_name (device name with slashes, and endpoint numeric ID or name) + + // First split the input token by the latest slash + const match = ID.match(entityIDRegex); + + // Try to match 'device_name/endpoint' pattern + let entityName = match[1]; + let deviceOrGroup = this.resolveEntity(match[1]); + let endpointNameOrID = match[2]; + + // If 'device_name/endpoint' pattern does not match, perhaps this is device name with slashes + if (!deviceOrGroup) { + entityName = ID; + deviceOrGroup = this.resolveEntity(ID); + endpointNameOrID = null; + } + + // If the function returns non-null endpoint name, but the endpoint field is null, then + // it means that endpoint was not matched because there is no such endpoint on the device + // (or the entity is a group) + const endpoint = deviceOrGroup?.isDevice() ? deviceOrGroup.endpoint(endpointNameOrID) : null; + + return {ID: entityName, entity: deviceOrGroup, endpointID: endpointNameOrID, endpoint: endpoint}; + } + firstCoordinatorEndpoint(): zh.Endpoint { return this.herdsman.getDevicesByType('Coordinator')[0].endpoints[0]; } diff --git a/test/bind.test.js b/test/bind.test.js index 06eb460843..5e39bbe5c6 100644 --- a/test/bind.test.js +++ b/test/bind.test.js @@ -387,7 +387,7 @@ describe('Bind', () => { ); }); - it('Error bind fails when source not existing', async () => { + it('Error bind fails when source device does not exist', async () => { const device = zigbeeHerdsman.devices.remote; const target = zigbeeHerdsman.devices.bulb_color.getEndpoint(1); const endpoint = device.getEndpoint(1); @@ -401,7 +401,21 @@ describe('Bind', () => { ); }); - it('Error bind fails when target not existing', async () => { + it("Error bind fails when source device's endpoint does not exist", async () => { + const device = zigbeeHerdsman.devices.remote; + const target = zigbeeHerdsman.devices.bulb_color.getEndpoint(1); + const endpoint = device.getEndpoint(1); + mockClear(device); + MQTT.events.message('zigbee2mqtt/bridge/request/device/bind', stringify({from: 'remote/not_existing_endpoint', to: 'bulb_color'})); + await flushPromises(); + expect(MQTT.publish).toHaveBeenCalledWith( + 'zigbee2mqtt/bridge/response/device/bind', + stringify({"data":{"from":"remote/not_existing_endpoint","to":"bulb_color"},"status":"error","error":"Source device 'remote' does not have endpoint 'not_existing_endpoint'"}), + {retain: false, qos: 0}, expect.any(Function) + ); + }); + + it('Error bind fails when target device or group does not exist', async () => { const device = zigbeeHerdsman.devices.remote; const target = zigbeeHerdsman.devices.bulb_color.getEndpoint(1); const endpoint = device.getEndpoint(1); @@ -415,6 +429,20 @@ describe('Bind', () => { ); }); + it("Error bind fails when target device's endpoint does not exist", async () => { + const device = zigbeeHerdsman.devices.remote; + const target = zigbeeHerdsman.devices.bulb_color.getEndpoint(1); + const endpoint = device.getEndpoint(1); + mockClear(device); + MQTT.events.message('zigbee2mqtt/bridge/request/device/bind', stringify({from: 'remote', to: 'bulb_color/not_existing_endpoint'})); + await flushPromises(); + expect(MQTT.publish).toHaveBeenCalledWith( + 'zigbee2mqtt/bridge/response/device/bind', + stringify({"data":{"from":"remote","to":"bulb_color/not_existing_endpoint"},"status":"error","error":"Target device 'bulb_color' does not have endpoint 'not_existing_endpoint'"}), + {retain: false, qos: 0}, expect.any(Function) + ); + }); + it('Legacy api: Should bind', async () => { const device = zigbeeHerdsman.devices.remote; const target = zigbeeHerdsman.devices.bulb_color.getEndpoint(1); diff --git a/test/bridge.test.js b/test/bridge.test.js index 21746dbf63..6ca218d719 100644 --- a/test/bridge.test.js +++ b/test/bridge.test.js @@ -681,6 +681,17 @@ describe('Bridge', () => { ); }); + it('Should error when generate_external_definition requested for unknown device', async () => { + MQTT.publish.mockClear(); + MQTT.events.message('zigbee2mqtt/bridge/request/device/generate_external_definition', stringify({id: 'non_existing_device'})); + await flushPromises(); + expect(MQTT.publish).toHaveBeenCalledWith( + 'zigbee2mqtt/bridge/response/device/generate_external_definition', + stringify({ data: {}, error: "Device 'non_existing_device' does not exist", status: 'error' }), + {retain: false, qos: 0}, expect.any(Function) + ); + }); + it('Should allow to generate device definition', async () => { MQTT.publish.mockClear(); MQTT.events.message('zigbee2mqtt/bridge/request/device/generate_external_definition', stringify({id: ZNCZ02LM.ieeeAddr})); @@ -1132,6 +1143,38 @@ describe('Bridge', () => { ); }); + it('Should throw error when configure reporting is called for non-existing device', async () => { + const device = zigbeeHerdsman.devices.bulb; + const endpoint = device.getEndpoint(1); + endpoint.configureReporting.mockClear(); + zigbeeHerdsman.permitJoin.mockClear(); + MQTT.publish.mockClear(); + MQTT.events.message('zigbee2mqtt/bridge/request/device/configure_reporting', stringify({id: 'non_existing_device', cluster: 'genLevelCtrl', attribute: 'currentLevel', maximum_report_interval: 10, minimum_report_interval: 1, reportable_change: 1})); + await flushPromises(); + expect(endpoint.configureReporting).toHaveBeenCalledTimes(0); + expect(MQTT.publish).toHaveBeenCalledWith( + 'zigbee2mqtt/bridge/response/device/configure_reporting', + stringify({"data":{},"status":"error","error":"Device 'non_existing_device' does not exist"}), + {retain: false, qos: 0}, expect.any(Function) + ); + }); + + it('Should throw error when configure reporting is called for non-existing endpoint', async () => { + const device = zigbeeHerdsman.devices.bulb; + const endpoint = device.getEndpoint(1); + endpoint.configureReporting.mockClear(); + zigbeeHerdsman.permitJoin.mockClear(); + MQTT.publish.mockClear(); + MQTT.events.message('zigbee2mqtt/bridge/request/device/configure_reporting', stringify({id: '0x000b57fffec6a5b2/non_existing_endpoint', cluster: 'genLevelCtrl', attribute: 'currentLevel', maximum_report_interval: 10, minimum_report_interval: 1, reportable_change: 1})); + await flushPromises(); + expect(endpoint.configureReporting).toHaveBeenCalledTimes(0); + expect(MQTT.publish).toHaveBeenCalledWith( + 'zigbee2mqtt/bridge/response/device/configure_reporting', + stringify({"data":{},"status":"error","error":"Device '0x000b57fffec6a5b2' does not have endpoint 'non_existing_endpoint'"}), + {retain: false, qos: 0}, expect.any(Function) + ); + }); + it('Should allow to create a backup', async () => { fs.mkdirSync(path.join(data.mockDir, 'ext_converters')); fs.writeFileSync(path.join(data.mockDir, 'ext_converters', 'afile.js'), 'test123') diff --git a/test/group.test.js b/test/group.test.js index 2259cf20c2..cda17407c3 100644 --- a/test/group.test.js +++ b/test/group.test.js @@ -273,7 +273,7 @@ describe('Groups', () => { expect(logger.error).toHaveBeenCalledWith("Group 'group_1_not_existing' does not exist"); }); - it('Legacy api: Log when adding to non-existing device', async () => { + it('Legacy api: Log when adding a non-existing device', async () => { await resetExtension(); logger.error.mockClear(); MQTT.events.message('zigbee2mqtt/bridge/group/group_1/add', 'bulb_color_not_existing'); @@ -281,6 +281,14 @@ describe('Groups', () => { expect(logger.error).toHaveBeenCalledWith("Device 'bulb_color_not_existing' does not exist"); }); + it('Legacy api: Log when adding a non-existing endpoint', async () => { + await resetExtension(); + logger.error.mockClear(); + MQTT.events.message('zigbee2mqtt/bridge/group/group_1/add', 'bulb_color/not_existing_endpoint'); + await flushPromises(); + expect(logger.error).toHaveBeenCalledWith("Device 'bulb_color' does not have endpoint 'not_existing_endpoint'"); + }); + it('Should publish group state change when a device in it changes state', async () => { const device = zigbeeHerdsman.devices.bulb_color; const endpoint = device.getEndpoint(1); @@ -869,7 +877,7 @@ describe('Groups', () => { ); }); - it('Error when adding to non-existing device', async () => { + it('Error when adding a non-existing device', async () => { await resetExtension(); logger.error.mockClear(); MQTT.publish.mockClear(); @@ -883,6 +891,20 @@ describe('Groups', () => { ); }); + it('Error when adding a non-existing endpoint', async () => { + await resetExtension(); + logger.error.mockClear(); + MQTT.publish.mockClear(); + MQTT.events.message('zigbee2mqtt/bridge/request/group/members/add', stringify({group: 'group_1', device: 'bulb_color/not_existing_endpoint'})); + await flushPromises(); + expect(MQTT.publish).not.toHaveBeenCalledWith('zigbee2mqtt/bridge/groups', expect.any(String), expect.any(Object), expect.any(Function)); + expect(MQTT.publish).toHaveBeenCalledWith( + 'zigbee2mqtt/bridge/response/group/members/add', + stringify({"data":{"device":"bulb_color/not_existing_endpoint","group":"group_1"},"status":"error","error":"Device 'bulb_color' does not have endpoint 'not_existing_endpoint'"}), + {retain: false, qos: 0}, expect.any(Function) + ); + }); + it('Should only include relevant properties when publishing member states', async () => { const bulbColor = zigbeeHerdsman.devices.bulb_color; const bulbColorTemp = zigbeeHerdsman.devices.bulb;