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-2859 Add search index management helpers #1311

Merged
merged 15 commits into from
Sep 19, 2023

Conversation

@qingyang-hu qingyang-hu force-pushed the godriver2859 branch 9 times, most recently from 935d275 to e6dbc69 Compare July 25, 2023 21:41
@qingyang-hu qingyang-hu force-pushed the godriver2859 branch 17 times, most recently from a65419d to 0a4ffa2 Compare August 6, 2023 20:37
@qingyang-hu qingyang-hu force-pushed the godriver2859 branch 4 times, most recently from a91f983 to ae7a0f5 Compare August 22, 2023 21:49
@qingyang-hu qingyang-hu temporarily deployed to api-report August 22, 2023 21:49 — with GitHub Actions Inactive
.evergreen/config.yml Show resolved Hide resolved
Makefile Outdated
@@ -164,6 +164,10 @@ evg-test-load-balancers:
go test $(BUILD_TAGS) ./mongo/integration -run TestLoadBalancerSupport -v -timeout $(TEST_TIMEOUT)s >> test.suite
go test $(BUILD_TAGS) ./mongo/integration/unified -run TestUnifiedSpec -v -timeout $(TEST_TIMEOUT)s >> test.suite

.PHONY: evg-test-search-index
evg-test-search-index:
go test ./mongo/integration -run TestSearchIndexProse -v -timeout $(TEST_TIMEOUT)s
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should append to the test.suite file so that the test output can be parsed by Evergreen.

Suggested change
go test ./mongo/integration -run TestSearchIndexProse -v -timeout $(TEST_TIMEOUT)s
go test ./mongo/integration -run TestSearchIndexProse -v -timeout $(TEST_TIMEOUT)s >> test.suite

//
// The opts parameter can be used to specify options for this operation (see the options.ListSearchIndexesOptions
// documentation).
func (siv SearchIndexView) List(ctx context.Context, name *string,
Copy link
Collaborator

@matthewdale matthewdale Sep 1, 2023

Choose a reason for hiding this comment

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

I'm a little worried about the usability problems that using string pointers introduces, especially because there is no Go syntax for declaring pointers to literal strings. We use that pattern in the WriteConcern type, but the negative usability impact seems greater than anticipated.

If we need to be able to distinguish between empty string and no value provided, would adding name to ListSearchIndexesOptions be a better option?

E.g. using string pointer:

coll.SearchIndexes().List(ctx, nil)

name := "myIndex"
coll.SearchIndexes().List(ctx, &name)

E.g. using ListSearchIndexOptions:

coll.SearchIndexes().List(ctx)

coll.SearchIndexes().List(ctx, options.ListSearchIndexes().Name("myIndex"))

Another consideration is that the $listSearchIndexes aggregation pipeline supports both "id" and "name filters. If we ever need to support the "id" filter in the future, we would need to add it somewhere. We could accept a filter interface{} parameter, similar to ListCollections. However, that would be less usable for a simple string filter because users would have to figure out how to assemble the filter document themselves. Also, the filter document format for ListCollections is very different than the filter document format for $listSearchIndexes.

E.g. using filter:

coll.SearchIndexes().List(ctx, nil)
coll.SearchIndexes().List(ctx, bson.D{{"name", "myIndex"}})

I don't think any of those are the obvious best choice. Which do you prefer?

bson/raw.go Outdated
Comment on lines 63 to 73
var relems []RawElement
if doc := bsoncore.Document(r); len(doc) > 0 {
elems, err := doc.Elements()
if err != nil {
return relems, err
}
relems = make([]RawElement, 0, len(elems))
for _, elem := range elems {
relems = append(relems, RawElement(elem))
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Optional: Consider inverting the if condition and returning early so you can un-indent the remaining code.

E.g.

func (r Raw) Elements() ([]RawElement, error) {
	doc := bsoncore.Document(r)
	if len(doc) == 0 {
		return nil, nil
	}

	elems, err := doc.Elements()
	if err != nil {
		return nil, err
	}

	relems := make([]RawElement, 0, len(elems))
	for _, elem := range elems {
		relems = append(relems, RawElement(elem))
	}

	return relems, nil
}

@prestonvasquez prestonvasquez changed the base branch from master to v1 September 13, 2023 18:04
Copy link
Collaborator

@matthewdale matthewdale left a comment

Choose a reason for hiding this comment

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

Looks good 👍

//
// The opts parameter can be used to specify options for this operation (see the options.ListSearchIndexesOptions
// documentation).
func (siv SearchIndexView) List(ctx context.Context, searchIdxOpts *options.SearchIndexesOptions, opts ...*options.ListSearchIndexesOptions) (*Cursor, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Optional: Long lines can be difficult to read. Consider wrapping them.

E.g.

func (siv SearchIndexView) List(
	ctx context.Context,
	searchIdxOpts *options.SearchIndexesOptions,
	opts ...*options.ListSearchIndexesOptions,
) (*Cursor, error) {

That suggestion applies to all long function signatures in this PR.

@qingyang-hu qingyang-hu merged commit f192906 into mongodb:v1 Sep 19, 2023
blink1073 pushed a commit to blink1073/mongo-go-driver that referenced this pull request Sep 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants