From 3ad240c16766ba68adf2d26c98cb7a09e58ede11 Mon Sep 17 00:00:00 2001 From: Konstantinos Pittas Date: Thu, 12 Jan 2023 13:17:53 +0200 Subject: [PATCH] [MM-46579] Add a limit for preferences (#21780) * add a limit for preferences * add limits to preferences * add config to telemetry * remove unneeded tags from config --- app/preference.go | 3 ++- model/config.go | 5 +++++ services/telemetry/telemetry.go | 1 + store/opentracinglayer/opentracinglayer.go | 4 ++-- store/retrylayer/retrylayer.go | 4 ++-- store/sqlstore/preference_store.go | 17 +++++++++++------ store/store.go | 2 +- store/storetest/mocks/PreferenceStore.go | 14 +++++++------- store/storetest/preference_store.go | 18 +++++++++--------- store/timerlayer/timerlayer.go | 4 ++-- 10 files changed, 42 insertions(+), 30 deletions(-) diff --git a/app/preference.go b/app/preference.go index f995eb9391..6bf536249f 100644 --- a/app/preference.go +++ b/app/preference.go @@ -33,7 +33,8 @@ func (w *preferencesServiceWrapper) DeletePreferencesForUser(userID string, pref } func (a *App) GetPreferencesForUser(userID string) (model.Preferences, *model.AppError) { - preferences, err := a.Srv().Store().Preference().GetAll(userID) + limit := *a.Config().ServiceSettings.ExperimentalMaxUserPreferences + preferences, err := a.Srv().Store().Preference().GetAll(userID, limit) if err != nil { return nil, model.NewAppError("GetPreferencesForUser", "app.preference.get_all.app_error", nil, "", http.StatusBadRequest).Wrap(err) } diff --git a/model/config.go b/model/config.go index ef948375cf..2a8fb848c9 100644 --- a/model/config.go +++ b/model/config.go @@ -385,6 +385,7 @@ type ServiceSettings struct { EnableCustomGroups *bool `access:"site_users_and_teams"` SelfHostedPurchase *bool `access:"write_restrictable,cloud_restrictable"` AllowSyncedDrafts *bool `access:"site_posts"` + ExperimentalMaxUserPreferences *int } func (s *ServiceSettings) SetDefaults(isUpdate bool) { @@ -857,6 +858,10 @@ func (s *ServiceSettings) SetDefaults(isUpdate bool) { if s.SelfHostedPurchase == nil { s.SelfHostedPurchase = NewBool(true) } + + if s.ExperimentalMaxUserPreferences == nil { + s.ExperimentalMaxUserPreferences = NewInt(1000) + } } type ClusterSettings struct { diff --git a/services/telemetry/telemetry.go b/services/telemetry/telemetry.go index 860960c379..fb30db7979 100644 --- a/services/telemetry/telemetry.go +++ b/services/telemetry/telemetry.go @@ -460,6 +460,7 @@ func (ts *TelemetryService) trackConfig() { "post_priority": *cfg.ServiceSettings.PostPriority, "self_hosted_purchase": *cfg.ServiceSettings.SelfHostedPurchase, "allow_synced_drafts": *cfg.ServiceSettings.AllowSyncedDrafts, + "experimental_max_user_preferences": *cfg.ServiceSettings.ExperimentalMaxUserPreferences, }) ts.SendTelemetry(TrackConfigTeam, map[string]any{ diff --git a/store/opentracinglayer/opentracinglayer.go b/store/opentracinglayer/opentracinglayer.go index 129ef5254b..e7e9aa5264 100644 --- a/store/opentracinglayer/opentracinglayer.go +++ b/store/opentracinglayer/opentracinglayer.go @@ -6919,7 +6919,7 @@ func (s *OpenTracingLayerPreferenceStore) Get(userID string, category string, na return result, err } -func (s *OpenTracingLayerPreferenceStore) GetAll(userID string) (model.Preferences, error) { +func (s *OpenTracingLayerPreferenceStore) GetAll(userID string, limit int) (model.Preferences, error) { origCtx := s.Root.Store.Context() span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "PreferenceStore.GetAll") s.Root.Store.SetContext(newCtx) @@ -6928,7 +6928,7 @@ func (s *OpenTracingLayerPreferenceStore) GetAll(userID string) (model.Preferenc }() defer span.Finish() - result, err := s.PreferenceStore.GetAll(userID) + result, err := s.PreferenceStore.GetAll(userID, limit) 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 065a1400a9..cfe39a65d4 100644 --- a/store/retrylayer/retrylayer.go +++ b/store/retrylayer/retrylayer.go @@ -7845,11 +7845,11 @@ func (s *RetryLayerPreferenceStore) Get(userID string, category string, name str } -func (s *RetryLayerPreferenceStore) GetAll(userID string) (model.Preferences, error) { +func (s *RetryLayerPreferenceStore) GetAll(userID string, limit int) (model.Preferences, error) { tries := 0 for { - result, err := s.PreferenceStore.GetAll(userID) + result, err := s.PreferenceStore.GetAll(userID, limit) if err == nil { return result, nil } diff --git a/store/sqlstore/preference_store.go b/store/sqlstore/preference_store.go index 6815e950e8..ed223e7233 100644 --- a/store/sqlstore/preference_store.go +++ b/store/sqlstore/preference_store.go @@ -175,17 +175,22 @@ func (s SqlPreferenceStore) GetCategory(userId string, category string) (model.P } -func (s SqlPreferenceStore) GetAll(userId string) (model.Preferences, error) { - var preferences model.Preferences - query, args, err := s.getQueryBuilder(). +func (s SqlPreferenceStore) GetAll(userId string, limit int) (model.Preferences, error) { + query := s.getQueryBuilder(). Select("*"). From("Preferences"). - Where(sq.Eq{"UserId": userId}). - ToSql() + Where(sq.Eq{"UserId": userId}) + if limit > 0 { + query = query.Limit(uint64(limit)) + } + + queryString, args, err := query.ToSql() if err != nil { return nil, errors.Wrap(err, "could not build sql query to get preference") } - if err = s.GetReplicaX().Select(&preferences, query, args...); err != nil { + + var preferences model.Preferences + if err = s.GetReplicaX().Select(&preferences, queryString, args...); err != nil { return nil, errors.Wrapf(err, "failed to find Preference with userId=%s", userId) } return preferences, nil diff --git a/store/store.go b/store/store.go index 8a1c69d91a..94156513fd 100644 --- a/store/store.go +++ b/store/store.go @@ -641,7 +641,7 @@ type PreferenceStore interface { GetCategory(userID string, category string) (model.Preferences, error) GetCategoryAndName(category string, nane string) (model.Preferences, error) Get(userID string, category string, name string) (*model.Preference, error) - GetAll(userID string) (model.Preferences, error) + GetAll(userID string, limit int) (model.Preferences, error) Delete(userID, category, name string) error DeleteCategory(userID string, category string) error DeleteCategoryAndName(category string, name string) error diff --git a/store/storetest/mocks/PreferenceStore.go b/store/storetest/mocks/PreferenceStore.go index c651e905bc..9c7cbd84c4 100644 --- a/store/storetest/mocks/PreferenceStore.go +++ b/store/storetest/mocks/PreferenceStore.go @@ -121,13 +121,13 @@ func (_m *PreferenceStore) Get(userID string, category string, name string) (*mo return r0, r1 } -// GetAll provides a mock function with given fields: userID -func (_m *PreferenceStore) GetAll(userID string) (model.Preferences, error) { - ret := _m.Called(userID) +// GetAll provides a mock function with given fields: userID, limit +func (_m *PreferenceStore) GetAll(userID string, limit int) (model.Preferences, error) { + ret := _m.Called(userID, limit) var r0 model.Preferences - if rf, ok := ret.Get(0).(func(string) model.Preferences); ok { - r0 = rf(userID) + if rf, ok := ret.Get(0).(func(string, int) model.Preferences); ok { + r0 = rf(userID, limit) } else { if ret.Get(0) != nil { r0 = ret.Get(0).(model.Preferences) @@ -135,8 +135,8 @@ func (_m *PreferenceStore) GetAll(userID string) (model.Preferences, error) { } var r1 error - if rf, ok := ret.Get(1).(func(string) error); ok { - r1 = rf(userID) + if rf, ok := ret.Get(1).(func(string, int) error); ok { + r1 = rf(userID, limit) } else { r1 = ret.Error(1) } diff --git a/store/storetest/preference_store.go b/store/storetest/preference_store.go index 5d645f345a..42a5ae9ed1 100644 --- a/store/storetest/preference_store.go +++ b/store/storetest/preference_store.go @@ -184,7 +184,7 @@ func testPreferenceGetAll(t *testing.T, ss store.Store) { err := ss.Preference().Save(preferences) require.NoError(t, err) - result, err := ss.Preference().GetAll(userId) + result, err := ss.Preference().GetAll(userId, 0) require.NoError(t, err) require.Equal(t, 3, len(result), "got the wrong number of preferences") @@ -243,13 +243,13 @@ func testPreferenceDelete(t *testing.T, ss store.Store) { err := ss.Preference().Save(model.Preferences{preference}) require.NoError(t, err) - preferences, err := ss.Preference().GetAll(preference.UserId) + preferences, err := ss.Preference().GetAll(preference.UserId, 0) require.NoError(t, err) assert.Len(t, preferences, 1, "should've returned 1 preference") err = ss.Preference().Delete(preference.UserId, preference.Category, preference.Name) require.NoError(t, err) - preferences, err = ss.Preference().GetAll(preference.UserId) + preferences, err = ss.Preference().GetAll(preference.UserId, 0) require.NoError(t, err) assert.Empty(t, preferences, "should've returned no preferences") } @@ -275,14 +275,14 @@ func testPreferenceDeleteCategory(t *testing.T, ss store.Store) { err := ss.Preference().Save(model.Preferences{preference1, preference2}) require.NoError(t, err) - preferences, err := ss.Preference().GetAll(userId) + preferences, err := ss.Preference().GetAll(userId, 0) require.NoError(t, err) assert.Len(t, preferences, 2, "should've returned 2 preferences") err = ss.Preference().DeleteCategory(userId, category) require.NoError(t, err) - preferences, err = ss.Preference().GetAll(userId) + preferences, err = ss.Preference().GetAll(userId, 0) require.NoError(t, err) assert.Empty(t, preferences, "should've returned no preferences") } @@ -310,22 +310,22 @@ func testPreferenceDeleteCategoryAndName(t *testing.T, ss store.Store) { err := ss.Preference().Save(model.Preferences{preference1, preference2}) require.NoError(t, err) - preferences, err := ss.Preference().GetAll(userId) + preferences, err := ss.Preference().GetAll(userId, 0) require.NoError(t, err) assert.Len(t, preferences, 1, "should've returned 1 preference") - preferences, err = ss.Preference().GetAll(userId2) + preferences, err = ss.Preference().GetAll(userId2, 0) require.NoError(t, err) assert.Len(t, preferences, 1, "should've returned 1 preference") err = ss.Preference().DeleteCategoryAndName(category, name) require.NoError(t, err) - preferences, err = ss.Preference().GetAll(userId) + preferences, err = ss.Preference().GetAll(userId, 0) require.NoError(t, err) assert.Empty(t, preferences, "should've returned no preference") - preferences, err = ss.Preference().GetAll(userId2) + preferences, err = ss.Preference().GetAll(userId2, 0) require.NoError(t, err) assert.Empty(t, preferences, "should've returned no preference") } diff --git a/store/timerlayer/timerlayer.go b/store/timerlayer/timerlayer.go index 19ccaf656d..2a2ed7eefe 100644 --- a/store/timerlayer/timerlayer.go +++ b/store/timerlayer/timerlayer.go @@ -6256,10 +6256,10 @@ func (s *TimerLayerPreferenceStore) Get(userID string, category string, name str return result, err } -func (s *TimerLayerPreferenceStore) GetAll(userID string) (model.Preferences, error) { +func (s *TimerLayerPreferenceStore) GetAll(userID string, limit int) (model.Preferences, error) { start := time.Now() - result, err := s.PreferenceStore.GetAll(userID) + result, err := s.PreferenceStore.GetAll(userID, limit) elapsed := float64(time.Since(start)) / float64(time.Second) if s.Root.Metrics != nil {