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.

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

Jump to bottom

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

Merged
merged 6 commits into from Jul 28, 2021
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
dda834d
fix: remove parent parameter from AggregateOperation and parent refer…
W-A-James Jul 26, 2021
5b4bc3f
refactor: Change AggregateOperation construtor parameter ordering
W-A-James Jul 26, 2021
895e447
fix: change coalesce nullish to boolean or
W-A-James Jul 26, 2021
f3d21de
style: remove commented-out code
W-A-James Jul 26, 2021
d54fff4
style: make check for undefined and empty string more verbose
W-A-James Jul 26, 2021
e59f480
refactor: Restore nullish check and add comment for clarity
W-A-James Jul 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 5 additions & 9 deletions src/change_stream.ts
View file
Open in desktop
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
View file
Open in desktop
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
14 changes: 4 additions & 10 deletions src/cursor/aggregation_cursor.ts
View file
Open in desktop
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,21 @@ 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[kParent] = parent;
nbbeeken marked this conversation as resolved.
Show resolved Hide resolved
this[kPipeline] = pipeline;
this[kOptions] = options;
}
Expand All @@ -59,7 +53,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 +64,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 +91,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
View file
Open in desktop
Expand Up @@ -301,7 +301,6 @@ export class Db {
}

return new AggregationCursor(
this,
getTopology(this),
this.s.namespace,
pipeline,
Expand Down
18 changes: 5 additions & 13 deletions src/operations/aggregate.ts
View file
Open in desktop
@@ -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,11 @@ 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;
this.target = ns.collection || DB_AGGREGATE_COLLECTION;
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
this.target = ns.collection || DB_AGGREGATE_COLLECTION;
this.target = ns.collection ?? DB_AGGREGATE_COLLECTION;

just a nit, optional

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed that because when ns.collection is set to the empty string, we set the target to the empty string instead of DB_AGGREGATE_COLLECTION, which leads to a few test failures

Copy link
Contributor

Choose a reason for hiding this comment

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

ah Fascinating, I feel like that makes this worthy of more explicit code.

This but with curly braces

if (ns.collection === undefined || ns.collection === '') this.target = DB_AGGREGATE_COLLECTION;
else this.target = ns.collection;

Or we can bring back the ternary, Or leave it as is with a comment, // covers undefined and empty string, It's just easy to over look if the code itself or a comment doesn't call it out, what do you think?

Copy link
Contributor Author

@W-A-James W-A-James Jul 26, 2021 •
edited

Choose a reason for hiding this comment

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

yeah that sounds good. Makes sense to have it be more explicit so the next person to see it and thinks to change it won't have to go bughunting like I did

Copy link
Contributor

@dariakp dariakp Jul 27, 2021 •
edited

Choose a reason for hiding this comment

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

Lol I just noticed this after doing my review... I think a comment would be better; explicit undefined checks are somewhat weird and puts into question why we aren't validating for the other excluded cases... if we're concerned about nullish values but only these ones, why aren't we throwing errors on the other kinds? Edit: just to clarify, what I'm trying to convey is that reading that code with just that check there communicates to me that we are ok with assigning null to the target, but not undefined, and that, logically, feels wrong; but if we aren't ok with assigning null to the target, then I would expect some sort of validation that throws an error if we somehow do pass in null, and that validation is missing here, which makes the intent of the code seemingly inconsistent (it's important to remember that typescript types don't guarantee that at runtime only those types will actually be passed in those inputs, so "manual" validation is still valuable wherever such validation is important).

Copy link
Contributor

Choose a reason for hiding this comment

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

if (typeof ns.collection === 'string' && ns.collection !== '') this.target = ns.collection
else this.target = DB_AGGREGATE_COLLECTION;

Ok one more code suggestion ^
But also reverting to the ternary and using the same nullish that was there before along with a comment is a good solution to me as well. Your point is correct we should make sure that we are actually filtering for our intention and not leaving a gap that might suggest null is okay here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd recommend against the potential behavior change unless truly necessary; and if we do make that behavior change, we'd want to throw on ns.collection not being null/undefined so that we can guard against unexpected input sneaking through (one of those runtime errors)... feels like overkill for this ticket though, hence easier to just keep the original behavior (this.target = ns.collection || DB_AGGREGATE_COLLECTION;)


this.pipeline = pipeline;

Expand Down
2 changes: 1 addition & 1 deletion src/operations/count_documents.ts
View file
Open in desktop
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