diff --git a/server/channels/app/custom_profile_attributes.go b/server/channels/app/custom_profile_attributes.go index caf9ee23fb..09c5f7e5d1 100644 --- a/server/channels/app/custom_profile_attributes.go +++ b/server/channels/app/custom_profile_attributes.go @@ -12,7 +12,9 @@ import ( "github.com/pkg/errors" ) -const CustomProfileAttributesFieldLimit = 20 +const ( + CustomProfileAttributesFieldLimit = 20 +) var cpaGroupID string @@ -58,7 +60,6 @@ func (a *App) ListCPAFields() ([]*model.PropertyField, *model.AppError) { opts := model.PropertyFieldSearchOpts{ GroupID: groupID, - Page: 0, PerPage: CustomProfileAttributesFieldLimit, } @@ -76,12 +77,12 @@ func (a *App) CreateCPAField(field *model.PropertyField) (*model.PropertyField, return nil, model.NewAppError("CreateCPAField", "app.custom_profile_attributes.cpa_group_id.app_error", nil, "", http.StatusInternalServerError).Wrap(err) } - existingFields, appErr := a.ListCPAFields() - if appErr != nil { - return nil, appErr + fieldCount, err := a.Srv().propertyService.CountActivePropertyFieldsForGroup(groupID) + if err != nil { + return nil, model.NewAppError("CreateCPAField", "app.custom_profile_attributes.count_property_fields.app_error", nil, "", http.StatusInternalServerError).Wrap(err) } - if len(existingFields) >= CustomProfileAttributesFieldLimit { + if fieldCount >= CustomProfileAttributesFieldLimit { return nil, model.NewAppError("CreateCPAField", "app.custom_profile_attributes.limit_reached.app_error", nil, "", http.StatusUnprocessableEntity).Wrap(err) } @@ -171,19 +172,16 @@ func (a *App) ListCPAValues(userID string) ([]*model.PropertyValue, *model.AppEr return nil, model.NewAppError("GetCPAFields", "app.custom_profile_attributes.cpa_group_id.app_error", nil, "", http.StatusInternalServerError).Wrap(err) } - opts := model.PropertyValueSearchOpts{ - GroupID: groupID, - TargetID: userID, - Page: 0, - PerPage: 999999, - IncludeDeleted: false, - } - fields, err := a.Srv().propertyService.SearchPropertyValues(opts) + values, err := a.Srv().propertyService.SearchPropertyValues(model.PropertyValueSearchOpts{ + GroupID: groupID, + TargetID: userID, + PerPage: CustomProfileAttributesFieldLimit, + }) if err != nil { return nil, model.NewAppError("ListCPAValues", "app.custom_profile_attributes.list_property_values.app_error", nil, "", http.StatusInternalServerError).Wrap(err) } - return fields, nil + return values, nil } func (a *App) GetCPAValue(valueID string) (*model.PropertyValue, *model.AppError) { diff --git a/server/channels/app/custom_profile_attributes_test.go b/server/channels/app/custom_profile_attributes_test.go index e972f270fb..e3c3fb542d 100644 --- a/server/channels/app/custom_profile_attributes_test.go +++ b/server/channels/app/custom_profile_attributes_test.go @@ -517,3 +517,60 @@ func TestPatchCPAValue(t *testing.T) { require.Equal(t, userID, updatedValue.TargetID) }) } + +func TestListCPAValues(t *testing.T) { + os.Setenv("MM_FEATUREFLAGS_CUSTOMPROFILEATTRIBUTES", "true") + defer os.Unsetenv("MM_FEATUREFLAGS_CUSTOMPROFILEATTRIBUTES") + th := Setup(t).InitBasic() + defer th.TearDown() + + cpaGroupID, cErr := th.App.cpaGroupID() + require.NoError(t, cErr) + + userID := model.NewId() + + t.Run("should return empty list when user has no values", func(t *testing.T) { + values, appErr := th.App.ListCPAValues(userID) + require.Nil(t, appErr) + require.Empty(t, values) + }) + + t.Run("should list all values for a user", func(t *testing.T) { + var expectedValues []json.RawMessage + + for i := 1; i <= CustomProfileAttributesFieldLimit; i++ { + field := &model.PropertyField{ + GroupID: cpaGroupID, + Name: fmt.Sprintf("Field %d", i), + Type: model.PropertyFieldTypeText, + } + _, err := th.App.Srv().propertyService.CreatePropertyField(field) + require.NoError(t, err) + + value := &model.PropertyValue{ + TargetID: userID, + TargetType: "user", + GroupID: cpaGroupID, + FieldID: field.ID, + Value: json.RawMessage(fmt.Sprintf(`"Value %d"`, i)), + } + _, err = th.App.Srv().propertyService.CreatePropertyValue(value) + require.NoError(t, err) + expectedValues = append(expectedValues, value.Value) + } + + // List values for original user + values, appErr := th.App.ListCPAValues(userID) + require.Nil(t, appErr) + require.Len(t, values, CustomProfileAttributesFieldLimit) + + actualValues := make([]json.RawMessage, len(values)) + for i, value := range values { + require.Equal(t, userID, value.TargetID) + require.Equal(t, "user", value.TargetType) + require.Equal(t, cpaGroupID, value.GroupID) + actualValues[i] = value.Value + } + require.ElementsMatch(t, expectedValues, actualValues) + }) +} diff --git a/server/channels/app/properties/property_field.go b/server/channels/app/properties/property_field.go index d93b81547e..d3b5968190 100644 --- a/server/channels/app/properties/property_field.go +++ b/server/channels/app/properties/property_field.go @@ -19,6 +19,10 @@ func (ps *PropertyService) GetPropertyFields(ids []string) ([]*model.PropertyFie return ps.fieldStore.GetMany(ids) } +func (ps *PropertyService) CountActivePropertyFieldsForGroup(groupID string) (int64, error) { + return ps.fieldStore.CountForGroup(groupID, false) +} + func (ps *PropertyService) SearchPropertyFields(opts model.PropertyFieldSearchOpts) ([]*model.PropertyField, error) { return ps.fieldStore.SearchPropertyFields(opts) } diff --git a/server/channels/db/migrations/migrations.list b/server/channels/db/migrations/migrations.list index 8bf3e7a518..0888404443 100644 --- a/server/channels/db/migrations/migrations.list +++ b/server/channels/db/migrations/migrations.list @@ -257,6 +257,10 @@ channels/db/migrations/mysql/000129_add_property_system_architecture.down.sql channels/db/migrations/mysql/000129_add_property_system_architecture.up.sql channels/db/migrations/mysql/000130_system_console_stats.down.sql channels/db/migrations/mysql/000130_system_console_stats.up.sql +channels/db/migrations/mysql/000131_create_index_pagination_on_property_values.down.sql +channels/db/migrations/mysql/000131_create_index_pagination_on_property_values.up.sql +channels/db/migrations/mysql/000132_create_index_pagination_on_property_fields.down.sql +channels/db/migrations/mysql/000132_create_index_pagination_on_property_fields.up.sql channels/db/migrations/postgres/000001_create_teams.down.sql channels/db/migrations/postgres/000001_create_teams.up.sql channels/db/migrations/postgres/000002_create_team_members.down.sql @@ -515,3 +519,7 @@ channels/db/migrations/postgres/000129_add_property_system_architecture.down.sql channels/db/migrations/postgres/000129_add_property_system_architecture.up.sql channels/db/migrations/postgres/000130_system_console_stats.down.sql channels/db/migrations/postgres/000130_system_console_stats.up.sql +channels/db/migrations/postgres/000131_create_index_pagination_on_property_values.down.sql +channels/db/migrations/postgres/000131_create_index_pagination_on_property_values.up.sql +channels/db/migrations/postgres/000132_create_index_pagination_on_property_fields.down.sql +channels/db/migrations/postgres/000132_create_index_pagination_on_property_fields.up.sql diff --git a/server/channels/db/migrations/mysql/000131_create_index_pagination_on_property_values.down.sql b/server/channels/db/migrations/mysql/000131_create_index_pagination_on_property_values.down.sql new file mode 100644 index 0000000000..339619c981 --- /dev/null +++ b/server/channels/db/migrations/mysql/000131_create_index_pagination_on_property_values.down.sql @@ -0,0 +1,14 @@ +SET @preparedStatement = (SELECT IF( + ( + SELECT COUNT(*) FROM INFORMATION_SCHEMA.STATISTICS + WHERE table_name = 'PropertyValues' + AND table_schema = DATABASE() + AND index_name = 'idx_propertyvalues_create_at_id' + ) > 0, + 'DROP INDEX idx_propertyvalues_create_at_id ON PropertyValues;', + 'SELECT 1' +)); + +PREPARE removeIndexIfExists FROM @preparedStatement; +EXECUTE removeIndexIfExists; +DEALLOCATE PREPARE removeIndexIfExists; diff --git a/server/channels/db/migrations/mysql/000131_create_index_pagination_on_property_values.up.sql b/server/channels/db/migrations/mysql/000131_create_index_pagination_on_property_values.up.sql new file mode 100644 index 0000000000..ec2c8558bb --- /dev/null +++ b/server/channels/db/migrations/mysql/000131_create_index_pagination_on_property_values.up.sql @@ -0,0 +1,14 @@ +SET @preparedStatement = (SELECT IF( + ( + SELECT COUNT(*) FROM INFORMATION_SCHEMA.STATISTICS + WHERE table_name = 'PropertyValues' + AND table_schema = DATABASE() + AND index_name = 'idx_propertyvalues_create_at_id' + ) > 0, + 'SELECT 1', + 'CREATE INDEX idx_propertyvalues_create_at_id ON PropertyValues(CreateAt, ID);' +)); + +PREPARE createIndexIfNotExists FROM @preparedStatement; +EXECUTE createIndexIfNotExists; +DEALLOCATE PREPARE createIndexIfNotExists; diff --git a/server/channels/db/migrations/mysql/000132_create_index_pagination_on_property_fields.down.sql b/server/channels/db/migrations/mysql/000132_create_index_pagination_on_property_fields.down.sql new file mode 100644 index 0000000000..2197078e7c --- /dev/null +++ b/server/channels/db/migrations/mysql/000132_create_index_pagination_on_property_fields.down.sql @@ -0,0 +1,14 @@ +SET @preparedStatement = (SELECT IF( + ( + SELECT COUNT(*) FROM INFORMATION_SCHEMA.STATISTICS + WHERE table_name = 'PropertyFields' + AND table_schema = DATABASE() + AND index_name = 'idx_propertyfields_create_at_id' + ) > 0, + 'DROP INDEX idx_propertyfields_create_at_id ON PropertyFields;', + 'SELECT 1' +)); + +PREPARE removeIndexIfExists FROM @preparedStatement; +EXECUTE removeIndexIfExists; +DEALLOCATE PREPARE removeIndexIfExists; diff --git a/server/channels/db/migrations/mysql/000132_create_index_pagination_on_property_fields.up.sql b/server/channels/db/migrations/mysql/000132_create_index_pagination_on_property_fields.up.sql new file mode 100644 index 0000000000..3529573346 --- /dev/null +++ b/server/channels/db/migrations/mysql/000132_create_index_pagination_on_property_fields.up.sql @@ -0,0 +1,14 @@ +SET @preparedStatement = (SELECT IF( + ( + SELECT COUNT(*) FROM INFORMATION_SCHEMA.STATISTICS + WHERE table_name = 'PropertyFields' + AND table_schema = DATABASE() + AND index_name = 'idx_propertyfields_create_at_id' + ) > 0, + 'SELECT 1', + 'CREATE INDEX idx_propertyfields_create_at_id ON PropertyFields(CreateAt, ID);' +)); + +PREPARE createIndexIfNotExists FROM @preparedStatement; +EXECUTE createIndexIfNotExists; +DEALLOCATE PREPARE createIndexIfNotExists; diff --git a/server/channels/db/migrations/postgres/000131_create_index_pagination_on_property_values.down.sql b/server/channels/db/migrations/postgres/000131_create_index_pagination_on_property_values.down.sql new file mode 100644 index 0000000000..763fadfb0d --- /dev/null +++ b/server/channels/db/migrations/postgres/000131_create_index_pagination_on_property_values.down.sql @@ -0,0 +1,2 @@ +-- morph:nontransactional +DROP INDEX CONCURRENTLY IF EXISTS idx_propertyvalues_create_at_id; diff --git a/server/channels/db/migrations/postgres/000131_create_index_pagination_on_property_values.up.sql b/server/channels/db/migrations/postgres/000131_create_index_pagination_on_property_values.up.sql new file mode 100644 index 0000000000..4b4a17885e --- /dev/null +++ b/server/channels/db/migrations/postgres/000131_create_index_pagination_on_property_values.up.sql @@ -0,0 +1,2 @@ +-- morph:nontransactional +CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_propertyvalues_create_at_id ON PropertyValues(CreateAt, ID) diff --git a/server/channels/db/migrations/postgres/000132_create_index_pagination_on_property_fields.down.sql b/server/channels/db/migrations/postgres/000132_create_index_pagination_on_property_fields.down.sql new file mode 100644 index 0000000000..397d82a9ad --- /dev/null +++ b/server/channels/db/migrations/postgres/000132_create_index_pagination_on_property_fields.down.sql @@ -0,0 +1,2 @@ +-- morph:nontransactional +DROP INDEX CONCURRENTLY IF EXISTS idx_propertyfields_create_at_id; diff --git a/server/channels/db/migrations/postgres/000132_create_index_pagination_on_property_fields.up.sql b/server/channels/db/migrations/postgres/000132_create_index_pagination_on_property_fields.up.sql new file mode 100644 index 0000000000..7c6801e081 --- /dev/null +++ b/server/channels/db/migrations/postgres/000132_create_index_pagination_on_property_fields.up.sql @@ -0,0 +1,2 @@ +-- morph:nontransactional +CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_propertyfields_create_at_id ON PropertyFields(CreateAt, ID) diff --git a/server/channels/store/retrylayer/retrylayer.go b/server/channels/store/retrylayer/retrylayer.go index a6f738d80b..6353a85c4b 100644 --- a/server/channels/store/retrylayer/retrylayer.go +++ b/server/channels/store/retrylayer/retrylayer.go @@ -8949,6 +8949,27 @@ func (s *RetryLayerProductNoticesStore) View(userID string, notices []string) er } +func (s *RetryLayerPropertyFieldStore) CountForGroup(groupID string, includeDeleted bool) (int64, error) { + + tries := 0 + for { + result, err := s.PropertyFieldStore.CountForGroup(groupID, includeDeleted) + if err == nil { + return result, nil + } + if !isRepeatableError(err) { + return result, err + } + tries++ + if tries >= 3 { + err = errors.Wrap(err, "giving up after 3 consecutive repeatable transaction failures") + return result, err + } + timepkg.Sleep(100 * timepkg.Millisecond) + } + +} + func (s *RetryLayerPropertyFieldStore) Create(field *model.PropertyField) (*model.PropertyField, error) { tries := 0 diff --git a/server/channels/store/sqlstore/property_field_store.go b/server/channels/store/sqlstore/property_field_store.go index 1f10ec8475..d19737b1cf 100644 --- a/server/channels/store/sqlstore/property_field_store.go +++ b/server/channels/store/sqlstore/property_field_store.go @@ -78,9 +78,26 @@ func (s *SqlPropertyFieldStore) GetMany(ids []string) ([]*model.PropertyField, e return fields, nil } +func (s *SqlPropertyFieldStore) CountForGroup(groupID string, includeDeleted bool) (int64, error) { + var count int64 + builder := s.getQueryBuilder(). + Select("COUNT(id)"). + From("PropertyFields"). + Where(sq.Eq{"GroupID": groupID}) + + if !includeDeleted { + builder = builder.Where(sq.Eq{"DeleteAt": 0}) + } + + if err := s.GetReplica().GetBuilder(&count, builder); err != nil { + return int64(0), errors.Wrap(err, "failed to count Sessions") + } + return count, nil +} + func (s *SqlPropertyFieldStore) SearchPropertyFields(opts model.PropertyFieldSearchOpts) ([]*model.PropertyField, error) { - if opts.Page < 0 { - return nil, errors.New("page must be positive integer") + if err := opts.Cursor.IsValid(); err != nil { + return nil, fmt.Errorf("cursor is invalid: %w", err) } if opts.PerPage < 1 { @@ -88,10 +105,19 @@ func (s *SqlPropertyFieldStore) SearchPropertyFields(opts model.PropertyFieldSea } builder := s.tableSelectQuery. - OrderBy("CreateAt ASC"). - Offset(uint64(opts.Page * opts.PerPage)). + OrderBy("CreateAt ASC, Id ASC"). Limit(uint64(opts.PerPage)) + if !opts.Cursor.IsEmpty() { + builder = builder.Where(sq.Or{ + sq.Gt{"CreateAt": opts.Cursor.CreateAt}, + sq.And{ + sq.Eq{"CreateAt": opts.Cursor.CreateAt}, + sq.Gt{"Id": opts.Cursor.PropertyFieldID}, + }, + }) + } + if !opts.IncludeDeleted { builder = builder.Where(sq.Eq{"DeleteAt": 0}) } diff --git a/server/channels/store/sqlstore/property_value_store.go b/server/channels/store/sqlstore/property_value_store.go index d9d12603a3..d0c37ce501 100644 --- a/server/channels/store/sqlstore/property_value_store.go +++ b/server/channels/store/sqlstore/property_value_store.go @@ -83,8 +83,8 @@ func (s *SqlPropertyValueStore) GetMany(ids []string) ([]*model.PropertyValue, e } func (s *SqlPropertyValueStore) SearchPropertyValues(opts model.PropertyValueSearchOpts) ([]*model.PropertyValue, error) { - if opts.Page < 0 { - return nil, errors.New("page must be positive integer") + if err := opts.Cursor.IsValid(); err != nil { + return nil, fmt.Errorf("cursor is invalid: %w", err) } if opts.PerPage < 1 { @@ -92,10 +92,19 @@ func (s *SqlPropertyValueStore) SearchPropertyValues(opts model.PropertyValueSea } builder := s.tableSelectQuery. - OrderBy("CreateAt ASC"). - Offset(uint64(opts.Page * opts.PerPage)). + OrderBy("CreateAt ASC, Id ASC"). Limit(uint64(opts.PerPage)) + if !opts.Cursor.IsEmpty() { + builder = builder.Where(sq.Or{ + sq.Gt{"CreateAt": opts.Cursor.CreateAt}, + sq.And{ + sq.Eq{"CreateAt": opts.Cursor.CreateAt}, + sq.Gt{"Id": opts.Cursor.PropertyValueID}, + }, + }) + } + if !opts.IncludeDeleted { builder = builder.Where(sq.Eq{"DeleteAt": 0}) } diff --git a/server/channels/store/store.go b/server/channels/store/store.go index 88f2d29fe0..5809f74c55 100644 --- a/server/channels/store/store.go +++ b/server/channels/store/store.go @@ -1089,6 +1089,7 @@ type PropertyFieldStore interface { Create(field *model.PropertyField) (*model.PropertyField, error) Get(id string) (*model.PropertyField, error) GetMany(ids []string) ([]*model.PropertyField, error) + CountForGroup(groupID string, includeDeleted bool) (int64, error) SearchPropertyFields(opts model.PropertyFieldSearchOpts) ([]*model.PropertyField, error) Update(fields []*model.PropertyField) ([]*model.PropertyField, error) Delete(id string) error diff --git a/server/channels/store/storetest/mocks/PropertyFieldStore.go b/server/channels/store/storetest/mocks/PropertyFieldStore.go index b32b267da7..a1baac5217 100644 --- a/server/channels/store/storetest/mocks/PropertyFieldStore.go +++ b/server/channels/store/storetest/mocks/PropertyFieldStore.go @@ -14,6 +14,34 @@ type PropertyFieldStore struct { mock.Mock } +// CountForGroup provides a mock function with given fields: groupID, includeDeleted +func (_m *PropertyFieldStore) CountForGroup(groupID string, includeDeleted bool) (int64, error) { + ret := _m.Called(groupID, includeDeleted) + + if len(ret) == 0 { + panic("no return value specified for CountForGroup") + } + + var r0 int64 + var r1 error + if rf, ok := ret.Get(0).(func(string, bool) (int64, error)); ok { + return rf(groupID, includeDeleted) + } + if rf, ok := ret.Get(0).(func(string, bool) int64); ok { + r0 = rf(groupID, includeDeleted) + } else { + r0 = ret.Get(0).(int64) + } + + if rf, ok := ret.Get(1).(func(string, bool) error); ok { + r1 = rf(groupID, includeDeleted) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + // Create provides a mock function with given fields: field func (_m *PropertyFieldStore) Create(field *model.PropertyField) (*model.PropertyField, error) { ret := _m.Called(field) diff --git a/server/channels/store/storetest/property_field_store.go b/server/channels/store/storetest/property_field_store.go index 19fe3d6801..d6f8ce6a40 100644 --- a/server/channels/store/storetest/property_field_store.go +++ b/server/channels/store/storetest/property_field_store.go @@ -5,6 +5,7 @@ package storetest import ( "database/sql" + "fmt" "testing" "time" @@ -21,6 +22,7 @@ func TestPropertyFieldStore(t *testing.T, rctx request.CTX, ss store.Store, s Sq t.Run("UpdatePropertyField", func(t *testing.T) { testUpdatePropertyField(t, rctx, ss) }) t.Run("DeletePropertyField", func(t *testing.T) { testDeletePropertyField(t, rctx, ss) }) t.Run("SearchPropertyFields", func(t *testing.T) { testSearchPropertyFields(t, rctx, ss) }) + t.Run("CountForGroup", func(t *testing.T) { testCountForGroup(t, rctx, ss) }) } func testCreatePropertyField(t *testing.T, _ request.CTX, ss store.Store) { @@ -356,6 +358,97 @@ func testDeletePropertyField(t *testing.T, _ request.CTX, ss store.Store) { }) } +func testCountForGroup(t *testing.T, _ request.CTX, ss store.Store) { + t.Run("should return 0 for group with no properties", func(t *testing.T) { + count, err := ss.PropertyField().CountForGroup(model.NewId(), false) + require.NoError(t, err) + require.Equal(t, int64(0), count) + }) + + t.Run("should return correct count for group with properties", func(t *testing.T) { + groupID := model.NewId() + + // Create 5 property fields + for i := 0; i < 5; i++ { + field := &model.PropertyField{ + GroupID: groupID, + Name: fmt.Sprintf("Field %d", i), + Type: model.PropertyFieldTypeText, + } + _, err := ss.PropertyField().Create(field) + require.NoError(t, err) + } + + count, err := ss.PropertyField().CountForGroup(groupID, false) + require.NoError(t, err) + require.Equal(t, int64(5), count) + }) + + t.Run("should not count deleted properties when includeDeleted is false", func(t *testing.T) { + groupID := model.NewId() + + // Create 5 property fields + for i := 0; i < 5; i++ { + field := &model.PropertyField{ + GroupID: groupID, + Name: fmt.Sprintf("Field %d", i), + Type: model.PropertyFieldTypeText, + } + _, err := ss.PropertyField().Create(field) + require.NoError(t, err) + } + + // Create one more and delete it + deletedField := &model.PropertyField{ + GroupID: groupID, + Name: "To be deleted", + Type: model.PropertyFieldTypeText, + } + _, err := ss.PropertyField().Create(deletedField) + require.NoError(t, err) + + err = ss.PropertyField().Delete(deletedField.ID) + require.NoError(t, err) + + // Count should be 5 since the deleted field shouldn't be counted + count, err := ss.PropertyField().CountForGroup(groupID, false) + require.NoError(t, err) + require.Equal(t, int64(5), count) + }) + + t.Run("should count deleted properties when includeDeleted is true", func(t *testing.T) { + groupID := model.NewId() + + // Create 5 property fields + for i := 0; i < 5; i++ { + field := &model.PropertyField{ + GroupID: groupID, + Name: fmt.Sprintf("Field %d", i), + Type: model.PropertyFieldTypeText, + } + _, err := ss.PropertyField().Create(field) + require.NoError(t, err) + } + + // Create one more and delete it + deletedField := &model.PropertyField{ + GroupID: groupID, + Name: "To be deleted", + Type: model.PropertyFieldTypeText, + } + _, err := ss.PropertyField().Create(deletedField) + require.NoError(t, err) + + err = ss.PropertyField().Delete(deletedField.ID) + require.NoError(t, err) + + // Count should be 6 since we're including deleted fields + count, err := ss.PropertyField().CountForGroup(groupID, true) + require.NoError(t, err) + require.Equal(t, int64(6), count) + }) +} + func testSearchPropertyFields(t *testing.T, _ request.CTX, ss store.Store) { groupID := model.NewId() targetID := model.NewId() @@ -406,18 +499,9 @@ func testSearchPropertyFields(t *testing.T, _ request.CTX, ss store.Store) { expectedError bool expectedIDs []string }{ - { - name: "negative page", - opts: model.PropertyFieldSearchOpts{ - Page: -1, - PerPage: 10, - }, - expectedError: true, - }, { name: "negative per_page", opts: model.PropertyFieldSearchOpts{ - Page: 0, PerPage: -1, }, expectedError: true, @@ -426,7 +510,6 @@ func testSearchPropertyFields(t *testing.T, _ request.CTX, ss store.Store) { name: "filter by group_id", opts: model.PropertyFieldSearchOpts{ GroupID: groupID, - Page: 0, PerPage: 10, }, expectedIDs: []string{field1.ID, field2.ID}, @@ -435,7 +518,6 @@ func testSearchPropertyFields(t *testing.T, _ request.CTX, ss store.Store) { name: "filter by group_id including deleted", opts: model.PropertyFieldSearchOpts{ GroupID: groupID, - Page: 0, PerPage: 10, IncludeDeleted: true, }, @@ -445,7 +527,6 @@ func testSearchPropertyFields(t *testing.T, _ request.CTX, ss store.Store) { name: "filter by target_type", opts: model.PropertyFieldSearchOpts{ TargetType: "test_type", - Page: 0, PerPage: 10, }, expectedIDs: []string{field1.ID, field3.ID}, @@ -454,7 +535,6 @@ func testSearchPropertyFields(t *testing.T, _ request.CTX, ss store.Store) { name: "filter by target_id", opts: model.PropertyFieldSearchOpts{ TargetID: targetID, - Page: 0, PerPage: 10, }, expectedIDs: []string{field1.ID, field2.ID}, @@ -463,7 +543,6 @@ func testSearchPropertyFields(t *testing.T, _ request.CTX, ss store.Store) { name: "pagination page 0", opts: model.PropertyFieldSearchOpts{ GroupID: groupID, - Page: 0, PerPage: 2, IncludeDeleted: true, }, @@ -472,8 +551,11 @@ func testSearchPropertyFields(t *testing.T, _ request.CTX, ss store.Store) { { name: "pagination page 1", opts: model.PropertyFieldSearchOpts{ - GroupID: groupID, - Page: 1, + GroupID: groupID, + Cursor: model.PropertyFieldSearchCursor{ + CreateAt: field2.CreateAt, + PropertyFieldID: field2.ID, + }, PerPage: 2, IncludeDeleted: true, }, @@ -490,7 +572,7 @@ func testSearchPropertyFields(t *testing.T, _ request.CTX, ss store.Store) { } require.NoError(t, err) - var ids = make([]string, len(results)) + ids := make([]string, len(results)) for i, field := range results { ids[i] = field.ID } diff --git a/server/channels/store/storetest/property_value_store.go b/server/channels/store/storetest/property_value_store.go index b360380b62..de72b46675 100644 --- a/server/channels/store/storetest/property_value_store.go +++ b/server/channels/store/storetest/property_value_store.go @@ -567,18 +567,9 @@ func testSearchPropertyValues(t *testing.T, _ request.CTX, ss store.Store) { expectedError bool expectedIDs []string }{ - { - name: "negative page", - opts: model.PropertyValueSearchOpts{ - Page: -1, - PerPage: 10, - }, - expectedError: true, - }, { name: "negative per_page", opts: model.PropertyValueSearchOpts{ - Page: 0, PerPage: -1, }, expectedError: true, @@ -587,7 +578,6 @@ func testSearchPropertyValues(t *testing.T, _ request.CTX, ss store.Store) { name: "filter by group_id", opts: model.PropertyValueSearchOpts{ GroupID: groupID, - Page: 0, PerPage: 10, }, expectedIDs: []string{value1.ID, value2.ID}, @@ -597,7 +587,6 @@ func testSearchPropertyValues(t *testing.T, _ request.CTX, ss store.Store) { opts: model.PropertyValueSearchOpts{ GroupID: groupID, TargetType: "test_type", - Page: 0, PerPage: 10, }, expectedIDs: []string{value1.ID}, @@ -608,7 +597,6 @@ func testSearchPropertyValues(t *testing.T, _ request.CTX, ss store.Store) { GroupID: groupID, TargetType: "test_type", IncludeDeleted: true, - Page: 0, PerPage: 10, }, expectedIDs: []string{value1.ID, value4.ID}, @@ -617,7 +605,6 @@ func testSearchPropertyValues(t *testing.T, _ request.CTX, ss store.Store) { name: "filter by target_id", opts: model.PropertyValueSearchOpts{ TargetID: targetID, - Page: 0, PerPage: 10, }, expectedIDs: []string{value1.ID, value2.ID}, @@ -627,7 +614,6 @@ func testSearchPropertyValues(t *testing.T, _ request.CTX, ss store.Store) { opts: model.PropertyValueSearchOpts{ GroupID: groupID, TargetID: targetID, - Page: 0, PerPage: 10, }, expectedIDs: []string{value1.ID, value2.ID}, @@ -636,7 +622,6 @@ func testSearchPropertyValues(t *testing.T, _ request.CTX, ss store.Store) { name: "filter by field_id", opts: model.PropertyValueSearchOpts{ FieldID: fieldID, - Page: 0, PerPage: 10, }, expectedIDs: []string{value1.ID}, @@ -646,7 +631,6 @@ func testSearchPropertyValues(t *testing.T, _ request.CTX, ss store.Store) { opts: model.PropertyValueSearchOpts{ FieldID: fieldID, IncludeDeleted: true, - Page: 0, PerPage: 10, }, expectedIDs: []string{value1.ID, value4.ID}, @@ -655,7 +639,6 @@ func testSearchPropertyValues(t *testing.T, _ request.CTX, ss store.Store) { name: "pagination page 0", opts: model.PropertyValueSearchOpts{ GroupID: groupID, - Page: 0, PerPage: 1, }, expectedIDs: []string{value1.ID}, @@ -664,7 +647,10 @@ func testSearchPropertyValues(t *testing.T, _ request.CTX, ss store.Store) { name: "pagination page 1", opts: model.PropertyValueSearchOpts{ GroupID: groupID, - Page: 1, + Cursor: model.PropertyValueSearchCursor{ + CreateAt: value1.CreateAt, + PropertyValueID: value1.ID, + }, PerPage: 1, }, expectedIDs: []string{value2.ID}, @@ -680,7 +666,7 @@ func testSearchPropertyValues(t *testing.T, _ request.CTX, ss store.Store) { } require.NoError(t, err) - var ids = make([]string, len(results)) + ids := make([]string, len(results)) for i, value := range results { ids[i] = value.ID } diff --git a/server/channels/store/timerlayer/timerlayer.go b/server/channels/store/timerlayer/timerlayer.go index 0867090d74..d0af751ca0 100644 --- a/server/channels/store/timerlayer/timerlayer.go +++ b/server/channels/store/timerlayer/timerlayer.go @@ -7105,6 +7105,22 @@ func (s *TimerLayerProductNoticesStore) View(userID string, notices []string) er return err } +func (s *TimerLayerPropertyFieldStore) CountForGroup(groupID string, includeDeleted bool) (int64, error) { + start := time.Now() + + result, err := s.PropertyFieldStore.CountForGroup(groupID, includeDeleted) + + elapsed := float64(time.Since(start)) / float64(time.Second) + if s.Root.Metrics != nil { + success := "false" + if err == nil { + success = "true" + } + s.Root.Metrics.ObserveStoreMethodDuration("PropertyFieldStore.CountForGroup", success, elapsed) + } + return result, err +} + func (s *TimerLayerPropertyFieldStore) Create(field *model.PropertyField) (*model.PropertyField, error) { start := time.Now() diff --git a/server/i18n/en.json b/server/i18n/en.json index ec9892aa76..781bae8c76 100644 --- a/server/i18n/en.json +++ b/server/i18n/en.json @@ -5006,6 +5006,10 @@ "id": "app.custom_group.unique_name", "translation": "group name is not unique" }, + { + "id": "app.custom_profile_attributes.count_property_fields.app_error", + "translation": "Unable to count the number of fields for the custom profile attribute group" + }, { "id": "app.custom_profile_attributes.cpa_group_id.app_error", "translation": "Cannot register Custom Profile Attributes property group" diff --git a/server/public/model/property_field.go b/server/public/model/property_field.go index 4a07d65712..531b81ceb8 100644 --- a/server/public/model/property_field.go +++ b/server/public/model/property_field.go @@ -4,6 +4,7 @@ package model import ( + "errors" "net/http" "strings" ) @@ -141,11 +142,35 @@ func (pf *PropertyField) Patch(patch *PropertyFieldPatch) { } } +type PropertyFieldSearchCursor struct { + PropertyFieldID string + CreateAt int64 +} + +func (p PropertyFieldSearchCursor) IsEmpty() bool { + return p.PropertyFieldID == "" && p.CreateAt == 0 +} + +func (p PropertyFieldSearchCursor) IsValid() error { + if p.IsEmpty() { + return nil + } + + if p.CreateAt <= 0 { + return errors.New("create at cannot be negative or zero") + } + + if !IsValidId(p.PropertyFieldID) { + return errors.New("property field id is invalid") + } + return nil +} + type PropertyFieldSearchOpts struct { GroupID string TargetType string TargetID string IncludeDeleted bool - Page int + Cursor PropertyFieldSearchCursor PerPage int } diff --git a/server/public/model/property_field_test.go b/server/public/model/property_field_test.go new file mode 100644 index 0000000000..eac0adc211 --- /dev/null +++ b/server/public/model/property_field_test.go @@ -0,0 +1,218 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package model + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestPropertyField_PreSave(t *testing.T) { + t.Run("sets ID if empty", func(t *testing.T) { + pf := &PropertyField{} + pf.PreSave() + assert.NotEmpty(t, pf.ID) + assert.Len(t, pf.ID, 26) // Length of NewId() + }) + + t.Run("keeps existing ID", func(t *testing.T) { + pf := &PropertyField{ID: "existing_id"} + pf.PreSave() + assert.Equal(t, "existing_id", pf.ID) + }) + + t.Run("sets CreateAt if zero", func(t *testing.T) { + pf := &PropertyField{} + pf.PreSave() + assert.NotZero(t, pf.CreateAt) + }) + + t.Run("sets UpdateAt equal to CreateAt", func(t *testing.T) { + pf := &PropertyField{} + pf.PreSave() + assert.Equal(t, pf.CreateAt, pf.UpdateAt) + }) +} + +func TestPropertyField_IsValid(t *testing.T) { + t.Run("valid field", func(t *testing.T) { + pf := &PropertyField{ + ID: NewId(), + GroupID: NewId(), + Name: "test field", + Type: PropertyFieldTypeText, + CreateAt: GetMillis(), + UpdateAt: GetMillis(), + } + require.NoError(t, pf.IsValid()) + }) + + t.Run("invalid ID", func(t *testing.T) { + pf := &PropertyField{ + ID: "invalid", + GroupID: NewId(), + Name: "test field", + Type: PropertyFieldTypeText, + CreateAt: GetMillis(), + UpdateAt: GetMillis(), + } + require.Error(t, pf.IsValid()) + }) + + t.Run("invalid GroupID", func(t *testing.T) { + pf := &PropertyField{ + ID: NewId(), + GroupID: "invalid", + Name: "test field", + Type: PropertyFieldTypeText, + CreateAt: GetMillis(), + UpdateAt: GetMillis(), + } + require.Error(t, pf.IsValid()) + }) + + t.Run("empty name", func(t *testing.T) { + pf := &PropertyField{ + ID: NewId(), + GroupID: NewId(), + Name: "", + Type: PropertyFieldTypeText, + CreateAt: GetMillis(), + UpdateAt: GetMillis(), + } + require.Error(t, pf.IsValid()) + }) + + t.Run("invalid type", func(t *testing.T) { + pf := &PropertyField{ + ID: NewId(), + GroupID: NewId(), + Name: "test field", + Type: "invalid", + CreateAt: GetMillis(), + UpdateAt: GetMillis(), + } + require.Error(t, pf.IsValid()) + }) + + t.Run("zero CreateAt", func(t *testing.T) { + pf := &PropertyField{ + ID: NewId(), + GroupID: NewId(), + Name: "test field", + Type: PropertyFieldTypeText, + CreateAt: 0, + UpdateAt: GetMillis(), + } + require.Error(t, pf.IsValid()) + }) + + t.Run("zero UpdateAt", func(t *testing.T) { + pf := &PropertyField{ + ID: NewId(), + GroupID: NewId(), + Name: "test field", + Type: PropertyFieldTypeText, + CreateAt: GetMillis(), + UpdateAt: 0, + } + require.Error(t, pf.IsValid()) + }) +} + +func TestPropertyField_SanitizeInput(t *testing.T) { + t.Run("trims spaces from name", func(t *testing.T) { + pf := &PropertyField{Name: " test field "} + pf.SanitizeInput() + assert.Equal(t, "test field", pf.Name) + }) +} + +func TestPropertyField_Patch(t *testing.T) { + t.Run("patches all fields", func(t *testing.T) { + pf := &PropertyField{ + Name: "original name", + Type: PropertyFieldTypeText, + TargetID: "original_target", + TargetType: "original_type", + } + + patch := &PropertyFieldPatch{ + Name: NewPointer("new name"), + Type: NewPointer(PropertyFieldTypeSelect), + TargetID: NewPointer("new_target"), + TargetType: NewPointer("new_type"), + Attrs: &map[string]any{"key": "value"}, + } + + pf.Patch(patch) + + assert.Equal(t, "new name", pf.Name) + assert.Equal(t, PropertyFieldTypeSelect, pf.Type) + assert.Equal(t, "new_target", pf.TargetID) + assert.Equal(t, "new_type", pf.TargetType) + assert.EqualValues(t, StringInterface{"key": "value"}, pf.Attrs) + }) + + t.Run("patches only specified fields", func(t *testing.T) { + pf := &PropertyField{ + Name: "original name", + Type: PropertyFieldTypeText, + TargetID: "original_target", + TargetType: "original_type", + } + + patch := &PropertyFieldPatch{ + Name: NewPointer("new name"), + } + + pf.Patch(patch) + + assert.Equal(t, "new name", pf.Name) + assert.Equal(t, PropertyFieldTypeText, pf.Type) + assert.Equal(t, "original_target", pf.TargetID) + assert.Equal(t, "original_type", pf.TargetType) + }) +} + +func TestPropertyFieldSearchCursor_IsValid(t *testing.T) { + t.Run("empty cursor is valid", func(t *testing.T) { + cursor := PropertyFieldSearchCursor{} + assert.NoError(t, cursor.IsValid()) + }) + + t.Run("valid cursor", func(t *testing.T) { + cursor := PropertyFieldSearchCursor{ + PropertyFieldID: NewId(), + CreateAt: GetMillis(), + } + assert.NoError(t, cursor.IsValid()) + }) + + t.Run("invalid PropertyFieldID", func(t *testing.T) { + cursor := PropertyFieldSearchCursor{ + PropertyFieldID: "invalid", + CreateAt: GetMillis(), + } + assert.Error(t, cursor.IsValid()) + }) + + t.Run("zero CreateAt", func(t *testing.T) { + cursor := PropertyFieldSearchCursor{ + PropertyFieldID: NewId(), + CreateAt: 0, + } + assert.Error(t, cursor.IsValid()) + }) + + t.Run("negative CreateAt", func(t *testing.T) { + cursor := PropertyFieldSearchCursor{ + PropertyFieldID: NewId(), + CreateAt: -1, + } + assert.Error(t, cursor.IsValid()) + }) +} diff --git a/server/public/model/property_value.go b/server/public/model/property_value.go index 17ef1709be..ea2d75a089 100644 --- a/server/public/model/property_value.go +++ b/server/public/model/property_value.go @@ -6,6 +6,8 @@ package model import ( "encoding/json" "net/http" + + "github.com/pkg/errors" ) type PropertyValue struct { @@ -63,12 +65,36 @@ func (pv *PropertyValue) IsValid() error { return nil } +type PropertyValueSearchCursor struct { + PropertyValueID string + CreateAt int64 +} + +func (p PropertyValueSearchCursor) IsEmpty() bool { + return p.PropertyValueID == "" && p.CreateAt == 0 +} + +func (p PropertyValueSearchCursor) IsValid() error { + if p.IsEmpty() { + return nil + } + + if p.CreateAt <= 0 { + return errors.New("create at cannot be negative or zero") + } + + if !IsValidId(p.PropertyValueID) { + return errors.New("property field id is invalid") + } + return nil +} + type PropertyValueSearchOpts struct { GroupID string TargetType string TargetID string FieldID string IncludeDeleted bool - Page int + Cursor PropertyValueSearchCursor PerPage int } diff --git a/server/public/model/property_value_test.go b/server/public/model/property_value_test.go new file mode 100644 index 0000000000..f9e4cc6298 --- /dev/null +++ b/server/public/model/property_value_test.go @@ -0,0 +1,186 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package model + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestPropertyValue_PreSave(t *testing.T) { + t.Run("sets ID if empty", func(t *testing.T) { + pv := &PropertyValue{} + pv.PreSave() + assert.NotEmpty(t, pv.ID) + assert.Len(t, pv.ID, 26) // Length of NewId() + }) + + t.Run("keeps existing ID", func(t *testing.T) { + pv := &PropertyValue{ID: "existing_id"} + pv.PreSave() + assert.Equal(t, "existing_id", pv.ID) + }) + + t.Run("sets CreateAt if zero", func(t *testing.T) { + pv := &PropertyValue{} + pv.PreSave() + assert.NotZero(t, pv.CreateAt) + }) + + t.Run("sets UpdateAt equal to CreateAt", func(t *testing.T) { + pv := &PropertyValue{} + pv.PreSave() + assert.Equal(t, pv.CreateAt, pv.UpdateAt) + }) +} + +func TestPropertyValue_IsValid(t *testing.T) { + t.Run("valid value", func(t *testing.T) { + value := json.RawMessage(`{"test": "value"}`) + pv := &PropertyValue{ + ID: NewId(), + TargetID: NewId(), + TargetType: "test_type", + GroupID: NewId(), + FieldID: NewId(), + Value: value, + CreateAt: GetMillis(), + UpdateAt: GetMillis(), + } + require.NoError(t, pv.IsValid()) + }) + + t.Run("invalid ID", func(t *testing.T) { + pv := &PropertyValue{ + ID: "invalid", + TargetID: NewId(), + TargetType: "test_type", + GroupID: NewId(), + FieldID: NewId(), + CreateAt: GetMillis(), + UpdateAt: GetMillis(), + } + require.Error(t, pv.IsValid()) + }) + + t.Run("invalid TargetID", func(t *testing.T) { + pv := &PropertyValue{ + ID: NewId(), + TargetID: "invalid", + TargetType: "test_type", + GroupID: NewId(), + FieldID: NewId(), + CreateAt: GetMillis(), + UpdateAt: GetMillis(), + } + require.Error(t, pv.IsValid()) + }) + + t.Run("empty TargetType", func(t *testing.T) { + pv := &PropertyValue{ + ID: NewId(), + TargetID: NewId(), + TargetType: "", + GroupID: NewId(), + FieldID: NewId(), + CreateAt: GetMillis(), + UpdateAt: GetMillis(), + } + require.Error(t, pv.IsValid()) + }) + + t.Run("invalid GroupID", func(t *testing.T) { + pv := &PropertyValue{ + ID: NewId(), + TargetID: NewId(), + TargetType: "test_type", + GroupID: "invalid", + FieldID: NewId(), + CreateAt: GetMillis(), + UpdateAt: GetMillis(), + } + require.Error(t, pv.IsValid()) + }) + + t.Run("invalid FieldID", func(t *testing.T) { + pv := &PropertyValue{ + ID: NewId(), + TargetID: NewId(), + TargetType: "test_type", + GroupID: NewId(), + FieldID: "invalid", + CreateAt: GetMillis(), + UpdateAt: GetMillis(), + } + require.Error(t, pv.IsValid()) + }) + + t.Run("zero CreateAt", func(t *testing.T) { + pv := &PropertyValue{ + ID: NewId(), + TargetID: NewId(), + TargetType: "test_type", + GroupID: NewId(), + FieldID: NewId(), + CreateAt: 0, + UpdateAt: GetMillis(), + } + require.Error(t, pv.IsValid()) + }) + + t.Run("zero UpdateAt", func(t *testing.T) { + pv := &PropertyValue{ + ID: NewId(), + TargetID: NewId(), + TargetType: "test_type", + GroupID: NewId(), + FieldID: NewId(), + CreateAt: GetMillis(), + UpdateAt: 0, + } + require.Error(t, pv.IsValid()) + }) +} + +func TestPropertyValueSearchCursor_IsValid(t *testing.T) { + t.Run("empty cursor is valid", func(t *testing.T) { + cursor := PropertyValueSearchCursor{} + assert.NoError(t, cursor.IsValid()) + }) + + t.Run("valid cursor", func(t *testing.T) { + cursor := PropertyValueSearchCursor{ + PropertyValueID: NewId(), + CreateAt: GetMillis(), + } + assert.NoError(t, cursor.IsValid()) + }) + + t.Run("invalid PropertyValueID", func(t *testing.T) { + cursor := PropertyValueSearchCursor{ + PropertyValueID: "invalid", + CreateAt: GetMillis(), + } + assert.Error(t, cursor.IsValid()) + }) + + t.Run("zero CreateAt", func(t *testing.T) { + cursor := PropertyValueSearchCursor{ + PropertyValueID: NewId(), + CreateAt: 0, + } + assert.Error(t, cursor.IsValid()) + }) + + t.Run("negative CreateAt", func(t *testing.T) { + cursor := PropertyValueSearchCursor{ + PropertyValueID: NewId(), + CreateAt: -1, + } + assert.Error(t, cursor.IsValid()) + }) +}