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 {