From 1683efaf6aaa4a504a8ccfbc23d60fba27e07008 Mon Sep 17 00:00:00 2001
From: Alec Gibson <12036746+alecgibson@users.noreply.github.com>
Date: Tue, 14 May 2024 15:41:07 +0100
Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Avoid=20mutating=20milestone=20s?=
 =?UTF-8?q?napshots=20when=20building=20from=20ops?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

At the moment, we directly pass our milestone snapshot to
`ot.applyOps()`, which [mutates the snapshot][1].

In most Production-grade implementations of the `MilestoneDb`, this is
fine, since mutating the in-memory `Snapshot` will have no effect on the
underlying database representation.

However, this does break the `MemoryMilestoneDb` (eg in tests).

In order to fix this, and generally be defensive (eg if db adapters
decide to implement an in-memory cache), this change clones the snapshot
before calling `ot.applyOps()` on it.

[1]: https://github.com/share/sharedb/blob/a5c19b986d65c93abbc9ed11f7a25c0dbaf0788c/lib/ot.js#L172
---
 lib/backend.js                          |  2 +-
 test/client/snapshot-version-request.js | 34 +++++++++++++++++++++++++
 2 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/lib/backend.js b/lib/backend.js
index c69693a56..7a3cd41cc 100644
--- a/lib/backend.js
+++ b/lib/backend.js
@@ -909,7 +909,7 @@ Backend.prototype._fetchSnapshotByTimestamp = function(collection, id, timestamp
 };
 
 Backend.prototype._buildSnapshotFromOps = function(id, startingSnapshot, ops, callback) {
-  var snapshot = startingSnapshot || new Snapshot(id, 0, null, undefined, null);
+  var snapshot = util.clone(startingSnapshot) || new Snapshot(id, 0, null, undefined, null);
   var error = ot.applyOps(snapshot, ops, {_normalizeLegacyJson0Ops: true});
   callback(error, snapshot);
 };
diff --git a/test/client/snapshot-version-request.js b/test/client/snapshot-version-request.js
index ae3f2ce72..61491358d 100644
--- a/test/client/snapshot-version-request.js
+++ b/test/client/snapshot-version-request.js
@@ -6,6 +6,7 @@ var sinon = require('sinon');
 var async = require('async');
 var json0v2 = require('ot-json0-v2').type;
 var types = require('../../lib/types');
+var clone = require('../../lib/util').clone;
 
 describe('SnapshotVersionRequest', function() {
   var backend;
@@ -440,6 +441,39 @@ describe('SnapshotVersionRequest', function() {
       ], done);
     });
 
+    it('does not mutate the milestone snapshot', function(done) {
+      var connection = backendWithMilestones.connect();
+      var doc = connection.get('books', 'mocking-bird');
+      var milestoneDb = backendWithMilestones.milestoneDb;
+      var milestone;
+
+      async.waterfall([
+        doc.create.bind(doc, {title: 'To Kill a Mocking Bird'}),
+        doc.submitOp.bind(doc, {p: ['author'], oi: 'Harper Lea'}),
+        doc.submitOp.bind(doc, {p: ['author'], od: 'Harper Lea', oi: 'Harper Lee'}),
+        milestoneDb.getMilestoneSnapshot.bind(milestoneDb, 'books', 'mocking-bird', 3),
+        function(snapshot, next) {
+          milestone = clone(snapshot);
+          next();
+        },
+        function(next) {
+          // Internally, this calls ot.applyOps(), which mutates the snapshot it's passed.
+          // This call shouldn't cause the in-memory milestone snapshot to be mutated.
+          connection.fetchSnapshot('books', 'mocking-bird', 3, next);
+        },
+        function(snapshot, next) {
+          expect(snapshot.v).to.equal(3);
+          expect(snapshot.data).to.eql({title: 'To Kill a Mocking Bird', author: 'Harper Lee'});
+          next();
+        },
+        milestoneDb.getMilestoneSnapshot.bind(milestoneDb, 'books', 'mocking-bird', 3),
+        function(snapshot, next) {
+          expect(snapshot).to.eql(milestone);
+          next();
+        }
+      ], done);
+    });
+
     describe('with version null', function() {
       it('fetches a latest version', function(done) {
         var doc = backendWithMilestones.connect().get('books', 'mocking-bird');