From ca9fd45408edca8a42e741a83a2cbf3eceb1fd79 Mon Sep 17 00:00:00 2001 From: Miguel de la Cruz Date: Thu, 10 Apr 2025 19:22:05 +0200 Subject: [PATCH] Adds a mechanism to delete CPA values for a given user (#30330) * Adds a mechanism to delete CPA values for a given user This requires improving the Property Value service to enable delete all values for a given target, so a new method was created that allows to delete filtering by targetType and targetID (required) and optionally for a specific groupID in case the caller wants to affect all values for a target (useful in case you remove a post for example and want to delete all values pointing to that post regardless of the feature they belong to) or only those that belong to a specific feature. * Fix property value tests * Fix after merge and update method name * Fix linter --------- Co-authored-by: Miguel de la Cruz Co-authored-by: Mattermost Build --- .../channels/app/custom_profile_attributes.go | 18 +++ .../app/custom_profile_attributes_test.go | 71 ++++++++++ .../channels/app/properties/property_value.go | 4 + .../channels/store/retrylayer/retrylayer.go | 21 +++ .../store/sqlstore/property_value_store.go | 23 +++ server/channels/store/store.go | 1 + .../storetest/mocks/PropertyValueStore.go | 18 +++ .../store/storetest/property_value_store.go | 131 ++++++++++++++++++ .../channels/store/timerlayer/timerlayer.go | 16 +++ server/i18n/en.json | 4 + 10 files changed, 307 insertions(+) diff --git a/server/channels/app/custom_profile_attributes.go b/server/channels/app/custom_profile_attributes.go index 656b1eb69d..535d469207 100644 --- a/server/channels/app/custom_profile_attributes.go +++ b/server/channels/app/custom_profile_attributes.go @@ -272,3 +272,21 @@ func (a *App) PatchCPAValues(userID string, fieldValueMap map[string]json.RawMes return updatedValues, nil } + +func (a *App) DeleteCPAValues(userID string) *model.AppError { + groupID, err := a.CpaGroupID() + if err != nil { + return model.NewAppError("DeleteCPAValues", "app.custom_profile_attributes.cpa_group_id.app_error", nil, "", http.StatusInternalServerError).Wrap(err) + } + + if err := a.Srv().propertyService.DeletePropertyValuesForTarget(groupID, "user", userID); err != nil { + return model.NewAppError("DeleteCPAValues", "app.custom_profile_attributes.delete_property_values_for_user.app_error", nil, "", http.StatusInternalServerError).Wrap(err) + } + + message := model.NewWebSocketEvent(model.WebsocketEventCPAValuesUpdated, "", "", "", nil, "") + message.Add("user_id", userID) + message.Add("values", map[string]json.RawMessage{}) + a.Publish(message) + + return nil +} diff --git a/server/channels/app/custom_profile_attributes_test.go b/server/channels/app/custom_profile_attributes_test.go index e28fd39e7a..b930da271d 100644 --- a/server/channels/app/custom_profile_attributes_test.go +++ b/server/channels/app/custom_profile_attributes_test.go @@ -698,3 +698,74 @@ func TestPatchCPAValue(t *testing.T) { }) }) } + +func TestDeleteCPAValues(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() + otherUserID := model.NewId() + + // Create multiple fields and values for the user + var createdFields []*model.PropertyField + for i := 1; i <= 3; i++ { + field, err := model.NewCPAFieldFromPropertyField(&model.PropertyField{ + GroupID: cpaGroupID, + Name: fmt.Sprintf("Field %d", i), + Type: model.PropertyFieldTypeText, + }) + require.NoError(t, err) + createdField, appErr := th.App.CreateCPAField(field) + require.Nil(t, appErr) + createdFields = append(createdFields, createdField) + + // Create a value for this field + value, appErr := th.App.PatchCPAValue(userID, createdField.ID, json.RawMessage(fmt.Sprintf(`"Value %d"`, i))) + require.Nil(t, appErr) + require.NotNil(t, value) + } + + // Verify values exist before deletion + values, appErr := th.App.ListCPAValues(userID) + require.Nil(t, appErr) + require.Len(t, values, 3) + + // Test deleting values for user + t.Run("should delete all values for a user", func(t *testing.T) { + appErr := th.App.DeleteCPAValues(userID) + require.Nil(t, appErr) + + // Verify values are gone + values, appErr := th.App.ListCPAValues(userID) + require.Nil(t, appErr) + require.Empty(t, values) + }) + + t.Run("should handle deleting values for a user with no values", func(t *testing.T) { + appErr := th.App.DeleteCPAValues(otherUserID) + require.Nil(t, appErr) + }) + + t.Run("should not affect values for other users", func(t *testing.T) { + // Create values for another user + for _, field := range createdFields { + value, appErr := th.App.PatchCPAValue(otherUserID, field.ID, json.RawMessage(`"Other user value"`)) + require.Nil(t, appErr) + require.NotNil(t, value) + } + + // Delete values for original user + appErr := th.App.DeleteCPAValues(userID) + require.Nil(t, appErr) + + // Verify other user's values still exist + values, appErr := th.App.ListCPAValues(otherUserID) + require.Nil(t, appErr) + require.Len(t, values, 3) + }) +} diff --git a/server/channels/app/properties/property_value.go b/server/channels/app/properties/property_value.go index 2a8775f572..895be79ae9 100644 --- a/server/channels/app/properties/property_value.go +++ b/server/channels/app/properties/property_value.go @@ -56,3 +56,7 @@ func (ps *PropertyService) UpsertPropertyValues(values []*model.PropertyValue) ( func (ps *PropertyService) DeletePropertyValue(groupID, id string) error { return ps.valueStore.Delete(groupID, id) } + +func (ps *PropertyService) DeletePropertyValuesForTarget(groupID string, targetType string, targetID string) error { + return ps.valueStore.DeleteForTarget(groupID, targetType, targetID) +} diff --git a/server/channels/store/retrylayer/retrylayer.go b/server/channels/store/retrylayer/retrylayer.go index a93fbd8ac1..7af52085bc 100644 --- a/server/channels/store/retrylayer/retrylayer.go +++ b/server/channels/store/retrylayer/retrylayer.go @@ -9337,6 +9337,27 @@ func (s *RetryLayerPropertyValueStore) DeleteForField(id string) error { } +func (s *RetryLayerPropertyValueStore) DeleteForTarget(groupID string, targetType string, targetID string) error { + + tries := 0 + for { + err := s.PropertyValueStore.DeleteForTarget(groupID, targetType, targetID) + if err == nil { + return nil + } + if !isRepeatableError(err) { + return err + } + tries++ + if tries >= 3 { + err = errors.Wrap(err, "giving up after 3 consecutive repeatable transaction failures") + return err + } + timepkg.Sleep(100 * timepkg.Millisecond) + } + +} + func (s *RetryLayerPropertyValueStore) Get(groupID string, id string) (*model.PropertyValue, error) { tries := 0 diff --git a/server/channels/store/sqlstore/property_value_store.go b/server/channels/store/sqlstore/property_value_store.go index 30cc286409..ecd1d436a5 100644 --- a/server/channels/store/sqlstore/property_value_store.go +++ b/server/channels/store/sqlstore/property_value_store.go @@ -339,3 +339,26 @@ func (s *SqlPropertyValueStore) DeleteForField(fieldID string) error { return nil } + +func (s *SqlPropertyValueStore) DeleteForTarget(groupID string, targetType string, targetID string) error { + if targetType == "" || targetID == "" { + return store.NewErrInvalidInput("PropertyValue", "target", "type or id empty") + } + + builder := s.getQueryBuilder(). + Delete("PropertyValues"). + Where(sq.Eq{ + "TargetType": targetType, + "TargetID": targetID, + }) + + if groupID != "" { + builder = builder.Where(sq.Eq{"GroupID": groupID}) + } + + if _, err := s.GetMaster().ExecBuilder(builder); err != nil { + return errors.Wrap(err, "property_value_delete_for_target_exec") + } + + return nil +} diff --git a/server/channels/store/store.go b/server/channels/store/store.go index 0a329febb2..c624face8a 100644 --- a/server/channels/store/store.go +++ b/server/channels/store/store.go @@ -1106,6 +1106,7 @@ type PropertyValueStore interface { Upsert(values []*model.PropertyValue) ([]*model.PropertyValue, error) Delete(groupID string, id string) error DeleteForField(id string) error + DeleteForTarget(groupID string, targetType string, targetID string) error } type AccessControlPolicyStore interface { diff --git a/server/channels/store/storetest/mocks/PropertyValueStore.go b/server/channels/store/storetest/mocks/PropertyValueStore.go index 4fae9dcd65..1157073db2 100644 --- a/server/channels/store/storetest/mocks/PropertyValueStore.go +++ b/server/channels/store/storetest/mocks/PropertyValueStore.go @@ -80,6 +80,24 @@ func (_m *PropertyValueStore) DeleteForField(id string) error { return r0 } +// DeleteForTarget provides a mock function with given fields: groupID, targetType, targetID +func (_m *PropertyValueStore) DeleteForTarget(groupID string, targetType string, targetID string) error { + ret := _m.Called(groupID, targetType, targetID) + + if len(ret) == 0 { + panic("no return value specified for DeleteForTarget") + } + + var r0 error + if rf, ok := ret.Get(0).(func(string, string, string) error); ok { + r0 = rf(groupID, targetType, targetID) + } else { + r0 = ret.Error(0) + } + + return r0 +} + // 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) diff --git a/server/channels/store/storetest/property_value_store.go b/server/channels/store/storetest/property_value_store.go index 8c3fa64530..54860258e9 100644 --- a/server/channels/store/storetest/property_value_store.go +++ b/server/channels/store/storetest/property_value_store.go @@ -26,6 +26,7 @@ func TestPropertyValueStore(t *testing.T, rctx request.CTX, ss store.Store, s Sq t.Run("DeletePropertyValue", func(t *testing.T) { testDeletePropertyValue(t, rctx, ss) }) t.Run("SearchPropertyValues", func(t *testing.T) { testSearchPropertyValues(t, rctx, ss) }) t.Run("DeleteForField", func(t *testing.T) { testDeleteForField(t, rctx, ss) }) + t.Run("DeleteForTarget", func(t *testing.T) { testDeleteForTarget(t, rctx, ss) }) } func testCreatePropertyValue(t *testing.T, _ request.CTX, ss store.Store) { @@ -981,3 +982,133 @@ func testDeleteForField(t *testing.T, _ request.CTX, ss store.Store) { require.NoError(t, err) require.Zero(t, nonDeletedValue.DeleteAt) } + +func testDeleteForTarget(t *testing.T, _ request.CTX, ss store.Store) { + groupID1 := model.NewId() + groupID2 := model.NewId() + groupID3 := model.NewId() + targetID1 := model.NewId() + targetID2 := model.NewId() + targetType := "test_type" + + // Create test values for first group and first target + value1 := &model.PropertyValue{ + TargetID: targetID1, + TargetType: targetType, + GroupID: groupID1, + FieldID: model.NewId(), + Value: json.RawMessage(`"value 1"`), + } + + value2 := &model.PropertyValue{ + TargetID: targetID1, + TargetType: targetType, + GroupID: groupID1, + FieldID: model.NewId(), + Value: json.RawMessage(`"value 2"`), + } + + // Create test value for second group but same target + value3 := &model.PropertyValue{ + TargetID: targetID1, + TargetType: targetType, + GroupID: groupID2, + FieldID: model.NewId(), + Value: json.RawMessage(`"value 3"`), + } + + // Create test value for first group but different target + value4 := &model.PropertyValue{ + TargetID: targetID2, + TargetType: targetType, + GroupID: groupID1, + FieldID: model.NewId(), + Value: json.RawMessage(`"value 4"`), + } + + // Create test value with different target type + value5 := &model.PropertyValue{ + TargetID: targetID1, + TargetType: "other_type", + GroupID: groupID1, + FieldID: model.NewId(), + Value: json.RawMessage(`"value 5"`), + } + + // Create test value for third group and first target + value6 := &model.PropertyValue{ + TargetID: targetID1, + TargetType: targetType, + GroupID: groupID3, + FieldID: model.NewId(), + Value: json.RawMessage(`"value 6"`), + } + + for _, value := range []*model.PropertyValue{value1, value2, value3, value4, value5, value6} { + _, err := ss.PropertyValue().Create(value) + require.NoError(t, err) + } + + t.Run("should return error if targetType or targetID is empty", func(t *testing.T) { + err := ss.PropertyValue().DeleteForTarget(groupID1, "", targetID1) + require.Error(t, err) + var eii *store.ErrInvalidInput + require.ErrorAs(t, err, &eii) + + err = ss.PropertyValue().DeleteForTarget(groupID1, targetType, "") + require.Error(t, err) + require.ErrorAs(t, err, &eii) + + // Verify values were not deleted + values, err := ss.PropertyValue().GetMany("", []string{value1.ID, value2.ID}) + require.NoError(t, err) + require.NotZero(t, values) + require.Len(t, values, 2) + }) + + t.Run("should delete only property values for a target within the specified group", func(t *testing.T) { + // Delete values for the first target in first group + err := ss.PropertyValue().DeleteForTarget(groupID1, targetType, targetID1) + require.NoError(t, err) + + // Verify values from first group and first target were hard-deleted + deletedValues, err := ss.PropertyValue().GetMany("", []string{value1.ID, value2.ID}) + require.Error(t, err) + require.Zero(t, deletedValues) + + // Verify value from second group was not deleted + nonDeletedGroupValue, err := ss.PropertyValue().Get("", value3.ID) + require.NoError(t, err) + require.NotNil(t, nonDeletedGroupValue) + + // Verify value from first group but different target was not deleted + nonDeletedTargetValue, err := ss.PropertyValue().Get("", value4.ID) + require.NoError(t, err) + require.NotNil(t, nonDeletedTargetValue) + + // Verify value with different target type was not deleted + nonDeletedTypeValue, err := ss.PropertyValue().Get("", value5.ID) + require.NoError(t, err) + require.NotNil(t, nonDeletedTypeValue) + }) + + t.Run("should delete all values for a target regardless of group", func(t *testing.T) { + err := ss.PropertyValue().DeleteForTarget("", targetType, targetID1) + require.NoError(t, err) + + // Verify values from other groups with targetID1 were deleted + deletedValues, err := ss.PropertyValue().GetMany("", []string{value3.ID, value6.ID}) + require.Error(t, err) + require.Zero(t, deletedValues) + + // Verify value with different target ID was not deleted + nonDeletedValue, err := ss.PropertyValue().Get("", value4.ID) + require.NoError(t, err) + require.NotNil(t, nonDeletedValue) + + // Verify value with different target type was not deleted + nonDeletedTypeValue, err := ss.PropertyValue().Get("", value5.ID) + require.NoError(t, err) + require.NotNil(t, nonDeletedTypeValue) + }) +} diff --git a/server/channels/store/timerlayer/timerlayer.go b/server/channels/store/timerlayer/timerlayer.go index 50cc2e05ec..9b00a476cb 100644 --- a/server/channels/store/timerlayer/timerlayer.go +++ b/server/channels/store/timerlayer/timerlayer.go @@ -7403,6 +7403,22 @@ func (s *TimerLayerPropertyValueStore) DeleteForField(id string) error { return err } +func (s *TimerLayerPropertyValueStore) DeleteForTarget(groupID string, targetType string, targetID string) error { + start := time.Now() + + err := s.PropertyValueStore.DeleteForTarget(groupID, targetType, targetID) + + elapsed := float64(time.Since(start)) / float64(time.Second) + if s.Root.Metrics != nil { + success := "false" + if err == nil { + success = "true" + } + s.Root.Metrics.ObserveStoreMethodDuration("PropertyValueStore.DeleteForTarget", success, elapsed) + } + return err +} + func (s *TimerLayerPropertyValueStore) Get(groupID string, id string) (*model.PropertyValue, error) { start := time.Now() diff --git a/server/i18n/en.json b/server/i18n/en.json index e96c8aebaa..70ddb22ca4 100644 --- a/server/i18n/en.json +++ b/server/i18n/en.json @@ -5038,6 +5038,10 @@ "id": "app.custom_profile_attributes.create_property_field.app_error", "translation": "Unable to create Custom Profile Attribute field" }, + { + "id": "app.custom_profile_attributes.delete_property_values_for_user.app_error", + "translation": "Unable to delete Custom Profile Attribute Values for user" + }, { "id": "app.custom_profile_attributes.get_property_field.app_error", "translation": "Unable to get Custom Profile Attribute field"