From c5e6d9f57003b251afc02a622ad02f8eb00ea1bb Mon Sep 17 00:00:00 2001 From: Miguel de la Cruz Date: Mon, 10 Mar 2025 15:26:47 +0100 Subject: [PATCH] Updates the property service and store method signatures (#30103) * Updates the property service and store method signatures Getters can now receive a `groupID` that narrows down the query if present, so it's not necessary to check for the group ID on the returning values from the outside layers. The Search methods now receive the `groupID` and the `targetID` explicitly as parameters, incentivizing the use of the indexes that the underlying tables have on the searches. * Fix tests --------- Co-authored-by: Miguel de la Cruz Co-authored-by: Mattermost Build --- .../channels/app/custom_profile_attributes.go | 42 ++++--- .../app/custom_profile_attributes_test.go | 18 +-- .../channels/app/properties/property_field.go | 15 ++- .../channels/app/properties/property_value.go | 14 ++- .../channels/store/retrylayer/retrylayer.go | 16 +-- .../store/sqlstore/property_field_store.go | 12 +- .../store/sqlstore/property_value_store.go | 12 +- server/channels/store/store.go | 8 +- .../storetest/mocks/PropertyFieldStore.go | 36 +++--- .../storetest/mocks/PropertyValueStore.go | 36 +++--- .../store/storetest/property_field_store.go | 86 +++++++++----- .../store/storetest/property_value_store.go | 106 ++++++++++++------ .../channels/store/timerlayer/timerlayer.go | 16 +-- 13 files changed, 256 insertions(+), 161 deletions(-) diff --git a/server/channels/app/custom_profile_attributes.go b/server/channels/app/custom_profile_attributes.go index 6b248a71b8..5d14519301 100644 --- a/server/channels/app/custom_profile_attributes.go +++ b/server/channels/app/custom_profile_attributes.go @@ -4,6 +4,7 @@ package app import ( + "database/sql" "encoding/json" "net/http" "sort" @@ -41,13 +42,14 @@ func (a *App) GetCPAField(fieldID string) (*model.PropertyField, *model.AppError return nil, model.NewAppError("GetCPAField", "app.custom_profile_attributes.cpa_group_id.app_error", nil, "", http.StatusInternalServerError).Wrap(err) } - field, err := a.Srv().propertyService.GetPropertyField(fieldID) + field, err := a.Srv().propertyService.GetPropertyField(groupID, fieldID) if err != nil { - return nil, model.NewAppError("GetCPAField", "app.custom_profile_attributes.get_property_field.app_error", nil, "", http.StatusInternalServerError).Wrap(err) - } - - if field.GroupID != groupID { - return nil, model.NewAppError("GetCPAField", "app.custom_profile_attributes.property_field_not_found.app_error", nil, "", http.StatusNotFound) + switch { + case errors.Is(err, sql.ErrNoRows): + return nil, model.NewAppError("GetCPAField", "app.custom_profile_attributes.property_field_not_found.app_error", nil, "", http.StatusNotFound).Wrap(err) + default: + return nil, model.NewAppError("GetCPAField", "app.custom_profile_attributes.get_property_field.app_error", nil, "", http.StatusInternalServerError).Wrap(err) + } } return field, nil @@ -64,7 +66,7 @@ func (a *App) ListCPAFields() ([]*model.PropertyField, *model.AppError) { PerPage: CustomProfileAttributesFieldLimit, } - fields, err := a.Srv().propertyService.SearchPropertyFields(opts) + fields, err := a.Srv().propertyService.SearchPropertyFields(groupID, "", opts) if err != nil { return nil, model.NewAppError("GetCPAFields", "app.custom_profile_attributes.search_property_fields.app_error", nil, "", http.StatusInternalServerError).Wrap(err) } @@ -145,13 +147,13 @@ func (a *App) DeleteCPAField(id string) *model.AppError { return model.NewAppError("DeleteCPAField", "app.custom_profile_attributes.cpa_group_id.app_error", nil, "", http.StatusInternalServerError).Wrap(err) } - existingField, err := a.Srv().propertyService.GetPropertyField(id) - if err != nil { - return model.NewAppError("DeleteCPAField", "app.custom_profile_attributes.get_property_field.app_error", nil, "", http.StatusInternalServerError).Wrap(err) - } - - if existingField.GroupID != groupID { - return model.NewAppError("DeleteCPAField", "app.custom_profile_attributes.property_field_not_found.app_error", nil, "", http.StatusNotFound) + if _, err := a.Srv().propertyService.GetPropertyField(groupID, id); err != nil { + switch { + case errors.Is(err, sql.ErrNoRows): + return model.NewAppError("DeleteCPAField", "app.custom_profile_attributes.property_field_not_found.app_error", nil, "", http.StatusNotFound) + default: + return model.NewAppError("DeleteCPAField", "app.custom_profile_attributes.get_property_field.app_error", nil, "", http.StatusInternalServerError).Wrap(err) + } } if err := a.Srv().propertyService.DeletePropertyField(id); err != nil { @@ -177,10 +179,8 @@ 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) } - values, err := a.Srv().propertyService.SearchPropertyValues(model.PropertyValueSearchOpts{ - GroupID: groupID, - TargetID: userID, - PerPage: CustomProfileAttributesFieldLimit, + values, err := a.Srv().propertyService.SearchPropertyValues(groupID, userID, model.PropertyValueSearchOpts{ + PerPage: CustomProfileAttributesFieldLimit, }) if err != nil { return nil, model.NewAppError("ListCPAValues", "app.custom_profile_attributes.list_property_values.app_error", nil, "", http.StatusInternalServerError).Wrap(err) @@ -195,15 +195,11 @@ func (a *App) GetCPAValue(valueID string) (*model.PropertyValue, *model.AppError return nil, model.NewAppError("GetCPAValue", "app.custom_profile_attributes.cpa_group_id.app_error", nil, "", http.StatusInternalServerError).Wrap(err) } - value, err := a.Srv().propertyService.GetPropertyValue(valueID) + value, err := a.Srv().propertyService.GetPropertyValue(groupID, valueID) if err != nil { return nil, model.NewAppError("GetCPAValue", "app.custom_profile_attributes.get_property_field.app_error", nil, "", http.StatusInternalServerError).Wrap(err) } - if value.GroupID != groupID { - return nil, model.NewAppError("GetCPAValue", "app.custom_profile_attributes.property_field_not_found.app_error", nil, "", http.StatusNotFound) - } - return value, nil } diff --git a/server/channels/app/custom_profile_attributes_test.go b/server/channels/app/custom_profile_attributes_test.go index f943759b56..e589dab1b3 100644 --- a/server/channels/app/custom_profile_attributes_test.go +++ b/server/channels/app/custom_profile_attributes_test.go @@ -25,9 +25,9 @@ func TestGetCPAField(t *testing.T) { require.NoError(t, cErr) t.Run("should fail when getting a non-existent field", func(t *testing.T) { - field, err := th.App.GetCPAField(model.NewId()) - require.NotNil(t, err) - require.Equal(t, "app.custom_profile_attributes.get_property_field.app_error", err.Id) + field, appErr := th.App.GetCPAField(model.NewId()) + require.NotNil(t, appErr) + require.Equal(t, "app.custom_profile_attributes.property_field_not_found.app_error", appErr.Id) require.Empty(t, field) }) @@ -153,7 +153,7 @@ func TestCreateCPAField(t *testing.T) { require.Equal(t, cpaGroupID, createdField.GroupID) require.Equal(t, model.StringInterface{"visibility": "hidden"}, createdField.Attrs) - fetchedField, gErr := th.App.Srv().propertyService.GetPropertyField(createdField.ID) + fetchedField, gErr := th.App.Srv().propertyService.GetPropertyField("", createdField.ID) require.NoError(t, gErr) require.Equal(t, field.Name, fetchedField.Name) require.NotZero(t, fetchedField.CreateAt) @@ -304,7 +304,7 @@ func TestDeleteCPAField(t *testing.T) { t.Run("should fail if the field doesn't exist", func(t *testing.T) { err := th.App.DeleteCPAField(model.NewId()) require.NotNil(t, err) - require.Equal(t, "app.custom_profile_attributes.get_property_field.app_error", err.Id) + require.Equal(t, "app.custom_profile_attributes.property_field_not_found.app_error", err.Id) }) t.Run("should not allow to delete a field outside of CPA", func(t *testing.T) { @@ -324,7 +324,7 @@ func TestDeleteCPAField(t *testing.T) { t.Run("should correctly delete the field", func(t *testing.T) { // check that we have the associated values to the field prior deletion opts := model.PropertyValueSearchOpts{PerPage: 10, FieldID: createdField.ID} - values, err := th.App.Srv().propertyService.SearchPropertyValues(opts) + values, err := th.App.Srv().propertyService.SearchPropertyValues(cpaGroupID, "", opts) require.NoError(t, err) require.Len(t, values, 3) @@ -332,17 +332,17 @@ func TestDeleteCPAField(t *testing.T) { require.Nil(t, th.App.DeleteCPAField(createdField.ID)) // check that it is marked as deleted - fetchedField, err := th.App.Srv().propertyService.GetPropertyField(createdField.ID) + fetchedField, err := th.App.Srv().propertyService.GetPropertyField("", createdField.ID) require.NoError(t, err) require.NotZero(t, fetchedField.DeleteAt) // ensure that the associated fields have been marked as deleted too - values, err = th.App.Srv().propertyService.SearchPropertyValues(opts) + values, err = th.App.Srv().propertyService.SearchPropertyValues(cpaGroupID, "", opts) require.NoError(t, err) require.Len(t, values, 0) opts.IncludeDeleted = true - values, err = th.App.Srv().propertyService.SearchPropertyValues(opts) + values, err = th.App.Srv().propertyService.SearchPropertyValues(cpaGroupID, "", opts) require.NoError(t, err) require.Len(t, values, 3) for _, value := range values { diff --git a/server/channels/app/properties/property_field.go b/server/channels/app/properties/property_field.go index d3b5968190..322f53877b 100644 --- a/server/channels/app/properties/property_field.go +++ b/server/channels/app/properties/property_field.go @@ -11,19 +11,24 @@ func (ps *PropertyService) CreatePropertyField(field *model.PropertyField) (*mod return ps.fieldStore.Create(field) } -func (ps *PropertyService) GetPropertyField(id string) (*model.PropertyField, error) { - return ps.fieldStore.Get(id) +func (ps *PropertyService) GetPropertyField(groupID, id string) (*model.PropertyField, error) { + return ps.fieldStore.Get(groupID, id) } -func (ps *PropertyService) GetPropertyFields(ids []string) ([]*model.PropertyField, error) { - return ps.fieldStore.GetMany(ids) +func (ps *PropertyService) GetPropertyFields(groupID string, ids []string) ([]*model.PropertyField, error) { + return ps.fieldStore.GetMany(groupID, 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) { +func (ps *PropertyService) SearchPropertyFields(groupID, targetID string, opts model.PropertyFieldSearchOpts) ([]*model.PropertyField, error) { + // groupID and targetID are part of the search method signature to + // incentivize the use of the database indexes in searches + opts.GroupID = groupID + opts.TargetID = targetID + return ps.fieldStore.SearchPropertyFields(opts) } diff --git a/server/channels/app/properties/property_value.go b/server/channels/app/properties/property_value.go index dbb79afed9..11b65743d0 100644 --- a/server/channels/app/properties/property_value.go +++ b/server/channels/app/properties/property_value.go @@ -11,15 +11,19 @@ func (ps *PropertyService) CreatePropertyValue(value *model.PropertyValue) (*mod return ps.valueStore.Create(value) } -func (ps *PropertyService) GetPropertyValue(id string) (*model.PropertyValue, error) { - return ps.valueStore.Get(id) +func (ps *PropertyService) GetPropertyValue(groupID, id string) (*model.PropertyValue, error) { + return ps.valueStore.Get(groupID, id) } -func (ps *PropertyService) GetPropertyValues(ids []string) ([]*model.PropertyValue, error) { - return ps.valueStore.GetMany(ids) +func (ps *PropertyService) GetPropertyValues(groupID string, ids []string) ([]*model.PropertyValue, error) { + return ps.valueStore.GetMany(groupID, ids) } -func (ps *PropertyService) SearchPropertyValues(opts model.PropertyValueSearchOpts) ([]*model.PropertyValue, error) { +func (ps *PropertyService) SearchPropertyValues(groupID, targetID string, opts model.PropertyValueSearchOpts) ([]*model.PropertyValue, error) { + // groupID and targetID are part of the search method signature to + // incentivize the use of the database indexes in searches + opts.GroupID = groupID + opts.TargetID = targetID return ps.valueStore.SearchPropertyValues(opts) } diff --git a/server/channels/store/retrylayer/retrylayer.go b/server/channels/store/retrylayer/retrylayer.go index 0fe7c3e269..111509144a 100644 --- a/server/channels/store/retrylayer/retrylayer.go +++ b/server/channels/store/retrylayer/retrylayer.go @@ -9012,11 +9012,11 @@ func (s *RetryLayerPropertyFieldStore) Delete(id string) error { } -func (s *RetryLayerPropertyFieldStore) Get(id string) (*model.PropertyField, error) { +func (s *RetryLayerPropertyFieldStore) Get(groupID string, id string) (*model.PropertyField, error) { tries := 0 for { - result, err := s.PropertyFieldStore.Get(id) + result, err := s.PropertyFieldStore.Get(groupID, id) if err == nil { return result, nil } @@ -9033,11 +9033,11 @@ func (s *RetryLayerPropertyFieldStore) Get(id string) (*model.PropertyField, err } -func (s *RetryLayerPropertyFieldStore) GetMany(ids []string) ([]*model.PropertyField, error) { +func (s *RetryLayerPropertyFieldStore) GetMany(groupID string, ids []string) ([]*model.PropertyField, error) { tries := 0 for { - result, err := s.PropertyFieldStore.GetMany(ids) + result, err := s.PropertyFieldStore.GetMany(groupID, ids) if err == nil { return result, nil } @@ -9201,11 +9201,11 @@ func (s *RetryLayerPropertyValueStore) DeleteForField(id string) error { } -func (s *RetryLayerPropertyValueStore) Get(id string) (*model.PropertyValue, error) { +func (s *RetryLayerPropertyValueStore) Get(groupID string, id string) (*model.PropertyValue, error) { tries := 0 for { - result, err := s.PropertyValueStore.Get(id) + result, err := s.PropertyValueStore.Get(groupID, id) if err == nil { return result, nil } @@ -9222,11 +9222,11 @@ func (s *RetryLayerPropertyValueStore) Get(id string) (*model.PropertyValue, err } -func (s *RetryLayerPropertyValueStore) GetMany(ids []string) ([]*model.PropertyValue, error) { +func (s *RetryLayerPropertyValueStore) GetMany(groupID string, ids []string) ([]*model.PropertyValue, error) { tries := 0 for { - result, err := s.PropertyValueStore.GetMany(ids) + result, err := s.PropertyValueStore.GetMany(groupID, ids) if err == nil { return result, nil } diff --git a/server/channels/store/sqlstore/property_field_store.go b/server/channels/store/sqlstore/property_field_store.go index d19737b1cf..85f07c3ce6 100644 --- a/server/channels/store/sqlstore/property_field_store.go +++ b/server/channels/store/sqlstore/property_field_store.go @@ -52,9 +52,13 @@ func (s *SqlPropertyFieldStore) Create(field *model.PropertyField) (*model.Prope return field, nil } -func (s *SqlPropertyFieldStore) Get(id string) (*model.PropertyField, error) { +func (s *SqlPropertyFieldStore) Get(groupID, id string) (*model.PropertyField, error) { builder := s.tableSelectQuery.Where(sq.Eq{"id": id}) + if groupID != "" { + builder = builder.Where(sq.Eq{"GroupID": groupID}) + } + var field model.PropertyField if err := s.GetReplica().GetBuilder(&field, builder); err != nil { return nil, errors.Wrap(err, "property_field_get_select") @@ -63,9 +67,13 @@ func (s *SqlPropertyFieldStore) Get(id string) (*model.PropertyField, error) { return &field, nil } -func (s *SqlPropertyFieldStore) GetMany(ids []string) ([]*model.PropertyField, error) { +func (s *SqlPropertyFieldStore) GetMany(groupID string, ids []string) ([]*model.PropertyField, error) { builder := s.tableSelectQuery.Where(sq.Eq{"id": ids}) + if groupID != "" { + builder = builder.Where(sq.Eq{"GroupID": groupID}) + } + fields := []*model.PropertyField{} if err := s.GetReplica().SelectBuilder(&fields, builder); err != nil { return nil, errors.Wrap(err, "property_field_get_many_query") diff --git a/server/channels/store/sqlstore/property_value_store.go b/server/channels/store/sqlstore/property_value_store.go index d0c37ce501..a06b2d3547 100644 --- a/server/channels/store/sqlstore/property_value_store.go +++ b/server/channels/store/sqlstore/property_value_store.go @@ -56,9 +56,13 @@ func (s *SqlPropertyValueStore) Create(value *model.PropertyValue) (*model.Prope return value, nil } -func (s *SqlPropertyValueStore) Get(id string) (*model.PropertyValue, error) { +func (s *SqlPropertyValueStore) Get(groupID, id string) (*model.PropertyValue, error) { builder := s.tableSelectQuery.Where(sq.Eq{"id": id}) + if groupID != "" { + builder = builder.Where(sq.Eq{"GroupID": groupID}) + } + var value model.PropertyValue if err := s.GetReplica().GetBuilder(&value, builder); err != nil { return nil, errors.Wrap(err, "property_value_get_select") @@ -67,9 +71,13 @@ func (s *SqlPropertyValueStore) Get(id string) (*model.PropertyValue, error) { return &value, nil } -func (s *SqlPropertyValueStore) GetMany(ids []string) ([]*model.PropertyValue, error) { +func (s *SqlPropertyValueStore) GetMany(groupID string, ids []string) ([]*model.PropertyValue, error) { builder := s.tableSelectQuery.Where(sq.Eq{"id": ids}) + if groupID != "" { + builder = builder.Where(sq.Eq{"GroupID": groupID}) + } + var values []*model.PropertyValue if err := s.GetReplica().SelectBuilder(&values, builder); err != nil { return nil, errors.Wrap(err, "property_value_get_many_query") diff --git a/server/channels/store/store.go b/server/channels/store/store.go index 8121c64138..2d929b65a0 100644 --- a/server/channels/store/store.go +++ b/server/channels/store/store.go @@ -1087,8 +1087,8 @@ type PropertyGroupStore interface { type PropertyFieldStore interface { Create(field *model.PropertyField) (*model.PropertyField, error) - Get(id string) (*model.PropertyField, error) - GetMany(ids []string) ([]*model.PropertyField, error) + Get(groupID, id string) (*model.PropertyField, error) + GetMany(groupID string, 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) @@ -1097,8 +1097,8 @@ type PropertyFieldStore interface { type PropertyValueStore interface { Create(value *model.PropertyValue) (*model.PropertyValue, error) - Get(id string) (*model.PropertyValue, error) - GetMany(ids []string) ([]*model.PropertyValue, error) + Get(groupID, id string) (*model.PropertyValue, error) + GetMany(groupID string, ids []string) ([]*model.PropertyValue, error) SearchPropertyValues(opts model.PropertyValueSearchOpts) ([]*model.PropertyValue, error) Update(values []*model.PropertyValue) ([]*model.PropertyValue, error) Upsert(values []*model.PropertyValue) ([]*model.PropertyValue, error) diff --git a/server/channels/store/storetest/mocks/PropertyFieldStore.go b/server/channels/store/storetest/mocks/PropertyFieldStore.go index a1baac5217..d4139251ad 100644 --- a/server/channels/store/storetest/mocks/PropertyFieldStore.go +++ b/server/channels/store/storetest/mocks/PropertyFieldStore.go @@ -90,9 +90,9 @@ func (_m *PropertyFieldStore) Delete(id string) error { return r0 } -// Get provides a mock function with given fields: id -func (_m *PropertyFieldStore) Get(id string) (*model.PropertyField, error) { - ret := _m.Called(id) +// Get provides a mock function with given fields: groupID, id +func (_m *PropertyFieldStore) Get(groupID string, id string) (*model.PropertyField, error) { + ret := _m.Called(groupID, id) if len(ret) == 0 { panic("no return value specified for Get") @@ -100,19 +100,19 @@ func (_m *PropertyFieldStore) Get(id string) (*model.PropertyField, error) { var r0 *model.PropertyField var r1 error - if rf, ok := ret.Get(0).(func(string) (*model.PropertyField, error)); ok { - return rf(id) + if rf, ok := ret.Get(0).(func(string, string) (*model.PropertyField, error)); ok { + return rf(groupID, id) } - if rf, ok := ret.Get(0).(func(string) *model.PropertyField); ok { - r0 = rf(id) + if rf, ok := ret.Get(0).(func(string, string) *model.PropertyField); ok { + r0 = rf(groupID, id) } else { if ret.Get(0) != nil { r0 = ret.Get(0).(*model.PropertyField) } } - if rf, ok := ret.Get(1).(func(string) error); ok { - r1 = rf(id) + if rf, ok := ret.Get(1).(func(string, string) error); ok { + r1 = rf(groupID, id) } else { r1 = ret.Error(1) } @@ -120,9 +120,9 @@ func (_m *PropertyFieldStore) Get(id string) (*model.PropertyField, error) { return r0, r1 } -// GetMany provides a mock function with given fields: ids -func (_m *PropertyFieldStore) GetMany(ids []string) ([]*model.PropertyField, error) { - ret := _m.Called(ids) +// GetMany provides a mock function with given fields: groupID, ids +func (_m *PropertyFieldStore) GetMany(groupID string, ids []string) ([]*model.PropertyField, error) { + ret := _m.Called(groupID, ids) if len(ret) == 0 { panic("no return value specified for GetMany") @@ -130,19 +130,19 @@ func (_m *PropertyFieldStore) GetMany(ids []string) ([]*model.PropertyField, err var r0 []*model.PropertyField var r1 error - if rf, ok := ret.Get(0).(func([]string) ([]*model.PropertyField, error)); ok { - return rf(ids) + if rf, ok := ret.Get(0).(func(string, []string) ([]*model.PropertyField, error)); ok { + return rf(groupID, ids) } - if rf, ok := ret.Get(0).(func([]string) []*model.PropertyField); ok { - r0 = rf(ids) + if rf, ok := ret.Get(0).(func(string, []string) []*model.PropertyField); ok { + r0 = rf(groupID, ids) } else { if ret.Get(0) != nil { r0 = ret.Get(0).([]*model.PropertyField) } } - if rf, ok := ret.Get(1).(func([]string) error); ok { - r1 = rf(ids) + if rf, ok := ret.Get(1).(func(string, []string) error); ok { + r1 = rf(groupID, ids) } else { r1 = ret.Error(1) } diff --git a/server/channels/store/storetest/mocks/PropertyValueStore.go b/server/channels/store/storetest/mocks/PropertyValueStore.go index 0218927b43..c2296d7183 100644 --- a/server/channels/store/storetest/mocks/PropertyValueStore.go +++ b/server/channels/store/storetest/mocks/PropertyValueStore.go @@ -80,9 +80,9 @@ func (_m *PropertyValueStore) DeleteForField(id string) error { return r0 } -// Get provides a mock function with given fields: id -func (_m *PropertyValueStore) Get(id string) (*model.PropertyValue, error) { - ret := _m.Called(id) +// Get provides a mock function with given fields: groupID, id +func (_m *PropertyValueStore) Get(groupID string, id string) (*model.PropertyValue, error) { + ret := _m.Called(groupID, id) if len(ret) == 0 { panic("no return value specified for Get") @@ -90,19 +90,19 @@ func (_m *PropertyValueStore) Get(id string) (*model.PropertyValue, error) { var r0 *model.PropertyValue var r1 error - if rf, ok := ret.Get(0).(func(string) (*model.PropertyValue, error)); ok { - return rf(id) + if rf, ok := ret.Get(0).(func(string, string) (*model.PropertyValue, error)); ok { + return rf(groupID, id) } - if rf, ok := ret.Get(0).(func(string) *model.PropertyValue); ok { - r0 = rf(id) + if rf, ok := ret.Get(0).(func(string, string) *model.PropertyValue); ok { + r0 = rf(groupID, id) } else { if ret.Get(0) != nil { r0 = ret.Get(0).(*model.PropertyValue) } } - if rf, ok := ret.Get(1).(func(string) error); ok { - r1 = rf(id) + if rf, ok := ret.Get(1).(func(string, string) error); ok { + r1 = rf(groupID, id) } else { r1 = ret.Error(1) } @@ -110,9 +110,9 @@ func (_m *PropertyValueStore) Get(id string) (*model.PropertyValue, error) { return r0, r1 } -// GetMany provides a mock function with given fields: ids -func (_m *PropertyValueStore) GetMany(ids []string) ([]*model.PropertyValue, error) { - ret := _m.Called(ids) +// GetMany provides a mock function with given fields: groupID, ids +func (_m *PropertyValueStore) GetMany(groupID string, ids []string) ([]*model.PropertyValue, error) { + ret := _m.Called(groupID, ids) if len(ret) == 0 { panic("no return value specified for GetMany") @@ -120,19 +120,19 @@ func (_m *PropertyValueStore) GetMany(ids []string) ([]*model.PropertyValue, err var r0 []*model.PropertyValue var r1 error - if rf, ok := ret.Get(0).(func([]string) ([]*model.PropertyValue, error)); ok { - return rf(ids) + if rf, ok := ret.Get(0).(func(string, []string) ([]*model.PropertyValue, error)); ok { + return rf(groupID, ids) } - if rf, ok := ret.Get(0).(func([]string) []*model.PropertyValue); ok { - r0 = rf(ids) + if rf, ok := ret.Get(0).(func(string, []string) []*model.PropertyValue); ok { + r0 = rf(groupID, ids) } else { if ret.Get(0) != nil { r0 = ret.Get(0).([]*model.PropertyValue) } } - if rf, ok := ret.Get(1).(func([]string) error); ok { - r1 = rf(ids) + if rf, ok := ret.Get(1).(func(string, []string) error); ok { + r1 = rf(groupID, ids) } else { r1 = ret.Error(1) } diff --git a/server/channels/store/storetest/property_field_store.go b/server/channels/store/storetest/property_field_store.go index d6f8ce6a40..a37214c8d1 100644 --- a/server/channels/store/storetest/property_field_store.go +++ b/server/channels/store/storetest/property_field_store.go @@ -75,44 +75,59 @@ func testCreatePropertyField(t *testing.T, _ request.CTX, ss store.Store) { func testGetPropertyField(t *testing.T, _ request.CTX, ss store.Store) { t.Run("should fail on nonexisting field", func(t *testing.T) { - field, err := ss.PropertyField().Get(model.NewId()) + field, err := ss.PropertyField().Get("", model.NewId()) require.Zero(t, field) require.ErrorIs(t, err, sql.ErrNoRows) }) - t.Run("should be able to retrieve an existing property field", func(t *testing.T) { - newField := &model.PropertyField{ - GroupID: model.NewId(), - Name: "My new property field", - Type: model.PropertyFieldTypeText, - Attrs: map[string]any{ - "locked": true, - "special": "value", - }, - } - _, err := ss.PropertyField().Create(newField) - require.NoError(t, err) - require.NotZero(t, newField.ID) + groupID := model.NewId() + newField := &model.PropertyField{ + GroupID: groupID, + Name: "My new property field", + Type: model.PropertyFieldTypeText, + Attrs: map[string]any{ + "locked": true, + "special": "value", + }, + } + _, err := ss.PropertyField().Create(newField) + require.NoError(t, err) + require.NotZero(t, newField.ID) - field, err := ss.PropertyField().Get(newField.ID) + t.Run("should be able to retrieve an existing property field", func(t *testing.T) { + field, err := ss.PropertyField().Get(groupID, newField.ID) + require.NoError(t, err) + require.Equal(t, newField.ID, field.ID) + require.True(t, field.Attrs["locked"].(bool)) + require.Equal(t, "value", field.Attrs["special"]) + + // should work without specifying the group ID as well + field, err = ss.PropertyField().Get("", newField.ID) require.NoError(t, err) require.Equal(t, newField.ID, field.ID) require.True(t, field.Attrs["locked"].(bool)) require.Equal(t, "value", field.Attrs["special"]) }) + + t.Run("should not be able to retrieve an existing field when specifying a different group ID", func(t *testing.T) { + field, err := ss.PropertyField().Get(model.NewId(), newField.ID) + require.Zero(t, field) + require.ErrorIs(t, err, sql.ErrNoRows) + }) } func testGetManyPropertyFields(t *testing.T, _ request.CTX, ss store.Store) { t.Run("should fail on nonexisting fields", func(t *testing.T) { - fields, err := ss.PropertyField().GetMany([]string{model.NewId(), model.NewId()}) + fields, err := ss.PropertyField().GetMany("", []string{model.NewId(), model.NewId()}) require.Empty(t, fields) require.ErrorContains(t, err, "missmatch results") }) + groupID := model.NewId() newFields := []*model.PropertyField{} for _, fieldName := range []string{"field1", "field2", "field3"} { newField := &model.PropertyField{ - GroupID: model.NewId(), + GroupID: groupID, Name: fieldName, Type: model.PropertyFieldTypeText, } @@ -123,18 +138,39 @@ func testGetManyPropertyFields(t *testing.T, _ request.CTX, ss store.Store) { newFields = append(newFields, newField) } + newFieldOutsideGroup := &model.PropertyField{ + GroupID: model.NewId(), + Name: "field outside the groupID", + Type: model.PropertyFieldTypeText, + } + _, err := ss.PropertyField().Create(newFieldOutsideGroup) + require.NoError(t, err) + require.NotZero(t, newFieldOutsideGroup.ID) + t.Run("should fail if at least one of the ids is nonexistent", func(t *testing.T) { - fields, err := ss.PropertyField().GetMany([]string{newFields[0].ID, newFields[1].ID, model.NewId()}) + fields, err := ss.PropertyField().GetMany(groupID, []string{newFields[0].ID, newFields[1].ID, model.NewId()}) require.Empty(t, fields) require.ErrorContains(t, err, "missmatch results") }) t.Run("should be able to retrieve existing property fields", func(t *testing.T) { - fields, err := ss.PropertyField().GetMany([]string{newFields[0].ID, newFields[1].ID, newFields[2].ID}) + fields, err := ss.PropertyField().GetMany(groupID, []string{newFields[0].ID, newFields[1].ID, newFields[2].ID}) require.NoError(t, err) require.Len(t, fields, 3) require.ElementsMatch(t, newFields, fields) }) + + t.Run("should fail if asked for valid IDs but outside the group", func(t *testing.T) { + fields, err := ss.PropertyField().GetMany(groupID, []string{newFields[0].ID, newFieldOutsideGroup.ID}) + require.Empty(t, fields) + require.ErrorContains(t, err, "missmatch results") + }) + + t.Run("should be able to retrieve existing property fields from multiple groups", func(t *testing.T) { + fields, err := ss.PropertyField().GetMany("", []string{newFields[0].ID, newFieldOutsideGroup.ID}) + require.NoError(t, err) + require.Len(t, fields, 2) + }) } func testUpdatePropertyField(t *testing.T, _ request.CTX, ss store.Store) { @@ -216,7 +252,7 @@ func testUpdatePropertyField(t *testing.T, _ request.CTX, ss store.Store) { require.NoError(t, err) // Verify first field - updated1, err := ss.PropertyField().Get(field1.ID) + updated1, err := ss.PropertyField().Get("", field1.ID) require.NoError(t, err) require.Equal(t, "Updated first", updated1.Name) require.Equal(t, model.PropertyFieldTypeSelect, updated1.Type) @@ -226,7 +262,7 @@ func testUpdatePropertyField(t *testing.T, _ request.CTX, ss store.Store) { require.Greater(t, updated1.UpdateAt, updated1.CreateAt) // Verify second field - updated2, err := ss.PropertyField().Get(field2.ID) + updated2, err := ss.PropertyField().Get("", field2.ID) require.NoError(t, err) require.Equal(t, "Updated second", updated2.Name) require.Equal(t, model.PropertyFieldTypeSelect, updated2.Type) @@ -271,12 +307,12 @@ func testUpdatePropertyField(t *testing.T, _ request.CTX, ss store.Store) { require.ErrorContains(t, err, "model.property_field.is_valid.app_error") // Check that fields were not updated - updated1, err := ss.PropertyField().Get(field1.ID) + updated1, err := ss.PropertyField().Get(groupID, field1.ID) require.NoError(t, err) require.Equal(t, "Field 1", updated1.Name) require.Equal(t, originalUpdateAt1, updated1.UpdateAt) - updated2, err := ss.PropertyField().Get(field2.ID) + updated2, err := ss.PropertyField().Get(groupID, field2.ID) require.NoError(t, err) require.Equal(t, groupID, updated2.GroupID) require.Equal(t, originalUpdateAt2, updated2.UpdateAt) @@ -316,7 +352,7 @@ func testUpdatePropertyField(t *testing.T, _ request.CTX, ss store.Store) { require.ErrorContains(t, err, "failed to update, some property fields were not found") // Check that the valid field was not updated - updated1, err := ss.PropertyField().Get(field1.ID) + updated1, err := ss.PropertyField().Get("", field1.ID) require.NoError(t, err) require.Equal(t, "First field", updated1.Name) require.Equal(t, originalUpdateAt, updated1.UpdateAt) @@ -345,7 +381,7 @@ func testDeletePropertyField(t *testing.T, _ request.CTX, ss store.Store) { require.NoError(t, err) // Verify the field was soft-deleted - deletedField, err := ss.PropertyField().Get(field.ID) + deletedField, err := ss.PropertyField().Get("", field.ID) require.NoError(t, err) require.NotZero(t, deletedField.DeleteAt) }) diff --git a/server/channels/store/storetest/property_value_store.go b/server/channels/store/storetest/property_value_store.go index cad73212ac..7d458f639d 100644 --- a/server/channels/store/storetest/property_value_store.go +++ b/server/channels/store/storetest/property_value_store.go @@ -76,43 +76,57 @@ func testCreatePropertyValue(t *testing.T, _ request.CTX, ss store.Store) { func testGetPropertyValue(t *testing.T, _ request.CTX, ss store.Store) { t.Run("should fail on nonexisting value", func(t *testing.T) { - value, err := ss.PropertyValue().Get(model.NewId()) + value, err := ss.PropertyValue().Get("", model.NewId()) require.Zero(t, value) require.ErrorIs(t, err, sql.ErrNoRows) }) - t.Run("should be able to retrieve an existing property value", func(t *testing.T) { - newValue := &model.PropertyValue{ - TargetID: model.NewId(), - TargetType: "test_type", - GroupID: model.NewId(), - FieldID: model.NewId(), - Value: json.RawMessage(`"test value"`), - } - _, err := ss.PropertyValue().Create(newValue) - require.NoError(t, err) - require.NotZero(t, newValue.ID) + groupID := model.NewId() + newValue := &model.PropertyValue{ + TargetID: model.NewId(), + TargetType: "test_type", + GroupID: groupID, + FieldID: model.NewId(), + Value: json.RawMessage(`"test value"`), + } + _, err := ss.PropertyValue().Create(newValue) + require.NoError(t, err) + require.NotZero(t, newValue.ID) - value, err := ss.PropertyValue().Get(newValue.ID) + t.Run("should be able to retrieve an existing property value", func(t *testing.T) { + value, err := ss.PropertyValue().Get(groupID, newValue.ID) require.NoError(t, err) require.Equal(t, newValue.ID, value.ID) require.Equal(t, newValue.Value, value.Value) + + // should work without specifying the group ID as well + value, err = ss.PropertyValue().Get("", newValue.ID) + require.NoError(t, err) + require.Equal(t, newValue.ID, value.ID) + require.Equal(t, newValue.Value, value.Value) + }) + + t.Run("should not be able to retrieve an existing value when specifying a different group ID", func(t *testing.T) { + value, err := ss.PropertyValue().Get(model.NewId(), newValue.ID) + require.Zero(t, value) + require.ErrorIs(t, err, sql.ErrNoRows) }) } func testGetManyPropertyValues(t *testing.T, _ request.CTX, ss store.Store) { t.Run("should fail on nonexisting values", func(t *testing.T) { - values, err := ss.PropertyValue().GetMany([]string{model.NewId(), model.NewId()}) + values, err := ss.PropertyValue().GetMany("", []string{model.NewId(), model.NewId()}) require.Empty(t, values) require.ErrorContains(t, err, "missmatch results") }) + groupID := model.NewId() newValues := []*model.PropertyValue{} for i := 0; i < 3; i++ { newValue := &model.PropertyValue{ TargetID: model.NewId(), TargetType: "test_type", - GroupID: model.NewId(), + GroupID: groupID, FieldID: model.NewId(), Value: json.RawMessage(fmt.Sprintf(`"test value %d"`, i)), } @@ -123,18 +137,41 @@ func testGetManyPropertyValues(t *testing.T, _ request.CTX, ss store.Store) { newValues = append(newValues, newValue) } + newValueOutsideGroup := &model.PropertyValue{ + TargetID: model.NewId(), + TargetType: "test_type", + GroupID: model.NewId(), + FieldID: model.NewId(), + Value: json.RawMessage(`"value outside the groupID"`), + } + _, err := ss.PropertyValue().Create(newValueOutsideGroup) + require.NoError(t, err) + require.NotZero(t, newValueOutsideGroup.ID) + t.Run("should fail if at least one of the ids is nonexistent", func(t *testing.T) { - values, err := ss.PropertyValue().GetMany([]string{newValues[0].ID, newValues[1].ID, model.NewId()}) + values, err := ss.PropertyValue().GetMany(groupID, []string{newValues[0].ID, newValues[1].ID, model.NewId()}) require.Empty(t, values) require.ErrorContains(t, err, "missmatch results") }) t.Run("should be able to retrieve existing property values", func(t *testing.T) { - values, err := ss.PropertyValue().GetMany([]string{newValues[0].ID, newValues[1].ID, newValues[2].ID}) + values, err := ss.PropertyValue().GetMany(groupID, []string{newValues[0].ID, newValues[1].ID, newValues[2].ID}) require.NoError(t, err) require.Len(t, values, 3) require.ElementsMatch(t, newValues, values) }) + + t.Run("should fail if asked for valid IDs but outside the group", func(t *testing.T) { + values, err := ss.PropertyValue().GetMany(groupID, []string{newValues[0].ID, newValueOutsideGroup.ID}) + require.Empty(t, values) + require.ErrorContains(t, err, "missmatch results") + }) + + t.Run("should be able to retrieve existing property values from multiple groups", func(t *testing.T) { + fields, err := ss.PropertyValue().GetMany("", []string{newValues[0].ID, newValueOutsideGroup.ID}) + require.NoError(t, err) + require.Len(t, fields, 2) + }) } func testUpdatePropertyValue(t *testing.T, _ request.CTX, ss store.Store) { @@ -208,13 +245,13 @@ func testUpdatePropertyValue(t *testing.T, _ request.CTX, ss store.Store) { require.NoError(t, err) // Verify first value - updated1, err := ss.PropertyValue().Get(value1.ID) + updated1, err := ss.PropertyValue().Get("", value1.ID) require.NoError(t, err) require.Equal(t, json.RawMessage(`"updated value 1"`), updated1.Value) require.Greater(t, updated1.UpdateAt, updated1.CreateAt) // Verify second value - updated2, err := ss.PropertyValue().Get(value2.ID) + updated2, err := ss.PropertyValue().Get("", value2.ID) require.NoError(t, err) require.Equal(t, json.RawMessage(`"updated value 2"`), updated2.Value) require.Greater(t, updated2.UpdateAt, updated2.CreateAt) @@ -256,12 +293,12 @@ func testUpdatePropertyValue(t *testing.T, _ request.CTX, ss store.Store) { require.Contains(t, err.Error(), "model.property_value.is_valid.app_error") // Check that values were not updated - updated1, err := ss.PropertyValue().Get(value1.ID) + updated1, err := ss.PropertyValue().Get("", value1.ID) require.NoError(t, err) require.Equal(t, json.RawMessage(`"Value 1"`), updated1.Value) require.Equal(t, originalUpdateAt1, updated1.UpdateAt) - updated2, err := ss.PropertyValue().Get(value2.ID) + updated2, err := ss.PropertyValue().Get("", value2.ID) require.NoError(t, err) require.Equal(t, groupID, updated2.GroupID) require.Equal(t, originalUpdateAt2, updated2.UpdateAt) @@ -300,7 +337,7 @@ func testUpdatePropertyValue(t *testing.T, _ request.CTX, ss store.Store) { require.ErrorContains(t, err, "failed to update, some property values were not found") // Check that the valid value was not updated - updated1, err := ss.PropertyValue().Get(value1.ID) + updated1, err := ss.PropertyValue().Get("", value1.ID) require.NoError(t, err) require.Equal(t, json.RawMessage(`"Value 1"`), updated1.Value) require.Equal(t, originalUpdateAt, updated1.UpdateAt) @@ -352,7 +389,7 @@ func testUpsertPropertyValue(t *testing.T, _ request.CTX, ss store.Store) { require.NotZero(t, values[0].CreateAt) require.NotZero(t, values[1].CreateAt) - valuesFromStore, err := ss.PropertyValue().GetMany([]string{values[0].ID, values[1].ID}) + valuesFromStore, err := ss.PropertyValue().GetMany("", []string{values[0].ID, values[1].ID}) require.NoError(t, err) require.Len(t, valuesFromStore, 2) }) @@ -383,7 +420,7 @@ func testUpsertPropertyValue(t *testing.T, _ request.CTX, ss store.Store) { require.Greater(t, values[0].UpdateAt, values[0].CreateAt) // Verify in database - updated, err := ss.PropertyValue().Get(valueID) + updated, err := ss.PropertyValue().Get("", valueID) require.NoError(t, err) require.Equal(t, json.RawMessage(`"updated value"`), updated.Value) require.Greater(t, updated.UpdateAt, updated.CreateAt) @@ -417,10 +454,10 @@ func testUpsertPropertyValue(t *testing.T, _ request.CTX, ss store.Store) { require.Len(t, values, 2) // Verify both values - newValueUpserted, err := ss.PropertyValue().Get(newValue.ID) + newValueUpserted, err := ss.PropertyValue().Get("", newValue.ID) require.NoError(t, err) require.Equal(t, json.RawMessage(`"new value"`), newValueUpserted.Value) - existingValueUpserted, err := ss.PropertyValue().Get(existingValue.ID) + existingValueUpserted, err := ss.PropertyValue().Get("", existingValue.ID) require.NoError(t, err) require.Equal(t, json.RawMessage(`"updated existing"`), existingValueUpserted.Value) }) @@ -455,7 +492,7 @@ func testUpsertPropertyValue(t *testing.T, _ request.CTX, ss store.Store) { require.Contains(t, err.Error(), "model.property_value.is_valid.app_error") // Verify the existing value was not changed - retrieved, err := ss.PropertyValue().Get(existingValue.ID) + retrieved, err := ss.PropertyValue().Get("", existingValue.ID) require.NoError(t, err) require.Equal(t, originalValue.Value, retrieved.Value) require.Equal(t, originalValue.UpdateAt, retrieved.UpdateAt) @@ -493,7 +530,7 @@ func testDeletePropertyValue(t *testing.T, _ request.CTX, ss store.Store) { require.NoError(t, err) // Verify the value was soft-deleted - deletedValue, err := ss.PropertyValue().Get(value.ID) + deletedValue, err := ss.PropertyValue().Get("", value.ID) require.NoError(t, err) require.NotZero(t, deletedValue.DeleteAt) }) @@ -716,7 +753,7 @@ func testCreatePropertyValueWithArray(t *testing.T, _ request.CTX, ss store.Stor require.NotZero(t, updated) // Verify updated array values - retrieved, err := ss.PropertyValue().Get(created.ID) + retrieved, err := ss.PropertyValue().Get("", created.ID) require.NoError(t, err) var arrayValues []string require.NoError(t, json.Unmarshal(retrieved.Value, &arrayValues)) @@ -726,12 +763,13 @@ func testCreatePropertyValueWithArray(t *testing.T, _ request.CTX, ss store.Stor func testDeleteForField(t *testing.T, _ request.CTX, ss store.Store) { fieldID := model.NewId() + groupID := model.NewId() // Create test values value1 := &model.PropertyValue{ TargetID: model.NewId(), TargetType: "test_type", - GroupID: model.NewId(), + GroupID: groupID, FieldID: fieldID, Value: json.RawMessage(`"value 1"`), } @@ -739,7 +777,7 @@ func testDeleteForField(t *testing.T, _ request.CTX, ss store.Store) { value2 := &model.PropertyValue{ TargetID: model.NewId(), TargetType: "test_type", - GroupID: model.NewId(), + GroupID: groupID, FieldID: fieldID, Value: json.RawMessage(`"value 2"`), } @@ -747,7 +785,7 @@ func testDeleteForField(t *testing.T, _ request.CTX, ss store.Store) { value3 := &model.PropertyValue{ TargetID: model.NewId(), TargetType: "test_type", - GroupID: model.NewId(), + GroupID: groupID, FieldID: model.NewId(), // Different field ID Value: json.RawMessage(`"value 3"`), } @@ -762,14 +800,14 @@ func testDeleteForField(t *testing.T, _ request.CTX, ss store.Store) { require.NoError(t, err) // Verify values were soft-deleted - deletedValues, err := ss.PropertyValue().GetMany([]string{value1.ID, value2.ID}) + deletedValues, err := ss.PropertyValue().GetMany(groupID, []string{value1.ID, value2.ID}) require.NoError(t, err) require.Len(t, deletedValues, 2) require.NotZero(t, deletedValues[0].DeleteAt) require.NotZero(t, deletedValues[1].DeleteAt) // Verify value with different field ID was not deleted - nonDeletedValue, err := ss.PropertyValue().Get(value3.ID) + nonDeletedValue, err := ss.PropertyValue().Get(groupID, value3.ID) require.NoError(t, err) require.Zero(t, nonDeletedValue.DeleteAt) } diff --git a/server/channels/store/timerlayer/timerlayer.go b/server/channels/store/timerlayer/timerlayer.go index c803ec4fb6..bdd0944462 100644 --- a/server/channels/store/timerlayer/timerlayer.go +++ b/server/channels/store/timerlayer/timerlayer.go @@ -7153,10 +7153,10 @@ func (s *TimerLayerPropertyFieldStore) Delete(id string) error { return err } -func (s *TimerLayerPropertyFieldStore) Get(id string) (*model.PropertyField, error) { +func (s *TimerLayerPropertyFieldStore) Get(groupID string, id string) (*model.PropertyField, error) { start := time.Now() - result, err := s.PropertyFieldStore.Get(id) + result, err := s.PropertyFieldStore.Get(groupID, id) elapsed := float64(time.Since(start)) / float64(time.Second) if s.Root.Metrics != nil { @@ -7169,10 +7169,10 @@ func (s *TimerLayerPropertyFieldStore) Get(id string) (*model.PropertyField, err return result, err } -func (s *TimerLayerPropertyFieldStore) GetMany(ids []string) ([]*model.PropertyField, error) { +func (s *TimerLayerPropertyFieldStore) GetMany(groupID string, ids []string) ([]*model.PropertyField, error) { start := time.Now() - result, err := s.PropertyFieldStore.GetMany(ids) + result, err := s.PropertyFieldStore.GetMany(groupID, ids) elapsed := float64(time.Since(start)) / float64(time.Second) if s.Root.Metrics != nil { @@ -7297,10 +7297,10 @@ func (s *TimerLayerPropertyValueStore) DeleteForField(id string) error { return err } -func (s *TimerLayerPropertyValueStore) Get(id string) (*model.PropertyValue, error) { +func (s *TimerLayerPropertyValueStore) Get(groupID string, id string) (*model.PropertyValue, error) { start := time.Now() - result, err := s.PropertyValueStore.Get(id) + result, err := s.PropertyValueStore.Get(groupID, id) elapsed := float64(time.Since(start)) / float64(time.Second) if s.Root.Metrics != nil { @@ -7313,10 +7313,10 @@ func (s *TimerLayerPropertyValueStore) Get(id string) (*model.PropertyValue, err return result, err } -func (s *TimerLayerPropertyValueStore) GetMany(ids []string) ([]*model.PropertyValue, error) { +func (s *TimerLayerPropertyValueStore) GetMany(groupID string, ids []string) ([]*model.PropertyValue, error) { start := time.Now() - result, err := s.PropertyValueStore.GetMany(ids) + result, err := s.PropertyValueStore.GetMany(groupID, ids) elapsed := float64(time.Since(start)) / float64(time.Second) if s.Root.Metrics != nil {