From 8bb772638c5569e932d86956be0d6fe195e72f45 Mon Sep 17 00:00:00 2001 From: Harrison Healey Date: Thu, 29 Oct 2020 10:24:01 -0400 Subject: [PATCH] MM-29067 Add deterministic IDs for default sidebar categories (#16030) Automatic Merge --- api4/channel.go | 6 +-- model/channel_sidebar.go | 13 ++++++ model/channel_sidebar_test.go | 49 +++++++++++++++++++++ store/sqlstore/channel_store.go | 4 +- store/sqlstore/channel_store_categories.go | 11 +++-- store/sqlstore/upgrade.go | 14 ++++++ store/storetest/channel_store_categories.go | 24 ++++++++++ web/context.go | 2 +- 8 files changed, 113 insertions(+), 10 deletions(-) create mode 100644 model/channel_sidebar_test.go diff --git a/api4/channel.go b/api4/channel.go index 0d533d01a5..df262cb524 100644 --- a/api4/channel.go +++ b/api4/channel.go @@ -40,9 +40,9 @@ func (api *API) InitChannel() { api.BaseRoutes.ChannelCategories.Handle("", api.ApiSessionRequired(updateCategoriesForTeamForUser)).Methods("PUT") api.BaseRoutes.ChannelCategories.Handle("/order", api.ApiSessionRequired(getCategoryOrderForTeamForUser)).Methods("GET") api.BaseRoutes.ChannelCategories.Handle("/order", api.ApiSessionRequired(updateCategoryOrderForTeamForUser)).Methods("PUT") - api.BaseRoutes.ChannelCategories.Handle("/{category_id:[A-Za-z0-9]+}", api.ApiSessionRequired(getCategoryForTeamForUser)).Methods("GET") - api.BaseRoutes.ChannelCategories.Handle("/{category_id:[A-Za-z0-9]+}", api.ApiSessionRequired(updateCategoryForTeamForUser)).Methods("PUT") - api.BaseRoutes.ChannelCategories.Handle("/{category_id:[A-Za-z0-9]+}", api.ApiSessionRequired(deleteCategoryForTeamForUser)).Methods("DELETE") + api.BaseRoutes.ChannelCategories.Handle("/{category_id:[A-Za-z0-9_-]+}", api.ApiSessionRequired(getCategoryForTeamForUser)).Methods("GET") + api.BaseRoutes.ChannelCategories.Handle("/{category_id:[A-Za-z0-9_-]+}", api.ApiSessionRequired(updateCategoryForTeamForUser)).Methods("PUT") + api.BaseRoutes.ChannelCategories.Handle("/{category_id:[A-Za-z0-9_-]+}", api.ApiSessionRequired(deleteCategoryForTeamForUser)).Methods("DELETE") api.BaseRoutes.Channel.Handle("", api.ApiSessionRequired(getChannel)).Methods("GET") api.BaseRoutes.Channel.Handle("", api.ApiSessionRequired(updateChannel)).Methods("PUT") diff --git a/model/channel_sidebar.go b/model/channel_sidebar.go index 6a79593c9a..d05c6c9db4 100644 --- a/model/channel_sidebar.go +++ b/model/channel_sidebar.go @@ -6,6 +6,7 @@ package model import ( "encoding/json" "io" + "regexp" ) type SidebarCategoryType string @@ -109,3 +110,15 @@ func (o OrderedSidebarCategories) ToJson() []byte { return b } } + +var categoryIdPattern = regexp.MustCompile("(favorites|channels|direct_messages)_[a-z0-9]{26}_[a-z0-9]{26}") + +func IsValidCategoryId(s string) bool { + // Category IDs can either be regular IDs + if IsValidId(s) { + return true + } + + // Or default categories can follow the pattern {type}_{userID}_{teamID} + return categoryIdPattern.MatchString(s) +} diff --git a/model/channel_sidebar_test.go b/model/channel_sidebar_test.go new file mode 100644 index 0000000000..8d4d406c55 --- /dev/null +++ b/model/channel_sidebar_test.go @@ -0,0 +1,49 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package model + +import ( + "fmt" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestIsValidCategoryId(t *testing.T) { + for _, test := range []struct { + Name string + Input string + Expected bool + }{ + { + Name: "should accept a regular ID", + Input: NewId(), + Expected: true, + }, + { + Name: "should accept a favorites ID", + Input: fmt.Sprintf("favorites_%s_%s", NewId(), NewId()), + Expected: true, + }, + { + Name: "should accept a channels ID", + Input: fmt.Sprintf("channels_%s_%s", NewId(), NewId()), + Expected: true, + }, + { + Name: "should accept a direct messages ID", + Input: fmt.Sprintf("direct_messages_%s_%s", NewId(), NewId()), + Expected: true, + }, + { + Name: "should reject a garbage ID", + Input: "a garbage ID", + Expected: false, + }, + } { + t.Run(test.Name, func(t *testing.T) { + assert.Equal(t, test.Expected, IsValidCategoryId(test.Input)) + }) + } +} diff --git a/store/sqlstore/channel_store.go b/store/sqlstore/channel_store.go index b281ec52e4..6e0b9e4027 100644 --- a/store/sqlstore/channel_store.go +++ b/store/sqlstore/channel_store.go @@ -391,7 +391,7 @@ func newSqlChannelStore(sqlStore SqlStore, metrics einterfaces.MetricsInterface) tablePublicChannels.ColMap("Purpose").SetMaxSize(250) tableSidebarCategories := db.AddTableWithName(model.SidebarCategory{}, "SidebarCategories").SetKeys(false, "Id") - tableSidebarCategories.ColMap("Id").SetMaxSize(26) + tableSidebarCategories.ColMap("Id").SetMaxSize(128) tableSidebarCategories.ColMap("UserId").SetMaxSize(26) tableSidebarCategories.ColMap("TeamId").SetMaxSize(26) tableSidebarCategories.ColMap("Sorting").SetMaxSize(64) @@ -401,7 +401,7 @@ func newSqlChannelStore(sqlStore SqlStore, metrics einterfaces.MetricsInterface) tableSidebarChannels := db.AddTableWithName(model.SidebarChannel{}, "SidebarChannels").SetKeys(false, "ChannelId", "UserId", "CategoryId") tableSidebarChannels.ColMap("ChannelId").SetMaxSize(26) tableSidebarChannels.ColMap("UserId").SetMaxSize(26) - tableSidebarChannels.ColMap("CategoryId").SetMaxSize(26) + tableSidebarChannels.ColMap("CategoryId").SetMaxSize(128) } return s diff --git a/store/sqlstore/channel_store_categories.go b/store/sqlstore/channel_store_categories.go index 2c0a29305d..bdf6cb9112 100644 --- a/store/sqlstore/channel_store_categories.go +++ b/store/sqlstore/channel_store_categories.go @@ -53,9 +53,12 @@ func (s SqlChannelStore) createInitialSidebarCategoriesT(transaction *gorp.Trans hasCategoryOfType[existingType] = true } - if !hasCategoryOfType[model.SidebarCategoryFavorites] { - favoritesCategoryId := model.NewId() + // Use deterministic IDs for default categories to prevent potentially creating multiple copies of a default category + favoritesCategoryId := fmt.Sprintf("%s_%s_%s", model.SidebarCategoryFavorites, userId, teamId) + channelsCategoryId := fmt.Sprintf("%s_%s_%s", model.SidebarCategoryChannels, userId, teamId) + directMessagesCategoryId := fmt.Sprintf("%s_%s_%s", model.SidebarCategoryDirectMessages, userId, teamId) + if !hasCategoryOfType[model.SidebarCategoryFavorites] { // Create the SidebarChannels first since there's more opportunity for something to fail here if err := s.migrateFavoritesToSidebarT(transaction, userId, teamId, favoritesCategoryId); err != nil { return errors.Wrap(err, "createInitialSidebarCategoriesT: failed to migrate favorites to sidebar") @@ -77,7 +80,7 @@ func (s SqlChannelStore) createInitialSidebarCategoriesT(transaction *gorp.Trans if !hasCategoryOfType[model.SidebarCategoryChannels] { if err := transaction.Insert(&model.SidebarCategory{ DisplayName: "Channels", // This will be retranslateed by the client into the user's locale - Id: model.NewId(), + Id: channelsCategoryId, UserId: userId, TeamId: teamId, Sorting: model.SidebarCategorySortDefault, @@ -91,7 +94,7 @@ func (s SqlChannelStore) createInitialSidebarCategoriesT(transaction *gorp.Trans if !hasCategoryOfType[model.SidebarCategoryDirectMessages] { if err := transaction.Insert(&model.SidebarCategory{ DisplayName: "Direct Messages", // This will be retranslateed by the client into the user's locale - Id: model.NewId(), + Id: directMessagesCategoryId, UserId: userId, TeamId: teamId, Sorting: model.SidebarCategorySortRecent, diff --git a/store/sqlstore/upgrade.go b/store/sqlstore/upgrade.go index a9bbf014e5..93737ae13c 100644 --- a/store/sqlstore/upgrade.go +++ b/store/sqlstore/upgrade.go @@ -20,6 +20,7 @@ import ( const ( CURRENT_SCHEMA_VERSION = VERSION_5_28_1 + VERSION_5_29_0 = "5.29.0" VERSION_5_28_1 = "5.28.1" VERSION_5_28_0 = "5.28.0" VERSION_5_27_0 = "5.27.0" @@ -190,6 +191,7 @@ func upgradeDatabase(sqlStore SqlStore, currentModelVersionString string) error upgradeDatabaseToVersion527(sqlStore) upgradeDatabaseToVersion528(sqlStore) upgradeDatabaseToVersion5281(sqlStore) + upgradeDatabaseToVersion529(sqlStore) return nil } @@ -915,3 +917,15 @@ func precheckMigrationToVersion528(sqlStore SqlStore) error { return nil } + +func upgradeDatabaseToVersion529(sqlStore SqlStore) { + // if shouldPerformUpgrade(sqlStore, VERSION_5_28_0, VERSION_5_29_0) { + + sqlStore.AlterColumnTypeIfExists("SidebarCategories", "Id", "VARCHAR(128)", "VARCHAR(128)") + sqlStore.AlterColumnDefaultIfExists("SidebarCategories", "Id", model.NewString(""), nil) + sqlStore.AlterColumnTypeIfExists("SidebarChannels", "CategoryId", "VARCHAR(128)", "VARCHAR(128)") + sqlStore.AlterColumnDefaultIfExists("SidebarChannels", "CategoryId", model.NewString(""), nil) + + // saveSchemaVersion(sqlStore, VERSION_5_29_0) + // } +} diff --git a/store/storetest/channel_store_categories.go b/store/storetest/channel_store_categories.go index 77c0ed0837..305a51d4f0 100644 --- a/store/storetest/channel_store_categories.go +++ b/store/storetest/channel_store_categories.go @@ -6,6 +6,7 @@ package storetest import ( "database/sql" "errors" + "sync" "testing" "github.com/mattermost/mattermost-server/v5/model" @@ -99,6 +100,29 @@ func testCreateInitialSidebarCategories(t *testing.T, ss store.Store) { assert.Equal(t, initialCategories.Categories, res.Categories) }) + t.Run("shouldn't create additional categories when ones already exist even when ran simultaneously", func(t *testing.T) { + userId := model.NewId() + teamId := model.NewId() + + var wg sync.WaitGroup + + for i := 0; i < 10; i++ { + wg.Add(1) + + go func() { + defer wg.Done() + + _ = ss.Channel().CreateInitialSidebarCategories(userId, teamId) + }() + } + + wg.Wait() + + res, err := ss.Channel().GetSidebarCategories(userId, teamId) + assert.Nil(t, err) + assert.Len(t, res.Categories, 3) + }) + t.Run("should populate the Favorites category with regular channels", func(t *testing.T) { userId := model.NewId() teamId := model.NewId() diff --git a/web/context.go b/web/context.go index 0927baec60..34d5d8a23b 100644 --- a/web/context.go +++ b/web/context.go @@ -301,7 +301,7 @@ func (c *Context) RequireCategoryId() *Context { return c } - if len(c.Params.CategoryId) != 26 { + if !model.IsValidCategoryId(c.Params.CategoryId) { c.SetInvalidUrlParam("category_id") } return c