Clean update callback signature and tests

baninaveen · Feb 14, 2016 · 4adf9c8 · 4adf9c8
1 parent 65cc00b
commit 4adf9c8
Show file tree

Hide file tree

Showing 2 changed files with 96 additions and 2 deletions.
diff --git a/lib/datastore.js b/lib/datastore.js
@@ -538,7 +538,20 @@ Datastore.prototype.findOne = function (query, projection, callback) {
  *                 options.multi If true, can update multiple documents (defaults to false)
  *                 options.upsert If true, document is inserted if the query doesn't match anything
  *                 options.returnUpdatedDocs Defaults to false, if true return as third argument the array of updated matched documents (even if no change actually took place)
- * @param {Function} cb Optional callback, signature: err, numReplaced, upsert (set to true if the update was in fact an upsert)
+ * @param {Function} cb Optional callback, signature: (err, numAffected, affectedDocuments, upsert)
+ *                      If update was an upsert, upsert flag is set to true
+ *                      affectedDocuments can be one of the following:
+ *                        * For an upsert, the upserted document
+ *                        * For an update with returnUpdatedDocs option false, null
+ *                        * For an update with returnUpdatedDocs true and multi false, the updated document
+ *                        * For an update with returnUpdatedDocs true and multi true, the array of updated documents
+ *
+ * WARNING: The API was changed between v1.7.4 and v1.8, for consistency and readability reasons. Prior and including to v1.7.4,
+ *          the callback signature was (err, numAffected, updated) where updated was the updated document in case of an upsert
+ *          or the array of updated documents for an update if the returnUpdatedDocs option was true. That meant that the type of
+ *          affectedDocuments in a non multi update depended on whether there was an upsert or not, leaving only two ways for the
+ *          user to check whether an upsert had occured: checking the type of affectedDocuments or running another find query on
+ *          the whole dataset to check its size. Both options being ugly, the breaking change was necessary.
  *
  * @api private Use Datastore.update which has the same signature
  */
@@ -584,7 +597,7 @@ Datastore.prototype._update = function (query, updateQuery, options, cb) {
 
         return self._insert(toBeInserted, function (err, newDoc) {
           if (err) { return callback(err); }
-          return callback(null, 1, newDoc);
+          return callback(null, 1, newDoc, true);
         });
       }
     });
@@ -630,6 +643,7 @@ Datastore.prototype._update = function (query, updateQuery, options, cb) {
         } else {
           var updatedDocsDC = [];
           updatedDocs.forEach(function (doc) { updatedDocsDC.push(model.deepCopy(doc)); });
+          if (! multi) { updatedDocsDC = updatedDocsDC[0]; }
           return callback(null, numReplaced, updatedDocsDC);
         }
       });

diff --git a/test/db.test.js b/test/db.test.js
@@ -1647,6 +1647,86 @@ describe('Database', function () {
       });
     });
 
+
+    describe("Callback signature", function () {
+
+      it("Regular update, multi false", function (done) {
+        d.insert({ a: 1 });
+        d.insert({ a: 2 });
+
+        // returnUpdatedDocs set to false
+        d.update({ a: 1 }, { $set: { b: 20 } }, {}, function (err, numAffected, affectedDocuments, upsert) {
+          assert.isNull(err);
+          numAffected.should.equal(1);
+          assert.isUndefined(affectedDocuments);
+          assert.isUndefined(upsert);
+
+          // returnUpdatedDocs set to true
+          d.update({ a: 1 }, { $set: { b: 21 } }, { returnUpdatedDocs: true }, function (err, numAffected, affectedDocuments, upsert) {
+            assert.isNull(err);
+            numAffected.should.equal(1);
+            affectedDocuments.a.should.equal(1);
+            affectedDocuments.b.should.equal(21);
+            assert.isUndefined(upsert);
+
+            done();
+          });
+        });
+      });
+
+      it("Regular update, multi true", function (done) {
+        d.insert({ a: 1 });
+        d.insert({ a: 2 });
+
+        // returnUpdatedDocs set to false
+        d.update({}, { $set: { b: 20 } }, { multi: true }, function (err, numAffected, affectedDocuments, upsert) {
+          assert.isNull(err);
+          numAffected.should.equal(2);
+          assert.isUndefined(affectedDocuments);
+          assert.isUndefined(upsert);
+
+          // returnUpdatedDocs set to true
+          d.update({}, { $set: { b: 21 } }, { multi: true, returnUpdatedDocs: true }, function (err, numAffected, affectedDocuments, upsert) {
+            assert.isNull(err);
+            numAffected.should.equal(2);
+            affectedDocuments.length.should.equal(2);
+            assert.isUndefined(upsert);
+
+            done();
+          });
+        });
+      });
+
+      it("Upsert", function (done) {
+        d.insert({ a: 1 });
+        d.insert({ a: 2 });
+
+        // Upsert flag not set
+        d.update({ a: 3 }, { $set: { b: 20 } }, {}, function (err, numAffected, affectedDocuments, upsert) {
+          assert.isNull(err);
+          numAffected.should.equal(0);
+          assert.isUndefined(affectedDocuments);
+          assert.isUndefined(upsert);
+
+          // Upsert flag set
+          d.update({ a: 3 }, { $set: { b: 21 } }, { upsert: true }, function (err, numAffected, affectedDocuments, upsert) {
+            assert.isNull(err);
+            numAffected.should.equal(1);
+            affectedDocuments.a.should.equal(3);
+            affectedDocuments.b.should.equal(21);
+            upsert.should.equal(true);
+
+            d.find({}, function (err, docs) {
+              docs.length.should.equal(3);
+              done();
+            });
+          });
+        });
+      });
+
+
+    });   // ==== End of 'Update - Callback signature' ==== //
+
   });   // ==== End of 'Update' ==== //