TOOLS-2645: Check for duplicate index keys after converting legacy in… #282
Conversation
mongorestore/options.go
Outdated
| TempRolesCollOption = "--tempRolesColl" | ||
| BulkBufferSizeOption = "--batchSize" | ||
| FixDottedHashedIndexesOption = "--fixDottedHashIndex" | ||
| FixDuplicatedLegacyIndexesOption = "--fixDuplicatedLegacyIndexes" |
There was a problem hiding this comment.
I may be recalling an outdated discussion, but I thought when we last discussed we were not going to add a new flag and instead make it part of the default --convertLegacyIndexes behavior
There was a problem hiding this comment.
I may remember incorrectly, @tfogo Do you remember if we would need a flag or not?
There was a problem hiding this comment.
I think we agreed on not having a flag but adding a note to the documentation.
There was a problem hiding this comment.
ah, cool. I will remove the flag then. Thanks!
mongorestore/restore.go
Outdated
| @@ -306,17 +306,27 @@ func (restore *MongoRestore) RestoreIntent(intent *intents.Intent) Result { | |||
| return result | |||
| } | |||
|
|
|||
| func (restore *MongoRestore) convertLegacyIndexes(indexes []IndexDocument, ns string) { | |||
| func (restore *MongoRestore) convertLegacyIndexes(indexes []IndexDocument, ns string) []IndexDocument { | |||
| indexKeys := make(map[string]bool) // A set contains all the unique index's json strings | |||
There was a problem hiding this comment.
small nit, but the go idiom is to use map[string]struct{} for sets
| indexString := bsonutil.CreateExtJSONString(index.Key) | ||
| if _, ok := indexKeys[indexString]; ok { | ||
| // skip duplicated indexes | ||
| continue |
There was a problem hiding this comment.
I think we want to log whenever we skip because we should notify the user that there won't be an index with that particular name in the destination. Might be important if the user is using the name as an index hint. We should also tell them what the name of the index that does exist is.
There was a problem hiding this comment.
Make sense, added the log
mongorestore/mongorestore_test.go
Outdated
| restore, err := getRestoreWithArgs(args...) | ||
| So(err, ShouldBeNil) | ||
|
|
||
| Convey("Creating index with invalid option and --convertLegacyIndexes should succeed", func() { |
There was a problem hiding this comment.
This description should be updated.
There was a problem hiding this comment.
Good catch. fixed!
…dex definitions
This PR depends on https://github.com/mongodb/mongo-tools-common/pull/45/files . I will revendor after the MTC PR gets merged.
Here I introduced flag fixDuplicatedLegacyIndexes. When enabled, after legacy indexes get converted, the indexes with duplicated index key will be skipped.