-
Notifications
You must be signed in to change notification settings - Fork 537
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
feat(tree): Add isRepoSuperset to validate the incompatibilities between … #22045
feat(tree): Add isRepoSuperset to validate the incompatibilities between … #22045
Conversation
🦋 Changeset detectedLatest commit: 1f5d940 The changes in this PR will be included in the next version bump. This PR includes changesets to release 158 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
⯅ @fluid-example/bundle-size-tests: +245 Bytes
Baseline commit: 614392a |
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.
Please add a changeset.
* 2. Expanding the set of allowed types for a field. | ||
* 3. Relaxing a field kind to a more general field kind. | ||
* | ||
* Notes: We expect isRepoSuperset to return consistent results with allowsRepoSuperset. However, currently there are some inconsistencies: |
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.
here and below seems like a more appropriate comment for the test file
* This function uses incompatibilities to determine if changes to a document schema are backward-compatible. | ||
* According to the policy of schema evolution, `isRepoSuperset` supports three types of changes: |
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.
this is good doc, but probably more appropriate for a remarks block. For the main documentation, something more like "Determines whether the view
schema allows a superset of the documents that the stored
schema allows.`
return false; | ||
} | ||
} | ||
return true; |
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.
this seems wrong. the last return true
should handle things. Add a test to cover this case as well.
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.
Yes good call. It should continue validating the next incompatibility item, instead of returning result directly. The same mistake occurred in other branches.
packages/dds/tree/src/feature-libraries/modular-schema/discrepancies.ts
Outdated
Show resolved
Hide resolved
packages/dds/tree/src/feature-libraries/modular-schema/discrepancies.ts
Outdated
Show resolved
Hide resolved
packages/dds/tree/src/test/feature-libraries/modular-schema/discrepancies.spec.ts
Outdated
Show resolved
Hide resolved
@@ -103,6 +127,13 @@ describe("Schema Discrepancies", () => { | |||
]); | |||
|
|||
assert.deepEqual(getAllowedContentIncompatibilities(mapNodeSchema, mapNodeSchema), []); | |||
|
|||
assert.equal(isRepoSuperset(objectNodeSchema, mapNodeSchema), false); | |||
// there is inconsistency |
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.
could you track this with some work and elaborate here so we can come to a decision on it?
Something like this for the comment:
"Current logic allows relaxing an object node to a map node which allows the union of all types allowed on the object node's fields. It is not clear this is something we want to support, and getAllowedContentIncompatibilities does not allow it today".
If we do want to support it:
- We'll need better e2e tests for this case. I don't remember if map node encoding works differently from object node encoding, but I believe it might, which would be a problem.
packages/dds/tree/src/test/feature-libraries/modular-schema/discrepancies.spec.ts
Outdated
Show resolved
Hide resolved
packages/dds/tree/src/test/feature-libraries/modular-schema/discrepancies.spec.ts
Outdated
Show resolved
Hide resolved
packages/dds/tree/src/test/feature-libraries/modular-schema/discrepancies.spec.ts
Outdated
Show resolved
Hide resolved
…ancies.ts Co-authored-by: Abram Sanderson <[email protected]>
…screpancies.spec.ts Co-authored-by: Abram Sanderson <[email protected]>
.changeset/strong-mice-talk.md
Outdated
"@fluidframework/tree": minor | ||
--- | ||
|
||
Add a function `isRepoSuperset` to determines whether the `view` schema allows a superset of the documents that the `stored` schema allows. |
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.
Since we're not exporting this, I feel like more generic language would be better, or at least add a disclaimer to the changeset along the lines of "changes are not customer-facing and make progress toward future plans in Tree's schema evolution space."
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.
looks good enough to check in. FYI--I filed https://dev.azure.com/fluidframework/internal/_workitems/edit/11525 to deal with never tree issues, which I still think aren't totally right in this change.
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.
Thank you!
…two repos
AB#8717
The PR consists of two parts:
allowsRepoSuperset
The previous tests contained schemas that were incorrectly treated as
neverTree
, it may not change the test result but influence the the analysis process ofallowsRepoSuperset
. This fix ensures accurate schema creation and validation.isRepoSuperset
:This function uses incompatibilities to determine if changes to a document schema are backward-compatible. According to the doc of schema evolution,
isRepoSuperset
supports three types of changes:Adding an optional field to an object node.
Expanding the set of allowed types for a field.
Relaxing a field kind to a more general field kind.
Notes: We expect
isRepoSuperset
to return consistent results withallowsRepoSuperset
. However, there are some inconsistencies:objectNodeSchema
and b is amapNodeSchema
),isRepoSuperse
will determine that a can never be the superset of b. In contrast,allowsRepoSuperset
will continue validating internal fields.Reviewer Guidance
The review process is outlined on this wiki page.