[gold] Refactor ignorestore to use string ids
They were effectively strings in the frontend before (due to the 64 bit
ints not fitting in a JS number), and if we migrate to firestore, I want
the flexibility of having strings as the IDs.
Change-Id: I2dbbfb3b81e2f03f552e7dd9a2d6a45024a25a76
Reviewed-on: https://skia-review.googlesource.com/c/buildbot/+/251456
Commit-Queue: Kevin Lubick <kjlubick@google.com>
Reviewed-by: Leandro Lovisolo <lovisolo@google.com>
diff --git a/go/ds/ds.go b/go/ds/ds.go
index 46f258f..90c9df8 100644
--- a/go/ds/ds.go
+++ b/go/ds/ds.go
@@ -42,9 +42,6 @@
ALERT Kind = "Alert"
// Gold
- ISSUE Kind = "Issue"
- TRYJOB Kind = "Tryjob"
- TRYJOB_RESULT Kind = "TryjobResult"
IGNORE_RULE Kind = "IgnoreRule"
HELPER_RECENT_KEYS Kind = "HelperRecentKeys"
diff --git a/golden/go/ignore/ds_ignorestore/ds_ignorestore.go b/golden/go/ignore/ds_ignorestore/ds_ignorestore.go
index d724d91..7d93320 100644
--- a/golden/go/ignore/ds_ignorestore/ds_ignorestore.go
+++ b/golden/go/ignore/ds_ignorestore/ds_ignorestore.go
@@ -3,6 +3,7 @@
import (
"context"
"sort"
+ "strconv"
"cloud.google.com/go/datastore"
"go.skia.org/infra/go/ds"
@@ -39,7 +40,7 @@
func (c *DSIgnoreStore) Create(ctx context.Context, ignoreRule *ignore.Rule) error {
createFn := func(tx *datastore.Transaction) error {
key := dsutil.TimeSortableKey(ds.IGNORE_RULE, 0)
- ignoreRule.ID = key.ID
+ ignoreRule.ID = strconv.FormatInt(key.ID, 10)
// Add the new rule and put its key with the recently added keys.
if _, err := tx.Put(key, ignoreRule); err != nil {
@@ -93,15 +94,23 @@
}
// Update implements the IgnoreStore interface.
-func (c *DSIgnoreStore) Update(ctx context.Context, id int64, rule *ignore.Rule) error {
+func (c *DSIgnoreStore) Update(ctx context.Context, id string, rule *ignore.Rule) error {
key := ds.NewKey(ds.IGNORE_RULE)
- key.ID = id
- _, err := c.client.Mutate(ctx, datastore.NewUpdate(key, rule))
+ var err error
+ key.ID, err = strconv.ParseInt(id, 10, 64)
+ if err != nil {
+ return skerr.Wrapf(err, "id must be int64: %q", id)
+ }
+ _, err = c.client.Mutate(ctx, datastore.NewUpdate(key, rule))
return skerr.Wrap(err)
}
// Delete implements the IgnoreStore interface.
-func (c *DSIgnoreStore) Delete(ctx context.Context, id int64) (int, error) {
+func (c *DSIgnoreStore) Delete(ctx context.Context, idStr string) (int, error) {
+ id, err := strconv.ParseInt(idStr, 10, 64)
+ if err != nil {
+ return 0, skerr.Wrapf(err, "id must be int64: %q", idStr)
+ }
if id <= 0 {
return 0, skerr.Fmt("Given id does not exist: %d", id)
}
@@ -123,7 +132,7 @@
}
// Run the relevant updates in a transaction.
- _, err := c.client.RunInTransaction(ctx, deleteFn)
+ _, err = c.client.RunInTransaction(ctx, deleteFn)
if err != nil {
// Don't report an error if the item did not exist.
if err == datastore.ErrNoSuchEntity {
diff --git a/golden/go/ignore/ds_ignorestore/ds_ignorestore_test.go b/golden/go/ignore/ds_ignorestore/ds_ignorestore_test.go
index 51ac5e1..fa083bf 100644
--- a/golden/go/ignore/ds_ignorestore/ds_ignorestore_test.go
+++ b/golden/go/ignore/ds_ignorestore/ds_ignorestore_test.go
@@ -81,7 +81,7 @@
// Try to update a non-existent rule.
updatedRule = *allRules[0]
- err = store.Update(context.Background(), 100001, &updatedRule)
+ err = store.Update(context.Background(), "100001", &updatedRule)
require.Error(t, err, "Update should fail for a bad id.")
delCount, err = store.Delete(context.Background(), r2.ID)
@@ -93,7 +93,7 @@
require.Equal(t, 0, len(allRules))
// This id doesn't exist, so we shouldn't be able to delete it.
- delCount, err = store.Delete(context.Background(), 1000000)
+ delCount, err = store.Delete(context.Background(), "1000000")
require.NoError(t, err)
require.Equal(t, delCount, 0)
allRules, err = store.List(context.Background())
diff --git a/golden/go/ignore/ignorestore.go b/golden/go/ignore/ignorestore.go
index 25b105c..e9be168 100644
--- a/golden/go/ignore/ignorestore.go
+++ b/golden/go/ignore/ignorestore.go
@@ -20,16 +20,16 @@
List(context.Context) ([]*Rule, error)
// Update sets a Rule.
- Update(ctx context.Context, id int64, rule *Rule) error
+ Update(ctx context.Context, id string, rule *Rule) error
// Delete removes a Rule from the store. The return value is the number of
// records that were deleted (either 0 or 1).
- Delete(ctx context.Context, id int64) (int, error)
+ Delete(ctx context.Context, id string) (int, error)
}
// Rule is the GUI struct for dealing with Ignore rules.
type Rule struct {
- ID int64 `json:"id,string"`
+ ID string `json:"id"`
Name string `json:"name"`
UpdatedBy string `json:"updatedBy"`
Expires time.Time `json:"expires"`
@@ -43,7 +43,7 @@
for _, ignore := range ignores {
v, err := url.ParseQuery(ignore.Query)
if err != nil {
- return nil, skerr.Wrapf(err, "invalid ignore rule %d %s", ignore.ID, ignore.Query)
+ return nil, skerr.Wrapf(err, "invalid ignore rule id %q; query %q", ignore.ID, ignore.Query)
}
ret = append(ret, v)
}
diff --git a/golden/go/ignore/mocks/Store.go b/golden/go/ignore/mocks/Store.go
index ee390f4..72f6dfd 100644
--- a/golden/go/ignore/mocks/Store.go
+++ b/golden/go/ignore/mocks/Store.go
@@ -29,18 +29,18 @@
}
// Delete provides a mock function with given fields: ctx, id
-func (_m *Store) Delete(ctx context.Context, id int64) (int, error) {
+func (_m *Store) Delete(ctx context.Context, id string) (int, error) {
ret := _m.Called(ctx, id)
var r0 int
- if rf, ok := ret.Get(0).(func(context.Context, int64) int); ok {
+ if rf, ok := ret.Get(0).(func(context.Context, string) int); ok {
r0 = rf(ctx, id)
} else {
r0 = ret.Get(0).(int)
}
var r1 error
- if rf, ok := ret.Get(1).(func(context.Context, int64) error); ok {
+ if rf, ok := ret.Get(1).(func(context.Context, string) error); ok {
r1 = rf(ctx, id)
} else {
r1 = ret.Error(1)
@@ -73,11 +73,11 @@
}
// Update provides a mock function with given fields: ctx, id, rule
-func (_m *Store) Update(ctx context.Context, id int64, rule *ignore.Rule) error {
+func (_m *Store) Update(ctx context.Context, id string, rule *ignore.Rule) error {
ret := _m.Called(ctx, id, rule)
var r0 error
- if rf, ok := ret.Get(0).(func(context.Context, int64, *ignore.Rule) error); ok {
+ if rf, ok := ret.Get(0).(func(context.Context, string, *ignore.Rule) error); ok {
r0 = rf(ctx, id, rule)
} else {
r0 = ret.Error(0)
diff --git a/golden/go/web/web.go b/golden/go/web/web.go
index 13ee22d..f94a609 100644
--- a/golden/go/web/web.go
+++ b/golden/go/web/web.go
@@ -8,7 +8,6 @@
"net/http"
"net/url"
"sort"
- "strconv"
"strings"
"time"
@@ -619,13 +618,6 @@
sendJSONResponse(w, ret)
}
-// IgnoresRequest encapsulates a single ignore rule that is submitted for addition or update.
-type IgnoresRequest struct {
- Duration string `json:"duration"`
- Filter string `json:"filter"`
- Note string `json:"note"`
-}
-
// IgnoresHandler returns the current ignore rules in JSON format.
func (wh *Handlers) IgnoresHandler(w http.ResponseWriter, r *http.Request) {
defer metrics2.FuncTimer().Stop()
@@ -637,45 +629,51 @@
w.Header().Set("Content-Type", "application/json")
// TODO(kjlubick): these ignore structs used to have counts of how often they were applied
- // in the file - Fix that after the Storages refactoring.
+ // in the file - Fix that after the Storages refactoring.
ignores, err := wh.IgnoreStore.List(r.Context())
if err != nil {
httputils.ReportError(w, err, "Failed to retrieve ignore rules, there may be none.", http.StatusInternalServerError)
return
}
- // TODO(stephana): Wrap in response envelope if it makes sense !
enc := json.NewEncoder(w)
if err := enc.Encode(ignores); err != nil {
sklog.Errorf("Failed to write or encode result: %s", err)
}
}
+// IgnoresRequest encapsulates a single ignore rule that is submitted for addition or update.
+type IgnoresRequest struct {
+ Duration string `json:"duration"`
+ Filter string `json:"filter"`
+ Note string `json:"note"`
+}
+
// IgnoresUpdateHandler updates an existing ignores rule.
func (wh *Handlers) IgnoresUpdateHandler(w http.ResponseWriter, r *http.Request) {
defer metrics2.FuncTimer().Stop()
user := login.LoggedInAs(r)
if user == "" {
- httputils.ReportError(w, fmt.Errorf("Not logged in."), "You must be logged in to update an ignore rule.", http.StatusInternalServerError)
+ http.Error(w, "You must be logged in to update an ignore rule.", http.StatusUnauthorized)
return
}
- id, err := strconv.ParseInt(mux.Vars(r)["id"], 10, 0)
- if err != nil {
- httputils.ReportError(w, err, "ID must be valid integer.", http.StatusInternalServerError)
+ id := mux.Vars(r)["id"]
+ if id == "" {
+ http.Error(w, "ID must be non-empty.", http.StatusBadRequest)
return
}
req := &IgnoresRequest{}
if err := parseJSON(r, req); err != nil {
- httputils.ReportError(w, err, "Failed to parse submitted data.", http.StatusInternalServerError)
+ httputils.ReportError(w, err, "Failed to parse submitted data", http.StatusBadRequest)
return
}
if req.Filter == "" {
- httputils.ReportError(w, fmt.Errorf("Invalid Filter: %q", req.Filter), "Filters can't be empty.", http.StatusInternalServerError)
+ http.Error(w, "Filter can't be empty", http.StatusBadRequest)
return
}
d, err := human.ParseDuration(req.Duration)
if err != nil {
- httputils.ReportError(w, err, "Failed to parse duration", http.StatusInternalServerError)
+ httputils.ReportError(w, err, "Failed to parse duration", http.StatusBadRequest)
return
}
ignoreRule := ignore.NewRule(user, time.Now().Add(d), req.Filter, req.Note)
@@ -683,7 +681,7 @@
err = wh.IgnoreStore.Update(r.Context(), id, ignoreRule)
if err != nil {
- httputils.ReportError(w, err, "Unable to update ignore rule.", http.StatusInternalServerError)
+ httputils.ReportError(w, err, "Unable to update ignore rule", http.StatusInternalServerError)
return
}
@@ -696,23 +694,24 @@
defer metrics2.FuncTimer().Stop()
user := login.LoggedInAs(r)
if user == "" {
- httputils.ReportError(w, fmt.Errorf("Not logged in."), "You must be logged in to add an ignore rule.", http.StatusInternalServerError)
+ http.Error(w, "You must be logged in to delete an ignore rule", http.StatusUnauthorized)
return
}
- id, err := strconv.ParseInt(mux.Vars(r)["id"], 10, 0)
- if err != nil {
- httputils.ReportError(w, err, "ID must be valid integer.", http.StatusInternalServerError)
+ id := mux.Vars(r)["id"]
+ if id == "" {
+ http.Error(w, "ID must be non-empty.", http.StatusBadRequest)
return
}
if numDeleted, err := wh.IgnoreStore.Delete(r.Context(), id); err != nil {
- httputils.ReportError(w, err, "Unable to delete ignore rule.", http.StatusInternalServerError)
+ httputils.ReportError(w, err, "Unable to delete ignore rule", http.StatusInternalServerError)
+ return
} else if numDeleted == 1 {
- sklog.Infof("Successfully deleted ignore with id %d", id)
+ sklog.Infof("Successfully deleted ignore with id %s", id)
// If delete worked just list the current ignores and return them.
wh.IgnoresHandler(w, r)
} else {
- sklog.Infof("Deleting ignore with id %d from ignorestore failed", id)
+ sklog.Infof("Deleting ignore with id %s from ignorestore failed", id)
http.Error(w, "Could not delete ignore - try again later", http.StatusInternalServerError)
return
}
@@ -723,27 +722,27 @@
defer metrics2.FuncTimer().Stop()
user := login.LoggedInAs(r)
if user == "" {
- httputils.ReportError(w, fmt.Errorf("Not logged in."), "You must be logged in to add an ignore rule.", http.StatusInternalServerError)
+ http.Error(w, "You must be logged in to add an ignore rule", http.StatusUnauthorized)
return
}
req := &IgnoresRequest{}
if err := parseJSON(r, req); err != nil {
- httputils.ReportError(w, err, "Failed to parse submitted data.", http.StatusInternalServerError)
+ httputils.ReportError(w, err, "Failed to parse submitted data", http.StatusBadRequest)
return
}
if req.Filter == "" {
- httputils.ReportError(w, fmt.Errorf("Invalid Filter: %q", req.Filter), "Filters can't be empty.", http.StatusInternalServerError)
+ http.Error(w, "Filter can't be empty", http.StatusBadRequest)
return
}
d, err := human.ParseDuration(req.Duration)
if err != nil {
- httputils.ReportError(w, err, "Failed to parse duration", http.StatusInternalServerError)
+ httputils.ReportError(w, err, "Failed to parse duration", http.StatusBadRequest)
return
}
ignoreRule := ignore.NewRule(user, time.Now().Add(d), req.Filter, req.Note)
if err = wh.IgnoreStore.Create(r.Context(), ignoreRule); err != nil {
- httputils.ReportError(w, err, "Failed to create ignore rule.", http.StatusInternalServerError)
+ httputils.ReportError(w, err, "Failed to create ignore rule", http.StatusInternalServerError)
return
}
