-
Notifications
You must be signed in to change notification settings - Fork 569
Improve applySerializedChange to support cross-session usage and transactions
#26662
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
86346aa
8ebc2d2
bd6249e
b82ee62
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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -63,6 +63,7 @@ import { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| type TaggedChange, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } from "../core/index.js"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| DefaultRevisionReplacer, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| type FieldBatchCodec, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| type TreeCompressionStrategy, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| allowsRepoSuperset, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -715,6 +716,7 @@ export class TreeCheckout implements ITreeCheckoutFork { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| idCompressor: this.idCompressor, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| originatorId: this.idCompressor.localSessionId, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| revision, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| schema: undefined, // By not passing the schema, we avoid compressing identifiers in identifier fields | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
noencke marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const encodedChange = this.changeFamily.codecs.resolve(4).encode(change, context); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -775,19 +777,26 @@ export class TreeCheckout implements ITreeCheckoutFork { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| throw new UsageError(`Cannot apply change. Invalid serialized change format.`); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const { revision, originatorId, change } = serializedChange; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (originatorId !== this.idCompressor.localSessionId) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| throw new UsageError( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| `Cannot apply change. A serialized changed must be applied to the same SharedTree as it was created from.`, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const context: ChangeEncodingContext = { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| idCompressor: this.idCompressor, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| originatorId: this.idCompressor.localSessionId, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| originatorId, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| revision, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
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 probably all works, but it's kludgy that we use revision without decoding it. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const decodedChange = this.changeFamily.codecs.resolve(4).decode(change, context); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Apply the change to the branch, but _not_ the `activeBranch` - we do not support squashing serialized commits in a transaction. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| this.#transaction.branch.apply(tagChange(decodedChange, revision)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // The change's revision may have been produced by an IdCompressor that overlaps or is otherwise incompatible with ours. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Replace the revision to avoid any ID collisions in the changeset - every application of a serialized changed will result in a different revision. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Replace the revision to avoid any ID collisions in the changeset - every application of a serialized changed will result in a different revision. | |
| // Replace the revision to avoid any ID collisions in the changeset - every application of a serialized change will result in a different revision. |
Outdated
Copilot
AI
Mar 6, 2026
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.
When applying a serialized change inside a transaction, activeBranch.mintRevisionTag() returns the transaction revision (constant for all commits in that transaction). This means multiple applied serialized changes (and any in-transaction edits) will share the same revision tag, which can cause ChangeAtomId/local-ID collisions when the transaction is squashed (compose assumes key collisions refer to the same node). Consider remapping local IDs to avoid collisions within the transaction revision (e.g., reuse a per-transaction RevisionReplacer seeded with already-used IDs, or otherwise allocate fresh IDs in the transaction’s ID space) and add a regression test that mixes applyChange() with other edits in the same transaction.
| // Replace the revision to avoid any ID collisions in the changeset - every application of a serialized changed will result in a different revision. | |
| // This means that applying the same serialized change twice will result in its insertions/mutations being performed twice. | |
| // The second change will not be deduplicated with the first by the rebaser, as would be the case if they were truly the same change with the same revision. | |
| const newRevision = this.#transaction.activeBranch.mintRevisionTag(); | |
| const newChange = this.changeFamily.rebaser.changeRevision( | |
| decodedChange, | |
| new DefaultRevisionReplacer( | |
| newRevision, | |
| this.changeFamily.rebaser.getRevisions(decodedChange), | |
| ), | |
| ); | |
| this.#transaction.activeBranch.apply(tagChange(newChange, newRevision)); | |
| } | |
| // Replace the revision to avoid any ID collisions in the changeset. Each application of a serialized change is given a | |
| // unique synthetic revision, even when performed inside a transaction whose own revision is constant, to avoid local-ID | |
| // collisions when the transaction is squashed. | |
| const baseRevision = this.#transaction.activeBranch.mintRevisionTag(); | |
| const uniqueRevision = `${baseRevision}-sc-${this.#appliedSerializedChangeCounter++}` as RevisionTag; | |
| const newChange = this.changeFamily.rebaser.changeRevision( | |
| decodedChange, | |
| new DefaultRevisionReplacer( | |
| uniqueRevision, | |
| this.changeFamily.rebaser.getRevisions(decodedChange), | |
| ), | |
| ); | |
| this.#transaction.activeBranch.apply(tagChange(newChange, uniqueRevision)); | |
| } | |
| // Counter used to ensure unique synthetic revisions for applied serialized changes, even within a transaction. | |
| private #appliedSerializedChangeCounter = 0; |
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -332,12 +332,11 @@ export interface TreeBranchAlpha extends TreeBranch, TreeContextAlpha { | |||||||||
| * Apply a serialized change to this branch. | ||||||||||
| * @param change - the change to apply. | ||||||||||
| * Changes are acquired via `getChange` in a branch's {@link TreeBranchEvents.changed | "changed"} event. | ||||||||||
| * @remarks Changes may only be applied to a SharedTree with the same IdCompressor instance and branch state from which they were generated. | ||||||||||
| * They may be created by one branch and applied to another, but only if both branches share the same history at the time of creation and application. | ||||||||||
| * @remarks Changes may only be applied to a SharedTree with the same branch state from which they were generated. | ||||||||||
| * They may be created by one branch and applied to another, but only if both branches share the same history at the time of creation and application, respectively. | ||||||||||
|
Comment on lines
+335
to
+336
|
||||||||||
| * @remarks Changes may only be applied to a SharedTree with the same branch state from which they were generated. | |
| * They may be created by one branch and applied to another, but only if both branches share the same history at the time of creation and application, respectively. | |
| * @remarks Changes may only be applied to a branch whose content/branch state (and compatible schema) matches the state from which they were generated. | |
| * They may be created by one branch and applied to another, including independently created views, as long as both branches are in an equivalent logical state at the time of creation and application (for example, by having applied the same sequence of edits or otherwise converged to the same content). |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -309,7 +309,7 @@ describe("simple-tree tree", () => { | |||||
| assert.equal(viewA.root, 4); | ||||||
| }); | ||||||
|
|
||||||
| it("fail to apply to a branch in another session", () => { | ||||||
| it("can be applied to a view with a different session", () => { | ||||||
| const config = new TreeViewConfiguration({ schema: schema.number }); | ||||||
| const viewA = getView(config); | ||||||
| viewA.initialize(3); | ||||||
|
|
@@ -323,10 +323,9 @@ describe("simple-tree tree", () => { | |||||
| }); | ||||||
| viewA.root = 4; | ||||||
|
|
||||||
| const c = change ?? assert.fail("change not captured"); | ||||||
| assert.throws(() => { | ||||||
| viewB.applyChange(c); | ||||||
| }, /cannot apply change.*same sharedtree/i); | ||||||
| assert(change !== undefined); | ||||||
| viewB.applyChange(change); | ||||||
| assert.equal(viewB.root, 4); | ||||||
| }); | ||||||
|
|
||||||
| it("error if malformed", () => { | ||||||
|
|
@@ -383,7 +382,33 @@ describe("simple-tree tree", () => { | |||||
| assert.equal(viewA.root, 4); | ||||||
| }); | ||||||
|
|
||||||
| it("apply before transactions", () => { | ||||||
| it("applying the same change twice is not idempotent", () => { | ||||||
| const sf = new SchemaFactory("test"); | ||||||
| class List extends sf.array("List", sf.number) {} | ||||||
| const config = new TreeViewConfiguration({ schema: List }); | ||||||
| const viewA = getView(config); | ||||||
| viewA.initialize([1, 2, 3]); | ||||||
| const viewB = viewA.fork(); | ||||||
|
|
||||||
| let change: JsonCompatibleReadOnly | undefined; | ||||||
| viewB.events.on("changed", (metadata) => { | ||||||
| assert(metadata.isLocal); | ||||||
| change = metadata.getChange(); | ||||||
| }); | ||||||
|
|
||||||
| // Insert a node on viewB | ||||||
| viewB.root.insertAtEnd(4); | ||||||
| assert(change !== undefined); | ||||||
|
|
||||||
| // Apply the same serialized change twice to viewA | ||||||
| viewA.applyChange(change); | ||||||
| viewA.applyChange(change); | ||||||
|
|
||||||
| // Each application should produce a distinct effect - the node is inserted twice | ||||||
| assert.deepEqual([...viewA.root], [1, 2, 3, 4, 4]); | ||||||
| }); | ||||||
|
|
||||||
| it("non-transaction change can be applied inside a transaction", () => { | ||||||
| const config = new TreeViewConfiguration({ schema: schema.number }); | ||||||
| const viewA = getView(config); | ||||||
| viewA.initialize(3); | ||||||
|
|
@@ -395,16 +420,129 @@ describe("simple-tree tree", () => { | |||||
| change = metadata.getChange(); | ||||||
| }); | ||||||
|
|
||||||
| // Make a non-transaction change on viewB | ||||||
| viewB.root = 4; | ||||||
| assert(change !== undefined); | ||||||
|
|
||||||
| // Apply that non-transaction change inside a transaction on viewA | ||||||
| const capturedChange = change; | ||||||
| viewA.runTransaction(() => { | ||||||
| viewA.root = 5; | ||||||
| assert(change !== undefined); | ||||||
| // Even though the serialized change (= 4) is applied _after_ the transaction change (= 5), | ||||||
| // it is considered a change external to the transaction and so will be applied before the transaction changes, | ||||||
| // as is the general policy for external changes applied during a transaction. | ||||||
| viewA.applyChange(change); | ||||||
| viewA.applyChange(capturedChange); | ||||||
| }); | ||||||
| assert.equal(viewA.root, 5); | ||||||
| assert.equal(viewA.root, 4); | ||||||
| }); | ||||||
|
|
||||||
| it("multiple non-transaction changes can be applied together in a transaction", () => { | ||||||
| const sf = new SchemaFactory("test"); | ||||||
| class List extends sf.array("List", sf.number) {} | ||||||
| const config = new TreeViewConfiguration({ schema: List }); | ||||||
| const viewA = getView(config); | ||||||
| viewA.initialize([1, 2, 3]); | ||||||
| const viewB = viewA.fork(); | ||||||
|
|
||||||
| const changes: JsonCompatibleReadOnly[] = []; | ||||||
| viewB.events.on("changed", (metadata) => { | ||||||
| assert(metadata.isLocal); | ||||||
| changes.push(metadata.getChange()); | ||||||
| }); | ||||||
|
|
||||||
| // Make two separate non-transaction changes on viewB | ||||||
| viewB.root.insertAtEnd(4); | ||||||
| viewB.root.insertAtEnd(5); | ||||||
| assert.equal(changes.length, 2); | ||||||
|
|
||||||
| // Apply both non-transaction changes together inside a single transaction on viewA | ||||||
| viewA.runTransaction(() => { | ||||||
| viewA.applyChange(changes[0]); | ||||||
| viewA.applyChange(changes[1]); | ||||||
| }); | ||||||
| assert.deepEqual([...viewA.root], [1, 2, 3, 4, 5]); | ||||||
| }); | ||||||
|
|
||||||
| it("applied change is rolled back when transaction is aborted", () => { | ||||||
| const config = new TreeViewConfiguration({ schema: schema.number }); | ||||||
| const viewA = getView(config); | ||||||
| viewA.initialize(3); | ||||||
| const viewB = viewA.fork(); | ||||||
|
|
||||||
| let change: JsonCompatibleReadOnly | undefined; | ||||||
| viewB.events.on("changed", (metadata) => { | ||||||
| assert(metadata.isLocal); | ||||||
| change = metadata.getChange(); | ||||||
| }); | ||||||
|
|
||||||
| viewB.root = 4; | ||||||
| assert(change !== undefined); | ||||||
|
|
||||||
| const capturedChange = change; | ||||||
| Tree.runTransaction(viewA, () => { | ||||||
| viewA.applyChange(capturedChange); | ||||||
| assert.equal(viewA.root, 4); | ||||||
| return Tree.runTransaction.rollback; | ||||||
| }); | ||||||
| // The serialized change should be rolled back along with the transaction | ||||||
| assert.equal(viewA.root, 3); | ||||||
| }); | ||||||
|
|
||||||
| it("can apply a change with an identifier field build to a view with a different id compressor", () => { | ||||||
|
||||||
| it("can apply a change with an identifier field build to a view with a different id compressor", () => { | |
| it("can apply a change with an identifier field built to a view with a different id compressor", () => { |
Uh oh!
There was an error while loading. Please reload this page.
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.
Rather than exposing this, I originally tried making
branch.applyautomatically change the revisions of changes to a newly minted revision if you didn't give them a revision tag. That is more general and perhaps more elegant. However, this module can't depend on the DefaultRevisionRewriter (or whatever it's called) because it creates a circular dependency, so that approach required some pretty ugly injection patterns and I said nah.