From 7c395ac4d13bbe8339c3de3e755b2e13a6119cb4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Kuba=20Podg=C3=B3rski?= <kuba--@users.noreply.github.com>
Date: Mon, 3 Feb 2020 17:41:06 +0100
Subject: [PATCH] Simplify Holder's logic for CreateIndex (#104)

---
 holder.go      | 28 +++++++++-------------------
 holder_test.go | 26 ++++++++++++++++++++++++++
 2 files changed, 35 insertions(+), 19 deletions(-)

diff --git a/holder.go b/holder.go
index 63ea13896..15aa17f6c 100644
--- a/holder.go
+++ b/holder.go
@@ -438,7 +438,7 @@ func (h *Holder) CreateIndex(name string, opt IndexOptions) (*Index, error) {
 	defer h.mu.Unlock()
 
 	// Ensure index doesn't already exist.
-	if h.indexes[name] != nil {
+	if h.index(name) != nil {
 		return nil, newConflictError(ErrIndexExists)
 	}
 	return h.createIndex(name, opt)
@@ -447,21 +447,15 @@ func (h *Holder) CreateIndex(name string, opt IndexOptions) (*Index, error) {
 // CreateIndexIfNotExists returns an index by name.
 // The index is created if it does not already exist.
 func (h *Holder) CreateIndexIfNotExists(name string, opt IndexOptions) (*Index, error) {
-	h.mu.RLock()
+	h.mu.Lock()
+	defer h.mu.Unlock()
 
-	// Find index in cache first.
-	if index := h.indexes[name]; index != nil {
-		h.mu.RUnlock()
+	// Return index if it exists.
+	if index := h.index(name); index != nil {
 		return index, nil
 	}
 
-	h.mu.RUnlock()
-
-	index, err := h.CreateIndex(name, opt)
-	if _, ok := err.(ConflictError); err != nil && !ok {
-		return nil, err
-	}
-	return index, nil
+	return h.createIndex(name, opt)
 }
 
 func (h *Holder) createIndex(name string, opt IndexOptions) (*Index, error) {
@@ -469,11 +463,6 @@ func (h *Holder) createIndex(name string, opt IndexOptions) (*Index, error) {
 		return nil, errors.New("index name required")
 	}
 
-	// Return index if it exists.
-	if index := h.index(name); index != nil {
-		return index, nil
-	}
-
 	// Otherwise create a new index.
 	index, err := h.newIndex(h.IndexPath(name), name)
 	if err != nil {
@@ -483,9 +472,10 @@ func (h *Holder) createIndex(name string, opt IndexOptions) (*Index, error) {
 	index.keys = opt.Keys
 	index.trackExistence = opt.TrackExistence
 
-	if err := index.Open(); err != nil {
+	if err = index.Open(); err != nil {
 		return nil, errors.Wrap(err, "opening")
-	} else if err := index.saveMeta(); err != nil {
+	}
+	if err = index.saveMeta(); err != nil {
 		return nil, errors.Wrap(err, "meta")
 	}
 
diff --git a/holder_test.go b/holder_test.go
index 6d2c5f0c7..2319a1f5f 100644
--- a/holder_test.go
+++ b/holder_test.go
@@ -275,6 +275,32 @@ func TestHolder_Open(t *testing.T) {
 				t.Fatalf("unexpected error: %s", err)
 			}
 		})
+
+		// Try to re-create existing index
+		t.Run("CreateIndexIfNotExists", func(t *testing.T) {
+			h := test.MustOpenHolder()
+			defer h.Close()
+
+			idx1, err := h.CreateIndexIfNotExists("aaa", pilosa.IndexOptions{})
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			if _, err = h.CreateIndex("aaa", pilosa.IndexOptions{}); err == nil {
+				t.Fatalf("expected: ConflictError, got: nil")
+			} else if _, ok := err.(pilosa.ConflictError); !ok {
+				t.Fatalf("expected: ConflictError, got: %s", err)
+			}
+
+			idx2, err := h.CreateIndexIfNotExists("aaa", pilosa.IndexOptions{})
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			if idx1 != idx2 {
+				t.Fatalf("expected the same indexes, got: %s and %s", idx1.Name(), idx2.Name())
+			}
+		})
 	})
 }