-
-
Notifications
You must be signed in to change notification settings - Fork 58
Fir for Add sparse options for index #11
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.
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
base: master
Are you sure you want to change the base?
Changes from all commits
79f2fdc
f6e47e7
ce222c6
7e79705
7b0f4ad
f71fdc3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -122,7 +122,7 @@ vows.describe('mongodb').addBatch({ | |
return this.callback(err); | ||
} | ||
|
||
db.addIndex('event', 'event_title', 'title', false, this.callback); | ||
db.addIndex('event', { name: 'event_title', columns: 'title', unique: false, sparse: true }, this.callback); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here |
||
}.bind(this)); | ||
}, | ||
|
||
|
@@ -230,7 +230,7 @@ vows.describe('mongodb').addBatch({ | |
return this.callback(err); | ||
} | ||
|
||
db.addIndex('event', 'event_title', 'title', false, function(err, data) { | ||
db.addIndex('event', { name: 'event_title', columns: 'title', unique: false }, function(err, data) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Last thing missing here: You have deleted the old tests, but to actually cover the old and new functionality, the old and the new tests have to coexist, rather than one gets rewritten. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But these are internal calls made within the test case hence I changed them to use the updates style. I have also added a separate test case for addIndex for the previous style. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, this wasn't clear from the diff. Please don't adjust tests in the future and instead add directly new ones instead :) |
||
|
||
if(err) { | ||
return this.callback(err); | ||
|
@@ -299,7 +299,7 @@ vows.describe('mongodb').addBatch({ | |
return this.callback(err); | ||
} | ||
|
||
db.addIndex('event', 'event_title', 'title', false, function(err, data) { | ||
db.addIndex('event', { name: 'event_title', columns: 'title', unique: false }, function(err, data) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here |
||
|
||
if(err) { | ||
return this.callback(err); | ||
|
@@ -360,4 +360,51 @@ vows.describe('mongodb').addBatch({ | |
} | ||
} | ||
} | ||
}).export(module); | ||
}) | ||
.addBatch({ | ||
'addIndex-backward-compatibility': { | ||
topic: function() { | ||
db.createCollection('event', function(err, collection) { | ||
if(err) { | ||
return this.callback(err); | ||
} | ||
|
||
db.addIndex('event', 'event_title','title', false, this.callback); | ||
}.bind(this)); | ||
}, | ||
|
||
teardown: function() { | ||
db.dropCollection('event', this.callback); | ||
}, | ||
|
||
'preserves case': { | ||
topic: function() { | ||
db._getCollectionNames(this.callback); | ||
}, | ||
|
||
'of the functions original table': function(err, tables) { | ||
var index = 0; | ||
assert.isNotNull(tables); | ||
assert.equal(tables.length, 2); // Should be 2 b/c of the system collection | ||
|
||
if( tables[0].collectionName === 'system.indexes' ) | ||
index = 1; | ||
|
||
assert.equal(tables[index].collectionName, 'event'); | ||
} | ||
}, | ||
|
||
'has resulting index metadata': { | ||
topic: function() { | ||
db._getIndexes('event', this.callback); | ||
}, | ||
|
||
'with additional index': function(err, indexes) { | ||
assert.isDefined(indexes); | ||
assert.isNotNull(indexes); | ||
assert.include(indexes, 'event_title'); | ||
} | ||
} | ||
} | ||
}) | ||
.export(module); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would be the benefit over
indexOptions.sparse
here? As what I can see here, it is just a simple object being passed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indexOptions.sparse
can only be true or false, this option will be considered only if indexOption contains a property sparse. Asif(false)
will be falseThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, lets be a bit more specific: What is the default value of sparse? If it is
false
, then it can just be omitted. If the default is true, well then please check forindexOptions.sparse === false
instead.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sparse
itself is optional parameter for addIndex, hence we should use the value as set by user. Hence the presence of sparse is checked before passing them tocreateIndex
call.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But
sparse
itself does only accepttrue
orfalse
itself. So I would rather check for the non default value in a type safe manner instead of doinghasOwnProperty
.https://docs.mongodb.com/manual/core/index-sparse/#create-a-sparse-index
As
false
is the default, we can just safely omit it in the case of the value being non true.