From 4156112f7c1c1dd564d8d20e56c0574f9e9c45bc Mon Sep 17 00:00:00 2001 From: Ben Schumacher Date: Thu, 27 Mar 2025 20:17:20 +0100 Subject: [PATCH] Return error as last value in BulkImport functions (#30575) --- server/channels/app/export_test.go | 47 +++++++++---------- server/channels/app/import.go | 22 ++++----- server/channels/app/import_test.go | 14 +++--- server/channels/jobs/import_process/worker.go | 4 +- server/cmd/mattermost/commands/import.go | 2 +- 5 files changed, 44 insertions(+), 45 deletions(-) diff --git a/server/channels/app/export_test.go b/server/channels/app/export_test.go index 8ce29d278c..5637be7dab 100644 --- a/server/channels/app/export_test.go +++ b/server/channels/app/export_test.go @@ -206,7 +206,7 @@ func TestExportAllUsers(t *testing.T) { th2 := Setup(t) defer th2.TearDown() - err, i := th2.App.BulkImport(th2.Context, &b, nil, false, 5) + i, err := th2.App.BulkImport(th2.Context, &b, nil, false, 5) assert.Nil(t, err) assert.EqualValues(t, 0, i) @@ -258,7 +258,7 @@ func TestExportAllBots(t *testing.T) { th2 := Setup(t) defer th2.TearDown() - err, i := th2.App.BulkImport(th2.Context, &b, nil, false, 5) + i, err := th2.App.BulkImport(th2.Context, &b, nil, false, 5) require.Nil(t, err) assert.EqualValues(t, 0, i) @@ -309,8 +309,7 @@ func TestExportDMChannel(t *testing.T) { assert.Equal(t, 0, len(channels)) // import the exported channel - var i int - appErr, i = th2.App.BulkImport(th2.Context, &b, nil, false, 5) + i, appErr := th2.App.BulkImport(th2.Context, &b, nil, false, 5) require.Nil(t, appErr) assert.Equal(t, 0, i) @@ -352,7 +351,7 @@ func TestExportDMChannel(t *testing.T) { defer th2.TearDown() // import the exported channel - appErr, _ = th2.App.BulkImport(th2.Context, &b, nil, true, 5) + _, appErr = th2.App.BulkImport(th2.Context, &b, nil, true, 5) require.Nil(t, appErr) channels, nErr = th2.App.Srv().Store().Channel().GetAllDirectChannelsForExportAfter(1000, "00000000", false) @@ -388,7 +387,7 @@ func TestExportDMChannel(t *testing.T) { defer th2.TearDown() // import the exported channel - err, _ = th2.App.BulkImport(th2.Context, &b, nil, false, 5) + _, err = th2.App.BulkImport(th2.Context, &b, nil, false, 5) require.Nil(t, err) channels, nErr = th2.App.Srv().Store().Channel().GetAllDirectChannelsForExportAfter(1000, "00000000", false) @@ -429,7 +428,7 @@ func TestExportDMChannelToSelf(t *testing.T) { assert.Equal(t, 0, len(channels)) // import the exported channel - err, i := th2.App.BulkImport(th2.Context, &b, nil, false, 5) + i, err := th2.App.BulkImport(th2.Context, &b, nil, false, 5) assert.Nil(t, err) assert.EqualValues(t, 0, i) @@ -501,7 +500,7 @@ func TestExportGMandDMChannels(t *testing.T) { assert.Equal(t, 0, len(channels)) // import the exported channel - err, i := th2.App.BulkImport(th2.Context, &b, nil, false, 5) + i, err := th2.App.BulkImport(th2.Context, &b, nil, false, 5) assert.Nil(t, err) assert.Equal(t, 0, i) @@ -584,7 +583,7 @@ func TestExportDMandGMPost(t *testing.T) { assert.Equal(t, 0, len(posts)) // import the exported posts - appErr, i := th2.App.BulkImport(th2.Context, &b, nil, false, 5) + i, appErr := th2.App.BulkImport(th2.Context, &b, nil, false, 5) assert.Nil(t, appErr) assert.Equal(t, 0, i) @@ -661,7 +660,7 @@ func TestExportPostWithProps(t *testing.T) { assert.Len(t, posts, 0) // import the exported posts - appErr, i := th2.App.BulkImport(th2.Context, &b, nil, false, 5) + i, appErr := th2.App.BulkImport(th2.Context, &b, nil, false, 5) assert.Nil(t, appErr) assert.Equal(t, 0, i) @@ -700,7 +699,7 @@ func TestExportUserCustomStatus(t *testing.T) { th2 := Setup(t) defer th2.TearDown() - appErr, i := th2.App.BulkImport(th2.Context, &b, nil, false, 1) + i, appErr := th2.App.BulkImport(th2.Context, &b, nil, false, 1) require.Nil(t, appErr) assert.Equal(t, 0, i) @@ -737,7 +736,7 @@ func TestExportDMPostWithSelf(t *testing.T) { assert.Equal(t, 0, len(posts)) // import the exported posts - err, i := th2.App.BulkImport(th2.Context, &b, nil, false, 5) + i, err := th2.App.BulkImport(th2.Context, &b, nil, false, 5) assert.Nil(t, err) assert.Equal(t, 0, i) @@ -922,7 +921,7 @@ func TestExportFileWarnings(t *testing.T) { jsonFile := extractImportFile(filepath.Join(testsDir, "import_test.zip")) defer jsonFile.Close() - appErr, _ := th.App.BulkImportWithPath(th.Context, jsonFile, nil, false, true, 1, dir) + _, appErr := th.App.BulkImportWithPath(th.Context, jsonFile, nil, false, true, 1, dir) require.Nil(t, appErr) // delete one of the files @@ -1014,7 +1013,7 @@ func TestBulkExport(t *testing.T) { jsonFile := extractImportFile(filepath.Join(testsDir, "import_test.zip")) defer jsonFile.Close() - appErr, _ := th.App.BulkImportWithPath(th.Context, jsonFile, nil, false, true, 1, dir) + _, appErr := th.App.BulkImportWithPath(th.Context, jsonFile, nil, false, true, 1, dir) require.Nil(t, appErr) exportFile, err := os.Create(filepath.Join(dir, "export.zip")) @@ -1035,7 +1034,7 @@ func TestBulkExport(t *testing.T) { jsonFile = extractImportFile(filepath.Join(dir, "export.zip")) defer jsonFile.Close() - appErr, _ = th.App.BulkImportWithPath(th.Context, jsonFile, nil, false, true, 1, filepath.Join(dir, "data")) + _, appErr = th.App.BulkImportWithPath(th.Context, jsonFile, nil, false, true, 1, filepath.Join(dir, "data")) require.Nil(t, appErr) } @@ -1121,7 +1120,7 @@ func TestExportDeletedTeams(t *testing.T) { th2 := Setup(t) defer th2.TearDown() - err, i := th2.App.BulkImport(th2.Context, &b, nil, false, 5) + i, err := th2.App.BulkImport(th2.Context, &b, nil, false, 5) assert.Nil(t, err) assert.Equal(t, 0, i) @@ -1161,7 +1160,7 @@ func TestExportArchivedChannels(t *testing.T) { th2 := Setup(t) defer th2.TearDown() - err, i := th2.App.BulkImport(th2.Context, &b, nil, false, 5) + i, err := th2.App.BulkImport(th2.Context, &b, nil, false, 5) assert.Nil(t, err) assert.Equal(t, 0, i) @@ -1194,7 +1193,7 @@ func TestExportRoles(t *testing.T) { th2 := Setup(t) defer th2.TearDown() - appErr, i := th2.App.BulkImport(th2.Context, &b, nil, false, 1) + i, appErr := th2.App.BulkImport(th2.Context, &b, nil, false, 1) assert.Nil(t, appErr) assert.Equal(t, 0, i) @@ -1225,7 +1224,7 @@ func TestExportRoles(t *testing.T) { th2 := Setup(t) defer th2.TearDown() - appErr, i := th2.App.BulkImport(th2.Context, &b, nil, false, 1) + i, appErr := th2.App.BulkImport(th2.Context, &b, nil, false, 1) require.Nil(t, appErr) require.Equal(t, 0, i) @@ -1262,7 +1261,7 @@ func TestExportRoles(t *testing.T) { th2 := Setup(t) defer th2.TearDown() - appErr, i := th2.App.BulkImport(th2.Context, &b, nil, false, 1) + i, appErr := th2.App.BulkImport(th2.Context, &b, nil, false, 1) require.Nil(t, appErr) require.Equal(t, 0, i) @@ -1308,7 +1307,7 @@ func TestExportSchemes(t *testing.T) { err = th2.App.Srv().Store().System().Save(&model.System{Name: model.MigrationKeyAdvancedPermissionsPhase2, Value: "true"}) require.NoError(t, err) - appErr, i := th2.App.BulkImport(th2.Context, &b, nil, false, 1) + i, appErr := th2.App.BulkImport(th2.Context, &b, nil, false, 1) require.Nil(t, appErr) require.Equal(t, 0, i) @@ -1348,7 +1347,7 @@ func TestExportSchemes(t *testing.T) { err = th2.App.Srv().Store().System().Save(&model.System{Name: model.MigrationKeyAdvancedPermissionsPhase2, Value: "true"}) require.NoError(t, err) - appErr, i := th2.App.BulkImport(th2.Context, &b, nil, false, 1) + i, appErr := th2.App.BulkImport(th2.Context, &b, nil, false, 1) require.Nil(t, appErr) require.Equal(t, 0, i) @@ -1412,7 +1411,7 @@ func TestExportSchemes(t *testing.T) { require.Nil(t, appErr) require.Len(t, roles, builtInRoles) - appErr, i := th2.App.BulkImport(th2.Context, &b, nil, false, 1) + i, appErr := th2.App.BulkImport(th2.Context, &b, nil, false, 1) require.Nil(t, appErr) require.Equal(t, 0, i) @@ -1520,7 +1519,7 @@ func TestExportSchemes(t *testing.T) { require.Nil(t, appErr) require.Len(t, roles, builtInRoles) - appErr, i := th2.App.BulkImport(th2.Context, &b, nil, false, 1) + i, appErr := th2.App.BulkImport(th2.Context, &b, nil, false, 1) require.Nil(t, appErr) require.Equal(t, 0, i) diff --git a/server/channels/app/import.go b/server/channels/app/import.go index 57e5e8213d..fc41fc890e 100644 --- a/server/channels/app/import.go +++ b/server/channels/app/import.go @@ -181,11 +181,11 @@ func (a *App) bulkImportWorker(c request.CTX, dryRun, extractContent bool, wg *s } } -func (a *App) BulkImport(c request.CTX, jsonlReader io.Reader, attachmentsReader *zip.Reader, dryRun bool, workers int) (*model.AppError, int) { +func (a *App) BulkImport(c request.CTX, jsonlReader io.Reader, attachmentsReader *zip.Reader, dryRun bool, workers int) (int, *model.AppError) { return a.bulkImport(c, jsonlReader, attachmentsReader, dryRun, true, workers, "") } -func (a *App) BulkImportWithPath(c request.CTX, jsonlReader io.Reader, attachmentsReader *zip.Reader, dryRun, extractContent bool, workers int, importPath string) (*model.AppError, int) { +func (a *App) BulkImportWithPath(c request.CTX, jsonlReader io.Reader, attachmentsReader *zip.Reader, dryRun, extractContent bool, workers int, importPath string) (int, *model.AppError) { return a.bulkImport(c, jsonlReader, attachmentsReader, dryRun, extractContent, workers, importPath) } @@ -193,7 +193,7 @@ func (a *App) BulkImportWithPath(c request.CTX, jsonlReader io.Reader, attachmen // not nil. If it is nil, it will look for attachments on the // filesystem in the locations specified by the JSONL file according // to the older behavior -func (a *App) bulkImport(c request.CTX, jsonlReader io.Reader, attachmentsReader *zip.Reader, dryRun, extractContent bool, workers int, importPath string) (*model.AppError, int) { +func (a *App) bulkImport(c request.CTX, jsonlReader io.Reader, attachmentsReader *zip.Reader, dryRun, extractContent bool, workers int, importPath string) (int, *model.AppError) { scanner := bufio.NewScanner(jsonlReader) buf := make([]byte, 0, 64*1024) scanner.Buffer(buf, maxScanTokenSize) @@ -224,7 +224,7 @@ func (a *App) bulkImport(c request.CTX, jsonlReader io.Reader, attachmentsReader var line imports.LineImportData if err := json.Unmarshal(scanner.Bytes(), &line); err != nil { - return model.NewAppError("BulkImport", "app.import.bulk_import.json_decode.error", nil, "", http.StatusBadRequest).Wrap(err), lineNumber + return lineNumber, model.NewAppError("BulkImport", "app.import.bulk_import.json_decode.error", nil, "", http.StatusBadRequest).Wrap(err) } if err := processAttachments(c, &line, importPath, attachedFiles); err != nil { @@ -234,11 +234,11 @@ func (a *App) bulkImport(c request.CTX, jsonlReader io.Reader, attachmentsReader if lineNumber == 1 { importDataFileVersion, appErr := processImportDataFileVersionLine(line) if appErr != nil { - return appErr, lineNumber + return lineNumber, appErr } if importDataFileVersion != 1 { - return model.NewAppError("BulkImport", "app.import.bulk_import.unsupported_version.error", nil, "", http.StatusBadRequest), lineNumber + return lineNumber, model.NewAppError("BulkImport", "app.import.bulk_import.unsupported_version.error", nil, "", http.StatusBadRequest) } lastLineType = line.Type continue @@ -261,7 +261,7 @@ func (a *App) bulkImport(c request.CTX, jsonlReader io.Reader, attachmentsReader if len(errorsChan) != 0 { err := <-errorsChan if stopOnError(c, err) { - return err.Error, err.LineNumber + return err.LineNumber, err.Error } } } @@ -288,7 +288,7 @@ func (a *App) bulkImport(c request.CTX, jsonlReader io.Reader, attachmentsReader if stopOnError(c, err) { close(linesChan) wg.Wait() - return err.Error, err.LineNumber + return err.LineNumber, err.Error } } } @@ -303,15 +303,15 @@ func (a *App) bulkImport(c request.CTX, jsonlReader io.Reader, attachmentsReader if len(errorsChan) != 0 { err := <-errorsChan if stopOnError(c, err) { - return err.Error, err.LineNumber + return err.LineNumber, err.Error } } if err := scanner.Err(); err != nil { - return model.NewAppError("BulkImport", "app.import.bulk_import.file_scan.error", nil, "", http.StatusInternalServerError).Wrap(err), 0 + return 0, model.NewAppError("BulkImport", "app.import.bulk_import.file_scan.error", nil, "", http.StatusInternalServerError).Wrap(err) } - return nil, 0 + return 0, nil } func processImportDataFileVersionLine(line imports.LineImportData) (int, *model.AppError) { diff --git a/server/channels/app/import_test.go b/server/channels/app/import_test.go index 60a386adef..db7b3afe8e 100644 --- a/server/channels/app/import_test.go +++ b/server/channels/app/import_test.go @@ -178,13 +178,13 @@ func TestImportBulkImport(t *testing.T) { {"type": "direct_post", "direct_post": {"channel_members": ["` + username + `", "` + username2 + `", "` + username3 + `"], "user": "` + username + `", "message": "Hello Group Channel", "create_at": 123456789015}} {"type": "emoji", "emoji": {"name": "` + emojiName + `", "image": "` + testImage + `"}}` - err, line := th.App.BulkImport(th.Context, strings.NewReader(data1), nil, false, 2) + line, err := th.App.BulkImport(th.Context, strings.NewReader(data1), nil, false, 2) require.Nil(t, err, "BulkImport should have succeeded") require.Equal(t, 0, line, "BulkImport line should be 0") // Run bulk import using a string that contains a line with invalid json. data2 := `{"type": "version", "version": 1` - err, line = th.App.BulkImport(th.Context, strings.NewReader(data2), nil, false, 2) + line, err = th.App.BulkImport(th.Context, strings.NewReader(data2), nil, false, 2) require.NotNil(t, err, "Should have failed due to invalid JSON on line 1.") require.Equal(t, 1, line, "Should have failed due to invalid JSON on line 1.") @@ -193,7 +193,7 @@ func TestImportBulkImport(t *testing.T) { {"type": "channel", "channel": {"type": "O", "display_name": "xr6m6udffngark2uekvr3hoeny", "team": "` + teamName + `", "name": "` + channelName + `"}} {"type": "user", "user": {"username": "kufjgnkxkrhhfgbrip6qxkfsaa", "email": "kufjgnkxkrhhfgbrip6qxkfsaa@example.com"}} {"type": "user", "user": {"username": "bwshaim6qnc2ne7oqkd5b2s2rq", "email": "bwshaim6qnc2ne7oqkd5b2s2rq@example.com", "teams": [{"name": "` + teamName + `", "channels": [{"name": "` + channelName + `"}]}]}}` - err, line = th.App.BulkImport(th.Context, strings.NewReader(data3), nil, false, 2) + line, err = th.App.BulkImport(th.Context, strings.NewReader(data3), nil, false, 2) require.NotNil(t, err, "Should have failed due to missing version line on line 1.") require.Equal(t, 1, line, "Should have failed due to missing version line on line 1.") @@ -205,7 +205,7 @@ func TestImportBulkImport(t *testing.T) { {"type": "channel", "channel": {"type": "O", "display_name": "xr6m6udffngark2uekvr3hoeny", "team": "` + teamName + `", "name": "` + channelName + `"}} {"type": "user", "user": {"username": "` + username + `", "email": "` + username + `@example.com", "teams": [{"name": "` + teamName + `","theme": "` + teamTheme1 + `", "channels": [{"name": "` + channelName + `"}]}]}} {"type": "post", "post": {"team": "` + teamName + `", "channel": "` + channelName + `", "user": "` + username + `", "message": "Hello World", "create_at": 123456789012}}` - err, line = th.App.BulkImport(th.Context, strings.NewReader(data4+"\r\n"+posts), nil, false, 2) + line, err = th.App.BulkImport(th.Context, strings.NewReader(data4+"\r\n"+posts), nil, false, 2) require.Nil(t, err, "BulkImport should have succeeded") require.Equal(t, 0, line, "BulkImport line should be 0") }) @@ -213,7 +213,7 @@ func TestImportBulkImport(t *testing.T) { t.Run("First item after version without type", func(t *testing.T) { data := `{"type": "version", "version": 1} {"name": "custom-emoji-troll", "image": "bulkdata/emoji/trollolol.png"}` - err, line := th.App.BulkImport(th.Context, strings.NewReader(data), nil, false, 2) + line, err := th.App.BulkImport(th.Context, strings.NewReader(data), nil, false, 2) require.NotNil(t, err, "Should have failed due to invalid type on line 2.") require.Equal(t, 2, line, "Should have failed due to invalid type on line 2.") }) @@ -227,7 +227,7 @@ func TestImportBulkImport(t *testing.T) { {"type": "direct_channel", "direct_channel": {"members": ["` + username + `", "` + username + `"]}} {"type": "direct_post", "direct_post": {"channel_members": ["` + username + `", "` + username + `"], "user": "` + username + `", "message": "Hello Direct Channel to myself", "create_at": 123456789014, "props":{"attachments":[{"id":0,"fallback":"[February 4th, 2020 2:46 PM] author: fallback","color":"D0D0D0","pretext":"","author_name":"author","author_link":"","title":"","title_link":"","text":"this post has props","fields":null,"image_url":"","thumb_url":"","footer":"Posted in #general","footer_icon":"","ts":"1580823992.000100"}]}}}` - err, line := th.App.BulkImport(th.Context, strings.NewReader(data6), nil, false, 2) + line, err := th.App.BulkImport(th.Context, strings.NewReader(data6), nil, false, 2) require.Nil(t, err, "BulkImport should have succeeded") require.Equal(t, 0, line, "BulkImport line should be 0") }) @@ -489,7 +489,7 @@ func TestImportBulkImportWithAttachments(t *testing.T) { th.App.UpdateConfig(func(cfg *model.Config) { cfg.TeamSettings.MaxUsersPerTeam = model.NewPointer(1000) }) - appErr, _ := th.App.BulkImportWithPath(th.Context, jsonFile, importZipReader, false, true, 1, model.ExportDataDir) + _, appErr := th.App.BulkImportWithPath(th.Context, jsonFile, importZipReader, false, true, 1, model.ExportDataDir) require.Nil(t, appErr) adminUser, appErr := th.App.GetUserByUsername("sysadmin") diff --git a/server/channels/jobs/import_process/worker.go b/server/channels/jobs/import_process/worker.go index 1ff5304e74..17e7700377 100644 --- a/server/channels/jobs/import_process/worker.go +++ b/server/channels/jobs/import_process/worker.go @@ -29,7 +29,7 @@ type AppIface interface { FileExists(path string) (bool, *model.AppError) FileSize(path string) (int64, *model.AppError) FileReader(path string) (filestore.ReadCloseSeeker, *model.AppError) - BulkImportWithPath(c request.CTX, jsonlReader io.Reader, attachmentsReader *zip.Reader, dryRun, extractContent bool, workers int, importPath string) (*model.AppError, int) + BulkImportWithPath(c request.CTX, jsonlReader io.Reader, attachmentsReader *zip.Reader, dryRun, extractContent bool, workers int, importPath string) (int, *model.AppError) Log() *mlog.Logger } @@ -125,7 +125,7 @@ func MakeWorker(jobServer *jobs.JobServer, app AppIface) *jobs.SimpleWorker { extractContent := job.Data["extract_content"] == "true" // do the actual import. - appErr, lineNumber := app.BulkImportWithPath(appContext, jsonFile, importZipReader, false, extractContent, runtime.NumCPU(), model.ExportDataDir) + lineNumber, appErr := app.BulkImportWithPath(appContext, jsonFile, importZipReader, false, extractContent, runtime.NumCPU(), model.ExportDataDir) if appErr != nil { job.Data["line_number"] = strconv.Itoa(lineNumber) return appErr diff --git a/server/cmd/mattermost/commands/import.go b/server/cmd/mattermost/commands/import.go index e02cebdc32..434403d830 100644 --- a/server/cmd/mattermost/commands/import.go +++ b/server/cmd/mattermost/commands/import.go @@ -156,7 +156,7 @@ func bulkImportCmdF(command *cobra.Command, args []string) error { CommandPrettyPrintln("") - if err, lineNumber := a.BulkImportWithPath(rctx, fileReader, nil, true, !apply, workers, importPath); err != nil { + if lineNumber, err := a.BulkImportWithPath(rctx, fileReader, nil, true, !apply, workers, importPath); err != nil { CommandPrintErrorln(err.Error()) if lineNumber != 0 { CommandPrintErrorln(fmt.Sprintf("Error occurred on data file line %v", lineNumber))