From e6f4abf5376f43df4f45675a69b4e45dcb2b78ef Mon Sep 17 00:00:00 2001 From: Greg Lauckhart Date: Fri, 15 Nov 2024 11:49:48 -0800 Subject: [PATCH] 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 --- packages/model/src/logic/ModelTraversal.ts | 21 +++++++- .../src/behavior/state/managed/Datasource.ts | 27 +++++++++- packages/node/test/node/ServerNodeTest.ts | 54 +++++++++++++++++++ 3 files changed, 100 insertions(+), 2 deletions(-) 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..dbd9811a23 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,28 @@ 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(","); + if (storedValues?.[FEATURES_KEY] !== undefined && storedValues[FEATURES_KEY] !== featuresKey) { + logger.warn( + `Ignoring persisted values for ${options.path} because features changed from "${values.featuresKey}" 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 +256,7 @@ function configure(options: Datasource.Options): Internals { ...options, version: Crypto.getRandomUInt32(), values: values, + featuresKey, interactionObserver() { function handleObserverError(error: any) { @@ -567,6 +588,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) {