diff --git a/app/webhook.go b/app/webhook.go index 7283a8765e..0c815572e5 100644 --- 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 } diff --git a/i18n/en.json b/i18n/en.json index 073364f092..65383b5423 100644 --- 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." diff --git a/store/opentracing_layer.go b/store/opentracing_layer.go index 11c4307774..b2e38c27ba 100644 --- 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) diff --git a/store/sqlstore/command_webhook_store.go b/store/sqlstore/command_webhook_store.go index 184c3c5fe0..7eee1038e8 100644 --- 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 diff --git a/store/store.go b/store/store.go index 06e6bc0d80..b829a63661 100644 --- 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() } diff --git a/store/storetest/command_webhook_store.go b/store/storetest/command_webhook_store.go index e1b3a558c2..fc0fcdc2d5 100644 --- 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") } diff --git a/store/storetest/mocks/CommandWebhookStore.go b/store/storetest/mocks/CommandWebhookStore.go index 16742cc14c..87d9102517 100644 --- 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) } } diff --git a/store/timer_layer.go b/store/timer_layer.go index 710ff221ec..6ea4af21e3 100644 --- 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)