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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e24b5fa
GODRIVER-2589 Clarify `*Cursor.All()` behavior in comment.
qingyang-hu Sep 11, 2024
a9774d1
update tests.
qingyang-hu Sep 12, 2024
3b4d7ab
test updates
qingyang-hu Sep 17, 2024
796367e
minor change
qingyang-hu Sep 20, 2024
1cc21cd
update assertion
qingyang-hu Sep 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions mongo/cursor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,8 +286,9 @@ func (c *Cursor) Close(ctx context.Context) error {
}

// All iterates the cursor and decodes each document into results. The results parameter must be a pointer to a slice.
// 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.

// iterated, any previously iterated documents will not be included in results.
//
// This method requires driver version >= 1.1.0.
func (c *Cursor) All(ctx context.Context, results interface{}) error {
Expand Down
74 changes: 47 additions & 27 deletions mongo/cursor_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,37 +102,57 @@ 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")

err = cursor.All(context.Background(), []bson.D{})
assert.NotNil(t, err, "expected error, got nil")
assert.Error(t, err, "expected error, got nil")
})

t.Run("fills slice with all documents", 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)

var docs []bson.D
err = cursor.All(context.Background(), &docs)
assert.Nil(t, err, "All error: %v", err)
assert.Equal(t, 5, len(docs), "expected 5 docs, got %v", len(docs))
assert.NoError(t, err, "All error: %v", err)
assert.Len(t, docs, 5, "expected 5 docs, got %v", len(docs))

for index, doc := range docs {
expected := bson.D{{"foo", int32(index)}}
assert.Equal(t, expected, doc, "expected doc %v, got %v", expected, doc)
}
})

t.Run("nil slice", func(t *testing.T) {
cursor, err := newCursor(newTestBatchCursor(0, 0), nil, nil)
assert.NoError(t, err, "newCursor error: %v", err)

var docs []bson.D
err = cursor.All(context.Background(), &docs)
assert.NoError(t, err, "All error: %v", err)
assert.Nil(t, docs, "expected nil docs")
})

t.Run("empty slice", func(t *testing.T) {
cursor, err := newCursor(newTestBatchCursor(0, 0), nil, nil)
assert.NoError(t, err, "newCursor error: %v", err)

docs := []bson.D{{{"foo", "bar"}}, {{"hello", "world"}, {"pi", 3.14159}}}
err = cursor.All(context.Background(), &docs)
assert.NotNil(t, docs, "expected non-nil docs")
assert.Len(t, docs, 0, "expected 0 docs, got %v", len(docs))
})

t.Run("decodes each document into slice type", 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)

type Document struct {
Foo int32 `bson:"foo"`
}
var docs []Document
err = cursor.All(context.Background(), &docs)
assert.Nil(t, err, "All error: %v", err)
assert.Equal(t, 5, len(docs), "expected 5 documents, got %v", len(docs))
assert.NoError(t, err, "All error: %v", err)
assert.Len(t, docs, 5, "expected 5 documents, got %v", len(docs))

