MM-14675: Eliminates implicit GroupChannel/GroupTeam associations. (#13199)

* MM-14675: Upserts GroupTeam when GroupChannel is linked.
MM-14675: Delete all associated GroupChannels when deleting a GroupTeam.
MM-14675: Using replica DB where possible.
MM-14675: Updates create method used in tests.

* MM-14675: Removes unnecessary DB retrieval of GroupSyncable record.

api4/channel_test.go

@@ -2269,7 +2269,7 @@ func TestAddChannelMember(t *testing.T) {
 	CheckErrorMessage(t, resp, "api.channel.add_members.user_denied")
 
 	// Associate group to team
-	_, appErr = th.App.CreateGroupSyncable(&model.GroupSyncable{
+	_, appErr = th.App.UpsertGroupSyncable(&model.GroupSyncable{
 		GroupId:    th.Group.Id,
 		SyncableId: privateChannel.Id,
 		Type:       model.GroupSyncableTypeChannel,

api4/group.go

@@ -4,7 +4,6 @@
 package api4
 
 import (
-	"database/sql"
 	"encoding/json"
 	"fmt"
 	"io/ioutil"
@@ -181,34 +180,18 @@ func linkGroupSyncable(c *Context, w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	groupSyncable, appErr := c.App.GetGroupSyncable(c.Params.GroupId, syncableID, syncableType)
-	if appErr != nil && appErr.DetailedError != sql.ErrNoRows.Error() {
+	groupSyncable := &model.GroupSyncable{
+		GroupId:    c.Params.GroupId,
+		SyncableId: syncableID,
+		Type:       syncableType,
+	}
+	groupSyncable.Patch(patch)
+	groupSyncable, appErr = c.App.UpsertGroupSyncable(groupSyncable)
+	if appErr != nil {
 		c.Err = appErr
 		return
 	}
 
-	if groupSyncable == nil {
-		groupSyncable = &model.GroupSyncable{
-			GroupId:    c.Params.GroupId,
-			SyncableId: syncableID,
-			Type:       syncableType,
-		}
-		groupSyncable.Patch(patch)
-		groupSyncable, appErr = c.App.CreateGroupSyncable(groupSyncable)
-		if appErr != nil {
-			c.Err = appErr
-			return
-		}
-	} else {
-		groupSyncable.DeleteAt = 0
-		groupSyncable.Patch(patch)
-		groupSyncable, appErr = c.App.UpdateGroupSyncable(groupSyncable)
-		if appErr != nil {
-			c.Err = appErr
-			return
-		}
-	}
-
 	w.WriteHeader(http.StatusCreated)
 
 	b, marshalErr := json.Marshal(groupSyncable)

api4/group_test.go

@@ -645,7 +645,7 @@ func TestGetGroupsByChannel(t *testing.T) {
 	})
 	assert.Nil(t, err)
 
-	_, err = th.App.CreateGroupSyncable(&model.GroupSyncable{
+	_, err = th.App.UpsertGroupSyncable(&model.GroupSyncable{
 		AutoAdd:    true,
 		SyncableId: th.BasicChannel.Id,
 		Type:       model.GroupSyncableTypeChannel,
@@ -698,7 +698,7 @@ func TestGetGroupsByTeam(t *testing.T) {
 	})
 	assert.Nil(t, err)
 
-	_, err = th.App.CreateGroupSyncable(&model.GroupSyncable{
+	_, err = th.App.UpsertGroupSyncable(&model.GroupSyncable{
 		AutoAdd:    true,
 		SyncableId: th.BasicTeam.Id,
 		Type:       model.GroupSyncableTypeTeam,

api4/team_test.go

@@ -1546,7 +1546,7 @@ func TestAddTeamMember(t *testing.T) {
 	CheckErrorMessage(t, resp, "api.team.add_members.user_denied")
 
 	// Associate group to team
-	_, err = th.App.CreateGroupSyncable(&model.GroupSyncable{
+	_, err = th.App.UpsertGroupSyncable(&model.GroupSyncable{
 		GroupId:    th.Group.Id,
 		SyncableId: team.Id,
 		Type:       model.GroupSyncableTypeTeam,
@@ -1754,7 +1754,7 @@ func TestAddTeamMembers(t *testing.T) {
 	CheckErrorMessage(t, resp, "api.team.add_members.user_denied")
 
 	// Associate group to team
-	_, err = th.App.CreateGroupSyncable(&model.GroupSyncable{
+	_, err = th.App.UpsertGroupSyncable(&model.GroupSyncable{
 		GroupId:    th.Group.Id,
 		SyncableId: team.Id,
 		Type:       model.GroupSyncableTypeTeam,

app/group.go

@@ -64,8 +64,42 @@ func (a *App) DeleteGroupMember(groupID string, userID string) (*model.GroupMemb
 	return a.Srv.Store.Group().DeleteMember(groupID, userID)
 }
 
-func (a *App) CreateGroupSyncable(groupSyncable *model.GroupSyncable) (*model.GroupSyncable, *model.AppError) {
-	return a.Srv.Store.Group().CreateGroupSyncable(groupSyncable)
+func (a *App) UpsertGroupSyncable(groupSyncable *model.GroupSyncable) (*model.GroupSyncable, *model.AppError) {
+	gs, err := a.Srv.Store.Group().GetGroupSyncable(groupSyncable.GroupId, groupSyncable.SyncableId, groupSyncable.Type)
+	if err != nil && err.Id != "store.sql_group.no_rows" {
+		return nil, err
+	}
+
+	if gs == nil {
+		gs, err = a.Srv.Store.Group().CreateGroupSyncable(groupSyncable)
+		if err != nil {
+			return nil, err
+		}
+	} else {
+		gs, err = a.Srv.Store.Group().UpdateGroupSyncable(groupSyncable)
+		if err != nil {
+			return nil, err
+		}
+	}
+
+	// if the type is channel, then upsert the associated GroupTeam [MM-14675]
+	if gs.Type == model.GroupSyncableTypeChannel {
+		channel, err := a.Srv.Store.Channel().Get(gs.SyncableId, true)
+		if err != nil {
+			return nil, err
+		}
+		_, err = a.UpsertGroupSyncable(&model.GroupSyncable{
+			GroupId:    gs.GroupId,
+			SyncableId: channel.TeamId,
+			Type:       model.GroupSyncableTypeTeam,
+			AutoAdd:    gs.AutoAdd,
+		})
+		if err != nil {
+			return nil, err
+		}
+	}
+
+	return gs, nil
 }
 
 func (a *App) GetGroupSyncable(groupID string, syncableID string, syncableType model.GroupSyncableType) (*model.GroupSyncable, *model.AppError) {
@@ -77,11 +111,45 @@ func (a *App) GetGroupSyncables(groupID string, syncableType model.GroupSyncable
 }
 
 func (a *App) UpdateGroupSyncable(groupSyncable *model.GroupSyncable) (*model.GroupSyncable, *model.AppError) {
-	return a.Srv.Store.Group().UpdateGroupSyncable(groupSyncable)
+	var gs *model.GroupSyncable
+	var err *model.AppError
+
+	if groupSyncable.DeleteAt == 0 {
+		// updating a *deleted* GroupSyncable, so no need to ensure the GroupTeam is present (as done in the upsert)
+		gs, err = a.Srv.Store.Group().UpdateGroupSyncable(groupSyncable)
+	} else {
+		// do an upsert to ensure that there's an associated GroupTeam
+		gs, err = a.UpsertGroupSyncable(groupSyncable)
+	}
+	if err != nil {
+		return nil, err
+	}
+
+	return gs, nil
 }
 
 func (a *App) DeleteGroupSyncable(groupID string, syncableID string, syncableType model.GroupSyncableType) (*model.GroupSyncable, *model.AppError) {
-	return a.Srv.Store.Group().DeleteGroupSyncable(groupID, syncableID, syncableType)
+	gs, err := a.Srv.Store.Group().DeleteGroupSyncable(groupID, syncableID, syncableType)
+	if err != nil {
+		return nil, err
+	}
+
+	// if a GroupTeam is being deleted delete all associated GroupChannels
+	if gs.Type == model.GroupSyncableTypeTeam {
+		allGroupChannels, err := a.Srv.Store.Group().GetAllGroupSyncablesByGroupId(gs.GroupId, model.GroupSyncableTypeChannel)
+		if err != nil {
+			return nil, err
+		}
+
+		for _, groupChannel := range allGroupChannels {
+			_, err = a.Srv.Store.Group().DeleteGroupSyncable(groupChannel.GroupId, groupChannel.SyncableId, groupChannel.Type)
+			if err != nil {
+				return nil, err
+			}
+		}
+	}
+
+	return gs, nil
 }
 
 func (a *App) TeamMembersToAdd(since int64) ([]*model.UserTeamIDPair, *model.AppError) {

app/group_test.go

@@ -131,19 +131,30 @@ func TestDeleteGroupMember(t *testing.T) {
 	require.Nil(t, groupMember)
 }
 
-func TestCreateGroupSyncable(t *testing.T) {
+func TestUpsertGroupSyncable(t *testing.T) {
 	th := Setup(t).InitBasic()
 	defer th.TearDown()
 	group := th.CreateGroup()
 	groupSyncable := model.NewGroupTeam(group.Id, th.BasicTeam.Id, false)
 
-	gs, err := th.App.CreateGroupSyncable(groupSyncable)
+	gs, err := th.App.UpsertGroupSyncable(groupSyncable)
 	require.Nil(t, err)
 	require.NotNil(t, gs)
 
-	gs, err = th.App.CreateGroupSyncable(groupSyncable)
-	require.NotNil(t, err)
-	require.Nil(t, gs)
+	// can update again without error
+	gs, err = th.App.UpsertGroupSyncable(groupSyncable)
+	require.Nil(t, err)
+	require.NotNil(t, gs)
+
+	gs, err = th.App.DeleteGroupSyncable(gs.GroupId, gs.SyncableId, gs.Type)
+	require.Nil(t, err)
+	require.NotEqual(t, int64(0), gs.DeleteAt)
+
+	// Un-deleting works
+	gs.DeleteAt = 0
+	gs, err = th.App.UpsertGroupSyncable(gs)
+	require.Nil(t, err)
+	require.Equal(t, int64(0), gs.DeleteAt)
 }
 
 func TestGetGroupSyncable(t *testing.T) {
@@ -152,7 +163,7 @@ func TestGetGroupSyncable(t *testing.T) {
 	group := th.CreateGroup()
 	groupSyncable := model.NewGroupTeam(group.Id, th.BasicTeam.Id, false)
 
-	gs, err := th.App.CreateGroupSyncable(groupSyncable)
+	gs, err := th.App.UpsertGroupSyncable(groupSyncable)
 	require.Nil(t, err)
 	require.NotNil(t, gs)
 
@@ -169,7 +180,7 @@ func TestGetGroupSyncables(t *testing.T) {
 	// Create a group team
 	groupSyncable := model.NewGroupTeam(group.Id, th.BasicTeam.Id, false)
 
-	gs, err := th.App.CreateGroupSyncable(groupSyncable)
+	gs, err := th.App.UpsertGroupSyncable(groupSyncable)
 	require.Nil(t, err)
 	require.NotNil(t, gs)
 
@@ -185,7 +196,7 @@ func TestDeleteGroupSyncable(t *testing.T) {
 	group := th.CreateGroup()
 	groupChannel := model.NewGroupChannel(group.Id, th.BasicChannel.Id, false)
 
-	gs, err := th.App.CreateGroupSyncable(groupChannel)
+	gs, err := th.App.UpsertGroupSyncable(groupChannel)
 	require.Nil(t, err)
 	require.NotNil(t, gs)
 
@@ -211,7 +222,7 @@ func TestGetGroupsByChannel(t *testing.T) {
 		Type:       model.GroupSyncableTypeChannel,
 	}
 
-	gs, err := th.App.CreateGroupSyncable(groupSyncable)
+	gs, err := th.App.UpsertGroupSyncable(groupSyncable)
 	require.Nil(t, err)
 	require.NotNil(t, gs)
 
@@ -244,7 +255,7 @@ func TestGetGroupsByTeam(t *testing.T) {
 		Type:       model.GroupSyncableTypeTeam,
 	}
 
-	gs, err := th.App.CreateGroupSyncable(groupSyncable)
+	gs, err := th.App.UpsertGroupSyncable(groupSyncable)
 	require.Nil(t, err)
 	require.NotNil(t, gs)
 

app/notification_test.go

@@ -275,7 +275,7 @@ func TestFilterOutOfChannelMentions(t *testing.T) {
 		constrainedChannel, appErr = th.App.UpdateChannel(constrainedChannel)
 		require.Nil(t, appErr)
 
-		_, appErr = th.App.CreateGroupSyncable(&model.GroupSyncable{
+		_, appErr = th.App.UpsertGroupSyncable(&model.GroupSyncable{
 			GroupId:    group.Id,
 			Type:       model.GroupSyncableTypeChannel,
 			SyncableId: constrainedChannel.Id,

app/syncables_test.go

@@ -74,17 +74,17 @@ func TestCreateDefaultMemberships(t *testing.T) {
 		t.Errorf("test group not created: %s", err.Error())
 	}
 
-	_, err = th.App.CreateGroupSyncable(model.NewGroupChannel(gleeGroup.Id, practiceChannel.Id, true))
+	_, err = th.App.UpsertGroupSyncable(model.NewGroupChannel(gleeGroup.Id, practiceChannel.Id, true))
 	if err != nil {
 		t.Errorf("test groupchannel not created: %s", err.Error())
 	}
 
-	scienceTeamGroupSyncable, err := th.App.CreateGroupSyncable(model.NewGroupTeam(scienceGroup.Id, nerdsTeam.Id, false))
+	scienceTeamGroupSyncable, err := th.App.UpsertGroupSyncable(model.NewGroupTeam(scienceGroup.Id, nerdsTeam.Id, false))
 	if err != nil {
 		t.Errorf("test groupteam not created: %s", err.Error())
 	}
 
-	scienceChannelGroupSyncable, err := th.App.CreateGroupSyncable(model.NewGroupChannel(scienceGroup.Id, experimentsChannel.Id, false))
+	scienceChannelGroupSyncable, err := th.App.UpsertGroupSyncable(model.NewGroupChannel(scienceGroup.Id, experimentsChannel.Id, false))
 	if err != nil {
 		t.Errorf("test groupchannel not created: %s", err.Error())
 	}
@@ -363,9 +363,9 @@ func TestDeleteGroupMemberships(t *testing.T) {
 	require.True(t, *channel.GroupConstrained)
 
 	// create groupteam and groupchannel
-	_, err = th.App.CreateGroupSyncable(model.NewGroupTeam(group.Id, team.Id, true))
+	_, err = th.App.UpsertGroupSyncable(model.NewGroupTeam(group.Id, team.Id, true))
 	require.Nil(t, err)
-	_, err = th.App.CreateGroupSyncable(model.NewGroupChannel(group.Id, channel.Id, true))
+	_, err = th.App.UpsertGroupSyncable(model.NewGroupChannel(group.Id, channel.Id, true))
 	require.Nil(t, err)
 
 	// verify the member count

cmd/mattermost/commands/group_test.go

@@ -36,7 +36,7 @@ func TestChannelGroupEnable(t *testing.T) {
 	})
 	require.Nil(t, err)
 
-	_, err = th.App.CreateGroupSyncable(&model.GroupSyncable{
+	_, err = th.App.UpsertGroupSyncable(&model.GroupSyncable{
 		AutoAdd:    true,
 		SyncableId: channel.Id,
 		Type:       model.GroupSyncableTypeChannel,
@@ -80,7 +80,7 @@ func TestChannelGroupDisable(t *testing.T) {
 	})
 	require.Nil(t, err)
 
-	_, err = th.App.CreateGroupSyncable(&model.GroupSyncable{
+	_, err = th.App.UpsertGroupSyncable(&model.GroupSyncable{
 		AutoAdd:    true,
 		SyncableId: channel.Id,
 		Type:       model.GroupSyncableTypeChannel,
@@ -127,7 +127,7 @@ func TestChannelGroupStatus(t *testing.T) {
 	})
 	require.Nil(t, err)
 
-	_, err = th.App.CreateGroupSyncable(&model.GroupSyncable{
+	_, err = th.App.UpsertGroupSyncable(&model.GroupSyncable{
 		AutoAdd:    true,
 		SyncableId: channel.Id,
 		Type:       model.GroupSyncableTypeChannel,
@@ -170,7 +170,7 @@ func TestChannelGroupList(t *testing.T) {
 	})
 	require.Nil(t, err)
 
-	_, err = th.App.CreateGroupSyncable(&model.GroupSyncable{
+	_, err = th.App.UpsertGroupSyncable(&model.GroupSyncable{
 		AutoAdd:    true,
 		SyncableId: channel.Id,
 		Type:       model.GroupSyncableTypeChannel,
@@ -188,7 +188,7 @@ func TestChannelGroupList(t *testing.T) {
 	})
 	require.Nil(t, err)
 
-	_, err = th.App.CreateGroupSyncable(&model.GroupSyncable{
+	_, err = th.App.UpsertGroupSyncable(&model.GroupSyncable{
 		AutoAdd:    true,
 		SyncableId: channel.Id,
 		Type:       model.GroupSyncableTypeChannel,
@@ -229,7 +229,7 @@ func TestTeamGroupEnable(t *testing.T) {
 	})
 	require.Nil(t, err)
 
-	_, err = th.App.CreateGroupSyncable(&model.GroupSyncable{
+	_, err = th.App.UpsertGroupSyncable(&model.GroupSyncable{
 		AutoAdd:    true,
 		SyncableId: th.BasicTeam.Id,
 		Type:       model.GroupSyncableTypeTeam,
@@ -270,7 +270,7 @@ func TestTeamGroupDisable(t *testing.T) {
 	})
 	require.Nil(t, err)
 
-	_, err = th.App.CreateGroupSyncable(&model.GroupSyncable{
+	_, err = th.App.UpsertGroupSyncable(&model.GroupSyncable{
 		AutoAdd:    true,
 		SyncableId: team.Id,
 		Type:       model.GroupSyncableTypeTeam,
@@ -314,7 +314,7 @@ func TestTeamGroupStatus(t *testing.T) {
 	})
 	require.Nil(t, err)
 
-	_, err = th.App.CreateGroupSyncable(&model.GroupSyncable{
+	_, err = th.App.UpsertGroupSyncable(&model.GroupSyncable{
 		AutoAdd:    true,
 		SyncableId: th.BasicTeam.Id,
 		Type:       model.GroupSyncableTypeTeam,
@@ -354,7 +354,7 @@ func TestTeamGroupList(t *testing.T) {
 	})
 	require.Nil(t, err)
 
-	_, err = th.App.CreateGroupSyncable(&model.GroupSyncable{
+	_, err = th.App.UpsertGroupSyncable(&model.GroupSyncable{
 		AutoAdd:    true,
 		SyncableId: th.BasicTeam.Id,
 		Type:       model.GroupSyncableTypeTeam,
@@ -372,7 +372,7 @@ func TestTeamGroupList(t *testing.T) {
 	})
 	require.Nil(t, err)
 
-	_, err = th.App.CreateGroupSyncable(&model.GroupSyncable{
+	_, err = th.App.UpsertGroupSyncable(&model.GroupSyncable{
 		AutoAdd:    true,
 		SyncableId: th.BasicTeam.Id,
 		Type:       model.GroupSyncableTypeTeam,

store/sqlstore/group_store.go

@@ -192,7 +192,7 @@ func (s *SqlGroupStore) GetByUser(userId string) ([]*model.Group, *model.AppErro
 
 func (s *SqlGroupStore) Update(group *model.Group) (*model.Group, *model.AppError) {
 	var retrievedGroup *model.Group
-	if err := s.GetMaster().SelectOne(&retrievedGroup, "SELECT * FROM UserGroups WHERE Id = :Id", map[string]interface{}{"Id": group.Id}); err != nil {
+	if err := s.GetReplica().SelectOne(&retrievedGroup, "SELECT * FROM UserGroups WHERE Id = :Id", map[string]interface{}{"Id": group.Id}); err != nil {
 		if err == sql.ErrNoRows {
 			return nil, model.NewAppError("SqlGroupStore.GroupUpdate", "store.sql_group.no_rows", nil, "id="+group.Id+","+err.Error(), http.StatusNotFound)
 		}
@@ -322,12 +322,12 @@ func (s *SqlGroupStore) UpsertMember(groupID string, userID string) (*model.Grou
 	}
 
 	var retrievedGroup *model.Group
-	if err := s.GetMaster().SelectOne(&retrievedGroup, "SELECT * FROM UserGroups WHERE Id = :Id", map[string]interface{}{"Id": groupID}); err != nil {
+	if err := s.GetReplica().SelectOne(&retrievedGroup, "SELECT * FROM UserGroups WHERE Id = :Id", map[string]interface{}{"Id": groupID}); err != nil {
 		return nil, model.NewAppError("SqlGroupStore.GroupCreateOrRestoreMember", "store.insert_error", nil, "group_id="+member.GroupId+"user_id="+member.UserId+","+err.Error(), http.StatusInternalServerError)
 	}
 
 	var retrievedMember *model.GroupMember
-	if err := s.GetMaster().SelectOne(&retrievedMember, "SELECT * FROM GroupMembers WHERE GroupId = :GroupId AND UserId = :UserId", map[string]interface{}{"GroupId": member.GroupId, "UserId": member.UserId}); err != nil {
+	if err := s.GetReplica().SelectOne(&retrievedMember, "SELECT * FROM GroupMembers WHERE GroupId = :GroupId AND UserId = :UserId", map[string]interface{}{"GroupId": member.GroupId, "UserId": member.UserId}); err != nil {
 		if err != sql.ErrNoRows {
 			return nil, model.NewAppError("SqlGroupStore.GroupCreateOrRestoreMember", "store.select_error", nil, "group_id="+member.GroupId+"user_id="+member.UserId+","+err.Error(), http.StatusInternalServerError)
 		}
@@ -357,7 +357,7 @@ func (s *SqlGroupStore) UpsertMember(groupID string, userID string) (*model.Grou
 
 func (s *SqlGroupStore) DeleteMember(groupID string, userID string) (*model.GroupMember, *model.AppError) {
 	var retrievedMember *model.GroupMember
-	if err := s.GetMaster().SelectOne(&retrievedMember, "SELECT * FROM GroupMembers WHERE GroupId = :GroupId AND UserId = :UserId AND DeleteAt = 0", map[string]interface{}{"GroupId": groupID, "UserId": userID}); err != nil {
+	if err := s.GetReplica().SelectOne(&retrievedMember, "SELECT * FROM GroupMembers WHERE GroupId = :GroupId AND UserId = :UserId AND DeleteAt = 0", map[string]interface{}{"GroupId": groupID, "UserId": userID}); err != nil {
 		if err == sql.ErrNoRows {
 			return nil, model.NewAppError("SqlGroupStore.GroupDeleteMember", "store.sql_group.no_rows", nil, "group_id="+groupID+"user_id="+userID+","+err.Error(), http.StatusNotFound)
 		}
@@ -434,9 +434,9 @@ func (s *SqlGroupStore) getGroupSyncable(groupID string, syncableID string, sync
 
 	switch syncableType {
 	case model.GroupSyncableTypeTeam:
-		result, err = s.GetMaster().Get(groupTeam{}, groupID, syncableID)
+		result, err = s.GetReplica().Get(groupTeam{}, groupID, syncableID)
 	case model.GroupSyncableTypeChannel:
-		result, err = s.GetMaster().Get(groupChannel{}, groupID, syncableID)
+		result, err = s.GetReplica().Get(groupChannel{}, groupID, syncableID)
 	}
 
 	if err != nil {
@@ -497,7 +497,7 @@ func (s *SqlGroupStore) GetAllGroupSyncablesByGroupId(groupID string, syncableTy
 				GroupId = :GroupId AND GroupTeams.DeleteAt = 0`
 
 		results := []*groupTeamJoin{}
-		_, err := s.GetMaster().Select(&results, sqlQuery, args)
+		_, err := s.GetReplica().Select(&results, sqlQuery, args)
 		if err != nil {
 			return nil, appErrF(err.Error())
 		}
@@ -532,7 +532,7 @@ func (s *SqlGroupStore) GetAllGroupSyncablesByGroupId(groupID string, syncableTy
 				GroupId = :GroupId AND GroupChannels.DeleteAt = 0`
 
 		results := []*groupChannelJoin{}
-		_, err := s.GetMaster().Select(&results, sqlQuery, args)
+		_, err := s.GetReplica().Select(&results, sqlQuery, args)
 		if err != nil {
 			return nil, appErrF(err.Error())
 		}