From 3d0fd16666c22dd1120f57c98ac0a275c0bc220a Mon Sep 17 00:00:00 2001 From: Harrison Healey Date: Tue, 17 Oct 2023 12:03:28 -0400 Subject: [PATCH] MM-54640 Add API to get multiple emojis by name at once (#24651) * MM-54640 Add API to get multiple emojis by name at once * Fix status code when too many names are requested * Address feedback * Update unit tests * Fix styling * Fix more styling * Fix mismatched i18n id --- server/channels/api4/emoji.go | 37 ++++++++ server/channels/api4/emoji_test.go | 63 +++++++++++++ server/channels/testlib/helper.go | 1 + server/i18n/en.json | 4 + server/public/model/client4.go | 20 ++++ .../src/actions/emojis.test.ts | 93 +++++++++++++------ .../mattermost-redux/src/actions/emojis.ts | 51 ++++++++-- webapp/platform/client/src/client4.ts | 7 ++ 8 files changed, 241 insertions(+), 35 deletions(-) diff --git a/server/channels/api4/emoji.go b/server/channels/api4/emoji.go index 3f9ed7dedf..4d29267512 100644 --- a/server/channels/api4/emoji.go +++ b/server/channels/api4/emoji.go @@ -17,11 +17,13 @@ import ( const ( EmojiMaxAutocompleteItems = 100 + GetEmojisByNamesMax = 200 ) func (api *API) InitEmoji() { api.BaseRoutes.Emojis.Handle("", api.APISessionRequired(createEmoji)).Methods("POST") api.BaseRoutes.Emojis.Handle("", api.APISessionRequired(getEmojiList)).Methods("GET") + api.BaseRoutes.Emojis.Handle("/names", api.APISessionRequired(getEmojisByNames)).Methods("POST") api.BaseRoutes.Emojis.Handle("/search", api.APISessionRequired(searchEmojis)).Methods("POST") api.BaseRoutes.Emojis.Handle("/autocomplete", api.APISessionRequired(autocompleteEmojis)).Methods("GET") api.BaseRoutes.Emoji.Handle("", api.APISessionRequired(deleteEmoji)).Methods("DELETE") @@ -221,6 +223,11 @@ func getEmojiByName(c *Context, w http.ResponseWriter, r *http.Request) { return } + if !*c.App.Config().ServiceSettings.EnableCustomEmoji { + c.Err = model.NewAppError("getEmojiByName", "api.emoji.disabled.app_error", nil, "", http.StatusNotImplemented) + return + } + emoji, err := c.App.GetEmojiByName(c.AppContext, c.Params.EmojiName) if err != nil { c.Err = err @@ -232,6 +239,36 @@ func getEmojiByName(c *Context, w http.ResponseWriter, r *http.Request) { } } +func getEmojisByNames(c *Context, w http.ResponseWriter, r *http.Request) { + names := model.ArrayFromJSON(r.Body) + if len(names) == 0 { + c.SetInvalidParam("names") + return + } + + if !*c.App.Config().ServiceSettings.EnableCustomEmoji { + c.Err = model.NewAppError("getEmojisByNames", "api.emoji.disabled.app_error", nil, "", http.StatusNotImplemented) + return + } + + if len(names) > GetEmojisByNamesMax { + c.Err = model.NewAppError("getEmojisByNames", "api.emoji.get_multiple_by_name_too_many.request_error", map[string]any{ + "MaxNames": GetEmojisByNamesMax, + }, "", http.StatusBadRequest) + return + } + + emojis, err := c.App.GetMultipleEmojiByName(c.AppContext, names) + if err != nil { + c.Err = err + return + } + + if err := json.NewEncoder(w).Encode(emojis); err != nil { + c.Logger.Warn("Error while writing response", mlog.Err(err)) + } +} + func getEmojiImage(c *Context, w http.ResponseWriter, r *http.Request) { c.RequireEmojiId() if c.Err != nil { diff --git a/server/channels/api4/emoji_test.go b/server/channels/api4/emoji_test.go index a1e1dcb0a8..fcfe58519f 100644 --- a/server/channels/api4/emoji_test.go +++ b/server/channels/api4/emoji_test.go @@ -280,6 +280,69 @@ func TestGetEmojiList(t *testing.T) { require.Greater(t, len(listEmoji), 0, "should return more than 0") } +func TestGetEmojisByNames(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + + // Set up some custom emojis + adminClient := th.SystemAdminClient + + imageBytes := utils.CreateTestJpeg(t, 10, 10) + + emoji1 := &model.Emoji{ + CreatorId: th.SystemAdminUser.Id, + Name: model.NewId(), + } + emoji1, _, err := adminClient.CreateEmoji(context.Background(), emoji1, imageBytes, "emoji.jpg") + require.NoError(t, err) + + emoji2 := &model.Emoji{ + CreatorId: th.SystemAdminUser.Id, + Name: model.NewId(), + } + emoji2, _, err = adminClient.CreateEmoji(context.Background(), emoji2, imageBytes, "emoji.jpg") + require.NoError(t, err) + + client := th.Client + + t.Run("should return a single emoji", func(t *testing.T) { + emojis, _, err := client.GetEmojisByNames(context.Background(), []string{emoji1.Name}) + + require.NoError(t, err) + require.Len(t, emojis, 1) + assert.Equal(t, emoji1.Id, emojis[0].Id) + }) + + t.Run("should return multiple emojis", func(t *testing.T) { + emojis, _, err := client.GetEmojisByNames(context.Background(), []string{emoji1.Name, emoji2.Name}) + + require.NoError(t, err) + require.Len(t, emojis, 2) + assert.Equal(t, emoji1.Id, emojis[0].Id) + assert.Equal(t, emoji2.Id, emojis[1].Id) + }) + + t.Run("should ignore non-existent emojis", func(t *testing.T) { + emojis, _, err := client.GetEmojisByNames(context.Background(), []string{emoji1.Name, emoji2.Name, model.NewId()}) + + require.NoError(t, err) + require.Len(t, emojis, 2) + assert.Equal(t, emoji1.Id, emojis[0].Id) + assert.Equal(t, emoji2.Id, emojis[1].Id) + }) + + t.Run("should return an error when too many emojis are requested", func(t *testing.T) { + names := make([]string, GetEmojisByNamesMax+1) + for i := 0; i < len(names); i++ { + names[i] = emoji1.Name + } + + _, _, err := client.GetEmojisByNames(context.Background(), names) + + require.Error(t, err) + }) +} + func TestDeleteEmoji(t *testing.T) { th := Setup(t).InitBasic() defer th.TearDown() diff --git a/server/channels/testlib/helper.go b/server/channels/testlib/helper.go index b6f1596198..6083817138 100644 --- a/server/channels/testlib/helper.go +++ b/server/channels/testlib/helper.go @@ -61,6 +61,7 @@ func NewMainHelperWithOptions(options *HelperOptions) *MainHelper { // Unset environment variables commonly set for development that interfere with tests. os.Unsetenv("MM_SERVICESETTINGS_SITEURL") os.Unsetenv("MM_SERVICESETTINGS_LISTENADDRESS") + os.Unsetenv("MM_SERVICESETTINGS_CONNECTIONSECURITY") os.Unsetenv("MM_SERVICESETTINGS_ENABLEDEVELOPER") var mainHelper MainHelper diff --git a/server/i18n/en.json b/server/i18n/en.json index 4b07187ab7..d2fa0fdcc6 100644 --- a/server/i18n/en.json +++ b/server/i18n/en.json @@ -1796,6 +1796,10 @@ "id": "api.emoji.get_image.read.app_error", "translation": "Unable to read image file for emoji." }, + { + "id": "api.emoji.get_multiple_by_name_too_many.request_error", + "translation": "Unable to get that many emojis by name. Only {{.MaxNames}} emojis can be requested at once." + }, { "id": "api.emoji.storage.app_error", "translation": "File storage not configured properly. Please configure for either S3 or local server file storage." diff --git a/server/public/model/client4.go b/server/public/model/client4.go index 65626ca3fb..53306f4aef 100644 --- a/server/public/model/client4.go +++ b/server/public/model/client4.go @@ -6618,6 +6618,26 @@ func (c *Client4) GetSortedEmojiList(ctx context.Context, page, perPage int, sor return list, BuildResponse(r), nil } +// GetEmojisByNames takes an array of custom emoji names and returns an array of those emojis. +func (c *Client4) GetEmojisByNames(ctx context.Context, names []string) ([]*Emoji, *Response, error) { + buf, err := json.Marshal(names) + if err != nil { + return nil, nil, NewAppError("GetEmojisByNames", "api.marshal_error", nil, "", http.StatusInternalServerError).Wrap(err) + } + + r, err := c.DoAPIPostBytes(ctx, c.emojisRoute()+"/names", buf) + if err != nil { + return nil, BuildResponse(r), err + } + defer closeBody(r) + + var list []*Emoji + if err := json.NewDecoder(r.Body).Decode(&list); err != nil { + return nil, nil, NewAppError("GetEmojisByNames", "api.unmarshal_error", nil, "", http.StatusInternalServerError).Wrap(err) + } + return list, BuildResponse(r), nil +} + // DeleteEmoji delete an custom emoji on the provided emoji id string. func (c *Client4) DeleteEmoji(ctx context.Context, emojiId string) (*Response, error) { r, err := c.DoAPIDelete(ctx, c.emojiRoute(emojiId)) diff --git a/webapp/channels/src/packages/mattermost-redux/src/actions/emojis.test.ts b/webapp/channels/src/packages/mattermost-redux/src/actions/emojis.test.ts index 95031b010a..2e6daffc08 100644 --- a/webapp/channels/src/packages/mattermost-redux/src/actions/emojis.test.ts +++ b/webapp/channels/src/packages/mattermost-redux/src/actions/emojis.test.ts @@ -264,36 +264,75 @@ describe('Actions.Emojis', () => { expect(state.entities.emojis.nonExistentEmoji.has(missingName)).toBeTruthy(); }); - it('getCustomEmojisByName', async () => { - const testImageData = fs.createReadStream('src/packages/mattermost-redux/test/assets/images/test.png'); + describe('getCustomEmojisByName', () => { + test('should be able to request a single emoji', async () => { + const emoji1 = TestHelper.getCustomEmojiMock({name: 'emoji1', id: 'emojiId1'}); - nock(Client4.getBaseRoute()). - post('/emoji'). - reply(201, {id: TestHelper.generateId(), create_at: 1507918415696, update_at: 1507918415696, delete_at: 0, creator_id: TestHelper.basicUser!.id, name: TestHelper.generateId()}); + nock(Client4.getBaseRoute()). + post('/emoji/names', ['emoji1']). + reply(200, [emoji1]); - const {data: created} = await Actions.createCustomEmoji( - { - name: TestHelper.generateId(), - creator_id: TestHelper.basicUser!.id, - }, - testImageData, - )(store.dispatch, store.getState) as ActionResult; + await store.dispatch(Actions.getCustomEmojisByName(['emoji1'])); - nock(Client4.getBaseRoute()). - get(`/emoji/name/${created.name}`). - reply(200, created); + const state = store.getState(); + expect(state.entities.emojis.customEmoji[emoji1.id]).toEqual(emoji1); + }); - const missingName = TestHelper.generateId(); + test('should be able to request multiple emojis', async () => { + const emoji1 = TestHelper.getCustomEmojiMock({name: 'emoji1', id: 'emojiId1'}); + const emoji2 = TestHelper.getCustomEmojiMock({name: 'emoji2', id: 'emojiId2'}); - nock(Client4.getBaseRoute()). - get(`/emoji/name/${missingName}`). - reply(404, {message: 'Not found', status_code: 404}); + nock(Client4.getBaseRoute()). + post('/emoji/names', ['emoji1', 'emoji2']). + reply(200, [emoji1, emoji2]); - await Actions.getCustomEmojisByName([created.name, missingName])(store.dispatch, store.getState); + await store.dispatch(Actions.getCustomEmojisByName(['emoji1', 'emoji2'])); - const state = store.getState(); - expect(state.entities.emojis.customEmoji[created.id]).toBeTruthy(); - expect(state.entities.emojis.nonExistentEmoji.has(missingName)).toBeTruthy(); + const state = store.getState(); + expect(state.entities.emojis.customEmoji[emoji1.id]).toEqual(emoji1); + expect(state.entities.emojis.customEmoji[emoji2.id]).toEqual(emoji2); + }); + + test('should correctly track non-existent emojis', async () => { + const emoji1 = TestHelper.getCustomEmojiMock({name: 'emoji1', id: 'emojiId1'}); + + nock(Client4.getBaseRoute()). + post('/emoji/names', ['emoji1', 'emoji2']). + reply(200, [emoji1]); + + await store.dispatch(Actions.getCustomEmojisByName(['emoji1', 'emoji2'])); + + const state = store.getState(); + expect(state.entities.emojis.customEmoji[emoji1.id]).toEqual(emoji1); + expect(state.entities.emojis.nonExistentEmoji).toEqual(new Set(['emoji2'])); + }); + + test('should be able to request over 200 emojis', async () => { + const emojis = []; + for (let i = 0; i < 500; i++) { + emojis.push(TestHelper.getCustomEmojiMock({name: 'emoji' + i, id: 'emojiId' + i})); + } + + const names = emojis.map((emoji) => emoji.name); + + nock(Client4.getBaseRoute()). + post('/emoji/names', names.slice(0, 200)). + reply(200, emojis.slice(0, 200)); + nock(Client4.getBaseRoute()). + post('/emoji/names', names.slice(200, 400)). + reply(200, emojis.slice(200, 400)); + nock(Client4.getBaseRoute()). + post('/emoji/names', names.slice(400, 500)). + reply(200, emojis.slice(400, 500)); + + await store.dispatch(Actions.getCustomEmojisByName(names)); + + const state = store.getState(); + expect(Object.keys(state.entities.emojis.customEmoji)).toHaveLength(emojis.length); + for (const emoji of emojis) { + expect(state.entities.emojis.customEmoji[emoji.id]).toEqual(emoji); + } + }); }); it('getCustomEmojisInText', async () => { @@ -311,15 +350,11 @@ describe('Actions.Emojis', () => { testImageData, )(store.dispatch, store.getState) as ActionResult; - nock(Client4.getBaseRoute()). - get(`/emoji/name/${created.name}`). - reply(200, created); - const missingName = TestHelper.generateId(); nock(Client4.getBaseRoute()). - get(`/emoji/name/${missingName}`). - reply(404, {message: 'Not found', status_code: 404}); + post('/emoji/names', [created.name, missingName]). + reply(200, [created]); await Actions.getCustomEmojisInText(`some text :${created.name}: :${missingName}:`)(store.dispatch, store.getState); diff --git a/webapp/channels/src/packages/mattermost-redux/src/actions/emojis.ts b/webapp/channels/src/packages/mattermost-redux/src/actions/emojis.ts index 9f6538a018..140eb29adc 100644 --- a/webapp/channels/src/packages/mattermost-redux/src/actions/emojis.ts +++ b/webapp/channels/src/packages/mattermost-redux/src/actions/emojis.ts @@ -1,12 +1,15 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. +import type {AnyAction} from 'redux'; +import {batchActions} from 'redux-batched-actions'; + import type {CustomEmoji} from '@mattermost/types/emojis'; import {EmojiTypes} from 'mattermost-redux/action_types'; import {Client4} from 'mattermost-redux/client'; import {getCustomEmojisByName as selectCustomEmojisByName} from 'mattermost-redux/selectors/entities/emojis'; -import type {GetStateFunc, DispatchFunc, ActionFunc, ActionResult} from 'mattermost-redux/types/actions'; +import type {GetStateFunc, DispatchFunc, ActionFunc} from 'mattermost-redux/types/actions'; import {parseNeededCustomEmojisFromText} from 'mattermost-redux/utils/emoji_utils'; import {logError} from './errors'; @@ -69,16 +72,52 @@ export function getCustomEmojiByName(name: string): ActionFunc { } export function getCustomEmojisByName(names: string[]): ActionFunc { - return async (dispatch: DispatchFunc) => { + return async (dispatch: DispatchFunc, getState: GetStateFunc) => { if (!names || names.length === 0) { return {data: true}; } - const promises: Array> = []; - names.forEach((name) => promises.push(dispatch(getCustomEmojiByName(name)))); + // If necessary, split up the list of names into batches based on api4.GetEmojisByNamesMax on the server + const batchSize = 200; - await Promise.all(promises); - return {data: true}; + const batches = []; + for (let i = 0; i < names.length; i += batchSize) { + batches.push(names.slice(i, i + batchSize)); + } + + let results; + try { + results = await Promise.all(batches.map((batch) => { + return Client4.getCustomEmojisByNames(batch); + })); + } catch (error) { + forceLogoutIfNecessary(error, dispatch, getState); + dispatch(logError(error)); + return {error}; + } + + const data = results.flat(); + const actions: AnyAction[] = [{ + type: EmojiTypes.RECEIVED_CUSTOM_EMOJIS, + data, + }]; + + if (data.length !== names.length) { + const foundNames = new Set(data.map((emoji) => emoji.name)); + + for (const name of names) { + if (foundNames.has(name)) { + continue; + } + + actions.push({ + type: EmojiTypes.CUSTOM_EMOJI_DOES_NOT_EXIST, + data: name, + }); + } + } + + return dispatch(actions.length > 1 ? batchActions(actions) : actions[0]); }; } diff --git a/webapp/platform/client/src/client4.ts b/webapp/platform/client/src/client4.ts index 102e743978..ae4582c20a 100644 --- a/webapp/platform/client/src/client4.ts +++ b/webapp/platform/client/src/client4.ts @@ -2749,6 +2749,13 @@ export default class Client4 { ); }; + getCustomEmojisByNames = (names: string[]) => { + return this.doFetch( + `${this.getEmojisRoute()}/names`, + {method: 'post', body: JSON.stringify(names)}, + ); + }; + getCustomEmojis = (page = 0, perPage = PER_PAGE_DEFAULT, sort = '') => { return this.doFetch( `${this.getEmojisRoute()}${buildQueryString({page, per_page: perPage, sort})}`,