From 7260f68bca6bff0a624801694e1b6b0053c0fbd2 Mon Sep 17 00:00:00 2001
From: Tom Clegg <tom@curii.com>
Date: Wed, 1 Sep 2021 14:22:59 -0400
Subject: [PATCH] 17995: Accept boolean expressions as strings without
 ["...",=,true].

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom@curii.com>
---
 doc/api/methods.html.textile.liquid  | 30 ++++++------
 lib/controller/router/router_test.go | 43 ++++++++++++++---
 sdk/go/arvados/resource_list.go      | 45 ++++++++++-------
 sdk/go/arvados/resource_list_test.go | 72 ++++++++++++----------------
 4 files changed, 110 insertions(+), 80 deletions(-)

diff --git a/doc/api/methods.html.textile.liquid b/doc/api/methods.html.textile.liquid
index 87ff621b8b..fd52917928 100644
--- a/doc/api/methods.html.textile.liquid
+++ b/doc/api/methods.html.textile.liquid
@@ -111,20 +111,6 @@ table(table table-bordered table-condensed).
 |@contains@|string, array of strings|Presence of one or more keys or array elements|@["storage_classes_desired", "contains", ["foo", "bar"]]@ (matches both @["foo", "bar"]@ and @["foo", "bar", "baz"]@)
 (note @[..., "contains", "foo"]@ is also accepted, and is equivalent to @[..., "contains", ["foo"]]@)|
 
