diff --git a/.changelog/v0.13.0/features/168-iter-key.md b/.changelog/v0.13.0/features/168-iter-key.md new file mode 100644 index 0000000..cc7db58 --- /dev/null +++ b/.changelog/v0.13.0/features/168-iter-key.md @@ -0,0 +1,2 @@ +- Iterator Key and Value APIs now return an object that must be copied before + use ([\#168](https://github.com/cometbft/cometbft-db/pull/168)) \ No newline at end of file diff --git a/.changelog/v0.13.0/summary.md b/.changelog/v0.13.0/summary.md new file mode 100644 index 0000000..3e860de --- /dev/null +++ b/.changelog/v0.13.0/summary.md @@ -0,0 +1,7 @@ + +This release changes the contract of the Iterator Key() and Value() APIs. +Namely, the caller is now responsible for creating a copy of their returned value if they want to modify it. diff --git a/backend_test.go b/backend_test.go index 7bab0b8..b30600f 100644 --- a/backend_test.go +++ b/backend_test.go @@ -348,7 +348,8 @@ func verifyIterator(t *testing.T, itr Iterator, expected []int64, msg string) { var list []int64 for itr.Valid() { - key := itr.Key() + key := make([]byte, len(itr.Key())) + copy(key, itr.Key()) list = append(list, bytes2Int64(key)) itr.Next() } @@ -450,7 +451,10 @@ func assertKeyValues(t *testing.T, db DB, expect map[string][]byte) { actual := make(map[string][]byte) for ; iter.Valid(); iter.Next() { require.NoError(t, iter.Error()) - actual[string(iter.Key())] = iter.Value() + + value := make([]byte, len(iter.Value())) + copy(value, iter.Value()) + actual[string(iter.Key())] = value } assert.Equal(t, expect, actual) diff --git a/badger_db.go b/badger_db.go index 227bd8c..703c8a3 100644 --- a/badger_db.go +++ b/badger_db.go @@ -279,19 +279,23 @@ func (i *badgerDBIterator) Valid() bool { return true } +// Key implements Iterator. +// The caller should not modify the contents of the returned slice. +// Instead, the caller should make a copy and work on the copy. func (i *badgerDBIterator) Key() []byte { if !i.Valid() { panic("iterator is invalid") } - // Note that we don't use KeyCopy, so this is only valid until the next - // call to Next. - return i.iter.Item().KeyCopy(nil) + return i.iter.Item().Key() } +// Value implements Iterator. +// The returned slice is a copy of the original data, therefore it is safe to modify. func (i *badgerDBIterator) Value() []byte { if !i.Valid() { panic("iterator is invalid") } + val, err := i.iter.Item().ValueCopy(nil) if err != nil { i.lastErr = err diff --git a/common_test.go b/common_test.go index f4e97c2..742d772 100644 --- a/common_test.go +++ b/common_test.go @@ -72,7 +72,9 @@ func checkKeyPanics(t *testing.T, itr Iterator) { func checkValuePanics(t *testing.T, itr Iterator) { t.Helper() - assert.Panics(t, func() { itr.Value() }) + + msg := "checkValuePanics expected panic but didn't" + assert.Panics(t, func() { itr.Value() }, msg) } func newTempDB(t *testing.T, backend BackendType) (db DB, dbDir string) { diff --git a/goleveldb_iterator.go b/goleveldb_iterator.go index 5341d1a..6a3c445 100644 --- a/goleveldb_iterator.go +++ b/goleveldb_iterator.go @@ -93,19 +93,19 @@ func (itr *goLevelDBIterator) Valid() bool { } // Key implements Iterator. +// The caller should not modify the contents of the returned slice. +// Instead, the caller should make a copy and work on the copy. func (itr *goLevelDBIterator) Key() []byte { - // Key returns a copy of the current key. - // See https://github.com/syndtr/goleveldb/blob/52c212e6c196a1404ea59592d3f1c227c9f034b2/leveldb/iterator/iter.go#L88 itr.assertIsValid() - return cp(itr.source.Key()) + return itr.source.Key() } // Value implements Iterator. +// The caller should not modify the contents of the returned slice. +// Instead, the caller should make a copy and work on the copy. func (itr *goLevelDBIterator) Value() []byte { - // Value returns a copy of the current value. - // See https://github.com/syndtr/goleveldb/blob/52c212e6c196a1404ea59592d3f1c227c9f034b2/leveldb/iterator/iter.go#L88 itr.assertIsValid() - return cp(itr.source.Value()) + return itr.source.Value() } // Next implements Iterator. diff --git a/pebble.go b/pebble.go index 946c23b..ed8f21a 100644 --- a/pebble.go +++ b/pebble.go @@ -396,19 +396,19 @@ func (itr *pebbleDBIterator) Valid() bool { } // Key implements Iterator. +// The caller should not modify the contents of the returned slice. +// Instead, the caller should make a copy and work on the copy. func (itr *pebbleDBIterator) Key() []byte { - // Key returns a copy of the current key. - // See https://github.com/cockroachdb/pebble/blob/v1.0.0/iterator.go#L2106 itr.assertIsValid() - return cp(itr.source.Key()) + return itr.source.Key() } // Value implements Iterator. +// The caller should not modify the contents of the returned slice. +// Instead, the caller should make a copy and work on the copy. func (itr *pebbleDBIterator) Value() []byte { - // Value returns a copy of the current value. - // See https://github.com/cockroachdb/pebble/blob/v1.0.0/iterator.go#L2116 itr.assertIsValid() - return cp(itr.source.Value()) + return itr.source.Value() } // Next implements Iterator. diff --git a/rocksdb_iterator.go b/rocksdb_iterator.go index 53fa9a9..e998f1c 100644 --- a/rocksdb_iterator.go +++ b/rocksdb_iterator.go @@ -94,12 +94,14 @@ func (itr *rocksDBIterator) Valid() bool { } // Key implements Iterator. +// The returned slice is a copy of the original data, therefore it is safe to modify. func (itr *rocksDBIterator) Key() []byte { itr.assertIsValid() return moveSliceToBytes(itr.source.Key()) } // Value implements Iterator. +// The returned slice is a copy of the original data, therefore it is safe to modify. func (itr *rocksDBIterator) Value() []byte { itr.assertIsValid() return moveSliceToBytes(itr.source.Value()) diff --git a/types.go b/types.go index 74da21e..11b0c07 100644 --- a/types.go +++ b/types.go @@ -136,11 +136,17 @@ type Iterator interface { Next() // Key returns the key at the current position. Panics if the iterator is invalid. - // CONTRACT: key readonly []byte + // Key returns the key of the current key/value pair, or nil if done. + // The caller should not modify the contents of the returned slice, and + // its contents may change on the next call to any 'seeks method'. + // Instead, the caller should make a copy and work on the copy. Key() (key []byte) // Value returns the value at the current position. Panics if the iterator is invalid. - // CONTRACT: value readonly []byte + // Value returns the value of the current key/value pair, or nil if done. + // The caller should not modify the contents of the returned slice, and + // its contents may change on the next call to any 'seeks method'. + // Instead, the caller should make a copy and work on the copy. Value() (value []byte) // Error returns the last error encountered by the iterator, if any.