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

[dariakp]

Description

NODE-1812

Deprecated returnOriginal in favor of returnDocument option in order to conform with shared drivers spec for findOneAndReplace() and findOneAndUpdate() collection methods.

[nbbeeken]

LGTM!

[emadum]

One small request on the type of error thrown, other than that LGTM 👍

[emadum]

Suggested change

	throw new MongoError(
	throw new TypeError(

I think this would be more appropriate than MongoError

[dariakp]

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.

[emadum]

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.

[dariakp]

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

[emadum]

Same here