-
Notifications
You must be signed in to change notification settings - Fork 556
feat(tree): staged allowed types #24631
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
060acdd
3d4d85e
fed6358
a8e69d5
5724351
984b829
4ecd8b7
1be4f30
90737e6
bcecd66
9a2036a
41f76db
e1acdd0
56f25e2
4d7fbfb
dc6e729
8858102
9c51607
cf3984e
7b0eeb7
e272d83
f42b75e
fb1704f
ce39396
cc76230
66021d5
c35b163
5a42cef
d792096
0d979aa
866ef86
f399aef
df586bf
574fd6c
56c8a8f
2ebe5fb
c2e0792
53b58f5
364511d
3065dd5
4b28d19
aaf5b11
b68c3c8
c877a3b
a23714e
1780379
26184b0
4dc0c5a
032204f
5e21db1
0f7bd8f
f3d1811
4b18ccf
d2493f3
72e7b22
aa6c1f1
4faed42
9e8728c
1a34af1
e831ff7
8792f09
16d2396
9986975
58201d3
426ef3d
f732d2f
630b151
18e5318
4c5a5b4
3d12712
616dba6
593e70b
4addf0d
59e0919
1cb7ca2
5c4e3cc
0c0d1c2
9a7102d
79065bb
95945f9
2ac5480
058687e
508e6b4
68f1350
12b28df
3e27e94
ed00bd7
1e122cb
8107622
5b6b110
d6cddef
55420e8
29fb81f
dd106d7
6fd8393
b4e7de5
2519edf
5a2e184
2ab61e9
4d07d9e
8100000
979c3e6
1f246f1
2c5dee9
e39d8de
e705e79
6ea6283
6fe2be2
e690541
fc7a72d
191eca5
cbb386a
69cfd02
6c6b547
3fd37e9
0190b8b
a99262c
57d47c6
cba6464
2619a19
750c2bf
aed13b9
4a142f2
2336f8b
51d728e
6934dd0
35bb59c
0dbb76d
c724bca
3d67469
8d4efda
926189d
e599596
4f67663
2940a9e
89fc5f7
6df6775
fd11892
bda5104
fa38d4d
4856e0d
caa6661
519cb84
9dc2628
839d11e
7a6d416
b2c3b42
6cb2a1c
f6952ea
781ed52
69f097e
96f58a1
0be9310
7765358
b67c203
0eeea9f
f8b3d08
139e2d8
1e4d3b0
2dcce56
5daef37
334979c
9cb4647
3cdfca2
a3e13e8
211ea73
13583af
d465cf4
4627fee
6a4a2dc
20f8710
0370c15
876ab10
c9ca957
16209ae
cf1c35f
5af0ba2
bf141b8
1c828f8
369fe1b
e86e8c2
1d512f5
fc30fd9
a9f779a
6a59513
e5d9d3f
dac094e
320c180
f1cfde5
6102ab4
2e401fa
e1e7b38
81144bc
8e348e3
f4344a1
03565b6
8da4483
458f1e1
d7cbd8e
2dc21e3
143cfc6
e5b6742
1cc1426
85aad37
ff2c612
208d885
79b9ba7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,94 @@ | ||
--- | ||
"fluid-framework": minor | ||
"@fluidframework/tree": minor | ||
"__section": feature | ||
--- | ||
Adds staged allowed types to SchemaFactoryAlpha | ||
|
||
This adds the `staged` API to [`SchemaFactoryAlpha`](https://fluidframework.com/docs/api/fluid-framework/schemafactoryalpha-class). | ||
Staged allowed types can be used for schema evolution to add members to an [`AllowedTypes`](https://fluidframework.com/docs/api/fluid-framework/allowedtypes-typealias) while supporting cross version collaboration. | ||
|
||
Staged allowed types are [allowed types](https://fluidframework.com/docs/api/fluid-framework/allowedtypes-typealias) that can be upgraded by [schema upgrades}(https://fluidframework.com/docs/api/fluid-framework/treeview-interface#upgradeschema-methodsignature). | ||
Before being upgraded, any attempt to insert or move a node to a location which requires its type to be upgraded to be valid will throw an error. | ||
|
||
To enable this feature, [schema validation](https://fluidframework.com/docs/api/fluid-framework/treeviewconfiguration-class#enableschemavalidation-property) is now performed by default when editing the tree. | ||
|
||
To add a new member to an `AllowedTypes`, add the type wrapped by `staged`. | ||
For example, migrating an array which previously supported only numbers to support both numbers and strings would start by deploying a version of the app using `staged`: | ||
```typescript | ||
class TestArray extends schemaFactoryAlpha.arrayAlpha("TestArray", [SchemaFactoryAlpha.number, SchemaFactoryAlpha.staged(SchemaFactoryAlpha.string)]) {} | ||
``` | ||
|
||
Once enough clients have this code update, it is safe to allow writing strings to the array. | ||
To allow writing strings to the array, a code change must be made to remove the staged annotation: | ||
```typescript | ||
class TestArray extends schemaFactoryAlpha.arrayAlpha("TestArray", [schemaFactoryAlpha.number, schemaFactoryAlpha.string]) {} | ||
``` | ||
|
||
Then when opening old documents [upgradeSchema](https://fluidframework.com/docs/api/fluid-framework/treeview-interface#upgradeschema-methodsignature) is used to upgrade the stored schema: | ||
```typescript | ||
view.upgradeSchema() | ||
``` | ||
|
||
In the future, SharedTree may add an API that allows staged allowed types to be upgraded via a runtime schema upgrade so that the type can be more easily deployed using a configuration flag change rather than a code change. | ||
|
||
Below is a full example of how the schema migration process works. This can also be found in our [tests](https://github.com/jenn-le/FluidFramework/blob/main/packages/dds/tree/src/test/simple-tree/api/stagedSchemaUpgrade.spec.ts). | ||
|
||
```typescript | ||
// schema A: only number allowed | ||
const schemaA = factory.optional([SchemaFactoryAlpha.number]); | ||
|
||
// schema B: number or string (string is staged) | ||
const schemaB = factory.optional([ | ||
SchemaFactoryAlpha.number, | ||
factory.staged(SchemaFactoryAlpha.string), | ||
]); | ||
|
||
// schema C: number or string, both fully allowed | ||
const schemaC = factory.optional([SchemaFactoryAlpha.number, SchemaFactoryAlpha.string]); | ||
|
||
const provider = new TestTreeProviderLite(3); | ||
|
||
// initialize with schema A | ||
const configA = new TreeViewConfiguration({ | ||
schema: schemaA, | ||
}); | ||
const viewA = provider.trees[0].viewWith(configA); | ||
viewA.initialize(5); | ||
provider.synchronizeMessages(); | ||
|
||
assert.deepEqual(viewA.root, 5); | ||
|
||
// view second tree with schema B | ||
const configB = new TreeViewConfiguration({ | ||
schema: schemaB, | ||
}); | ||
const viewB = provider.trees[1].viewWith(configB); | ||
// check that we can read the tree | ||
assert.deepEqual(viewB.root, 5); | ||
// upgrade to schema B | ||
viewB.upgradeSchema(); | ||
provider.synchronizeMessages(); | ||
|
||
// check view A can read the document | ||
assert.deepEqual(viewA.root, 5); | ||
// check view B cannot write strings to the root | ||
assert.throws(() => { | ||
viewB.root = "test"; | ||
}); | ||
|
||
// view third tree with schema C | ||
const configC = new TreeViewConfiguration({ | ||
schema: schemaC, | ||
}); | ||
const viewC = provider.trees[2].viewWith(configC); | ||
// upgrade to schema C and change the root to a string | ||
viewC.upgradeSchema(); | ||
viewC.root = "test"; | ||
provider.synchronizeMessages(); | ||
|
||
// view A is now incompatible with the stored schema | ||
assert.throws(viewA.canView, false); | ||
assert.deepEqual(viewB.root, "test"); | ||
assert.deepEqual(viewC.root, "test"); | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -217,7 +217,7 @@ export class ObjectForest implements IEditableForest, WithBreakable { | |
if (this.forest.additionalAsserts) { | ||
// Schema validation: | ||
// When doing "additionalAsserts", validate the content against the schema after every batch of edits. | ||
this.forest.checkSchema(); | ||
// this.forest.checkSchema(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I noticed this was introduced in this change: #24658 Since this feature requires schema validation to always be done, does it make sense to revert this change or is there a different way we want to disable it? I commented it out here because it interferes with initialization. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we want to keep this enabled. It is only opt in as part of https://fluidframework.com/docs/api/fluid-framework/#foresttypeexpensivedebug-variable and is there to mainly detect document corruption. No reason to disable it. Its perfectly possible that document corruption can still happen, from older version, via initialize, or from edits other than insert, bugs in insert etc. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Making a note here that we'll want to change schema validation in the initialization case to take view schema into account for staged allowed types and allowUnknownOptionalFields. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. View schema is not relevant here. What is being checked is that the content stored in the document complies with the stored schema. This is required to be true regardless of what the view schema is doing: any violation of it is document corruption. |
||
} | ||
} | ||
public destroy(detachedField: FieldKey, count: number): void { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,11 +3,10 @@ | |
* Licensed under the MIT License. | ||
*/ | ||
|
||
import { assert, unreachableCase } from "@fluidframework/core-utils/internal"; | ||
import { unreachableCase } from "@fluidframework/core-utils/internal"; | ||
|
||
import type { TreeStoredSchema } from "../../core/index.js"; | ||
import { | ||
allowsRepoSuperset, | ||
FieldKinds, | ||
type FullSchemaPolicy, | ||
isNeverTree, | ||
|
@@ -22,7 +21,6 @@ import { | |
PosetComparisonResult, | ||
type FieldDiscrepancy, | ||
} from "../discrepancies.js"; | ||
import { toStoredSchema } from "../toStoredSchema.js"; | ||
|
||
/** | ||
* A collection of View information for schema, including policy. | ||
|
@@ -142,7 +140,7 @@ export class SchemaCompatibilityTester { | |
// View schema has added a node type that the stored schema doesn't know about. | ||
// Note that all cases which trigger this should also trigger an AllowedTypeDiscrepancy (where the type is used). | ||
// This means this case should be redundant and could be removed in the future if there is a reason to do so | ||
// (like simplifying enablable type support). | ||
// (like simplifying staged type support). | ||
// See the TODO in getAllowedContentDiscrepancies. | ||
canView = false; | ||
} else if (discrepancy.view === undefined) { | ||
|
@@ -181,12 +179,15 @@ export class SchemaCompatibilityTester { | |
} | ||
} | ||
|
||
if (canUpgrade) { | ||
assert( | ||
allowsRepoSuperset(this.policy, stored, toStoredSchema(this.viewSchemaRoot)), | ||
"View schema must be a superset of the stored schema to allow upgrade", | ||
); | ||
} | ||
// TODO: It is no longer guaranteed that the result of toStoredSchema (which removes any staged allowed types in its conversion) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you have an idea for an alternative? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This also applies to optional fields via There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SchemaPolicy still has kludges for allowUnknownOptionalFields in it. I'm not sure if that's impacting this, but we should really clean that up now that the schema compatibility tester has been fixed to use view schema. I'd like to land that cleanup, and some working alternative for this validation (or a decision we are confident without it) before we further complicate things with staged. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm going to try and remove createUnknownOptionalFieldPolicy and FullSchemaPolicy.allowUnknownOptionalFields and see what happens. |
||
// is a superset of the stored schema, which may have upgraded staged allowed types from another client. | ||
// if (canUpgrade) { | ||
// // This includes staged allowed types in the conversion before their upgrade to ensure compatibility. | ||
// assert( | ||
// allowsRepoSuperset(this.policy, stored, toStoredSchema(this.viewSchemaRoot)), | ||
// "View schema must be a superset of the stored schema to allow upgrade", | ||
// ); | ||
// } | ||
|
||
return { | ||
canView, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -131,7 +131,30 @@ export interface AllowedTypeMetadata { | |
*/ | ||
readonly custom?: unknown; | ||
|
||
// TODO metadata for enablable types will be added here | ||
/** | ||
* If defined, indicates that an allowed type is {@link SchemaFactoryAlpha.staged | staged}. | ||
*/ | ||
readonly stagedSchemaUpgrade?: SchemaUpgrade; | ||
} | ||
|
||
/** | ||
* Package internal construction API. | ||
*/ | ||
export let createSchemaUpgrade: () => SchemaUpgrade; | ||
|
||
/** | ||
* Unique token used to upgrade schemas and determine if a particular upgrade has been completed. | ||
* | ||
* TODO:#38722 implement runtime schema upgrades until then, the class purely behaves as a placeholder and we disable no-extraneous-class | ||
* @sealed @alpha | ||
*/ | ||
// eslint-disable-next-line @typescript-eslint/no-extraneous-class | ||
export class SchemaUpgrade { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this a class? Why not a symbol? And later on, simply a bool/function, etc? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using classes like this is the cleanest way in typescript to be able to make strongly typed objects which users cannot construct. Each schema upgrade gets its own unique identity. I'm not sure how you would use symbols Booleans or functions to define a type which can hold values with distinct identities that are not user constructable. |
||
static { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: public static There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @noencke thats not a member, it's a static initialization block. This package has automation that would have detected that issue if it was what you though it was: no need to be a human linter. |
||
createSchemaUpgrade = () => new SchemaUpgrade(); | ||
} | ||
|
||
private constructor() {} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The class is marked as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For classes who's constructors we do not expose and explicitly have a different way to get an instance of them where no following that will break things and is tempting, this is a good pattern. FieldSchema and IterableTreeArrayContent follow it for example. I prefer when we can make doing things wrong a compile error rather than rely on someone noticing a type is sealed: good luck catching that in a code review: its hard to check the tags on every constructure called when reviewing code. Do actually have an example of a sealed class we export as a non-type export that requires a special non-constructor code-path to build that does not follow this pattern? I'm not aware of any, and I listed two classes that use this pattern above. |
||
} | ||
|
||
/** | ||
|
@@ -237,29 +260,41 @@ export type UnannotateAllowedType<T extends AnnotatedAllowedType> = | |
* @internal | ||
*/ | ||
export function normalizeAllowedTypes( | ||
types: ImplicitAllowedTypes, | ||
types: ImplicitAnnotatedAllowedTypes, | ||
): ReadonlySet<TreeNodeSchema> { | ||
// remove annotations before normalizing | ||
const unannotated = unannotateImplicitAllowedTypes(types); | ||
const normalized = new Set<TreeNodeSchema>(); | ||
if (isReadonlyArray(types)) { | ||
if (isReadonlyArray(unannotated)) { | ||
// Types array must not be modified after it is normalized since that would result in the user of the normalized data having wrong (out of date) content. | ||
Object.freeze(types); | ||
for (const lazyType of types) { | ||
Object.freeze(unannotated); | ||
for (const lazyType of unannotated) { | ||
normalized.add(evaluateLazySchema(lazyType)); | ||
} | ||
} else { | ||
normalized.add(evaluateLazySchema(types)); | ||
normalized.add(evaluateLazySchema(unannotated)); | ||
} | ||
return normalized; | ||
} | ||
|
||
/** | ||
* Normalizes an allowed type to an {@link AnnotatedAllowedType}, by adding empty annotations if they don't already exist. | ||
* Normalizes an allowed type to an {@link AnnotatedAllowedType}, by adding empty annotations if they don't already exist | ||
* and eagerly evaluating any lazy schema declarations. | ||
* | ||
* @remarks | ||
* Note: this must only be called after all required schemas have been declared, otherwise evaluation of | ||
* recursive schemas may fail. | ||
* type is frozen and should not be modified after being passed in. | ||
*/ | ||
export function normalizeToAnnotatedAllowedType<T extends TreeNodeSchema>( | ||
type: T | AnnotatedAllowedType<T> | AnnotatedAllowedType<LazyItem<T>>, | ||
): AnnotatedAllowedType<T> | AnnotatedAllowedType<LazyItem<T>> { | ||
): AnnotatedAllowedType<T> { | ||
Object.freeze(type); | ||
return isAnnotatedAllowedType(type) | ||
? type | ||
? { | ||
metadata: type.metadata, | ||
type: evaluateLazySchema(type.type), | ||
} | ||
: { | ||
metadata: {}, | ||
type, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Advertising to customers that how you write simple examples for how to use our public API surface is to use a non exported private test utility from the tree package is not great.
I think we should try and do this using the actual public API surface. If that means this version of the code has to be in the end-to-end tests instead of the tree package, or that it has to use independentView, then it can be updated accordingly.