From c923f6d7540c6a6a0d7ba9e048d46602d0c4165c Mon Sep 17 00:00:00 2001 From: lauckhart Date: Fri, 15 Nov 2024 14:15:28 -0800 Subject: [PATCH] Ignore persisted values when features change (#1399) * Ignore persisted values when features change Affects individual behaviors on endpoints with persisted values. Storage must be tagged with features, so storage created in previous versions of code will not benefit from this enhancement. Fixes #1161 * Ignore persisted values when features change Affects individual behaviors on endpoints with persisted values. Storage must be tagged with features, so storage created in previous versions of code will not benefit from this enhancement. Fixes #1161 * Fix from PR feedback --- CHANGELOG.md | 1 + packages/model/src/logic/ModelTraversal.ts | 21 +++++++- .../src/behavior/state/managed/Datasource.ts | 28 +++++++++- packages/node/test/node/ServerNodeTest.ts | 54 +++++++++++++++++++ 4 files changed, 102 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index de73733c29..ef313ceba7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ The main work (all changes without a GitHub username in brackets in the below li - @matter/node - Enhancement: The `with` functions on endpoint and cluster behavior types now alias to `withBehaviors` and `withFeatures` respectively to make their function more explicit + - Enhancement: Endpoints now ignore persisted values for clusters when features change across restarts. This allows for startup when persisted values become invalid due to conformance rules - Fix: Triggers CommissioningServer#initiateCommissioning when server restarts outside of factory reset - @matter/nodejs diff --git a/packages/model/src/logic/ModelTraversal.ts b/packages/model/src/logic/ModelTraversal.ts index 9e404cd9b2..6f05e55c85 100644 --- a/packages/model/src/logic/ModelTraversal.ts +++ b/packages/model/src/logic/ModelTraversal.ts @@ -28,6 +28,10 @@ const OPERATION_DEPTH_LIMIT = 20; let memos: Memos | undefined; +// Member caches. Only populated for frozen models +const activeMemberCache = new WeakMap(); +const conformantMemberCache = new WeakMap(); + /** * This class performs lookups of models in the scope of a specific model. We use a class so the lookup can maintain * state and guard against circular references. @@ -526,12 +530,21 @@ export class ModelTraversal { * * - If there are multiple applicable members based on conformance the definitions conflict and throw an error * + * If the model is frozen we cache the return value. + * * Note that "active" in this case does not imply the member is conformant, only that conflicts are resolved. * * Note 2 - members may not be differentiated with conformance rules that rely on field values in this way. That * will probably never be necessary and would require an entirely different (more complicated) structure. */ findActiveMembers(scope: Model & { members: PropertyModel[] }, conformantOnly: boolean, cluster?: ClusterModel) { + const cache = Object.isFrozen(scope) ? (conformantOnly ? conformantMemberCache : activeMemberCache) : undefined; + + const cached = cache?.get(scope); + if (cached) { + return cached; + } + const features = cluster?.featureNames ?? new FeatureSet(); const supportedFeatures = cluster?.supportedFeatures ?? new FeatureSet(); @@ -564,7 +577,13 @@ export class ModelTraversal { selectedMembers[member.name] = member; } - return Object.values(selectedMembers); + const result = Object.values(selectedMembers); + + if (cache) { + cache.set(scope, result); + } + + return result; } /** diff --git a/packages/node/src/behavior/state/managed/Datasource.ts b/packages/node/src/behavior/state/managed/Datasource.ts index f1d99a9975..5afa8fa75c 100644 --- a/packages/node/src/behavior/state/managed/Datasource.ts +++ b/packages/node/src/behavior/state/managed/Datasource.ts @@ -28,6 +28,8 @@ import { ReadOnlyTransaction } from "../transaction/Tx.js"; const logger = Logger.get("Datasource"); +const FEATURES_KEY = "__features__"; + /** * Datasource manages the canonical root of a state tree. The "state" property of a Behavior is a reference to a * Datasource. @@ -206,6 +208,7 @@ interface Internals extends Datasource.Options { values: Val.Struct; version: number; sessions?: Map; + featuresKey?: string; interactionObserver(): MaybePromise; } @@ -223,11 +226,29 @@ interface CommitChanges { function configure(options: Datasource.Options): Internals { const values = new options.type() as Val.Struct; + let storedValues = options.store?.initialValues; + + let featuresKey: undefined | string; + if (options.supervisor.featureMap.children.length) { + featuresKey = [...options.supervisor.supportedFeatures].join(","); + const storedFeaturesKey = storedValues?.[FEATURES_KEY]; + if (storedFeaturesKey !== undefined && storedFeaturesKey !== featuresKey) { + logger.warn( + `Ignoring persisted values for ${options.path} because features changed from "${storedFeaturesKey}" to "${featuresKey}"`, + ); + storedValues = undefined; + } + } + const initialValues = { ...options.defaults, - ...options.store?.initialValues, + ...storedValues, }; + if (FEATURES_KEY in initialValues) { + delete initialValues[FEATURES_KEY]; + } + for (const key in initialValues) { values[key] = initialValues[key]; } @@ -236,6 +257,7 @@ function configure(options: Datasource.Options): Internals { ...options, version: Crypto.getRandomUInt32(), values: values, + featuresKey, interactionObserver() { function handleObserverError(error: any) { @@ -567,6 +589,10 @@ function createSessionContext(resource: Resource, internals: Internals, session: return; } + if (internals.featuresKey !== undefined) { + persistent[FEATURES_KEY] = internals.featuresKey; + } + return internals.store?.set(session.transaction, persistent); } diff --git a/packages/node/test/node/ServerNodeTest.ts b/packages/node/test/node/ServerNodeTest.ts index d79cde85a9..25d3a5eecd 100644 --- a/packages/node/test/node/ServerNodeTest.ts +++ b/packages/node/test/node/ServerNodeTest.ts @@ -10,6 +10,8 @@ import { DescriptorBehavior } from "#behaviors/descriptor"; import { PumpConfigurationAndControlServer } from "#behaviors/pump-configuration-and-control"; import { GeneralCommissioning } from "#clusters/general-commissioning"; import { PumpConfigurationAndControl } from "#clusters/pump-configuration-and-control"; +import { ColorTemperatureLightDevice } from "#devices/color-temperature-light"; +import { ExtendedColorLightDevice } from "#devices/extended-color-light"; import { LightSensorDevice } from "#devices/light-sensor"; import { OnOffLightDevice } from "#devices/on-off-light"; import { PumpDevice } from "#devices/pump"; @@ -28,6 +30,9 @@ import { MockUdpChannel, NetworkSimulator, PrivateKey, + StorageBackendMemory, + StorageManager, + StorageService, } from "#general"; import { ServerNode } from "#node/ServerNode.js"; import { AttestationCertificateManager, CertificationDeclarationManager, FabricManager } from "#protocol"; @@ -492,6 +497,55 @@ describe("ServerNode", () => { }); }); }); + + it("is resilient to conformance changes that affect persisted data", async () => { + const environment = new Environment("test"); + const service = environment.get(StorageService); + + // Configure storage that will survive node replacement + const storage = new StorageManager(new StorageBackendMemory()); + storage.close = () => {}; + await storage.initialize(); + service.open = () => Promise.resolve(storage); + + // Initialize a node with extended color light, ensure levelX persists + { + const node = new MockServerNode({ id: "node0", environment }); + + await node.construction.ready; + + const originalEndpoint = await node.add(ExtendedColorLightDevice, { + id: "foo", + number: 1, + colorControl: { + startUpColorTemperatureMireds: 0, + coupleColorTempToLevelMinMireds: 0, + }, + }); + + await originalEndpoint.set({ colorControl: { currentX: 12 } }); + + await node.close(); + } + + // Initialize a node with color temp light, levelX won't be supported + { + const node = new MockServerNode({ id: "node0", environment }); + + await node.construction.ready; + + await node.add(ColorTemperatureLightDevice, { + id: "foo", + number: 1, + colorControl: { + startUpColorTemperatureMireds: 0, + coupleColorTempToLevelMinMireds: 0, + }, + }); + + await node.close(); + } + }); }); async function almostCommission(node?: MockServerNode, number = 0) {