-h4(#filterexpression). Filtering using boolean expressions
-
-In place of an attribute, the first element of the three-element filter array can be a boolean expression. The following restrictions apply:
-* The expression must contain exactly one operator.
-* The operator must be @=@, @<@, @<=@, @>@, or @>=@.
-* There must be exactly one pair of parentheses, surrounding the entire expression.
-* Each operand must be the name of a numeric attribute like @replication_desired@ (literal values like @3@ and non-numeric attributes like @uuid@ are not accepted).
-* The expression must not contain whitespace other than an ASCII space (newline and tab characters are not accepted).
-* The second and third elements of the filter array must be @"="@ and @true@ respectively.
-
-Examples:
-* @["(replication_desired < replication_confirmed)", "=", true]@
-* @["(replication_desired = replication_confirmed)", "=", true]@
-
 h4(#substringsearchfilter). Filtering using substring search
 
 Resources can also be filtered by searching for a substring in attributes of type @string@, @array of strings@, @text@, and @hash@, which are indexed in the database specifically for search. To use substring search, the filter must:
@@ -150,6 +136,22 @@ table(table table-bordered table-condensed).
 
 Note that exclusion filters @!=@ and @not in@ will return records for which the property is not defined at all.  To restrict filtering to records on which the subproperty is defined, combine with an @exists@ filter.
 
+h4(#filterexpression). Filtering using boolean expressions
+
+In addition to the three-element array form described above, a string containing a boolean expression is also accepted. The following restrictions apply:
+* The expression must contain exactly one operator.
+* The operator must be @=@, @<@, @<=@, @>@, or @>=@.
+* There must be exactly one pair of parentheses, surrounding the entire expression.
+* Each operand must be the name of a numeric attribute like @replication_desired@ (literal values like @3@ and non-numeric attributes like @uuid@ are not accepted).
+* The expression must not contain whitespace other than an ASCII space (newline and tab characters are not accepted).
+
+Examples:
+* @(replication_desired > replication_confirmed)@
+* @(replication_desired = replication_confirmed)@
+
+Both types of filter (boolean expressions and @[attribute, operator, operand]@ filters) can be combined in the same API call. Example:
+* @{"filters": ["(replication_desired > replication_confirmed)", ["replication_desired", "<", 2]]}@
+
 h4. Federated listing
 
 Federated listing forwards a request to multiple clusters and combines the results.  Currently only a very restricted form of the "list" method is supported.
diff --git a/lib/controller/router/router_test.go b/lib/controller/router/router_test.go
index 0330ec4252..639d2a28b4 100644
--- a/lib/controller/router/router_test.go
+++ b/lib/controller/router/router_test.go
@@ -47,6 +47,7 @@ func (s *RouterSuite) SetUpTest(c *check.C) {
 func (s *RouterSuite) TestOptions(c *check.C) {
 	token := arvadostest.ActiveToken
 	for _, trial := range []struct {
+		comment      string // unparsed -- only used to help match test failures to trials
 		method       string
 		path         string
 		header       http.Header
@@ -120,6 +121,32 @@ func (s *RouterSuite) TestOptions(c *check.C) {
 			shouldCall:  "CollectionList",
 			withOptions: arvados.ListOptions{Limit: 123, Offset: 456, IncludeTrash: true, IncludeOldVersions: true},
 		},
+		{
+			comment:     "form-encoded expression filter in query string",
+			method:      "GET",
+			path:        "/arvados/v1/collections?filters=[%22(foo<bar)%22]",
+			header:      http.Header{"Content-Type": {"application/x-www-form-urlencoded"}},
+			shouldCall:  "CollectionList",
+			withOptions: arvados.ListOptions{Limit: -1, Filters: []arvados.Filter{{"(foo<bar)", "=", true}}},
+		},
+		{
+			comment:     "form-encoded expression filter in POST body",
+			method:      "POST",
+			path:        "/arvados/v1/collections",
+			body:        "filters=[\"(foo<bar)\"]&_method=GET",
+			header:      http.Header{"Content-Type": {"application/x-www-form-urlencoded"}},
+			shouldCall:  "CollectionList",
+			withOptions: arvados.ListOptions{Limit: -1, Filters: []arvados.Filter{{"(foo<bar)", "=", true}}},
+		},
+		{
+			comment:     "json-encoded expression filter in POST body",
+			method:      "POST",
+			path:        "/arvados/v1/collections?_method=GET",
+			body:        `{"filters":["(foo<bar)",["bar","=","baz"]],"limit":2}`,
+			header:      http.Header{"Content-Type": {"application/json"}},
+			shouldCall:  "CollectionList",
+			withOptions: arvados.ListOptions{Limit: 2, Filters: []arvados.Filter{{"(foo<bar)", "=", true}, {"bar", "=", "baz"}}},
+		},
 		{
 			method:       "PATCH",
 			path:         "/arvados/v1/collections",
@@ -139,21 +166,23 @@ func (s *RouterSuite) TestOptions(c *check.C) {
 		// Reset calls captured in previous trial
 		s.stub = arvadostest.APIStub{}
 
-		c.Logf("trial: %#v", trial)
+		c.Logf("trial: %+v", trial)
+		comment := check.Commentf("trial comment: %s", trial.comment)
+
 		_, rr, _ := doRequest(c, s.rtr, token, trial.method, trial.path, trial.header, bytes.NewBufferString(trial.body))
 		if trial.shouldStatus == 0 {
-			c.Check(rr.Code, check.Equals, http.StatusOK)
+			c.Check(rr.Code, check.Equals, http.StatusOK, comment)
 		} else {
-			c.Check(rr.Code, check.Equals, trial.shouldStatus)
+			c.Check(rr.Code, check.Equals, trial.shouldStatus, comment)
 		}
 		calls := s.stub.Calls(nil)
 		if trial.shouldCall == "" {
-			c.Check(calls, check.HasLen, 0)
+			c.Check(calls, check.HasLen, 0, comment)
 		} else if len(calls) != 1 {
-			c.Check(calls, check.HasLen, 1)
+			c.Check(calls, check.HasLen, 1, comment)
 		} else {
-			c.Check(calls[0].Method, isMethodNamed, trial.shouldCall)
-			c.Check(calls[0].Options, check.DeepEquals, trial.withOptions)
+			c.Check(calls[0].Method, isMethodNamed, trial.shouldCall, comment)
+			c.Check(calls[0].Options, check.DeepEquals, trial.withOptions, comment)
 		}
 	}
 }
diff --git a/sdk/go/arvados/resource_list.go b/sdk/go/arvados/resource_list.go
index a5cc7d3b90..7f319b4121 100644
--- a/sdk/go/arvados/resource_list.go
+++ b/sdk/go/arvados/resource_list.go
@@ -37,28 +37,37 @@ func (f *Filter) MarshalJSON() ([]byte, error) {
 
 // UnmarshalJSON decodes a JSON array to a Filter.
 func (f *Filter) UnmarshalJSON(data []byte) error {
-	var elements []interface{}
-	err := json.Unmarshal(data, &elements)
+	var decoded interface{}
+	err := json.Unmarshal(data, &decoded)
 	if err != nil {
 		return err
 	}
-	if len(elements) != 3 {
-		return fmt.Errorf("invalid filter %q: must have 3 elements", data)
-	}
-	attr, ok := elements[0].(string)
-	if !ok {
-		return fmt.Errorf("invalid filter attr %q", elements[0])
-	}
-	op, ok := elements[1].(string)
-	if !ok {
-		return fmt.Errorf("invalid filter operator %q", elements[1])
-	}
-	operand := elements[2]
-	switch operand.(type) {
-	case string, float64, []interface{}, nil, bool:
+	switch decoded := decoded.(type) {
+	case string:
+		// Accept "(foo < bar)" as a more obvious way to spell
+		// ["(foo < bar)","=",true]
+		*f = Filter{decoded, "=", true}
+	case []interface{}:
+		if len(decoded) != 3 {
+			return fmt.Errorf("invalid filter %q: must have 3 decoded", data)
+		}
+		attr, ok := decoded[0].(string)
+		if !ok {
+			return fmt.Errorf("invalid filter attr %q", decoded[0])
+		}
+		op, ok := decoded[1].(string)
+		if !ok {
+			return fmt.Errorf("invalid filter operator %q", decoded[1])
+		}
+		operand := decoded[2]
+		switch operand.(type) {
+		case string, float64, []interface{}, nil, bool:
+		default:
+			return fmt.Errorf("invalid filter operand %q", decoded[2])
+		}
+		*f = Filter{attr, op, operand}
 	default:
-		return fmt.Errorf("invalid filter operand %q", elements[2])
+		return fmt.Errorf("invalid filter: json decoded as %T instead of array or string", decoded)
 	}
-	*f = Filter{attr, op, operand}
 	return nil
 }
diff --git a/sdk/go/arvados/resource_list_test.go b/sdk/go/arvados/resource_list_test.go
index b36e82c918..e4d89d62ad 100644
--- a/sdk/go/arvados/resource_list_test.go
+++ b/sdk/go/arvados/resource_list_test.go
@@ -5,69 +5,59 @@
 package arvados
 
 import (
-	"bytes"
 	"encoding/json"
-	"testing"
 	"time"
+
+	check "gopkg.in/check.v1"
 )
 
-func TestMarshalFiltersWithNanoseconds(t *testing.T) {
+var _ = check.Suite(&filterEncodingSuite{})
+
+type filterEncodingSuite struct{}
+
+func (s *filterEncodingSuite) TestMarshalNanoseconds(c *check.C) {
 	t0 := time.Now()
 	t0str := t0.Format(time.RFC3339Nano)
 	buf, err := json.Marshal([]Filter{
 		{Attr: "modified_at", Operator: "=", Operand: t0}})
-	if err != nil {
-		t.Fatal(err)
-	}
-	if expect := []byte(`[["modified_at","=","` + t0str + `"]]`); 0 != bytes.Compare(buf, expect) {
-		t.Errorf("Encoded as %q, expected %q", buf, expect)
-	}
+	c.Assert(err, check.IsNil)
+	c.Check(string(buf), check.Equals, `[["modified_at","=","`+t0str+`"]]`)
 }
 
-func TestMarshalFiltersWithNil(t *testing.T) {
+func (s *filterEncodingSuite) TestMarshalNil(c *check.C) {
 	buf, err := json.Marshal([]Filter{
 		{Attr: "modified_at", Operator: "=", Operand: nil}})
-	if err != nil {
-		t.Fatal(err)
-	}
-	if expect := []byte(`[["modified_at","=",null]]`); 0 != bytes.Compare(buf, expect) {
-		t.Errorf("Encoded as %q, expected %q", buf, expect)
-	}
+	c.Assert(err, check.IsNil)
+	c.Check(string(buf), check.Equals, `[["modified_at","=",null]]`)
 }
 
-func TestUnmarshalFiltersWithNil(t *testing.T) {
+func (s *filterEncodingSuite) TestUnmarshalNil(c *check.C) {
 	buf := []byte(`["modified_at","=",null]`)
-	f := &Filter{}
+	var f Filter
 	err := f.UnmarshalJSON(buf)
-	if err != nil {
-		t.Fatal(err)
-	}
-	expect := Filter{Attr: "modified_at", Operator: "=", Operand: nil}
-	if f.Attr != expect.Attr || f.Operator != expect.Operator || f.Operand != expect.Operand {
-		t.Errorf("Decoded as %q, expected %q", f, expect)
-	}
+	c.Assert(err, check.IsNil)
+	c.Check(f, check.DeepEquals, Filter{Attr: "modified_at", Operator: "=", Operand: nil})
 }
 
-func TestMarshalFiltersWithBoolean(t *testing.T) {
+func (s *filterEncodingSuite) TestMarshalBoolean(c *check.C) {
 	buf, err := json.Marshal([]Filter{
 		{Attr: "is_active", Operator: "=", Operand: true}})
-	if err != nil {
-		t.Fatal(err)
-	}
-	if expect := []byte(`[["is_active","=",true]]`); 0 != bytes.Compare(buf, expect) {
-		t.Errorf("Encoded as %q, expected %q", buf, expect)
-	}
+	c.Assert(err, check.IsNil)
+	c.Check(string(buf), check.Equals, `[["is_active","=",true]]`)
 }
 
-func TestUnmarshalFiltersWithBoolean(t *testing.T) {
+func (s *filterEncodingSuite) TestUnmarshalBoolean(c *check.C) {
 	buf := []byte(`["is_active","=",true]`)
-	f := &Filter{}
+	var f Filter
+	err := f.UnmarshalJSON(buf)
+	c.Assert(err, check.IsNil)
+	c.Check(f, check.DeepEquals, Filter{Attr: "is_active", Operator: "=", Operand: true})
+}
+
+func (s *filterEncodingSuite) TestUnmarshalBooleanExpression(c *check.C) {
+	buf := []byte(`"(foo < bar)"`)
+	var f Filter
 	err := f.UnmarshalJSON(buf)
-	if err != nil {
-		t.Fatal(err)
-	}
-	expect := Filter{Attr: "is_active", Operator: "=", Operand: true}
-	if f.Attr != expect.Attr || f.Operator != expect.Operator || f.Operand != expect.Operand {
-		t.Errorf("Decoded as %q, expected %q", f, expect)
-	}
+	c.Assert(err, check.IsNil)
+	c.Check(f, check.DeepEquals, Filter{Attr: "(foo < bar)", Operator: "=", Operand: true})
 }
-- 
2.30.2