diff --git a/datastore/query.go b/datastore/query.go index c2ed359eeaf..f2e6d9c2a6c 100644 --- a/datastore/query.go +++ b/datastore/query.go @@ -38,9 +38,6 @@ const ( equal greaterEq greaterThan - in - notIn - notEqual keyFieldName = "__key__" ) @@ -51,9 +48,6 @@ 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. @@ -177,46 +171,25 @@ 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 ">", "<", ">=", "<=", "=", and "!=". +// followed by an operator, one of ">", "<", ">=", "<=", or "=". // 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.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: fieldName, + FieldName: strings.TrimRight(filterStr, " ><=!"), Value: value, } - - switch o := strings.TrimSpace(operator); o { + switch op := strings.TrimSpace(filterStr[len(f.FieldName):]); op { case "<=": f.Op = lessEq case ">=": @@ -227,14 +200,8 @@ func (q *Query) FilterField(fieldName, operator string, value interface{}) *Quer 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", operator) + q.err = fmt.Errorf("datastore: invalid operator %q in filter %q", op, filterStr) return q } var err error diff --git a/datastore/query_test.go b/datastore/query_test.go index 72c0544eab9..c28723a212b 100644 --- a/datastore/query_test.go +++ b/datastore/query_test.go @@ -391,61 +391,57 @@ func TestQueriesAreImmutable(t *testing.T) { } } -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, ""}, - } -) - func TestFilterParser(t *testing.T) { - // Success cases - for _, tc := range filterTestCases { + 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}, + } + for _, tc := range testCases { q := NewQuery("foo").Filter(tc.filterStr, 42) - if q.err != nil { - t.Errorf("%q: error=%v", tc.filterStr, q.err) + if ok := q.err == nil; ok != tc.wantOK { + t.Errorf("%q: ok=%t, want %t", tc.filterStr, ok, tc.wantOK) + continue + } + if !tc.wantOK { continue } if len(q.filter) != 1 { @@ -458,62 +454,6 @@ 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) {