Skip to content

refactor: consolidate options inheritance logic #2614

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 13 commits into from
Nov 9, 2020
Merged

refactor: consolidate options inheritance logic #2614

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
5832e23
use resolveInheritedOptions in collection and db
HanaPearlman Nov 3, 2020
373fa7c
consolidate resolve logic
HanaPearlman Nov 4, 2020
a9d4de5
handle NO_INHERIT_OPTIONS
HanaPearlman Nov 4, 2020
5427509
undo changes to bulk
HanaPearlman Nov 4, 2020
2199194
clean up
HanaPearlman Nov 4, 2020
ce026fc
bug fix
HanaPearlman Nov 4, 2020
80eec57
bug fixes for mapReduce and inheriting with transactions
HanaPearlman Nov 5, 2020
756b5ca
group bug fix
HanaPearlman Nov 5, 2020
7a4ed82
clean up
HanaPearlman Nov 5, 2020
1785609
rename resolve function and style fixes
HanaPearlman Nov 6, 2020
8f5fb8b
slightly rearrange logic in command
HanaPearlman Nov 6, 2020
34d8bba
respond to comments
HanaPearlman Nov 6, 2020
7d9c436
add remarks for cmds that dont inherit options from parent
HanaPearlman Nov 9, 2020
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
129 changes: 74 additions & 55 deletions src/collection.ts
View file
Open in desktop

Large diffs are not rendered by default.

123 changes: 54 additions & 69 deletions src/db.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,13 @@
import { deprecate } from 'util';
import { emitDeprecatedOptionWarning, Callback } from './utils';
import {
emitDeprecatedOptionWarning,
Callback,
resolveOptions,
filterOptions,
deprecateOptions,
MongoDBNamespace,
getTopology
} from './utils';
import { loadAdmin } from './dynamic_loaders';
import { AggregationCursor, CommandCursor } from './cursor';
import { ObjectId, Code, Document, BSONSerializeOptions, resolveBSONOptions } from './bson';
Expand All @@ -11,13 +19,6 @@ import * as CONSTANTS from './constants';
import { WriteConcern, WriteConcernOptions } from './write_concern';
import { ReadConcern } from './read_concern';
import { Logger, LoggerOptions } from './logger';
import {
filterOptions,
mergeOptionsAndWriteConcern,
deprecateOptions,
MongoDBNamespace,
getTopology
} from './utils';
import { AggregateOperation, AggregateOptions } from './operations/aggregate';
import { AddUserOperation, AddUserOptions } from './operations/add_user';
import { CollectionsOperation } from './operations/collections';
Expand Down Expand Up @@ -254,19 +255,20 @@ export class Db implements OperationParent {
callback?: Callback<Collection>
): Promise<Collection> | void {
if (typeof options === 'function') (callback = options), (options = {});
options = options || {};
options.readConcern = ReadConcern.fromOptions(options) ?? this.readConcern;

return executeOperation(
getTopology(this),
new CreateCollectionOperation(this, name, options),
new CreateCollectionOperation(this, name, resolveOptions(this, options)),
callback
);
}

