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

[W-A-James]

Description

Remove parent parameter from AggregateOperation and parent reference from AggregationCursor and change any instantiation of these internal classes to match their new signature

What changed?

• src/change_stream.ts
• src/collection.ts
• src/cursor/aggregation_cursor.ts
• src/db.ts
• src/operations/aggregate.ts
• src/operations/count_documents.ts

[nbbeeken]

Suggested change

	this.target = ns.collection || DB_AGGREGATE_COLLECTION;
	this.target = ns.collection ?? DB_AGGREGATE_COLLECTION;

just a nit, optional

[W-A-James]

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

[nbbeeken]

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?

[W-A-James]

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

[dariakp]

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).

[nbbeeken]

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.

[dariakp]

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;)

[nbbeeken]

LGTM

[dariakp]

Just one small thing here

[dariakp]

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)

[dariakp]

(See my comment on the other thread)