batch: return errors for all operations (#96)

Modify batch `Set`, `Delete`, and `Close` to return errors. The main error condition currently is using closed batches, but future ones include databases which implement batches as transactions (e.g. BadgerDB) and rejecting empty keys (#2).

CHANGELOG.md

@@ -2,6 +2,12 @@
 
 ## Unreleased
 
+### Breaking Changes
+
+- Batch `Set()`, `Delete()`, and `Close()` may now error (@erikgrinaker)
+
+- The `SetDeleter` interface has been removed (@erikgrinaker)
+
 ## 0.5.1
 
 **2020-03-30**

backend_test.go

@@ -449,46 +449,47 @@ func testDBBatch(t *testing.T, backend BackendType) {
 
 	// create a new batch, and some items - they should not be visible until we write
 	batch := db.NewBatch()
-	batch.Set([]byte("a"), []byte{1})
-	batch.Set([]byte("b"), []byte{2})
-	batch.Set([]byte("c"), []byte{3})
+	require.NoError(t, batch.Set([]byte("a"), []byte{1}))
+	require.NoError(t, batch.Set([]byte("b"), []byte{2}))
+	require.NoError(t, batch.Set([]byte("c"), []byte{3}))
 	assertKeyValues(t, db, map[string][]byte{})
 
 	err := batch.Write()
 	require.NoError(t, err)
 	assertKeyValues(t, db, map[string][]byte{"a": {1}, "b": {2}, "c": {3}})
 
-	// trying to modify or rewrite a written batch should panic, but closing it should work
-	require.Panics(t, func() { batch.Set([]byte("a"), []byte{9}) })
-	require.Panics(t, func() { batch.Delete([]byte("a")) })
-	require.Panics(t, func() { batch.Write() })     // nolint: errcheck
-	require.Panics(t, func() { batch.WriteSync() }) // nolint: errcheck
-	batch.Close()
+	// trying to modify or rewrite a written batch should error, but closing it should work
+	require.Error(t, batch.Set([]byte("a"), []byte{9}))
+	require.Error(t, batch.Delete([]byte("a")))
+	require.Error(t, batch.Write())
+	require.Error(t, batch.WriteSync())
+	require.NoError(t, batch.Close())
 
 	// batches should write changes in order
 	batch = db.NewBatch()
-	batch.Delete([]byte("a"))
-	batch.Set([]byte("a"), []byte{1})
-	batch.Set([]byte("b"), []byte{1})
-	batch.Set([]byte("b"), []byte{2})
-	batch.Set([]byte("c"), []byte{3})
-	batch.Delete([]byte("c"))
-	err = batch.Write()
-	require.NoError(t, err)
-	batch.Close()
+	require.NoError(t, batch.Delete([]byte("a")))
+	require.NoError(t, batch.Set([]byte("a"), []byte{1}))
+	require.NoError(t, batch.Set([]byte("b"), []byte{1}))
+	require.NoError(t, batch.Set([]byte("b"), []byte{2}))
+	require.NoError(t, batch.Set([]byte("c"), []byte{3}))
+	require.NoError(t, batch.Delete([]byte("c")))
+	require.NoError(t, batch.Write())
+	require.NoError(t, batch.Close())
 	assertKeyValues(t, db, map[string][]byte{"a": {1}, "b": {2}})
 
 	// writing nil keys and values should be the same as empty keys and values
 	// FIXME CLevelDB panics here: https://github.com/jmhodges/levigo/issues/55
 	if backend != CLevelDBBackend {
 		batch = db.NewBatch()
-		batch.Set(nil, nil)
+		err = batch.Set(nil, nil)
+		require.NoError(t, err)
 		err = batch.WriteSync()
 		require.NoError(t, err)
 		assertKeyValues(t, db, map[string][]byte{"": {}, "a": {1}, "b": {2}})
 
 		batch = db.NewBatch()
-		batch.Delete(nil)
+		err = batch.Delete(nil)
+		require.NoError(t, err)
 		err = batch.Write()
 		require.NoError(t, err)
 		assertKeyValues(t, db, map[string][]byte{"a": {1}, "b": {2}})
@@ -505,11 +506,11 @@ func testDBBatch(t *testing.T, backend BackendType) {
 	batch.Close()
 	batch.Close()
 
-	// all other operations on a closed batch should panic
-	require.Panics(t, func() { batch.Set([]byte("a"), []byte{9}) })
-	require.Panics(t, func() { batch.Delete([]byte("a")) })
-	require.Panics(t, func() { batch.Write() })     // nolint: errcheck
-	require.Panics(t, func() { batch.WriteSync() }) // nolint: errcheck
+	// all other operations on a closed batch should error
+	require.Error(t, batch.Set([]byte("a"), []byte{9}))
+	require.Error(t, batch.Delete([]byte("a")))
+	require.Error(t, batch.Write())
+	require.Error(t, batch.WriteSync())
 }
 
 func assertKeyValues(t *testing.T, db DB, expect map[string][]byte) {

boltdb_batch.go

@@ -19,27 +19,29 @@ func newBoltDBBatch(db *BoltDB) *boltDBBatch {
 	}
 }
 
-func (b *boltDBBatch) assertOpen() {
+// Set implements Batch.
+func (b *boltDBBatch) Set(key, value []byte) error {
 	if b.ops == nil {
-		panic("batch has been written or closed")
+		return errBatchClosed
 	}
-}
-
-// Set implements Batch.
-func (b *boltDBBatch) Set(key, value []byte) {
-	b.assertOpen()
 	b.ops = append(b.ops, operation{opTypeSet, key, value})
+	return nil
 }
 
 // Delete implements Batch.
-func (b *boltDBBatch) Delete(key []byte) {
-	b.assertOpen()
+func (b *boltDBBatch) Delete(key []byte) error {
+	if b.ops == nil {
+		return errBatchClosed
+	}
 	b.ops = append(b.ops, operation{opTypeDelete, key, nil})
+	return nil
 }
 
 // Write implements Batch.
 func (b *boltDBBatch) Write() error {
-	b.assertOpen()
+	if b.ops == nil {
+		return errBatchClosed
+	}
 	err := b.db.db.Batch(func(tx *bbolt.Tx) error {
 		bkt := tx.Bucket(bucket)
 		for _, op := range b.ops {
@@ -61,8 +63,7 @@ func (b *boltDBBatch) Write() error {
 		return err
 	}
 	// Make sure batch cannot be used afterwards. Callers should still call Close(), for errors.
-	b.Close()
-	return nil
+	return b.Close()
 }
 
 // WriteSync implements Batch.
@@ -71,6 +72,7 @@ func (b *boltDBBatch) WriteSync() error {
 }
 
 // Close implements Batch.
-func (b *boltDBBatch) Close() {
+func (b *boltDBBatch) Close() error {
 	b.ops = nil
+	return nil
 }

cleveldb_batch.go

@@ -17,39 +17,42 @@ func newCLevelDBBatch(db *CLevelDB) *cLevelDBBatch {
 	}
 }
 
-func (b *cLevelDBBatch) assertOpen() {
+// Set implements Batch.
+func (b *cLevelDBBatch) Set(key, value []byte) error {
 	if b.batch == nil {
-		panic("batch has been written or closed")
+		return errBatchClosed
 	}
-}
-
-// Set implements Batch.
-func (b *cLevelDBBatch) Set(key, value []byte) {
-	b.assertOpen()
 	b.batch.Put(nonNilBytes(key), nonNilBytes(value))
+	return nil
 }
 
 // Delete implements Batch.
-func (b *cLevelDBBatch) Delete(key []byte) {
-	b.assertOpen()
+func (b *cLevelDBBatch) Delete(key []byte) error {
+	if b.batch == nil {
+		return errBatchClosed
+	}
 	b.batch.Delete(nonNilBytes(key))
+	return nil
 }
 
 // Write implements Batch.
 func (b *cLevelDBBatch) Write() error {
-	b.assertOpen()
+	if b.batch == nil {
+		return errBatchClosed
+	}
 	err := b.db.db.Write(b.db.wo, b.batch)
 	if err != nil {
 		return err
 	}
 	// Make sure batch cannot be used afterwards. Callers should still call Close(), for errors.
-	b.Close()
-	return nil
+	return b.Close()
 }
 
 // WriteSync implements Batch.
 func (b *cLevelDBBatch) WriteSync() error {
-	b.assertOpen()
+	if b.batch == nil {
+		return errBatchClosed
+	}
 	err := b.db.db.Write(b.db.woSync, b.batch)
 	if err != nil {
 		return err
@@ -60,9 +63,10 @@ func (b *cLevelDBBatch) WriteSync() error {
 }
 
 // Close implements Batch.
-func (b *cLevelDBBatch) Close() {
+func (b *cLevelDBBatch) Close() error {
 	if b.batch != nil {
 		b.batch.Close()
 		b.batch = nil
 	}
+	return nil
 }

goleveldb_batch.go

@@ -19,22 +19,22 @@ func newGoLevelDBBatch(db *GoLevelDB) *goLevelDBBatch {
 	}
 }
 
-func (b *goLevelDBBatch) assertOpen() {
+// Set implements Batch.
+func (b *goLevelDBBatch) Set(key, value []byte) error {
 	if b.batch == nil {
-		panic("batch has been written or closed")
+		return errBatchClosed
 	}
-}
-
-// Set implements Batch.
-func (b *goLevelDBBatch) Set(key, value []byte) {
-	b.assertOpen()
 	b.batch.Put(key, value)
+	return nil
 }
 
 // Delete implements Batch.
-func (b *goLevelDBBatch) Delete(key []byte) {
-	b.assertOpen()
+func (b *goLevelDBBatch) Delete(key []byte) error {
+	if b.batch == nil {
+		return errBatchClosed
+	}
 	b.batch.Delete(key)
+	return nil
 }
 
 // Write implements Batch.
@@ -48,20 +48,22 @@ func (b *goLevelDBBatch) WriteSync() error {
 }
 
 func (b *goLevelDBBatch) write(sync bool) error {
-	b.assertOpen()
+	if b.batch == nil {
+		return errBatchClosed
+	}
 	err := b.db.db.Write(b.batch, &opt.WriteOptions{Sync: sync})
 	if err != nil {
 		return err
 	}
 	// Make sure batch cannot be used afterwards. Callers should still call Close(), for errors.
-	b.Close()
-	return nil
+	return b.Close()
 }
 
 // Close implements Batch.
-func (b *goLevelDBBatch) Close() {
+func (b *goLevelDBBatch) Close() error {
 	if b.batch != nil {
 		b.batch.Reset()
 		b.batch = nil
 	}
+	return nil
 }

memdb_batch.go

@@ -32,27 +32,29 @@ func newMemDBBatch(db *MemDB) *memDBBatch {
 	}
 }
 
-func (b *memDBBatch) assertOpen() {
+// Set implements Batch.
+func (b *memDBBatch) Set(key, value []byte) error {
 	if b.ops == nil {
-		panic("batch has been written or closed")
+		return errBatchClosed
 	}
-}
-
-// Set implements Batch.
-func (b *memDBBatch) Set(key, value []byte) {
-	b.assertOpen()
 	b.ops = append(b.ops, operation{opTypeSet, key, value})
+	return nil
 }
 
 // Delete implements Batch.
-func (b *memDBBatch) Delete(key []byte) {
-	b.assertOpen()
+func (b *memDBBatch) Delete(key []byte) error {
+	if b.ops == nil {
+		return errBatchClosed
+	}
 	b.ops = append(b.ops, operation{opTypeDelete, key, nil})
+	return nil
 }
 
 // Write implements Batch.
 func (b *memDBBatch) Write() error {
-	b.assertOpen()
+	if b.ops == nil {
+		return errBatchClosed
+	}
 	b.db.mtx.Lock()
 	defer b.db.mtx.Unlock()
 
@@ -68,8 +70,7 @@ func (b *memDBBatch) Write() error {
 	}
 
 	// Make sure batch cannot be used afterwards. Callers should still call Close(), for errors.
-	b.Close()
-	return nil
+	return b.Close()
 }
 
 // WriteSync implements Batch.
@@ -78,6 +79,7 @@ func (b *memDBBatch) WriteSync() error {
 }
 
 // Close implements Batch.
-func (b *memDBBatch) Close() {
+func (b *memDBBatch) Close() error {
 	b.ops = nil
+	return nil
 }

prefixdb_batch.go

@@ -15,15 +15,15 @@ func newPrefixBatch(prefix []byte, source Batch) prefixDBBatch {
 }
 
 // Set implements Batch.
-func (pb prefixDBBatch) Set(key, value []byte) {
+func (pb prefixDBBatch) Set(key, value []byte) error {
 	pkey := append(cp(pb.prefix), key...)
-	pb.source.Set(pkey, value)
+	return pb.source.Set(pkey, value)
 }
 
 // Delete implements Batch.
-func (pb prefixDBBatch) Delete(key []byte) {
+func (pb prefixDBBatch) Delete(key []byte) error {
 	pkey := append(cp(pb.prefix), key...)
-	pb.source.Delete(pkey)
+	return pb.source.Delete(pkey)
 }
 
 // Write implements Batch.
@@ -37,6 +37,6 @@ func (pb prefixDBBatch) WriteSync() error {
 }
 
 // Close implements Batch.
-func (pb prefixDBBatch) Close() {
-	pb.source.Close()
+func (pb prefixDBBatch) Close() error {
+	return pb.source.Close()
 }