/**
* Execute a command
*
* @remarks
* This command does not inherit options from the MongoClient.
*
* @param command - The command to run
* @param options - Optional settings for the command
* @param callback - An optional callback, a Promise will be returned if none is provided
Expand All @@ -281,11 +283,11 @@ export class Db implements OperationParent {
callback?: Callback<Document>
): Promise<Document> | void {
if (typeof options === 'function') (callback = options), (options = {});
options = options || {};

// Intentionally, we do not inherit options from parent for this operation.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should add this to the function's docs for the ops that do this. Particularly with BSON options the return value could be unexpectedly different.

return executeOperation(
getTopology(this),
new RunCommandOperation(this, command, options),
new RunCommandOperation(this, command, options ?? {}),
callback
);
}
Expand All @@ -307,8 +309,7 @@ export class Db implements OperationParent {
throw new TypeError('`options` parameter must not be function');
}

options = options || {};

options = resolveOptions(this, options);
const cursor = new AggregationCursor(
getTopology(this),
new AggregateOperation(this, pipeline, options),
Expand Down Expand Up @@ -341,21 +342,10 @@ export class Db implements OperationParent {
callback?: Callback<Collection>
): Collection | void {
if (typeof options === 'function') (callback = options), (options = {});
options = Object.assign({}, options);

// If we have not set a collection level readConcern set the db level one
options.readConcern = ReadConcern.fromOptions(options) ?? this.readConcern;

// Merge in all needed options and ensure correct writeConcern merging from db level
const finalOptions = mergeOptionsAndWriteConcern(
options,
this.s.options ?? {},
collectionKeys,
true
) as CollectionOptions;
const finalOptions = resolveOptions(this, options);

// Execute
if (finalOptions == null || !finalOptions.strict) {
if (!finalOptions.strict) {
try {
const collection = new Collection(this, name, finalOptions);
if (callback) callback(undefined, collection);
Expand Down Expand Up @@ -414,9 +404,11 @@ export class Db implements OperationParent {
callback?: Callback<Document>
): Promise<Document> | void {
if (typeof options === 'function') (callback = options), (options = {});
options = options || {};

return executeOperation(getTopology(this), new DbStatsOperation(this, options), callback);
return executeOperation(
getTopology(this),
new DbStatsOperation(this, resolveOptions(this, options)),
callback
);
}

/**
Expand All @@ -427,7 +419,7 @@ export class Db implements OperationParent {
*/
listCollections(filter?: Document, options?: ListCollectionsOptions): CommandCursor {
filter = filter || {};
options = options || {};
options = resolveOptions(this, options);

return new CommandCursor(
getTopology(this),
Expand All @@ -439,6 +431,9 @@ export class Db implements OperationParent {
/**
* Rename a collection.
*
* @remarks
* This operation does not inherit options from the MongoClient.
*
* @param fromCollection - Name of current collection to rename
* @param toCollection - New name of of the collection
* @param options - Optional settings for the command
Expand Down Expand Up @@ -468,7 +463,9 @@ export class Db implements OperationParent {
callback?: Callback<Collection>
): Promise<Collection> | void {
if (typeof options === 'function') (callback = options), (options = {});
options = Object.assign({}, options, { readPreference: ReadPreference.PRIMARY });

// Intentionally, we do not inherit options from parent for this operation.
options = { ...options, readPreference: ReadPreference.PRIMARY };

// Add return new collection
options.new_collection = true;
Expand Down Expand Up @@ -497,11 +494,10 @@ export class Db implements OperationParent {
callback?: Callback<boolean>
): Promise<boolean> | void {
if (typeof options === 'function') (callback = options), (options = {});
options = options || {};

return executeOperation(
getTopology(this),
new DropCollectionOperation(this, name, options),
new DropCollectionOperation(this, name, resolveOptions(this, options)),
callback
);
}
Expand All @@ -521,9 +517,12 @@ export class Db implements OperationParent {
callback?: Callback<boolean>
): Promise<boolean> | void {
if (typeof options === 'function') (callback = options), (options = {});
options = options || {};

return executeOperation(getTopology(this), new DropDatabaseOperation(this, options), callback);
return executeOperation(
getTopology(this),
new DropDatabaseOperation(this, resolveOptions(this, options)),
callback
);
}

/**
Expand All @@ -541,14 +540,20 @@ export class Db implements OperationParent {
callback?: Callback<Collection[]>
): Promise<Collection[]> | void {
if (typeof options === 'function') (callback = options), (options = {});
options = options || {};

return executeOperation(getTopology(this), new CollectionsOperation(this, options), callback);
return executeOperation(
getTopology(this),
new CollectionsOperation(this, resolveOptions(this, options)),
callback
);
}

/**
* Runs a command on the database as admin.
*
* @remarks
* This command does not inherit options from the MongoClient.
*
* @param command - The command to run
* @param options - Optional settings for the command
* @param callback - An optional callback, a Promise will be returned if none is provided
Expand All @@ -567,11 +572,11 @@ export class Db implements OperationParent {
callback?: Callback<void>
): Promise<void> | void {
if (typeof options === 'function') (callback = options), (options = {});
options = options || {};

// Intentionally, we do not inherit options from parent for this operation.
return executeOperation(
getTopology(this),
new RunAdminCommandOperation(this, command, options),
new RunAdminCommandOperation(this, command, options ?? {}),
callback
);
}
Expand Down Expand Up @@ -604,11 +609,10 @@ export class Db implements OperationParent {
callback?: Callback<Document>
): Promise<Document> | void {
if (typeof options === 'function') (callback = options), (options = {});
options = options ? Object.assign({}, options) : {};

return executeOperation(
getTopology(this),
new CreateIndexOperation(this, name, indexSpec, options),
new CreateIndexOperation(this, name, indexSpec, resolveOptions(this, options)),
callback
);
}
Expand Down Expand Up @@ -652,10 +656,9 @@ export class Db implements OperationParent {
if (typeof options === 'function') (callback = options), (options = {});
}

options = options || {};
return executeOperation(
getTopology(this),
new AddUserOperation(this, username, password, options),
new AddUserOperation(this, username, password, resolveOptions(this, options)),
callback
);
}
Expand All @@ -677,11 +680,10 @@ export class Db implements OperationParent {
callback?: Callback<boolean>
): Promise<boolean> | void {
if (typeof options === 'function') (callback = options), (options = {});
options = options || {};

return executeOperation(
getTopology(this),
new RemoveUserOperation(this, username, options),
new RemoveUserOperation(this, username, resolveOptions(this, options)),
callback
);
}
Expand Down Expand Up @@ -710,11 +712,10 @@ export class Db implements OperationParent {
callback?: Callback<ProfilingLevel>
): Promise<ProfilingLevel> | void {
if (typeof options === 'function') (callback = options), (options = {});
options = options || {};

return executeOperation(
getTopology(this),
new SetProfilingLevelOperation(this, level, options),
new SetProfilingLevelOperation(this, level, resolveOptions(this, options)),
callback
);
}
Expand All @@ -734,11 +735,10 @@ export class Db implements OperationParent {
callback?: Callback<string>
): Promise<string> | void {
if (typeof options === 'function') (callback = options), (options = {});
options = options || {};

return executeOperation(
getTopology(this),
new ProfilingLevelOperation(this, options),
new ProfilingLevelOperation(this, resolveOptions(this, options)),
callback
);
}
Expand All @@ -764,11 +764,10 @@ export class Db implements OperationParent {
callback?: Callback<Document>
): Promise<Document> | void {
if (typeof options === 'function') (callback = options), (options = {});
options = options || {};

return executeOperation(
getTopology(this),
new IndexInformationOperation(this, name, options),
new IndexInformationOperation(this, name, resolveOptions(this, options)),
callback
);
}
Expand Down Expand Up @@ -835,11 +834,10 @@ export class Db implements OperationParent {
callback?: Callback<Document>
): Promise<Document> | void {
if (typeof options === 'function') (callback = options), (options = {});
options = options || {};

return executeOperation(
getTopology(this),
new EvalOperation(this, code, parameters, options),
new EvalOperation(this, code, parameters, resolveOptions(this, options)),
callback
);
}
Expand Down Expand Up @@ -873,11 +871,10 @@ export class Db implements OperationParent {
callback?: Callback<Document>
): Promise<Document> | void {
if (typeof options === 'function') (callback = options), (options = {});
options = options || {};

return executeOperation(
getTopology(this),
new EnsureIndexOperation(this, name, fieldOrSpec, options),
new EnsureIndexOperation(this, name, fieldOrSpec, resolveOptions(this, options)),
callback
);
}
Expand Down Expand Up @@ -905,18 +902,6 @@ export class Db implements OperationParent {
}
}

const collectionKeys = [
'pkFactory',
'readPreference',
'serializeFunctions',
'strict',
'readConcern',
'ignoreUndefined',
'promoteValues',
'promoteBuffers',
'promoteLongs'
];

Db.prototype.createCollection = deprecateOptions(
{
name: 'Db.createCollection',
Expand Down
20 changes: 5 additions & 15 deletions src/operations/command.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,12 +73,13 @@ export abstract class CommandOperation<
: new MongoDBNamespace('admin', '$cmd');
}

const propertyProvider = this.hasAspect(Aspect.NO_INHERIT_OPTIONS) ? undefined : parent;
this.readPreference = this.hasAspect(Aspect.WRITE_OPERATION)
? ReadPreference.primary
: ReadPreference.resolve(propertyProvider, this.options);
this.readConcern = resolveReadConcern(propertyProvider, this.options);
this.writeConcern = resolveWriteConcern(propertyProvider, this.options);
: ReadPreference.fromOptions(options) ?? ReadPreference.primary;
this.readConcern = ReadConcern.fromOptions(options);
this.writeConcern = WriteConcern.fromOptions(options);
this.bsonOptions = resolveBSONOptions(options);

this.explain = false;
this.fullResponse =
options && typeof options.fullResponse === 'boolean' ? options.fullResponse : false;
Expand All @@ -91,9 +92,6 @@ export abstract class CommandOperation<
if (parent && parent.logger) {
this.logger = parent.logger;
}

// Assign BSON serialize options to OperationBase, preferring options over parent options.
this.bsonOptions = resolveBSONOptions(options, parent);
}

abstract execute(server: Server, callback: Callback<TResult>): void;
Expand Down Expand Up @@ -149,11 +147,3 @@ export abstract class CommandOperation<
);
}
}

function resolveWriteConcern(parent: OperationParent | undefined, options: any) {
return WriteConcern.fromOptions(options) || parent?.writeConcern;
}

function resolveReadConcern(parent: OperationParent | undefined, options: any) {
return ReadConcern.fromOptions(options) || parent?.readConcern;
}
10 changes: 8 additions & 2 deletions src/operations/create_collection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,19 @@ const ILLEGAL_COMMAND_FIELDS = new Set([
'j',
'fsync',
'autoIndexId',
'serializeFunctions',
'pkFactory',
'raw',
'readPreference',
'session',
'readConcern',
'writeConcern'
'writeConcern',
'raw',
'fieldsAsRaw',
'promoteLongs',
'promoteValues',
'promoteBuffers',
'serializeFunctions',
'ignoreUndefined'
]);

/** @public */
Expand Down
Loading