From 7fb818d4cef09dd74f9fcfd99381a7c0e7064a56 Mon Sep 17 00:00:00 2001 From: Harrison Healey Date: Thu, 6 Oct 2016 16:44:41 -0400 Subject: PLT-3105 Fixed bugs with FileInfos migration, including duplicate FileInfos being saved (#4134) * Added a limit to GetByPath for the rare cases when two old files had the same path * Fixed files still being displayed for deleted posts * Added a lock to prevent migrateFilenamesToFileInfos from migrating multiple posts at once --- api/file.go | 44 +++++++++++++++++++++++++++++++++++-------- i18n/en.json | 18 +++++++++++++++++- store/sql_file_info_store.go | 3 ++- webapp/stores/post_store.jsx | 2 +- webapp/utils/async_client.jsx | 1 + 5 files changed, 57 insertions(+), 11 deletions(-) diff --git a/api/file.go b/api/file.go index 9cf513ebf..a9bb2a451 100644 --- a/api/file.go +++ b/api/file.go @@ -21,6 +21,7 @@ import ( "path/filepath" "strconv" "strings" + "sync" "time" l4g "github.com/alecthomas/log4go" @@ -571,6 +572,8 @@ func generatePublicLinkHash(fileId, salt string) string { return base64.RawURLEncoding.EncodeToString(hash.Sum(nil)) } +var fileMigrationLock sync.Mutex + // Creates and stores FileInfos for a post created before the FileInfos table existed. func migrateFilenamesToFileInfos(post *model.Post) []*model.FileInfo { if len(post.Filenames) == 0 { @@ -602,7 +605,6 @@ func migrateFilenamesToFileInfos(post *model.Post) []*model.FileInfo { // Create FileInfo objects for this post infos := make([]*model.FileInfo, 0, len(filenames)) - fileIds := make([]string, 0, len(filenames)) if teamId == "" { l4g.Error(utils.T("api.file.migrate_filenames_to_file_infos.team_id.error"), post.Id, filenames) } else { @@ -612,16 +614,42 @@ func migrateFilenamesToFileInfos(post *model.Post) []*model.FileInfo { continue } - if result := <-Srv.Store.FileInfo().Save(info); result.Err != nil { - l4g.Error(utils.T("api.file.migrate_filenames_to_file_infos.save_file_info.app_error"), post.Id, info.Id, filename, result.Err) - continue - } - - fileIds = append(fileIds, info.Id) infos = append(infos, info) } } + // Lock to prevent only one migration thread from trying to update the post at once, preventing duplicate FileInfos from being created + fileMigrationLock.Lock() + defer fileMigrationLock.Unlock() + + if result := <-Srv.Store.Post().Get(post.Id); result.Err != nil { + l4g.Error(utils.T("api.file.migrate_filenames_to_file_infos.get_post_again.app_error"), post.Id, result.Err) + return []*model.FileInfo{} + } else if newPost := result.Data.(*model.PostList).Posts[post.Id]; len(newPost.Filenames) != len(post.Filenames) { + // Another thread has already created FileInfos for this post, so just return those + if result := <-Srv.Store.FileInfo().GetForPost(post.Id); result.Err != nil { + l4g.Error(utils.T("api.file.migrate_filenames_to_file_infos.get_post_file_infos_again.app_error"), post.Id, result.Err) + return []*model.FileInfo{} + } else { + l4g.Debug(utils.T("api.file.migrate_filenames_to_file_infos.not_migrating_post.debug"), post.Id) + return result.Data.([]*model.FileInfo) + } + } + + l4g.Debug(utils.T("api.file.migrate_filenames_to_file_infos.migrating_post.debug"), post.Id) + + savedInfos := make([]*model.FileInfo, 0, len(infos)) + fileIds := make([]string, 0, len(filenames)) + for _, info := range infos { + if result := <-Srv.Store.FileInfo().Save(info); result.Err != nil { + l4g.Error(utils.T("api.file.migrate_filenames_to_file_infos.save_file_info.app_error"), post.Id, info.Id, info.Path, result.Err) + continue + } + + savedInfos = append(savedInfos, info) + fileIds = append(fileIds, info.Id) + } + // Copy and save the updated post newPost := &model.Post{} *newPost = *post @@ -634,7 +662,7 @@ func migrateFilenamesToFileInfos(post *model.Post) []*model.FileInfo { l4g.Error(utils.T("api.file.migrate_filenames_to_file_infos.save_post.app_error"), post.Id, newPost.FileIds, post.Filenames, result.Err) return []*model.FileInfo{} } else { - return infos + return savedInfos } } diff --git a/i18n/en.json b/i18n/en.json index fcbae1af9..5dc7dc6aa 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -885,10 +885,22 @@ "id": "api.file.migrate_filenames_to_file_infos.file_not_found.warn", "translation": "Unable to find file when migrating post to use FileInfos, post_id=%v, filename=%v, path=%v, err=%v" }, + { + "id": "api.file.migrate_filenames_to_file_infos.get_file_infos_again.warn", + "translation": "Unable to get FileInfos for post after migratio, post_id=%v, err=%v" + }, + { + "id": "api.file.migrate_filenames_to_file_infos.get_post_again.warn", + "translation": "Unable to get post when migrating to use FileInfos, post_id=%v, err=%v" + }, { "id": "api.file.migrate_filenames_to_file_infos.info.app_error", "translation": "Unable to fully decode file info when migrating post to use FileInfos, post_id=%v, filename=%v, err=%v" }, + { + "id": "api.file.migrate_filenames_to_file_infos.migrating_post.debug", + "translation": "Migrating post to use FileInfos, post_id=%v, err=%v" + }, { "id": "api.file.migrate_filenames_to_file_infos.mismatched_filename.warn", "translation": "Found an unusual filename when migrating post to use FileInfos, post_id=%v, channel_id=%v, user_id=%v, filename=%v" @@ -897,9 +909,13 @@ "id": "api.file.migrate_filenames_to_file_infos.no_filenames.warn", "translation": "Unable to migrate post to use FileInfos with an empty Filenames field, post_id=%v" }, + { + "id": "api.file.migrate_filenames_to_file_infos.not_migrating_post.debug", + "translation": "Post already migrated to use FileInfos, post_id=%v, err=%v" + }, { "id": "api.file.migrate_filenames_to_file_infos.save_file_info.warn", - "translation": "Unable to save post when migrating post to use FileInfos, post_id=%v, new_file_ids=%v, old_filenames=%v, err=%v" + "translation": "Unable to save post when migrating post to use FileInfos, post_id=%v, file_id=%v, path=%v, err=%v" }, { "id": "api.file.migrate_filenames_to_file_infos.save_post.warn", diff --git a/store/sql_file_info_store.go b/store/sql_file_info_store.go index 5c3f6b1a4..78c36c08a 100644 --- a/store/sql_file_info_store.go +++ b/store/sql_file_info_store.go @@ -101,7 +101,8 @@ func (fs SqlFileInfoStore) GetByPath(path string) StoreChannel { FileInfo WHERE Path = :Path - AND DeleteAt = 0`, map[string]interface{}{"Path": path}); err != nil { + AND DeleteAt = 0 + LIMIT 1`, map[string]interface{}{"Path": path}); err != nil { result.Err = model.NewLocAppError("SqlFileInfoStore.GetByPath", "store.sql_file_info.get_by_path.app_error", nil, "path="+path+", "+err.Error()) } else { result.Data = info diff --git a/webapp/stores/post_store.jsx b/webapp/stores/post_store.jsx index 22f47fd40..2d0d7a674 100644 --- a/webapp/stores/post_store.jsx +++ b/webapp/stores/post_store.jsx @@ -318,7 +318,7 @@ class PostStoreClass extends EventEmitter { // make sure to copy the post so that component state changes work properly postList.posts[post.id] = Object.assign({}, post, { state: Constants.POST_DELETED, - fileIds: [] + file_ids: [] }); } } diff --git a/webapp/utils/async_client.jsx b/webapp/utils/async_client.jsx index c5b0d015b..30bc474f8 100644 --- a/webapp/utils/async_client.jsx +++ b/webapp/utils/async_client.jsx @@ -722,6 +722,7 @@ export function getFileInfosForPost(channelId, postId) { return; } + callTracker[callName] = utils.getTimestamp(); Client.getFileInfosForPost( channelId, postId, -- cgit v1.2.3-1-g7c22