[GH-25484] Fix draft removal on post deletion (#25715)

* [GH-25484] Fix draft removal on post deletion

* [GH-25484] Add batch migration to remove orphan drafts

* [GH-25484] Fix tests of migration and draft store

* [GH-25484] Remove translation file changes.

* [GH-25484] Remove translation file changes.

---------

Co-authored-by: Devin Binnie <52460000+devinbinnie@users.noreply.github.com>
Co-authored-by: Mattermost Build <build@mattermost.com>
Co-authored-by: Harrison Healey <harrisonmhealey@gmail.com>
Этот коммит содержится в:
Utsav Ladani
2024-01-29 20:25:34 +05:30
коммит произвёл GitHub
родитель 435da9bea7
Коммит 3ac6edb406
19 изменённых файлов: 1046 добавлений и 15 удалений

Просмотреть файл

@@ -19,10 +19,12 @@ func TestDraftStore(t *testing.T, rctx request.CTX, ss store.Store, s SqlStore)
t.Run("SaveDraft", func(t *testing.T) { testSaveDraft(t, rctx, ss) })
t.Run("UpdateDraft", func(t *testing.T) { testUpdateDraft(t, rctx, ss) })
t.Run("DeleteDraft", func(t *testing.T) { testDeleteDraft(t, rctx, ss) })
t.Run("DeleteDraftsAssociatedWithPost", func(t *testing.T) { testDeleteDraftsAssociatedWithPost(t, rctx, ss) })
t.Run("GetDraft", func(t *testing.T) { testGetDraft(t, rctx, ss) })
t.Run("GetDraftsForUser", func(t *testing.T) { testGetDraftsForUser(t, rctx, ss) })
t.Run("GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration", func(t *testing.T) { testGetLastCreateAtAndUserIdValuesForEmptyDraftsMigration(t, rctx, ss) })
t.Run("DeleteEmptyDraftsByCreateAtAndUserId", func(t *testing.T) { testDeleteEmptyDraftsByCreateAtAndUserId(t, rctx, ss) })
t.Run("DeleteOrphanDraftsByCreateAtAndUserId", func(t *testing.T) { testDeleteOrphanDraftsByCreateAtAndUserId(t, rctx, ss) })
}
func testSaveDraft(t *testing.T, rctx request.CTX, ss store.Store) {
@@ -371,6 +373,16 @@ func makeDrafts(t *testing.T, ss store.Store, count int, message string) {
}
}
func countDrafts(t *testing.T, rctx request.CTX, ss store.Store) int {
t.Helper()
var count int
err := ss.GetInternalMasterDB().QueryRow("SELECT COUNT(*) FROM Drafts").Scan(&count)
require.NoError(t, err)
return count
}
func countDraftPages(t *testing.T, rctx request.CTX, ss store.Store) int {
t.Helper()
@@ -401,6 +413,76 @@ func countDraftPages(t *testing.T, rctx request.CTX, ss store.Store) int {
return pages
}
func clearPosts(t *testing.T, rctx request.CTX, ss store.Store) {
t.Helper()
_, err := ss.GetInternalMasterDB().Exec("DELETE FROM Posts")
require.NoError(t, err)
}
func makeDraftsWithNonDeletedPosts(t *testing.T, ss store.Store, count int, message string) {
t.Helper()
for i := 1; i <= count; i++ {
post, err := ss.Post().Save(&model.Post{
CreateAt: model.GetMillis(),
UpdateAt: model.GetMillis(),
UserId: model.NewId(),
ChannelId: model.NewId(),
Message: message,
})
require.NoError(t, err)
_, err = ss.Draft().Upsert(&model.Draft{
CreateAt: model.GetMillis(),
UpdateAt: model.GetMillis(),
UserId: post.UserId,
ChannelId: post.ChannelId,
RootId: post.Id,
Message: message,
})
require.NoError(t, err)
if i%100 == 0 {
time.Sleep(5 * time.Millisecond)
}
}
time.Sleep(5 * time.Millisecond)
}
func makeDraftsWithDeletedPosts(t *testing.T, ss store.Store, count int, message string) {
t.Helper()
for i := 1; i <= count; i++ {
post, err := ss.Post().Save(&model.Post{
CreateAt: model.GetMillis(),
UpdateAt: model.GetMillis(),
DeleteAt: model.GetMillis(),
UserId: model.NewId(),
ChannelId: model.NewId(),
Message: message,
})
require.NoError(t, err)
_, err = ss.Draft().Upsert(&model.Draft{
CreateAt: model.GetMillis(),
UpdateAt: model.GetMillis(),
UserId: post.UserId,
ChannelId: post.ChannelId,
RootId: post.Id,
Message: message,
})
require.NoError(t, err)
if i%100 == 0 {
time.Sleep(5 * time.Millisecond)
}
}
time.Sleep(5 * time.Millisecond)
}
func testGetLastCreateAtAndUserIdValuesForEmptyDraftsMigration(t *testing.T, rctx request.CTX, ss store.Store) {
t.Run("no drafts", func(t *testing.T) {
clearDrafts(t, rctx, ss)
@@ -560,3 +642,377 @@ func testDeleteEmptyDraftsByCreateAtAndUserId(t *testing.T, rctx request.CTX, ss
assert.Equal(t, "", nextUserId, "should have finished iterating through drafts")
})
}
func testDeleteOrphanDraftsByCreateAtAndUserId(t *testing.T, rctx request.CTX, ss store.Store) {
t.Run("nil parameters", func(t *testing.T) {
clearDrafts(t, rctx, ss)
clearPosts(t, rctx, ss)
err := ss.Draft().DeleteOrphanDraftsByCreateAtAndUserId(0, "")
require.NoError(t, err)
})
t.Run("delete single page, drafts with no post", func(t *testing.T) {
clearDrafts(t, rctx, ss)
clearPosts(t, rctx, ss)
makeDrafts(t, ss, 100, "Okay")
createAt, userId := int64(0), ""
nextCreateAt, nextUserId, err := ss.Draft().GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration(createAt, userId)
require.NoError(t, err)
err = ss.Draft().DeleteOrphanDraftsByCreateAtAndUserId(createAt, userId)
require.NoError(t, err)
createAt, userId = nextCreateAt, nextUserId
assert.Equal(t, 0, countDraftPages(t, rctx, ss), "incorrect number of pages")
nextCreateAt, nextUserId, err = ss.Draft().GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration(createAt, userId)
require.NoError(t, err)
assert.EqualValues(t, 0, nextCreateAt, "should have finished iterating through drafts")
assert.Equal(t, "", nextUserId, "should have finished iterating through drafts")
})
t.Run("delete multiple pages, drafts with no post", func(t *testing.T) {
clearDrafts(t, rctx, ss)
clearPosts(t, rctx, ss)
makeDrafts(t, ss, 300, "Okay")
createAt, userId := int64(0), ""
nextCreateAt, nextUserId, err := ss.Draft().GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration(createAt, userId)
require.NoError(t, err)
err = ss.Draft().DeleteOrphanDraftsByCreateAtAndUserId(createAt, userId)
require.NoError(t, err)
createAt, userId = nextCreateAt, nextUserId
assert.Equal(t, 2, countDraftPages(t, rctx, ss), "incorrect number of pages")
nextCreateAt, nextUserId, err = ss.Draft().GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration(createAt, userId)
require.NoError(t, err)
err = ss.Draft().DeleteOrphanDraftsByCreateAtAndUserId(createAt, userId)
require.NoError(t, err)
createAt, userId = nextCreateAt, nextUserId
assert.Equal(t, 1, countDraftPages(t, rctx, ss), "incorrect number of pages")
nextCreateAt, nextUserId, err = ss.Draft().GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration(createAt, userId)
require.NoError(t, err)
err = ss.Draft().DeleteOrphanDraftsByCreateAtAndUserId(createAt, userId)
require.NoError(t, err)
createAt, userId = nextCreateAt, nextUserId
assert.Equal(t, 0, countDraftPages(t, rctx, ss), "incorrect number of pages")
nextCreateAt, nextUserId, err = ss.Draft().GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration(createAt, userId)
require.NoError(t, err)
assert.EqualValues(t, 0, nextCreateAt, "should have finished iterating through drafts")
assert.Equal(t, "", nextUserId, "should have finished iterating through drafts")
})
t.Run("delete single page, drafts with deleted post", func(t *testing.T) {
clearDrafts(t, rctx, ss)
clearPosts(t, rctx, ss)
makeDraftsWithDeletedPosts(t, ss, 100, "Okay")
createAt, userId := int64(0), ""
nextCreateAt, nextUserId, err := ss.Draft().GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration(createAt, userId)
require.NoError(t, err)
err = ss.Draft().DeleteOrphanDraftsByCreateAtAndUserId(createAt, userId)
require.NoError(t, err)
createAt, userId = nextCreateAt, nextUserId
assert.Equal(t, 0, countDraftPages(t, rctx, ss), "incorrect number of pages")
nextCreateAt, nextUserId, err = ss.Draft().GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration(createAt, userId)
require.NoError(t, err)
assert.EqualValues(t, 0, nextCreateAt, "should have finished iterating through drafts")
assert.Equal(t, "", nextUserId, "should have finished iterating through drafts")
})
t.Run("delete multiple pages, drafts with deleted post", func(t *testing.T) {
clearDrafts(t, rctx, ss)
clearPosts(t, rctx, ss)
makeDraftsWithDeletedPosts(t, ss, 300, "Okay")
createAt, userId := int64(0), ""
nextCreateAt, nextUserId, err := ss.Draft().GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration(createAt, userId)
require.NoError(t, err)
err = ss.Draft().DeleteOrphanDraftsByCreateAtAndUserId(createAt, userId)
require.NoError(t, err)
createAt, userId = nextCreateAt, nextUserId
assert.Equal(t, 2, countDraftPages(t, rctx, ss), "incorrect number of pages")
nextCreateAt, nextUserId, err = ss.Draft().GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration(createAt, userId)
require.NoError(t, err)
err = ss.Draft().DeleteOrphanDraftsByCreateAtAndUserId(createAt, userId)
require.NoError(t, err)
createAt, userId = nextCreateAt, nextUserId
assert.Equal(t, 1, countDraftPages(t, rctx, ss), "incorrect number of pages")
nextCreateAt, nextUserId, err = ss.Draft().GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration(createAt, userId)
require.NoError(t, err)
err = ss.Draft().DeleteOrphanDraftsByCreateAtAndUserId(createAt, userId)
require.NoError(t, err)
createAt, userId = nextCreateAt, nextUserId
assert.Equal(t, 0, countDraftPages(t, rctx, ss), "incorrect number of pages")
nextCreateAt, nextUserId, err = ss.Draft().GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration(createAt, userId)
require.NoError(t, err)
assert.EqualValues(t, 0, nextCreateAt, "should have finished iterating through drafts")
assert.Equal(t, "", nextUserId, "should have finished iterating through drafts")
})
t.Run("delete single page, drafts with non deleted post", func(t *testing.T) {
clearDrafts(t, rctx, ss)
clearPosts(t, rctx, ss)
makeDraftsWithNonDeletedPosts(t, ss, 100, "Okay")
createAt, userId := int64(0), ""
nextCreateAt, nextUserId, err := ss.Draft().GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration(createAt, userId)
require.NoError(t, err)
err = ss.Draft().DeleteOrphanDraftsByCreateAtAndUserId(createAt, userId)
require.NoError(t, err)
createAt, userId = nextCreateAt, nextUserId
assert.Equal(t, 100, countDrafts(t, rctx, ss), "incorrect number of drafts")
assert.Equal(t, 1, countDraftPages(t, rctx, ss), "incorrect number of pages")
nextCreateAt, nextUserId, err = ss.Draft().GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration(createAt, userId)
require.NoError(t, err)
assert.EqualValues(t, 0, nextCreateAt, "should have finished iterating through drafts")
assert.Equal(t, "", nextUserId, "should have finished iterating through drafts")
})
t.Run("delete multiple pages, drafts with non deleted post", func(t *testing.T) {
clearDrafts(t, rctx, ss)
clearPosts(t, rctx, ss)
makeDraftsWithNonDeletedPosts(t, ss, 300, "Okay")
createAt, userId := int64(0), ""
nextCreateAt, nextUserId, err := ss.Draft().GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration(createAt, userId)
require.NoError(t, err)
err = ss.Draft().DeleteOrphanDraftsByCreateAtAndUserId(createAt, userId)
require.NoError(t, err)
createAt, userId = nextCreateAt, nextUserId
assert.Equal(t, 300, countDrafts(t, rctx, ss), "incorrect number of drafts")
assert.Equal(t, 3, countDraftPages(t, rctx, ss), "incorrect number of pages")
nextCreateAt, nextUserId, err = ss.Draft().GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration(createAt, userId)
require.NoError(t, err)
err = ss.Draft().DeleteOrphanDraftsByCreateAtAndUserId(createAt, userId)
require.NoError(t, err)
createAt, userId = nextCreateAt, nextUserId
assert.Equal(t, 300, countDrafts(t, rctx, ss), "incorrect number of drafts")
assert.Equal(t, 3, countDraftPages(t, rctx, ss), "incorrect number of pages")
nextCreateAt, nextUserId, err = ss.Draft().GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration(createAt, userId)
require.NoError(t, err)
err = ss.Draft().DeleteOrphanDraftsByCreateAtAndUserId(createAt, userId)
require.NoError(t, err)
createAt, userId = nextCreateAt, nextUserId
assert.Equal(t, 300, countDrafts(t, rctx, ss), "incorrect number of drafts")
assert.Equal(t, 3, countDraftPages(t, rctx, ss), "incorrect number of pages")
nextCreateAt, nextUserId, err = ss.Draft().GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration(createAt, userId)
require.NoError(t, err)
assert.EqualValues(t, 0, nextCreateAt, "should have finished iterating through drafts")
assert.Equal(t, "", nextUserId, "should have finished iterating through drafts")
})
// This test is a bit more complicated, but it's the most realistic scenario and covers all the remaining cases
t.Run("delete multiple pages, some drafts with deleted post, some with non deleted post, and some with no post", func(t *testing.T) {
clearDrafts(t, rctx, ss)
clearPosts(t, rctx, ss)
// 50 drafts will be deleted from this page
makeDrafts(t, ss, 50, "Yup")
makeDraftsWithNonDeletedPosts(t, ss, 50, "Okay")
// 100 drafts will be deleted from this page
makeDrafts(t, ss, 50, "Yup")
makeDraftsWithDeletedPosts(t, ss, 50, "Okay")
// 50 drafts will be deleted from this page
makeDraftsWithDeletedPosts(t, ss, 50, "Okay")
makeDraftsWithNonDeletedPosts(t, ss, 50, "Okay")
// 70 drafts will be deleted from this page
makeDrafts(t, ss, 40, "Yup")
makeDraftsWithDeletedPosts(t, ss, 30, "Okay")
makeDraftsWithNonDeletedPosts(t, ss, 30, "Okay")
// No drafts will be deleted from this page
makeDraftsWithNonDeletedPosts(t, ss, 100, "Okay")
// Verify initially 5 pages with 500 drafts
assert.Equal(t, 5, countDraftPages(t, rctx, ss), "incorrect number of pages")
assert.Equal(t, 500, countDrafts(t, rctx, ss), "incorrect number of drafts")
createAt, userId := int64(0), ""
nextCreateAt, nextUserId, err := ss.Draft().GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration(createAt, userId)
require.NoError(t, err)
err = ss.Draft().DeleteOrphanDraftsByCreateAtAndUserId(createAt, userId)
require.NoError(t, err)
createAt, userId = nextCreateAt, nextUserId
// Only deleted 50, so still 5 pages
assert.Equal(t, 5, countDraftPages(t, rctx, ss), "incorrect number of pages")
assert.Equal(t, 450, countDrafts(t, rctx, ss), "incorrect number of drafts")
nextCreateAt, nextUserId, err = ss.Draft().GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration(createAt, userId)
require.NoError(t, err)
err = ss.Draft().DeleteOrphanDraftsByCreateAtAndUserId(createAt, userId)
require.NoError(t, err)
createAt, userId = nextCreateAt, nextUserId
// Now deleted 150, so down to 4 pages
assert.Equal(t, 4, countDraftPages(t, rctx, ss), "incorrect number of pages")
assert.Equal(t, 350, countDrafts(t, rctx, ss), "incorrect number of drafts")
nextCreateAt, nextUserId, err = ss.Draft().GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration(createAt, userId)
require.NoError(t, err)
err = ss.Draft().DeleteOrphanDraftsByCreateAtAndUserId(createAt, userId)
require.NoError(t, err)
createAt, userId = nextCreateAt, nextUserId
// Now deleted 200 now, so down to 3 pages
assert.Equal(t, 3, countDraftPages(t, rctx, ss), "incorrect number of pages")
assert.Equal(t, 300, countDrafts(t, rctx, ss), "incorrect number of drafts")
nextCreateAt, nextUserId, err = ss.Draft().GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration(createAt, userId)
require.NoError(t, err)
err = ss.Draft().DeleteOrphanDraftsByCreateAtAndUserId(createAt, userId)
require.NoError(t, err)
createAt, userId = nextCreateAt, nextUserId
// Now deleted 270 empty messages, so still 3 pages
assert.Equal(t, 3, countDraftPages(t, rctx, ss), "incorrect number of pages")
assert.Equal(t, 230, countDrafts(t, rctx, ss), "incorrect number of drafts")
// Keep going through all pages to verify nothing else gets deleted.
nextCreateAt, nextUserId, err = ss.Draft().GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration(createAt, userId)
require.NoError(t, err)
err = ss.Draft().DeleteOrphanDraftsByCreateAtAndUserId(createAt, userId)
require.NoError(t, err)
createAt, userId = nextCreateAt, nextUserId
// Verify we're done iterating
nextCreateAt, nextUserId, err = ss.Draft().GetLastCreateAtAndUserIdValuesForEmptyDraftsMigration(createAt, userId)
require.NoError(t, err)
assert.EqualValues(t, 0, nextCreateAt, "should have finished iterating through drafts")
assert.Equal(t, "", nextUserId, "should have finished iterating through drafts")
})
}
func testDeleteDraftsAssociatedWithPost(t *testing.T, rctx request.CTX, ss store.Store) {
user1 := &model.User{
Id: model.NewId(),
}
user2 := &model.User{
Id: model.NewId(),
}
channel1 := &model.Channel{
Id: model.NewId(),
}
channel2 := &model.Channel{
Id: model.NewId(),
}
_, err := ss.Channel().SaveMember(&model.ChannelMember{
ChannelId: channel1.Id,
UserId: user1.Id,
NotifyProps: model.GetDefaultChannelNotifyProps(),
})
require.NoError(t, err)
_, err = ss.Channel().SaveMember(&model.ChannelMember{
ChannelId: channel2.Id,
UserId: user2.Id,
NotifyProps: model.GetDefaultChannelNotifyProps(),
})
require.NoError(t, err)
post1, err := ss.Post().Save(&model.Post{
UserId: user1.Id,
ChannelId: channel1.Id,
Message: "post1",
})
require.NoError(t, err)
post2, err := ss.Post().Save(&model.Post{
UserId: user2.Id,
ChannelId: channel2.Id,
Message: "post2",
})
require.NoError(t, err)
_, err = ss.Draft().Upsert(&model.Draft{
UserId: user1.Id,
ChannelId: channel1.Id,
RootId: post1.Id,
Message: "draft1",
})
require.NoError(t, err)
_, err = ss.Draft().Upsert(&model.Draft{
UserId: user2.Id,
ChannelId: channel1.Id,
RootId: post1.Id,
Message: "draft2",
})
require.NoError(t, err)
draft3, err := ss.Draft().Upsert(&model.Draft{
UserId: user1.Id,
ChannelId: channel2.Id,
RootId: post2.Id,
Message: "draft3",
})
require.NoError(t, err)
draft4, err := ss.Draft().Upsert(&model.Draft{
UserId: user2.Id,
ChannelId: channel2.Id,
RootId: post2.Id,
Message: "draft4",
})
require.NoError(t, err)
t.Run("delete drafts associated with post", func(t *testing.T) {
err = ss.Draft().DeleteDraftsAssociatedWithPost(channel1.Id, post1.Id)
require.NoError(t, err)
_, err = ss.Draft().Get(user1.Id, channel1.Id, post1.Id, false)
require.Error(t, err)
assert.IsType(t, &store.ErrNotFound{}, err)
_, err = ss.Draft().Get(user2.Id, channel1.Id, post1.Id, false)
require.Error(t, err)
assert.IsType(t, &store.ErrNotFound{}, err)
draft, err := ss.Draft().Get(user1.Id, channel2.Id, post2.Id, false)
require.NoError(t, err)
assert.Equal(t, draft3.Message, draft.Message)
draft, err = ss.Draft().Get(user2.Id, channel2.Id, post2.Id, false)
require.NoError(t, err)
assert.Equal(t, draft4.Message, draft.Message)
})
}

Просмотреть файл

@@ -28,6 +28,20 @@ func (_m *DraftStore) Delete(userID string, channelID string, rootID string) err
return r0
}
// DeleteDraftsAssociatedWithPost provides a mock function with given fields: channelID, rootID
func (_m *DraftStore) DeleteDraftsAssociatedWithPost(channelID string, rootID string) error {
ret := _m.Called(channelID, rootID)
var r0 error
if rf, ok := ret.Get(0).(func(string, string) error); ok {
r0 = rf(channelID, rootID)
} else {
r0 = ret.Error(0)
}
return r0
}
// DeleteEmptyDraftsByCreateAtAndUserId provides a mock function with given fields: createAt, userId
func (_m *DraftStore) DeleteEmptyDraftsByCreateAtAndUserId(createAt int64, userId string) error {
ret := _m.Called(createAt, userId)
@@ -42,6 +56,20 @@ func (_m *DraftStore) DeleteEmptyDraftsByCreateAtAndUserId(createAt int64, userI
return r0
}
// DeleteOrphanDraftsByCreateAtAndUserId provides a mock function with given fields: createAt, userId
func (_m *DraftStore) DeleteOrphanDraftsByCreateAtAndUserId(createAt int64, userId string) error {
ret := _m.Called(createAt, userId)
var r0 error
if rf, ok := ret.Get(0).(func(int64, string) error); ok {
r0 = rf(createAt, userId)
} else {
r0 = ret.Error(0)
}
return r0
}
// Get provides a mock function with given fields: userID, channelID, rootID, includeDeleted
func (_m *DraftStore) Get(userID string, channelID string, rootID string, includeDeleted bool) (*model.Draft, error) {
ret := _m.Called(userID, channelID, rootID, includeDeleted)