Repository: pilosa/pilosa
Pull request #2122 (2020-08-27 16:38:15)

Make not found error more verbose (add name)
## Overview

Make `not found index/field` error more verbose (contains name)

## Pull request checklist

- [X] I have read the [contributing guide](https://github.com/pilosa/pilosa/blob/master/CONTRIBUTING.md).
- [X] I have agreed to the [Contributor License Agreement](https://cla-assistant.io/pilosa/pilosa).
- [X] I have updated the [documentation](https://github.com/pilosa/pilosa/tree/master/docs).
- [X] I have resolved any merge conflicts.
- [X] I have included tests that cover my changes.
- [X] All new and existing tests pass.
- [ ] Make sure PR title conforms to convention in CHANGELOG.md.
- [ ] Add appropriate changelog label to PR (if applicable).

## Code review checklist
This is the checklist that the reviewer will follow while reviewing your pull request. You do not need to do anything with this checklist, but be aware of what the reviewer will be looking for.

- [ ] Ensure that any changes to external docs have been included in this pull request.
- [ ] If the changes require that minor/major versions need to be updated, tag the PR appropriately.
- [ ] Ensure the new code is [properly commented](https://github.com/golang/go/wiki/CodeReviewComments#doc-comments) and follows [Idiomatic Go](https://dmitri.shuralyov.com/idiomatic-go).
- [ ] Check that tests have been written and that they cover the new functionality.
- [ ] Run tests and ensure they pass.
- [ ] Build and run the code, performing any applicable integration testing.
- [ ] Make sure PR title conforms to convention in CHANGELOG.md.
- [ ] Make sure PR is tagged with appropriate changelog label.


travisturner: this looks ok, but i think the tests need some adjustments

Patch:
diff --git a/api.go b/api.go
index 875d350d..ef535b87 100644
--- a/api.go
+++ b/api.go
@@ -196,7 +196,7 @@ func (api *API) Index(ctx context.Context, indexName string) (*Index, error) {
 
 	index := api.holder.Index(indexName)
 	if index == nil {
-		return nil, newNotFoundError(ErrIndexNotFound)
+		return nil, newNotFoundError(ErrIndexNotFound, indexName)
 	}
 	return index, nil
 }
@@ -252,7 +252,7 @@ func (api *API) CreateField(ctx context.Context, indexName string, fieldName str
 	// Find index.
 	index := api.holder.Index(indexName)
 	if index == nil {
-		return nil, newNotFoundError(ErrIndexNotFound)
+		return nil, newNotFoundError(ErrIndexNotFound, indexName)
 	}
 
 	// Create field.
@@ -287,7 +287,7 @@ func (api *API) Field(ctx context.Context, indexName, fieldName string) (*Field,
 
 	field := api.holder.Field(indexName, fieldName)
 	if field == nil {
-		return nil, newNotFoundError(ErrFieldNotFound)
+		return nil, newNotFoundError(ErrFieldNotFound, fieldName)
 	}
 	return field, nil
 }
@@ -378,7 +378,7 @@ func (api *API) ImportRoaring(ctx context.Context, indexName, fieldName string,
 
 	field := api.holder.Field(indexName, fieldName)
 	if field == nil {
-		return newNotFoundError(ErrFieldNotFound)
+		return newNotFoundError(ErrFieldNotFound, fieldName)
 	}
 
 	// only set and time fields are supported
@@ -441,7 +441,7 @@ func (api *API) DeleteField(ctx context.Context, indexName string, fieldName str
 	// Find index.
 	index := api.holder.Index(indexName)
 	if index == nil {
-		return newNotFoundError(ErrIndexNotFound)
+		return newNotFoundError(ErrIndexNotFound, indexName)
 	}
 
 	// Delete field from the index.
@@ -472,7 +472,7 @@ func (api *API) DeleteAvailableShard(_ context.Context, indexName, fieldName str
 	// Find field.
 	field := api.holder.Field(indexName, fieldName)
 	if field == nil {
-		return newNotFoundError(ErrFieldNotFound)
+		return newNotFoundError(ErrFieldNotFound, fieldName)
 	}
 
 	// Delete shard from the cache.
@@ -514,13 +514,13 @@ func (api *API) ExportCSV(ctx context.Context, indexName string, fieldName strin
 	// Find index.
 	index := api.holder.Index(indexName)
 	if index == nil {
-		return newNotFoundError(ErrIndexNotFound)
+		return newNotFoundError(ErrIndexNotFound, indexName)
 	}
 
 	// Find field from the index.
 	field := index.Field(fieldName)
 	if field == nil {
-		return newNotFoundError(ErrFieldNotFound)
+		return newNotFoundError(ErrFieldNotFound, fieldName)
 	}
 
 	// Find the fragment.
@@ -768,7 +768,7 @@ func (api *API) Views(ctx context.Context, indexName string, fieldName string) (
 	// Retrieve views.
 	f := api.holder.Field(indexName, fieldName)
 	if f == nil {
-		return nil, ErrFieldNotFound
+		return nil, newNotFoundError(ErrFieldNotFound, fieldName)
 	}
 
 	// Fetch views.
@@ -788,7 +788,7 @@ func (api *API) DeleteView(ctx context.Context, indexName string, fieldName stri
 	// Retrieve field.
 	f := api.holder.Field(indexName, fieldName)
 	if f == nil {
-		return ErrFieldNotFound
+		return newNotFoundError(ErrFieldNotFound, fieldName)
 	}
 
 	// Delete the view.
@@ -825,7 +825,7 @@ func (api *API) IndexAttrDiff(ctx context.Context, indexName string, blocks []At
 	// Retrieve index from holder.
 	index := api.holder.Index(indexName)
 	if index == nil {
-		return nil, newNotFoundError(ErrIndexNotFound)
+		return nil, newNotFoundError(ErrIndexNotFound, indexName)
 	}
 
 	// Retrieve local blocks.
@@ -863,7 +863,7 @@ func (api *API) FieldAttrDiff(ctx context.Context, indexName string, fieldName s
 	// Retrieve index from holder.
 	f := api.holder.Field(indexName, fieldName)
 	if f == nil {
-		return nil, ErrFieldNotFound
+		return nil, newNotFoundError(ErrFieldNotFound, fieldName)
 	}
 
 	// Retrieve local blocks.
@@ -1177,14 +1177,14 @@ func (api *API) indexField(indexName string, fieldName string, shard uint64) (*I
 	index := api.holder.Index(indexName)
 	if index == nil {
 		api.server.logger.Printf("fragment error: index=%s, field=%s, shard=%d, err=%s", indexName, fieldName, shard, ErrIndexNotFound.Error())
-		return nil, nil, newNotFoundError(ErrIndexNotFound)
+		return nil, nil, newNotFoundError(ErrIndexNotFound, indexName)
 	}
 
 	// Retrieve field.
 	field := index.Field(fieldName)
 	if field == nil {
 		api.server.logger.Printf("field error: index=%s, field=%s, shard=%d, err=%s", indexName, fieldName, shard, ErrFieldNotFound.Error())
-		return nil, nil, ErrFieldNotFound
+		return nil, nil, newNotFoundError(ErrFieldNotFound, fieldName)
 	}
 	return index, field, nil
 }
diff --git a/cluster.go b/cluster.go
index 272c3aaa..f033ded8 100644
--- a/cluster.go
+++ b/cluster.go
@@ -1356,7 +1356,7 @@ func (c *cluster) followResizeInstruction(instr *ResizeInstruction) error {
 				// Retrieve field.
 				f := c.holder.Field(src.Index, src.Field)
 				if f == nil {
-					return ErrFieldNotFound
+					return newNotFoundError(ErrFieldNotFound, src.Field)
 				}
 
 				// Create view.
diff --git a/executor.go b/executor.go
index 1e030b82..2de696f6 100644
--- a/executor.go
+++ b/executor.go
@@ -128,7 +128,7 @@ func (e *executor) Execute(ctx context.Context, index string, q *pql.Query, shar
 
 	idx := e.Holder.Index(index)
 	if idx == nil {
-		return resp, ErrIndexNotFound
+		return resp, newNotFoundError(ErrIndexNotFound, index)
 	}
 
 	// Verify that the number of writes do not exceed the maximum.
@@ -241,7 +241,7 @@ func (e *executor) execute(ctx context.Context, index string, q *pql.Query, shar
 		// Round up the number of shards.
 		idx := e.Holder.Index(index)
 		if idx == nil {
-			return nil, ErrIndexNotFound
+			return nil, newNotFoundError(ErrIndexNotFound, index)
 		}
 		shards = idx.AvailableShards().Slice()
 		if len(shards) == 0 {
@@ -1320,12 +1320,12 @@ func (e *executor) executeRowsShard(_ context.Context, index string, fieldName s
 	// Fetch index.
 	idx := e.Holder.Index(index)
 	if idx == nil {
-		return nil, ErrIndexNotFound
+		return nil, newNotFoundError(ErrIndexNotFound, index)
 	}
 	// Fetch field.
 	f := e.Holder.Field(index, fieldName)
 	if f == nil {
-		return nil, ErrFieldNotFound
+		return nil, newNotFoundError(ErrFieldNotFound, fieldName)
 	}
 
 	// rowIDs is the result set.
@@ -1450,7 +1450,7 @@ func (e *executor) executeRowShard(ctx context.Context, index string, c *pql.Cal
 	// Fetch column label from index.
 	idx := e.Holder.Index(index)
 	if idx == nil {
-		return nil, ErrIndexNotFound
+		return nil, newNotFoundError(ErrIndexNotFound, index)
 	}
 
 	// Fetch field name from argument.
@@ -1460,7 +1460,7 @@ func (e *executor) executeRowShard(ctx context.Context, index string, c *pql.Cal
 	}
 	f := e.Holder.Field(index, fieldName)
 	if f == nil {
-		return nil, ErrFieldNotFound
+		return nil, newNotFoundError(ErrFieldNotFound, fieldName)
 	}
 
 	rowID, rowOK, rowErr := c.UintArg(fieldName)
@@ -1554,7 +1554,7 @@ func (e *executor) executeRowBSIGroupShard(ctx context.Context, index string, c
 
 	f := e.Holder.Field(index, fieldName)
 	if f == nil {
-		return nil, ErrFieldNotFound
+		return nil, newNotFoundError(ErrFieldNotFound, fieldName)
 	}
 
 	// EQ null           (not implemented: flip frag.NotNull with max ColumnID)
@@ -1744,7 +1744,7 @@ func (e *executor) executeNotShard(ctx context.Context, index string, c *pql.Cal
 	// Make sure the index supports existence tracking.
 	idx := e.Holder.Index(index)
 	if idx == nil {
-		return nil, ErrIndexNotFound
+		return nil, newNotFoundError(ErrIndexNotFound, index)
 	} else if idx.existenceField() == nil {
 		return nil, errors.Errorf("index does not support existence tracking: %s", index)
 	}
@@ -1834,11 +1834,11 @@ func (e *executor) executeClearBit(ctx context.Context, index string, c *pql.Cal
 	// Retrieve field.
 	idx := e.Holder.Index(index)
 	if idx == nil {
-		return false, ErrIndexNotFound
+		return false, newNotFoundError(ErrIndexNotFound, index)
 	}
 	f := idx.Field(fieldName)
 	if f == nil {
-		return false, ErrFieldNotFound
+		return false, newNotFoundError(ErrFieldNotFound, fieldName)
 	}
 
 	// Read fields using labels.
@@ -1904,7 +1904,7 @@ func (e *executor) executeClearRow(ctx context.Context, index string, c *pql.Cal
 	}
 	field := e.Holder.Field(index, fieldName)
 	if field == nil {
-		return false, ErrFieldNotFound
+		return false, newNotFoundError(ErrFieldNotFound, fieldName)
 	}
 
 	switch field.Type() {
@@ -1955,7 +1955,7 @@ func (e *executor) executeClearRowShard(ctx context.Context, index string, c *pq
 
 	field := e.Holder.Field(index, fieldName)
 	if field == nil {
-		return false, ErrFieldNotFound
+		return false, newNotFoundError(ErrFieldNotFound, fieldName)
 	}
 
 	// Remove the row from all views.
@@ -1984,7 +1984,7 @@ func (e *executor) executeSetRow(ctx context.Context, index string, c *pql.Call,
 	}
 	field := e.Holder.Field(index, fieldName)
 	if field == nil {
-		return false, ErrFieldNotFound
+		return false, newNotFoundError(ErrFieldNotFound, fieldName)
 	}
 	if field.Type() != FieldTypeSet {
 		return false, fmt.Errorf("can't Store() on a %s field", field.Type())
@@ -2025,7 +2025,7 @@ func (e *executor) executeSetRowShard(ctx context.Context, index string, c *pql.
 
 	field := e.Holder.Field(index, fieldName)
 	if field == nil {
-		return false, ErrFieldNotFound
+		return false, newNotFoundError(ErrFieldNotFound, fieldName)
 	}
 
 	// Retrieve source row.
@@ -2085,11 +2085,11 @@ func (e *executor) executeSet(ctx context.Context, index string, c *pql.Call, op
 	// Retrieve field.
 	idx := e.Holder.Index(index)
 	if idx == nil {
-		return false, ErrIndexNotFound
+		return false, newNotFoundError(ErrIndexNotFound, index)
 	}
 	f := idx.Field(fieldName)
 	if f == nil {
-		return false, ErrFieldNotFound
+		return false, newNotFoundError(ErrFieldNotFound, fieldName)
 	}
 
 	// Set column on existence field.
@@ -2216,7 +2216,7 @@ func (e *executor) executeSetRowAttrs(ctx context.Context, index string, c *pql.
 	// Retrieve field.
 	field := e.Holder.Field(index, fieldName)
 	if field == nil {
-		return ErrFieldNotFound
+		return newNotFoundError(ErrFieldNotFound, fieldName)
 	}
 
 	// Parse labels.
@@ -2285,7 +2285,7 @@ func (e *executor) executeBulkSetRowAttrs(ctx context.Context, index string, cal
 		// Retrieve field.
 		f := e.Holder.Field(index, field)
 		if f == nil {
-			return nil, ErrFieldNotFound
+			return nil, newNotFoundError(ErrFieldNotFound, field)
 		}
 
 		rowID, ok, err := c.UintArg("_" + rowLabel)
@@ -2323,7 +2323,7 @@ func (e *executor) executeBulkSetRowAttrs(ctx context.Context, index string, cal
 		// Retrieve field.
 		field := e.Holder.Field(index, name)
 		if field == nil {
-			return nil, ErrFieldNotFound
+			return nil, newNotFoundError(ErrFieldNotFound, name)
 		}
 
 		// Set attributes.
@@ -2367,7 +2367,7 @@ func (e *executor) executeSetColumnAttrs(ctx context.Context, index string, c *p
 	// Retrieve index.
 	idx := e.Holder.Index(index)
 	if idx == nil {
-		return ErrIndexNotFound
+		return newNotFoundError(ErrIndexNotFound, index)
 	}
 
 	col, okCol, errCol := c.UintArg("_" + columnLabel)
@@ -2857,7 +2857,7 @@ func (e *executor) translateResult(index string, idx *Index, call *pql.Call, res
 				// TODO: It may be useful to cache this field lookup.
 				field := idx.Field(g.Field)
 				if field == nil {
-					return nil, ErrFieldNotFound
+					return nil, newNotFoundError(ErrFieldNotFound, g.Field)
 				}
 				if field.keys() {
 					key, err := field.translateStore.TranslateID(g.RowID)
@@ -2884,7 +2884,7 @@ func (e *executor) translateResult(index string, idx *Index, call *pql.Call, res
 		}
 
 		if field := idx.Field(fieldName); field == nil {
-			return nil, ErrFieldNotFound
+			return nil, newNotFoundError(ErrFieldNotFound, fieldName)
 		} else if field.keys() {
 			other.Keys = make([]string, len(result))
 			for i, id := range result {
@@ -3099,7 +3099,7 @@ func newGroupByIterator(rowIDs []RowIDs, children []*pql.Call, filter *Row, inde
 			return nil, errors.Errorf("%s call must have field with valid (string) field name. Got %v of type %[2]T", call.Name, call.Args["_field"])
 		}
 		if holder.Field(index, fieldName) == nil {
-			return nil, ErrFieldNotFound
+			return nil, newNotFoundError(ErrFieldNotFound, fieldName)
 		}
 		gbi.fields[i].Field = fieldName
 		// Fetch fragment.
diff --git a/holder.go b/holder.go
index a73550c2..de015099 100644
--- a/holder.go
+++ b/holder.go
@@ -455,7 +455,7 @@ func (h *Holder) DeleteIndex(name string) error {
 	// Confirm index exists.
 	index := h.index(name)
 	if index == nil {
-		return newNotFoundError(ErrIndexNotFound)
+		return newNotFoundError(ErrIndexNotFound, name)
 	}
 
 	// Close index.
@@ -1042,7 +1042,7 @@ func (s *holderSyncer) syncField(index, name string) error {
 		// Retrieve attributes from differing blocks.
 		// Skip update and recomputation if no attributes have changed.
 		m, err := s.Cluster.InternalClient.RowAttrDiff(ctx, &node.URI, index, name, blks)
-		if err == ErrFieldNotFound {
+		if errors.Cause(err) == ErrFieldNotFound {
 			continue // field not created remotely yet, skip
 		} else if err != nil {
 			return errors.Wrap(err, "getting differing blocks")
@@ -1071,7 +1071,7 @@ func (s *holderSyncer) syncFragment(index, field, view string, shard uint64) err
 	// Retrieve local field.
 	f := s.Holder.Field(index, field)
 	if f == nil {
-		return ErrFieldNotFound
+		return newNotFoundError(ErrFieldNotFound, field)
 	}
 
 	// Ensure view exists locally.
diff --git a/http/client.go b/http/client.go
index b23678c7..46a6c1a6 100644
--- a/http/client.go
+++ b/http/client.go
@@ -999,7 +999,7 @@ func (c *InternalClient) RowAttrDiff(ctx context.Context, uri *pilosa.URI, index
 	resp, err := c.executeRequest(req.WithContext(ctx))
 	if err != nil {
 		if resp != nil && resp.StatusCode == http.StatusNotFound {
-			return nil, pilosa.ErrFieldNotFound
+			return nil, errors.Wrap(pilosa.ErrFieldNotFound, field)
 		}
 		return nil, err
 	}
diff --git a/index.go b/index.go
index 5c73809c..95650f0c 100644
--- a/index.go
+++ b/index.go
@@ -475,7 +475,7 @@ func (i *Index) DeleteField(name string) error {
 	// Confirm field exists.
 	f := i.field(name)
 	if f == nil {
-		return newNotFoundError(ErrFieldNotFound)
+		return newNotFoundError(ErrFieldNotFound, name)
 	}
 
 	// Close field.
diff --git a/pilosa.go b/pilosa.go
index 42ab3d3c..ebabf52d 100644
--- a/pilosa.go
+++ b/pilosa.go
@@ -108,13 +108,11 @@ func newConflictError(err error) ConflictError {
 
 // NotFoundError wraps an error value to signify that a resource was not found
 // such that in an HTTP scenario, http.StatusNotFound would be returned.
-type NotFoundError struct {
-	error
-}
+type NotFoundError error
 
 // newNotFoundError returns err wrapped in a NotFoundError.
-func newNotFoundError(err error) NotFoundError {
-	return NotFoundError{err}
+func newNotFoundError(err error, name string) NotFoundError {
+	return NotFoundError(errors.WithMessage(err, name))
 }
 
 // Regular expression to validate index and field names.


Test patch:
diff --git a/api_test.go b/api_test.go
index c019e8f6..85cb9260 100644
--- a/api_test.go
+++ b/api_test.go
@@ -135,11 +135,11 @@ func TestAPI_Import(t *testing.T) {
 		index := "rkci"
 		field := "f"
 
-		_, err := m0.API.CreateIndex(ctx, index, pilosa.IndexOptions{Keys: false})
+		_, err := m1.API.CreateIndex(ctx, index, pilosa.IndexOptions{Keys: false})
 		if err != nil {
 			t.Fatalf("creating index: %v", err)
 		}
-		_, err = m0.API.CreateField(ctx, index, field, pilosa.OptFieldTypeSet(pilosa.DefaultCacheType, 100), pilosa.OptFieldKeys())
+		_, err = m1.API.CreateField(ctx, index, field, pilosa.OptFieldTypeSet(pilosa.DefaultCacheType, 100), pilosa.OptFieldKeys())
 		if err != nil {
 			t.Fatalf("creating field: %v", err)
 		}
diff --git a/server/handler_test.go b/server/handler_test.go
index c67d9865..1dd3cae6 100644
--- a/server/handler_test.go
+++ b/server/handler_test.go
@@ -514,7 +514,7 @@ func TestHandler_Endpoints(t *testing.T) {
 		h.ServeHTTP(w, test.MustNewHTTPRequest("POST", "/index/i0/query", strings.NewReader(`Row(row=30)`)))
 		if w.Code != gohttp.StatusBadRequest {
 			t.Fatalf("unexpected status code: %d", w.Code)
-		} else if body := w.Body.String(); body != `{"error":"executing: map reduce: field not found"}`+"\n" {
+		} else if body := w.Body.String(); body != `{"error":"executing: map reduce: row: field not found"}`+"\n" {
 			t.Fatalf("unexpected body: %q", body)
 		}
 	})
@@ -531,7 +531,7 @@ func TestHandler_Endpoints(t *testing.T) {
 		var resp pilosa.QueryResponse
 		if err := cmd.API.Serializer.Unmarshal(w.Body.Bytes(), &resp); err != nil {
 			t.Fatal(err)
-		} else if s := resp.Err.Error(); s != `executing: map reduce: field not found` {
+		} else if s := resp.Err.Error(); s != `executing: map reduce: row: field not found` {
 			t.Fatalf("unexpected error: %s", s)
 		}
 	})
@@ -915,7 +915,7 @@ func TestHandler_Endpoints(t *testing.T) {
 		h.ServeHTTP(w, r)
 		if w.Code != gohttp.StatusNotFound {
 			t.Errorf("unexpected status code: %d", w.Code)
-		} else if w.Body.String() != `{"success":false,"error":{"message":"deleting field: field not found"}}`+"\n" {
+		} else if w.Body.String() != `{"success":false,"error":{"message":"deleting field: fld1: field not found"}}`+"\n" {
 			t.Errorf("unexpected body: %q", w.Body.String())
 		}
 
@@ -935,7 +935,7 @@ func TestHandler_Endpoints(t *testing.T) {
 		h.ServeHTTP(w, r)
 		if w.Code != gohttp.StatusNotFound {
 			t.Errorf("unexpected status code: %d", w.Code)
-		} else if w.Body.String() != `{"success":false,"error":{"message":"deleting index: index not found"}}`+"\n" {
+		} else if w.Body.String() != `{"success":false,"error":{"message":"deleting index: idx1: index not found"}}`+"\n" {
 			t.Errorf("unexpected body: %q", w.Body.String())
 		}
 	})
diff --git a/test/holder.go b/test/holder.go
index b5e7c126..c90278bf 100644
--- a/test/holder.go
+++ b/test/holder.go
@@ -21,6 +21,7 @@ import (
 
 	"github.com/pilosa/pilosa/v2"
 	"github.com/pilosa/pilosa/v2/boltdb"
+	"github.com/pkg/errors"
 )
 
 // Holder is a test wrapper for pilosa.Holder.
@@ -96,7 +97,7 @@ func (h *Holder) Row(index, field string, rowID uint64) *pilosa.Row {
 func (h *Holder) ReadRow(index, field string, rowID uint64) *pilosa.Row {
 	f := h.Holder.Field(index, field)
 	if f == nil {
-		panic(pilosa.ErrFieldNotFound)
+		panic(errors.WithMessage(pilosa.ErrFieldNotFound, field))
 	}
 	row, err := f.Row(rowID)
 	if err != nil {