From d1daaf0686810ddd325c284a1c8e20aba38767b3 Mon Sep 17 00:00:00 2001 From: James Scott Date: Mon, 5 Feb 2024 19:29:58 +0000 Subject: [PATCH] Use FilterEntity instead of Filter The datastore library has marked Filter as deprecated. By moving to FilterEntity, we can use more advanced queries as well. This commit introduces a new EntityFilter type. That type just wraps datastore.EntityFilter because there are no exposed methods on datastore.EntityFilter for us to manually declare. In order to create an EntityFilter, another type called FilterBuilder is created which will allow consumers to make a particular Filter that reteurns an EntityFilter. More about the deprecation here: https://pkg.go.dev/cloud.google.com/go/datastore#Query.Filter --- api/checks/api.go | 3 ++- api/checks/suites.go | 14 ++++++++------ api/pending_test_runs.go | 9 +++++---- api/screenshot/model.go | 2 +- api/test_history.go | 3 ++- api/versions.go | 8 +++++--- shared/datastore.go | 14 +++++++++++++- shared/datastore_cloud.go | 21 +++++++++++++++++++++ shared/test_run_query.go | 35 +++++++++++++++++++++-------------- 9 files changed, 78 insertions(+), 31 deletions(-) diff --git a/api/checks/api.go b/api/checks/api.go index e4ecf545dd2..f2f0208caba 100644 --- a/api/checks/api.go +++ b/api/checks/api.go @@ -76,7 +76,8 @@ func (s checksAPIImpl) ScheduleResultsProcessing(sha string, product shared.Prod func (s checksAPIImpl) GetSuitesForSHA(sha string) ([]shared.CheckSuite, error) { var suites []shared.CheckSuite store := shared.NewAppEngineDatastore(s.Context(), false) - _, err := store.GetAll(store.NewQuery("CheckSuite").Filter("SHA =", sha), &suites) + q := store.NewQuery("CheckSuite") + _, err := store.GetAll(q.FilterEntity(q.FilterBuilder().PropertyFilter("SHA", "=", sha)), &suites) return suites, err } diff --git a/api/checks/suites.go b/api/checks/suites.go index c28243be763..f1a93a91af7 100644 --- a/api/checks/suites.go +++ b/api/checks/suites.go @@ -20,12 +20,14 @@ func getOrCreateCheckSuite( prNumbers ...int, ) (*shared.CheckSuite, error) { ds := shared.NewAppEngineDatastore(ctx, false) - query := ds.NewQuery("CheckSuite"). - Filter("SHA =", sha). - Filter("AppID =", appID). - Filter("InstallationID =", installationID). - Filter("Owner =", owner). - Filter("Repo =", repo). + query := ds.NewQuery("CheckSuite") + filterBuilder := query.FilterBuilder() + query = query. + FilterEntity(filterBuilder.PropertyFilter("SHA", "=", sha)). + FilterEntity(filterBuilder.PropertyFilter("AppID", "=", appID)). + FilterEntity(filterBuilder.PropertyFilter("InstallationID", "=", installationID)). + FilterEntity(filterBuilder.PropertyFilter("Owner", "=", owner)). + FilterEntity(filterBuilder.PropertyFilter("Repo", "=", repo)). KeysOnly() var suite shared.CheckSuite if keys, err := ds.GetAll(query, nil); err != nil { diff --git a/api/pending_test_runs.go b/api/pending_test_runs.go index 811a50fe530..85370192c58 100644 --- a/api/pending_test_runs.go +++ b/api/pending_test_runs.go @@ -23,15 +23,16 @@ func apiPendingTestRunsHandler(w http.ResponseWriter, r *http.Request) { filter := strings.ToLower(mux.Vars(r)["filter"]) q := store.NewQuery("PendingTestRun") + filterBuilder := q.FilterBuilder() switch filter { case "pending": - q = q.Order("-Stage").Filter("Stage < ", int(shared.StageValid)) + q = q.Order("-Stage").FilterEntity(filterBuilder.PropertyFilter("Stage", "<", int(shared.StageValid))) case "invalid": - q = q.Filter("Stage = ", int(shared.StageInvalid)) + q = q.FilterEntity(filterBuilder.PropertyFilter("Stage", "=", int(shared.StageInvalid))) case "empty": - q = q.Filter("Stage = ", int(shared.StageEmpty)) + q = q.FilterEntity(filterBuilder.PropertyFilter("Stage", "=", int(shared.StageEmpty))) case "duplicate": - q = q.Filter("Stage = ", int(shared.StageDuplicate)) + q = q.FilterEntity(filterBuilder.PropertyFilter("Stage", "= ", int(shared.StageDuplicate))) case "": // No-op default: diff --git a/api/screenshot/model.go b/api/screenshot/model.go index 49b9af9a987..5383282fdf9 100644 --- a/api/screenshot/model.go +++ b/api/screenshot/model.go @@ -140,7 +140,7 @@ func RecentScreenshotHashes( for all.Cardinality() < totalLimit { query := ds.NewQuery("Screenshot") for _, l := range labels { - query = query.Filter("Labels =", l) + query = query.FilterEntity(query.FilterBuilder().PropertyFilter("Labels", "=", l)) } query = query.Order("-LastUsed").Limit(totalLimit) diff --git a/api/test_history.go b/api/test_history.go index 2cdc15b2c9e..5956536942f 100644 --- a/api/test_history.go +++ b/api/test_history.go @@ -54,7 +54,8 @@ func testHistoryHandler(w http.ResponseWriter, r *http.Request) { } store := shared.NewAppEngineDatastore(ctx, false) - q := store.NewQuery("TestHistoryEntry").Filter("TestName =", reqBody.TestName) + q := store.NewQuery("TestHistoryEntry") + q = q.FilterEntity(q.FilterBuilder().PropertyFilter("TestName", "=", reqBody.TestName)) var runs []shared.TestHistoryEntry _, err = store.GetAll(q, &runs) diff --git a/api/versions.go b/api/versions.go index 5d638de5018..0493d0ab1c6 100644 --- a/api/versions.go +++ b/api/versions.go @@ -49,10 +49,12 @@ func (h VersionsHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { ctx := h.ctx store := shared.NewAppEngineDatastore(ctx, false) - query := store.NewQuery("TestRun").Filter("BrowserName =", product.BrowserName) + query := store.NewQuery("TestRun") + filterBuilder := query.FilterBuilder() + query = query.FilterEntity(filterBuilder.PropertyFilter("BrowserName", "=", product.BrowserName)) if product.Labels != nil { for label := range product.Labels.Iter() { - query = query.Filter("Labels =", label) + query = query.FilterEntity(filterBuilder.PropertyFilter("Labels", "=", label)) } } distinctQuery := query.Project("BrowserVersion").Distinct() @@ -61,7 +63,7 @@ func (h VersionsHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { queries = []shared.Query{distinctQuery} } else { queries = []shared.Query{ - query.Filter("BrowserVersion =", product.BrowserVersion).Limit(1), + query.FilterEntity(filterBuilder.PropertyFilter("BrowserVersion", "=", product.BrowserVersion)).Limit(1), shared.VersionPrefix(distinctQuery, "BrowserVersion", product.BrowserVersion, false /*desc*/), } } diff --git a/shared/datastore.go b/shared/datastore.go index 791b7c20c76..ed2cc4666d0 100644 --- a/shared/datastore.go +++ b/shared/datastore.go @@ -36,7 +36,7 @@ type Iterator interface { // Query abstracts a datastore.Query type Query interface { - Filter(filterStr string, value interface{}) Query + FilterEntity(EntityFilter) Query Project(fields ...string) Query Limit(limit int) Query Offset(offset int) Query @@ -44,6 +44,18 @@ type Query interface { KeysOnly() Query Distinct() Query Run(Datastore) Iterator + + FilterBuilder() FilterBuilder +} + +// PropertyFilter is a particular filter that filters based on property value. +type PropertyFilter interface { + EntityFilter +} + +// FilterBuilder contains the logic to create different types of filters +type FilterBuilder interface { + PropertyFilter(FieldName string, Operator string, Value interface{}) EntityFilter } // Datastore abstracts a datastore, hiding the distinctions between cloud and diff --git a/shared/datastore_cloud.go b/shared/datastore_cloud.go index 40980916d4d..6973a7ee364 100644 --- a/shared/datastore_cloud.go +++ b/shared/datastore_cloud.go @@ -197,6 +197,14 @@ type cloudQuery struct { query *datastore.Query } +func (q cloudQuery) FilterBuilder() FilterBuilder { + return cloudFilterBuilder{} +} + +func (q cloudQuery) FilterEntity(entityFilter EntityFilter) Query { + return cloudQuery{q.query.FilterEntity(entityFilter)} +} + func (q cloudQuery) Filter(filterStr string, value interface{}) Query { return cloudQuery{q.query.Filter(filterStr, value)} } @@ -240,3 +248,16 @@ func (i cloudIterator) Next(dst interface{}) (Key, error) { key, err := i.iter.Next(dst) return cloudKey{key}, err } + +// EntityFilter wraps datastore.EntityFilter. +// datastore.EntityFilter does not expose any methods. But using this type +// allows us to be strict on the filters returned by the FilterBuilder. +type EntityFilter interface { + datastore.EntityFilter +} + +type cloudFilterBuilder struct{} + +func (b cloudFilterBuilder) PropertyFilter(FieldName string, Operator string, Value interface{}) EntityFilter { + return datastore.PropertyFilter{FieldName: FieldName, Operator: Operator, Value: Value} +} diff --git a/shared/test_run_query.go b/shared/test_run_query.go index 45584007eaa..48e1ac69c2a 100644 --- a/shared/test_run_query.go +++ b/shared/test_run_query.go @@ -119,13 +119,14 @@ func (t testRunQueryImpl) LoadTestRunKeys( log := GetLogger(t.store.Context()) result = make(KeysByProduct, len(products)) baseQuery := t.store.NewQuery("TestRun") + filterBuilder := baseQuery.FilterBuilder() if offset != nil { baseQuery = baseQuery.Offset(*offset) } if labels != nil { labels.Remove("") // Ensure the empty string isn't present. for i := range labels.Iter() { - baseQuery = baseQuery.Filter("Labels =", i.(string)) + baseQuery = baseQuery.FilterEntity(filterBuilder.PropertyFilter("Labels", "=", i.(string))) } } var globalIDFilter mapset.Set @@ -143,10 +144,10 @@ func (t testRunQueryImpl) LoadTestRunKeys( for i, product := range products { var productIDFilter = merge(globalIDFilter, nil) - query := baseQuery.Filter("BrowserName =", product.BrowserName) + query := baseQuery.FilterEntity(filterBuilder.PropertyFilter("BrowserName", "=", product.BrowserName)) if product.Labels != nil { for i := range product.Labels.Iter() { - query = query.Filter("Labels =", i.(string)) + query = query.FilterEntity(filterBuilder.PropertyFilter("Labels", "=", i.(string))) } } if !IsLatest(product.Revision) { @@ -180,10 +181,10 @@ func (t testRunQueryImpl) LoadTestRunKeys( // TODO(lukebjerring): Indexes + filtering for OS + version. query = query.Order("-TimeStart") if from != nil { - query = query.Filter("TimeStart >=", *from) + query = query.FilterEntity(filterBuilder.PropertyFilter("TimeStart", ">=", *from)) } if to != nil { - query = query.Filter("TimeStart <", *to) + query = query.FilterEntity(filterBuilder.PropertyFilter("TimeStart", "<", *to)) } max := MaxCountMaxValue if limit != nil && *limit < MaxCountMaxValue { @@ -267,17 +268,18 @@ func (t testRunQueryImpl) GetAlignedRunSHAs( query := t.store. NewQuery("TestRun"). Order("-TimeStart") + filterBuilder := query.FilterBuilder() if labels != nil { for i := range labels.Iter() { - query = query.Filter("Labels =", i.(string)) + query = query.FilterEntity(filterBuilder.PropertyFilter("Labels", "=", i.(string))) } } if from != nil { - query = query.Filter("TimeStart >=", *from) + query = query.FilterEntity(filterBuilder.PropertyFilter("TimeStart", ">=", *from)) } if to != nil { - query = query.Filter("TimeStart <", *to) + query = query.FilterEntity(filterBuilder.PropertyFilter("TimeStart", "<", *to)) } productsBySHA := make(map[string]mapset.Set) @@ -356,16 +358,17 @@ func contains(s []string, x string) bool { func loadIDsForRevision(store Datastore, query Query, sha string) (result mapset.Set, err error) { log := GetLogger(store.Context()) var revQuery Query + filterBuilder := query.FilterBuilder() if len(sha) < 40 { log.Debugf("Finding revisions %s <= SHA < %s", sha, sha+"g") revQuery = query. Order("FullRevisionHash"). Limit(MaxCountMaxValue). - Filter("FullRevisionHash >=", sha). - Filter("FullRevisionHash <", sha+"g") // g > f + FilterEntity(filterBuilder.PropertyFilter("FullRevisionHash", ">=", sha)). + FilterEntity(filterBuilder.PropertyFilter("FullRevisionHash", "<", sha+"g")) // g > f } else { log.Debugf("Finding exact revision %s", sha) - revQuery = query.Filter("FullRevisionHash =", sha[:40]) + revQuery = query.FilterEntity(filterBuilder.PropertyFilter("FullRevisionHash", "=", sha[:40])) } var keys []Key @@ -393,7 +396,8 @@ func loadIDsForBrowserVersion(store Datastore, query Query, version string) (res result.Add(id) } // By exact match - if keys, err = store.GetAll(query.Filter("BrowserVersion =", version).KeysOnly(), nil); err != nil { + if keys, err = store.GetAll(query.FilterEntity( + query.FilterBuilder().PropertyFilter("BrowserVersion", "=", version)).KeysOnly(), nil); err != nil { return nil, err } for _, id := range GetTestRunIDs(keys) { @@ -409,11 +413,14 @@ func VersionPrefix(query Query, fieldName, versionPrefix string, desc bool) Quer if desc { order = "-" + order } + filterBuilder := query.FilterBuilder() return query. Limit(MaxCountMaxValue). Order(order). - Filter(fieldName+" >=", fmt.Sprintf("%s.", versionPrefix)). - Filter(fieldName+" <=", fmt.Sprintf("%s.%c", versionPrefix, '9'+1)) + FilterEntity( + filterBuilder.PropertyFilter(fieldName, ">=", fmt.Sprintf("%s.", versionPrefix))). + FilterEntity( + filterBuilder.PropertyFilter(fieldName, "<=", fmt.Sprintf("%s.%c", versionPrefix, '9'+1))) } func getTestRunRedisKey(id int64) string {