feature(aggregate): added support for 2 additional use-cases while ma… #316
Conversation
…intaining backwards-compatibility chore(package): bumped package minor version
| let index = _.findIndex(params.aggregation, (step) => !_.isEmpty(step.$match)); | ||
|
|
||
| if (index < 0) { | ||
| params.aggregation.unshift({ $match: cursorQuery }); |
There was a problem hiding this comment.
My understanding of the existing behavior is that if a $match step isn't included params.aggregation, this code will add $match as the first step. Is this behavior preserved in the refactor?
|
|
||
| params.aggregation.splice(index + 1, 0, { $sort }); | ||
| params.aggregation.splice(index + 2, 0, { $limit: params.limit + 1 }); | ||
| // assumes consecutive matches can/should be optimized by the server engine rather than client side |
There was a problem hiding this comment.
Can you test this on a large collection to be sure? I don't recall why we optimized this in client-side code (by merging the $match queries together), but we might have tested it and realized it was necessary.
| @@ -34,29 +35,28 @@ const config = require('./config'); | |||
| * -after {String} The _id to start querying the page. | |||
| * -before {String} The _id to start querying previous page. | |||
| * -options {Object} Aggregation options | |||
| * @param {(() => number) | undefined | null} getPipelineIndexFn Functor that determines where to insert the pagination-specific operations in the pipeline | |||
There was a problem hiding this comment.
Please add a unit test demonstrating a "real world" scenario for this parameter. My understanding from reading the code is that it's useful only if you have multiple $match stages in your aggregation. This parameter will allow you to pick where in the pipeline to insert the 3 stages required for pagination.
Please add unit tests that cover the newly added lines. We want to maintain near-100% unit test coverage in this module.
Changes Made
Added new optional param to aggregate function to support more use-cases
Potential Risks
The assumptions in usage scenario 1 are the same as for 2. In both cases, there's still room for user error, if the user does not consider the established assumptions adequately.
The 3rd scenario can lead to even more user errors since the user must be aware that he may be providing additional steps to the pipeline after the pagination operations and as such must take into consideration any prior data transformations. Ultimately, in this scenario, the user could even override the $sort assumption so only advanced users should use it.
Test Plan
The performance implications are directly related with the pipeline data transformation/summarization steps prior to the pagination steps and those that follow for both scenarios 2) and 3) respectively
Checklist
Sorry but at this time I don't have the time to add any additional meaningful tests for the new sample use cases. The changes are "minimal" and I'm convinced that added usefulness at this point suffice to justify this addition even though the additional tests could be helpful even if to explain the use cases better with practical examples.
I've attested though that all previous tests still succeed.