Fix various helpers that were not handling name collisions between global and cluster-specific structs. (#1584)

Merging bypassing CI failures: matter failures are expected due to sorting order change. CI will turn back green once we use the new version of ZAP in matter.

Author: bzbarsky-apple

docs/api.md

@@ -19169,6 +19169,7 @@ This module provides the API to access various zcl utilities.
     * [~commandComparator(a, b)](#module_REST API_ various zcl utilities..commandComparator) ⇒
     * [~eventComparator(a, b)](#module_REST API_ various zcl utilities..eventComparator) ⇒
     * [~findStructByName(structs, name)](#module_REST API_ various zcl utilities..findStructByName) ⇒
+    * [~sortStructsByDependencyHelper()](#module_REST API_ various zcl utilities..sortStructsByDependencyHelper)
     * [~sortStructsByDependency(structs)](#module_REST API_ various zcl utilities..sortStructsByDependency) ⇒
     * [~calculateBytes(res, options, db, packageIds, isStructType)](#module_REST API_ various zcl utilities..calculateBytes)
     * [~optionsHashOrDefault(options, optionsKey, defaultValue)](#module_REST API_ various zcl utilities..optionsHashOrDefault)
@@ -19252,6 +19253,12 @@ Find struct by name from the given list of structs.
 | structs | <code>\*</code> | 
 | name | <code>\*</code> | 
 
+<a name="module_REST API_ various zcl utilities..sortStructsByDependencyHelper"></a>
+
+### REST API: various zcl utilities~sortStructsByDependencyHelper()
+Non-exported helper for sortStructsByDependency.
+
+**Kind**: inner method of [<code>REST API: various zcl utilities</code>](#module_REST API_ various zcl utilities)  
 <a name="module_REST API_ various zcl utilities..sortStructsByDependency"></a>
 
 ### REST API: various zcl utilities~sortStructsByDependency(structs) ⇒

src-electron/db/query-util.js

@@ -115,6 +115,7 @@ function sqlQueryForDataTypeByNameAndClusterName(
     ${typeTableName}.${typeTableName}_ID,
     ${structExtensionString}
     DATA_TYPE.NAME AS NAME,
+    (SELECT COUNT(1) FROM DATA_TYPE_CLUSTER WHERE DATA_TYPE_CLUSTER.DATA_TYPE_REF = ${typeTableName}.${typeTableName}_ID) AS ${typeTableName}_CLUSTER_COUNT,
     ${numberExtensionString}
     ${typeTableName}.SIZE AS SIZE,
     CLUSTER.NAME AS CLUSTER_NAME

src-electron/db/query-zcl.js

@@ -356,7 +356,7 @@ async function selectStructsWithItemsImpl(db, packageIds, clusterId) {
       S.STRUCT_ID = SI.STRUCT_REF
     WHERE
       DT.PACKAGE_REF IN (${dbApi.toInClause(packageIds)})
-    ORDER BY DT.NAME, SI.FIELD_IDENTIFIER`
+    ORDER BY DT.NAME, DT.DATA_TYPE_ID, SI.FIELD_IDENTIFIER`
   } else {
     query = `
     SELECT
@@ -395,18 +395,19 @@ async function selectStructsWithItemsImpl(db, packageIds, clusterId) {
       DT.PACKAGE_REF IN (${dbApi.toInClause(packageIds)})
     AND
       DTC.CLUSTER_REF = ?
-    ORDER BY DT.NAME, SI.FIELD_IDENTIFIER`
+    ORDER BY DT.NAME, DT.DATA_TYPE_ID, SI.FIELD_IDENTIFIER`
     args = [clusterId]
   }
 
   let rows = await dbApi.dbAll(db, query, args)
   return rows.reduce((acc, value) => {
     let objectToActOn
-    if (acc.length == 0 || acc[acc.length - 1].name != value.STRUCT_NAME) {
+    if (acc.length == 0 || acc[acc.length - 1].struct_id != value.STRUCT_ID) {
       // Create a new object
       objectToActOn = {
         id: value.STRUCT_ID,
         name: value.STRUCT_NAME,
+        struct_id: value.STRUCT_ID,
         isFabricScoped: dbApi.fromDbBool(value.IS_FABRIC_SCOPED),
         apiMaturity: value.API_MATURITY,
         label: value.STRUCT_NAME,

src-electron/generator/matter/app/zap-templates/templates/app/helper.js

@@ -733,15 +733,42 @@ async function zapTypeToClusterObjectType(type, isDecodable, options) {
 
     if (types.isStruct) {
       passByReference = true;
-      const structObj = await zclQuery.selectStructByName(
-        this.global.db,
-        type,
-        pkgId
-      );
-      const ns = nsValueToNamespace(
-        options.hash.ns,
-        structObj.structClusterCount
-      );
+      let clusterCount;
+      if (options.hash.cluster === '') {
+        // This is a non-global struct that is associated with multiple
+        // clusters: in that case our caller can pass in cluster="", and
+        // we know that clusterCount > 1, so just set it to 2.
+        clusterCount = 2;
+      } else {
+        const cluster = options.hash.cluster || options.hash.ns;
+        const structObj = await zclQuery.selectStructByNameAndClusterName(
+          this.global.db,
+          type,
+          cluster,
+          pkgId
+        );
+        if (structObj) {
+          clusterCount = structObj.structClusterCount;
+        } else if (options.hash.cluster === undefined) {
+          // Backwards-compat case: we were called without ns or cluster at all
+          // (not even cluster="").  Just get by name, since that's all we have to
+          // work with.  It won't work right when names are not unique, but that's
+          // the best we can do.
+          const backwardsCompatStructObj = await zclQuery.selectStructByName(
+            this.global.db,
+            type,
+            pkgId
+          );
+          clusterCount = backwardsCompatStructObj.structClusterCount;
+        } else {
+          // Something is wrong here.  Possibly the caller is passing in a munged
+          // cluster name.  Just fail out instead of silently returning bad data.
+          throw new Error(
+            `Unable to find struct ${type} in cluster ${options.hash.cluster}`
+          );
+        }
+      }
+      const ns = nsValueToNamespace(options.hash.ns, clusterCount);
       return (
         ns +
         'Structs::' +
@@ -847,11 +874,23 @@ async function _zapTypeToPythonClusterObjectType(type, options) {
     }
 
     if (await typeChecker('isStruct')) {
-      const structObj = await zclQuery.selectStructByName(
-        this.global.db,
-        type,
-        pkgId
-      );
+      let structObj;
+      if (options.hash.cluster) {
+        structObj = await zclQuery.selectStructByNameAndClusterName(
+          this.global.db,
+          type,
+          options.hash.cluster,
+          pkgId
+        );
+      } else {
+        // Backwards-compat case, which won't work when multiple structs have
+        // the same name.
+        structObj = await zclQuery.selectStructByName(
+          this.global.db,
+          type,
+          pkgId
+        );
+      }
 
       const ns = nsValueToPythonNamespace(
         options.hash.ns,
@@ -947,11 +986,23 @@ async function _getPythonFieldDefault(type, options) {
     }
 
     if (await typeChecker('isStruct')) {
-      const structObj = await zclQuery.selectStructByName(
-        this.global.db,
-        type,
-        pkgId
-      );
+      let structObj;
+      if (options.hash.cluster) {
+        structObj = await zclQuery.selectStructByNameAndClusterName(
+          this.global.db,
+          type,
+          options.hash.cluster,
+          pkgId
+        );
+      } else {
+        // Backwards-compat case, which won't work when multiple structs have
+        // the same name.
+        structObj = await zclQuery.selectStructByName(
+          this.global.db,
+          type,
+          pkgId
+        );
+      }
 
       const ns = nsValueToPythonNamespace(
         options.hash.ns,
@@ -1133,7 +1184,19 @@ async function zcl_commands_that_need_timed_invoke(options) {
 // struct.
 async function if_is_fabric_scoped_struct(type, options) {
   let packageIds = await templateUtil.ensureZclPackageIds(this);
-  let st = await zclQuery.selectStructByName(this.global.db, type, packageIds);
+  let st;
+  if (options.hash.cluster) {
+    st = await zclQuery.selectStructByNameAndClusterName(
+      this.global.db,
+      type,
+      options.hash.cluster,
+      packageIds
+    );
+  } else {
+    // Backwards compat case; will not work right when multiple structs share
+    // the same name.
+    st = await zclQuery.selectStructByName(this.global.db, type, packageIds);
+  }
 
   if (st && st.isFabricScoped) {
     return options.fn(this);

src-electron/util/zcl-util.js

@@ -110,16 +110,9 @@ function findStructByName(structs, name) {
 }
 
 /**
- * This method retrieves a bunch of structs sorted
- * alphabetically. It's expected to resort the structs into a list
- * where they are sorted in a way where dependency is observed.
- *
- * It uses the DFS-based topological sort algorithm.
- *
- * @param {*} structs
- * @returns sorted structs according to topological search.
+ * Non-exported helper for sortStructsByDependency.
  */
-async function sortStructsByDependency(structs) {
+function sortStructsByDependencyHelper(structs) {
   let allStructNames = structs.map((s) => s.name)
   let edges = []
 
@@ -146,6 +139,37 @@ async function sortStructsByDependency(structs) {
   return finalSort
 }
 
+/**
+ * This method retrieves a bunch of structs sorted
+ * alphabetically. It's expected to resort the structs into a list
+ * where they are sorted in a way where dependency is observed.
+ *
+ * It uses the DFS-based topological sort algorithm.
+ *
+ * @param {*} structs
+ * @returns sorted structs according to topological search.
+ */
+async function sortStructsByDependency(structs) {
+  // Given global and non-global structs, the way dependencies work is this:
+  //
+  // 1) Global structs can only depend on other global structs.
+  // 2) Non-global structs can depend on either non-global or global structs.
+  //
+  // So we can output all global structs first (sorted by dependency), and then
+  // the non-global structs, again sorted by dependency.
+  //
+  // NOTE: the topological sort we use is not a stable sort, so adding some
+  // structs to the list can entirely change the order of all the structs.
+  let sortedGlobalStructs = sortStructsByDependencyHelper(
+    structs.filter((s) => s.struct_cluster_count == 0)
+  )
+  let sortedNonGlobalStructs = sortStructsByDependencyHelper(
+    structs.filter((s) => s.struct_cluster_count != 0)
+  )
+
+  return sortedGlobalStructs.concat(sortedNonGlobalStructs)
+}
+
 /**
  * This function calculates the number of bytes in the data type and based on
  * that returns the option specified in the template.