Merge pull request #1938 from travisturner/duplicate-pql-args

validate (and panic) on duplicate PQL arguments

pql/ast.go

@@ -103,7 +103,9 @@ func (q *Query) endConditional() {
 
 func (q *Query) addField(field string) {
 	elem := q.lastCallStackElem()
-	if elem == nil || elem.lastField != "" {
+	if elem == nil {
+		panic(fmt.Sprintf("addField called with '%s' while element is nil", field))
+	} else if elem.lastField != "" {
 		panic(fmt.Sprintf("addField called with '%s' while field is not empty, it's: %s", field, elem.lastField))
 	}
 	elem.lastField = field
@@ -112,6 +114,15 @@ func (q *Query) addField(field string) {
 	}
 }
 
+// validateArgField ensures that field does not already
+// exist as a key in the Args map before adding the new
+// key/value.
+func (q *Query) validateArgField(elem *callStackElem) {
+	if _, exists := elem.call.Args[elem.lastField]; exists {
+		panic(fmt.Sprintf("%s: %s", duplicateArgErrorMessage, elem.lastField))
+	}
+}
+
 func (q *Query) addVal(val interface{}) {
 	elem := q.lastCallStackElem()
 	if elem == nil || elem.lastField == "" {
@@ -123,11 +134,13 @@ func (q *Query) addVal(val interface{}) {
 		return
 	}
 	if elem.lastCond != ILLEGAL {
+		q.validateArgField(elem) // case 1
 		elem.call.Args[elem.lastField] = &Condition{
 			Op:    elem.lastCond,
 			Value: val,
 		}
 	} else {
+		q.validateArgField(elem) // case 2
 		elem.call.Args[elem.lastField] = val
 	}
 	elem.lastField = ""
@@ -162,11 +175,13 @@ func (q *Query) addNumVal(val string) {
 		}
 		return
 	} else if elem.lastCond != ILLEGAL {
+		q.validateArgField(elem) // case 3
 		elem.call.Args[elem.lastField] = &Condition{
 			Op:    elem.lastCond,
 			Value: ival,
 		}
 	} else {
+		q.validateArgField(elem) // case 4
 		elem.call.Args[elem.lastField] = ival
 	}
 	elem.lastField = ""
@@ -175,6 +190,7 @@ func (q *Query) addNumVal(val string) {
 
 func (q *Query) startList() {
 	elem := q.lastCallStackElem()
+	q.validateArgField(elem) // case 5
 	if elem.lastCond != ILLEGAL {
 		elem.call.Args[elem.lastField] = &Condition{
 			Op:    elem.lastCond,

pql/parser.go

@@ -15,6 +15,7 @@
 package pql
 
 import (
+	"fmt"
 	"io"
 	"io/ioutil"
 	"strings"
@@ -25,6 +26,9 @@ import (
 // timeFormat is the go-style time format used to parse string dates.
 const timeFormat = "2006-01-02T15:04"
 
+// duplicateArgErrorMessage is used as an error string in the parser.
+const duplicateArgErrorMessage = "duplicate argument provided"
+
 // parser represents a parser for the PQL language.
 type parser struct {
 	r io.Reader
@@ -59,6 +63,20 @@ func (p *parser) Parse() (*Query, error) {
 	if err != nil {
 		return nil, errors.Wrap(err, "parsing")
 	}
-	p.Execute()
+
+	// Handle specific panics from the parser and return them as errors.
+	var v interface{}
+	func() {
+		defer func() { v = recover() }()
+		p.Execute()
+	}()
+	if v != nil {
+		if strings.HasPrefix(v.(string), duplicateArgErrorMessage) {
+			return nil, fmt.Errorf("%s", v)
+		} else {
+			panic(v)
+		}
+	}
+
 	return &p.Query, nil
 }

pql/pqlpeg_test.go

@@ -1,6 +1,21 @@
+// Copyright 2017 Pilosa Corp.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
 package pql
 
 import (
+	"fmt"
 	"reflect"
 	"strconv"
 	"testing"
@@ -676,3 +691,47 @@ func TestPQLDeepEquality(t *testing.T) {
 		})
 	}
 }
+
+func TestDuplicateArgError(t *testing.T) {
+	tests := []struct {
+		name string
+		call string
+	}{
+		// case 1
+		{
+			name: "StringConditional",
+			call: "Row(a==foo, a==bar)",
+		},
+		// case 2
+		{
+			name: "StringValue",
+			call: "Row(a=foo, a=bar)",
+		},
+		// case 3
+		{
+			name: "IntConditional",
+			call: "Row(a>5, a>6)",
+		},
+		// case 4
+		{
+			name: "IntValue",
+			call: "Row(a=7, a=8)",
+		},
+		// case 5
+		{
+			name: "List",
+			call: "Row(a=[7], a=[7,8])",
+		},
+	}
+	for i, test := range tests {
+		t.Run(test.name+strconv.Itoa(i), func(t *testing.T) {
+			_, err := ParseString(test.call)
+			expErr := fmt.Sprintf("%s: a", duplicateArgErrorMessage)
+			if err == nil {
+				t.Fatalf("expected error for duplicate argument: %s", test.call)
+			} else if err.Error() != expErr {
+				t.Fatalf("expected error: %s, but got: %v", expErr, err.Error())
+			}
+		})
+	}
+}