From 72f3bb10c761f2506ea159f09cc6f6c3da373219 Mon Sep 17 00:00:00 2001 From: Claudio Costa Date: Wed, 4 Dec 2019 16:56:08 +0100 Subject: [PATCH] [MM-20761] Fix and improve integrity check tool (#13246) * Fix query and add tests * Support NULL values and improve output --- cmd/mattermost/commands/integrity.go | 24 +++- store/sqlstore/integrity.go | 21 ++-- store/sqlstore/integrity_test.go | 172 ++++++++++++++++----------- store/store.go | 4 +- 4 files changed, 136 insertions(+), 85 deletions(-) diff --git a/cmd/mattermost/commands/integrity.go b/cmd/mattermost/commands/integrity.go index 433cdee6be..160bdf58bc 100644 --- a/cmd/mattermost/commands/integrity.go +++ b/cmd/mattermost/commands/integrity.go @@ -31,10 +31,28 @@ func printRelationalIntegrityCheckResult(data store.RelationalIntegrityCheckData return } for _, record := range data.Records { - if record.ChildId != "" { - fmt.Println(fmt.Sprintf(" Child %s (%s.%s) is missing Parent %s (%s.%s)", record.ChildId, data.ChildName, data.ChildIdAttr, record.ParentId, data.ChildName, data.ParentIdAttr)) + var parentId string + + if record.ParentId == nil { + parentId = "NULL" + } else if *record.ParentId == "" { + parentId = "empty" } else { - fmt.Println(fmt.Sprintf(" Child is missing Parent %s (%s.%s)", record.ParentId, data.ChildName, data.ParentIdAttr)) + parentId = *record.ParentId + } + + if record.ChildId != nil { + if parentId == "NULL" || parentId == "empty" { + fmt.Println(fmt.Sprintf(" Child %s (%s.%s) has %s ParentIdAttr (%s.%s)", *record.ChildId, data.ChildName, data.ChildIdAttr, parentId, data.ChildName, data.ParentIdAttr)) + } else { + fmt.Println(fmt.Sprintf(" Child %s (%s.%s) is missing Parent %s (%s.%s)", *record.ChildId, data.ChildName, data.ChildIdAttr, parentId, data.ChildName, data.ParentIdAttr)) + } + } else { + if parentId == "NULL" || parentId == "empty" { + fmt.Println(fmt.Sprintf(" Child has %s ParentIdAttr (%s.%s)", parentId, data.ChildName, data.ParentIdAttr)) + } else { + fmt.Println(fmt.Sprintf(" Child is missing Parent %s (%s.%s)", parentId, data.ChildName, data.ParentIdAttr)) + } } } } diff --git a/store/sqlstore/integrity.go b/store/sqlstore/integrity.go index c7fb884e93..ea9d02a631 100644 --- a/store/sqlstore/integrity.go +++ b/store/sqlstore/integrity.go @@ -24,30 +24,31 @@ func getOrphanedRecords(ss *SqlSupplier, cfg relationalCheckConfig) ([]store.Orp sub := ss.getQueryBuilder(). Select("TRUE"). - From(cfg.parentName). + From(cfg.parentName + " AS PT"). Prefix("NOT EXISTS ("). Suffix(")"). - Where(sq.Eq{"id": cfg.childName + "." + cfg.parentIdAttr}) + Where("PT.id = CT." + cfg.parentIdAttr) main := ss.getQueryBuilder(). Select(). - Column(cfg.parentIdAttr + " AS ParentId"). - From(cfg.childName). + Column("CT." + cfg.parentIdAttr + " AS ParentId"). + From(cfg.childName + " AS CT"). Where(sub) if cfg.childIdAttr != "" { - main = main.Column(cfg.childIdAttr + " AS ChildId") + main = main.Column("CT." + cfg.childIdAttr + " AS ChildId") } if cfg.canParentIdBeEmpty { - main = main.Where(sq.NotEq{cfg.parentIdAttr: ""}) + main = main.Where(sq.NotEq{"CT." + cfg.parentIdAttr: ""}) } if cfg.sortRecords { - main = main.OrderBy(cfg.parentIdAttr) + main = main.OrderBy("CT." + cfg.parentIdAttr) } query, args, _ := main.ToSql() + _, err := ss.GetMaster().Select(&records, query, args...) return records, err @@ -140,7 +141,7 @@ func checkPostsFileInfoIntegrity(ss *SqlSupplier) store.IntegrityCheckResult { parentName: "Posts", parentIdAttr: "PostId", childName: "FileInfo", - childIdAttr: "", + childIdAttr: "Id", }) } @@ -324,10 +325,10 @@ func checkUsersEmojiIntegrity(ss *SqlSupplier) store.IntegrityCheckResult { func checkUsersFileInfoIntegrity(ss *SqlSupplier) store.IntegrityCheckResult { return checkParentChildIntegrity(ss, relationalCheckConfig{ - parentName: "Posts", + parentName: "Users", parentIdAttr: "CreatorId", childName: "FileInfo", - childIdAttr: "", + childIdAttr: "Id", }) } diff --git a/store/sqlstore/integrity_test.go b/store/sqlstore/integrity_test.go index 7e52b02654..9709485c33 100644 --- a/store/sqlstore/integrity_test.go +++ b/store/sqlstore/integrity_test.go @@ -397,8 +397,8 @@ func TestCheckChannelsCommandWebhooksIntegrity(t *testing.T) { data := result.Data.(store.RelationalIntegrityCheckData) require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ - ParentId: channelId, - ChildId: cwh.Id, + ParentId: &channelId, + ChildId: &cwh.Id, }, data.Records[0]) dbmap.Delete(cwh) }) @@ -427,7 +427,7 @@ func TestCheckChannelsChannelMemberHistoryIntegrity(t *testing.T) { data := result.Data.(store.RelationalIntegrityCheckData) require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ - ParentId: cmh.ChannelId, + ParentId: &cmh.ChannelId, }, data.Records[0]) dbmap.Delete(user) dbmap.Exec(`DELETE FROM ChannelMemberHistory`) @@ -456,7 +456,7 @@ func TestCheckChannelsChannelMembersIntegrity(t *testing.T) { data := result.Data.(store.RelationalIntegrityCheckData) require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ - ParentId: member.ChannelId, + ParentId: &member.ChannelId, }, data.Records[0]) ss.Channel().PermanentDeleteMembersByChannel(member.ChannelId) }) @@ -483,8 +483,8 @@ func TestCheckChannelsIncomingWebhooksIntegrity(t *testing.T) { data := result.Data.(store.RelationalIntegrityCheckData) require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ - ParentId: channelId, - ChildId: wh.Id, + ParentId: &channelId, + ChildId: &wh.Id, }, data.Records[0]) dbmap.Delete(wh) }) @@ -513,8 +513,8 @@ func TestCheckChannelsOutgoingWebhooksIntegrity(t *testing.T) { data := result.Data.(store.RelationalIntegrityCheckData) require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ - ParentId: channelId, - ChildId: wh.Id, + ParentId: &channelId, + ChildId: &wh.Id, }, data.Records[0]) dbmap.Delete(wh) }) @@ -540,8 +540,8 @@ func TestCheckChannelsPostsIntegrity(t *testing.T) { data := result.Data.(store.RelationalIntegrityCheckData) require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ - ParentId: post.ChannelId, - ChildId: post.Id, + ParentId: &post.ChannelId, + ChildId: &post.Id, }, data.Records[0]) dbmap.Delete(post) }) @@ -568,8 +568,8 @@ func TestCheckCommandsCommandWebhooksIntegrity(t *testing.T) { data := result.Data.(store.RelationalIntegrityCheckData) require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ - ParentId: commandId, - ChildId: cwh.Id, + ParentId: &commandId, + ChildId: &cwh.Id, }, data.Records[0]) dbmap.Delete(cwh) }) @@ -596,7 +596,8 @@ func TestCheckPostsFileInfoIntegrity(t *testing.T) { data := result.Data.(store.RelationalIntegrityCheckData) require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ - ParentId: postId, + ParentId: &postId, + ChildId: &info.Id, }, data.Records[0]) dbmap.Delete(info) }) @@ -615,6 +616,19 @@ func TestCheckPostsPostsParentIdIntegrity(t *testing.T) { require.Len(t, data.Records, 0) }) + t.Run("should generate a report with no records", func(t *testing.T) { + root := createPost(ss, model.NewId(), model.NewId(), "", "") + parent := createPost(ss, model.NewId(), model.NewId(), root.Id, root.Id) + post := createPost(ss, model.NewId(), model.NewId(), root.Id, parent.Id) + result := checkPostsPostsParentIdIntegrity(supplier) + require.Nil(t, result.Err) + data := result.Data.(store.RelationalIntegrityCheckData) + require.Len(t, data.Records, 0) + dbmap.Delete(parent) + dbmap.Delete(root) + dbmap.Delete(post) + }) + t.Run("should generate a report with one record", func(t *testing.T) { root := createPost(ss, model.NewId(), model.NewId(), "", "") parent := createPost(ss, model.NewId(), model.NewId(), root.Id, root.Id) @@ -626,8 +640,8 @@ func TestCheckPostsPostsParentIdIntegrity(t *testing.T) { data := result.Data.(store.RelationalIntegrityCheckData) require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ - ParentId: parentId, - ChildId: post.Id, + ParentId: &parentId, + ChildId: &post.Id, }, data.Records[0]) dbmap.Delete(root) dbmap.Delete(post) @@ -657,8 +671,8 @@ func TestCheckPostsPostsRootIdIntegrity(t *testing.T) { data := result.Data.(store.RelationalIntegrityCheckData) require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ - ParentId: rootId, - ChildId: post.Id, + ParentId: &rootId, + ChildId: &post.Id, }, data.Records[0]) dbmap.Delete(post) }) @@ -685,7 +699,7 @@ func TestCheckPostsReactionsIntegrity(t *testing.T) { data := result.Data.(store.RelationalIntegrityCheckData) require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ - ParentId: postId, + ParentId: &postId, }, data.Records[0]) dbmap.Delete(reaction) }) @@ -715,8 +729,8 @@ func TestCheckSchemesChannelsIntegrity(t *testing.T) { data := result.Data.(store.RelationalIntegrityCheckData) require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ - ParentId: schemeId, - ChildId: channel.Id, + ParentId: &schemeId, + ChildId: &channel.Id, }, data.Records[0]) dbmap.Delete(channel) }) @@ -746,8 +760,8 @@ func TestCheckSchemesTeamsIntegrity(t *testing.T) { data := result.Data.(store.RelationalIntegrityCheckData) require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ - ParentId: schemeId, - ChildId: team.Id, + ParentId: &schemeId, + ChildId: &team.Id, }, data.Records[0]) dbmap.Delete(team) }) @@ -777,8 +791,8 @@ func TestCheckSessionsAuditsIntegrity(t *testing.T) { data := result.Data.(store.RelationalIntegrityCheckData) require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ - ParentId: sessionId, - ChildId: audit.Id, + ParentId: &sessionId, + ChildId: &audit.Id, }, data.Records[0]) ss.Audit().PermanentDeleteByUser(userId) }) @@ -804,8 +818,8 @@ func TestCheckTeamsChannelsIntegrity(t *testing.T) { data := result.Data.(store.RelationalIntegrityCheckData) require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ - ParentId: channel.TeamId, - ChildId: channel.Id, + ParentId: &channel.TeamId, + ChildId: &channel.Id, }, data.Records[0]) dbmap.Delete(channel) }) @@ -832,8 +846,8 @@ func TestCheckTeamsCommandsIntegrity(t *testing.T) { data := result.Data.(store.RelationalIntegrityCheckData) require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ - ParentId: teamId, - ChildId: cmd.Id, + ParentId: &teamId, + ChildId: &cmd.Id, }, data.Records[0]) dbmap.Delete(cmd) }) @@ -860,8 +874,8 @@ func TestCheckTeamsIncomingWebhooksIntegrity(t *testing.T) { data := result.Data.(store.RelationalIntegrityCheckData) require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ - ParentId: teamId, - ChildId: wh.Id, + ParentId: &teamId, + ChildId: &wh.Id, }, data.Records[0]) dbmap.Delete(wh) }) @@ -888,8 +902,8 @@ func TestCheckTeamsOutgoingWebhooksIntegrity(t *testing.T) { data := result.Data.(store.RelationalIntegrityCheckData) require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ - ParentId: teamId, - ChildId: wh.Id, + ParentId: &teamId, + ChildId: &wh.Id, }, data.Records[0]) dbmap.Delete(wh) }) @@ -917,7 +931,7 @@ func TestCheckTeamsTeamMembersIntegrity(t *testing.T) { data := result.Data.(store.RelationalIntegrityCheckData) require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ - ParentId: team.Id, + ParentId: &team.Id, }, data.Records[0]) ss.Team().RemoveAllMembersByTeam(member.TeamId) }) @@ -946,8 +960,8 @@ func TestCheckUsersAuditsIntegrity(t *testing.T) { data := result.Data.(store.RelationalIntegrityCheckData) require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ - ParentId: userId, - ChildId: audit.Id, + ParentId: &userId, + ChildId: &audit.Id, }, data.Records[0]) ss.Audit().PermanentDeleteByUser(userId) }) @@ -974,8 +988,8 @@ func TestCheckUsersCommandWebhooksIntegrity(t *testing.T) { data := result.Data.(store.RelationalIntegrityCheckData) require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ - ParentId: userId, - ChildId: cwh.Id, + ParentId: &userId, + ChildId: &cwh.Id, }, data.Records[0]) dbmap.Delete(cwh) }) @@ -1001,8 +1015,8 @@ func TestCheckUsersChannelsIntegrity(t *testing.T) { data := result.Data.(store.RelationalIntegrityCheckData) require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ - ParentId: channel.CreatorId, - ChildId: channel.Id, + ParentId: &channel.CreatorId, + ChildId: &channel.Id, }, data.Records[0]) dbmap.Delete(channel) }) @@ -1031,7 +1045,7 @@ func TestCheckUsersChannelMemberHistoryIntegrity(t *testing.T) { data := result.Data.(store.RelationalIntegrityCheckData) require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ - ParentId: cmh.UserId, + ParentId: &cmh.UserId, }, data.Records[0]) dbmap.Delete(channel) dbmap.Exec(`DELETE FROM ChannelMemberHistory`) @@ -1061,7 +1075,7 @@ func TestCheckUsersChannelMembersIntegrity(t *testing.T) { data := result.Data.(store.RelationalIntegrityCheckData) require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ - ParentId: member.UserId, + ParentId: &member.UserId, }, data.Records[0]) dbmap.Delete(channel) ss.Channel().PermanentDeleteMembersByUser(member.UserId) @@ -1089,8 +1103,8 @@ func TestCheckUsersCommandsIntegrity(t *testing.T) { data := result.Data.(store.RelationalIntegrityCheckData) require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ - ParentId: userId, - ChildId: cmd.Id, + ParentId: &userId, + ChildId: &cmd.Id, }, data.Records[0]) dbmap.Delete(cmd) }) @@ -1119,8 +1133,8 @@ func TestCheckUsersCompliancesIntegrity(t *testing.T) { data := result.Data.(store.RelationalIntegrityCheckData) require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ - ParentId: userId, - ChildId: compliance.Id, + ParentId: &userId, + ChildId: &compliance.Id, }, data.Records[0]) dbmap.Delete(compliance) }) @@ -1149,8 +1163,8 @@ func TestCheckUsersEmojiIntegrity(t *testing.T) { data := result.Data.(store.RelationalIntegrityCheckData) require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ - ParentId: userId, - ChildId: emoji.Id, + ParentId: &userId, + ChildId: &emoji.Id, }, data.Records[0]) dbmap.Delete(emoji) }) @@ -1179,7 +1193,8 @@ func TestCheckUsersFileInfoIntegrity(t *testing.T) { data := result.Data.(store.RelationalIntegrityCheckData) require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ - ParentId: userId, + ParentId: &userId, + ChildId: &info.Id, }, data.Records[0]) dbmap.Delete(info) }) @@ -1206,8 +1221,8 @@ func TestCheckUsersIncomingWebhooksIntegrity(t *testing.T) { data := result.Data.(store.RelationalIntegrityCheckData) require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ - ParentId: userId, - ChildId: wh.Id, + ParentId: &userId, + ChildId: &wh.Id, }, data.Records[0]) dbmap.Delete(wh) }) @@ -1236,8 +1251,8 @@ func TestCheckUsersOAuthAccessDataIntegrity(t *testing.T) { data := result.Data.(store.RelationalIntegrityCheckData) require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ - ParentId: userId, - ChildId: ad.Token, + ParentId: &userId, + ChildId: &ad.Token, }, data.Records[0]) ss.OAuth().RemoveAccessData(ad.Token) }) @@ -1266,8 +1281,8 @@ func TestCheckUsersOAuthAppsIntegrity(t *testing.T) { data := result.Data.(store.RelationalIntegrityCheckData) require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ - ParentId: userId, - ChildId: app.Id, + ParentId: &userId, + ChildId: &app.Id, }, data.Records[0]) ss.OAuth().DeleteApp(app.Id) }) @@ -1296,8 +1311,8 @@ func TestCheckUsersOAuthAuthDataIntegrity(t *testing.T) { data := result.Data.(store.RelationalIntegrityCheckData) require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ - ParentId: userId, - ChildId: ad.Code, + ParentId: &userId, + ChildId: &ad.Code, }, data.Records[0]) ss.OAuth().RemoveAuthData(ad.Code) }) @@ -1324,8 +1339,8 @@ func TestCheckUsersOutgoingWebhooksIntegrity(t *testing.T) { data := result.Data.(store.RelationalIntegrityCheckData) require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ - ParentId: userId, - ChildId: wh.Id, + ParentId: &userId, + ChildId: &wh.Id, }, data.Records[0]) dbmap.Delete(wh) }) @@ -1351,8 +1366,8 @@ func TestCheckUsersPostsIntegrity(t *testing.T) { data := result.Data.(store.RelationalIntegrityCheckData) require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ - ParentId: post.UserId, - ChildId: post.Id, + ParentId: &post.UserId, + ChildId: &post.Id, }, data.Records[0]) dbmap.Delete(post) }) @@ -1371,19 +1386,36 @@ func TestCheckUsersPreferencesIntegrity(t *testing.T) { require.Len(t, data.Records, 0) }) - t.Run("should generate a report with one record", func(t *testing.T) { + t.Run("should generate a report with no records", func(t *testing.T) { user := createUser(ss) + require.NotNil(t, user) userId := user.Id preferences := createPreferences(ss, userId) + require.NotNil(t, preferences) + result := checkUsersPreferencesIntegrity(supplier) + require.Nil(t, result.Err) + data := result.Data.(store.RelationalIntegrityCheckData) + require.Len(t, data.Records, 0) + dbmap.Exec(`DELETE FROM Preferences`) + dbmap.Delete(user) + }) + + t.Run("should generate a report with one record", func(t *testing.T) { + user := createUser(ss) + require.NotNil(t, user) + userId := user.Id + preferences := createPreferences(ss, userId) + require.NotNil(t, preferences) dbmap.Delete(user) result := checkUsersPreferencesIntegrity(supplier) require.Nil(t, result.Err) data := result.Data.(store.RelationalIntegrityCheckData) require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ - ParentId: userId, + ParentId: &userId, }, data.Records[0]) - dbmap.Delete(preferences) + dbmap.Exec(`DELETE FROM Preferences`) + dbmap.Delete(user) }) }) } @@ -1410,7 +1442,7 @@ func TestCheckUsersReactionsIntegrity(t *testing.T) { data := result.Data.(store.RelationalIntegrityCheckData) require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ - ParentId: userId, + ParentId: &userId, }, data.Records[0]) dbmap.Delete(reaction) }) @@ -1437,8 +1469,8 @@ func TestCheckUsersSessionsIntegrity(t *testing.T) { data := result.Data.(store.RelationalIntegrityCheckData) require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ - ParentId: userId, - ChildId: session.Id, + ParentId: &userId, + ChildId: &session.Id, }, data.Records[0]) dbmap.Delete(session) }) @@ -1467,7 +1499,7 @@ func TestCheckUsersStatusIntegrity(t *testing.T) { data := result.Data.(store.RelationalIntegrityCheckData) require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ - ParentId: userId, + ParentId: &userId, }, data.Records[0]) dbmap.Delete(status) }) @@ -1496,7 +1528,7 @@ func TestCheckUsersTeamMembersIntegrity(t *testing.T) { data := result.Data.(store.RelationalIntegrityCheckData) require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ - ParentId: member.UserId, + ParentId: &member.UserId, }, data.Records[0]) ss.Team().RemoveAllMembersByTeam(member.TeamId) dbmap.Delete(team) @@ -1526,8 +1558,8 @@ func TestCheckUsersUserAccessTokensIntegrity(t *testing.T) { data := result.Data.(store.RelationalIntegrityCheckData) require.Len(t, data.Records, 1) require.Equal(t, store.OrphanedRecord{ - ParentId: userId, - ChildId: uat.Id, + ParentId: &userId, + ChildId: &uat.Id, }, data.Records[0]) ss.UserAccessToken().Delete(uat.Id) }) diff --git a/store/store.go b/store/store.go index 6dd4f7d031..f7bca759aa 100644 --- a/store/store.go +++ b/store/store.go @@ -652,8 +652,8 @@ type UserGetByIdsOpts struct { } type OrphanedRecord struct { - ParentId string - ChildId string + ParentId *string + ChildId *string } type RelationalIntegrityCheckData struct {