diff --git a/README.md b/README.md index 2be669a60..495a93ef8 100644 --- a/README.md +++ b/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 diff --git a/cursor.go b/cursor.go index 5dafb0cac..bbfd92a9b 100644 --- a/cursor.go +++ b/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] } diff --git a/cursor_test.go b/cursor_test.go index 8e112c14e..944118686 100644 --- a/cursor_test.go +++ b/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)