Skip to content

Commit

Permalink
Ignore persisted values when features change
Browse files Browse the repository at this point in the history
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
  • Loading branch information
lauckhart committed Nov 15, 2024
1 parent 6465702 commit e6f4abf
Show file tree
Hide file tree
Showing 3 changed files with 100 additions and 2 deletions.
21 changes: 20 additions & 1 deletion packages/model/src/logic/ModelTraversal.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,10 @@ const OPERATION_DEPTH_LIMIT = 20;

let memos: Memos | undefined;

// Member caches. Only populated for frozen models
const activeMemberCache = new WeakMap<Model, ValueModel[]>();
const conformantMemberCache = new WeakMap<Model, ValueModel[]>();

/**
* 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.
Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -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;
}

/**
Expand Down
27 changes: 26 additions & 1 deletion packages/node/src/behavior/state/managed/Datasource.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -206,6 +208,7 @@ interface Internals extends Datasource.Options {
values: Val.Struct;
version: number;
sessions?: Map<ValueSupervisor.Session, SessionContext>;
featuresKey?: string;
interactionObserver(): MaybePromise<void>;
}

Expand All @@ -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];
}
Expand All @@ -236,6 +256,7 @@ function configure(options: Datasource.Options): Internals {
...options,
version: Crypto.getRandomUInt32(),
values: values,
featuresKey,

interactionObserver() {
function handleObserverError(error: any) {
Expand Down Expand Up @@ -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);
}

Expand Down
54 changes: 54 additions & 0 deletions packages/node/test/node/ServerNodeTest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -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";
Expand Down Expand Up @@ -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) {
Expand Down

0 comments on commit e6f4abf

Please sign in to comment.