From 308a88efb17e5756812a992ea17dd2cb346bcb8a Mon Sep 17 00:00:00 2001 From: Claudio Costa Date: Sat, 14 Aug 2021 07:40:16 +0200 Subject: [PATCH] [MM-37898] Exclude bots from initial user store emptiness check (#18139) * Exclude bots from initial user store emptiness check * Add test --- services/users/users.go | 2 +- services/users/users_test.go | 60 ++++++++++++++++++++++ store/opentracinglayer/opentracinglayer.go | 4 +- store/retrylayer/retrylayer.go | 4 +- store/sqlstore/user_store.go | 20 +++++++- store/store.go | 2 +- store/storetest/mocks/UserStore.go | 14 ++--- store/storetest/user_store.go | 35 +++++++++++-- store/timerlayer/timerlayer.go | 4 +- 9 files changed, 123 insertions(+), 22 deletions(-) diff --git a/services/users/users.go b/services/users/users.go index 80a68c5faf..8bc49bb8fd 100644 --- a/services/users/users.go +++ b/services/users/users.go @@ -43,7 +43,7 @@ func (us *UserService) CreateUser(user *model.User, opts UserCreateOptions) (*mo // Below is a special case where the first user in the entire // system is granted the system_admin role - if ok, err := us.store.IsEmpty(); err != nil { + if ok, err := us.store.IsEmpty(true); err != nil { return nil, errors.Wrap(UserStoreIsEmptyError, err.Error()) } else if ok { user.Roles = model.SystemAdminRoleId + " " + model.SystemUserRoleId diff --git a/services/users/users_test.go b/services/users/users_test.go index 06896134f5..8dfd14dc1c 100644 --- a/services/users/users_test.go +++ b/services/users/users_test.go @@ -5,6 +5,10 @@ package users import ( "testing" + + "github.com/mattermost/mattermost-server/v6/model" + + "github.com/stretchr/testify/require" ) func TestIsUsernameTaken(t *testing.T) { @@ -27,3 +31,59 @@ func TestIsUsernameTaken(t *testing.T) { t.FailNow() } } + +func TestFirstUserPromoted(t *testing.T) { + th := Setup(t) + defer th.TearDown() + + user, err := th.service.CreateUser(&model.User{ + Username: model.NewId(), + Password: model.NewId(), + Email: "user@example.com", + }, UserCreateOptions{}) + require.NoError(t, err) + require.NotNil(t, user) + + require.Equal(t, model.SystemAdminRoleId+" "+model.SystemUserRoleId, user.Roles) + + user2, err := th.service.CreateUser(&model.User{ + Username: model.NewId(), + Password: model.NewId(), + Email: "user2@example.com", + }, UserCreateOptions{}) + require.NoError(t, err) + require.NotNil(t, user2) + + require.Equal(t, model.SystemUserRoleId, user2.Roles) + + th.dbStore.User().PermanentDelete(user.Id) + + b := &model.Bot{ + UserId: user2.Id, + OwnerId: model.NewId(), + Username: model.NewId(), + } + + _, err = th.dbStore.Bot().Save(b) + require.NoError(t, err) + + user3, err := th.service.CreateUser(&model.User{ + Username: model.NewId(), + Password: model.NewId(), + Email: "user3@example.com", + }, UserCreateOptions{}) + require.NoError(t, err) + require.NotNil(t, user3) + + require.Equal(t, model.SystemAdminRoleId+" "+model.SystemUserRoleId, user3.Roles) + + user4, err := th.service.CreateUser(&model.User{ + Username: model.NewId(), + Password: model.NewId(), + Email: "user4@example.com", + }, UserCreateOptions{}) + require.NoError(t, err) + require.NotNil(t, user4) + + require.Equal(t, model.SystemUserRoleId, user4.Roles) +} diff --git a/store/opentracinglayer/opentracinglayer.go b/store/opentracinglayer/opentracinglayer.go index 41fc871c43..d915eff7c3 100644 --- a/store/opentracinglayer/opentracinglayer.go +++ b/store/opentracinglayer/opentracinglayer.go @@ -10361,7 +10361,7 @@ func (s *OpenTracingLayerUserStore) InvalidateProfilesInChannelCacheByUser(userI } -func (s *OpenTracingLayerUserStore) IsEmpty() (bool, error) { +func (s *OpenTracingLayerUserStore) IsEmpty(excludeBots bool) (bool, error) { origCtx := s.Root.Store.Context() span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "UserStore.IsEmpty") s.Root.Store.SetContext(newCtx) @@ -10370,7 +10370,7 @@ func (s *OpenTracingLayerUserStore) IsEmpty() (bool, error) { }() defer span.Finish() - result, err := s.UserStore.IsEmpty() + result, err := s.UserStore.IsEmpty(excludeBots) if err != nil { span.LogFields(spanlog.Error(err)) ext.Error.Set(span, true) diff --git a/store/retrylayer/retrylayer.go b/store/retrylayer/retrylayer.go index c4813ac718..fa1d5e5232 100644 --- a/store/retrylayer/retrylayer.go +++ b/store/retrylayer/retrylayer.go @@ -11250,11 +11250,11 @@ func (s *RetryLayerUserStore) InvalidateProfilesInChannelCacheByUser(userID stri } -func (s *RetryLayerUserStore) IsEmpty() (bool, error) { +func (s *RetryLayerUserStore) IsEmpty(excludeBots bool) (bool, error) { tries := 0 for { - result, err := s.UserStore.IsEmpty() + result, err := s.UserStore.IsEmpty(excludeBots) if err == nil { return result, nil } diff --git a/store/sqlstore/user_store.go b/store/sqlstore/user_store.go index e0135be991..e3934754cd 100644 --- a/store/sqlstore/user_store.go +++ b/store/sqlstore/user_store.go @@ -2004,10 +2004,26 @@ func (us SqlUserStore) GetKnownUsers(userId string) ([]string, error) { } // IsEmpty returns whether or not the Users table is empty. -func (us SqlUserStore) IsEmpty() (bool, error) { +func (us SqlUserStore) IsEmpty(excludeBots bool) (bool, error) { var hasRows bool - err := us.GetReplica().SelectOne(&hasRows, `SELECT EXISTS (SELECT 1 FROM Users)`) + + builder := us.getQueryBuilder(). + Select("1"). + Prefix("SELECT EXISTS ("). + From("Users") + + if excludeBots { + builder = builder.LeftJoin("Bots ON Users.Id = Bots.UserId").Where("Bots.UserId IS NULL") + } + + builder = builder.Suffix(")") + + query, args, err := builder.ToSql() if err != nil { + return false, errors.Wrapf(err, "users_is_empty_to_sql") + } + + if err = us.GetReplica().SelectOne(&hasRows, query, args...); err != nil { return false, errors.Wrap(err, "failed to check if table is empty") } return !hasRows, nil diff --git a/store/store.go b/store/store.go index 23e4af63de..b6e6d5992e 100644 --- a/store/store.go +++ b/store/store.go @@ -424,7 +424,7 @@ type UserStore interface { DeactivateGuests() ([]string, error) AutocompleteUsersInChannel(teamID, channelID, term string, options *model.UserSearchOptions) (*model.UserAutocompleteInChannel, error) GetKnownUsers(userID string) ([]string, error) - IsEmpty() (bool, error) + IsEmpty(excludeBots bool) (bool, error) } type BotStore interface { diff --git a/store/storetest/mocks/UserStore.go b/store/storetest/mocks/UserStore.go index ea08c35a0b..e37e80bd0f 100644 --- a/store/storetest/mocks/UserStore.go +++ b/store/storetest/mocks/UserStore.go @@ -1038,20 +1038,20 @@ func (_m *UserStore) InvalidateProfilesInChannelCacheByUser(userID string) { _m.Called(userID) } -// IsEmpty provides a mock function with given fields: -func (_m *UserStore) IsEmpty() (bool, error) { - ret := _m.Called() +// IsEmpty provides a mock function with given fields: excludeBots +func (_m *UserStore) IsEmpty(excludeBots bool) (bool, error) { + ret := _m.Called(excludeBots) var r0 bool - if rf, ok := ret.Get(0).(func() bool); ok { - r0 = rf() + if rf, ok := ret.Get(0).(func(bool) bool); ok { + r0 = rf(excludeBots) } else { r0 = ret.Get(0).(bool) } var r1 error - if rf, ok := ret.Get(1).(func() error); ok { - r1 = rf() + if rf, ok := ret.Get(1).(func(bool) error); ok { + r1 = rf(excludeBots) } else { r1 = ret.Error(1) } diff --git a/store/storetest/user_store.go b/store/storetest/user_store.go index 87185b4145..26d97294cf 100644 --- a/store/storetest/user_store.go +++ b/store/storetest/user_store.go @@ -5714,26 +5714,51 @@ func testGetKnownUsers(t *testing.T, ss store.Store) { } func testIsEmpty(t *testing.T, ss store.Store) { - ok, err := ss.User().IsEmpty() + ok, err := ss.User().IsEmpty(false) require.NoError(t, err) require.True(t, ok) - u := model.User{ + ok, err = ss.User().IsEmpty(true) + require.NoError(t, err) + require.True(t, ok) + + u := &model.User{ Email: MakeEmail(), Username: model.NewId(), } - _, err = ss.User().Save(&u) + u, err = ss.User().Save(u) require.NoError(t, err) - ok, err = ss.User().IsEmpty() + ok, err = ss.User().IsEmpty(false) require.NoError(t, err) require.False(t, ok) + ok, err = ss.User().IsEmpty(true) + require.NoError(t, err) + require.False(t, ok) + + b := &model.Bot{ + UserId: u.Id, + OwnerId: model.NewId(), + Username: model.NewId(), + } + + _, err = ss.Bot().Save(b) + require.NoError(t, err) + + ok, err = ss.User().IsEmpty(false) + require.NoError(t, err) + require.False(t, ok) + + ok, err = ss.User().IsEmpty(true) + require.NoError(t, err) + require.True(t, ok) + err = ss.User().PermanentDelete(u.Id) require.NoError(t, err) - ok, err = ss.User().IsEmpty() + ok, err = ss.User().IsEmpty(false) require.NoError(t, err) require.True(t, ok) } diff --git a/store/timerlayer/timerlayer.go b/store/timerlayer/timerlayer.go index d8b6e63ee5..7543b686f8 100644 --- a/store/timerlayer/timerlayer.go +++ b/store/timerlayer/timerlayer.go @@ -9346,10 +9346,10 @@ func (s *TimerLayerUserStore) InvalidateProfilesInChannelCacheByUser(userID stri } } -func (s *TimerLayerUserStore) IsEmpty() (bool, error) { +func (s *TimerLayerUserStore) IsEmpty(excludeBots bool) (bool, error) { start := timemodule.Now() - result, err := s.UserStore.IsEmpty() + result, err := s.UserStore.IsEmpty(excludeBots) elapsed := float64(timemodule.Since(start)) / float64(timemodule.Second) if s.Root.Metrics != nil {