Repository: googleforgames/open-match
Pull request #1278 (2020-11-03 16:36:39)

Add ability to filter backfills
<!--  Thanks for sending a pull request!  Here are some tips for you:
If this is your first time, please read our contributor guidelines: https://github.com/googleforgames/open-match/blob/master/CONTRIBUTING.md and developer guide https://github.com/googleforgames/open-match/blob/master/docs/development.md
-->

**What this PR does / Why we need it**:
This PR has changes that allow to filter tickets and backfills using search criteria





Patch:
diff --git a/internal/filter/filter.go b/internal/filter/filter.go
index 8bbd8f58..fb8d587a 100644
--- a/internal/filter/filter.go
+++ b/internal/filter/filter.go
@@ -21,6 +21,7 @@ import (
 	"time"
 
 	"github.com/golang/protobuf/ptypes"
+	"github.com/golang/protobuf/ptypes/timestamp"
 	"github.com/sirupsen/logrus"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
@@ -72,16 +73,23 @@ func NewPoolFilter(pool *pb.Pool) (*PoolFilter, error) {
 	}, nil
 }
 
+type filteredEntity interface {
+	GetId() string
+	GetSearchFields() *pb.SearchFields
+	GetCreateTime() *timestamp.Timestamp
+}
+
 // In returns true if the Ticket meets all the criteria for this PoolFilter.
