fix(property-changeset): don't throw on a duplicate identical insert into an indexed collection#27702
Conversation
…into an indexed collection Merging a ChangeSet into an indexed (set/map-typed) collection throws "CS-003: Internal error: Added an already existing entry" whenever the incoming insert's key already has an insert recorded in the base ChangeSet — even when the two inserts are identical, e.g. from a duplicated or replayed op. Since this runs on the normal op-processing path, a duplicate op can crash-loop a document's summarizer. Treat an insert that exactly matches the already-recorded insert for the same key as a safe no-op, for both primitive and typed/polymorphic collections. A mismatched value for the same key is still a genuine conflict and continues to throw.
|
Azure Pipelines: 1 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Azure Pipelines: 1 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Hi! Thank you for opening this PR. Want me to review it? Based on the diff (68 lines, 3 files), I've queued these reviewers:
How this works
|
dannimad
left a comment
dannimad
left a comment
There was a problem hiding this comment.
Do not merge until the release process is complete
@dannimad When is the release process complete date |
Description
ChangeSetIndexedCollectionFunctions's merge logic throwsCS-003: Internal error: Added an already existing entry: <key>whenever an incoming insert's key already has an insert recorded in the base ChangeSet — even when the two inserts are identical (e.g. from a duplicated or replayed op). Since this runs on the normal op-processing path (SharedPropertyTree._applyRemoteChangeSet→processMessage), a duplicate op can put a document's summarizer into a crash-loop.This PR treats an insert that exactly matches the already-recorded insert for the same key as a safe no-op, for both primitive and typed/polymorphic collections. A mismatched value for the same key is still treated as a genuine conflict and continues to throw.
To reproduce: apply a ChangeSet insert for a key to a base ChangeSet that already has an identical insert for that same key (see the new test cases in
indexedCollection.spec.tsfor a minimal repro). Before this change that throws; after, it's a no-op.Reviewer Guidance
I haven't been able to run this package's own test suite locally yet (environment setup issue unrelated to this change — Azure DevOps Artifacts feed auth). The two new test cases closely follow this file's existing patterns, and the equivalent fix (applied to the compiled/patched form of this exact function) has been running against a real local Fluid server with 113 passing tests in a downstream consumer — but I'd like CI/a reviewer to confirm this package's own suite passes before merging. Opening as a draft until I can confirm locally.