From 5f079163cbf2c5570f580f5d7eaa18ff69c7fde2 Mon Sep 17 00:00:00 2001 From: Travis Turner Date: Wed, 10 Apr 2019 16:30:40 -0500 Subject: [PATCH 1/2] validate (and panic) on duplicate PQL arguments --- pql/ast.go | 18 +++++++++++++++- pql/pqlpeg_test.go | 52 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 1 deletion(-) diff --git a/pql/ast.go b/pql/ast.go index d78ad2828..36d6e35fb 100644 --- a/pql/ast.go +++ b/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("multiple instances of argument '%s' provided", 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, diff --git a/pql/pqlpeg_test.go b/pql/pqlpeg_test.go index 8492d3531..f8a6597b5 100644 --- a/pql/pqlpeg_test.go +++ b/pql/pqlpeg_test.go @@ -676,3 +676,55 @@ func TestPQLDeepEquality(t *testing.T) { }) } } + +func TestPQLPanic(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) { + var v interface{} + func() { + defer func() { v = recover() }() + + q, err := ParseString(test.call) + if err != nil { + t.Fatalf("parsing query '%s': %v", test.call, err) + } + _ = q + }() + + if !reflect.DeepEqual(v, "multiple instances of argument 'a' provided") { + t.Fatalf("unexpected panic value: %#v", v) + } + + }) + } +} From bdc4f3b07eb7bb4b1eee3a5c148e58fc640e1509 Mon Sep 17 00:00:00 2001 From: Travis Turner Date: Wed, 10 Apr 2019 23:48:17 -0500 Subject: [PATCH 2/2] recover the duplicate arg panic from parser, treat as error --- pql/ast.go | 2 +- pql/parser.go | 20 +++++++++++++++++++- pql/pqlpeg_test.go | 37 ++++++++++++++++++++++--------------- 3 files changed, 42 insertions(+), 17 deletions(-) diff --git a/pql/ast.go b/pql/ast.go index 36d6e35fb..20b757946 100644 --- a/pql/ast.go +++ b/pql/ast.go @@ -119,7 +119,7 @@ func (q *Query) addField(field string) { // key/value. func (q *Query) validateArgField(elem *callStackElem) { if _, exists := elem.call.Args[elem.lastField]; exists { - panic(fmt.Sprintf("multiple instances of argument '%s' provided", elem.lastField)) + panic(fmt.Sprintf("%s: %s", duplicateArgErrorMessage, elem.lastField)) } } diff --git a/pql/parser.go b/pql/parser.go index 611294971..6a28f560e 100644 --- a/pql/parser.go +++ b/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 } diff --git a/pql/pqlpeg_test.go b/pql/pqlpeg_test.go index f8a6597b5..a90d58a5d 100644 --- a/pql/pqlpeg_test.go +++ b/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" @@ -677,7 +692,7 @@ func TestPQLDeepEquality(t *testing.T) { } } -func TestPQLPanic(t *testing.T) { +func TestDuplicateArgError(t *testing.T) { tests := []struct { name string call string @@ -710,21 +725,13 @@ func TestPQLPanic(t *testing.T) { } for i, test := range tests { t.Run(test.name+strconv.Itoa(i), func(t *testing.T) { - var v interface{} - func() { - defer func() { v = recover() }() - - q, err := ParseString(test.call) - if err != nil { - t.Fatalf("parsing query '%s': %v", test.call, err) - } - _ = q - }() - - if !reflect.DeepEqual(v, "multiple instances of argument 'a' provided") { - t.Fatalf("unexpected panic value: %#v", v) + _, 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()) } - }) } }