8000 Clean up helper when structs with same name exist as a global struct and a cluster specific one by brdandu · Pull Request #1562 · project-chip/zap · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Clean up helper when structs with same name exist as a global struct and a cluster specific one #1562

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Apr 8, 2025
Merged

Clean up helper when structs with same name exist as a global struct and a cluster specific one #1562

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
03c93d2
Clean up helper when structs with same name exist as a global struct …
brdandu Apr 3, 2025
89dafe1
Minor cleanup
brdandu Apr 7, 2025
8559d0a
Cleanup
brdandu Apr 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
238 changes: 238 additions & 0 deletions docs/api.md
View file
Open in desktop

Large diffs are not rendered by default.

48 changes: 46 additions & 2 deletions src-electron/db/query-struct.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ const dbApi = require('./db-api')
const dbCache = require('./db-cache')
const dbMapping = require('./db-mapping')
const queryUtil = require('./query-util')
const dbEnum = require('../../src-shared/db-enum.js')

/**
* Get all structs from a given package ID.
Expand Down Expand Up @@ -132,12 +133,12 @@ ORDER BY
*/
async function selectStructByNameAndClusterId(db, name, clusterId, packageIds) {
let queryWithoutClusterId = queryUtil.sqlQueryForDataTypeByNameAndClusterId(
'struct',
dbEnum.zclType.struct,
null,
packageIds
)
let queryWithClusterId = queryUtil.sqlQueryForDataTypeByNameAndClusterId(
'struct',
dbEnum.zclType.struct,
clusterId,
packageIds
)
Expand All @@ -154,6 +155,48 @@ async function selectStructByNameAndClusterId(db, name, clusterId, packageIds) {
}
}

/**
* Select a struct matched by name and cluster name
* Note: Use selectStructByNameAndClusterName but this was needed for backwards compatibility.
* @param {*} db
* @param {*} name
* @param {*} clusterName
* @param {*} packageIds
* @returns struct information or undefined
*/
async function selectStructByNameAndClusterName(
db,
name,
clusterName,
packageIds
) {
let queryWithClusterName = queryUtil.sqlQueryForDataTypeByNameAndClusterName(
dbEnum.zclType.struct,
name,
clusterName,
packageIds
)
let res = await dbApi
.dbAll(db, queryWithClusterName)
.then((rows) => rows.map(dbMapping.map.struct))
if (res && res.length == 1) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this checking for res.length? If length is not 1 here, I would expect that's an error condition of some sort, no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added that to cover the empty array returned from dbAll if not found. I can add the error conditioning for safety

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, so there are three cases: length 0 (do the query without cluster), length 1 (found it), length > 1 (error)?

And "res" is never falsy? Or sometimes res is falsy and sometimes length 0 depending on ... what?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe dbGet could have returned undefined/null and dbAll returns an empty array if no entries are found on the query. Based on that the cases can differ.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is dbGet involved here? res is coming from dbAll, and either we get an exception or an array there, as far as I can tell, since that's what the last then clause does.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mentioned dbGet for just information purposes. In this case it is just dbAll. Your understanding is correct.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then why are we checking whether "res" is falsy? It can't be: it's an array.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. A lot of the times just end up applying defensive coding because it does not hurt having to check for nulls.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess my point is that defending against things that obviously can't happen just leads to confusion as here... Anyway, approving, either way.

return res[0]
} else if (res && res.length > 1) {
throw new Error(
`More than one struct ${name} exists with same name for ${clusterName} cluster.`
)
} else {
queryWithClusterName = queryUtil.sqlQueryForDataTypeByNameAndClusterName(
dbEnum.zclType.struct,
name,
null, // Retrieving global data types since cluster specific ones were not found.
packageIds
)
res = await dbApi.dbGet(db, queryWithClusterName).then(dbMapping.map.struct)
return res
}
}

