Fix aggregate cursor als#15094
Conversation
46fc7ed to
51e6b08
Compare
vkarpov15
left a comment
There was a problem hiding this comment.
I had a couple of minor style suggestions, but looks good overall.
lib/aggregate.js
Outdated
| */ | ||
|
|
||
| Aggregate.prototype._optionsForExec = function() { | ||
| const options = clone(this.options || {}); |
There was a problem hiding this comment.
clone() shouldn't be necessary here, would be ideal to avoid cloning for performance purposes
There was a problem hiding this comment.
The previous implementation also used clone(), so I retained it to ensure no existing functionality was unintentionally affected. However, I agree that removing clone() could improve performance. If it’s deemed unnecessary, I suggest calling the function directly in the constructor to initialize the options instead.
lib/cursor/aggregationCursor.js
Outdated
| function _init(model, c, agg) { | ||
| if (!model.collection.buffer) { | ||
| model.hooks.execPre('aggregate', agg, function() { | ||
| Object.assign(c.options, agg._optionsForExec()); |
There was a problem hiding this comment.
Similar to previous comments, is there any need to set c.options at all? Could just call aggregate(agg._pipeline, agg.optionsForExec())
There was a problem hiding this comment.
In QueryCursor, options is also set to a new object, so I followed the same approach here.
That said, QueryCursor uses the options object for some checks while AggregateCursor only uses it in _init, so it might make sense to remove it from AggregateCursor for now.
|
Removed the cloning in _optionsForExec and also sest the options before creating AggregationCursor in Aggregate.cursor(). Passed all tests with npm test. |
…s in AggregationCursor.
6ec8cd5 to
4b8a25e
Compare
test(transactions): add test case for #15094
Summary
Aggregate cursors didn't automatically add session if it was present in AsyncLocalStorage. Also the options weren't the same for exec and cursor so I added a function to make them the same just like in QueryCursor.
Examples