[MM-25477] - Migrate command webhook store AppError to error (#14703)

* Migrate command webhook store AppError to error * Migrate command webhook store AppError to error * Migrate command webhook store AppError to error * Migrate command webhook store AppError to error * Migrate command webhook store AppError to error * Migrate command webhook store AppError to error * Migrate command webhook store AppError to error * Changes requested in the review. * Changing http status * fix i18n Co-authored-by: Mattermod <mattermod@users.noreply.github.com> Co-authored-by: Agniva De Sarker <agnivade@yahoo.co.in>
2025-02-25 18:55:24 -06:00 · 2020-07-19 00:12:03 -03:00
parent c7f7bef9ec
commit aae3b9650f
8 changed files with 113 additions and 82 deletions
--- a/app/webhook.go
+++ b/app/webhook.go
@@ -711,17 +711,37 @@ func (a *App) CreateCommandWebhook(commandId string, args *model.CommandArgs) (*
 		ParentId:  args.ParentId,
 	}

-	return a.Srv().Store.CommandWebhook().Save(hook)
+	savedHook, err := a.Srv().Store.CommandWebhook().Save(hook)
+	if err != nil {
+		var invErr *store.ErrInvalidInput
+		var appErr *model.AppError
+		switch {
+		case errors.As(err, &invErr):
+			return nil, model.NewAppError("CreateCommandWebhook", "app.command_webhook.create_command_webhook.existing", nil, invErr.Error(), http.StatusBadRequest)
+		case errors.As(err, &appErr):
+			return nil, appErr
+		default:
+			return nil, model.NewAppError("CreateCommandWebhook", "app.command_webhook.create_command_webhook.internal_error", nil, err.Error(), http.StatusInternalServerError)
+		}
+
+	}
+	return savedHook, nil
 }

 func (a *App) HandleCommandWebhook(hookId string, response *model.CommandResponse) *model.AppError {
 	if response == nil {
-		return model.NewAppError("HandleCommandWebhook", "web.command_webhook.parse.app_error", nil, "", http.StatusBadRequest)
+		return model.NewAppError("HandleCommandWebhook", "app.command_webhook.handle_command_webhook.parse", nil, "", http.StatusBadRequest)
 	}

-	hook, err := a.Srv().Store.CommandWebhook().Get(hookId)
-	if err != nil {
-		return model.NewAppError("HandleCommandWebhook", "web.command_webhook.invalid.app_error", nil, "err="+err.Message, err.StatusCode)
+	hook, nErr := a.Srv().Store.CommandWebhook().Get(hookId)
+	if nErr != nil {
+		var nfErr *store.ErrNotFound
+		switch {
+		case errors.As(nErr, &nfErr):
+			return model.NewAppError("HandleCommandWebhook", "app.command_webhook.get.missing", map[string]interface{}{"hook_id": hookId}, nfErr.Error(), http.StatusNotFound)
+		default:
+			return model.NewAppError("HandleCommandWebhook", "app.command_webhook.get.internal_error", nil, nErr.Error(), http.StatusInternalServerError)
+		}
 	}

 	cmd, cmdErr := a.Srv().Store.Command().Get(hook.CommandId)
@@ -743,10 +763,16 @@ func (a *App) HandleCommandWebhook(hookId string, response *model.CommandRespons
 		ParentId:  hook.ParentId,
 	}

-	if err = a.Srv().Store.CommandWebhook().TryUse(hook.Id, 5); err != nil {
-		return model.NewAppError("HandleCommandWebhook", "web.command_webhook.invalid.app_error", nil, "err="+err.Message, err.StatusCode)
+	if nErr := a.Srv().Store.CommandWebhook().TryUse(hook.Id, 5); nErr != nil {
+		var invErr *store.ErrInvalidInput
+		switch {
+		case errors.As(nErr, &invErr):
+			return model.NewAppError("HandleCommandWebhook", "app.command_webhook.try_use.invalid", nil, invErr.Error(), http.StatusBadRequest)
+		default:
+			return model.NewAppError("HandleCommandWebhook", "app.command_webhook.try_use.internal_error", nil, nErr.Error(), http.StatusInternalServerError)
+		}
 	}

-	_, err = a.HandleCommandResponse(cmd, args, response, false)
+	_, err := a.HandleCommandResponse(cmd, args, response, false)
 	return err
 }
--- a/i18n/en.json
+++ b/i18n/en.json
@@ -3190,6 +3190,34 @@
    "id": "app.command.updatecommand.internal_error",
    "translation": "Unable to update the command."
  },
+  {
+    "id": "app.command_webhook.create_command_webhook.existing",
+    "translation": "You cannot update an existing CommandWebhook."
+  },
+  {
+    "id": "app.command_webhook.create_command_webhook.internal_error",
+    "translation": "Unable to save the CommandWebhook."
+  },
+  {
+    "id": "app.command_webhook.get.internal_error",
+    "translation": "Unable to get the webhook."
+  },
+  {
+    "id": "app.command_webhook.get.missing",
+    "translation": "Unable to find the webhook."
+  },
+  {
+    "id": "app.command_webhook.handle_command_webhook.parse",
+    "translation": "Unable to parse incoming data."
+  },
+  {
+    "id": "app.command_webhook.try_use.internal_error",
+    "translation": "Unable to use the webhook."
+  },
+  {
+    "id": "app.command_webhook.try_use.invalid",
+    "translation": "Invalid webhook."
+  },
  {
    "id": "app.emoji.create.internal_error",
    "translation": "Unable to save emoji."
@@ -6642,26 +6670,6 @@
    "id": "store.sql_command.update.missing.app_error",
    "translation": "Command does not exist."
  },
-  {
-    "id": "store.sql_command_webhooks.get.app_error",
-    "translation": "Unable to get the webhook."
-  },
-  {
-    "id": "store.sql_command_webhooks.save.app_error",
-    "translation": "Unable to save the CommandWebhook."
-  },
-  {
-    "id": "store.sql_command_webhooks.save.existing.app_error",
-    "translation": "You cannot update an existing CommandWebhook."
-  },
-  {
-    "id": "store.sql_command_webhooks.try_use.app_error",
-    "translation": "Unable to use the webhook."
-  },
-  {
-    "id": "store.sql_command_webhooks.try_use.invalid.app_error",
-    "translation": "Invalid webhook."
-  },
  {
    "id": "store.sql_compliance.get.finding.app_error",
    "translation": "We encountered an error retrieving the compliance reports."
@@ -7738,10 +7746,6 @@
    "id": "web.command_webhook.command.app_error",
    "translation": "Couldn't find the command."
  },
-  {
-    "id": "web.command_webhook.invalid.app_error",
-    "translation": "Invalid webhook."
-  },
  {
    "id": "web.command_webhook.parse.app_error",
    "translation": "Unable to parse incoming data."
--- a/store/opentracing_layer.go
+++ b/store/opentracing_layer.go
@@ -2596,7 +2596,7 @@ func (s *OpenTracingLayerCommandWebhookStore) Cleanup() {

 }

-func (s *OpenTracingLayerCommandWebhookStore) Get(id string) (*model.CommandWebhook, *model.AppError) {
+func (s *OpenTracingLayerCommandWebhookStore) Get(id string) (*model.CommandWebhook, error) {
 	origCtx := s.Root.Store.Context()
 	span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "CommandWebhookStore.Get")
 	s.Root.Store.SetContext(newCtx)
@@ -2614,7 +2614,7 @@ func (s *OpenTracingLayerCommandWebhookStore) Get(id string) (*model.CommandWebh
 	return resultVar0, resultVar1
 }

-func (s *OpenTracingLayerCommandWebhookStore) Save(webhook *model.CommandWebhook) (*model.CommandWebhook, *model.AppError) {
+func (s *OpenTracingLayerCommandWebhookStore) Save(webhook *model.CommandWebhook) (*model.CommandWebhook, error) {
 	origCtx := s.Root.Store.Context()
 	span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "CommandWebhookStore.Save")
 	s.Root.Store.SetContext(newCtx)
@@ -2632,7 +2632,7 @@ func (s *OpenTracingLayerCommandWebhookStore) Save(webhook *model.CommandWebhook
 	return resultVar0, resultVar1
 }

-func (s *OpenTracingLayerCommandWebhookStore) TryUse(id string, limit int) *model.AppError {
+func (s *OpenTracingLayerCommandWebhookStore) TryUse(id string, limit int) error {
 	origCtx := s.Root.Store.Context()
 	span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "CommandWebhookStore.TryUse")
 	s.Root.Store.SetContext(newCtx)
--- a/store/sqlstore/command_webhook_store.go
+++ b/store/sqlstore/command_webhook_store.go
@@ -5,11 +5,12 @@ package sqlstore

 import (
 	"database/sql"
-	"net/http"

 	"github.com/mattermost/mattermost-server/v5/mlog"
 	"github.com/mattermost/mattermost-server/v5/model"
 	"github.com/mattermost/mattermost-server/v5/store"
+
+	"github.com/pkg/errors"
 )

 type SqlCommandWebhookStore struct {
@@ -36,9 +37,9 @@ func (s SqlCommandWebhookStore) createIndexesIfNotExists() {
 	s.CreateIndexIfNotExists("idx_command_webhook_create_at", "CommandWebhooks", "CreateAt")
 }

-func (s SqlCommandWebhookStore) Save(webhook *model.CommandWebhook) (*model.CommandWebhook, *model.AppError) {
+func (s SqlCommandWebhookStore) Save(webhook *model.CommandWebhook) (*model.CommandWebhook, error) {
 	if len(webhook.Id) > 0 {
-		return nil, model.NewAppError("SqlCommandWebhookStore.Save", "store.sql_command_webhooks.save.existing.app_error", nil, "id="+webhook.Id, http.StatusBadRequest)
+		return nil, store.NewErrInvalidInput("CommandWebhook", "id", webhook.Id)
 	}

 	webhook.PreSave()
@@ -47,33 +48,31 @@ func (s SqlCommandWebhookStore) Save(webhook *model.CommandWebhook) (*model.Comm
 	}

 	if err := s.GetMaster().Insert(webhook); err != nil {
-		return nil, model.NewAppError("SqlCommandWebhookStore.Save", "store.sql_command_webhooks.save.app_error", nil, "id="+webhook.Id+", "+err.Error(), http.StatusInternalServerError)
+		return nil, errors.Wrapf(err, "save: id=%s", webhook.Id)
 	}

 	return webhook, nil
 }

-func (s SqlCommandWebhookStore) Get(id string) (*model.CommandWebhook, *model.AppError) {
+func (s SqlCommandWebhookStore) Get(id string) (*model.CommandWebhook, error) {
 	var webhook model.CommandWebhook

 	exptime := model.GetMillis() - model.COMMAND_WEBHOOK_LIFETIME
-	var appErr *model.AppError
 	if err := s.GetReplica().SelectOne(&webhook, "SELECT * FROM CommandWebhooks WHERE Id = :Id AND CreateAt > :ExpTime", map[string]interface{}{"Id": id, "ExpTime": exptime}); err != nil {
-		appErr = model.NewAppError("SqlCommandWebhookStore.Get", "store.sql_command_webhooks.get.app_error", nil, "id="+id+", err="+err.Error(), http.StatusInternalServerError)
 		if err == sql.ErrNoRows {
-			appErr.StatusCode = http.StatusNotFound
+			return nil, store.NewErrNotFound("CommandWebhook", id)
 		}
-		return nil, appErr
+		return nil, errors.Wrapf(err, "get: id=%s", id)
 	}

 	return &webhook, nil
 }

-func (s SqlCommandWebhookStore) TryUse(id string, limit int) *model.AppError {
+func (s SqlCommandWebhookStore) TryUse(id string, limit int) error {
 	if sqlResult, err := s.GetMaster().Exec("UPDATE CommandWebhooks SET UseCount = UseCount + 1 WHERE Id = :Id AND UseCount < :UseLimit", map[string]interface{}{"Id": id, "UseLimit": limit}); err != nil {
-		return model.NewAppError("SqlCommandWebhookStore.TryUse", "store.sql_command_webhooks.try_use.app_error", nil, "id="+id+", err="+err.Error(), http.StatusInternalServerError)
+		return errors.Wrapf(err, "tryuse: id=%s limit=%d", id, limit)
 	} else if rows, _ := sqlResult.RowsAffected(); rows == 0 {
-		return model.NewAppError("SqlCommandWebhookStore.TryUse", "store.sql_command_webhooks.try_use.invalid.app_error", nil, "id="+id, http.StatusBadRequest)
+		return store.NewErrInvalidInput("CommandWebhook", "id", id)
 	}

 	return nil
--- a/store/store.go
+++ b/store/store.go
@@ -488,9 +488,9 @@ type CommandStore interface {
 }

 type CommandWebhookStore interface {
-	Save(webhook *model.CommandWebhook) (*model.CommandWebhook, *model.AppError)
-	Get(id string) (*model.CommandWebhook, *model.AppError)
-	TryUse(id string, limit int) *model.AppError
+	Save(webhook *model.CommandWebhook) (*model.CommandWebhook, error)
+	Get(id string) (*model.CommandWebhook, error)
+	TryUse(id string, limit int) error
 	Cleanup()
 }

--- a/store/storetest/command_webhook_store.go
+++ b/store/storetest/command_webhook_store.go
@@ -4,7 +4,7 @@
 package storetest

 import (
-	"net/http"
+	"errors"
 	"testing"

 	"github.com/mattermost/mattermost-server/v5/model"
@@ -28,12 +28,13 @@ func testCommandWebhookStore(t *testing.T, ss store.Store) {
 	require.Nil(t, err)

 	var r1 *model.CommandWebhook
-	r1, err = cws.Get(h1.Id)
-	require.Nil(t, err)
+	r1, nErr := cws.Get(h1.Id)
+	require.Nil(t, nErr)
 	assert.Equal(t, *r1, *h1, "invalid returned webhook")

-	_, err = cws.Get("123")
-	assert.Equal(t, err.StatusCode, http.StatusNotFound, "Should have set the status as not found for missing id")
+	_, nErr = cws.Get("123")
+	var nfErr *store.ErrNotFound
+	require.True(t, errors.As(nErr, &nfErr), "Should have set the status as not found for missing id")

 	h2 := &model.CommandWebhook{}
 	h2.CreateAt = model.GetMillis() - 2*model.COMMAND_WEBHOOK_LIFETIME
@@ -43,22 +44,23 @@ func testCommandWebhookStore(t *testing.T, ss store.Store) {
 	h2, err = cws.Save(h2)
 	require.Nil(t, err)

-	_, err = cws.Get(h2.Id)
-	require.NotNil(t, err, "Should have set the status as not found for expired webhook")
-	assert.Equal(t, err.StatusCode, http.StatusNotFound, "Should have set the status as not found for expired webhook")
+	_, nErr = cws.Get(h2.Id)
+	require.NotNil(t, nErr, "Should have set the status as not found for expired webhook")
+	require.True(t, errors.As(nErr, &nfErr), "Should have set the status as not found for expired webhook")

 	cws.Cleanup()

-	_, err = cws.Get(h1.Id)
-	require.Nil(t, err, "Should have no error getting unexpired webhook")
+	_, nErr = cws.Get(h1.Id)
+	require.Nil(t, nErr, "Should have no error getting unexpired webhook")

-	_, err = cws.Get(h2.Id)
-	assert.Equal(t, err.StatusCode, http.StatusNotFound, "Should have set the status as not found for expired webhook")
+	_, nErr = cws.Get(h2.Id)
+	require.True(t, errors.As(nErr, &nfErr), "Should have set the status as not found for expired webhook")

-	err = cws.TryUse(h1.Id, 1)
-	require.Nil(t, err, "Should be able to use webhook once")
+	nErr = cws.TryUse(h1.Id, 1)
+	require.Nil(t, nErr, "Should be able to use webhook once")

-	err = cws.TryUse(h1.Id, 1)
-	require.NotNil(t, err, "Should be able to use webhook once")
-	assert.Equal(t, err.StatusCode, http.StatusBadRequest, "Should be able to use webhook once")
+	nErr = cws.TryUse(h1.Id, 1)
+	require.NotNil(t, nErr, "Should be able to use webhook once")
+	var invErr *store.ErrInvalidInput
+	require.True(t, errors.As(nErr, &invErr), "Should be able to use webhook once")
 }
--- a/store/storetest/mocks/CommandWebhookStore.go
+++ b/store/storetest/mocks/CommandWebhookStore.go
@@ -20,7 +20,7 @@ func (_m *CommandWebhookStore) Cleanup() {
 }

 // Get provides a mock function with given fields: id
-func (_m *CommandWebhookStore) Get(id string) (*model.CommandWebhook, *model.AppError) {
+func (_m *CommandWebhookStore) Get(id string) (*model.CommandWebhook, error) {
 	ret := _m.Called(id)

 	var r0 *model.CommandWebhook
@@ -32,12 +32,12 @@ func (_m *CommandWebhookStore) Get(id string) (*model.CommandWebhook, *model.App
 		}
 	}

-	var r1 *model.AppError
-	if rf, ok := ret.Get(1).(func(string) *model.AppError); ok {
+	var r1 error
+	if rf, ok := ret.Get(1).(func(string) error); ok {
 		r1 = rf(id)
 	} else {
 		if ret.Get(1) != nil {
-			r1 = ret.Get(1).(*model.AppError)
+			r1 = ret.Get(1).(error)
 		}
 	}

@@ -45,7 +45,7 @@ func (_m *CommandWebhookStore) Get(id string) (*model.CommandWebhook, *model.App
 }

 // Save provides a mock function with given fields: webhook
-func (_m *CommandWebhookStore) Save(webhook *model.CommandWebhook) (*model.CommandWebhook, *model.AppError) {
+func (_m *CommandWebhookStore) Save(webhook *model.CommandWebhook) (*model.CommandWebhook, error) {
 	ret := _m.Called(webhook)

 	var r0 *model.CommandWebhook
@@ -57,12 +57,12 @@ func (_m *CommandWebhookStore) Save(webhook *model.CommandWebhook) (*model.Comma
 		}
 	}

-	var r1 *model.AppError
-	if rf, ok := ret.Get(1).(func(*model.CommandWebhook) *model.AppError); ok {
+	var r1 error
+	if rf, ok := ret.Get(1).(func(*model.CommandWebhook) error); ok {
 		r1 = rf(webhook)
 	} else {
 		if ret.Get(1) != nil {
-			r1 = ret.Get(1).(*model.AppError)
+			r1 = ret.Get(1).(error)
 		}
 	}

@@ -70,15 +70,15 @@ func (_m *CommandWebhookStore) Save(webhook *model.CommandWebhook) (*model.Comma
 }

 // TryUse provides a mock function with given fields: id, limit
-func (_m *CommandWebhookStore) TryUse(id string, limit int) *model.AppError {
+func (_m *CommandWebhookStore) TryUse(id string, limit int) error {
 	ret := _m.Called(id, limit)

-	var r0 *model.AppError
-	if rf, ok := ret.Get(0).(func(string, int) *model.AppError); ok {
+	var r0 error
+	if rf, ok := ret.Get(0).(func(string, int) error); ok {
 		r0 = rf(id, limit)
 	} else {
 		if ret.Get(0) != nil {
-			r0 = ret.Get(0).(*model.AppError)
+			r0 = ret.Get(0).(error)
 		}
 	}

--- a/store/timer_layer.go
+++ b/store/timer_layer.go
@@ -2384,7 +2384,7 @@ func (s *TimerLayerCommandWebhookStore) Cleanup() {
 	}
 }

-func (s *TimerLayerCommandWebhookStore) Get(id string) (*model.CommandWebhook, *model.AppError) {
+func (s *TimerLayerCommandWebhookStore) Get(id string) (*model.CommandWebhook, error) {
 	start := timemodule.Now()

 	resultVar0, resultVar1 := s.CommandWebhookStore.Get(id)
@@ -2400,7 +2400,7 @@ func (s *TimerLayerCommandWebhookStore) Get(id string) (*model.CommandWebhook, *
 	return resultVar0, resultVar1
 }

-func (s *TimerLayerCommandWebhookStore) Save(webhook *model.CommandWebhook) (*model.CommandWebhook, *model.AppError) {
+func (s *TimerLayerCommandWebhookStore) Save(webhook *model.CommandWebhook) (*model.CommandWebhook, error) {
 	start := timemodule.Now()

 	resultVar0, resultVar1 := s.CommandWebhookStore.Save(webhook)
@@ -2416,7 +2416,7 @@ func (s *TimerLayerCommandWebhookStore) Save(webhook *model.CommandWebhook) (*mo
 	return resultVar0, resultVar1
 }

-func (s *TimerLayerCommandWebhookStore) TryUse(id string, limit int) *model.AppError {
+func (s *TimerLayerCommandWebhookStore) TryUse(id string, limit int) error {
 	start := timemodule.Now()

 	resultVar0 := s.CommandWebhookStore.TryUse(id, limit)