/**
* Get all structs which have a cluster associated with them. If a struct is
* present in more than one cluster then it can be grouped by struct name to
Expand Down Expand Up @@ -208,5 +251,6 @@ exports.selectStructByName = dbCache.cacheQuery(selectStructByName)
exports.selectStructByNameAndClusterId = dbCache.cacheQuery(
selectStructByNameAndClusterId
)
exports.selectStructByNameAndClusterName = selectStructByNameAndClusterName
exports.selectStructsWithClusterAssociation =
selectStructsWithClusterAssociation
65 changes: 65 additions & 0 deletions src-electron/db/query-util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,5 +83,70 @@ function sqlQueryForDataTypeByNameAndClusterId(
return clusterId ? queryWithClusterId : queryWithoutClusterId
}

/**
* Formulate a sqlite query string for a data type from the given cluster name and package IDs.
* @param {*} typeDiscriminator
* @param {*} name data type name
* @param {*} clusterName
* @param {*} packageIds
* @param {*} options
* @returns SQLite query string
*/
function sqlQueryForDataTypeByNameAndClusterName(
typeDiscriminator,
name,
clusterName,
packageIds,
options = {}
) {
let typeTableName = typeDiscriminator.toUpperCase()
let numberExtensionString =
typeDiscriminator == 'number' ? 'NUMBER.IS_SIGNED, ' : ''
let checkLowerCaseString =
typeDiscriminator != 'number' && typeDiscriminator != 'struct'
? `OR DATA_TYPE.NAME = '${name}'`
: ''
let structExtensionString =
typeDiscriminator == 'struct'
? 'STRUCT.IS_FABRIC_SCOPED, DATA_TYPE.DISCRIMINATOR_REF, '
: ''
let selectQueryString = `
SELECT
${typeTableName}.${typeTableName}_ID,
${structExtensionString}
DATA_TYPE.NAME AS NAME,
${numberExtensionString}
${typeTableName}.SIZE AS SIZE,
CLUSTER.NAME AS CLUSTER_NAME
FROM ${typeTableName}
INNER JOIN
DATA_TYPE
ON
${typeTableName}.${typeTableName}_ID = DATA_TYPE.DATA_TYPE_ID
LEFT JOIN
DATA_TYPE_CLUSTER
ON
DATA_TYPE_CLUSTER.DATA_TYPE_REF = ${typeTableName}.${typeTableName}_ID
LEFT JOIN
CLUSTER
ON
DATA_TYPE_CLUSTER.CLUSTER_REF = CLUSTER.CLUSTER_ID
WHERE
(DATA_TYPE.NAME = '${name}' ${checkLowerCaseString})
AND
DATA_TYPE.PACKAGE_REF IN (${dbApi.toInClause(packageIds)}) `

let whereClause = !options.ignoreClusterWhereClause
? clusterName
? `AND CLUSTER.NAME = '${clusterName}'`
: `AND CLUSTER.NAME IS NULL`
: ``

let resultingQuery = selectQueryString + whereClause
return resultingQuery
}

exports.sqlQueryForDataTypeByNameAndClusterId =
sqlQueryForDataTypeByNameAndClusterId
exports.sqlQueryForDataTypeByNameAndClusterName =
sqlQueryForDataTypeByNameAndClusterName
2 changes: 2 additions & 0 deletions src-electron/db/query-zcl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1332,6 +1332,8 @@ exports.selectStructById = queryStruct.selectStructById
exports.selectStructByName = queryStruct.selectStructByName
exports.selectStructByNameAndClusterId =
queryStruct.selectStructByNameAndClusterId
exports.selectStructByNameAndClusterName =
queryStruct.selectStructByNameAndClusterName

exports.selectBitmapById = queryBitmap.selectBitmapById
exports.selectAllBitmaps = queryBitmap.selectAllBitmaps
Expand Down
19 changes: 17 additions & 2 deletions src-electron/generator/matter/darwin/Framework/CHIP/templates/helper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,12 +240,27 @@ async function asObjectiveCClass(type, cluster, options) {
}

if (isStruct) {
const structObj = await zclQuery.selectStructByName(
let structObj = await zclQuery.selectStructByNameAndClusterName(
this.global.db,
type,
cluster,
pkgIds
);
if (structObj.structClusterCount == 0) {

let isGlobalStruct;
if (!structObj) {
// Can end up here when our "cluster name" is a backwards compat name.
// Just fetch by struct name only in that case.
structObj = await zclQuery.selectStructByName(
this.global.db,
type,
pkgIds
);
isGlobalStruct = structObj.structClusterCount == 0;
} else {
isGlobalStruct = !structObj.clusterName;
}
if (isGlobalStruct) {
// This is a global struct.
return `${
options.hash.structTypePrefix || 'MTR'
Expand Down
16 changes: 16 additions & 0 deletions test/gen-matter-3-1.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ const testUtil = require('./test-util')
const queryEndpoint = require('../src-electron/db/query-endpoint')
const queryEndpointType = require('../src-electron/db/query-endpoint-type')
const queryConfig = require('../src-electron/db/query-config')
const queryZcl = require('../src-electron/db/query-zcl')
const queryDeviceType = require('../src-electron/db/query-device-type')
const util = require('../src-electron/util/util')
const testQuery = require('./test-query')
Expand Down Expand Up @@ -521,6 +522,21 @@ test(
'SemanticTagStruct item 3 from Identify cluster: Label'
)
expect(ept).not.toContain('SemanticTagStruct item 4 from Identify cluster')

// Testing selectStructByNameAndClusterName for struct names
let globalStruct = await queryZcl.selectStructByNameAndClusterName(
db,
'SemanticTagStruct',
'Descriptor',
zclPackageId
)
let clusterStruct = await await queryZcl.selectStructByNameAndClusterName(
db,
'SemanticTagStruct',
'Mode Select',
zclPackageId
)
expect(globalStruct.id).not.toEqual(clusterStruct.id)
},
testUtil.timeout.long()
)
Expand Down
0