Fix panic: index out of range (#346)

* Fix panic: index out of range

* Validate that right operand is provided

pkg/query/selection.go

@@ -18,6 +18,7 @@ package query
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"sort"
 	"strconv"
@@ -137,23 +138,24 @@ func NewCriterion(leftOp string, operator Operator, rightOp []string, criteriaTy
 
 // Validate the criterion fields
 func (c Criterion) Validate() error {
+	if len(c.RightOp) == 0 {
+		return errors.New("missing right operand")
+	}
+
 	if c.Type == ResultQuery {
 		if c.LeftOp == Limit {
 			limit, err := strconv.Atoi(c.RightOp[0])
 			if err != nil {
-				return fmt.Errorf("could not cast string to int: %s", err.Error())
+				return fmt.Errorf("could not convert string to int: %s", err.Error())
 			}
 			if limit < 0 {
 				return &util.UnsupportedQueryError{Message: fmt.Sprintf("limit (%d) is invalid. Limit should be positive number", limit)}
 			}
 		}
 
 		if c.LeftOp == OrderBy {
-			if len(c.RightOp) < 1 {
-				return &util.UnsupportedQueryError{Message: "order by result expects field and order type, but has none"}
-			}
 			if len(c.RightOp) < 2 {
-				return &util.UnsupportedQueryError{Message: fmt.Sprintf(`order by result for field "%s" expects order type, but has none`, c.RightOp[0])}
+				return &util.UnsupportedQueryError{Message: "order by result expects field name and order type"}
 			}
 		}
 

pkg/query/selection_test.go

@@ -21,9 +21,11 @@ import (
 	"fmt"
 	"strings"
 
-	. "github.com/Peripli/service-manager/pkg/query"
 	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/ginkgo/extensions/table"
 	. "github.com/onsi/gomega"
+
+	. "github.com/Peripli/service-manager/pkg/query"
 )
 
 var _ = Describe("Selection", func() {
@@ -54,7 +56,7 @@ var _ = Describe("Selection", func() {
 				addInvalidCriterion(ByField(LessThanOrEqualOperator, "leftOp", "non-numeric"))
 			})
 			Specify("Right operand containing new line", func() {
-				addInvalidCriterion(ByField(EqualsOperator, "leftOp", `value with 
+				addInvalidCriterion(ByField(EqualsOperator, "leftOp", `value with
 new line`))
 			})
 			Specify("Left operand with query separator", func() {
@@ -291,4 +293,47 @@ new line`))
 			})
 		}
 	})
+
+	DescribeTable("Validate Criterion",
+		func(c Criterion, expectedErr ...string) {
+			err := c.Validate()
+			if len(expectedErr) == 0 {
+				Expect(err).To(BeNil())
+			} else {
+				Expect(err).NotTo(BeNil())
+				Expect(err.Error()).To(ContainSubstring(expectedErr[0]))
+			}
+		},
+		Entry("Empty right operand is not allowed",
+			ByField(InOperator, "left"),
+			"missing right operand"),
+		Entry("Limit that is not a number is not allowed",
+			NewCriterion(Limit, NoOperator, []string{"not a number"}, ResultQuery),
+			"could not convert string to int"),
+		Entry("Negative limit is not allowed",
+			LimitResultBy(-1),
+			"should be positive"),
+		Entry("Valid limit is allowed",
+			LimitResultBy(10)),
+		Entry("Missing order type is not allowed",
+			NewCriterion(OrderBy, NoOperator, []string{"field"}, ResultQuery),
+			"expects field name and order type"),
+		Entry("Valid order by is allowed",
+			OrderResultBy("field", AscOrder)),
+		Entry("Multiple right operands are not allowed for univariate operators",
+			ByField(EqualsOperator, "left", "right1", "right2"),
+			"single value operation"),
+		Entry("Operators working with nil are not allowed for label queries",
+			ByLabel(EqualsOrNilOperator, "left", "right"),
+			"only for field queries"),
+		Entry("Non-numeric operand is not allowed by comparison operators",
+			ByField(LessThanOperator, "left", "not numeric"),
+			"not numeric or datetime"),
+		Entry("Separator word 'and' is not allowed to appear in left operand",
+			ByField(EqualsOperator, "band", "music"),
+			"separator and is not allowed"),
+		Entry("New line character is not allowed in right operand",
+			ByField(EqualsOperator, "left", "one\ntwo"),
+			"forbidden new line character"),
+	)
 })

storage/postgres/query_builder_test.go

@@ -193,7 +193,7 @@ ORDER BY id DESC, created_at ASC;`)))
 						}).
 						List(ctx)
 					Expect(err).Should(HaveOccurred())
-					Expect(err.Error()).To(ContainSubstring(`order by result for field "id" expects order type, but has none`))
+					Expect(err.Error()).To(ContainSubstring("order by result expects field name and order type"))
 				})
 			})
 
@@ -207,7 +207,7 @@ ORDER BY id DESC, created_at ASC;`)))
 						}).
 						List(ctx)
 					Expect(err).Should(HaveOccurred())
-					Expect(err.Error()).To(ContainSubstring("order by result expects field and order type, but has none"))
+					Expect(err.Error()).To(ContainSubstring("missing right operand"))
 				})
 			})
 		})
@@ -390,8 +390,8 @@ FROM visibilities ;`)))
 					Count(ctx)
 				Expect(err).ShouldNot(HaveOccurred())
 				Expect(executedQuery).Should(Equal(trim(`
-SELECT COUNT(DISTINCT visibilities.id) 
-FROM visibilities 
+SELECT COUNT(DISTINCT visibilities.id)
+FROM visibilities
 LIMIT ?;`)))
 				Expect(queryArgs).To(HaveLen(1))
 				Expect(queryArgs[0]).Should(Equal("10"))
@@ -508,7 +508,7 @@ WHERE visibilities.id = t.id ;`)))
 
 				Expect(executedQuery).Should(Equal(trim(`
 DELETE
-FROM visibilities 
+FROM visibilities
 WHERE visibilities.id::text = ? ;`)))
 				Expect(queryArgs).To(HaveLen(1))
 				Expect(queryArgs[0]).Should(Equal("1"))
@@ -529,8 +529,8 @@ WHERE visibilities.id::text = ? ;`)))
 				Expect(err).ToNot(HaveOccurred())
 
 				Expect(executedQuery).To(Equal(trim(`
-DELETE 
-FROM visibilities 
+DELETE
+FROM visibilities
 RETURNING id, service_plan_id;`)))
 			})
 
@@ -539,8 +539,8 @@ RETURNING id, service_plan_id;`)))
 				Expect(err).ToNot(HaveOccurred())
 
 				Expect(executedQuery).To(Equal(trim(`
-DELETE 
-FROM visibilities 
+DELETE
+FROM visibilities
 RETURNING *;`)))
 			})
 

storage/postgres/where_clause_tree.go

@@ -99,10 +99,12 @@ func criterionSQL(c query.Criterion, dbTags []tagType, tableAlias string) (strin
 
 func buildRightOp(operator query.Operator, rightOp []string) (string, interface{}) {
 	rightOpBindVar := "?"
-	var rhs interface{} = rightOp[0]
+	var rhs interface{}
 	if operator.Type() == query.MultivariateOperator {
 		rightOpBindVar = "(?)"
 		rhs = rightOp
+	} else {
+		rhs = rightOp[0]
 	}
 	return rightOpBindVar, rhs
 }