Skip to content
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

Merged
merged 12 commits into from
Aug 2, 2024

Conversation

clarenceli-msft
Copy link
Contributor

@clarenceli-msft clarenceli-msft commented Jul 29, 2024

…two repos

AB#8717

The PR consists of two parts:

  1. Fixing previous unit tests for 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 of allowsRepoSuperset. This fix ensures accurate schema creation and validation.

  1. Introducing 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 with allowsRepoSuperset. However, there are some inconsistencies:

  • Different Node Kinds: If a and b have different node kinds (e.g., a is an objectNodeSchema and b is a mapNodeSchema), 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.

@clarenceli-msft clarenceli-msft requested a review from a team as a code owner July 29, 2024 03:45
Copy link

changeset-bot bot commented Jul 29, 2024

🦋 Changeset detected

Latest commit: 1f5d940

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 158 packages
Name Type
fluid-framework Major
@fluidframework/tree Major
@fluid-example/presence-tracker Major
@fluid-example/inventory-app Major
@fluid-example/app-integration-external-controller Major
@fluid-example/shared-tree-demo Major
@fluid-example/bundle-size-tests Major
@fluid-example/tree-comparison Major
@fluid-example/bubblebench-shared-tree-flex-tree Major
@fluid-example/bubblebench-shared-tree Major
@fluid-internal/tablebench Major
@fluid-example/tree-shim Major
@fluid-experimental/property-shared-tree-interop Major
@fluid-experimental/tree Major
@fluid-experimental/tree-react-api Major
@fluidframework/fluid-telemetry Major
@fluidframework/azure-client Major
@fluidframework/azure-end-to-end-tests Major
@fluid-internal/functional-tests Major
@fluid-private/test-end-to-end-tests Major
@fluidframework/devtools-core Major
@fluid-example/devtools-example Major
@fluid-example/bubblebench-common Major
@fluid-example/bubblebench-experimental-tree Major
@fluid-internal/devtools-browser-extension Major
@fluid-internal/devtools-view Major
@fluidframework/devtools Major
@fluid-example/bubblebench-baseline Major
@fluid-example/bubblebench-ot Major
@fluid-example/attributable-map Major
@fluid-example/collaborative-textarea Major
@fluid-example/contact-collection Major
@fluid-example/data-object-grid Major
@fluid-example/task-selection Major
@fluid-example/odspsnapshotfetch-perftestapp Major
@fluid-example/app-insights-logger Major
@fluid-example/canvas Major
@fluid-example/clicker Major
@fluid-example/codemirror Major
@fluid-example/diceroller Major
@fluid-example/monaco Major
@fluid-example/multiview-constellation-model Major
@fluid-example/multiview-constellation-view Major
@fluid-example/multiview-container Major
@fluid-example/multiview-coordinate-model Major
@fluid-example/multiview-coordinate-interface Major
@fluid-example/multiview-plot-coordinate-view Major
@fluid-example/multiview-slider-coordinate-view Major
@fluid-example/multiview-triangle-view Major
@fluid-example/prosemirror Major
@fluid-example/smde Major
@fluid-example/table-document Major
@fluid-example/todo Major
@fluid-example/webflow Major
@fluid-example/app-integration-external-data Major
@fluid-example/example-utils Major
@fluid-example/webpack-fluid-loader Major
@fluid-example/app-integration-live-schema-upgrade Major
@fluid-example/version-migration-same-container Major
@fluid-example/app-integration-schema-upgrade Major
@fluid-example/app-integration-container-views Major
@fluid-example/app-integration-external-views Major
@fluid-example/view-framework-sampler Major
@fluid-example/property-inspector Major
@fluid-example/schemas Major
@fluid-experimental/azure-scenario-runner Major
@fluid-experimental/property-binder Major
@fluid-experimental/property-changeset Major
@fluid-experimental/property-common Major
@fluid-experimental/property-dds Major
@fluid-experimental/property-inspector-table Major
@fluid-experimental/property-properties Major
@fluid-experimental/property-proxy Major
@fluid-experimental/property-query Major
@fluid-experimental/attributable-map Major
@fluid-experimental/ot Major
@fluid-experimental/sharejs-json1 Major
@fluid-experimental/sequence-deprecated Major
@fluid-experimental/data-objects Major
@fluid-experimental/last-edited Major
@fluid-experimental/ink Major
@fluid-experimental/pact-map Major
@fluid-experimental/attributor Major
@fluid-experimental/data-object-base Major
@fluid-experimental/dds-interceptions Major
@fluid-experimental/oldest-client-observer Major
@fluid-experimental/odsp-end-to-end-tests Major
@fluid-internal/platform-dependent Major
@fluid-internal/client-utils Major
@fluid-internal/local-server-tests Major
@fluid-internal/mocha-test-setup Major
@fluid-internal/test-snapshots Major
@fluid-internal/test-app-insights-logger Major
@fluid-internal/test-driver-definitions Major
@fluid-internal/test-service-load Major
@fluid-internal/replay-tool Major
@fluid-private/test-dds-utils Major
@fluid-private/test-loader-utils Major
@fluid-private/stochastic-test-utils Major
@fluid-private/test-drivers Major
@fluid-private/test-pairwise-generator Major
@fluid-private/test-version-utils Major
@fluid-private/changelog-generator-wrapper Major
@fluid-tools/fetch-tool Major
@fluid-tools/markdown-magic Major
@fluidframework/azure-local-service Major
@fluidframework/azure-service-utils Major
@fluidframework/container-definitions Major
@fluidframework/core-interfaces Major
@fluidframework/core-utils Major
@fluidframework/driver-definitions Major
@fluidframework/cell Major
@fluidframework/counter Major
@fluidframework/map Major
@fluidframework/matrix Major
@fluidframework/merge-tree Major
@fluidframework/ordered-collection Major
@fluidframework/register-collection Major
@fluidframework/sequence Major
@fluidframework/shared-object-base Major
@fluidframework/shared-summary-block Major
@fluidframework/task-manager Major
@fluidframework/debugger Major
@fluidframework/driver-base Major
@fluidframework/driver-web-cache Major
@fluidframework/file-driver Major
@fluidframework/local-driver Major
@fluidframework/odsp-driver-definitions Major
@fluidframework/odsp-driver Major
@fluidframework/odsp-urlresolver Major
@fluidframework/replay-driver Major
@fluidframework/routerlicious-driver Major
@fluidframework/routerlicious-urlresolver Major
@fluidframework/tinylicious-driver Major
@fluidframework/agent-scheduler Major
@fluidframework/aqueduct Major
@fluidframework/app-insights-logger Major
@fluidframework/fluid-static Major
@fluidframework/request-handler Major
@fluidframework/synthesize Major
@fluidframework/undo-redo Major
@fluidframework/container-loader Major
@fluidframework/driver-utils Major
@fluidframework/container-runtime-definitions Major
@fluidframework/container-runtime Major
@fluidframework/datastore-definitions Major
@fluidframework/datastore Major
@fluidframework/id-compressor Major
@fluidframework/runtime-definitions Major
@fluidframework/runtime-utils Major
@fluidframework/test-runtime-utils Major
@fluidframework/odsp-client Major
@fluidframework/tinylicious-client Major
@fluidframework/test-utils Major
@fluidframework/fluid-runner Major
@fluidframework/odsp-doclib-utils Major
@fluidframework/telemetry-utils Major
@fluidframework/tool-utils Major

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

