From add9368e9d40daa899e31c7a53a584cc9a020852 Mon Sep 17 00:00:00 2001 From: Roman Khimov Date: Mon, 7 Oct 2019 17:05:53 +0300 Subject: [PATCH] storage: use strings as keys for memory batch Using pointers is just plain wrong here, because the batch can be updated with newer values for the same keys. Fixes Seek() to use HasPrefix also because this is the intended behavior. --- pkg/core/storage/boltdb_store.go | 2 +- pkg/core/storage/boltdb_store_test.go | 15 -------------- pkg/core/storage/memory_store.go | 28 ++++++++++----------------- pkg/core/storage/redis_store.go | 2 +- pkg/core/storage/redis_store_test.go | 10 +--------- 5 files changed, 13 insertions(+), 44 deletions(-) diff --git a/pkg/core/storage/boltdb_store.go b/pkg/core/storage/boltdb_store.go index 18283bc63..e2c6c52a0 100644 --- a/pkg/core/storage/boltdb_store.go +++ b/pkg/core/storage/boltdb_store.go @@ -76,7 +76,7 @@ func (s *BoltDBStore) PutBatch(batch Batch) error { return s.db.Batch(func(tx *bbolt.Tx) error { b := tx.Bucket(Bucket) for k, v := range batch.(*MemoryBatch).m { - err := b.Put(*k, v) + err := b.Put([]byte(k), v) if err != nil { return err } diff --git a/pkg/core/storage/boltdb_store_test.go b/pkg/core/storage/boltdb_store_test.go index e5cf397b1..299a74816 100644 --- a/pkg/core/storage/boltdb_store_test.go +++ b/pkg/core/storage/boltdb_store_test.go @@ -3,27 +3,12 @@ package storage import ( "io/ioutil" "os" - "reflect" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -func TestBoltDBBatch(t *testing.T) { - boltDB := BoltDBStore{} - want := &MemoryBatch{m: map[*[]byte][]byte{}} - if got := boltDB.Batch(); !reflect.DeepEqual(got, want) { - t.Errorf("BoltDB Batch() = %v, want %v", got, want) - } -} - -func TestBoltDBBatch_Len(t *testing.T) { - batch := &MemoryBatch{m: map[*[]byte][]byte{}} - want := len(map[*[]byte][]byte{}) - assert.Equal(t, want, batch.Len()) -} - func TestBoltDBBatch_PutBatchAndGet(t *testing.T) { key := []byte("foo") keycopy := make([]byte, len(key)) diff --git a/pkg/core/storage/memory_store.go b/pkg/core/storage/memory_store.go index 789dffdbb..8ce29c7db 100644 --- a/pkg/core/storage/memory_store.go +++ b/pkg/core/storage/memory_store.go @@ -1,7 +1,6 @@ package storage import ( - "encoding/hex" "strings" "sync" ) @@ -15,16 +14,15 @@ type MemoryStore struct { // MemoryBatch a in-memory batch compatible with MemoryStore. type MemoryBatch struct { - m map[*[]byte][]byte + m map[string][]byte } // Put implements the Batch interface. func (b *MemoryBatch) Put(k, v []byte) { vcopy := make([]byte, len(v)) copy(vcopy, v) - kcopy := make([]byte, len(k)) - copy(kcopy, k) - b.m[&kcopy] = vcopy + kcopy := string(k) + b.m[kcopy] = vcopy } // Len implements the Batch interface. @@ -43,7 +41,7 @@ func NewMemoryStore() *MemoryStore { func (s *MemoryStore) Get(key []byte) ([]byte, error) { s.mut.RLock() defer s.mut.RUnlock() - if val, ok := s.mem[makeKey(key)]; ok { + if val, ok := s.mem[string(key)]; ok { return val, nil } return nil, ErrKeyNotFound @@ -52,7 +50,7 @@ func (s *MemoryStore) Get(key []byte) ([]byte, error) { // Put implements the Store interface. Never returns an error. func (s *MemoryStore) Put(key, value []byte) error { s.mut.Lock() - s.mem[makeKey(key)] = value + s.mem[string(key)] = value s.mut.Unlock() return nil } @@ -61,7 +59,7 @@ func (s *MemoryStore) Put(key, value []byte) error { func (s *MemoryStore) PutBatch(batch Batch) error { b := batch.(*MemoryBatch) for k, v := range b.m { - _ = s.Put(*k, v) + _ = s.Put([]byte(k), v) } return nil } @@ -69,9 +67,8 @@ func (s *MemoryStore) PutBatch(batch Batch) error { // Seek implements the Store interface. func (s *MemoryStore) Seek(key []byte, f func(k, v []byte)) { for k, v := range s.mem { - if strings.Contains(k, hex.EncodeToString(key)) { - decodeString, _ := hex.DecodeString(k) - f(decodeString, v) + if strings.HasPrefix(k, string(key)) { + f([]byte(k), v) } } } @@ -84,7 +81,7 @@ func (s *MemoryStore) Batch() Batch { // newMemoryBatch returns new memory batch. func newMemoryBatch() *MemoryBatch { return &MemoryBatch{ - m: make(map[*[]byte][]byte), + m: make(map[string][]byte), } } @@ -96,8 +93,7 @@ func (s *MemoryStore) Persist(ps Store) (int, error) { batch := ps.Batch() keys := 0 for k, v := range s.mem { - kb, _ := hex.DecodeString(k) - batch.Put(kb, v) + batch.Put([]byte(k), v) keys++ } var err error @@ -118,7 +114,3 @@ func (s *MemoryStore) Close() error { s.mut.Unlock() return nil } - -func makeKey(k []byte) string { - return hex.EncodeToString(k) -} diff --git a/pkg/core/storage/redis_store.go b/pkg/core/storage/redis_store.go index 5dd5ac4e0..30021c9fd 100644 --- a/pkg/core/storage/redis_store.go +++ b/pkg/core/storage/redis_store.go @@ -58,7 +58,7 @@ func (s *RedisStore) Put(k, v []byte) error { func (s *RedisStore) PutBatch(b Batch) error { pipe := s.client.Pipeline() for k, v := range b.(*MemoryBatch).m { - pipe.Set(string(*k), v, 0) + pipe.Set(k, v, 0) } _, err := pipe.Exec() return err diff --git a/pkg/core/storage/redis_store_test.go b/pkg/core/storage/redis_store_test.go index f60f76922..919c5683f 100644 --- a/pkg/core/storage/redis_store_test.go +++ b/pkg/core/storage/redis_store_test.go @@ -23,14 +23,6 @@ func TestNewRedisStore(t *testing.T) { redisMock.Close() } -func TestRedisBatch_Len(t *testing.T) { - want := len(map[string]string{}) - b := &MemoryBatch{ - m: map[*[]byte][]byte{}, - } - assert.Equal(t, len(b.m), want) -} - func TestRedisStore_GetAndPut(t *testing.T) { prepareRedisMock(t) type args struct { @@ -82,7 +74,7 @@ func TestRedisStore_GetAndPut(t *testing.T) { } func TestRedisStore_PutBatch(t *testing.T) { - batch := &MemoryBatch{m: map[*[]byte][]byte{&[]byte{'f', 'o', 'o', '1'}: []byte("bar1")}} + batch := &MemoryBatch{m: map[string][]byte{"foo1": []byte("bar1")}} mock, redisStore := prepareRedisMock(t) err := redisStore.PutBatch(batch) assert.Nil(t, err, "Error while PutBatch")