Skip to content

Commit bc4484e

Browse files
CraigMacomberanthony-murphy-agent
authored andcommitted
Freeze AllowedTypes (microsoft#25767)
## Description The annotated allowed type work reintroduced the possibility of modifying an allowed type array after derived data was computed. This change closes that gap and adds some tests.
1 parent 0b824d0 commit bc4484e

File tree

2 files changed

+46
-13
lines changed

2 files changed

+46
-13
lines changed

packages/dds/tree/src/simple-tree/core/allowedTypes.ts

Lines changed: 10 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
*/
55

66
import { UsageError } from "@fluidframework/telemetry-utils/internal";
7-
import { assert, Lazy } from "@fluidframework/core-utils/internal";
7+
import { Lazy } from "@fluidframework/core-utils/internal";
88

99
import {
1010
type ErasedBaseType,
@@ -332,15 +332,16 @@ export class AnnotatedAllowedTypesInternal<
332332
public static create<const T extends readonly AnnotatedAllowedType[]>(
333333
types: T,
334334
metadata: AllowedTypesMetadata = {},
335-
): AnnotatedAllowedTypesInternal<T> & AllowedTypesFull<T> {
335+
): AnnotatedAllowedTypesInternal<Readonly<T>> & AllowedTypesFull<Readonly<T>> {
336336
const result = new AnnotatedAllowedTypesInternal(types, metadata);
337337
return result as typeof result & UnannotateAllowedTypesList<T>;
338338
}
339339

340340
public static createUnannotated<const T extends AllowedTypes>(
341341
types: T,
342342
metadata: AllowedTypesMetadata = {},
343-
): AnnotatedAllowedTypesInternal & T {
343+
): AnnotatedAllowedTypesInternal & Readonly<T> {
344+
Object.freeze(types);
344345
const annotatedTypes: AnnotatedAllowedType[] = types.map(normalizeToAnnotatedAllowedType);
345346
const result = AnnotatedAllowedTypesInternal.create(annotatedTypes, metadata);
346347
return result as typeof result & T;
@@ -349,6 +350,7 @@ export class AnnotatedAllowedTypesInternal<
349350
public static createMixed<
350351
const T extends readonly (AnnotatedAllowedType | LazyItem<TreeNodeSchema>)[],
351352
>(types: T, metadata: AllowedTypesMetadata = {}): AllowedTypesFullFromMixed<T> {
353+
Object.freeze(types);
352354
const annotatedTypes: AnnotatedAllowedType[] = types.map(normalizeToAnnotatedAllowedType);
353355
const result = AnnotatedAllowedTypesInternal.create(annotatedTypes, metadata);
354356
return result as AllowedTypesFullFromMixed<T>;
@@ -524,14 +526,9 @@ export function normalizeAllowedTypesInternal(
524526
// Adding this cache improved the performance of the "large recursive union" test (which mostly just constructs a TreeConfiguration) by ~5 times.
525527
// This cache is strictly a performance optimization: it is not required for correctness.
526528
return getOrCreate(cachedNormalize, type, () => {
527-
// Due to more specific internal type, the above does not narrow sufficiently, so more narrowing is needed.
528-
// It is possible this will give a false error if a TreeNodeSchema which matches this check is used.
529-
assert(
530-
!("types" in type && "metadata" in type),
531-
0xc7d /* invalid AnnotatedAllowedTypes */,
532-
);
533-
534-
const annotatedTypes: AnnotatedAllowedType[] = (isReadonlyArray(type) ? type : [type]).map(
529+
const inputArray = isReadonlyArray(type) ? type : [type];
530+
Object.freeze(inputArray);
531+
const annotatedTypes: AnnotatedAllowedType[] = inputArray.map(
535532
normalizeToAnnotatedAllowedType,
536533
);
537534

@@ -637,8 +634,8 @@ export type TreeNodeFromImplicitAllowedTypes<
637634
* This type exists only to be linked from documentation to provide a single linkable place to document some details of
638635
* "Input" types and how they handle schema.
639636
*
640-
* When a schema is used to describe data which is an input into an API, the API is {@link https://en.wikipedia.org/wiki/Covariance_and_contravariance_(computer_science | contravariant}) over the schema.
641-
* (See also, {@link https://www.typescriptlang.org/docs/handbook/2/generics.html#variance-annotations | TypeScript Variance Annotations}).
637+
* When a schema is used to describe data which is an input into an API, the API is {@link https://en.wikipedia.org/wiki/Type_variance | contravariant}) over the schema.
638+
* (See also {@link https://www.typescriptlang.org/docs/handbook/2/generics.html#variance-annotations | TypeScript Variance Annotations}).
642639
*
643640
* Since these schema are expressed using TypeScript types, it is possible for the user of the API to provide non-exact values of these types which has implications that depended on the variance.
644641
*

packages/dds/tree/src/test/simple-tree/core/allowedTypes.spec.ts

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -568,4 +568,40 @@ describe("allowedTypes", () => {
568568
type X7 = InsertableTreeFieldFromImplicitField<typeof Canvas.info.stuff>;
569569
});
570570
});
571+
572+
// If derived data is computed based on an allowed type array, then modifications to that array would cause the derived data to become invalid.
573+
// As there is no invalidation mechanism, this would lead to incorrect behavior, and is prevented by freezing the arrays when the derived data is computed.
574+
// These are glass box tests: they are testing that the cases where the code currently derives data from the arrays results in the arrays being frozen.
575+
// Future changes could be made to delay both the freezing and the computation of the derived data:
576+
// if such changes are made these tests will need to be updated.
577+
// If done, these tests may need updates to instead test that modifying the arrays does not expose incorrect derived data.
578+
describe("freezes inputs producing derived data", () => {
579+
it("AnnotatedAllowedTypesInternal.create", () => {
580+
const input = [{ type: stringSchema, metadata: {} }];
581+
const result = AnnotatedAllowedTypesInternal.create(input);
582+
assert(Object.isFrozen(input));
583+
assert.throws(() => {
584+
// @ts-expect-error Array should be readonly, so this error is good.
585+
result.push(stringSchema);
586+
}, "TypeError: result.push is not a function");
587+
});
588+
589+
it("normalizeAllowedTypes", () => {
590+
const input = [stringSchema];
591+
const _ = normalizeAllowedTypes(input);
592+
assert(Object.isFrozen(input));
593+
});
594+
595+
it("AnnotatedAllowedTypesInternal.createUnannotated", () => {
596+
const input = [stringSchema];
597+
const _ = AnnotatedAllowedTypesInternal.createUnannotated(input);
598+
assert(Object.isFrozen(input));
599+
});
600+
601+
it("AnnotatedAllowedTypesInternal.createMixed", () => {
602+
const input = [stringSchema];
603+
const _ = AnnotatedAllowedTypesInternal.createMixed(input);
604+
assert(Object.isFrozen(input));
605+
});
606+
});
571607
});

0 commit comments

Comments
 (0)