From 72192d8a4a6d8288b56e07d57fc03ba603f6268c Mon Sep 17 00:00:00 2001 From: Dhruv Date: Tue, 29 Oct 2024 11:26:53 -0400 Subject: [PATCH] Handling Cluster Extension loading when custom XML is reloaded (#1471) * Handling Cluster Extension loading when custom XML is reloaded: * Making sure cluster extension is not duplicated when custom XML is reloaded * Added a MANUFACTURER_CODE_DERIVED column since NULL!=NULL in SQL * Added SIDE to the UNIQUE constraint in attributes * Added logic to catch duplicates during loading before insertion * Added relevant tests JIRA ZAPP-1486 --- docs/api.md | 19 ++ src-electron/db/query-loader.js | 231 ++++++++++++------ src-electron/db/zap-schema.sql | 12 +- test/custom-matter-xml.test.js | 14 +- .../custom-cluster/matter-bad-custom.xml | 10 +- test/test-util.js | 6 +- test/zcl-loader.test.js | 9 + 7 files changed, 218 insertions(+), 83 deletions(-) diff --git a/docs/api.md b/docs/api.md index 03ae696c24..c21557c392 100644 --- a/docs/api.md +++ b/docs/api.md @@ -3673,6 +3673,7 @@ This module provides queries for ZCL loading * [~commandMap(clusterId, packageId, commands)](#module_DB API_ zcl loading queries..commandMap) ⇒ * [~fieldMap(eventId, packageId, fields)](#module_DB API_ zcl loading queries..fieldMap) ⇒ * [~argMap(cmdId, packageId, args)](#module_DB API_ zcl loading queries..argMap) ⇒ + * [~filterDuplicates(db, packageId, data, keys, elementName)](#module_DB API_ zcl loading queries..filterDuplicates) ⇒ Array * [~insertAttributeAccessData(db, packageId, accessData)](#module_DB API_ zcl loading queries..insertAttributeAccessData) ⇒ * [~insertCommandAccessData(db, packageId, accessData)](#module_DB API_ zcl loading queries..insertCommandAccessData) ⇒ * [~insertEventAccessData(db, packageId, accessData)](#module_DB API_ zcl loading queries..insertEventAccessData) ⇒ @@ -3785,6 +3786,24 @@ Transforms the array of command args in a certain format and returns it. | packageId | \* | | args | \* | + + +### DB API: zcl loading queries~filterDuplicates(db, packageId, data, keys, elementName) ⇒ Array +Filters out duplicates in an array of objects based on specified keys and logs a warning for each duplicate found. +This function is used to filter out duplicates in command, attribute, and event data before inserting into the database. +Treats `null` and `0` as equivalent. + +**Kind**: inner method of [DB API: zcl loading queries](#module_DB API_ zcl loading queries) +**Returns**: Array - - Array of unique objects (duplicates removed). + +| Param | Type | Description | +| --- | --- | --- | +| db | \* | | +| packageId | \* | | +| data | Array | Array of objects. | +| keys | Array | Array of keys to compare for duplicates (e.g., ['code', 'manufacturerCode']). | +| elementName | \* | | + ### DB API: zcl loading queries~insertAttributeAccessData(db, packageId, accessData) ⇒ diff --git a/src-electron/db/query-loader.js b/src-electron/db/query-loader.js index 680ce6c04b..8fe03b6421 100644 --- a/src-electron/db/query-loader.js +++ b/src-electron/db/query-loader.js @@ -138,7 +138,7 @@ INSERT INTO COMMAND_ARG ( // Attribute table needs to be unique based on: // UNIQUE("CLUSTER_REF", "PACKAGE_REF", "CODE", "MANUFACTURER_CODE") const INSERT_ATTRIBUTE_QUERY = ` -INSERT OR REPLACE INTO ATTRIBUTE ( +INSERT INTO ATTRIBUTE ( CLUSTER_REF, PACKAGE_REF, CODE, @@ -360,6 +360,50 @@ function argMap(cmdId, packageId, args) { packageId ]) } +/** + * Filters out duplicates in an array of objects based on specified keys and logs a warning for each duplicate found. + * This function is used to filter out duplicates in command, attribute, and event data before inserting into the database. + * Treats `null` and `0` as equivalent. + * + * @param {*} db + * @param {*} packageId + * @param {Array} data - Array of objects. + * @param {Array} keys - Array of keys to compare for duplicates (e.g., ['code', 'manufacturerCode']). + * @param {*} elementName + * @returns {Array} - Array of unique objects (duplicates removed). + */ +function filterDuplicates(db, packageId, data, keys, elementName) { + let seen = new Map() + let uniqueItems = [] + + data.forEach((item, index) => { + let anyKeysPresent = keys.some((key) => key in item) + + if (!anyKeysPresent) { + // If all keys are missing, treat this item as unique + uniqueItems.push(item) + } else { + let uniqueKey = keys + .map((key) => (item[key] === null || item[key] === 0 ? 0 : item[key])) + .join('|') + + if (seen.has(uniqueKey)) { + // Log a warning with the duplicate information + queryNotification.setNotification( + db, + 'ERROR', + `Duplicate ${elementName} found: ${JSON.stringify(item)}`, + packageId + ) + } else { + seen.set(uniqueKey, true) + uniqueItems.push(item) + } + } + }) + + return uniqueItems +} /** * access data is array of objects, containing id/op/role/modifier. @@ -647,81 +691,101 @@ async function insertGlobals(db, packageId, data) { * @returns Promise of cluster extension insertion. */ async function insertClusterExtensions(db, packageId, knownPackages, data) { - return dbApi - .dbMultiSelect( - db, - `SELECT CLUSTER_ID FROM CLUSTER WHERE PACKAGE_REF IN (${dbApi.toInClause( - knownPackages - )}) AND CODE = ?`, - data.map((cluster) => [cluster.code]) - ) - .then((rows) => { - let commands = { - data: [], - args: [], - access: [] - } - let events = { - data: [], - fields: [], - access: [] + let rows = await dbApi.dbMultiSelect( + db, + `SELECT CLUSTER_ID FROM CLUSTER WHERE PACKAGE_REF IN (${dbApi.toInClause( + knownPackages + )}) AND CODE = ?`, + data.map((cluster) => [cluster.code]) + ) + + let commands = { + data: [], + args: [], + access: [] + } + let events = { + data: [], + fields: [], + access: [] + } + let attributes = { + data: [], + access: [] + } + + let i, lastId + for (i = 0; i < rows.length; i++) { + let row = rows[i] + if (row != null) { + lastId = row.CLUSTER_ID + // NOTE: This code must stay in sync with insertClusters + if ('commands' in data[i]) { + let cmds = filterDuplicates( + db, + packageId, + data[i].commands, + ['code', 'manufacturerCode', 'source'], + 'command' + ) // Removes any duplicates based of db unique constraint and logs package notification (avoids SQL error) + commands.data.push(...commandMap(lastId, packageId, cmds)) + commands.args.push(...cmds.map((command) => command.args)) + commands.access.push(...cmds.map((command) => command.access)) } - let attributes = { - data: [], - access: [] + if ('attributes' in data[i]) { + let atts = filterDuplicates( + db, + packageId, + data[i].attributes, + ['code', 'manufacturerCode', 'side'], + 'attribute' + ) // Removes any duplicates based of db unique constraint and logs package notification (avoids SQL error) + attributes.data.push(...attributeMap(lastId, packageId, atts)) + attributes.access.push(...atts.map((at) => at.access)) } - let i, lastId - for (i = 0; i < rows.length; i++) { - let row = rows[i] - if (row != null) { - lastId = row.CLUSTER_ID - // NOTE: This code must stay in sync with insertClusters - if ('commands' in data[i]) { - let cmds = data[i].commands - commands.data.push(...commandMap(lastId, packageId, cmds)) - commands.args.push(...cmds.map((command) => command.args)) - commands.access.push(...cmds.map((command) => command.access)) - } - if ('attributes' in data[i]) { - let atts = data[i].attributes - attributes.data.push(...attributeMap(lastId, packageId, atts)) - attributes.access.push(...atts.map((at) => at.access)) - } - if ('events' in data[i]) { - let evs = data[i].events - events.data.push(...eventMap(lastId, packageId, evs)) - events.fields.push(...evs.map((event) => event.fields)) - events.access.push(...evs.map((event) => event.access)) - } - } else { - // DANGER: We got here because we are adding a cluster extension for a - // cluster which is not defined. For eg: - // - // sw build id - // - // If a cluster with code 0x0000 does not exist then we run into this - // issue. - let message = `Attempting to insert cluster extension for a cluster which does not - exist. Check clusterExtension meta data in xml file. - Cluster Code: ${data[i].code}` - env.logWarning(message) - queryNotification.setNotification( - db, - 'WARNING', - message, - packageId, - 2 - ) - } + if ('events' in data[i]) { + let evs = filterDuplicates( + db, + packageId, + data[i].events, + ['code', 'manufacturerCode'], + 'event' + ) // Removes any duplicates based of db unique constraint and logs package notification (avoids SQL error) + events.data.push(...eventMap(lastId, packageId, evs)) + events.fields.push(...evs.map((event) => event.fields)) + events.access.push(...evs.map((event) => event.access)) } - let pCommand = insertCommands(db, packageId, commands) - let pAttribute = insertAttributes(db, packageId, attributes) - let pEvent = insertEvents(db, packageId, events) - return Promise.all([pCommand, pAttribute, pEvent]) - }) + } else { + // DANGER: We got here because we are adding a cluster extension for a + // cluster which is not defined. For eg: + // + // sw build id + // + // If a cluster with code 0x0000 does not exist then we run into this + // issue. + let message = `Attempting to insert cluster extension for a cluster which does not + exist. Check clusterExtension meta data in xml file. + Cluster Code: ${data[i].code}` + env.logWarning(message) + queryNotification.setNotification(db, 'WARNING', message, packageId, 2) + } + } + + let pCommand = insertCommands(db, packageId, commands) + let pAttribute = insertAttributes(db, packageId, attributes) + let pEvent = insertEvents(db, packageId, events) + return Promise.all([pCommand, pAttribute, pEvent]).catch((err) => { + if (err.includes('SQLITE_CONSTRAINT') && err.includes('UNIQUE')) { + env.logDebug( + `CRC match for file with package id ${packageId}, skipping parsing.` + ) + } else { + throw err + } + }) } /** @@ -783,17 +847,38 @@ async function insertClusters(db, packageId, data) { // NOTE: This code must stay in sync with insertClusterExtensions if ('commands' in data[i]) { let cmds = data[i].commands + cmds = filterDuplicates( + db, + packageId, + cmds, + ['code', 'manufacturerCode', 'source'], + 'command' + ) // Removes any duplicates based of db unique constraint and logs package notification (avoids SQL error) commands.data.push(...commandMap(lastId, packageId, cmds)) commands.args.push(...cmds.map((command) => command.args)) commands.access.push(...cmds.map((command) => command.access)) } if ('attributes' in data[i]) { let atts = data[i].attributes + atts = filterDuplicates( + db, + packageId, + atts, + ['code', 'manufacturerCode', 'side'], + 'attribute' + ) // Removes any duplicates based of db unique constraint and logs package notification (avoids SQL error) attributes.data.push(...attributeMap(lastId, packageId, atts)) attributes.access.push(...atts.map((at) => at.access)) } if ('events' in data[i]) { let evs = data[i].events + evs = filterDuplicates( + db, + packageId, + evs, + ['code', 'manufacturerCode'], + 'event' + ) // Removes any duplicates based of db unique constraint and logs package notification (avoids SQL error) events.data.push(...eventMap(lastId, packageId, evs)) events.fields.push(...evs.map((event) => event.fields)) events.access.push(...evs.map((event) => event.access)) diff --git a/src-electron/db/zap-schema.sql b/src-electron/db/zap-schema.sql index 797ecf02bb..b1fb5e8352 100644 --- a/src-electron/db/zap-schema.sql +++ b/src-electron/db/zap-schema.sql @@ -156,10 +156,11 @@ CREATE TABLE IF NOT EXISTS "CLUSTER" ( "INTRODUCED_IN_REF" integer, "REMOVED_IN_REF" integer, "API_MATURITY" text, + "MANUFACTURER_CODE_DERIVED" AS (COALESCE(MANUFACTURER_CODE, 0)), foreign key (INTRODUCED_IN_REF) references SPEC(SPEC_ID) ON DELETE CASCADE ON UPDATE CASCADE, foreign key (REMOVED_IN_REF) references SPEC(SPEC_ID) ON DELETE CASCADE ON UPDATE CASCADE, foreign key (PACKAGE_REF) references PACKAGE(PACKAGE_ID) ON DELETE CASCADE ON UPDATE CASCADE - UNIQUE(PACKAGE_REF, CODE, MANUFACTURER_CODE) + UNIQUE(PACKAGE_REF, CODE, MANUFACTURER_CODE_DERIVED) ); /* COMMAND table contains commands contained inside a cluster. @@ -183,12 +184,13 @@ CREATE TABLE IF NOT EXISTS "COMMAND" ( "RESPONSE_REF" integer, "IS_DEFAULT_RESPONSE_ENABLED" integer, "IS_LARGE_MESSAGE" integer, + "MANUFACTURER_CODE_DERIVED" AS (COALESCE(MANUFACTURER_CODE, 0)), foreign key (INTRODUCED_IN_REF) references SPEC(SPEC_ID) ON DELETE CASCADE ON UPDATE CASCADE, foreign key (REMOVED_IN_REF) references SPEC(SPEC_ID) ON DELETE CASCADE ON UPDATE CASCADE, foreign key (CLUSTER_REF) references CLUSTER(CLUSTER_ID) ON DELETE CASCADE ON UPDATE CASCADE, foreign key (PACKAGE_REF) references PACKAGE(PACKAGE_ID) ON DELETE CASCADE ON UPDATE CASCADE, foreign key (RESPONSE_REF) references COMMAND(COMMAND_ID) ON DELETE CASCADE ON UPDATE CASCADE - UNIQUE(CLUSTER_REF, PACKAGE_REF, CODE, MANUFACTURER_CODE, SOURCE) + UNIQUE(CLUSTER_REF, PACKAGE_REF, CODE, MANUFACTURER_CODE_DERIVED, SOURCE) ); /* COMMAND_ARG table contains arguments for a command. @@ -233,11 +235,12 @@ CREATE TABLE IF NOT EXISTS "EVENT" ( "PRIORITY" text, "INTRODUCED_IN_REF" integer, "REMOVED_IN_REF" integer, + "MANUFACTURER_CODE_DERIVED" AS (COALESCE(MANUFACTURER_CODE, 0)), foreign key (CLUSTER_REF) references CLUSTER(CLUSTER_ID) ON DELETE CASCADE ON UPDATE CASCADE, foreign key (PACKAGE_REF) references PACKAGE(PACKAGE_ID) ON DELETE CASCADE ON UPDATE CASCADE, foreign key (INTRODUCED_IN_REF) references SPEC(SPEC_ID) ON DELETE CASCADE ON UPDATE CASCADE, foreign key (REMOVED_IN_REF) references SPEC(SPEC_ID) ON DELETE CASCADE ON UPDATE CASCADE - UNIQUE(CLUSTER_REF, PACKAGE_REF, CODE, MANUFACTURER_CODE) + UNIQUE(CLUSTER_REF, PACKAGE_REF, CODE, MANUFACTURER_CODE_DERIVED) ); /* EVENT_FIELD table contains events for a given cluster. @@ -294,11 +297,12 @@ CREATE TABLE IF NOT EXISTS "ATTRIBUTE" ( "API_MATURITY" text, "IS_CHANGE_OMITTED" integer, "PERSISTENCE" text, + "MANUFACTURER_CODE_DERIVED" AS (COALESCE(MANUFACTURER_CODE, 0)), foreign key (INTRODUCED_IN_REF) references SPEC(SPEC_ID) ON DELETE CASCADE ON UPDATE CASCADE, foreign key (REMOVED_IN_REF) references SPEC(SPEC_ID) ON DELETE CASCADE ON UPDATE CASCADE, foreign key (CLUSTER_REF) references CLUSTER(CLUSTER_ID) ON DELETE CASCADE ON UPDATE CASCADE, foreign key (PACKAGE_REF) references PACKAGE(PACKAGE_ID) ON DELETE CASCADE ON UPDATE CASCADE - UNIQUE("CLUSTER_REF", "PACKAGE_REF", "CODE", "MANUFACTURER_CODE") + UNIQUE("CLUSTER_REF", "PACKAGE_REF", "CODE", "MANUFACTURER_CODE_DERIVED", "SIDE") ); /* diff --git a/test/custom-matter-xml.test.js b/test/custom-matter-xml.test.js index c8e540bcce..65839d7860 100644 --- a/test/custom-matter-xml.test.js +++ b/test/custom-matter-xml.test.js @@ -448,7 +448,19 @@ test( ) expect( packageNotif.some((notif) => notif.message.includes('type contradiction')) - ).toBeTruthy() // checks if the correct warning is thrown + ).toBeTruthy() // checks if the correct type contradiction warning is thrown + + expect( + packageNotif.some((notif) => + notif.message.includes('Duplicate attribute found') + ) + ).toBeTruthy() // checks if the correct duplicate attribute error is thrown + + expect( + packageNotif.some((notif) => + notif.message.includes('Duplicate command found') + ) + ).toBeTruthy() // checks if the correct duplicate command error is thrown let sessionNotif = await querySessionNotification.getNotification(db, sid) expect( diff --git a/test/resource/custom-cluster/matter-bad-custom.xml b/test/resource/custom-cluster/matter-bad-custom.xml index 9c05d06d58..6dc21dccf8 100644 --- a/test/resource/custom-cluster/matter-bad-custom.xml +++ b/test/resource/custom-cluster/matter-bad-custom.xml @@ -1,3 +1,4 @@ + @@ -11,10 +12,15 @@ // intentional undefined type errors Sample Mfg Specific Attribute 6 + Sample Mfg Specific Attribute 6 Duplicate Sample Mfg Specific Attribute 8 - Client command that turns the device on with a transition given - by the transition time in the Ember Sample transition time attribute. + Client command that turns the device on with a transition given + by the transition time in the Ember Sample transition time attribute. + + + Client command that turns the device on with a transition given + by the transition time in the Ember Sample transition time attribute. Client command that toggles the device with a transition given diff --git a/test/test-util.js b/test/test-util.js index d6157c3deb..7aff4c5e92 100644 --- a/test/test-util.js +++ b/test/test-util.js @@ -170,12 +170,12 @@ exports.testMatterCustomZap = exports.totalClusterCount = 111 exports.totalDomainCount = 20 -exports.totalCommandArgsCount = 1786 -exports.totalCommandCount = 632 +exports.totalCommandArgsCount = 1785 +exports.totalCommandCount = 631 exports.totalEventFieldCount = 3 exports.totalEventCount = 1 exports.totalAttributeCount = 3438 -exports.totalClusterCommandCount = 609 +exports.totalClusterCommandCount = 608 exports.totalServerAttributeCount = 2962 exports.totalSpecCount = 24 exports.totalEnumCount = 211 diff --git a/test/zcl-loader.test.js b/test/zcl-loader.test.js index 218d3e0d2b..2c5740584c 100644 --- a/test/zcl-loader.test.js +++ b/test/zcl-loader.test.js @@ -24,6 +24,7 @@ const queryZcl = require('../src-electron/db/query-zcl') const queryDeviceType = require('../src-electron/db/query-device-type') const queryCommand = require('../src-electron/db/query-command') const queryPackage = require('../src-electron/db/query-package') +const queryPackageNotification = require('../src-electron/db/query-package-notification') const zclLoader = require('../src-electron/zcl/zcl-loader') const env = require('../src-electron/util/env') const types = require('../src-electron/util/types') @@ -64,6 +65,14 @@ test( let ctx = await zclLoader.loadZcl(db, env.builtinSilabsZclMetafile()) let packageId = ctx.packageId + let packageNotif = + await queryPackageNotification.getNotificationByPackageId(db, packageId) + expect( + packageNotif.some((notif) => + notif.message.includes('Duplicate command found') + ) + ).toBeTruthy() // checks if the correct duplicate command error is thrown + let p = await queryPackage.getPackageByPackageId(ctx.db, ctx.packageId) expect(p.version).toEqual(1) expect(p.description).toEqual('ZigbeePro test data')