GODRIVER-3043 Use default write/read concerns in the index search commands.

[qingyang-hu]

GODRIVER-3043
GODRIVER-3122

Summary

• Clone a new session and remove the read/write concerns before using it in the search index commands.
• Add the prose test to verify that write/read concerns are not appended to Atlas Search Index commands.
• Fix the failed test case in the GODRIVER-3122.

Background & Motivation

The unified tests for GODRIVER-3043 have been added in GODRIVER-3074.

[mongodb-drivers-pr-bot]

API Change Report

./x/mongo/driver/operation

incompatible changes

(*CreateSearchIndexes).WriteConcern: removed
(*DropSearchIndex).WriteConcern: removed
(*UpdateSearchIndex).WriteConcern: removed

[prestonvasquez]

Can you add a comment explaining why we have to hardcode a timeout. Also, we could avoid a magic string using shell inline:

evg-test-search-index:
	go test ./mongo/integration -run TestSearchIndexProse -v -timeout $(shell echo "$$(( $(TEST_TIMEOUT) * 2))")s >> test.suite

[prestonvasquez]

Suggested change

	searchName := "test-search-index6"
	const searchName = "test-search-index-case6'"

Suggest constantizing and using the name specified in the prose test: "test-search-index-case6'"

[prestonvasquez]

It's peculiar all this data was missing from the operation. Was it just missed in the original implementation?

[qingyang-hu]

I think it will be better to align with the implementation in "create_indexes.go".

[matthewdale]

IIUC, the search index management server commands actually manage resources on an Apache Lucene cluster (and maybe some database configuration), so it's possible some of the previously omitted info was unnecessary. However, it's not clear what the impact of adding or removing those fields is.

I generally agree that passing the same info as CreateIndexes seems like a good idea, but should we test for that? Or if the info was truly unnecessary, should we continue to omit it?

Edit: To clarify, we shouldn't block merging the PR, but we should try to figure out whether we're missing test coverage for this type of behavior change that could be hiding other omissions.

[prestonvasquez]

Why not exhaust the cursor?

var doc bson.Raw
for cursor.Next(ctx) {
	name := cursor.Current.Lookup("name").StringValue()
	queryable := cursor.Current.Lookup("queryable").Boolean()
	if name == searchName && queryable {
		doc = cursor.Current
		break
	}
}

[qingyang-hu]

I break on negative cursor.Next to reduce the deep nesting block and simplify the exiting of the recurring List calls.

[prestonvasquez]

I mention doing this because this is what CXX does: https://github.com/kevinAlbs/mongo-cxx-driver/blob/fff7c2c71438f9f62c6b1a6ca5dee20f1c1749ac/src/mongocxx/test/search_index_view.cpp#L19

I think the current implementation is fine, though. IIUC in case 6 there would only ever be one doc on the cursor for view.List. Is that correct?

[qingyang-hu]

Correct.

[prestonvasquez]

LGTM!

[matthewdale]

Looks good! 👍