SERVER-32300 Aggregation shell helper should not modify options document #1219
Conversation
d-d-ddl
changed the title
d-d-ddl
changed the title
| // Probably a parallel array. | ||
| if (ErrorCodes::CannotIndexParallelArrays == e.code()) { | ||
| return Status(ErrorCodes::BadValue, "cannot sort with keys that are parallel arrays"); | ||
| return Status(ErrorCodes::BadValue, "cannot sort on multiple fields containing arrays"); |
There was a problem hiding this comment.
Please separate out this change into a separate pull request - that way the commit messages can be separate and it will be clear what commit fixed which ticket, etc.
| @@ -0,0 +1,19 @@ | |||
| // Core ServerSERVER-32300 Aggregation shell helper should not modify options document | |||
There was a problem hiding this comment.
Please use a full sentence here, and describe what we're testing - not just the title of the ticket. Something like "Test that the explain helper does not modify the options document passed to it. This test was designed to reproduce SERVER-32300".
| @@ -0,0 +1,19 @@ | |||
| // Core ServerSERVER-32300 Aggregation shell helper should not modify options document | |||
|
|
|||
| load("src/mongo/shell/explainable.js") | |||
There was a problem hiding this comment.
Before we commit this, we should remove this 'load'. It should be unnecessary once the fix is in - and our testing infrastructure will recompile everything before it runs the tests.
|
|
||
| load("src/mongo/shell/explainable.js") | ||
|
|
||
| let collation = {collation: {locale: "fr", backwards: true}}; |
There was a problem hiding this comment.
Use 'const' for variables which will not change - like here.
|
|
||
| // Computes correct result of the aggragation | ||
| // Computes an empty array | ||
| jsTestLog("desired output: " + tojson(db.collection.aggregate([], collation).toArray())); |
There was a problem hiding this comment.
Instead of just logging here, we should make assertions on the behavior. In this case, I would recommend inserting some data into a collection, then asserting that the results of an aggregation are the same before and after an explain. Something like:
const coll = db.explain_options;
coll.drop();
for (let i = 0; i < 10; ++i) {
assert.writeOK(coll.insert({_id: i}));
}
const firstResults = coll.aggregate([{$sort: {_id: 1}}], collation).toArray();
// Here we issue an explain in order to verify that 'collation' is not modified to include the explain flag. We do not care about the result of the explain.
assert.commandWorked(coll.explain().aggregate([], collation));
const secondResults = coll.aggregate([{$sort: {_id: 1}}], collation).toArray();
assert.eq(firstResults, secondResults);
src/mongo/shell/explainable.js
Outdated
| return this._collection.aggregate(pipeline, extraOptsCopy); | ||
| } else { | ||
| // The aggregate command requires a cursor field. | ||
| if (!extraOpts.hasOwnProperty("cursor")) { |
There was a problem hiding this comment.
Please use 'extraOptsCopy' throughout this whole function, in order to prevent a similar bug from creeping in in the future.
| "use strict"; | ||
|
|
||
| const coll = db.explain_options; | ||
| // Clear db |
There was a problem hiding this comment.
Please remove this comment - it doesn't say much that the 'drop' doesn't already say.
| assert.commandWorked(coll.explain().aggregate([], collation)); | ||
|
|
||
| const secondResults = coll.aggregate([{$sort: {_id: 1}}], collation).toArray(); | ||
| // Assert that the command worked. |
There was a problem hiding this comment.
Be a little more specific here, maybe "Assert that the results didn't change after using the explain helper."
| @@ -0,0 +1,24 @@ | |||
| // Test that the explain helper does not modify the options document passed to it. | |||
There was a problem hiding this comment.
I see you've used the name 'server32300.js' for this test. This is the old way of naming tests, we now prefer to provide some hint of what the test is doing in the name, I'd suggest renaming this to 'explain_helper_options.js' or something like that.
|
Thanks for the fix @Dandanlin0702, this bug was really annoying :) |
3 participants
No description provided.