Skip to content

refactor(NODE-1812): Deprecate returnOriginal in favor of returnDocument #2808

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 17, 2021
Merged

refactor(NODE-1812): Deprecate returnOriginal in favor of returnDocument #2808

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/reference/content/reference/ecmascriptnext/crud.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
});
Expand Down
6 changes: 3 additions & 3 deletions docs/reference/content/tutorials/crud.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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
});
Expand Down Expand Up @@ -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.

Expand Down
78 changes: 47 additions & 31 deletions lib/collection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*/
Expand Down Expand Up @@ -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.
Expand All @@ -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.
Expand All @@ -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.
Expand All @@ -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.
Expand Down
10 changes: 8 additions & 2 deletions lib/operations/find_one_and_replace.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
throw new MongoError(
throw new TypeError(

I think this would be more appropriate than MongoError

Copy link
Contributor Author

@dariakp dariakp May 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually disagree with this use of TypeError, because a TypeError to a developer is usually an indication that the wrong data type was passed in, which in this case isn't true of the individual options. If we consider the entirety of the options object as being of a particular type (defined by the allowed combinations of keys), TypeError might technically be applicable, but I think it's a bit odd to do type validation on a dictionary as a whole instead of individual members (particularly when we don't actually validate any of the other options at the same time). What we really need is a validation error type as distinct from a server or other sort of error; unfortunately, the driver currently silently overwrites options in the other cases of deprecation and actual option validation is sparse and examining the code base, most of the other instances of non-type-related option validation use MongoError, hence its use here for consistency.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other errors like this throw TypeErrors; I think it would be good to be consistent, though I don't feel strongly about this kind of errors being TypeErrors and would be OK with a new error type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah those definitely should not be TypeErrors. And aside from the read preference file, other places do use MongoError:

node-mongodb-native/src/utils.ts

Lines 55 to 79 in 1cdc8a8

export function checkCollectionName(collectionName: string): void {
if ('string' !== typeof collectionName) {
throw new MongoError('collection name must be a String');
}
if (!collectionName || collectionName.indexOf('..') !== -1) {
throw new MongoError('collection names cannot be empty');
}
if (
collectionName.indexOf('$') !== -1 &&
collectionName.match(/((^\$cmd)|(oplog\.\$main))/) == null
) {
throw new MongoError("collection names must not contain '$'");
}
if (collectionName.match(/^\.|\.$/) != null) {
throw new MongoError("collection names must not start or end with '.'");
}
// Validate that we are not passing 0x00 in the collection name
if (collectionName.indexOf('\x00') !== -1) {
throw new MongoError('collection names cannot contain a null character');
}
}

https://github.com/mongodb/node-mongodb-native/blob/4.0/src/operations/aggregate.ts#L76
https://github.com/mongodb/node-mongodb-native/blob/4.0/src/db.ts#L756
with the cleanest example using the MongoParseError:
https://github.com/mongodb/node-mongodb-native/blob/4.0/src/connection_string.ts#L136
right now though the MongoParseError is limited to the connection string parsing and I'm not sure we want to expand its use elsewhere

'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');
Expand Down
11 changes: 8 additions & 3 deletions lib/operations/find_one_and_update.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

'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');
Expand Down
4 changes: 2 additions & 2 deletions test/functional/crud_api.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down
3 changes: 1 addition & 2 deletions test/functional/crud_spec.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand Down
4 changes: 2 additions & 2 deletions test/functional/operation_example.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down
4 changes: 2 additions & 2 deletions test/functional/operation_generators_example.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6385,7 +6385,7 @@ describe('Operation (Generators)', function() {
{
projection: { b: 1, c: 1 },
sort: { a: 1 },
returnOriginal: false,
returnDocument: 'after',
upsert: true
}
);
Expand Down Expand Up @@ -6442,7 +6442,7 @@ describe('Operation (Generators)', function() {
{
projection: { b: 1, d: 1 },
sort: { a: 1 },
returnOriginal: false,
returnDocument: 'after',
upsert: true
}
);
Expand Down
4 changes: 2 additions & 2 deletions test/functional/operation_promises_example.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6953,7 +6953,7 @@ describe('Operation (Promises)', function() {
{
projection: { b: 1, c: 1 },
sort: { a: 1 },
returnOriginal: false,
returnDocument: 'after',
upsert: true
}
)
Expand Down Expand Up @@ -7011,7 +7011,7 @@ describe('Operation (Promises)', function() {
{
projection: { b: 1, d: 1 },
sort: { a: 1 },
returnOriginal: false,
returnDocument: 'after',
upsert: true
}
);
Expand Down
2 changes: 1 addition & 1 deletion test/functional/retryable_writes.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]);
}
Expand Down
2 changes: 1 addition & 1 deletion test/functional/spec-runner/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
55 changes: 43 additions & 12 deletions test/unit/collection.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: false, returnDocument: 'after' }
);
}).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/);
}
});
});
});