Skip to content

GODRIVER-2589 Clarify *Cursor.All() behavior in comment. #1804

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

Merged
merged 5 commits into from
Sep 24, 2024

Conversation

qingyang-hu
Copy link
Collaborator

GODRIVER-2589

Summary

Clarify the *Cursor.All() behavior in the comment that a nil slice pointer won't be modified in the case of no results.

Background & Motivation

The current behavior is a feature rather than a bug as long as we clarify it in the documentation.

Currently, in the case of no results:

  1. if the pointer is nil, it will be left as is.
  2. if the pointer points to an initialized slice, the slice will be set to empty.

The caller controls the received type either way.

Also, as mentioned in the comment, users may have depended on the behavior.

We need to merge the update to the master branch as well.

@mongodb-drivers-pr-bot mongodb-drivers-pr-bot bot added the priority-3-low Low Priority PR for Review label Sep 11, 2024
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.

This behavior makes sense to me. Initializing the slice seems like strange behavior. However, it's notable that encoding/json does this:

package main

import (
	"encoding/json"
	"fmt"
	"strings"
)

func main() {
	decoder := json.NewDecoder(strings.NewReader(""))

	var data []any

	err := decoder.Decode(&data)
	if err != nil {
		panic(err)
	}

	fmt.Println(data == nil, data)
}

mongo/cursor.go Outdated
// The slice pointed to by results will be completely overwritten. This method will close the cursor after retrieving
// all documents. If the cursor has been iterated, any previously iterated documents will not be included in results.
// The slice pointed to by results will be completely overwritten. A nil slice pointer will not be modified if the cursor
// has been closed or exhausted. This method will close the cursor after retrieving all documents. If the cursor has been
Copy link
Member

Choose a reason for hiding this comment

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

Should we also clarify the empty case?

A nil slice pointer will not be modified if the cursor has been closed, exhausted, or is empty.

@qingyang-hu
Copy link
Collaborator Author

... However, it's notable that encoding/json does this ...

The json Decode() is different from our case because returning nothing is not an exception for a query.

@@ -102,37 +102,69 @@ func TestCursor(t *testing.T) {
t.Run("TestAll", func(t *testing.T) {
t.Run("errors if argument is not pointer to slice", func(t *testing.T) {
cursor, err := newCursor(newTestBatchCursor(1, 5), nil, nil)
assert.Nil(t, err, "newCursor error: %v", err)
assert.NoError(t, err, "newCursor error: %v", err)
Copy link
Member

Choose a reason for hiding this comment

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

If we are checking an error can we use require.NoError? It seems there's no point in continuing the test otherwise. Additionally, we don't have to include the error in the message since the assertion will provide it:

require.NoError(t, err, "failed to create cursor")

joyjwang
joyjwang previously approved these changes Sep 18, 2024
Copy link
Contributor

@joyjwang joyjwang left a comment

Choose a reason for hiding this comment

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

LGTM!

prestonvasquez
prestonvasquez previously approved these changes Sep 23, 2024
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, with suggested test updates.

@qingyang-hu qingyang-hu merged commit 0178864 into mongodb:v1 Sep 24, 2024
28 of 33 checks passed
qingyang-hu added a commit that referenced this pull request Sep 24, 2024
blink1073 pushed a commit to blink1073/mongo-go-driver that referenced this pull request Sep 30, 2024
prestonvasquez pushed a commit to prestonvasquez/mongo-go-driver that referenced this pull request Feb 21, 2025
alcaeus added a commit that referenced this pull request Mar 4, 2025
* commit '0dc2e05e': (184 commits)
  GODRIVER-3448 Limit GOMAXPROCS for fuzz tests (#1939) [v1] (#1943)
  GODRIVER-3470 Correct BSON unmarshaling logic for null values (#1924)
  GODRIVER-3370 Add bypassEmptyTsReplacement option. (#1927)
  BUMP v1.17.2
  GODRIVER-3436 Avoid initializing null data given custom decoder (#1902)
  GODRIVER-3340 Add a test for goroutine leaks. (#1874)
  Update reviewers.txt (#1855) [v1] (#1883)
  Fix data race in 'discard connections' pool test. [v1] (#1877)
  Bump golangci-lint for 1.23 compatibility [v1] (#1875)
  GODRIVER-3340 Bump github.com/klauspost/compress from 1.13.6 to 1.16.7 [v1] (#1869)
  GODRIVER-3374 Add ReadCompressedCompressedMessage back to wiremessage API (#1870)
  GODRIVER-3156 Detect and discard closed idle connections. (#1815)
  GODRIVER-3358 Do not override authSource from TXT record (#1830)
  DEVPROD-10453 Use assume_role for s3 uploads [v1] (#1824)
  GODRIVER-2589 Clarify `*Cursor.All()` behavior in comment. (#1804)
  GODRIVER-3313 Skip CSOT spec tests on Windows and macOS. (#1818)
  BUMP v1.17.0
  GODRIVER-3302 Handle malformatted message length properly. (#1758)
  GODRIVER-3312 Use remaining test secrets from the vault [v1] (#1811)
  Remove GCP from supplied callback example (#1809)
  ...
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.

3 participants