@github-actions github-actions bot added base: main PRs targeted against main branch area: dds Issues related to distributed data structures area: dds: tree labels Jul 29, 2024
@msfluid-bot
Copy link
Collaborator

msfluid-bot commented Jul 29, 2024

@fluid-example/bundle-size-tests: +245 Bytes
Metric NameBaseline SizeCompare SizeSize Diff
aqueduct.js 459.56 KB 459.6 KB +35 Bytes
azureClient.js 556.61 KB 556.66 KB +49 Bytes
connectionState.js 680 Bytes 680 Bytes No change
containerRuntime.js 260.12 KB 260.13 KB +14 Bytes
fluidFramework.js 409.15 KB 409.17 KB +14 Bytes
loader.js 134.04 KB 134.05 KB +14 Bytes
map.js 42.13 KB 42.14 KB +7 Bytes
matrix.js 146.53 KB 146.54 KB +7 Bytes
odspClient.js 524.76 KB 524.8 KB +49 Bytes
odspDriver.js 97.55 KB 97.57 KB +21 Bytes
odspPrefetchSnapshot.js 42.61 KB 42.62 KB +14 Bytes
sharedString.js 163.26 KB 163.27 KB +7 Bytes
sharedTree.js 399.67 KB 399.67 KB +7 Bytes
Total Size 3.31 MB 3.31 MB +245 Bytes

Baseline commit: 614392a

Generated by 🚫 dangerJS against 1f5d940

@clarenceli-msft clarenceli-msft requested a review from Abe27342 July 29, 2024 15:00
@markfields markfields changed the title feat(tree): Add isRepoSupet to validate the incompatibilities between … feat(tree): Add isRepoSuperset to validate the incompatibilities between … Jul 29, 2024
Copy link
Member

@tylerbutler tylerbutler left a 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:
Copy link
Contributor

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

Comment on lines 394 to 395
* 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:
Copy link
Contributor

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;
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@@ -103,6 +127,13 @@ describe("Schema Discrepancies", () => {
]);

assert.deepEqual(getAllowedContentIncompatibilities(mapNodeSchema, mapNodeSchema), []);

assert.equal(isRepoSuperset(objectNodeSchema, mapNodeSchema), false);
// there is inconsistency
Copy link
Contributor

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.

@clarenceli-msft clarenceli-msft requested a review from a team as a code owner August 2, 2024 15:08
"@fluidframework/tree": minor
---

Add a function `isRepoSuperset` to determines whether the `view` schema allows a superset of the documents that the `stored` schema allows.
Copy link
Contributor

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."

Copy link
Contributor

@Abe27342 Abe27342 left a 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.

Copy link
Member

@tylerbutler tylerbutler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@clarenceli-msft clarenceli-msft merged commit f6fdc95 into microsoft:main Aug 2, 2024
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: dds: tree area: dds Issues related to distributed data structures base: main PRs targeted against main branch changeset-present
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants