From db9991f8dd9c7034766c05c2773bd4dc1f1a80d0 Mon Sep 17 00:00:00 2001 From: Eric Schmidt Date: Fri, 6 May 2022 09:23:38 -0700 Subject: [PATCH] feat(datastore): adds in, not-in, and != query operations (#5975) * feat(datastore): adds in, not-in, and != query ops * Update datastore/query_test.go Co-authored-by: Tyler Bui-Palsulich <26876514+tbpg@users.noreply.github.com> * Update datastore/query.go Co-authored-by: Tyler Bui-Palsulich <26876514+tbpg@users.noreply.github.com> * fix: per reviewer Co-authored-by: Tyler Bui-Palsulich <26876514+tbpg@users.noreply.github.com> --- datastore/query.go | 43 +++++++++-- datastore/query_test.go | 158 +++++++++++++++++++++++++++------------- 2 files changed, 147 insertions(+), 54 deletions(-) diff --git a/datastore/query.go b/datastore/query.go index f2e6d9c2a6c..c2ed359eeaf 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. @@ -171,25 +177,46 @@ func (q *Query) Transaction(t *Transaction) *Query { } // Filter returns a derivative query with a field-based filter. +// +// 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, -// followed by an operator, one of ">", "<", ">=", "<=", or "=". +// 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 { - q = q.clone() + // TODO( #5977 ): Add better string parsing (or something) filterStr = 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.FilterField(f, op, value) +} + +// 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. +// 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) FilterField(fieldName, operator string, value interface{}) *Query { + q = q.clone() + f := filter{ - FieldName: strings.TrimRight(filterStr, " ><=!"), + FieldName: fieldName, Value: value, } - switch op := strings.TrimSpace(filterStr[len(f.FieldName):]); op { + + switch o := strings.TrimSpace(operator); o { case "<=": f.Op = lessEq case ">=": @@ -200,8 +227,14 @@ 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) + 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 c28723a212b..72c0544eab9 100644 --- a/datastore/query_test.go +++ b/datastore/query_test.go @@ -391,57 +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}, - // Valid but (currently) unsupported ops. - {"x!=", false, "", 0}, - {"x !=", false, "", 0}, - {" x != ", false, "", 0}, - {"x IN", false, "", 0}, - {"x in", false, "", 0}, - // 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}, - {"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"}, } - for _, tc := range testCases { + // 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, ""}, + } +) + +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 { @@ -454,6 +458,62 @@ 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) { + 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.fieldName, len(q.filter), 1) + continue + } + got, want := q.filter[0], filter{tc.fieldName, tc.wantOp, 42} + if 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) { + 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) {