From 6d79484e55c9d89abebbde2d2fcef53c0015c79c Mon Sep 17 00:00:00 2001 From: Jesse Hallam Date: Fri, 17 Jan 2020 17:16:18 -0400 Subject: [PATCH] MM-21645: COALESCE(Bots.LastIconUpdate, 0) (#13638) Handle a potentially `NULL` Bots.LastIconUpdate and `COALESCE` back to `0`. This column has no default, and while v5.20 should never write it as `NULL`, any pre-v5.20 server writing to the new schema will. As a result, this change ensures backwards compatibility. Fixes: https://mattermost.atlassian.net/browse/MM-21645 Fixes: https://mattermost.atlassian.net/browse/MM-21585 --- store/sqlstore/bot_store.go | 4 +- store/sqlstore/bot_store_test.go | 2 +- store/storetest/bot_store.go | 87 +++++++++++++++++++------------- 3 files changed, 56 insertions(+), 37 deletions(-) diff --git a/store/sqlstore/bot_store.go b/store/sqlstore/bot_store.go index b51fb2e017..c9b72ec018 100644 --- a/store/sqlstore/bot_store.go +++ b/store/sqlstore/bot_store.go @@ -91,7 +91,7 @@ func (us SqlBotStore) Get(botUserId string, includeDeleted bool) (*model.Bot, *m u.FirstName AS DisplayName, b.Description, b.OwnerId, - b.LastIconUpdate, + COALESCE(b.LastIconUpdate, 0) AS LastIconUpdate, b.CreateAt, b.UpdateAt, b.DeleteAt @@ -148,7 +148,7 @@ func (us SqlBotStore) GetAll(options *model.BotGetOptions) ([]*model.Bot, *model u.FirstName AS DisplayName, b.Description, b.OwnerId, - b.LastIconUpdate, + COALESCE(b.LastIconUpdate, 0) AS LastIconUpdate, b.CreateAt, b.UpdateAt, b.DeleteAt diff --git a/store/sqlstore/bot_store_test.go b/store/sqlstore/bot_store_test.go index 28f28a260f..691135f23d 100644 --- a/store/sqlstore/bot_store_test.go +++ b/store/sqlstore/bot_store_test.go @@ -10,5 +10,5 @@ import ( ) func TestBotStore(t *testing.T) { - StoreTest(t, storetest.TestBotStore) + StoreTestWithSqlSupplier(t, storetest.TestBotStore) } diff --git a/store/storetest/bot_store.go b/store/storetest/bot_store.go index 0eb63a85a6..5d2b15b782 100644 --- a/store/storetest/bot_store.go +++ b/store/storetest/bot_store.go @@ -24,19 +24,20 @@ func makeBotWithUser(t *testing.T, ss store.Store, bot *model.Bot) (*model.Bot, return bot, user } -func TestBotStore(t *testing.T, ss store.Store) { - t.Run("Get", func(t *testing.T) { testBotStoreGet(t, ss) }) - t.Run("GetAll", func(t *testing.T) { testBotStoreGetAll(t, ss) }) +func TestBotStore(t *testing.T, ss store.Store, s SqlSupplier) { + t.Run("Get", func(t *testing.T) { testBotStoreGet(t, ss, s) }) + t.Run("GetAll", func(t *testing.T) { testBotStoreGetAll(t, ss, s) }) t.Run("Save", func(t *testing.T) { testBotStoreSave(t, ss) }) t.Run("Update", func(t *testing.T) { testBotStoreUpdate(t, ss) }) t.Run("PermanentDelete", func(t *testing.T) { testBotStorePermanentDelete(t, ss) }) } -func testBotStoreGet(t *testing.T, ss store.Store) { +func testBotStoreGet(t *testing.T, ss store.Store, s SqlSupplier) { deletedBot, _ := makeBotWithUser(t, ss, &model.Bot{ - Username: "deleted_bot", - Description: "A deleted bot", - OwnerId: model.NewId(), + Username: "deleted_bot", + Description: "A deleted bot", + OwnerId: model.NewId(), + LastIconUpdate: model.GetMillis(), }) deletedBot.DeleteAt = 1 deletedBot, err := ss.Bot().Update(deletedBot) @@ -45,30 +46,37 @@ func testBotStoreGet(t *testing.T, ss store.Store) { defer func() { require.Nil(t, ss.User().PermanentDelete(deletedBot.UserId)) }() permanentlyDeletedBot, _ := makeBotWithUser(t, ss, &model.Bot{ - Username: "permanently_deleted_bot", - Description: "A permanently deleted bot", - OwnerId: model.NewId(), - DeleteAt: 0, + Username: "permanently_deleted_bot", + Description: "A permanently deleted bot", + OwnerId: model.NewId(), + LastIconUpdate: model.GetMillis(), + DeleteAt: 0, }) require.Nil(t, ss.Bot().PermanentDelete(permanentlyDeletedBot.UserId)) defer func() { require.Nil(t, ss.User().PermanentDelete(permanentlyDeletedBot.UserId)) }() b1, _ := makeBotWithUser(t, ss, &model.Bot{ - Username: "b1", - Description: "The first bot", - OwnerId: model.NewId(), + Username: "b1", + Description: "The first bot", + OwnerId: model.NewId(), + LastIconUpdate: model.GetMillis(), }) defer func() { require.Nil(t, ss.Bot().PermanentDelete(b1.UserId)) }() defer func() { require.Nil(t, ss.User().PermanentDelete(b1.UserId)) }() b2, _ := makeBotWithUser(t, ss, &model.Bot{ - Username: "b2", - Description: "The second bot", - OwnerId: model.NewId(), + Username: "b2", + Description: "The second bot", + OwnerId: model.NewId(), + LastIconUpdate: 0, }) defer func() { require.Nil(t, ss.Bot().PermanentDelete(b2.UserId)) }() defer func() { require.Nil(t, ss.User().PermanentDelete(b2.UserId)) }() + // Artificially set b2.LastIconUpdate to NULL to verify handling of same. + _, sqlErr := s.GetMaster().Exec("UPDATE Bots SET LastIconUpdate = NULL WHERE UserId = '" + b2.UserId + "'") + require.NoError(t, sqlErr) + t.Run("get non-existent bot", func(t *testing.T) { _, err := ss.Bot().Get("unknown", false) require.NotNil(t, err) @@ -106,14 +114,15 @@ func testBotStoreGet(t *testing.T, ss store.Store) { }) } -func testBotStoreGetAll(t *testing.T, ss store.Store) { +func testBotStoreGetAll(t *testing.T, ss store.Store, s SqlSupplier) { OwnerId1 := model.NewId() OwnerId2 := model.NewId() deletedBot, _ := makeBotWithUser(t, ss, &model.Bot{ - Username: "deleted_bot", - Description: "A deleted bot", - OwnerId: OwnerId1, + Username: "deleted_bot", + Description: "A deleted bot", + OwnerId: OwnerId1, + LastIconUpdate: model.GetMillis(), }) deletedBot.DeleteAt = 1 deletedBot, err := ss.Bot().Update(deletedBot) @@ -122,30 +131,37 @@ func testBotStoreGetAll(t *testing.T, ss store.Store) { defer func() { require.Nil(t, ss.User().PermanentDelete(deletedBot.UserId)) }() permanentlyDeletedBot, _ := makeBotWithUser(t, ss, &model.Bot{ - Username: "permanently_deleted_bot", - Description: "A permanently deleted bot", - OwnerId: OwnerId1, - DeleteAt: 0, + Username: "permanently_deleted_bot", + Description: "A permanently deleted bot", + OwnerId: OwnerId1, + LastIconUpdate: model.GetMillis(), + DeleteAt: 0, }) require.Nil(t, ss.Bot().PermanentDelete(permanentlyDeletedBot.UserId)) defer func() { require.Nil(t, ss.User().PermanentDelete(permanentlyDeletedBot.UserId)) }() b1, _ := makeBotWithUser(t, ss, &model.Bot{ - Username: "b1", - Description: "The first bot", - OwnerId: OwnerId1, + Username: "b1", + Description: "The first bot", + OwnerId: OwnerId1, + LastIconUpdate: model.GetMillis(), }) defer func() { require.Nil(t, ss.Bot().PermanentDelete(b1.UserId)) }() defer func() { require.Nil(t, ss.User().PermanentDelete(b1.UserId)) }() b2, _ := makeBotWithUser(t, ss, &model.Bot{ - Username: "b2", - Description: "The second bot", - OwnerId: OwnerId1, + Username: "b2", + Description: "The second bot", + OwnerId: OwnerId1, + LastIconUpdate: 0, }) defer func() { require.Nil(t, ss.Bot().PermanentDelete(b2.UserId)) }() defer func() { require.Nil(t, ss.User().PermanentDelete(b2.UserId)) }() + // Artificially set b2.LastIconUpdate to NULL to verify handling of same. + _, sqlErr := s.GetMaster().Exec("UPDATE Bots SET LastIconUpdate = NULL WHERE UserId = '" + b2.UserId + "'") + require.NoError(t, sqlErr) + t.Run("get original bots", func(t *testing.T) { bot, err := ss.Bot().GetAll(&model.BotGetOptions{Page: 0, PerPage: 10}) require.Nil(t, err) @@ -364,9 +380,10 @@ func testBotStoreUpdate(t *testing.T, ss store.Store) { bot := existingBot.Clone() bot.OwnerId = model.NewId() bot.Description = "updated description" - bot.CreateAt = 999999 // Ignored - bot.UpdateAt = 999999 // Ignored - bot.DeleteAt = 100000 // Allowed + bot.CreateAt = 999999 // Ignored + bot.UpdateAt = 999999 // Ignored + bot.LastIconUpdate = 100000 // Allowed + bot.DeleteAt = 100000 // Allowed returnedBot, err := ss.Bot().Update(bot) require.Nil(t, err) @@ -376,6 +393,8 @@ func testBotStoreUpdate(t *testing.T, ss store.Store) { require.NotEqual(t, bot.UpdateAt, returnedBot.UpdateAt, "update should have advanced UpdateAt") require.True(t, returnedBot.UpdateAt > bot.UpdateAt, "update should have advanced UpdateAt") require.NotEqual(t, 99999, returnedBot.UpdateAt, "should have ignored user-provided UpdateAt") + require.Equal(t, bot.LastIconUpdate, returnedBot.LastIconUpdate, "should have marked icon as updated") + require.Equal(t, bot.DeleteAt, returnedBot.DeleteAt, "should have marked bot as deleted") bot.CreateAt = returnedBot.CreateAt bot.UpdateAt = returnedBot.UpdateAt