Resolve element optionality via both optional attribute and conformance (#1578)

* parse isOptional from both optional attribute and conformance

Author: ethanzhouyc

docs/api.md

@@ -17935,6 +17935,7 @@ Environment utilities for ZAP
     * [.logBrowser(msg, err)](#module_JS API_ Environment utilities.logBrowser)
     * [.logIpc(msg, err)](#module_JS API_ Environment utilities.logIpc)
     * [.logDebug(msg, err)](#module_JS API_ Environment utilities.logDebug)
+    * [.logWarningToFile(msg)](#module_JS API_ Environment utilities.logWarningToFile)
     * [.isMatchingVersion(versionsArray, providedVersion)](#module_JS API_ Environment utilities.isMatchingVersion) ⇒
     * [.versionsCheck()](#module_JS API_ Environment utilities.versionsCheck) ⇒
     * [.httpStaticContent()](#module_JS API_ Environment utilities.httpStaticContent) ⇒
@@ -18244,6 +18245,17 @@ Debug level message.
 | msg | <code>\*</code> | 
 | err | <code>\*</code> | 
 
+<a name="module_JS API_ Environment utilities.logWarningToFile"></a>
+
+### JS API: Environment utilities.logWarningToFile(msg)
+Log Warning level message to zap.log file.
+
+**Kind**: static method of [<code>JS API: Environment utilities</code>](#module_JS API_ Environment utilities)  
+
+| Param | Type |
+| --- | --- |
+| msg | <code>\*</code> | 
+
 <a name="module_JS API_ Environment utilities.isMatchingVersion"></a>
 
 ### JS API: Environment utilities.isMatchingVersion(versionsArray, providedVersion) ⇒
@@ -20175,6 +20187,7 @@ This module provides utilities for evaluating conformance expressions.
         * [~evaluateBooleanExpression(expr)](#module_Validation API_ Evaluate conformance expressions..evaluateConformanceExpression..evaluateBooleanExpression)
         * [~evaluateWithParentheses(expr)](#module_Validation API_ Evaluate conformance expressions..evaluateConformanceExpression..evaluateWithParentheses)
     * [~checkMissingTerms(expression, elementMap)](#module_Validation API_ Evaluate conformance expressions..checkMissingTerms) ⇒
+    * [~checkIfExpressionHasTerm(expression, term)](#module_Validation API_ Evaluate conformance expressions..checkIfExpressionHasTerm) ⇒
 
 <a name="module_Validation API_ Evaluate conformance expressions..evaluateConformanceExpression"></a>
 
@@ -20235,6 +20248,19 @@ If so, it means the conformance depends on terms with unknown values and changes
 | expression | <code>\*</code> | 
 | elementMap | <code>\*</code> | 
 
+<a name="module_Validation API_ Evaluate conformance expressions..checkIfExpressionHasTerm"></a>
+
+### Validation API: Evaluate conformance expressions~checkIfExpressionHasTerm(expression, term) ⇒
+Check if the expression contains a given term.
+
+**Kind**: inner method of [<code>Validation API: Evaluate conformance expressions</code>](#module_Validation API_ Evaluate conformance expressions)  
+**Returns**: true if the expression contains the term, false otherwise  
+
+| Param |
+| --- |
+| expression | 
+| term | 
+
 <a name="module_Validation API_ Parse conformance data from XML"></a>
 
 ## Validation API: Parse conformance data from XML
@@ -20244,6 +20270,7 @@ This module provides utilities for parsing conformance data from XML into expres
 * [Validation API: Parse conformance data from XML](#module_Validation API_ Parse conformance data from XML)
     * [~parseConformanceFromXML(operand)](#module_Validation API_ Parse conformance data from XML..parseConformanceFromXML) ⇒
     * [~parseConformanceRecursively(operand, depth, parentJoinChar)](#module_Validation API_ Parse conformance data from XML..parseConformanceRecursively) ⇒
+    * [~getOptionalAttributeFromXML(element, elementType)](#module_Validation API_ Parse conformance data from XML..getOptionalAttributeFromXML) ⇒
 
 <a name="module_Validation API_ Parse conformance data from XML..parseConformanceFromXML"></a>
 
@@ -20305,6 +20332,23 @@ When they appear, stop recursing and return the name inside directly
 | depth | <code>\*</code> | <code>0</code> | 
 | parentJoinChar | <code>\*</code> |  | 
 
+<a name="module_Validation API_ Parse conformance data from XML..getOptionalAttributeFromXML"></a>
+
+### Validation API: Parse conformance data from XML~getOptionalAttributeFromXML(element, elementType) ⇒
+if optional attribute is defined, return its value
+if optional attribute is undefined, check if the element conformance is mandatory
+if both optional attribute and conformance are undefined, return false
+Optional attribute takes precedence over conformance for backward compatibility on certain elements
+Log warnings to zap.log if both optional attribute and conformance are defined
+
+**Kind**: inner method of [<code>Validation API: Parse conformance data from XML</code>](#module_Validation API_ Parse conformance data from XML)  
+**Returns**: true if the element is optional, false if the element is mandatory  
+
+| Param | Type |
+| --- | --- |
+| element | <code>\*</code> | 
+| elementType | <code>\*</code> | 
+
 <a name="module_Validation API_ Validation APIs"></a>
 
 ## Validation API: Validation APIs

src-electron/util/env.js

@@ -196,6 +196,11 @@ let httpStaticContentPath = path.join(__dirname, '../../../spa')
 let versionObject = null
 let applicationStateDirectory = null
 
+let file_pino_logger = pino(
+  pinoOptions,
+  pino.destination({ dest: path.join(appDirectory(), 'zap.log'), sync: true })
+)
+
 /**
  * Set up the devlopment environment.
  */
@@ -524,6 +529,18 @@ export function logDebug(msg, err = null) {
   log('debug', msg, err)
 }
 
+/**
+ * Log Warning level message to zap.log file.
+ *
+ * @param {*} msg
+ */
+export function logWarningToFile(msg) {
+  let objectToLog = {
+    msg: msg
+  }
+  file_pino_logger.warn(objectToLog)
+}
+
 /**
  * Returns true if major or minor component of versions is different.
  *

src-electron/validation/conformance-expression-evaluator.js

@@ -132,5 +132,18 @@ function checkMissingTerms(expression, elementMap) {
   return missingTerms
 }
 
+/**
+ * Check if the expression contains a given term.
+ *
+ * @param expression
+ * @param term
+ * @returns true if the expression contains the term, false otherwise
+ */
+function checkIfExpressionHasTerm(expression, term) {
+  let terms = expression.match(/[A-Za-z][A-Za-z0-9_]*/g)
+  return terms && terms.includes(term)
+}
+
 exports.evaluateConformanceExpression = evaluateConformanceExpression
 exports.checkMissingTerms = checkMissingTerms
+exports.checkIfExpressionHasTerm = checkIfExpressionHasTerm

src-electron/validation/conformance-xml-parser.js

@@ -21,6 +21,10 @@
  * @module Validation API: Parse conformance data from XML
  */
 
+const dbEnum = require('../../src-shared/db-enum')
+const conformEvaluator = require('./conformance-expression-evaluator')
+const env = require('../util/env')
+
 /**
  * Parses conformance from XML data.
  * The conformance could come from features, attributes, commands, or events
@@ -155,4 +159,38 @@ function parseConformanceRecursively(operand, depth = 0, parentJoinChar = '') {
   }
 }
 
+/**
+ * if optional attribute is defined, return its value
+ * if optional attribute is undefined, check if the element conformance is mandatory
+ * if both optional attribute and conformance are undefined, return false
+ * Optional attribute takes precedence over conformance for backward compatibility on certain elements
+ * Log warnings to zap.log if both optional attribute and conformance are defined
+ *
+ * @param {*} element
+ * @param {*} elementType
+ * @returns true if the element is optional, false if the element is mandatory
+ */
+function getOptionalAttributeFromXML(element, elementType) {
+  let conformance = parseConformanceFromXML(element)
+  if (element.$.optional) {
+    if (conformance) {
+      env.logWarningToFile(
+        `Redundant 'optional' attribute and 'conformance' tag defined for ${elementType}: ${element.$.name}.` +
+          " 'optional' takes precedence, but consider removing it as 'conformance' is the recommended format."
+      )
+    }
+    return element.$.optional == 'true'
+  } else {
+    if (conformance) {
+      return !conformEvaluator.checkIfExpressionHasTerm(
+        conformance,
+        dbEnum.conformance.mandatory
+      )
+    } else {
+      return false
+    }
+  }
+}
+
 exports.parseConformanceFromXML = parseConformanceFromXML
+exports.getOptionalAttributeFromXML = getOptionalAttributeFromXML

src-electron/zcl/zcl-loader-silabs.js

@@ -506,7 +506,10 @@ function prepareCluster(cluster, context, isExtension = false) {
         name: command.$.name,
         description: command.description ? command.description[0].trim() : '',
         source: command.$.source,
-        isOptional: command.$.optional == 'true' ? true : false,
+        isOptional: conformParser.getOptionalAttributeFromXML(
+          command,
+          'command'
+        ),
         conformance: conformParser.parseConformanceFromXML(command),
         mustUseTimedInvoke: command.$.mustUseTimedInvoke == 'true',
         introducedIn: command.$.introducedIn,
@@ -567,7 +570,7 @@ function prepareCluster(cluster, context, isExtension = false) {
         conformance: conformParser.parseConformanceFromXML(event),
         priority: event.$.priority,
         description: event.description ? event.description[0].trim() : '',
-        isOptional: event.$.optional == 'true',
+        isOptional: conformParser.getOptionalAttributeFromXML(event, 'event'),
         isFabricSensitive: event.$.isFabricSensitive == 'true'
       }
       ev.access = extractAccessIntoArray(event)
@@ -685,7 +688,10 @@ function prepareCluster(cluster, context, isExtension = false) {
           : null,
         isWritable: attribute.$.writable == 'true',
         defaultValue: attribute.$.default,
-        isOptional: attribute.$.optional == 'true',
+        isOptional: conformParser.getOptionalAttributeFromXML(
+          attribute,
+          'attribute'
+        ),
         reportingPolicy: reportingPolicy,
         storagePolicy: storagePolicy,
         isSceneRequired:

src-shared/db-enum.js

@@ -247,3 +247,12 @@ exports.featureMapAttribute = {
   name: 'FeatureMap',
   code: 65532
 }
+
+exports.conformance = {
+  mandatory: 'M',
+  optional: 'O',
+  disallowed: 'X',
+  deprecated: 'D',
+  provisional: 'P',
+  desc: 'desc'
+}

test/test-query.js

@@ -255,6 +255,17 @@ WHERE
     .then((rows) => rows.map(dbMapping.map.cluster))
 }
 
+/**
+ *
+ * @param {*} elements
+ * @param {*} name
+ * @returns true if the element is optional, false if the element is mandatory
+ */
+function checkIfElementIsOptional(elements, name) {
+  let element = elements.find((element) => element.name == name)
+  return element ? element.isOptional : false
+}
+
 exports.getAllEndpointTypeClusterState = getAllEndpointTypeClusterState
 exports.selectCountFrom = selectCountFrom
 exports.getEndpointTypeAttributes = getEndpointTypeAttributes
@@ -263,3 +274,4 @@ exports.createSession = createSession
 exports.getAllSessionNotifications = getAllSessionNotifications
 exports.getAllNotificationMessages = getAllNotificationMessages
 exports.getAllSessionClusters = getAllSessionClusters
+exports.checkIfElementIsOptional = checkIfElementIsOptional

test/zcl-loader.test.js

@@ -23,6 +23,7 @@ const dbEnum = require('../src-shared/db-enum')
 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 queryEvent = require('../src-electron/db/query-event')
 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')
@@ -594,14 +595,14 @@ test(
       let ctx = await zclLoader.loadZcl(db, env.builtinMatterZclMetafile())
       let packageId = ctx.packageId
 
-      let zclCluster = await queryZcl.selectClusterByCode(db, packageId, 0x001f)
+      let aclCluster = await queryZcl.selectClusterByCode(db, packageId, 0x001f)
 
       /* Verify that the ACL attribute, defined using the list type format `array="true" type="X"` in XML,
         is correctly parsed and stored in the database as an array of AccessControlEntryStruct. */
       let attributes = await dbApi.dbAll(
         db,
         "SELECT * FROM ATTRIBUTE WHERE CLUSTER_REF = ? AND CODE = 0x0000 AND NAME = 'ACL'",
-        [zclCluster.id]
+        [aclCluster.id]
       )
       expect(attributes.length).toBe(1)
       let aclAttribute = attributes[0]
@@ -623,3 +624,111 @@ test(
   },
   testUtil.timeout.long()
 )
+
+test(
+  'test loading IS_OPTIONAL column for Matter attributes, commands, and events',
+  async () => {
+    let db = await dbApi.initRamDatabase()
+    try {
+      await dbApi.loadSchema(db, env.schemaFile(), env.zapVersion())
+      let ctx = await zclLoader.loadZcl(db, env.builtinMatterZclMetafile())
+      let packageId = ctx.packageId
+
+      let doorLockCluster = await queryZcl.selectClusterByCode(
+        db,
+        packageId,
+        0x0101
+      )
+      let doorLockClusterId = doorLockCluster.id
+
+      let attributes =
+        await queryZcl.selectAttributesByClusterIdIncludingGlobal(
+          db,
+          doorLockClusterId,
+          [packageId]
+        )
+      // optional attribute undefined, conformance undefined -> mandatory
+      expect(
+        testQuery.checkIfElementIsOptional(attributes, 'ActuatorEnabled')
+      ).toBeFalsy()
+      // optional="true", conformance undefined -> optional
+      expect(
+        testQuery.checkIfElementIsOptional(attributes, 'DoorClosedEvents')
+      ).toBeTruthy()
+      // optional="false", conformance undefined -> mandatory
+      expect(
+        testQuery.checkIfElementIsOptional(attributes, 'LockType')
+      ).toBeFalsy()
+      // mandatory conformance, optional attribute undefined -> mandatory
+      expect(
+        testQuery.checkIfElementIsOptional(attributes, 'OperatingMode')
+      ).toBeFalsy()
+      // optionalConform to feature DPS, optional="true" -> optional
+      expect(
+        testQuery.checkIfElementIsOptional(attributes, 'DoorOpenEvents')
+      ).toBeTruthy()
+      // mandatory conformance, optional="true" -> optional as optional="true" takes precedence
+      expect(
+        testQuery.checkIfElementIsOptional(attributes, 'LockState')
+      ).toBeTruthy()
+      // mandatoryConform to feature DPS, optional="false" -> mandatory as optional="false" takes precedence
+      expect(
+        testQuery.checkIfElementIsOptional(attributes, 'DoorState')
+      ).toBeFalsy()
+
+      let commands = await queryCommand.selectCommandsByClusterId(
+        db,
+        doorLockClusterId,
+        [packageId]
+      )
+      // optional attribute undefined, conformance undefined -> mandatory
+      expect(
+        testQuery.checkIfElementIsOptional(commands, 'LockDoor')
+      ).toBeFalsy()
+      // optional="true", conformance undefined -> optional
+      expect(
+        testQuery.checkIfElementIsOptional(commands, 'GetWeekDaySchedule')
+      ).toBeTruthy()
+      // mandatory conformance, optional attribute undefined -> mandatory
+      expect(
+        testQuery.checkIfElementIsOptional(commands, 'UnlockDoor')
+      ).toBeFalsy()
+      // mandatoryConform to feature WDSCH, optional="true" -> optional
+      expect(
+        testQuery.checkIfElementIsOptional(commands, 'SetWeekDaySchedule')
+      ).toBeTruthy()
+      // optional conformance, optional="false" -> mandatory as optional="false" takes precedence
+      expect(
+        testQuery.checkIfElementIsOptional(commands, 'UnlockWithTimeout')
+      ).toBeFalsy()
+
+      let events = await queryEvent.selectEventsByClusterId(
+        db,
+        doorLockClusterId
+      )
+      // optional attribute undefined, conformance undefined -> mandatory
+      expect(
+        testQuery.checkIfElementIsOptional(events, 'LockUserChange')
+      ).toBeFalsy()
+      // optional="true", conformance undefined -> optional
+      expect(
+        testQuery.checkIfElementIsOptional(events, 'LockOperation')
+      ).toBeTruthy()
+      // mandatory conformance, optional attribute undefined -> mandatory
+      expect(
+        testQuery.checkIfElementIsOptional(events, 'LockOperationError')
+      ).toBeFalsy()
+      // mandatoryConform to feature DPS, optional="true" -> optional
+      expect(
+        testQuery.checkIfElementIsOptional(events, 'DoorStateChange')
+      ).toBeTruthy()
+      // mandatory conformance, optional="true" -> optional as optional="true" takes precedence
+      expect(
+        testQuery.checkIfElementIsOptional(events, 'DoorLockAlarm')
+      ).toBeTruthy()
+    } finally {
+      await dbApi.closeDatabase(db)
+    }
+  },
+  testUtil.timeout.long()
+)