From 1358d816b19271b90858d7a19eabed95e649e31e Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Mon, 5 Jun 2023 13:08:38 +0200 Subject: [PATCH 1/4] feat: sort users by username --- coderd/database/dbfake/databasefake.go | 9 ++------- coderd/database/dbfake/databasefake_test.go | 20 ++++++++++---------- coderd/database/queries.sql.go | 5 ++--- coderd/database/queries/users.sql | 5 ++--- 4 files changed, 16 insertions(+), 23 deletions(-) diff --git a/coderd/database/dbfake/databasefake.go b/coderd/database/dbfake/databasefake.go index c35acd6d3b1da..cdcd93e11b7be 100644 --- a/coderd/database/dbfake/databasefake.go +++ b/coderd/database/dbfake/databasefake.go @@ -931,14 +931,9 @@ func (q *fakeQuerier) GetUsers(_ context.Context, params database.GetUsersParams users := make([]database.User, len(q.users)) copy(users, q.users) - // Database orders by created_at + // Database orders by username slices.SortFunc(users, func(a, b database.User) bool { - if a.CreatedAt.Equal(b.CreatedAt) { - // Technically the postgres database also orders by uuid. So match - // that behavior - return a.ID.String() < b.ID.String() - } - return a.CreatedAt.Before(b.CreatedAt) + return sort.StringsAreSorted([]string{a.Username, b.Username}) }) // Filter out deleted since they should never be returned.. diff --git a/coderd/database/dbfake/databasefake_test.go b/coderd/database/dbfake/databasefake_test.go index f2468d9329bb4..dd1a3ee445ae4 100644 --- a/coderd/database/dbfake/databasefake_test.go +++ b/coderd/database/dbfake/databasefake_test.go @@ -5,6 +5,7 @@ import ( "database/sql" "fmt" "reflect" + "sort" "testing" "time" @@ -106,26 +107,25 @@ func TestExactMethods(t *testing.T) { } } -// TestUserOrder ensures that the fake database returns users in the order they -// were created. +// TestUserOrder ensures that the fake database returns users sorted by username. func TestUserOrder(t *testing.T) { t.Parallel() db := dbfake.New() now := database.Now() - count := 10 - for i := 0; i < count; i++ { - dbgen.User(t, db, database.User{ - Username: fmt.Sprintf("user%d", i), - CreatedAt: now, - }) + + usernames := []string{"b-user", "d-user", "a-user", "c-user", "e-user"} + for _, username := range usernames { + dbgen.User(t, db, database.User{Username: username, CreatedAt: now}) } users, err := db.GetUsers(context.Background(), database.GetUsersParams{}) require.NoError(t, err) - require.Lenf(t, users, count, "expected %d users", count) + require.Lenf(t, users, len(usernames), "expected %d users", len(usernames)) + + sort.Strings(usernames) for i, user := range users { - require.Equal(t, fmt.Sprintf("user%d", i), user.Username) + require.Equal(t, usernames[i], user.Username) } } diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index ba730283e4a9f..65b0b7681696b 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -4817,9 +4817,8 @@ WHERE END -- End of filters ORDER BY - -- Deterministic and consistent ordering of all users, even if they share - -- a timestamp. This is to ensure consistent pagination. - (created_at, id) ASC OFFSET $5 + -- Deterministic and consistent ordering of all users. This is to ensure consistent pagination. + username ASC OFFSET $5 LIMIT -- A null limit means "no limit", so 0 means return all NULLIF($6 :: int, 0) diff --git a/coderd/database/queries/users.sql b/coderd/database/queries/users.sql index aabba8eda96c9..4ba95ec21bcb7 100644 --- a/coderd/database/queries/users.sql +++ b/coderd/database/queries/users.sql @@ -183,9 +183,8 @@ WHERE END -- End of filters ORDER BY - -- Deterministic and consistent ordering of all users, even if they share - -- a timestamp. This is to ensure consistent pagination. - (created_at, id) ASC OFFSET @offset_opt + -- Deterministic and consistent ordering of all users. This is to ensure consistent pagination. + username ASC OFFSET @offset_opt LIMIT -- A null limit means "no limit", so 0 means return all NULLIF(@limit_opt :: int, 0); From b2e45d8b74a02aab54a7ffc46605d68cabc918dd Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Mon, 5 Jun 2023 15:40:41 +0200 Subject: [PATCH 2/4] fix --- coderd/database/dbauthz/querier_test.go | 8 ++++---- coderd/users_test.go | 10 ++-------- 2 files changed, 6 insertions(+), 12 deletions(-) diff --git a/coderd/database/dbauthz/querier_test.go b/coderd/database/dbauthz/querier_test.go index ad85ecadfd1ed..fd158ef49f5fc 100644 --- a/coderd/database/dbauthz/querier_test.go +++ b/coderd/database/dbauthz/querier_test.go @@ -726,14 +726,14 @@ func (s *MethodTestSuite) TestUser() { check.Args(database.GetFilteredUserCountParams{}).Asserts().Returns(int64(1)) })) s.Run("GetUsers", s.Subtest(func(db database.Store, check *expects) { - a := dbgen.User(s.T(), db, database.User{CreatedAt: database.Now().Add(-time.Hour)}) - b := dbgen.User(s.T(), db, database.User{CreatedAt: database.Now()}) + a := dbgen.User(s.T(), db, database.User{Username: "GetUsers-a-user"}) + b := dbgen.User(s.T(), db, database.User{Username: "GetUsers-b-user"}) check.Args(database.GetUsersParams{}). Asserts(a, rbac.ActionRead, b, rbac.ActionRead) })) s.Run("GetUsersWithCount", s.Subtest(func(db database.Store, check *expects) { - a := dbgen.User(s.T(), db, database.User{CreatedAt: database.Now().Add(-time.Hour)}) - b := dbgen.User(s.T(), db, database.User{CreatedAt: database.Now()}) + a := dbgen.User(s.T(), db, database.User{Username: "GetUsersWithCount-a-user"}) + b := dbgen.User(s.T(), db, database.User{Username: "GetUsersWithCount-b-user"}) check.Args(database.GetUsersParams{}).Asserts(a, rbac.ActionRead, b, rbac.ActionRead) })) s.Run("InsertUser", s.Subtest(func(db database.Store, check *expects) { diff --git a/coderd/users_test.go b/coderd/users_test.go index 053a86fd2be17..cea19caddab50 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -1579,10 +1579,7 @@ func TestPaginatedUsers(t *testing.T) { err = eg.Wait() require.NoError(t, err, "create users failed") - // Sorting the users will sort by (created_at, uuid). This is to handle - // the off case that created_at is identical for 2 users. - // This is a really rare case in production, but does happen in unit tests - // due to the fake database being in memory and exceptionally quick. + // Sorting the users will sort by username. sortUsers(allUsers) sortUsers(specialUsers) @@ -1697,10 +1694,7 @@ func assertPagination(ctx context.Context, t *testing.T, client *codersdk.Client // sortUsers sorts by (created_at, id) func sortUsers(users []codersdk.User) { sort.Slice(users, func(i, j int) bool { - if users[i].CreatedAt.Equal(users[j].CreatedAt) { - return users[i].ID.String() < users[j].ID.String() - } - return users[i].CreatedAt.Before(users[j].CreatedAt) + return sort.StringsAreSorted([]string{users[i].Username, users[j].Username}) }) } From 8bf7d54a6a3c8822b9f10c11e064d1b6b52bfba0 Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Mon, 5 Jun 2023 15:56:17 +0200 Subject: [PATCH 3/4] possible fix --- coderd/database/queries.sql.go | 6 +++--- coderd/database/queries/users.sql | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 65b0b7681696b..89ae570cc90ba 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -4777,11 +4777,11 @@ WHERE -- duplicating or missing data. WHEN $1 :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN ( -- The pagination cursor is the last ID of the previous page. - -- The query is ordered by the created_at field, so select all + -- The query is ordered by the username field, so select all -- rows after the cursor. - (created_at, id) > ( + (username) > ( SELECT - created_at, id + username FROM users WHERE diff --git a/coderd/database/queries/users.sql b/coderd/database/queries/users.sql index 4ba95ec21bcb7..80f4a26f9e656 100644 --- a/coderd/database/queries/users.sql +++ b/coderd/database/queries/users.sql @@ -143,11 +143,11 @@ WHERE -- duplicating or missing data. WHEN @after_id :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN ( -- The pagination cursor is the last ID of the previous page. - -- The query is ordered by the created_at field, so select all + -- The query is ordered by the username field, so select all -- rows after the cursor. - (created_at, id) > ( + (username) > ( SELECT - created_at, id + username FROM users WHERE From 97e52c85587baed0d78a17c02e566ec36fff0d96 Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Mon, 5 Jun 2023 17:43:01 +0200 Subject: [PATCH 4/4] Address PR comments --- coderd/database/dbfake/databasefake.go | 2 +- coderd/users_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/coderd/database/dbfake/databasefake.go b/coderd/database/dbfake/databasefake.go index cdcd93e11b7be..1546c8cf03c43 100644 --- a/coderd/database/dbfake/databasefake.go +++ b/coderd/database/dbfake/databasefake.go @@ -933,7 +933,7 @@ func (q *fakeQuerier) GetUsers(_ context.Context, params database.GetUsersParams // Database orders by username slices.SortFunc(users, func(a, b database.User) bool { - return sort.StringsAreSorted([]string{a.Username, b.Username}) + return a.Username < b.Username }) // Filter out deleted since they should never be returned.. diff --git a/coderd/users_test.go b/coderd/users_test.go index cea19caddab50..73e4ea538dd12 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -1694,7 +1694,7 @@ func assertPagination(ctx context.Context, t *testing.T, client *codersdk.Client // sortUsers sorts by (created_at, id) func sortUsers(users []codersdk.User) { sort.Slice(users, func(i, j int) bool { - return sort.StringsAreSorted([]string{users[i].Username, users[j].Username}) + return users[i].Username < users[j].Username }) }