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

@@ -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
 	}
 }
 

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",

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

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: []
             });
         }
     }

webapp/utils/async_client.jsx

@@ -722,6 +722,7 @@ export function getFileInfosForPost(channelId, postId) {
         return;
     }
 
+    callTracker[callName] = utils.getTimestamp();
     Client.getFileInfosForPost(
         channelId,
         postId,