Skip to content

Commit

Permalink
feature(datastore): adds in, not-in, and != query operators (#6017)
Browse files Browse the repository at this point in the history 
* feat(datastore): adds in, not-in, and != query ops

* fix: tests

* fix: lint

* fix: adjusted string operations

* fix: linter

* fix: linter

* fix: per reviewer

* 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

* chore: more test cases

* tests: added integration tests (kinda)

Co-authored-by: Tyler Bui-Palsulich <26876514+tbpg@users.noreply.github.com>
  • Loading branch information
@tbpg
Eric Schmidt and tbpg committed Jun 16, 2022
1 parent e2d14a0 commit e926fb4
Show file tree
Hide file tree
Showing 3 changed files with 179 additions and 53 deletions.
20 changes: 20 additions & 0 deletions datastore/integration_test.go
View file
Expand Up @@ -483,6 +483,26 @@ func TestIntegration_Filters(t *testing.T) {
1,
4,
},
// TODO(#6184): Uncomment the following test cases once test DB is updated.
/*
{
"I!=191",
baseQuery.FilterField("I", "!=", 191),
8,
28,
},
{
"I in {2, 4}",
baseQuery.FilterField("I", "in", []interface{}{2, 4}),
2,
6,
},
{
"I not in {1, 3, 5, 7}",
baseQuery.FilterField("I", "not-in", []interface{}{1, 3, 5, 7}),
4,
12,
}*/
}, func() {
got := []*SQChild{}
want := []*SQChild{
Expand Down
43 changes: 38 additions & 5 deletions datastore/query.go
View file
Expand Up @@ -38,6 +38,9 @@ const (
equal
greaterEq
greaterThan
in
notIn
notEqual

keyFieldName = "__key__"
)
Expand All @@ -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.
Expand Down Expand Up @@ -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 ">=":
Expand All @@ -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
Expand Down
169 changes: 121 additions & 48 deletions datastore/query_test.go
View file
Expand Up @@ -391,57 +391,74 @@ 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},
type testFilterCase struct {
filterStr string
fieldName string
operator string
wantOp operator
wantFieldName string
}

var (
/*
// 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},
*/

// 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"},
{"x > y =", "x > y", "=", equal, "x > y"},
{"` x ` =", " x ", "=", equal, " x "},
{`" x " =`, " x ", "=", equal, " x "},
{`" \"x " =`, ` "x `, "=", equal, ` "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, ""},
{`" 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 {
Expand All @@ -454,6 +471,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) {
Expand Down

0 comments on commit e926fb4

Please sign in to comment.