MM-24133: Migrate AppError from bot_store.go (#14339)

* MM-24135: Migrate AppError from SaveChannel/channel_store.go

This is the first POC of migration of store app errors to plain error.

We create a few basic error types in the store package and use
them to return the errors from store methods. In the app layer,
we inspect the error and re-create the exact app errors. This lets
us preserve the same error content, but yet move to plain errors.

Since this is a gradual migration, this means that the error inspection
code will be duplicated across the app layer whenever a store method
is invoked. But all of that should go away once we start propagating
the errors higher up the hierarchy.

There have been a significant amount of changes in the storetest and searchtest
layer, primarily because we have to rename the err variable now that it is of
a different type.

* Addressed review comments

* MM-24132: Migrate AppError from SaveDirectChannel/channel_store.go

This PR migrates 2 new methods SaveDirectChannel and CreateDirectChannel
to return error instead of AppError.

We also need to handle the error internally in SaveMultipleMember for now
until that is migrated too.

* MM-24133: Migrate AppError from bot_store.go

* Fix errors

* Fix err

* Fix bad return

* Fix vet errors

* Fix incorrect error check

Co-authored-by: mattermod <mattermod@users.noreply.github.com>

store/sqlstore/bot_store.go

@@ -5,12 +5,14 @@ package sqlstore
 
 import (
 	"database/sql"
-	"net/http"
+	"fmt"
 	"strings"
 
 	"github.com/mattermost/mattermost-server/v5/einterfaces"
 	"github.com/mattermost/mattermost-server/v5/model"
 	"github.com/mattermost/mattermost-server/v5/store"
+
+	"github.com/pkg/errors"
 )
 
 // bot is a subset of the model.Bot type, omitting the model.User fields.
@@ -64,21 +66,8 @@ func newSqlBotStore(sqlStore SqlStore, metrics einterfaces.MetricsInterface) sto
 func (us SqlBotStore) createIndexesIfNotExists() {
 }
 
-// traceBot is a helper function for adding to a bot trace when logging.
-func traceBot(bot *model.Bot, extra map[string]interface{}) map[string]interface{} {
-	trace := make(map[string]interface{})
-	for key, value := range bot.Trace() {
-		trace[key] = value
-	}
-	for key, value := range extra {
-		trace[key] = value
-	}
-
-	return trace
-}
-
 // Get fetches the given bot in the database.
-func (us SqlBotStore) Get(botUserId string, includeDeleted bool) (*model.Bot, *model.AppError) {
+func (us SqlBotStore) Get(botUserId string, includeDeleted bool) (*model.Bot, error) {
 	var excludeDeletedSql = "AND b.DeleteAt = 0"
 	if includeDeleted {
 		excludeDeletedSql = ""
@@ -106,16 +95,16 @@ func (us SqlBotStore) Get(botUserId string, includeDeleted bool) (*model.Bot, *m
 
 	var bot *model.Bot
 	if err := us.GetReplica().SelectOne(&bot, query, map[string]interface{}{"user_id": botUserId}); err == sql.ErrNoRows {
-		return nil, model.MakeBotNotFoundError(botUserId)
+		return nil, store.NewErrNotFound("Bot", botUserId)
 	} else if err != nil {
-		return nil, model.NewAppError("SqlBotStore.Get", "store.sql_bot.get.app_error", map[string]interface{}{"user_id": botUserId}, err.Error(), http.StatusInternalServerError)
+		return nil, errors.Wrapf(err, "selectone: user_id=%s", botUserId)
 	}
 
 	return bot, nil
 }
 
 // GetAll fetches from all bots in the database.
-func (us SqlBotStore) GetAll(options *model.BotGetOptions) ([]*model.Bot, *model.AppError) {
+func (us SqlBotStore) GetAll(options *model.BotGetOptions) ([]*model.Bot, error) {
 	params := map[string]interface{}{
 		"offset": options.Page * options.PerPage,
 		"limit":  options.PerPage,
@@ -169,7 +158,7 @@ func (us SqlBotStore) GetAll(options *model.BotGetOptions) ([]*model.Bot, *model
 
 	var bots []*model.Bot
 	if _, err := us.GetReplica().Select(&bots, sql, params); err != nil {
-		return nil, model.NewAppError("SqlBotStore.GetAll", "store.sql_bot.get_all.app_error", nil, err.Error(), http.StatusInternalServerError)
+		return nil, errors.Wrap(err, "select")
 	}
 
 	return bots, nil
@@ -177,16 +166,16 @@ func (us SqlBotStore) GetAll(options *model.BotGetOptions) ([]*model.Bot, *model
 
 // Save persists a new bot to the database.
 // It assumes the corresponding user was saved via the user store.
-func (us SqlBotStore) Save(bot *model.Bot) (*model.Bot, *model.AppError) {
+func (us SqlBotStore) Save(bot *model.Bot) (*model.Bot, error) {
 	bot = bot.Clone()
 	bot.PreSave()
 
-	if err := bot.IsValid(); err != nil {
+	if err := bot.IsValid(); err != nil { // TODO: change to return error in v6.
 		return nil, err
 	}
 
 	if err := us.GetMaster().Insert(botFromModel(bot)); err != nil {
-		return nil, model.NewAppError("SqlBotStore.Save", "store.sql_bot.save.app_error", bot.Trace(), err.Error(), http.StatusInternalServerError)
+		return nil, errors.Wrapf(err, "insert: user_id=%s", bot.UserId)
 	}
 
 	return bot, nil
@@ -194,11 +183,11 @@ func (us SqlBotStore) Save(bot *model.Bot) (*model.Bot, *model.AppError) {
 
 // Update persists an updated bot to the database.
 // It assumes the corresponding user was updated via the user store.
-func (us SqlBotStore) Update(bot *model.Bot) (*model.Bot, *model.AppError) {
+func (us SqlBotStore) Update(bot *model.Bot) (*model.Bot, error) {
 	bot = bot.Clone()
 
 	bot.PreUpdate()
-	if err := bot.IsValid(); err != nil {
+	if err := bot.IsValid(); err != nil { // TODO: needs to return error in v6
 		return nil, err
 	}
 
@@ -215,9 +204,9 @@ func (us SqlBotStore) Update(bot *model.Bot) (*model.Bot, *model.AppError) {
 	bot = oldBot
 
 	if count, err := us.GetMaster().Update(botFromModel(bot)); err != nil {
-		return nil, model.NewAppError("SqlBotStore.Update", "store.sql_bot.update.updating.app_error", bot.Trace(), err.Error(), http.StatusInternalServerError)
+		return nil, errors.Wrapf(err, "update: user_id=%s", bot.UserId)
 	} else if count != 1 {
-		return nil, model.NewAppError("SqlBotStore.Update", "store.sql_bot.update.app_error", traceBot(bot, map[string]interface{}{"count": count}), "", http.StatusInternalServerError)
+		return nil, fmt.Errorf("unexpected count while updating bot: count=%d, userId=%s", count, bot.UserId)
 	}
 
 	return bot, nil
@@ -225,10 +214,10 @@ func (us SqlBotStore) Update(bot *model.Bot) (*model.Bot, *model.AppError) {
 
 // PermanentDelete removes the bot from the database altogether.
 // If the corresponding user is to be deleted, it must be done via the user store.
-func (us SqlBotStore) PermanentDelete(botUserId string) *model.AppError {
+func (us SqlBotStore) PermanentDelete(botUserId string) error {
 	query := "DELETE FROM Bots WHERE UserId = :user_id"
 	if _, err := us.GetMaster().Exec(query, map[string]interface{}{"user_id": botUserId}); err != nil {
-		return model.NewAppError("SqlBotStore.Update", "store.sql_bot.delete.app_error", map[string]interface{}{"user_id": botUserId}, err.Error(), http.StatusBadRequest)
+		return store.NewErrInvalidInput("Bot", "UserId", botUserId)
 	}
 	return nil
 }