-
Notifications
You must be signed in to change notification settings - Fork 82
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handling Cluster Extension loading when custom XML is reloaded #1471
Conversation
dhchandw
commented
Oct 24, 2024
- 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
* 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
b5dbe1e
to
8aeffd0
Compare
src-electron/db/query-loader.js
Outdated
@@ -655,7 +699,7 @@ async function insertClusterExtensions(db, packageId, knownPackages, data) { | |||
)}) AND CODE = ?`, | |||
data.map((cluster) => [cluster.code]) | |||
) | |||
.then((rows) => { | |||
.then(async (rows) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never been a fan of this. Just break the code into separate statements using await
src-electron/db/query-loader.js
Outdated
@@ -678,17 +722,38 @@ async function insertClusterExtensions(db, packageId, knownPackages, data) { | |||
// NOTE: This code must stay in sync with insertClusters | |||
if ('commands' in data[i]) { | |||
let cmds = data[i].commands |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let cmds = filterDuplicates(
db,
packageId,
data[i].commands,
['code', 'manufacturerCode', 'source'],
'command'
)
src-electron/db/query-loader.js
Outdated
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.` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This message does not seem right. Also use env.logWarning or env.logError
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not an error or a warning. We get to this point if we try to reload a cluster extension for a top-level package that already has it.
@@ -170,12 +170,12 @@ exports.testMatterCustomZap = | |||
|
|||
exports.totalClusterCount = 111 | |||
exports.totalDomainCount = 20 | |||
exports.totalCommandArgsCount = 1786 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am always a little cautious about tests changing...could you explain to me why you did this please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of the unique constraints in the schema were not being hit because for standard clusters, attributes, etc we had manufacturer code set to NULL and NULL does not equal to NULL in SQL. When I added a derived column to circumvent this, I found a duplicate command in zcl-builtin/silabs/demo.xml
. With my changes this duplicate command wouldn't be loaded in and hence the command count decreased by one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool, approved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assuming CI passes of course
The PR looks good. Please take a look at the unify code gen CI failure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming codegen failure is due to the fix