Migration of AuditStore to return plain errors (#14825)

Automatic Merge

app/audit.go

@@ -4,12 +4,15 @@
 package app
 
 import (
+	"errors"
 	"fmt"
+	"net/http"
 	"os/user"
 
 	"github.com/mattermost/mattermost-server/v5/audit"
 	"github.com/mattermost/mattermost-server/v5/mlog"
 	"github.com/mattermost/mattermost-server/v5/model"
+	"github.com/mattermost/mattermost-server/v5/store"
 )
 
 const (
@@ -27,11 +30,31 @@ var (
 )
 
 func (a *App) GetAudits(userId string, limit int) (model.Audits, *model.AppError) {
-	return a.Srv().Store.Audit().Get(userId, 0, limit)
+	audits, err := a.Srv().Store.Audit().Get(userId, 0, limit)
+	if err != nil {
+		var outErr *store.ErrOutOfBounds
+		switch {
+		case errors.As(err, &outErr):
+			return nil, model.NewAppError("GetAudits", "app.audit.get.limit.app_error", nil, err.Error(), http.StatusBadRequest)
+		default:
+			return nil, model.NewAppError("GetAudits", "app.audit.get.finding.app_error", nil, err.Error(), http.StatusInternalServerError)
+		}
+	}
+	return audits, nil
 }
 
 func (a *App) GetAuditsPage(userId string, page int, perPage int) (model.Audits, *model.AppError) {
-	return a.Srv().Store.Audit().Get(userId, page*perPage, perPage)
+	audits, err := a.Srv().Store.Audit().Get(userId, page*perPage, perPage)
+	if err != nil {
+		var outErr *store.ErrOutOfBounds
+		switch {
+		case errors.As(err, &outErr):
+			return nil, model.NewAppError("GetAuditsPage", "app.audit.get.limit.app_error", nil, err.Error(), http.StatusBadRequest)
+		default:
+			return nil, model.NewAppError("GetAuditsPage", "app.audit.get.finding.app_error", nil, err.Error(), http.StatusInternalServerError)
+		}
+	}
+	return audits, nil
 }
 
 // LogAuditRec logs an audit record using default CLILevel.

app/channel.go

@@ -536,10 +536,10 @@ func (a *App) UpdateChannel(channel *model.Channel) (*model.Channel, *model.AppE
 	_, err := a.Srv().Store.Channel().Update(channel)
 	if err != nil {
 		var appErr *model.AppError
-		var iErr *store.ErrInvalidInput
+		var invErr *store.ErrInvalidInput
 		switch {
-		case errors.As(err, &iErr):
+		case errors.As(err, &invErr):
-			return nil, model.NewAppError("UpdateChannel", "app.channel.update.bad_id", nil, iErr.Error(), http.StatusBadRequest)
+			return nil, model.NewAppError("UpdateChannel", "app.channel.update.bad_id", nil, invErr.Error(), http.StatusBadRequest)
 		case errors.As(err, &appErr):
 			return nil, appErr
 		default:
@@ -2382,10 +2382,10 @@ func (a *App) MoveChannel(team *model.Team, channel *model.Channel, user *model.
 	channel.TeamId = team.Id
 	if _, err := a.Srv().Store.Channel().Update(channel); err != nil {
 		var appErr *model.AppError
-		var iErr *store.ErrInvalidInput
+		var invErr *store.ErrInvalidInput
 		switch {
-		case errors.As(err, &iErr):
+		case errors.As(err, &invErr):
-			return model.NewAppError("MoveChannel", "app.channel.update.bad_id", nil, iErr.Error(), http.StatusBadRequest)
+			return model.NewAppError("MoveChannel", "app.channel.update.bad_id", nil, invErr.Error(), http.StatusBadRequest)
 		case errors.As(err, &appErr):
 			return appErr
 		default:

app/terms_of_service.go

@@ -23,10 +23,10 @@ func (a *App) CreateTermsOfService(text, userId string) (*model.TermsOfService,
 
 	var err error
 	if termsOfService, err = a.Srv().Store.TermsOfService().Save(termsOfService); err != nil {
-		var iErr *store.ErrInvalidInput
+		var invErr *store.ErrInvalidInput
 		var appErr *model.AppError
 		switch {
-		case errors.As(err, &iErr):
+		case errors.As(err, &invErr):
 			return nil, model.NewAppError("CreateTermsOfService", "app.terms_of_service.create.existing.app_error", nil, "id="+termsOfService.Id, http.StatusBadRequest)
 		case errors.As(err, &appErr):
 			return nil, appErr

app/user.go

@@ -1533,7 +1533,7 @@ func (a *App) PermanentDeleteUser(user *model.User) *model.AppError {
 	}
 
 	if err := a.Srv().Store.Audit().PermanentDeleteByUser(user.Id); err != nil {
-		return err
+		return model.NewAppError("PermanentDeleteUser", "app.audit.permanent_delete_by_user.app_error", nil, err.Error(), http.StatusInternalServerError)
 	}
 
 	if err := a.Srv().Store.Team().RemoveAllMembersByUser(user.Id); err != nil {

i18n/en.json

@@ -2962,6 +2962,22 @@
     "id": "app.admin.test_site_url.failure",
     "translation": "This is not a valid live URL"
   },
+  {
+    "id": "app.audit.get.finding.app_error",
+    "translation": "We encountered an error finding the audits."
+  },
+  {
+    "id": "app.audit.get.limit.app_error",
+    "translation": "Limit exceeded for paging."
+  },
+  {
+    "id": "app.audit.permanent_delete_by_user.app_error",
+    "translation": "We encountered an error deleting the audits."
+  },
+  {
+    "id": "app.audit.save.saving.app_error",
+    "translation": "We encountered an error saving the audit."
+  },
   {
     "id": "app.bot.createbot.internal_error",
     "translation": "Unable to save the bot."
@@ -6174,26 +6190,6 @@
     "id": "store.sql.convert_string_map",
     "translation": "FromDb: Unable to convert StringMap to *string"
   },
-  {
-    "id": "store.sql_audit.app_error",
-    "translation": "Failed to build query."
-  },
-  {
-    "id": "store.sql_audit.get.finding.app_error",
-    "translation": "We encountered an error finding the audits."
-  },
-  {
-    "id": "store.sql_audit.get.limit.app_error",
-    "translation": "Limit exceeded for paging."
-  },
-  {
-    "id": "store.sql_audit.permanent_delete_by_user.app_error",
-    "translation": "We encountered an error deleting the audits."
-  },
-  {
-    "id": "store.sql_audit.save.saving.app_error",
-    "translation": "We encountered an error saving the audit."
-  },
   {
     "id": "store.sql_bot.get.missing.app_error",
     "translation": "Bot does not exist."

store/errors.go

@@ -88,3 +88,17 @@ func NewErrNotFound(resource, id string) *ErrNotFound {
 func (e *ErrNotFound) Error() string {
 	return "resource: " + e.resource + " id: " + e.Id
 }
+
+// ErrOutOfBounds indicates that the requested total numbers of rows
+// was greater than the allowed limit.
+type ErrOutOfBounds struct {
+	value int
+}
+
+func (e *ErrOutOfBounds) Error() string {
+	return fmt.Sprintf("invalid limit parameter: %d", e.value)
+}
+
+func NewErrOutOfBounds(value int) *ErrOutOfBounds {
+	return &ErrOutOfBounds{value: value}
+}

store/opentracing_layer.go

@@ -329,7 +329,7 @@ type OpenTracingLayerWebhookStore struct {
 	Root *OpenTracingLayer
 }
 
-func (s *OpenTracingLayerAuditStore) Get(user_id string, offset int, limit int) (model.Audits, *model.AppError) {
+func (s *OpenTracingLayerAuditStore) Get(user_id string, offset int, limit int) (model.Audits, error) {
 	origCtx := s.Root.Store.Context()
 	span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "AuditStore.Get")
 	s.Root.Store.SetContext(newCtx)
@@ -347,7 +347,7 @@ func (s *OpenTracingLayerAuditStore) Get(user_id string, offset int, limit int)
 	return resultVar0, resultVar1
 }
 
-func (s *OpenTracingLayerAuditStore) PermanentDeleteByUser(userId string) *model.AppError {
+func (s *OpenTracingLayerAuditStore) PermanentDeleteByUser(userId string) error {
 	origCtx := s.Root.Store.Context()
 	span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "AuditStore.PermanentDeleteByUser")
 	s.Root.Store.SetContext(newCtx)
@@ -365,7 +365,7 @@ func (s *OpenTracingLayerAuditStore) PermanentDeleteByUser(userId string) *model
 	return resultVar0
 }
 
-func (s *OpenTracingLayerAuditStore) Save(audit *model.Audit) *model.AppError {
+func (s *OpenTracingLayerAuditStore) Save(audit *model.Audit) error {
 	origCtx := s.Root.Store.Context()
 	span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "AuditStore.Save")
 	s.Root.Store.SetContext(newCtx)

store/sqlstore/audit_store.go

@@ -4,9 +4,8 @@
 package sqlstore
 
 import (
-	"net/http"
-
 	sq "github.com/Masterminds/squirrel"
+	"github.com/pkg/errors"
 
 	"github.com/mattermost/mattermost-server/v5/model"
 	"github.com/mattermost/mattermost-server/v5/store"
@@ -36,19 +35,19 @@ func (s SqlAuditStore) createIndexesIfNotExists() {
 	s.CreateIndexIfNotExists("idx_audits_user_id", "Audits", "UserId")
 }
 
-func (s SqlAuditStore) Save(audit *model.Audit) *model.AppError {
+func (s SqlAuditStore) Save(audit *model.Audit) error {
 	audit.Id = model.NewId()
 	audit.CreateAt = model.GetMillis()
 
 	if err := s.GetMaster().Insert(audit); err != nil {
-		return model.NewAppError("SqlAuditStore.Save", "store.sql_audit.save.saving.app_error", nil, "user_id="+audit.UserId+" action="+audit.Action, http.StatusInternalServerError)
+		return errors.Wrapf(err, "failed to save Audit with userId=%s and action=%s", audit.UserId, audit.Action)
 	}
 	return nil
 }
 
-func (s SqlAuditStore) Get(user_id string, offset int, limit int) (model.Audits, *model.AppError) {
+func (s SqlAuditStore) Get(userId string, offset int, limit int) (model.Audits, error) {
 	if limit > 1000 {
-		return nil, model.NewAppError("SqlAuditStore.Get", "store.sql_audit.get.limit.app_error", nil, "user_id="+user_id, http.StatusBadRequest)
+		return nil, store.NewErrOutOfBounds(limit)
 	}
 
 	query := s.getQueryBuilder().
@@ -58,26 +57,26 @@ func (s SqlAuditStore) Get(user_id string, offset int, limit int) (model.Audits,
 		Limit(uint64(limit)).
 		Offset(uint64(offset))
 
-	if len(user_id) != 0 {
+	if len(userId) != 0 {
-		query = query.Where(sq.Eq{"UserId": user_id})
+		query = query.Where(sq.Eq{"UserId": userId})
 	}
 
 	queryString, args, err := query.ToSql()
 	if err != nil {
-		return nil, model.NewAppError("SqlAuditStore.Get", "store.sql_audit.app_error", nil, err.Error(), http.StatusInternalServerError)
+		return nil, errors.Wrap(err, "audits_tosql")
 	}
 
 	var audits model.Audits
 	if _, err := s.GetReplica().Select(&audits, queryString, args...); err != nil {
-		return nil, model.NewAppError("SqlAuditStore.Get", "store.sql_audit.get.finding.app_error", nil, "user_id="+user_id, http.StatusInternalServerError)
+		return nil, errors.Wrapf(err, "failed to get Audit list for userId=%s", userId)
 	}
 	return audits, nil
 }
 
-func (s SqlAuditStore) PermanentDeleteByUser(userId string) *model.AppError {
+func (s SqlAuditStore) PermanentDeleteByUser(userId string) error {
 	if _, err := s.GetMaster().Exec("DELETE FROM Audits WHERE UserId = :userId",
 		map[string]interface{}{"userId": userId}); err != nil {
-		return model.NewAppError("SqlAuditStore.Delete", "store.sql_audit.permanent_delete_by_user.app_error", nil, "user_id="+userId, http.StatusInternalServerError)
+		return errors.Wrapf(err, "failed to delete Audit with userId=%s", userId)
 	}
 	return nil
 }

store/store.go

@@ -374,9 +374,9 @@ type SessionStore interface {
 }
 
 type AuditStore interface {
-	Save(audit *model.Audit) *model.AppError
+	Save(audit *model.Audit) error
-	Get(user_id string, offset int, limit int) (model.Audits, *model.AppError)
+	Get(user_id string, offset int, limit int) (model.Audits, error)
-	PermanentDeleteByUser(userId string) *model.AppError
+	PermanentDeleteByUser(userId string) error
 }
 
 type ClusterDiscoveryStore interface {

store/storetest/mocks/AuditStore.go

@@ -15,7 +15,7 @@ type AuditStore struct {
 }
 
 // Get provides a mock function with given fields: user_id, offset, limit
-func (_m *AuditStore) Get(user_id string, offset int, limit int) (model.Audits, *model.AppError) {
+func (_m *AuditStore) Get(user_id string, offset int, limit int) (model.Audits, error) {
 	ret := _m.Called(user_id, offset, limit)
 
 	var r0 model.Audits
@@ -27,45 +27,39 @@ func (_m *AuditStore) Get(user_id string, offset int, limit int) (model.Audits,
 		}
 	}
 
-	var r1 *model.AppError
+	var r1 error
-	if rf, ok := ret.Get(1).(func(string, int, int) *model.AppError); ok {
+	if rf, ok := ret.Get(1).(func(string, int, int) error); ok {
 		r1 = rf(user_id, offset, limit)
 	} else {
-		if ret.Get(1) != nil {
+		r1 = ret.Error(1)
-			r1 = ret.Get(1).(*model.AppError)
-		}
 	}
 
 	return r0, r1
 }
 
 // PermanentDeleteByUser provides a mock function with given fields: userId
-func (_m *AuditStore) PermanentDeleteByUser(userId string) *model.AppError {
+func (_m *AuditStore) PermanentDeleteByUser(userId string) error {
 	ret := _m.Called(userId)
 
-	var r0 *model.AppError
+	var r0 error
-	if rf, ok := ret.Get(0).(func(string) *model.AppError); ok {
+	if rf, ok := ret.Get(0).(func(string) error); ok {
 		r0 = rf(userId)
 	} else {
-		if ret.Get(0) != nil {
+		r0 = ret.Error(0)
-			r0 = ret.Get(0).(*model.AppError)
-		}
 	}
 
 	return r0
 }
 
 // Save provides a mock function with given fields: audit
-func (_m *AuditStore) Save(audit *model.Audit) *model.AppError {
+func (_m *AuditStore) Save(audit *model.Audit) error {
 	ret := _m.Called(audit)
 
-	var r0 *model.AppError
+	var r0 error
-	if rf, ok := ret.Get(0).(func(*model.Audit) *model.AppError); ok {
+	if rf, ok := ret.Get(0).(func(*model.Audit) error); ok {
 		r0 = rf(audit)
 	} else {
-		if ret.Get(0) != nil {
+		r0 = ret.Error(0)
-			r0 = ret.Get(0).(*model.AppError)
-		}
 	}
 
 	return r0

store/timer_layer.go

@@ -329,7 +329,7 @@ type TimerLayerWebhookStore struct {
 	Root *TimerLayer
 }
 
-func (s *TimerLayerAuditStore) Get(user_id string, offset int, limit int) (model.Audits, *model.AppError) {
+func (s *TimerLayerAuditStore) Get(user_id string, offset int, limit int) (model.Audits, error) {
 	start := timemodule.Now()
 
 	resultVar0, resultVar1 := s.AuditStore.Get(user_id, offset, limit)
@@ -345,7 +345,7 @@ func (s *TimerLayerAuditStore) Get(user_id string, offset int, limit int) (model
 	return resultVar0, resultVar1
 }
 
-func (s *TimerLayerAuditStore) PermanentDeleteByUser(userId string) *model.AppError {
+func (s *TimerLayerAuditStore) PermanentDeleteByUser(userId string) error {
 	start := timemodule.Now()
 
 	resultVar0 := s.AuditStore.PermanentDeleteByUser(userId)
@@ -361,7 +361,7 @@ func (s *TimerLayerAuditStore) PermanentDeleteByUser(userId string) *model.AppEr
 	return resultVar0
 }
 
-func (s *TimerLayerAuditStore) Save(audit *model.Audit) *model.AppError {
+func (s *TimerLayerAuditStore) Save(audit *model.Audit) error {
 	start := timemodule.Now()
 
 	resultVar0 := s.AuditStore.Save(audit)

web/context.go

@@ -65,7 +65,8 @@ func (c *Context) MakeAuditRecord(event string, initialStatus string) *audit.Rec
 func (c *Context) LogAudit(extraInfo string) {
 	audit := &model.Audit{UserId: c.App.Session().UserId, IpAddress: c.App.IpAddress(), Action: c.App.Path(), ExtraInfo: extraInfo, SessionId: c.App.Session().Id}
 	if err := c.App.Srv().Store.Audit().Save(audit); err != nil {
-		c.LogError(err)
+		appErr := model.NewAppError("LogAudit", "app.audit.save.saving.app_error", nil, err.Error(), http.StatusInternalServerError)
+		c.LogError(appErr)
 	}
 }
 
@@ -77,7 +78,8 @@ func (c *Context) LogAuditWithUserId(userId, extraInfo string) {
 
 	audit := &model.Audit{UserId: userId, IpAddress: c.App.IpAddress(), Action: c.App.Path(), ExtraInfo: extraInfo, SessionId: c.App.Session().Id}
 	if err := c.App.Srv().Store.Audit().Save(audit); err != nil {
-		c.LogError(err)
+		appErr := model.NewAppError("LogAuditWithUserId", "app.audit.save.saving.app_error", nil, err.Error(), http.StatusInternalServerError)
+		c.LogError(appErr)
 	}
 }