Merge pull request #637 from nyaruka/preview_start_fixes

Preview start endpoint improvements

core/search/queries.go

@@ -1,15 +1,15 @@
 package search
 
 import (
-	"fmt"
-	"strings"
 	"time"
 
 	"github.com/nyaruka/gocommon/dates"
 	"github.com/nyaruka/gocommon/urns"
+	"github.com/nyaruka/goflow/contactql"
 	"github.com/nyaruka/goflow/envs"
 	"github.com/nyaruka/goflow/flows"
 	"github.com/nyaruka/mailroom/core/models"
+	"github.com/pkg/errors"
 )
 
 // Exclusions are preset exclusion conditions
@@ -21,56 +21,56 @@ type Exclusions struct {
 }
 
 // BuildStartQuery builds a start query for the given flow and start options
-func BuildStartQuery(env envs.Environment, flow *models.Flow, groups []*models.Group, contactUUIDs []flows.ContactUUID, urnz []urns.URN, userQuery string, excs Exclusions) string {
-	inclusions := make([]string, 0, 10)
+func BuildStartQuery(oa *models.OrgAssets, flow *models.Flow, groups []*models.Group, contactUUIDs []flows.ContactUUID, urnz []urns.URN, userQuery string, excs Exclusions) (string, error) {
+	var parsedQuery *contactql.ContactQuery
+	var err error
+
+	if userQuery != "" {
+		parsedQuery, err = contactql.ParseQuery(oa.Env(), userQuery, oa.SessionAssets())
+		if err != nil {
+			return "", errors.Wrap(err, "invalid user query")
+		}
+	}
+
+	return contactql.Stringify(buildStartQuery(oa.Env(), flow, groups, contactUUIDs, urnz, parsedQuery, excs)), nil
+}
+
+func buildStartQuery(env envs.Environment, flow *models.Flow, groups []*models.Group, contactUUIDs []flows.ContactUUID, urnz []urns.URN, userQuery *contactql.ContactQuery, excs Exclusions) contactql.QueryNode {
+	inclusions := make([]contactql.QueryNode, 0, 10)
 
 	for _, group := range groups {
-		inclusions = append(inclusions, fmt.Sprintf("group = \"%s\"", group.Name()))
+		inclusions = append(inclusions, contactql.NewCondition("group", contactql.PropertyTypeAttribute, contactql.OpEqual, group.Name()))
 	}
 	for _, contactUUID := range contactUUIDs {
-		inclusions = append(inclusions, fmt.Sprintf("uuid = \"%s\"", contactUUID))
+		inclusions = append(inclusions, contactql.NewCondition("uuid", contactql.PropertyTypeAttribute, contactql.OpEqual, string(contactUUID)))
 	}
 	for _, urn := range urnz {
 		scheme, path, _, _ := urn.ToParts()
-		inclusions = append(inclusions, fmt.Sprintf("%s = \"%s\"", scheme, path))
+		inclusions = append(inclusions, contactql.NewCondition(scheme, contactql.PropertyTypeScheme, contactql.OpEqual, path))
 	}
-	if userQuery != "" {
-		if len(inclusions) > 0 {
-			userQuery = fmt.Sprintf("(%s)", userQuery)
-		}
-		inclusions = append(inclusions, userQuery)
+	if userQuery != nil {
+		inclusions = append(inclusions, userQuery.Root())
 	}
 
-	exclusions := make([]string, 0, 10)
+	exclusions := make([]contactql.QueryNode, 0, 10)
 	if excs.NonActive {
-		exclusions = append(exclusions, "status = \"active\"")
+		exclusions = append(exclusions, contactql.NewCondition("status", contactql.PropertyTypeAttribute, contactql.OpEqual, "active"))
 	}
 	if excs.InAFlow {
-		exclusions = append(exclusions, "flow = \"\"")
+		exclusions = append(exclusions, contactql.NewCondition("flow", contactql.PropertyTypeAttribute, contactql.OpEqual, ""))
 	}
 	if excs.StartedPreviously {
-		exclusions = append(exclusions, fmt.Sprintf("history != \"%s\"", flow.Name()))
+		exclusions = append(exclusions, contactql.NewCondition("history", contactql.PropertyTypeAttribute, contactql.OpNotEqual, flow.Name()))
 	}
 	if excs.NotSeenSinceDays > 0 {
 		seenSince := dates.Now().Add(-time.Hour * time.Duration(24*excs.NotSeenSinceDays))
-		exclusions = append(exclusions, fmt.Sprintf("last_seen_on > %s", formatQueryDate(env, seenSince)))
+		exclusions = append(exclusions, contactql.NewCondition("last_seen_on", contactql.PropertyTypeAttribute, contactql.OpGreaterThan, formatQueryDate(env, seenSince)))
 	}
 
-	inclusionCmp := strings.Join(inclusions, " OR ")
-	exclusionCmp := strings.Join(exclusions, " AND ")
-
-	if len(inclusions) > 1 && len(exclusions) > 0 {
-		inclusionCmp = fmt.Sprintf("(%s)", inclusionCmp)
-	}
-
-	conditions := make([]string, 0, 2)
-	if inclusionCmp != "" {
-		conditions = append(conditions, inclusionCmp)
-	}
-	if exclusionCmp != "" {
-		conditions = append(conditions, exclusionCmp)
-	}
-	return strings.Join(conditions, " AND ")
+	return contactql.NewBoolCombination(contactql.BoolOperatorAnd,
+		contactql.NewBoolCombination(contactql.BoolOperatorOr, inclusions...),
+		contactql.NewBoolCombination(contactql.BoolOperatorAnd, exclusions...),
+	).Simplify()
 }
 
 // formats a date for use in a query

core/search/queries_test.go

@@ -35,13 +35,14 @@ func TestBuildStartQuery(t *testing.T) {
 		userQuery    string
 		exclusions   search.Exclusions
 		expected     string
+		err          string
 	}{
 		{
 			groups:       []*models.Group{doctors, testers},
 			contactUUIDs: []flows.ContactUUID{testdata.Cathy.UUID, testdata.George.UUID},
 			urns:         []urns.URN{"tel:+1234567890", "telegram:9876543210"},
 			exclusions:   search.Exclusions{},
-			expected:     `group = "Doctors" OR group = "Testers" OR uuid = "6393abc0-283d-4c9b-a1b3-641a035c34bf" OR uuid = "8d024bcd-f473-4719-a00a-bd0bb1190135" OR tel = "+1234567890" OR telegram = "9876543210"`,
+			expected:     `group = "Doctors" OR group = "Testers" OR uuid = "6393abc0-283d-4c9b-a1b3-641a035c34bf" OR uuid = "8d024bcd-f473-4719-a00a-bd0bb1190135" OR tel = "+1234567890" OR telegram = 9876543210`,
 		},
 		{
 			groups:       []*models.Group{doctors},
@@ -53,7 +54,7 @@ func TestBuildStartQuery(t *testing.T) {
 				StartedPreviously: true,
 				NotSeenSinceDays:  90,
 			},
-			expected: `(group = "Doctors" OR uuid = "6393abc0-283d-4c9b-a1b3-641a035c34bf" OR tel = "+1234567890") AND status = "active" AND flow = "" AND history != "Favorites" AND last_seen_on > 20-01-2022`,
+			expected: `(group = "Doctors" OR uuid = "6393abc0-283d-4c9b-a1b3-641a035c34bf" OR tel = "+1234567890") AND status = "active" AND flow = "" AND history != "Favorites" AND last_seen_on > "20-01-2022"`,
 		},
 		{
 			contactUUIDs: []flows.ContactUUID{testdata.Cathy.UUID},
@@ -63,24 +64,60 @@ func TestBuildStartQuery(t *testing.T) {
 			expected: `uuid = "6393abc0-283d-4c9b-a1b3-641a035c34bf" AND status = "active"`,
 		},
 		{
-			userQuery:  "gender = M",
+			userQuery:  `gender = "M"`,
 			exclusions: search.Exclusions{},
-			expected:   "gender = M",
+			expected:   `gender = "M"`,
 		},
 		{
-			userQuery: "gender = M",
+			userQuery: `gender = "M"`,
 			exclusions: search.Exclusions{
 				NonActive:         true,
 				InAFlow:           true,
 				StartedPreviously: true,
 				NotSeenSinceDays:  30,
 			},
-			expected: `gender = M AND status = "active" AND flow = "" AND history != "Favorites" AND last_seen_on > 21-03-2022`,
+			expected: `gender = "M" AND status = "active" AND flow = "" AND history != "Favorites" AND last_seen_on > "21-03-2022"`,
+		},
+		{
+			userQuery: `name ~ ben`,
+			exclusions: search.Exclusions{
+				NonActive:         false,
+				InAFlow:           false,
+				StartedPreviously: false,
+				NotSeenSinceDays:  30,
+			},
+			expected: `name ~ "ben" AND last_seen_on > "21-03-2022"`,
+		},
+		{
+			userQuery: `name ~ ben OR name ~ eric`,
+			exclusions: search.Exclusions{
+				NonActive:         false,
+				InAFlow:           false,
+				StartedPreviously: false,
+				NotSeenSinceDays:  30,
+			},
+			expected: `(name ~ "ben" OR name ~ "eric") AND last_seen_on > "21-03-2022"`,
+		},
+		{
+			userQuery:  `name ~`, // syntactically invalid user query
+			exclusions: search.Exclusions{},
+			err:        "invalid user query: mismatched input '<EOF>' expecting {TEXT, STRING}",
+		},
+		{
+			userQuery:  `goats > 14`, // no such field
+			exclusions: search.Exclusions{},
+			err:        "invalid user query: can't resolve 'goats' to attribute, scheme or field",
 		},
 	}
 
 	for _, tc := range tcs {
-		actual := search.BuildStartQuery(oa.Env(), flow, tc.groups, tc.contactUUIDs, tc.urns, tc.userQuery, tc.exclusions)
-		assert.Equal(t, tc.expected, actual)
+		actual, err := search.BuildStartQuery(oa, flow, tc.groups, tc.contactUUIDs, tc.urns, tc.userQuery, tc.exclusions)
+		if tc.err != "" {
+			assert.Equal(t, "", actual)
+			assert.EqualError(t, err, tc.err)
+		} else {
+			assert.Equal(t, tc.expected, actual)
+			assert.NoError(t, err)
+		}
 	}
 }

go.mod

@@ -16,7 +16,7 @@ require (
 	github.com/lib/pq v1.10.6
 	github.com/nyaruka/ezconf v0.2.1
 	github.com/nyaruka/gocommon v1.22.4
-	github.com/nyaruka/goflow v0.161.2
+	github.com/nyaruka/goflow v0.162.1
 	github.com/nyaruka/logrus_sentry v0.8.2-0.20190129182604-c2962b80ba7d
 	github.com/nyaruka/null v1.2.0
 	github.com/nyaruka/redisx v0.2.1

go.sum

@@ -220,8 +220,8 @@ github.com/nyaruka/ezconf v0.2.1 h1:TDXWoqjqYya1uhou1mAJZg7rgFYL98EB0Tb3+BWtUh0=
 github.com/nyaruka/ezconf v0.2.1/go.mod h1:ey182kYkw2MIi4XiWe1FR/mzI33WCmTWuceDYYxgnQw=
 github.com/nyaruka/gocommon v1.22.4 h1:NCAItnrQbXlipDeOszoYbjXEFa1J1M+alS8VSk/uero=
 github.com/nyaruka/gocommon v1.22.4/go.mod h1:g6/d9drZXDUrtRSPe2Kf8lTUS+baHt/0G0dwHq3qeIU=
-github.com/nyaruka/goflow v0.161.2 h1:56HkdueEqyxisxENHbPdux4QXtTw43MTcIL0ZNAhXtY=
-github.com/nyaruka/goflow v0.161.2/go.mod h1:XUPFWNEgimo+hsOOonH8bN6I8V5LE2cxSYgSC0mdxas=
+github.com/nyaruka/goflow v0.162.1 h1:MEUcRUYu8JLLdvVVpZvkmsPUSVS1+u2GYChM0tb2LhI=
+github.com/nyaruka/goflow v0.162.1/go.mod h1:XUPFWNEgimo+hsOOonH8bN6I8V5LE2cxSYgSC0mdxas=
 github.com/nyaruka/librato v1.0.0 h1:Vznj9WCeC1yZXbBYyYp40KnbmXLbEkjKmHesV/v2SR0=
 github.com/nyaruka/librato v1.0.0/go.mod h1:pkRNLFhFurOz0QqBz6/DuTFhHHxAubWxs4Jx+J7yUgg=
 github.com/nyaruka/logrus_sentry v0.8.2-0.20190129182604-c2962b80ba7d h1:hyp9u36KIwbTCo2JAJ+TuJcJBc+UZzEig7RI/S5Dvkc=

web/flow/start.go

@@ -96,7 +96,14 @@ func handlePreviewStart(ctx context.Context, rt *runtime.Runtime, r *http.Reques
 		}
 	}
 
-	query := search.BuildStartQuery(oa.Env(), flow, groups, request.Include.ContactUUIDs, request.Include.URNs, request.Include.Query, request.Exclude)
+	query, err := search.BuildStartQuery(oa, flow, groups, request.Include.ContactUUIDs, request.Include.URNs, request.Include.Query, request.Exclude)
+	if err != nil {
+		isQueryError, qerr := contactql.IsQueryError(err)
+		if isQueryError {
+			return qerr, http.StatusBadRequest, nil
+		}
+		return nil, http.StatusInternalServerError, err
+	}
 	if query == "" {
 		return &previewStartResponse{SampleIDs: []models.ContactID{}}, http.StatusOK, nil
 	}

web/flow/testdata/preview_start.json

@@ -234,5 +234,49 @@
                 "allow_as_group": false
             }
         }
+    },
+    {
+        "label": "invalid query inclusion (bad syntax)",
+        "method": "POST",
+        "path": "/mr/flow/preview_start",
+        "body": {
+            "org_id": 1,
+            "flow_id": 10001,
+            "include": {
+                "query": "gender ="
+            },
+            "exclude": {},
+            "sample_size": 3
+        },
+        "status": 400,
+        "response": {
+            "code": "unexpected_token",
+            "error": "mismatched input '<EOF>' expecting {TEXT, STRING}",
+            "extra": {
+                "token": "<EOF>"
+            }
+        }
+    },
+    {
+        "label": "invalid query inclusion (missing field)",
+        "method": "POST",
+        "path": "/mr/flow/preview_start",
+        "body": {
+            "org_id": 1,
+            "flow_id": 10001,
+            "include": {
+                "query": "goats > 10"
+            },
+            "exclude": {},
+            "sample_size": 3
+        },
+        "status": 400,
+        "response": {
+            "code": "unknown_property",
+            "error": "can't resolve 'goats' to attribute, scheme or field",
+            "extra": {
+                "property": "goats"
+            }
+        }
     }
 ]