Skip to content

Commit 270e69c

Browse files
jcscottiiijcscottiii
committed
Add checks for the number of bookmarks and prevent removal of owned searches.
Change 1: Limit number of bookmarks We already have a limit for the number of saved searches a user can own. This code adds a new configuration for the number of bookmarks they can have. In addition to the 25 saved searches they can own, they also can bookmark 25 other saved searches that they don't own. Change 2: Prevent bookmark removal of owned searches When a user creates a saved search, they are automatically bookmarked (Similar to [issue tracker](https://developers.google.com/issue-tracker/guides/work-with-hotlist#:~:text=Hotlists%20that%20you%20own%20are%20starred%20automatically)). And in Issue Tracker, you can't remove the star if you are the admin. This change adds the same type of check.
1 parent 80e7bcc commit 270e69c

File tree

3 files changed

+239
-54
lines changed

3 files changed

+239
-54
lines changed

‎lib/gcpspanner/client.go

+9-1
Original file line numberDiff line numberDiff line change
@@ -121,10 +121,14 @@ func (w LocalBatchWriter) BatchWriteMutations(
121121

122122
// searchConfig holds the application configuation for the saved search feature.
123123
type searchConfig struct {
124+
// Max number saved searches per user.
124125
maxOwnedSearchesPerUser uint32
126+
// Max number of bookmarks per user (excluding the saved searches they own)
127+
maxBookmarksPerUser uint32
125128
}
126129

127130
const defaultMaxOwnedSearchesPerUser = 25
131+
const defaultMaxBookmarksPerUser = 25
128132
const defaultBatchSize = 10000
129133
const defaultBatchWriters = 8
130134

@@ -193,7 +197,10 @@ func NewSpannerClient(projectID string, instanceID string, name string) (*Client
193197
client,
194198
GCPFeatureSearchBaseQuery{},
195199
GCPMissingOneImplementationQuery{},
196-
searchConfig{maxOwnedSearchesPerUser: defaultMaxOwnedSearchesPerUser},
200+
searchConfig{
201+
maxOwnedSearchesPerUser: defaultMaxOwnedSearchesPerUser,
202+
maxBookmarksPerUser: defaultMaxBookmarksPerUser,
203+
},
197204
bw,
198205
defaultBatchSize,
199206
defaultBatchWriters,
@@ -730,6 +737,7 @@ type entityRemover[
730737
}
731738

732739
// remove performs an delete operation on an entity.
740+
// nolint: unused // TODO: Remove nolint directive once the method is used.
733741
func (c *entityRemover[M, ExternalStruct, SpannerStruct, ExternalKey]) remove(ctx context.Context,
734742
input ExternalStruct) error {
735743
_, err := c.ReadWriteTransaction(ctx, func(ctx context.Context, txn *spanner.ReadWriteTransaction) error {

‎lib/gcpspanner/user_search_bookmarks.go

+104-2
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ package gcpspanner
1616

1717
import (
1818
"context"
19+
"errors"
1920
"fmt"
2021

2122
"cloud.google.com/go/spanner"
@@ -74,10 +75,111 @@ func (m userSavedSearchBookmarkMapper) DeleteKey(key userSavedSearchBookmarkKey)
7475
return spanner.Key{key.UserID, key.SavedSearchID}
7576
}
7677

78+
var (
79+
// ErrUserSearchBookmarkLimitExceeded indicates that the user already has
80+
// reached the limit of saved searches that a given user can bookmark.
81+
ErrUserSearchBookmarkLimitExceeded = errors.New("user bookmark limit reached")
82+
// ErrOwnerCannotDeleteBookmark indicates that the user is the owner of the
83+
// saved search and cannot delete the bookmark.
84+
ErrOwnerCannotDeleteBookmark = errors.New("user is the owner of the saved search and cannot delete the bookmark")
85+
)
86+
7787
func (c *Client) AddUserSearchBookmark(ctx context.Context, req UserSavedSearchBookmark) error {
78-
return newEntityWriter[userSavedSearchBookmarkMapper](c).upsert(ctx, req)
88+
_, err := c.ReadWriteTransaction(ctx, func(ctx context.Context, txn *spanner.ReadWriteTransaction) error {
89+
// 1. Check if search id exists for this user
90+
_, err := newEntityReader[
91+
authenticatedUserSavedSearchMapper,
92+
UserSavedSearch,
93+
authenticatedUserSavedSearchMapperKey,
94+
](c).readRowByKeyWithTransaction(ctx, authenticatedUserSavedSearchMapperKey{
95+
UserID: req.UserID,
96+
ID: req.SavedSearchID,
97+
}, txn)
98+
if err != nil {
99+
return err
100+
}
101+
102+
// 2. Read the current count of bookmarks where the user is not the owner
103+
var count int64
104+
stmt := spanner.Statement{
105+
SQL: fmt.Sprintf(`
106+
SELECT COUNT(us.SavedSearchID)
107+
FROM %s us
108+
LEFT JOIN %s sr ON us.SavedSearchID = sr.SavedSearchID AND us.UserID = sr.UserID
109+
WHERE us.UserID = @UserID AND (sr.UserRole != @Role OR sr.UserRole IS NULL);
110+
`, userSavedSearchBookmarksTable, savedSearchUserRolesTable),
111+
Params: map[string]interface{}{
112+
"UserID": req.UserID,
113+
"Role": SavedSearchOwner,
114+
},
115+
}
116+
row, err := txn.Query(ctx, stmt).Next()
117+
if err != nil {
118+
return err
119+
}
120+
if err := row.Columns(&count); err != nil {
121+
return err
122+
}
123+
124+
// 3. Check against the limit
125+
if count >= int64(c.searchCfg.maxBookmarksPerUser) {
126+
return ErrUserSearchBookmarkLimitExceeded
127+
}
128+
129+
// 4. Insert the bookmark
130+
return newEntityWriter[userSavedSearchBookmarkMapper](c).upsertWithTransaction(ctx, txn, req)
131+
})
132+
133+
return err
79134
}
80135

81136
func (c *Client) DeleteUserSearchBookmark(ctx context.Context, req UserSavedSearchBookmark) error {
82-
return newEntityRemover[userSavedSearchBookmarkMapper, UserSavedSearchBookmark](c).remove(ctx, req)
137+
_, err := c.ReadWriteTransaction(ctx, func(ctx context.Context, txn *spanner.ReadWriteTransaction) error {
138+
// 1. Check if search id exists for this user
139+
_, err := newEntityReader[
140+
authenticatedUserSavedSearchMapper,
141+
UserSavedSearch,
142+
authenticatedUserSavedSearchMapperKey,
143+
](c).readRowByKeyWithTransaction(ctx, authenticatedUserSavedSearchMapperKey{
144+
UserID: req.UserID,
145+
ID: req.SavedSearchID,
146+
}, txn)
147+
if err != nil {
148+
return err
149+
}
150+
151+
// 2. Check if the user is the owner of the saved search
152+
var isOwner bool
153+
stmt := spanner.Statement{
154+
SQL: fmt.Sprintf(`
155+
SELECT EXISTS(
156+
SELECT 1
157+
FROM %s
158+
WHERE UserID = @UserID AND SavedSearchID = @SavedSearchID AND UserRole = @Role
159+
)
160+
`, savedSearchUserRolesTable),
161+
Params: map[string]interface{}{
162+
"UserID": req.UserID,
163+
"SavedSearchID": req.SavedSearchID,
164+
"Role": SavedSearchOwner,
165+
},
166+
}
167+
row, err := txn.Query(ctx, stmt).Next()
168+
if err != nil {
169+
return err
170+
}
171+
if err := row.Columns(&isOwner); err != nil {
172+
return err
173+
}
174+
175+
if isOwner {
176+
return ErrOwnerCannotDeleteBookmark
177+
}
178+
179+
// 3. Delete the bookmark
180+
return newEntityRemover[userSavedSearchBookmarkMapper, UserSavedSearchBookmark](c).
181+
removeWithTransaction(ctx, txn, req)
182+
})
183+
184+
return err
83185
}

‎lib/gcpspanner/user_search_bookmarks_test.go

+126-51
Original file line numberDiff line numberDiff line change
@@ -16,15 +16,30 @@ package gcpspanner
1616

1717
import (
1818
"context"
19+
"errors"
1920
"testing"
2021

2122
"cloud.google.com/go/spanner"
2223
)
2324

25+
func assertUserSearchSearchWithBookmarkStatus(
26+
ctx context.Context, t *testing.T, expectedSavedSearch *UserSavedSearch, savedSearchID *string, userID string) {
27+
actual, err := spannerClient.GetUserSavedSearch(ctx, *savedSearchID, valuePtr(userID))
28+
if err != nil {
29+
t.Errorf("expected nil error. received %s", err)
30+
}
31+
if !userSavedSearchEquality(expectedSavedSearch, actual) {
32+
t.Errorf("different saved searches\nexpected: %+v\nreceived: %v", expectedSavedSearch, actual)
33+
}
34+
}
35+
2436
func TestUserSearchBookmark(t *testing.T) {
2537
restartDatabaseContainer(t)
2638
ctx := context.Background()
2739

40+
// Reset the max bookmarks to 1.
41+
spannerClient.searchCfg.maxBookmarksPerUser = 1
42+
2843
savedSearchID, err := spannerClient.CreateNewUserSavedSearch(ctx, CreateUserSavedSearchRequest{
2944
Name: "my little search",
3045
Query: "group:css",
@@ -38,35 +53,24 @@ func TestUserSearchBookmark(t *testing.T) {
3853
t.Fatal("expected non-nil id.")
3954
}
4055

41-
const testUser = "test-user"
42-
43-
// user initially does not have a bookmark
44-
expectedSavedSearch := &UserSavedSearch{
45-
IsBookmarked: valuePtr(false),
46-
Role: nil,
47-
SavedSearch: SavedSearch{
48-
ID: *savedSearchID,
49-
Name: "my little search",
50-
Query: "group:css",
51-
Scope: "USER_PUBLIC",
52-
AuthorID: "userID1",
53-
Description: nil,
54-
// Don't actually compare the last two values.
55-
CreatedAt: spanner.CommitTimestamp,
56-
UpdatedAt: spanner.CommitTimestamp,
57-
},
58-
}
59-
actual, err := spannerClient.GetUserSavedSearch(ctx, *savedSearchID, valuePtr(testUser))
56+
savedSearchID2, err := spannerClient.CreateNewUserSavedSearch(ctx, CreateUserSavedSearchRequest{
57+
Name: "my big search",
58+
Query: "group:html",
59+
OwnerUserID: "userID1",
60+
Description: nil,
61+
})
6062
if err != nil {
6163
t.Errorf("expected nil error. received %s", err)
6264
}
63-
if !userSavedSearchEquality(expectedSavedSearch, actual) {
64-
t.Errorf("different saved searches\nexpected: %+v\nreceived: %v", expectedSavedSearch, actual)
65+
if savedSearchID2 == nil {
66+
t.Fatal("expected non-nil id.")
6567
}
6668

67-
// user can successfully have a bookmark added
68-
expectedSavedSearchAfter := &UserSavedSearch{
69-
IsBookmarked: valuePtr(true),
69+
var testUserSavedSearchID *string
70+
const testUser = "test-user"
71+
// user initially does not have a bookmark
72+
expectedSavedSearchBeforeBookmark := &UserSavedSearch{
73+
IsBookmarked: valuePtr(false),
7074
Role: nil,
7175
SavedSearch: SavedSearch{
7276
ID: *savedSearchID,
@@ -80,35 +84,106 @@ func TestUserSearchBookmark(t *testing.T) {
8084
UpdatedAt: spanner.CommitTimestamp,
8185
},
8286
}
83-
err = spannerClient.AddUserSearchBookmark(ctx, UserSavedSearchBookmark{
84-
UserID: testUser,
85-
SavedSearchID: *savedSearchID,
87+
t.Run("the test user can see they don't have bookmark status", func(t *testing.T) {
88+
assertUserSearchSearchWithBookmarkStatus(ctx, t, expectedSavedSearchBeforeBookmark, savedSearchID, testUser)
8689
})
87-
if err != nil {
88-
t.Errorf("expected nil error. received %s", err)
89-
}
90-
actual, err = spannerClient.GetUserSavedSearch(ctx, *savedSearchID, valuePtr(testUser))
91-
if err != nil {
92-
t.Errorf("expected nil error. received %s", err)
93-
}
94-
if !userSavedSearchEquality(expectedSavedSearchAfter, actual) {
95-
t.Errorf("different saved searches\nexpected: %+v\nreceived: %v", expectedSavedSearchAfter, actual)
96-
}
9790

98-
// user can successfully have a bookmark removed
99-
err = spannerClient.DeleteUserSearchBookmark(ctx, UserSavedSearchBookmark{
100-
UserID: testUser,
101-
SavedSearchID: *savedSearchID,
91+
t.Run("the test user can bookmark it", func(t *testing.T) {
92+
// user can successfully have a bookmark added
93+
expectedSavedSearchAfter := &UserSavedSearch{
94+
IsBookmarked: valuePtr(true),
95+
Role: nil,
96+
SavedSearch: SavedSearch{
97+
ID: *savedSearchID,
98+
Name: "my little search",
99+
Query: "group:css",
100+
Scope: "USER_PUBLIC",
101+
AuthorID: "userID1",
102+
Description: nil,
103+
// Don't actually compare the last two values.
104+
CreatedAt: spanner.CommitTimestamp,
105+
UpdatedAt: spanner.CommitTimestamp,
106+
},
107+
}
108+
err = spannerClient.AddUserSearchBookmark(ctx, UserSavedSearchBookmark{
109+
UserID: testUser,
110+
SavedSearchID: *savedSearchID,
111+
})
112+
if err != nil {
113+
t.Errorf("expected nil error. received %s", err)
114+
}
115+
116+
assertUserSearchSearchWithBookmarkStatus(ctx, t, expectedSavedSearchAfter, savedSearchID, testUser)
102117
})
103-
if err != nil {
104-
t.Errorf("expected nil error. received %s", err)
105-
}
106-
actual, err = spannerClient.GetUserSavedSearch(ctx, *savedSearchID, valuePtr(testUser))
107-
if err != nil {
108-
t.Errorf("expected nil error. received %s", err)
109-
}
110-
if !userSavedSearchEquality(expectedSavedSearch, actual) {
111-
t.Errorf("different saved searches\nexpected: %+v\nreceived: %v", expectedSavedSearch, actual)
112-
}
113118

119+
t.Run("the test user gets a limit error once it tries to bookmark too many", func(t *testing.T) {
120+
err = spannerClient.AddUserSearchBookmark(ctx, UserSavedSearchBookmark{
121+
UserID: testUser,
122+
SavedSearchID: *savedSearchID2,
123+
})
124+
if !errors.Is(err, ErrUserSearchBookmarkLimitExceeded) {
125+
t.Errorf("expected ErrUserSearchBookmarkLimitExceeded error. received %s", err)
126+
}
127+
})
128+
129+
// Assuming they have not hit the saved search limit. (That is tested in create_user_saved_search_test.go)
130+
t.Run("the test user can still make saved searches even after hitting the bookmark limit", func(t *testing.T) {
131+
var err error
132+
testUserSavedSearchID, err = spannerClient.CreateNewUserSavedSearch(ctx, CreateUserSavedSearchRequest{
133+
Name: "my really big search",
134+
Query: "group:html OR group:css",
135+
OwnerUserID: testUser,
136+
Description: nil,
137+
})
138+
if err != nil {
139+
t.Errorf("expected nil error. received %s", err)
140+
}
141+
if testUserSavedSearchID == nil {
142+
t.Fatal("expected non-nil id.")
143+
}
144+
})
145+
146+
t.Run("the test user can remove a bookmark for a saved search they don't own", func(t *testing.T) {
147+
err = spannerClient.DeleteUserSearchBookmark(ctx, UserSavedSearchBookmark{
148+
UserID: testUser,
149+
SavedSearchID: *savedSearchID,
150+
})
151+
if err != nil {
152+
t.Errorf("expected nil error. received %s", err)
153+
}
154+
155+
assertUserSearchSearchWithBookmarkStatus(ctx, t, expectedSavedSearchBeforeBookmark, savedSearchID, testUser)
156+
})
157+
158+
t.Run("the test user gets ErrQueryReturnedNoResults when trying to remove bookmark that does not exist (anymore)",
159+
func(t *testing.T) {
160+
err = spannerClient.DeleteUserSearchBookmark(ctx, UserSavedSearchBookmark{
161+
UserID: testUser,
162+
SavedSearchID: *savedSearchID,
163+
})
164+
if !errors.Is(err, ErrQueryReturnedNoResults) {
165+
t.Errorf("expected ErrQueryReturnedNoResults error. received %s", err)
166+
}
167+
})
168+
169+
t.Run("the test user gets ErrQueryReturnedNoResults when trying to add a bookmark for search that does not exist",
170+
func(t *testing.T) {
171+
err = spannerClient.AddUserSearchBookmark(ctx, UserSavedSearchBookmark{
172+
UserID: testUser,
173+
SavedSearchID: "fake-uuid",
174+
})
175+
if !errors.Is(err, ErrQueryReturnedNoResults) {
176+
t.Errorf("expected ErrQueryReturnedNoResults error. received %s", err)
177+
}
178+
})
179+
180+
t.Run("the test user cannot remove a bookmark for a saved search they own", func(t *testing.T) {
181+
err = spannerClient.DeleteUserSearchBookmark(ctx, UserSavedSearchBookmark{
182+
UserID: testUser,
183+
SavedSearchID: *testUserSavedSearchID,
184+
})
185+
if !errors.Is(err, ErrOwnerCannotDeleteBookmark) {
186+
t.Errorf("expected ErrOwnerCannotDeleteBookmark error. received %s", err)
187+
}
188+
})
114189
}

0 commit comments

Comments
 (0)