Repository: genjidb/genji (go)

Issue:
Dropping a table removes **all** indexes
Given the following queries:
```
On-disk database using BoltDB engine at path my.db.
genji> create table user(name text);
genji> create index idx_user_name on user(name);
genji> create table cars(brand text);
genji> create index idx_cars_brand on cars(brand);
```

Using `boltbrowser` I can see the indexes:

![image](https://user-images.githubusercontent.com/4570448/84804989-d4cfcc80-b003-11ea-8877-ffde284179aa.png)


If I remove the `cars` table:
```
genji> drop table cars;
```

⚠️    **All indexes get removed**:

![image](https://user-images.githubusercontent.com/4570448/84805103-09dc1f00-b004-11ea-9ffa-f16b21f1f834.png)

This is critical and should be fixed as soon as possible.


Pull request:
Dropping a table should remove only indexes associated to that table
This PR fixes #99.

The fix is really trivial, just checking that the index we are iterating on is associated to the table we want to drop.

I take the opportunity to improve tests for `sql/query/drop.go` and I also added a method for listing the indexes (it's a good start for #100).

Patch:
diff --git a/database/config.go b/database/config.go
index dd45bbe4..8746cc10 100644
--- a/database/config.go
+++ b/database/config.go
@@ -321,6 +321,37 @@ func (t *indexStore) Delete(indexName string) error {
 	return err
 }
 
+func (t *indexStore) ListAll() ([]*IndexConfig, error) {
+	var idxList []*IndexConfig
+	it := t.st.NewIterator(engine.IteratorConfig{})
+
+	var buf encoding.EncodedDocument
+	var err error
+	for it.Seek(nil); it.Valid(); it.Next() {
+		item := it.Item()
+		var opts IndexConfig
+		buf, err = item.ValueCopy(buf)
+		if err != nil {
+			it.Close()
+			return nil, err
+		}
+
+		err = opts.ScanDocument(&buf)
+		if err != nil {
+			it.Close()
+			return nil, err
+		}
+
+		idxList = append(idxList, &opts)
+	}
+	err = it.Close()
+	if err != nil {
+		return nil, err
+	}
+
+	return idxList, nil
+}
+
 func arrayToValuePath(v document.Value) (document.ValuePath, error) {
 	ar, err := v.ConvertToArray()
 	if err != nil {
diff --git a/database/transaction.go b/database/transaction.go
index 25a85bf4..d3445292 100644
--- a/database/transaction.go
+++ b/database/transaction.go
@@ -124,6 +124,11 @@ func (tx Transaction) DropTable(name string) error {
 			return err
 		}
 
+		// Remove only indexes associated with the target table.
+		if opts.TableName != name {
+			continue
+		}
+
 		err = tx.DropIndex(opts.IndexName)
 		if err != nil {
 			it.Close()
@@ -221,6 +226,11 @@ func (tx Transaction) DropIndex(name string) error {
 	return idx.Truncate()
 }
 
+// ListIndexes lists all indexes.
+func (tx Transaction) ListIndexes() ([]*IndexConfig, error) {
+	return tx.indexStore.ListAll()
+}
+
 // ReIndex truncates and recreates selected index from scratch.
 func (tx Transaction) ReIndex(indexName string) error {
 	idx, err := tx.GetIndex(indexName)
diff --git a/engine/engine.go b/engine/engine.go
index c4cd0311..618cac12 100644
--- a/engine/engine.go
+++ b/engine/engine.go
@@ -98,7 +98,7 @@ type Item interface {
 	// The key is only guaranteed to be valid until the next call to the Next method of
 	// the iterator.
 	Key() []byte
-	// ValueCopy copies the key to the given byte slice returns it.
+	// ValueCopy copies the key to the given byte slice and returns it.
 	// If the slice is not big enough, it must create a new one and return it.
 	ValueCopy([]byte) ([]byte, error)
 }


Test patch:
diff --git a/database/config_test.go b/database/config_test.go
index 1af0e394..7d6e1a0b 100644
--- a/database/config_test.go
+++ b/database/config_test.go
@@ -55,3 +55,70 @@ func TestTableConfigStore(t *testing.T) {
 	err = tcs.Delete("foo-table")
 	require.Equal(t, ErrTableNotFound, err)
 }
+
+func TestIndexStore(t *testing.T) {
+	ng := memoryengine.NewEngine()
+	defer ng.Close()
+
+	tx, err := ng.Begin(true)
+	require.NoError(t, err)
+	defer tx.Rollback()
+
+	err = tx.CreateStore("test")
+	require.NoError(t, err)
+	st, err := tx.GetStore("test")
+	require.NoError(t, err)
+
+	idxs := indexStore{st}
+
+	t.Run("Basic operations", func(t *testing.T) {
+		cfg := IndexConfig{
+			TableName: "test",
+			IndexName: "idx_test",
+			Unique:    true,
+		}
+
+		err = idxs.Insert(cfg)
+		require.NoError(t, err)
+
+		// Inserting the same index should fail.
+		err = idxs.Insert(cfg)
+		require.EqualError(t, err, ErrIndexAlreadyExists.Error())
+
+		idxcfg, err := idxs.Get("idx_test")
+		require.NoError(t, err)
+		require.Equal(t, &cfg, idxcfg)
+
+		err = idxs.Delete("idx_test")
+		require.NoError(t, err)
+
+		// Getting a non existing index should fail.
+		_, err = idxs.Get("idx_test")
+		require.EqualError(t, err, ErrIndexNotFound.Error())
+	})
+
+	t.Run("List all indexes", func(t *testing.T) {
+		idxcfgs := []*IndexConfig{
+			{TableName: "test1", IndexName: "idx_test1", Unique: true},
+			{TableName: "test2", IndexName: "idx_test2", Unique: true},
+			{TableName: "test3", IndexName: "idx_test3", Unique: true},
+		}
+		for _, v := range idxcfgs {
+			err = idxs.Insert(*v)
+			require.NoError(t, err)
+		}
+
+		list, err := idxs.ListAll()
+		require.NoError(t, err)
+		require.Len(t, list, len(idxcfgs))
+		require.EqualValues(t, idxcfgs, list)
+
+		// Removing one index should remove only one index.
+		err = idxs.Delete("idx_test1")
+		require.NoError(t, err)
+
+		list, err = idxs.ListAll()
+		require.NoError(t, err)
+		require.Len(t, list, len(idxcfgs)-1)
+	})
+}
diff --git a/database/table_test.go b/database/table_test.go
index d4c80611..405e8071 100644
--- a/database/table_test.go
+++ b/database/table_test.go
@@ -11,6 +11,17 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+func newTestTable(t testing.TB) (*database.Table, func()) {
+	tx, fn := newTestDB(t)
+
+	err := tx.CreateTable("test", nil)
+	require.NoError(t, err)
+	tb, err := tx.GetTable("test")
+	require.NoError(t, err)
+
+	return tb, fn
+}
+
 // TestTableIterate verifies Iterate behaviour.
 func TestTableIterate(t *testing.T) {
 	t.Run("Should not fail with no documents", func(t *testing.T) {
diff --git a/database/transaction_test.go b/database/transaction_test.go
index 1f6a88c4..bdae3a95 100644
--- a/database/transaction_test.go
+++ b/database/transaction_test.go
@@ -22,17 +22,6 @@ func newTestDB(t testing.TB) (*database.Transaction, func()) {
 	}
 }
 
-func newTestTable(t testing.TB) (*database.Table, func()) {
-	tx, fn := newTestDB(t)
-
-	err := tx.CreateTable("test", nil)
-	require.NoError(t, err)
-	tb, err := tx.GetTable("test")
-	require.NoError(t, err)
-
-	return tb, fn
-}
-
 func TestTxCreateIndex(t *testing.T) {
 	t.Run("Should create an index and return it", func(t *testing.T) {
 		tx, cleanup := newTestDB(t)
diff --git a/sql/query/drop_test.go b/sql/query/drop_test.go
index f7957f2e..42699013 100644
--- a/sql/query/drop_test.go
+++ b/sql/query/drop_test.go
@@ -4,36 +4,63 @@ import (
 	"testing"
 
 	"github.com/genjidb/genji"
+	"github.com/genjidb/genji/database"
 	"github.com/stretchr/testify/require"
 )
 
-func TestDrop(t *testing.T) {
-	tests := []struct {
-		name  string
-		query string
-		fails bool
-	}{
-		{"Drop table", "DROP TABLE test", false},
-		{"Drop table If not exists", "DROP TABLE IF EXISTS test", false},
-		{"Drop index", "DROP INDEX idx", false},
-		{"Drop index if exists", "DROP INDEX IF EXISTS idx", false},
-	}
-
-	for _, test := range tests {
-		t.Run(test.name, func(t *testing.T) {
-			db, err := genji.Open(":memory:")
-			require.NoError(t, err)
-			defer db.Close()
-
-			err = db.Exec("CREATE TABLE test; CREATE INDEX idx ON test (foo)")
-			require.NoError(t, err)
-
-			err = db.Exec(test.query)
-			if test.fails {
-				require.Error(t, err)
-				return
-			}
-			require.NoError(t, err)
-		})
-	}
+func TestDropTable(t *testing.T) {
+	db, err := genji.Open(":memory:")
+	require.NoError(t, err)
+	defer db.Close()
+
+	err = db.Exec("CREATE TABLE test1; CREATE TABLE test2; CREATE TABLE test3")
+	require.NoError(t, err)
+
+	err = db.Exec("DROP TABLE test1")
+	require.NoError(t, err)
+
+	err = db.Exec("DROP TABLE IF EXISTS test1")
+	require.NoError(t, err)
+
+	// Dropping a table that doesn't exist without "IF EXISTS"
+	// should return an error.
+	err = db.Exec("DROP TABLE test1")
+	require.Error(t, err)
+
+	// Assert that only the table `test1` has been dropped.
+	var tables []string
+	err = db.View(func(tx *genji.Tx) error {
+		var err error
+
+		tables, err = tx.ListTables()
+		return err
+	})
+	require.Len(t, tables, 2)
+}
+
+func TestDropIndex(t *testing.T) {
+	db, err := genji.Open(":memory:")
+	require.NoError(t, err)
+	defer db.Close()
+
+	err = db.Exec(`
+		CREATE TABLE test1(foo text); CREATE INDEX idx_test1_foo ON test1(foo);
+		CREATE TABLE test2(bar text); CREATE INDEX idx_test2_bar ON test2(bar);
+	`)
+	require.NoError(t, err)
+
+	err = db.Exec("DROP INDEX idx_test2_bar")
+	require.NoError(t, err)
+
+	// Assert that the good index has been dropped.
+	var indexes []*database.IndexConfig
+	err = db.View(func(tx *genji.Tx) error {
+		var err error
+		indexes, err = tx.ListIndexes()
+		return err
+	})
+	require.Len(t, indexes, 1)
+	require.Equal(t, "test1", indexes[0].TableName)
+	require.Equal(t, "idx_test1_foo", indexes[0].IndexName)
+	require.Equal(t, false, indexes[0].Unique)
 }