for index, doc := range docs {
expected := Document{Foo: int32(index)}
Expand All @@ -142,11 +162,11 @@ func TestCursor(t *testing.T) {

t.Run("multiple batches are included", func(t *testing.T) {
cursor, err := newCursor(newTestBatchCursor(2, 5), nil, nil)
assert.Nil(t, err, "newCursor error: %v", err)
assert.NoError(t, err, "newCursor error: %v", err)
var docs []bson.D
err = cursor.All(context.Background(), &docs)
assert.Nil(t, err, "All error: %v", err)
assert.Equal(t, 10, len(docs), "expected 10 docs, got %v", len(docs))
assert.NoError(t, err, "All error: %v", err)
assert.Len(t, docs, 10, "expected 10 docs, got %v", len(docs))

for index, doc := range docs {
expected := bson.D{{"foo", int32(index)}}
Expand All @@ -159,31 +179,31 @@ func TestCursor(t *testing.T) {

tbc := newTestBatchCursor(1, 5)
cursor, err := newCursor(tbc, nil, nil)
assert.Nil(t, err, "newCursor error: %v", err)
assert.NoError(t, err, "newCursor error: %v", err)

err = cursor.All(context.Background(), &docs)
assert.Nil(t, err, "All error: %v", err)
assert.NoError(t, err, "All error: %v", err)
assert.True(t, tbc.closed, "expected batch cursor to be closed but was not")
})

t.Run("does not error given interface as parameter", func(t *testing.T) {
var docs interface{} = []bson.D{}

cursor, err := newCursor(newTestBatchCursor(1, 5), nil, nil)
assert.Nil(t, err, "newCursor error: %v", err)
assert.NoError(t, err, "newCursor error: %v", err)

err = cursor.All(context.Background(), &docs)
assert.Nil(t, err, "expected Nil, got error: %v", err)
assert.Equal(t, 5, len(docs.([]bson.D)), "expected 5 documents, got %v", len(docs.([]bson.D)))
assert.NoError(t, err, "All error: %v", err)
assert.Len(t, docs.([]bson.D), 5, "expected 5 documents, got %v", len(docs.([]bson.D)))
})
t.Run("errors when not given pointer to slice", func(t *testing.T) {
var docs interface{} = "test"

cursor, err := newCursor(newTestBatchCursor(1, 5), nil, nil)
assert.Nil(t, err, "newCursor error: %v", err)
assert.NoError(t, err, "newCursor error: %v", err)

err = cursor.All(context.Background(), &docs)
assert.NotNil(t, err, "expected error, got: %v", err)
assert.Error(t, err, "expected error, got: %v", err)
})
t.Run("with BSONOptions", func(t *testing.T) {
cursor, err := newCursor(
Expand All @@ -192,15 +212,15 @@ func TestCursor(t *testing.T) {
UseJSONStructTags: true,
},
nil)
require.NoError(t, err, "newCursor error")
require.NoError(t, err, "newCursor error: %v", err)

type myDocument struct {
A int32 `json:"foo"`
}
var got []myDocument

err = cursor.All(context.Background(), &got)
require.NoError(t, err, "All error")
require.NoError(t, err, "All error: %v", err)

want := []myDocument{{A: 0}, {A: 1}, {A: 2}, {A: 3}, {A: 4}}

Expand All @@ -218,18 +238,18 @@ func TestNewCursorFromDocuments(t *testing.T) {
bson.D{{"_id", 2}, {"quux", "quuz"}},
}
cur, err := NewCursorFromDocuments(findResult, nil, nil)
assert.Nil(t, err, "NewCursorFromDocuments error: %v", err)
assert.NoError(t, err, "NewCursorFromDocuments error: %v", err)

// Assert that decoded documents are as expected.
var i int
for cur.Next(context.Background()) {
docBytes, err := bson.Marshal(findResult[i])
assert.Nil(t, err, "Marshal error: %v", err)
assert.NoError(t, err, "Marshal error: %v", err)
expectedDecoded := bson.Raw(docBytes)

var decoded bson.Raw
err = cur.Decode(&decoded)
assert.Nil(t, err, "Decode error: %v", err)
assert.NoError(t, err, "Decode error: %v", err)
assert.Equal(t, expectedDecoded, decoded,
"expected decoded document %v of Cursor to be %v, got %v",
i, expectedDecoded, decoded)
Expand All @@ -238,26 +258,26 @@ func TestNewCursorFromDocuments(t *testing.T) {
assert.Equal(t, 3, i, "expected 3 calls to cur.Next, got %v", i)

// Check for error on Cursor.
assert.Nil(t, cur.Err(), "Cursor error: %v", cur.Err())
assert.NoError(t, cur.Err(), "Cursor error: %v", cur.Err())

// Assert that a call to cur.Close will not fail.
err = cur.Close(context.Background())
assert.Nil(t, err, "Close error: %v", err)
assert.NoError(t, err, "Close error: %v", err)
})

// Mock an error in a Cursor.
t.Run("mock Find with error", func(t *testing.T) {
mockErr := fmt.Errorf("mock error")
findResult := []interface{}{bson.D{{"_id", 0}, {"foo", "bar"}}}
cur, err := NewCursorFromDocuments(findResult, mockErr, nil)
assert.Nil(t, err, "NewCursorFromDocuments error: %v", err)
assert.NoError(t, err, "NewCursorFromDocuments error: %v", err)

// Assert that a call to Next will return false because of existing error.
next := cur.Next(context.Background())
assert.False(t, next, "expected call to Next to return false, got true")

// Check for error on Cursor.
assert.NotNil(t, cur.Err(), "expected Cursor error, got nil")
assert.Error(t, cur.Err(), "expected Cursor error, got nil")
assert.Equal(t, mockErr, cur.Err(), "expected Cursor error %v, got %v",
mockErr, cur.Err())
})
Expand Down
Loading