From 5b45ad62beaa03bed68904e26be21c987a6a8e17 Mon Sep 17 00:00:00 2001 From: Eric Schmidt Date: Mon, 2 May 2022 15:17:49 -0700 Subject: [PATCH 01/10] feat(datastore): adds in, not-in, and != query ops --- datastore/query.go | 19 ++++++++++++++++--- datastore/query_test.go | 15 +++++++++------ 2 files changed, 25 insertions(+), 9 deletions(-) diff --git a/datastore/query.go b/datastore/query.go index f2e6d9c2a6c..1a856c1c434 100644 --- a/datastore/query.go +++ b/datastore/query.go @@ -38,6 +38,9 @@ const ( equal greaterEq greaterThan + in + notIn + notEqual keyFieldName = "__key__" ) @@ -48,6 +51,9 @@ var operatorToProto = map[operator]pb.PropertyFilter_Operator{ equal: pb.PropertyFilter_EQUAL, greaterEq: pb.PropertyFilter_GREATER_THAN_OR_EQUAL, greaterThan: pb.PropertyFilter_GREATER_THAN, + in: pb.PropertyFilter_IN, + notIn: pb.PropertyFilter_NOT_IN, + notEqual: pb.PropertyFilter_NOT_EQUAL, } // filter is a conditional filter on query results. @@ -172,7 +178,8 @@ func (q *Query) Transaction(t *Transaction) *Query { // Filter returns a derivative query with a field-based filter. // The filterStr argument must be a field name followed by optional space, -// followed by an operator, one of ">", "<", ">=", "<=", or "=". +// followed by an operator, one of ">", "<", ">=", "<=", "=", "in", "not-in", +// and "!=". // Fields are compared against the provided value using the operator. // Multiple filters are AND'ed together. // Field names which contain spaces, quote marks, or operator characters @@ -180,13 +187,13 @@ func (q *Query) Transaction(t *Transaction) *Query { // or the fmt package's %q verb. func (q *Query) Filter(filterStr string, value interface{}) *Query { q = q.clone() - filterStr = strings.TrimSpace(filterStr) + filterStr = strings.ToLower(strings.TrimSpace(filterStr)) if filterStr == "" { q.err = fmt.Errorf("datastore: invalid filter %q", filterStr) return q } f := filter{ - FieldName: strings.TrimRight(filterStr, " ><=!"), + FieldName: strings.TrimRight(filterStr, " ><=!not-in"), Value: value, } switch op := strings.TrimSpace(filterStr[len(f.FieldName):]); op { @@ -200,6 +207,12 @@ func (q *Query) Filter(filterStr string, value interface{}) *Query { f.Op = greaterThan case "=": f.Op = equal + case "in": + f.Op = in + case "not-in": + f.Op = notIn + case "!=": + f.Op = notEqual default: q.err = fmt.Errorf("datastore: invalid operator %q in filter %q", op, filterStr) return q diff --git a/datastore/query_test.go b/datastore/query_test.go index c28723a212b..4990ae02a88 100644 --- a/datastore/query_test.go +++ b/datastore/query_test.go @@ -409,12 +409,15 @@ func TestFilterParser(t *testing.T) { {"x >", true, "x", greaterThan}, {"in >", true, "in", greaterThan}, {"in>", true, "in", greaterThan}, - // Valid but (currently) unsupported ops. - {"x!=", false, "", 0}, - {"x !=", false, "", 0}, - {" x != ", false, "", 0}, - {"x IN", false, "", 0}, - {"x in", false, "", 0}, + {"x!=", true, "x", notEqual}, + {"x !=", true, "x", notEqual}, + {" x != ", true, "x", notEqual}, + {"x IN", true, "x", in}, + {"x in", true, "x", in}, + {"x NOT-IN", true, "x", notIn}, + {"x not-in", true, "x", notIn}, + {"ins not-in", true, "ins", notIn}, + {"in not-in", true, "in", notIn}, // Invalid ops. {"x EQ", false, "", 0}, {"x lt", false, "", 0}, From 2b89638f0877fa6741f57f4f4f7e251e2a5b1b3c Mon Sep 17 00:00:00 2001 From: Eric Schmidt Date: Tue, 3 May 2022 12:32:52 -0700 Subject: [PATCH 02/10] fix: tests --- datastore/query.go | 32 +++++++++++--- datastore/query_test.go | 92 ++++++++++++++++++++++++++++++++++++++--- 2 files changed, 112 insertions(+), 12 deletions(-) diff --git a/datastore/query.go b/datastore/query.go index 1a856c1c434..a8168e59ded 100644 --- a/datastore/query.go +++ b/datastore/query.go @@ -176,27 +176,47 @@ func (q *Query) Transaction(t *Transaction) *Query { return q } +// DEPRECATED. Use the FilterWithArgs() method instead, which supports more +// more operations. +// // Filter returns a derivative query with a field-based filter. // The filterStr argument must be a field name followed by optional space, -// followed by an operator, one of ">", "<", ">=", "<=", "=", "in", "not-in", -// and "!=". +// followed by an operator, one of ">", "<", ">=", "<=", "=", and "!=". // Fields are compared against the provided value using the operator. // Multiple filters are AND'ed together. // Field names which contain spaces, quote marks, or operator characters // should be passed as quoted Go string literals as returned by strconv.Quote // or the fmt package's %q verb. func (q *Query) Filter(filterStr string, value interface{}) *Query { + // TODO( #5977 ): Add better string parsing (or something) q = q.clone() filterStr = strings.ToLower(strings.TrimSpace(filterStr)) if filterStr == "" { q.err = fmt.Errorf("datastore: invalid filter %q", filterStr) return q } + f := strings.TrimRight(filterStr, " ><=!") + op := strings.TrimSpace(filterStr[len(f):]) + return q.FilterWithArgs(f, op, value) +} + +// FilterWithArgs returns a derivative query with a field-based filter. +// The operation parameter takes the following strings: ">", "<", ">=", "<=", +// "=", "!=", "in", and "not-in". +// Fields are compared against the provided value using the operator. +// Multiple filters are AND'ed together. +// Field names which contain spaces, quote marks, or operator characters +// should be passed as quoted Go string literals as returned by strconv.Quote +// or the fmt package's %q verb. +func (q *Query) FilterWithArgs(fieldName, operation string, val interface{}) *Query { + q = q.clone() + f := filter{ - FieldName: strings.TrimRight(filterStr, " ><=!not-in"), - Value: value, + FieldName: fieldName, + Value: val, } - switch op := strings.TrimSpace(filterStr[len(f.FieldName):]); op { + + switch o := strings.TrimSpace(strings.ToLower(operation)); o { case "<=": f.Op = lessEq case ">=": @@ -214,7 +234,7 @@ func (q *Query) Filter(filterStr string, value interface{}) *Query { case "!=": f.Op = notEqual default: - q.err = fmt.Errorf("datastore: invalid operator %q in filter %q", op, filterStr) + q.err = fmt.Errorf("datastore: invalid operator %q in filter", operation) return q } var err error diff --git a/datastore/query_test.go b/datastore/query_test.go index 4990ae02a88..9b15ffc3118 100644 --- a/datastore/query_test.go +++ b/datastore/query_test.go @@ -412,13 +412,13 @@ func TestFilterParser(t *testing.T) { {"x!=", true, "x", notEqual}, {"x !=", true, "x", notEqual}, {" x != ", true, "x", notEqual}, - {"x IN", true, "x", in}, - {"x in", true, "x", in}, - {"x NOT-IN", true, "x", notIn}, - {"x not-in", true, "x", notIn}, - {"ins not-in", true, "ins", notIn}, - {"in not-in", true, "in", notIn}, // Invalid ops. + {"x IN", false, "", 0}, + {"x in", false, "", 0}, + {"x NOT-IN", false, "", 0}, + {"x not-in", false, "", 0}, + {"ins not-in", false, "", 0}, + {"in not-in", false, "", 0}, {"x EQ", false, "", 0}, {"x lt", false, "", 0}, {"x <>", false, "", 0}, @@ -459,6 +459,86 @@ func TestFilterParser(t *testing.T) { } } +func TestFilterWithArgs(t *testing.T) { + testCases := []struct { + f string + o string + wantOK bool + wantOp operator + }{ + // Supported ops. + {"x", "<", true, lessThan}, + {" x ", " < ", true, lessThan}, + {"x", "<=", true, lessEq}, + {"x", "=", true, equal}, + {"x", ">=", true, greaterEq}, + {"x", ">", true, greaterThan}, + {"in", ">", true, greaterThan}, + {"x", "!=", true, notEqual}, + {"x", "IN", true, in}, + {"x", "in", true, in}, + {"in", "in", true, in}, + {"x", "NOT-IN", true, notIn}, + {"x", "not-in", true, notIn}, + {"ins", "not-in", true, notIn}, + {"in", "not-in", true, notIn}, + // Invalid ops. + {"x", "EQ", false, 0}, + {"x", "lt", false, 0}, + {"x", "<>", false, 0}, + {"x", ">>", false, 0}, + {"x", "==", false, 0}, + {"x", "=<", false, 0}, + {"x", "=>", false, 0}, + {"x", "!", false, 0}, + // Quoted and interesting field names. + {"x > y", "=", true, equal}, + {`" x`, "=", false, 0}, + {"` x \"", "=", false, 0}, + } + for _, tc := range testCases { + q := NewQuery("foo").FilterWithArgs(tc.f, tc.o, 42) + if ok := q.err == nil; ok != tc.wantOK { + t.Errorf("%q %q: ok=%t, want %t", tc.f, tc.o, ok, tc.wantOK) + continue + } + if !tc.wantOK { + continue + } + if len(q.filter) != 1 { + t.Errorf("%q: len=%d, want %d", tc.f, len(q.filter), 1) + continue + } + got, want := q.filter[0], filter{tc.f, tc.wantOp, 42} + if got != want { + t.Errorf("%q %q: got %v, want %v", tc.f, tc.o, got, want) + continue + } + } +} + +func TestUnquote(t *testing.T) { + testCases := []struct { + input string + want string + }{ + {`" x "`, ` x `}, + {`"\" \\\"x \""`, `" \"x "`}, + } + + for _, tc := range testCases { + got, err := unquote(tc.input) + + if err != nil { + t.Errorf("error parsing field name: %v", err) + } + + if got != tc.want { + t.Errorf("field name parsing error: \nwant %v,\ngot %v", tc.want, got) + } + } +} + func TestNamespaceQuery(t *testing.T) { gotNamespace := make(chan string, 1) ctx := context.Background() From 377db4a6a8d817c40b75143102b4960dea0c0a54 Mon Sep 17 00:00:00 2001 From: Eric Schmidt Date: Tue, 3 May 2022 12:53:34 -0700 Subject: [PATCH 03/10] fix: lint --- datastore/query.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/datastore/query.go b/datastore/query.go index a8168e59ded..c23d5ae8b37 100644 --- a/datastore/query.go +++ b/datastore/query.go @@ -176,7 +176,7 @@ func (q *Query) Transaction(t *Transaction) *Query { return q } -// DEPRECATED. Use the FilterWithArgs() method instead, which supports more +// Deprecated. Use the FilterWithArgs() method instead, which supports more // more operations. // // Filter returns a derivative query with a field-based filter. From cf0af83cf733baf026cce78427301223459188ec Mon Sep 17 00:00:00 2001 From: Eric Schmidt Date: Tue, 3 May 2022 13:04:10 -0700 Subject: [PATCH 04/10] fix: adjusted string operations --- datastore/query.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/datastore/query.go b/datastore/query.go index c23d5ae8b37..08b191d5a21 100644 --- a/datastore/query.go +++ b/datastore/query.go @@ -190,7 +190,7 @@ func (q *Query) Transaction(t *Transaction) *Query { func (q *Query) Filter(filterStr string, value interface{}) *Query { // TODO( #5977 ): Add better string parsing (or something) q = q.clone() - filterStr = strings.ToLower(strings.TrimSpace(filterStr)) + filterStr = strings.TrimSpace(filterStr) if filterStr == "" { q.err = fmt.Errorf("datastore: invalid filter %q", filterStr) return q @@ -208,12 +208,12 @@ func (q *Query) Filter(filterStr string, value interface{}) *Query { // Field names which contain spaces, quote marks, or operator characters // should be passed as quoted Go string literals as returned by strconv.Quote // or the fmt package's %q verb. -func (q *Query) FilterWithArgs(fieldName, operation string, val interface{}) *Query { +func (q *Query) FilterWithArgs(fieldName, operation string, value interface{}) *Query { q = q.clone() f := filter{ FieldName: fieldName, - Value: val, + Value: value, } switch o := strings.TrimSpace(strings.ToLower(operation)); o { From 0aad2698e3279d2bc8ffbed9b3b3d350b5bbd16e Mon Sep 17 00:00:00 2001 From: Eric Schmidt Date: Tue, 3 May 2022 13:54:44 -0700 Subject: [PATCH 05/10] fix: linter --- datastore/query.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/datastore/query.go b/datastore/query.go index 08b191d5a21..e90e978d2e0 100644 --- a/datastore/query.go +++ b/datastore/query.go @@ -176,10 +176,11 @@ func (q *Query) Transaction(t *Transaction) *Query { return q } -// Deprecated. Use the FilterWithArgs() method instead, which supports more -// more operations. -// // Filter returns a derivative query with a field-based filter. +// +// DEPRECATED. Use the FilterWithArgs() method instead, which supports the same +// set of operations (and more). +// // The filterStr argument must be a field name followed by optional space, // followed by an operator, one of ">", "<", ">=", "<=", "=", and "!=". // Fields are compared against the provided value using the operator. From 4c5c864da677abfbd8b1c96d5a08a8c1c2467f52 Mon Sep 17 00:00:00 2001 From: Eric Schmidt Date: Thu, 5 May 2022 10:50:49 -0700 Subject: [PATCH 06/10] fix: linter --- datastore/query.go | 3 +-- datastore/query_test.go | 4 ++-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/datastore/query.go b/datastore/query.go index e90e978d2e0..49393523ef8 100644 --- a/datastore/query.go +++ b/datastore/query.go @@ -190,7 +190,6 @@ func (q *Query) Transaction(t *Transaction) *Query { // or the fmt package's %q verb. func (q *Query) Filter(filterStr string, value interface{}) *Query { // TODO( #5977 ): Add better string parsing (or something) - q = q.clone() filterStr = strings.TrimSpace(filterStr) if filterStr == "" { q.err = fmt.Errorf("datastore: invalid filter %q", filterStr) @@ -217,7 +216,7 @@ func (q *Query) FilterWithArgs(fieldName, operation string, value interface{}) * Value: value, } - switch o := strings.TrimSpace(strings.ToLower(operation)); o { + switch o := strings.TrimSpace(operation); o { case "<=": f.Op = lessEq case ">=": diff --git a/datastore/query_test.go b/datastore/query_test.go index 9b15ffc3118..72b4b2a09a9 100644 --- a/datastore/query_test.go +++ b/datastore/query_test.go @@ -475,14 +475,14 @@ func TestFilterWithArgs(t *testing.T) { {"x", ">", true, greaterThan}, {"in", ">", true, greaterThan}, {"x", "!=", true, notEqual}, - {"x", "IN", true, in}, {"x", "in", true, in}, {"in", "in", true, in}, - {"x", "NOT-IN", true, notIn}, {"x", "not-in", true, notIn}, {"ins", "not-in", true, notIn}, {"in", "not-in", true, notIn}, // Invalid ops. + {"x", "IN", false, 0}, + {"x", "NOT-IN", false, 0}, {"x", "EQ", false, 0}, {"x", "lt", false, 0}, {"x", "<>", false, 0}, From 6169ccbaaec336432d283b855c5815f33f2832eb Mon Sep 17 00:00:00 2001 From: Eric Schmidt Date: Thu, 5 May 2022 11:59:01 -0700 Subject: [PATCH 07/10] fix: per reviewer --- datastore/query.go | 10 +++++----- datastore/query_test.go | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/datastore/query.go b/datastore/query.go index 49393523ef8..032a97f0a84 100644 --- a/datastore/query.go +++ b/datastore/query.go @@ -197,10 +197,10 @@ func (q *Query) Filter(filterStr string, value interface{}) *Query { } f := strings.TrimRight(filterStr, " ><=!") op := strings.TrimSpace(filterStr[len(f):]) - return q.FilterWithArgs(f, op, value) + return q.FilterField(f, op, value) } -// FilterWithArgs returns a derivative query with a field-based filter. +// FilterField returns a derivative query with a field-based filter. // The operation parameter takes the following strings: ">", "<", ">=", "<=", // "=", "!=", "in", and "not-in". // Fields are compared against the provided value using the operator. @@ -208,7 +208,7 @@ func (q *Query) Filter(filterStr string, value interface{}) *Query { // Field names which contain spaces, quote marks, or operator characters // should be passed as quoted Go string literals as returned by strconv.Quote // or the fmt package's %q verb. -func (q *Query) FilterWithArgs(fieldName, operation string, value interface{}) *Query { +func (q *Query) FilterField(fieldName, operator string, value interface{}) *Query { q = q.clone() f := filter{ @@ -216,7 +216,7 @@ func (q *Query) FilterWithArgs(fieldName, operation string, value interface{}) * Value: value, } - switch o := strings.TrimSpace(operation); o { + switch o := strings.TrimSpace(operator); o { case "<=": f.Op = lessEq case ">=": @@ -234,7 +234,7 @@ func (q *Query) FilterWithArgs(fieldName, operation string, value interface{}) * case "!=": f.Op = notEqual default: - q.err = fmt.Errorf("datastore: invalid operator %q in filter", operation) + q.err = fmt.Errorf("datastore: invalid operator %q in filter", operator) return q } var err error diff --git a/datastore/query_test.go b/datastore/query_test.go index 72b4b2a09a9..02d2c48804a 100644 --- a/datastore/query_test.go +++ b/datastore/query_test.go @@ -497,7 +497,7 @@ func TestFilterWithArgs(t *testing.T) { {"` x \"", "=", false, 0}, } for _, tc := range testCases { - q := NewQuery("foo").FilterWithArgs(tc.f, tc.o, 42) + q := NewQuery("foo").FilterField(tc.f, tc.o, 42) if ok := q.err == nil; ok != tc.wantOK { t.Errorf("%q %q: ok=%t, want %t", tc.f, tc.o, ok, tc.wantOK) continue From 6b96029fe898eb061f594737ff6cb57d443ed959 Mon Sep 17 00:00:00 2001 From: Eric Schmidt Date: Thu, 5 May 2022 12:34:13 -0700 Subject: [PATCH 08/10] Update datastore/query_test.go Co-authored-by: Tyler Bui-Palsulich <26876514+tbpg@users.noreply.github.com> --- datastore/query_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/datastore/query_test.go b/datastore/query_test.go index 02d2c48804a..cf501f44a8e 100644 --- a/datastore/query_test.go +++ b/datastore/query_test.go @@ -459,7 +459,7 @@ func TestFilterParser(t *testing.T) { } } -func TestFilterWithArgs(t *testing.T) { +func TestFilterField(t *testing.T) { testCases := []struct { f string o string From 46a6703b3c5296e5dc855aa05dbf4949d78b55de Mon Sep 17 00:00:00 2001 From: Eric Schmidt Date: Thu, 5 May 2022 12:34:48 -0700 Subject: [PATCH 09/10] Update datastore/query.go Co-authored-by: Tyler Bui-Palsulich <26876514+tbpg@users.noreply.github.com> --- datastore/query.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/datastore/query.go b/datastore/query.go index 032a97f0a84..c2ed359eeaf 100644 --- a/datastore/query.go +++ b/datastore/query.go @@ -178,7 +178,7 @@ func (q *Query) Transaction(t *Transaction) *Query { // Filter returns a derivative query with a field-based filter. // -// DEPRECATED. Use the FilterWithArgs() method instead, which supports the same +// Deprecated: Use the FilterField method instead, which supports the same // set of operations (and more). // // The filterStr argument must be a field name followed by optional space, From abcb1f3543b77dacbf752f09d166bc9757773f69 Mon Sep 17 00:00:00 2001 From: Eric Schmidt Date: Thu, 5 May 2022 14:19:03 -0700 Subject: [PATCH 10/10] fix: per reviewer --- datastore/query_test.go | 173 +++++++++++++++++----------------------- 1 file changed, 75 insertions(+), 98 deletions(-) diff --git a/datastore/query_test.go b/datastore/query_test.go index cf501f44a8e..72c0544eab9 100644 --- a/datastore/query_test.go +++ b/datastore/query_test.go @@ -391,60 +391,61 @@ func TestQueriesAreImmutable(t *testing.T) { } } -func TestFilterParser(t *testing.T) { - testCases := []struct { - filterStr string - wantOK bool - wantFieldName string - wantOp operator - }{ - // Supported ops. - {"x<", true, "x", lessThan}, - {"x <", true, "x", lessThan}, - {"x <", true, "x", lessThan}, - {" x < ", true, "x", lessThan}, - {"x <=", true, "x", lessEq}, - {"x =", true, "x", equal}, - {"x >=", true, "x", greaterEq}, - {"x >", true, "x", greaterThan}, - {"in >", true, "in", greaterThan}, - {"in>", true, "in", greaterThan}, - {"x!=", true, "x", notEqual}, - {"x !=", true, "x", notEqual}, - {" x != ", true, "x", notEqual}, - // Invalid ops. - {"x IN", false, "", 0}, - {"x in", false, "", 0}, - {"x NOT-IN", false, "", 0}, - {"x not-in", false, "", 0}, - {"ins not-in", false, "", 0}, - {"in not-in", false, "", 0}, - {"x EQ", false, "", 0}, - {"x lt", false, "", 0}, - {"x <>", false, "", 0}, - {"x >>", false, "", 0}, - {"x ==", false, "", 0}, - {"x =<", false, "", 0}, - {"x =>", false, "", 0}, - {"x !", false, "", 0}, - {"x ", false, "", 0}, - {"x", false, "", 0}, - // Quoted and interesting field names. - {"x > y =", true, "x > y", equal}, - {"` x ` =", true, " x ", equal}, - {`" x " =`, true, " x ", equal}, - {`" \"x " =`, true, ` "x `, equal}, - {`" x =`, false, "", 0}, - {`" x ="`, false, "", 0}, - {"` x \" =", false, "", 0}, +type testFilterCase struct { + filterStr string + fieldName string + operator string + wantOp operator + wantFieldName string +} + +var ( + // Supported ops both filters. + filterTestCases = []testFilterCase{ + {"x<", "x", "<", lessThan, "x"}, + {"x <", "x", "<", lessThan, "x"}, + {"x <", "x", "<", lessThan, "x"}, + {" x < ", "x", "<", lessThan, "x"}, + {"x <=", "x", "<=", lessEq, "x"}, + {"x =", "x", "=", equal, "x"}, + {"x >=", "x", ">=", greaterEq, "x"}, + {"x >", "x", ">", greaterThan, "x"}, + {"in >", "in", ">", greaterThan, "in"}, + {"in>", "in", ">", greaterThan, "in"}, + {"x!=", "x", "!=", notEqual, "x"}, + {"x !=", "x", "!=", notEqual, "x"}, + {" x != ", "x", "!=", notEqual, "x"}, + } + // Supported in FilterField only. + filterFieldTestCases = []testFilterCase{ + {"x in", "x", "in", in, "x"}, + {"x not-in", "x", "not-in", notIn, "x"}, + {"ins in", "ins", "in", in, "ins"}, + {"in not-in", "in", "not-in", notIn, "in"}, + } + // Operators not supported in either filter method + filterUnsupported = []testFilterCase{ + {"x IN", "x", "IN", 0, ""}, + {"x NOT-IN", "x", "NOT-IN", 0, ""}, + {"x EQ", "x", "EQ", 0, ""}, + {"x lt", "x", "lt", 0, ""}, + {"x <>", "x", "<>", 0, ""}, + {"x >>", "x", ">>", 0, ""}, + {"x ==", "x", "==", 0, ""}, + {"x =<", "x", "=<", 0, ""}, + {"x =>", "x", "=>", 0, ""}, + {"x !", "x", "!", 0, ""}, + {"x ", "x", "", 0, ""}, + {"x", "x", "", 0, ""}, } - for _, tc := range testCases { +) + +func TestFilterParser(t *testing.T) { + // Success cases + for _, tc := range filterTestCases { q := NewQuery("foo").Filter(tc.filterStr, 42) - if ok := q.err == nil; ok != tc.wantOK { - t.Errorf("%q: ok=%t, want %t", tc.filterStr, ok, tc.wantOK) - continue - } - if !tc.wantOK { + if q.err != nil { + t.Errorf("%q: error=%v", tc.filterStr, q.err) continue } if len(q.filter) != 1 { @@ -457,64 +458,40 @@ func TestFilterParser(t *testing.T) { continue } } + // Failure cases + failureTestCases := append(filterFieldTestCases, filterUnsupported...) + for _, tc := range failureTestCases { + q := NewQuery("foo").Filter(tc.filterStr, 42) + if q.err == nil { + t.Errorf("%q: should have thrown error", tc.filterStr) + } + } } func TestFilterField(t *testing.T) { - testCases := []struct { - f string - o string - wantOK bool - wantOp operator - }{ - // Supported ops. - {"x", "<", true, lessThan}, - {" x ", " < ", true, lessThan}, - {"x", "<=", true, lessEq}, - {"x", "=", true, equal}, - {"x", ">=", true, greaterEq}, - {"x", ">", true, greaterThan}, - {"in", ">", true, greaterThan}, - {"x", "!=", true, notEqual}, - {"x", "in", true, in}, - {"in", "in", true, in}, - {"x", "not-in", true, notIn}, - {"ins", "not-in", true, notIn}, - {"in", "not-in", true, notIn}, - // Invalid ops. - {"x", "IN", false, 0}, - {"x", "NOT-IN", false, 0}, - {"x", "EQ", false, 0}, - {"x", "lt", false, 0}, - {"x", "<>", false, 0}, - {"x", ">>", false, 0}, - {"x", "==", false, 0}, - {"x", "=<", false, 0}, - {"x", "=>", false, 0}, - {"x", "!", false, 0}, - // Quoted and interesting field names. - {"x > y", "=", true, equal}, - {`" x`, "=", false, 0}, - {"` x \"", "=", false, 0}, - } - for _, tc := range testCases { - q := NewQuery("foo").FilterField(tc.f, tc.o, 42) - if ok := q.err == nil; ok != tc.wantOK { - t.Errorf("%q %q: ok=%t, want %t", tc.f, tc.o, ok, tc.wantOK) - continue - } - if !tc.wantOK { + successTestCases := append(filterTestCases, filterFieldTestCases...) + for _, tc := range successTestCases { + q := NewQuery("foo").FilterField(tc.fieldName, tc.operator, 42) + if q.err != nil { + t.Errorf("%q %q: error: %v", tc.fieldName, tc.operator, q.err) continue } if len(q.filter) != 1 { - t.Errorf("%q: len=%d, want %d", tc.f, len(q.filter), 1) + t.Errorf("%q: len=%d, want %d", tc.fieldName, len(q.filter), 1) continue } - got, want := q.filter[0], filter{tc.f, tc.wantOp, 42} + got, want := q.filter[0], filter{tc.fieldName, tc.wantOp, 42} if got != want { - t.Errorf("%q %q: got %v, want %v", tc.f, tc.o, got, want) + t.Errorf("%q %q: got %v, want %v", tc.fieldName, tc.operator, got, want) continue } } + for _, tc := range filterUnsupported { + q := NewQuery("foo").Filter(tc.filterStr, 42) + if q.err == nil { + t.Errorf("%q: should have thrown error", tc.filterStr) + } + } } func TestUnquote(t *testing.T) {