Skip to content
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.

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

fix(NODE-2883): Aggregate Operation should not require parent parameter #2918

Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
14 changes: 5 additions & 9 deletions src/change_stream.ts
Original file line number Diff line number Diff line change
Expand Up @@ -489,15 +489,11 @@ export class ChangeStreamCursor<TSchema extends Document = Document> extends Abs
}

_initialize(session: ClientSession, callback: Callback<ExecutionResult>): void {
const aggregateOperation = new AggregateOperation(
{ s: { namespace: this.namespace } },
this.pipeline,
{
...this.cursorOptions,
...this.options,
session
}
);
const aggregateOperation = new AggregateOperation(this.namespace, this.pipeline, {
...this.cursorOptions,
...this.options,
session
});

executeOperation(this.topology, aggregateOperation, (err, response) => {
if (err || response == null) {
Expand Down
1 change: 0 additions & 1 deletion src/collection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1387,7 +1387,6 @@ export class Collection<TSchema extends Document = Document> {
}

return new AggregationCursor(
this as TODO_NODE_3286,
getTopology(this),
this.s.namespace,
pipeline,
Expand Down
13 changes: 3 additions & 10 deletions src/cursor/aggregation_cursor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,13 @@ 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';

/** @public */
export interface AggregationCursorOptions extends AbstractCursorOptions, AggregateOptions {}

/** @internal */
const kParent = Symbol('parent');
/** @internal */
const kPipeline = Symbol('pipeline');
/** @internal */
Expand All @@ -30,24 +27,20 @@ const kOptions = Symbol('options');
* @public
*/
export class AggregationCursor<TSchema = Document> extends AbstractCursor<TSchema> {
/** @internal */
[kParent]: OperationParent; // TODO: NODE-2883
/** @internal */
[kPipeline]: Document[];
/** @internal */
[kOptions]: AggregateOptions;

/** @internal */
constructor(
parent: OperationParent,
topology: Topology,
namespace: MongoDBNamespace,
pipeline: Document[] = [],
options: AggregateOptions = {}
) {
super(topology, namespace, options);

this[kParent] = parent;
this[kPipeline] = pipeline;
this[kOptions] = options;
}
Expand All @@ -59,7 +52,7 @@ export class AggregationCursor<TSchema = Document> extends AbstractCursor<TSchem
clone(): AggregationCursor<TSchema> {
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
});
}
Expand All @@ -70,7 +63,7 @@ export class AggregationCursor<TSchema = Document> extends AbstractCursor<TSchem

/** @internal */
_initialize(session: ClientSession | undefined, callback: Callback<ExecutionResult>): void {
const aggregateOperation = new AggregateOperation(this[kParent], this[kPipeline], {
const aggregateOperation = new AggregateOperation(this.namespace, this[kPipeline], {
...this[kOptions],
...this.cursorOptions,
session
Expand All @@ -97,7 +90,7 @@ export class AggregationCursor<TSchema = Document> extends AbstractCursor<TSchem

return executeOperation(
this.topology,
new AggregateOperation(this[kParent], this[kPipeline], {
new AggregateOperation(this.namespace, this[kPipeline], {
...this[kOptions], // NOTE: order matters here, we may need to refine this
...this.cursorOptions,
explain: verbosity
Expand Down
1 change: 0 additions & 1 deletion src/db.ts
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,6 @@ export class Db {
}

return new AggregationCursor(
this,
getTopology(this),
this.s.namespace,
pipeline,
Expand Down
23 changes: 10 additions & 13 deletions src/operations/aggregate.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,7 @@
import {
CommandOperation,
CommandOperationOptions,
OperationParent,
CollationOptions
} from './command';
import { CommandOperation, CommandOperationOptions, CollationOptions } from './command';
import { ReadPreference } from '../read_preference';
import { MongoInvalidArgumentError } from '../error';
import { maxWireVersion } from '../utils';
import { maxWireVersion, MongoDBNamespace } from '../utils';
import { Aspect, defineAspects, Hint } from './operation';
import type { Callback } from '../utils';
import type { Document } from '../bson';
Expand Down Expand Up @@ -47,14 +42,16 @@ export class AggregateOperation<T = Document> extends CommandOperation<T> {
pipeline: Document[];
hasWriteStage: boolean;

constructor(parent: OperationParent, pipeline: Document[], options?: AggregateOptions) {
super(parent, options);
constructor(ns: MongoDBNamespace, pipeline: Document[], options?: AggregateOptions) {
super(undefined, { ...options, dbName: ns.db });

this.options = options ?? {};
this.target =
parent.s.namespace && parent.s.namespace.collection
? parent.s.namespace.collection
: DB_AGGREGATE_COLLECTION;

if (ns.collection === undefined || ns.collection === '') {
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 can capture the desired outcome with just this.target = ns.collection || DB_AGGREGATE_COLLECTION (it would preserve the previous logic, and it's different from existing logic in that it would still assign the target to null or 0 if those happened to get passed in as values; however, I don't see why we would want to start doing that)

Copy link
Contributor

Choose a reason for hiding this comment

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

(See my comment on the other thread)

this.target = DB_AGGREGATE_COLLECTION;
} else {
this.target = ns.collection;
}

this.pipeline = pipeline;

Expand Down
2 changes: 1 addition & 1 deletion src/operations/count_documents.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ export class CountDocumentsOperation extends AggregateOperation<number> {

pipeline.push({ $group: { _id: 1, n: { $sum: 1 } } });

super(collection, pipeline, options);
super(collection.s.namespace, pipeline, options);
}

execute(server: Server, session: ClientSession, callback: Callback<number>): void {
Expand Down