refactor: use a closeFn in the GetIndex instead of checking value

The closeFn implicitly checks the value as the implementation decides
what to do (either do nothing or close the index). Calling the closeFn
is safe and we don't need conditions.

Author: jvillafanez

services/search/pkg/engine/bleve.go

@@ -60,7 +60,8 @@ func NewBleveEngine(indexGetter bleveEngine.IndexGetter, queryCreator searchQuer
 // This method is useful if "memory" and "persistent" (not "persistentScale")
 // index getters are used.
 func (b *Bleve) Close() error {
-	bleveIndex, err := b.indexGetter.GetIndex()
+	// regardless of the implementation, we want to close the index
+	bleveIndex, _, err := b.indexGetter.GetIndex()
 	if err != nil {
 		return err
 	}
@@ -121,13 +122,11 @@ func BuildBleveMapping() (mapping.IndexMapping, error) {
 // Search executes a search request operation within the index.
 // Returns a SearchIndexResponse object or an error.
 func (b *Bleve) Search(ctx context.Context, sir *searchService.SearchIndexRequest) (*searchService.SearchIndexResponse, error) {
-	bleveIndex, err := b.indexGetter.GetIndex(bleveEngine.ReadOnly(true))
+	bleveIndex, closeFn, err := b.indexGetter.GetIndex(bleveEngine.ReadOnly(true))
 	if err != nil {
 		return nil, err
 	}
-	if b.indexGetter.IndexCanBeClosed() {
-		defer bleveIndex.Close()
-	}
+	defer closeFn()
 
 	createdQuery, err := b.queryCreator.Create(sir.Query)
 	if err != nil {
@@ -243,26 +242,22 @@ func (b *Bleve) Search(ctx context.Context, sir *searchService.SearchIndexReques
 
 // Upsert indexes or stores Resource data fields.
 func (b *Bleve) Upsert(id string, r Resource) error {
-	bleveIndex, err := b.indexGetter.GetIndex()
+	bleveIndex, closeFn, err := b.indexGetter.GetIndex()
 	if err != nil {
 		return err
 	}
-	if b.indexGetter.IndexCanBeClosed() {
-		defer bleveIndex.Close()
-	}
+	defer closeFn()
 
 	return bleveIndex.Index(id, r)
 }
 
 // Move updates the resource location and all of its necessary fields.
 func (b *Bleve) Move(id string, parentid string, target string) error {
-	bleveIndex, err := b.indexGetter.GetIndex()
+	bleveIndex, closeFn, err := b.indexGetter.GetIndex()
 	if err != nil {
 		return err
 	}
-	if b.indexGetter.IndexCanBeClosed() {
-		defer bleveIndex.Close()
-	}
+	defer closeFn()
 
 	r, err := b.getResource(bleveIndex, id)
 	if err != nil {
@@ -311,53 +306,45 @@ func (b *Bleve) Move(id string, parentid string, target string) error {
 // instead of removing the resource it just marks it as deleted!
 // can be undone
 func (b *Bleve) Delete(id string) error {
-	bleveIndex, err := b.indexGetter.GetIndex()
+	bleveIndex, closeFn, err := b.indexGetter.GetIndex()
 	if err != nil {
 		return err
 	}
-	if b.indexGetter.IndexCanBeClosed() {
-		defer bleveIndex.Close()
-	}
+	defer closeFn()
 
 	return b.setDeleted(bleveIndex, id, true)
 }
 
 // Restore is the counterpart to Delete.
 // It restores the resource which makes it available again.
 func (b *Bleve) Restore(id string) error {
-	bleveIndex, err := b.indexGetter.GetIndex()
+	bleveIndex, closeFn, err := b.indexGetter.GetIndex()
 	if err != nil {
 		return err
 	}
-	if b.indexGetter.IndexCanBeClosed() {
-		defer bleveIndex.Close()
-	}
+	defer closeFn()
 
 	return b.setDeleted(bleveIndex, id, false)
 }
 
 // Purge removes a resource from the index, irreversible operation.
 func (b *Bleve) Purge(id string) error {
-	bleveIndex, err := b.indexGetter.GetIndex()
+	bleveIndex, closeFn, err := b.indexGetter.GetIndex()
 	if err != nil {
 		return err
 	}
-	if b.indexGetter.IndexCanBeClosed() {
-		defer bleveIndex.Close()
-	}
+	defer closeFn()
 
 	return bleveIndex.Delete(id)
 }
 
 // DocCount returns the number of resources in the index.
 func (b *Bleve) DocCount() (uint64, error) {
-	bleveIndex, err := b.indexGetter.GetIndex(bleveEngine.ReadOnly(true))
+	bleveIndex, closeFn, err := b.indexGetter.GetIndex(bleveEngine.ReadOnly(true))
 	if err != nil {
 		return 0, err
 	}
-	if b.indexGetter.IndexCanBeClosed() {
-		defer bleveIndex.Close()
-	}
+	defer closeFn()
 
 	return bleveIndex.DocCount()
 }

services/search/pkg/engine/bleve/index.go

@@ -12,16 +12,26 @@ import (
 // Implementations might differ in how the index is created and how the
 // index is gotten (reused, created on the fly, etc).
 //
+// The GetIndex method returns a function that must be called to close the index.
 // Some implementations might require the index to be kept opened, meaning
 // the index should be closed only when the application is shutting down. In
-// this case, IndexCanBeClosed should return false. If the index can be
-// closed and reopened safely at any time, IndexCanBeClosed should
-// return true.
+// this case, the returned function to close the index should do nothing (not
+// closing the index). If the index can be closed and reopened safely at any
+// time, the returned function should close the index.
+// Calling the returned function to close the index is fine regardless of the
+// implementation, and it will act as a no-op if the index should be kept opened.
 type IndexGetter interface {
-	GetIndex(opts ...GetIndexOption) (bleve.Index, error)
-	IndexCanBeClosed() bool
+	GetIndex(opts ...GetIndexOption) (bleve.Index, func(), error)
 }
 
+// IndexGetterMemory is an implementation of IndexGetter that uses an in-memory
+// index. The implementation caches the index and returns the same index every
+// time GetIndex is called.
+// The data won't be persisted between runs, and closing the index will wipe
+// the data.
+// The close function returned by GetIndex won't do anything. The index should
+// be kept opened until the application is shutting down.
+// This is useful for testing and small datasets.
 type IndexGetterMemory struct {
 	mapping mapping.IndexMapping
 	index   bleve.Index
@@ -39,25 +49,26 @@ func NewIndexGetterMemory(mapping mapping.IndexMapping) *IndexGetterMemory {
 
 // GetIndex creates a new in-memory index every time it is called.
 // The options are ignored in this implementation.
-func (i *IndexGetterMemory) GetIndex(opts ...GetIndexOption) (bleve.Index, error) {
+func (i *IndexGetterMemory) GetIndex(opts ...GetIndexOption) (bleve.Index, func(), error) {
+	closeFn := func() {} // no-op
 	if i.index != nil {
-		return i.index, nil
+		return i.index, closeFn, nil
 	}
 
 	index, err := bleve.NewMemOnly(i.mapping)
 	if err != nil {
-		return nil, err
+		return nil, closeFn, err
 	}
 
 	i.index = index
-	return i.index, nil
-}
-
-// IndexCanBeClosed returns false, meaning the index must be kept opened.
-func (i *IndexGetterMemory) IndexCanBeClosed() bool {
-	return false
+	return i.index, closeFn, nil
 }
 
+// IndexGetterPersistent is an implementation of IndexGetter that persists the
+// index on the filesystem. The implementation caches the index and returns the
+// same index every time GetIndex is called.
+// The close function returned by GetIndex won't do anything. The index should
+// be kept opened until the application is shutting down.
 type IndexGetterPersistent struct {
 	rootDir string
 	mapping mapping.IndexMapping
@@ -79,29 +90,30 @@ func NewIndexGetterPersistent(rootDir string, mapping mapping.IndexMapping) *Ind
 
 // GetIndex returns the cached index. The options are ignored in this
 // implementation.
-func (i *IndexGetterPersistent) GetIndex(opts ...GetIndexOption) (bleve.Index, error) {
+func (i *IndexGetterPersistent) GetIndex(opts ...GetIndexOption) (bleve.Index, func(), error) {
+	closeFn := func() {} // no-op
 	if i.index != nil {
-		return i.index, nil
+		return i.index, closeFn, nil
 	}
 
 	destination := filepath.Join(i.rootDir, "bleve")
 	index, err := bleve.Open(destination)
 	if errors.Is(bleve.ErrorIndexPathDoesNotExist, err) {
 		index, err = bleve.New(destination, i.mapping)
 		if err != nil {
-			return nil, err
+			return nil, closeFn, err
 		}
 	}
 
 	i.index = index
-	return i.index, nil
-}
-
-// IndexCanBeClosed returns false, meaning the index must be kept opened.
-func (i *IndexGetterPersistent) IndexCanBeClosed() bool {
-	return false
+	return i.index, closeFn, nil
 }
 
+// IndexGetterPersistentScale is an implementation of IndexGetter that persists
+// the index on the filesystem. The implementation does not cache the index and
+// creates a new connection to the index every time GetIndex is called.
+// The close function returned by GetIndex must be called to close the index, as
+// soon as you the operations on the index are done.
 type IndexGetterPersistentScale struct {
 	rootDir string
 	mapping mapping.IndexMapping
@@ -124,7 +136,7 @@ func NewIndexGetterPersistentScale(rootDir string, mapping mapping.IndexMapping)
 // allow read-only operations to be performed in parallel.
 // In order to avoid blocking write operations, you should close the index
 // as soon as you are done with it.
-func (i *IndexGetterPersistentScale) GetIndex(opts ...GetIndexOption) (bleve.Index, error) {
+func (i *IndexGetterPersistentScale) GetIndex(opts ...GetIndexOption) (bleve.Index, func(), error) {
 	options := newGetIndexOptions(opts...)
 	destination := filepath.Join(i.rootDir, "bleve")
 	params := map[string]interface{}{
@@ -134,17 +146,12 @@ func (i *IndexGetterPersistentScale) GetIndex(opts ...GetIndexOption) (bleve.Ind
 	if errors.Is(bleve.ErrorIndexPathDoesNotExist, err) {
 		index, err = bleve.New(destination, i.mapping)
 		if err != nil {
-			return nil, err
+			closeFn := func() {} // no-op
+			return nil, closeFn, err
 		}
 
-		return index, nil
+		return index, func() { index.Close() }, nil
 	}
 
-	return index, err
-}
-
-// IndexCanBeClosed returns true, meaning the index can be closed and
-// reopened. You should close the index as soon as you are done with it.
-func (i *IndexGetterPersistentScale) IndexCanBeClosed() bool {
-	return true
+	return index, func() { index.Close() }, err
 }

services/search/pkg/engine/bleve/option.go

@@ -1,17 +1,23 @@
 package bleve
 
+// GetIndexOption is a function that sets some option for the GetIndex method.
 type GetIndexOption func(o *GetIndexOptions)
 
+// GetIndexOptions contains the options for the GetIndex method.
 type GetIndexOptions struct {
 	ReadOnly bool
 }
 
+// ReadOnly is an option to opens the index in read-only mode.
+// This option should allow running multiple read-only operations in parallel.
+// The behavior of write operations is not defined when this option is used.
 func ReadOnly(b bool) GetIndexOption {
 	return func(o *GetIndexOptions) {
 		o.ReadOnly = b
 	}
 }
 
+// newGetIndexOptions creates a new GetIndexOptions with the given options.
 func newGetIndexOptions(opts ...GetIndexOption) GetIndexOptions {
 	o := GetIndexOptions{}
 	for _, opt := range opts {