[MM-19969] - When bulk import finds an already existing post, it should delete existing files (#13037)

app/import_functions.go

@@ -886,12 +886,18 @@ func (a *App) ImportReply(data *ReplyImportData, post *model.Post, teamId string
 	reply.Message = *data.Message
 	reply.CreateAt = *data.CreateAt
 
-	if data.Attachments != nil {
 	fileIds, err := a.uploadAttachments(data.Attachments, reply, teamId, dryRun)
 	if err != nil {
 		return err
 	}
-		reply.FileIds = append(reply.FileIds, fileIds...)
+	for _, fileID := range reply.FileIds {
+		if _, ok := fileIds[fileID]; !ok {
+			a.Srv.Store.FileInfo().PermanentDelete(fileID)
+		}
+	}
+	reply.FileIds = make([]string, 0)
+	for fileID := range fileIds {
+		reply.FileIds = append(reply.FileIds, fileID)
 	}
 
 	if reply.Id == "" {
@@ -911,16 +917,19 @@ func (a *App) ImportReply(data *ReplyImportData, post *model.Post, teamId string
 
 func (a *App) ImportAttachment(data *AttachmentImportData, post *model.Post, teamId string, dryRun bool) (*model.FileInfo, *model.AppError) {
 	file, err := os.Open(*data.Path)
-	if err != nil {
+	if file == nil || err != nil {
 		return nil, model.NewAppError("BulkImport", "app.import.attachment.bad_file.error", map[string]interface{}{"FilePath": *data.Path}, "", http.StatusBadRequest)
 	}
-	if file != nil {
+
 	timestamp := utils.TimeFromMillis(post.CreateAt)
 	buf := bytes.NewBuffer(nil)
 	_, _ = io.Copy(buf, file)
 	// Go over existing files in the post and see if there already exists a file with the same name, size and hash. If so - skip it
 	if post.Id != "" {
-			if oldFiles, err := a.GetFileInfosForPost(post.Id, true); err == nil {
+		oldFiles, err := a.GetFileInfosForPost(post.Id, true)
+		if err != nil {
+			return nil, model.NewAppError("BulkImport", "app.import.attachment.file_upload.error", map[string]interface{}{"FilePath": *data.Path}, "", http.StatusBadRequest)
+		}
 		for _, oldFile := range oldFiles {
 			if oldFile.Name != path.Base(file.Name()) || oldFile.Size != int64(buf.Len()) {
 				continue
@@ -935,27 +944,20 @@ func (a *App) ImportAttachment(data *AttachmentImportData, post *model.Post, tea
 
 			if bytes.Equal(oldHash[:], newHash[:]) {
 				mlog.Info("Skipping uploading of file because name already exists", mlog.Any("file_name", file.Name()))
-						return nil, nil
+				return oldFile, nil
-					}
-
-				}
-			} else {
-				return nil, model.NewAppError("BulkImport", "app.import.attachment.file_upload.error", map[string]interface{}{"FilePath": *data.Path}, "", http.StatusBadRequest)
 			}
 		}
-		fileInfo, err := a.DoUploadFile(timestamp, teamId, post.ChannelId, post.UserId, file.Name(), buf.Bytes())
+	}
-
+	fileInfo, appErr := a.DoUploadFile(timestamp, teamId, post.ChannelId, post.UserId, file.Name(), buf.Bytes())
-		if err != nil {
+	if appErr != nil {
 		mlog.Error("Failed to upload file:", mlog.Err(err))
-			return nil, err
+		return nil, appErr
 	}
 
 	a.HandleImages([]string{fileInfo.PreviewPath}, []string{fileInfo.ThumbnailPath}, [][]byte{buf.Bytes()})
 
 	mlog.Info("Uploading file with name", mlog.String("file_name", file.Name()))
 	return fileInfo, nil
-	}
-	return nil, model.NewAppError("BulkImport", "app.import.attachment.file_upload.error", map[string]interface{}{"FilePath": *data.Path}, "", http.StatusBadRequest)
 }
 
 func (a *App) ImportPost(data *PostImportData, dryRun bool) *model.AppError {
@@ -1009,13 +1011,18 @@ func (a *App) ImportPost(data *PostImportData, dryRun bool) *model.AppError {
 
 	post.Hashtags, _ = model.ParseHashtags(post.Message)
 
-	if data.Attachments != nil {
+	fileIds, err := a.uploadAttachments(data.Attachments, post, team.Id, dryRun)
-		var fileIds []string
-		fileIds, err = a.uploadAttachments(data.Attachments, post, team.Id, dryRun)
 	if err != nil {
 		return err
 	}
-		post.FileIds = append(post.FileIds, fileIds...)
+	for _, fileID := range post.FileIds {
+		if _, ok := fileIds[fileID]; !ok {
+			a.Srv.Store.FileInfo().PermanentDelete(fileID)
+		}
+	}
+	post.FileIds = make([]string, 0)
+	for fileID := range fileIds {
+		post.FileIds = append(post.FileIds, fileID)
 	}
 
 	if post.Id == "" {
@@ -1073,16 +1080,18 @@ func (a *App) ImportPost(data *PostImportData, dryRun bool) *model.AppError {
 	return nil
 }
 
-func (a *App) uploadAttachments(attachments *[]AttachmentImportData, post *model.Post, teamId string, dryRun bool) ([]string, *model.AppError) {
+// uploadAttachments imports new attachments and returns current attachments of the post as a map
-	fileIds := []string{}
+func (a *App) uploadAttachments(attachments *[]AttachmentImportData, post *model.Post, teamId string, dryRun bool) (map[string]bool, *model.AppError) {
+	if attachments == nil {
+		return nil, nil
+	}
+	fileIds := make(map[string]bool)
 	for _, attachment := range *attachments {
 		fileInfo, err := a.ImportAttachment(&attachment, post, teamId, dryRun)
 		if err != nil {
 			return nil, err
 		}
-		if fileInfo != nil { // nil is returned when the file was skipped due to duplication
+		fileIds[fileInfo.Id] = true
-			fileIds = append(fileIds, fileInfo.Id)
-		}
 	}
 	return fileIds, nil
 }
@@ -1238,13 +1247,18 @@ func (a *App) ImportDirectPost(data *DirectPostImportData, dryRun bool) *model.A
 
 	post.Hashtags, _ = model.ParseHashtags(post.Message)
 
-	if data.Attachments != nil {
+	fileIds, err := a.uploadAttachments(data.Attachments, post, "noteam", dryRun)
-		var fileIds []string
-		fileIds, err = a.uploadAttachments(data.Attachments, post, "noteam", dryRun)
 	if err != nil {
 		return err
 	}
-		post.FileIds = append(post.FileIds, fileIds...)
+	for _, fileID := range post.FileIds {
+		if _, ok := fileIds[fileID]; !ok {
+			a.Srv.Store.FileInfo().PermanentDelete(fileID)
+		}
+	}
+	post.FileIds = make([]string, 0)
+	for fileID := range fileIds {
+		post.FileIds = append(post.FileIds, fileID)
 	}
 
 	if post.Id == "" {

app/import_functions_test.go

@@ -2569,6 +2569,7 @@ func TestImportPostAndRepliesWithAttachments(t *testing.T) {
 		}},
 	}
 
+	// import with attachments
 	err = th.App.ImportPost(data, false)
 	assert.Nil(t, err)
 
@@ -2578,6 +2579,16 @@ func TestImportPostAndRepliesWithAttachments(t *testing.T) {
 	assert.Contains(t, attachments[1].Path, team.Id)
 	AssertFileIdsInPost(attachments, th, t)
 
+	// import existing post with new attachments
+	data.Attachments = &[]AttachmentImportData{{Path: &testImage}}
+	err = th.App.ImportPost(data, false)
+	assert.Nil(t, err)
+
+	attachments = GetAttachments(user3.Id, th, t)
+	assert.Equal(t, len(attachments), 1)
+	assert.Contains(t, attachments[0].Path, team.Id)
+	AssertFileIdsInPost(attachments, th, t)
+
 	attachments = GetAttachments(user4.Id, th, t)
 	assert.Equal(t, len(attachments), 1)
 	assert.Contains(t, attachments[0].Path, team.Id)