diff --git a/server/channels/app/emoji.go b/server/channels/app/emoji.go index 73924114e8..273282181d 100644 --- a/server/channels/app/emoji.go +++ b/server/channels/app/emoji.go @@ -191,7 +191,7 @@ func (a *App) GetEmoji(c request.CTX, emojiId string) (*model.Emoji, *model.AppE return nil, model.NewAppError("GetEmoji", "api.emoji.storage.app_error", nil, "", http.StatusForbidden) } - emoji, err := a.Srv().Store().Emoji().Get(context.Background(), emojiId, true) + emoji, err := a.Srv().Store().Emoji().Get(c.Context(), emojiId, true) if err != nil { var nfErr *store.ErrNotFound switch { @@ -214,7 +214,7 @@ func (a *App) GetEmojiByName(c request.CTX, emojiName string) (*model.Emoji, *mo return nil, model.NewAppError("GetEmojiByName", "api.emoji.storage.app_error", nil, "", http.StatusForbidden) } - emoji, err := a.Srv().Store().Emoji().GetByName(context.Background(), emojiName, true) + emoji, err := a.Srv().Store().Emoji().GetByName(c.Context(), emojiName, true) if err != nil { var nfErr *store.ErrNotFound switch { @@ -233,7 +233,21 @@ func (a *App) GetMultipleEmojiByName(c request.CTX, names []string) ([]*model.Em return nil, model.NewAppError("GetMultipleEmojiByName", "api.emoji.disabled.app_error", nil, "", http.StatusForbidden) } - emoji, err := a.Srv().Store().Emoji().GetMultipleByName(names) + // Filtering out system emojis + i := 0 + for _, n := range names { + if _, ok := model.GetSystemEmojiId(n); !ok { + names[i] = n + i++ + } + } + names = names[:i] + + if len(names) == 0 { + return []*model.Emoji{}, nil + } + + emoji, err := a.Srv().Store().Emoji().GetMultipleByName(c.Context(), names) if err != nil { return nil, model.NewAppError("GetMultipleEmojiByName", "app.emoji.get_by_name.app_error", nil, fmt.Sprintf("names=%v, %v", names, err.Error()), http.StatusInternalServerError) } @@ -242,7 +256,7 @@ func (a *App) GetMultipleEmojiByName(c request.CTX, names []string) ([]*model.Em } func (a *App) GetEmojiImage(c request.CTX, emojiId string) ([]byte, string, *model.AppError) { - _, storeErr := a.Srv().Store().Emoji().Get(context.Background(), emojiId, true) + _, storeErr := a.Srv().Store().Emoji().Get(c.Context(), emojiId, true) if storeErr != nil { var nfErr *store.ErrNotFound switch { diff --git a/server/channels/app/emoji_test.go b/server/channels/app/emoji_test.go new file mode 100644 index 0000000000..dfaacc123c --- /dev/null +++ b/server/channels/app/emoji_test.go @@ -0,0 +1,29 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package app + +import ( + "testing" + + "github.com/mattermost/mattermost/server/public/model" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestGetMultipleEmojiByName(t *testing.T) { + // The fact that we use mock store ensures that + // the call to the DB does not happen. If it did, we would have needed + // to provide the mock explicitly. + th := SetupWithStoreMock(t) + defer th.TearDown() + + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.EnableCustomEmoji = true + }) + + // Ensure it returns empty for system emojis + emojis, appErr := th.App.GetMultipleEmojiByName(th.Context, []string{"+1"}) + require.Nil(t, appErr) + assert.Empty(t, emojis) +} diff --git a/server/channels/app/post_metadata.go b/server/channels/app/post_metadata.go index 55a555d96c..94ad1b0dfa 100644 --- a/server/channels/app/post_metadata.go +++ b/server/channels/app/post_metadata.go @@ -438,7 +438,6 @@ func (a *App) getCustomEmojisForPost(c request.CTX, post *model.Post, reactions } names := getEmojiNamesForPost(post, reactions) - if len(names) == 0 { return []*model.Emoji{}, nil } diff --git a/server/channels/store/localcachelayer/emoji_layer.go b/server/channels/store/localcachelayer/emoji_layer.go index 93039de03c..cb5b4659c6 100644 --- a/server/channels/store/localcachelayer/emoji_layer.go +++ b/server/channels/store/localcachelayer/emoji_layer.go @@ -90,12 +90,50 @@ func (es *LocalCacheEmojiStore) GetByName(ctx context.Context, name string, allo es.emojiByNameMut.Unlock() emoji, err := es.EmojiStore.GetByName(ctx, name, allowFromCache) + if err != nil { + return nil, err + } - if allowFromCache && err == nil { + if allowFromCache { es.addToCache(emoji) } - return emoji, err + return emoji, nil +} + +func (es *LocalCacheEmojiStore) GetMultipleByName(ctx context.Context, names []string) ([]*model.Emoji, error) { + emojis := []*model.Emoji{} + remainingEmojiNames := make([]string, 0) + + for _, name := range names { + if emoji, ok := es.getFromCacheByName(name); ok { + emojis = append(emojis, emoji) + } else { + // If it was invalidated, then we need to query master. + es.emojiByNameMut.Lock() + if es.emojiByNameInvalidations[name] { + ctx = sqlstore.WithMaster(ctx) + // And then remove the key from the map. + delete(es.emojiByNameInvalidations, name) + } + es.emojiByNameMut.Unlock() + + remainingEmojiNames = append(remainingEmojiNames, name) + } + } + + if len(remainingEmojiNames) > 0 { + remainingEmojis, err := es.EmojiStore.GetMultipleByName(ctx, remainingEmojiNames) + if err != nil { + return nil, err + } + for _, emoji := range remainingEmojis { + es.addToCache(emoji) + emojis = append(emojis, emoji) + } + } + + return emojis, nil } func (es *LocalCacheEmojiStore) Delete(emoji *model.Emoji, time int64) error { diff --git a/server/channels/store/localcachelayer/emoji_layer_test.go b/server/channels/store/localcachelayer/emoji_layer_test.go index 3c1e7cb0ad..f3b643ff7b 100644 --- a/server/channels/store/localcachelayer/emoji_layer_test.go +++ b/server/channels/store/localcachelayer/emoji_layer_test.go @@ -21,6 +21,7 @@ func TestEmojiStore(t *testing.T) { func TestEmojiStoreCache(t *testing.T) { fakeEmoji := model.Emoji{Id: "123", Name: "name123"} + fakeEmoji2 := model.Emoji{Id: "321", Name: "name321"} ctxEmoji := model.Emoji{Id: "master", Name: "name123"} t.Run("first call by id not cached, second cached and returning same data", func(t *testing.T) { @@ -39,7 +40,7 @@ func TestEmojiStoreCache(t *testing.T) { mockStore.Emoji().(*mocks.EmojiStore).AssertNumberOfCalls(t, "Get", 1) }) - t.Run("first call by name not cached, second cached and returning same data", func(t *testing.T) { + t.Run("GetByName: first call by name not cached, second cached and returning same data", func(t *testing.T) { mockStore := getMockStore() mockCacheProvider := getMockCacheProvider() cachedStore, err := NewLocalCacheLayer(mockStore, nil, nil, mockCacheProvider) @@ -55,6 +56,43 @@ func TestEmojiStoreCache(t *testing.T) { mockStore.Emoji().(*mocks.EmojiStore).AssertNumberOfCalls(t, "GetByName", 1) }) + t.Run("GetMultipleByName: first call by name not cached, second cached and returning same data", func(t *testing.T) { + mockStore := getMockStore() + mockCacheProvider := getMockCacheProvider() + cachedStore, err := NewLocalCacheLayer(mockStore, nil, nil, mockCacheProvider) + require.NoError(t, err) + + emojis, err := cachedStore.Emoji().GetMultipleByName(context.Background(), []string{"name123"}) + require.NoError(t, err) + require.Len(t, emojis, 1) + assert.Equal(t, emojis[0], &fakeEmoji) + mockStore.Emoji().(*mocks.EmojiStore).AssertNumberOfCalls(t, "GetMultipleByName", 1) + emojis, err = cachedStore.Emoji().GetMultipleByName(context.Background(), []string{"name123"}) + require.NoError(t, err) + require.Len(t, emojis, 1) + assert.Equal(t, emojis[0], &fakeEmoji) + mockStore.Emoji().(*mocks.EmojiStore).AssertNumberOfCalls(t, "GetMultipleByName", 1) + }) + + t.Run("GetMultipleByName: multiple elements", func(t *testing.T) { + mockStore := getMockStore() + mockCacheProvider := getMockCacheProvider() + cachedStore, err := NewLocalCacheLayer(mockStore, nil, nil, mockCacheProvider) + require.NoError(t, err) + + emojis, err := cachedStore.Emoji().GetMultipleByName(context.Background(), []string{"name123", "name321"}) + require.NoError(t, err) + require.Len(t, emojis, 2) + assert.Equal(t, emojis[0], &fakeEmoji) + assert.Equal(t, emojis[1], &fakeEmoji2) + mockStore.Emoji().(*mocks.EmojiStore).AssertNumberOfCalls(t, "GetMultipleByName", 1) + emojis, err = cachedStore.Emoji().GetMultipleByName(context.Background(), []string{"name123"}) + require.NoError(t, err) + require.Len(t, emojis, 1) + assert.Equal(t, emojis[0], &fakeEmoji) + mockStore.Emoji().(*mocks.EmojiStore).AssertNumberOfCalls(t, "GetMultipleByName", 1) + }) + t.Run("first call by id not cached, second force not cached", func(t *testing.T) { mockStore := getMockStore() mockCacheProvider := getMockCacheProvider() @@ -107,7 +145,7 @@ func TestEmojiStoreCache(t *testing.T) { mockStore.Emoji().(*mocks.EmojiStore).AssertNumberOfCalls(t, "GetByName", 2) }) - t.Run("first call by id, second call by name cached", func(t *testing.T) { + t.Run("first call by id, second call by name and GetMultipleByName cached", func(t *testing.T) { mockStore := getMockStore() mockCacheProvider := getMockCacheProvider() cachedStore, err := NewLocalCacheLayer(mockStore, nil, nil, mockCacheProvider) @@ -117,6 +155,8 @@ func TestEmojiStoreCache(t *testing.T) { mockStore.Emoji().(*mocks.EmojiStore).AssertNumberOfCalls(t, "Get", 1) cachedStore.Emoji().GetByName(context.Background(), "name123", true) mockStore.Emoji().(*mocks.EmojiStore).AssertNumberOfCalls(t, "GetByName", 0) + cachedStore.Emoji().GetMultipleByName(context.Background(), []string{"name123"}) + mockStore.Emoji().(*mocks.EmojiStore).AssertNumberOfCalls(t, "GetMultipleByName", 0) }) t.Run("first call by name, second call by id cached", func(t *testing.T) { diff --git a/server/channels/store/localcachelayer/main_test.go b/server/channels/store/localcachelayer/main_test.go index 787dd1d594..4a21a320c4 100644 --- a/server/channels/store/localcachelayer/main_test.go +++ b/server/channels/store/localcachelayer/main_test.go @@ -69,6 +69,7 @@ func getMockStore() *mocks.Store { mockStore.On("Webhook").Return(&mockWebhookStore) fakeEmoji := model.Emoji{Id: "123", Name: "name123"} + fakeEmoji2 := model.Emoji{Id: "321", Name: "name321"} ctxEmoji := model.Emoji{Id: "master", Name: "name123"} mockEmojiStore := mocks.EmojiStore{} mockEmojiStore.On("Get", mock.Anything, "123", true).Return(&fakeEmoji, nil) @@ -77,6 +78,8 @@ func getMockStore() *mocks.Store { mockEmojiStore.On("Get", sqlstore.WithMaster(context.Background()), "master", true).Return(&ctxEmoji, nil) mockEmojiStore.On("GetByName", mock.Anything, "name123", true).Return(&fakeEmoji, nil) mockEmojiStore.On("GetByName", mock.Anything, "name123", false).Return(&fakeEmoji, nil) + mockEmojiStore.On("GetMultipleByName", context.Background(), []string{"name123"}).Return([]*model.Emoji{&fakeEmoji}, nil) + mockEmojiStore.On("GetMultipleByName", context.Background(), []string{"name123", "name321"}).Return([]*model.Emoji{&fakeEmoji, &fakeEmoji2}, nil) mockEmojiStore.On("GetByName", context.Background(), "master", true).Return(&ctxEmoji, nil) mockEmojiStore.On("GetByName", sqlstore.WithMaster(context.Background()), "master", false).Return(&ctxEmoji, nil) mockEmojiStore.On("Delete", &fakeEmoji, int64(0)).Return(nil) diff --git a/server/channels/store/opentracinglayer/opentracinglayer.go b/server/channels/store/opentracinglayer/opentracinglayer.go index 45ab9d90f6..7aa193a317 100644 --- a/server/channels/store/opentracinglayer/opentracinglayer.go +++ b/server/channels/store/opentracinglayer/opentracinglayer.go @@ -3415,7 +3415,7 @@ func (s *OpenTracingLayerEmojiStore) GetList(offset int, limit int, sort string) return result, err } -func (s *OpenTracingLayerEmojiStore) GetMultipleByName(names []string) ([]*model.Emoji, error) { +func (s *OpenTracingLayerEmojiStore) GetMultipleByName(ctx context.Context, names []string) ([]*model.Emoji, error) { origCtx := s.Root.Store.Context() span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "EmojiStore.GetMultipleByName") s.Root.Store.SetContext(newCtx) @@ -3424,7 +3424,7 @@ func (s *OpenTracingLayerEmojiStore) GetMultipleByName(names []string) ([]*model }() defer span.Finish() - result, err := s.EmojiStore.GetMultipleByName(names) + result, err := s.EmojiStore.GetMultipleByName(ctx, names) if err != nil { span.LogFields(spanlog.Error(err)) ext.Error.Set(span, true) diff --git a/server/channels/store/retrylayer/retrylayer.go b/server/channels/store/retrylayer/retrylayer.go index 740f8ab3f6..7f1f602e91 100644 --- a/server/channels/store/retrylayer/retrylayer.go +++ b/server/channels/store/retrylayer/retrylayer.go @@ -3818,11 +3818,11 @@ func (s *RetryLayerEmojiStore) GetList(offset int, limit int, sort string) ([]*m } -func (s *RetryLayerEmojiStore) GetMultipleByName(names []string) ([]*model.Emoji, error) { +func (s *RetryLayerEmojiStore) GetMultipleByName(ctx context.Context, names []string) ([]*model.Emoji, error) { tries := 0 for { - result, err := s.EmojiStore.GetMultipleByName(names) + result, err := s.EmojiStore.GetMultipleByName(ctx, names) if err == nil { return result, nil } diff --git a/server/channels/store/sqlstore/emoji_store.go b/server/channels/store/sqlstore/emoji_store.go index 9500749770..7429966867 100644 --- a/server/channels/store/sqlstore/emoji_store.go +++ b/server/channels/store/sqlstore/emoji_store.go @@ -52,13 +52,13 @@ func (es SqlEmojiStore) GetByName(ctx context.Context, name string, allowFromCac return es.getBy(ctx, "Name", name) } -func (es SqlEmojiStore) GetMultipleByName(names []string) ([]*model.Emoji, error) { +func (es SqlEmojiStore) GetMultipleByName(ctx context.Context, names []string) ([]*model.Emoji, error) { // Creating (?, ?, ?) len(names) number of times. keys := strings.Join(strings.Fields(strings.Repeat("? ", len(names))), ",") args := makeStringArgs(names) emojis := []*model.Emoji{} - if err := es.GetReplicaX().Select(&emojis, + if err := es.DBXFromContext(ctx).Select(&emojis, `SELECT * FROM diff --git a/server/channels/store/store.go b/server/channels/store/store.go index 27a49f8b1f..3e2798e5e4 100644 --- a/server/channels/store/store.go +++ b/server/channels/store/store.go @@ -669,7 +669,7 @@ type EmojiStore interface { Save(emoji *model.Emoji) (*model.Emoji, error) Get(ctx context.Context, id string, allowFromCache bool) (*model.Emoji, error) GetByName(ctx context.Context, name string, allowFromCache bool) (*model.Emoji, error) - GetMultipleByName(names []string) ([]*model.Emoji, error) + GetMultipleByName(ctx context.Context, names []string) ([]*model.Emoji, error) GetList(offset, limit int, sort string) ([]*model.Emoji, error) Delete(emoji *model.Emoji, timestamp int64) error Search(name string, prefixOnly bool, limit int) ([]*model.Emoji, error) diff --git a/server/channels/store/storetest/emoji_store.go b/server/channels/store/storetest/emoji_store.go index bd39d0fa0f..bfb8ee9c0c 100644 --- a/server/channels/store/storetest/emoji_store.go +++ b/server/channels/store/storetest/emoji_store.go @@ -154,26 +154,26 @@ func testEmojiGetMultipleByName(t *testing.T, ss store.Store) { }() t.Run("one emoji", func(t *testing.T) { - received, err := ss.Emoji().GetMultipleByName([]string{emojis[0].Name}) + received, err := ss.Emoji().GetMultipleByName(context.Background(), []string{emojis[0].Name}) require.NoError(t, err, "could not get emoji") require.Len(t, received, 1, "got incorrect emoji") require.Equal(t, *received[0], emojis[0], "got incorrect emoji") }) t.Run("multiple emojis", func(t *testing.T) { - received, err := ss.Emoji().GetMultipleByName([]string{emojis[0].Name, emojis[1].Name, emojis[2].Name}) + received, err := ss.Emoji().GetMultipleByName(context.Background(), []string{emojis[0].Name, emojis[1].Name, emojis[2].Name}) require.NoError(t, err, "could not get emojis") require.Len(t, received, 3, "got incorrect emojis") }) t.Run("one nonexistent emoji", func(t *testing.T) { - received, err := ss.Emoji().GetMultipleByName([]string{"ab"}) + received, err := ss.Emoji().GetMultipleByName(context.Background(), []string{"ab"}) require.NoError(t, err, "could not get emoji", err) require.Empty(t, received, "got incorrect emoji") }) t.Run("multiple emojis with nonexistent names", func(t *testing.T) { - received, err := ss.Emoji().GetMultipleByName([]string{emojis[0].Name, emojis[1].Name, emojis[2].Name, "abcd", "1234"}) + received, err := ss.Emoji().GetMultipleByName(context.Background(), []string{emojis[0].Name, emojis[1].Name, emojis[2].Name, "abcd", "1234"}) require.NoError(t, err, "could not get emojis") require.Len(t, received, 3, "got incorrect emojis") }) diff --git a/server/channels/store/storetest/mocks/EmojiStore.go b/server/channels/store/storetest/mocks/EmojiStore.go index f9cfe28f03..9c1045a8e3 100644 --- a/server/channels/store/storetest/mocks/EmojiStore.go +++ b/server/channels/store/storetest/mocks/EmojiStore.go @@ -108,25 +108,25 @@ func (_m *EmojiStore) GetList(offset int, limit int, sort string) ([]*model.Emoj return r0, r1 } -// GetMultipleByName provides a mock function with given fields: names -func (_m *EmojiStore) GetMultipleByName(names []string) ([]*model.Emoji, error) { - ret := _m.Called(names) +// GetMultipleByName provides a mock function with given fields: ctx, names +func (_m *EmojiStore) GetMultipleByName(ctx context.Context, names []string) ([]*model.Emoji, error) { + ret := _m.Called(ctx, names) var r0 []*model.Emoji var r1 error - if rf, ok := ret.Get(0).(func([]string) ([]*model.Emoji, error)); ok { - return rf(names) + if rf, ok := ret.Get(0).(func(context.Context, []string) ([]*model.Emoji, error)); ok { + return rf(ctx, names) } - if rf, ok := ret.Get(0).(func([]string) []*model.Emoji); ok { - r0 = rf(names) + if rf, ok := ret.Get(0).(func(context.Context, []string) []*model.Emoji); ok { + r0 = rf(ctx, names) } else { if ret.Get(0) != nil { r0 = ret.Get(0).([]*model.Emoji) } } - if rf, ok := ret.Get(1).(func([]string) error); ok { - r1 = rf(names) + if rf, ok := ret.Get(1).(func(context.Context, []string) error); ok { + r1 = rf(ctx, names) } else { r1 = ret.Error(1) } diff --git a/server/channels/store/timerlayer/timerlayer.go b/server/channels/store/timerlayer/timerlayer.go index 67447d90ad..8fe2245795 100644 --- a/server/channels/store/timerlayer/timerlayer.go +++ b/server/channels/store/timerlayer/timerlayer.go @@ -3128,10 +3128,10 @@ func (s *TimerLayerEmojiStore) GetList(offset int, limit int, sort string) ([]*m return result, err } -func (s *TimerLayerEmojiStore) GetMultipleByName(names []string) ([]*model.Emoji, error) { +func (s *TimerLayerEmojiStore) GetMultipleByName(ctx context.Context, names []string) ([]*model.Emoji, error) { start := time.Now() - result, err := s.EmojiStore.GetMultipleByName(names) + result, err := s.EmojiStore.GetMultipleByName(ctx, names) elapsed := float64(time.Since(start)) / float64(time.Second) if s.Root.Metrics != nil {