Migrate Get/GetFromMaster methods from ChannelStore to return error interface (#14688)

* Advances

* Migration finished

* Rename err to normalized error

* fix imports

* Renamed key

* Renamed key

* Suggestions

* Fix i18n

* Fix tests

Co-authored-by: Jesús Espino <jespinog@gmail.com>
Co-authored-by: Agniva De Sarker <agnivade@yahoo.co.in>

api4/group_test.go

@@ -714,7 +714,7 @@ func TestGetGroupsByChannel(t *testing.T) {
 		require.True(t, *groups[0].SchemeAdmin)
 
 		groups, _, response = client.GetGroupsByChannel(model.NewId(), opts)
-		assert.Equal(t, "store.sql_channel.get.existing.app_error", response.Error.Id)
+		assert.Equal(t, "app.channel.get.existing.app_error", response.Error.Id)
 		assert.Empty(t, groups)
 	})
 }

app/channel.go

@@ -1471,14 +1471,15 @@ func (a *App) PostUpdateChannelDisplayNameMessage(userId string, channel *model.
 }
 
 func (a *App) GetChannel(channelId string) (*model.Channel, *model.AppError) {
-	channel, errCh := a.Srv().Store.Channel().Get(channelId, true)
+	channel, err := a.Srv().Store.Channel().Get(channelId, true)
-	if errCh != nil {
+	if err != nil {
-		if errCh.Id == "store.sql_channel.get.existing.app_error" {
+		var nfErr *store.ErrNotFound
-			errCh.StatusCode = http.StatusNotFound
+		switch {
-			return nil, errCh
+		case errors.As(err, &nfErr):
+			return nil, model.NewAppError("GetChannel", "app.channel.get.existing.app_error", nil, nfErr.Error(), http.StatusNotFound)
+		default:
+			return nil, model.NewAppError("GetChannel", "app.channel.get.find.app_error", nil, err.Error(), http.StatusInternalServerError)
 		}
-		errCh.StatusCode = http.StatusBadRequest
-		return nil, errCh
 	}
 	return channel, nil
 }
@@ -1772,7 +1773,7 @@ func (a *App) LeaveChannel(channelId string, userId string) *model.AppError {
 	sc := make(chan store.StoreResult, 1)
 	go func() {
 		channel, err := a.Srv().Store.Channel().Get(channelId, true)
-		sc <- store.StoreResult{Data: channel, Err: err}
+		sc <- store.StoreResult{Data: channel, NErr: err}
 		close(sc)
 	}()
 
@@ -1795,8 +1796,14 @@ func (a *App) LeaveChannel(channelId string, userId string) *model.AppError {
 		return cresult.Err
 	}
 	uresult := <-uc
-	if uresult.Err != nil {
+	if uresult.NErr != nil {
-		return cresult.Err
+		var nfErr *store.ErrNotFound
+		switch {
+		case errors.As(uresult.NErr, &nfErr):
+			return model.NewAppError("LeaveChannel", "app.channel.get.existing.app_error", nil, nfErr.Error(), http.StatusNotFound)
+		default:
+			return model.NewAppError("LeaveChannel", "app.channel.get.find.app_error", nil, uresult.NErr.Error(), http.StatusInternalServerError)
+		}
 	}
 	ccresult := <-mcc
 	if ccresult.Err != nil {

app/command.go

@@ -4,6 +4,7 @@
 package app
 
 import (
+	"errors"
 	"io"
 	"io/ioutil"
 	"net/http"
@@ -344,7 +345,7 @@ func (a *App) tryExecuteCustomCommand(args *model.CommandArgs, trigger string, m
 	chanChan := make(chan store.StoreResult, 1)
 	go func() {
 		channel, err := a.Srv().Store.Channel().Get(args.ChannelId, true)
-		chanChan <- store.StoreResult{Data: channel, Err: err}
+		chanChan <- store.StoreResult{Data: channel, NErr: err}
 		close(chanChan)
 	}()
 
@@ -380,8 +381,14 @@ func (a *App) tryExecuteCustomCommand(args *model.CommandArgs, trigger string, m
 	user := ur.Data.(*model.User)
 
 	cr := <-chanChan
-	if cr.Err != nil {
+	if cr.NErr != nil {
-		return nil, nil, cr.Err
+		var nfErr *store.ErrNotFound
+		switch {
+		case errors.As(cr.NErr, &nfErr):
+			return nil, nil, model.NewAppError("tryExecuteCustomCommand", "app.channel.get.existing.app_error", nil, nfErr.Error(), http.StatusNotFound)
+		default:
+			return nil, nil, model.NewAppError("tryExecuteCustomCommand", "app.channel.get.find.app_error", nil, cr.NErr.Error(), http.StatusInternalServerError)
+		}
 	}
 	channel := cr.Data.(*model.Channel)
 

app/group.go

@@ -4,9 +4,11 @@
 package app
 
 import (
+	"errors"
 	"net/http"
 
 	"github.com/mattermost/mattermost-server/v5/model"
+	"github.com/mattermost/mattermost-server/v5/store"
 )
 
 func (a *App) GetGroup(id string) (*model.Group, *model.AppError) {
@@ -90,10 +92,15 @@ func (a *App) UpsertGroupSyncable(groupSyncable *model.GroupSyncable) (*model.Gr
 
 	// reject the syncable creation if the group isn't already associated to the parent team
 	if groupSyncable.Type == model.GroupSyncableTypeChannel {
-		var channel *model.Channel
+		channel, nErr := a.Srv().Store.Channel().Get(groupSyncable.SyncableId, true)
-		channel, err = a.Srv().Store.Channel().Get(groupSyncable.SyncableId, true)
+		if nErr != nil {
-		if err != nil {
+			var nfErr *store.ErrNotFound
-			return nil, err
+			switch {
+			case errors.As(nErr, &nfErr):
+				return nil, model.NewAppError("UpsertGroupSyncable", "app.channel.get.existing.app_error", nil, nfErr.Error(), http.StatusNotFound)
+			default:
+				return nil, model.NewAppError("UpsertGroupSyncable", "app.channel.get.find.app_error", nil, nErr.Error(), http.StatusInternalServerError)
+			}
 		}
 
 		var team *model.Team

app/integration_action.go

@@ -20,6 +20,7 @@ package app
 import (
 	"bytes"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io/ioutil"
 	"net/http"
@@ -101,7 +102,13 @@ func (a *App) DoPostActionWithCookie(postId, actionId, userId, selectedOption st
 
 		channel, err := a.Srv().Store.Channel().Get(cookie.ChannelId, true)
 		if err != nil {
-			return "", err
+			var nfErr *store.ErrNotFound
+			switch {
+			case errors.As(err, &nfErr):
+				return "", model.NewAppError("DoPostActionWithCookie", "app.channel.get.existing.app_error", nil, nfErr.Error(), http.StatusNotFound)
+			default:
+				return "", model.NewAppError("DoPostActionWithCookie", "app.channel.get.find.app_error", nil, err.Error(), http.StatusInternalServerError)
+			}
 		}
 
 		upstreamRequest.ChannelId = cookie.ChannelId

app/post.go

@@ -5,6 +5,7 @@ package app
 
 import (
 	"encoding/json"
+	"errors"
 	"fmt"
 	"net/http"
 	"strings"
@@ -94,7 +95,13 @@ func (a *App) CreatePostAsUser(post *model.Post, currentSessionId string, setOnl
 func (a *App) CreatePostMissingChannel(post *model.Post, triggerWebhooks bool) (*model.Post, *model.AppError) {
 	channel, err := a.Srv().Store.Channel().Get(post.ChannelId, true)
 	if err != nil {
-		return nil, err
+		var nfErr *store.ErrNotFound
+		switch {
+		case errors.As(err, &nfErr):
+			return nil, model.NewAppError("CreatePostMissingChannel", "app.channel.get.existing.app_error", nil, nfErr.Error(), http.StatusNotFound)
+		default:
+			return nil, model.NewAppError("CreatePostMissingChannel", "app.channel.get.find.app_error", nil, err.Error(), http.StatusInternalServerError)
+		}
 	}
 
 	return a.CreatePost(post, channel, triggerWebhooks, true)

app/webhook.go

@@ -4,6 +4,7 @@
 package app
 
 import (
+	"errors"
 	"io"
 	"net/http"
 	"regexp"
@@ -412,7 +413,13 @@ func (a *App) CreateOutgoingWebhook(hook *model.OutgoingWebhook) (*model.Outgoin
 	if len(hook.ChannelId) != 0 {
 		channel, errCh := a.Srv().Store.Channel().Get(hook.ChannelId, true)
 		if errCh != nil {
-			return nil, errCh
+			var nfErr *store.ErrNotFound
+			switch {
+			case errors.As(errCh, &nfErr):
+				return nil, model.NewAppError("CreateOutgoingWebhook", "app.channel.get.existing.app_error", nil, nfErr.Error(), http.StatusNotFound)
+			default:
+				return nil, model.NewAppError("CreateOutgoingWebhook", "app.channel.get.find.app_error", nil, errCh.Error(), http.StatusInternalServerError)
+			}
 		}
 
 		if channel.Type != model.CHANNEL_OPEN {
@@ -633,10 +640,16 @@ func (a *App) HandleIncomingWebhook(hookId string, req *model.IncomingWebhookReq
 			}()
 		}
 	} else {
-		var err *model.AppError
+		var err error
 		channel, err = a.Srv().Store.Channel().Get(hook.ChannelId, true)
 		if err != nil {
-			return model.NewAppError("HandleIncomingWebhook", "web.incoming_webhook.channel.app_error", nil, "err="+err.Message, err.StatusCode)
+			var nfErr *store.ErrNotFound
+			switch {
+			case errors.As(err, &nfErr):
+				return model.NewAppError("HandleIncomingWebhook", "app.channel.get.existing.app_error", nil, nfErr.Error(), http.StatusNotFound)
+			default:
+				return model.NewAppError("HandleIncomingWebhook", "app.channel.get.find.app_error", nil, err.Error(), http.StatusInternalServerError)
+			}
 		}
 	}
 

i18n/en.json

@@ -2974,6 +2974,14 @@
     "id": "app.channel.create_direct_channel.internal_error",
     "translation": "Unable to save direct channel."
   },
+  {
+    "id": "app.channel.get.existing.app_error",
+    "translation": "Unable to find the existing channel."
+  },
+  {
+    "id": "app.channel.get.find.app_error",
+    "translation": "We encountered an error finding the channel."
+  },
   {
     "id": "app.channel.move_channel.members_do_not_match.error",
     "translation": "Unable to move a channel unless all its members are already members of the destination team."

store/localcachelayer/channel_layer.go

@@ -150,7 +150,7 @@ func (s LocalCacheChannelStore) GetPinnedPostCount(channelId string, allowFromCa
 	return count, nil
 }
 
-func (s LocalCacheChannelStore) Get(id string, allowFromCache bool) (*model.Channel, *model.AppError) {
+func (s LocalCacheChannelStore) Get(id string, allowFromCache bool) (*model.Channel, error) {
 
 	if allowFromCache {
 		if cacheItem := s.rootStore.doStandardReadCache(s.rootStore.channelByIdCache, id); cacheItem != nil {

store/opentracing_layer.go

@@ -630,7 +630,7 @@ func (s *OpenTracingLayerChannelStore) Delete(channelId string, time int64) *mod
 	return resultVar0
 }
 
-func (s *OpenTracingLayerChannelStore) Get(id string, allowFromCache bool) (*model.Channel, *model.AppError) {
+func (s *OpenTracingLayerChannelStore) Get(id string, allowFromCache bool) (*model.Channel, error) {
 	origCtx := s.Root.Store.Context()
 	span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "ChannelStore.Get")
 	s.Root.Store.SetContext(newCtx)
@@ -1026,7 +1026,7 @@ func (s *OpenTracingLayerChannelStore) GetForPost(postId string) (*model.Channel
 	return resultVar0, resultVar1
 }
 
-func (s *OpenTracingLayerChannelStore) GetFromMaster(id string) (*model.Channel, *model.AppError) {
+func (s *OpenTracingLayerChannelStore) GetFromMaster(id string) (*model.Channel, error) {
 	origCtx := s.Root.Store.Context()
 	span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "ChannelStore.GetFromMaster")
 	s.Root.Store.SetContext(newCtx)

store/sqlstore/channel_store.go

@@ -725,7 +725,7 @@ func (s SqlChannelStore) InvalidateChannelByName(teamId, name string) {
 	}
 }
 
-func (s SqlChannelStore) Get(id string, allowFromCache bool) (*model.Channel, *model.AppError) {
+func (s SqlChannelStore) Get(id string, allowFromCache bool) (*model.Channel, error) {
 	return s.get(id, false, allowFromCache)
 }
 
@@ -743,11 +743,11 @@ func (s SqlChannelStore) GetPinnedPosts(channelId string) (*model.PostList, *mod
 	return pl, nil
 }
 
-func (s SqlChannelStore) GetFromMaster(id string) (*model.Channel, *model.AppError) {
+func (s SqlChannelStore) GetFromMaster(id string) (*model.Channel, error) {
 	return s.get(id, true, false)
 }
 
-func (s SqlChannelStore) get(id string, master bool, allowFromCache bool) (*model.Channel, *model.AppError) {
+func (s SqlChannelStore) get(id string, master bool, allowFromCache bool) (*model.Channel, error) {
 	var db *gorp.DbMap
 
 	if master {
@@ -758,11 +758,11 @@ func (s SqlChannelStore) get(id string, master bool, allowFromCache bool) (*mode
 
 	obj, err := db.Get(model.Channel{}, id)
 	if err != nil {
-		return nil, model.NewAppError("SqlChannelStore.Get", "store.sql_channel.get.find.app_error", nil, "id="+id+", "+err.Error(), http.StatusInternalServerError)
+		return nil, errors.Wrapf(err, "failed to find channel with id = %s", id)
 	}
 
 	if obj == nil {
-		return nil, model.NewAppError("SqlChannelStore.Get", "store.sql_channel.get.existing.app_error", nil, "id="+id, http.StatusNotFound)
+		return nil, store.NewErrNotFound("Channel", id)
 	}
 
 	ch := obj.(*model.Channel)

store/sqlstore/group_store.go

@@ -10,6 +10,7 @@ import (
 	"strings"
 
 	sq "github.com/Masterminds/squirrel"
+	"github.com/pkg/errors"
 
 	"github.com/mattermost/mattermost-server/v5/model"
 	"github.com/mattermost/mattermost-server/v5/store"
@@ -480,7 +481,13 @@ func (s *SqlGroupStore) CreateGroupSyncable(groupSyncable *model.GroupSyncable)
 		insertErr = s.GetMaster().Insert(groupSyncableToGroupTeam(groupSyncable))
 	case model.GroupSyncableTypeChannel:
 		if _, err := s.Channel().Get(groupSyncable.SyncableId, false); err != nil {
-			return nil, err
+			var nfErr *store.ErrNotFound
+			switch {
+			case errors.As(err, &nfErr):
+				return nil, model.NewAppError("CreateGroupSyncable", "store.sql_channel.get.existing.app_error", nil, nfErr.Error(), http.StatusNotFound)
+			default:
+				return nil, model.NewAppError("CreateGroupSyncable", "store.sql_channel.get.find.app_error", nil, err.Error(), http.StatusInternalServerError)
+			}
 		}
 
 		insertErr = s.GetMaster().Insert(groupSyncableToGroupChannel(groupSyncable))

store/store.go

@@ -15,6 +15,9 @@ import (
 type StoreResult struct {
 	Data interface{}
 	Err  *model.AppError
+
+	// NErr a temporary field used by the new code for the AppError migration. This will later become Err when the entire store is migrated.
+	NErr error
 }
 
 type Store interface {
@@ -131,10 +134,10 @@ type ChannelStore interface {
 	CreateDirectChannel(userId *model.User, otherUserId *model.User) (*model.Channel, error)
 	SaveDirectChannel(channel *model.Channel, member1 *model.ChannelMember, member2 *model.ChannelMember) (*model.Channel, error)
 	Update(channel *model.Channel) (*model.Channel, error)
-	Get(id string, allowFromCache bool) (*model.Channel, *model.AppError)
+	Get(id string, allowFromCache bool) (*model.Channel, error)
 	InvalidateChannel(id string)
 	InvalidateChannelByName(teamId, name string)
-	GetFromMaster(id string) (*model.Channel, *model.AppError)
+	GetFromMaster(id string) (*model.Channel, error)
 	Delete(channelId string, time int64) *model.AppError
 	Restore(channelId string, time int64) *model.AppError
 	SetDeleteAt(channelId string, deleteAt int64, updateAt int64) *model.AppError

store/storetest/mocks/ChannelStore.go

@@ -195,7 +195,7 @@ func (_m *ChannelStore) Delete(channelId string, time int64) *model.AppError {
 }
 
 // Get provides a mock function with given fields: id, allowFromCache
-func (_m *ChannelStore) Get(id string, allowFromCache bool) (*model.Channel, *model.AppError) {
+func (_m *ChannelStore) Get(id string, allowFromCache bool) (*model.Channel, error) {
 	ret := _m.Called(id, allowFromCache)
 
 	var r0 *model.Channel
@@ -207,13 +207,11 @@ func (_m *ChannelStore) Get(id string, allowFromCache bool) (*model.Channel, *mo
 		}
 	}
 
-	var r1 *model.AppError
+	var r1 error
-	if rf, ok := ret.Get(1).(func(string, bool) *model.AppError); ok {
+	if rf, ok := ret.Get(1).(func(string, bool) error); ok {
 		r1 = rf(id, allowFromCache)
 	} else {
-		if ret.Get(1) != nil {
+		r1 = ret.Error(1)
-			r1 = ret.Get(1).(*model.AppError)
-		}
 	}
 
 	return r0, r1
@@ -743,7 +741,7 @@ func (_m *ChannelStore) GetForPost(postId string) (*model.Channel, *model.AppErr
 }
 
 // GetFromMaster provides a mock function with given fields: id
-func (_m *ChannelStore) GetFromMaster(id string) (*model.Channel, *model.AppError) {
+func (_m *ChannelStore) GetFromMaster(id string) (*model.Channel, error) {
 	ret := _m.Called(id)
 
 	var r0 *model.Channel
@@ -755,13 +753,11 @@ func (_m *ChannelStore) GetFromMaster(id string) (*model.Channel, *model.AppErro
 		}
 	}
 
-	var r1 *model.AppError
+	var r1 error
-	if rf, ok := ret.Get(1).(func(string) *model.AppError); ok {
+	if rf, ok := ret.Get(1).(func(string) error); ok {
 		r1 = rf(id)
 	} else {
-		if ret.Get(1) != nil {
+		r1 = ret.Error(1)
-			r1 = ret.Get(1).(*model.AppError)
-		}
 	}
 
 	return r0, r1

store/storetest/scheme_store.go

@@ -450,8 +450,8 @@ func testSchemeStoreDelete(t *testing.T, ss store.Store) {
 	_, err = ss.Scheme().Delete(d5.Id)
 	assert.Nil(t, err)
 
-	c6, err := ss.Channel().Get(c5.Id, true)
+	c6, nErr := ss.Channel().Get(c5.Id, true)
-	assert.Nil(t, err)
+	assert.Nil(t, nErr)
 	assert.Equal(t, "", *c6.SchemeId)
 }
 

store/timer_layer.go

@@ -600,7 +600,7 @@ func (s *TimerLayerChannelStore) Delete(channelId string, time int64) *model.App
 	return resultVar0
 }
 
-func (s *TimerLayerChannelStore) Get(id string, allowFromCache bool) (*model.Channel, *model.AppError) {
+func (s *TimerLayerChannelStore) Get(id string, allowFromCache bool) (*model.Channel, error) {
 	start := timemodule.Now()
 
 	resultVar0, resultVar1 := s.ChannelStore.Get(id, allowFromCache)
@@ -952,7 +952,7 @@ func (s *TimerLayerChannelStore) GetForPost(postId string) (*model.Channel, *mod
 	return resultVar0, resultVar1
 }
 
-func (s *TimerLayerChannelStore) GetFromMaster(id string) (*model.Channel, *model.AppError) {
+func (s *TimerLayerChannelStore) GetFromMaster(id string) (*model.Channel, error) {
 	start := timemodule.Now()
 
 	resultVar0, resultVar1 := s.ChannelStore.GetFromMaster(id)