From 7419898449e3331f894f844f450149dc04c1594e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jes=C3=BAs=20Espino?= Date: Sun, 20 Dec 2020 12:53:07 +0100 Subject: [PATCH] Remove usages of AppError on filesstore service (#15841) * Remove usages of AppError on filesstore service * Fixing a golint error * Fixing shadowed variable * Adding err.Error() to the NewAppError calls * Fixing tests * Adding missed translations * Fix error handling and updating the translation that affects it * Fixing two typos --- api4/system.go | 21 +--- api4/system_test.go | 2 +- app/app_iface.go | 4 + app/file.go | 100 ++++++++++++++++-- app/opentracing/opentracing_layer.go | 88 ++++++++++++++++ app/server.go | 14 ++- i18n/en.json | 124 +++++++---------------- services/filesstore/filesstore.go | 33 +++--- services/filesstore/localstore.go | 64 ++++++------ services/filesstore/mocks/FileBackend.go | 112 ++++++++------------ services/filesstore/s3store.go | 75 +++++++------- services/filesstore/s3store_test.go | 2 +- services/mailservice/mail.go | 16 +-- 13 files changed, 366 insertions(+), 289 deletions(-) diff --git a/api4/system.go b/api4/system.go index 82b1af0874..63acdbeaa5 100644 --- a/api4/system.go +++ b/api4/system.go @@ -19,7 +19,6 @@ import ( "github.com/mattermost/mattermost-server/v5/mlog" "github.com/mattermost/mattermost-server/v5/model" "github.com/mattermost/mattermost-server/v5/services/cache" - "github.com/mattermost/mattermost-server/v5/services/filesstore" "github.com/mattermost/mattermost-server/v5/services/upgrader" ) @@ -117,16 +116,8 @@ func getSystemPing(c *Context, w http.ResponseWriter, r *http.Request) { filestoreStatusKey := "filestore_status" s[filestoreStatusKey] = model.STATUS_OK - license := c.App.Srv().License() - backend, appErr := filesstore.NewFileBackend(&c.App.Config().FileSettings, license != nil && *license.Features.Compliance) - if appErr == nil { - appErr = backend.TestConnection() - if appErr != nil { - s[filestoreStatusKey] = model.STATUS_UNHEALTHY - s[model.STATUS] = model.STATUS_UNHEALTHY - } - } else { - mlog.Debug("Unable to get filestore for ping status.", mlog.Err(appErr)) + appErr := c.App.TestFilesStoreConnection() + if appErr != nil { s[filestoreStatusKey] = model.STATUS_UNHEALTHY s[model.STATUS] = model.STATUS_UNHEALTHY } @@ -393,7 +384,7 @@ func testS3(c *Context, w http.ResponseWriter, r *http.Request) { return } - err := filesstore.CheckMandatoryS3Fields(&cfg.FileSettings) + err := c.App.CheckMandatoryS3Fields(&cfg.FileSettings) if err != nil { c.Err = err return @@ -403,11 +394,7 @@ func testS3(c *Context, w http.ResponseWriter, r *http.Request) { cfg.FileSettings.AmazonS3SecretAccessKey = c.App.Config().FileSettings.AmazonS3SecretAccessKey } - license := c.App.Srv().License() - backend, appErr := filesstore.NewFileBackend(&cfg.FileSettings, license != nil && *license.Features.Compliance) - if appErr == nil { - appErr = backend.TestConnection() - } + appErr := c.App.TestFilesStoreConnectionWithConfig(&cfg.FileSettings) if appErr != nil { c.Err = appErr return diff --git a/api4/system_test.go b/api4/system_test.go index 285e8d3662..08b24224a0 100644 --- a/api4/system_test.go +++ b/api4/system_test.go @@ -478,7 +478,7 @@ func TestS3TestConnection(t *testing.T) { config.FileSettings.AmazonS3Bucket = model.NewString("Wrong_bucket") _, resp = th.SystemAdminClient.TestS3Connection(&config) CheckInternalErrorStatus(t, resp) - assert.Equal(t, "Unable to create bucket.", resp.Error.Message) + assert.Equal(t, "api.file.test_connection.app_error", resp.Error.Id) *config.FileSettings.AmazonS3Bucket = "shouldcreatenewbucket" _, resp = th.SystemAdminClient.TestS3Connection(&config) diff --git a/app/app_iface.go b/app/app_iface.go index 6776391d76..e432671afc 100644 --- a/app/app_iface.go +++ b/app/app_iface.go @@ -393,6 +393,7 @@ type AppIface interface { ChannelMembersToRemove(teamID *string) ([]*model.ChannelMember, *model.AppError) CheckAndSendUserLimitWarningEmails() *model.AppError CheckForClientSideCert(r *http.Request) (string, string, string) + CheckMandatoryS3Fields(settings *model.FileSettings) *model.AppError CheckPasswordAndAllCriteria(user *model.User, password string, mfaToken string) *model.AppError CheckRolesExist(roleNames []string) *model.AppError CheckUserAllAuthenticationCriteria(user *model.User, mfaToken string) *model.AppError @@ -834,6 +835,7 @@ type AppIface interface { ReloadConfig() error RemoveAllDeactivatedMembersFromChannel(channel *model.Channel) *model.AppError RemoveConfigListener(id string) + RemoveDirectory(path string) *model.AppError RemoveFile(path string) *model.AppError RemoveLdapPrivateCertificate() *model.AppError RemoveLdapPublicCertificate() *model.AppError @@ -957,6 +959,8 @@ type AppIface interface { TelemetryId() string TestElasticsearch(cfg *model.Config) *model.AppError TestEmail(userId string, cfg *model.Config) *model.AppError + TestFilesStoreConnection() *model.AppError + TestFilesStoreConnectionWithConfig(cfg *model.FileSettings) *model.AppError TestLdap() *model.AppError TestSiteURL(siteURL string) *model.AppError Timezones() *timezones.Timezones diff --git a/app/file.go b/app/file.go index f910002019..e6b6de734a 100644 --- a/app/file.go +++ b/app/file.go @@ -76,12 +76,49 @@ func (a *App) FileBackend() (filesstore.FileBackend, *model.AppError) { return a.Srv().FileBackend() } +func (a *App) CheckMandatoryS3Fields(settings *model.FileSettings) *model.AppError { + err := filesstore.CheckMandatoryS3Fields(settings) + if err != nil { + return model.NewAppError("CheckMandatoryS3Fields", "api.admin.test_s3.missing_s3_bucket", nil, err.Error(), http.StatusBadRequest) + } + return nil +} + +func (a *App) TestFilesStoreConnection() *model.AppError { + backend, err := a.FileBackend() + if err != nil { + return err + } + nErr := backend.TestConnection() + if nErr != nil { + return model.NewAppError("TestConnection", "api.file.test_connection.app_error", nil, nErr.Error(), http.StatusInternalServerError) + } + return nil +} + +func (a *App) TestFilesStoreConnectionWithConfig(cfg *model.FileSettings) *model.AppError { + license := a.Srv().License() + backend, err := filesstore.NewFileBackend(cfg, license != nil && *license.Features.Compliance) + if err != nil { + return model.NewAppError("FileBackend", "api.file.no_driver.app_error", nil, err.Error(), http.StatusInternalServerError) + } + nErr := backend.TestConnection() + if nErr != nil { + return model.NewAppError("TestConnection", "api.file.test_connection.app_error", nil, nErr.Error(), http.StatusInternalServerError) + } + return nil +} + func (a *App) ReadFile(path string) ([]byte, *model.AppError) { backend, err := a.FileBackend() if err != nil { return nil, err } - return backend.ReadFile(path) + result, nErr := backend.ReadFile(path) + if nErr != nil { + return nil, model.NewAppError("ReadFile", "api.file.read_file.app_error", nil, nErr.Error(), http.StatusInternalServerError) + } + return result, nil } // Caller must close the first return value @@ -90,7 +127,11 @@ func (a *App) FileReader(path string) (filesstore.ReadCloseSeeker, *model.AppErr if err != nil { return nil, err } - return backend.Reader(path) + result, nErr := backend.Reader(path) + if nErr != nil { + return nil, model.NewAppError("FileReader", "api.file.file_reader.app_error", nil, nErr.Error(), http.StatusInternalServerError) + } + return result, nil } func (a *App) FileExists(path string) (bool, *model.AppError) { @@ -98,7 +139,11 @@ func (a *App) FileExists(path string) (bool, *model.AppError) { if err != nil { return false, err } - return backend.FileExists(path) + result, nErr := backend.FileExists(path) + if nErr != nil { + return false, model.NewAppError("FileExists", "api.file.file_exists.app_error", nil, nErr.Error(), http.StatusInternalServerError) + } + return result, nil } func (a *App) FileSize(path string) (int64, *model.AppError) { @@ -106,7 +151,11 @@ func (a *App) FileSize(path string) (int64, *model.AppError) { if err != nil { return 0, err } - return backend.FileSize(path) + size, nErr := backend.FileSize(path) + if nErr != nil { + return 0, model.NewAppError("FileSize", "api.file.file_size.app_error", nil, nErr.Error(), http.StatusInternalServerError) + } + return size, nil } func (a *App) MoveFile(oldPath, newPath string) *model.AppError { @@ -114,7 +163,11 @@ func (a *App) MoveFile(oldPath, newPath string) *model.AppError { if err != nil { return err } - return backend.MoveFile(oldPath, newPath) + nErr := backend.MoveFile(oldPath, newPath) + if nErr != nil { + return model.NewAppError("MoveFile", "api.file.move_file.app_error", nil, nErr.Error(), http.StatusInternalServerError) + } + return nil } func (a *App) WriteFile(fr io.Reader, path string) (int64, *model.AppError) { @@ -123,7 +176,11 @@ func (a *App) WriteFile(fr io.Reader, path string) (int64, *model.AppError) { return 0, err } - return backend.WriteFile(fr, path) + result, nErr := backend.WriteFile(fr, path) + if nErr != nil { + return result, model.NewAppError("WriteFile", "api.file.write_file.app_error", nil, nErr.Error(), http.StatusInternalServerError) + } + return result, nil } func (a *App) AppendFile(fr io.Reader, path string) (int64, *model.AppError) { @@ -132,7 +189,11 @@ func (a *App) AppendFile(fr io.Reader, path string) (int64, *model.AppError) { return 0, err } - return backend.AppendFile(fr, path) + result, nErr := backend.AppendFile(fr, path) + if nErr != nil { + return result, model.NewAppError("AppendFile", "api.file.append_file.app_error", nil, nErr.Error(), http.StatusInternalServerError) + } + return result, nil } func (a *App) RemoveFile(path string) *model.AppError { @@ -140,7 +201,11 @@ func (a *App) RemoveFile(path string) *model.AppError { if err != nil { return err } - return backend.RemoveFile(path) + nErr := backend.RemoveFile(path) + if nErr != nil { + return model.NewAppError("RemoveFile", "api.file.remove_file.app_error", nil, nErr.Error(), http.StatusInternalServerError) + } + return nil } func (a *App) ListDirectory(path string) ([]string, *model.AppError) { @@ -148,14 +213,27 @@ func (a *App) ListDirectory(path string) ([]string, *model.AppError) { if err != nil { return nil, err } - paths, err := backend.ListDirectory(path) - if err != nil { - return nil, err + paths, nErr := backend.ListDirectory(path) + if nErr != nil { + return nil, model.NewAppError("ListDirectory", "api.file.list_directory.app_error", nil, nErr.Error(), http.StatusInternalServerError) } return *paths, nil } +func (a *App) RemoveDirectory(path string) *model.AppError { + backend, err := a.FileBackend() + if err != nil { + return err + } + nErr := backend.RemoveDirectory(path) + if nErr != nil { + return model.NewAppError("RemoveDirectory", "api.file.remove_directory.app_error", nil, nErr.Error(), http.StatusInternalServerError) + } + + return nil +} + func (a *App) getInfoForFilename(post *model.Post, teamId, channelId, userId, oldId, filename string) *model.FileInfo { name, _ := url.QueryUnescape(filename) pathPrefix := fmt.Sprintf("teams/%s/channels/%s/users/%s/%s/", teamId, channelId, userId, oldId) diff --git a/app/opentracing/opentracing_layer.go b/app/opentracing/opentracing_layer.go index 7749db6fd2..c4e350b431 100644 --- a/app/opentracing/opentracing_layer.go +++ b/app/opentracing/opentracing_layer.go @@ -1071,6 +1071,28 @@ func (a *OpenTracingAppLayer) CheckForClientSideCert(r *http.Request) (string, s return resultVar0, resultVar1, resultVar2 } +func (a *OpenTracingAppLayer) CheckMandatoryS3Fields(settings *model.FileSettings) *model.AppError { + origCtx := a.ctx + span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.CheckMandatoryS3Fields") + + a.ctx = newCtx + a.app.Srv().Store.SetContext(newCtx) + defer func() { + a.app.Srv().Store.SetContext(origCtx) + a.ctx = origCtx + }() + + defer span.Finish() + resultVar0 := a.app.CheckMandatoryS3Fields(settings) + + if resultVar0 != nil { + span.LogFields(spanlog.Error(resultVar0)) + ext.Error.Set(span, true) + } + + return resultVar0 +} + func (a *OpenTracingAppLayer) CheckPasswordAndAllCriteria(user *model.User, password string, mfaToken string) *model.AppError { origCtx := a.ctx span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.CheckPasswordAndAllCriteria") @@ -11709,6 +11731,28 @@ func (a *OpenTracingAppLayer) RemoveConfigListener(id string) { a.app.RemoveConfigListener(id) } +func (a *OpenTracingAppLayer) RemoveDirectory(path string) *model.AppError { + origCtx := a.ctx + span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.RemoveDirectory") + + a.ctx = newCtx + a.app.Srv().Store.SetContext(newCtx) + defer func() { + a.app.Srv().Store.SetContext(origCtx) + a.ctx = origCtx + }() + + defer span.Finish() + resultVar0 := a.app.RemoveDirectory(path) + + if resultVar0 != nil { + span.LogFields(spanlog.Error(resultVar0)) + ext.Error.Set(span, true) + } + + return resultVar0 +} + func (a *OpenTracingAppLayer) RemoveFile(path string) *model.AppError { origCtx := a.ctx span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.RemoveFile") @@ -14300,6 +14344,50 @@ func (a *OpenTracingAppLayer) TestEmail(userId string, cfg *model.Config) *model return resultVar0 } +func (a *OpenTracingAppLayer) TestFilesStoreConnection() *model.AppError { + origCtx := a.ctx + span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.TestFilesStoreConnection") + + a.ctx = newCtx + a.app.Srv().Store.SetContext(newCtx) + defer func() { + a.app.Srv().Store.SetContext(origCtx) + a.ctx = origCtx + }() + + defer span.Finish() + resultVar0 := a.app.TestFilesStoreConnection() + + if resultVar0 != nil { + span.LogFields(spanlog.Error(resultVar0)) + ext.Error.Set(span, true) + } + + return resultVar0 +} + +func (a *OpenTracingAppLayer) TestFilesStoreConnectionWithConfig(cfg *model.FileSettings) *model.AppError { + origCtx := a.ctx + span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.TestFilesStoreConnectionWithConfig") + + a.ctx = newCtx + a.app.Srv().Store.SetContext(newCtx) + defer func() { + a.app.Srv().Store.SetContext(origCtx) + a.ctx = origCtx + }() + + defer span.Finish() + resultVar0 := a.app.TestFilesStoreConnectionWithConfig(cfg) + + if resultVar0 != nil { + span.LogFields(spanlog.Error(resultVar0)) + ext.Error.Set(span, true) + } + + return resultVar0 +} + func (a *OpenTracingAppLayer) TestLdap() *model.AppError { origCtx := a.ctx span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.TestLdap") diff --git a/app/server.go b/app/server.go index 92ab55d3cc..2052aed425 100644 --- a/app/server.go +++ b/app/server.go @@ -465,11 +465,13 @@ func NewServer(options ...Option) (*Server, error) { } backend, appErr := s.FileBackend() - if appErr == nil { - appErr = backend.TestConnection() - } if appErr != nil { mlog.Error("Problem with file storage settings", mlog.Err(appErr)) + } else { + nErr := backend.TestConnection() + if nErr != nil { + mlog.Error("Problem with file storage settings", mlog.Err(nErr)) + } } s.timezones = timezones.New() @@ -1531,7 +1533,11 @@ func (s *Server) stopSearchEngine() { func (s *Server) FileBackend() (filesstore.FileBackend, *model.AppError) { license := s.License() - return filesstore.NewFileBackend(&s.Config().FileSettings, license != nil && *license.Features.Compliance) + backend, err := filesstore.NewFileBackend(&s.Config().FileSettings, license != nil && *license.Features.Compliance) + if err != nil { + return nil, model.NewAppError("FileBackend", "api.file.no_driver.app_error", nil, err.Error(), http.StatusInternalServerError) + } + return backend, nil } func (s *Server) TotalWebsocketConnections() int { diff --git a/i18n/en.json b/i18n/en.json index 6efb2edbd6..6ebf8943c0 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -1317,39 +1317,23 @@ "translation": "Unable to create the emoji. An error occurred when trying to open the attached image." }, { - "id": "api.file.append_file.no_exist.app_error", - "translation": "File does not exist." - }, - { - "id": "api.file.append_file.opening.app_error", - "translation": "Encountered an error opening the file." - }, - { - "id": "api.file.append_file.s3.app_error", - "translation": "Encountered an error appending to a file in S3." - }, - { - "id": "api.file.append_file.writing.app_error", - "translation": "Encountered an error writing to S3." + "id": "api.file.append_file.app_error", + "translation": "Unable to append data to the file." }, { "id": "api.file.attachments.disabled.app_error", "translation": "File attachments have been disabled on this server." }, { - "id": "api.file.file_exists.exists_local.app_error", + "id": "api.file.file_exists.app_error", "translation": "Unable to check if the file exists." }, { - "id": "api.file.file_exists.s3.app_error", - "translation": "Unable to check if the file exists." + "id": "api.file.file_reader.app_error", + "translation": "Unable to get a file reader." }, { - "id": "api.file.file_size.local.app_error", - "translation": "Unable to get the file size." - }, - { - "id": "api.file.file_size.s3.app_error", + "id": "api.file.file_size.app_error", "translation": "Unable to get the file size." }, { @@ -1373,56 +1357,36 @@ "translation": "Unable to get public link for file. File must be attached to a post that can be read by the current user." }, { - "id": "api.file.move_file.copy_within_s3.app_error", - "translation": "Unable to copy file within S3." + "id": "api.file.list_directory.app_error", + "translation": "Unable to list directory." }, { - "id": "api.file.move_file.delete_from_s3.app_error", - "translation": "Unable to delete file from S3." - }, - { - "id": "api.file.move_file.rename.app_error", - "translation": "Unable to move file locally." - }, - { - "id": "api.file.new_backend.s3.app_error", - "translation": "Encountered an error opening a connection to S3." + "id": "api.file.move_file.app_error", + "translation": "Unable to move file." }, { "id": "api.file.no_driver.app_error", "translation": "No file driver selected." }, + { + "id": "api.file.read_file.app_error", + "translation": "Unable to read the file." + }, { "id": "api.file.read_file.reading_local.app_error", "translation": "Encountered an error reading from local server file storage." }, { - "id": "api.file.read_file.s3.app_error", - "translation": "Encountered an error reading from S3 storage." + "id": "api.file.remove_directory.app_error", + "translation": "Unable to remove the directory." }, { - "id": "api.file.reader.reading_local.app_error", - "translation": "Encountered an error opening a reader from local server file storage." + "id": "api.file.remove_file.app_error", + "translation": "Unable to remove the file." }, { - "id": "api.file.reader.s3.app_error", - "translation": "Encountered an error opening a reader from S3 storage." - }, - { - "id": "api.file.test_connection.local.connection.app_error", - "translation": "Don't have permissions to write to local path specified or other error." - }, - { - "id": "api.file.test_connection.s3.bucket_create.app_error", - "translation": "Unable to create bucket." - }, - { - "id": "api.file.test_connection.s3.bucket_exists.app_error", - "translation": "Error checking if bucket exists." - }, - { - "id": "api.file.test_connection.s3.list_objects.app_error", - "translation": "Error trying to list objects." + "id": "api.file.test_connection.app_error", + "translation": "Unable to access the file storage." }, { "id": "api.file.upload_file.incorrect_channelId.app_error", @@ -1465,16 +1429,8 @@ "translation": "Unable to upload file {{.Filename}}. {{.Length}} bytes exceeds the maximum allowed {{.Limit}} bytes." }, { - "id": "api.file.write_file.s3.app_error", - "translation": "Encountered an error writing to S3." - }, - { - "id": "api.file.write_file_locally.create_dir.app_error", - "translation": "Encountered an error creating the directory for the new file." - }, - { - "id": "api.file.write_file_locally.writing.app_error", - "translation": "Encountered an error writing to local server storage." + "id": "api.file.write_file.app_error", + "translation": "Unable to write the file." }, { "id": "api.image.get.app_error", @@ -6026,6 +5982,10 @@ "id": "ent.actiance.export.marshalToXml.appError", "translation": "Unable to convert export to XML." }, + { + "id": "ent.actiance.export.write_file.appError", + "translation": "Unable to write the export file." + }, { "id": "ent.api.post.send_notifications_and_forget.push_image_only", "translation": " attached a file." @@ -6106,6 +6066,10 @@ "id": "ent.compliance.csv.warning.appError", "translation": "Unable to create the warning file." }, + { + "id": "ent.compliance.csv.write_file.appError", + "translation": "Unable to write the csv file." + }, { "id": "ent.compliance.csv.zip.creation.appError", "translation": "Unable to create the zip export file." @@ -6122,6 +6086,10 @@ "id": "ent.compliance.global_relay.rewind_temporary_file.appError", "translation": "Unable to re-read the Global Relay temporary export file." }, + { + "id": "ent.compliance.global_relay.write_file.appError", + "translation": "Unable to write the global relay file." + }, { "id": "ent.compliance.licence_disable.app_error", "translation": "Compliance functionality disabled by current license. Please contact your system administrator about upgrading your enterprise license." @@ -8246,30 +8214,6 @@ "id": "system.message.name", "translation": "System" }, - { - "id": "utils.file.list_directory.local.app_error", - "translation": "Encountered an error listing directory from local server file storage." - }, - { - "id": "utils.file.list_directory.s3.app_error", - "translation": "Encountered an error listing directory from S3." - }, - { - "id": "utils.file.remove_directory.local.app_error", - "translation": "Encountered an error removing directory from local server file storage." - }, - { - "id": "utils.file.remove_directory.s3.app_error", - "translation": "Encountered an error removing directory from S3." - }, - { - "id": "utils.file.remove_file.local.app_error", - "translation": "Encountered an error removing file from local server file storage." - }, - { - "id": "utils.file.remove_file.s3.app_error", - "translation": "Encountered an error removing file from S3." - }, { "id": "utils.mail.connect_smtp.helo.app_error", "translation": "Failed to set HELO." diff --git a/services/filesstore/filesstore.go b/services/filesstore/filesstore.go index 3233510285..c6d64039eb 100644 --- a/services/filesstore/filesstore.go +++ b/services/filesstore/filesstore.go @@ -5,7 +5,8 @@ package filesstore import ( "io" - "net/http" + + "github.com/pkg/errors" "github.com/mattermost/mattermost-server/v5/model" ) @@ -16,28 +17,28 @@ type ReadCloseSeeker interface { } type FileBackend interface { - TestConnection() *model.AppError + TestConnection() error - Reader(path string) (ReadCloseSeeker, *model.AppError) - ReadFile(path string) ([]byte, *model.AppError) - FileExists(path string) (bool, *model.AppError) - FileSize(path string) (int64, *model.AppError) - CopyFile(oldPath, newPath string) *model.AppError - MoveFile(oldPath, newPath string) *model.AppError - WriteFile(fr io.Reader, path string) (int64, *model.AppError) - AppendFile(fr io.Reader, path string) (int64, *model.AppError) - RemoveFile(path string) *model.AppError + Reader(path string) (ReadCloseSeeker, error) + ReadFile(path string) ([]byte, error) + FileExists(path string) (bool, error) + FileSize(path string) (int64, error) + CopyFile(oldPath, newPath string) error + MoveFile(oldPath, newPath string) error + WriteFile(fr io.Reader, path string) (int64, error) + AppendFile(fr io.Reader, path string) (int64, error) + RemoveFile(path string) error - ListDirectory(path string) (*[]string, *model.AppError) - RemoveDirectory(path string) *model.AppError + ListDirectory(path string) (*[]string, error) + RemoveDirectory(path string) error } -func NewFileBackend(settings *model.FileSettings, enableComplianceFeatures bool) (FileBackend, *model.AppError) { +func NewFileBackend(settings *model.FileSettings, enableComplianceFeatures bool) (FileBackend, error) { switch *settings.DriverName { case model.IMAGE_DRIVER_S3: backend, err := NewS3FileBackend(settings, enableComplianceFeatures) if err != nil { - return nil, model.NewAppError("NewFileBackend", "api.file.new_backend.s3.app_error", nil, err.Error(), http.StatusInternalServerError) + return nil, errors.Wrap(err, "unable to connect to the s3 backend") } return backend, nil case model.IMAGE_DRIVER_LOCAL: @@ -45,5 +46,5 @@ func NewFileBackend(settings *model.FileSettings, enableComplianceFeatures bool) directory: *settings.Directory, }, nil } - return nil, model.NewAppError("NewFileBackend", "api.file.no_driver.app_error", nil, "", http.StatusInternalServerError) + return nil, errors.New("no valid filestorage driver found") } diff --git a/services/filesstore/localstore.go b/services/filesstore/localstore.go index 0ebd768545..2434cf95f6 100644 --- a/services/filesstore/localstore.go +++ b/services/filesstore/localstore.go @@ -7,12 +7,12 @@ import ( "bytes" "io" "io/ioutil" - "net/http" "os" "path/filepath" + "github.com/pkg/errors" + "github.com/mattermost/mattermost-server/v5/mlog" - "github.com/mattermost/mattermost-server/v5/model" "github.com/mattermost/mattermost-server/v5/utils" ) @@ -24,33 +24,33 @@ type LocalFileBackend struct { directory string } -func (b *LocalFileBackend) TestConnection() *model.AppError { +func (b *LocalFileBackend) TestConnection() error { f := bytes.NewReader([]byte("testingwrite")) if _, err := writeFileLocally(f, filepath.Join(b.directory, TEST_FILE_PATH)); err != nil { - return model.NewAppError("TestFileConnection", "api.file.test_connection.local.connection.app_error", nil, err.Error(), http.StatusInternalServerError) + return errors.Wrap(err, "unable to write to the local filesystem storage") } os.Remove(filepath.Join(b.directory, TEST_FILE_PATH)) mlog.Debug("Able to write files to local storage.") return nil } -func (b *LocalFileBackend) Reader(path string) (ReadCloseSeeker, *model.AppError) { +func (b *LocalFileBackend) Reader(path string) (ReadCloseSeeker, error) { f, err := os.Open(filepath.Join(b.directory, path)) if err != nil { - return nil, model.NewAppError("Reader", "api.file.reader.reading_local.app_error", nil, err.Error(), http.StatusInternalServerError) + return nil, errors.Wrapf(err, "unable to open file %s", path) } return f, nil } -func (b *LocalFileBackend) ReadFile(path string) ([]byte, *model.AppError) { +func (b *LocalFileBackend) ReadFile(path string) ([]byte, error) { f, err := ioutil.ReadFile(filepath.Join(b.directory, path)) if err != nil { - return nil, model.NewAppError("ReadFile", "api.file.read_file.reading_local.app_error", nil, err.Error(), http.StatusInternalServerError) + return nil, errors.Wrapf(err, "unable to read file %s", path) } return f, nil } -func (b *LocalFileBackend) FileExists(path string) (bool, *model.AppError) { +func (b *LocalFileBackend) FileExists(path string) (bool, error) { _, err := os.Stat(filepath.Join(b.directory, path)) if os.IsNotExist(err) { @@ -58,91 +58,91 @@ func (b *LocalFileBackend) FileExists(path string) (bool, *model.AppError) { } if err != nil { - return false, model.NewAppError("ReadFile", "api.file.file_exists.exists_local.app_error", nil, err.Error(), http.StatusInternalServerError) + return false, errors.Wrapf(err, "unable to know if file %s exists", path) } return true, nil } -func (b *LocalFileBackend) FileSize(path string) (int64, *model.AppError) { +func (b *LocalFileBackend) FileSize(path string) (int64, error) { info, err := os.Stat(filepath.Join(b.directory, path)) if err != nil { - return 0, model.NewAppError("FileSize", "api.file.file_size.local.app_error", nil, err.Error(), http.StatusInternalServerError) + return 0, errors.Wrapf(err, "unable to get file size for %s", path) } return info.Size(), nil } -func (b *LocalFileBackend) CopyFile(oldPath, newPath string) *model.AppError { +func (b *LocalFileBackend) CopyFile(oldPath, newPath string) error { if err := utils.CopyFile(filepath.Join(b.directory, oldPath), filepath.Join(b.directory, newPath)); err != nil { - return model.NewAppError("copyFile", "api.file.move_file.rename.app_error", nil, err.Error(), http.StatusInternalServerError) + return errors.Wrapf(err, "unable to copy file from %s to %s", oldPath, newPath) } return nil } -func (b *LocalFileBackend) MoveFile(oldPath, newPath string) *model.AppError { +func (b *LocalFileBackend) MoveFile(oldPath, newPath string) error { if err := os.MkdirAll(filepath.Dir(filepath.Join(b.directory, newPath)), 0750); err != nil { - return model.NewAppError("moveFile", "api.file.move_file.rename.app_error", nil, err.Error(), http.StatusInternalServerError) + return errors.Wrapf(err, "unable to create the new destination directory %s", filepath.Dir(newPath)) } if err := os.Rename(filepath.Join(b.directory, oldPath), filepath.Join(b.directory, newPath)); err != nil { - return model.NewAppError("moveFile", "api.file.move_file.rename.app_error", nil, err.Error(), http.StatusInternalServerError) + return errors.Wrapf(err, "unable to move the file to %s to the destination directory", newPath) } return nil } -func (b *LocalFileBackend) WriteFile(fr io.Reader, path string) (int64, *model.AppError) { +func (b *LocalFileBackend) WriteFile(fr io.Reader, path string) (int64, error) { return writeFileLocally(fr, filepath.Join(b.directory, path)) } -func writeFileLocally(fr io.Reader, path string) (int64, *model.AppError) { +func writeFileLocally(fr io.Reader, path string) (int64, error) { if err := os.MkdirAll(filepath.Dir(path), 0750); err != nil { directory, _ := filepath.Abs(filepath.Dir(path)) - return 0, model.NewAppError("WriteFile", "api.file.write_file_locally.create_dir.app_error", nil, "directory="+directory+", err="+err.Error(), http.StatusInternalServerError) + return 0, errors.Wrapf(err, "unable to create the directory %s for the file %s", directory, path) } fw, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0600) if err != nil { - return 0, model.NewAppError("WriteFile", "api.file.write_file_locally.writing.app_error", nil, err.Error(), http.StatusInternalServerError) + return 0, errors.Wrapf(err, "unable to open the file %s to write the data", path) } defer fw.Close() written, err := io.Copy(fw, fr) if err != nil { - return written, model.NewAppError("WriteFile", "api.file.write_file_locally.writing.app_error", nil, err.Error(), http.StatusInternalServerError) + return written, errors.Wrapf(err, "unable write the data in the file %s", path) } return written, nil } -func (b *LocalFileBackend) AppendFile(fr io.Reader, path string) (int64, *model.AppError) { +func (b *LocalFileBackend) AppendFile(fr io.Reader, path string) (int64, error) { fp := filepath.Join(b.directory, path) if _, err := os.Stat(fp); err != nil { - return 0, model.NewAppError("AppendFile", "api.file.append_file.no_exist.app_error", nil, err.Error(), http.StatusInternalServerError) + return 0, errors.Wrapf(err, "unable to find the file %s to append the data", path) } fw, err := os.OpenFile(fp, os.O_WRONLY|os.O_APPEND, 0600) if err != nil { - return 0, model.NewAppError("AppendFile", "api.file.append_file.opening.app_error", nil, err.Error(), http.StatusInternalServerError) + return 0, errors.Wrapf(err, "unable to open the file %s to append the data", path) } defer fw.Close() written, err := io.Copy(fw, fr) if err != nil { - return written, model.NewAppError("AppendFile", "api.file.append_file.writing.app_error", nil, err.Error(), http.StatusInternalServerError) + return written, errors.Wrapf(err, "unable append the data in the file %s", path) } return written, nil } -func (b *LocalFileBackend) RemoveFile(path string) *model.AppError { +func (b *LocalFileBackend) RemoveFile(path string) error { if err := os.Remove(filepath.Join(b.directory, path)); err != nil { - return model.NewAppError("RemoveFile", "utils.file.remove_file.local.app_error", nil, err.Error(), http.StatusInternalServerError) + return errors.Wrapf(err, "unable to remove the file %s", path) } return nil } -func (b *LocalFileBackend) ListDirectory(path string) (*[]string, *model.AppError) { +func (b *LocalFileBackend) ListDirectory(path string) (*[]string, error) { var paths []string fileInfos, err := ioutil.ReadDir(filepath.Join(b.directory, path)) if err != nil { if os.IsNotExist(err) { return &paths, nil } - return nil, model.NewAppError("ListDirectory", "utils.file.list_directory.local.app_error", nil, err.Error(), http.StatusInternalServerError) + return nil, errors.Wrapf(err, "unable to list the directory %s", path) } for _, fileInfo := range fileInfos { paths = append(paths, filepath.Join(path, fileInfo.Name())) @@ -150,9 +150,9 @@ func (b *LocalFileBackend) ListDirectory(path string) (*[]string, *model.AppErro return &paths, nil } -func (b *LocalFileBackend) RemoveDirectory(path string) *model.AppError { +func (b *LocalFileBackend) RemoveDirectory(path string) error { if err := os.RemoveAll(filepath.Join(b.directory, path)); err != nil { - return model.NewAppError("RemoveDirectory", "utils.file.remove_directory.local.app_error", nil, err.Error(), http.StatusInternalServerError) + return errors.Wrapf(err, "unable to remove the directory %s", path) } return nil } diff --git a/services/filesstore/mocks/FileBackend.go b/services/filesstore/mocks/FileBackend.go index c5690c4483..250f8a40a8 100644 --- a/services/filesstore/mocks/FileBackend.go +++ b/services/filesstore/mocks/FileBackend.go @@ -10,8 +10,6 @@ import ( filesstore "github.com/mattermost/mattermost-server/v5/services/filesstore" mock "github.com/stretchr/testify/mock" - - model "github.com/mattermost/mattermost-server/v5/model" ) // FileBackend is an autogenerated mock type for the FileBackend type @@ -20,7 +18,7 @@ type FileBackend struct { } // AppendFile provides a mock function with given fields: fr, path -func (_m *FileBackend) AppendFile(fr io.Reader, path string) (int64, *model.AppError) { +func (_m *FileBackend) AppendFile(fr io.Reader, path string) (int64, error) { ret := _m.Called(fr, path) var r0 int64 @@ -30,36 +28,32 @@ func (_m *FileBackend) AppendFile(fr io.Reader, path string) (int64, *model.AppE r0 = ret.Get(0).(int64) } - var r1 *model.AppError - if rf, ok := ret.Get(1).(func(io.Reader, string) *model.AppError); ok { + var r1 error + if rf, ok := ret.Get(1).(func(io.Reader, string) error); ok { r1 = rf(fr, path) } else { - if ret.Get(1) != nil { - r1 = ret.Get(1).(*model.AppError) - } + r1 = ret.Error(1) } return r0, r1 } // CopyFile provides a mock function with given fields: oldPath, newPath -func (_m *FileBackend) CopyFile(oldPath string, newPath string) *model.AppError { +func (_m *FileBackend) CopyFile(oldPath string, newPath string) error { ret := _m.Called(oldPath, newPath) - var r0 *model.AppError - if rf, ok := ret.Get(0).(func(string, string) *model.AppError); ok { + var r0 error + if rf, ok := ret.Get(0).(func(string, string) error); ok { r0 = rf(oldPath, newPath) } else { - if ret.Get(0) != nil { - r0 = ret.Get(0).(*model.AppError) - } + r0 = ret.Error(0) } return r0 } // FileExists provides a mock function with given fields: path -func (_m *FileBackend) FileExists(path string) (bool, *model.AppError) { +func (_m *FileBackend) FileExists(path string) (bool, error) { ret := _m.Called(path) var r0 bool @@ -69,20 +63,18 @@ func (_m *FileBackend) FileExists(path string) (bool, *model.AppError) { r0 = ret.Get(0).(bool) } - var r1 *model.AppError - if rf, ok := ret.Get(1).(func(string) *model.AppError); ok { + var r1 error + if rf, ok := ret.Get(1).(func(string) error); ok { r1 = rf(path) } else { - if ret.Get(1) != nil { - r1 = ret.Get(1).(*model.AppError) - } + r1 = ret.Error(1) } return r0, r1 } // ListDirectory provides a mock function with given fields: path -func (_m *FileBackend) ListDirectory(path string) (*[]string, *model.AppError) { +func (_m *FileBackend) ListDirectory(path string) (*[]string, error) { ret := _m.Called(path) var r0 *[]string @@ -94,36 +86,32 @@ func (_m *FileBackend) ListDirectory(path string) (*[]string, *model.AppError) { } } - var r1 *model.AppError - if rf, ok := ret.Get(1).(func(string) *model.AppError); ok { + var r1 error + if rf, ok := ret.Get(1).(func(string) error); ok { r1 = rf(path) } else { - if ret.Get(1) != nil { - r1 = ret.Get(1).(*model.AppError) - } + r1 = ret.Error(1) } return r0, r1 } // MoveFile provides a mock function with given fields: oldPath, newPath -func (_m *FileBackend) MoveFile(oldPath string, newPath string) *model.AppError { +func (_m *FileBackend) MoveFile(oldPath string, newPath string) error { ret := _m.Called(oldPath, newPath) - var r0 *model.AppError - if rf, ok := ret.Get(0).(func(string, string) *model.AppError); ok { + var r0 error + if rf, ok := ret.Get(0).(func(string, string) error); ok { r0 = rf(oldPath, newPath) } else { - if ret.Get(0) != nil { - r0 = ret.Get(0).(*model.AppError) - } + r0 = ret.Error(0) } return r0 } // ReadFile provides a mock function with given fields: path -func (_m *FileBackend) ReadFile(path string) ([]byte, *model.AppError) { +func (_m *FileBackend) ReadFile(path string) ([]byte, error) { ret := _m.Called(path) var r0 []byte @@ -135,20 +123,18 @@ func (_m *FileBackend) ReadFile(path string) ([]byte, *model.AppError) { } } - var r1 *model.AppError - if rf, ok := ret.Get(1).(func(string) *model.AppError); ok { + var r1 error + if rf, ok := ret.Get(1).(func(string) error); ok { r1 = rf(path) } else { - if ret.Get(1) != nil { - r1 = ret.Get(1).(*model.AppError) - } + r1 = ret.Error(1) } return r0, r1 } // Reader provides a mock function with given fields: path -func (_m *FileBackend) Reader(path string) (filesstore.ReadCloseSeeker, *model.AppError) { +func (_m *FileBackend) Reader(path string) (filesstore.ReadCloseSeeker, error) { ret := _m.Called(path) var r0 filesstore.ReadCloseSeeker @@ -160,68 +146,60 @@ func (_m *FileBackend) Reader(path string) (filesstore.ReadCloseSeeker, *model.A } } - var r1 *model.AppError - if rf, ok := ret.Get(1).(func(string) *model.AppError); ok { + var r1 error + if rf, ok := ret.Get(1).(func(string) error); ok { r1 = rf(path) } else { - if ret.Get(1) != nil { - r1 = ret.Get(1).(*model.AppError) - } + r1 = ret.Error(1) } return r0, r1 } // RemoveDirectory provides a mock function with given fields: path -func (_m *FileBackend) RemoveDirectory(path string) *model.AppError { +func (_m *FileBackend) RemoveDirectory(path string) error { ret := _m.Called(path) - var r0 *model.AppError - if rf, ok := ret.Get(0).(func(string) *model.AppError); ok { + var r0 error + if rf, ok := ret.Get(0).(func(string) error); ok { r0 = rf(path) } else { - if ret.Get(0) != nil { - r0 = ret.Get(0).(*model.AppError) - } + r0 = ret.Error(0) } return r0 } // RemoveFile provides a mock function with given fields: path -func (_m *FileBackend) RemoveFile(path string) *model.AppError { +func (_m *FileBackend) RemoveFile(path string) error { ret := _m.Called(path) - var r0 *model.AppError - if rf, ok := ret.Get(0).(func(string) *model.AppError); ok { + var r0 error + if rf, ok := ret.Get(0).(func(string) error); ok { r0 = rf(path) } else { - if ret.Get(0) != nil { - r0 = ret.Get(0).(*model.AppError) - } + r0 = ret.Error(0) } return r0 } // TestConnection provides a mock function with given fields: -func (_m *FileBackend) TestConnection() *model.AppError { +func (_m *FileBackend) TestConnection() error { ret := _m.Called() - var r0 *model.AppError - if rf, ok := ret.Get(0).(func() *model.AppError); ok { + var r0 error + if rf, ok := ret.Get(0).(func() error); ok { r0 = rf() } else { - if ret.Get(0) != nil { - r0 = ret.Get(0).(*model.AppError) - } + r0 = ret.Error(0) } return r0 } // WriteFile provides a mock function with given fields: fr, path -func (_m *FileBackend) WriteFile(fr io.Reader, path string) (int64, *model.AppError) { +func (_m *FileBackend) WriteFile(fr io.Reader, path string) (int64, error) { ret := _m.Called(fr, path) var r0 int64 @@ -231,13 +209,11 @@ func (_m *FileBackend) WriteFile(fr io.Reader, path string) (int64, *model.AppEr r0 = ret.Get(0).(int64) } - var r1 *model.AppError - if rf, ok := ret.Get(1).(func(io.Reader, string) *model.AppError); ok { + var r1 error + if rf, ok := ret.Get(1).(func(io.Reader, string) error); ok { r1 = rf(fr, path) } else { - if ret.Get(1) != nil { - r1 = ret.Get(1).(*model.AppError) - } + r1 = ret.Error(1) } return r0, r1 diff --git a/services/filesstore/s3store.go b/services/filesstore/s3store.go index 687e228ada..9b616efb83 100644 --- a/services/filesstore/s3store.go +++ b/services/filesstore/s3store.go @@ -5,10 +5,8 @@ package filesstore import ( "context" - "errors" "io" "io/ioutil" - "net/http" "os" "path/filepath" "strings" @@ -16,6 +14,7 @@ import ( s3 "github.com/minio/minio-go/v7" "github.com/minio/minio-go/v7/pkg/credentials" "github.com/minio/minio-go/v7/pkg/encrypt" + "github.com/pkg/errors" "github.com/mattermost/mattermost-server/v5/mlog" "github.com/mattermost/mattermost-server/v5/model" @@ -118,7 +117,7 @@ func (b *S3FileBackend) s3New() (*s3.Client, error) { return s3Clnt, nil } -func (b *S3FileBackend) TestConnection() *model.AppError { +func (b *S3FileBackend) TestConnection() error { exists := true var err error // If a path prefix is present, we attempt to test the bucket by listing objects under the path @@ -129,14 +128,14 @@ func (b *S3FileBackend) TestConnection() *model.AppError { if obj.Err != nil { typedErr := s3.ToErrorResponse(obj.Err) if typedErr.Code != bucketNotFound { - return model.NewAppError("TestFileConnection", "api.file.test_connection.s3.list_objects.app_error", nil, obj.Err.Error(), http.StatusInternalServerError) + return errors.Wrap(err, "unable to list objects in the s3 bucket") } exists = false } } else { exists, err = b.client.BucketExists(context.Background(), b.bucket) if err != nil { - return model.NewAppError("TestFileConnection", "api.file.test_connection.s3.bucket_exists.app_error", nil, err.Error(), http.StatusInternalServerError) + return errors.Wrap(err, "unable to check if the s3 bucket exists") } } @@ -146,7 +145,7 @@ func (b *S3FileBackend) TestConnection() *model.AppError { mlog.Warn("Bucket specified does not exist. Attempting to create...") err := b.client.MakeBucket(context.Background(), b.bucket, s3.MakeBucketOptions{Region: b.region}) if err != nil { - return model.NewAppError("TestFileConnection", "api.file.test_connection.s3.bucket_create.app_error", nil, err.Error(), http.StatusInternalServerError) + return errors.Wrap(err, "unable to create the s3 bucket") } } @@ -154,32 +153,32 @@ func (b *S3FileBackend) TestConnection() *model.AppError { } // Caller must close the first return value -func (b *S3FileBackend) Reader(path string) (ReadCloseSeeker, *model.AppError) { +func (b *S3FileBackend) Reader(path string) (ReadCloseSeeker, error) { path = filepath.Join(b.pathPrefix, path) minioObject, err := b.client.GetObject(context.Background(), b.bucket, path, s3.GetObjectOptions{}) if err != nil { - return nil, model.NewAppError("Reader", "api.file.reader.s3.app_error", nil, err.Error(), http.StatusInternalServerError) + return nil, errors.Wrapf(err, "unable to open file %s", path) } return minioObject, nil } -func (b *S3FileBackend) ReadFile(path string) ([]byte, *model.AppError) { +func (b *S3FileBackend) ReadFile(path string) ([]byte, error) { path = filepath.Join(b.pathPrefix, path) minioObject, err := b.client.GetObject(context.Background(), b.bucket, path, s3.GetObjectOptions{}) if err != nil { - return nil, model.NewAppError("ReadFile", "api.file.read_file.s3.app_error", nil, err.Error(), http.StatusInternalServerError) + return nil, errors.Wrapf(err, "unable to open file %s", path) } defer minioObject.Close() if f, err := ioutil.ReadAll(minioObject); err != nil { - return nil, model.NewAppError("ReadFile", "api.file.read_file.s3.app_error", nil, err.Error(), http.StatusInternalServerError) + return nil, errors.Wrapf(err, "unable to read file %s", path) } else { return f, nil } } -func (b *S3FileBackend) FileExists(path string) (bool, *model.AppError) { +func (b *S3FileBackend) FileExists(path string) (bool, error) { path = filepath.Join(b.pathPrefix, path) _, err := b.client.StatObject(context.Background(), b.bucket, path, s3.StatObjectOptions{}) @@ -192,21 +191,21 @@ func (b *S3FileBackend) FileExists(path string) (bool, *model.AppError) { return false, nil } - return false, model.NewAppError("FileExists", "api.file.file_exists.s3.app_error", nil, err.Error(), http.StatusInternalServerError) + return false, errors.Wrapf(err, "unable to know if file %s exists", path) } -func (b *S3FileBackend) FileSize(path string) (int64, *model.AppError) { +func (b *S3FileBackend) FileSize(path string) (int64, error) { path = filepath.Join(b.pathPrefix, path) info, err := b.client.StatObject(context.Background(), b.bucket, path, s3.StatObjectOptions{}) if err != nil { - return 0, model.NewAppError("FileSize", "api.file.file_size.s3.app_error", nil, err.Error(), http.StatusInternalServerError) + return 0, errors.Wrapf(err, "unable to get file size for %s", path) } return info.Size, nil } -func (b *S3FileBackend) CopyFile(oldPath, newPath string) *model.AppError { +func (b *S3FileBackend) CopyFile(oldPath, newPath string) error { oldPath = filepath.Join(b.pathPrefix, oldPath) newPath = filepath.Join(b.pathPrefix, newPath) srcOpts := s3.CopySrcOptions{ @@ -220,12 +219,12 @@ func (b *S3FileBackend) CopyFile(oldPath, newPath string) *model.AppError { Encryption: encrypt.NewSSE(), } if _, err := b.client.CopyObject(context.Background(), dstOpts, srcOpts); err != nil { - return model.NewAppError("copyFile", "api.file.move_file.copy_within_s3.app_error", nil, err.Error(), http.StatusInternalServerError) + return errors.Wrapf(err, "unable to copy file from %s to %s", oldPath, newPath) } return nil } -func (b *S3FileBackend) MoveFile(oldPath, newPath string) *model.AppError { +func (b *S3FileBackend) MoveFile(oldPath, newPath string) error { oldPath = filepath.Join(b.pathPrefix, oldPath) newPath = filepath.Join(b.pathPrefix, newPath) srcOpts := s3.CopySrcOptions{ @@ -240,17 +239,17 @@ func (b *S3FileBackend) MoveFile(oldPath, newPath string) *model.AppError { } if _, err := b.client.CopyObject(context.Background(), dstOpts, srcOpts); err != nil { - return model.NewAppError("moveFile", "api.file.move_file.copy_within_s3.app_error", nil, err.Error(), http.StatusInternalServerError) + return errors.Wrapf(err, "unable to copy the file to %s to the new destionation", newPath) } if err := b.client.RemoveObject(context.Background(), b.bucket, oldPath, s3.RemoveObjectOptions{}); err != nil { - return model.NewAppError("moveFile", "api.file.move_file.delete_from_s3.app_error", nil, err.Error(), http.StatusInternalServerError) + return errors.Wrapf(err, "unable to remove the file old file %s", oldPath) } return nil } -func (b *S3FileBackend) WriteFile(fr io.Reader, path string) (int64, *model.AppError) { +func (b *S3FileBackend) WriteFile(fr io.Reader, path string) (int64, error) { var contentType string path = filepath.Join(b.pathPrefix, path) if ext := filepath.Ext(path); model.IsFileExtImage(ext) { @@ -262,16 +261,16 @@ func (b *S3FileBackend) WriteFile(fr io.Reader, path string) (int64, *model.AppE options := s3PutOptions(b.encrypt, contentType) info, err := b.client.PutObject(context.Background(), b.bucket, path, fr, -1, options) if err != nil { - return info.Size, model.NewAppError("WriteFile", "api.file.write_file.s3.app_error", nil, err.Error(), http.StatusInternalServerError) + return info.Size, errors.Wrapf(err, "unable write the data in the file %s", path) } return info.Size, nil } -func (b *S3FileBackend) AppendFile(fr io.Reader, path string) (int64, *model.AppError) { +func (b *S3FileBackend) AppendFile(fr io.Reader, path string) (int64, error) { fp := filepath.Join(b.pathPrefix, path) if _, err := b.client.StatObject(context.Background(), b.bucket, fp, s3.StatObjectOptions{}); err != nil { - return 0, model.NewAppError("AppendFile", "api.file.append_file.s3.app_error", nil, err.Error(), http.StatusInternalServerError) + return 0, errors.Wrapf(err, "unable to find the file %s to append the data", path) } var contentType string @@ -302,23 +301,18 @@ func (b *S3FileBackend) AppendFile(fr io.Reader, path string) (int64, *model.App } _, err = b.client.ComposeObject(context.Background(), dstOpts, src1Opts, src2Opts) if err != nil { - return 0, model.NewAppError("AppendFile", "api.file.append_file.s3.app_error", nil, err.Error(), http.StatusInternalServerError) + return 0, errors.Wrapf(err, "unable append the data in the file %s", path) } return info.Size, nil } - var errString string - if err != nil { - errString = err.Error() - } - - return 0, model.NewAppError("AppendFile", "api.file.append_file.s3.app_error", nil, errString, http.StatusInternalServerError) + return 0, errors.Wrapf(err, "unable append the data in the file %s", path) } -func (b *S3FileBackend) RemoveFile(path string) *model.AppError { +func (b *S3FileBackend) RemoveFile(path string) error { path = filepath.Join(b.pathPrefix, path) if err := b.client.RemoveObject(context.Background(), b.bucket, path, s3.RemoveObjectOptions{}); err != nil { - return model.NewAppError("RemoveFile", "utils.file.remove_file.s3.app_error", nil, err.Error(), http.StatusInternalServerError) + return errors.Wrapf(err, "unable to remove the file %s", path) } return nil @@ -344,9 +338,7 @@ func getPathsFromObjectInfos(in <-chan s3.ObjectInfo) <-chan s3.ObjectInfo { return out } -func (b *S3FileBackend) ListDirectory(path string) (*[]string, *model.AppError) { - var paths []string - +func (b *S3FileBackend) ListDirectory(path string) (*[]string, error) { path = filepath.Join(b.pathPrefix, path) if !strings.HasSuffix(path, "/") && len(path) > 0 { // s3Clnt returns only the path itself when "/" is not present @@ -357,9 +349,10 @@ func (b *S3FileBackend) ListDirectory(path string) (*[]string, *model.AppError) opts := s3.ListObjectsOptions{ Prefix: path, } + var paths []string for object := range b.client.ListObjects(context.Background(), b.bucket, opts) { if object.Err != nil { - return nil, model.NewAppError("ListDirectory", "utils.file.list_directory.s3.app_error", nil, object.Err.Error(), http.StatusInternalServerError) + return nil, errors.Wrapf(object.Err, "unable to list the directory %s", path) } // We strip the path prefix that gets applied, // so that it remains transparent to the application. @@ -373,7 +366,7 @@ func (b *S3FileBackend) ListDirectory(path string) (*[]string, *model.AppError) return &paths, nil } -func (b *S3FileBackend) RemoveDirectory(path string) *model.AppError { +func (b *S3FileBackend) RemoveDirectory(path string) error { opts := s3.ListObjectsOptions{ Prefix: filepath.Join(b.pathPrefix, path), Recursive: true, @@ -382,7 +375,7 @@ func (b *S3FileBackend) RemoveDirectory(path string) *model.AppError { objectsCh := b.client.RemoveObjects(context.Background(), b.bucket, getPathsFromObjectInfos(list), s3.RemoveObjectsOptions{}) for err := range objectsCh { if err.Err != nil { - return model.NewAppError("RemoveDirectory", "utils.file.remove_directory.s3.app_error", nil, err.Err.Error(), http.StatusInternalServerError) + return errors.Wrapf(err.Err, "unable to remove the directory %s", path) } } @@ -402,9 +395,9 @@ func s3PutOptions(encrypted bool, contentType string) s3.PutObjectOptions { return options } -func CheckMandatoryS3Fields(settings *model.FileSettings) *model.AppError { +func CheckMandatoryS3Fields(settings *model.FileSettings) error { if settings.AmazonS3Bucket == nil || len(*settings.AmazonS3Bucket) == 0 { - return model.NewAppError("S3File", "api.admin.test_s3.missing_s3_bucket", nil, "", http.StatusBadRequest) + return errors.New("missing s3 bucket settings") } // if S3 endpoint is not set call the set defaults to set that diff --git a/services/filesstore/s3store_test.go b/services/filesstore/s3store_test.go index a72a4cc6b1..898e4ad1bb 100644 --- a/services/filesstore/s3store_test.go +++ b/services/filesstore/s3store_test.go @@ -15,7 +15,7 @@ func TestCheckMandatoryS3Fields(t *testing.T) { err := CheckMandatoryS3Fields(&cfg) require.NotNil(t, err) - require.Equal(t, err.Message, "api.admin.test_s3.missing_s3_bucket", "should've failed with missing s3 bucket") + require.Equal(t, err.Error(), "missing s3 bucket settings", "should've failed with missing s3 bucket") cfg.AmazonS3Bucket = model.NewString("test-mm") err = CheckMandatoryS3Fields(&cfg) diff --git a/services/mailservice/mail.go b/services/mailservice/mail.go index 33fde79e7c..e9d3abe67b 100644 --- a/services/mailservice/mail.go +++ b/services/mailservice/mail.go @@ -298,9 +298,9 @@ func sendMailUsingConfigAdvanced(mail mailData, config *model.Config, enableComp defer c.Quit() defer c.Close() - fileBackend, err := filesstore.NewFileBackend(&config.FileSettings, enableComplianceFeatures) - if err != nil { - return err + fileBackend, nErr := filesstore.NewFileBackend(&config.FileSettings, enableComplianceFeatures) + if nErr != nil { + return model.NewAppError("sendMailUsingConfigAdvanced", "api.file.no_driver.app_error", nil, nErr.Error(), http.StatusInternalServerError) } return SendMail(c, mail, fileBackend, time.Now()) @@ -349,14 +349,14 @@ func SendMail(c smtpClient, mail mailData, fileBackend filesstore.FileBackend, d } for _, fileInfo := range mail.attachments { - bytes, err := fileBackend.ReadFile(fileInfo.Path) - if err != nil { - return err + bytes, nErr := fileBackend.ReadFile(fileInfo.Path) + if nErr != nil { + return model.NewAppError("SendMail", "api.file.read_file.app_error", nil, nErr.Error(), http.StatusInternalServerError) } m.Attach(fileInfo.Name, gomail.SetCopyFunc(func(writer io.Writer) error { - if _, err := writer.Write(bytes); err != nil { - return model.NewAppError("SendMail", "utils.mail.sendMail.attachments.write_error", nil, err.Error(), http.StatusInternalServerError) + if _, nErr = writer.Write(bytes); nErr != nil { + return model.NewAppError("SendMail", "utils.mail.sendMail.attachments.write_error", nil, nErr.Error(), http.StatusInternalServerError) } return nil }))