Skip to content

Conversation

@qingyang-hu
Copy link
Collaborator

@qingyang-hu qingyang-hu commented Nov 26, 2025

GODRIVER-3704

Summary

  • Fix search index failure on empty "Options".
  • Fix "test-search-index".

Background & Motivation

@github-actions github-actions bot added the review-priority-normal Medium Priority PR for Review: within 1 business day label Nov 26, 2025
@mongodb-drivers-pr-bot
Copy link
Contributor

mongodb-drivers-pr-bot bot commented Nov 26, 2025

🧪 Performance Results

Commit SHA: 0508d4b

There were no significant changes to the performance to report for version 693370dd89f67600071024af.

For a comprehensive view of all microbenchmark results for this PR's commit, please check out the Evergreen perf task for this patch.

@mongodb-drivers-pr-bot
Copy link
Contributor

API Change Report

No changes found!

@qingyang-hu qingyang-hu force-pushed the godriver3704 branch 5 times, most recently from 1c96085 to ee65456 Compare November 26, 2025 23:07
Comment on lines -29 to -33
assume-test-secrets-ec2-role:
- command: ec2.assume_role
params:
role_arn: ${aws_test_secrets_role}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Duplicate of L175

@qingyang-hu qingyang-hu marked this pull request as ready for review November 26, 2025 23:55
@qingyang-hu qingyang-hu requested a review from a team as a code owner November 26, 2025 23:55
Copilot AI review requested due to automatic review settings November 26, 2025 23:55
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a bug where search index creation failed when SearchIndexModel.Options is nil. The fix ensures that the document element is always started before checking for optional fields.

Key changes:

  • Moved AppendDocumentElementStart call outside the Options != nil check to ensure proper BSON document construction
  • Added integration test to verify search index creation with empty options
  • Standardized environment variable naming from TEST_INDEX_URI to SEARCH_INDEX_URI

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
mongo/search_index_view.go Fixed bug by moving AppendDocumentElementStart before the Options != nil check, ensuring proper BSON document structure regardless of Options value
internal/integration/search_index_test.go Added new integration test to verify search index creation with empty Options
internal/integration/search_index_prose_test.go Updated environment variable name from TEST_INDEX_URI to SEARCH_INDEX_URI for consistency
Taskfile.yml Updated test runner to execute both TestSearchIndex and TestSearchIndexProse tests
.evergreen/config.yml Updated environment variable naming to SEARCH_INDEX_URI and removed duplicate assume-test-secrets-ec2-role function definition

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@qingyang-hu qingyang-hu marked this pull request as draft December 1, 2025 14:54
@qingyang-hu qingyang-hu marked this pull request as ready for review December 1, 2025 16:26

evg-test-search-index:
# Use the long timeout to wait for the responses from the server.
- go test ./internal/integration -run TestSearchIndexProse -v -timeout {{.LONG_TEST_TIMEOUT}}s >> test.suite
Copy link
Member

@prestonvasquez prestonvasquez Dec 1, 2025

Choose a reason for hiding this comment

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

[blocking] We should define TestSearchIndexProse explicitly rather than relying on regex. Also note that the prose tests are being skipped since TEST_SEARCH_INDEX no longer is set.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TestSearchIndexProse need to be manually triggered under test-search-index because it's time consuming.

working_dir: src/go.mongodb.org/mongo-driver
shell: bash
script: |-
echo "SEARCH_INDEX_URI: ${MONGODB_URI}" > atlas-expansion.yml
Copy link
Member

@prestonvasquez prestonvasquez Dec 1, 2025

Choose a reason for hiding this comment

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

[blocking] The EG changes seem to be trying to avoid using TEST_SEARCH_INDEX: "${MONGODB_URI}". If that's the case, we should keep the existing behavior for simplicity and roll TestSearchIndex into TestSearchIndexProse.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TestSearchIndexProse only runs when SEARCH_INDEX_URI is set. This test requires being manually triggered here because it's time consuming. However, it was corrupted since 78b0f61

Copy link
Member

Choose a reason for hiding this comment

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

It seems that TestSearchIndexProse passes under the current TEST_SEARCH_INDEX paradigm, given the example you shared. So why do we need to add the SEARCH_INDEX_URI changes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The current codebase completely skips TestSearchIndexProse because MONGODB_URI is not set even in the test-search-index. The patch here is to set MONGODB_URI in the atlas env.

I also changed the value name from TEST_SEARCH_INDEX to SEARCH_INDEX_URI to avoid the misunderstandings in 78b0f61.

"go.mongodb.org/mongo-driver/v2/x/bsonx/bsoncore"
)

func TestSearchIndex(t *testing.T) {
Copy link
Member

@prestonvasquez prestonvasquez Dec 1, 2025

Choose a reason for hiding this comment

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

[Blocking] We should roll this test into TestSearchIndexProse and open a DRIVERS ticket to extend the Search Index Management Helpers prose tests to include this gap case:

#### Case 9: Drivers use server default for unspecified name (`default`) and type (`search`)

opts := options.Client().ApplyURI(uri).SetTimeout(timeout)
mt := mtest.New(t, mtest.NewOptions().ClientOptions(opts).MinServerVersion("7.0").Topologies(mtest.ReplicaSet))

mt.Run("search indexes with empty option", func(mt *mtest.T) {
Copy link
Member

Choose a reason for hiding this comment

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

[blocking] The prose test should ensure that not specifying the name and type when creating a search index will result in the name being "default" and the type being "search". See this test for example: https://github.com/prestonvasquez/go-playground/blob/827f1733956815d63bf6de702384c59c89a868f1/mgd_search_index_test.go#L17

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍 Good point! Filed a DRIVERS ticket and a PR.

@prestonvasquez
Copy link
Member

@qingyang-hu Should this PR be targeting release/2.4?

@qingyang-hu qingyang-hu changed the base branch from master to release/2.4 December 2, 2025 02:49
working_dir: src/go.mongodb.org/mongo-driver
shell: bash
script: |-
echo "SEARCH_INDEX_URI: ${MONGODB_URI}" > atlas-expansion.yml
Copy link
Member

Choose a reason for hiding this comment

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

It seems that TestSearchIndexProse passes under the current TEST_SEARCH_INDEX paradigm, given the example you shared. So why do we need to add the SEARCH_INDEX_URI changes?

require.Equal(mt, "default", indexName)

var doc bson.Raw
for doc == nil {
Copy link
Member

Choose a reason for hiding this comment

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

This will run for the 10 minute default testing timeout. Suggest defining a context with a timeout to pass to view.List(). I'm not sure what a good value would be. Perhaps 1 minute?

Copy link
Member

@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 👍

@qingyang-hu qingyang-hu merged commit c3077a1 into mongodb:release/2.4 Dec 6, 2025
33 of 35 checks passed
qingyang-hu added a commit to qingyang-hu/mongo-go-driver that referenced this pull request Dec 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug review-priority-normal Medium Priority PR for Review: within 1 business day

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants