From dda834d1f4c9b3cf99b032dd8139152b198fdeb1 Mon Sep 17 00:00:00 2001 From: Warren James Date: Mon, 26 Jul 2021 14:29:41 -0400 Subject: [PATCH 1/6] fix: remove parent parameter from AggregateOperation and parent reference from AggregationCursor --- src/change_stream.ts | 14 +++++--------- src/collection.ts | 1 - src/cursor/aggregation_cursor.ts | 15 +++++---------- src/db.ts | 1 - src/operations/aggregate.ts | 18 +++++------------- src/operations/count_documents.ts | 2 +- 6 files changed, 16 insertions(+), 35 deletions(-) diff --git a/src/change_stream.ts b/src/change_stream.ts index b260b4d80a..895d23916f 100644 --- a/src/change_stream.ts +++ b/src/change_stream.ts @@ -489,15 +489,11 @@ export class ChangeStreamCursor extends Abs } _initialize(session: ClientSession, callback: Callback): void { - const aggregateOperation = new AggregateOperation( - { s: { namespace: this.namespace } }, - this.pipeline, - { - ...this.cursorOptions, - ...this.options, - session - } - ); + const aggregateOperation = new AggregateOperation(this.pipeline, this.namespace, { + ...this.cursorOptions, + ...this.options, + session + }); executeOperation(this.topology, aggregateOperation, (err, response) => { if (err || response == null) { diff --git a/src/collection.ts b/src/collection.ts index f9ec68a439..f7d1840c3b 100644 --- a/src/collection.ts +++ b/src/collection.ts @@ -1387,7 +1387,6 @@ export class Collection { } return new AggregationCursor( - this as TODO_NODE_3286, getTopology(this), this.s.namespace, pipeline, diff --git a/src/cursor/aggregation_cursor.ts b/src/cursor/aggregation_cursor.ts index 12f50c3155..17a9638c2e 100644 --- a/src/cursor/aggregation_cursor.ts +++ b/src/cursor/aggregation_cursor.ts @@ -7,7 +7,6 @@ import type { Sort } from '../sort'; import type { Topology } from '../sdam/topology'; import type { Callback, MongoDBNamespace } from '../utils'; import type { ClientSession } from '../sessions'; -import type { OperationParent } from '../operations/command'; import type { AbstractCursorOptions } from './abstract_cursor'; import type { ExplainVerbosityLike } from '../explain'; import type { Projection } from '../mongo_types'; @@ -15,8 +14,6 @@ import type { Projection } from '../mongo_types'; /** @public */ export interface AggregationCursorOptions extends AbstractCursorOptions, AggregateOptions {} -/** @internal */ -const kParent = Symbol('parent'); /** @internal */ const kPipeline = Symbol('pipeline'); /** @internal */ @@ -30,8 +27,6 @@ const kOptions = Symbol('options'); * @public */ export class AggregationCursor extends AbstractCursor { - /** @internal */ - [kParent]: OperationParent; // TODO: NODE-2883 /** @internal */ [kPipeline]: Document[]; /** @internal */ @@ -39,7 +34,7 @@ export class AggregationCursor extends AbstractCursor extends AbstractCursor extends AbstractCursor { const clonedOptions = mergeOptions({}, this[kOptions]); delete clonedOptions.session; - return new AggregationCursor(this[kParent], this.topology, this.namespace, this[kPipeline], { + return new AggregationCursor(this.topology, this.namespace, this[kPipeline], { ...clonedOptions }); } @@ -70,7 +65,7 @@ export class AggregationCursor extends AbstractCursor): void { - const aggregateOperation = new AggregateOperation(this[kParent], this[kPipeline], { + const aggregateOperation = new AggregateOperation(this[kPipeline], this.namespace, { ...this[kOptions], ...this.cursorOptions, session @@ -97,7 +92,7 @@ export class AggregationCursor extends AbstractCursor extends CommandOperation { pipeline: Document[]; hasWriteStage: boolean; - constructor(parent: OperationParent, pipeline: Document[], options?: AggregateOptions) { - super(parent, options); + constructor(pipeline: Document[], ns?: MongoDBNamespace, options?: AggregateOptions) { + super(undefined, Object.assign({ dbName: ns?.db }, options)); this.options = options ?? {}; - this.target = - parent.s.namespace && parent.s.namespace.collection - ? parent.s.namespace.collection - : DB_AGGREGATE_COLLECTION; + this.target = ns && ns.collection ? ns.collection : DB_AGGREGATE_COLLECTION; this.pipeline = pipeline; diff --git a/src/operations/count_documents.ts b/src/operations/count_documents.ts index d3bce65fbc..d6db635782 100644 --- a/src/operations/count_documents.ts +++ b/src/operations/count_documents.ts @@ -29,7 +29,7 @@ export class CountDocumentsOperation extends AggregateOperation { pipeline.push({ $group: { _id: 1, n: { $sum: 1 } } }); - super(collection, pipeline, options); + super(pipeline, collection.s.namespace, options); } execute(server: Server, session: ClientSession, callback: Callback): void { From 5b4bc3fab2dc989e0dc9646fc1501f4f87a6ab5b Mon Sep 17 00:00:00 2001 From: Warren James Date: Mon, 26 Jul 2021 15:42:48 -0400 Subject: [PATCH 2/6] refactor: Change AggregateOperation construtor parameter ordering --- src/change_stream.ts | 2 +- src/cursor/aggregation_cursor.ts | 5 ++--- src/operations/aggregate.ts | 6 +++--- src/operations/count_documents.ts | 2 +- 4 files changed, 7 insertions(+), 8 deletions(-) diff --git a/src/change_stream.ts b/src/change_stream.ts index 895d23916f..aab273b06c 100644 --- a/src/change_stream.ts +++ b/src/change_stream.ts @@ -489,7 +489,7 @@ export class ChangeStreamCursor extends Abs } _initialize(session: ClientSession, callback: Callback): void { - const aggregateOperation = new AggregateOperation(this.pipeline, this.namespace, { + const aggregateOperation = new AggregateOperation(this.namespace, this.pipeline, { ...this.cursorOptions, ...this.options, session diff --git a/src/cursor/aggregation_cursor.ts b/src/cursor/aggregation_cursor.ts index 17a9638c2e..e617430ebc 100644 --- a/src/cursor/aggregation_cursor.ts +++ b/src/cursor/aggregation_cursor.ts @@ -34,7 +34,6 @@ export class AggregationCursor extends AbstractCursor extends AbstractCursor): void { - const aggregateOperation = new AggregateOperation(this[kPipeline], this.namespace, { + const aggregateOperation = new AggregateOperation(this.namespace, this[kPipeline], { ...this[kOptions], ...this.cursorOptions, session @@ -92,7 +91,7 @@ export class AggregationCursor extends AbstractCursor extends CommandOperation { pipeline: Document[]; hasWriteStage: boolean; - constructor(pipeline: Document[], ns?: MongoDBNamespace, options?: AggregateOptions) { - super(undefined, Object.assign({ dbName: ns?.db }, options)); + constructor(ns: MongoDBNamespace, pipeline: Document[], options?: AggregateOptions) { + super(undefined, { dbName: ns?.db, ...options }); this.options = options ?? {}; - this.target = ns && ns.collection ? ns.collection : DB_AGGREGATE_COLLECTION; + this.target = ns?.collection ?? DB_AGGREGATE_COLLECTION; this.pipeline = pipeline; diff --git a/src/operations/count_documents.ts b/src/operations/count_documents.ts index d6db635782..27b65ec3db 100644 --- a/src/operations/count_documents.ts +++ b/src/operations/count_documents.ts @@ -29,7 +29,7 @@ export class CountDocumentsOperation extends AggregateOperation { pipeline.push({ $group: { _id: 1, n: { $sum: 1 } } }); - super(pipeline, collection.s.namespace, options); + super(collection.s.namespace, pipeline, options); } execute(server: Server, session: ClientSession, callback: Callback): void { From 895e447ada4187970375705d7d0187b427e428d0 Mon Sep 17 00:00:00 2001 From: Warren James Date: Mon, 26 Jul 2021 16:29:06 -0400 Subject: [PATCH 3/6] fix: change coalesce nullish to boolean or --- src/operations/aggregate.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/operations/aggregate.ts b/src/operations/aggregate.ts index a403c0803f..e85cde29b3 100644 --- a/src/operations/aggregate.ts +++ b/src/operations/aggregate.ts @@ -43,10 +43,10 @@ export class AggregateOperation extends CommandOperation { hasWriteStage: boolean; constructor(ns: MongoDBNamespace, pipeline: Document[], options?: AggregateOptions) { - super(undefined, { dbName: ns?.db, ...options }); + super(undefined, { ...options, dbName: ns.db }); this.options = options ?? {}; - this.target = ns?.collection ?? DB_AGGREGATE_COLLECTION; + this.target = ns.collection || DB_AGGREGATE_COLLECTION; this.pipeline = pipeline; From f3d21de2958f9890f695453b79f0d19cb77017e5 Mon Sep 17 00:00:00 2001 From: Warren James Date: Mon, 26 Jul 2021 16:38:43 -0400 Subject: [PATCH 4/6] style: remove commented-out code --- src/cursor/aggregation_cursor.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/cursor/aggregation_cursor.ts b/src/cursor/aggregation_cursor.ts index e617430ebc..5b1e3fa119 100644 --- a/src/cursor/aggregation_cursor.ts +++ b/src/cursor/aggregation_cursor.ts @@ -41,7 +41,6 @@ export class AggregationCursor extends AbstractCursor Date: Mon, 26 Jul 2021 17:07:20 -0400 Subject: [PATCH 5/6] style: make check for undefined and empty string more verbose --- src/operations/aggregate.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/operations/aggregate.ts b/src/operations/aggregate.ts index e85cde29b3..0fcacaff21 100644 --- a/src/operations/aggregate.ts +++ b/src/operations/aggregate.ts @@ -46,7 +46,12 @@ export class AggregateOperation extends CommandOperation { super(undefined, { ...options, dbName: ns.db }); this.options = options ?? {}; - this.target = ns.collection || DB_AGGREGATE_COLLECTION; + + if (ns.collection === undefined || ns.collection === '') { + this.target = DB_AGGREGATE_COLLECTION; + } else { + this.target = ns.collection; + } this.pipeline = pipeline; From e59f48065c180b610352976099aec1c458336c0b Mon Sep 17 00:00:00 2001 From: Warren James Date: Wed, 28 Jul 2021 16:19:21 -0400 Subject: [PATCH 6/6] refactor: Restore nullish check and add comment for clarity --- src/operations/aggregate.ts | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/operations/aggregate.ts b/src/operations/aggregate.ts index 0fcacaff21..7d09495eeb 100644 --- a/src/operations/aggregate.ts +++ b/src/operations/aggregate.ts @@ -47,11 +47,8 @@ export class AggregateOperation extends CommandOperation { this.options = options ?? {}; - if (ns.collection === undefined || ns.collection === '') { - this.target = DB_AGGREGATE_COLLECTION; - } else { - this.target = ns.collection; - } + // Covers when ns.collection is null, undefined or the empty string, use DB_AGGREGATE_COLLECTION + this.target = ns.collection || DB_AGGREGATE_COLLECTION; this.pipeline = pipeline;