diff --git a/.changeset/modern-candies-clean.md b/.changeset/modern-candies-clean.md new file mode 100644 index 000000000000..a8a8665570c3 --- /dev/null +++ b/.changeset/modern-candies-clean.md @@ -0,0 +1,7 @@ +--- +"@fluidframework/presence": minor +"__section": breaking +--- +Removal of number key support in LatestMap + +`number` keys have never been successfully propagated as `number`s at runtime and this type clarification makes that clear. See [issue 25322](https://github.com/microsoft/FluidFramework/issues/25919) for more details. diff --git a/packages/framework/presence/api-report/presence.beta.api.md b/packages/framework/presence/api-report/presence.beta.api.md index 2b91d8cc46cb..7bd1c85158e5 100644 --- a/packages/framework/presence/api-report/presence.beta.api.md +++ b/packages/framework/presence/api-report/presence.beta.api.md @@ -65,7 +65,7 @@ export namespace InternalTypes { manager: StateValue; }); // @system - export interface MapValueState { + export interface MapValueState { // (undocumented) items: { [name in Keys]: ValueOptionalState; @@ -118,6 +118,9 @@ export namespace InternalTypes { } } +// @beta +export type KeySchemaValidator = (unvalidatedKey: string) => unvalidatedKey is Keys; + // @beta @sealed export interface Latest = ProxiedValueAccessor> { readonly controls: BroadcastControls; @@ -169,7 +172,7 @@ export interface LatestFactory { } // @beta @sealed -export interface LatestMap = ProxiedValueAccessor> { +export interface LatestMap = ProxiedValueAccessor> { readonly controls: BroadcastControls; readonly events: Listenable>; getRemote(attendee: Attendee): ReadonlyMap>; @@ -180,12 +183,13 @@ export interface LatestMap extends LatestMapArgumentsRaw { +export interface LatestMapArguments extends LatestMapArgumentsRaw { + keyValidator?: KeySchemaValidator; validator: StateSchemaValidator; } // @beta @input -export interface LatestMapArgumentsRaw { +export interface LatestMapArgumentsRaw { local?: { [K in Keys]: JsonSerializable; }; @@ -193,13 +197,13 @@ export interface LatestMapArgumentsRaw, SpecificAttendeeId extends AttendeeId = AttendeeId> { +export interface LatestMapClientData, SpecificAttendeeId extends AttendeeId = AttendeeId> { attendee: Attendee; items: ReadonlyMap>; } // @beta @sealed -export interface LatestMapEvents = ProxiedValueAccessor> { +export interface LatestMapEvents = ProxiedValueAccessor> { // @eventProperty localItemRemoved: (removedItem: { key: K; @@ -219,27 +223,27 @@ export interface LatestMapEvents(args: LatestMapArguments): InternalTypes.ManagerFactory, LatestMap>; - (args?: LatestMapArgumentsRaw): InternalTypes.ManagerFactory, LatestMapRaw>; + (args: LatestMapArguments): InternalTypes.ManagerFactory, LatestMap>; + (args?: LatestMapArgumentsRaw): InternalTypes.ManagerFactory, LatestMapRaw>; } // @beta @sealed -export interface LatestMapItemRemovedClientData { +export interface LatestMapItemRemovedClientData { attendee: Attendee; key: K; metadata: LatestMetadata; } // @beta @sealed -export interface LatestMapItemUpdatedClientData> extends LatestClientData { +export interface LatestMapItemUpdatedClientData> extends LatestClientData { key: K; } // @beta @sealed -export type LatestMapRaw = LatestMap>; +export type LatestMapRaw = LatestMap>; // @beta @sealed -export type LatestMapRawEvents = LatestMapEvents>; +export type LatestMapRawEvents = LatestMapEvents>; // @beta @sealed export interface LatestMetadata { @@ -295,7 +299,7 @@ export const StateFactory: { }; // @beta @sealed -export interface StateMap { +export interface StateMap { clear(): void; delete(key: K): boolean; forEach(callbackfn: (value: DeepReadonly>, key: K, map: StateMap) => void, thisArg?: unknown): void; diff --git a/packages/framework/presence/api-report/presence.legacy.alpha.api.md b/packages/framework/presence/api-report/presence.legacy.alpha.api.md index 0c39bbf9c8db..05e6f4660225 100644 --- a/packages/framework/presence/api-report/presence.legacy.alpha.api.md +++ b/packages/framework/presence/api-report/presence.legacy.alpha.api.md @@ -81,7 +81,7 @@ export namespace InternalTypes { manager: StateValue; }); // @system - export interface MapValueState { + export interface MapValueState { // (undocumented) items: { [name in Keys]: ValueOptionalState; @@ -148,6 +148,9 @@ export namespace InternalUtilityTypes { }; } +// @beta +export type KeySchemaValidator = (unvalidatedKey: string) => unvalidatedKey is Keys; + // @beta @sealed export interface Latest = ProxiedValueAccessor> { readonly controls: BroadcastControls; @@ -199,7 +202,7 @@ export interface LatestFactory { } // @beta @sealed -export interface LatestMap = ProxiedValueAccessor> { +export interface LatestMap = ProxiedValueAccessor> { readonly controls: BroadcastControls; readonly events: Listenable>; getRemote(attendee: Attendee): ReadonlyMap>; @@ -210,12 +213,13 @@ export interface LatestMap extends LatestMapArgumentsRaw { +export interface LatestMapArguments extends LatestMapArgumentsRaw { + keyValidator?: KeySchemaValidator; validator: StateSchemaValidator; } // @beta @input -export interface LatestMapArgumentsRaw { +export interface LatestMapArgumentsRaw { local?: { [K in Keys]: JsonSerializable; }; @@ -223,13 +227,13 @@ export interface LatestMapArgumentsRaw, SpecificAttendeeId extends AttendeeId = AttendeeId> { +export interface LatestMapClientData, SpecificAttendeeId extends AttendeeId = AttendeeId> { attendee: Attendee; items: ReadonlyMap>; } // @beta @sealed -export interface LatestMapEvents = ProxiedValueAccessor> { +export interface LatestMapEvents = ProxiedValueAccessor> { // @eventProperty localItemRemoved: (removedItem: { key: K; @@ -249,27 +253,27 @@ export interface LatestMapEvents(args: LatestMapArguments): InternalTypes.ManagerFactory, LatestMap>; - (args?: LatestMapArgumentsRaw): InternalTypes.ManagerFactory, LatestMapRaw>; + (args: LatestMapArguments): InternalTypes.ManagerFactory, LatestMap>; + (args?: LatestMapArgumentsRaw): InternalTypes.ManagerFactory, LatestMapRaw>; } // @beta @sealed -export interface LatestMapItemRemovedClientData { +export interface LatestMapItemRemovedClientData { attendee: Attendee; key: K; metadata: LatestMetadata; } // @beta @sealed -export interface LatestMapItemUpdatedClientData> extends LatestClientData { +export interface LatestMapItemUpdatedClientData> extends LatestClientData { key: K; } // @beta @sealed -export type LatestMapRaw = LatestMap>; +export type LatestMapRaw = LatestMap>; // @beta @sealed -export type LatestMapRawEvents = LatestMapEvents>; +export type LatestMapRawEvents = LatestMapEvents>; // @beta @sealed export interface LatestMetadata { @@ -380,7 +384,7 @@ export const StateFactory: { }; // @beta @sealed -export interface StateMap { +export interface StateMap { clear(): void; delete(key: K): boolean; forEach(callbackfn: (value: DeepReadonly>, key: K, map: StateMap) => void, thisArg?: unknown): void; diff --git a/packages/framework/presence/src/exposedInternalTypes.ts b/packages/framework/presence/src/exposedInternalTypes.ts index 5f345e719aeb..79db290bbdad 100644 --- a/packages/framework/presence/src/exposedInternalTypes.ts +++ b/packages/framework/presence/src/exposedInternalTypes.ts @@ -93,7 +93,7 @@ export namespace InternalTypes { * * @system */ - export interface MapValueState { + export interface MapValueState { rev: number; items: { // Caution: any particular item may or may not exist diff --git a/packages/framework/presence/src/index.ts b/packages/framework/presence/src/index.ts index f278ddb981a8..73b67c6ac8f0 100644 --- a/packages/framework/presence/src/index.ts +++ b/packages/framework/presence/src/index.ts @@ -51,6 +51,7 @@ export { } from "./datastorePresenceManagerFactory.js"; export type { + KeySchemaValidator, LatestMap, LatestMapArguments, LatestMapArgumentsRaw, diff --git a/packages/framework/presence/src/internalTypes.ts b/packages/framework/presence/src/internalTypes.ts index d44bb760600c..b192d2e1e784 100644 --- a/packages/framework/presence/src/internalTypes.ts +++ b/packages/framework/presence/src/internalTypes.ts @@ -79,6 +79,13 @@ export interface ValueManager< > { // State objects should provide value - implement Required> readonly value?: TValueState; + + /** + * Process an update of value for remote attendee. + * @param attendee - The attendee whose value is being updated + * @param received - The revision number received + * @param value - The new value state + */ update(attendee: Attendee, received: number, value: TValueState): PostUpdateAction[]; } diff --git a/packages/framework/presence/src/latestMapValueManager.ts b/packages/framework/presence/src/latestMapValueManager.ts index 23ac6bb2d205..fd47420b0f03 100644 --- a/packages/framework/presence/src/latestMapValueManager.ts +++ b/packages/framework/presence/src/latestMapValueManager.ts @@ -42,19 +42,33 @@ import type { AttendeeId, Attendee, Presence, SpecificAttendee } from "./presenc import { datastoreFromHandle, type StateDatastore } from "./stateDatastore.js"; import { brandIVM } from "./valueManager.js"; +/** + * A validator function that can optionally be provided to do runtime validation + * of the custom key listed in a {@link LatestMap}. + * + * @param unvalidatedKey - The unknown key that should be validated. + * + * @returns True if the key is valid. + * + * @beta + */ +export type KeySchemaValidator = ( + unvalidatedKey: string, +) => unvalidatedKey is Keys; + /** * Collection of validatable optional values in a "map" structure. * * @remarks * Validatable equivalent of {@link InternalTypes.MapValueState}. */ -interface ValidatableMapValueState { +interface ValidatableMapValueState { rev: number; items: { // Caution: any particular item may or may not exist // Typescript does not support absent keys without forcing type to also be undefined. // See https://github.com/microsoft/TypeScript/issues/42810. - [name in Keys]: ValidatableOptionalState; + [name in string]: ValidatableOptionalState; }; } @@ -66,7 +80,7 @@ interface ValidatableMapValueState { */ export interface LatestMapClientData< T, - Keys extends string | number, + Keys extends string, TValueAccessor extends ValueAccessor, SpecificAttendeeId extends AttendeeId = AttendeeId, > { @@ -92,7 +106,7 @@ export interface LatestMapClientData< */ export interface LatestMapItemUpdatedClientData< T, - K extends string | number, + K extends string, TValueAccessor extends ValueAccessor, > extends LatestClientData { /** @@ -107,7 +121,7 @@ export interface LatestMapItemUpdatedClientData< * @sealed * @beta */ -export interface LatestMapItemRemovedClientData { +export interface LatestMapItemRemovedClientData { /** * Associated {@link Attendee}. */ @@ -130,7 +144,7 @@ export interface LatestMapItemRemovedClientData { */ export interface LatestMapEvents< T, - K extends string | number, + K extends string, TRemoteValueAccessor extends ValueAccessor = ProxiedValueAccessor, > { /** @@ -189,7 +203,7 @@ export interface LatestMapEvents< * @sealed * @beta */ -export type LatestMapRawEvents = LatestMapEvents< +export type LatestMapRawEvents = LatestMapEvents< T, K, RawValueAccessor @@ -201,7 +215,7 @@ export type LatestMapRawEvents = LatestMapEvents< * @sealed * @beta */ -export interface StateMap { +export interface StateMap { /** * ${@link StateMap.delete}s all elements in the StateMap. * @remarks This is not yet implemented. @@ -282,7 +296,7 @@ export interface StateMap { // values(): IterableIterator>>; } -class ValueMapImpl implements StateMap { +class ValueMapImpl implements StateMap { private countDefined: number; public constructor( private readonly value: InternalTypes.MapValueState, @@ -293,7 +307,7 @@ class ValueMapImpl implements StateMap { updates: InternalTypes.MapValueState< T, // This should be `K`, but will only work if properties are optional. - string | number + string >, ) => void, ) { @@ -343,7 +357,8 @@ class ValueMapImpl implements StateMap { ): void { for (const [key, item] of objectEntries(this.value.items)) { if (item.value !== undefined) { - callbackfn(asDeeplyReadonlyDeserializedJson(item.value), key, this); + // TODO: try fixing typing of objectEntries to avoid this cast + callbackfn(asDeeplyReadonlyDeserializedJson(item.value), key as unknown as K, this); } } } @@ -370,7 +385,8 @@ class ValueMapImpl implements StateMap { const keys: K[] = []; for (const [key, item] of objectEntries(this.value.items)) { if (item.value !== undefined) { - keys.push(key); + // TODO: try fixing typing of objectEntries to avoid this cast + keys.push(key as unknown as K); } } return keys[Symbol.iterator](); @@ -390,7 +406,7 @@ class ValueMapImpl implements StateMap { */ export interface LatestMap< T, - Keys extends string | number = string | number, + Keys extends string = string, TRemoteAccessor extends ValueAccessor = ProxiedValueAccessor, > { /** @@ -437,16 +453,23 @@ export interface LatestMap< * @sealed * @beta */ -export type LatestMapRaw = LatestMap< +export type LatestMapRaw = LatestMap< T, Keys, RawValueAccessor >; +/** + * Simply returns true for all given string keys. + */ +function anyKeyIsValid(unvalidatedKey: string): unvalidatedKey is Keys { + return true; +} + class LatestMapValueManagerImpl< T, RegistrationKey extends string, - Keys extends string | number = string | number, + Keys extends string = string, > implements LatestMapRaw, LatestMap, @@ -460,11 +483,12 @@ class LatestMapValueManagerImpl< private readonly datastore: StateDatastore< RegistrationKey, InternalTypes.MapValueState, - ValidatableMapValueState + ValidatableMapValueState >, public readonly value: InternalTypes.MapValueState, controlSettings: BroadcastControlSettings | undefined, private readonly validator: StateSchemaValidator | undefined, + private readonly isValidKey: KeySchemaValidator, ) { this.controls = new OptionalBroadcastControl(controlSettings); @@ -513,7 +537,7 @@ class LatestMapValueManagerImpl< } const items = new Map>>(); for (const [key, item] of objectEntries(clientStateMap.items)) { - if (isValueRequiredState(item)) { + if (this.isValidKey(key) && isValueRequiredState(item)) { items.set(key, { value: createValidatedGetter(item, validator), metadata: { revision: item.rev, timestamp: item.timestamp }, @@ -526,7 +550,7 @@ class LatestMapValueManagerImpl< public update( attendee: SpecificAttendee, _received: number, - value: InternalTypes.MapValueState, + value: InternalTypes.MapValueState, ): PostUpdateAction[] { const allKnownStates = this.datastore.knownValues(this.key); const attendeeId: SpecificAttendeeId = attendee.attendeeId; @@ -534,20 +558,18 @@ class LatestMapValueManagerImpl< // New attendee - prepare new attendee state directory { rev: value.rev, - // eslint-disable-next-line @typescript-eslint/consistent-type-assertions -- items entries can't be optional per https://github.com/microsoft/TypeScript/issues/42810; so forced cast - items: {} as ValidatableMapValueState["items"], + items: {}, }); // Accumulate individual update keys - const updatedItemKeys: Keys[] = []; + const updatedKeyItemPairs: [string, InternalTypes.ValueOptionalState][] = []; for (const [key, item] of objectEntries(value.items)) { - // TODO: Key validation needs to be added here. - const validKey = key as Keys; - if (!(key in currentState.items) || currentState.items[validKey].rev < item.rev) { - updatedItemKeys.push(validKey); + const currentItem = currentState.items[key]; + if (currentItem === undefined || currentItem.rev < item.rev) { + updatedKeyItemPairs.push([key, item]); } } - if (updatedItemKeys.length === 0) { + if (updatedKeyItemPairs.length === 0) { return []; } @@ -560,10 +582,14 @@ class LatestMapValueManagerImpl< items: new Map>>(), }; const postUpdateActions: PostUpdateAction[] = []; - for (const key of updatedItemKeys) { - const item = value.items[key]; + for (const [key, item] of updatedKeyItemPairs) { const hadPriorValue = currentState.items[key]?.value; currentState.items[key] = item; + + // Prepare update events, but only for valid keys. + if (!this.isValidKey(key)) { + continue; + } const metadata = { revision: item.rev, timestamp: item.timestamp, @@ -591,7 +617,11 @@ class LatestMapValueManagerImpl< } } this.datastore.update(this.key, attendeeId, currentState); - postUpdateActions.push(() => this.events.emit("remoteUpdated", allUpdates)); + // Only emit remoteUpdated if there are any individual updates, which + // accounts for the case where all updates were for invalid keys. + if (postUpdateActions.length > 0) { + postUpdateActions.push(() => this.events.emit("remoteUpdated", allUpdates)); + } return postUpdateActions; } } @@ -602,7 +632,7 @@ class LatestMapValueManagerImpl< * @input * @beta */ -export interface LatestMapArgumentsRaw { +export interface LatestMapArgumentsRaw { /** * The initial value of the local state. */ @@ -622,13 +652,19 @@ export interface LatestMapArgumentsRaw +export interface LatestMapArguments extends LatestMapArgumentsRaw { /** - * An optional function that will be called at runtime to validate the presence data. A runtime validator is strongly + * An optional function that will be called at runtime to validate the key presence data value. A runtime validator is strongly * recommended. See {@link StateSchemaValidator}. */ validator: StateSchemaValidator; + /** + * An optional function that will be called at runtime to validate the presence + * data key. A runtime validator is strongly recommended when key type is not + * simply `string`. See {@link KeySchemaValidator}. + */ + keyValidator?: KeySchemaValidator; } // #region factory function overloads @@ -648,7 +684,7 @@ export interface LatestMapFactory { * This overload is used when called with {@link LatestMapArguments}. * That is, if a validator function is provided. */ - ( + ( args: LatestMapArguments, ): InternalTypes.ManagerFactory< RegistrationKey, @@ -663,7 +699,7 @@ export interface LatestMapFactory { * This overload is used when called with {@link LatestMapArgumentsRaw}. * That is, if a validator function is _not_ provided. */ - ( + ( args?: LatestMapArgumentsRaw, ): InternalTypes.ManagerFactory< RegistrationKey, @@ -679,7 +715,7 @@ export interface LatestMapFactory { */ export const latestMap: LatestMapFactory = < T, - Keys extends string | number = string | number, + Keys extends string = string, RegistrationKey extends string = string, >( args?: Partial>, @@ -691,20 +727,23 @@ export const latestMap: LatestMapFactory = < const settings = args?.settings; const initialValues = args?.local; const validator = args?.validator; + const isKeyValid = args?.keyValidator ?? anyKeyIsValid; const timestamp = Date.now(); const value: InternalTypes.MapValueState< T, // This should be `Keys`, but will only work if properties are optional. - string | number + string > = { rev: 0, items: {} }; // LatestMapRaw takes ownership of values within initialValues. if (initialValues !== undefined) { - for (const key of objectKeys(initialValues)) { - value.items[key] = { + for (const [key, item] of objectEntries(initialValues)) { + // TODO: try fixing typing of objectEntries to avoid this cast + const assumedValidKey = key as unknown as Keys; + value.items[assumedValidKey] = { rev: 0, timestamp, - value: toOpaqueJson(initialValues[key]), + value: toOpaqueJson(item), }; } } @@ -730,6 +769,7 @@ export const latestMap: LatestMapFactory = < value, settings, validator, + isKeyValid, ), ), }); diff --git a/packages/framework/presence/src/stateDatastore.ts b/packages/framework/presence/src/stateDatastore.ts index 0be44f1ddd6e..68a8e0c7cb31 100644 --- a/packages/framework/presence/src/stateDatastore.ts +++ b/packages/framework/presence/src/stateDatastore.ts @@ -47,14 +47,14 @@ export interface LocalStateUpdateOptions { */ export interface StateDatastore< TKey extends string, - TUpdateValue extends InternalTypes.ValueDirectoryOrState, + TLocalUpdateValue extends InternalTypes.ValueDirectoryOrState, TStoredValue extends - ValidatableValueDirectoryOrState = ValidatableValueStructure, + ValidatableValueDirectoryOrState = ValidatableValueStructure, > { readonly presence: Presence; localUpdate( key: TKey, - value: TUpdateValue & { + value: TLocalUpdateValue & { ignoreUnmonitored?: true; }, options: LocalStateUpdateOptions, diff --git a/packages/framework/presence/src/test/latestMapValueManager.spec.ts b/packages/framework/presence/src/test/latestMapValueManager.spec.ts index d59d40e729fd..9ca02d8da5c6 100644 --- a/packages/framework/presence/src/test/latestMapValueManager.spec.ts +++ b/packages/framework/presence/src/test/latestMapValueManager.spec.ts @@ -51,7 +51,7 @@ describe("Presence", () => { x: number; y: number; }, - string + `key${number}` > { const presence = createPresenceManager(new MockEphemeralRuntime()); const workspace = presence.states.getWorkspace(testWorkspaceName, { @@ -216,7 +216,7 @@ export function checkCompiles(): void { key, value, }: Pick< - LatestMapItemUpdatedClientData>, + LatestMapItemUpdatedClientData>, "attendee" | "key" | "value" >): void { console.log(attendee.attendeeId, key, value); diff --git a/packages/framework/presence/src/test/schemaValidation/valueManagers.spec.ts b/packages/framework/presence/src/test/schemaValidation/valueManagers.spec.ts index acf6910ba413..7ee86c3a0ad6 100644 --- a/packages/framework/presence/src/test/schemaValidation/valueManagers.spec.ts +++ b/packages/framework/presence/src/test/schemaValidation/valueManagers.spec.ts @@ -9,7 +9,8 @@ import type { InboundExtensionMessage } from "@fluidframework/container-runtime- import type { OpaqueJsonDeserialized } from "@fluidframework/core-interfaces/internal/exposedUtilityTypes"; import { EventAndErrorTrackingLogger } from "@fluidframework/test-utils/internal"; import { describe, it, after, afterEach, before, beforeEach } from "mocha"; -import { useFakeTimers, type SinonFakeTimers } from "sinon"; +import type { SinonFakeTimers } from "sinon"; +import { useFakeTimers } from "sinon"; import { toOpaqueJson } from "../../internalUtils.js"; import type { Presence } from "../../presence.js"; @@ -71,6 +72,11 @@ const latestMapUpdate = { "timestamp": 0, "value": toOpaqueJson({ x: 2, y: 2, z: 2 }), }, + "invalidKey": { + "rev": 1, + "timestamp": 0, + "value": toOpaqueJson({ x: -1, y: -1, z: -1 }), + }, }, }, }, @@ -553,13 +559,25 @@ describe("Presence", () => { }); describe("LatestMap with validation", () => { - let latestMap: LatestMap; + let latestMap: LatestMap; + let keyValidatorCallExpectedCount: number; + + function keyValidatorFunction(key: string): key is `key${string}` { + assert( + keyValidatorCallExpectedCount > 0, + "keyValidatorFunction should not be called at this time", + ); + keyValidatorCallExpectedCount--; + return key.startsWith("key"); + } /** * This beforeEach sets up the presence workspace itself and gets a * reference to it. */ beforeEach(() => { + keyValidatorCallExpectedCount = 0; + // workspace setup's initialization signal runtime.signalsExpected.push([ { @@ -605,6 +623,7 @@ describe("Presence", () => { latestMap: StateFactory.latestMap({ local: { "key1": { x: 0, y: 0, z: 0 }, "key2": { x: 0, y: 0, z: 0 } }, validator: point3DValidatorFunction, + keyValidator: keyValidatorFunction, settings: { allowableUpdateLatencyMs: 0 }, }), }); @@ -613,6 +632,7 @@ describe("Presence", () => { }); function sendInvalidData(key: string = "key1"): void { + keyValidatorCallExpectedCount = 1; const timestamp = clock.now - 15; processSignal( [], @@ -628,9 +648,10 @@ describe("Presence", () => { }), false, ); + assert.equal(keyValidatorCallExpectedCount, 0); } - describe("validator is not called", () => { + describe("no validator is not called", () => { beforeEach(() => { validatorCallExpected = false; }); @@ -686,18 +707,65 @@ describe("Presence", () => { // Verify assert.equal(point3DValidatorFunction.callCount, 0); }); + }); + + it("update with invalid key invokes key validator per key and does not raise events", () => { + // Setup + for (const event of [ + "remoteUpdated", + "remoteItemUpdated", + "remoteItemRemoved", + ] as const) { + afterCleanUp.push( + latestMap.events.on(event, () => { + assert.fail(`${event} event should not be raised for invalid key`); + }), + ); + } + keyValidatorCallExpectedCount = 1; + // Act + sendInvalidData("invalidKey"); + // Verify + assert.equal(keyValidatorCallExpectedCount, 0); // all expected calls were made + assert.equal(point3DValidatorFunction.callCount, 0); + }); + + /** + * Sets validator call expectations and gets remote value data. + * @returns `latestMap.getRemote(remoteAttendee)` + * @remarks + * `validatorCallExpected` will be `false` and `keyValidatorCallExpectedCount` + * will be `0` after the call. + */ + function getRemoteValue(): ReturnType { + validatorCallExpected = false; + keyValidatorCallExpectedCount = 3; + const remoteData = latestMap.getRemote(remoteAttendee); + assert.equal(keyValidatorCallExpectedCount, 0); + return remoteData; + } - it("when calling .get() on remote map", () => { - const remoteData = latestMap.getRemote(remoteAttendee); + it(".getRemote call invokes key validator per key but not value validator", () => { + // Setup and Act + getRemoteValue(); + // Verify + assert.equal(keyValidatorCallExpectedCount, 0); // all expected calls were made + assert.equal(point3DValidatorFunction.callCount, 0); + }); + + describe("remote value map", () => { + it(".get() does not call any validator", () => { + // Setup + const remoteData = getRemoteValue(); // Act remoteData.get("key1"); // Verify assert.equal(point3DValidatorFunction.callCount, 0); }); - it("when accessing keys only in .forEach()", () => { + it(".forEach() iteration does not call any validator", () => { // Setup - const remoteData = latestMap.getRemote(remoteAttendee); + const remoteData = getRemoteValue(); let counter = 0; const expectedValues = [ ["key1", { x: 1, y: 1, z: 1 }], @@ -718,198 +786,215 @@ describe("Presence", () => { "counter should match expected values length", ); }); - }); - describe("validator is called", () => { - beforeEach(() => { - validatorCallExpected = "once"; - }); + describe("value validator is called", () => { + for (const { desc, setup, expectedValue } of [ + { + desc: "for valid value", + setup: () => {}, + expectedValue: { x: 1, y: 1, z: 1 }, + }, + { + desc: "for invalid value", + setup: sendInvalidData, + expectedValue: undefined, + }, + ] as const) { + it(`once when key.value() is called ${desc}`, () => { + // Setup + setup(); + const remoteData = getRemoteValue(); + validatorCallExpected = "once"; + // Act + runValidatorTest({ + getRemoteValue: () => remoteData.get("key1")?.value(), + expectedCallCount: 1, + expectedValue, + validatorFunction: point3DValidatorFunction, + }); + }); - for (const { desc, setup, expectedValue } of [ - { - desc: "for valid value", - setup: () => {}, - expectedValue: { x: 1, y: 1, z: 1 }, - }, - { - desc: "for invalid value", - setup: sendInvalidData, - expectedValue: undefined, - }, - ] as const) { - it(`once when key.value() is called ${desc}`, () => { - setup(); - const remoteData = latestMap.getRemote(remoteAttendee); - runValidatorTest({ - getRemoteValue: () => remoteData.get("key1")?.value(), - expectedCallCount: 1, - expectedValue, - validatorFunction: point3DValidatorFunction, + it(`only once for multiple key.value() calls in series and .value() always returned same result ${desc}`, () => { + // Setup + setup(); + const remoteData = getRemoteValue(); + validatorCallExpected = "once"; + // Act + runMultipleCallsTest({ + getRemoteValue: () => remoteData.get("key1")?.value(), + expectedValue, + validatorFunction: point3DValidatorFunction, + }); }); - }); - it(`only once for multiple key.value() calls in series and .value() always returned same result ${desc}`, () => { - setup(); - const remoteData = latestMap.getRemote(remoteAttendee); - runMultipleCallsTest({ - getRemoteValue: () => remoteData.get("key1")?.value(), - expectedValue, - validatorFunction: point3DValidatorFunction, + it(`exactly once for each value's .value() calls in .forEach() and .value() always returns same result ${desc}`, () => { + // Setup + setup(); + const remoteData = getRemoteValue(); + let counter = 0; + const expectedValues = [ + // only key1 value might be altered by the setup function + ["key1", expectedValue], + // key2 value will always be the valid + ["key2", { x: 2, y: 2, z: 2 }], + ]; + // Act + // eslint-disable-next-line unicorn/no-array-for-each -- forEach is being tested here + remoteData.forEach((value, key) => { + assert.equal(key, expectedValues[counter][0]); + // reset call expectations/count for each iteration + validatorCallExpected = "once"; + point3DValidatorFunction.callCount = 0; + + // Act - first call + const valueData = value?.value(); + // Verify + assert.equal( + point3DValidatorFunction.callCount, + 1, + "validator was not called", + ); + // double check value + assert.deepEqual( + valueData, + expectedValues[counter][1], + `value at key "${key}" is wrong`, + ); + + // Act - second call + // Access value a second time; should not affect validator call count + const valueDataRedux = value?.value(); + // Verify + assert.equal( + point3DValidatorFunction.callCount, + 1, + "validator was called a second time", + ); + assert.strictEqual( + valueDataRedux, + valueData, + `value at key "${key}" is wrong`, + ); + + counter++; + }); + // Make sure forEach iterated through all keys + assert.equal( + counter, + expectedValues.length, + "counter should match expected values length", + ); }); - }); + } + + for (const { desc, setup, expectedInitialValue, newData, expectedValue } of [ + { + desc: "from valid to different valid value", + setup: () => {}, + expectedInitialValue: { x: 1, y: 1, z: 1 }, + newData: { x: 4, y: 4, z: 4 }, + expectedValue: { x: 4, y: 4, z: 4 }, + }, + { + desc: "from valid to same valid value", + setup: () => {}, + expectedInitialValue: { x: 1, y: 1, z: 1 }, + newData: { x: 1, y: 1, z: 1 }, + expectedValue: { x: 1, y: 1, z: 1 }, + }, + { + desc: "from valid to invalid value", + setup: () => {}, + expectedInitialValue: { x: 1, y: 1, z: 1 }, + newData: "invalid", + expectedValue: undefined, + }, + { + desc: "from invalid to valid value", + setup: sendInvalidData, + expectedInitialValue: undefined, + newData: { x: 4, y: 4, z: 4 }, + expectedValue: { x: 4, y: 4, z: 4 }, + }, + { + desc: "from invalid to invalid value", + setup: sendInvalidData, + expectedInitialValue: undefined, + newData: "invalid", + expectedValue: undefined, + }, + ] as const) { + it(`during .value() call after remote key data has changed ${desc}`, () => { + // Setup + setup(); - it(`exactly once for each value's .value() calls in .forEach() and .value() always returns same result ${desc}`, () => { - setup(); - const remoteData = latestMap.getRemote(remoteAttendee); - let counter = 0; - const expectedValues = [ - // only key1 value might be altered by the setup function - ["key1", expectedValue], - // key2 value will always be the valid - ["key2", { x: 2, y: 2, z: 2 }], - ]; - // eslint-disable-next-line unicorn/no-array-for-each -- forEach is being tested here - remoteData.forEach((value, key) => { - assert.equal(key, expectedValues[counter][0]); - // reset call expectations/count for each iteration + const originalMap = getRemoteValue(); validatorCallExpected = "once"; + // Get the remote data and read it, expect that the validator is called once. + const key1Value = originalMap.get("key1")?.value(); + assert.equal(point3DValidatorFunction.callCount, 1, "first call count is wrong"); + assert.deepEqual(key1Value, expectedInitialValue); + // Reset call count for next verification point3DValidatorFunction.callCount = 0; - // Act - first call - const valueData = value?.value(); - // Verify - assert.equal(point3DValidatorFunction.callCount, 1, "validator was not called"); - // double check value - assert.deepEqual( - valueData, - expectedValues[counter][1], - `value at key "${key}" is wrong`, + // Process updated key data from remote client + keyValidatorCallExpectedCount = 1; + const timestamp = clock.now - 15; + processSignal( + [], + datastoreUpdateSignal(timestamp, "latestMap", { + "rev": 3, + "items": { + "key1": { + "rev": 3, + timestamp, + "value": toOpaqueJson(newData), + }, + }, + }), + false, ); + assert.equal(keyValidatorCallExpectedCount, 0); - // Act - second call - // Access value a second time; should not affect validator call count - const valueDataRedux = value?.value(); - // Verify + // Verify no call yet assert.equal( point3DValidatorFunction.callCount, - 1, - "validator was called a second time", + 0, + "call count after update is wrong", ); - assert.strictEqual(valueDataRedux, valueData, `value at key "${key}" is wrong`); - counter++; - }); - // Make sure forEach iterated through all keys - assert.equal( - counter, - expectedValues.length, - "counter should match expected values length", - ); - }); - } + // Reading the remote value should cause the validator to be + // called since the data has been changed. + const updatedMap = getRemoteValue(); - for (const { desc, setup, expectedInitialValue, newData, expectedValue } of [ - { - desc: "from valid to different valid value", - setup: () => {}, - expectedInitialValue: { x: 1, y: 1, z: 1 }, - newData: { x: 4, y: 4, z: 4 }, - expectedValue: { x: 4, y: 4, z: 4 }, - }, - { - desc: "from valid to same valid value", - setup: () => {}, - expectedInitialValue: { x: 1, y: 1, z: 1 }, - newData: { x: 1, y: 1, z: 1 }, - expectedValue: { x: 1, y: 1, z: 1 }, - }, - { - desc: "from valid to invalid value", - setup: () => {}, - expectedInitialValue: { x: 1, y: 1, z: 1 }, - newData: "invalid", - expectedValue: undefined, - }, - { - desc: "from invalid to valid value", - setup: sendInvalidData, - expectedInitialValue: undefined, - newData: { x: 4, y: 4, z: 4 }, - expectedValue: { x: 4, y: 4, z: 4 }, - }, - { - desc: "from invalid to invalid value", - setup: sendInvalidData, - expectedInitialValue: undefined, - newData: "invalid", - expectedValue: undefined, - }, - ] as const) { - it(`during .value() call after remote key data has changed ${desc}`, () => { - // Setup - setup(); - - const originalMap = latestMap.getRemote(remoteAttendee); - // Get the remote data and read it, expect that the validator is called once. - const key1Value = originalMap.get("key1")?.value(); - assert.equal(point3DValidatorFunction.callCount, 1, "first call count is wrong"); - assert.deepEqual(key1Value, expectedInitialValue); - // Reset call count for next verification - point3DValidatorFunction.callCount = 0; - - // Process updated key data from remote client - const timestamp = clock.now - 15; - processSignal( - [], - datastoreUpdateSignal(timestamp, "latestMap", { - "rev": 3, - "items": { - "key1": { - "rev": 3, - timestamp, - "value": toOpaqueJson(newData), - }, - }, - }), - false, - ); - - // Verify no call yet - assert.equal( - point3DValidatorFunction.callCount, - 0, - "call count after update is wrong", - ); - - // Reading the remote value should cause the validator to be - // called since the data has been changed. - const updatedMap = latestMap.getRemote(remoteAttendee); - - validatorCallExpected = "once"; + validatorCallExpected = "once"; - // Act - read updated key value - const updatedKey1Value = updatedMap.get("key1")?.value(); + // Act - read updated key value + const updatedKey1Value = updatedMap.get("key1")?.value(); - // Verify - assert.equal( - point3DValidatorFunction.callCount, - 1, - "validator should be called twice", - ); - assert.deepEqual( - updatedKey1Value, - expectedValue, - "updated remote key value is wrong", - ); - }); - } + // Verify + assert.equal( + point3DValidatorFunction.callCount, + 1, + "validator should be called twice", + ); + assert.deepEqual( + updatedKey1Value, + expectedValue, + "updated remote key value is wrong", + ); + }); + } + }); }); // Not this block is after "is called" as it is only valid when those tests are passing. - describe("validator is not called", () => { + describe("no validator is not called", () => { it("during .value() call for unchanged keys", () => { // Setup + keyValidatorCallExpectedCount = 3; const originalMap = latestMap.getRemote(remoteAttendee); + assert.equal(keyValidatorCallExpectedCount, 0); validatorCallExpected = "once"; // Read key1 value - should call validator once const key1Value = originalMap.get("key1")?.value(); @@ -921,6 +1006,7 @@ describe("Presence", () => { assert.deepEqual(key1Value, { x: 1, y: 1, z: 1 }); // Update key2 (different key) with new data, keeping key1 unchanged + keyValidatorCallExpectedCount = 1; const timestamp = clock.now - 15; processSignal( [], @@ -936,12 +1022,15 @@ describe("Presence", () => { }), false, ); + assert.equal(keyValidatorCallExpectedCount, 0); - const updatedMap = latestMap.getRemote(remoteAttendee); + const updatedMap = getRemoteValue(); const key1Redux = updatedMap.get("key1"); assert.ok(key1Redux); - // Verify + assert.equal(keyValidatorCallExpectedCount, 0); + assert.equal(point3DValidatorFunction.callCount, 1); + // Act & Verify // Read key1 value (again) but from updated map - should NOT call validator again since key1 data hasn't changed const key1ValueRedux = key1Redux.value(); assert.equal( diff --git a/packages/service-clients/end-to-end-tests/azure-client/src/test/multiprocess/childClient.tool.ts b/packages/service-clients/end-to-end-tests/azure-client/src/test/multiprocess/childClient.tool.ts index 87cbbe2a6c2f..53cbccf2f04a 100644 --- a/packages/service-clients/end-to-end-tests/azure-client/src/test/multiprocess/childClient.tool.ts +++ b/packages/service-clients/end-to-end-tests/azure-client/src/test/multiprocess/childClient.tool.ts @@ -334,16 +334,15 @@ class MessageHandler { if (latestMap && !workspace.states.latestMap) { workspace.add( "latestMap", - StateFactory.latestMap<{ value: Record }, string>({ + StateFactory.latestMap<{ value: Record }>({ local: {}, }), ); // Cast required due to optional keys in WorkspaceSchema // TODO: AB#47518 - const latestMapState = workspace.states.latestMap as LatestMapRaw< - { value: Record }, - string - >; + const latestMapState = workspace.states.latestMap as LatestMapRaw<{ + value: Record; + }>; latestMapState.events.on("remoteUpdated", (update) => { for (const [key, valueWithMetadata] of update.items) { this.send({ @@ -614,7 +613,7 @@ class MessageHandler { // Cast required due to optional keys in WorkspaceSchema // TODO: AB#47518 const latestMapState = workspace.states.latestMap as - | LatestMapRaw<{ value: Record }, string> + | LatestMapRaw<{ value: Record }> | undefined; if (!latestMapState) { this.send({ @@ -692,7 +691,7 @@ class MessageHandler { // Cast required due to optional keys in WorkspaceSchema // TODO: AB#47518 const latestMapState = workspace.states.latestMap as - | LatestMapRaw<{ value: Record }, string> + | LatestMapRaw<{ value: Record }> | undefined; if (!latestMapState) { this.send({