Skip to content
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

GODRIVER-2701 Implement a variant of DropIndex that gets keys rather than name #1683

Merged
merged 24 commits into from
Jul 17, 2024

Conversation

timothy-kim-mongo
Copy link
Contributor

@timothy-kim-mongo timothy-kim-mongo commented Jun 20, 2024

GODRIVER-2701

Summary

The changes in this pull-request answers the ticket GODRIVER-2701 of implements a variant of DropIndex that searches for the index using the key rather than the name of the index. This creates an additional method that allows users to drop index with the freedom of either a name or key.

Background & Motivation

The motivation behind this is to be consistent with the functionality of the MongoDB db.collection.dropIndex() function which allows dropping the index with either a key or name. This is an optional part of the spec - https://github.com/mongodb/specifications/blob/master/source/index-management/index-management.md#standard-api.

@timothy-kim-mongo timothy-kim-mongo self-assigned this Jun 20, 2024
@timothy-kim-mongo
Copy link
Contributor Author

Note: 56850c7 and before have an implementation of DropOne method taking in a generic of either the key or name which could be considered for 2.0.

@mongodb-drivers-pr-bot mongodb-drivers-pr-bot bot added the priority-3-low Low Priority PR for Review label Jun 20, 2024
Copy link
Contributor

mongodb-drivers-pr-bot bot commented Jun 20, 2024

API Change Report

./mongo

compatible changes

IndexView.DropWithKey: added

./x/mongo/driver/operation

incompatible changes

(*DropIndexes).Index: changed from func(string) *DropIndexes to func(any) *DropIndexes
NewDropIndexes: changed from func(string) *DropIndexes to func(any) *DropIndexes

@qingyang-hu qingyang-hu requested review from prestonvasquez and removed request for blink1073 June 21, 2024 13:27
mongo/index_view.go Outdated Show resolved Hide resolved
mongo/index_view.go Outdated Show resolved Hide resolved
mongo/index_view.go Outdated Show resolved Hide resolved
mongo/index_view.go Outdated Show resolved Hide resolved
mongo/integration/main.go Outdated Show resolved Hide resolved
mongo/integration/index_view_test.go Outdated Show resolved Hide resolved
mongo/index_view.go Outdated Show resolved Hide resolved
mongo/integration/index_view_test.go Show resolved Hide resolved
mongo/index_view.go Outdated Show resolved Hide resolved
mongo/integration/index_view_test.go Outdated Show resolved Hide resolved
prestonvasquez
prestonvasquez previously approved these changes Jul 16, 2024
Copy link
Collaborator

@prestonvasquez prestonvasquez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

To fix the failings tests, suggest verifying that the indexes were created before dropping by key. You can accomplish this using the verifyIndexExists helper function:

for _, name := range indexNames {
	verifyIndexExists(mt, iv, index{
		Key:  test.index,
		Name: name,
	})
}

You will also need to type the +/- 1 index values as int32, e.g. 1 -> int32(1). See here for the suggested changes.

@timothy-kim-mongo
Copy link
Contributor Author

LGTM!

To fix the failings tests, suggest verifying that the indexes were created before dropping by key. You can accomplish this using the verifyIndexExists helper function:

for _, name := range indexNames {
	verifyIndexExists(mt, iv, index{
		Key:  test.index,
		Name: name,
	})
}

You will also need to type the +/- 1 index values as int32, e.g. 1 -> int32(1). See here for the suggested changes.

It seems like the failed tests for the other tests in older version of RHEL are continuing to happen. I will try to investigate this.

Copy link
Collaborator

@prestonvasquez prestonvasquez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@timothy-kim-mongo timothy-kim-mongo merged commit 45f9059 into mongodb:v1 Jul 17, 2024
31 of 33 checks passed
@timothy-kim-mongo timothy-kim-mongo deleted the GODRIVER-2701 branch July 17, 2024 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority-3-low Low Priority PR for Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants