diff --git a/app/webhook.go b/app/webhook.go index 69be77cd06..3938fd1f6c 100644 --- a/app/webhook.go +++ b/app/webhook.go @@ -380,11 +380,7 @@ func (a *App) GetIncomingWebhook(hookId string) (*model.IncomingWebhook, *model. return nil, model.NewAppError("GetIncomingWebhook", "api.incoming_webhook.disabled.app_error", nil, "", http.StatusNotImplemented) } - if result := <-a.Srv.Store.Webhook().GetIncoming(hookId, true); result.Err != nil { - return nil, result.Err - } else { - return result.Data.(*model.IncomingWebhook), nil - } + return a.Srv.Store.Webhook().GetIncoming(hookId, true) } func (a *App) GetIncomingWebhooksForTeamPage(teamId string, page, perPage int) ([]*model.IncomingWebhook, *model.AppError) { @@ -589,7 +585,12 @@ func (a *App) HandleIncomingWebhook(hookId string, req *model.IncomingWebhookReq return model.NewAppError("HandleIncomingWebhook", "web.incoming_webhook.disabled.app_error", nil, "", http.StatusNotImplemented) } - hchan := a.Srv.Store.Webhook().GetIncoming(hookId, true) + hchan := make(chan store.StoreResult, 1) + go func() { + webhook, err := a.Srv.Store.Webhook().GetIncoming(hookId, true) + hchan <- store.StoreResult{Data: webhook, Err: err} + close(hchan) + }() if req == nil { return model.NewAppError("HandleIncomingWebhook", "web.incoming_webhook.parse.app_error", nil, "", http.StatusBadRequest) diff --git a/store/sqlstore/webhook_store.go b/store/sqlstore/webhook_store.go index 94eadf836f..c4ad26eb12 100644 --- a/store/sqlstore/webhook_store.go +++ b/store/sqlstore/webhook_store.go @@ -121,38 +121,30 @@ func (s SqlWebhookStore) UpdateIncoming(hook *model.IncomingWebhook) store.Store }) } -func (s SqlWebhookStore) GetIncoming(id string, allowFromCache bool) store.StoreChannel { - return store.Do(func(result *store.StoreResult) { - if allowFromCache { - if cacheItem, ok := webhookCache.Get(id); ok { - if s.metrics != nil { - s.metrics.IncrementMemCacheHitCounter("Webhook") - } - result.Data = cacheItem.(*model.IncomingWebhook) - return - } else { - if s.metrics != nil { - s.metrics.IncrementMemCacheMissCounter("Webhook") - } +func (s SqlWebhookStore) GetIncoming(id string, allowFromCache bool) (*model.IncomingWebhook, *model.AppError) { + if allowFromCache { + if cacheItem, ok := webhookCache.Get(id); ok { + if s.metrics != nil { + s.metrics.IncrementMemCacheHitCounter("Webhook") } + return cacheItem.(*model.IncomingWebhook), nil } - - var webhook model.IncomingWebhook - - if err := s.GetReplica().SelectOne(&webhook, "SELECT * FROM IncomingWebhooks WHERE Id = :Id AND DeleteAt = 0", map[string]interface{}{"Id": id}); err != nil { - if err == sql.ErrNoRows { - result.Err = model.NewAppError("SqlWebhookStore.GetIncoming", "store.sql_webhooks.get_incoming.app_error", nil, "id="+id+", err="+err.Error(), http.StatusNotFound) - } else { - result.Err = model.NewAppError("SqlWebhookStore.GetIncoming", "store.sql_webhooks.get_incoming.app_error", nil, "id="+id+", err="+err.Error(), http.StatusInternalServerError) - } + if s.metrics != nil { + s.metrics.IncrementMemCacheMissCounter("Webhook") } + } - if result.Err == nil { - webhookCache.AddWithExpiresInSecs(id, &webhook, WEBHOOK_CACHE_SEC) + var webhook model.IncomingWebhook + if err := s.GetReplica().SelectOne(&webhook, "SELECT * FROM IncomingWebhooks WHERE Id = :Id AND DeleteAt = 0", map[string]interface{}{"Id": id}); err != nil { + if err == sql.ErrNoRows { + return nil, model.NewAppError("SqlWebhookStore.GetIncoming", "store.sql_webhooks.get_incoming.app_error", nil, "id="+id+", err="+err.Error(), http.StatusNotFound) } + return nil, model.NewAppError("SqlWebhookStore.GetIncoming", "store.sql_webhooks.get_incoming.app_error", nil, "id="+id+", err="+err.Error(), http.StatusInternalServerError) + } - result.Data = &webhook - }) + webhookCache.AddWithExpiresInSecs(id, &webhook, WEBHOOK_CACHE_SEC) + + return &webhook, nil } func (s SqlWebhookStore) DeleteIncoming(webhookId string, time int64) store.StoreChannel { diff --git a/store/store.go b/store/store.go index 2de7cb1142..73555371b8 100644 --- a/store/store.go +++ b/store/store.go @@ -378,7 +378,7 @@ type SystemStore interface { type WebhookStore interface { SaveIncoming(webhook *model.IncomingWebhook) StoreChannel - GetIncoming(id string, allowFromCache bool) StoreChannel + GetIncoming(id string, allowFromCache bool) (*model.IncomingWebhook, *model.AppError) GetIncomingList(offset, limit int) StoreChannel GetIncomingByTeam(teamId string, offset, limit int) StoreChannel UpdateIncoming(webhook *model.IncomingWebhook) StoreChannel diff --git a/store/storetest/mocks/UserStore.go b/store/storetest/mocks/UserStore.go index 2a8c24f336..95b8f9323c 100644 --- a/store/storetest/mocks/UserStore.go +++ b/store/storetest/mocks/UserStore.go @@ -267,6 +267,22 @@ func (_m *UserStore) GetByUsername(username string) store.StoreChannel { return r0 } +// GetChannelGroupUsers provides a mock function with given fields: channelID +func (_m *UserStore) GetChannelGroupUsers(channelID string) store.StoreChannel { + ret := _m.Called(channelID) + + var r0 store.StoreChannel + if rf, ok := ret.Get(0).(func(string) store.StoreChannel); ok { + r0 = rf(channelID) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(store.StoreChannel) + } + } + + return r0 +} + // GetEtagForAllProfiles provides a mock function with given fields: func (_m *UserStore) GetEtagForAllProfiles() store.StoreChannel { ret := _m.Called() @@ -507,6 +523,22 @@ func (_m *UserStore) GetSystemAdminProfiles() store.StoreChannel { return r0 } +// GetTeamGroupUsers provides a mock function with given fields: teamID +func (_m *UserStore) GetTeamGroupUsers(teamID string) store.StoreChannel { + ret := _m.Called(teamID) + + var r0 store.StoreChannel + if rf, ok := ret.Get(0).(func(string) store.StoreChannel); ok { + r0 = rf(teamID) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(store.StoreChannel) + } + } + + return r0 +} + // GetUnreadCount provides a mock function with given fields: userId func (_m *UserStore) GetUnreadCount(userId string) store.StoreChannel { ret := _m.Called(userId) @@ -857,35 +889,3 @@ func (_m *UserStore) VerifyEmail(userId string, email string) store.StoreChannel return r0 } - -// GetTeamGroupUsers provides a mock function with given fields: userId, email -func (_m *UserStore) GetTeamGroupUsers(teamID string) store.StoreChannel { - ret := _m.Called(teamID) - - var r0 store.StoreChannel - if rf, ok := ret.Get(0).(func(string) store.StoreChannel); ok { - r0 = rf(teamID) - } else { - if ret.Get(0) != nil { - r0 = ret.Get(0).(store.StoreChannel) - } - } - - return r0 -} - -// GetChannelGroupUsers provides a mock function with given fields: userId, email -func (_m *UserStore) GetChannelGroupUsers(teamID string) store.StoreChannel { - ret := _m.Called(teamID) - - var r0 store.StoreChannel - if rf, ok := ret.Get(0).(func(string) store.StoreChannel); ok { - r0 = rf(teamID) - } else { - if ret.Get(0) != nil { - r0 = ret.Get(0).(store.StoreChannel) - } - } - - return r0 -} diff --git a/store/storetest/mocks/WebhookStore.go b/store/storetest/mocks/WebhookStore.go index a0b2b0beec..0be4836fd3 100644 --- a/store/storetest/mocks/WebhookStore.go +++ b/store/storetest/mocks/WebhookStore.go @@ -83,19 +83,28 @@ func (_m *WebhookStore) DeleteOutgoing(webhookId string, time int64) store.Store } // GetIncoming provides a mock function with given fields: id, allowFromCache -func (_m *WebhookStore) GetIncoming(id string, allowFromCache bool) store.StoreChannel { +func (_m *WebhookStore) GetIncoming(id string, allowFromCache bool) (*model.IncomingWebhook, *model.AppError) { ret := _m.Called(id, allowFromCache) - var r0 store.StoreChannel - if rf, ok := ret.Get(0).(func(string, bool) store.StoreChannel); ok { + var r0 *model.IncomingWebhook + if rf, ok := ret.Get(0).(func(string, bool) *model.IncomingWebhook); ok { r0 = rf(id, allowFromCache) } else { if ret.Get(0) != nil { - r0 = ret.Get(0).(store.StoreChannel) + r0 = ret.Get(0).(*model.IncomingWebhook) } } - return r0 + var r1 *model.AppError + if rf, ok := ret.Get(1).(func(string, bool) *model.AppError); ok { + r1 = rf(id, allowFromCache) + } else { + if ret.Get(1) != nil { + r1 = ret.Get(1).(*model.AppError) + } + } + + return r0, r1 } // GetIncomingByChannel provides a mock function with given fields: channelId diff --git a/store/storetest/webhook_store.go b/store/storetest/webhook_store.go index 2b30f2d333..486a389290 100644 --- a/store/storetest/webhook_store.go +++ b/store/storetest/webhook_store.go @@ -11,6 +11,7 @@ import ( "github.com/mattermost/mattermost-server/model" "github.com/mattermost/mattermost-server/store" + "github.com/stretchr/testify/require" ) func TestWebhookStore(t *testing.T, ss store.Store) { @@ -72,31 +73,27 @@ func testWebhookStoreGetIncoming(t *testing.T, ss store.Store) { o1 := buildIncomingWebhook() o1 = (<-ss.Webhook().SaveIncoming(o1)).Data.(*model.IncomingWebhook) - if r1 := <-ss.Webhook().GetIncoming(o1.Id, false); r1.Err != nil { - t.Fatal(r1.Err) - } else { - if r1.Data.(*model.IncomingWebhook).CreateAt != o1.CreateAt { - t.Fatal("invalid returned webhook") - } + webhook, err := ss.Webhook().GetIncoming(o1.Id, false) + require.Nil(t, err) + if webhook.CreateAt != o1.CreateAt { + t.Fatal("invalid returned webhook") } - if r1 := <-ss.Webhook().GetIncoming(o1.Id, true); r1.Err != nil { - t.Fatal(r1.Err) - } else { - if r1.Data.(*model.IncomingWebhook).CreateAt != o1.CreateAt { - t.Fatal("invalid returned webhook") - } + webhook, err = ss.Webhook().GetIncoming(o1.Id, true) + require.Nil(t, err) + if webhook.CreateAt != o1.CreateAt { + t.Fatal("invalid returned webhook") } - if err := (<-ss.Webhook().GetIncoming("123", false)).Err; err == nil { + if _, err = ss.Webhook().GetIncoming("123", false); err == nil { t.Fatal("Missing id should have failed") } - if err := (<-ss.Webhook().GetIncoming("123", true)).Err; err == nil { + if _, err = ss.Webhook().GetIncoming("123", true); err == nil { t.Fatal("Missing id should have failed") } - if err := (<-ss.Webhook().GetIncoming("123", true)).Err; err.StatusCode != http.StatusNotFound { + if _, err = ss.Webhook().GetIncoming("123", true); err.StatusCode != http.StatusNotFound { t.Fatal("Should have set the status as not found for missing id") } } @@ -160,22 +157,18 @@ func testWebhookStoreDeleteIncoming(t *testing.T, ss store.Store) { o1 = (<-ss.Webhook().SaveIncoming(o1)).Data.(*model.IncomingWebhook) - if r1 := <-ss.Webhook().GetIncoming(o1.Id, true); r1.Err != nil { - t.Fatal(r1.Err) - } else { - if r1.Data.(*model.IncomingWebhook).CreateAt != o1.CreateAt { - t.Fatal("invalid returned webhook") - } + webhook, err := ss.Webhook().GetIncoming(o1.Id, true) + require.Nil(t, err) + if webhook.CreateAt != o1.CreateAt { + t.Fatal("invalid returned webhook") } if r2 := <-ss.Webhook().DeleteIncoming(o1.Id, model.GetMillis()); r2.Err != nil { t.Fatal(r2.Err) } - if r3 := (<-ss.Webhook().GetIncoming(o1.Id, true)); r3.Err == nil { - t.Log(r3.Data) - t.Fatal("Missing id should have failed") - } + webhook, err = ss.Webhook().GetIncoming(o1.Id, true) + require.NotNil(t, err) } func testWebhookStoreDeleteIncomingByChannel(t *testing.T, ss store.Store) { @@ -183,20 +176,17 @@ func testWebhookStoreDeleteIncomingByChannel(t *testing.T, ss store.Store) { o1 = (<-ss.Webhook().SaveIncoming(o1)).Data.(*model.IncomingWebhook) - if r1 := <-ss.Webhook().GetIncoming(o1.Id, true); r1.Err != nil { - t.Fatal(r1.Err) - } else { - if r1.Data.(*model.IncomingWebhook).CreateAt != o1.CreateAt { - t.Fatal("invalid returned webhook") - } + webhook, err := ss.Webhook().GetIncoming(o1.Id, true) + require.Nil(t, err) + if webhook.CreateAt != o1.CreateAt { + t.Fatal("invalid returned webhook") } if r2 := <-ss.Webhook().PermanentDeleteIncomingByChannel(o1.ChannelId); r2.Err != nil { t.Fatal(r2.Err) } - if r3 := (<-ss.Webhook().GetIncoming(o1.Id, true)); r3.Err == nil { - t.Log(r3.Data) + if _, err = ss.Webhook().GetIncoming(o1.Id, true); err == nil { t.Fatal("Missing id should have failed") } } @@ -206,20 +196,17 @@ func testWebhookStoreDeleteIncomingByUser(t *testing.T, ss store.Store) { o1 = (<-ss.Webhook().SaveIncoming(o1)).Data.(*model.IncomingWebhook) - if r1 := <-ss.Webhook().GetIncoming(o1.Id, true); r1.Err != nil { - t.Fatal(r1.Err) - } else { - if r1.Data.(*model.IncomingWebhook).CreateAt != o1.CreateAt { - t.Fatal("invalid returned webhook") - } + webhook, err := ss.Webhook().GetIncoming(o1.Id, true) + require.Nil(t, err) + if webhook.CreateAt != o1.CreateAt { + t.Fatal("invalid returned webhook") } if r2 := <-ss.Webhook().PermanentDeleteIncomingByUser(o1.UserId); r2.Err != nil { t.Fatal(r2.Err) } - if r3 := (<-ss.Webhook().GetIncoming(o1.Id, true)); r3.Err == nil { - t.Log(r3.Data) + if _, err = ss.Webhook().GetIncoming(o1.Id, true); err == nil { t.Fatal("Missing id should have failed") } }