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
2025-02-25 18:55:24 -06:00 · 2020-01-17 17:16:18 -04:00
parent 7d99d8fba7
commit 6d79484e55
3 changed files with 56 additions and 37 deletions
--- 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
--- 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)
 }
--- 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