From 5bbb3e2065efc3965da05a76b1d2a3078bcb6165 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Kuba=20Podg=C3=B3rski?= <kuba--@users.noreply.github.com>
Date: Wed, 1 Jul 2020 11:36:42 +0200
Subject: [PATCH] FieldValue - check if column arg exists

---
 executor.go      | 12 ++++++++----
 executor_test.go | 11 ++++++-----
 pilosa.go        |  7 ++++---
 3 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/executor.go b/executor.go
index 57bc2abca..25dff738b 100644
--- a/executor.go
+++ b/executor.go
@@ -699,7 +699,12 @@ func (e *executor) executeIncludesColumnCall(ctx context.Context, index string,
 func (e *executor) executeFieldValueCall(ctx context.Context, index string, c *pql.Call, shards []uint64, opt *execOptions) (ValCount, error) {
 	fieldName, ok := c.Args["field"].(string)
 	if !ok || fieldName == "" {
-		return ValCount{}, errors.New("FieldValue(): field required")
+		return ValCount{}, ErrFieldRequired
+	}
+
+	colKey, ok := c.Args["column"]
+	if !ok || colKey == "" {
+		return ValCount{}, ErrColumnRequired
 	}
 
 	// Fetch index.
@@ -715,8 +720,8 @@ func (e *executor) executeFieldValueCall(ctx context.Context, index string, c *p
 	}
 
 	var colID uint64
-	if colKey, ok := c.Args["column"].(string); ok && idx.Keys() {
-		id, err := e.Cluster.translateIndexKey(ctx, index, colKey)
+	if key, ok := colKey.(string); ok && idx.Keys() {
+		id, err := e.Cluster.translateIndexKey(ctx, index, key)
 		if err != nil {
 			return ValCount{}, errors.Wrap(err, "getting column id")
 		}
@@ -724,7 +729,6 @@ func (e *executor) executeFieldValueCall(ctx context.Context, index string, c *p
 	} else {
 		id, ok, err := c.UintArg("column")
 		if !ok || err != nil {
-			// TODO: this error is getting swallowed somewhere (via curl)
 			return ValCount{}, errors.Wrap(err, "getting column argument")
 		}
 		colID = id
diff --git a/executor_test.go b/executor_test.go
index bc990dff9..0bbf3bde8 100644
--- a/executor_test.go
+++ b/executor_test.go
@@ -3522,7 +3522,6 @@ func TestExecutor_Execute_Not(t *testing.T) {
 func TestExecutor_Execute_FieldValue(t *testing.T) {
 	c := test.MustRunCluster(t, 2)
 	defer c.Close()
-	//hldr := test.Holder{Holder: c[0].Server.Holder()}
 
 	node0 := c[0]
 	node1 := c[1]
@@ -3535,8 +3534,8 @@ func TestExecutor_Execute_FieldValue(t *testing.T) {
 			Set(1, f=3)
 			Set(2, f=-4)
 			Set(` + strconv.Itoa(ShardWidth+1) + `, f=3)
-            Set(1, dec=12.985)
-            Set(2, dec=-4.234)
+			Set(1, dec=12.985)
+			Set(2, dec=-4.234)
 		`}); err != nil {
 		t.Fatal(err)
 	}
@@ -3548,8 +3547,8 @@ func TestExecutor_Execute_FieldValue(t *testing.T) {
 	if _, err := node0.API.Query(context.Background(), &pilosa.QueryRequest{Index: "ik", Query: `
 			Set("one", f=3)
 			Set("two", f=-4)
-            Set("one", dec=12.985)
-            Set("two", dec=-4.234)
+			Set("one", dec=12.985)
+			Set("two", dec=-4.234)
 		`}); err != nil {
 		t.Fatal(err)
 	}
@@ -3577,6 +3576,8 @@ func TestExecutor_Execute_FieldValue(t *testing.T) {
 
 		// Errors
 		{index: "i", qry: "FieldValue()", expErr: pilosa.ErrFieldRequired.Error()},
+		{index: "i", qry: "FieldValue(field=dec)", expErr: pilosa.ErrColumnRequired.Error()},
+		{index: "ik", qry: "FieldValue(field=f)", expErr: pilosa.ErrColumnRequired.Error()},
 	}
 	for n, node := range []*test.Command{node0, node1} {
 		for i, test := range tests {
diff --git a/pilosa.go b/pilosa.go
index 53733d9ce..481c8ff07 100644
--- a/pilosa.go
+++ b/pilosa.go
@@ -33,9 +33,10 @@ var (
 	ErrForeignIndexNotFound = errors.New("foreign index not found")
 
 	// ErrFieldRequired is returned when no field is specified.
-	ErrFieldRequired = errors.New("field required")
-	ErrFieldExists   = errors.New("field already exists")
-	ErrFieldNotFound = errors.New("field not found")
+	ErrFieldRequired  = errors.New("field required")
+	ErrColumnRequired = errors.New("column required")
+	ErrFieldExists    = errors.New("field already exists")
+	ErrFieldNotFound  = errors.New("field not found")
 
 	ErrBSIGroupNotFound         = errors.New("bsigroup not found")
 	ErrBSIGroupExists           = errors.New("bsigroup already exists")