fix(document): handle setting array to itself after saving and pushin…

…g a new value Fix #12656
Automattic · Nov 10, 2022 · 4ca7d2f · 4ca7d2f
1 parent f1c433c
commit 4ca7d2f
Show file tree

Hide file tree

Showing 2 changed files with 47 additions and 10 deletions.
diff --git a/lib/document.js b/lib/document.js
@@ -1468,13 +1468,7 @@ Document.prototype.$set = function $set(path, val, type, options) {
       const doc = this.$isSubdocument ? this.ownerDocument() : this;
       savedState = doc.$__.savedState;
       savedStatePath = this.$isSubdocument ? this.$__.fullPath + '.' + path : path;
-      if (savedState != null) {
-        const firstDot = savedStatePath.indexOf('.');
-        const topLevelPath = firstDot === -1 ? savedStatePath : savedStatePath.slice(0, firstDot);
-        if (!savedState.hasOwnProperty(topLevelPath)) {
-          savedState[topLevelPath] = utils.clone(doc.$__getValue(topLevelPath));
-        }
-      }
+      doc.$__saveInitialState(savedStatePath);
     }
 
     this.$__set(pathToMark, path, options, constructing, parts, schema, val, priorVal);
@@ -1583,6 +1577,10 @@ Document.prototype.$__shouldModify = function(pathToMark, path, options, constru
   if (this.$isNew) {
     return true;
   }
+  // Is path already modified? If so, always modify. We may unmark modified later.
+  if (path in this.$__.activePaths.getStatePaths('modify')) {
+    return true;
+  }
 
   // Re: the note about gh-7196, `val` is the raw value without casting or
   // setters if the full path is under a single nested subdoc because we don't
@@ -1780,11 +1778,10 @@ Document.prototype.$inc = function $inc(path, val) {
 
   const currentValue = this.$__getValue(path) || 0;
 
-  this.$__setValue(path, currentValue + val);
-
   this.$__.primitiveAtomics = this.$__.primitiveAtomics || {};
   this.$__.primitiveAtomics[path] = { $inc: val };
   this.markModified(path);
+  this.$__setValue(path, currentValue + val);
 
   return this;
 };
@@ -1927,13 +1924,31 @@ Document.prototype.$__path = function(path) {
  */
 
 Document.prototype.markModified = function(path, scope) {
+  this.$__saveInitialState(path);
+
   this.$__.activePaths.modify(path);
   if (scope != null && !this.$isSubdocument) {
     this.$__.pathsToScopes = this.$__pathsToScopes || {};
     this.$__.pathsToScopes[path] = scope;
   }
 };
 
+/*!
+ * ignore
+ */
+
+Document.prototype.$__saveInitialState = function $__saveInitialState(path) {
+  const savedState = this.$__.savedState;
+  const savedStatePath = path;
+  if (savedState != null) {
+    const firstDot = savedStatePath.indexOf('.');
+    const topLevelPath = firstDot === -1 ? savedStatePath : savedStatePath.slice(0, firstDot);
+    if (!savedState.hasOwnProperty(topLevelPath)) {
+      savedState[topLevelPath] = utils.clone(this.$__getValue(topLevelPath));
+    }
+  }
+};
+
 /**
  * Clears the modified state on the specified path.
  *
@@ -3379,6 +3394,7 @@ Document.prototype.$__dirty = function() {
       schema: _this.$__path(path)
     };
   });
+
   // gh-2558: if we had to set a default and the value is not undefined,
   // we have to save as well
   all = all.concat(this.$__.activePaths.map('default', function(path) {

diff --git a/test/document.test.js b/test/document.test.js
@@ -9191,7 +9191,6 @@ describe('document', function() {
 
     const Test = db.model('Test', schema);
 
-
     const foo = new Test({ bar: 'bar' });
     await foo.save();
     assert.ok(!foo.isModified('bar'));
@@ -11996,6 +11995,28 @@ describe('document', function() {
       title: 'The power of JavaScript'
     });
   });
+
+  it('handles setting array to itself after saving and pushing a new value (gh-12656)', async function() {
+    const Test = db.model('Test', new Schema({
+      list: [{
+        a: Number
+      }]
+    }));
+    await Test.create({ list: [{ a: 1, b: 11 }] });
+
+    let doc = await Test.findOne();
+    doc.list.push({ a: 2 });
+    doc.list = [...doc.list];
+    await doc.save();
+
+    doc.list.push({ a: 3 });
+    doc.list = [...doc.list];
+    await doc.save();
+
+    doc = await Test.findOne();
+    assert.equal(doc.list.length, 3);
+    assert.deepStrictEqual(doc.list.map(el => el.a), [1, 2, 3]);
+  });
 });
 
 describe('Check if instance function that is supplied in schema option is availabe', function() {