[gold] Remove pointers from ignore.Store interface

Because the ignore.Rule is a small struct, there's no real benefit
to it being passed as a pointer everywhere. In fact, this is dangerous
because we were caching the list of rules, which could have been
accidentally modified by a user, impacting future reads of the cache.

Change-Id: I5cae23a904036d2ee11c3ae1003b88de6aa471ec
Bug: skia:9778
Reviewed-on: https://skia-review.googlesource.com/c/buildbot/+/264304
Reviewed-by: Leandro Lovisolo <lovisolo@google.com>
diff --git a/golden/go/ignore/ds_ignorestore/ds_ignorestore.go b/golden/go/ignore/ds_ignorestore/ds_ignorestore.go
index c581234..813035f 100644
--- a/golden/go/ignore/ds_ignorestore/ds_ignorestore.go
+++ b/golden/go/ignore/ds_ignorestore/ds_ignorestore.go
@@ -43,7 +43,7 @@
 	Note      string
 }
 
-func fromRule(r *ignore.Rule) (*dsRule, error) {
+func fromRule(r ignore.Rule) (*dsRule, error) {
 	var id int64
 	if r.ID != "" {
 		var err error
@@ -63,8 +63,8 @@
 	}, nil
 }
 
-func (r *dsRule) ToRule() *ignore.Rule {
-	return &ignore.Rule{
+func (r *dsRule) toRule() ignore.Rule {
+	return ignore.Rule{
 		ID:        strconv.FormatInt(r.ID, 10),
 		Name:      r.Name,
 		UpdatedBy: r.UpdatedBy,
@@ -92,7 +92,7 @@
 }
 
 // Create implements the IgnoreStore interface.
-func (c *DSIgnoreStore) Create(ctx context.Context, ir *ignore.Rule) error {
+func (c *DSIgnoreStore) Create(ctx context.Context, ir ignore.Rule) error {
 	c.ignoreCache.Delete(listCacheKey)
 
 	r, err := fromRule(ir)
@@ -119,9 +119,9 @@
 }
 
 // List implements the IgnoreStore interface.
-func (c *DSIgnoreStore) List(ctx context.Context) ([]*ignore.Rule, error) {
+func (c *DSIgnoreStore) List(ctx context.Context) ([]ignore.Rule, error) {
 	if rules, ok := c.ignoreCache.Get(listCacheKey); ok {
-		rv, ok := rules.([]*ignore.Rule)
+		rv, ok := rules.([]ignore.Rule)
 		if ok {
 			return rv, nil
 		}
@@ -152,7 +152,7 @@
 	// Merge the keys to get all of the current keys.
 	allKeys := recently.Combine(queriedKeys)
 	if len(allKeys) == 0 {
-		return []*ignore.Rule{}, nil
+		return []ignore.Rule{}, nil
 	}
 
 	rules := make([]*dsRule, len(allKeys))
@@ -160,9 +160,9 @@
 		return nil, skerr.Wrap(err)
 	}
 
-	ret := make([]*ignore.Rule, 0, len(rules))
+	ret := make([]ignore.Rule, 0, len(rules))
 	for _, r := range rules {
-		ret = append(ret, r.ToRule())
+		ret = append(ret, r.toRule())
 	}
 
 	sort.Slice(ret, func(i, j int) bool { return ret[i].Expires.Before(ret[j].Expires) })
@@ -172,7 +172,7 @@
 }
 
 // Update implements the IgnoreStore interface.
-func (c *DSIgnoreStore) Update(ctx context.Context, rule *ignore.Rule) error {
+func (c *DSIgnoreStore) Update(ctx context.Context, rule ignore.Rule) error {
 	if rule.ID == "" {
 		return skerr.Fmt("Updated an empty rule")
 	}
diff --git a/golden/go/ignore/ds_ignorestore/ds_ignorestore_test.go b/golden/go/ignore/ds_ignorestore/ds_ignorestore_test.go
index c33eb63..39bf68d 100644
--- a/golden/go/ignore/ds_ignorestore/ds_ignorestore_test.go
+++ b/golden/go/ignore/ds_ignorestore/ds_ignorestore_test.go
@@ -6,13 +6,13 @@
 	"time"
 
 	"github.com/stretchr/testify/assert"
-	"go.skia.org/infra/go/sktest"
-	"go.skia.org/infra/golden/go/ignore"
-
 	"github.com/stretchr/testify/require"
+
 	"go.skia.org/infra/go/ds"
 	ds_testutil "go.skia.org/infra/go/ds/testutil"
+	"go.skia.org/infra/go/sktest"
 	"go.skia.org/infra/go/testutils/unittest"
+	"go.skia.org/infra/golden/go/ignore"
 )
 
 func TestDatastoreIgnoreStore(t *testing.T) {
@@ -38,15 +38,16 @@
 	require.NoError(t, store.Create(context.Background(), r3))
 	require.NoError(t, store.Create(context.Background(), r4))
 
+	allRules, err := store.List(context.Background())
+	require.NoError(t, err)
+	require.Equal(t, 4, len(allRules))
+
+	r1, r2, r3, r4 = allRules[0], allRules[1], allRules[2], allRules[3]
 	assert.NotZero(t, r1.ID)
 	assert.NotZero(t, r2.ID)
 	assert.NotZero(t, r3.ID)
 	assert.NotZero(t, r4.ID)
 
-	allRules, err := store.List(context.Background())
-	require.NoError(t, err)
-	require.Equal(t, 4, len(allRules))
-
 	// Remove the third and fourth rule
 	delCount, err := store.Delete(context.Background(), r3.ID)
 	require.NoError(t, err)
@@ -75,9 +76,9 @@
 	require.Equal(t, r2.ID, allRules[0].ID)
 
 	// Update a rule.
-	updatedRule := *allRules[0]
+	updatedRule := allRules[0]
 	updatedRule.Note = "an updated rule"
-	err = store.Update(context.Background(), &updatedRule)
+	err = store.Update(context.Background(), updatedRule)
 	require.NoError(t, err, "Update should succeed.")
 	allRules, err = store.List(context.Background())
 	require.NoError(t, err)
@@ -87,7 +88,7 @@
 
 	// Try to update a rule with an empty ID
 	updatedRule = ignore.Rule{}
-	err = store.Update(context.Background(), &updatedRule)
+	err = store.Update(context.Background(), updatedRule)
 	require.Error(t, err, "Update should fail for an empty id.")
 
 	delCount, err = store.Delete(context.Background(), r2.ID)
diff --git a/golden/go/ignore/ignore.go b/golden/go/ignore/ignore.go
index 6dc62722..15794a7 100644
--- a/golden/go/ignore/ignore.go
+++ b/golden/go/ignore/ignore.go
@@ -16,13 +16,13 @@
 // Store is an interface for a database that saves ignore rules.
 type Store interface {
 	// Create adds a new rule to the ignore store. The ID will be set if this call is successful.
-	Create(context.Context, *Rule) error
+	Create(context.Context, Rule) error
 
 	// List returns all ignore rules in the ignore store.
-	List(context.Context) ([]*Rule, error)
+	List(context.Context) ([]Rule, error)
 
 	// Update sets a Rule.
-	Update(ctx context.Context, rule *Rule) error
+	Update(ctx context.Context, 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).
@@ -49,8 +49,8 @@
 }
 
 // NewRule creates a new ignore rule with the given data.
-func NewRule(createdByUser string, expires time.Time, queryStr string, note string) *Rule {
-	return &Rule{
+func NewRule(createdByUser string, expires time.Time, queryStr string, note string) Rule {
+	return Rule{
 		Name:      createdByUser,
 		UpdatedBy: createdByUser,
 		Expires:   expires,
@@ -60,7 +60,7 @@
 }
 
 // toQuery makes a slice of url.Values from the given slice of Rules.
-func toQuery(ignores []*Rule) ([]url.Values, error) {
+func toQuery(ignores []Rule) ([]url.Values, error) {
 	var ret []url.Values
 	for _, ignore := range ignores {
 		v, err := url.ParseQuery(ignore.Query)
@@ -75,7 +75,7 @@
 // FilterIgnored returns a copy of the given tile with all traces removed
 // that match the ignore rules in the given ignore store. It also returns the
 // ignore rules for later matching.
-func FilterIgnored(inputTile *tiling.Tile, ignores []*Rule) (*tiling.Tile, paramtools.ParamMatcher, error) {
+func FilterIgnored(inputTile *tiling.Tile, ignores []Rule) (*tiling.Tile, paramtools.ParamMatcher, error) {
 	// Make a shallow copy with a new Traces map
 	ret := &tiling.Tile{
 		Traces:   map[tiling.TraceID]tiling.Trace{},
diff --git a/golden/go/ignore/ignore_test.go b/golden/go/ignore/ignore_test.go
index 76b97c6..4c1981a 100644
--- a/golden/go/ignore/ignore_test.go
+++ b/golden/go/ignore/ignore_test.go
@@ -13,18 +13,18 @@
 
 func TestToQuery(t *testing.T) {
 	unittest.SmallTest(t)
-	queries, err := toQuery([]*Rule{})
+	queries, err := toQuery([]Rule{})
 	require.NoError(t, err)
 	require.Len(t, queries, 0)
 
 	r1 := NewRule("jon@example.com", time.Now().Add(time.Hour), "config=gpu", "reason")
-	queries, err = toQuery([]*Rule{r1})
+	queries, err = toQuery([]Rule{r1})
 	require.NoError(t, err)
 	require.Equal(t, queries[0], url.Values{"config": []string{"gpu"}})
 
 	// A bad rule won't get converted
 	r1 = NewRule("jon@example.com", time.Now().Add(time.Hour), "bad=%", "reason")
-	queries, err = toQuery([]*Rule{r1})
+	queries, err = toQuery([]Rule{r1})
 	require.NotNil(t, err)
 	require.Empty(t, queries)
 }
@@ -39,7 +39,7 @@
 	require.Equal(t, data.MakeTestTile(), ft)
 
 	future := time.Now().Add(time.Hour)
-	ignores := []*Rule{
+	ignores := []Rule{
 		NewRule("user@example.com", future, "device=crosshatch", "note"),
 	}
 
diff --git a/golden/go/ignore/mocks/Store.go b/golden/go/ignore/mocks/Store.go
index a65c761..173e4cf 100644
--- a/golden/go/ignore/mocks/Store.go
+++ b/golden/go/ignore/mocks/Store.go
@@ -15,11 +15,11 @@
 }
 
 // Create provides a mock function with given fields: _a0, _a1
-func (_m *Store) Create(_a0 context.Context, _a1 *ignore.Rule) error {
+func (_m *Store) Create(_a0 context.Context, _a1 ignore.Rule) error {
 	ret := _m.Called(_a0, _a1)
 
 	var r0 error
-	if rf, ok := ret.Get(0).(func(context.Context, *ignore.Rule) error); ok {
+	if rf, ok := ret.Get(0).(func(context.Context, ignore.Rule) error); ok {
 		r0 = rf(_a0, _a1)
 	} else {
 		r0 = ret.Error(0)
@@ -50,15 +50,15 @@
 }
 
 // List provides a mock function with given fields: _a0
-func (_m *Store) List(_a0 context.Context) ([]*ignore.Rule, error) {
+func (_m *Store) List(_a0 context.Context) ([]ignore.Rule, error) {
 	ret := _m.Called(_a0)
 
-	var r0 []*ignore.Rule
-	if rf, ok := ret.Get(0).(func(context.Context) []*ignore.Rule); ok {
+	var r0 []ignore.Rule
+	if rf, ok := ret.Get(0).(func(context.Context) []ignore.Rule); ok {
 		r0 = rf(_a0)
 	} else {
 		if ret.Get(0) != nil {
-			r0 = ret.Get(0).([]*ignore.Rule)
+			r0 = ret.Get(0).([]ignore.Rule)
 		}
 	}
 
@@ -73,11 +73,11 @@
 }
 
 // Update provides a mock function with given fields: ctx, rule
-func (_m *Store) Update(ctx context.Context, rule *ignore.Rule) error {
+func (_m *Store) Update(ctx context.Context, rule ignore.Rule) error {
 	ret := _m.Called(ctx, rule)
 
 	var r0 error
-	if rf, ok := ret.Get(0).(func(context.Context, *ignore.Rule) error); ok {
+	if rf, ok := ret.Get(0).(func(context.Context, ignore.Rule) error); ok {
 		r0 = rf(ctx, rule)
 	} else {
 		r0 = ret.Error(0)
diff --git a/golden/go/tilesource/tilesource_test.go b/golden/go/tilesource/tilesource_test.go
index 59011bf..763ecb7 100644
--- a/golden/go/tilesource/tilesource_test.go
+++ b/golden/go/tilesource/tilesource_test.go
@@ -351,7 +351,7 @@
 	mts.On("GetDenseTile", testutils.AnyContext, nCommits).Return(data.MakeTestTile(), makeSparseTilingCommits(), nil)
 
 	// No ignores in this test
-	mis.On("List", testutils.AnyContext).Return([]*ignore.Rule{
+	mis.On("List", testutils.AnyContext).Return([]ignore.Rule{
 		{
 			Query: "device=crosshatch&name=test_beta", // hides one trace
 		},
diff --git a/golden/go/web/frontend/types.go b/golden/go/web/frontend/types.go
index d9826b1..6733522 100644
--- a/golden/go/web/frontend/types.go
+++ b/golden/go/web/frontend/types.go
@@ -161,12 +161,12 @@
 
 // ConvertIgnoreRule converts a backend ignore.Rule into its frontend
 // counterpart.
-func ConvertIgnoreRule(r *ignore.Rule) (*IgnoreRule, error) {
+func ConvertIgnoreRule(r ignore.Rule) (IgnoreRule, error) {
 	pq, err := url.ParseQuery(r.Query)
 	if err != nil {
-		return nil, skerr.Wrapf(err, "invalid rule id %q; query %q", r.ID, r.Query)
+		return IgnoreRule{}, skerr.Wrapf(err, "invalid rule id %q; query %q", r.ID, r.Query)
 	}
-	return &IgnoreRule{
+	return IgnoreRule{
 		ID:          r.ID,
 		Name:        r.Name,
 		UpdatedBy:   r.UpdatedBy,
diff --git a/golden/go/web/web.go b/golden/go/web/web.go
index 529eb1b..aa698d6 100644
--- a/golden/go/web/web.go
+++ b/golden/go/web/web.go
@@ -662,16 +662,18 @@
 		return nil, skerr.Wrapf(err, "fetching ignores from store")
 	}
 
+	// We want to make a slice of pointers because addIgnoreCounts will add the counts in-place.
 	ret := make([]*frontend.IgnoreRule, 0, len(rules))
 	for _, r := range rules {
 		fr, err := frontend.ConvertIgnoreRule(r)
 		if err != nil {
 			return nil, skerr.Wrap(err)
 		}
-		ret = append(ret, fr)
+		ret = append(ret, &fr)
 	}
 
 	if withCounts {
+		// addIgnoreCounts updates the values of ret directly
 		if err := wh.addIgnoreCounts(ctx, ret); err != nil {
 			return nil, skerr.Wrapf(err, "adding ignore counts to %d rules", len(ret))
 		}
diff --git a/golden/go/web/web_test.go b/golden/go/web/web_test.go
index 6e6be58..0b56775 100644
--- a/golden/go/web/web_test.go
+++ b/golden/go/web/web_test.go
@@ -168,7 +168,7 @@
 // with the given ignores. Like makeBugRevertIndex, we return a real SearchIndex.
 // If multiplier is > 1, duplicate traces will be added to the tile to make it artificially
 // bigger.
-func makeBugRevertIndexWithIgnores(ir []*ignore.Rule, multiplier int) *indexer.SearchIndex {
+func makeBugRevertIndexWithIgnores(ir []ignore.Rule, multiplier int) *indexer.SearchIndex {
 	tile := bug_revert.MakeTestTile()
 	add := make([]types.TracePair, 0, multiplier*len(tile.Traces))
 	for i := 1; i < multiplier; i++ {
@@ -950,7 +950,7 @@
 		Query:     filter,
 		Note:      note,
 	}
-	mis.On("Create", testutils.AnyContext, &expectedRule).Return(nil)
+	mis.On("Create", testutils.AnyContext, expectedRule).Return(nil)
 
 	wh := Handlers{
 		HandlersConfig: HandlersConfig{
@@ -984,7 +984,7 @@
 		Query:     filter,
 		Note:      note,
 	}
-	mis.On("Update", testutils.AnyContext, &expectedRule).Return(nil)
+	mis.On("Update", testutils.AnyContext, expectedRule).Return(nil)
 
 	wh := Handlers{
 		HandlersConfig: HandlersConfig{
@@ -1007,8 +1007,8 @@
 	thirdRuleExpire  = time.Date(2020, time.November, 27, 3, 4, 5, 0, time.UTC)
 )
 
-func makeIgnoreRules() []*ignore.Rule {
-	return []*ignore.Rule{
+func makeIgnoreRules() []ignore.Rule {
+	return []ignore.Rule{
 		{
 			ID:        "1234",
 			Name:      "user@example.com",