refactor(NODE-1812): Deprecate returnOriginal in favor of returnDocum…

…ent option in findOneAndReplace and findOneAndUpdate methods

docs/reference/content/reference/ecmascriptnext/crud.md

@@ -213,7 +213,7 @@ const dbName = 'myproject';
 
     // Modify and return the modified document
     r = await col.findOneAndUpdate({a:1}, {$set: {b: 1}}, {
-      returnOriginal: false,
+      returnDocument: 'after',
       sort: [['a',1]],
       upsert: true
     });

docs/reference/content/tutorials/crud.md

@@ -354,7 +354,7 @@ methods, the operation takes a write lock for the duration of the operation in o
 
     // Modify and return the modified document
     col.findOneAndUpdate({a:1}, {$set: {b: 1}}, {
-        returnOriginal: false
+        returnDocument: 'after'
       , sort: [[a,1]]
       , upsert: true
     }, function(err, r) {
@@ -382,7 +382,7 @@ methods, the operation takes a write lock for the duration of the operation in o
 
     // Modify and return the modified document
     r = await col.findOneAndUpdate({a:1}, {$set: {b: 1}}, {
-        returnOriginal: false
+        returnDocument: 'after'
       , sort: [[a,1]]
       , upsert: true
     });
@@ -410,7 +410,7 @@ The ``findOneAndUpdate`` method also accepts a third argument which can be an op
 | `upsert` | (Boolean, default:false) | Perform an upsert operation. |
 | `sort` | (Object, default:null) | Sort for find operation. |
 | `projection` | (Object, default:null) | Projection for returned result |
-| `returnOriginal` | (Boolean, default:true) | Set to false if you want to return the modified object rather than the original. Ignored for remove. |
+| `returnDocument` | (String, 'before' \|\| 'after', default:'before') | Set to 'after' if you want to return the modified document rather than the original. Ignored for remove. |
 
 The ``findOneAndDelete`` function is designed to help remove a document.
 

lib/collection.js

@@ -1655,7 +1655,7 @@ Collection.prototype.stats = function(options, callback) {
 
 /**
  * @typedef {Object} Collection~findAndModifyWriteOpResult
- * @property {object} value Document returned from the `findAndModify` command. If no documents were found, `value` will be `null` by default (`returnOriginal: true`), even if a document was upserted; if `returnOriginal` was false, the upserted document will be returned in that case.
+ * @property {object} value Document returned from the `findAndModify` command. If no documents were found, `value` will be `null` by default even if a document was upserted unless `returnDocument` is specified as `'after'`, in which case the upserted document will be returned.
  * @property {object} lastErrorObject The raw lastErrorObject returned from the command. See {@link https://docs.mongodb.com/manual/reference/command/findAndModify/index.html#lasterrorobject|findAndModify command documentation}.
  * @property {Number} ok Is 1 if the command executed correctly.
  */
@@ -1716,7 +1716,8 @@ Collection.prototype.findOneAndDelete = function(filter, options, callback) {
  * @param {object} [options.projection] Limits the fields to return for all matching documents.
  * @param {object} [options.sort] Determines which document the operation modifies if the query selects multiple documents.
  * @param {boolean} [options.upsert=false] Upsert the document if it does not exist.
- * @param {boolean} [options.returnOriginal=true] When false, returns the updated document rather than the original. The default is true.
+ * @param {'before'|'after'} [options.returnDocument='before'] When set to `'after'`, returns the updated document rather than the original. The default is `'before'`.
+ * @param {boolean} [options.returnOriginal=true] **Deprecated** Use `options.returnDocument` instead.
  * @param {boolean} [options.checkKeys=false] If true, will throw if bson documents start with `$` or include a `.` in any key value
  * @param {boolean} [options.serializeFunctions=false] Serialize functions on any object.
  * @param {boolean} [options.ignoreUndefined=false] Specify if the BSON serializer should ignore undefined fields.
@@ -1725,22 +1726,29 @@ Collection.prototype.findOneAndDelete = function(filter, options, callback) {
  * @param {Collection~findAndModifyCallback} [callback] The collection result callback
  * @return {Promise<Collection~findAndModifyWriteOpResultObject>} returns Promise if no callback passed
  */
-Collection.prototype.findOneAndReplace = function(filter, replacement, options, callback) {
-  if (typeof options === 'function') (callback = options), (options = {});
-  options = options || {};
+Collection.prototype.findOneAndReplace = deprecateOptions(
+  {
+    name: 'collection.findOneAndReplace',
+    deprecatedOptions: ['returnOriginal'],
+    optionsIndex: 2
+  },
+  function(filter, replacement, options, callback) {
+    if (typeof options === 'function') (callback = options), (options = {});
+    options = options || {};
 
-  // Add ignoreUndefined
-  if (this.s.options.ignoreUndefined) {
-    options = Object.assign({}, options);
-    options.ignoreUndefined = this.s.options.ignoreUndefined;
-  }
+    // Add ignoreUndefined
+    if (this.s.options.ignoreUndefined) {
+      options = Object.assign({}, options);
+      options.ignoreUndefined = this.s.options.ignoreUndefined;
+    }
 
-  return executeOperation(
-    this.s.topology,
-    new FindOneAndReplaceOperation(this, filter, replacement, options),
-    callback
-  );
-};
+    return executeOperation(
+      this.s.topology,
+      new FindOneAndReplaceOperation(this, filter, replacement, options),
+      callback
+    );
+  }
+);
 
 /**
  * Find a document and update it in one atomic operation. Requires a write lock for the duration of the operation.
@@ -1757,7 +1765,8 @@ Collection.prototype.findOneAndReplace = function(filter, replacement, options,
  * @param {object} [options.projection] Limits the fields to return for all matching documents.
  * @param {object} [options.sort] Determines which document the operation modifies if the query selects multiple documents.
  * @param {boolean} [options.upsert=false] Upsert the document if it does not exist.
- * @param {boolean} [options.returnOriginal=true] When false, returns the updated document rather than the original. The default is true.
+ * @param {'before'|'after'} [options.returnDocument='before'] When set to `'after'`, returns the updated document rather than the original. The default is `'before'`.
+ * @param {boolean} [options.returnOriginal=true] **Deprecated** Use `options.returnDocument` instead.
  * @param {boolean} [options.checkKeys=false] If true, will throw if bson documents start with `$` or include a `.` in any key value
  * @param {boolean} [options.serializeFunctions=false] Serialize functions on any object.
  * @param {boolean} [options.ignoreUndefined=false] Specify if the BSON serializer should ignore undefined fields.
@@ -1766,22 +1775,29 @@ Collection.prototype.findOneAndReplace = function(filter, replacement, options,
  * @param {Collection~findAndModifyCallback} [callback] The collection result callback
  * @return {Promise<Collection~findAndModifyWriteOpResultObject>} returns Promise if no callback passed
  */
-Collection.prototype.findOneAndUpdate = function(filter, update, options, callback) {
-  if (typeof options === 'function') (callback = options), (options = {});
-  options = options || {};
+Collection.prototype.findOneAndUpdate = deprecateOptions(
+  {
+    name: 'collection.findOneAndUpdate',
+    deprecatedOptions: ['returnOriginal'],
+    optionsIndex: 2
+  },
+  function(filter, update, options, callback) {
+    if (typeof options === 'function') (callback = options), (options = {});
+    options = options || {};
 
-  // Add ignoreUndefined
-  if (this.s.options.ignoreUndefined) {
-    options = Object.assign({}, options);
-    options.ignoreUndefined = this.s.options.ignoreUndefined;
-  }
+    // Add ignoreUndefined
+    if (this.s.options.ignoreUndefined) {
+      options = Object.assign({}, options);
+      options.ignoreUndefined = this.s.options.ignoreUndefined;
+    }
 
-  return executeOperation(
-    this.s.topology,
-    new FindOneAndUpdateOperation(this, filter, update, options),
-    callback
-  );
-};
+    return executeOperation(
+      this.s.topology,
+      new FindOneAndUpdateOperation(this, filter, update, options),
+      callback
+    );
+  }
+);
 
 /**
  * Find and update a document.

lib/operations/find_one_and_replace.js

@@ -1,16 +1,22 @@
 'use strict';
 
+const MongoError = require('../core').MongoError;
 const FindAndModifyOperation = require('./find_and_modify');
 const hasAtomicOperators = require('../utils').hasAtomicOperators;
 
 class FindOneAndReplaceOperation extends FindAndModifyOperation {
   constructor(collection, filter, replacement, options) {
+    if ('returnDocument' in options && 'returnOriginal' in options) {
+      throw new MongoError(
+        'findOneAndReplace option returnOriginal is deprecated in favor of returnDocument and cannot be combined'
+      );
+    }
     // Final options
     const finalOptions = Object.assign({}, options);
     finalOptions.fields = options.projection;
     finalOptions.update = true;
-    finalOptions.new = options.returnOriginal !== void 0 ? !options.returnOriginal : false;
-    finalOptions.upsert = options.upsert !== void 0 ? !!options.upsert : false;
+    finalOptions.new = options.returnDocument === 'after' || options.returnOriginal === false;
+    finalOptions.upsert = options.upsert === true;
 
     if (filter == null || typeof filter !== 'object') {
       throw new TypeError('Filter parameter must be an object');

lib/operations/find_one_and_update.js

@@ -1,17 +1,22 @@
 'use strict';
 
+const MongoError = require('../core').MongoError;
 const FindAndModifyOperation = require('./find_and_modify');
 const hasAtomicOperators = require('../utils').hasAtomicOperators;
 
 class FindOneAndUpdateOperation extends FindAndModifyOperation {
   constructor(collection, filter, update, options) {
+    if ('returnDocument' in options && 'returnOriginal' in options) {
+      throw new MongoError(
+        'findOneAndUpdate option returnOriginal is deprecated in favor of returnDocument and cannot be combined'
+      );
+    }
     // Final options
     const finalOptions = Object.assign({}, options);
     finalOptions.fields = options.projection;
     finalOptions.update = true;
-    finalOptions.new =
-      typeof options.returnOriginal === 'boolean' ? !options.returnOriginal : false;
-    finalOptions.upsert = typeof options.upsert === 'boolean' ? options.upsert : false;
+    finalOptions.new = options.returnDocument === 'after' || options.returnOriginal === false;
+    finalOptions.upsert = options.upsert === true;
 
     if (filter == null || typeof filter !== 'object') {
       throw new TypeError('Filter parameter must be an object');

test/functional/crud_api.test.js

@@ -766,7 +766,7 @@ describe('CRUD API', function() {
               {
                 projection: { b: 1, c: 1 },
                 sort: { a: 1 },
-                returnOriginal: false,
+                returnOriginal: false, // keeping the deprecated option for compatibility testing since returnDocument is covered in spec tests
                 upsert: true
               },
               function(err, r) {
@@ -795,7 +795,7 @@ describe('CRUD API', function() {
               {
                 projection: { b: 1, d: 1 },
                 sort: { a: 1 },
-                returnOriginal: false,
+                returnOriginal: false, // keeping the deprecated option for compatibility testing since returnDocument is covered in spec tests
                 upsert: true
               },
               function(err, r) {

test/functional/crud_spec.test.js

@@ -359,13 +359,12 @@ describe('CRUD spec', function() {
     const second = args.update || args.replacement;
     const options = Object.assign({}, args);
     if (options.returnDocument) {
-      options.returnOriginal = options.returnDocument === 'After' ? false : true;
+      options.returnDocument = options.returnDocument.toLowerCase();
     }
 
     delete options.filter;
     delete options.update;
     delete options.replacement;
-    delete options.returnDocument;
 
     const opName = scenarioTest.operation.name;
     const findPromise =

test/functional/operation_example.test.js

@@ -9269,7 +9269,7 @@ describe('Operation Examples', function() {
             {
               projection: { b: 1, c: 1 },
               sort: { a: 1 },
-              returnOriginal: false,
+              returnDocument: 'after',
               upsert: true
             },
             function(err, r) {
@@ -9327,7 +9327,7 @@ describe('Operation Examples', function() {
             {
               projection: { b: 1, d: 1 },
               sort: { a: 1 },
-              returnOriginal: false,
+              returnDocument: 'after',
               upsert: true
             },
             function(err, r) {

test/functional/operation_generators_example.test.js

@@ -6385,7 +6385,7 @@ describe('Operation (Generators)', function() {
           {
             projection: { b: 1, c: 1 },
             sort: { a: 1 },
-            returnOriginal: false,
+            returnDocument: 'after',
             upsert: true
           }
         );
@@ -6442,7 +6442,7 @@ describe('Operation (Generators)', function() {
           {
             projection: { b: 1, d: 1 },
             sort: { a: 1 },
-            returnOriginal: false,
+            returnDocument: 'after',
             upsert: true
           }
         );

test/functional/operation_promises_example.test.js

@@ -6953,7 +6953,7 @@ describe('Operation (Promises)', function() {
               {
                 projection: { b: 1, c: 1 },
                 sort: { a: 1 },
-                returnOriginal: false,
+                returnDocument: 'after',
                 upsert: true
               }
             )
@@ -7011,7 +7011,7 @@ describe('Operation (Promises)', function() {
               {
                 projection: { b: 1, d: 1 },
                 sort: { a: 1 },
-                returnOriginal: false,
+                returnDocument: 'after',
                 upsert: true
               }
             );

test/functional/retryable_writes.test.js

@@ -131,7 +131,7 @@ function generateArguments(test) {
         options.upsert = test.operation.arguments[arg];
       } else if (arg === 'returnDocument') {
         const returnDocument = test.operation.arguments[arg];
-        options.returnOriginal = returnDocument === 'Before';
+        options.returnDocument = returnDocument.toLowerCase();
       } else {
         args.push(test.operation.arguments[arg]);
       }

test/functional/spec-runner/index.js

@@ -661,7 +661,7 @@ function testOperation(operation, obj, context, options) {
           }
 
           if (key === 'returnDocument') {
-            opOptions.returnOriginal = operation.arguments[key] === 'Before' ? true : false;
+            opOptions.returnDocument = operation.arguments[key].toLowerCase();
             return;
           }
 

test/unit/collection.test.js

@@ -16,17 +16,48 @@ class MockTopology extends EventEmitter {
 }
 
 describe('Collection', function() {
-  /**
-   * @ignore
-   */
-  it('should not allow atomic operators for findOneAndReplace', {
-    metadata: { requires: { topology: 'single' } },
-    test: function() {
-      const db = new Db('fakeDb', new MockTopology());
-      const collection = db.collection('test');
-      expect(() => {
-        collection.findOneAndReplace({ a: 1 }, { $set: { a: 14 } });
-      }).to.throw(/must not contain atomic operators/);
-    }
+  describe('findOneAndReplace()', function() {
+    it('should throw on atomic operators in replacement document', {
+      metadata: { requires: { topology: 'single' } },
+      test: function() {
+        const db = new Db('fakeDb', new MockTopology());
+        const collection = db.collection('test');
+        expect(() => {
+          collection.findOneAndReplace({ a: 1 }, { $set: { a: 14 } });
+        }).to.throw(/must not contain atomic operators/);
+      }
+    });
+
+    it('should throw if returnOriginal is specified with returnDocument as an option', {
+      metadata: { requires: { topology: 'single' } },
+      test: function() {
+        const db = new Db('fakeDb', new MockTopology());
+        const collection = db.collection('test');
+        expect(() => {
+          collection.findOneAndReplace(
+            { a: 1 },
+            { b: 2 },
+            { returnOriginal: true, returnDocument: 'before' }
+          );
+        }).to.throw(/returnOriginal is deprecated in favor of returnDocument/);
+      }
+    });
+  });
+
+  describe('findOneAndUpdate()', function() {
+    it('should throw if returnOriginal is specified with returnDocument as an option', {
+      metadata: { requires: { topology: 'single' } },
+      test: function() {
+        const db = new Db('fakeDb', new MockTopology());
+        const collection = db.collection('test');
+        expect(() => {
+          collection.findOneAndUpdate(
+            { a: 1 },
+            { $set: { a: 14 } },
+            { returnOriginal: true, returnDocument: 'before' }
+          );
+        }).to.throw(/returnOriginal is deprecated in favor of returnDocument/);
+      }
+    });
   });
 });