From 09c39cf3ec330a353eaff7e4161501734f28ff24 Mon Sep 17 00:00:00 2001 From: Ben Schumacher Date: Tue, 7 May 2024 15:30:48 +0200 Subject: [PATCH] [MM-58020] Improve error message of NotFound errors in store (#26870) * Improve error message of NotFound errors in store * update mmctl tests --- server/channels/store/errors.go | 4 +-- server/channels/store/errors_test.go | 28 +++++++++++++++++++ server/cmd/mmctl/commands/channel_e2e_test.go | 4 +-- server/cmd/mmctl/commands/user_e2e_test.go | 10 +++---- 4 files changed, 37 insertions(+), 9 deletions(-) create mode 100644 server/channels/store/errors_test.go diff --git a/server/channels/store/errors.go b/server/channels/store/errors.go index fcd733aff7..8b2cd8fab9 100644 --- a/server/channels/store/errors.go +++ b/server/channels/store/errors.go @@ -120,10 +120,10 @@ func (e *ErrNotFound) Wrap(err error) *ErrNotFound { func (e *ErrNotFound) Error() string { if e.wrapped != nil { - return fmt.Sprintf("resource: %s id: %s error: %s", e.resource, e.ID, e.wrapped) + return fmt.Sprintf("resource %q not found, id: %s, error: %s", e.resource, e.ID, e.wrapped) } - return fmt.Sprintf("resource: %s id: %s", e.resource, e.ID) + return fmt.Sprintf("resource %q not found, id: %s", e.resource, e.ID) } // IsErrNotFound allows easy type assertion without adding store as a dependency. diff --git a/server/channels/store/errors_test.go b/server/channels/store/errors_test.go new file mode 100644 index 0000000000..4279afd2ff --- /dev/null +++ b/server/channels/store/errors_test.go @@ -0,0 +1,28 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package store + +import ( + "errors" + "testing" + + "github.com/mattermost/mattermost/server/public/model" + "github.com/stretchr/testify/assert" +) + +func TestErrNotFound(t *testing.T) { + id := model.NewId() + + t.Run("plain", func(t *testing.T) { + err := NewErrNotFound("channel", id) + + assert.EqualError(t, err, "resource \"channel\" not found, id: "+id) + }) + t.Run("with wrapped error", func(t *testing.T) { + err := NewErrNotFound("channel", id) + err = err.Wrap(errors.New("some error")) + + assert.EqualError(t, err, "resource \"channel\" not found, id: "+id+", error: some error") + }) +} diff --git a/server/cmd/mmctl/commands/channel_e2e_test.go b/server/cmd/mmctl/commands/channel_e2e_test.go index f047ca7d2c..e88edf575d 100644 --- a/server/cmd/mmctl/commands/channel_e2e_test.go +++ b/server/cmd/mmctl/commands/channel_e2e_test.go @@ -357,7 +357,7 @@ func (s *MmctlE2ETestSuite) TestDeleteChannelsCmd() { _, err = s.th.App.GetChannel(s.th.Context, channel.Id) s.Require().NotNil(err) - s.Require().Equal(fmt.Sprintf("GetChannel: Unable to find the existing channel., resource: Channel id: %s", channel.Id), err.Error()) + s.Require().Equal(fmt.Sprintf("GetChannel: Unable to find the existing channel., resource \"Channel\" not found, id: %s", channel.Id), err.Error()) }) s.Run("Delete channel without permissions", func() { @@ -401,7 +401,7 @@ func (s *MmctlE2ETestSuite) TestDeleteChannelsCmd() { s.Require().Nil(channel) s.Require().NotNil(err) - s.Require().Equal(fmt.Sprintf("GetChannel: Unable to find the existing channel., resource: Channel id: %s", notExistingChannelID), err.Error()) + s.Require().Equal(fmt.Sprintf("GetChannel: Unable to find the existing channel., resource \"Channel\" not found, id: %s", notExistingChannelID), err.Error()) }) } diff --git a/server/cmd/mmctl/commands/user_e2e_test.go b/server/cmd/mmctl/commands/user_e2e_test.go index db806be901..68c62cb312 100644 --- a/server/cmd/mmctl/commands/user_e2e_test.go +++ b/server/cmd/mmctl/commands/user_e2e_test.go @@ -495,7 +495,7 @@ func (s *MmctlE2ETestSuite) TestCreateUserCmd() { s.Require().Empty(printer.GetLines()) _, err = s.th.App.GetUserByEmail(email) s.Require().NotNil(err) - s.Require().ErrorContains(err, "GetUserByEmail: Unable to find the user., failed to find User: resource: User id: email="+email) + s.Require().ErrorContains(err, "GetUserByEmail: Unable to find the user., failed to find User: resource \"User\" not found, id: email="+email) }) s.RunForAllClients("Should not create a user w/o email", func(c client.Client) { @@ -510,7 +510,7 @@ func (s *MmctlE2ETestSuite) TestCreateUserCmd() { s.Require().Empty(printer.GetLines()) _, err = s.th.App.GetUserByUsername(username) s.Require().NotNil(err) - s.Require().ErrorContains(err, "GetUserByUsername: Unable to find an existing account matching your username for this team. This team may require an invite from the team owner to join., failed to find User: resource: User id: username="+username) + s.Require().ErrorContains(err, "GetUserByUsername: Unable to find an existing account matching your username for this team. This team may require an invite from the team owner to join., failed to find User: resource \"User\" not found, id: username="+username) }) s.RunForAllClients("Should not create a user w/o password", func(c client.Client) { @@ -525,7 +525,7 @@ func (s *MmctlE2ETestSuite) TestCreateUserCmd() { s.Require().Empty(printer.GetLines()) _, err = s.th.App.GetUserByEmail(email) s.Require().NotNil(err) - s.Require().ErrorContains(err, "GetUserByEmail: Unable to find the user., failed to find User: resource: User id: email="+email) + s.Require().ErrorContains(err, "GetUserByEmail: Unable to find the user., failed to find User: resource \"User\" not found, id: email="+email) }) s.Run("Should create a user but w/o system-admin privileges", func() { @@ -720,7 +720,7 @@ func (s *MmctlE2ETestSuite) TestDeleteUsersCmd() { // expect user deleted _, err = s.th.App.GetUser(newUser.Id) s.Require().NotNil(err) - s.Require().Equal("GetUser: Unable to find the user., resource: User id: "+newUser.Id, err.Error()) + s.Require().Equal("GetUser: Unable to find the user., resource \"User\" not found, id: "+newUser.Id, err.Error()) }) s.RunForSystemAdminAndLocal("Delete nonexistent user", func(c client.Client) { @@ -821,7 +821,7 @@ func (s *MmctlE2ETestSuite) TestDeleteUsersCmd() { // expect user deleted _, err = s.th.App.GetUser(newUser.Id) s.Require().NotNil(err) - s.Require().EqualError(err, "GetUser: Unable to find the user., resource: User id: "+newUser.Id) + s.Require().EqualError(err, "GetUser: Unable to find the user., resource \"User\" not found, id: "+newUser.Id) }) }