-func (pf *PoolFilter) In(ticket *pb.Ticket) bool {
-	s := ticket.GetSearchFields()
+func (pf *PoolFilter) In(entity filteredEntity) bool {
+	s := entity.GetSearchFields()
+
 	if s == nil {
 		s = emptySearchFields
 	}
 
 	if !pf.CreatedAfter.IsZero() || !pf.CreatedBefore.IsZero() {
 		// CreateTime is only populated by Open Match and hence expected to be valid.
-		if ct, err := ptypes.Timestamp(ticket.CreateTime); err == nil {
+		if ct, err := ptypes.Timestamp(entity.GetCreateTime()); err == nil {
 			if !pf.CreatedAfter.IsZero() {
 				if !ct.After(pf.CreatedAfter) {
 					return false
@@ -96,7 +104,7 @@ func (pf *PoolFilter) In(ticket *pb.Ticket) bool {
 		} else {
 			logger.WithFields(logrus.Fields{
 				"error": err.Error(),
-				"id":    ticket.GetId(),
+				"id":    entity.GetId(),
 			}).Error("failed to get time from Timestamp proto")
 		}
 	}


Test patch:
diff --git a/internal/filter/filter_test.go b/internal/filter/filter_test.go
index 87730365..417fddb1 100644
--- a/internal/filter/filter_test.go
+++ b/internal/filter/filter_test.go
@@ -27,33 +27,53 @@ import (
 )
 
 func TestMeetsCriteria(t *testing.T) {
+	testInclusion := func(t *testing.T, pool *pb.Pool, entity filteredEntity) {
+		pf, err := NewPoolFilter(pool)
+
+		require.NoError(t, err)
+		require.NotNil(t, pf)
+
+		if !pf.In(entity) {
+			t.Error("entity should be included in the pool")
+		}
+	}
+
 	for _, tc := range testcases.IncludedTestCases() {
 		tc := tc
 		t.Run(tc.Name, func(t *testing.T) {
-			pf, err := NewPoolFilter(tc.Pool)
+			testInclusion(t, tc.Pool, &pb.Ticket{
+				SearchFields: tc.SearchFields,
+				CreateTime:   ptypes.TimestampNow(),
+			})
+			testInclusion(t, tc.Pool, &pb.Backfill{
+				SearchFields: tc.SearchFields,
+				CreateTime:   ptypes.TimestampNow(),
+			})
+		})
+	}
 
-			require.NoError(t, err)
-			require.NotNil(t, pf)
+	testExclusion := func(t *testing.T, pool *pb.Pool, entity filteredEntity) {
+		pf, err := NewPoolFilter(pool)
 
-			tc.Ticket.CreateTime = ptypes.TimestampNow()
-			if !pf.In(tc.Ticket) {
-				t.Error("ticket should be included in the pool")
-			}
-		})
+		require.NoError(t, err)
+		require.NotNil(t, pf)
+
+		if pf.In(entity) {
+			t.Error("ticket should be excluded from the pool")
+		}
 	}
 
 	for _, tc := range testcases.ExcludedTestCases() {
 		tc := tc
 		t.Run(tc.Name, func(t *testing.T) {
-			pf, err := NewPoolFilter(tc.Pool)
-
-			require.NoError(t, err)
-			require.NotNil(t, pf)
-
-			tc.Ticket.CreateTime = ptypes.TimestampNow()
-			if pf.In(tc.Ticket) {
-				t.Error("ticket should be excluded from the pool")
-			}
+			testExclusion(t, tc.Pool, &pb.Ticket{
+				SearchFields: tc.SearchFields,
+				CreateTime:   ptypes.TimestampNow(),
+			})
+			testExclusion(t, tc.Pool, &pb.Backfill{
+				SearchFields: tc.SearchFields,
+				CreateTime:   ptypes.TimestampNow(),
+			})
 		})
 	}
 }
diff --git a/internal/filter/testcases/testcases.go b/internal/filter/testcases/testcases.go
index 1c0f6e9b..99121ba8 100644
--- a/internal/filter/testcases/testcases.go
+++ b/internal/filter/testcases/testcases.go
@@ -27,9 +27,9 @@ import (
 
 // TestCase defines a single filtering test case to run.
 type TestCase struct {
-	Name   string
-	Ticket *pb.Ticket
-	Pool   *pb.Pool
+	Name         string
+	SearchFields *pb.SearchFields
+	Pool         *pb.Pool
 }
 
 // IncludedTestCases returns a list of test cases where using the given filter,
@@ -39,7 +39,7 @@ func IncludedTestCases() []TestCase {
 	return []TestCase{
 		{
 			"no filters or fields",
-			&pb.Ticket{},
+			nil,
 			&pb.Pool{},
 		},
 
@@ -68,11 +68,9 @@ func IncludedTestCases() []TestCase {
 
 		{
 			"String equals simple positive",
-			&pb.Ticket{
-				SearchFields: &pb.SearchFields{
-					StringArgs: map[string]string{
-						"field": "value",
-					},
+			&pb.SearchFields{
+				StringArgs: map[string]string{
+					"field": "value",
 				},
 			},
 			&pb.Pool{
@@ -87,11 +85,9 @@ func IncludedTestCases() []TestCase {
 
 		{
 			"TagPresent simple positive",
-			&pb.Ticket{
-				SearchFields: &pb.SearchFields{
-					Tags: []string{
-						"mytag",
-					},
+			&pb.SearchFields{
+				Tags: []string{
+					"mytag",
 				},
 			},
 			&pb.Pool{
@@ -105,11 +101,9 @@ func IncludedTestCases() []TestCase {
 
 		{
 			"TagPresent multiple all present",
-			&pb.Ticket{
-				SearchFields: &pb.SearchFields{
-					Tags: []string{
-						"A", "B", "C",
-					},
+			&pb.SearchFields{
+				Tags: []string{
+					"A", "B", "C",
 				},
 			},
 			&pb.Pool{
@@ -131,21 +125,21 @@ func IncludedTestCases() []TestCase {
 
 		{
 			"CreatedBefore simple positive",
-			&pb.Ticket{},
+			nil,
 			&pb.Pool{
 				CreatedBefore: timestamp(now.Add(time.Hour * 1)),
 			},
 		},
 		{
 			"CreatedAfter simple positive",
-			&pb.Ticket{},
+			nil,
 			&pb.Pool{
 				CreatedAfter: timestamp(now.Add(time.Hour * -1)),
 			},
 		},
 		{
 			"Between CreatedBefore and CreatedAfter positive",
-			&pb.Ticket{},
+			nil,
 			&pb.Pool{
 				CreatedBefore: timestamp(now.Add(time.Hour * 1)),
 				CreatedAfter:  timestamp(now.Add(time.Hour * -1)),
@@ -153,7 +147,7 @@ func IncludedTestCases() []TestCase {
 		},
 		{
 			"No time search criteria positive",
-			&pb.Ticket{},
+			nil,
 			&pb.Pool{},
 		},
 	}
@@ -166,7 +160,7 @@ func ExcludedTestCases() []TestCase {
 	return []TestCase{
 		{
 			"DoubleRange no SearchFields",
-			&pb.Ticket{},
+			nil,
 			&pb.Pool{
 				DoubleRangeFilters: []*pb.DoubleRangeFilter{
 					{
@@ -179,7 +173,7 @@ func ExcludedTestCases() []TestCase {
 		},
 		{
 			"StringEquals no SearchFields",
-			&pb.Ticket{},
+			nil,
 			&pb.Pool{
 				StringEqualsFilters: []*pb.StringEqualsFilter{
 					{
@@ -191,7 +185,7 @@ func ExcludedTestCases() []TestCase {
 		},
 		{
 			"TagPresent no SearchFields",
-			&pb.Ticket{},
+			nil,
 			&pb.Pool{
 				TagPresentFilters: []*pb.TagPresentFilter{
 					{
@@ -202,11 +196,9 @@ func ExcludedTestCases() []TestCase {
 		},
 		{
 			"double range missing field",
-			&pb.Ticket{
-				SearchFields: &pb.SearchFields{
-					DoubleArgs: map[string]float64{
-						"otherfield": 0,
-					},
+			&pb.SearchFields{
+				DoubleArgs: map[string]float64{
+					"otherfield": 0,
 				},
 			},
 			&pb.Pool{
@@ -277,11 +269,9 @@ func ExcludedTestCases() []TestCase {
 
 		{
 			"String equals simple negative", // and case sensitivity
-			&pb.Ticket{
-				SearchFields: &pb.SearchFields{
-					StringArgs: map[string]string{
-						"field": "value",
-					},
+			&pb.SearchFields{
+				StringArgs: map[string]string{
+					"field": "value",
 				},
 			},
 			&pb.Pool{
@@ -296,11 +286,9 @@ func ExcludedTestCases() []TestCase {
 
 		{
 			"String equals missing field",
-			&pb.Ticket{
-				SearchFields: &pb.SearchFields{
-					StringArgs: map[string]string{
-						"otherfield": "othervalue",
-					},
+			&pb.SearchFields{
+				StringArgs: map[string]string{
+					"otherfield": "othervalue",
 				},
 			},
 			&pb.Pool{
@@ -315,11 +303,9 @@ func ExcludedTestCases() []TestCase {
 
 		{
 			"TagPresent simple negative", // and case sensitivity
-			&pb.Ticket{
-				SearchFields: &pb.SearchFields{
-					Tags: []string{
-						"MYTAG",
-					},
+			&pb.SearchFields{
+				Tags: []string{
+					"MYTAG",
 				},
 			},
 			&pb.Pool{
@@ -333,11 +319,9 @@ func ExcludedTestCases() []TestCase {
 
 		{
 			"TagPresent multiple with one missing",
-			&pb.Ticket{
-				SearchFields: &pb.SearchFields{
-					Tags: []string{
-						"A", "B", "C",
-					},
+			&pb.SearchFields{
+				Tags: []string{
+					"A", "B", "C",
 				},
 			},
 			&pb.Pool{
@@ -357,21 +341,21 @@ func ExcludedTestCases() []TestCase {
 
 		{
 			"CreatedBefore simple negative",
-			&pb.Ticket{},
+			nil,
 			&pb.Pool{
 				CreatedBefore: timestamp(now.Add(time.Hour * -1)),
 			},
 		},
 		{
 			"CreatedAfter simple negative",
-			&pb.Ticket{},
+			nil,
 			&pb.Pool{
 				CreatedAfter: timestamp(now.Add(time.Hour * 1)),
 			},
 		},
 		{
 			"Created before time range negative",
-			&pb.Ticket{},
+			nil,
 			&pb.Pool{
 				CreatedBefore: timestamp(now.Add(time.Hour * 2)),
 				CreatedAfter:  timestamp(now.Add(time.Hour * 1)),
@@ -379,7 +363,7 @@ func ExcludedTestCases() []TestCase {
 		},
 		{
 			"Created after time range negative",
-			&pb.Ticket{},
+			nil,
 			&pb.Pool{
 				CreatedBefore: timestamp(now.Add(time.Hour * -1)),
 				CreatedAfter:  timestamp(now.Add(time.Hour * -2)),
@@ -395,11 +379,9 @@ func ExcludedTestCases() []TestCase {
 func simpleDoubleRange(name string, value, min, max float64, exclude pb.DoubleRangeFilter_Exclude) TestCase {
 	return TestCase{
 		"double range " + name,
-		&pb.Ticket{
-			SearchFields: &pb.SearchFields{
-				DoubleArgs: map[string]float64{
-					"field": value,
-				},
+		&pb.SearchFields{
+			DoubleArgs: map[string]float64{
+				"field": value,
 			},
 		},
 		&pb.Pool{
@@ -433,16 +415,14 @@ func multipleFilters(doubleRange, stringEquals, tagPresent bool) TestCase {
 
 	return TestCase{
 		fmt.Sprintf("multiplefilters: %v, %v, %v", doubleRange, stringEquals, tagPresent),
-		&pb.Ticket{
-			SearchFields: &pb.SearchFields{
-				DoubleArgs: map[string]float64{
-					"a": a,
-				},
-				StringArgs: map[string]string{
-					"b": b,
-				},
-				Tags: []string{c},
+		&pb.SearchFields{
+			DoubleArgs: map[string]float64{
+				"a": a,
+			},
+			StringArgs: map[string]string{
+				"b": b,
 			},
+			Tags: []string{c},
 		},
 		&pb.Pool{
 			DoubleRangeFilters: []*pb.DoubleRangeFilter{
diff --git a/internal/testing/e2e/query_tickets_test.go b/internal/testing/e2e/query_tickets_test.go
index 22cb3013..f5e09624 100644
--- a/internal/testing/e2e/query_tickets_test.go
+++ b/internal/testing/e2e/query_tickets_test.go
@@ -153,7 +153,11 @@ func returnedByQuery(t *testing.T, tc testcases.TestCase) (found bool) {
 	om := newOM(t)
 
 	{
-		resp, err := om.Frontend().CreateTicket(context.Background(), &pb.CreateTicketRequest{Ticket: tc.Ticket})
+		ticket := pb.Ticket{
+			SearchFields: tc.SearchFields,
+		}
+		resp, err := om.Frontend().CreateTicket(context.Background(), &pb.CreateTicketRequest{Ticket: &ticket})
+
 		require.NotNil(t, resp)
 		require.Nil(t, err)
 	}
@@ -183,7 +187,11 @@ func returnedByQueryID(t *testing.T, tc testcases.TestCase) (found bool) {
 	om := newOM(t)
 
 	{
-		resp, err := om.Frontend().CreateTicket(context.Background(), &pb.CreateTicketRequest{Ticket: tc.Ticket})
+		ticket := pb.Ticket{
+			SearchFields: tc.SearchFields,
+		}
+		resp, err := om.Frontend().CreateTicket(context.Background(), &pb.CreateTicketRequest{Ticket: &ticket})
+
 		require.NotNil(t, resp)
 		require.Nil(t, err)
 	}