Merge pull request #744 from ahrtr/cursor_20240502

[1.3] Ensure a cursor can continue to iterate elements in reverse direction by call Next when it has already reached the beginning

Author: ahrtr

README.md

@@ -421,10 +421,19 @@ Prev()   Move to the previous key.
 ```
 
 Each of those functions has a return signature of `(key []byte, value []byte)`.
-When you have iterated to the end of the cursor then `Next()` will return a
-`nil` key.  You must seek to a position using `First()`, `Last()`, or `Seek()`
-before calling `Next()` or `Prev()`. If you do not seek to a position then
-these functions will return a `nil` key.
+You must seek to a position using `First()`, `Last()`, or `Seek()` before calling
+`Next()` or `Prev()`. If you do not seek to a position then these functions will
+return a `nil` key.
+
+When you have iterated to the end of the cursor, then `Next()` will return a
+`nil` key and the cursor still points to the last element if present. When you
+have iterated to the beginning of the cursor, then `Prev()` will return a `nil`
+key and the cursor still points to the first element if present.
+
+If you remove key/value pairs during iteration, the cursor may automatically
+move to the next position if present in current node each time removing a key.
+When you call `c.Next()` after removing a key, it may skip one key/value pair.
+Refer to [pull/611](https://github.com/etcd-io/bbolt/pull/611) to get more detailed info.
 
 During iteration, if the key is non-`nil` but the value is `nil`, that means
 the key refers to a bucket rather than a value.  Use `Bucket.Bucket()` to
@@ -850,6 +859,12 @@ Here are a few things to note when evaluating and using Bolt:
   to grow. However, it's important to note that deleting large chunks of data
   will not allow you to reclaim that space on disk.
 
+* Removing key/values pairs in a bucket during iteration on the bucket using
+  cursor may not work properly. Each time when removing a key/value pair, the
+  cursor may automatically move to the next position if present. When users
+  call `c.Next()` after removing a key, it may skip one key/value pair.
+  Refer to https://github.com/etcd-io/bbolt/pull/611 for more detailed info.
+
   For more information on page allocation, [see this comment][page-allocation].
 
 [page-allocation]: https://github.com/boltdb/bolt/issues/308#issuecomment-74811638

cursor.go

@@ -71,7 +71,7 @@ func (c *Cursor) Last() (key []byte, value []byte) {
 
 	// If this is an empty page (calling Delete may result in empty pages)
 	// we call prev to find the last page that is not empty
-	for len(c.stack) > 0 && c.stack[len(c.stack)-1].count() == 0 {
+	for len(c.stack) > 1 && c.stack[len(c.stack)-1].count() == 0 {
 		c.prev()
 	}
 
@@ -254,6 +254,15 @@ func (c *Cursor) prev() (key []byte, value []byte, flags uint32) {
 			elem.index--
 			break
 		}
+		// If we've hit the beginning, we should stop moving the cursor,
+		// and stay at the first element, so that users can continue to
+		// iterate over the elements in reverse direction by calling `Next`.
+		// We should return nil in such case.
+		// Refer to https://github.com/etcd-io/bbolt/issues/733
+		if len(c.stack) == 1 {
+			c.first()
+			return nil, nil, 0
+		}
 		c.stack = c.stack[:i]
 	}
 

cursor_test.go

@@ -11,10 +11,143 @@ import (
 	"testing"
 	"testing/quick"
 
+	"github.com/stretchr/testify/require"
+
 	bolt "go.etcd.io/bbolt"
 	"go.etcd.io/bbolt/internal/btesting"
 )
 
+// TestCursor_RepeatOperations verifies that a cursor can continue to
+// iterate over all elements in reverse direction when it has already
+// reached to the end or beginning.
+// Refer to https://github.com/etcd-io/bbolt/issues/733
+func TestCursor_RepeatOperations(t *testing.T) {
+	testCases := []struct {
+		name     string
+		testFunc func(t2 *testing.T, bucket *bolt.Bucket)
+	}{
+		{
+			name:     "Repeat NextPrevNext",
+			testFunc: testRepeatCursorOperations_NextPrevNext,
+		},
+		{
+			name:     "Repeat PrevNextPrev",
+			testFunc: testRepeatCursorOperations_PrevNextPrev,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			db := btesting.MustCreateDBWithOption(t, &bolt.Options{PageSize: 4096})
+
+			bucketName := []byte("data")
+
+			_ = db.Update(func(tx *bolt.Tx) error {
+				b, _ := tx.CreateBucketIfNotExists(bucketName)
+				testCursorRepeatOperations_PrepareData(t, b)
+				return nil
+			})
+
+			_ = db.View(func(tx *bolt.Tx) error {
+				b := tx.Bucket(bucketName)
+				tc.testFunc(t, b)
+				return nil
+			})
+		})
+	}
+}
+
+func testCursorRepeatOperations_PrepareData(t *testing.T, b *bolt.Bucket) {
+	// ensure we have at least one branch page.
+	for i := 0; i < 1000; i++ {
+		k := []byte(fmt.Sprintf("%05d", i))
+		err := b.Put(k, k)
+		require.NoError(t, err)
+	}
+}
+
+func testRepeatCursorOperations_NextPrevNext(t *testing.T, b *bolt.Bucket) {
+	c := b.Cursor()
+	c.First()
+	startKey := []byte(fmt.Sprintf("%05d", 2))
+	returnedKey, _ := c.Seek(startKey)
+	require.Equal(t, startKey, returnedKey)
+
+	// Step 1: verify next
+	for i := 3; i < 1000; i++ {
+		expectedKey := []byte(fmt.Sprintf("%05d", i))
+		actualKey, _ := c.Next()
+		require.Equal(t, expectedKey, actualKey)
+	}
+
+	// Once we've reached the end, it should always return nil no matter how many times we call `Next`.
+	for i := 0; i < 10; i++ {
+		k, _ := c.Next()
+		require.Equal(t, []byte(nil), k)
+	}
+
+	// Step 2: verify prev
+	for i := 998; i >= 0; i-- {
+		expectedKey := []byte(fmt.Sprintf("%05d", i))
+		actualKey, _ := c.Prev()
+		require.Equal(t, expectedKey, actualKey)
+	}
+
+	// Once we've reached the beginning, it should always return nil no matter how many times we call `Prev`.
+	for i := 0; i < 10; i++ {
+		k, _ := c.Prev()
+		require.Equal(t, []byte(nil), k)
+	}
+
+	// Step 3: verify next again
+	for i := 1; i < 1000; i++ {
+		expectedKey := []byte(fmt.Sprintf("%05d", i))
+		actualKey, _ := c.Next()
+		require.Equal(t, expectedKey, actualKey)
+	}
+}
+
+func testRepeatCursorOperations_PrevNextPrev(t *testing.T, b *bolt.Bucket) {
+	c := b.Cursor()
+
+	startKey := []byte(fmt.Sprintf("%05d", 998))
+	returnedKey, _ := c.Seek(startKey)
+	require.Equal(t, startKey, returnedKey)
+
+	// Step 1: verify prev
+	for i := 997; i >= 0; i-- {
+		expectedKey := []byte(fmt.Sprintf("%05d", i))
+		actualKey, _ := c.Prev()
+		require.Equal(t, expectedKey, actualKey)
+	}
+
+	// Once we've reached the beginning, it should always return nil no matter how many times we call `Prev`.
+	for i := 0; i < 10; i++ {
+		k, _ := c.Prev()
+		require.Equal(t, []byte(nil), k)
+	}
+
+	// Step 2: verify next
+	for i := 1; i < 1000; i++ {
+		expectedKey := []byte(fmt.Sprintf("%05d", i))
+		actualKey, _ := c.Next()
+		require.Equal(t, expectedKey, actualKey)
+	}
+
+	// Once we've reached the end, it should always return nil no matter how many times we call `Next`.
+	for i := 0; i < 10; i++ {
+		k, _ := c.Next()
+		require.Equal(t, []byte(nil), k)
+	}
+
+	// Step 3: verify prev again
+	for i := 998; i >= 0; i-- {
+		expectedKey := []byte(fmt.Sprintf("%05d", i))
+		actualKey, _ := c.Prev()
+		require.Equal(t, expectedKey, actualKey)
+	}
+}
+
 // Ensure that a cursor can return a reference to the bucket that created it.
 func TestCursor_Bucket(t *testing.T) {
 	db := btesting.MustCreateDB(t)